diff mbox

[U-Boot,V4,6/7] mx6q: mx6qsabrelite: Provide default serial flash bus and chip-select

Message ID 1327863595-1524-7-git-send-email-eric.nelson@boundarydevices.com
State Superseded
Headers show

Commit Message

Eric Nelson Jan. 29, 2012, 6:59 p.m. UTC 
Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
Acked-by: Dirk Behme <dirk.behme@de.bosch.com>
Acked-by: Stefano Babic <sbabic@denx.de>
---
 include/configs/mx6qsabrelite.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Marek Vasut Jan. 29, 2012, 7:18 p.m. UTC | #1 
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> Acked-by: Dirk Behme <dirk.behme@de.bosch.com>
> Acked-by: Stefano Babic <sbabic@denx.de>
> ---
>  include/configs/mx6qsabrelite.h |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/mx6qsabrelite.h
> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
> --- a/include/configs/mx6qsabrelite.h
> +++ b/include/configs/mx6qsabrelite.h
> @@ -46,9 +46,12 @@
> 
>  #define CONFIG_CMD_SF
>  #ifdef CONFIG_CMD_SF
> +#define GPIO_3_19 ((2*32)+19)

I'd expect this to be in platform headers?

>  #define CONFIG_SPI_FLASH
>  #define CONFIG_SPI_FLASH_SST
>  #define CONFIG_MXC_SPI
> +#define CONFIG_SF_DEFAULT_BUS  0
> +#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
>  #define CONFIG_SF_DEFAULT_SPEED 25000000
>  #define CONFIG_SF_DEFAULT_MODE (SPI_MODE_0)
>  #endif
Eric Nelson Jan. 29, 2012, 8:02 p.m. UTC | #2 
On 01/29/2012 12:18 PM, Marek Vasut wrote:
>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
>> Acked-by: Stefano Babic<sbabic@denx.de>
>> ---
>>   include/configs/mx6qsabrelite.h |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/configs/mx6qsabrelite.h
>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
>> --- a/include/configs/mx6qsabrelite.h
>> +++ b/include/configs/mx6qsabrelite.h
>> @@ -46,9 +46,12 @@
>>
>>   #define CONFIG_CMD_SF
>>   #ifdef CONFIG_CMD_SF
>> +#define GPIO_3_19 ((2*32)+19)
>
> I'd expect this to be in platform headers?
>

This is a choice made in the SabreLite design. I don't think
the same choice has been made for other i.MX6 boards.
Marek Vasut Jan. 29, 2012, 8:11 p.m. UTC | #3 
> On 01/29/2012 12:18 PM, Marek Vasut wrote:
> >> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
> >> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
> >> Acked-by: Stefano Babic<sbabic@denx.de>
> >> ---
> >> 
> >>   include/configs/mx6qsabrelite.h |    3 +++
> >>   1 files changed, 3 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/include/configs/mx6qsabrelite.h
> >> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
> >> --- a/include/configs/mx6qsabrelite.h
> >> +++ b/include/configs/mx6qsabrelite.h
> >> @@ -46,9 +46,12 @@
> >> 
> >>   #define CONFIG_CMD_SF
> >>   #ifdef CONFIG_CMD_SF
> >> 
> >> +#define GPIO_3_19 ((2*32)+19)
> > 
> > I'd expect this to be in platform headers?
> 
> This is a choice made in the SabreLite design. I don't think
> the same choice has been made for other i.MX6 boards.

I mean the definition of the GPIO_3_19 ...
Eric Nelson Jan. 29, 2012, 9:35 p.m. UTC | #4 
On 01/29/2012 01:11 PM, Marek Vasut wrote:
>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
>>>> Acked-by: Stefano Babic<sbabic@denx.de>
>>>> ---
>>>>
>>>>    include/configs/mx6qsabrelite.h |    3 +++
>>>>    1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/configs/mx6qsabrelite.h
>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
>>>> --- a/include/configs/mx6qsabrelite.h
>>>> +++ b/include/configs/mx6qsabrelite.h
>>>> @@ -46,9 +46,12 @@
>>>>
>>>>    #define CONFIG_CMD_SF
>>>>    #ifdef CONFIG_CMD_SF
>>>>
>>>> +#define GPIO_3_19 ((2*32)+19)
>>>
>>> I'd expect this to be in platform headers?
>>
>> This is a choice made in the SabreLite design. I don't think
>> the same choice has been made for other i.MX6 boards.
>
> I mean the definition of the GPIO_3_19 ...
>

