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 Nelsoneric.nelson@boundarydevices.com >>>> Acked-by: Dirk Behmedirk.behme@de.bosch.com >>>> Acked-by: Stefano Babicsbabic@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/c8b2870efd041f820a91eebcb888...
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