On Fri, Dec 16, 2022 at 05:17:19PM +0100, Christian Marangi wrote:
> This reverts commit 2481d206fae7884cd07014fd1318e63af35e99eb.
>
> The Documentation is very confusing about the topic.
> The cache logic for hi and lo is wrong and actually miss some regs to be
> actually written.
>
> What the Docuemntation actually intended was that it's possible to skip
Just notice that I forgot to fix a typo here! Hope it's not a problem if
this can be fixed when merged. If it is I will happly send v2 if the
rest of the changes are OK.
> writing hi OR lo if half of the reg is not needed to be written or read.
>
> Revert the change in favor of a better and correct implementation.
>
> Reported-by: Ronald Wahl <ronald.wahl@xxxxxxxxxxx>
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v5.18+
> ---
> drivers/net/dsa/qca/qca8k-8xxx.c | 61 +++++++-------------------------
> drivers/net/dsa/qca/qca8k.h | 5 ---
> 2 files changed, 12 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 46151320b2a8..fbcd5c2b13ae 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -36,44 +36,6 @@ qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> *page = regaddr & 0x3ff;
> }
>
> -static int
> -qca8k_set_lo(struct qca8k_priv *priv, int phy_id, u32 regnum, u16 lo)
> -{
> - u16 *cached_lo = &priv->mdio_cache.lo;
> - struct mii_bus *bus = priv->bus;
> - int ret;
> -
> - if (lo == *cached_lo)
> - return 0;
> -
> - ret = bus->write(bus, phy_id, regnum, lo);
> - if (ret < 0)
> - dev_err_ratelimited(&bus->dev,
> - "failed to write qca8k 32bit lo register\n");
> -
> - *cached_lo = lo;
> - return 0;
> -}
> -
> -static int
> -qca8k_set_hi(struct qca8k_priv *priv, int phy_id, u32 regnum, u16 hi)
> -{
> - u16 *cached_hi = &priv->mdio_cache.hi;
> - struct mii_bus *bus = priv->bus;
> - int ret;
> -
> - if (hi == *cached_hi)
> - return 0;
> -
> - ret = bus->write(bus, phy_id, regnum, hi);
> - if (ret < 0)
> - dev_err_ratelimited(&bus->dev,
> - "failed to write qca8k 32bit hi register\n");
> -
> - *cached_hi = hi;
> - return 0;
> -}
> -
> static int
> qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
> {
> @@ -97,7 +59,7 @@ qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
> }
>
> static void
> -qca8k_mii_write32(struct qca8k_priv *priv, int phy_id, u32 regnum, u32 val)
> +qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
> {
> u16 lo, hi;
> int ret;
> @@ -105,9 +67,12 @@ qca8k_mii_write32(struct qca8k_priv *priv, int phy_id, u32 regnum, u32 val)
> lo = val & 0xffff;
> hi = (u16)(val >> 16);
>
> - ret = qca8k_set_lo(priv, phy_id, regnum, lo);
> + ret = bus->write(bus, phy_id, regnum, lo);
> if (ret >= 0)
> - ret = qca8k_set_hi(priv, phy_id, regnum + 1, hi);
> + ret = bus->write(bus, phy_id, regnum + 1, hi);
> + if (ret < 0)
> + dev_err_ratelimited(&bus->dev,
> + "failed to write qca8k 32bit register\n");
> }
>
> static int
> @@ -442,7 +407,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
> if (ret < 0)
> goto exit;
>
> - qca8k_mii_write32(priv, 0x10 | r2, r1, val);
> + qca8k_mii_write32(bus, 0x10 | r2, r1, val);
>
> exit:
> mutex_unlock(&bus->mdio_lock);
> @@ -475,7 +440,7 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
>
> val &= ~mask;
> val |= write_val;
> - qca8k_mii_write32(priv, 0x10 | r2, r1, val);
> + qca8k_mii_write32(bus, 0x10 | r2, r1, val);
>
> exit:
> mutex_unlock(&bus->mdio_lock);
> @@ -750,14 +715,14 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
> if (ret)
> goto exit;
>
> - qca8k_mii_write32(priv, 0x10 | r2, r1, val);
> + qca8k_mii_write32(bus, 0x10 | r2, r1, val);
>
> ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
> QCA8K_MDIO_MASTER_BUSY);
>
> exit:
> /* even if the busy_wait timeouts try to clear the MASTER_EN */
> - qca8k_mii_write32(priv, 0x10 | r2, r1, 0);
> + qca8k_mii_write32(bus, 0x10 | r2, r1, 0);
>
> mutex_unlock(&bus->mdio_lock);
>
> @@ -787,7 +752,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
> if (ret)
> goto exit;
>
> - qca8k_mii_write32(priv, 0x10 | r2, r1, val);
> + qca8k_mii_write32(bus, 0x10 | r2, r1, val);
>
> ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
> QCA8K_MDIO_MASTER_BUSY);
> @@ -798,7 +763,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
>
> exit:
> /* even if the busy_wait timeouts try to clear the MASTER_EN */
> - qca8k_mii_write32(priv, 0x10 | r2, r1, 0);
> + qca8k_mii_write32(bus, 0x10 | r2, r1, 0);
>
> mutex_unlock(&bus->mdio_lock);
>
> @@ -1968,8 +1933,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> }
>
> priv->mdio_cache.page = 0xffff;
> - priv->mdio_cache.lo = 0xffff;
> - priv->mdio_cache.hi = 0xffff;
>
> /* Check the detected switch id */
> ret = qca8k_read_switch_id(priv);
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index 0b7a5cb12321..03514f7a20be 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -375,11 +375,6 @@ struct qca8k_mdio_cache {
> * mdio writes
> */
> u16 page;
> -/* lo and hi can also be cached and from Documentation we can skip one
> - * extra mdio write if lo or hi is didn't change.
> - */
> - u16 lo;
> - u16 hi;
> };
>
> struct qca8k_pcs {
> --
> 2.37.2
>
--
Ansuel
