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

[Chintan Vankar]

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 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.


>>>> +	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)
>