| Message ID | 20240705045030.1141934-5-c-vankar@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for Ethernet Boot on SK-AM62 | expand |
Hi Chintan, Vignesh, On Fri, 2024-07-05 at 10:20 +0530, Chintan Vankar wrote: > From: Vignesh Raghavendra <vigneshr@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@ti.com> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > Signed-off-by: Chintan Vankar <c-vankar@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? > + 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)
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@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@ti.com> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> Signed-off-by: Chintan Vankar <c-vankar@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? >> + 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) >
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@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@ti.com> > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > Signed-off-by: Chintan Vankar <c-vankar@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)
On 23/08/24 16:03, Sverdlin, Alexander wrote: > 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@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@ti.com> >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> Signed-off-by: Chintan Vankar <c-vankar@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. >>>> + 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) >
Hi Chintan, On Mon, 2024-08-26 at 09:24 +0530, Chintan Vankar wrote: > > > > > From: Vignesh Raghavendra <vigneshr@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@ti.com> > > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > > > Signed-off-by: Chintan Vankar <c-vankar@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!
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; + 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)