2022-04-16 01:23:30

by Christian Marangi

[permalink] [raw]
Subject: [net-next PATCH v3 0/6] Reduce qca8k_priv space usage

These 6 patch is a first attempt at reducting qca8k_priv space.
The code changed a lot during times and we have many old logic
that can be replaced with new implementation

The first patch drop the tracking of MTU. We mimic what was done
for mtk and we change MTU only when CPU port is changed.

The second patch finally drop a piece of story of this driver.
The ar8xxx_port_status struct was used by the first implementation
of this driver to put all sort of status data for the port...
With the evolution of DSA all that stuff got dropped till only
the enabled state was the only part of the that struct.
Since it's overkill to keep an array of int, we convert the variable
to a simple u8 where we store the status of each port. This is needed
to don't reanable ports on system resume.

The third patch is a preparation for patch 4. As Vladimir explained
in another patch, we waste a tons of space by keeping a duplicate of
the switch dsa ops in qca8k_priv. The only reason for this is to
dynamically set the correct mdiobus configuration (a legacy dsa one,
or a custom dedicated one)
To solve this problem, we just drop the phy_read/phy_write and we
declare a custom mdiobus in any case.
This way we can use a static dsa switch ops struct and we can drop it
from qca8k_priv

Patch 4 drop the duplicated dsa_switch_ops.

Patch 5 is a fixup for mdio read error.

Patch 6 is an effort to standardize how bus name are done.

This series is just a start of more cleanup.

The idea is to move this driver to the qca dir and split common code
from specific code. Also the mgmt eth code still requires some love
and can totally be optimized by recycling the same skb over time.

Also while working on the MTU it was notice some problem with
the stmmac driver and with the reloading phase that cause all
sort of problems with qca8k.

I'm sending this here just to try to keep small series instead of
proposing monster series hard to review.

v2:
- Rework MTU patch
v3:
- Drop unrealated changes from patch 3
- Add fixup patch for mdio read
- Unify bus name for legacy and OF mdio bus

*** BLURB HERE ***

Ansuel Smith (6):
net: dsa: qca8k: drop MTU tracking from qca8k_priv
net: dsa: qca8k: drop port_sts from qca8k_priv
net: dsa: qca8k: rework and simplify mdiobus logic
net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv
net: dsa: qca8k: correctly handle mdio read error
net: dsa: qca8k: unify bus id naming with legacy and OF mdio bus

drivers/net/dsa/qca8k.c | 145 +++++++++++++++-------------------------
drivers/net/dsa/qca8k.h | 12 ++--
2 files changed, 57 insertions(+), 100 deletions(-)

--
2.34.1


2022-04-16 01:33:32

by Christian Marangi

[permalink] [raw]
Subject: [net-next PATCH v3 3/6] net: dsa: qca8k: rework and simplify mdiobus logic

In an attempt to reduce qca8k_priv space, rework and simplify mdiobus
logic.
We now declare a mdiobus instead of relying on DSA phy_read/write even
if a mdio node is not present. This is all to make the qca8k ops static
and not switch specific. With a legacy implementation where port doesn't
have a phy map declared in the dts with a mdio node, we declare a
'qca8k-legacy' mdiobus. The conversion logic is used as legacy read and
write ops are used instead of the internal one.
Also drop the legacy_phy_port_mapping as we now declare mdiobus with ops
that already address the workaround.

Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 95 +++++++++++++----------------------------
drivers/net/dsa/qca8k.h | 1 -
2 files changed, 29 insertions(+), 67 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 766db0d43092..24d57083ee2c 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1291,83 +1291,63 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
}

static int
-qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
+qca8k_legacy_mdio_write(struct mii_bus *slave_bus, int port, int regnum, u16 data)
{
- struct qca8k_priv *priv = ds->priv;
- int ret;
-
- /* Check if the legacy mapping should be used and the
- * port is not correctly mapped to the right PHY in the
- * devicetree
- */
- if (priv->legacy_phy_port_mapping)
- port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+ port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;

- /* Use mdio Ethernet when available, fallback to legacy one on error */
- ret = qca8k_phy_eth_command(priv, false, port, regnum, 0);
- if (!ret)
- return ret;
-
- return qca8k_mdio_write(priv, port, regnum, data);
+ return qca8k_internal_mdio_write(slave_bus, port, regnum, data);
}

static int
-qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
+qca8k_legacy_mdio_read(struct mii_bus *slave_bus, int port, int regnum)
{
- struct qca8k_priv *priv = ds->priv;
- int ret;
+ port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;

- /* Check if the legacy mapping should be used and the
- * port is not correctly mapped to the right PHY in the
- * devicetree
- */
- if (priv->legacy_phy_port_mapping)
- port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
-
- /* Use mdio Ethernet when available, fallback to legacy one on error */
- ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
- if (ret >= 0)
- return ret;
-
- ret = qca8k_mdio_read(priv, port, regnum);
-
- if (ret < 0)
- return 0xffff;
-
- return ret;
+ return qca8k_internal_mdio_read(slave_bus, port, regnum);
}

