diff mbox series

[net-next,v11,6/9] net: mdio: Add Airoha AN8855 Switch MDIO Passtrough

Message ID 20241209134459.27110-7-ansuelsmth@gmail.com (mailing list archive)
State New
Headers show
Series net: dsa: Add Airoha AN8855 support | expand

Commit Message

Christian Marangi Dec. 9, 2024, 1:44 p.m. UTC 
Add Airoha AN8855 Switch driver to register a MDIO passtrough as switch
address is shared with the internal PHYs and require additional page
handling.

This requires the upper Switch MFD to be probed and init to actually
work.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 MAINTAINERS                    |   1 +
 drivers/net/mdio/Kconfig       |   9 +++
 drivers/net/mdio/Makefile      |   1 +
 drivers/net/mdio/mdio-an8855.c | 113 +++++++++++++++++++++++++++++++++
 4 files changed, 124 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-an8855.c

Comments

Andrew Lunn Dec. 10, 2024, 1:53 a.m. UTC | #1 
> +static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
> +				  int phy) __must_hold(&priv->bus->mdio_lock)
> +{
> +	/* Check PHY page only for addr shared with switch */
> +	if (phy != priv->switch_addr)
> +		return 0;
> +
> +	/* Don't restore page if it's not set to switch page */
> +	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
> +					    AN8855_PHY_PAGE_EXTENDED_4))
> +		return 0;
> +
> +	/* Restore page to 0, PHY might change page right after but that
> +	 * will be ignored as it won't be a switch page.
> +	 */
> +	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
> +}

I don't really understand what is going on here. Maybe the commit
message needs expanding, or the function names changing.

Generally, i would expect a save/restore action. Save the current
page, swap to the PHY page, do the PHY access, and then restore to the
saved page.

	Andrew
Christian Marangi Dec. 10, 2024, 12:06 p.m. UTC | #2 
On Tue, Dec 10, 2024 at 02:53:34AM +0100, Andrew Lunn wrote:
> > +static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
> > +				  int phy) __must_hold(&priv->bus->mdio_lock)
> > +{
> > +	/* Check PHY page only for addr shared with switch */
> > +	if (phy != priv->switch_addr)
> > +		return 0;
> > +
> > +	/* Don't restore page if it's not set to switch page */
> > +	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
> > +					    AN8855_PHY_PAGE_EXTENDED_4))
> > +		return 0;
> > +
> > +	/* Restore page to 0, PHY might change page right after but that
> > +	 * will be ignored as it won't be a switch page.
> > +	 */
> > +	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
> > +}
> 
> I don't really understand what is going on here. Maybe the commit
> message needs expanding, or the function names changing.
> 
> Generally, i would expect a save/restore action. Save the current
> page, swap to the PHY page, do the PHY access, and then restore to the
> saved page.
>

Idea is to save on extra read/write on subsequent write on the same
page.

Idea here is that PHY will receive most of the read (for status
poll) hence in 90% of the time page will be 0.

And switch will receive read/write only on setup or fdb/vlan access on
configuration so it will receive subsequent write on the same page.
(page 4)

PHY might also need to write on page 1 on setup but never on page 4 as
that is reserved for switch.

Making the read/swap/write/restore adds 2 additional operation that can
really be skipped 90% of the time.

Also curret_page cache is indirectly protected by the mdio lock.

So in short this function makes sure PHY for port 0 is configured on the
right page based on the prev page set.

An alternative way might be assume PHY is always on page 0 and any
switch operation save and restore the page.

Hope it's clear now why this is needed. Is this ok or you prefer the
alternative way?
Andrew Lunn Dec. 10, 2024, 1:38 p.m. UTC | #3 
On Tue, Dec 10, 2024 at 01:06:29PM +0100, Christian Marangi wrote:
> On Tue, Dec 10, 2024 at 02:53:34AM +0100, Andrew Lunn wrote:
> > > +static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
> > > +				  int phy) __must_hold(&priv->bus->mdio_lock)
> > > +{
> > > +	/* Check PHY page only for addr shared with switch */
> > > +	if (phy != priv->switch_addr)
> > > +		return 0;
> > > +
> > > +	/* Don't restore page if it's not set to switch page */
> > > +	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
> > > +					    AN8855_PHY_PAGE_EXTENDED_4))
> > > +		return 0;
> > > +
> > > +	/* Restore page to 0, PHY might change page right after but that
> > > +	 * will be ignored as it won't be a switch page.
> > > +	 */
> > > +	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
> > > +}
> > 
> > I don't really understand what is going on here. Maybe the commit
> > message needs expanding, or the function names changing.
> > 
> > Generally, i would expect a save/restore action. Save the current
> > page, swap to the PHY page, do the PHY access, and then restore to the
> > saved page.
> >
> 
> Idea is to save on extra read/write on subsequent write on the same
> page.
> 
> Idea here is that PHY will receive most of the read (for status
> poll) hence in 90% of the time page will be 0.
> 
> And switch will receive read/write only on setup or fdb/vlan access on
> configuration so it will receive subsequent write on the same page.
> (page 4)
> 
> PHY might also need to write on page 1 on setup but never on page 4 as
> that is reserved for switch.
> 
> Making the read/swap/write/restore adds 2 additional operation that can
> really be skipped 90% of the time.
> 
> Also curret_page cache is indirectly protected by the mdio lock.
> 
> So in short this function makes sure PHY for port 0 is configured on the
> right page based on the prev page set.
> 
> An alternative way might be assume PHY is always on page 0 and any
> switch operation save and restore the page.
> 
> Hope it's clear now why this is needed. Is this ok or you prefer the
> alternative way? 

