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

[Sverdlin, Alexander]

Hi Chintan,

On Mon, 2024-08-26 at 09:24 +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?
> > 
> 
> Hello Alexander, Thank you for asking above questions, I got your point.
> You are correct that we need to "AND" negation of
> KNAV_RINGACC_CFG_RING_SIZE_MASK with reg.
> 
> By doing so we can avoid clearing out fields other than size, and also
> at the same time it will make sure to clear size field in
> ring->cfg->size and later we can "OR" it with requested size to not
> change other field in the reg and only update the size field.
> 
> I hope this is what you wanted to say at your very first comment.

Yes, I think now we have common understanding!
Thanks for looking into this!

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com