[net-next,v3,2/4] net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx
Commit Message
The newly introduced regmap-based MDIO driver allows for an easy mapping
of an mdiodevice onto the memory-mapped TSE PCS, which is actually a
Lynx PCS.
Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which
is nothing more than a memory-mapped Lynx PCS.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V2->V3 : No changes
V1->V2 : No changes
drivers/net/ethernet/altera/altera_tse.h | 1 +
drivers/net/ethernet/altera/altera_tse_main.c | 57 +++++++++++++++++--
include/linux/mdio/mdio-regmap.h | 2 +
3 files changed, 54 insertions(+), 6 deletions(-)
Comments
On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote:
> The newly introduced regmap-based MDIO driver allows for an easy mapping
> of an mdiodevice onto the memory-mapped TSE PCS, which is actually a
> Lynx PCS.
>
> Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which
> is nothing more than a memory-mapped Lynx PCS.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Hi Maxime,
I have some concerns about the error paths in this patch.
...
> @@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct platform_device *pdev)
> const struct of_device_id *of_id = NULL;
> struct altera_tse_private *priv;
> struct resource *control_port;
> + struct regmap *pcs_regmap;
> struct resource *dma_res;
> struct resource *pcs_res;
> + struct mii_bus *pcs_bus;
> struct net_device *ndev;
> void __iomem *descmap;
> - int pcs_reg_width = 2;
> int ret = -ENODEV;
>
> + struct regmap_config pcs_regmap_cfg;
nit: this probably belongs in with the bunch of declarations above it.
> +
> + struct mdio_regmap_config mrc = {
> + .parent = &pdev->dev,
> + .valid_addr = 0x0,
> + };
nit: maybe this too.
> +
> ndev = alloc_etherdev(sizeof(struct altera_tse_private));
> if (!ndev) {
> dev_err(&pdev->dev, "Could not allocate network device\n");
> @@ -1258,10 +1268,29 @@ static int altera_tse_probe(struct platform_device *pdev)
> ret = request_and_map(pdev, "pcs", &pcs_res,
> &priv->pcs_base);
> if (ret) {
> + /* If we can't find a dedicated resource for the PCS, fallback
> + * to the internal PCS, that has a different address stride
> + */
> priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0);
> - pcs_reg_width = 4;
> + pcs_regmap_cfg.reg_bits = 32;
> + /* Values are MDIO-like values, on 16 bits */
> + pcs_regmap_cfg.val_bits = 16;
> + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2);
> + } else {
> + pcs_regmap_cfg.reg_bits = 16;
> + pcs_regmap_cfg.val_bits = 16;
> + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
> }
>
> + /* Create a regmap for the PCS so that it can be used by the PCS driver */
> + pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base,
> + &pcs_regmap_cfg);
> + if (IS_ERR(pcs_regmap)) {
> + ret = PTR_ERR(pcs_regmap);
> + goto err_free_netdev;
> + }
> + mrc.regmap = pcs_regmap;
> +
> /* Rx IRQ */
> priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
> if (priv->rx_irq == -ENXIO) {
> @@ -1384,7 +1413,20 @@ static int altera_tse_probe(struct platform_device *pdev)
> (unsigned long) control_port->start, priv->rx_irq,
> priv->tx_irq);
>
> - priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, pcs_reg_width);
> + snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
> + pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
> + if (IS_ERR(pcs_bus)) {
> + ret = PTR_ERR(pcs_bus);
> + goto err_init_phy;
> + }
> +
> + priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0);
mdio_device_create() can fail. Should that be handled here?
> +
> + priv->pcs = lynx_pcs_create(priv->pcs_mdiodev);
> + if (!priv->pcs) {
> + ret = -ENODEV;
> + goto err_init_phy;
Does this leak priv->pcs_mdiodev?
> + }
>
> priv->phylink_config.dev = &ndev->dev;
> priv->phylink_config.type = PHYLINK_NETDEV;
> @@ -1407,11 +1449,12 @@ static int altera_tse_probe(struct platform_device *pdev)
> if (IS_ERR(priv->phylink)) {
> dev_err(&pdev->dev, "failed to create phylink\n");
> ret = PTR_ERR(priv->phylink);
> - goto err_init_phy;
> + goto err_pcs;
Does this leak priv->pcs ?
> }
>
> return 0;
> -
> +err_pcs:
> + mdio_device_free(priv->pcs_mdiodev);
> err_init_phy:
> unregister_netdev(ndev);
> err_register_netdev:
> @@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct platform_device *pdev)
> altera_tse_mdio_destroy(ndev);
> unregister_netdev(ndev);
> phylink_destroy(priv->phylink);
> + mdio_device_free(priv->pcs_mdiodev);
> +
> free_netdev(ndev);
>
> return 0;
> diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
> index b8508f152552..679d9069846b 100644
> --- a/include/linux/mdio/mdio-regmap.h
> +++ b/include/linux/mdio/mdio-regmap.h
> @@ -7,6 +7,8 @@
> #ifndef MDIO_REGMAP_H
> #define MDIO_REGMAP_H
>
> +#include <linux/phy.h>
> +
> struct device;
> struct regmap;
>
This hunk doesn't seem strictly related to the patch.
Perhaps the include belongs elsewhere.
Or the hunk belongs in another patch.
On Fri, May 26, 2023 at 10:39:08AM +0200, Simon Horman wrote:
> On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote:
> > The newly introduced regmap-based MDIO driver allows for an easy mapping
> > of an mdiodevice onto the memory-mapped TSE PCS, which is actually a
> > Lynx PCS.
> >
> > Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which
> > is nothing more than a memory-mapped Lynx PCS.
> >
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>
> Hi Maxime,
>
> I have some concerns about the error paths in this patch.
We've had similar problems with mdio_device_create() vs the XPCS
driver.
I think it's time that we made this easier for users.
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index b87c69c4cdd7..802222581feb 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
if (!xpcs)
return ERR_PTR(-ENOMEM);
+ mdio_device_get(mdiodev);
xpcs->mdiodev = mdiodev;
xpcs_id = xpcs_get_id(xpcs);
@@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
ret = -ENODEV;
out:
+ mdio_device_put(mdiodev);
kfree(xpcs);
return ERR_PTR(ret);
@@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create);
void xpcs_destroy(struct dw_xpcs *xpcs)
{
+ mdio_device_put(mdiodev);
kfree(xpcs);
}
EXPORT_SYMBOL_GPL(xpcs_destroy);
+struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
+ phy_interface_t interface)
+{
+ struct mdio_device *mdiodev;
+ struct dw_xpcs *xpcs;
+
+ mdiodev = mdio_device_create(bus, addr);
+ if (IS_ERR(mdiodev))
+ return ERR_CAST(mdiodev);
+
+ xpcs = xpcs_create(mdiodev, interface);
+
+ /* xpcs_create() has taken a refcount on the mdiodev if it was
+ * successful. If xpcs_create() fails, this will free the mdio
+ * device here. In any case, we don't need to hold our reference
+ * anymore, and putting it here will allow mdio_device_put() in
+ * xpcs_destroy() to automatically free the mdio device.
+ */
+ mdio_device_put(mdiodev);
+
+ return xpcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
+
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 1d7d550bbf1a..537b62330c90 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv);
void mdio_driver_unregister(struct mdio_driver *drv);
int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
+static inline void mdio_device_get(struct mdio_device *mdiodev)
+{
+ get_device(&mdiodev->dev);
+}
+
+static inline void mdio_device_put(struct mdio_device *mdiodev)
+{
+ mdio_device_free(mdiodev);
+}
+
static inline bool mdio_phy_id_is_c45(int phy_id)
{
return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);
The same for pcs-lynx. That way we remove the need for driver authors
to get the creation and destruction of the mdio device correct
without messing up the existing users - they only have to worry about
creating the PCS via the xxx_create_mdiodev() function and destroying
it via xxx_destroy().
On Fri, May 26, 2023 at 10:05:17AM +0100, Russell King (Oracle) wrote:
> On Fri, May 26, 2023 at 10:39:08AM +0200, Simon Horman wrote:
> > On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote:
> > > The newly introduced regmap-based MDIO driver allows for an easy mapping
> > > of an mdiodevice onto the memory-mapped TSE PCS, which is actually a
> > > Lynx PCS.
> > >
> > > Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which
> > > is nothing more than a memory-mapped Lynx PCS.
> > >
> > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> >
> > Hi Maxime,
> >
> > I have some concerns about the error paths in this patch.
>
> We've had similar problems with mdio_device_create() vs the XPCS
> driver.
>
> I think it's time that we made this easier for users.
Patch series here:
https://lore.kernel.org/all/ZHCGZ8IgAAwr8bla@shell.armlinux.org.uk/
Hello Simon,
On Fri, 26 May 2023 10:39:08 +0200
Simon Horman <simon.horman@corigine.com> wrote:
> On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote:
> > The newly introduced regmap-based MDIO driver allows for an easy
> > mapping of an mdiodevice onto the memory-mapped TSE PCS, which is
> > actually a Lynx PCS.
> >
> > Convert Altera TSE to use this PCS instead of the pcs-altera-tse,
> > which is nothing more than a memory-mapped Lynx PCS.
> >
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>
> Hi Maxime,
>
> I have some concerns about the error paths in this patch.
Heh I didn't do a very good job in that regard indeed :/ I'll address
these, but I'll wait for Russell series to make it through though.
Thanks for spotting this.
> ...
>
> > @@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct
> > platform_device *pdev) const struct of_device_id *of_id = NULL;
> > struct altera_tse_private *priv;
> > struct resource *control_port;
> > + struct regmap *pcs_regmap;
> > struct resource *dma_res;
> > struct resource *pcs_res;
> > + struct mii_bus *pcs_bus;
> > struct net_device *ndev;
> > void __iomem *descmap;
> > - int pcs_reg_width = 2;
> > int ret = -ENODEV;
> >
> > + struct regmap_config pcs_regmap_cfg;
>
> nit: this probably belongs in with the bunch of declarations above it.
>
> > +
> > + struct mdio_regmap_config mrc = {
> > + .parent = &pdev->dev,
> > + .valid_addr = 0x0,
> > + };
>
> nit: maybe this too.
>
> > +
> > ndev = alloc_etherdev(sizeof(struct altera_tse_private));
> > if (!ndev) {
> > dev_err(&pdev->dev, "Could not allocate network
> > device\n"); @@ -1258,10 +1268,29 @@ static int
> > altera_tse_probe(struct platform_device *pdev) ret =
> > request_and_map(pdev, "pcs", &pcs_res, &priv->pcs_base);
> > if (ret) {
> > + /* If we can't find a dedicated resource for the
> > PCS, fallback
> > + * to the internal PCS, that has a different
> > address stride
> > + */
> > priv->pcs_base = priv->mac_dev +
> > tse_csroffs(mdio_phy0);
> > - pcs_reg_width = 4;
> > + pcs_regmap_cfg.reg_bits = 32;
> > + /* Values are MDIO-like values, on 16 bits */
> > + pcs_regmap_cfg.val_bits = 16;
> > + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2);
> > + } else {
> > + pcs_regmap_cfg.reg_bits = 16;
> > + pcs_regmap_cfg.val_bits = 16;
> > + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
> > }
> >
> > + /* Create a regmap for the PCS so that it can be used by
> > the PCS driver */
> > + pcs_regmap = devm_regmap_init_mmio(&pdev->dev,
> > priv->pcs_base,
> > + &pcs_regmap_cfg);
> > + if (IS_ERR(pcs_regmap)) {
> > + ret = PTR_ERR(pcs_regmap);
> > + goto err_free_netdev;
> > + }
> > + mrc.regmap = pcs_regmap;
> > +
> > /* Rx IRQ */
> > priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
> > if (priv->rx_irq == -ENXIO) {
> > @@ -1384,7 +1413,20 @@ static int altera_tse_probe(struct
> > platform_device *pdev) (unsigned long) control_port->start,
> > priv->rx_irq, priv->tx_irq);
> >
> > - priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base,
> > pcs_reg_width);
> > + snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii",
> > ndev->name);
> > + pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
> > + if (IS_ERR(pcs_bus)) {
> > + ret = PTR_ERR(pcs_bus);
> > + goto err_init_phy;
> > + }
> > +
> > + priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0);
>
> mdio_device_create() can fail. Should that be handled here?
>
> > +
> > + priv->pcs = lynx_pcs_create(priv->pcs_mdiodev);
> > + if (!priv->pcs) {
> > + ret = -ENODEV;
> > + goto err_init_phy;
>
> Does this leak priv->pcs_mdiodev?
>
> > + }
> >
> > priv->phylink_config.dev = &ndev->dev;
> > priv->phylink_config.type = PHYLINK_NETDEV;
> > @@ -1407,11 +1449,12 @@ static int altera_tse_probe(struct
> > platform_device *pdev) if (IS_ERR(priv->phylink)) {
> > dev_err(&pdev->dev, "failed to create phylink\n");
> > ret = PTR_ERR(priv->phylink);
> > - goto err_init_phy;
> > + goto err_pcs;
>
> Does this leak priv->pcs ?
>
> > }
> >
> > return 0;
> > -
> > +err_pcs:
> > + mdio_device_free(priv->pcs_mdiodev);
> > err_init_phy:
> > unregister_netdev(ndev);
> > err_register_netdev:
> > @@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct
> > platform_device *pdev) altera_tse_mdio_destroy(ndev);
> > unregister_netdev(ndev);
> > phylink_destroy(priv->phylink);
> > + mdio_device_free(priv->pcs_mdiodev);
> > +
> > free_netdev(ndev);
> >
> > return 0;
> > diff --git a/include/linux/mdio/mdio-regmap.h
> > b/include/linux/mdio/mdio-regmap.h index b8508f152552..679d9069846b
> > 100644 --- a/include/linux/mdio/mdio-regmap.h
> > +++ b/include/linux/mdio/mdio-regmap.h
> > @@ -7,6 +7,8 @@
> > #ifndef MDIO_REGMAP_H
> > #define MDIO_REGMAP_H
> >
> > +#include <linux/phy.h>
> > +
> > struct device;
> > struct regmap;
> >
>
> This hunk doesn't seem strictly related to the patch.
> Perhaps the include belongs elsewhere.
> Or the hunk belongs in another patch.
Indeed, I had the same issue in another patch in this series. I'll
re-split everything correctly.
Thank you for the review,
Maxime
@@ -477,6 +477,7 @@ struct altera_tse_private {
struct phylink *phylink;
struct phylink_config phylink_config;
struct phylink_pcs *pcs;
+ struct mdio_device *pcs_mdiodev;
};
/* Function prototypes
@@ -27,14 +27,16 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mii.h>
+#include <linux/mdio/mdio-regmap.h>
#include <linux/netdevice.h>
#include <linux/of_device.h>
#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <linux/of_platform.h>
-#include <linux/pcs-altera-tse.h>
+#include <linux/pcs-lynx.h>
#include <linux/phy.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/skbuff.h>
#include <asm/cacheflush.h>
@@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct platform_device *pdev)
const struct of_device_id *of_id = NULL;
struct altera_tse_private *priv;
struct resource *control_port;
+ struct regmap *pcs_regmap;
struct resource *dma_res;
struct resource *pcs_res;
+ struct mii_bus *pcs_bus;
struct net_device *ndev;
void __iomem *descmap;
- int pcs_reg_width = 2;
int ret = -ENODEV;
+ struct regmap_config pcs_regmap_cfg;
+
+ struct mdio_regmap_config mrc = {
+ .parent = &pdev->dev,
+ .valid_addr = 0x0,
+ };
+
ndev = alloc_etherdev(sizeof(struct altera_tse_private));
if (!ndev) {
dev_err(&pdev->dev, "Could not allocate network device\n");
@@ -1258,10 +1268,29 @@ static int altera_tse_probe(struct platform_device *pdev)
ret = request_and_map(pdev, "pcs", &pcs_res,
&priv->pcs_base);
if (ret) {
+ /* If we can't find a dedicated resource for the PCS, fallback
+ * to the internal PCS, that has a different address stride
+ */
priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0);
- pcs_reg_width = 4;
+ pcs_regmap_cfg.reg_bits = 32;
+ /* Values are MDIO-like values, on 16 bits */
+ pcs_regmap_cfg.val_bits = 16;
+ pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2);
+ } else {
+ pcs_regmap_cfg.reg_bits = 16;
+ pcs_regmap_cfg.val_bits = 16;
+ pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
}
+ /* Create a regmap for the PCS so that it can be used by the PCS driver */
+ pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base,
+ &pcs_regmap_cfg);
+ if (IS_ERR(pcs_regmap)) {
+ ret = PTR_ERR(pcs_regmap);
+ goto err_free_netdev;
+ }
+ mrc.regmap = pcs_regmap;
+
/* Rx IRQ */
priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
if (priv->rx_irq == -ENXIO) {
@@ -1384,7 +1413,20 @@ static int altera_tse_probe(struct platform_device *pdev)
(unsigned long) control_port->start, priv->rx_irq,
priv->tx_irq);
- priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, pcs_reg_width);
+ snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
+ pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
+ if (IS_ERR(pcs_bus)) {
+ ret = PTR_ERR(pcs_bus);
+ goto err_init_phy;
+ }
+
+ priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0);
+
+ priv->pcs = lynx_pcs_create(priv->pcs_mdiodev);
+ if (!priv->pcs) {
+ ret = -ENODEV;
+ goto err_init_phy;
+ }
priv->phylink_config.dev = &ndev->dev;
priv->phylink_config.type = PHYLINK_NETDEV;
@@ -1407,11 +1449,12 @@ static int altera_tse_probe(struct platform_device *pdev)
if (IS_ERR(priv->phylink)) {
dev_err(&pdev->dev, "failed to create phylink\n");
ret = PTR_ERR(priv->phylink);
- goto err_init_phy;
+ goto err_pcs;
}
return 0;
-
+err_pcs:
+ mdio_device_free(priv->pcs_mdiodev);
err_init_phy:
unregister_netdev(ndev);
err_register_netdev:
@@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct platform_device *pdev)
altera_tse_mdio_destroy(ndev);
unregister_netdev(ndev);
phylink_destroy(priv->phylink);
+ mdio_device_free(priv->pcs_mdiodev);
+
free_netdev(ndev);
return 0;
@@ -7,6 +7,8 @@
#ifndef MDIO_REGMAP_H
#define MDIO_REGMAP_H
+#include <linux/phy.h>
+
struct device;
struct regmap;