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

[Michal Suchanek <hramrach@xxxxxxxxx>]

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.

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