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

[Chris Johns]

On 9/7/21 7:32 am, Kinsey Moore wrote:
> 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.

Yeap.

>>
>> 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
>>

The problem here is some PHYs are covered by NDAs and a PHY that uses just the
IEEE defined registers, ie UKPHY, is a good solution for a number of cases.

I do not think the PHY drivers are the issue here.

>> 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

Yes. Adding FDT and not breaking existing users is not easy.

>> ** Lets nexus-devices.h contain all interfaces that exist on the hardware,
>> possibly applicable to Zynq7000 and versal as well

Yeah. The Versal MRMAC may be an exception here. I have not had time to examine
all the detail that surrounds that MAC implementation.

>> 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

It is hard to remove once released and user depend on it. We spend a lot of time
in RTEMS attempting to handle these cases further down the road.

>> 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 wonder if it is worth considering how a user would add and remove interfaces
using this approach before we head down this path?

>> 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.

The mii.c only comes into play when the probe passes and we are attaching the
device.

I was thinking of a MII probe call ...

#if __rtems__
int mii_probe()
{
  for (phy = 0; phy < 31; phy++) {
    WR4(sc, CGEM_PHY_MAINT,
      CGEM_PHY_MAINT_CLAUSE_22 | CGEM_PHY_MAINT_MUST_10 |
      CGEM_PHY_MAINT_OP_READ |
      (phy << CGEM_PHY_MAINT_PHY_ADDR_SHIFT) |
      (reg << CGEM_PHY_MAINT_REG_ADDR_SHIFT));

    /* Wait for completion. */
    tries=0;
    for (tries = 0; tries < 5; ++tries) {
      if ((RD4(sc, CGEM_NET_STAT) & CGEM_NET_STAT_PHY_MGMT_IDLE) != 0) {
        return OK;
      }
      DELAY(2);
    }
  }
  return NOT_FOUND;
}
#endif

This only works if the MAC can be accessed this early in a boot and dos not need
anything else to be enabled. Maybe the enables are easy as well, I have not looked.

For an interface with no PHY and not being used the MII probe delay is 192msec
which I think is just OK, maybe (??), but more than I would like.

Chris