| Message ID | 20220620150225.1307946-9-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ACPI support for DSA | expand |
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?
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 <andriy.shevchenko@linux.intel.com> 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 <andrew@lunn.ch> 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 <mw@semihalf.com> wrote: > > pon., 20 cze 2022 o 19:53 Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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 <rafael@kernel.org> napisał(a): > > On Tue, Jun 21, 2022 at 1:05 AM Marcin Wojtas <mw@semihalf.com> wrote: > > > > pon., 20 cze 2022 o 19:53 Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> 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 <andrew@lunn.ch> 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.
On Wed, Jun 22, 2022 at 6:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 6/22/22 05:05, Rafael J. Wysocki wrote: > > On Mon, Jun 20, 2022 at 9:08 PM Andrew Lunn <andrew@lunn.ch> 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 <andrew@lunn.ch> 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
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") ||
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 <mw@semihalf.com> --- drivers/acpi/scan.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)