On 7/11/22 3:42 PM, Saravana Kannan wrote: > 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. OK > 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? OK, I'll put it before patch 9/9 (which will likely need to be applied much after the rest of this series. --Sean > 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 >> >
