Do not mux and setup SPI if disabled in the config

PATCH New Clemens Gruber 3 participants 9 messages Aug 4, 2015 – Aug 5, 2015

lore

Share & tools

lore

Clemens Gruber

author

2015-08-04 17:28 UTC #0 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

This allows baseboards without SPI to compile.

Signed-off-by Clemens Gruber <[email protected]>

Cc Markus Niebel <[email protected]>

Cc Tom Rini <[email protected]>

---
 board/tqc/tqma6/tqma6.c | 6 ++++++
 1 file changed, 6 insertions(+)

board/tqc/tqma6/tqma6.c +6 -0

index 29db838..5c4d104 100644
@@ -131,6 +131,8 @@ int board_mmc_init(bd_t *bis)
 	return 0;
 }
 
+#if defined(CONFIG_MXC_SPI)
+
 static iomux_v3_cfg_t const tqma6_ecspi1_pads[] = {
 	/* SS1 */
 	NEW_PAD_CTRL(MX6_PAD_EIM_D19__GPIO3_IO19, SPI_PAD_CTRL),

@@ -161,6 +163,8 @@ int board_spi_cs_gpio(unsigned bus, unsigned cs)
 		(cs == CONFIG_SF_DEFAULT_CS)) ? TQMA6_SF_CS_GPIO : -1;
 }
 
+#endif /* defined(CONFIG_MXC_SPI) */
+
 static struct i2c_pads_info tqma6_i2c3_pads = {
 	/* I2C3: on board LM75, M24C64,  */
 	.scl = {

@@ -201,7 +205,9 @@ int board_init(void)
 	/* address of boot parameters */
 	gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
 
+#if defined(CONFIG_MXC_SPI)
 	tqma6_iomuxc_spi();
+#endif
 	tqma6_setup_i2c();
 
 	tqma6_bb_board_init();

Signature · 1 line

2.5.0

Stefan Roese

2015-08-05 05:19 UTC #1 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

(added Stefano as imx custodian)

Quoted 32 lines · Clemens Gruber

This allows baseboards without SPI to compile.

Signed-off-by Clemens Gruber <[email protected]>

Cc Markus Niebel <[email protected]>

Cc Tom Rini <[email protected]>

---
  board/tqc/tqma6/tqma6.c | 6 ++++++
  1 file changed, 6 insertions(+)

board/tqc/tqma6/tqma6.c +4 -0

index 29db838..5c4d104 100644
@@ -131,6 +131,8 @@ int board_mmc_init(bd_t *bis)
  	return 0;
  }
 
+#if defined(CONFIG_MXC_SPI)
+
  static iomux_v3_cfg_t const tqma6_ecspi1_pads[] = {
  	/* SS1 */
  	NEW_PAD_CTRL(MX6_PAD_EIM_D19__GPIO3_IO19, SPI_PAD_CTRL),

@@ -161,6 +163,8 @@ int board_spi_cs_gpio(unsigned bus, unsigned cs)
  		(cs == CONFIG_SF_DEFAULT_CS)) ? TQMA6_SF_CS_GPIO : -1;
  }
 
