| Message ID | 20200420232624.9127-1-michael@walle.cc |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,net-next,1/3] net: phy: add concept of shared storage for PHYs | expand |
On Tue, Apr 21, 2020 at 01:26:22AM +0200, Michael Walle wrote: > There are packages which contain multiple PHY devices, eg. a quad PHY > transceiver. Provide functions to allocate and free shared storage. > > Usually, a quad PHY contains global registers, which don't belong to any > PHY. Provide convenience functions to access these registers. Hi Michael Please provide a patch 0/3 cover note. DaveM will uses it for the merge commit, etc. > +void phy_package_leave(struct phy_device *phydev) > +{ > + struct mii_bus *bus = phydev->mdio.bus; > + struct phy_package_shared *shared = phydev->shared; Reverse Christmas tree. > +static inline bool phy_package_init_once(struct phy_device *phydev) > +{ > + struct phy_package_shared *shared = phydev->shared; > + > + if (!shared) > + return false; > + > + return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags); > +} I need to look at how you actually use this, but i wonder if this is sufficient. Can two PHYs probe at the same time? Could we have one PHY be busy setting up the global init, and the other thinks the global setup is complete? Do we want a comment like: 'Returns true when the global package initialization is either under way or complete'? Andrew
On Tue, Apr 21, 2020 at 04:34:55PM +0200, Andrew Lunn wrote: > > +static inline bool phy_package_init_once(struct phy_device *phydev) > > +{ > > + struct phy_package_shared *shared = phydev->shared; > > + > > + if (!shared) > > + return false; > > + > > + return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags); > > +} > > I need to look at how you actually use this, but i wonder if this is > sufficient. Can two PHYs probe at the same time? Could we have one PHY > be busy setting up the global init, and the other thinks the global > setup is complete? Do we want a comment like: 'Returns true when the > global package initialization is either under way or complete'? IIRC, probe locking in the driver model is by per-driver locks, so any particular driver won't probe more than one device at a time.
On Tue, Apr 21, 2020 at 03:43:02PM +0100, Russell King - ARM Linux admin wrote: > On Tue, Apr 21, 2020 at 04:34:55PM +0200, Andrew Lunn wrote: > > > +static inline bool phy_package_init_once(struct phy_device *phydev) > > > +{ > > > + struct phy_package_shared *shared = phydev->shared; > > > + > > > + if (!shared) > > > + return false; > > > + > > > + return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags); > > > +} > > > > I need to look at how you actually use this, but i wonder if this is > > sufficient. Can two PHYs probe at the same time? Could we have one PHY > > be busy setting up the global init, and the other thinks the global > > setup is complete? Do we want a comment like: 'Returns true when the > > global package initialization is either under way or complete'? > > IIRC, probe locking in the driver model is by per-driver locks, so > any particular driver won't probe more than one device at a time. Hi Russel Cool, thanks for the info. Andrew
Am 2020-04-21 16:52, schrieb Andrew Lunn: > On Tue, Apr 21, 2020 at 03:43:02PM +0100, Russell King - ARM Linux > admin wrote: >> On Tue, Apr 21, 2020 at 04:34:55PM +0200, Andrew Lunn wrote: >> > > +static inline bool phy_package_init_once(struct phy_device *phydev) >> > > +{ >> > > + struct phy_package_shared *shared = phydev->shared; >> > > + >> > > + if (!shared) >> > > + return false; >> > > + >> > > + return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags); >> > > +} >> > >> > I need to look at how you actually use this, but i wonder if this is >> > sufficient. Can two PHYs probe at the same time? Could we have one PHY >> > be busy setting up the global init, and the other thinks the global >> > setup is complete? So with Russells answer below, this should be clarified and the test_and_set_bit() is enough correct? >> > Do we want a comment like: 'Returns true when the >> > global package initialization is either under way or complete'? I've forgot the whole annotation here. >> IIRC, probe locking in the driver model is by per-driver locks, so >> any particular driver won't probe more than one device at a time. -michael
Am 2020-04-21 01:26, schrieb Michael Walle: > + > +/* Represents a shared structure between different phydev's in the > same > + * package, for example a quad PHY. See phy_package_join() and > + * phy_package_leave(). > + */ > +struct phy_package_shared { > + int addr; > + refcount_t refcnt; > + unsigned long flags; > + > + /* private data pointer */ > + /* note that this pointer is shared between different phydevs and > + * the user has to take care of appropriate locking. > + */ > + void *priv; btw. how should a driver actually use this? I mean, it can allocate memory if its still NULL but when will it be freed again. Do we need a callback? Is there something better than a callback? -michael
On Tue, Apr 21, 2020 at 05:25:19PM +0200, Michael Walle wrote: > Am 2020-04-21 01:26, schrieb Michael Walle: > > + > > +/* Represents a shared structure between different phydev's in the same > > + * package, for example a quad PHY. See phy_package_join() and > > + * phy_package_leave(). > > + */ > > +struct phy_package_shared { > > + int addr; > > + refcount_t refcnt; > > + unsigned long flags; > > + > > + /* private data pointer */ > > + /* note that this pointer is shared between different phydevs and > > + * the user has to take care of appropriate locking. > > + */ > > + void *priv; > > btw. how should a driver actually use this? I mean, it can allocate > memory if its still NULL but when will it be freed again. Do we need > a callback? Is there something better than a callback? Good point. phy_package_join() should take a size_t and do the allocation. phy_package_leave() would then free it. But since we don't have a user at the moment, maybe leave it out. Andrew
Am 2020-04-21 17:50, schrieb Andrew Lunn: > On Tue, Apr 21, 2020 at 05:25:19PM +0200, Michael Walle wrote: >> Am 2020-04-21 01:26, schrieb Michael Walle: >> > + >> > +/* Represents a shared structure between different phydev's in the same >> > + * package, for example a quad PHY. See phy_package_join() and >> > + * phy_package_leave(). >> > + */ >> > +struct phy_package_shared { >> > + int addr; >> > + refcount_t refcnt; >> > + unsigned long flags; >> > + >> > + /* private data pointer */ >> > + /* note that this pointer is shared between different phydevs and >> > + * the user has to take care of appropriate locking. >> > + */ >> > + void *priv; >> >> btw. how should a driver actually use this? I mean, it can allocate >> memory if its still NULL but when will it be freed again. Do we need >> a callback? Is there something better than a callback? > > Good point. phy_package_join() should take a size_t and do the > allocation. phy_package_leave() would then free it. > > But since we don't have a user at the moment, maybe leave it out. Speaking of it. Does anyone have an idea how I could create the hwmon device without the PHY device? At the moment it is attached to the first PHY device and is removed when the PHY is removed, although there might be still other PHYs in this package. Its unlikely to happen though, but if someone has a good idea how to handle that, I'd give it a try. -michael
> Speaking of it. Does anyone have an idea how I could create the hwmon > device without the PHY device? At the moment it is attached to the > first PHY device and is removed when the PHY is removed, although > there might be still other PHYs in this package. Its unlikely to > happen though, but if someone has a good idea how to handle that, > I'd give it a try. There is a somewhat similar problem with Marvell Ethernet switches and their internal PHYs. The PHYs are the same as the discrete PHYs, and the usual Marvell PHY driver is used. But there is only one temperature sensor for the whole switch, and it is mapped into all the PHYs. So we end up creating multiple hwmon devices for the one temperature sensor, one per PHY. You could take the same approach here. Each PHY exposes a hwmon device? Looking at static struct device * __hwmon_device_register(struct device *dev, const char *name, void *drvdata, const struct hwmon_chip_info *chip, const struct attribute_group **groups) I think it is O.K. to pass dev as NULL. You don't have to associate it to a device. So you could create the hwmon device as part of package initialisation and put it into shared->priv. Andrew
On Tue, Apr 21, 2020 at 09:30:55PM +0200, Andrew Lunn wrote: > > Speaking of it. Does anyone have an idea how I could create the hwmon > > device without the PHY device? At the moment it is attached to the > > first PHY device and is removed when the PHY is removed, although > > there might be still other PHYs in this package. Its unlikely to > > happen though, but if someone has a good idea how to handle that, > > I'd give it a try. > > There is a somewhat similar problem with Marvell Ethernet switches and > their internal PHYs. The PHYs are the same as the discrete PHYs, and > the usual Marvell PHY driver is used. But there is only one > temperature sensor for the whole switch, and it is mapped into all the > PHYs. So we end up creating multiple hwmon devices for the one > temperature sensor, one per PHY. And sometimes we really mess it up - like on the 88e6141: cp1configspacef4000000mdio12a200switch04mdio14-mdio-e Adapter: MDIO adapter temp1: -75.0°C because DSA forces the 6390 PHY ID for this PHY, and the marvell driver tries to drive the PHY as if it's a different switch, so we end up reading a register that isn't meaningful. So, imho, the current approach isn't as good as you think it is. That aside from wasting memory allocating multiple sensors when there's really only one.
Am 2020-04-21 21:30, schrieb Andrew Lunn: >> Speaking of it. Does anyone have an idea how I could create the hwmon >> device without the PHY device? At the moment it is attached to the >> first PHY device and is removed when the PHY is removed, although >> there might be still other PHYs in this package. Its unlikely to >> happen though, but if someone has a good idea how to handle that, >> I'd give it a try. > > There is a somewhat similar problem with Marvell Ethernet switches and > their internal PHYs. The PHYs are the same as the discrete PHYs, and > the usual Marvell PHY driver is used. But there is only one > temperature sensor for the whole switch, and it is mapped into all the > PHYs. So we end up creating multiple hwmon devices for the one > temperature sensor, one per PHY. > > You could take the same approach here. Each PHY exposes a hwmon > device? > > Looking at > static struct device * > __hwmon_device_register(struct device *dev, const char *name, void > *drvdata, > const struct hwmon_chip_info *chip, > const struct attribute_group **groups) > > I think it is O.K. to pass dev as NULL. You don't have to associate it > to a device. So you could create the hwmon device as part of package > initialisation and put it into shared->priv. I actually tried that before writing my mail. Have a look at commit 59df4f4e8e0b ("hwmon: (core) check parent dev != NULL when chip != NULL") and the corresponding discussion here: https://patchwork.kernel.org/patch/10381759/ And if I'd had to choose, I'd prefer having one hwmon device on the first PHY (with its drawback) rather than having it four times. -michael
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 7a4eb3f2cb74..247cb6610a05 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -611,6 +611,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) } mutex_init(&bus->mdio_lock); + mutex_init(&bus->shared_lock); /* de-assert bus level PHY GPIO reset */ gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index ac2784192472..420af3f732c3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1457,6 +1457,118 @@ bool phy_driver_is_genphy_10g(struct phy_device *phydev) } EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g); +/** + * phy_package_join - join a common PHY group + * @phydev: target phy_device struct + * @addr: cookie and PHY address for global register access + * + * This joins a PHY group and provides a shared storage for all phydevs in + * this group. This is intended to be used for packages which contain + * more than one PHY, for example a quad PHY transceiver. + * + * The addr parameter serves as a cookie which has to have the same value + * for all members of one group and as a PHY address to access generic + * registers of a PHY package. Usually, one of the PHY addresses of the + * different PHYs in the package provides access to these global registers. + * The address which is given here, will be used in the phy_package_read() + * and phy_package_write() convenience functions. If your PHY doesn't have + * global registers you can just pick any of the PHY addresses. + * + * This will set the shared pointer of the phydev to the shared storage. + * If this is the first call for a this cookie the share storage will be + * allocated. + */ +int phy_package_join(struct phy_device *phydev, int addr) +{ + struct mii_bus *bus = phydev->mdio.bus; + struct phy_package_shared *shared; + + if (addr >= PHY_MAX_ADDR) + return -EINVAL; + + mutex_lock(&bus->shared_lock); + shared = bus->shared[addr]; + if (!shared) { + shared = kzalloc(sizeof(*shared), GFP_KERNEL); + shared->addr = addr; + refcount_set(&shared->refcnt, 1); + bus->shared[addr] = shared; + } else { + refcount_inc(&shared->refcnt); + } + mutex_unlock(&bus->shared_lock); + + phydev->shared = shared; + + return 0; +} +EXPORT_SYMBOL_GPL(phy_package_join); + +/** + * phy_package_leave - leave a common PHY group + * @phydev: target phy_device struct + * + * This leaves a PHY group created by phy_package_join(). If this phydev + * was the last user of the shared data between the group, this data is + * freed. Resets the phydev->shared pointer to NULL. + */ +void phy_package_leave(struct phy_device *phydev) +{ + struct mii_bus *bus = phydev->mdio.bus; + struct phy_package_shared *shared = phydev->shared; + + if (!shared) + return; + + if (refcount_dec_and_mutex_lock(&shared->refcnt, &bus->shared_lock)) { + bus->shared[shared->addr] = NULL; + mutex_unlock(&bus->shared_lock); + kfree(shared); + } + + phydev->shared = NULL; +} +EXPORT_SYMBOL_GPL(phy_package_leave); + +static void devm_phy_package_leave(struct device *dev, void *res) +{ + phy_package_leave(*(struct phy_device **)res); +} + +/** + * devm_phy_package_join - resource managed phy_package_join() + * @dev: device that is registering this GPIO device + * @phydev: target phy_device struct + * @addr: cookie and PHY address for global register access + * + * Managed phy_package_join(). Shared storage fetched by this function, + * phy_package_leave() is automatically called on driver detach. See + * phy_package_join() for more information. + */ +int devm_phy_package_join(struct device *dev, struct phy_device *phydev, + int addr) +{ + struct phy_device **ptr; + int ret; + + ptr = devres_alloc(devm_phy_package_leave, sizeof(*ptr), + GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ret = phy_package_join(phydev, addr); + + if (!ret) { + *ptr = phydev; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return ret; +} +EXPORT_SYMBOL_GPL(devm_phy_package_join); + /** * phy_detach - detach a PHY device from its network device * @phydev: target phy_device struct diff --git a/include/linux/phy.h b/include/linux/phy.h index 2432ca463ddc..d0d85868af76 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -25,6 +25,7 @@ #include <linux/u64_stats_sync.h> #include <linux/irqreturn.h> #include <linux/iopoll.h> +#include <linux/refcount.h> #include <linux/atomic.h> @@ -227,6 +228,25 @@ struct mdio_bus_stats { struct u64_stats_sync syncp; }; + +/* Represents a shared structure between different phydev's in the same + * package, for example a quad PHY. See phy_package_join() and + * phy_package_leave(). + */ +struct phy_package_shared { + int addr; + refcount_t refcnt; + unsigned long flags; + + /* private data pointer */ + /* note that this pointer is shared between different phydevs and + * the user has to take care of appropriate locking. + */ + void *priv; +}; + +#define PHY_SHARED_F_INIT_DONE 0 + /* * The Bus class for PHYs. Devices which provide access to * PHYs should register using this structure @@ -275,6 +295,12 @@ struct mii_bus { int reset_delay_us; /* RESET GPIO descriptor pointer */ struct gpio_desc *reset_gpiod; + + /* protect access to the shared element */ + struct mutex shared_lock; + + /* shared state across different PHYs */ + struct phy_package_shared *shared[PHY_MAX_ADDR]; }; #define to_mii_bus(d) container_of(d, struct mii_bus, dev) @@ -461,6 +487,10 @@ struct phy_device { /* For use by PHYs to maintain extra state */ void *priv; + /* shared data pointer */ + /* For use by PHYs inside the same package and needs a shared state. */ + struct phy_package_shared *shared; + /* Interrupt and Polling infrastructure */ struct delayed_work state_queue; @@ -1341,6 +1371,10 @@ int phy_ethtool_get_link_ksettings(struct net_device *ndev, int phy_ethtool_set_link_ksettings(struct net_device *ndev, const struct ethtool_link_ksettings *cmd); int phy_ethtool_nway_reset(struct net_device *ndev); +int phy_package_join(struct phy_device *phydev, int addr); +void phy_package_leave(struct phy_device *phydev); +int devm_phy_package_join(struct device *dev, struct phy_device *phydev, + int addr); #if IS_ENABLED(CONFIG_PHYLIB) int __init mdio_bus_init(void); @@ -1393,6 +1427,58 @@ static inline int phy_ethtool_get_stats(struct phy_device *phydev, return 0; } +static inline int phy_package_read(struct phy_device *phydev, u32 regnum) +{ + struct phy_package_shared *shared = phydev->shared; + + if (!shared) + return -EIO; + + return mdiobus_read(phydev->mdio.bus, shared->addr, regnum); +} + +static inline int __phy_package_read(struct phy_device *phydev, u32 regnum) +{ + struct phy_package_shared *shared = phydev->shared; + + if (!shared) + return -EIO; + + return __mdiobus_read(phydev->mdio.bus, shared->addr, regnum); +} + +static inline int phy_package_write(struct phy_device *phydev, + u32 regnum, u16 val) +{ + struct phy_package_shared *shared = phydev->shared; + + if (!shared) + return -EIO; + + return mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val); +} + +static inline int __phy_package_write(struct phy_device *phydev, + u32 regnum, u16 val) +{ + struct phy_package_shared *shared = phydev->shared; + + if (!shared) + return -EIO; + + return __mdiobus_write(phydev->mdio.bus, shared->addr, regnum, val); +} + +static inline bool phy_package_init_once(struct phy_device *phydev) +{ + struct phy_package_shared *shared = phydev->shared; + + if (!shared) + return false; + + return !test_and_set_bit(PHY_SHARED_F_INIT_DONE, &shared->flags); +} + extern struct bus_type mdio_bus_type; struct mdio_board_info {
There are packages which contain multiple PHY devices, eg. a quad PHY transceiver. Provide functions to allocate and free shared storage. Usually, a quad PHY contains global registers, which don't belong to any PHY. Provide convenience functions to access these registers. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/phy/mdio_bus.c | 1 + drivers/net/phy/phy_device.c | 112 +++++++++++++++++++++++++++++++++++ include/linux/phy.h | 86 +++++++++++++++++++++++++++ 3 files changed, 199 insertions(+)