Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1184570 > unrolled thread

Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Michal Suchanek <hramrach@gmail.com> - 2015-07-15 11:50 +0200
    Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Marek Vasut <marex@denx.de> - 2015-07-15 14:40 +0200
      Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Brian Norris <computersforpeace@gmail.com> - 2015-07-16 03:20 +0200
        Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Marek Vasut <marex@denx.de> - 2015-07-16 03:50 +0200
      Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Michal Suchanek <hramrach@gmail.com> - 2015-07-19 21:10 +0200
        Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Michal Suchanek <hramrach@gmail.com> - 2015-07-22 09:40 +0200

#1184570 — Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.

On 4 June 2015 at 19:15, Richard Cochran <richardcochran@gmail.com> wrote:
> On Thu, Jun 04, 2015 at 10:31:45AM +0200, Michal Suchanek wrote:
>> You might want to try to run the bus at 60MHz or 80MHz and then the
>> values would probably again be different.
>>
>> The first two values are set in DT so the logical place for setting
>> the third is also in DT.
>>
>> Otherwise you would need some in-kernel table of these settings.
>
> Or a formula.
>

This formula probably needs to take into account

 - the unknown reason for the pl330 to fail transfer
 - the device transfer speed and transfer phase as set in DT
 - possibly device-specific latency and board-specific trace design
and assembly tolerances

Seriously, until I have at least a vague idea why the transfer fails I
am not comfortable pulling some formula out of thin air and pretending
I have a working patch.

On the other hand, a parameter you can set in the DT and which comes
with a suggested value which can be tuned depending on the system
seems more viable.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1184766

On Wednesday, July 15, 2015 at 11:45:07 AM, Michal Suchanek wrote:
> On 4 June 2015 at 19:15, Richard Cochran <richardcochran@gmail.com> wrote:
> > On Thu, Jun 04, 2015 at 10:31:45AM +0200, Michal Suchanek wrote:
> >> You might want to try to run the bus at 60MHz or 80MHz and then the
> >> values would probably again be different.
> >> 
> >> The first two values are set in DT so the logical place for setting
> >> the third is also in DT.
> >> 
> >> Otherwise you would need some in-kernel table of these settings.
> > 
> > Or a formula.
> 
> This formula probably needs to take into account
> 
>  - the unknown reason for the pl330 to fail transfer

Shouldn't that be fixed at the PL330 level ? This looks like fixing
a problem at the wrong place :)

>  - the device transfer speed and transfer phase as set in DT
>  - possibly device-specific latency and board-specific trace design
> and assembly tolerances

If the design is broken, then cap the speed as for normal SPI device.

> Seriously, until I have at least a vague idea why the transfer fails I
> am not comfortable pulling some formula out of thin air and pretending
> I have a working patch.
> 
> On the other hand, a parameter you can set in the DT and which comes
> with a suggested value which can be tuned depending on the system
> seems more viable.

The problem is, if you add a new DT binding, you'd have to support it
forever, no matter how bad idea that binding turned out to be.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185199

On Wed, Jul 15, 2015 at 07:15:50PM +0200, Marek Vasut wrote:
> On Wednesday, July 15, 2015 at 05:59:46 PM, Brian Norris wrote:
> > 1. Fix up the SPI driver so that it knows how to break large SPI
> > transfers up into smaller segments that its constituent hardware (DMA
> > controllers, fast clocks, etc.) can handle.

BTW, Mark Brown already commented on this approach:

http://lists.infradead.org/pipermail/linux-mtd/2015-May/059364.html

I quote:

| With modern drivers using transfer_one() where we have control of the
| chip select we do also have the ability to add support to the core for
| chopping large transfers up into smaller ones that the hardware can
| handle transparently.  That won't for everything though since it depends
| on us being able to manually control the chip select which not all
| hardware can do.

> I think this might actually be easier -- just do a transfer where you
> don't toggle CS and just stops the clock at the last bit, then do another
> (multiple) transfers which don't toggle CS at all, then finally do a
> transfer which toggles a CS at the end.

Sounds OK to me. And as Mark noted, this could probably be done in the
core. I suppose Mark could suggest whether the most expedient path is to
hack the buggy driver to do this, or whether reworking the SPI core to
have the appropriate spi_master field(s) (and caveats) to support this.

> This should be pretty trivial
> to do and I think for example spi-mxs.c does this.

I don't think spi-mxs.c really does this; it chooses between PIO and DMA
based on length, but with either option, it runs through each SPI
transfer in one go. You might be confused by the fact that this driver
implements ->transfer_one_message instead of ->transfer_one, so it has
to loop through all transfers in the message.

(IIUC, spi-mxs.c could easily be rewritten to use ->transfer, and some
of that not-too-complicated boilerplate could be killed off.)

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185301

