| Message ID | 1477894297-12249-1-git-send-email-andrew@aj.id.au |
|---|---|
| State | Superseded, archived |
| Delegated to: | Joel Stanley |
| Headers | show |
On 10/31/2016 07:11 AM, Andrew Jeffery wrote: > The description of SCU90[6] from the datasheet: 'Reserved, must keep at > value ”0”'. > > Switch from the bit-flipping macro to explicitly configuring > .enable = .disable = 0. > > If a pin depending on SCU90[6] is requested for GPIO, the export will > succeed but changes to the GPIO's value fail to stick. With the fix the > value can be toggled as expected. The patch has been tested on an > AST2500 EVB. That is an important condition for SP1 also. Could that have an impact on the spi1debug setting in the witherspoon DTS ? Is it still needed ? C. > Reported-by: Uma Yadlapati <yadlapat@us.ibm.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > index 481e836d12e5..9a3139c13ffc 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > @@ -26,7 +26,7 @@ > > #define ASPEED_G5_NR_PINS 232 > > -#define COND1 SIG_DESC_BIT(SCU90, 6, 0) > +#define COND1 { SCU90, BIT(6), 0, 0 } > #define COND2 { SCU94, GENMASK(1, 0), 0, 0 } > > #define LHCR0 SIG_DESC_TO_REG(ASPEED_IP_LPC, 0xA0) >
Hi Andrew, Thanks for finding a fix so quickly. On Mon, Oct 31, 2016 at 4:41 PM, Andrew Jeffery <andrew@aj.id.au> wrote: > The description of SCU90[6] from the datasheet: 'Reserved, must keep at > value ”0”'. > > Switch from the bit-flipping macro to explicitly configuring > .enable = .disable = 0. > > If a pin depending on SCU90[6] is requested for GPIO, the export will > succeed but changes to the GPIO's value fail to stick. With the fix the > value can be toggled as expected. The patch has been tested on an > AST2500 EVB. I couldn't follow what the issue was and how you fixed it, so I've rewritten it in laymans terms. Let me know what you think: If a pin depending on bit 6 in SCU90 is requested for GPIO, the export will succeed but changes to the GPIO's value will not be accepted by the hardware. This is because pinmux has misconfigured the SCU by writing 1 to the reserved bit. The description of SCU90[6] from the datasheet is 'Reserved, must keep at value ”0”'. The fix is to switch pinmux from the bit-flipping macro to explicitly configuring the .enable and .disable values to zero. The patch has been tested on an AST2500 EVB. Cheers, Joel > > Reported-by: Uma Yadlapati <yadlapat@us.ibm.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > index 481e836d12e5..9a3139c13ffc 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > @@ -26,7 +26,7 @@ > > #define ASPEED_G5_NR_PINS 232 > > -#define COND1 SIG_DESC_BIT(SCU90, 6, 0) > +#define COND1 { SCU90, BIT(6), 0, 0 } > #define COND2 { SCU94, GENMASK(1, 0), 0, 0 } > > #define LHCR0 SIG_DESC_TO_REG(ASPEED_IP_LPC, 0xA0) > -- > 2.7.4 >
On Mon, 2016-10-31 at 07:31 +0100, Cédric Le Goater wrote: > On 10/31/2016 07:11 AM, Andrew Jeffery wrote: > > > > The description of SCU90[6] from the datasheet: 'Reserved, must keep at > > value ”0”'. > > > > Switch from the bit-flipping macro to explicitly configuring > > .enable = .disable = 0. > > > > If a pin depending on SCU90[6] is requested for GPIO, the export will > > succeed but changes to the GPIO's value fail to stick. With the fix the > > value can be toggled as expected. The patch has been tested on an > > AST2500 EVB. > That is an important condition for SP1 also. Indeed, though in this case the mux is not affected because SPI1 is the highest priority function for the relevant pins, and SCU90[6]'s default value is zero. Therefore it won't ever have been (wrongly) toggled by this bug in the pinmux driver configuration. > Could that have an impact on > the spi1debug setting in the witherspoon DTS ? No: The need to configure spi1debug in the devicetree is purely driven by the value in bits 12 and 13 of the strap register. If we want to use some other configuration in the devicetree, e.g. spi1master, then we need to change the strapping register in some way (e.g. hardware or u- boot). However, Kun Yi was looking at whether we can make this more flexible. > Is it still needed ? Yes, per the above. > > C. > > > > > Reported-by: Uma Yadlapati <yadlapat@us.ibm.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > > index 481e836d12e5..9a3139c13ffc 100644 > > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > > @@ -26,7 +26,7 @@ > > > > #define ASPEED_G5_NR_PINS 232 > > > > -#define COND1 SIG_DESC_BIT(SCU90, 6, 0) > > +#define COND1 { SCU90, BIT(6), 0, 0 } > > #define COND2 { SCU94, GENMASK(1, 0), 0, 0 } > > > > #define LHCR0 SIG_DESC_TO_REG(ASPEED_IP_LPC, 0xA0) > > > _______________________________________________ > openbmc mailing list > openbmc@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/openbmc
On Mon, 2016-10-31 at 17:01 +1030, Joel Stanley wrote: > Hi Andrew, > > Thanks for finding a fix so quickly. > > On Mon, Oct 31, 2016 at 4:41 PM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > The description of SCU90[6] from the datasheet: 'Reserved, must keep at > > value ”0”'. > > > > Switch from the bit-flipping macro to explicitly configuring > > .enable = .disable = 0. > > > > If a pin depending on SCU90[6] is requested for GPIO, the export will > > succeed but changes to the GPIO's value fail to stick. With the fix the > > value can be toggled as expected. The patch has been tested on an > > AST2500 EVB. > I couldn't follow what the issue was and how you fixed it, so I've > rewritten it in laymans terms. Let me know what you think: > > If a pin depending on bit 6 in SCU90 is requested for GPIO, the export > will succeed but changes to the GPIO's value will not be accepted by > the hardware. This is because pinmux has misconfigured the SCU by > writing 1 to the reserved bit. > > The description of SCU90[6] from the datasheet is 'Reserved, must keep > at value ”0”'. The fix is to switch pinmux from the bit-flipping macro > to explicitly configuring the .enable and .disable values to zero. > > The patch has been tested on an AST2500 EVB. > This flows a lot better. I'll send a v2. Thanks, Andrew > Cheers, > > Joel > > > > > > > Reported-by: Uma Yadlapati <yadlapat@us.ibm.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > > index 481e836d12e5..9a3139c13ffc 100644 > > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c > > @@ -26,7 +26,7 @@ > > > > #define ASPEED_G5_NR_PINS 232 > > > > -#define COND1 SIG_DESC_BIT(SCU90, 6, 0) > > +#define COND1 { SCU90, BIT(6), 0, 0 } > > #define COND2 { SCU94, GENMASK(1, 0), 0, 0 } > > > > #define LHCR0 SIG_DESC_TO_REG(ASPEED_IP_LPC, 0xA0) > > -- > > 2.7.4 > >
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c index 481e836d12e5..9a3139c13ffc 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c @@ -26,7 +26,7 @@ #define ASPEED_G5_NR_PINS 232 -#define COND1 SIG_DESC_BIT(SCU90, 6, 0) +#define COND1 { SCU90, BIT(6), 0, 0 } #define COND2 { SCU94, GENMASK(1, 0), 0, 0 } #define LHCR0 SIG_DESC_TO_REG(ASPEED_IP_LPC, 0xA0)
The description of SCU90[6] from the datasheet: 'Reserved, must keep at value ”0”'. Switch from the bit-flipping macro to explicitly configuring .enable = .disable = 0. If a pin depending on SCU90[6] is requested for GPIO, the export will succeed but changes to the GPIO's value fail to stick. With the fix the value can be toggled as expected. The patch has been tested on an AST2500 EVB. Reported-by: Uma Yadlapati <yadlapat@us.ibm.com> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)