[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Ben Warren]

Signed-off-by: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/spi/Makefile      |    2 +-
 drivers/spi/mpc8xxx_spi.c |    2 --
 2 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index bc8a104..eab9d93 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -25,7 +25,7 @@ include $(TOPDIR)/config.mk
 
 LIB	:= $(obj)libspi.a
 
-COBJS-y += mpc8xxx_spi.o
+COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
 COBJS-$(CONFIG_MXC_SPI) += mxc_spi.o
 
 COBJS	:= $(COBJS-y)
diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
index 2fe838c..5e2f67a 100644
--- a/drivers/spi/mpc8xxx_spi.c
+++ b/drivers/spi/mpc8xxx_spi.c
@@ -22,7 +22,6 @@
  */
 
 #include <common.h>
-#if defined(CONFIG_MPC8XXX_SPI) && defined(CONFIG_HARD_SPI)
 
 #include <spi.h>
 #include <asm/mpc8xxx_spi.h>
@@ -140,4 +139,3 @@ int spi_xfer(spi_chipsel_type chipsel, int bitlen, uchar *dout, uchar *din)
 
 	return 0;
 }
-#endif				/* CONFIG_HARD_SPI */
-- 
1.5.4.3

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Wolfgang Denk]

Dear Ben,

in message <1211783488-30985-1-git-send-email-biggerbadderben@gmail.com> you wrote:
>
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
...
> -COBJS-y += mpc8xxx_spi.o
> +COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o

i. e. we move CONFIG_MPC8XXX_SPI into the Makefile...

> diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
...
> -#if defined(CONFIG_MPC8XXX_SPI) && defined(CONFIG_HARD_SPI)

...but that would leave the "#ifded CONFIG_HARD_SPI" part still in
place. Or am I missing something?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A rolling stone gathers momentum.

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Ben Warren]

Wolfgang Denk wrote:
> Dear Ben,
>
> in message <1211783488-30985-1-git-send-email-biggerbadderben@gmail.com> you wrote:
>   
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>>     
> ...
>   
>> -COBJS-y += mpc8xxx_spi.o
>> +COBJS-$(CONFIG_MPC8XXX_SPI) += mpc8xxx_spi.o
>>     
>
> i. e. we move CONFIG_MPC8XXX_SPI into the Makefile...
>
>   
>> diff --git a/drivers/spi/mpc8xxx_spi.c b/drivers/spi/mpc8xxx_spi.c
>>     
> ...
>   
>> -#if defined(CONFIG_MPC8XXX_SPI) && defined(CONFIG_HARD_SPI)
>>     
>
> ...but that would leave the "#ifded CONFIG_HARD_SPI" part still in
> place. Or am I missing something?
>   
If CONFIG_MPC8XXX_SPI is defined but CONFIG_HARD_SPI is not, compilation 
will fail, since CONFIG_HARD_SPI activates the common SPI code on 
PowerPC.  I guess I figured Kconfig would eventually manage the 
dependencies of these options, but I will put the #ifdef back in if you 
want.

regards,
Ben

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Wolfgang Denk]

In message <483AD1FC.7040805@gmail.com> you wrote:
>
> If CONFIG_MPC8XXX_SPI is defined but CONFIG_HARD_SPI is not, compilation 
> will fail, since CONFIG_HARD_SPI activates the common SPI code on 

How would I configure a system to use CONFIG_SOFT_SPI on a MPC8XXX
system?

> PowerPC.  I guess I figured Kconfig would eventually manage the 
> dependencies of these options, but I will put the #ifdef back in if you 
> want.

At the moment it seems logical to me to leave this test as it was.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A modem is a baudy house.

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Ben Warren]

Wolfgang Denk wrote:
> In message <483AD1FC.7040805@gmail.com> you wrote:
>   
>> If CONFIG_MPC8XXX_SPI is defined but CONFIG_HARD_SPI is not, compilation 
>> will fail, since CONFIG_HARD_SPI activates the common SPI code on 
>>     
>
> How would I configure a system to use CONFIG_SOFT_SPI on a MPC8XXX
> system?
>
>   
That's actually pretty easy:

1. #define CONFIG_SOFT_SPI in board config
2. define bit-bang parameters in board code
3. Call spi_init() from board code

The CONFIG_HARD_SPI define doesn't actually do anything 
hardware-related.  The only place it's used is in lib_ppc/board.c to 
initialize a SPI controller, but the code could equally-well start a 
soft SPI controller.  I would have used CONFIG_SPI, but that was already 
taken by SPI EEPROM stuff.  Maybe it's time to refactor a bit more...