Please think about naming. I'm not sure _restore_ is the correct name
for the function. Maybe select. And i suggest adding some comments to
explain what is going on, since it is not obvious from the code in
this file, you probably need to cross reference it with code in the
DSA driver as well.

	Andrew
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 7f4d7c48b6e1..38c7b2362c92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -722,6 +722,7 @@  F:	Documentation/devicetree/bindings/net/airoha,an8855-mdio.yaml
 F:	Documentation/devicetree/bindings/net/dsa/airoha,an8855-switch.yaml
 F:	Documentation/devicetree/bindings/nvmem/airoha,an8855-efuse.yaml
 F:	drivers/mfd/airoha-an8855.c
+F:	drivers/net/mdio/mdio-an8855.c
 
 AIROHA ETHERNET DRIVER
 M:	Lorenzo Bianconi <lorenzo@kernel.org>
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 4a7a303be2f7..3d417948e73a 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -61,6 +61,15 @@  config MDIO_XGENE
 	  This module provides a driver for the MDIO busses found in the
 	  APM X-Gene SoC's.
 
+config MDIO_AN8855
+	tristate "Airoha AN8855 Switch MDIO bus controller"
+	depends on MFD_AIROHA_AN8855
+	depends on OF_MDIO
+	help
+	  This module provides a driver for the Airoha AN8855 Switch
+	  that requires a MDIO passtrough as switch address is shared
+	  with the internal PHYs and requires additional page handling.
+
 config MDIO_ASPEED
 	tristate "ASPEED MDIO bus controller"
 	depends on ARCH_ASPEED || COMPILE_TEST
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 1015f0db4531..546c4e55b475 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -5,6 +5,7 @@  obj-$(CONFIG_ACPI_MDIO)		+= acpi_mdio.o
 obj-$(CONFIG_FWNODE_MDIO)	+= fwnode_mdio.o
 obj-$(CONFIG_OF_MDIO)		+= of_mdio.o
 
+obj-$(CONFIG_MDIO_AN8855)		+= mdio-an8855.o
 obj-$(CONFIG_MDIO_ASPEED)		+= mdio-aspeed.o
 obj-$(CONFIG_MDIO_BCM_IPROC)		+= mdio-bcm-iproc.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)		+= mdio-bcm-unimac.o
diff --git a/drivers/net/mdio/mdio-an8855.c b/drivers/net/mdio/mdio-an8855.c
new file mode 100644
index 000000000000..5feba72c021b
--- /dev/null
+++ b/drivers/net/mdio/mdio-an8855.c
@@ -0,0 +1,113 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MDIO passthrough driver for Airoha AN8855 Switch
+ */
+
+#include <linux/mfd/airoha-an8855-mfd.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/platform_device.h>
+
+static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
+				  int phy) __must_hold(&priv->bus->mdio_lock)
+{
+	/* Check PHY page only for addr shared with switch */
+	if (phy != priv->switch_addr)
+		return 0;
+
+	/* Don't restore page if it's not set to switch page */
+	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
+					    AN8855_PHY_PAGE_EXTENDED_4))
+		return 0;
+
+	/* Restore page to 0, PHY might change page right after but that
+	 * will be ignored as it won't be a switch page.
+	 */
+	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
+}
+
+static int an8855_phy_read(struct mii_bus *bus, int phy, int regnum)
+{
+	struct an8855_mfd_priv *priv = bus->priv;
+	struct mii_bus *real_bus = priv->bus;
+	int ret;
+
+	mutex_lock_nested(&real_bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	ret = an855_phy_restore_page(priv, phy);
+	if (ret)
+		goto exit;
+
+	ret = __mdiobus_read(real_bus, phy, regnum);
+exit:
+	mutex_unlock(&real_bus->mdio_lock);
+
+	return ret;
+}
+
+static int an8855_phy_write(struct mii_bus *bus, int phy, int regnum, u16 val)
+{
+	struct an8855_mfd_priv *priv = bus->priv;
+	struct mii_bus *real_bus = priv->bus;
+	int ret;
+
+	mutex_lock_nested(&real_bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	ret = an855_phy_restore_page(priv, phy);
+	if (ret)
+		goto exit;
+
+	ret = __mdiobus_write(real_bus, phy, regnum, val);
+exit:
+	mutex_unlock(&real_bus->mdio_lock);
+
+	return ret;
+}
+
+static int an8855_mdio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct an8855_mfd_priv *priv;
+	struct mii_bus *bus;
+	int ret;
+
+	/* Get priv of MFD */
+	priv = dev_get_drvdata(dev->parent);
+
+	bus = devm_mdiobus_alloc(dev);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->priv = priv;
+	bus->name = KBUILD_MODNAME "-mii";
+	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d",
+		 priv->switch_addr);
+	bus->parent = dev;
+	bus->read = an8855_phy_read;
+	bus->write = an8855_phy_write;
+
+	ret = devm_of_mdiobus_register(dev, bus, dev->of_node);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register MDIO bus\n");
+
+	return ret;
+}
+
+static const struct of_device_id an8855_mdio_of_match[] = {
+	{ .compatible = "airoha,an8855-mdio", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, an8855_mdio_of_match);
+
+static struct platform_driver an8855_mdio_driver = {
+	.probe	= an8855_mdio_probe,
+	.driver = {
+		.name = "an8855-mdio",
+		.of_match_table = an8855_mdio_of_match,
+	},
+};
+module_platform_driver(an8855_mdio_driver);
+
+MODULE_AUTHOR("Christian Marangi <ansuelsmth@gmail.com>");
+MODULE_DESCRIPTION("Driver for AN8855 MDIO passthrough");
+MODULE_LICENSE("GPL");