The MDIO bus is responsible for probing and registering its respective
children, such as PHYs or other kind of devices.
It is required that ACPI scan code should not enumerate such
devices, leaving this task for the generic MDIO bus routines,
which are initiated by the controller driver.
This patch prevents unwanted enumeration of the devices by setting
'enumeration_by_parent' flag, depending on whether their parent
device is a member of a known list of MDIO controllers. For now,
the Marvell MDIO controllers' IDs are added.
Signed-off-by: Marcin Wojtas <[email protected]>
---
drivers/acpi/scan.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 762b61f67e6c..d703c35dc218 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1716,6 +1716,18 @@ static bool acpi_is_indirect_io_slave(struct acpi_device *device)
return parent && !acpi_match_device_ids(parent, indirect_io_hosts);
}
+static bool acpi_is_mdio_child(struct acpi_device *device)
+{
+ struct acpi_device *parent = device->parent;
+ static const struct acpi_device_id mdio_controllers[] = {
+ {"MRVL0100", 0},
+ {"MRVL0101", 0},
+ {}
+ };
+
+ return parent && !acpi_match_device_ids(parent, mdio_controllers);
+}
+
static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
{
struct list_head resource_list;
@@ -1756,6 +1768,9 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
if (acpi_is_indirect_io_slave(device))
return true;
+ if (acpi_is_mdio_child(device))
+ return true;
+
/* Macs use device properties in lieu of _CRS resources */
if (x86_apple_machine &&
(fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
--
2.29.0
On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> The MDIO bus is responsible for probing and registering its respective
> children, such as PHYs or other kind of devices.
>
> It is required that ACPI scan code should not enumerate such
> devices, leaving this task for the generic MDIO bus routines,
> which are initiated by the controller driver.
>
> This patch prevents unwanted enumeration of the devices by setting
> 'enumeration_by_parent' flag, depending on whether their parent
> device is a member of a known list of MDIO controllers. For now,
> the Marvell MDIO controllers' IDs are added.
This flag is used for serial buses that are not self-discoverable. Not sure
about MDIO, but the current usage has a relation to the _CRS. Have you
considered to propose the MdioSerialBus() resource type to ACPI specification?
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> The MDIO bus is responsible for probing and registering its respective
> children, such as PHYs or other kind of devices.
>
> It is required that ACPI scan code should not enumerate such
> devices, leaving this task for the generic MDIO bus routines,
> which are initiated by the controller driver.
I suppose the question is, should you ignore the ACPI way of doing
things, or embrace the ACPI way?
At least please add a comment why the ACPI way is wrong, despite this
being an ACPI binding.
Andrew
pon., 20 cze 2022 o 19:53 Andy Shevchenko
<[email protected]> napisał(a):
>
> On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> > The MDIO bus is responsible for probing and registering its respective
> > children, such as PHYs or other kind of devices.
> >
> > It is required that ACPI scan code should not enumerate such
> > devices, leaving this task for the generic MDIO bus routines,
> > which are initiated by the controller driver.
> >
> > This patch prevents unwanted enumeration of the devices by setting
> > 'enumeration_by_parent' flag, depending on whether their parent
> > device is a member of a known list of MDIO controllers. For now,
> > the Marvell MDIO controllers' IDs are added.
>
> This flag is used for serial buses that are not self-discoverable. Not sure
> about MDIO, but the current usage has a relation to the _CRS. Have you
> considered to propose the MdioSerialBus() resource type to ACPI specification?
>
Indeed, one of the cases checked in the
acpi_device_enumeration_by_parent() is checking _CRS (of the bus child
device) for being of the serial bus type. Currently I see
I2C/SPI/UARTSerialBus resource descriptors in the specification. Since
MDIO doesn't seem to require any special description macros like the
mentioned ones (for instance see I2CSerialBusV2 [1]), Based on
example: dfda4492322ed ("ACPI / scan: Do not enumerate Indirect IO
host children"), I thought of similar one perhaps being applicable.
Maybe there is some different, more proper solution, I'd be happy to
hear from the ACPI Maintainers.
[1] https://uefi.org/specs/ACPI/6.4/19_ASL_Reference/ACPI_Source_Language_Reference.html?highlight=i2cserialbus#i2cserialbusterm
Best regards,
Marcin
On Mon, Jun 20, 2022 at 9:08 PM Andrew Lunn <[email protected]> wrote:
>
> On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> > The MDIO bus is responsible for probing and registering its respective
> > children, such as PHYs or other kind of devices.
> >
> > It is required that ACPI scan code should not enumerate such
> > devices, leaving this task for the generic MDIO bus routines,
> > which are initiated by the controller driver.
>
> I suppose the question is, should you ignore the ACPI way of doing
> things, or embrace the ACPI way?
What do you mean by "the ACPI way"?
> At least please add a comment why the ACPI way is wrong, despite this
> being an ACPI binding.
The question really is whether or not it is desirable to create
platform devices for all of the objects found in the ACPI tables that
correspond to the devices on the MDIO bus.
I don't think it is, so it should be avoided.
On Tue, Jun 21, 2022 at 1:05 AM Marcin Wojtas <[email protected]> wrote:
>
> pon., 20 cze 2022 o 19:53 Andy Shevchenko
> <[email protected]> napisał(a):
> >
> > On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> > > The MDIO bus is responsible for probing and registering its respective
> > > children, such as PHYs or other kind of devices.
> > >
> > > It is required that ACPI scan code should not enumerate such
> > > devices, leaving this task for the generic MDIO bus routines,
> > > which are initiated by the controller driver.
> > >
> > > This patch prevents unwanted enumeration of the devices by setting
> > > 'enumeration_by_parent' flag, depending on whether their parent
> > > device is a member of a known list of MDIO controllers. For now,
> > > the Marvell MDIO controllers' IDs are added.
> >
> > This flag is used for serial buses that are not self-discoverable. Not sure
> > about MDIO, but the current usage has a relation to the _CRS. Have you
> > considered to propose the MdioSerialBus() resource type to ACPI specification?
> >
>
> Indeed, one of the cases checked in the
> acpi_device_enumeration_by_parent() is checking _CRS (of the bus child
> device) for being of the serial bus type. Currently I see
> I2C/SPI/UARTSerialBus resource descriptors in the specification. Since
> MDIO doesn't seem to require any special description macros like the
> mentioned ones (for instance see I2CSerialBusV2 [1]), Based on
> example: dfda4492322ed ("ACPI / scan: Do not enumerate Indirect IO
> host children"), I thought of similar one perhaps being applicable.
>
> Maybe there is some different, more proper solution, I'd be happy to
> hear from the ACPI Maintainers.
>
> [1] https://uefi.org/specs/ACPI/6.4/19_ASL_Reference/ACPI_Source_Language_Reference.html?highlight=i2cserialbus#i2cserialbusterm
Well, the approach based on lists of device IDs is not scalable and
generally used as the last resort one.
It would be a lot better to have a way of representing connections to
the MDIO bus as resources in _CRS.
śr., 22 cze 2022 o 14:09 Rafael J. Wysocki <[email protected]> napisał(a):
>
> On Tue, Jun 21, 2022 at 1:05 AM Marcin Wojtas <[email protected]> wrote:
> >
> > pon., 20 cze 2022 o 19:53 Andy Shevchenko
> > <[email protected]> napisał(a):
> > >
> > > On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> > > > The MDIO bus is responsible for probing and registering its respective
> > > > children, such as PHYs or other kind of devices.
> > > >
> > > > It is required that ACPI scan code should not enumerate such
> > > > devices, leaving this task for the generic MDIO bus routines,
> > > > which are initiated by the controller driver.
> > > >
> > > > This patch prevents unwanted enumeration of the devices by setting
> > > > 'enumeration_by_parent' flag, depending on whether their parent
> > > > device is a member of a known list of MDIO controllers. For now,
> > > > the Marvell MDIO controllers' IDs are added.
> > >
> > > This flag is used for serial buses that are not self-discoverable. Not sure
> > > about MDIO, but the current usage has a relation to the _CRS. Have you
> > > considered to propose the MdioSerialBus() resource type to ACPI specification?
> > >
> >
> > Indeed, one of the cases checked in the
> > acpi_device_enumeration_by_parent() is checking _CRS (of the bus child
> > device) for being of the serial bus type. Currently I see
> > I2C/SPI/UARTSerialBus resource descriptors in the specification. Since
> > MDIO doesn't seem to require any special description macros like the
> > mentioned ones (for instance see I2CSerialBusV2 [1]), Based on
> > example: dfda4492322ed ("ACPI / scan: Do not enumerate Indirect IO
> > host children"), I thought of similar one perhaps being applicable.
> >
> > Maybe there is some different, more proper solution, I'd be happy to
> > hear from the ACPI Maintainers.
> >
> > [1] https://uefi.org/specs/ACPI/6.4/19_ASL_Reference/ACPI_Source_Language_Reference.html?highlight=i2cserialbus#i2cserialbusterm
>
> Well, the approach based on lists of device IDs is not scalable and
> generally used as the last resort one.
>
> It would be a lot better to have a way of representing connections to
> the MDIO bus as resources in _CRS.
Thank you for your input. I will submit a proposal for MDIOSerialBus
_CRS resource macro then.
Best regards,
Marcin
On 6/22/22 05:05, Rafael J. Wysocki wrote:
> On Mon, Jun 20, 2022 at 9:08 PM Andrew Lunn <[email protected]> wrote:
>>
>> On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
>>> The MDIO bus is responsible for probing and registering its respective
>>> children, such as PHYs or other kind of devices.
>>>
>>> It is required that ACPI scan code should not enumerate such
>>> devices, leaving this task for the generic MDIO bus routines,
>>> which are initiated by the controller driver.
>>
>> I suppose the question is, should you ignore the ACPI way of doing
>> things, or embrace the ACPI way?
>
> What do you mean by "the ACPI way"?
>
>> At least please add a comment why the ACPI way is wrong, despite this
>> being an ACPI binding.
>
> The question really is whether or not it is desirable to create
> platform devices for all of the objects found in the ACPI tables that
> correspond to the devices on the MDIO bus.
If we have devices hanging off a MDIO bus then they are mdio_device (and
possibly a more specialized object with the phy_device which does embedd
a mdio_device object), not platform devices, since MDIO is a bus in itself.
--
Florian
On Wed, Jun 22, 2022 at 6:12 PM Florian Fainelli <[email protected]> wrote:
>
> On 6/22/22 05:05, Rafael J. Wysocki wrote:
> > On Mon, Jun 20, 2022 at 9:08 PM Andrew Lunn <[email protected]> wrote:
> >>
> >> On Mon, Jun 20, 2022 at 05:02:21PM +0200, Marcin Wojtas wrote:
> >>> The MDIO bus is responsible for probing and registering its respective
> >>> children, such as PHYs or other kind of devices.
> >>>
> >>> It is required that ACPI scan code should not enumerate such
> >>> devices, leaving this task for the generic MDIO bus routines,
> >>> which are initiated by the controller driver.
> >>
> >> I suppose the question is, should you ignore the ACPI way of doing
> >> things, or embrace the ACPI way?
> >
> > What do you mean by "the ACPI way"?
> >
> >> At least please add a comment why the ACPI way is wrong, despite this
> >> being an ACPI binding.
> >
> > The question really is whether or not it is desirable to create
> > platform devices for all of the objects found in the ACPI tables that
> > correspond to the devices on the MDIO bus.
>
> If we have devices hanging off a MDIO bus then they are mdio_device (and
> possibly a more specialized object with the phy_device which does embedd
> a mdio_device object), not platform devices, since MDIO is a bus in itself.
Well, that's what I'm saying.
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.
> 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.
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.
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.
Andrew
czw., 23 cze 2022 o 09:42 Andrew Lunn <[email protected]> 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