On Mon, Jul 11, 2022 at 9:05 AM Sean Anderson <sean.anderson@xxxxxxxx> wrote: > > This adds support for getting PCS devices from the device tree. PCS > drivers must first register with phylink_register_pcs. After that, MAC > drivers may look up their PCS using phylink_get_pcs. > > To prevent the PCS driver from leaving suddenly, we use try_module_get. To > provide some ordering during probing/removal, we use device links managed > by of_fwnode_add_links. This will reduce the number of probe failures due > to deferral. It will not prevent this for non-standard properties (aka > pcsphy-handle), but the worst that happens is that we re-probe a few times. > > At the moment there is no support for specifying the interface used to > talk to the PCS. The MAC driver is expected to know how to talk to the > PCS. This is not a change, but it is perhaps an area for improvement. > > Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx> > --- > This is adapted from [1], primarily incorporating the changes discussed > there. > > [1] https://lore.kernel.org/netdev/9f73bc4f-5f99-95f5-78fa-dac96f9e0146@xxxxxxxx/ > > MAINTAINERS | 1 + > drivers/net/pcs/Kconfig | 12 +++ > drivers/net/pcs/Makefile | 2 + > drivers/net/pcs/core.c | 226 +++++++++++++++++++++++++++++++++++++++ > drivers/of/property.c | 2 + > include/linux/pcs.h | 33 ++++++ > include/linux/phylink.h | 6 ++ > 7 files changed, 282 insertions(+) > create mode 100644 drivers/net/pcs/core.c > create mode 100644 include/linux/pcs.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index ca95b1833b97..3965d49753d3 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7450,6 +7450,7 @@ F: include/linux/*mdio*.h > F: include/linux/mdio/*.h > F: include/linux/mii.h > F: include/linux/of_net.h > +F: include/linux/pcs.h > F: include/linux/phy.h > F: include/linux/phy_fixed.h > F: include/linux/platform_data/mdio-bcm-unimac.h > diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig > index 22ba7b0b476d..fed6264fdf33 100644 > --- a/drivers/net/pcs/Kconfig > +++ b/drivers/net/pcs/Kconfig > @@ -5,6 +5,18 @@ > > menu "PCS device drivers" > > +config PCS > + bool "PCS subsystem" > + help > + This provides common helper functions for registering and looking up > + Physical Coding Sublayer (PCS) devices. PCS devices translate between > + different interface types. In some use cases, they may either > + translate between different types of Medium-Independent Interfaces > + (MIIs), such as translating GMII to SGMII. This allows using a fast > + serial interface to talk to the phy which translates the MII to the > + Medium-Dependent Interface. Alternatively, they may translate a MII > + directly to an MDI, such as translating GMII to 1000Base-X. > + > config PCS_XPCS > tristate "Synopsys DesignWare XPCS controller" > depends on MDIO_DEVICE && MDIO_BUS > diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile > index 0603d469bd57..1fd21a1619d4 100644 > --- a/drivers/net/pcs/Makefile > +++ b/drivers/net/pcs/Makefile > @@ -1,6 +1,8 @@ > # SPDX-License-Identifier: GPL-2.0 > # Makefile for Linux PCS drivers > > +obj-$(CONFIG_PCS) += core.o > + > pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o > > obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o > diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c > new file mode 100644 > index 000000000000..b39ff1ccdb34 > --- /dev/null > +++ b/drivers/net/pcs/core.c > @@ -0,0 +1,226 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2022 Sean Anderson <sean.anderson@xxxxxxxx> > + */ > + > +#include <linux/fwnode.h> > +#include <linux/list.h> > +#include <linux/mutex.h> > +#include <linux/pcs.h> > +#include <linux/phylink.h> > +#include <linux/property.h> > + > +static LIST_HEAD(pcs_devices); > +static DEFINE_MUTEX(pcs_mutex); > + > +/** > + * pcs_register() - register a new PCS > + * @pcs: the PCS to register > + * > + * Registers a new PCS which can be automatically attached to a phylink. > + * > + * Return: 0 on success, or -errno on error > + */ > +int pcs_register(struct phylink_pcs *pcs) > +{ > + if (!pcs->dev || !pcs->ops) > + return -EINVAL; > + if (!pcs->ops->pcs_an_restart || !pcs->ops->pcs_config || > + !pcs->ops->pcs_get_state) > + return -EINVAL; > + > + INIT_LIST_HEAD(&pcs->list); > + mutex_lock(&pcs_mutex); > + list_add(&pcs->list, &pcs_devices); > + mutex_unlock(&pcs_mutex); > + return 0; > +} > +EXPORT_SYMBOL_GPL(pcs_register); > + > +/** > + * pcs_unregister() - unregister a PCS > + * @pcs: a PCS previously registered with pcs_register() > + */ > +void pcs_unregister(struct phylink_pcs *pcs) > +{ > + mutex_lock(&pcs_mutex); > + list_del(&pcs->list); > + mutex_unlock(&pcs_mutex); > +} > +EXPORT_SYMBOL_GPL(pcs_unregister); > + > +static void devm_pcs_release(struct device *dev, void *res) > +{ > + pcs_unregister(*(struct phylink_pcs **)res); > +} > + > +/** > + * devm_pcs_register - resource managed pcs_register() > + * @dev: device that is registering this PCS > + * @pcs: the PCS to register > + * > + * Managed pcs_register(). For PCSs registered by this function, > + * pcs_unregister() is automatically called on driver detach. See > + * pcs_register() for more information. > + * > + * Return: 0 on success, or -errno on failure > + */ > +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs) > +{ > + struct phylink_pcs **pcsp; > + int ret; > + > + pcsp = devres_alloc(devm_pcs_release, sizeof(*pcsp), > + GFP_KERNEL); > + if (!pcsp) > + return -ENOMEM; > + > + ret = pcs_register(pcs); > + if (ret) { > + devres_free(pcsp); > + return ret; > + } > + > + *pcsp = pcs; > + devres_add(dev, pcsp); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(devm_pcs_register); > + > +/** > + * pcs_find() - Find the PCS associated with a fwnode or device > + * @fwnode: The PCS's fwnode > + * @dev: The PCS's device > + * > + * Search PCSs registered with pcs_register() for one with a matching > + * fwnode or device. Either @fwnode or @dev may be %NULL if matching against a > + * fwnode or device is not desired (respectively). > + * > + * Return: a matching PCS, or %NULL if not found > + */ > +static struct phylink_pcs *pcs_find(const struct fwnode_handle *fwnode, > + const struct device *dev) > +{ > + struct phylink_pcs *pcs; > + > + mutex_lock(&pcs_mutex); > + list_for_each_entry(pcs, &pcs_devices, list) { > + if (dev && pcs->dev == dev) > + goto out; > + if (fwnode && pcs->dev->fwnode == fwnode) > + goto out; > + } > + pcs = NULL; > + > +out: > + mutex_unlock(&pcs_mutex); > + pr_devel("%s: looking for %pfwf or %s %s...%s found\n", __func__, > + fwnode, dev ? dev_driver_string(dev) : "(null)", > + dev ? dev_name(dev) : "(null)", pcs ? " not" : ""); > + return pcs; > +} > + > +/** > + * pcs_get_tail() - Finish getting a PCS > + * @pcs: The PCS to get, or %NULL if one could not be found > + * > + * This performs common operations necessary when getting a PCS (chiefly > + * incrementing reference counts) > + * > + * Return: @pcs, or an error pointer on failure > + */ > +static struct phylink_pcs *pcs_get_tail(struct phylink_pcs *pcs) > +{ > + if (!pcs) > + return ERR_PTR(-EPROBE_DEFER); > + > + if (!try_module_get(pcs->ops->owner)) > + return ERR_PTR(-ENODEV); > + get_device(pcs->dev); > + > + return pcs; > +} > + > +/** > + * _pcs_get_by_fwnode() - Get a PCS from a fwnode property > + * @fwnode: The fwnode to get an associated PCS of > + * @id: The name of the PCS to get. May be %NULL to get the first PCS. > + * @optional: Whether the PCS is optional or not > + * > + * Look up a PCS associated with @fwnode and return a reference to it. Every > + * call to pcs_get_by_fwnode() must be balanced with one to pcs_put(). > + * > + * If @optional is true, and @id is non-%NULL, then if @id cannot be found in > + * pcs-names, %NULL is returned (instead of an error). If @optional is true and > + * @id is %NULL, then no error is returned if pcs-handle is absent. > + * > + * Return: a PCS if found, or an error pointer on failure > + */ > +struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode, > + const char *id, bool optional) > +{ > + int index; > + struct phylink_pcs *pcs; > + struct fwnode_handle *pcs_fwnode; > + > + if (id) > + index = fwnode_property_match_string(fwnode, "pcs-names", id); > + else > + index = 0; > + if (index < 0) { > + if (optional && (index == -EINVAL || index == -ENODATA)) > + return NULL; > + return ERR_PTR(index); > + } > + > + /* First try pcs-handle, and if that doesn't work fall back to the > + * (legacy) pcsphy-handle. > + */ > + pcs_fwnode = fwnode_find_reference(fwnode, "pcs-handle", index); > + if (PTR_ERR(pcs_fwnode) == -ENOENT) > + pcs_fwnode = fwnode_find_reference(fwnode, "pcsphy-handle", > + index); > + if (optional && !id && PTR_ERR(pcs_fwnode) == -ENOENT) > + return NULL; > + else if (IS_ERR(pcs_fwnode)) > + return ERR_CAST(pcs_fwnode); > + > + pcs = pcs_find(pcs_fwnode, NULL); > + fwnode_handle_put(pcs_fwnode); > + return pcs_get_tail(pcs); > +} > +EXPORT_SYMBOL_GPL(pcs_get_by_fwnode); > + > +/** > + * pcs_get_by_provider() - Get a PCS from an existing provider > + * @dev: The device providing the PCS > + * > + * This finds the first PCS registersed by @dev and returns a reference to it. > + * Every call to pcs_get_by_provider() must be balanced with one to > + * pcs_put(). > + * > + * Return: a PCS if found, or an error pointer on failure > + */ > +struct phylink_pcs *pcs_get_by_provider(const struct device *dev) > +{ > + return pcs_get_tail(pcs_find(NULL, dev)); > +} > +EXPORT_SYMBOL_GPL(pcs_get_by_provider); > + > +/** > + * pcs_put() - Release a previously-acquired PCS > + * @pcs: The PCS to put > + * > + * This frees resources associated with the PCS which were acquired when it was > + * gotten. > + */ > +void pcs_put(struct phylink_pcs *pcs) > +{ > + if (!pcs) > + return; > + > + put_device(pcs->dev); > + module_put(pcs->ops->owner); > +} > +EXPORT_SYMBOL_GPL(pcs_put); > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 967f79b59016..860d35bde5e9 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1318,6 +1318,7 @@ DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL) > DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) > DEFINE_SIMPLE_PROP(remote_endpoint, "remote-endpoint", NULL) > +DEFINE_SIMPLE_PROP(pcs_handle, "pcs-handle", NULL) > DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells") > DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells") > DEFINE_SIMPLE_PROP(leds, "leds", NULL) > @@ -1406,6 +1407,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_pinctrl7, }, > { .parse_prop = parse_pinctrl8, }, > { .parse_prop = parse_remote_endpoint, .node_not_dev = true, }, > + { .parse_prop = parse_pcs_handle, }, > { .parse_prop = parse_pwms, }, > { .parse_prop = parse_resets, }, > { .parse_prop = parse_leds, }, Can you break the changes to this file into a separate patch please? That'll clarify that this doesn't depend on any of the other changes in this patch to work and it can stand on its own. Also, I don't know how the pcs-handle is used, but it's likely that this probe ordering enforcement could cause issues. So, if we need to revert it, having it as a separate patch would help too. And put this at the end of the series maybe? Thanks, Saravana > > diff --git a/include/linux/pcs.h b/include/linux/pcs.h > new file mode 100644 > index 000000000000..00e76594e03c > --- /dev/null > +++ b/include/linux/pcs.h > @@ -0,0 +1,33 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2022 Sean Anderson <sean.anderson@xxxxxxxx> > + */ > + > +#ifndef _PCS_H > +#define _PCS_H > + > +struct phylink_pcs; > +struct fwnode; > + > +int pcs_register(struct phylink_pcs *pcs); > +void pcs_unregister(struct phylink_pcs *pcs); > +int devm_pcs_register(struct device *dev, struct phylink_pcs *pcs); > +struct phylink_pcs *_pcs_get_by_fwnode(const struct fwnode_handle *fwnode, > + const char *id, bool optional); > +struct phylink_pcs *pcs_get_by_provider(const struct device *dev); > +void pcs_put(struct phylink_pcs *pcs); > + > +static inline struct phylink_pcs > +*pcs_get_by_fwnode(const struct fwnode_handle *fwnode, > + const char *id) > +{ > + return _pcs_get_by_fwnode(fwnode, id, false); > +} > + > +static inline struct phylink_pcs > +*pcs_get_by_fwnode_optional(const struct fwnode_handle *fwnode, const char *id) > +{ > + return _pcs_get_by_fwnode(fwnode, id, true); > +} > + > +#endif /* PCS_H */ > diff --git a/include/linux/phylink.h b/include/linux/phylink.h > index 6d06896fc20d..a713e70108a1 100644 > --- a/include/linux/phylink.h > +++ b/include/linux/phylink.h > @@ -396,19 +396,24 @@ struct phylink_pcs_ops; > > /** > * struct phylink_pcs - PHYLINK PCS instance > + * @dev: the device associated with this PCS > * @ops: a pointer to the &struct phylink_pcs_ops structure > + * @list: internal list of PCS devices > * @poll: poll the PCS for link changes > * > * This structure is designed to be embedded within the PCS private data, > * and will be passed between phylink and the PCS. > */ > struct phylink_pcs { > + struct device *dev; > const struct phylink_pcs_ops *ops; > + struct list_head list; > bool poll; > }; > > /** > * struct phylink_pcs_ops - MAC PCS operations structure. > + * @owner: the module which implements this PCS. > * @pcs_validate: validate the link configuration. > * @pcs_get_state: read the current MAC PCS link state from the hardware. > * @pcs_config: configure the MAC PCS for the selected mode and state. > @@ -417,6 +422,7 @@ struct phylink_pcs { > * (where necessary). > */ > struct phylink_pcs_ops { > + struct module *owner; > int (*pcs_validate)(struct phylink_pcs *pcs, unsigned long *supported, > const struct phylink_link_state *state); > void (*pcs_get_state)(struct phylink_pcs *pcs, > -- > 2.35.1.1320.gc452695387.dirty >