On Thursday, July 16, 2015 at 03:19:35 AM, Brian Norris wrote:
> On Wed, Jul 15, 2015 at 07:15:50PM +0200, Marek Vasut wrote:
> > On Wednesday, July 15, 2015 at 05:59:46 PM, Brian Norris wrote:
> > > 1. Fix up the SPI driver so that it knows how to break large SPI
> > > transfers up into smaller segments that its constituent hardware (DMA
> > > controllers, fast clocks, etc.) can handle.
> 
> BTW, Mark Brown already commented on this approach:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2015-May/059364.html
> 
> I quote:
> | With modern drivers using transfer_one() where we have control of the
> | chip select we do also have the ability to add support to the core for
> | chopping large transfers up into smaller ones that the hardware can
> | handle transparently.  That won't for everything though since it depends
> | on us being able to manually control the chip select which not all
> | hardware can do.
> | 
> > I think this might actually be easier -- just do a transfer where you
> > don't toggle CS and just stops the clock at the last bit, then do another
> > (multiple) transfers which don't toggle CS at all, then finally do a
> > transfer which toggles a CS at the end.
> 
> Sounds OK to me. And as Mark noted, this could probably be done in the
> core. I suppose Mark could suggest whether the most expedient path is to
> hack the buggy driver to do this, or whether reworking the SPI core to
> have the appropriate spi_master field(s) (and caveats) to support this.

Yep, agreed.

> > This should be pretty trivial
> > to do and I think for example spi-mxs.c does this.
> 
> I don't think spi-mxs.c really does this; it chooses between PIO and DMA
> based on length, but with either option, it runs through each SPI
> transfer in one go. You might be confused by the fact that this driver
> implements ->transfer_one_message instead of ->transfer_one, so it has
> to loop through all transfers in the message.

It does chop up the DMA transfers between multiple runs of the DMA engine
IIRC. But it does so within the driver, which apparently is not the way
to go :)

> (IIUC, spi-mxs.c could easily be rewritten to use ->transfer, and some
> of that not-too-complicated boilerplate could be killed off.)

Most likely, yes.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1187745

Hello,

On 15 July 2015 at 17:59, Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Michal,
>
> On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote:
>> The problem is, if you add a new DT binding, you'd have to support it
>> forever, no matter how bad idea that binding turned out to be.
>
> Agreed, and a solid NAK to this patch. I could have sworn I gave such a
> response when this was originally being discussed a month ago.
>
> AFAICT, you have one of two general approaches available to you:
>
> 1. Fix up the SPI driver so that it knows how to break large SPI
> transfers up into smaller segments that its constituent hardware (DMA
> controllers, fast clocks, etc.) can handle.
>
> 2. Utilize/create a parameter within the SPI subsystem to communicate
> that the SPI master has a limited max transfer size (notably: NOT a
> per-device DT property, but a SPI API property), and modify SPI device
> drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I
> thought he suggested it somewhere.

It is not known what exactly is limited here.

It seems that the pl330 fails but it is not possible to transfer that
much data over the spi bus in one go without the help of the pl330.

With either approach the limit depends on the SPI transfer settings
which are known the the SPI driver. The pl330 driver is oblivious to
these because it just transfers data from one port to another port and
has no idea that the port is wired to SPI in the SoC.

On the other hand, AFAICT the SPI driver only allocates a DMA channel
which it receives through DT binding and is oblivious to the fact the
DMA channel lives on a pl330. It could probably determine that the
channel is indeed driven by a pl330. I don't think it's a great idea
to add device-specific handling to a generic dmaengine driver or a
dmaengine-spiecific handling to a SPI driver.

It's technically possible to pass SPI transfer parameters to the
dmaengine driver prior to transfer and the dmaengine could impose some
limitation based on those parameters. However, generalising this to
drivers other than SPI might be problematic. Should this interface
also handle i2c parameters, VE parameters, audio parameters, ethernet
parameters, etc?

In the DT you have the information that this particular device is
connected to a Samsung SPI controller connected to a pl330 dma engine.

>
> It is most definitely wrong to put this information solely in the slave
> device DT (i.e., for the flash), when it is not a property of the flash
> device at all. It's a property of the SPI master and/or its clocks and
> DMA controllers.

However, the clocks are set by the parameters of the device, not the
parameters of the master. So the limitation applies for the master
with the settings of the particular slave device. Different slave
settings do impose different master limitations.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1189706

On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
>> > Or alternatively we could publish the limitations of the channel using
>> > capabilities so SPI knows I have a dmaengine channel and it can transfer max N
>> > length transfers so would be able to break rather than guessing it or coding
>> > in DT. Yes it may come from DT but that should be dmaengine driver rather
>> > than client driver :)
>> >
>> > This can be done by dma_get_slave_caps(chan, &caps)
>> >
>> > And we add max_length as one more parameter to existing set
>> >
>> > Also all this could be handled in generic SPI-dmaengine layer so that
>> > individual drivers don't have to code it in
>> >
>> > Let me know if this idea is okay, I can push the dmaengine bits...
>>
>> It would be ok if there was a fixed limit. However, the limit depends
>> on SPI slave settings. Presumably for other buses using the dmaengine
>> the limit would depend on the bus or slave settings as well. I do not
>> see a sane way of passing this all the way to the dmaengine driver.
> I don't see why this should be client (SPI) dependent. The max length
> supported is a dmaengine constraint, typically flowing from max
> blocks/length it can transfer. Know this limit can allow clients to split
> transfers.

In practice on the board I have the maximum transfer length before it
fails depends on SPI bus speed which is set up per slave. I did not
try searching the space of possible settings thorougly and settled for
a setting that gives reasonable speed and transfer length.

However, if this was not tied to the particular slave setting picked
in the current DT a formula would be needed that translates arbitrary
client settings to transfer size limit and there would be need to
somehow get the client settings to the formula in the dmaengine
driver.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web