[PATCH rtems-libbsd] rtemsbsd: Use a separate header for test devices

[Chris Johns]

On 8/7/21 11:11 am, Kinsey Moore wrote:
> On 7/7/2021 19:28, Chris Johns wrote:
>> On 7/7/21 11:26 pm, Kinsey Moore wrote:
>>> On 7/6/2021 21:20, Chris Johns wrote:
>>>> On 7/7/21 12:03 pm, Kinsey Moore wrote:
>>>>> On 7/6/2021 20:57, Chris Johns wrote:
>>>>>> On 7/7/21 11:05 am, Kinsey Moore wrote:
>>>>>>> The need for the difference on ZynqMP is that there are 4 different CGEM
>>>>>>> interfaces of which dev boards primarily make use of CGEM3.
>>>>>> RTEMS_BSD_DRIVER_XILINX_ZYNQMP_CGEM0(ZYNQMP_IRQ_ETHERNET_0);
>>>>>> RTEMS_BSD_DRIVER_XILINX_ZYNQMP_CGEM1(ZYNQMP_IRQ_ETHERNET_1);
>>>>>> RTEMS_BSD_DRIVER_XILINX_ZYNQMP_CGEM2(ZYNQMP_IRQ_ETHERNET_2);
>>>>>> RTEMS_BSD_DRIVER_XILINX_ZYNQMP_CGEM3(ZYNQMP_IRQ_ETHERNET_3);
>>>>>>
>>>>>> ?
>>>>> Yes, this does technically work
>>>> Hmm, I suggest this is what we should support as a default.
>>>>
>>>>> if you can read the shell output past the log
>>>>> spam. The other interfaces trying and failing to come up throw a gargantuan
>>>>> amount of messages to the console.
>>>> Why do these interfaces fail to initialise? What are the errors?
>>>>
>>>> The removed probe check is based around FDT and so I suspect is the reason
>>>> Sebastian suggested FDT support. Needing FDT support would break existing Zynq
>>>> users.
>>> The devices themselves are detected just fine and are likely operational. The
>>> MII busses are probably unterminated or pulled high/low which yields a ukphy
>>> detection on every available slot and constant PHY read/write timeouts. A small
>>> sample from enabling both CGEM2 and CGEM3 on this custom board:
>>>
>>> ukphy13:  none, 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 100baseT4,
>>> 1000baseSX, 1000baseSX-FDX, 1000baseT, 1000baseT-master, 1000baseT-FDX,
>>> 1000baseT-FDX-master, auto
>>> cgem3: phy read timeout: 0
>>> ukphy14: <Generic IEEE 802.3u media interface> PHY 14 on miibus0
>>> cgem3: phy write timeout: 0
>>> cgem3: phy read timeout: 0
>>> cgem3: phy read timeout: 0
>>> cgem3: phy read timeout: 0
>>> cgem3: phy read timeout: 0
>>> cgem3: phy read timeout: 0
>>> cgem3: phy read timeout: 0
>>>
>>> ...
>>>
>>> cgem3: phy read timeout: 0
>> That is noisy and painful.
>>
>>> This type of output reached ukphy31 before I was able to run the shutdown
>>> command with 50+ PHY read/write errors and more continuing afterward. I imagine
>>> it's worse if I enable all 4 interfaces.
>> The output is from the cgem driver which is good because it is localised. I
>> agree we need something to control what is enabled and I prefer we avoid special
>> build options, I will explain why I think this later. We can:
>>
>> 1. Add FDT support. This is something I would like to avoid as it adds an extra
>> layer of dependency and it complicates backwards compatibility for existing
>> users. I however wonder if this is just me and it is something that will be
>> needed more and more and cannot be avoided.
> 
> It's not just you, I (and Joel) lean this direction as well. Having the option
> of FDT would be nice and would certainly cover this case, but making it a hard
> requirement seems excessive.

If we could contain a suitable default FDT in the BSP that achieves the outcome
then I would be OK.

>> 2. Add a weak call that is RTEMS and BSP specific to return a probe state. The
>> default state could be enabled and the tests provide something from the network
>> config. Not a great solution but it is simple and cheap to implement. The driver
>> already has a weak call to set the reference clock.
> This patch (since omitted above) strikes a similar compromise but has the
> downside of the default application configuration not having network at all. The
> downside of this suggestion (option #2) is the addition of a second method to
> accomplish the same goal: define a new hook vs redefine or add to the existing
> definitions in nexus-devices.h. My initial patch addressed both, but added what
> appeared to be dead options to RTEMS and has since been reverted. The other
> current/alternate patch addresses both but leaks test configuration state into
> the application by installing the test network configuration.

I do not see them as being the same thing, yes they achieve the same result
however via different interfaces. Your patch creates a top level "user
interface", ie at the publicly exposed nexus bus interface while a weak driver
interface is a private agreement between the BSP and this driver plus users can
provide their own implementation to further change the mix without needing to
configure or touch any released code. I am concerned the pattern of defining
mixes in the nexus header is adopted by others on a look and copy basis and we
get more and more options that never get compiled and rot.

A 2.1 alternative could be adding a global define such as ....

 RTEMS_BSD_CONFIG_NEXUS_BUS_DISABLE_DEFAULT_DRIVERS

that disables all the provided definitions and the user can then provide their
own? Maybe the network config could be extended to allow such defines?

A global disable is needed so the BSD config header I posted before could be
used without nexus defines clashing.

>> 3. Try and detect a PHY in the probe? I am not sure if that is easy or hard. If
>> read works maybe that is something that may be suitable.
> That's what is currently happening. 

Are you sure? I see the probe as passing and the device is consider available
however there is no PHY and that creates the errors and that seems reasonable.
The UKPHY driver is designed to inspect all PHY addresses however needing 200
reties does seem excessive.

I was thinking about a simpler scan of the MII bus for a PHY in the probe and if
none found disable return -1 from the probe.

> Unfortunately, the ukphy driver is there to
> detect braindead PHYs (I have a board which needs it, but could probably use a
> more specific PHY driver in its place) and will pick up certain bits being set
> as being a valid PHY. Perhaps the ukphy driver just needs more specificity or
> maybe the ukphy driver should never be used in nexus-devices.h when there are
> multiple interfaces provided for a BSP.

I think the UKPHY driver is OK, it is the cgem driver that has a crazy high
retry count and generates the error message. A failing probe with no error
messages generates no output.

>> I would prefer we avoid special build options for BSPs. It is easy for us as
>> developers to make these changes and build a specific RTEMS plus we can be a
>> little narrow in our focus when delivering something for a client in the
>> defaults we select. Specific builds of a BSP to configure options becomes hard
>> for users where they have a few Zynq or ZynqMP designs all running RTEMS and
>> each with a different mix of network interfaces. This creates a complex set of
>> build options for common application code plus more configuration items to
>> control.
> 
> I wonder why this never came up with Zynq or QorIQ. Maybe no one wanted to run
> network tests on the alternate interfaces because dev boards with those
> interfaces configured didn't exist? 

I do not know, it could be.

> It's possible that the ukphy driver could be
> improved and this entire problem just goes away or we ban that driver from the
> default configuration for multi-interface BSPs and the problem goes away.

The UKPHY is an important driver and we need it. The MVME2700 runs a rev 1 Tulip
chip and the PHY on the board is so old there is no data on it and the company
has long gone. The PHY does support the IEEE MII basic registers and the UKPHY
works. I think our issue here is in the cgem driver and not else where.

Chris