>> PowerPC.  I guess I figured Kconfig would eventually manage the 
>> dependencies of these options, but I will put the #ifdef back in if you 
>> want.
>>     
>
> At the moment it seems logical to me to leave this test as it was.
>
> Best regards,
>
> Wolfgang Denk
>
>   
regards,
Ben

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Haavard Skinnemoen]

Ben Warren <biggerbadderben@gmail.com> wrote:
> > ...but that would leave the "#ifded CONFIG_HARD_SPI" part still in
> > place. Or am I missing something?
> >     
> If CONFIG_MPC8XXX_SPI is defined but CONFIG_HARD_SPI is not, compilation 
> will fail, since CONFIG_HARD_SPI activates the common SPI code on 
> PowerPC.  I guess I figured Kconfig would eventually manage the 
> dependencies of these options, but I will put the #ifdef back in if you 
> want.

This only makes a difference if a board config defines
CONFIG_MPC8XXX_SPI without defining CONFIG_HARD_SPI, which is arguably
a bug. I think it's better to get a compile error when this happens
than having the CONFIG_MPC8XXX_SPI define be silently ignored (which
most likely won't be detected until runtime.)

So I think the patch is fine as it is, for whatever that's worth.

Haavard

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Wolfgang Denk]

In message <20080527093432.3026cb15@hskinnemo-gx745.norway.atmel.com> you wrote:
>
> This only makes a difference if a board config defines
> CONFIG_MPC8XXX_SPI without defining CONFIG_HARD_SPI, which is arguably
> a bug. I think it's better to get a compile error when this happens

I can't really folow that logic.

If we define CONFIG_HARD_SPI, then I don't see why CONFIG_MPC8XXX_SPI
is needed at all if we're on a MPC8XXX system - that seems  redundant
to  me.  On the other hand, if you want to use CONFIG_MPC8XXX_SPI and
this implies that CONFIG_HARD_SPI must be set, too,  then  it  should
automatically  set  this  variable  instead of causing the compile to
fail.

This assumes that we use only one SPI  controller  (built-in  on  the
CPU/SOC).  We  should  also  keep  in  mind what happens when you use
CONFIG_SOFT_SPI, eventually even simultaneously.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Pray: To ask that the laws of the universe be annulled in behalf of a
single petitioner confessedly unworthy.              - Ambrose Bierce

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <20080527093432.3026cb15@hskinnemo-gx745.norway.atmel.com> you wrote:
> >
> > This only makes a difference if a board config defines
> > CONFIG_MPC8XXX_SPI without defining CONFIG_HARD_SPI, which is arguably
> > a bug. I think it's better to get a compile error when this happens
> 
> I can't really folow that logic.
> 
> If we define CONFIG_HARD_SPI, then I don't see why CONFIG_MPC8XXX_SPI
> is needed at all if we're on a MPC8XXX system - that seems  redundant
> to  me.  On the other hand, if you want to use CONFIG_MPC8XXX_SPI and
> this implies that CONFIG_HARD_SPI must be set, too,  then  it  should
> automatically  set  this  variable  instead of causing the compile to
> fail.

Sounds to me like you're asking for Kconfig. I can't think of any way
to meet those requirements with the current infrastructure...

To be more specific:
  * Which part of the system is responsible for figuring out that when
    CONFIG_HARD_SPI is set _and_ we're on a MPC8XXX system, the
    mpx8xxx_spi driver should be compiled?
  * Which part of the system is responsible for automatically selecting
    CONFIG_HARD_SPI when CONFIG_MPC8XXX_SPI is set?

On the other hand, it looks like everyone but powerpc is getting away
with not using CONFIG_HARD_SPI at all. Is it really necessary to have
two defines for essentially the same thing?

> This assumes that we use only one SPI  controller  (built-in  on  the
> CPU/SOC).  We  should  also  keep  in  mind what happens when you use
> CONFIG_SOFT_SPI, eventually even simultaneously.

The current infrastructure doesn't allow multiple SPI controllers.

The new infrastructure that I posted yesterday supports multiple SPI
controllers of the same type, but not different ones. I asked whether
supporting multiple different SPI controllers would be necessary but
got no response, so I assumed no.

Haavard

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Wolfgang Denk]

