From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Mon, 30 Jan 2012 11:10:21 -0700 Subject: [U-Boot] [PATCH V4 6/7] mx6q: mx6qsabrelite: Provide default serial flash bus and chip-select In-Reply-To: <201201300336.22615.marek.vasut@gmail.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> Message-ID: <4F26DD0D.8040604@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/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. Should this be based on Stefano's tree? Please advise, Eric