| Message ID | 20230405-net-next-topic-net-phy-reset-v1-0-7e5329f08002@pengutronix.de |
|---|---|
| Headers | show |
| Series | Rework PHY reset handling | expand |
> +void phy_device_set_miits(struct phy_device *phydev, > + struct mii_timestamper *mii_ts) > +{ > + if (!phydev) > + return; > + > + if (phydev->mii_ts) { > + phydev_dbg(phydev, > + "MII timestamper already set -> skip set\n"); > + return; > + } > + > + phydev->mii_ts = mii_ts; > +} We tend to be less paranoid. Few, if any, other functions test that phydev is not NULL. And the current code allows overwriting of an existing stamper. If you think overwriting should not be allowed return -EINVAL, and change all the callers to test for it. > +EXPORT_SYMBOL(phy_device_set_miits); _GPL please. The code is a bit inconsistent, but new symbols should be EXPORT_SYMBOL_GPL. I do however like this patch, hiding away the internals of phydev. Andrew
> To bundle the phy firmware parsing step within phx_device.c the commit > copies the required code from fwnode_mdio.c. After we converterd all > callers of fwnode_mdiobus_* to this new API we can remove the support > from fwnode_mdio.c. Why bundle the code? Why not call it in fwnode_mdio.c? The bundling in this patch makes it harder to see the interesting part of this patch, how the reset is handled. That is what this whole patchset is about, so you want the review focus to be on that. Andrew
On Wed, Apr 05, 2023 at 11:26:56AM +0200, Marco Felsch wrote: > Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port > report 0 for the 2nd port. To fix this a driver needs to supply the > phyid instead and tell the phy framework to not try to readout the > phyid. The latter case is done via the new 'phy_id_broken' flag which > tells the phy framework to skip phyid readout for the corresponding phy. In general, we try to avoid work around for broken hardware in the core. Please try to solve this within nxp-tja11xx.c. Andrew
On 4/5/2023 5:27 AM, Andrew Lunn wrote: > On Wed, Apr 05, 2023 at 11:26:56AM +0200, Marco Felsch wrote: >> Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port >> report 0 for the 2nd port. To fix this a driver needs to supply the >> phyid instead and tell the phy framework to not try to readout the >> phyid. The latter case is done via the new 'phy_id_broken' flag which >> tells the phy framework to skip phyid readout for the corresponding phy. > > In general, we try to avoid work around for broken hardware in the > core. Please try to solve this within nxp-tja11xx.c. Agreed, and one way to solve working around broken PHY identification registers is to provide them through the compatible string via "ethernet-phyAAAA.BBBB". This forces the PHY library not to read from those registers yet instantiate the PHY device and force it to bind to a certain phy_driver.
On Wed, Apr 05, 2023 at 11:26:51AM +0200, Marco Felsch wrote: > The current phy reset handling is broken in a way that it needs > pre-running firmware to setup the phy initially. Since the very first > step is to readout the PHYID1/2 registers before doing anything else. > > The whole dection logic will fall apart if the pre-running firmware > don't setup the phy accordingly or the kernel boot resets GPIOs states > or disables clocks. In such cases the PHYID1/2 read access will fail and > so the whole detection will fail. > > I fixed this via this series, the fix will include a new kernel API > called phy_device_atomic_register() which will do all necessary things > and return a 'struct phy_device' on success. So setting up a phy and the > phy state machine is more convenient. Please add a section explaining why the current API is broken beyond repair. You need to justify adding a new call, rather than fixing the existing code to just do what is necessary to allow the PHY to be found. Andrew
On Wed, Apr 05, 2023 at 11:27:03AM +0200, Marco Felsch wrote: > There are phy's not mention any assert/deassert delay within their > datasheets but the real world showed that this is not true. They need at > least a few us to be accessible and to readout the register values. So > add a sane default value of 1000us for both assert and deassert to fix > this in a global matter. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Marco, On 4/5/2023 2:26 AM, Marco Felsch wrote: > The current phy reset handling is broken in a way that it needs > pre-running firmware to setup the phy initially. Since the very first > step is to readout the PHYID1/2 registers before doing anything else. > > The whole dection logic will fall apart if the pre-running firmware > don't setup the phy accordingly or the kernel boot resets GPIOs states > or disables clocks. In such cases the PHYID1/2 read access will fail and > so the whole detection will fail. PHY reset is a bit too broad and should need some clarifications between: - external reset to the PHY whereby a hardware pin on the PHY IC may be used - internal reset to the PHY whereby we call into the PHY driver soft_reset function to have the PHY software reset itself You are changing the way the former happens, not the latter, at least not changing the latter intentionally if at all. This is important because your cover letter will be in the merge commit in the networking tree. Will do a more thorough review on a patch by patch basis. Thanks. > > I fixed this via this series, the fix will include a new kernel API > called phy_device_atomic_register() which will do all necessary things > and return a 'struct phy_device' on success. So setting up a phy and the > phy state machine is more convenient. > > I tested the series on a i.MX8MP-EVK and a custom board which have a > TJA1102 dual-port ethernet phy. Other testers are welcome :) > > Regards, > Marco > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Marco Felsch (12): > net: phy: refactor phy_device_create function > net: phy: refactor get_phy_device function > net: phy: add phy_device_set_miits helper > net: phy: unify get_phy_device and phy_device_create parameter list > net: phy: add phy_id_broken support > net: phy: add phy_device_atomic_register helper > net: mdio: make use of phy_device_atomic_register helper > net: phy: add possibility to specify mdio device parent > net: phy: nxp-tja11xx: make use of phy_device_atomic_register() > of: mdio: remove now unused of_mdiobus_phy_device_register() > net: mdiobus: remove now unused fwnode helpers > net: phy: add default gpio assert/deassert delay > > Documentation/firmware-guide/acpi/dsd/phy.rst | 2 +- > MAINTAINERS | 1 - > drivers/net/ethernet/adi/adin1110.c | 6 +- > drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +- > drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +- > drivers/net/ethernet/socionext/netsec.c | 7 +- > drivers/net/mdio/Kconfig | 7 - > drivers/net/mdio/Makefile | 1 - > drivers/net/mdio/acpi_mdio.c | 20 +- > drivers/net/mdio/fwnode_mdio.c | 183 ------------ > drivers/net/mdio/mdio-xgene.c | 6 +- > drivers/net/mdio/of_mdio.c | 23 +- > drivers/net/phy/bcm-phy-ptp.c | 2 +- > drivers/net/phy/dp83640.c | 2 +- > drivers/net/phy/fixed_phy.c | 6 +- > drivers/net/phy/mdio_bus.c | 7 +- > drivers/net/phy/micrel.c | 2 +- > drivers/net/phy/mscc/mscc_ptp.c | 2 +- > drivers/net/phy/nxp-c45-tja11xx.c | 2 +- > drivers/net/phy/nxp-tja11xx.c | 47 ++- > drivers/net/phy/phy_device.c | 348 +++++++++++++++++++--- > drivers/net/phy/sfp.c | 7 +- > include/linux/fwnode_mdio.h | 35 --- > include/linux/of_mdio.h | 8 - > include/linux/phy.h | 46 ++- > 25 files changed, 442 insertions(+), 347 deletions(-) > --- > base-commit: 054fbf7ff8143d35ca7d3bb5414bb44ee1574194 > change-id: 20230405-net-next-topic-net-phy-reset-4f79ff7df4a0 > > Best regards,
Hi Florian, On 23-04-05, Florian Fainelli wrote: > Hi Marco, > > On 4/5/2023 2:26 AM, Marco Felsch wrote: > > The current phy reset handling is broken in a way that it needs > > pre-running firmware to setup the phy initially. Since the very first > > step is to readout the PHYID1/2 registers before doing anything else. > > > > The whole dection logic will fall apart if the pre-running firmware > > don't setup the phy accordingly or the kernel boot resets GPIOs states > > or disables clocks. In such cases the PHYID1/2 read access will fail and > > so the whole detection will fail. > > PHY reset is a bit too broad and should need some clarifications between: > > - external reset to the PHY whereby a hardware pin on the PHY IC may be used > > - internal reset to the PHY whereby we call into the PHY driver soft_reset > function to have the PHY software reset itself > > You are changing the way the former happens, not the latter, at least not > changing the latter intentionally if at all. Yes. > This is important because your cover letter will be in the merge commit in > the networking tree. Ah okay, I didn't know that. I will adapt the cover letter accordingly. > Will do a more thorough review on a patch by patch basis. Thanks. Thanks a lot, looking forward to it. Regards, Marco
On 23-04-05, Andrew Lunn wrote: > > +void phy_device_set_miits(struct phy_device *phydev, > > + struct mii_timestamper *mii_ts) > > +{ > > + if (!phydev) > > + return; > > + > > + if (phydev->mii_ts) { > > + phydev_dbg(phydev, > > + "MII timestamper already set -> skip set\n"); > > + return; > > + } > > + > > + phydev->mii_ts = mii_ts; > > +} > > We tend to be less paranoid. Few, if any, other functions test that > phydev is not NULL. And the current code allows overwriting of an > existing stamper. If you think overwriting should not be allowed > return -EINVAL, and change all the callers to test for it. I can drop the 'if (!phydev)' check if you want. Return -EINVAL is possible too. > > +EXPORT_SYMBOL(phy_device_set_miits); > > _GPL please. The code is a bit inconsistent, but new symbols should be > EXPORT_SYMBOL_GPL. Sure! Don't know why I added it as EXPORT_SYMBOL in the first place. > I do however like this patch, hiding away the internals of phydev. Thanks for the fast response. Regards, Marco > > Andrew >
On 23-04-05, Andrew Lunn wrote: > > To bundle the phy firmware parsing step within phx_device.c the commit > > copies the required code from fwnode_mdio.c. After we converterd all > > callers of fwnode_mdiobus_* to this new API we can remove the support > > from fwnode_mdio.c. > > Why bundle the code? Why not call it in fwnode_mdio.c? The current fwnode_mdio.c don't provide the proper helper functions yet. Instead the parsing is spread between fwnode_mdiobus_register_phy() and fwnode_mdiobus_phy_device_register(). Of course these can be extracted and exported but I don't see the benefit. IMHO it just cause jumping around files and since fwnode is a proper firmware abstraction we could use is directly wihin core/lib files. > The bundling in this patch makes it harder to see the interesting part > of this patch, how the reset is handled. That is what this whole > patchset is about, so you want the review focus to be on that. I know and I thought about adding the firmware parsing helpers first but than I went this way. I can split this of course to make the patch smaller. Regards, Marco > > Andrew >
On 23-04-05, Florian Fainelli wrote: > On 4/5/2023 5:27 AM, Andrew Lunn wrote: > > On Wed, Apr 05, 2023 at 11:26:56AM +0200, Marco Felsch wrote: > > > Some phy's don't report the correct phy-id, e.g. the TJA1102 dual-port > > > report 0 for the 2nd port. To fix this a driver needs to supply the > > > phyid instead and tell the phy framework to not try to readout the > > > phyid. The latter case is done via the new 'phy_id_broken' flag which > > > tells the phy framework to skip phyid readout for the corresponding phy. > > > > In general, we try to avoid work around for broken hardware in the > > core. Please try to solve this within nxp-tja11xx.c. > > Agreed, and one way to solve working around broken PHY identification > registers is to provide them through the compatible string via > "ethernet-phyAAAA.BBBB". This forces the PHY library not to read from those > registers yet instantiate the PHY device and force it to bind to a certain > phy_driver. The nxp-tja11xx.c is a bit special in case of two-port devices since the 2nd port registers a 2nd phy device which is correct but don't have a dedicated compatible and so on. My 2nd idea here was to check if phy_id is !0 and in this case just use it. I went this way to make it a bit more explicit. Regards, Marco > -- > Florian >
Hi Andrew, On 23-04-05, Andrew Lunn wrote: > On Wed, Apr 05, 2023 at 11:26:51AM +0200, Marco Felsch wrote: > > The current phy reset handling is broken in a way that it needs > > pre-running firmware to setup the phy initially. Since the very first > > step is to readout the PHYID1/2 registers before doing anything else. > > > > The whole dection logic will fall apart if the pre-running firmware > > don't setup the phy accordingly or the kernel boot resets GPIOs states > > or disables clocks. In such cases the PHYID1/2 read access will fail and > > so the whole detection will fail. > > > > I fixed this via this series, the fix will include a new kernel API > > called phy_device_atomic_register() which will do all necessary things > > and return a 'struct phy_device' on success. So setting up a phy and the > > phy state machine is more convenient. > > Please add a section explaining why the current API is broken beyond > repair. You need to justify adding a new call, rather than fixing the > existing code to just do what is necessary to allow the PHY to be > found. TIL from Florian that you use the cover-letter information in your merge commits. I will adapt the cover-letter accordingly and mention why this PR introduces a new API. Regards, Marco > > Andrew >
> The current fwnode_mdio.c don't provide the proper helper functions yet. > Instead the parsing is spread between fwnode_mdiobus_register_phy() and > fwnode_mdiobus_phy_device_register(). Of course these can be extracted > and exported but I don't see the benefit. IMHO it just cause jumping > around files and since fwnode is a proper firmware abstraction we could > use is directly wihin core/lib files. No, assuming fwnode is the proper firmware abstraction is wrong. You need to be very careful any time you convert of_ to fwnode_ and look at the history of every property. Look at the number of deprecated OF properties in Documentation/devicetree/bindings. They should never be moved to fwnode_ because then you are moving deprecated properties to ACPI, which never had them in the first place! You cannot assume DT and ACPI are the same thing, have the same binding. And the same is true, in theory, in the opposite direction. We don't want the DT properties polluted with ACPI only properties. Not that anybody takes ACPI seriously in networking. > I know and I thought about adding the firmware parsing helpers first but > than I went this way. I can split this of course to make the patch > smaller. Please do. Also, i read your commit message thinking it was a straight copy of the code, and hence i did not need to review the code. But in fact it is new code. So i need to take a close look at it. But what i think is most important for this patchset is the justification for not fixing the current API. Why is it broken beyond repair? Andrew
> The nxp-tja11xx.c is a bit special in case of two-port devices since the > 2nd port registers a 2nd phy device which is correct but don't have a > dedicated compatible and so on. My 2nd idea here was to check if phy_id > is !0 and in this case just use it. I went this way to make it a bit > more explicit. What is actually wrong with the current solution? It is nicely hidden away in the driver, where workarounds for broken hardware should be. If you have found device 1 of 2, does that not suggest its resets are already in a good state and nothing needs to be done for the second PHY? So just register the second PHY with the code from within the driver. Andrew
On 23-04-05, Andrew Lunn wrote: > > The current fwnode_mdio.c don't provide the proper helper functions yet. > > Instead the parsing is spread between fwnode_mdiobus_register_phy() and > > fwnode_mdiobus_phy_device_register(). Of course these can be extracted > > and exported but I don't see the benefit. IMHO it just cause jumping > > around files and since fwnode is a proper firmware abstraction we could > > use is directly wihin core/lib files. > > No, assuming fwnode is the proper firmware abstraction is wrong. You > need to be very careful any time you convert of_ to fwnode_ and look > at the history of every property. Look at the number of deprecated OF > properties in Documentation/devicetree/bindings. They should never be > moved to fwnode_ because then you are moving deprecated properties to > ACPI, which never had them in the first place! The handling of deprecated properties is always a pain. Drivers handling deprecated properties correctly for of_ should handle it correctly for fwnode_ too. IMHO it would be driver bug if not existing deprecated properties cause an error. Of course there will be properties which need special attention for ACPI case but I don't see a problem for deprecated properties since those must be handled correctly for of_ case too. > You cannot assume DT and ACPI are the same thing, have the same > binding. And the same is true, in theory, in the opposite direction. > We don't want the DT properties polluted with ACPI only properties. > Not that anybody takes ACPI seriously in networking. My assumption was that ACPI is becoming more closer to OF and the fwnode/device abstraction is the abstraction to have one driver interacting correctly with OF and ACPI. As I said above there will be some corner-cases which need special attention of course :/ Also while answering this mail, I noticed that there are already some 'small' fwnode/device_ helpers within phy_device.c. So why not bundling everything within phy_device.c? > > I know and I thought about adding the firmware parsing helpers first but > > than I went this way. I can split this of course to make the patch > > smaller. > > Please do. Also, i read your commit message thinking it was a straight > copy of the code, and hence i did not need to review the code. But in > fact it is new code. So i need to take a close look at it. > > But what i think is most important for this patchset is the > justification for not fixing the current API. Why is it broken beyond > repair? Currently we have one API which creates/allocates the 'struct phy_device' and intialize the state which is: - phy_device_create() This function requests a driver based on the phy_id/c45_ids. The ID have to come from somewhere if autodection is used. For autodetection case - get_phy_device() is called. This function try to access the phy without taken possible hardware dependencies into account. These dependecies can be reset-lines (in my case), clocks, supplies, ... For taking fwnode (and possible dependencies) into account fwnode_mdio.c was written which provides two helpers: - fwnode_mdiobus_register_phy() - fwnode_mdiobus_phy_device_register(). The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around fwnode_mdiobus_register_phy(). fwnode_mdiobus_register_phy(): 1st) calls get_phy_device() in case of autodection or c45. If phy_id is provided and !c45 case phy_device_create() is called to get a 'struct phy_device' - The autodection/c45 case try to access the PHYID registers which is not possible, please see above. 2nd) call fwnode_mdiobus_phy_device_register() or phy_device_register() directly. - phy_device_register() is the first time we taking the possible hardware reset line into account, which is far to late. fwnode_mdiobus_phy_device_register(): - takes a 'struct phy_device' as parameter, again this have to come from somewhere. - calls phy_device_register() which is taken the possibel hardware reset into account, again to late. Why do I need the autodection? Because PHYs can be changed due to EOL, cheaper device, ... I don't wanna have a new devicetree/firmware for the updated product, just let the magic happen :) Why do I introduce a new API? 1st) There are working users of get_phy_device() and I don't wanna break their systems, so I left this logic untouched. 2nd) The fwnode API is replaced by this new one, since it is broken (please see above). 3rd) IMHO the 'phy request/phy create' handling is far to complex therefore I introduced a single API which: - intialize all structures and states - prepare the device for interaction by using fwnode - initialize/detect the device and requests the coorect phy module - applies the fixups - add the device to the kernel - finally return the 'struct phy_device' to the user, so the driver can do $stuff. 4th) The new 'struct phy_device_config' makes it easier to adapt/extend the API. Thanks a lot for your fast response and feedback :) Regards, Marco
> Currently we have one API which creates/allocates the 'struct > phy_device' and intialize the state which is: > - phy_device_create() > > This function requests a driver based on the phy_id/c45_ids. The ID have > to come from somewhere if autodection is used. For autodetection case > - get_phy_device() > > is called. This function try to access the phy without taken possible > hardware dependencies into account. These dependecies can be reset-lines > (in my case), clocks, supplies, ... > > For taking fwnode (and possible dependencies) into account fwnode_mdio.c > was written which provides two helpers: > - fwnode_mdiobus_register_phy() > - fwnode_mdiobus_phy_device_register(). > > The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around > fwnode_mdiobus_register_phy(). It seems to me that the real problem is that mdio_device_reset() takes an mdio_device. mdiobus_register_gpiod() and mdiobus_register_reset() also take an mdio_device. These are the functions you want to call before calling of_mdiobus_register_phy() in __of_mdiobus_register() to ensure the PHY is out of reset. But you don't have an mdio_device yet. So i think a better solution is to refactor this code. Move the resources into a structure of their own, and make that a member of mdio_device. You can create a stack version of this resource structure in __of_mdiobus_register(), parse DT to fill it out by calling mdiobus_register_gpiod() and mdiobus_register_reset() taking this new structure, take it out of reset by calling mdio_device_reset(), and then call of_mdiobus_register_phy(). If a PHY is found, copy the values in the resulting mdio_device. If not, release the resources. Doing it like this means there is no API change. Andrew
On 23-04-05, Andrew Lunn wrote: > > Currently we have one API which creates/allocates the 'struct > > phy_device' and intialize the state which is: > > - phy_device_create() > > > > This function requests a driver based on the phy_id/c45_ids. The ID have > > to come from somewhere if autodection is used. For autodetection case > > - get_phy_device() > > > > is called. This function try to access the phy without taken possible > > hardware dependencies into account. These dependecies can be reset-lines > > (in my case), clocks, supplies, ... > > > > For taking fwnode (and possible dependencies) into account fwnode_mdio.c > > was written which provides two helpers: > > - fwnode_mdiobus_register_phy() > > - fwnode_mdiobus_phy_device_register(). > > > > The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around > > fwnode_mdiobus_register_phy(). > > It seems to me that the real problem is that mdio_device_reset() takes > an mdio_device. mdiobus_register_gpiod() and mdiobus_register_reset() > also take an mdio_device. These are the functions you want to call > before calling of_mdiobus_register_phy() in __of_mdiobus_register() to > ensure the PHY is out of reset. But you don't have an mdio_device yet. Of course, this was the problem as well and therefore I did the split in the first two patches, to differentiate between allocation and init. > So i think a better solution is to refactor this code. Move the > resources into a structure of their own, and make that a member of > mdio_device. Sorry I can't follow you here, I did the refactoring already to differentiate between phy_device_alloc() and phy_device_init(). The parse and registration happen in between, like you descriped below. I didn't changed/touched the mdio_device and phy_device structs since those structs are very open and can be adapted by every driver. > You can create a stack version of this resource structure > in __of_mdiobus_register(), parse DT to fill it out by calling > mdiobus_register_gpiod() and mdiobus_register_reset() taking this new > structure, take it out of reset by calling mdio_device_reset(), and > then call of_mdiobus_register_phy(). If a PHY is found, copy the > values in the resulting mdio_device. If not, release the resources. It is not just the reset, my approach would be the ground work for adding support of other resources to, which are not handled yet. e.g. phy-supply, clocks, pwdn-lines, ... With my approach it is far easier of adding this > Doing it like this means there is no API change. Why is it that important? All users of the current fwnode API are changed and even better, they are replaced in a 2:1 ratio. The new API is the repaired version of the fwnode API which is already used by ACPI and OF to register a phy_device. For all non-ACPI/OF users the new API provides a way to allocate/identify and register a new phy within a single call. Regards, Marco
+Raju On 4/5/2023 2:56 PM, Marco Felsch wrote: > Currently these two public APIs are used to create a 'struct phy_device' > device. In addition to that the device get_phy_device() can adapt the > is_c45 parameter as well if supprted by the mii bus. > > Both APIs do have a different set of parameters which can be unified to > upcoming API extension. For this the 'struct phy_device_config' is > introduced which is the only parameter for both functions. This new > mechnism also adds the possiblity to read the configuration back within > the driver for possible validation checks. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> For the change on AMD XGBE driver. Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/net/ethernet/adi/adin1110.c | 6 ++- > drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +++- > drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +++-- > drivers/net/ethernet/socionext/netsec.c | 7 ++- > drivers/net/mdio/fwnode_mdio.c | 13 +++--- > drivers/net/mdio/mdio-xgene.c | 6 ++- > drivers/net/phy/fixed_phy.c | 6 ++- > drivers/net/phy/mdio_bus.c | 7 ++- > drivers/net/phy/nxp-tja11xx.c | 23 +++++----- > drivers/net/phy/phy_device.c | 55 ++++++++++++----------- > drivers/net/phy/sfp.c | 7 ++- > include/linux/phy.h | 29 +++++++++--- > 12 files changed, 121 insertions(+), 57 deletions(-) > > diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c > index 3f316a0f4158..01b0c2a3a294 100644 > --- a/drivers/net/ethernet/adi/adin1110.c > +++ b/drivers/net/ethernet/adi/adin1110.c > @@ -1565,6 +1565,9 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv) > { > struct device *dev = &priv->spidev->dev; > struct adin1110_port_priv *port_priv; > + struct phy_device_config config = { > + .mii_bus = priv->mii_bus, > + }; > struct net_device *netdev; > int ret; > int i; > @@ -1599,7 +1602,8 @@ static int adin1110_probe_netdevs(struct adin1110_priv *priv) > netdev->priv_flags |= IFF_UNICAST_FLT; > netdev->features |= NETIF_F_NETNS_LOCAL; > > - port_priv->phydev = get_phy_device(priv->mii_bus, i + 1, false); > + config.phy_addr = i + 1; > + port_priv->phydev = get_phy_device(&config); > if (IS_ERR(port_priv->phydev)) { > netdev_err(netdev, "Could not find PHY with device address: %d.\n", i); > return PTR_ERR(port_priv->phydev); > diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c > index 16e7fb2c0dae..27ba903bbd0a 100644 > --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c > +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c > @@ -1056,6 +1056,10 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata) > { > struct ethtool_link_ksettings *lks = &pdata->phy.lks; > struct xgbe_phy_data *phy_data = pdata->phy_data; > + struct phy_device_config config = { > + .mii_bus = phy_data->mii, > + .phy_addr = phy_data->mdio_addr, > + }; > struct phy_device *phydev; > int ret; > > @@ -1086,8 +1090,8 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata) > } > > /* Create and connect to the PHY device */ > - phydev = get_phy_device(phy_data->mii, phy_data->mdio_addr, > - (phy_data->phydev_mode == XGBE_MDIO_MODE_CL45)); > + config.is_c45 = phy_data->phydev_mode == XGBE_MDIO_MODE_CL45; > + phydev = get_phy_device(&config); > if (IS_ERR(phydev)) { > netdev_err(pdata->netdev, "get_phy_device failed\n"); > return -ENODEV; > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c > index 928d934cb21a..3c41c4db5d9b 100644 > --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c > +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c > @@ -687,9 +687,12 @@ static int > hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb, > u32 addr) > { > + struct phy_device_config config = { > + .mii_bus = mdio, > + .phy_addr = addr, > + }; > struct phy_device *phy; > const char *phy_type; > - bool is_c45; > int rc; > > rc = fwnode_property_read_string(mac_cb->fw_port, > @@ -698,13 +701,13 @@ hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb, > return rc; > > if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_XGMII))) > - is_c45 = true; > + config.is_c45 = true; > else if (!strcmp(phy_type, phy_modes(PHY_INTERFACE_MODE_SGMII))) > - is_c45 = false; > + config.is_c45 = false; > else > return -ENODATA; > > - phy = get_phy_device(mdio, addr, is_c45); > + phy = get_phy_device(&config); > if (!phy || IS_ERR(phy)) > return -EIO; > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > index 2d7347b71c41..a0786dfb827a 100644 > --- a/drivers/net/ethernet/socionext/netsec.c > +++ b/drivers/net/ethernet/socionext/netsec.c > @@ -1948,6 +1948,11 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr) > return ret; > } > } else { > + struct phy_device_config config = { > + .mii_bus = bus, > + .phy_addr = phy_addr, > + }; > + > /* Mask out all PHYs from auto probing. */ > bus->phy_mask = ~0; > ret = mdiobus_register(bus); > @@ -1956,7 +1961,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr) > return ret; > } > > - priv->phydev = get_phy_device(bus, phy_addr, false); > + priv->phydev = get_phy_device(&config); > if (IS_ERR(priv->phydev)) { > ret = PTR_ERR(priv->phydev); > dev_err(priv->dev, "get_phy_device err(%d)\n", ret); > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c > index 1183ef5e203e..47ef702d4ffd 100644 > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -112,10 +112,13 @@ EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register); > int fwnode_mdiobus_register_phy(struct mii_bus *bus, > struct fwnode_handle *child, u32 addr) > { > + struct phy_device_config config = { > + .mii_bus = bus, > + .phy_addr = addr, > + }; > struct mii_timestamper *mii_ts = NULL; > struct pse_control *psec = NULL; > struct phy_device *phy; > - bool is_c45; > u32 phy_id; > int rc; > > @@ -129,11 +132,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, > goto clean_pse; > } > > - is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); > - if (is_c45 || fwnode_get_phy_id(child, &phy_id)) > - phy = get_phy_device(bus, addr, is_c45); > + config.is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); > + if (config.is_c45 || fwnode_get_phy_id(child, &config.phy_id)) > + phy = get_phy_device(&config); > else > - phy = phy_device_create(bus, addr, phy_id, 0, NULL); > + phy = phy_device_create(&config); > if (IS_ERR(phy)) { > rc = PTR_ERR(phy); > goto clean_mii_ts; > diff --git a/drivers/net/mdio/mdio-xgene.c b/drivers/net/mdio/mdio-xgene.c > index 7aafc221b5cf..6754c35aa6c3 100644 > --- a/drivers/net/mdio/mdio-xgene.c > +++ b/drivers/net/mdio/mdio-xgene.c > @@ -262,9 +262,13 @@ static int xgene_xfi_mdio_read(struct mii_bus *bus, int phy_id, int reg) > > struct phy_device *xgene_enet_phy_register(struct mii_bus *bus, int phy_addr) > { > + struct phy_device_config config = { > + .mii_bus = bus, > + .phy_addr = phy_addr, > + }; > struct phy_device *phy_dev; > > - phy_dev = get_phy_device(bus, phy_addr, false); > + phy_dev = get_phy_device(&config); > if (!phy_dev || IS_ERR(phy_dev)) > return NULL; > > diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c > index aef739c20ac4..d217301a71d1 100644 > --- a/drivers/net/phy/fixed_phy.c > +++ b/drivers/net/phy/fixed_phy.c > @@ -229,6 +229,9 @@ static struct phy_device *__fixed_phy_register(unsigned int irq, > struct gpio_desc *gpiod) > { > struct fixed_mdio_bus *fmb = &platform_fmb; > + struct phy_device_config config = { > + .mii_bus = fmb->mii_bus, > + }; > struct phy_device *phy; > int phy_addr; > int ret; > @@ -254,7 +257,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq, > return ERR_PTR(ret); > } > > - phy = get_phy_device(fmb->mii_bus, phy_addr, false); > + config.phy_addr = phy_addr; > + phy = get_phy_device(&config); > if (IS_ERR(phy)) { > fixed_phy_del(phy_addr); > return ERR_PTR(-EINVAL); > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 389f33a12534..8390db7ae4bc 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -516,9 +516,14 @@ static int mdiobus_create_device(struct mii_bus *bus, > static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45) > { > struct phy_device *phydev = ERR_PTR(-ENODEV); > + struct phy_device_config config = { > + .mii_bus = bus, > + .phy_addr = addr, > + .is_c45 = c45, > + }; > int err; > > - phydev = get_phy_device(bus, addr, c45); > + phydev = get_phy_device(&config); > if (IS_ERR(phydev)) > return phydev; > > diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c > index ec91e671f8aa..b5e03d66b7f5 100644 > --- a/drivers/net/phy/nxp-tja11xx.c > +++ b/drivers/net/phy/nxp-tja11xx.c > @@ -554,17 +554,20 @@ static void tja1102_p1_register(struct work_struct *work) > struct device *dev = &phydev_phy0->mdio.dev; > struct device_node *np = dev->of_node; > struct device_node *child; > - int ret; > > for_each_available_child_of_node(np, child) { > + struct phy_device_config config = { > + .mii_bus = bus, > + /* Real PHY ID of Port 1 is 0 */ > + .phy_id = PHY_ID_TJA1102, > + }; > struct phy_device *phy; > - int addr; > > - addr = of_mdio_parse_addr(dev, child); > - if (addr < 0) { > + config.phy_addr = of_mdio_parse_addr(dev, child); > + if (config.phy_addr < 0) { > dev_err(dev, "Can't parse addr\n"); > continue; > - } else if (addr != phydev_phy0->mdio.addr + 1) { > + } else if (config.phy_addr != phydev_phy0->mdio.addr + 1) { > /* Currently we care only about double PHY chip TJA1102. > * If some day NXP will decide to bring chips with more > * PHYs, this logic should be reworked. > @@ -574,16 +577,15 @@ static void tja1102_p1_register(struct work_struct *work) > continue; > } > > - if (mdiobus_is_registered_device(bus, addr)) { > + if (mdiobus_is_registered_device(bus, config.phy_addr)) { > dev_err(dev, "device is already registered\n"); > continue; > } > > - /* Real PHY ID of Port 1 is 0 */ > - phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL); > + phy = phy_device_create(&config); > if (IS_ERR(phy)) { > dev_err(dev, "Can't create PHY device for Port 1: %i\n", > - addr); > + config.phy_addr); > continue; > } > > @@ -592,7 +594,8 @@ static void tja1102_p1_register(struct work_struct *work) > */ > phy->mdio.dev.parent = dev; > > - ret = of_mdiobus_phy_device_register(bus, phy, child, addr); > + ret = of_mdiobus_phy_device_register(bus, phy, child, > + config.phy_addr); > if (ret) { > /* All resources needed for Port 1 should be already > * available for Port 0. Both ports use the same > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index e4d08dcfed70..bb465a324eef 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -629,8 +629,10 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id) > return 0; > } > > -static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr) > +static struct phy_device *phy_device_alloc(struct phy_device_config *config) > { > + struct mii_bus *bus = config->mii_bus; > + int addr = config->phy_addr; > struct phy_device *dev; > struct mdio_device *mdiodev; > > @@ -656,9 +658,15 @@ static struct phy_device *phy_device_alloc(struct mii_bus *bus, int addr) > return dev; > } > > -static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45, > - struct phy_c45_device_ids *c45_ids) > +static int phy_device_init(struct phy_device *dev, > + struct phy_device_config *config) > { > + struct phy_c45_device_ids *c45_ids = &config->c45_ids; > + struct mii_bus *bus = config->mii_bus; > + bool is_c45 = config->is_c45; > + u32 phy_id = config->phy_id; > + int addr = config->phy_addr; > + > dev->speed = SPEED_UNKNOWN; > dev->duplex = DUPLEX_UNKNOWN; > dev->pause = 0; > @@ -711,18 +719,16 @@ static int phy_device_init(struct phy_device *dev, u32 phy_id, bool is_c45, > return phy_request_driver_module(dev, phy_id); > } > > -struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, > - bool is_c45, > - struct phy_c45_device_ids *c45_ids) > +struct phy_device *phy_device_create(struct phy_device_config *config) > { > struct phy_device *dev; > int ret; > > - dev = phy_device_alloc(bus, addr); > + dev = phy_device_alloc(config); > if (IS_ERR(dev)) > return dev; > > - ret = phy_device_init(dev, phy_id, is_c45, c45_ids); > + ret = phy_device_init(dev, config); > if (ret) { > put_device(&dev->mdio.dev); > dev = ERR_PTR(ret); > @@ -954,12 +960,16 @@ int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id) > } > EXPORT_SYMBOL(fwnode_get_phy_id); > > -static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45, > - u32 *phy_id, struct phy_c45_device_ids *c45_ids) > +static int phy_device_detect(struct phy_device_config *config) > { > + struct phy_c45_device_ids *c45_ids = &config->c45_ids; > + struct mii_bus *bus = config->mii_bus; > + u32 *phy_id = &config->phy_id; > + bool is_c45 = config->is_c45; > + int addr = config->phy_addr; > int r; > > - if (*is_c45) > + if (is_c45) > r = get_phy_c45_ids(bus, addr, c45_ids); > else > r = get_phy_c22_id(bus, addr, phy_id); > @@ -972,8 +982,8 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45, > * probe with C45 to see if we're able to get a valid PHY ID in the C45 > * space, if successful, create the C45 PHY device. > */ > - if (!*is_c45 && *phy_id == 0 && bus->read_c45) { > - *is_c45 = true; > + if (!is_c45 && *phy_id == 0 && bus->read_c45) { > + config->is_c45 = true; > return get_phy_c45_ids(bus, addr, c45_ids); > } > > @@ -983,11 +993,9 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45, > /** > * get_phy_device - reads the specified PHY device and returns its @phy_device > * struct > - * @bus: the target MII bus > - * @addr: PHY address on the MII bus > - * @is_c45: If true the PHY uses the 802.3 clause 45 protocol > + * @config: The PHY device config > * > - * Probe for a PHY at @addr on @bus. > + * Use the @config parameters to probe for a PHY. > * > * When probing for a clause 22 PHY, then read the ID registers. If we find > * a valid ID, allocate and return a &struct phy_device. > @@ -999,21 +1007,18 @@ static int phy_device_detect(struct mii_bus *bus, int addr, bool *is_c45, > * Returns an allocated &struct phy_device on success, %-ENODEV if there is > * no PHY present, or %-EIO on bus access error. > */ > -struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) > +struct phy_device *get_phy_device(struct phy_device_config *config) > { > - struct phy_c45_device_ids c45_ids; > - u32 phy_id = 0; > + struct phy_c45_device_ids *c45_ids = &config->c45_ids; > int r; > > - c45_ids.devices_in_package = 0; > - c45_ids.mmds_present = 0; > - memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids)); > + memset(c45_ids->device_ids, 0xff, sizeof(c45_ids->device_ids)); > > - r = phy_device_detect(bus, addr, &is_c45, &phy_id, &c45_ids); > + r = phy_device_detect(config); > if (r) > return ERR_PTR(r); > > - return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids); > + return phy_device_create(config); > } > EXPORT_SYMBOL(get_phy_device); > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index f0fcb06fbe82..8fb0a1a49aaf 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -1635,10 +1635,15 @@ static void sfp_sm_phy_detach(struct sfp *sfp) > > static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45) > { > + struct phy_device_config config = { > + .mii_bus = sfp->i2c_mii, > + .phy_addr = addr, > + .is_c45 = is_c45, > + }; > struct phy_device *phy; > int err; > > - phy = get_phy_device(sfp->i2c_mii, addr, is_c45); > + phy = get_phy_device(&config); > if (phy == ERR_PTR(-ENODEV)) > return PTR_ERR(phy); > if (IS_ERR(phy)) { > diff --git a/include/linux/phy.h b/include/linux/phy.h > index c17a98712555..498f4dc7669d 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -756,6 +756,27 @@ static inline struct phy_device *to_phy_device(const struct device *dev) > return container_of(to_mdio_device(dev), struct phy_device, mdio); > } > > +/** > + * struct phy_device_config - Configuration of a PHY > + * > + * @mii_bus: The target MII bus the PHY is connected to > + * @phy_addr: PHY address on the MII bus > + * @phy_id: UID for this device found during discovery > + * @c45_ids: 802.3-c45 Device Identifiers if is_c45. > + * @is_c45: If true the PHY uses the 802.3 clause 45 protocol > + * > + * The struct contain possible configuration parameters for a PHY device which > + * are used to setup the struct phy_device. > + */ > + > +struct phy_device_config { > + struct mii_bus *mii_bus; > + int phy_addr; > + u32 phy_id; > + struct phy_c45_device_ids c45_ids; > + bool is_c45; > +}; > + > /** > * struct phy_tdr_config - Configuration of a TDR raw test > * > @@ -1538,9 +1559,7 @@ int phy_modify_paged_changed(struct phy_device *phydev, int page, u32 regnum, > int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum, > u16 mask, u16 set); > > -struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, > - bool is_c45, > - struct phy_c45_device_ids *c45_ids); > +struct phy_device *phy_device_create(struct phy_device_config *config); > void phy_device_set_miits(struct phy_device *phydev, > struct mii_timestamper *mii_ts); > #if IS_ENABLED(CONFIG_PHYLIB) > @@ -1549,7 +1568,7 @@ struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode); > struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode); > struct phy_device *device_phy_find_device(struct device *dev); > struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode); > -struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45); > +struct phy_device *get_phy_device(struct phy_device_config *config); > int phy_device_register(struct phy_device *phy); > void phy_device_free(struct phy_device *phydev); > #else > @@ -1581,7 +1600,7 @@ struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode) > } > > static inline > -struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) > +struct phy_device *get_phy_device(struct phy_device_config *config) > { > return NULL; > } >
Lets try again....
There are a number of things i don't like about this patchset.
It does too many different things.
It pulls workarounds into the core.
I don't like the phy_device_config. It would make sense if there were
more than 6 arguments to pass to a function, but not for less.
I don't like the name phy_device_atomic_register(), but that is bike
shedding.
There is no really strong argument to change the API.
There is no really strong argument to move to fwnode.
The problem you are trying to solve is to call phy_device_reset()
earlier, before reading the ID registers. Please produce a patchset
which is only focused on that. Nothing else.
Andrew
The current phy reset handling is broken in a way that it needs pre-running firmware to setup the phy initially. Since the very first step is to readout the PHYID1/2 registers before doing anything else. The whole dection logic will fall apart if the pre-running firmware don't setup the phy accordingly or the kernel boot resets GPIOs states or disables clocks. In such cases the PHYID1/2 read access will fail and so the whole detection will fail. I fixed this via this series, the fix will include a new kernel API called phy_device_atomic_register() which will do all necessary things and return a 'struct phy_device' on success. So setting up a phy and the phy state machine is more convenient. I tested the series on a i.MX8MP-EVK and a custom board which have a TJA1102 dual-port ethernet phy. Other testers are welcome :) Regards, Marco Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Marco Felsch (12): net: phy: refactor phy_device_create function net: phy: refactor get_phy_device function net: phy: add phy_device_set_miits helper net: phy: unify get_phy_device and phy_device_create parameter list net: phy: add phy_id_broken support net: phy: add phy_device_atomic_register helper net: mdio: make use of phy_device_atomic_register helper net: phy: add possibility to specify mdio device parent net: phy: nxp-tja11xx: make use of phy_device_atomic_register() of: mdio: remove now unused of_mdiobus_phy_device_register() net: mdiobus: remove now unused fwnode helpers net: phy: add default gpio assert/deassert delay Documentation/firmware-guide/acpi/dsd/phy.rst | 2 +- MAINTAINERS | 1 - drivers/net/ethernet/adi/adin1110.c | 6 +- drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 8 +- drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 11 +- drivers/net/ethernet/socionext/netsec.c | 7 +- drivers/net/mdio/Kconfig | 7 - drivers/net/mdio/Makefile | 1 - drivers/net/mdio/acpi_mdio.c | 20 +- drivers/net/mdio/fwnode_mdio.c | 183 ------------ drivers/net/mdio/mdio-xgene.c | 6 +- drivers/net/mdio/of_mdio.c | 23 +- drivers/net/phy/bcm-phy-ptp.c | 2 +- drivers/net/phy/dp83640.c | 2 +- drivers/net/phy/fixed_phy.c | 6 +- drivers/net/phy/mdio_bus.c | 7 +- drivers/net/phy/micrel.c | 2 +- drivers/net/phy/mscc/mscc_ptp.c | 2 +- drivers/net/phy/nxp-c45-tja11xx.c | 2 +- drivers/net/phy/nxp-tja11xx.c | 47 ++- drivers/net/phy/phy_device.c | 348 +++++++++++++++++++--- drivers/net/phy/sfp.c | 7 +- include/linux/fwnode_mdio.h | 35 --- include/linux/of_mdio.h | 8 - include/linux/phy.h | 46 ++- 25 files changed, 442 insertions(+), 347 deletions(-) --- base-commit: 054fbf7ff8143d35ca7d3bb5414bb44ee1574194 change-id: 20230405-net-next-topic-net-phy-reset-4f79ff7df4a0 Best regards,