In message <20080527100426.0f40544e@hskinnemo-gx745.norway.atmel.com> you wrote:
>
> > If we define CONFIG_HARD_SPI, then I don't see why CONFIG_MPC8XXX_SPI
> > is needed at all if we're on a MPC8XXX system - that seems  redundant
> > to  me.  On the other hand, if you want to use CONFIG_MPC8XXX_SPI and
> > this implies that CONFIG_HARD_SPI must be set, too,  then  it  should
> > automatically  set  this  variable  instead of causing the compile to
> > fail.
> 
> Sounds to me like you're asking for Kconfig. I can't think of any way
> to meet those requirements with the current infrastructure...

No? Uuups... what's wrong with

	#ifdef CONFIG_MPC8XXX_SPI
	# ifndef CONFIG_HARD_SPI
	#  define CONFIG_HARD_SPI
	# endif
	#endif

Not exactly beautiful, but should work, shouldn't it?

> To be more specific:
>   * Which part of the system is responsible for figuring out that when
>     CONFIG_HARD_SPI is set _and_ we're on a MPC8XXX system, the
>     mpx8xxx_spi driver should be compiled?

The Makefile?

Yes, you ill have to use a conditional in the Makefile as long as you
ask for separate CONFIG_ options. That's one reason why I'm asking why
we need several.

>   * Which part of the system is responsible for automatically selecting
>     CONFIG_HARD_SPI when CONFIG_MPC8XXX_SPI is set?

include/spi.h ?

> On the other hand, it looks like everyone but powerpc is getting away
> with not using CONFIG_HARD_SPI at all. Is it really necessary to have
> two defines for essentially the same thing?

Bingo! That was exactly my question.

> > This assumes that we use only one SPI  controller  (built-in  on  the
> > CPU/SOC).  We  should  also  keep  in  mind what happens when you use
> > CONFIG_SOFT_SPI, eventually even simultaneously.
> 
> The current infrastructure doesn't allow multiple SPI controllers.

Yes, I know. But we should keep in mind that one day  such  a  system
may  have  to be supported, and we should try not to make support for
such systems more difficult than necessary.

> The new infrastructure that I posted yesterday supports multiple SPI
> controllers of the same type, but not different ones. I asked whether
> supporting multiple different SPI controllers would be necessary but
> got no response, so I assumed no.

I think that's a valid assumption, at least for now. I cannot imagine
why multiple different SPI controllers might be needed, but then  I'm
surprised  again  and again what hardware designers can come up with.
So we should try not to build barriers that can be avoided.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Generally speaking, there are other ways to accomplish whatever it is
that you think you need ...                               - Doug Gwyn

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Haavard Skinnemoen]

Wolfgang Denk <wd@denx.de> wrote:
> In message <20080527100426.0f40544e@hskinnemo-gx745.norway.atmel.com> you wrote:
> >
> > > If we define CONFIG_HARD_SPI, then I don't see why CONFIG_MPC8XXX_SPI
> > > is needed at all if we're on a MPC8XXX system - that seems  redundant
> > > to  me.  On the other hand, if you want to use CONFIG_MPC8XXX_SPI and
> > > this implies that CONFIG_HARD_SPI must be set, too,  then  it  should
> > > automatically  set  this  variable  instead of causing the compile to
> > > fail.
> > 
> > Sounds to me like you're asking for Kconfig. I can't think of any way
> > to meet those requirements with the current infrastructure...
> 
> No? Uuups... what's wrong with
> 
> 	#ifdef CONFIG_MPC8XXX_SPI
> 	# ifndef CONFIG_HARD_SPI
> 	#  define CONFIG_HARD_SPI
> 	# endif
> 	#endif
> 
> Not exactly beautiful, but should work, shouldn't it?

Sure, but I was mostly wondering about _where_ one would put something
like that...which you answered below.

> > To be more specific:
> >   * Which part of the system is responsible for figuring out that when
> >     CONFIG_HARD_SPI is set _and_ we're on a MPC8XXX system, the
> >     mpx8xxx_spi driver should be compiled?
> 
> The Makefile?
> 
> Yes, you ill have to use a conditional in the Makefile as long as you
> ask for separate CONFIG_ options. That's one reason why I'm asking why
> we need several.

Ah...but I'm _also_ wondering why we need several :-)

> >   * Which part of the system is responsible for automatically selecting
> >     CONFIG_HARD_SPI when CONFIG_MPC8XXX_SPI is set?
> 
> include/spi.h ?

Makes sense.

> > On the other hand, it looks like everyone but powerpc is getting away
> > with not using CONFIG_HARD_SPI at all. Is it really necessary to have
> > two defines for essentially the same thing?
> 
> Bingo! That was exactly my question.

I guess I misunderstood then.

