Re: [net PATCH 3/5] Revert "net: dsa: qca8k: cache lo and hi for mdio write"

[Christian Marangi]

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