From: Felix Wu <flwu@google.com>
Added 32 bits property for ASPEED GPIO. Previously it can only be access in bitwise manner.
This change gives ASPEED similar behavior as Nuvoton.
Signed-off-by: Felix Wu <flwu@google.com>
---
hw/gpio/aspeed_gpio.c | 57 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 609a556908..2d78bf9515 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -1308,6 +1308,57 @@ static void aspeed_gpio_2700_write(void *opaque, hwaddr offset,
}
/* Setup functions */
+static void aspeed_gpio_set_set(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ uint32_t set_val = 0;
+ AspeedGPIOState *s = ASPEED_GPIO(obj);
+ AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+ int set_idx = 0;
+
+ if (!visit_type_uint32(v, name, &set_val, errp)) {
+ return;
+ }
+
+ if (sscanf(name, "gpio-set[%d]", &set_idx) != 1) {
+ error_setg(errp, "%s: error reading %s", __func__, name);
+ return;
+ }
+
+ if (set_idx >= agc->nr_gpio_sets || set_idx < 0) {
+ error_setg(errp, "%s: invalid set_idx %s", __func__, name);
+ return;
+ }
+
+ aspeed_gpio_update(s, &s->sets[set_idx], set_val,
+ ~s->sets[set_idx].direction);
+}
+
+static void aspeed_gpio_get_set(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ uint32_t set_val = 0;
+ AspeedGPIOState *s = ASPEED_GPIO(obj);
+ AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+ int set_idx = 0;
+
+ if (sscanf(name, "gpio-set[%d]", &set_idx) != 1) {
+ error_setg(errp, "%s: error reading %s", __func__, name);
+ return;
+ }
+
+ if (set_idx >= agc->nr_gpio_sets || set_idx < 0) {
+ error_setg(errp, "%s: invalid set_idx %s", __func__, name);
+ return;
+ }
+
+ set_val = s->sets[set_idx].data_value;
+ visit_type_uint32(v, name, &set_val, errp);
+}
+
+/****************** Setup functions ******************/
static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
@@ -1435,6 +1486,12 @@ static void aspeed_gpio_init(Object *obj)
g_free(name);
}
}
+
+ for (int i = 0; i < agc->nr_gpio_sets; i++) {
+ char *name = g_strdup_printf("gpio-set[%d]", i);
+ object_property_add(obj, name, "uint32", aspeed_gpio_get_set,
+ aspeed_gpio_set_set, NULL, NULL);
+ }
}
static const VMStateDescription vmstate_gpio_regs = {
--
2.51.0.536.g15c5d4f767-googOn Thu, 2025-09-25 at 00:58 +0000, Coco Li wrote:
> From: Felix Wu <flwu@google.com>
>
> Added 32 bits property for ASPEED GPIO. Previously it can only be access in bitwise manner.
>
> This change gives ASPEED similar behavior as Nuvoton.
I don't think this has adequately addressed my request on the prior
series:
https://lore.kernel.org/all/e244fdb5d2d889674a583df8f8b9bc4bf8d476f4.camel@codeconstruct.com.au/
Can you please improve the commit message?
I don't have any particular concern with the implementation, other than
understanding whether it's something that's reasonable to add to begin
with. The "sets" and their indexes are somewhat an implementation
detail. Exposing them might preclude a different implementation design,
though I'm also not sure why we'd change at this point.
Andrew
>
> Signed-off-by: Felix Wu <flwu@google.com>
> ---
> hw/gpio/aspeed_gpio.c | 57 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index 609a556908..2d78bf9515 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -1308,6 +1308,57 @@ static void aspeed_gpio_2700_write(void *opaque, hwaddr offset,
> }
>
> /* Setup functions */
> +static void aspeed_gpio_set_set(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + uint32_t set_val = 0;
> + AspeedGPIOState *s = ASPEED_GPIO(obj);
> + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> + int set_idx = 0;
> +
> + if (!visit_type_uint32(v, name, &set_val, errp)) {
> + return;
> + }
> +
> + if (sscanf(name, "gpio-set[%d]", &set_idx) != 1) {
> + error_setg(errp, "%s: error reading %s", __func__, name);
> + return;
> + }
> +
> + if (set_idx >= agc->nr_gpio_sets || set_idx < 0) {
> + error_setg(errp, "%s: invalid set_idx %s", __func__, name);
> + return;
> + }
> +
> + aspeed_gpio_update(s, &s->sets[set_idx], set_val,
> + ~s->sets[set_idx].direction);
> +}
> +
> +static void aspeed_gpio_get_set(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + uint32_t set_val = 0;
> + AspeedGPIOState *s = ASPEED_GPIO(obj);
> + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> + int set_idx = 0;
> +
> + if (sscanf(name, "gpio-set[%d]", &set_idx) != 1) {
> + error_setg(errp, "%s: error reading %s", __func__, name);
> + return;
> + }
> +
> + if (set_idx >= agc->nr_gpio_sets || set_idx < 0) {
> + error_setg(errp, "%s: invalid set_idx %s", __func__, name);
> + return;
> + }
> +
> + set_val = s->sets[set_idx].data_value;
> + visit_type_uint32(v, name, &set_val, errp);
> +}
> +
> +/****************** Setup functions ******************/
> static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
> [0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
> [1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
> @@ -1435,6 +1486,12 @@ static void aspeed_gpio_init(Object *obj)
> g_free(name);
> }
> }
> +
> + for (int i = 0; i < agc->nr_gpio_sets; i++) {
> + char *name = g_strdup_printf("gpio-set[%d]", i);
> + object_property_add(obj, name, "uint32", aspeed_gpio_get_set,
> + aspeed_gpio_set_set, NULL, NULL);
> + }
> }
>
> static const VMStateDescription vmstate_gpio_regs = {
On Wed, Oct 1, 2025 at 4:24 PM Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> On Thu, 2025-09-25 at 00:58 +0000, Coco Li wrote:
> > From: Felix Wu <flwu@google.com>
> >
> > Added 32 bits property for ASPEED GPIO. Previously it can only be access in bitwise manner.
> >
> > This change gives ASPEED similar behavior as Nuvoton.
>
> I don't think this has adequately addressed my request on the prior
> series:
>
> https://lore.kernel.org/all/e244fdb5d2d889674a583df8f8b9bc4bf8d476f4.camel@codeconstruct.com.au/
>
> Can you please improve the commit message?
>
> I don't have any particular concern with the implementation, other than
> understanding whether it's something that's reasonable to add to begin
> with. The "sets" and their indexes are somewhat an implementation
> detail. Exposing them might preclude a different implementation design,
> though I'm also not sure why we'd change at this point.
>
> Andrew
>
Hello Andrew,
To confirm that I understand your request, I should do the following:
1) remove the reference to Nuvoton behavior in the ASPEED patches
(will do in follow up)
2) you asked for discussion of complex simulations, is the addition in
the cover patch sufficient? Otherwise, could you elaborate on your
comment here on what I can help provide please?
Best,
Coco
> >
> > Signed-off-by: Felix Wu <flwu@google.com>
> > ---
> > hw/gpio/aspeed_gpio.c | 57 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> > index 609a556908..2d78bf9515 100644
> > --- a/hw/gpio/aspeed_gpio.c
> > +++ b/hw/gpio/aspeed_gpio.c
> > @@ -1308,6 +1308,57 @@ static void aspeed_gpio_2700_write(void *opaque, hwaddr offset,
> > }
> >
> > /* Setup functions */
> > +static void aspeed_gpio_set_set(Object *obj, Visitor *v,
> > + const char *name, void *opaque,
> > + Error **errp)
> > +{
> > + uint32_t set_val = 0;
> > + AspeedGPIOState *s = ASPEED_GPIO(obj);
> > + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> > + int set_idx = 0;
> > +
> > + if (!visit_type_uint32(v, name, &set_val, errp)) {
> > + return;
> > + }
> > +
> > + if (sscanf(name, "gpio-set[%d]", &set_idx) != 1) {
> > + error_setg(errp, "%s: error reading %s", __func__, name);
> > + return;
> > + }
> > +
> > + if (set_idx >= agc->nr_gpio_sets || set_idx < 0) {
> > + error_setg(errp, "%s: invalid set_idx %s", __func__, name);
> > + return;
> > + }
> > +
> > + aspeed_gpio_update(s, &s->sets[set_idx], set_val,
> > + ~s->sets[set_idx].direction);
> > +}
> > +
> > +static void aspeed_gpio_get_set(Object *obj, Visitor *v,
> > + const char *name, void *opaque,
> > + Error **errp)
> > +{
> > + uint32_t set_val = 0;
> > + AspeedGPIOState *s = ASPEED_GPIO(obj);
> > + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> > + int set_idx = 0;
> > +
> > + if (sscanf(name, "gpio-set[%d]", &set_idx) != 1) {
> > + error_setg(errp, "%s: error reading %s", __func__, name);
> > + return;
> > + }
> > +
> > + if (set_idx >= agc->nr_gpio_sets || set_idx < 0) {
> > + error_setg(errp, "%s: invalid set_idx %s", __func__, name);
> > + return;
> > + }
> > +
> > + set_val = s->sets[set_idx].data_value;
> > + visit_type_uint32(v, name, &set_val, errp);
> > +}
> > +
> > +/****************** Setup functions ******************/
> > static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
> > [0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
> > [1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
> > @@ -1435,6 +1486,12 @@ static void aspeed_gpio_init(Object *obj)
> > g_free(name);
> > }
> > }
> > +
> > + for (int i = 0; i < agc->nr_gpio_sets; i++) {
> > + char *name = g_strdup_printf("gpio-set[%d]", i);
> > + object_property_add(obj, name, "uint32", aspeed_gpio_get_set,
> > + aspeed_gpio_set_set, NULL, NULL);
> > + }
> > }
> >
> > static const VMStateDescription vmstate_gpio_regs = {
On Fri, 2025-10-03 at 10:44 -0700, Coco Li wrote: > On Wed, Oct 1, 2025 at 4:24 PM Andrew Jeffery > <andrew@codeconstruct.com.au> wrote: > > > > On Thu, 2025-09-25 at 00:58 +0000, Coco Li wrote: > > > From: Felix Wu <flwu@google.com> > > > > > > Added 32 bits property for ASPEED GPIO. Previously it can only be access in bitwise manner. > > > > > > This change gives ASPEED similar behavior as Nuvoton. > > > > I don't think this has adequately addressed my request on the prior > > series: > > > > https://lore.kernel.org/all/e244fdb5d2d889674a583df8f8b9bc4bf8d476f4.camel@codeconstruct.com.au/ > > > > Can you please improve the commit message? > > > > I don't have any particular concern with the implementation, other than > > understanding whether it's something that's reasonable to add to begin > > with. The "sets" and their indexes are somewhat an implementation > > detail. Exposing them might preclude a different implementation design, > > though I'm also not sure why we'd change at this point. > > > > Andrew > > > > Hello Andrew, > > To confirm that I understand your request, I should do the following: > > 1) remove the reference to Nuvoton behavior in the ASPEED patches > (will do in follow up) > 2) you asked for discussion of complex simulations, is the addition in > the cover patch sufficient? Otherwise, could you elaborate on your > comment here on what I can help provide please? Can you please integrate the description from the cover letter into the commit message? It's not always the case that the series cover letter is tracked in the git history. Andrew
© 2016 - 2026 Red Hat, Inc.