| Message ID | 20220620150225.1307946-1-mw@semihalf.com |
|---|---|
| Headers | show |
| Series | ACPI support for DSA | expand |
On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote: > Hi! > > This patchset introduces the support for DSA in ACPI world. A couple of > words about the background and motivation behind those changes: > > The DSA code is strictly dependent on the Device Tree and Open Firmware > (of_*) interface, both in the drivers and the common high-level net/dsa API. > The only alternative is to pass the information about the topology via > platform data - a legacy approach used by older systems that compiled the > board description into the kernel. > > The above constraint is problematic for the embedded devices based e.g. on > x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks > and workarounds have to be applied. Addition of switch description to > DSDT/SSDT tables would help to solve many similar cases and use unmodified > kernel modules. It also enables this feature for ARM64 ACPI users. > > The key enablements allowing for adding ACPI support for DSA in Linux were > NIC drivers, MDIO, PHY, and phylink modifications – the latter three merged > in 2021. I thought it would be worth to experiment with DSA, which seemed > to be a natural follow-up challenge. > > It turned out that without much hassle it is possible to describe > DSA-compliant switches as child devices of the MDIO busses, which are > responsible for their enumeration based on the standard _ADR fields and > description in _DSD objects under 'device properties' UUID [1]. > The vast majority of required changes were simple of_* to fwnode_* > transition, as the DT and ACPI topolgies are analogous, except for > 'ports' and 'mdio' subnodes naming, as they don't conform ACPI > namespace constraints [2]. ... > Note that for now cascade topology remains unsupported in ACPI world > (based on "dsa" label and "link" property values). It seems to be feasible, > but would extend this patchset due to necessity of of_phandle_iterator > migration to fwnode_. Leave it as a possible future step. Wondering if this can be done using fwnode graph.
On Mon, Jun 20, 2022 at 05:02:14PM +0200, Marcin Wojtas wrote: > This patch allows to use fixed_phy driver and its helper > functions without Device Tree dependency, by swtiching from > of_ to fwnode_ API. ... > -#ifdef CONFIG_OF_GPIO Nice to see this gone, because it's my goal as well. ... > -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > +static struct gpio_desc *fixed_phy_get_gpiod(struct fwnode_handle *fwnode) > { > - struct device_node *fixed_link_node; > + struct fwnode_handle *fixed_link_node; > struct gpio_desc *gpiod; > - if (!np) > + if (!fwnode) > return NULL; Can be dropped altogether. The following call will fail and return the same. > - fixed_link_node = of_get_child_by_name(np, "fixed-link"); > + fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link"); > if (!fixed_link_node) > return NULL; > > @@ -204,7 +203,7 @@ static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > * Linux device associated with it, we simply have obtain > * the GPIO descriptor from the device tree like this. > */ > - gpiod = fwnode_gpiod_get_index(of_fwnode_handle(fixed_link_node), > + gpiod = fwnode_gpiod_get_index(fixed_link_node, > "link", 0, GPIOD_IN, "mdio"); Can fit one line now. > if (IS_ERR(gpiod) && PTR_ERR(gpiod) != -EPROBE_DEFER) { > if (PTR_ERR(gpiod) != -ENOENT) > @@ -212,20 +211,14 @@ static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np) > fixed_link_node); > gpiod = NULL; > } > - of_node_put(fixed_link_node); > + fwnode_handle_put(fixed_link_node); > > return gpiod; > } ... > - of_node_get(np); > - phy->mdio.dev.of_node = np; > + fwnode_handle_get(fwnode); > + phy->mdio.dev.fwnode = fwnode; Please, use device_set_node(). ... > + fwnode_handle_put(phy->mdio.dev.fwnode); dev_fwnode()
On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote: > Hi! > > This patchset introduces the support for DSA in ACPI world. A couple of > words about the background and motivation behind those changes: > > The DSA code is strictly dependent on the Device Tree and Open Firmware > (of_*) interface, both in the drivers and the common high-level net/dsa API. > The only alternative is to pass the information about the topology via > platform data - a legacy approach used by older systems that compiled the > board description into the kernel. Not true. There are deployed x86 systems which do this, and they are fully up to date, not legacy. There are however limitations in what you can do. So please drop this wording. > The above constraint is problematic for the embedded devices based e.g. on > x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks > and workarounds have to be applied. It would be good to describe the limitations. As i said, there are x86 systems running with marvell 6390 switches. > It turned out that without much hassle it is possible to describe > DSA-compliant switches as child devices of the MDIO busses, which are > responsible for their enumeration based on the standard _ADR fields and > description in _DSD objects under 'device properties' UUID [1]. No surprises there. That is how the DT binding works. And the current ACPI concept is basically DT in different words. Maybe the more important question is, is rewording DT in ACPI the correct approach, or should you bo doing a more native ACPI implementation? I cannot answer that, you need to ask the ACPI maintainers. > Note that for now cascade topology remains unsupported in ACPI world > (based on "dsa" label and "link" property values). It seems to be feasible, > but would extend this patchset due to necessity of of_phandle_iterator > migration to fwnode_. Leave it as a possible future step. We really do need to ensure this is possible. You are setting an ABI here, which everybody else in the ACPI world needs to follow. Cascaded switches is fundamental to DSA, it is the D in DSA. So i would prefer that you at least define and document the binding for D in DSA and get it sanity checked by the ACPI people. Andrew
On Mon, Jun 20, 2022 at 07:55:44PM +0200, Andrew Lunn wrote: > On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote: ... > > It turned out that without much hassle it is possible to describe > > DSA-compliant switches as child devices of the MDIO busses, which are > > responsible for their enumeration based on the standard _ADR fields and > > description in _DSD objects under 'device properties' UUID [1]. > > No surprises there. That is how the DT binding works. And the current > ACPI concept is basically DT in different words. Maybe the more > important question is, is rewording DT in ACPI the correct approach, > or should you bo doing a more native ACPI implementation? I cannot > answer that, you need to ask the ACPI maintainers. You beat me up to this. I also was about to mention that the problem with such conversions (like this series does) is not in the code. It's simplest part. The problem is bindings and how you get them to be a standard (at least de facto). > > Note that for now cascade topology remains unsupported in ACPI world > > (based on "dsa" label and "link" property values). It seems to be feasible, > > but would extend this patchset due to necessity of of_phandle_iterator > > migration to fwnode_. Leave it as a possible future step. > > We really do need to ensure this is possible. You are setting an ABI > here, which everybody else in the ACPI world needs to follow. Cascaded > switches is fundamental to DSA, it is the D in DSA. So i would prefer > that you at least define and document the binding for D in DSA and get > it sanity checked by the ACPI people.
> static int dsa_port_parse_dsa(struct dsa_port *dp) > { > + /* Cascade switch connection is not supported in ACPI world. */ > + if (is_acpi_node(dp->fwnode)) { > + dev_warn(dp->ds->dev, > + "DSA type is not supported with ACPI, disable port #%d\n", > + dp->index); > + dp->type = DSA_PORT_TYPE_UNUSED; > + return 0; > + } > + Did you try this? I'm not sure it will work correctly. When a switch registers with the DSA core, the core will poke around in DT and fill in various bits of information, including the DSA links. Once that has completed, the core will look at all the switches registered so far and try to determine if it has a complete set, i.e, it has both ends of all DSA links. If it does have a complete set, it then calls the setup methods on each switch, and gets them configured. If it finds it does not have a complete setup, it does nothing, waiting for the next switch to register. So if somebody passed an ACPI description with multiple switches, it is likely to call the setup methods as soon as the first switch is registered. And it might call those same setup methods a second time, when the second switch registers, on both switches. And when the third switch registers, it will probably call the setup methods yet again on all the switches.... You will have a much safer system if you return -EINVAL if you find a DSA link in ACPI. That should abort the switch probe. Andrew
> You beat me up to this. I also was about to mention that the problem with such > conversions (like this series does) is not in the code. It's simplest part. The > problem is bindings and how you get them to be a standard (at least de facto). De facto is easy. Get it merged. After that, i will simply refuse anything else, the same way i and other Maintainers would refuse a different DT binding. If the ACPI committee approve and publish a binding, we will naturally accept that as well. So in the end we might have two bindings. But so far in this whole ACPI for networking story, i've not heard anybody say they are going to submit anything for standardisation. So this might be a mute point. Andrew
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:46 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > On Mon, Jun 20, 2022 at 05:02:18PM +0200, Marcin Wojtas wrote: > > A helper function which allows getting the struct net_device pointer > > associated with a given device tree node can be more generic and > > also support alternative hardware description. Switch to fwnode_ > > and update the only existing caller in DSA subsystem. > > ... > > > +static int fwnode_dev_node_match(struct device *dev, const void *data) > > { > > for (; dev; dev = dev->parent) { > > - if (dev->of_node == data) > > > + if (dev->fwnode == data) > > > We have a helper in device/bus.h (?) device_match_fwnode(). > That's true, thanks. > > return 1; > > } > > But this all sounds like a good candidate to be generic. Do we have more users > in the kernel of a such? > Do you mean fwnode_dev_node_match? I haven't noticed. Indeed, it may be worth to move this one to drivers/base/property.c - what do you think? Thanks, Marcin
pon., 20 cze 2022 o 20:32 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > static int dsa_port_parse_dsa(struct dsa_port *dp) > > { > > + /* Cascade switch connection is not supported in ACPI world. */ > > + if (is_acpi_node(dp->fwnode)) { > > + dev_warn(dp->ds->dev, > > + "DSA type is not supported with ACPI, disable port #%d\n", > > + dp->index); > > + dp->type = DSA_PORT_TYPE_UNUSED; > > + return 0; > > + } > > + > > Did you try this? I'm not sure it will work correctly. When a switch > registers with the DSA core, the core will poke around in DT and fill > in various bits of information, including the DSA links. Once that has > completed, the core will look at all the switches registered so far > and try to determine if it has a complete set, i.e, it has both ends > of all DSA links. If it does have a complete set, it then calls the > setup methods on each switch, and gets them configured. If it finds it > does not have a complete setup, it does nothing, waiting for the next > switch to register. > > So if somebody passed an ACPI description with multiple switches, it > is likely to call the setup methods as soon as the first switch is > registered. And it might call those same setup methods a second time, > when the second switch registers, on both switches. And when the third > switch registers, it will probably call the setup methods yet again on > all the switches.... > > You will have a much safer system if you return -EINVAL if you find a > DSA link in ACPI. That should abort the switch probe. > I only set a single port to "dsa" label to check if this condition is entered. I see 2 devices in the arm64 tree (fsl-lx2160a-bluebox3.dts and armada-3720-turris-mox.dts) that support cascade switches via "link" property. I don't have access to real life setup (and those seem to not support ACPI anyway...). In case this temporarily would remain as unsupported feature, I agree -EINVAL is a safer solution. Thanks, Marcin
pon., 20 cze 2022 o 19:59 Andrew Lunn <andrew@lunn.ch> napisał(a): > > On Mon, Jun 20, 2022 at 05:02:14PM +0200, Marcin Wojtas wrote: > > This patch allows to use fixed_phy driver and its helper > > functions without Device Tree dependency, by swtiching from > > of_ to fwnode_ API. > > Do you actually need this? phylink does not use this code, it has its > own fixed link implementation. And that implementation is not limited > to 1G. > Yes, phylink has its own fixed-link handling, however the net/dsa/port.c relies on fixed_phy helpers these are not 1:1 equivalents. I assumed this migration (fixed_phy -> phylink) is not straightforward and IMO should be handled separately. Do you recall justification for not using phylink in this part of net/dsa/*? Thanks, Marcin
On Tue, Jun 21, 2022 at 11:56:06AM +0200, Marcin Wojtas wrote: > pon., 20 cze 2022 o 19:59 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > On Mon, Jun 20, 2022 at 05:02:14PM +0200, Marcin Wojtas wrote: > > > This patch allows to use fixed_phy driver and its helper > > > functions without Device Tree dependency, by swtiching from > > > of_ to fwnode_ API. > > > > Do you actually need this? phylink does not use this code, it has its > > own fixed link implementation. And that implementation is not limited > > to 1G. > > > > Yes, phylink has its own fixed-link handling, however the > net/dsa/port.c relies on fixed_phy helpers these are not 1:1 > equivalents. I assumed this migration (fixed_phy -> phylink) is not > straightforward and IMO should be handled separately. Do you recall > justification for not using phylink in this part of net/dsa/*? All modern DSA drivers use phylink and not fixed-phy as far as I'm aware - there are a number that still implement the .adjust_link callback, but note in dsa_port_link_register_of(): if (!ds->ops->adjust_link) { ... return 0; } dev_warn(ds->dev, "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n"); It's really just that they haven't been migrated.
pon., 20 cze 2022 o 19:21 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote: > > Hi! > > > > This patchset introduces the support for DSA in ACPI world. A couple of > > words about the background and motivation behind those changes: > > > > The DSA code is strictly dependent on the Device Tree and Open Firmware > > (of_*) interface, both in the drivers and the common high-level net/dsa API. > > The only alternative is to pass the information about the topology via > > platform data - a legacy approach used by older systems that compiled the > > board description into the kernel. > > > > The above constraint is problematic for the embedded devices based e.g. on > > x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks > > and workarounds have to be applied. Addition of switch description to > > DSDT/SSDT tables would help to solve many similar cases and use unmodified > > kernel modules. It also enables this feature for ARM64 ACPI users. > > > > The key enablements allowing for adding ACPI support for DSA in Linux were > > NIC drivers, MDIO, PHY, and phylink modifications – the latter three merged > > in 2021. I thought it would be worth to experiment with DSA, which seemed > > to be a natural follow-up challenge. > > > > It turned out that without much hassle it is possible to describe > > DSA-compliant switches as child devices of the MDIO busses, which are > > responsible for their enumeration based on the standard _ADR fields and > > description in _DSD objects under 'device properties' UUID [1]. > > The vast majority of required changes were simple of_* to fwnode_* > > transition, as the DT and ACPI topolgies are analogous, except for > > 'ports' and 'mdio' subnodes naming, as they don't conform ACPI > > namespace constraints [2]. > > ... > > > Note that for now cascade topology remains unsupported in ACPI world > > (based on "dsa" label and "link" property values). It seems to be feasible, > > but would extend this patchset due to necessity of of_phandle_iterator > > migration to fwnode_. Leave it as a possible future step. > > Wondering if this can be done using fwnode graph. > Probably yes. It's a general question whether to follow iterating over phandles pointed by properties, like DT with a minimal code change or do something completely different. Best regards, Marcin
pon., 20 cze 2022 o 19:56 Andrew Lunn <andrew@lunn.ch> napisał(a): > > On Mon, Jun 20, 2022 at 05:02:13PM +0200, Marcin Wojtas wrote: > > Hi! > > > > This patchset introduces the support for DSA in ACPI world. A couple of > > words about the background and motivation behind those changes: > > > > The DSA code is strictly dependent on the Device Tree and Open Firmware > > (of_*) interface, both in the drivers and the common high-level net/dsa API. > > The only alternative is to pass the information about the topology via > > platform data - a legacy approach used by older systems that compiled the > > board description into the kernel. > > Not true. There are deployed x86 systems which do this, and they are > fully up to date, not legacy. There are however limitations in what > you can do. So please drop this wording. > Ok, thanks for clarification, agree for rewording. Afair pdata was a legacy derived from the Orion/Dove times, but indeed it can be used in the new systems that lack other switch description. > > The above constraint is problematic for the embedded devices based e.g. on > > x86_64 SoCs, which are described by ACPI tables - to use DSA, some tricks > > and workarounds have to be applied. > > It would be good to describe the limitations. As i said, there are x86 > systems running with marvell 6390 switches. I'm aware of that and even saw some x86_64 + Marvell switch contemporary examples of how lack of DT in system was worked around: - out of tree updates to the module code - keep small DT blob on the system storage, from where the mv88e6xxx Those could be poor-coding / anecdotic showcases. I'd be happy to learn if there is a proper and recommended way, how to do it properly. > > > It turned out that without much hassle it is possible to describe > > DSA-compliant switches as child devices of the MDIO busses, which are > > responsible for their enumeration based on the standard _ADR fields and > > description in _DSD objects under 'device properties' UUID [1]. > > No surprises there. That is how the DT binding works. And the current > ACPI concept is basically DT in different words. Maybe the more > important question is, is rewording DT in ACPI the correct approach, > or should you bo doing a more native ACPI implementation? I cannot > answer that, you need to ask the ACPI maintainers. This is why I added linux-acpi list and the ACPI Maintainers to discuss > > > Note that for now cascade topology remains unsupported in ACPI world > > (based on "dsa" label and "link" property values). It seems to be feasible, > > but would extend this patchset due to necessity of of_phandle_iterator > > migration to fwnode_. Leave it as a possible future step. > > We really do need to ensure this is possible. You are setting an ABI > here, which everybody else in the ACPI world needs to follow. Cascaded > switches is fundamental to DSA, it is the D in DSA. So i would prefer > that you at least define and document the binding for D in DSA and get > it sanity checked by the ACPI people. > I'm aware of the "D" importance, just kept it aside for now due to lack of access to relevant HW and willing to discuss the overall approach first. WRT the technical side: multiple-phandle property is for sure supported in _DSD, so the most straightforward would be to follow that and simply migrate to fwnode_. The thing is in arm64 it's not widely used and (testing that with ACPI is making it even harder). There is also an alternative brought by Andy - definitely a thing to discuss further. I think we seem to have a quorum for that among recipents of this thread. Thanks, Marcin
pon., 20 cze 2022 o 20:45 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > You beat me up to this. I also was about to mention that the problem with such > > conversions (like this series does) is not in the code. It's simplest part. The > > problem is bindings and how you get them to be a standard (at least de facto). > > De facto is easy. Get it merged. After that, i will simply refuse > anything else, the same way i and other Maintainers would refuse a > different DT binding. > > If the ACPI committee approve and publish a binding, we will naturally > accept that as well. So in the end we might have two bindings. But so > far in this whole ACPI for networking story, i've not heard anybody > say they are going to submit anything for standardisation. So this > might be a mute point. > I understand your concern and of course it's better to be on a safe side from the beginning. Based on the hitherto discussion under this patchset, I would split the question about standardization to 2 orthogonal topics: 1. Relation to the bus and enumeration: * As pointed out in another patch some switches can be attached to SPI or I2C. In such a case this is simple - SPISerialBus / I2CSerialBus structures in _CRS are included in the ACPI Spec. They allow to comprise more bus specific information and the code in acpi/scan.c marks those child devices as to be enumerated by parent bus. * MDIO bus doesn't have its own _CRS macro in the Spec, on the other hand the _ADR seems to be the only object required for proper operation - this was my base for proposed solution in patch 06/12. 2. The device description (unrelated to which bus it is attached) * In Linux and other OS's there is a great amount of devices conforming the guidelines and using only the standard device identification/configuration objects as per [1]. * Above do not contain custom items and entire information can be obtained by existing, generic ACPI accessors - those devices (e.g. NICs, SD/MMC controllers and many others) are not explicitly mentioned in official standards. * The question, also related to this DSA case - is the ACPI device() hierarchical structure of this kind a subject for standardization for including in official ACPI specification? * In case not, where to document it? Is Linux' Documentation enough? I agree that in the moment of merge it becomes de facto standard ABI and it's worth to sort it out. Rafael, Len, any other ACPI expert - I would appreciate your inputs and clarification of the above. Your recommendation would be extremely helpful. 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.
Hi, wt., 21 cze 2022 o 12:46 Marcin Wojtas <mw@semihalf.com> napisał(a): > > pon., 20 cze 2022 o 20:45 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > > You beat me up to this. I also was about to mention that the problem with such > > > conversions (like this series does) is not in the code. It's simplest part. The > > > problem is bindings and how you get them to be a standard (at least de facto). > > > > De facto is easy. Get it merged. After that, i will simply refuse > > anything else, the same way i and other Maintainers would refuse a > > different DT binding. > > > > If the ACPI committee approve and publish a binding, we will naturally > > accept that as well. So in the end we might have two bindings. But so > > far in this whole ACPI for networking story, i've not heard anybody > > say they are going to submit anything for standardisation. So this > > might be a mute point. > > > > I understand your concern and of course it's better to be on a safe > side from the beginning. Based on the hitherto discussion under this > patchset, I would split the question about standardization to 2 > orthogonal topics: > > 1. Relation to the bus and enumeration: > * As pointed out in another patch some switches can be attached to > SPI or I2C. In such a case this is simple - SPISerialBus / > I2CSerialBus structures > in _CRS are included in the ACPI Spec. They allow to comprise more > bus specific > information and the code in acpi/scan.c marks those child devices > as to be enumerated > by parent bus. > * MDIO bus doesn't have its own _CRS macro in the Spec, on the other > hand the _ADR > seems to be the only object required for proper operation - this > was my base for > proposed solution in patch 06/12. > > 2. The device description (unrelated to which bus it is attached) > * In Linux and other OS's there is a great amount of devices > conforming the guidelines > and using only the standard device identification/configuration > objects as per [1]. > * Above do not contain custom items and entire information can be obtained by > existing, generic ACPI accessors - those devices (e.g. NICs, > SD/MMC controllers and > many others) are not explicitly mentioned in official standards. > * The question, also related to this DSA case - is the ACPI device() > hierarchical > structure of this kind a subject for standardization for including > in official ACPI specification? > * In case not, where to document it? Is Linux' Documentation enough? > I agree that in the moment of merge it becomes de facto standard ABI and > it's worth to sort it out. > > Rafael, Len, any other ACPI expert - I would appreciate your inputs > and clarification > of the above. Your recommendation would be extremely helpful. > Thank you all for vivid discussions. As it may take some time for the MDIOSerialBus _CRS macro review and approval, for now I plan to submit v2 of_ -> fwnode_/device_ migration (patches 1-7,11/12) and skip ACPI-specific additions until it is unblocked by spec extension. 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 5:44 PM Marcin Wojtas <mw@semihalf.com> wrote: > > Hi, > > wt., 21 cze 2022 o 12:46 Marcin Wojtas <mw@semihalf.com> napisał(a): > > > > pon., 20 cze 2022 o 20:45 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > > > > You beat me up to this. I also was about to mention that the problem with such > > > > conversions (like this series does) is not in the code. It's simplest part. The > > > > problem is bindings and how you get them to be a standard (at least de facto). > > > > > > De facto is easy. Get it merged. After that, i will simply refuse > > > anything else, the same way i and other Maintainers would refuse a > > > different DT binding. > > > > > > If the ACPI committee approve and publish a binding, we will naturally > > > accept that as well. So in the end we might have two bindings. But so > > > far in this whole ACPI for networking story, i've not heard anybody > > > say they are going to submit anything for standardisation. So this > > > might be a mute point. > > > > > > > I understand your concern and of course it's better to be on a safe > > side from the beginning. Based on the hitherto discussion under this > > patchset, I would split the question about standardization to 2 > > orthogonal topics: > > > > 1. Relation to the bus and enumeration: > > * As pointed out in another patch some switches can be attached to > > SPI or I2C. In such a case this is simple - SPISerialBus / > > I2CSerialBus structures > > in _CRS are included in the ACPI Spec. They allow to comprise more > > bus specific > > information and the code in acpi/scan.c marks those child devices > > as to be enumerated > > by parent bus. > > * MDIO bus doesn't have its own _CRS macro in the Spec, on the other > > hand the _ADR > > seems to be the only object required for proper operation - this > > was my base for > > proposed solution in patch 06/12. > > > > 2. The device description (unrelated to which bus it is attached) > > * In Linux and other OS's there is a great amount of devices > > conforming the guidelines > > and using only the standard device identification/configuration > > objects as per [1]. > > * Above do not contain custom items and entire information can be obtained by > > existing, generic ACPI accessors - those devices (e.g. NICs, > > SD/MMC controllers and > > many others) are not explicitly mentioned in official standards. > > * The question, also related to this DSA case - is the ACPI device() > > hierarchical > > structure of this kind a subject for standardization for including > > in official ACPI specification? > > * In case not, where to document it? Is Linux' Documentation enough? > > I agree that in the moment of merge it becomes de facto standard ABI and > > it's worth to sort it out. > > > > Rafael, Len, any other ACPI expert - I would appreciate your inputs > > and clarification > > of the above. Your recommendation would be extremely helpful. > > > > Thank you all for vivid discussions. As it may take some time for the > MDIOSerialBus _CRS macro review and approval, for now I plan to submit > v2 of_ -> fwnode_/device_ migration (patches 1-7,11/12) and skip > ACPI-specific additions until it is unblocked by spec extension. Sounds good to me (as from fwnode perspective).
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