[U-Boot] spi subystem maintainer?

[U-Boot] spi subystem maintainer?

[Kumar Gala]

Do we have one?

- k

[U-Boot] spi subystem maintainer?

[Wolfgang Denk]

Dear Kumar Gala,

In message <9A3702C1-3ED1-4F24-A0BC-CA7DAEA46182@kernel.crashing.org> you wrote:
> Do we have one?

Not yet.  Are you volunteering?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Any sufficiently advanced technology is indistinguishable from magic.
                                                   - Arthur C. Clarke

[U-Boot] spi subystem maintainer?

[Kumar Gala]

On Feb 1, 2011, at 1:29 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <9A3702C1-3ED1-4F24-A0BC-CA7DAEA46182@kernel.crashing.org> you wrote:
>> Do we have one?
> 
> Not yet.  Are you volunteering?

Nope, too much to do as it stands.

Wanted to see if anyone had input on how to deal with the SPI controller on some of our newer parts.  It expects command & data xfer's to happen together.  However our current code does not call spi_xfer() that way.

- k

[U-Boot] spi subystem maintainer?

[Stefano Babic]

On 02/02/2011 08:23 AM, Kumar Gala wrote:
> Wanted to see if anyone had input on how to deal with the SPI
> controller on some of our newer parts.  It expects command & data
> xfer's to happen together.  However our current code does not call
> spi_xfer() that way.

Which is your concrete case ? spi_xfer is responsible to setup the
controller and to start the transfer, and everything could be done
inside this function. What do you mean exactly with command and data ?

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] spi subystem maintainer?

[Reinhard Meyer]

Dear Stefano Babic:
> On 02/02/2011 08:23 AM, Kumar Gala wrote:
>> Wanted to see if anyone had input on how to deal with the SPI
>> controller on some of our newer parts.  It expects command & data
>> xfer's to happen together.  However our current code does not call
>> spi_xfer() that way.
> 
> Which is your concrete case ? spi_xfer is responsible to setup the
> controller and to start the transfer, and everything could be done
> inside this function. What do you mean exactly with command and data ?
> 
> Regards,
> Stefano
> 

I think he refers to the common "problem" that many SPI devices
require CS to stay low during both "phases" of issuing the
read/write command and transfering the actual data.

Current u-boot code calls spi_xfer() two times.

Hardware controlled CS often go high between both calls, which
requires you to (at least) use GPIO controlled CS, or, even worse,
use bitbang SPI (in cases where the SPI pin assignment is in groups,
not individually).

Best Regards,
Reinhard

[U-Boot] spi subystem maintainer?

[Kumar Gala]

On Feb 2, 2011, at 3:30 AM, Reinhard Meyer wrote:

> Dear Stefano Babic:
>> On 02/02/2011 08:23 AM, Kumar Gala wrote:
>>> Wanted to see if anyone had input on how to deal with the SPI
>>> controller on some of our newer parts.  It expects command & data
>>> xfer's to happen together.  However our current code does not call
>>> spi_xfer() that way.
>> 
>> Which is your concrete case ? spi_xfer is responsible to setup the
>> controller and to start the transfer, and everything could be done
>> inside this function. What do you mean exactly with command and data ?
>> 
>> Regards,
>> Stefano
>> 
> 
> I think he refers to the common "problem" that many SPI devices
> require CS to stay low during both "phases" of issuing the
> read/write command and transfering the actual data.
> 
> Current u-boot code calls spi_xfer() two times.
> 
> Hardware controlled CS often go high between both calls, which
> requires you to (at least) use GPIO controlled CS, or, even worse,
> use bitbang SPI (in cases where the SPI pin assignment is in groups,
> not individually).

That's correct, and with the newer FSL controller's we dont have direct control over the CS.  I'm thinking we need to have the command and response dealt with in a single call to spi_xfer instead of what we seem to do all over the place today:

        ret = spi_xfer(spi, 8, &cmd, NULL, flags);
        if (ret) {
                debug("SF: Failed to send command %02x: %d\n", cmd, ret);
                return ret;
        }

        if (len) {
                ret = spi_xfer(spi, len * 8, NULL, response, SPI_XFER_END);
                if (ret)
                        debug("SF: Failed to read response (%zu bytes): %d\n",
                                        len, ret);
        }

Needs to turn into something like:

	ret = spi_xfer(spi, 8 + len * 8, &cmd, response, flags | SPI_XFER_END)

