diff mbox

[U-Boot] Do not mux and setup SPI if disabled in the config

Message ID 55C1C3B6.6040606@denx.de
State RFC
Headers show

Commit Message

Stefan Roese Aug. 5, 2015, 8:05 a.m. UTC 
Hi Clemens,

On 05.08.2015 09:28, Clemens Gruber wrote:
>> On 04.08.2015 19:28, Clemens Gruber wrote:
>>> This allows baseboards without SPI to compile.
>>>
>>> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
>>> Cc: Markus Niebel <Markus.Niebel@tq-group.com>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> ---
>>>    board/tqc/tqma6/tqma6.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
>>> index 29db838..5c4d104 100644
>>> --- a/board/tqc/tqma6/tqma6.c
>>> +++ b/board/tqc/tqma6/tqma6.c
>>> @@ -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();
>>
>> 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.

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

> 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.

How about this patch:

Stefan

Comments

Jagan Teki Aug. 5, 2015, 8:16 a.m. UTC | #1 
On 5 August 2015 at 13:35, Stefan Roese <sr@denx.de> wrote:
> Hi Clemens,
>
>
> On 05.08.2015 09:28, Clemens Gruber wrote:
>>>
>>> On 04.08.2015 19:28, Clemens Gruber wrote:
>>>>
>>>> This allows baseboards without SPI to compile.
>>>>
>>>> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
>>>> Cc: Markus Niebel <Markus.Niebel@tq-group.com>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> ---
>>>>    board/tqc/tqma6/tqma6.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
>>>> index 29db838..5c4d104 100644
>>>> --- a/board/tqc/tqma6/tqma6.c
>>>> +++ b/board/tqc/tqma6/tqma6.c
>>>> @@ -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();
>>>
>>>
>>> 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.
>
>
> I would have expected that all boards using these TQ SoM's include this
> config header.
>
>> 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.
>
>
> How about this patch:
>
> diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
> index 29db838..345930f 100644
> --- a/board/tqc/tqma6/tqma6.c
> +++ b/board/tqc/tqma6/tqma6.c
> @@ -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

>
> Does this work for you?

thanks!
Clemens Gruber Aug. 5, 2015, 8:17 a.m. UTC | #2 
Hi Stefan,

On Wed, Aug 05, 2015 at 10:05:10AM +0200, Stefan Roese wrote:
> How about this patch:
> 
> diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
> index 29db838..345930f 100644
> --- a/board/tqc/tqma6/tqma6.c
> +++ b/board/tqc/tqma6/tqma6.c
> @@ -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?

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.

> 
> Thanks,
> Stefan

Regards,
Clemens
Clemens Gruber Aug. 5, 2015, 8:33 a.m. UTC | #3 
On Wed, Aug 05, 2015 at 01:46:42PM +0530, Jagan Teki wrote:
> On 5 August 2015 at 13:35, Stefan Roese <sr@denx.de> wrote:
> > How about this patch:
> >
> > diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
> > index 29db838..345930f 100644
> > --- a/board/tqc/tqma6/tqma6.c
> > +++ b/board/tqc/tqma6/tqma6.c
> > @@ -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

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?

> 
> >
> > Does this work for you?
> 
> thanks!
> -- 
> Jagan | openedev.

Thanks,
Clemens
Stefan Roese Aug. 5, 2015, 8:36 a.m. UTC | #4 
Hi Clemens,

On 05.08.2015 10:17, Clemens Gruber wrote:
> On Wed, Aug 05, 2015 at 10:05:10AM +0200, Stefan Roese wrote:
>> How about this patch:
>>
>> diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
>> index 29db838..345930f 100644
>> --- a/board/tqc/tqma6/tqma6.c
>> +++ b/board/tqc/tqma6/tqma6.c
>> @@ -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?
>
> 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
Stefan Roese Aug. 5, 2015, 8:37 a.m. UTC | #5 
On 05.08.2015 10:33, Clemens Gruber wrote:
> On Wed, Aug 05, 2015 at 01:46:42PM +0530, Jagan Teki wrote:
>> On 5 August 2015 at 13:35, Stefan Roese <sr@denx.de> wrote:
>>> How about this patch:
>>>
>>> diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
>>> index 29db838..345930f 100644
>>> --- a/board/tqc/tqma6/tqma6.c
>>> +++ b/board/tqc/tqma6/tqma6.c
>>> @@ -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
>
> 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
diff mbox

Patch

diff --git a/board/tqc/tqma6/tqma6.c b/board/tqc/tqma6/tqma6.c
index 29db838..345930f 100644
--- a/board/tqc/tqma6/tqma6.c
+++ b/board/tqc/tqma6/tqma6.c
@@ -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,