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

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

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.

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