Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-05-03 Thread Tom Warren
On Mon, May 2, 2011 at 4:16 PM, Mike Frysinger vap...@gentoo.org wrote:
 On Monday, May 02, 2011 19:06:51 Tom Warren wrote:
 Seaboard has a touchscreen on SPI1, but U-Boot doesn't care about that.

 fwiw, u-boot has splash screen support
 -mike

Thanks, Mike. I'm talking touchscreen support. We'll be submitting LCD
support for Tegra2 at a later date, so splash screens can be shown by
ODMs, etc.

Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-05-03 Thread Mike Frysinger
On Tue, May 3, 2011 at 11:13, Tom Warren wrote:
 On Mon, May 2, 2011 at 4:16 PM, Mike Frysinger wrote:
 On Monday, May 02, 2011 19:06:51 Tom Warren wrote:
 Seaboard has a touchscreen on SPI1, but U-Boot doesn't care about that.

 fwiw, u-boot has splash screen support

 Thanks, Mike. I'm talking touchscreen support. We'll be submitting LCD
 support for Tegra2 at a later date, so splash screens can be shown by
 ODMs, etc.

you could implement a simple console input driver for the touchscreen
(translate the gestures into up/down/enter/etc...), but you're
probably treading way into new territory for u-boot if you do :)
-mike
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-05-02 Thread Tom Warren
Mike,

On Fri, Apr 29, 2011 at 4:21 PM, Mike Frysinger vap...@gentoo.org wrote:
 On Thursday, April 28, 2011 10:55:13 Tom Warren wrote:
 Signed-off-by: Tom Warren twar...@nvidia.com
 ---
 This patch adds support for the SPIFLASH peripheral on Tegra2 (SPI 1).
 Probe, erase, read and write are all supported, as well as low-level
 SPI commands via 'sspi'.

 Note that, at this time, only Seaboard has a SPI flash part (Winbond).
 With this code, I've written U-Boot to SPI from within U-Boot, and
 booted the system from that SPI image (boot sel jumpers must be set
 to SPI [1000], and the UART ENABLE jumper must be set to GPIO CTRL).

 so this is a driver for a SPI bus, and the board you're working on just
 happens to have some SPI flashes connected to it ?  then the SPIFLASH part
 is irrelevant, as is the board in question.  please remove references to both
 and just refer to this as a SPI driver for Tegra2 processors.
No, there are 4 general purpose SPI interfaces in Tegra2 (SLINK), and
1 dedicated SPI FLASH controller. From the Tegra2 TRM:

The Serial Flash interface (SFLASH) Controller interfaces Tegra 2 to
SPI-capable devices such as Flash
memories and ADC/DAC devices. Tegra 2 supports only master mode of SPI
operation on this interface.
This interface is specifically intended for serial flash and similar
devices. For a general purpose SPI
interface, use the SLINK serial interface.

The register sets are similar, but not identical.

As to the board reference, it's in my comments so people wishing to
use this will know that only Seaboard is available w/a SPI chip -
there's no provision for that on the Harmony board.


 --- /dev/null
 +++ b/arch/arm/include/asm/arch-tegra2/tegra2_spi.h

 if the only thing that uses this is the spi driver, then please put this in
 drivers/spi/tegra2_spi.h
Will do.

 +#define      SPI_CMD_GO              (1  30)

 dont put a tab after the #define

OK. I'll fix 'em up.

 --- a/arch/arm/lib/board.c
 +++ b/arch/arm/lib/board.c

 this change needs to be split out into a dedicated one

 -

 this new line should not be deleted

 +#if defined(CONFIG_CMD_SPI)
 +#include spi.h
 +#endif

 no need to protect the inclusion.  just always include it.
OK.

  #endif
 -
  #if defined(CONFIG_CMD_NAND)

 nor should this newline be deleted

       puts (NAND:  );
       nand_init();            /* go init the NAND */
  #endif
 -
 +#if defined(CONFIG_CMD_SPI)

 nor this newline
