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

[Kinsey Moore]

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.


Kinsey