Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Mark Brown]

On Sun, Jun 05, 2016 at 01:27:11PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 16:26, Mark Brown  wrote:
> > On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:

> >> And the code added in that patch will never run unless you

> >> 1) use long spi transfers
> >> 2) compile in/load SPI without DMA support

> >> There is no reason for doing 2) since we have do DMA support for sunxi.

> > Well, presumably such code exists and is being worked on?

> Which code are you referring to?

> This is a reply to patch which adds DMA support to the SPI driver so
> that it can work for arbitrarily long transfers.

It sounded like there might be a missing DMA driver?  Though I think I
was misreading the above.

> > Oh, come on.  You might not want to use it yourself but the chances are
> > that someone will want to use it just like the situation with all the
> > other SPI drivers.  It's a perfectly reasonable and sensible feature to
> > support upstream.

> Is it?

> Once we have *one* driver that works for arbitrarily long transfers
> and it works out of the box with the board defconfig 99% of people who
> will use the SPI driver for anything will use this driver. Any other
> variant will go untested.

We don't want multiple drivers for this and general maintainability
reasons, I'm not sure where the idea that we might want multiple drivers
came from?

> And for the driver to also work without DMA you have to *tell* it to
> probe without DMA because it cannot know you are not going to load a
> DMA driver later.

Yes.  This puts the cost on the special snowflake systems that
absolutely cannot have any DMA support in their systems for some reason
while keeping the driver featureful by default, and keeps that cost
fairly small.

> > I really do not understand why there is such a strong desire to have
> > these devices be a special snowflake here, the worst that's likely to
> > happen here is that you're going to end up having to either remove the
> > DMA controller from the DT or load the driver for it neither of which
> > seem like the end of the world.

> Why would you do that?

Like I say I really don't understand why this is.  I'm just saying there
are ways to do this fairly simply if people really want it on their
systems.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Mark Brown]

On Sun, Jun 05, 2016 at 01:27:11PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 16:26, Mark Brown  wrote:
> > On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:

> >> And the code added in that patch will never run unless you

> >> 1) use long spi transfers
> >> 2) compile in/load SPI without DMA support

> >> There is no reason for doing 2) since we have do DMA support for sunxi.

> > Well, presumably such code exists and is being worked on?

> Which code are you referring to?

> This is a reply to patch which adds DMA support to the SPI driver so
> that it can work for arbitrarily long transfers.

It sounded like there might be a missing DMA driver?  Though I think I
was misreading the above.

> > Oh, come on.  You might not want to use it yourself but the chances are
> > that someone will want to use it just like the situation with all the
> > other SPI drivers.  It's a perfectly reasonable and sensible feature to
> > support upstream.

> Is it?

> Once we have *one* driver that works for arbitrarily long transfers
> and it works out of the box with the board defconfig 99% of people who
> will use the SPI driver for anything will use this driver. Any other
> variant will go untested.

We don't want multiple drivers for this and general maintainability
reasons, I'm not sure where the idea that we might want multiple drivers
came from?

> And for the driver to also work without DMA you have to *tell* it to
> probe without DMA because it cannot know you are not going to load a
> DMA driver later.

Yes.  This puts the cost on the special snowflake systems that
absolutely cannot have any DMA support in their systems for some reason
while keeping the driver featureful by default, and keeps that cost
fairly small.

> > I really do not understand why there is such a strong desire to have
> > these devices be a special snowflake here, the worst that's likely to
> > happen here is that you're going to end up having to either remove the
> > DMA controller from the DT or load the driver for it neither of which
> > seem like the end of the world.

> Why would you do that?

Like I say I really don't understand why this is.  I'm just saying there
are ways to do this fairly simply if people really want it on their
systems.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Michal Suchanek]

On 2 June 2016 at 16:26, Mark Brown  wrote:
> On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
>> On 2 June 2016 at 06:42, Priit Laes  wrote:
>> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>
>> > Actually it non-DMA case works fine if you don't need SPI transfers
>> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
>> > This was addressed by this patch, but was never applied:
>> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
>
>> And the code added in that patch will never run unless you
>
>> 1) use long spi transfers
>> 2) compile in/load SPI without DMA support
>
>> There is no reason for doing 2) since we have do DMA support for sunxi.
>
> Well, presumably such code exists and is being worked on?

Which code are you referring to?

This is a reply to patch which adds DMA support to the SPI driver so
that it can work for arbitrarily long transfers.

So there is code for fully working driver.

>
>> So that's another code path that needs maintenance and testing and
>> likely will not get it.
>
> Oh, come on.  You might not want to use it yourself but the chances are
> that someone will want to use it just like the situation with all the
> other SPI drivers.  It's a perfectly reasonable and sensible feature to
> support upstream.

Is it?