I'm going to remove these changes from arm/lib/board.c and move
spi_init() to our common board file (nvidia/common/board.c.


 +int spi_claim_bus(struct spi_slave *slave)
 +{
 +     /* Move bulk of spi_init code here? */

 yes, so do that

OK, I'll look into that.

 +void spi_release_bus(struct spi_slave *slave)
 +{

 this func should be disabling the spi bus
OK, I'll look into that, too.


 +void spi_cs_activate(struct spi_slave *slave)
 +{
 +     struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr 
 *)NV_PA_APB_MISC_BASE;
 +     struct spi_tegra *spi = (struct spi_tegra *)TEGRA2_SPI_BASE;
 +     u32 val;
 +
 +     /*
 +      * Delay here to clean up comms - spurious chars seen around SPI xfers.
 +      * Fine-tune later.
 +      */
 +     udelay(1000);

 fine tune now ?
The comment went in during development, and the 1000 values is the
final fine-tuned value (1ms). So I'll change the comment.


 +     /*
 +      * On Seaboard, MOSI/MISO are shared w/UART.
 +      * Use GPIO I3 (UART_DISABLE) to tristate UART during SPI activity.
 +      * Enable UART later (cs_deactivate) so we can use it for U-Boot comms.
 +      */
 +     gpio_direction_output(UART_DISABLE_GPIO, 1);

 board specific issues shouldnt be in a processor driver.
Please explain what you mean by a processor driver.  This does need an
ifdef, so any future board that doesn't have Seaboard's defiency of a
muxed SPIFLASH/UART won't have to mess with a GPIO, but for Seaboard,
we have to dynamically change GPIO_PI3 every SPI transaction to ensure
UART spew can continue (in DEBUG builds, etc.).

 +     int numBytes = (bitlen + 7) / 8;

 no camel case.  use num_bytes.
Sure, will do. This came over from the driver I ported from.


 +                                     for (i = 0; i  bytes; ++i) {
 +                                             ((u8 *)din)[i] = (tmpdin  
 24);

 create a dedicated pointer and deference that.  casts on the lhs are poor
 style.
Again, this was in the driver source I used as a porting reference.
I'll change it.


 --- a/include/configs/seaboard.h
 +++ b/include/configs/seaboard.h

 this should be split out into a dedicated patch
OK.

Thanks,

Tom
 -mike

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-05-02 Thread Mike Frysinger
On Monday, May 02, 2011 11:33:24 Tom Warren wrote:
 On Fri, Apr 29, 2011 at 4:21 PM, Mike Frysinger wrote:
  On Thursday, April 28, 2011 10:55:13 Tom Warren wrote:
  This patch adds support for the SPIFLASH peripheral on Tegra2 (SPI 1).
  Probe, erase, read and write are all supported, as well as low-level
  SPI commands via 'sspi'.
  
  Note that, at this time, only Seaboard has a SPI flash part (Winbond).
  With this code, I've written U-Boot to SPI from within U-Boot, and
  booted the system from that SPI image (boot sel jumpers must be set
  to SPI [1000], and the UART ENABLE jumper must be set to GPIO CTRL).
  
  so this is a driver for a SPI bus, and the board you're working on just
  happens to have some SPI flashes connected to it ?  then the SPIFLASH
  part is irrelevant, as is the board in question.  please remove
  references to both and just refer to this as a SPI driver for Tegra2
  processors.
 
 No, there are 4 general purpose SPI interfaces in Tegra2 (SLINK), and
 1 dedicated SPI FLASH controller. From the Tegra2 TRM:
 
 The Serial Flash interface (SFLASH) Controller interfaces Tegra 2 to
 SPI-capable devices such as Flash
 memories and ADC/DAC devices. Tegra 2 supports only master mode of SPI
 operation on this interface.
 This interface is specifically intended for serial flash and similar
 devices. For a general purpose SPI
 interface, use the SLINK serial interface.
 
 The register sets are similar, but not identical.

do you plan on writing a driver for the SLINK logic ?  would it be a new file, 
or would you keep the two integrated ?  if diff files, i'd name this one 
tegra2_sflash_spi.c.  if you keep them merged, then tegra2_spi.c is fine.

 As to the board reference, it's in my comments so people wishing to
 use this will know that only Seaboard is available w/a SPI chip -
 there's no provision for that on the Harmony board.

generally, processor drivers are not concerned with board issues.  if you want 
to document board-specific issues somewhere, add a doc/README.boardname.

  --- a/arch/arm/lib/board.c
  +++ b/arch/arm/lib/board.c
  +#if defined(CONFIG_CMD_SPI)
 
 I'm going to remove these changes from arm/lib/board.c and move
 spi_init() to our common board file (nvidia/common/board.c.

i dont think you want to do this.  other than the style changes, and needing a 
sep changeset, having the arm board.c file call spi_init() is desirable.

  +  * On Seaboard, MOSI/MISO are shared w/UART.
  +  * Use GPIO I3 (UART_DISABLE) to tristate UART during SPI
  activity.
  +  * Enable UART later (cs_deactivate) so we can use it
  for U-Boot comms.
  + gpio_direction_output(UART_DISABLE_GPIO, 1);
  
  board specific issues shouldnt be in a processor driver.
 
 Please explain what you mean by a processor driver.

you're writing a driver for the Tegra2 SoC.  that is the only thing this 
driver should be doing.  that means no board-specific logic is allowed.

 This does need an
 ifdef, so any future board that doesn't have Seaboard's defiency of a
 muxed SPIFLASH/UART won't have to mess with a GPIO, but for Seaboard,
 we have to dynamically change GPIO_PI3 every SPI transaction to ensure
 UART spew can continue (in DEBUG builds, etc.).

right, this is a board-specific issue.  thus it doesnt belong in a driver that 
is only concerned with the Tegra2 SoC.

what you could do is:
void spi_init_common(void) { ... only tegra2 code ... }
void spi_init(void) __attribute__((weak, alias(spi_init_common)));

and then in the seaboard directory, add:
void spi_init(void)
{
... do random gpio/portmux stuff ...
spi_init_common();
}

or another style would be to:
extern void spi_board_init(void) __weak;
void spi_init(void)
{
if (spi_board_init())
spi_board_init();
... only tegra2 code ...
}

and then in the seaboard directory, add:
void spi_board_init(void)
{
... do random gpio/portmux stuff ...
}
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-05-02 Thread Tom Warren
Mike,

On Mon, May 2, 2011 at 2:26 PM, Mike Frysinger vap...@gentoo.org wrote:
 On Monday, May 02, 2011 11:33:24 Tom Warren wrote:
 On Fri, Apr 29, 2011 at 4:21 PM, Mike Frysinger wrote:
  On Thursday, April 28, 2011 10:55:13 Tom Warren wrote:
  This patch adds support for the SPIFLASH peripheral on Tegra2 (SPI 1).
  Probe, erase, read and write are all supported, as well as low-level
  SPI commands via 'sspi'.
 
  Note that, at this time, only Seaboard has a SPI flash part (Winbond).
  With this code, I've written U-Boot to SPI from within U-Boot, and
  booted the system from that SPI image (boot sel jumpers must be set
  to SPI [1000], and the UART ENABLE jumper must be set to GPIO CTRL).
 
  so this is a driver for a SPI bus, and the board you're working on just
  happens to have some SPI flashes connected to it ?  then the SPIFLASH
  part is irrelevant, as is the board in question.  please remove
  references to both and just refer to this as a SPI driver for Tegra2
  processors.

 No, there are 4 general purpose SPI interfaces in Tegra2 (SLINK), and
 1 dedicated SPI FLASH controller. From the Tegra2 TRM:

 The Serial Flash interface (SFLASH) Controller interfaces Tegra 2 to
 SPI-capable devices such as Flash
 memories and ADC/DAC devices. Tegra 2 supports only master mode of SPI
 operation on this interface.
 This interface is specifically intended for serial flash and similar
 devices. For a general purpose SPI
 interface, use the SLINK serial interface.

 The register sets are similar, but not identical.

 do you plan on writing a driver for the SLINK logic ?  would it be a new file,
 or would you keep the two integrated ?  if diff files, i'd name this one
 tegra2_sflash_spi.c.  if you keep them merged, then tegra2_spi.c is fine.
No plan to write SLINK drivers for U-Boot.  Seaboard has a touchscreen
on SPI1, but U-Boot doesn't care about that.
So I'll keep it tegra2_spi.c for now.


 As to the board reference, it's in my comments so people wishing to
 use this will know that only Seaboard is available w/a SPI chip -
 there's no provision for that on the Harmony board.

 generally, processor drivers are not concerned with board issues.  if you want
 to document board-specific issues somewhere, add a doc/README.boardname.

Good idea. I'll gen one up for Seaboard.

  --- a/arch/arm/lib/board.c
  +++ b/arch/arm/lib/board.c
  +#if defined(CONFIG_CMD_SPI)

 I'm going to remove these changes from arm/lib/board.c and move
 spi_init() to our common board file (nvidia/common/board.c.

 i dont think you want to do this.  other than the style changes, and needing a
 sep changeset, having the arm board.c file call spi_init() is desirable.
OK. I'll fix up arm board.c with your comments in mind and put it in a
separate patch.


  +      * On Seaboard, MOSI/MISO are shared w/UART.
  +      * Use GPIO I3 (UART_DISABLE) to tristate UART during SPI
  activity.
  +      * Enable UART later (cs_deactivate) so we can use it
  for U-Boot comms.
  +     gpio_direction_output(UART_DISABLE_GPIO, 1);
 
  board specific issues shouldnt be in a processor driver.

 Please explain what you mean by a processor driver.

 you're writing a driver for the Tegra2 SoC.  that is the only thing this
 driver should be doing.  that means no board-specific logic is allowed.

 This does need an
 ifdef, so any future board that doesn't have Seaboard's defiency of a
 muxed SPIFLASH/UART won't have to mess with a GPIO, but for Seaboard,
 we have to dynamically change GPIO_PI3 every SPI transaction to ensure
 UART spew can continue (in DEBUG builds, etc.).

 right, this is a board-specific issue.  thus it doesnt belong in a driver that
 is only concerned with the Tegra2 SoC.

 what you could do is:
 void spi_init_common(void) { ... only tegra2 code ... }
 void spi_init(void) __attribute__((weak, alias(spi_init_common)));

 and then in the seaboard directory, add:
 void spi_init(void)
 {
        ... do random gpio/portmux stuff ...
        spi_init_common();
 }

 or another style would be to:
 extern void spi_board_init(void) __weak;
 void spi_init(void)
 {
        if (spi_board_init())
                spi_board_init();
        ... only tegra2 code ...
 }

 and then in the seaboard directory, add:
 void spi_board_init(void)
 {
        ... do random gpio/portmux stuff ...
 }

I'll try out these ideas. Thanks.

Tom
 -mike

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-05-02 Thread Mike Frysinger
On Monday, May 02, 2011 19:06:51 Tom Warren wrote:
 Seaboard has a touchscreen on SPI1, but U-Boot doesn't care about that.

fwiw, u-boot has splash screen support
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH] spi: Tegra2: Add SPI driver for SPIFLASH on Seaboard

2011-04-29 Thread Mike Frysinger
On Thursday, April 28, 2011 10:55:13 Tom Warren wrote:
 Signed-off-by: Tom Warren twar...@nvidia.com
 ---
 This patch adds support for the SPIFLASH peripheral on Tegra2 (SPI 1).
 Probe, erase, read and write are all supported, as well as low-level
 SPI commands via 'sspi'.
 
 Note that, at this time, only Seaboard has a SPI flash part (Winbond).
 With this code, I've written U-Boot to SPI from within U-Boot, and
 booted the system from that SPI image (boot sel jumpers must be set
 to SPI [1000], and the UART ENABLE jumper must be set to GPIO CTRL).

so this is a driver for a SPI bus, and the board you're working on just 
happens to have some SPI flashes connected to it ?  then the SPIFLASH part 
is irrelevant, as is the board in question.  please remove references to both 
and just refer to this as a SPI driver for Tegra2 processors.

 --- /dev/null
 +++ b/arch/arm/include/asm/arch-tegra2/tegra2_spi.h

if the only thing that uses this is the spi driver, then please put this in 
drivers/spi/tegra2_spi.h

 +#define  SPI_CMD_GO  (1  30)

dont put a tab after the #define

 --- a/arch/arm/lib/board.c
 +++ b/arch/arm/lib/board.c

this change needs to be split out into a dedicated one

 -

this new line should not be deleted

 +#if defined(CONFIG_CMD_SPI)
 +#include spi.h
 +#endif

no need to protect the inclusion.  just always include it.

  #endif
 -
  #if defined(CONFIG_CMD_NAND)

nor should this newline be deleted

   puts (NAND:  );
   nand_init();/* go init the NAND */
  #endif
 -
 +#if defined(CONFIG_CMD_SPI)

nor this newline

 +int spi_claim_bus(struct spi_slave *slave)
 +{
 + /* Move bulk of spi_init code here? */

yes, so do that

 +void spi_release_bus(struct spi_slave *slave)
 +{

this func should be disabling the spi bus

 +void spi_cs_activate(struct spi_slave *slave)
 +{
 + struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr *)NV_PA_APB_MISC_BASE;
 + struct spi_tegra *spi = (struct spi_tegra *)TEGRA2_SPI_BASE;
 + u32 val;
 +
 + /*
 +  * Delay here to clean up comms - spurious chars seen around SPI xfers.
 +  * Fine-tune later.
 +  */
 + udelay(1000);

fine tune now ?

 + /*
 +  * On Seaboard, MOSI/MISO are shared w/UART.
 +  * Use GPIO I3 (UART_DISABLE) to tristate UART during SPI activity.
 +  * Enable UART later (cs_deactivate) so we can use it for U-Boot comms.
 +  */
 + gpio_direction_output(UART_DISABLE_GPIO, 1);

board specific issues shouldnt be in a processor driver

 + int numBytes = (bitlen + 7) / 8;

no camel case.  use num_bytes.

 + for (i = 0; i  bytes; ++i) {
 + ((u8 *)din)[i] = (tmpdin  24);

create a dedicated pointer and deference that.  casts on the lhs are poor 
style.

 --- a/include/configs/seaboard.h
 +++ b/include/configs/seaboard.h

this should be split out into a dedicated patch
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot