Re: [EXTERNAL] Re: [RFC PATCH net-next 1/4] net: ti: icssg-prueth: Add helper functions to configure FDB

[MD Danish Anwar <danishanwar@ti.com>]

On 07/09/23 17:27, Roger Quadros wrote:
> 
> 
> On 05/09/2023 11:36, MD Danish Anwar wrote:
>> Hi Andrew
>>
>> On 04/09/23 19:32, Andrew Lunn wrote:
>>>> +int icssg_send_fdb_msg(struct prueth_emac *emac, struct mgmt_cmd *cmd,
>>>> +		       struct mgmt_cmd_rsp *rsp)
>>>> +{
>>>> +	struct prueth *prueth = emac->prueth;
>>>> +	int slice = prueth_emac_slice(emac);
>>>> +	int i = 10000;
>>>> +	int addr;
>>>> +
>>>> +	addr = icssg_queue_pop(prueth, slice == 0 ?
>>>> +			       ICSSG_CMD_POP_SLICE0 : ICSSG_CMD_POP_SLICE1);
>>>> +	if (addr < 0)
>>>> +		return addr;
>>>> +
>>>> +	/* First 4 bytes have FW owned buffer linking info which should
>>>> +	 * not be touched
>>>> +	 */
>>>> +	memcpy_toio(prueth->shram.va + addr + 4, cmd, sizeof(*cmd));
>>>> +	icssg_queue_push(prueth, slice == 0 ?
>>>> +			 ICSSG_CMD_PUSH_SLICE0 : ICSSG_CMD_PUSH_SLICE1, addr);
>>>> +	while (i--) {
>>>> +		addr = icssg_queue_pop(prueth, slice == 0 ?
>>>> +				       ICSSG_RSP_POP_SLICE0 : ICSSG_RSP_POP_SLICE1);
>>>> +		if (addr < 0) {
>>>> +			usleep_range(1000, 2000);
>>>> +			continue;
>>>> +		}
>>>
>>> Please try to make use of include/linux/iopoll.h.
>>>
>>
>> I don't think APIs from iopoll.h will be useful here.
>> readl_poll_timeout() periodically polls an address until a condition is
>> met or a timeout occurs. It takes address, condition as argument and
>> store the value read from the address in val.
> 
> You need to use read_poll_timeout() and provide the read function as
> first argument 'op'. The arguments to the read function can be passed as is
> at the end. Please read description of read_poll_timeout()
> 

I was only looking into real/b/w_poll_timeout() as it is mentioned in
iopoll.h to not use read_poll_timeout() directly. Anyways, I will use
read_poll_timeout() here with icssg_pop_queue() as a read API.

>>
>> Here in our use case we need to continuously read the value returned
>> from icssg_queue_pop() and check if that is valid or not. If it's not
>> valid, we keep polling until timeout happens.
>>
>> icssg_queue_pop() does two read operations. It checks if the queue
>> number is valid or not. Then it reads the ICSSG_QUEUE_CNT_OFFSET for
>> that queue, if the value read is zero it returns inval. After that it
>> reads the value from ICSSG_QUEUE_OFFSET of that queue and store it in
>> 'val'. The returned value from icssg_queue_pop() is checked
>> continuously, if it's an error code, we keep polling. If it's a good
>> value then we call icssg_queue_push() with that value. As you can see
>> from the below definition of icssg_queue_pop() we are doing two reads
>> and two checks for error. I don't think this can be achieved by using
>> APIs in iopoll.h. readl_poll_timeout() reads from a single address
>> directly but we don't ave a single address that we can pass to
>> readl_poll_timeout() as an argument as we have to do two reads from two
>> different addresses during each poll.
>>
>> So I don't think we can use iopoll.h here. Please let me know if this
>> looks ok to you or if there is any other way we can use iopoll.h here
>>
>> int icssg_queue_pop(struct prueth *prueth, u8 queue)
>> {
>>     u32 val, cnt;
>>
>>     if (queue >= ICSSG_QUEUES_MAX)
>> 	return -EINVAL;
>>
>>     regmap_read(prueth->miig_rt, ICSSG_QUEUE_CNT_OFFSET + 4*queue,&cnt);
>>     if (!cnt)
>> 	return -EINVAL;
>>
>>     regmap_read(prueth->miig_rt, ICSSG_QUEUE_OFFSET + 4 * queue, &val);
>>
>>     return val;
>> }
>>
>>>> +	if (i <= 0) {
>>>> +		netdev_err(emac->ndev, "Timedout sending HWQ message\n");
>>>> +		return -EINVAL;
>>>
>>> Using iopoll.h will fix this, but -ETIMEDOUT, not -EINVAL.
>>>
>>
>> -ETIMEDOUT is actually a better suited error code here, I will change
>> -EINVAL to -ETIMEDOUT in this if check.
>>
>>>       Andrew
>>>
>>
> 

-- 
Thanks and Regards,
Danish