Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

Am 09.06.2016 um 09:12 schrieb Michal Suchanek:
> On 8 June 2016 at 21:51, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>> Am 07.06.2016 um 08:03 schrieb Heiner Kallweit:
>>> Am 07.06.2016 um 01:07 schrieb Mark Brown:
>>>> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>>>>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>>>
>>>>>> That doesn't make any sense, the controller hardware doesn't
>>>>>> magically change based on what is connected to it.
>>>>
>>>>> The issue with fsl-espi is that the controller deactivates CS after
>>>>> each physical transfer. And a lot of HW designs use the hardware CS,
>>>>> therefore the advise to use a GPIO instead doesn't really help.
>>>>
>>>> There's no pinmux to fix the broken behaviour?  The Freescale SPI
>>>> controllers really are terrible :(
>>>>
>>>>> In case of SPI NOR CS needs to stay active between command and data
>>>>
>>>> In the case of *all* SPI transactions this needs to happen.  This is
>>>> *not* optional, it's not some weird requirement of NOR, it's one of the
>>>> most basic requirements for a SPI driver on Linux.
>>>>
>>>>> Well, we could reduce the max_transfer_size exposed by the driver by
>>>>> the max length of a m25p80 read command in general.
>>>>
>>>> No!  Apart from anything else this would be a complete and total
>>>> abstraction failure.  This is a driver for a SPI controller, not a
>>>> driver for a flash controller, which means that it should not have flash
>>>> specific hacks in it.
>>>>
>>>>> Then we might be too strict for other SPI devices, but this may
>>>>> be acceptable as the current fsl-espi driver is usable with SPI NOR
>>>>> devices only anyway.
>>>>
>>>> What makes you say this?  I'm guessing that the driver is just broken
>>>> but it would be good to understand the specifics.
>>>>
>>> See fsl_espi_rw_trans():
>>> In case of transfers > SPCOM_TRANLEN_MAX it loops and reads in
>>> SPCOM_TRANLEN_MAX chunks whilst replacing bytes 2-4 of the
>>> message buffer with the new start address to read from.
>>> Having said that it implicitely assumes that the first transfer in
>>> the message is a m25p80 read command with a 3 byte address.
>>>
>>> And in fsl_espi_do_one_msg() it assumes that each message includes
>>> max one r/o transfer (else rx_buf is overwritten).
>>>
>>> I'm not 100% sure whether any arbitrary combination of r/o, r/w, w/o
>>> transfers in a SPI message is acceptable. But this controller driver
>>> supports just the transfer combinations generated by the m25p80
>>> protocol driver.
>>>
>> I think in this discussion there was the idea already to introduce
>> a message size limit complementing the transfer size limit.
>> Could this be an acceptable way to go?
>> If yes I'd prepare a patch as basis for further discussion.
>>
> I think it would be better to introduce a flag saying that the
> transfer size limit is also a message size limit. We don't have a
> controller which has different limit for both and protocol drivers
> that only send one transfer in the message do not need to care that
> way.
> 
I like this idea. In spi_master we could introduce a bool flag
max_message_size_flag. This would indicate that the max transfer
size is also the max message size. Then we could change
m25p80_read to:

t[0].tx_buf = flash->command;
t[0].len = m25p_cmdsz(nor) + dummy;
spi_message_add_tail(&t[0], &m);

max_read_len = spi_max_transfer_size(spi);
if (spi->master->max_message_size_flag)
	max_read_len -= t[0].len;

t[1].rx_buf = buf;
t[1].rx_nbits = m25p80_rx_nbits(nor);
t[1].len = min(len, max_read_len);

> Also it might be nice to add functionality to spi core that detects
> drivers that don't have neither set_cs nor cs gpio and do message
> coalescing for them in the core transfer_one_message.
> 
> You could allocate two buffers of the same size, copy the transfer
> bits into the transfer buffer, do a transfer, and copy the receive
> bits out of the receive buffer.
> 
> Thanks
> 
> Michal
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html