static int
-qca8k_mdio_register(struct qca8k_priv *priv, struct device_node *mdio)
+qca8k_mdio_register(struct qca8k_priv *priv)
{
struct dsa_switch *ds = priv->ds;
+ struct device_node *mdio;
struct mii_bus *bus;

bus = devm_mdiobus_alloc(ds->dev);
-
if (!bus)
return -ENOMEM;

bus->priv = (void *)priv;
- bus->name = "qca8k slave mii";
- bus->read = qca8k_internal_mdio_read;
- bus->write = qca8k_internal_mdio_write;
- snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d",
- ds->index);
-
bus->parent = ds->dev;
bus->phy_mask = ~ds->phys_mii_mask;
-
ds->slave_mii_bus = bus;

- return devm_of_mdiobus_register(priv->dev, bus, mdio);
+ /* Check if the devicetree declare the port:phy mapping */
+ mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
+ if (of_device_is_available(mdio)) {
+ snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d", ds->index);
+ bus->name = "qca8k slave mii";
+ bus->read = qca8k_internal_mdio_read;
+ bus->write = qca8k_internal_mdio_write;
+ return devm_of_mdiobus_register(priv->dev, bus, mdio);
+ }
+
+ /* If a mapping can't be found the legacy mapping is used,
+ * using the qca8k_port_to_phy function
+ */
+ snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d.%d",
+ ds->dst->index, ds->index);
+ bus->name = "qca8k-legacy slave mii";
+ bus->read = qca8k_legacy_mdio_read;
+ bus->write = qca8k_legacy_mdio_write;
+ return devm_mdiobus_register(priv->dev, bus);
}

static int
qca8k_setup_mdio_bus(struct qca8k_priv *priv)
{
u32 internal_mdio_mask = 0, external_mdio_mask = 0, reg;
- struct device_node *ports, *port, *mdio;
+ struct device_node *ports, *port;
phy_interface_t mode;
int err;

@@ -1429,24 +1409,7 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
QCA8K_MDIO_MASTER_EN);
}

- /* Check if the devicetree declare the port:phy mapping */
- mdio = of_get_child_by_name(priv->dev->of_node, "mdio");
- if (of_device_is_available(mdio)) {
- err = qca8k_mdio_register(priv, mdio);
- if (err)
- of_node_put(mdio);
-
- return err;
- }
-
- /* If a mapping can't be found the legacy mapping is used,
- * using the qca8k_port_to_phy function
- */
- priv->legacy_phy_port_mapping = true;
- priv->ops.phy_read = qca8k_phy_read;
- priv->ops.phy_write = qca8k_phy_write;
-
- return 0;
+ return qca8k_mdio_register(priv);
}

static int
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 12d8d090298b..8bbe36f135b5 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -388,7 +388,6 @@ struct qca8k_priv {
* Bit 1: port enabled. Bit 0: port disabled.
*/
u8 port_enabled_map;
- bool legacy_phy_port_mapping;
struct qca8k_ports_config ports_config;
struct regmap *regmap;
struct mii_bus *bus;
--
2.34.1

2022-04-18 10:42:22

by patchwork-bot+netdevbpf

[permalink] [raw]
Subject: Re: [net-next PATCH v3 0/6] Reduce qca8k_priv space usage

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <[email protected]>:

On Sat, 16 Apr 2022 01:30:11 +0200 you wrote:
> These 6 patch is a first attempt at reducting qca8k_priv space.
> The code changed a lot during times and we have many old logic
> that can be replaced with new implementation
>
> The first patch drop the tracking of MTU. We mimic what was done
> for mtk and we change MTU only when CPU port is changed.
>
> [...]

Here is the summary with links:
- [net-next,v3,1/6] net: dsa: qca8k: drop MTU tracking from qca8k_priv
https://git.kernel.org/netdev/net-next/c/69fd055957a0
- [net-next,v3,2/6] net: dsa: qca8k: drop port_sts from qca8k_priv
https://git.kernel.org/netdev/net-next/c/2b8fd87af7f1
- [net-next,v3,3/6] net: dsa: qca8k: rework and simplify mdiobus logic
https://git.kernel.org/netdev/net-next/c/8255212e4130
- [net-next,v3,4/6] net: dsa: qca8k: drop dsa_switch_ops from qca8k_priv
https://git.kernel.org/netdev/net-next/c/2349b83a2486
- [net-next,v3,5/6] net: dsa: qca8k: correctly handle mdio read error
https://git.kernel.org/netdev/net-next/c/6cfc03b60220
- [net-next,v3,6/6] net: dsa: qca8k: unify bus id naming with legacy and OF mdio bus
https://git.kernel.org/netdev/net-next/c/8d1af50842bf

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html