Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Rayagonda Kokatanur]

On Wed, Nov 4, 2020 at 9:05 AM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
>
>
>
> On 11/2/2020 10:19 PM, Dhananjay Phadke wrote:
> > On Mon,  2 Nov 2020 09:24:32 +0530, Rayagonda Kokatanur wrote:
> >
> >> Handle single or multi byte master read request with or without
> >> repeated start.
> >>
> >> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
> >> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@xxxxxxxxxxxx>
> >> ---
> >>  drivers/i2c/busses/i2c-bcm-iproc.c | 215 +++++++++++++++++++++++------
> >>  1 file changed, 170 insertions(+), 45 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> >> index 7a235f9f5884..22e04055b447 100644
> >> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> >> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> >> @@ -160,6 +160,11 @@
> >>
> >>  #define IE_S_ALL_INTERRUPT_SHIFT     21
> >>  #define IE_S_ALL_INTERRUPT_MASK      0x3f
> >> +/*
> >> + * It takes ~18us to reading 10bytes of data, hence to keep tasklet
> >> + * running for less time, max slave read per tasklet is set to 10 bytes.
> >> + */
> >> +#define MAX_SLAVE_RX_PER_INT         10
> >>
> >
> > In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> > however it's not actually used in processing rx events.
> >
> > Instead of hardcoding this threshold here, it's better to add a
> > device-tree knob for rx threshold, program it in controller and handle
> > that RX_THLD interrupt. This will give more flexibility to drain the rx
> > fifo earlier than -
> > (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> > (2) waiting for start of read transaction in case of master write-read.

Yes this is one way to implement.
But do you see any issue in batching 64 bytes at a time in case of
transaction > 64 Bytes.
I feel batching will be more efficient as it avoids more number of
interrupts and hence context switch.

>
> The Device Tree is really intended to describe the hardware FIFO size,
> not watermarks, as those tend to be more of a policy/work load decision.
> Maybe this is something that can be added as a module parameter, or
> configurable via ioctl() at some point.

#define MAX_SLAVE_RX_PER_INT         10

is not hw fifo threshold level, it is a kind of watermark for the
tasklet to process the max number of packets in single run.
The intention to add the macro is to make sure the tasklet does not
run more than 20us.
If we keep this as a module parameter or dt parameter then there is a
good possibility that the number can be set to higher value.
This will make the tasklet run more than 20us and defeat the purpose.

This number is constant and not variable to change

Please feel free to add your comments.

Best regards,
Rayagonda

> --
> Florian

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature