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

Groups > linux.kernel > #1351204 > unrolled thread

Re: [PATCH 1/2] spi: sun4i: add DMA support

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 1/2] spi: sun4i: add DMA support Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-03-06 22:50 +0100
    Re: [PATCH 1/2] spi: sun4i: add DMA support Michal Suchanek <hramrach@gmail.com> - 2016-03-10 10:10 +0100
      Re: [PATCH 1/2] spi: sun4i: add DMA support Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-03-17 08:30 +0100
        Re: [PATCH 1/2] spi: sun4i: add DMA support Michal Suchanek <hramrach@gmail.com> - 2016-03-17 12:00 +0100
          Re: [PATCH 1/2] spi: sun4i: add DMA support Mark Brown <broonie@kernel.org> - 2016-03-17 12:50 +0100
            Re: [PATCH 1/2] spi: sun4i: add DMA support Michal Suchanek <hramrach@gmail.com> - 2016-03-17 13:00 +0100
              Re: [PATCH 1/2] spi: sun4i: add DMA support Mark Brown <broonie@kernel.org> - 2016-03-17 13:00 +0100

#1351204 — Re: [PATCH 1/2] spi: sun4i: add DMA support

[Multipart message — attachments visible in raw view] — view raw

On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
> Hello,
> 
> On 26 February 2016 at 13:25, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote:
> >> From: Emilio López <emilio@elopez.com.ar>
> >>
> >> This patch adds support for 64 byte or bigger transfers on the
> >> sun4i SPI controller. Said transfers will be performed via DMA.
> >>
> >> Signed-off-by: Emilio López <emilio@elopez.com.ar>
> >> Tested-by: Michal Suchanek <hramrach@gmail.com>
> >> Tested-by: Priit Laes <plaes@plaes.org>
> >> ---
> >
> > You *must* sign off any patches you are sending, please see
> > SubmittingPatches.
> 
> I have sent these patches in the past.

Mike's point was that since it's Priit that he's submitting the
patches, he must add his SoB.

> Besides this non-technical objection there were multiple technical
> objections.
> 
> IIRC one was that the driver does not handle the case when the DMA
> channels are not available. As I understand it the channels are
> exclusively reserved for a particular peripherial on sunxi platform so
> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
> probe  when you have broken DT or no DMA engine support for sunxi
> platform.

There's a quite trivial scenario that would trigger this: if the dma
driver or dmaengine is not enabled / loaded.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [next] | [standalone]

#1354933

Hello,

On 6 March 2016 at 22:42, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:

>> Besides this non-technical objection there were multiple technical
>> objections.
>>
>> IIRC one was that the driver does not handle the case when the DMA
>> channels are not available. As I understand it the channels are
>> exclusively reserved for a particular peripherial on sunxi platform so
>> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
>> probe  when you have broken DT or no DMA engine support for sunxi
>> platform.
>
> There's a quite trivial scenario that would trigger this: if the dma
> driver or dmaengine is not enabled / loaded.

There are other trivial scenarios under which the driver will fail
like loading wrong DT, not compiling or loading the sunxi pinmux
driver, and whatnot.

When you misconfigure your kernel it does not work. So long as the
driver just fails and does not crash and burn it's normal. Since the
driver is pretty much useless without DMA as it is now (63 byte
transfer limit) losing the non-DMA functionality when DMA engine is
not compiled-in does not seem that terrible.

Thanks

Michal

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

#1359605

[Multipart message — attachments visible in raw view] — view raw

On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote:
> Hello,
> 
> On 6 March 2016 at 22:42, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
> 
> >> Besides this non-technical objection there were multiple technical
> >> objections.
> >>
> >> IIRC one was that the driver does not handle the case when the DMA
> >> channels are not available. As I understand it the channels are
> >> exclusively reserved for a particular peripherial on sunxi platform so
> >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
> >> probe  when you have broken DT or no DMA engine support for sunxi
> >> platform.
> >
> > There's a quite trivial scenario that would trigger this: if the dma
> > driver or dmaengine is not enabled / loaded.
> 
> There are other trivial scenarios under which the driver will fail
> like loading wrong DT, not compiling or loading the sunxi pinmux
> driver, and whatnot.

I don't see what the pinmux has to do with SPI and DMA, and you're
wrong, the pinmux driver is always compiled in if you enable the sunxi
support.

And you're missing the whole point. DMA accesses are optional, pinmux
and proper machine support are not.

> When you misconfigure your kernel it does not work. So long as the
> driver just fails and does not crash and burn it's normal. Since the
> driver is pretty much useless without DMA as it is now (63 byte
> transfer limit) losing the non-DMA functionality when DMA engine is
> not compiled-in does not seem that terrible.