Once we have *one* driver that works for arbitrarily long transfers
and it works out of the box with the board defconfig 99% of people who
will use the SPI driver for anything will use this driver. Any other
variant will go untested.

And for the driver to also work without DMA you have to *tell* it to
probe without DMA because it cannot know you are not going to load a
DMA driver later.

>
> I really do not understand why there is such a strong desire to have
> these devices be a special snowflake here, the worst that's likely to
> happen here is that you're going to end up having to either remove the
> DMA controller from the DT or load the driver for it neither of which
> seem like the end of the world.

Why would you do that?

Thanks

Michal

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Michal Suchanek]

On 2 June 2016 at 16:26, Mark Brown  wrote:
> On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
>> On 2 June 2016 at 06:42, Priit Laes  wrote:
>> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>
>> > Actually it non-DMA case works fine if you don't need SPI transfers
>> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
>> > This was addressed by this patch, but was never applied:
>> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950
>
>> And the code added in that patch will never run unless you
>
>> 1) use long spi transfers
>> 2) compile in/load SPI without DMA support
>
>> There is no reason for doing 2) since we have do DMA support for sunxi.
>
> Well, presumably such code exists and is being worked on?

Which code are you referring to?

This is a reply to patch which adds DMA support to the SPI driver so
that it can work for arbitrarily long transfers.

So there is code for fully working driver.

>
>> So that's another code path that needs maintenance and testing and
>> likely will not get it.
>
> Oh, come on.  You might not want to use it yourself but the chances are
> that someone will want to use it just like the situation with all the
> other SPI drivers.  It's a perfectly reasonable and sensible feature to
> support upstream.

Is it?

Once we have *one* driver that works for arbitrarily long transfers
and it works out of the box with the board defconfig 99% of people who
will use the SPI driver for anything will use this driver. Any other
variant will go untested.

And for the driver to also work without DMA you have to *tell* it to
probe without DMA because it cannot know you are not going to load a
DMA driver later.

>
> I really do not understand why there is such a strong desire to have
> these devices be a special snowflake here, the worst that's likely to
> happen here is that you're going to end up having to either remove the
> DMA controller from the DT or load the driver for it neither of which
> seem like the end of the world.

Why would you do that?

Thanks

Michal

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Mark Brown]

On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 06:42, Priit Laes  wrote:
> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:

> > Actually it non-DMA case works fine if you don't need SPI transfers
> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

> > This was addressed by this patch, but was never applied:
> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

> And the code added in that patch will never run unless you

> 1) use long spi transfers
> 2) compile in/load SPI without DMA support

> There is no reason for doing 2) since we have do DMA support for sunxi.

Well, presumably such code exists and is being worked on?

> So that's another code path that needs maintenance and testing and
> likely will not get it.

Oh, come on.  You might not want to use it yourself but the chances are
that someone will want to use it just like the situation with all the
other SPI drivers.  It's a perfectly reasonable and sensible feature to
support upstream.

I really do not understand why there is such a strong desire to have
these devices be a special snowflake here, the worst that's likely to
happen here is that you're going to end up having to either remove the
DMA controller from the DT or load the driver for it neither of which
seem like the end of the world.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Mark Brown]

On Thu, Jun 02, 2016 at 02:14:26PM +0200, Michal Suchanek wrote:
> On 2 June 2016 at 06:42, Priit Laes  wrote:
> > On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:

> > Actually it non-DMA case works fine if you don't need SPI transfers
> > larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

> > This was addressed by this patch, but was never applied:
> > http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

> And the code added in that patch will never run unless you

> 1) use long spi transfers
> 2) compile in/load SPI without DMA support

> There is no reason for doing 2) since we have do DMA support for sunxi.

Well, presumably such code exists and is being worked on?

> So that's another code path that needs maintenance and testing and
> likely will not get it.

Oh, come on.  You might not want to use it yourself but the chances are
that someone will want to use it just like the situation with all the
other SPI drivers.  It's a perfectly reasonable and sensible feature to
support upstream.

I really do not understand why there is such a strong desire to have
these devices be a special snowflake here, the worst that's likely to
happen here is that you're going to end up having to either remove the
DMA controller from the DT or load the driver for it neither of which
seem like the end of the world.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Michal Suchanek]

On 2 June 2016 at 06:42, Priit Laes  wrote:
> On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
>> >
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> > >
>> > > On 30 May 2016 at 17:03, Mark Brown  wrote:
>> > >
>> > > >
>> > > > I really don't think it's worth caring too much about cases
>> > > > where the
>> > > > DMA driver hasn't been compiled in, it's not like SPI is the
>> > > > only thing
>> > >
>> > > It's what the driver did to start with and it was requested to
>> > > fall
>> > > back to non-DMA in the case DMA is not available.
>> > Why?  I really can't see any sensible use case for this that
>> > doesn't
>> > have a better solution available.
>> SPI works just fine without DMA, which might just be considered an
>> (optional) optimisation.
>>
>> We've been using it without DMA for years now, and it was working
>> just
>> fine, and it will work even better with the other patches in this
>> serie. There's no reason to add a hard dependency on something that
>> we
>> don't really need.
>>
>
> Actually it non-DMA case works fine if you don't need SPI transfers
> larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