+#endif /* defined(CONFIG_MXC_SPI) */
+
  static struct i2c_pads_info tqma6_i2c3_pads = {
  	/* I2C3: on board LM75, M24C64,  */
  	.scl = {

board/tqc/tqma6/tqma6.c int board_init(void) · line 211

Quoted 7 lines

board/tqc/tqma6/tqma6.c +2 -0

@@ -201,7 +205,9 @@ int board_init(void)
  	/* address of boot parameters */
  	gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
 
+#if defined(CONFIG_MXC_SPI)
  	tqma6_iomuxc_spi();
+#endif
  	tqma6_setup_i2c();

Hmmm. This should not be necessary. Please take a look at the wru4 baseboard addition that is already in mainline. Especially this patch:

commit 34ee786e5b388625e5a0e0891ee69d4bbf34178b

Author: Stefan Roese <[email protected]>
Date:   Thu Mar 12 13:34:30 2015 +0100

arm: mx6: tqma6: Update to optionally configure an alternative SPI setup

By making the tqma6_iomuxc_spi() weak, this patch adds the possibility to add a different function for this SPI configuration. This can be used by other baseboards, that might have a different SPI setup.

This patch will be used by the upcoming WRU-IV board support which also uses the TQMa6 SoM.

Signed-off-by Stefan Roese <[email protected]>

This is enough in your baseboard file now:

void tqma6_iomuxc_spi(void)

{
        /* No SPI on this baseboard */
}

Thanks,
Stefan

Clemens Gruber

author

2015-08-05 07:28 UTC #2 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

Hi Stefan,

Quoted 1 line · Stefan Roese

(added Stefano as imx custodian)

Thanks.

Quoted 37 lines · Clemens Gruber

Quoted 35 lines

This allows baseboards without SPI to compile.

Signed-off-by Clemens Gruber <[email protected]>

Cc Markus Niebel <[email protected]>

Cc Tom Rini <[email protected]>

---
  board/tqc/tqma6/tqma6.c | 6 ++++++
  1 file changed, 6 insertions(+)

board/tqc/tqma6/tqma6.c +4 -0

index 29db838..5c4d104 100644
@@ -131,6 +131,8 @@ int board_mmc_init(bd_t *bis)
  	return 0;
  }
 
+#if defined(CONFIG_MXC_SPI)
+
  static iomux_v3_cfg_t const tqma6_ecspi1_pads[] = {
  	/* SS1 */
  	NEW_PAD_CTRL(MX6_PAD_EIM_D19__GPIO3_IO19, SPI_PAD_CTRL),

@@ -161,6 +163,8 @@ int board_spi_cs_gpio(unsigned bus, unsigned cs)
  		(cs == CONFIG_SF_DEFAULT_CS)) ? TQMA6_SF_CS_GPIO : -1;
  }
 
+#endif /* defined(CONFIG_MXC_SPI) */
+
  static struct i2c_pads_info tqma6_i2c3_pads = {
  	/* I2C3: on board LM75, M24C64,  */
  	.scl = {

@@ -201,7 +205,9 @@ int board_init(void)
  	/* address of boot parameters */
  	gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;

Quoted 8 lines

board/tqc/tqma6/tqma6.c int board_init(void) · line 211
Quoted 4 lines
board/tqc/tqma6/tqma6.c +2 -0
+#if defined(CONFIG_MXC_SPI)
  	tqma6_iomuxc_spi();
+#endif
  	tqma6_setup_i2c();
Hmmm. This should not be necessary. Please take a look at the wru4 baseboard addition that is already in mainline. Especially this patch:

I noticed your patch but then we would still have to define CONFIG_SF_DEFAULT_BUS and CONFIG_SF_DEFAULT_CS in our board config or include the tqma6.h config, which defines too many things we do not need.

The problem is, we have different clocks and more c files, so we use a separate board directory and a separate board config without SPI defines and just use obj-y += ../../tqc/tqma6/tqma6.o in front of our object files in the Makefile.

If we don't exclude those SPI parts, we would either have to define those SPI config options (which does not make sense) or move all of our board code in one file under tqc/tqma6 which is not an option, we also define additional clocks via a separate IMX_CONFIG file.

Wouldn't it be better to always exclude this code when not using SPI instead of weakly defining the tqma6_iomuxc_spi function?
Then small baseboards could be put in tqc/tqma6 and larger projects could still just link to tqma6.o and put all their stuff in a separate board directory.

Quoted 19 lines

commit 34ee786e5b388625e5a0e0891ee69d4bbf34178b
Author: Stefan Roese <[email protected]>
Date:   Thu Mar 12 13:34:30 2015 +0100
arm: mx6: tqma6: Update to optionally configure an alternative SPI setup
By making the tqma6_iomuxc_spi() weak, this patch adds the possibility to add a different function for this SPI configuration. This can be used by other baseboards, that might have a different SPI setup.
This patch will be used by the upcoming WRU-IV board support which also uses the TQMa6 SoM.
Signed-off-by Stefan Roese <[email protected]>
This is enough in your baseboard file now:

Quoted 8 lines

void tqma6_iomuxc_spi(void)
{
        /* No SPI on this baseboard */
}
Thanks,
Stefan

Regards,
Clemens

Stefan Roese

2015-08-05 08:05 UTC #3 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

Hi Clemens,

Quoted 45 lines · Clemens Gruber

Quoted 36 lines · Clemens Gruber
Quoted 35 lines
This allows baseboards without SPI to compile.
Signed-off-by Clemens Gruber <[email protected]>
Cc Markus Niebel <[email protected]>
Cc Tom Rini <[email protected]>
---
   board/tqc/tqma6/tqma6.c | 6 ++++++
   1 file changed, 6 insertions(+)
board/tqc/tqma6/tqma6.c +4 -0
index 29db838..5c4d104 100644
@@ -131,6 +131,8 @@ int board_mmc_init(bd_t *bis)
   	return 0;
   }
 
+#if defined(CONFIG_MXC_SPI)
+
   static iomux_v3_cfg_t const tqma6_ecspi1_pads[] = {
   	/* SS1 */
   	NEW_PAD_CTRL(MX6_PAD_EIM_D19__GPIO3_IO19, SPI_PAD_CTRL),
@@ -161,6 +163,8 @@ int board_spi_cs_gpio(unsigned bus, unsigned cs)
   		(cs == CONFIG_SF_DEFAULT_CS)) ? TQMA6_SF_CS_GPIO : -1;
   }
 
