| Message ID | B0658F55E67EDE4A914644632835B2CAF7EB40@DEUTERIUM.delien.local |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
> This patch fixes a small bug that allowed unintended manipulation of > non-existing GPIO pins within a pin bank, clobbering reserved bits. > > Signed-off-by: Robert Deliën <robert at delien.nl> > diff --git a/arch/arm/include/asm/arch-mx28/iomux.h > b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..829d9a8 100644 > --- a/arch/arm/include/asm/arch-mx28/iomux.h > +++ b/arch/arm/include/asm/arch-mx28/iomux.h > @@ -56,6 +56,12 @@ typedef u32 iomux_cfg_t; > #define MXS_PAD_PULL_VALID_SHIFT 16 > #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << > MXS_PAD_PULL_VALID_SHIFT) > > +#define MXS_BANK0_PINS 29 > +#define MXS_BANK1_PINS 32 > +#define MXS_BANK2_PINS 28 > +#define MXS_BANK3_PINS 31 > +#define MXS_BANK4_PINS 21 > + > #define PAD_MUXSEL_0 0 > #define PAD_MUXSEL_1 1 > #define PAD_MUXSEL_2 2 > diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c > index 539738b..fbc6da3 100644 > --- a/drivers/gpio/mxs_gpio.c > +++ b/drivers/gpio/mxs_gpio.c > @@ -120,9 +120,34 @@ int gpio_direction_output(int gp, int value) > > int gpio_request(int gp, const char *label) > { > + int bank_pins; > + > if (PAD_BANK(gp) >= PINCTRL_BANKS) > return -EINVAL; > > + switch(PAD_BANK(gp)) { > + case 0: > + bank_pins = MXS_BANK0_PINS; > + break; > + case 1: > + bank_pins = MXS_BANK1_PINS; > + break; > + case 2: > + bank_pins = MXS_BANK2_PINS; > + break; > + case 3: > + bank_pins = MXS_BANK3_PINS; > + break; > + case 4: > + bank_pins = MXS_BANK4_PINS; > + break; > + default: > + bank_pins = 0; > + } > + > + if (PAD_PIN(gp) >= bank_pins) > + return -EINVAL; > + > return 0; > } Hey, this looks reasonable. Did you send similar patch to Linux too ? M
> Hi Marek, > > > Hey, this looks reasonable. Did you send similar patch to Linux too ? > > No, I'm not at that yet. I'm just starting to explore the New U-Boot: It's > been since version 1.1.2 that I have actually contributed something. > > My current project requires a mechanism to manipulate GPIO pins, to aid the > bring-up of a new board in a couple of weeks. Until last weeks I've been > using the ancient FreeScale supplied U-Boot. When I took that in, your > work didn't appear in the repository yet, so finding that was a big and > pleasant surprise. Especially because the FreeScale version had a couple > of bugs in it. Not to start about the bugs in the supplied bootlets... > > So taking in U-Boot with your work is killing at least 3 birds with one > stone. > > I will do some work on the Kernel later. For now we're using the FreeScale > supplied Kernel. I have tried 3.1rc4 before and it ran our application > perfectly, so that's looking good already. But changing Kernel 2 weeks > before board bring-up is a bit too exciting for me. For that reason I will > probably hold on to the FreeScale bootlets and U-Boot version too. > > But now I've got your attention anyway: Is there a reason why U-Boot > expects the bootstream at sector 1024 of the SD-Card? To make things simple for users, there's standard layout. It's actually at sector 2048 btw. > The internal bootrom > doesn't know this limitation. It is clever enough to interpret the > partition table, and start loading SRAM from the first sector of the first > partition of type 53. That's what happens internally, it's just that some pieces (like sector offset of the partition!) need to be filled into the bootstream header by the mxsboot utility -- see mxsboot -h for how to change that. The 2048 sector offset was chosen because that's standard in Linux now for first partiiton. > Now I figure that the internal bootrom can only load > the first part of U-Boot, because of SRAM limits, and that this first part > has to power-up all power rails, initialize SD-RAM and load the remainder > of U-Boot, but I don't see why it couldn't be as clever as the internal > bootrom. What do you mean? > The reason I'm asking is because our project intends to boot from > MMC all the time. We don't think unmanaged NAND is reliable enough for our > appliction. We would like to have the oppertunity to boot from an SD-Card, > that also usable on Windows. And since that poor OS doesn't know the > difference between a disk, a partition and a file system it gets confused > if the first partition on an SD-Card isn't FAT formatted. You can adjust that, see above. > > Cheers, > > Robert.
> To make things simple for users, there's standard layout. It's actually at > sector 2048 btw. Got that. > That's what happens internally, it's just that some pieces (like sector offset > of the partition!) need to be filled into the bootstream header by the mxsboot > utility -- see mxsboot -h for how to change that. You're completely right! This sector needs to be know build-time because it is specified in the Boot Stream Block of the Boot Stream header. I forgot about that, because I hid that in a script some time ago. That script was basically a rewritten version of the broken one that was supplied with the SDK. Come to think of it, it's a bit weird that the sector the BSB is located in, is specified in the BSB: When reading it, it already knows this sector, because that's the sector it is reading from at that moment... > The 2048 sector offset was chosen because that's standard in Linux now for first > partiiton. Agreed. > What do you mean? What I meant to ask was how you solved the paradoxic problem that U-Boot needs to initialize SD-RAM before it can be loaded (and obviously U-Boot needs to be loaded before it can initialize SD-RAM). Freescale solved this problem by putting multiple bootlets in their bootstream. The first one is small enough to fit SRAM and it enables the PMIC's LDOs to power up all power domains. The second one too is small enough to fit SRAM. Loading it overwrites the first one and running it initializes SD-RAM. The third one is either U-Boot or a Kernel and is directly loaded into the now initialized SD-RAM. I figured that your solution links U-Boot in a clever way that the lower part fitting SRAM will initialize the PMIC and SD-RAM, after which U-Boot will load the remainder of itself from MMC into SD-RAM. > You can adjust that, see above. Understood. Cheers, Robert.
> > To make things simple for users, there's standard layout. It's actually > > at sector 2048 btw. > > Got that. > > > That's what happens internally, it's just that some pieces (like sector > > offset of the partition!) need to be filled into the bootstream header > > by the mxsboot utility -- see mxsboot -h for how to change that. > > You're completely right! This sector needs to be know build-time because it > is specified in the Boot Stream Block of the Boot Stream header. I forgot > about that, because I hid that in a script some time ago. That script was > basically a rewritten version of the broken one that was supplied with the > SDK. > > Come to think of it, it's a bit weird that the sector the BSB is located > in, is specified in the BSB: When reading it, it already knows this > sector, because that's the sector it is reading from at that moment... > > > The 2048 sector offset was chosen because that's standard in Linux now > > for first partiiton. > > Agreed. > > > What do you mean? > > What I meant to ask was how you solved the paradoxic problem that U-Boot > needs to initialize SD-RAM before it can be loaded (and obviously U-Boot > needs to be loaded before it can initialize SD-RAM). Well we have U-Boot SPL, which is loaded into SRAM, inits DRAM, pinmux etc., then we pass execution back to BootROM to load U-Boot into DRAM and execute it. > > Freescale solved this problem by putting multiple bootlets in their > bootstream. The first one is small enough to fit SRAM and it enables the > PMIC's LDOs to power up all power domains. The second one too is small > enough to fit SRAM. Loading it overwrites the first one and running it > initializes SD-RAM. The third one is either U-Boot or a Kernel and is > directly loaded into the now initialized SD-RAM. Yea, you can load kernel directly via U-Boot SPL too. > > I figured that your solution links U-Boot in a clever way that the lower > part fitting SRAM will initialize the PMIC and SD-RAM, after which U-Boot > will load the remainder of itself from MMC into SD-RAM. See above. > > > You can adjust that, see above. > > Understood. > > Cheers, > > Robert.
> This patch fixes a small bug that allowed unintended manipulation of > non-existing GPIO pins within a pin bank, clobbering reserved bits. > > Signed-off-by: Robert Deliën <robert at delien.nl> > diff --git a/arch/arm/include/asm/arch-mx28/iomux.h > b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..829d9a8 100644 > --- a/arch/arm/include/asm/arch-mx28/iomux.h > +++ b/arch/arm/include/asm/arch-mx28/iomux.h > @@ -56,6 +56,12 @@ typedef u32 iomux_cfg_t; > #define MXS_PAD_PULL_VALID_SHIFT 16 > #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << > MXS_PAD_PULL_VALID_SHIFT) > > +#define MXS_BANK0_PINS 29 > +#define MXS_BANK1_PINS 32 > +#define MXS_BANK2_PINS 28 > +#define MXS_BANK3_PINS 31 > +#define MXS_BANK4_PINS 21 > + > #define PAD_MUXSEL_0 0 > #define PAD_MUXSEL_1 1 > #define PAD_MUXSEL_2 2 > diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c > index 539738b..fbc6da3 100644 > --- a/drivers/gpio/mxs_gpio.c > +++ b/drivers/gpio/mxs_gpio.c > @@ -120,9 +120,34 @@ int gpio_direction_output(int gp, int value) > > int gpio_request(int gp, const char *label) > { > + int bank_pins; > + > if (PAD_BANK(gp) >= PINCTRL_BANKS) > return -EINVAL; > > + switch(PAD_BANK(gp)) { > + case 0: > + bank_pins = MXS_BANK0_PINS; > + break; > + case 1: > + bank_pins = MXS_BANK1_PINS; > + break; > + case 2: > + bank_pins = MXS_BANK2_PINS; > + break; > + case 3: > + bank_pins = MXS_BANK3_PINS; > + break; > + case 4: > + bank_pins = MXS_BANK4_PINS; > + break; > + default: > + bank_pins = 0; > + } > + > + if (PAD_PIN(gp) >= bank_pins) > + return -EINVAL; > + > return 0; > } Careful here !! The driver _should_ work for MX233 too! What I'd like to see is you introducing a function like: int mxs_gpio_is_valid(gpio) { char mxs_banks[PINCTRL_BANKS] = PINCTRL_BANK_COUNTS; if (PAD_PIN(gpio) > mxs_bank[PAD_BANK(gpio)]) return -EINVAL; return 0; } And define PINCTRL_BANK_COUNTS in the section of mxs_gpio.c where all the remaining mx28 and mx233 specific defines are hoarded (near the top of the file). M
> Careful here !! > > The driver _should_ work for MX233 too! What I'd like to see is you introducing > a function like: > > int mxs_gpio_is_valid(gpio) > { > char mxs_banks[PINCTRL_BANKS] = PINCTRL_BANK_COUNTS; > > if (PAD_PIN(gpio) > mxs_bank[PAD_BANK(gpio)]) > return -EINVAL; > > return 0; > } > > And define PINCTRL_BANK_COUNTS in the section of mxs_gpio.c where all the > remaining mx28 and mx233 specific defines are hoarded (near the top of the > file). Does mx233 use arch/arm/include/asm/arch-mx28/iomux.h? In that case we should consider renaming it to arch/arm/include/asm/arch-mxs, for all Sigmatel base FreeScale SoCs. I do like the idea of a function validating pins. If I'm going to introduce changes that big, I may also introduce functionality to set pin functions run-time, as that is very useful during board bring-up.
On Tuesday 22 November 2011 09:41:48 Robert Deliën wrote: > + switch(PAD_BANK(gp)) { needs a space after that "switch" > + case 0: > + bank_pins = MXS_BANK0_PINS; > + break; > + case 1: > + bank_pins = MXS_BANK1_PINS; > + break; > + case 2: > + bank_pins = MXS_BANK2_PINS; > + break; > + case 3: > + bank_pins = MXS_BANK3_PINS; > + break; > + case 4: > + bank_pins = MXS_BANK4_PINS; > + break; > + default: > + bank_pins = 0; > + } static const int all_bank_pins[] = { MXS_BANK0_PINS, MXS_BANK1_PINS, MXS_BANK2_PINS, MXS_BANK3_PINS, MXS_BANK4_PINS, }; bank_pins = all_bank_pins[PAD_PANK(gp)]; -mike
> static const int all_bank_pins[] = { > MXS_BANK0_PINS, > MXS_BANK1_PINS, > MXS_BANK2_PINS, > MXS_BANK3_PINS, > MXS_BANK4_PINS, > }; > bank_pins = all_bank_pins[PAD_PANK(gp)]; Good one! But how about the changes Marek suggested about name_to_gpio_number? Robert.
> > Careful here !! > > > > The driver _should_ work for MX233 too! What I'd like to see is you > > introducing a function like: > > > > int mxs_gpio_is_valid(gpio) > > { > > char mxs_banks[PINCTRL_BANKS] = PINCTRL_BANK_COUNTS; > > > > if (PAD_PIN(gpio) > mxs_bank[PAD_BANK(gpio)]) > > > > return -EINVAL; > > > > return 0; > > } > > > > And define PINCTRL_BANK_COUNTS in the section of mxs_gpio.c where all the > > remaining mx28 and mx233 specific defines are hoarded (near the top of > > the file). > > Does mx233 use arch/arm/include/asm/arch-mx28/iomux.h? In that case we > should consider renaming it to arch/arm/include/asm/arch-mxs, for all > Sigmatel base FreeScale SoCs. It doesn't, because it's not yet supported. > > I do like the idea of a function validating pins. If I'm going to introduce > changes that big, I may also introduce functionality to set pin functions > run-time, as that is very useful during board bring-up. Please introduce the validation function first, the runtime stuff can be done later. M
> Careful here !! > > The driver _should_ work for MX233 too! What I'd like to see is you introducing > a function like: > > int mxs_gpio_is_valid(gpio) > { > char mxs_banks[PINCTRL_BANKS] = PINCTRL_BANK_COUNTS; > > if (PAD_PIN(gpio) > mxs_bank[PAD_BANK(gpio)]) > return -EINVAL; > > return 0; > } There's a bit of a paradox here: If a name is translated into a pin, bank numbers above 7 and pin numbers above 31 will have wrapped around in translation and won't be caught here. I could check for wrapping in the translating function and check for valid numbers within the assigned bit range here, but I'd rather not see validity check spread over two functions. > And define PINCTRL_BANK_COUNTS in the section of mxs_gpio.c where all the > remaining mx28 and mx233 specific defines are hoarded (near the top of the > file). At that spot I've put in Mike's (very similar) solution now: static const int mxs_bank_pins[] = { MXS_BANK0_PINS, MXS_BANK1_PINS, MXS_BANK2_PINS, #ifdef(CONFIG_MX28) MXS_BANK3_PINS, MXS_BANK4_PINS, #endif }; I'm considering to remove the macro PINCTRL_BANKS now, and use ARRAY_SIZE(mxs_bank_pins) instead, as it yields the same number and leads to a single point of definition.
> > Careful here !! > > > > The driver _should_ work for MX233 too! What I'd like to see is you > > introducing a function like: > > > > int mxs_gpio_is_valid(gpio) > > { > > char mxs_banks[PINCTRL_BANKS] = PINCTRL_BANK_COUNTS; > > > > if (PAD_PIN(gpio) > mxs_bank[PAD_BANK(gpio)]) > > return -EINVAL; > > > > return 0; > > } > > There's a bit of a paradox here: If a name is translated into a pin, bank > numbers above 7 and pin numbers above 31 will have wrapped around in > translation and won't be caught here. I could check for wrapping in the > translating function and check for valid numbers within the assigned bit > range here, but I'd rather not see validity check spread over two > functions. > > > And define PINCTRL_BANK_COUNTS in the section of mxs_gpio.c where all the > > remaining mx28 and mx233 specific defines are hoarded (near the top of > > the file). > > At that spot I've put in Mike's (very similar) solution now: > static const int mxs_bank_pins[] = { > MXS_BANK0_PINS, > MXS_BANK1_PINS, > MXS_BANK2_PINS, > #ifdef(CONFIG_MX28) > MXS_BANK3_PINS, > MXS_BANK4_PINS, > #endif > }; > > I'm considering to remove the macro PINCTRL_BANKS now, and use > ARRAY_SIZE(mxs_bank_pins) instead, as it yields the same number and leads > to a single point of definition. And are you sure the amound of pins in bank 0, 1, 2 is the same on mx233 and mx28 ?
And are you sure the amound of pins in bank 0, 1, 2 is the same on mx233 and
mx28 ?
Yes, I'm sure they're not ;-). I went through the manual and changed it into this:
static const int mxs_bank_pins[] = {
#if defined(CONFIG_MX23)
MX23_BANK0_PINS,
MX23_BANK1_PINS,
MX23_BANK2_PINS
#elif defined(CONFIG_MX28)
MX28_BANK0_PINS,
MX28_BANK1_PINS,
MX28_BANK2_PINS,
MX28_BANK3_PINS,
MX28_BANK4_PINS
#endif
};
I also implemented a non-static function gpio_name_to_pin. I didn't implement validity checking in it, but is does check for set bits outside the pin and bank masks. To check if a pin is actually valid, I have added a non-static function gpio_invalid, as you suggested. I just named it differently because a function called gpio_is_valid returning 0 in case the pin is valid might be a bit confusing when evaluating the return parameter.
As a result, there's no longer a validity check in gpio_request anymore. Or would you suggest to do this check at the beginning of each non-static function taking a gp input parameter?
I'm not really sure yet how to implement Mike's suggestion to deal with diversity, but I'll work on that now.
diff --git a/arch/arm/include/asm/arch-mx28/iomux.h b/arch/arm/include/asm/arch-mx28/iomux.h index 7abdf58..829d9a8 100644 --- a/arch/arm/include/asm/arch-mx28/iomux.h +++ b/arch/arm/include/asm/arch-mx28/iomux.h @@ -56,6 +56,12 @@ typedef u32 iomux_cfg_t; #define MXS_PAD_PULL_VALID_SHIFT 16 #define MXS_PAD_PULL_VALID_MASK ((iomux_cfg_t)0x1 << MXS_PAD_PULL_VALID_SHIFT) +#define MXS_BANK0_PINS 29 +#define MXS_BANK1_PINS 32 +#define MXS_BANK2_PINS 28 +#define MXS_BANK3_PINS 31 +#define MXS_BANK4_PINS 21 + #define PAD_MUXSEL_0 0 #define PAD_MUXSEL_1 1 #define PAD_MUXSEL_2 2 diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c index 539738b..fbc6da3 100644 --- a/drivers/gpio/mxs_gpio.c +++ b/drivers/gpio/mxs_gpio.c @@ -120,9 +120,34 @@ int gpio_direction_output(int gp, int value) int gpio_request(int gp, const char *label) { + int bank_pins; + if (PAD_BANK(gp) >= PINCTRL_BANKS) return -EINVAL; + switch(PAD_BANK(gp)) { + case 0: + bank_pins = MXS_BANK0_PINS; + break; + case 1: + bank_pins = MXS_BANK1_PINS; + break; + case 2: + bank_pins = MXS_BANK2_PINS; + break; + case 3: + bank_pins = MXS_BANK3_PINS; + break; + case 4: + bank_pins = MXS_BANK4_PINS; + break; + default: + bank_pins = 0; + } + + if (PAD_PIN(gp) >= bank_pins) + return -EINVAL; + return 0; }
This patch fixes a small bug that allowed unintended manipulation of non-existing GPIO pins within a pin bank, clobbering reserved bits. Signed-off-by: Robert Deliën <robert at delien.nl>