I don't think we want to set precedent for defining
constants for the 100s of GPIO numbers.

That said, I could achieve my objective of clarifying
what the number meant (that the constant refers to a GP) by
changing this:

	#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))

to this

	#define CONFIG_SF_DEFAULT_CS   (0|(IMX_GPIO_NR(3,19)<<8))

There's a bit of an issue with this. The IMX_GPIO_NR() macro
is defined in arch-mx6/gpio.h which is normally included after
mx6qsabrelite.h because the latter defines the machine.
Marek Vasut Jan. 29, 2012, 10:16 p.m. UTC | #5 
> On 01/29/2012 01:11 PM, Marek Vasut wrote:
> >> On 01/29/2012 12:18 PM, Marek Vasut wrote:
> >>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
> >>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
> >>>> Acked-by: Stefano Babic<sbabic@denx.de>
> >>>> ---
> >>>> 
> >>>>    include/configs/mx6qsabrelite.h |    3 +++
> >>>>    1 files changed, 3 insertions(+), 0 deletions(-)
> >>>> 
> >>>> diff --git a/include/configs/mx6qsabrelite.h
> >>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
> >>>> --- a/include/configs/mx6qsabrelite.h
> >>>> +++ b/include/configs/mx6qsabrelite.h
> >>>> @@ -46,9 +46,12 @@
> >>>> 
> >>>>    #define CONFIG_CMD_SF
> >>>>    #ifdef CONFIG_CMD_SF
> >>>> 
> >>>> +#define GPIO_3_19 ((2*32)+19)
> >>> 
> >>> I'd expect this to be in platform headers?
> >> 
> >> This is a choice made in the SabreLite design. I don't think
> >> the same choice has been made for other i.MX6 boards.
> > 
> > I mean the definition of the GPIO_3_19 ...
> 
> I don't think we want to set precedent for defining
> constants for the 100s of GPIO numbers.
> 
> That said, I could achieve my objective of clarifying
> what the number meant (that the constant refers to a GP) by
> changing this:
> 
> 	#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
> 
> to this
> 
> 	#define CONFIG_SF_DEFAULT_CS   (0|(IMX_GPIO_NR(3,19)<<8))