What I'm trying to say is that it makes sense to have one config symbol
per driver so that the board config header can specify exactly what it
wants instead of having something else try to figure it out
automatically.

So maybe we should try to get rid of the CONFIG_HARD_SPI define? I
don't think there's any fundamental difference between soft and hard
SPI anymore, at least not at the interface level.

> > > This assumes that we use only one SPI  controller  (built-in  on  the
> > > CPU/SOC).  We  should  also  keep  in  mind what happens when you use
> > > CONFIG_SOFT_SPI, eventually even simultaneously.
> > 
> > The current infrastructure doesn't allow multiple SPI controllers.
> 
> Yes, I know. But we should keep in mind that one day  such  a  system
> may  have  to be supported, and we should try not to make support for
> such systems more difficult than necessary.

Agree.

> > The new infrastructure that I posted yesterday supports multiple SPI
> > controllers of the same type, but not different ones. I asked whether
> > supporting multiple different SPI controllers would be necessary but
> > got no response, so I assumed no.
> 
> I think that's a valid assumption, at least for now. I cannot imagine
> why multiple different SPI controllers might be needed, but then  I'm
> surprised  again  and again what hardware designers can come up with.
> So we should try not to build barriers that can be avoided.

Sure, and I think my SPI patches brings us closer to the goal of
supporting multiple types of SPI hardware. But I don't want to take the
final step before we know for certain that we want it because it makes
the SPI layer more expensive in terms of code size and RAM usage.

I suspect that if we do want something like that, it will probably be
about combining hardware and software SPI (i.e. we want more SPI busses
than the chip can provide in hardware.)

Haavard

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Wolfgang Denk]

In message <20080527103245.565527ba@hskinnemo-gx745.norway.atmel.com> you wrote:
>
> So maybe we should try to get rid of the CONFIG_HARD_SPI define? I

Well, I think I'd rather get rid of  CONFIG_MPC8XXX_SPI  as  this  is
redundant - it's what you atomatically get when using CONFIG_HARD_SPI
on  a  MPC8XXX system. That would make this more in line with the I2C
code. And we can use something like

	#if defined(CONFIG_HARD_SPI) || defined (CONFIG_SOFT_SPI)

when we need to know if there is any SPI support at all (like we do
with I2C).

> I suspect that if we do want something like that, it will probably be
> about combining hardware and software SPI (i.e. we want more SPI busses
> than the chip can provide in hardware.)

Yes, I guess this is going to happen first. Hopefully not soon, though
;-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The human race has one really effective weapon, and that is laughter.
                                                         - Mark Twain

[U-Boot-Users] [PATCH] Move conditional compilation of MPC8XXX SPI driver to Makefile

[Ben Warren]

Sorry I'm just re-joining this conversation.  That's the fun of being
9 time zones apart...

On Tue, May 27, 2008 at 2:13 AM, Wolfgang Denk <wd@denx.de> wrote:
> In message <20080527103245.565527ba@hskinnemo-gx745.norway.atmel.com> you wrote:
>>
>> So maybe we should try to get rid of the CONFIG_HARD_SPI define? I
>
> Well, I think I'd rather get rid of  CONFIG_MPC8XXX_SPI  as  this  is
> redundant - it's what you atomatically get when using CONFIG_HARD_SPI
> on  a  MPC8XXX system. That would make this more in line with the I2C
> code. And we can use something like
>
>        #if defined(CONFIG_HARD_SPI) || defined (CONFIG_SOFT_SPI)
>
> when we need to know if there is any SPI support at all (like we do
> with I2C).
>

Ideally, we'd be able to re-claim CONFIG_SPI from whatever perverse
use it currently has (IIRC it CONFIG_BOOT_FROM_SPI_EEPROM), but that's
pretty invasive.
I don't think we want to take out CONFIG_MCP8XXX_SPI, because then
somebody needs to keep track of what CPUs have it and maintain the
header files appropriately.  I can just imagine the mess:

#if (defined MPC834X) || (defined MPC857X) || (defined MPC9766) || \
.
.
.
do your stuff
#endif

  Automatically generating CONFIG_HARD_SPI from it, though, makes
sense, though, although I hate to see architecture-specific stuff go
in spi.h.  NBD, I guess.

>> I suspect that if we do want something like that, it will probably be
>> about combining hardware and software SPI (i.e. we want more SPI busses
>> than the chip can provide in hardware.)
>
> Yes, I guess this is going to happen first. Hopefully not soon, though
> ;-)
>

Yeah, suggesting that something isn't necessary virtually assures that
it will be.

I'll put together a new patch and re-send later today.

regards,
Ben