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

[Eric Nelson <eric.nelson@boundarydevices.com>]

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