From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Mon, 30 Jan 2012 12:36:20 -0700 Subject: [U-Boot] [PATCH V4 6/7] mx6q: mx6qsabrelite: Provide default serial flash bus and chip-select In-Reply-To: <4F26E30B.60905@googlemail.com> References: <1327863595-1524-1-git-send-email-eric.nelson@boundarydevices.com> <201201292316.26332.marek.vasut@gmail.com> <4F25D069.4080806@boundarydevices.com> <201201300336.22615.marek.vasut@gmail.com> <4F26DD0D.8040604@boundarydevices.com> <4F26E30B.60905@googlemail.com> Message-ID: <4F26F134.70101@boundarydevices.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>>>>> Acked-by: Dirk Behme >>>>>>>>>> Acked-by: Stefano Babic >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> 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 >>>> #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