Re: [net-next: PATCH 08/12] ACPI: scan: prevent double enumeration of MDIO bus children

[Marcin Wojtas]

czw., 23 cze 2022 o 09:42 Andrew Lunn <andrew@xxxxxxx> napisał(a):
>
> > And when the ACPI subsystem finds those device objects present in the
> > ACPI tables, the mdio_device things have not been created yet and it
> > doesn't know which ACPI device object will correspond to mdio_device
> > eventually unless it is told about that somehow.  One way of doing
> > that is to use a list of device IDs in the kernel.  The other is to
> > have the firmware tell it about that which is what we are discussing.
>
> Device IDs is a complex subject with MDIO devices. It has somewhat
> evolved over time, and it could also be that ACPI decides to do
> something different, or simpler, to what DT does.
>
> If the device is an Ethernet PHY, and it follows C22, it has two
> registers in a well defined location, which when combined give you a
> vendor model and version. So we scan the bus, look at each address on
> the bus, try to read these registers and if we don't get 0xffff back,
> we assume it is a PHY, create an mdio_device, sub type it to
> phy_device, and then load and probe the driver based on the ID
> registers.
>
> If the device is using C45, we currently will not be able to enumerate
> it this way. We have a number of MDIO bus drivers which don't
> implement C45, but also don't return -EOPNOTSUPP. They will perform a
> malformed C22 transaction, or go wrong in some other horrible way. So
> in DT, we have a compatible string to indicate there is a C45 devices
> at the address. We then do look around in the C45 address space at the
> different locations where the ID registers can be, and if we get a
> valid looking ID, probe the driver using that.
>
> We also have some chicken/egg problems. Some PHYs won't respond when
> you read there ID registers until you have turned on clocks, disabled
> reset lines, enable regulators etc. For these devices, we place the ID
> as you would read from the ID registers in DT as the compatible
> string. The mdio_device is created, sub-types as a PHY and the probe
> happens using the ID register found in DT. The driver can then do what
> it needs to do to get the device to respond on the bus.
>

Currently the PHY detection (based on compatible string property in
_DSD) and handling of both ACPI and DT paths are shared by calling the
same routine fwnode_mdiobus_register_phy() and all the following
generic code. No ID's involved.

With MDIOSerialBus property we can probably pass additional
information about PHY's via one of the fields in _CRS, however, this
will implicate deviating from the common code with DT. Let's discuss
it under ECR.

> Then we have devices on the bus which are not PHYs, but generic
> mdio_devices. These are mostly Ethernet switches, but Broadcom have
> some other MDIO devices which are not switches. For these, we have
> compatible strings which identifies the device as a normal string,
> which then probes the correct driver in the normal way for a
> compatible string.

_HID/_CID fields will be used for that, as in any other driver. In
case Broadcom decides to support ACPI, they will have to define their
own ACPI ID and update the relevant driver (extend struct mdio_driver
with  .acpi_match_table field) - see patch 12/12 as an example.

>
> So giving the kernel a list of device IDs is not simple. I expect
> dealing with this will be a big part of defining how MDIOSerialBus
> works.
>

Actually the _HID/_CID fields values will still be required for the
devices on the bus and the relevant drivers will use it for matching,
which is analogous for the compatible string handling. The
MDIOSerialBus _CRS macro will not be used for this purpose, same as
already existing examples of I2CSerialBus or SPISerialBus (although
the child devices use them, they also have _HID/_CID for
identification).

What we agreed for is to get rid of is a static list of MDIO
controllers ID's, which I proposed in this patch, whose intention was
to prevent its enumeration by the default ACPI scan routines, in case
the device's parent is a listed MDIO bus. Instead, just the presence
of MDIOSerialBus macro in the _CRS method of the child device will
suffice to get it skipped at that point. Any other data in this macro
will be in fact something extra that we can use for any purpose.

Best regards,
Marcin