[U-Boot] [PATCH] spi: cadence_qspi_apb: Improve indirect write transaction

[Marek Vasut]

On 10/19/2016 05:19 PM, Jagan Teki wrote:
> On Wed, Oct 19, 2016 at 8:18 PM, Marek Vasut <marex at denx.de> wrote:
>> On 10/19/2016 04:41 PM, Jagan Teki wrote:
>>> On Wed, Oct 19, 2016 at 10:10 AM, Vignesh R <vigneshr at ti.com> wrote:
>>>> Hi,
>>>>
>>>> On Tuesday 18 October 2016 05:15 PM, Marek Vasut wrote:
>>>>> On 10/18/2016 10:23 AM, Vignesh R wrote:
>>>>>>
>>>>>>
>>>>>> On Thursday 06 October 2016 04:49 PM, Vignesh R wrote:
>>>>>>> If the write transaction size(write_bytes) is not a multiple of word
>>>>>>> length, then issue word length writes till the we reach the dangling
>>>>>>> bytes. On the final write, issue byte by byte write to complete the
>>>>>>> transaction. This marginally improves write throughput when performing
>>>>>>> random sized writes to the flash.
>>>>>>>
>>>>>>> Signed-off-by: Vignesh R <vigneshr at ti.com>
>>>>>>> ---
>>>>>>
>>>>>> Gentle ping... Any comments?
>>>>>>
>>>>>>>
>>>>>>> Tested on K2G GP EVM.
>>>>>>>
>>>>>>>  drivers/spi/cadence_qspi_apb.c | 10 ++++++++--
>>>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>>>>>>> index e285d3c1e761..4b891f227243 100644
>>>>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>>>>> @@ -752,10 +752,16 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
>>>>>>>     while (remaining > 0) {
>>>>>>>             write_bytes = remaining > page_size ? page_size : remaining;
>>>>>>>             /* Handle non-4-byte aligned access to avoid data abort. */
>>>>>>> -           if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
>>>>>>> +           if ((uintptr_t)txbuf % 4) {
>>>>>>>                     writesb(plat->ahbbase, txbuf, write_bytes);
>>>>>>> -           else
>>>>>>> +           } else {
>>>>>>>                     writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>>>>>>> +                   if (write_bytes % 4) {
>>>>>>> +                           writesb(plat->ahbbase,
>>>>>>> +                                   txbuf + rounddown(write_bytes, 4),
>>>>>>> +                                   write_bytes % 4);
>>>>>>> +                   }
>>>>>
>>>>> You can probably pull this block from the else branch.
>>>>
>>>> Yeah, I guess writesb() can handle zero byte write request I believe.
>>>>
>>>> With above change, can I have your Acked-by/Reviewed-by?
>>>
>>> Also try to get the 'sf update' data before and after and append it on
>>> commit message.
>>
>> Why? Seems useless to me.
> 
> Since it's a performance improvement patch better to have that
> numbers, no harm getting that data.

Urghhhh, sf update is mixing multiple access patterns, it is by no means
a good performance metric for evaluating performance of the
write path.

What you would need to do here is perform long unaligned writes
repeatedly (to eliminate outliers) and measure the improvement.
And you'd have to make sure the erase cycle is not counted in.

I suspect the performance improvement would be negligible, but
I'd be happy to be proven wrong. If it'd be negligible, then
we should probably not complicate the code more and just drop
this patch.
-- 
Best regards,
Marek Vasut