On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote:
> Hello,
>
> On 26 February 2016 at 13:25, Mark Brown <[email protected]> wrote:
> > On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote:
> >> From: Emilio L?pez <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Tested-by: Michal Suchanek <[email protected]>
> >> Tested-by: Priit Laes <[email protected]>
> >> ---
> >
> > 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
Hello,
On 6 March 2016 at 22:42, Maxime Ripard
<[email protected]> 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
On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote:
> Hello,
>
> On 6 March 2016 at 22:42, Maxime Ripard
> <[email protected]> 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
On 17 March 2016 at 08:27, Maxime Ripard
<[email protected]> wrote:
> On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote:
>> Hello,
>>
>> On 6 March 2016 at 22:42, Maxime Ripard
>> <[email protected]> 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
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.
On 17 March 2016 at 12:43, Mark Brown <[email protected]> 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
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.
On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote:
> On 17 March 2016 at 12:43, Mark Brown <[email protected]> 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.
I guess we just don't have the same defininition of "proper".
Let's take an opposite view. Can you have SPI working now if the
DMAEngine framework is not there? Yes. Why should it change? And even
more so, why should it change silently?
> 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.
That's not true. A significant portion would work. I don't like to
make up numbers, but I guess only the EEPROMs and rather big screens
wouldn't work.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Fri, Mar 18, 2016 at 09:23:10PM +0100, Maxime Ripard wrote:
> On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote:
> > 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.
> That's not true. A significant portion would work. I don't like to
> make up numbers, but I guess only the EEPROMs and rather big screens
> wouldn't work.
You'll probably also see devices doing things like firmware downloads,
there's a bunch of applications for larger transfers. Like you say it's
definitely not going to be a universal problem though, there's many
devices that just use SPI for fast register access.