| Message ID | 20210713065854.134634-2-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | hw: aspeed_gpio: MMIO region fix and cleanups | expand |
On Tue, 2021-07-13 at 16:28 +0930, Joel Stanley wrote: > The macro used to calculate the maximum memory size of the MMIO > region > had a mistake, causing all GPIO models to create a mapping of 0x9D8. > The intent was to have it be 0x9D8 - 0x800. > > This extra size doesn't matter on ast2400 and ast2500, which have a > 4KB > region set aside for the GPIO controller. > > On the ast2600 the 3.3V and 1.8V GPIO controllers are 2KB apart, so > the > regions would overlap. Worse was the 1.8V controller would map over > the > top of the following perianal, which happens to be the RTC. > > The mmio region used by each device is a maximum of 2KB, so avoid the > calculations and hard code this as the maximum. > > Fixes: 36d737ee82b2 ("hw/gpio: Add in AST2600 specific > implementation") > Signed-off-by: Joel Stanley <joel@jms.id.au> derp. Sorry about that. This looks correct. Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com> > --- > hw/gpio/aspeed_gpio.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c > index 6ae0116be70b..b3dec4448009 100644 > --- a/hw/gpio/aspeed_gpio.c > +++ b/hw/gpio/aspeed_gpio.c > @@ -207,7 +207,6 @@ > #define GPIO_1_8V_MEM_SIZE 0x9D8 > #define GPIO_1_8V_REG_ARRAY_SIZE ((GPIO_1_8V_MEM_SIZE - \ > GPIO_1_8V_REG_OFFSET) >> 2) > -#define GPIO_MAX_MEM_SIZE MAX(GPIO_3_6V_MEM_SIZE, > GPIO_1_8V_MEM_SIZE) > > static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, > int gpio) > { > @@ -849,7 +848,7 @@ static void aspeed_gpio_realize(DeviceState *dev, > Error **errp) > } > > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s, > - TYPE_ASPEED_GPIO, GPIO_MAX_MEM_SIZE); > + TYPE_ASPEED_GPIO, 0x800); > > sysbus_init_mmio(sbd, &s->iomem); > }
On 7/13/21 8:58 AM, Joel Stanley wrote: > The macro used to calculate the maximum memory size of the MMIO region > had a mistake, causing all GPIO models to create a mapping of 0x9D8. > The intent was to have it be 0x9D8 - 0x800. > > This extra size doesn't matter on ast2400 and ast2500, which have a 4KB > region set aside for the GPIO controller. > > On the ast2600 the 3.3V and 1.8V GPIO controllers are 2KB apart, so the > regions would overlap. Worse was the 1.8V controller would map over the > top of the following perianal, which happens to be the RTC. > > The mmio region used by each device is a maximum of 2KB, so avoid the > calculations and hard code this as the maximum. > > Fixes: 36d737ee82b2 ("hw/gpio: Add in AST2600 specific implementation") > Signed-off-by: Joel Stanley <joel@jms.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/gpio/aspeed_gpio.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c > index 6ae0116be70b..b3dec4448009 100644 > --- a/hw/gpio/aspeed_gpio.c > +++ b/hw/gpio/aspeed_gpio.c > @@ -207,7 +207,6 @@ > #define GPIO_1_8V_MEM_SIZE 0x9D8 > #define GPIO_1_8V_REG_ARRAY_SIZE ((GPIO_1_8V_MEM_SIZE - \ > GPIO_1_8V_REG_OFFSET) >> 2) > -#define GPIO_MAX_MEM_SIZE MAX(GPIO_3_6V_MEM_SIZE, GPIO_1_8V_MEM_SIZE) > > static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, int gpio) > { > @@ -849,7 +848,7 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp) > } > > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s, > - TYPE_ASPEED_GPIO, GPIO_MAX_MEM_SIZE); > + TYPE_ASPEED_GPIO, 0x800); > > sysbus_init_mmio(sbd, &s->iomem); > } >
On Mon, 19 Jul 2021 at 16:02, Cédric Le Goater <clg@kaod.org> wrote: > > On 7/13/21 8:58 AM, Joel Stanley wrote: > > The macro used to calculate the maximum memory size of the MMIO region > > had a mistake, causing all GPIO models to create a mapping of 0x9D8. > > The intent was to have it be 0x9D8 - 0x800. > > > > This extra size doesn't matter on ast2400 and ast2500, which have a 4KB > > region set aside for the GPIO controller. > > > > On the ast2600 the 3.3V and 1.8V GPIO controllers are 2KB apart, so the > > regions would overlap. Worse was the 1.8V controller would map over the > > top of the following perianal, which happens to be the RTC. > > > > The mmio region used by each device is a maximum of 2KB, so avoid the > > calculations and hard code this as the maximum. > > > > Fixes: 36d737ee82b2 ("hw/gpio: Add in AST2600 specific implementation") > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > Reviewed-by: Cédric Le Goater <clg@kaod.org> Cedric, Peter; can we please get this merged for 6.1? Without it the RTC model is not functional on the ast2500. The other patches in this series are cleanups that can wait for future releases. Cheers, Joel > > > --- > > hw/gpio/aspeed_gpio.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c > > index 6ae0116be70b..b3dec4448009 100644 > > --- a/hw/gpio/aspeed_gpio.c > > +++ b/hw/gpio/aspeed_gpio.c > > @@ -207,7 +207,6 @@ > > #define GPIO_1_8V_MEM_SIZE 0x9D8 > > #define GPIO_1_8V_REG_ARRAY_SIZE ((GPIO_1_8V_MEM_SIZE - \ > > GPIO_1_8V_REG_OFFSET) >> 2) > > -#define GPIO_MAX_MEM_SIZE MAX(GPIO_3_6V_MEM_SIZE, GPIO_1_8V_MEM_SIZE) > > > > static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, int gpio) > > { > > @@ -849,7 +848,7 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp) > > } > > > > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s, > > - TYPE_ASPEED_GPIO, GPIO_MAX_MEM_SIZE); > > + TYPE_ASPEED_GPIO, 0x800); > > > > sysbus_init_mmio(sbd, &s->iomem); > > } > > >
On Tue, 27 Jul 2021 at 09:02, Joel Stanley <joel@jms.id.au> wrote: > > On Mon, 19 Jul 2021 at 16:02, Cédric Le Goater <clg@kaod.org> wrote: > > > > On 7/13/21 8:58 AM, Joel Stanley wrote: > > > The macro used to calculate the maximum memory size of the MMIO region > > > had a mistake, causing all GPIO models to create a mapping of 0x9D8. > > > The intent was to have it be 0x9D8 - 0x800. > > > > > > This extra size doesn't matter on ast2400 and ast2500, which have a 4KB > > > region set aside for the GPIO controller. > > > > > > On the ast2600 the 3.3V and 1.8V GPIO controllers are 2KB apart, so the > > > regions would overlap. Worse was the 1.8V controller would map over the > > > top of the following perianal, which happens to be the RTC. I'm going to assume this is an unfortunate autocorrect for "following peripheral", and will tweak the commit message... > > > > > > The mmio region used by each device is a maximum of 2KB, so avoid the > > > calculations and hard code this as the maximum. > > > > > > Fixes: 36d737ee82b2 ("hw/gpio: Add in AST2600 specific implementation") > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Cedric, Peter; can we please get this merged for 6.1? Without it the > RTC model is not functional on the ast2500. I'm doing an arm pullreq today so I'll put it into that. thanks -- PMM
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c index 6ae0116be70b..b3dec4448009 100644 --- a/hw/gpio/aspeed_gpio.c +++ b/hw/gpio/aspeed_gpio.c @@ -207,7 +207,6 @@ #define GPIO_1_8V_MEM_SIZE 0x9D8 #define GPIO_1_8V_REG_ARRAY_SIZE ((GPIO_1_8V_MEM_SIZE - \ GPIO_1_8V_REG_OFFSET) >> 2) -#define GPIO_MAX_MEM_SIZE MAX(GPIO_3_6V_MEM_SIZE, GPIO_1_8V_MEM_SIZE) static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, int gpio) { @@ -849,7 +848,7 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp) } memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s, - TYPE_ASPEED_GPIO, GPIO_MAX_MEM_SIZE); + TYPE_ASPEED_GPIO, 0x800); sysbus_init_mmio(sbd, &s->iomem); }
The macro used to calculate the maximum memory size of the MMIO region had a mistake, causing all GPIO models to create a mapping of 0x9D8. The intent was to have it be 0x9D8 - 0x800. This extra size doesn't matter on ast2400 and ast2500, which have a 4KB region set aside for the GPIO controller. On the ast2600 the 3.3V and 1.8V GPIO controllers are 2KB apart, so the regions would overlap. Worse was the 1.8V controller would map over the top of the following perianal, which happens to be the RTC. The mmio region used by each device is a maximum of 2KB, so avoid the calculations and hard code this as the maximum. Fixes: 36d737ee82b2 ("hw/gpio: Add in AST2600 specific implementation") Signed-off-by: Joel Stanley <joel@jms.id.au> --- hw/gpio/aspeed_gpio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)