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

[Roger Quadros <rogerq@kernel.org>]

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

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

-- 
cheers,
-roger