SPI chip select problem

SPI chip select problem

[Antony Pavlov]

Hi!

I have added spi controller driver for one of my MIPS boards and
found, that there is a problem with chip select.

During initialisation we call *_spi_setup() method. It switch chip
select and frequency for every probing spi slave chip.
But after initialisation __we never__ call this method. So if I have
more than 1 spi slave chip, I can use only last of them.

There is the 'cs_change' flag for *_spi_transfer() method, but this
flag does not used at all!

I have made quick-and-dirty patch:

--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -196,6 +196,8 @@ EXPORT_SYMBOL(spi_register_master);

 int spi_sync(struct spi_device *spi, struct spi_message *message)
 {
+       spi->master->setup(spi);
+
        return spi->master->transfer(spi, message);
 }

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: SPI chip select problem

[Johannes Stezenbach]

Hi,

On Mon, Jun 25, 2012 at 11:45:06AM +0400, Antony Pavlov wrote:
> I have added spi controller driver for one of my MIPS boards and
> found, that there is a problem with chip select.
> 
> During initialisation we call *_spi_setup() method. It switch chip
> select and frequency for every probing spi slave chip.
> But after initialisation __we never__ call this method. So if I have
> more than 1 spi slave chip, I can use only last of them.
> 
> There is the 'cs_change' flag for *_spi_transfer() method, but this
> flag does not used at all!

altera_spi.c and mic_spi.c use it, but often this is not needed (e.g. for
SPI flashes) so to keep the code simple and small it might be better
to not implement cs_change handling.

> I have made quick-and-dirty patch:
> 
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -196,6 +196,8 @@ EXPORT_SYMBOL(spi_register_master);
> 
>  int spi_sync(struct spi_device *spi, struct spi_message *message)
>  {
> +       spi->master->setup(spi);
> +
>         return spi->master->transfer(spi, message);
>  }

I noticed this issue, too, but since barebox spi code is similar
to linux code I'd like to point out linux drivers must not modify the
chip select in ->setup().  Since bare box only does synchronous transfers
it isn't an issue, but in linux ->setup() is called when
a new message is queued, at this time the previous message
might still be transferring.  Thus the only purpose of ->setup()
is to do error checking on the provided parameters.
Chip select, frequency etc. must be set in ->transfer().
In the interest of portability/similarity barebox drivers should
do the same as linux.

That said, your proposed patch still looks OK.
BTW, ->cleanup() is also never called, but so far no driver needs it...


Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: SPI chip select problem

[Antony Pavlov]

On 25 June 2012 12:53, Johannes Stezenbach <js@sig21.net> wrote:
> Hi,
>
> On Mon, Jun 25, 2012 at 11:45:06AM +0400, Antony Pavlov wrote:
>> I have added spi controller driver for one of my MIPS boards and
>> found, that there is a problem with chip select.
>>
>> During initialisation we call *_spi_setup() method. It switch chip
>> select and frequency for every probing spi slave chip.
>> But after initialisation __we never__ call this method. So if I have
>> more than 1 spi slave chip, I can use only last of them.
>>
>> There is the 'cs_change' flag for *_spi_transfer() method, but this
>> flag does not used at all!
>
> altera_spi.c and mic_spi.c use it, but often this is not needed (e.g. for
> SPI flashes) so to keep the code simple and small it might be better
> to not implement cs_change handling.

But I have not found any common code where cs_change is managed.

>
>> I have made quick-and-dirty patch:
>>
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -196,6 +196,8 @@ EXPORT_SYMBOL(spi_register_master);
>>
>>  int spi_sync(struct spi_device *spi, struct spi_message *message)
>>  {
>> +       spi->master->setup(spi);
>> +
>>         return spi->master->transfer(spi, message);
>>  }
>
> I noticed this issue, too, but since barebox spi code is similar
> to linux code I'd like to point out linux drivers must not modify the
> chip select in ->setup().  Since bare box only does synchronous transfers
> it isn't an issue, but in linux ->setup() is called when
> a new message is queued, at this time the previous message
> might still be transferring.  Thus the only purpose of ->setup()
> is to do error checking on the provided parameters.
> Chip select, frequency etc. must be set in ->transfer().
> In the interest of portability/similarity barebox drivers should
> do the same as linux.
>
> That said, your proposed patch still looks OK.
> BTW, ->cleanup() is also never called, but so far no driver needs it...
>
>
> Johannes



-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: SPI chip select problem

[Johannes Stezenbach]

On Mon, Jun 25, 2012 at 02:07:34PM +0400, Antony Pavlov wrote:
> On 25 June 2012 12:53, Johannes Stezenbach <js@sig21.net> wrote:
> > On Mon, Jun 25, 2012 at 11:45:06AM +0400, Antony Pavlov wrote:
> >>
> >> There is the 'cs_change' flag for *_spi_transfer() method, but this
> >> flag does not used at all!
> >
> > altera_spi.c and mic_spi.c use it, but often this is not needed (e.g. for
> > SPI flashes) so to keep the code simple and small it might be better
> > to not implement cs_change handling.
> 
> But I have not found any common code where cs_change is managed.

It doesn't need to be handled in common code, the spi_device
driver requests a cs_change if it needs one, the spi_master driver
programs the hardware to do it.  Common code in drivers/spi/spi.c
doesn't need to care.  (To clarify: cs_change does not mean
to switch from one CS to another, it means to pulse the
CS line between spi_transfers of a spi_message.  Some devices
need that in some cases.)


Johannes

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox