Messages in this thread | | | Date | Tue, 5 Sep 2023 14:06:05 +0530 | Subject | Re: [RFC PATCH net-next 1/4] net: ti: icssg-prueth: Add helper functions to configure FDB | From | MD Danish Anwar <> |
| |
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
| |