[PATCH v3 04/10] soc: ti: k3-navss-ringacc: Fix reset ring API

[Sverdlin, Alexander]

Hi Chintan,

On Fri, 2024-08-23 at 15:22 +0530, Chintan Vankar wrote:
> On 16/08/24 17:58, Sverdlin, Alexander wrote:
> > Hi Chintan, Vignesh,
> > 
> > On Fri, 2024-07-05 at 10:20 +0530, Chintan Vankar wrote:
> > > From: Vignesh Raghavendra <vigneshr at ti.com>
> > > 
> > > Expectation of k3_ringacc_ring_reset_raw() is to reset the ring to
> > > requested size and not to 0. Fix this.
> > > 
> > > Signed-off-by: Vignesh Raghavendra <vigneshr at ti.com>
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli at ti.com>
> > > Signed-off-by: Chintan Vankar <c-vankar at ti.com>
> > > ---
> > > 
> > > Link to v2:
> > > https://lore.kernel.org/r/20240425120822.2048012-5-c-vankar@ti.com/
> > > 
> > > Changes from v2 to v3:
> > > - No changes.
> > > 
> > >    drivers/soc/ti/k3-navss-ringacc-u-boot.c | 9 ++++++++-
> > >    1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soc/ti/k3-navss-ringacc-u-boot.c b/drivers/soc/ti/k3-navss-ringacc-u-boot.c
> > > index f958239c2a..5d650b9de7 100644
> > > --- a/drivers/soc/ti/k3-navss-ringacc-u-boot.c
> > > +++ b/drivers/soc/ti/k3-navss-ringacc-u-boot.c
> > > @@ -25,9 +25,16 @@ struct k3_nav_ring_cfg_regs {
> > >    #define KNAV_RINGACC_CFG_RING_SIZE_ELSIZE_MASK		GENMASK(26, 24)
> > >    #define KNAV_RINGACC_CFG_RING_SIZE_ELSIZE_SHIFT		(24)
> > >    
> > > +#define KNAV_RINGACC_CFG_RING_SIZE_MASK			GENMASK(15, 0)
> > > +
> > >    static void k3_ringacc_ring_reset_raw(struct k3_nav_ring *ring)
> > >    {
> > > -	writel(0, &ring->cfg->size);
> > > +	u32 reg;
> > > +
> > > +	reg = readl(&ring->cfg->size);
> > > +	reg &= KNAV_RINGACC_CFG_RING_SIZE_MASK;
> > 
> > should it actually be "reg &= ~KNAV_RINGACC_CFG_RING_SIZE_MASK;"?
> > Otherwise you could potentially bump the size to some number higher than ring->size
> > if you "OR" ring->size with something lower than ring->size and ring->size is not
> > N^2-1?
> > 
> 
> Hello Alexander, I didn't get your comment, can you explain in more
> detail?

What is the exact purpose of "reg &= KNAV_RINGACC_CFG_RING_SIZE_MASK;"?
Which corner case is handled by it?

What happens if ring->cfg->size contains "1" and ring->size contains "2"?
What will be written into ring->cfg->size?

> > > +	reg |= ring->size;
> > > +	writel(reg, &ring->cfg->size);
> > >    }
> > >    
> > >    static void k3_ringacc_ring_reconfig_qmode_raw(struct k3_nav_ring *ring, enum k3_nav_ring_mode mode)

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com