Why the (0| part ? Anyway, that indeed looks better, more standard even.

And I think for MX5, there was even stuff defining the GPIO numbers imported 
from Linux.

M

> 
> There's a bit of an issue with this. The IMX_GPIO_NR() macro
> is defined in arch-mx6/gpio.h which is normally included after
> mx6qsabrelite.h because the latter defines the machine.

And the CPP will choke on that ?
Eric Nelson Jan. 29, 2012, 11:04 p.m. UTC | #6 
On 01/29/2012 03:16 PM, Marek Vasut wrote:
>> On 01/29/2012 01:11 PM, Marek Vasut wrote:
>>>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
>>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
>>>>>> Acked-by: Stefano Babic<sbabic@denx.de>
>>>>>> ---
>>>>>>
>>>>>>     include/configs/mx6qsabrelite.h |    3 +++
>>>>>>     1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/include/configs/mx6qsabrelite.h
>>>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
>>>>>> --- a/include/configs/mx6qsabrelite.h
>>>>>> +++ b/include/configs/mx6qsabrelite.h
>>>>>> @@ -46,9 +46,12 @@
>>>>>>
>>>>>>     #define CONFIG_CMD_SF
>>>>>>     #ifdef CONFIG_CMD_SF
>>>>>>
>>>>>> +#define GPIO_3_19 ((2*32)+19)
>>>>>
>>>>> I'd expect this to be in platform headers?
>>>>
>>>> This is a choice made in the SabreLite design. I don't think
>>>> the same choice has been made for other i.MX6 boards.
>>>
>>> I mean the definition of the GPIO_3_19 ...
>>
>> I don't think we want to set precedent for defining
>> constants for the 100s of GPIO numbers.
>>
>> That said, I could achieve my objective of clarifying
>> what the number meant (that the constant refers to a GP) by
>> changing this:
>>
>> 	#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
>>
>> to this
>>
>> 	#define CONFIG_SF_DEFAULT_CS   (0|(IMX_GPIO_NR(3,19)<<8))
>
> Why the (0| part ? Anyway, that indeed looks better, more standard even.
>
> And I think for MX5, there was even stuff defining the GPIO numbers imported
> from Linux.
>
> M
>
>>
>> There's a bit of an issue with this. The IMX_GPIO_NR() macro
>> is defined in arch-mx6/gpio.h which is normally included after
>> mx6qsabrelite.h because the latter defines the machine.
>
> And the CPP will choke on that ?
>

Assembler or linker. IMX_GPIO_NR will be undefined and treated as an
external unless we add this nested include into include/configs/mx6qsabrelite.h:

#ifndef __ASSEMBLY__
#include <asm/arch/gpio.h>
#endif

arch-mx6/gpio.h isn't directly ASM-friendly.

This seems like a lot of #include-foo for giving a name to a constant, don't
you think?
Marek Vasut Jan. 30, 2012, 2:36 a.m. UTC | #7 
> On 01/29/2012 03:16 PM, Marek Vasut wrote:
> >> On 01/29/2012 01:11 PM, Marek Vasut wrote:
> >>>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
> >>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
> >>>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
> >>>>>> Acked-by: Stefano Babic<sbabic@denx.de>
> >>>>>> ---
> >>>>>> 
> >>>>>>     include/configs/mx6qsabrelite.h |    3 +++
> >>>>>>     1 files changed, 3 insertions(+), 0 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/include/configs/mx6qsabrelite.h
> >>>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
> >>>>>> --- a/include/configs/mx6qsabrelite.h
> >>>>>> +++ b/include/configs/mx6qsabrelite.h
> >>>>>> @@ -46,9 +46,12 @@
> >>>>>> 
> >>>>>>     #define CONFIG_CMD_SF
> >>>>>>     #ifdef CONFIG_CMD_SF
> >>>>>> 
> >>>>>> +#define GPIO_3_19 ((2*32)+19)
> >>>>> 
> >>>>> I'd expect this to be in platform headers?
> >>>> 
> >>>> This is a choice made in the SabreLite design. I don't think
> >>>> the same choice has been made for other i.MX6 boards.
> >>> 
> >>> I mean the definition of the GPIO_3_19 ...
> >> 
> >> I don't think we want to set precedent for defining
> >> constants for the 100s of GPIO numbers.
> >> 
> >> That said, I could achieve my objective of clarifying
> >> what the number meant (that the constant refers to a GP) by
> >> 
> >> changing this:
> >> 	#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
> >> 
> >> to this
> >> 
> >> 	#define CONFIG_SF_DEFAULT_CS   (0|(IMX_GPIO_NR(3,19)<<8))
> > 
> > Why the (0| part ? Anyway, that indeed looks better, more standard even.
> > 
> > And I think for MX5, there was even stuff defining the GPIO numbers
> > imported from Linux.
> > 
> > M
> > 
> >> There's a bit of an issue with this. The IMX_GPIO_NR() macro
> >> is defined in arch-mx6/gpio.h which is normally included after
> >> mx6qsabrelite.h because the latter defines the machine.
> > 
> > And the CPP will choke on that ?
> 
> Assembler or linker. IMX_GPIO_NR will be undefined and treated as an
> external unless we add this nested include into
> include/configs/mx6qsabrelite.h:
> 
> #ifndef __ASSEMBLY__
> #include <asm/arch/gpio.h>
> #endif
> 
> arch-mx6/gpio.h isn't directly ASM-friendly.
> 
> This seems like a lot of #include-foo for giving a name to a constant,
> don't you think?

Better than redefining stuff, which will eventually lead to errors and breakage.
Eric Nelson Jan. 30, 2012, 6:10 p.m. UTC | #8 
On 01/29/2012 07:36 PM, Marek Vasut wrote:
>> On 01/29/2012 03:16 PM, Marek Vasut wrote:
>>>> On 01/29/2012 01:11 PM, Marek Vasut wrote:
>>>>>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
>>>>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>>>>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
>>>>>>>> Acked-by: Stefano Babic<sbabic@denx.de>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      include/configs/mx6qsabrelite.h |    3 +++
>>>>>>>>      1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/configs/mx6qsabrelite.h
>>>>>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
>>>>>>>> --- a/include/configs/mx6qsabrelite.h
>>>>>>>> +++ b/include/configs/mx6qsabrelite.h
>>>>>>>> @@ -46,9 +46,12 @@
>>>>>>>>
>>>>>>>>      #define CONFIG_CMD_SF
>>>>>>>>      #ifdef CONFIG_CMD_SF
>>>>>>>>
>>>>>>>> +#define GPIO_3_19 ((2*32)+19)
>>>>>>>
>>>>>>> I'd expect this to be in platform headers?
>>>>>>
>>>>>> This is a choice made in the SabreLite design. I don't think
>>>>>> the same choice has been made for other i.MX6 boards.
>>>>>
>>>>> I mean the definition of the GPIO_3_19 ...
>>>>
>>>> I don't think we want to set precedent for defining
>>>> constants for the 100s of GPIO numbers.
>>>>
>>>> That said, I could achieve my objective of clarifying
>>>> what the number meant (that the constant refers to a GP) by
>>>>
>>>> changing this:
>>>> 	#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
>>>>
>>>> to this
>>>>
>>>> 	#define CONFIG_SF_DEFAULT_CS   (0|(IMX_GPIO_NR(3,19)<<8))
>>>
>>> Why the (0| part ? Anyway, that indeed looks better, more standard even.
>>>
>>> And I think for MX5, there was even stuff defining the GPIO numbers
>>> imported from Linux.
>>>
>>> M
>>>
>>>> There's a bit of an issue with this. The IMX_GPIO_NR() macro
>>>> is defined in arch-mx6/gpio.h which is normally included after
>>>> mx6qsabrelite.h because the latter defines the machine.
>>>
>>> And the CPP will choke on that ?
>>
>> Assembler or linker. IMX_GPIO_NR will be undefined and treated as an
>> external unless we add this nested include into
>> include/configs/mx6qsabrelite.h:
>>
>> #ifndef __ASSEMBLY__
>> #include<asm/arch/gpio.h>
>> #endif
>>
>> arch-mx6/gpio.h isn't directly ASM-friendly.
>>
>> This seems like a lot of #include-foo for giving a name to a constant,
>> don't you think?
>
> Better than redefining stuff, which will eventually lead to errors and breakage.
>

Hmmm. I just noticed that the IMX_GPIO_NR() macro isn't in Stefano's tree.
My previous patches were against Dirk's	tree on GitHub, which has a patch
from Troy:
	https://github.com/dirkbehme/u-boot-imx6/commit/c8b2870efd041f820a91eebcb888c84a4f79f1f6

If we move this macro into arch-mx6/imx-regs.h, we avoid the #if.

Looking at the remaining content of gpio.h, it seems that it's driver-specific
anyway (only the driver should be worried about the register layout of the GPIO
controller).

If there are no objections, I'll forward a separate patch to define the macro.

Should this be based on Stefano's tree?

Please advise,


Eric
Marek Vasut Jan. 30, 2012, 6:28 p.m. UTC | #9 
> On 01/29/2012 07:36 PM, Marek Vasut wrote:
> >> On 01/29/2012 03:16 PM, Marek Vasut wrote:
> >>>> On 01/29/2012 01:11 PM, Marek Vasut wrote:
> >>>>>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
> >>>>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
> >>>>>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
> >>>>>>>> Acked-by: Stefano Babic<sbabic@denx.de>
> >>>>>>>> ---
> >>>>>>>> 
> >>>>>>>>      include/configs/mx6qsabrelite.h |    3 +++
> >>>>>>>>      1 files changed, 3 insertions(+), 0 deletions(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/include/configs/mx6qsabrelite.h
> >>>>>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
> >>>>>>>> --- a/include/configs/mx6qsabrelite.h
> >>>>>>>> +++ b/include/configs/mx6qsabrelite.h
> >>>>>>>> @@ -46,9 +46,12 @@
> >>>>>>>> 
> >>>>>>>>      #define CONFIG_CMD_SF
> >>>>>>>>      #ifdef CONFIG_CMD_SF
> >>>>>>>> 
> >>>>>>>> +#define GPIO_3_19 ((2*32)+19)
> >>>>>>> 
> >>>>>>> I'd expect this to be in platform headers?
> >>>>>> 
> >>>>>> This is a choice made in the SabreLite design. I don't think
> >>>>>> the same choice has been made for other i.MX6 boards.
> >>>>> 
> >>>>> I mean the definition of the GPIO_3_19 ...
> >>>> 
> >>>> I don't think we want to set precedent for defining
> >>>> constants for the 100s of GPIO numbers.
> >>>> 
> >>>> That said, I could achieve my objective of clarifying
> >>>> what the number meant (that the constant refers to a GP) by
> >>>> 
> >>>> changing this:
> >>>> 	#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
> >>>> 
> >>>> to this
> >>>> 
> >>>> 	#define CONFIG_SF_DEFAULT_CS   (0|(IMX_GPIO_NR(3,19)<<8))
> >>> 
> >>> Why the (0| part ? Anyway, that indeed looks better, more standard
> >>> even.
> >>> 
> >>> And I think for MX5, there was even stuff defining the GPIO numbers
> >>> imported from Linux.
> >>> 
> >>> M
> >>> 
> >>>> There's a bit of an issue with this. The IMX_GPIO_NR() macro
> >>>> is defined in arch-mx6/gpio.h which is normally included after
> >>>> mx6qsabrelite.h because the latter defines the machine.
> >>> 
> >>> And the CPP will choke on that ?
> >> 
> >> Assembler or linker. IMX_GPIO_NR will be undefined and treated as an
> >> external unless we add this nested include into
> >> include/configs/mx6qsabrelite.h:
> >> 
> >> #ifndef __ASSEMBLY__
> >> #include<asm/arch/gpio.h>
> >> #endif
> >> 
> >> arch-mx6/gpio.h isn't directly ASM-friendly.
> >> 
> >> This seems like a lot of #include-foo for giving a name to a constant,
> >> don't you think?
> > 
> > Better than redefining stuff, which will eventually lead to errors and
> > breakage.
> 
> Hmmm. I just noticed that the IMX_GPIO_NR() macro isn't in Stefano's tree.
> My previous patches were against Dirk's	tree on GitHub, which has a 
patch
> from Troy:
> 	https://github.com/dirkbehme/u-boot-
imx6/commit/c8b2870efd041f820a91eebcb8
> 88c84a4f79f1f6
> 
> If we move this macro into arch-mx6/imx-regs.h, we avoid the #if.
> 
> Looking at the remaining content of gpio.h, it seems that it's
> driver-specific anyway (only the driver should be worried about the
> register layout of the GPIO controller).
> 
> If there are no objections, I'll forward a separate patch to define the
> macro.
> 
> Should this be based on Stefano's tree?

Definitelly. Please rebase to u-boot-imx.

Thanks for the patches and good work so far!

M
> 
> Please advise,
> 
> 
> Eric
Dirk Behme Jan. 30, 2012, 6:35 p.m. UTC | #10 
On 30.01.2012 19:10, Eric Nelson wrote:
> On 01/29/2012 07:36 PM, Marek Vasut wrote:
>>> On 01/29/2012 03:16 PM, Marek Vasut wrote:
>>>>> On 01/29/2012 01:11 PM, Marek Vasut wrote:
>>>>>>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
>>>>>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>>>>>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
>>>>>>>>> Acked-by: Stefano Babic<sbabic@denx.de>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> include/configs/mx6qsabrelite.h | 3 +++
>>>>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/configs/mx6qsabrelite.h
>>>>>>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
>>>>>>>>> --- a/include/configs/mx6qsabrelite.h
>>>>>>>>> +++ b/include/configs/mx6qsabrelite.h
>>>>>>>>> @@ -46,9 +46,12 @@
>>>>>>>>>
>>>>>>>>> #define CONFIG_CMD_SF
>>>>>>>>> #ifdef CONFIG_CMD_SF
>>>>>>>>>
>>>>>>>>> +#define GPIO_3_19 ((2*32)+19)
>>>>>>>>
>>>>>>>> I'd expect this to be in platform headers?
>>>>>>>
>>>>>>> This is a choice made in the SabreLite design. I don't think
>>>>>>> the same choice has been made for other i.MX6 boards.
>>>>>>
>>>>>> I mean the definition of the GPIO_3_19 ...
>>>>>
>>>>> I don't think we want to set precedent for defining
>>>>> constants for the 100s of GPIO numbers.
>>>>>
>>>>> That said, I could achieve my objective of clarifying
>>>>> what the number meant (that the constant refers to a GP) by
>>>>>
>>>>> changing this:
>>>>> #define CONFIG_SF_DEFAULT_CS (0|(GPIO_3_19<<8))
>>>>>
>>>>> to this
>>>>>
>>>>> #define CONFIG_SF_DEFAULT_CS (0|(IMX_GPIO_NR(3,19)<<8))
>>>>
>>>> Why the (0| part ? Anyway, that indeed looks better, more standard
>>>> even.
>>>>
>>>> And I think for MX5, there was even stuff defining the GPIO numbers
>>>> imported from Linux.
>>>>
>>>> M
>>>>
>>>>> There's a bit of an issue with this. The IMX_GPIO_NR() macro
>>>>> is defined in arch-mx6/gpio.h which is normally included after
>>>>> mx6qsabrelite.h because the latter defines the machine.
>>>>
>>>> And the CPP will choke on that ?
>>>
>>> Assembler or linker. IMX_GPIO_NR will be undefined and treated as an
>>> external unless we add this nested include into
>>> include/configs/mx6qsabrelite.h:
>>>
>>> #ifndef __ASSEMBLY__
>>> #include<asm/arch/gpio.h>
>>> #endif
>>>
>>> arch-mx6/gpio.h isn't directly ASM-friendly.
>>>
>>> This seems like a lot of #include-foo for giving a name to a constant,
>>> don't you think?
>>
>> Better than redefining stuff, which will eventually lead to errors
>> and breakage.
>>
>
> Hmmm. I just noticed that the IMX_GPIO_NR() macro isn't in Stefano's
> tree.
> My previous patches were against Dirk's tree on GitHub, which has a patch
> from Troy:
> https://github.com/dirkbehme/u-boot-imx6/commit/c8b2870efd041f820a91eebcb888c84a4f79f1f6
>
>
> If we move this macro into arch-mx6/imx-regs.h, we avoid the #if.
>
> Looking at the remaining content of gpio.h, it seems that it's
> driver-specific
> anyway (only the driver should be worried about the register layout of
> the GPIO
> controller).
>
> If there are no objections, I'll forward a separate patch to define
> the macro.

Yes, please. I have this on my todo list, too. But I haven't found the 
time to look at the consequences trying to mainline this patch. I.e. 
if we try to mainline this, we have to touch all gpio_xxx() functions 
to use this new macro? At least for i.MX6? Or does this macro apply 
for more i.MX SoCs? If yes, do we have to find out for which it will 
work? And move it to a i.MX common gpio header? And then touch all 
i.MX code it applies to?

Anyway, many thanks for your good work!

Best regards

Dirk

Btw.: It looks to me that the patch series to introduce the i.MX6 SPI 
driver increases from each version of the patch series to the next 
one. I'm not sure if this is ok? Or if we should try to split it to 
smaller chunks which would be easier to get them merged?
Eric Nelson Jan. 30, 2012, 7:36 p.m. UTC | #11 
On 01/30/2012 11:35 AM, Dirk Behme wrote:
> On 30.01.2012 19:10, Eric Nelson wrote:
>> On 01/29/2012 07:36 PM, Marek Vasut wrote:
>>>> On 01/29/2012 03:16 PM, Marek Vasut wrote:
>>>>>> On 01/29/2012 01:11 PM, Marek Vasut wrote:
>>>>>>>> On 01/29/2012 12:18 PM, Marek Vasut wrote:
>>>>>>>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>>>>>>>>> Acked-by: Dirk Behme<dirk.behme@de.bosch.com>
>>>>>>>>>> Acked-by: Stefano Babic<sbabic@denx.de>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> include/configs/mx6qsabrelite.h | 3 +++
>>>>>>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/configs/mx6qsabrelite.h
>>>>>>>>>> b/include/configs/mx6qsabrelite.h index 8dd6e39..cc770e2 100644
>>>>>>>>>> --- a/include/configs/mx6qsabrelite.h
>>>>>>>>>> +++ b/include/configs/mx6qsabrelite.h
>>>>>>>>>> @@ -46,9 +46,12 @@
>>>>>>>>>>
>>>>>>>>>> #define CONFIG_CMD_SF
>>>>>>>>>> #ifdef CONFIG_CMD_SF
>>>>>>>>>>
>>>>>>>>>> +#define GPIO_3_19 ((2*32)+19)
>>>>>>>>>
>>>>>>>>> I'd expect this to be in platform headers?
>>>>>>>>
>>>>>>>> This is a choice made in the SabreLite design. I don't think
>>>>>>>> the same choice has been made for other i.MX6 boards.
>>>>>>>
>>>>>>> I mean the definition of the GPIO_3_19 ...
>>>>>>
>>>>>> I don't think we want to set precedent for defining
>>>>>> constants for the 100s of GPIO numbers.
>>>>>>
>>>>>> That said, I could achieve my objective of clarifying
>>>>>> what the number meant (that the constant refers to a GP) by
>>>>>>
>>>>>> changing this:
>>>>>> #define CONFIG_SF_DEFAULT_CS (0|(GPIO_3_19<<8))
>>>>>>
>>>>>> to this
>>>>>>
>>>>>> #define CONFIG_SF_DEFAULT_CS (0|(IMX_GPIO_NR(3,19)<<8))
>>>>>
>>>>> Why the (0| part ? Anyway, that indeed looks better, more standard
>>>>> even.
>>>>>
>>>>> And I think for MX5, there was even stuff defining the GPIO numbers
>>>>> imported from Linux.
>>>>>
>>>>> M
>>>>>
>>>>>> There's a bit of an issue with this. The IMX_GPIO_NR() macro
>>>>>> is defined in arch-mx6/gpio.h which is normally included after
>>>>>> mx6qsabrelite.h because the latter defines the machine.
>>>>>
>>>>> And the CPP will choke on that ?
>>>>
>>>> Assembler or linker. IMX_GPIO_NR will be undefined and treated as an
>>>> external unless we add this nested include into
>>>> include/configs/mx6qsabrelite.h:
>>>>
>>>> #ifndef __ASSEMBLY__
>>>> #include<asm/arch/gpio.h>
>>>> #endif
>>>>
>>>> arch-mx6/gpio.h isn't directly ASM-friendly.
>>>>
>>>> This seems like a lot of #include-foo for giving a name to a constant,
>>>> don't you think?
>>>
>>> Better than redefining stuff, which will eventually lead to errors
>>> and breakage.
>>>
>>
>> Hmmm. I just noticed that the IMX_GPIO_NR() macro isn't in Stefano's
>> tree.
>> My previous patches were against Dirk's tree on GitHub, which has a patch
>> from Troy:
>> https://github.com/dirkbehme/u-boot-imx6/commit/c8b2870efd041f820a91eebcb888c84a4f79f1f6
>>
>> If we move this macro into arch-mx6/imx-regs.h, we avoid the #if.
>>
>> Looking at the remaining content of gpio.h, it seems that it's
>> driver-specific
>> anyway (only the driver should be worried about the register layout of
>> the GPIO
>> controller).
>>
>> If there are no objections, I'll forward a separate patch to define
>> the macro.
>
> Yes, please. I have this on my todo list, too. But I haven't found the time to
> look at the consequences trying to mainline this patch. I.e. if we try to
> mainline this, we have to touch all gpio_xxx() functions to use this new macro?
> At least for i.MX6? Or does this macro apply for more i.MX SoCs? If yes, do we
> have to find out for which it will work? And move it to a i.MX common gpio
> header? And then touch all i.MX code it applies to?
>

At the moment, the only code which uses IMX_GPIO_NR() is specific to
MX6Q and Sabre-Lite.

I looked for some commonality, but didn't find any in the i.MX trees.

arch-mx35/mx35-pins.h defines GPIO_TO_PORT() and GPIO_TO_INDEX()
which are the opposite of IMX_GPIO_NR().

arch-mx5/mx5x_pins.h does the same.

arch-davinci and arch-tegra2 both define GPIO_BANK() and GPIO_PORT()
for the same purpose

mx28 defines PAD_BANK() and PAD_PIN() but use an input of iomux_cfg_t
and not an integer.

The Linux model allows the platform to define the mapping from GPIO
numbers <-> drivers but doesn't use the concept of banks except within
a driver.

IOW, it doesn't seem like there's an obvious right thing to do, so
I'd like to suggest that either:

	- We define GPIO_NUMBER(bank,offset) inside imx-regs.h for
	use in mapping to GPIO numbers

	- We follow the convention of arch-mx5/ and arch-mx35 and
	define macros GPIO_TO_PORT() and GPIO_TO_INDEX() to go the
	other direction

	- We update drivers/gpio/mxc_gpio.c to use the GPIO_TO_PORT()
	and GPIO_TO_INDEX() macros instead of code like this:

		unsigned int port = gpio >> 5;

Or we just use the constant 0x53 for this value (as is done for the
efikamx and vision2 boards).

> Anyway, many thanks for your good work!
>
> Best regards
>
> Dirk
>
> Btw.: It looks to me that the patch series to introduce the i.MX6 SPI driver
> increases from each version of the patch series to the next one. I'm not sure if
> this is ok? Or if we should try to split it to smaller chunks which would be
> easier to get them merged?
>

Point taken. I've been wondering whether there was a way to steer clear
of the rabbit hole...

At the moment, I think patches 1-3 have been acked and really comprise
the 'refactoring' part.

I think I've addressed all of the concerns about patch 4 and got an ack about
patch 5 from Mike (still need Marek and Matthias to give feedback). Since these
patches define a new feature (default bus and chip-select), they should be
pulled out of the bundle.

I think we also have sign-off on patch 6, which is pretty minor and specific
to Sabre-Lite.

Patch 7 seems to be the only laggard.

To recap, I'll re-submit in four parts.

Regards,


Eric
diff mbox

Patch

diff --git a/include/configs/mx6qsabrelite.h b/include/configs/mx6qsabrelite.h
index 8dd6e39..cc770e2 100644
--- a/include/configs/mx6qsabrelite.h
+++ b/include/configs/mx6qsabrelite.h
@@ -46,9 +46,12 @@ 
 
 #define CONFIG_CMD_SF
 #ifdef CONFIG_CMD_SF
+#define GPIO_3_19 ((2*32)+19)
 #define CONFIG_SPI_FLASH
 #define CONFIG_SPI_FLASH_SST
 #define CONFIG_MXC_SPI
+#define CONFIG_SF_DEFAULT_BUS  0
+#define CONFIG_SF_DEFAULT_CS   (0|(GPIO_3_19<<8))
 #define CONFIG_SF_DEFAULT_SPEED 25000000
 #define CONFIG_SF_DEFAULT_MODE (SPI_MODE_0)
 #endif