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

[Dirk Behme]

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 at boundarydevices.com>
>>>>>>>>> Acked-by: Dirk Behme<dirk.behme at de.bosch.com>
>>>>>>>>> Acked-by: Stefano Babic<sbabic at 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?