+#endif /* defined(CONFIG_MXC_SPI) */
+
   static struct i2c_pads_info tqma6_i2c3_pads = {
   	/* I2C3: on board LM75, M24C64,  */
   	.scl = {
@@ -201,7 +205,9 @@ int board_init(void)
   	/* address of boot parameters */
   	gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
 
Quoted 8 lines
board/tqc/tqma6/tqma6.c int board_init(void) · line 211
Quoted 4 lines
board/tqc/tqma6/tqma6.c +2 -0
+#if defined(CONFIG_MXC_SPI)
   	tqma6_iomuxc_spi();
+#endif
   	tqma6_setup_i2c();
Hmmm. This should not be necessary. Please take a look at the wru4 baseboard addition that is already in mainline. Especially this patch:

Quoted 3 lines

I noticed your patch but then we would still have to define CONFIG_SF_DEFAULT_BUS and CONFIG_SF_DEFAULT_CS in our board config or include the tqma6.h config, which defines too many things we do not need.

I would have expected that all boards using these TQ SoM's include this config header.

Quoted 9 lines

The problem is, we have different clocks and more c files, so we use a separate board directory and a separate board config without SPI defines and just use obj-y += ../../tqc/tqma6/tqma6.o in front of our object files in the Makefile.
If we don't exclude those SPI parts, we would either have to define those SPI config options (which does not make sense) or move all of our board code in one file under tqc/tqma6 which is not an option, we also define additional clocks via a separate IMX_CONFIG file.

Quoted 4 lines

Wouldn't it be better to always exclude this code when not using SPI instead of weakly defining the tqma6_iomuxc_spi function?
Then small baseboards could be put in tqc/tqma6 and larger projects could still just link to tqma6.o and put all their stuff in a separate board directory.

How about this patch:

board/tqc/tqma6/tqma6.c +6 -0

index 29db838..345930f 100644
@@ -28,6 +28,13 @@
 
  #include "tqma6_bb.h"
 
+#ifndef CONFIG_SF_DEFAULT_BUS
+#define CONFIG_SF_DEFAULT_BUS  0
+#endif
+#ifndef CONFIG_SF_DEFAULT_CS
+#define CONFIG_SF_DEFAULT_CS   0
+#endif

Does this work for you?

Thanks,
Stefan

Jagan Teki

2015-08-05 08:16 UTC #4 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

Quoted 93 lines · Stefan Roese

Hi Clemens,
Quoted 49 lines · Clemens Gruber
Quoted 38 lines · Clemens Gruber
Quoted 36 lines
This allows baseboards without SPI to compile.
Signed-off-by Clemens Gruber <[email protected]>
Cc Markus Niebel <[email protected]>
Cc Tom Rini <[email protected]>
---
   board/tqc/tqma6/tqma6.c | 6 ++++++
   1 file changed, 6 insertions(+)
board/tqc/tqma6/tqma6.c +4 -0
index 29db838..5c4d104 100644
@@ -131,6 +131,8 @@ int board_mmc_init(bd_t *bis)
        return 0;
   }
 
+#if defined(CONFIG_MXC_SPI)
+
   static iomux_v3_cfg_t const tqma6_ecspi1_pads[] = {
        /* SS1 */
        NEW_PAD_CTRL(MX6_PAD_EIM_D19__GPIO3_IO19, SPI_PAD_CTRL),
@@ -161,6 +163,8 @@ int board_spi_cs_gpio(unsigned bus, unsigned cs)
                (cs == CONFIG_SF_DEFAULT_CS)) ? TQMA6_SF_CS_GPIO : -1;
   }
 