You're mixing two things up: the fact that we can't do more than the
FIFO length in PIO and that we're missing DMA support. We have patches
to address both, and there's no depedency between the two.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

#1359728

On 17 March 2016 at 08:27, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote:
>> Hello,
>>
>> On 6 March 2016 at 22:42, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
>>
>> >> Besides this non-technical objection there were multiple technical
>> >> objections.
>> >>
>> >> IIRC one was that the driver does not handle the case when the DMA
>> >> channels are not available. As I understand it the channels are
>> >> exclusively reserved for a particular peripherial on sunxi platform so
>> >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail
>> >> probe  when you have broken DT or no DMA engine support for sunxi
>> >> platform.
>> >
>> > There's a quite trivial scenario that would trigger this: if the dma
>> > driver or dmaengine is not enabled / loaded.
>>
>> There are other trivial scenarios under which the driver will fail
>> like loading wrong DT, not compiling or loading the sunxi pinmux
>> driver, and whatnot.
>
> I don't see what the pinmux has to do with SPI and DMA, and you're
> wrong, the pinmux driver is always compiled in if you enable the sunxi
> support.
>
> And you're missing the whole point. DMA accesses are optional, pinmux
> and proper machine support are not.
>
>> When you misconfigure your kernel it does not work. So long as the
>> driver just fails and does not crash and burn it's normal. Since the
>> driver is pretty much useless without DMA as it is now (63 byte
>> transfer limit) losing the non-DMA functionality when DMA engine is
>> not compiled-in does not seem that terrible.
>
> You're mixing two things up: the fact that we can't do more than the
> FIFO length in PIO and that we're missing DMA support. We have patches
> to address both, and there's no depedency between the two.

The only thing is that although DMA is optional on pretty much any
system you will have DMA available unless you broke your config. You
really do want the DMA support when it is available. So there will be
nobody testing the non-DMA part and it will be prone to bitrot.

Thanks

Michal

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

#1359764

[Multipart message — attachments visible in raw view] — view raw

On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote:
> On 17 March 2016 at 08:27, Maxime Ripard

> > You're mixing two things up: the fact that we can't do more than the
> > FIFO length in PIO and that we're missing DMA support. We have patches
> > to address both, and there's no depedency between the two.

> The only thing is that although DMA is optional on pretty much any
> system you will have DMA available unless you broke your config. You
> really do want the DMA support when it is available. So there will be
> nobody testing the non-DMA part and it will be prone to bitrot.

Well, it might be worth doing PIO for very short transfers even if you
have DMA - it's quite common for this to perform better.

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

#1359770

On 17 March 2016 at 12:43, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote:
>> On 17 March 2016 at 08:27, Maxime Ripard
>
>> > You're mixing two things up: the fact that we can't do more than the
>> > FIFO length in PIO and that we're missing DMA support. We have patches
>> > to address both, and there's no depedency between the two.
>
>> The only thing is that although DMA is optional on pretty much any
>> system you will have DMA available unless you broke your config. You
>> really do want the DMA support when it is available. So there will be
>> nobody testing the non-DMA part and it will be prone to bitrot.
>
> Well, it might be worth doing PIO for very short transfers even if you
> have DMA - it's quite common for this to perform better.

That's what the driver does. The discussion revolves around the fact
that the driver does not attempt to work (even for very short
transfers) when the DMA channels are not configured and just bails
out. AFAICT the channels are always available when the system is
properly configured and the dmaengine driver loaded.

Very few device drivers would work with 63byte transfers only and the
code for manually driving the CS line in case the DMA engine fails to
configure will necessarily go untested most of the time since most
systems will have DMA configured properly.

Thanks

Michal

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

#1359778

[Multipart message — attachments visible in raw view] — view raw

On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote:

> That's what the driver does. The discussion revolves around the fact
> that the driver does not attempt to work (even for very short
> transfers) when the DMA channels are not configured and just bails
> out. AFAICT the channels are always available when the system is
> properly configured and the dmaengine driver loaded.

The driver should tell the core about this constraint so the core can
split the transfers for it.

> Very few device drivers would work with 63byte transfers only and the
> code for manually driving the CS line in case the DMA engine fails to
> configure will necessarily go untested most of the time since most
> systems will have DMA configured properly.

A lot of devices will be perfectly happy with 63 byte transfers,
register accesses for example tend to be much smaller than that.  The
manual /CS might be an issue but for most SoCs that is easily addressed
by driving the pin as a GPIO if there's an issue.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web