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

[Eric Nelson]

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

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