+#endif /* defined(CONFIG_MXC_SPI) */
+
   static struct i2c_pads_info tqma6_i2c3_pads = {
        /* I2C3: on board LM75, M24C64,  */
        .scl = {
@@ -201,7 +205,9 @@ int board_init(void)
        /* address of boot parameters */
        gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
 
Quoted 9 lines
board/tqc/tqma6/tqma6.c int board_init(void) · line 211
Quoted 4 lines
board/tqc/tqma6/tqma6.c +2 -0
+#if defined(CONFIG_MXC_SPI)
        tqma6_iomuxc_spi();
+#endif
        tqma6_setup_i2c();
Hmmm. This should not be necessary. Please take a look at the wru4 baseboard addition that is already in mainline. Especially this patch:
Quoted 4 lines
I noticed your patch but then we would still have to define CONFIG_SF_DEFAULT_BUS and CONFIG_SF_DEFAULT_CS in our board config or include
the tqma6.h config, which defines too many things we do not need.
I would have expected that all boards using these TQ SoM's include this config header.
Quoted 15 lines
The problem is, we have different clocks and more c files, so we use a separate
board directory and a separate board config without SPI defines and just use
obj-y += ../../tqc/tqma6/tqma6.o in front of our object files in the Makefile.
If we don't exclude those SPI parts, we would either have to define those SPI
config options (which does not make sense) or move all of our board code in one
file under tqc/tqma6 which is not an option, we also define additional clocks
via a separate IMX_CONFIG file.
Quoted 7 lines
Wouldn't it be better to always exclude this code when not using SPI instead of
weakly defining the tqma6_iomuxc_spi function?
Then small baseboards could be put in tqc/tqma6 and larger projects could still
just link to tqma6.o and put all their stuff in a separate board directory.
How about this patch:
board/tqc/tqma6/tqma6.c +0 -0
index 29db838..345930f 100644

board/tqc/tqma6/tqma6.c line 36

Quoted 9 lines

board/tqc/tqma6/tqma6.c +6 -0

@@ -28,6 +28,13 @@
 
 #include "tqma6_bb.h"
 
+#ifndef CONFIG_SF_DEFAULT_BUS
+#define CONFIG_SF_DEFAULT_BUS  0
+#endif
+#ifndef CONFIG_SF_DEFAULT_CS
+#define CONFIG_SF_DEFAULT_CS   0
+#endif

Why would need this - it's already part of spi_flash.h

Quoted 2 lines

Does this work for you?

thanks!

Signature · 1 line

Jagan | openedev.

Clemens Gruber

author

2015-08-05 08:33 UTC #5 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

Quoted 11 lines · Jagan Teki

Quoted 10 lines · Stefan Roese
How about this patch:
board/tqc/tqma6/tqma6.c +0 -0
index 29db838..345930f 100644
@@ -28,6 +28,13 @@
 
 #include "tqma6_bb.h"
 

Quoted 8 lines

board/tqc/tqma6/tqma6.c line 36
Quoted 6 lines
board/tqc/tqma6/tqma6.c +6 -0
+#ifndef CONFIG_SF_DEFAULT_BUS
+#define CONFIG_SF_DEFAULT_BUS  0
+#endif
+#ifndef CONFIG_SF_DEFAULT_CS
+#define CONFIG_SF_DEFAULT_CS   0
+#endif
Why would need this - it's already part of spi_flash.h

I am also wondering about commit f85764cc1f6ab01ffc60dd78de9c4de4cff2b5ce, which defines CONFIG_SPI_FLASH in the WRU4 defconfig file, even though it does not need SPI flash support, just to work around undefined reference errors regarding spi_flash_free, etc.

Couldn't those workarounds be avoided by excluding the SPI parts via ifdefs, as I did in my patch today?

Quoted 7 lines

Quoted 3 lines
Does this work for you?
thanks!
Signature · 1 line
Jagan | openedev.

Thanks,
Clemens

Stefan Roese

2015-08-05 08:37 UTC #6 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

Quoted 21 lines · Clemens Gruber

Quoted 11 lines · Jagan Teki
Quoted 10 lines · Stefan Roese
How about this patch:
board/tqc/tqma6/tqma6.c +0 -0
index 29db838..345930f 100644
@@ -28,6 +28,13 @@
 
  #include "tqma6_bb.h"
 
Quoted 8 lines
board/tqc/tqma6/tqma6.c line 36
Quoted 6 lines
board/tqc/tqma6/tqma6.c +6 -0
+#ifndef CONFIG_SF_DEFAULT_BUS
+#define CONFIG_SF_DEFAULT_BUS  0
+#endif
+#ifndef CONFIG_SF_DEFAULT_CS
+#define CONFIG_SF_DEFAULT_CS   0
+#endif
Why would need this - it's already part of spi_flash.h

Quoted 7 lines

I am also wondering about commit f85764cc1f6ab01ffc60dd78de9c4de4cff2b5ce, which defines CONFIG_SPI_FLASH in the WRU4 defconfig file, even though it does not need SPI flash support, just to work around undefined reference errors regarding spi_flash_free, etc.
Couldn't those workarounds be avoided by excluding the SPI parts via ifdefs, as I did in my patch today?

Right. But as mentioned before I really would like to avoid those ifdef's. So let me check if I can get this done in a more clean way.

Thanks,
Stefan

Clemens Gruber

author

2015-08-05 08:17 UTC #7 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

In reply to Stefan Roese

Hi Stefan,

Quoted 17 lines · Stefan Roese

How about this patch:

board/tqc/tqma6/tqma6.c +6 -0

index 29db838..345930f 100644
@@ -28,6 +28,13 @@
 
 #include "tqma6_bb.h"
 
+#ifndef CONFIG_SF_DEFAULT_BUS
+#define CONFIG_SF_DEFAULT_BUS  0
+#endif
+#ifndef CONFIG_SF_DEFAULT_CS
+#define CONFIG_SF_DEFAULT_CS   0
+#endif

Quoted 1 line

Does this work for you?

This should work, but wouldn't it be cleaner to just exclude the SPI parts? Why add 7 LOC when 4 LOC are enough to exclude the SPI parts? Besides that, it reduces the binary size.

Then we could also remove the empty SPI function from all board files, if SPI is not used.

Quoted 3 lines

Thanks,
Stefan

Regards,
Clemens

Stefan Roese

2015-08-05 08:36 UTC #8 lore raw

PATCH 1/1 Do not mux and setup SPI if disabled in the config

Hi Clemens,

Quoted 18 lines · Clemens Gruber

Quoted 17 lines · Stefan Roese

How about this patch:

board/tqc/tqma6/tqma6.c +6 -0

index 29db838..345930f 100644
@@ -28,6 +28,13 @@
 
  #include "tqma6_bb.h"
 
+#ifndef CONFIG_SF_DEFAULT_BUS
+#define CONFIG_SF_DEFAULT_BUS  0
+#endif
+#ifndef CONFIG_SF_DEFAULT_CS
+#define CONFIG_SF_DEFAULT_CS   0
+#endif

Quoted 5 lines

Quoted 1 line
Does this work for you?
This should work, but wouldn't it be cleaner to just exclude the SPI parts? Why add 7 LOC when 4 LOC are enough to exclude the SPI parts? Besides that, it reduces the binary size.

The binary size should be the same (I didn't check this). The linker should remove the not referenced functions / variables.

As for the added 7 LOC, Jagans suggestion to include "spi_flash.h" should also work instead. This seems to be a nice solution for me that doesn't add any new #ifdef's to the code.

Thanks,
Stefan