- k

[U-Boot] spi subystem maintainer?

[Stefano Babic]

On 02/03/2011 11:36 AM, Kumar Gala wrote:
> That's correct, and with the newer FSL controller's we dont have
> direct control over the CS.

I know. You are probably talking about PowerPC, but it is the same for
the i.MX processors.

>  I'm thinking we need to have the command
> and response dealt with in a single call to spi_xfer instead of what
> we seem to do all over the place today:
> 
> ret = spi_xfer(spi, 8, &cmd, NULL, flags); if (ret) { debug("SF:
> Failed to send command %02x: %d\n", cmd, ret); return ret; }

Ok. You are not talking generally about spi, but how to manage spi
flash. I saw the same problem with the i.MX51, but then a GPIO was used
instead of the internal SS and I forgot this issue. We do not need to
change the spi_xfer() call (I mean, in the spi controllers).

> 
> if (len) { ret = spi_xfer(spi, len * 8, NULL, response,
> SPI_XFER_END); if (ret) debug("SF: Failed to read response (%zu
> bytes): %d\n", len, ret); }
> 
> Needs to turn into something like:
> 
> ret = spi_xfer(spi, 8 + len * 8, &cmd, response, flags |
> SPI_XFER_END)

I think it could depend on the SPI flash you use. I checked in
stmicro.c, and the mechanism to split the enabling of SS and the
transfer of data is really used.

In stmicro_wait_ready(), it is used to poll the status of the flash. The
SS is active until the flash has complete the operation. Probably the
flash can accept a single call, too, but I am unsure.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] spi subystem maintainer?

[Mike Frysinger]

On Tuesday, February 01, 2011 11:00:39 Kumar Gala wrote:
> Do we have one?

someone else asked me that and the answer i gave was that arch-drivers should 
go through arch trees, but common code changes i can help run through my tree.  
but in this case i guess you're not asking about how to get a patch merged ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110215/8d35b886/attachment.pgp 

[U-Boot] spi subystem maintainer?

[Mike Frysinger]

On Thursday, February 03, 2011 05:36:38 Kumar Gala wrote:
> On Feb 2, 2011, at 3:30 AM, Reinhard Meyer wrote:
> > Dear Stefano Babic:
> >> On 02/02/2011 08:23 AM, Kumar Gala wrote:
> >>> Wanted to see if anyone had input on how to deal with the SPI
> >>> controller on some of our newer parts.  It expects command & data
> >>> xfer's to happen together.  However our current code does not call
> >>> spi_xfer() that way.
> >> 
> >> Which is your concrete case ? spi_xfer is responsible to setup the
> >> controller and to start the transfer, and everything could be done
> >> inside this function. What do you mean exactly with command and data ?
> >> 
> >> Regards,
> >> Stefano
> > 
> > I think he refers to the common "problem" that many SPI devices
> > require CS to stay low during both "phases" of issuing the
> > read/write command and transfering the actual data.
> > 
> > Current u-boot code calls spi_xfer() two times.
> > 
> > Hardware controlled CS often go high between both calls, which
> > requires you to (at least) use GPIO controlled CS, or, even worse,
> > use bitbang SPI (in cases where the SPI pin assignment is in groups,
> > not individually).
> 
> That's correct, and with the newer FSL controller's we dont have direct
> control over the CS.  I'm thinking we need to have the command and
> response dealt with in a single call to spi_xfer instead of what we seem
> to do all over the place today:
> 
>         ret = spi_xfer(spi, 8, &cmd, NULL, flags);
>         if (ret) {
>                 debug("SF: Failed to send command %02x: %d\n", cmd, ret);
>                 return ret;
>         }
> 
>         if (len) {
>                 ret = spi_xfer(spi, len * 8, NULL, response, SPI_XFER_END);
>                 if (ret)
>                         debug("SF: Failed to read response (%zu bytes):
> %d\n", len, ret);
>         }
> 
> Needs to turn into something like:
> 
> 	ret = spi_xfer(spi, 8 + len * 8, &cmd, response, flags | SPI_XFER_END)

this should be easier in my sf branch as i unified a bunch of the functions.  
but while this will probably work for the main commands, how is this supposed 
to work for the status polling ?  that function is fundamentally based around 
setting up a transfer/command and then continuously shifting out a single 
result and checking it before shifting out another.  for your controller, the 
only way to make it work is to do the full transaction every time.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110215/8c717ebd/attachment.pgp 

[U-Boot] spi subystem maintainer?

[Kumar Gala]

On Feb 15, 2011, at 2:36 AM, Mike Frysinger wrote:

> On Thursday, February 03, 2011 05:36:38 Kumar Gala wrote:
>> On Feb 2, 2011, at 3:30 AM, Reinhard Meyer wrote:
>>> Dear Stefano Babic:
>>>> On 02/02/2011 08:23 AM, Kumar Gala wrote:
>>>>> Wanted to see if anyone had input on how to deal with the SPI
>>>>> controller on some of our newer parts.  It expects command & data
>>>>> xfer's to happen together.  However our current code does not call
>>>>> spi_xfer() that way.
>>>> 
>>>> Which is your concrete case ? spi_xfer is responsible to setup the
>>>> controller and to start the transfer, and everything could be done
>>>> inside this function. What do you mean exactly with command and data ?
>>>> 
>>>> Regards,
>>>> Stefano
>>> 
>>> I think he refers to the common "problem" that many SPI devices
>>> require CS to stay low during both "phases" of issuing the
>>> read/write command and transfering the actual data.
>>> 
>>> Current u-boot code calls spi_xfer() two times.
>>> 
>>> Hardware controlled CS often go high between both calls, which
>>> requires you to (at least) use GPIO controlled CS, or, even worse,
>>> use bitbang SPI (in cases where the SPI pin assignment is in groups,
>>> not individually).
>> 
>> That's correct, and with the newer FSL controller's we dont have direct
>> control over the CS.  I'm thinking we need to have the command and
>> response dealt with in a single call to spi_xfer instead of what we seem
>> to do all over the place today:
>> 
>>        ret = spi_xfer(spi, 8, &cmd, NULL, flags);
>>        if (ret) {
>>                debug("SF: Failed to send command %02x: %d\n", cmd, ret);
>>                return ret;
>>        }
>> 
>>        if (len) {
>>                ret = spi_xfer(spi, len * 8, NULL, response, SPI_XFER_END);
>>                if (ret)
>>                        debug("SF: Failed to read response (%zu bytes):
>> %d\n", len, ret);
>>        }
>> 
>> Needs to turn into something like:
>> 
>> 	ret = spi_xfer(spi, 8 + len * 8, &cmd, response, flags | SPI_XFER_END)
> 
> this should be easier in my sf branch as i unified a bunch of the functions.  
> but while this will probably work for the main commands, how is this supposed 
> to work for the status polling ?  that function is fundamentally based around 
> setting up a transfer/command and then continuously shifting out a single 
> result and checking it before shifting out another.  for your controller, the 
> only way to make it work is to do the full transaction every time.
> -mike

Probably have to do a transaction every time.

Do you have a tree for these changes?  Do you expect them to be in place for release after v2011.03

- k

[U-Boot] spi subystem maintainer?

[Mike Frysinger]

On Tuesday, February 15, 2011 18:10:47 Kumar Gala wrote:
> On Feb 15, 2011, at 2:36 AM, Mike Frysinger wrote:
> > On Thursday, February 03, 2011 05:36:38 Kumar Gala wrote:
> >> On Feb 2, 2011, at 3:30 AM, Reinhard Meyer wrote:
> >>> Dear Stefano Babic:
> >>>> On 02/02/2011 08:23 AM, Kumar Gala wrote:
> >>>>> Wanted to see if anyone had input on how to deal with the SPI
> >>>>> controller on some of our newer parts.  It expects command & data
> >>>>> xfer's to happen together.  However our current code does not call
> >>>>> spi_xfer() that way.
> >>>> 
> >>>> Which is your concrete case ? spi_xfer is responsible to setup the
> >>>> controller and to start the transfer, and everything could be done
> >>>> inside this function. What do you mean exactly with command and data ?
> >>> 
> >>> I think he refers to the common "problem" that many SPI devices
> >>> require CS to stay low during both "phases" of issuing the
> >>> read/write command and transfering the actual data.
> >>> 
> >>> Current u-boot code calls spi_xfer() two times.
> >>> 
> >>> Hardware controlled CS often go high between both calls, which
> >>> requires you to (at least) use GPIO controlled CS, or, even worse,
> >>> use bitbang SPI (in cases where the SPI pin assignment is in groups,
> >>> not individually).
> >> 
> >> That's correct, and with the newer FSL controller's we dont have direct
> >> control over the CS.  I'm thinking we need to have the command and
> >> response dealt with in a single call to spi_xfer instead of what we seem
> >> 
> >> to do all over the place today:
> >>        ret = spi_xfer(spi, 8, &cmd, NULL, flags);
> >>        if (ret) {
> >>        
> >>                debug("SF: Failed to send command %02x: %d\n", cmd, ret);
> >>                return ret;
> >>        
> >>        }
> >>        
> >>        if (len) {
> >>        
> >>                ret = spi_xfer(spi, len * 8, NULL, response,
> >>                SPI_XFER_END); if (ret)
> >>                
> >>                        debug("SF: Failed to read response (%zu bytes):
> >> %d\n", len, ret);
> >> 
> >>        }
> >> 
> >> Needs to turn into something like:
> >> 	ret = spi_xfer(spi, 8 + len * 8, &cmd, response, flags | 
SPI_XFER_END)
> > 
> > this should be easier in my sf branch as i unified a bunch of the
> > functions. but while this will probably work for the main commands, how
> > is this supposed to work for the status polling ?  that function is
> > fundamentally based around setting up a transfer/command and then
> > continuously shifting out a single result and checking it before
> > shifting out another.  for your controller, the only way to make it work
> > is to do the full transaction every time.
> 
> Probably have to do a transaction every time.

looking at the Linux driver, it seems to do just that.  i guess if Linux is 
getting by with a stricter API, then there shouldnt be a problem for U-Boot 
either.  i dont suppose anyone knows of devices that are problematic in Linux 
or would be broken in U-Boot by this API change in general ?

this assumes of course that the SPI API as used in Linux works for you ?

> Do you have a tree for these changes?  Do you expect them to be in place
> for release after v2011.03

the sf branch of my blackfin tree.  if no one gives me any feedback (i posted 
the patches on the list a while ago), i guess i'll see about merging post the 
next release.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110217/67e718d7/attachment.pgp 

[U-Boot] spi subystem maintainer?

[Mike Frysinger]

On Thursday, February 17, 2011 00:04:35 Mike Frysinger wrote:
> On Tuesday, February 15, 2011 18:10:47 Kumar Gala wrote:
> > On Feb 15, 2011, at 2:36 AM, Mike Frysinger wrote:
> > > On Thursday, February 03, 2011 05:36:38 Kumar Gala wrote:
> > >> Needs to turn into something like:
> > >> 	ret = spi_xfer(spi, 8 + len * 8, &cmd, response, flags | SPI_XFER_END)
> > >
> > > this should be easier in my sf branch as i unified a bunch of the
> > > functions. but while this will probably work for the main commands, how
> > > is this supposed to work for the status polling ?  that function is
> > > fundamentally based around setting up a transfer/command and then
> > > continuously shifting out a single result and checking it before
> > > shifting out another.  for your controller, the only way to make it
> > > work is to do the full transaction every time.
> > 
> > Probably have to do a transaction every time.
> 
> looking at the Linux driver, it seems to do just that.  i guess if Linux is
> getting by with a stricter API, then there shouldnt be a problem for U-Boot
> either.  i dont suppose anyone knows of devices that are problematic in
> Linux or would be broken in U-Boot by this API change in general ?
> 
> this assumes of course that the SPI API as used in Linux works for you ?

seems this stalled because you guys instead took a queue+flush approach in 
your spi bus.  but ive come across another hardware bus which (sounds like it) 
has the same limitations -- the ICH SPI controller from intel.  you have to 
program into its hardware registers the command, the optional address, and 
whether you're going to be reading/writing afterwards.  they needed to drop 
the SPI_XFER_{START,END} flags just like you in order to make things work.

further, while talking to people, it isnt as big of a deal as i had originally 
thought.  the overhead from polling the SPI flash does go up slightly, but 
really only like by one byte on the bus (and the overhead to setup/bring down 
the transfer, but that's fairly minuscule).

so what i'm thinking is that since no other flags have materialized, we punt 
the SPI_XFER_{BEGIN,END} and make the API more Linux like.  when i looked 
through the source, i couldnt find any other consumers that'd be adversely 
affected.

any other thoughts on the matter ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110818/8c3ec1bf/attachment.pgp