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

[Ray Jui <ray.jui@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]



On 11/3/2020 7:57 PM, Rayagonda Kokatanur wrote:
> On Wed, Nov 4, 2020 at 9:05 AM Florian Fainelli <f.fainelli@gmail.com> 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@broadcom.com>
>>>> ---
>>>>  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.
> 

Yes, DT can have properties to describe the FIFO size, if there happens
to be some variants in the HW blocks in different versions. But that is
not the case here. DT should not be used to control SW/use case specific
behavior.


> #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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]