M28: Added guarding for reserved bits in GPIO driver
Subsystems (3)
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
This patch fixes a small bug that allowed unintended manipulation of non-existing GPIO pins within a pin bank, clobbering reserved bits.
arch/arm/include/asm/arch-mx28/iomux.h +6 -0
@@ -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
drivers/gpio/mxs_gpio.c +25 -0
@@ -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 ; }
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 50 lines
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 <[email protected]>(quoted diff) +0
-0
b/arch/arm/include/asm/arch-mx28/iomux.hindex 7abdf58..829d9a8 100644arch/arm/include/asm/arch-mx28/iomux.h +6
-0@@ -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_22drivers/gpio/mxs_gpio.c +17
-0@@ -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 ;
drivers/gpio/mxs_gpio.c int gpio_direction_output(int gp, int value) · line 152
Quoted 10 lines
drivers/gpio/mxs_gpio.c +8
-0+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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 17 lines
Hi Marek,
Quoted 1 line
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.
Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 5 lines
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?
Quoted 6 lines
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.
Quoted 4 lines
Cheers,
Robert.
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 2 lines
To make things simple for users, there's standard layout. It's actually at sector 2048 btw.
Got that.
Quoted 3 lines
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...
Quoted 2 lines
The 2048 sector offset was chosen because that's standard in Linux now for first partiiton.
Agreed.
Quoted 1 line
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.
Quoted 1 line
You can adjust that, see above.
Understood.
Cheers,
Robert.
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 22 lines
Quoted 2 lines
To make things simple for users, there's standard layout. It's actually at sector 2048 btw.
Got that.
Quoted 3 lines
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...
Quoted 2 lines
The 2048 sector offset was chosen because that's standard in Linux now for first partiiton.
Quoted 7 lines
Agreed.
Quoted 1 line
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.
Quoted 7 lines
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.
Quoted 4 lines
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.
Quoted 8 lines
Quoted 2 lines
You can adjust that, see above.
Understood.
Cheers,
Robert.
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
In reply to Robert DeliënQuoted 50 lines
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 <[email protected]>(quoted diff) +0
-0
b/arch/arm/include/asm/arch-mx28/iomux.hindex 7abdf58..829d9a8 100644arch/arm/include/asm/arch-mx28/iomux.h +6
-0@@ -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_22drivers/gpio/mxs_gpio.c +17
-0@@ -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 ;
drivers/gpio/mxs_gpio.c int gpio_direction_output(int gp, int value) · line 152
Quoted 10 lines
drivers/gpio/mxs_gpio.c +8
-0+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
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 10 lines
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)])
Quoted 8 lines
return -EINVAL;return 0;
}And define
PINCTRL_BANK_COUNTSin the section ofmxs_gpio.cwhere 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.
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 16 lines
Quoted 16 lines
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;
}
Quoted 7 lines
Quoted 3 lines
And define
PINCTRL_BANK_COUNTSin the section ofmxs_gpio.cwhere 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 toarch/arm/include/asm/arch-mxs, for all Sigmatel base FreeScale SoCs.
It doesn't, because it's not yet supported.
Quoted 4 lines
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
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
In reply to Marek VasutQuoted 9 lines
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;
Quoted 5 lines
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.
Quoted 3 lines
And define
PINCTRL_BANK_COUNTSin the section ofmxs_gpio.cwhere 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.
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 34 lines
Quoted 9 lines
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;Quoted 5 lines
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.
Quoted 3 lines
And define
PINCTRL_BANK_COUNTSin the section ofmxs_gpio.cwhere 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,
Quoted 6 lines
#endif
};I'm considering to remove the macro
PINCTRL_BANKSnow, and useARRAY_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 ?
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
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.
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
In reply to Robert DeliënQuoted 1 line · On Tuesday 22 November 2011 09:41:48 Robert Deli?n wrote:
+ switch(PAD_BANK(gp)) {
needs a space after that "switch"
Quoted 8 lines
drivers/gpio/mxs_gpio.c +8
-0+case 0 : + bank_pins =MXS_BANK0_PINS ; +break ; +case 1 : + bank_pins =MXS_BANK1_PINS ; +break ; +case 2 : + bank_pins =MXS_BANK2_PINS ;
drivers/gpio/mxs_gpio.c int gpio_direction_output(int gp, int value) · line 146
Quoted 10 lines
drivers/gpio/mxs_gpio.c +10
-0+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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20111122/9f361726/attachment.pgp>
PATCH 1/1 M28: Added guarding for reserved bits in GPIO driver
Quoted 8 lines
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.