Hi Chintan, On Mon, 2024-08-26 at 09:24 +0530, Chintan Vankar wrote: > > > > > From: Vignesh Raghavendra <vigne...@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 <vigne...@ti.com> > > > > > Signed-off-by: Siddharth Vadapalli <s-vadapa...@ti.com> > > > > > Signed-off-by: Chintan Vankar <c-van...@ti.com> > > > > > --- > > > > > > > > > > Link to v2: > > > > > https://lore.kernel.org/r/20240425120822.2048012-5-c-van...@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