And the code added in that patch will never run unless you

1) use long spi transfers
2) compile in/load SPI without DMA support

There is no reason for doing 2) since we have do DMA support for sunxi.

So that's another code path that needs maintenance and testing and
likely will not get it.

Thanks

Michal

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Michal Suchanek]

On 2 June 2016 at 06:42, Priit Laes  wrote:
> On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
>> Hi,
>>
>> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
>> >
>> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
>> > >
>> > > On 30 May 2016 at 17:03, Mark Brown  wrote:
>> > >
>> > > >
>> > > > I really don't think it's worth caring too much about cases
>> > > > where the
>> > > > DMA driver hasn't been compiled in, it's not like SPI is the
>> > > > only thing
>> > >
>> > > It's what the driver did to start with and it was requested to
>> > > fall
>> > > back to non-DMA in the case DMA is not available.
>> > Why?  I really can't see any sensible use case for this that
>> > doesn't
>> > have a better solution available.
>> SPI works just fine without DMA, which might just be considered an
>> (optional) optimisation.
>>
>> We've been using it without DMA for years now, and it was working
>> just
>> fine, and it will work even better with the other patches in this
>> serie. There's no reason to add a hard dependency on something that
>> we
>> don't really need.
>>
>
> Actually it non-DMA case works fine if you don't need SPI transfers
> larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.
>
> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

And the code added in that patch will never run unless you

1) use long spi transfers
2) compile in/load SPI without DMA support

There is no reason for doing 2) since we have do DMA support for sunxi.

So that's another code path that needs maintenance and testing and
likely will not get it.

Thanks

Michal

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Mark Brown]

On Thu, Jun 02, 2016 at 07:42:26AM +0300, Priit Laes wrote:

> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

I've never seen that patch before, if it was CCed to me there's a good
chance it'd have been deleted unread because you put ARM: at the start
of the subject line which says it's a patch for arch/arm.  You'll need
to resubmit.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Mark Brown]

On Thu, Jun 02, 2016 at 07:42:26AM +0300, Priit Laes wrote:

> This was addressed by this patch, but was never applied:
> http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

I've never seen that patch before, if it was CCed to me there's a good
chance it'd have been deleted unread because you put ARM: at the start
of the subject line which says it's a patch for arch/arm.  You'll need
to resubmit.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Priit Laes]

On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> > 
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > > 
> > > On 30 May 2016 at 17:03, Mark Brown  wrote:
> > > 
> > > > 
> > > > I really don't think it's worth caring too much about cases
> > > > where the
> > > > DMA driver hasn't been compiled in, it's not like SPI is the
> > > > only thing
> > > 
> > > It's what the driver did to start with and it was requested to
> > > fall
> > > back to non-DMA in the case DMA is not available.
> > Why?  I really can't see any sensible use case for this that
> > doesn't
> > have a better solution available.
> SPI works just fine without DMA, which might just be considered an
> (optional) optimisation.
> 
> We've been using it without DMA for years now, and it was working
> just
> fine, and it will work even better with the other patches in this
> serie. There's no reason to add a hard dependency on something that
> we
> don't really need.
> 

Actually it non-DMA case works fine if you don't need SPI transfers
larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

Re: [linux-sunxi] Re: [PATCH 5/5] RFC spi: sun4i: add DMA support

[Priit Laes]

On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Mon, May 30, 2016 at 04:50:16PM +0100, Mark Brown wrote:
> > 
> > On Mon, May 30, 2016 at 05:28:10PM +0200, Michal Suchanek wrote:
> > > 
> > > On 30 May 2016 at 17:03, Mark Brown  wrote:
> > > 
> > > > 
> > > > I really don't think it's worth caring too much about cases
> > > > where the
> > > > DMA driver hasn't been compiled in, it's not like SPI is the
> > > > only thing
> > > 
> > > It's what the driver did to start with and it was requested to
> > > fall
> > > back to non-DMA in the case DMA is not available.
> > Why?  I really can't see any sensible use case for this that
> > doesn't
> > have a better solution available.
> SPI works just fine without DMA, which might just be considered an
> (optional) optimisation.
> 
> We've been using it without DMA for years now, and it was working
> just
> fine, and it will work even better with the other patches in this
> serie. There's no reason to add a hard dependency on something that
> we
> don't really need.
> 

Actually it non-DMA case works fine if you don't need SPI transfers
larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950