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

[Kinsey Moore]

On 7/8/2021 10:36, Kinsey Moore wrote:
> On 7/8/2021 02:48, Chris Johns wrote:
>> On 8/7/21 11:11 am, Kinsey Moore wrote:
>>> On 7/7/2021 19:28, Chris Johns wrote:
>>>> 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.
> Sorry, I misunderstood what you were saying. You're correct that a PHY 
> detection does not occur in the CGEM probe, but that wouldn't improve 
> the current situation since PHYs are being detected on all available 
> slots. If that weren't happening there wouldn't be PHY read/write 
> errors, either.
>>> 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.
> From above, the probe wouldn't fail in that case due to the spurious 
> PHY detections unless you're trying to do a read/write transaction 
> with the PHY and waiting for the timeout. This doesn't seem reasonable 
> to do in a device probe.
>>>> 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.
>>> 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.
>
> Ok, so the ukphy driver is too risky for behavioral changes and the 
> CGEM driver needs to be tweaked to be a bit less verbose.
>
> The solutions to the remainder of the problem are:
>
> 1) Use a different, smarter PHY driver and avoid use of ukphy when 
> multiple interfaces are present
>
> ** This could possibly solve the general problem for ethernet 
> interfaces and replace the ukphy driver for new multi-interface hardware
>
> ** Lets nexus-devices.h contain all interfaces that exist on the 
> hardware, possibly applicable to Zynq7000 and versal as well
>
>
> 2) Disable unused interfaces via weak-reference call in the CGEM probe
>
> ** This solves the CGEM problem and provides a template on how to 
> solve the general problem for other BSPS/interfaces
>
> ** Allows the application to make arbitrarily different choices by 
> implementing a relatively trivial function
>
> ** This feels like it's working toward a partial reimplementation of 
> FDT with hardware configuration embedded in code
>
> ** Lets nexus-devices.h contain all interfaces that exist on the 
> hardware, possibly applicable to Zynq7000 and versal as well
>
>
> 3) Disable unused interfaces via header configuration conditionals
>
> ** Allows the application to define the interfaces it needs using 
> existing mechanisms
>
> ** Doesn't provide any network interfaces by default to the application
>
> ** Only solves the problem one BSP at a time with extra preprocessor 
> logic
>
>
> 4) Embed FDT somewhere that defines what interfaces get configured
>
> ** Pulls in extra dependencies
>
> ** Doesn't solve the test issue without generation/embedding of a FDT
>
> ** Allows the application to have exactly what it wants via a 
> relatively standard mechanism
>
> ** Lets nexus-devices.h contain all interfaces that exist on the 
> hardware, possibly applicable to Zynq7000 and versal as well
>
>
> I'm starting to lean toward option #1. It allows all interfaces to be 
> defined and the primary interface for testing can be selected by the 
> existing test configuration in config.inc. Applications can check the 
> available interfaces for connectivity or they can prune the nexus 
> devices in their own copy instead of pulling in nexus-devices.h.

I looked a little further into what it would take to make a smarter 
version of ukphy and ukphy isn't actually the issue here. The code in 
mii.c that detects whether a PHY exists at all has a bug in it where PHY 
read timeouts aren't detected and handled and so the 0xffffffff result 
of the failed read slips through the bad-PHY check. I'm working up and 
verifying the patch for that now. Fixing that should hopefully allow for 
all interfaces to be enabled without issue.


Kinsey