[U-Boot-Users] SPI support in U-boot

[U-Boot-Users] SPI support in U-boot

[Vladimir Gurevich]

Hello,

It looks like the basic SPI I/O function, spi_xfer() has two different 
prototypes in U-boot.

   1. in common/soft_spi.c, include/spi.h and cpu/nio/spi.c  it is
      declared as

       int spi_xfer(spi_chipsel_type chipsel, int bitlen, uchar *dout,
      uchar *din)

   2. in SPI drivers for MPC CPUs (cpu/mpc{8xx, 8260, 5xxx}/spi.c) it is
      declared as

       ssize_t spi_xfer (size_t count)

The code for "sspi" command (do_spi()) definitely uses the first 
prototype. Subsequently it doesn't work on the above mentioned CPUs 
(unless you use software SPI implementation).

On the other hand, the code for the "eeprom" command, which is another 
way to access an SPI EEPROM uses functions spi_read() and spi_write, 
which are implemented only in cpu/mpc{8xx, 8260, 5xxx}/spi.c. However, 
these functions are very basic. Fore example they do not allow you to 
control SPI chip select.

Can someone tell me what is going on. Is it that MPC drivers became 
outdated and should be modified? How do other people use them then? Or I 
am simply missing something?

Thanks,
Vladimir

[U-Boot-Users] SPI support in U-boot

[Wolfgang Denk]

In message <43D44482.7030300@paulidav.org> you wrote:
> 
> It looks like the basic SPI I/O function, spi_xfer() has two different 
> prototypes in U-boot.

This is what happens when two groups of people solve the same problem
independently.

> On the other hand, the code for the "eeprom" command, which is another 
> way to access an SPI EEPROM uses functions spi_read() and spi_write, 
> which are implemented only in cpu/mpc{8xx, 8260, 5xxx}/spi.c. However, 
> these functions are very basic. Fore example they do not allow you to 
> control SPI chip select.

That's because they just deal  with  that:  accessing  a  SPI  EEPROM
device.

> Can someone tell me what is going on. Is it that MPC drivers became 
> outdated and should be modified? How do other people use them then? Or I 
> am simply missing something?

They are not "outdated". It's just  a  different  (and  incompatible)
implementation.  If you can come up with a patch tp cleanup please do
so.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"When the only  tool  you  have  is  a  hammer,  you  tend  to  treat
everything as if it were a nail."                    - Abraham Maslow

[U-Boot-Users] SPI support in U-boot

[Vladimir Gurevich]

Hello Wolfgang,

Wolfgang Denk wrote:

>They are not "outdated". It's just  a  different  (and  incompatible)
>implementation.  If you can come up with a patch tp cleanup please do
>so.
>  
>
I decided to do that (and it was pretty easy to do), but now I have even 
more questions...

The major issue is the way chip selects are controlled. Currently, 
do_spi() function that implements "sspi" command calls spi_xfer() this way:

    spi_xfer(spi_chipsel[device], bitlen, dout, din)

where spi_chipsel is a global array of pointers to functions that are 
supposed to assert/de-assert chip selects for the specified target(s).

I looked at the code for the boards that use this mechanism, and I can 
see the array statically initialized, like (in board/sacsng/sacsng.c):

    /*
     * The SPI command uses this table of functions for controlling the SPI
     * chip selects: it calls the appropriate function to control the SPI
     * chip selects.
     */
    spi_chipsel_type spi_chipsel[] = {
        spi_adc_chipsel,
        spi_dac_chipsel
    };
    int spi_chipsel_cnt = sizeof(spi_chipsel) / sizeof(spi_chipsel[0]);

My question is: where these addresses are relocated? My understanding is 
that relocation for this type of data should be done manually, but 
nowhere in the code can I see it. Not for a single board. That means 
that if people got lucky, they execute the copy of the code from the 
FLASH, not the relocated one.

Is that OK? I also noticed the same mechanism being used in the 
FPGA-related code.

And another question. The current implementation(s) of the "eeprom" 
command assume that there is only 1 SPI device and do not bothr 
themselves with the chip selects at all. That means, that if you try to 
execute "eeprom" command after you executed "sspi" (that will de-assert 
the chip-select at the end or can choose a different one), the results 
will be unpredictable. I have no problem modifying "eeprom" command for 
my board, but this will force other people to do modifications as well, 
so I am not sure what should we do.

Thanks,
Vladimir

[U-Boot-Users] SPI support in U-boot

[Wolfgang Denk]

Dear Vladimir,

in message <43D733BF.4030207@paulidav.org> you wrote:
> 
> I looked at the code for the boards that use this mechanism, and I can 
> see the array statically initialized, like (in board/sacsng/sacsng.c):
...
> My question is: where these addresses are relocated? My understanding is 
> that relocation for this type of data should be done manually, but 
> nowhere in the code can I see it. Not for a single board. That means 
> that if people got lucky, they execute the copy of the code from the 
> FLASH, not the relocated one.

Good catch!

> Is that OK? I also noticed the same mechanism being used in the 
> FPGA-related code.

I think your interpretation is correct.

> And another question. The current implementation(s) of the "eeprom" 
> command assume that there is only 1 SPI device and do not bothr 
> themselves with the chip selects at all. That means, that if you try to 
> execute "eeprom" command after you executed "sspi" (that will de-assert 
> the chip-select at the end or can choose a different one), the results 
> will be unpredictable. I have no problem modifying "eeprom" command for 
> my board, but this will force other people to do modifications as well, 
> so I am not sure what should we do.

It would be nice to preserve the currnt behaviour, so a patch  should
try  to  implement the required changes for all boiards. Testing will
have to be performed by the respective board maintainers, of course.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I've got to get something inside me. Some coffee  or  something.  And
then the world will somehow be better.
                                     - Terry Pratchett, _Men at Arms_

[U-Boot-Users] SPI support in U-boot

[Vladimir Gurevich]

Hello Wolfgang,

Wolfgang Denk wrote:

>Good catch!
>  
>
>>Is that OK? I also noticed the same mechanism being used in the 
>>FPGA-related code.
>>    
>>
>
>I think your interpretation is correct.
>  
>
And? :)

OK, so here are the questions:

   1. Should the code be left like that (i.e. some parts are executing
      from FLASH instead of RAM) because it "just works for me" or it is
      not acceptable to code like that?
   2. What is the right way to do that: to use an uninitialized variable
      and initialize it from board_misc_init_r with something like

                   myfuncptr = &myfunc + bd->reloc_off

      or to have initialized variable myfuncptr = myfunc and simply do
      myfuncptr += bd->reloc_off in board_misc_init_r?
   3. Ironically enough, I noticed that fpga_init() is declared like 
      extern void fpga_init( ulong reloc_off ), but all the
      implementations use void fpga_init(void)!
   4. Wouldn't it make sense to add a small section to README,
      explaining how to use function pointer tables in U-boot (not sure
      how much sense it makes to begin with).

>It would be nice to preserve the currnt behaviour, so a patch  should
>try  to  implement the required changes for all boiards. Testing will
>have to be performed by the respective board maintainers, of course.
>  
>
This seems to be difficult, since the current interface only allows you 
to assert/de-assert a particular chipselect, but not to query which one 
is currently active (so that "sspi" command could restore what was 
before it. I guess, I can do three things:

   1. Extend sspi command, so that if there is only 1 arg, chip_select,
      it will simply assert it and do nothing else
   2. Hope, that people who use "eeprom" command don't really care about
      "sspi" so far, otherwise it would be working!
   3. Document the changes.

Thanks,
Vladimir