Re: [RFC PATCH net-next 3/9] net: pcs: Add helpers for registering and finding PCSs
From: Sean Anderson
Date: Mon Jul 11 2022 - 15:53:37 EST
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
>>
>