| Message ID | 20230405-net-next-topic-net-phy-reset-v1-6-7e5329f08002@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Rework PHY reset handling | expand |
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 >
> 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
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
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
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 7e4b3b3caba9..a784ac06e6a9 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -3124,6 +3124,214 @@ struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fwnode_get_phy_node); +static int fwnode_setup_phy_irq(struct phy_device *phydev, struct mii_bus *bus, + struct fwnode_handle *fwnode) +{ + u32 addr = phydev->mdio.addr; + int ret; + + if (is_acpi_node(fwnode)) { + phydev->irq = bus->irq[addr]; + return 0; + } + + /* of_node */ + ret = fwnode_irq_get(fwnode, 0); + /* Don't wait forever if the IRQ provider doesn't become available, + * just fall back to poll mode + */ + if (ret == -EPROBE_DEFER) + ret = driver_deferred_probe_check_state(&phydev->mdio.dev); + if (ret == -EPROBE_DEFER) + return ret; + + if (ret > 0) { + phydev->irq = ret; + bus->irq[addr] = ret; + } else { + phydev->irq = bus->irq[addr]; + } + + return 0; +} + +static struct pse_control * +fwnode_find_pse_control(struct fwnode_handle *fwnode) +{ + struct pse_control *psec; + struct device_node *np; + + if (!IS_ENABLED(CONFIG_PSE_CONTROLLER)) + return NULL; + + np = to_of_node(fwnode); + if (!np) + return NULL; + + psec = of_pse_control_get(np); + if (PTR_ERR(psec) == -ENOENT) + return NULL; + + return psec; +} + +static struct mii_timestamper * +fwnode_find_mii_timestamper(struct fwnode_handle *fwnode) +{ + struct of_phandle_args arg; + int err; + + if (is_acpi_node(fwnode)) + return NULL; + + err = of_parse_phandle_with_fixed_args(to_of_node(fwnode), + "timestamper", 1, 0, &arg); + if (err == -ENOENT) + return NULL; + else if (err) + return ERR_PTR(err); + + if (arg.args_count != 1) + return ERR_PTR(-EINVAL); + + return register_mii_timestamper(arg.np, arg.args[0]); +} + +static int +phy_device_parse_fwnode(struct phy_device *phydev, + struct phy_device_config *config) +{ + struct fwnode_handle *fwnode = config->fwnode; + struct mii_bus *bus = config->mii_bus; + u32 addr = phydev->mdio.addr; + int ret; + + if (!fwnode) + return 0; + + if (!is_acpi_node(fwnode) && !is_of_node(fwnode)) + return 0; + + ret = fwnode_setup_phy_irq(phydev, bus, fwnode); + if (ret) + return ret; + + ret = fwnode_property_match_string(fwnode, "compatible", + "ethernet-phy-ieee802.3-c45"); + if (ret >= 0) + config->is_c45 = true; + + if (fwnode_property_read_bool(fwnode, "broken-turn-around")) + bus->phy_ignore_ta_mask |= 1 << addr; + fwnode_property_read_u32(fwnode, "reset-assert-us", + &phydev->mdio.reset_assert_delay); + fwnode_property_read_u32(fwnode, "reset-deassert-us", + &phydev->mdio.reset_deassert_delay); + + fwnode_handle_get(fwnode); + if (is_acpi_node(fwnode)) + phydev->mdio.dev.fwnode = fwnode; + else if (is_of_node(fwnode)) + device_set_node(&phydev->mdio.dev, fwnode); + + phydev->psec = fwnode_find_pse_control(fwnode); + if (IS_ERR(phydev->psec)) { + ret = PTR_ERR(phydev->psec); + goto put_fwnode; + } + + /* A mii_timestamper probed via the device tree will have precedence. */ + phydev->mii_ts = fwnode_find_mii_timestamper(fwnode); + if (IS_ERR(phydev->mii_ts)) { + ret = PTR_ERR(phydev->mii_ts); + goto put_pse; + } + + return 0; + +put_pse: + pse_control_put(phydev->psec); +put_fwnode: + fwnode_handle_put(phydev->mdio.dev.fwnode); + + return ret; +} + +/** + * phy_device_atomic_register - Setup, init and register a PHY on the MDIO bus + * @config: The PHY config + * + * Probe, initialise and register a PHY at @addr on @bus. + * + * Returns an allocated and registered &struct phy_device on success. + */ +struct phy_device *phy_device_atomic_register(struct phy_device_config *config) +{ + struct phy_c45_device_ids *c45_ids = &config->c45_ids; + struct phy_device *phydev; + int err; + + phydev = phy_device_alloc(config); + if (IS_ERR(phydev)) + return ERR_CAST(phydev); + + err = phy_device_parse_fwnode(phydev, config); + if (err) { + phydev_err(phydev, "failed to parse fwnode\n"); + goto err_free_phydev; + } + + err = mdiobus_register_device(&phydev->mdio); + if (err) { + phydev_err(phydev, "pre-init step failed\n"); + goto err_free_fwnode; + } + + phy_device_reset(phydev, 0); + + memset(c45_ids->device_ids, 0xff, sizeof(c45_ids->device_ids)); + + err = phy_device_detect(config); + if (err) { + phydev_err(phydev, "failed to query the phyid\n"); + goto err_unregister_mdiodev; + } + + err = phy_device_init(phydev, config); + if (err) { + phydev_err(phydev, "failed to initialize\n"); + goto err_unregister_mdiodev; + } + + err = phy_scan_fixups(phydev); + if (err) { + phydev_err(phydev, "failed to apply fixups\n"); + goto err_unregister_mdiodev; + } + + err = device_add(&phydev->mdio.dev); + if (err) { + phydev_err(phydev, "failed to add\n"); + goto err_out; + } + + return 0; + +err_out: + phy_device_reset(phydev, 1); +err_unregister_mdiodev: + mdiobus_unregister_device(&phydev->mdio); +err_free_fwnode: + unregister_mii_timestamper(phydev->mii_ts); + pse_control_put(phydev->psec); + fwnode_handle_put(phydev->mdio.dev.fwnode); +err_free_phydev: + kfree(phydev); + + return ERR_PTR(err); +} +EXPORT_SYMBOL(phy_device_atomic_register); + /** * phy_probe - probe and init a PHY device * @dev: device to probe and init diff --git a/include/linux/phy.h b/include/linux/phy.h index 0f0cb72a08ab..bdf6d27faefb 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -761,6 +761,7 @@ static inline struct phy_device *to_phy_device(const struct device *dev) * * @mii_bus: The target MII bus the PHY is connected to * @phy_addr: PHY address on the MII bus + * @fwnode: The PHY firmware handle * @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 @@ -774,6 +775,7 @@ static inline struct phy_device *to_phy_device(const struct device *dev) struct phy_device_config { struct mii_bus *mii_bus; int phy_addr; + struct fwnode_handle *fwnode; u32 phy_id; struct phy_c45_device_ids c45_ids; bool is_c45; @@ -1573,6 +1575,7 @@ 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 phy_device_config *config); int phy_device_register(struct phy_device *phy); +struct phy_device *phy_device_atomic_register(struct phy_device_config *config); void phy_device_free(struct phy_device *phydev); #else static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id) @@ -1613,6 +1616,12 @@ static inline int phy_device_register(struct phy_device *phy) return 0; } +static inline +struct phy_device *phy_device_atomic_register(struct phy_device_config *config) +{ + return NULL; +} + static inline void phy_device_free(struct phy_device *phydev) { } #endif /* CONFIG_PHYLIB */ void phy_device_remove(struct phy_device *phydev);
Currently the usually way to probe and setup a phy is done via: 1) get_phy_device()/phy_device_create() 2) phy_device_register. During get_phy_device() the PHYID1/2 registers are read which assumes that the phy is already accessible. This is not always the case, e.g. - if the pre-running firmware did not initialize the phy or - if the kernel does gate important clocks while booting and the phy isn't accessible after the pre-running firmware anymore. To fix this we need to: - parse the phy's fwnode first, - do some basic setup like: bring it out of the reset state and - finally read the PHYID1/2 registers to probe the correct driver This patch adds a new helper called phy_device_atomic_register() to not break exisiting running systems based on the current mdio/phy handling. This new helper bundles all required steps into a single function to make it easier for driver developers. 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. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/net/phy/phy_device.c | 208 +++++++++++++++++++++++++++++++++++++++++++ include/linux/phy.h | 9 ++ 2 files changed, 217 insertions(+)