| Message ID | 20230508075859.3326566-6-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aspeed: fixes and extensions | expand |
On 8/5/23 09:58, Cédric Le Goater wrote: > Simple routine to retrieve a DeviceState object on a SPI bus using its > address/cs. It will be useful for the board to wire the CS lines. > > Cc: Alistair Francis <alistair@alistair23.me> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ssi/ssi.h | 2 ++ > hw/ssi/ssi.c | 15 +++++++++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h > index ffd3a34ba4..c7beabdb09 100644 > --- a/include/hw/ssi/ssi.h > +++ b/include/hw/ssi/ssi.h > @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); > > uint32_t ssi_transfer(SSIBus *bus, uint32_t val); > > +DeviceState *ssi_get_cs(SSIBus *bus, int addr); Previous patch use uint32_t. uint8_t is probably enough, otherwise 'unsigned'? Otherwise Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: > On 8/5/23 09:58, Cédric Le Goater wrote: >> Simple routine to retrieve a DeviceState object on a SPI bus using its >> address/cs. It will be useful for the board to wire the CS lines. >> >> Cc: Alistair Francis <alistair@alistair23.me> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ssi/ssi.h | 2 ++ >> hw/ssi/ssi.c | 15 +++++++++++++++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >> index ffd3a34ba4..c7beabdb09 100644 >> --- a/include/hw/ssi/ssi.h >> +++ b/include/hw/ssi/ssi.h >> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const >> char *name); >> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); Also, this helper should (preferably) return a SSIPeripheral type. > Previous patch use uint32_t. uint8_t is probably enough, > otherwise 'unsigned'? Otherwise > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On 5/30/23 22:34, Philippe Mathieu-Daudé wrote: > On 8/5/23 09:58, Cédric Le Goater wrote: >> Simple routine to retrieve a DeviceState object on a SPI bus using its >> address/cs. It will be useful for the board to wire the CS lines. >> >> Cc: Alistair Francis <alistair@alistair23.me> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ssi/ssi.h | 2 ++ >> hw/ssi/ssi.c | 15 +++++++++++++++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >> index ffd3a34ba4..c7beabdb09 100644 >> --- a/include/hw/ssi/ssi.h >> +++ b/include/hw/ssi/ssi.h >> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); >> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); > > Previous patch use uint32_t. uint8_t is probably enough, > otherwise 'unsigned'? Otherwise uint8_t is fine. Thanks, C. > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On 5/30/23 23:15, Philippe Mathieu-Daudé wrote: > On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: >> On 8/5/23 09:58, Cédric Le Goater wrote: >>> Simple routine to retrieve a DeviceState object on a SPI bus using its >>> address/cs. It will be useful for the board to wire the CS lines. >>> >>> Cc: Alistair Francis <alistair@alistair23.me> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> include/hw/ssi/ssi.h | 2 ++ >>> hw/ssi/ssi.c | 15 +++++++++++++++ >>> 2 files changed, 17 insertions(+) >>> >>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >>> index ffd3a34ba4..c7beabdb09 100644 >>> --- a/include/hw/ssi/ssi.h >>> +++ b/include/hw/ssi/ssi.h >>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); >>> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); > > Also, this helper should (preferably) return a SSIPeripheral type. Well, having a DeviceState is handy for the callers (today) and ssi_create_peripheral() returns a DeviceState. Let's keep it that way. Thanks, C. > >> Previous patch use uint32_t. uint8_t is probably enough, >> otherwise 'unsigned'? Otherwise >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >
+QOM tinkerers On 31/5/23 07:59, Cédric Le Goater wrote: > On 5/30/23 23:15, Philippe Mathieu-Daudé wrote: >> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: >>> On 8/5/23 09:58, Cédric Le Goater wrote: >>>> Simple routine to retrieve a DeviceState object on a SPI bus using its >>>> address/cs. It will be useful for the board to wire the CS lines. >>>> >>>> Cc: Alistair Francis <alistair@alistair23.me> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> include/hw/ssi/ssi.h | 2 ++ >>>> hw/ssi/ssi.c | 15 +++++++++++++++ >>>> 2 files changed, 17 insertions(+) >>>> >>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >>>> index ffd3a34ba4..c7beabdb09 100644 >>>> --- a/include/hw/ssi/ssi.h >>>> +++ b/include/hw/ssi/ssi.h >>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, >>>> const char *name); >>>> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); >> >> Also, this helper should (preferably) return a SSIPeripheral type. > > Well, having a DeviceState is handy for the callers (today) and > ssi_create_peripheral() returns a DeviceState. Let's keep it that > way. Yes I know it is handy :) I'm not against your patch; besides other APIs do that. I'm wondering about QOM design here. Having Foo device, should FOO API return the common qdev abstract type (DeviceState) or a Foo type? Either ways we keep QOM-casting around, but I still tend to consider FOO API returning Foo pointer provides some type check safety, and also provides the API user hints about what is used. Need more coffee.
On 5/31/23 08:17, Philippe Mathieu-Daudé wrote: > +QOM tinkerers > > On 31/5/23 07:59, Cédric Le Goater wrote: >> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote: >>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: >>>> On 8/5/23 09:58, Cédric Le Goater wrote: >>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its >>>>> address/cs. It will be useful for the board to wire the CS lines. >>>>> >>>>> Cc: Alistair Francis <alistair@alistair23.me> >>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>> --- >>>>> include/hw/ssi/ssi.h | 2 ++ >>>>> hw/ssi/ssi.c | 15 +++++++++++++++ >>>>> 2 files changed, 17 insertions(+) >>>>> >>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >>>>> index ffd3a34ba4..c7beabdb09 100644 >>>>> --- a/include/hw/ssi/ssi.h >>>>> +++ b/include/hw/ssi/ssi.h >>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); >>>>> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); >>> >>> Also, this helper should (preferably) return a SSIPeripheral type. >> >> Well, having a DeviceState is handy for the callers (today) and >> ssi_create_peripheral() returns a DeviceState. Let's keep it that >> way. > > Yes I know it is handy :) I'm not against your patch; besides other > APIs do that. I'm wondering about QOM design here. Having Foo device, > should FOO API return the common qdev abstract type (DeviceState) or > a Foo type? Either ways we keep QOM-casting around, but I still tend > to consider FOO API returning Foo pointer provides some type check > safety, and also provides the API user hints about what is used. > Need more coffee. It is used in two code paths today: ... DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0); BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL; ... and ... DeviceState *dev = ssi_get_cs(s->spi, i); if (dev) { qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); ... Changing the interface is not a radical change, it will add two QOM-casts. I can give it a try in v2. Thanks, C.
On 31/5/23 08:36, Cédric Le Goater wrote: > On 5/31/23 08:17, Philippe Mathieu-Daudé wrote: >> +QOM tinkerers >> >> On 31/5/23 07:59, Cédric Le Goater wrote: >>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote: >>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: >>>>> On 8/5/23 09:58, Cédric Le Goater wrote: >>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using >>>>>> its >>>>>> address/cs. It will be useful for the board to wire the CS lines. >>>>>> >>>>>> Cc: Alistair Francis <alistair@alistair23.me> >>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>>> --- >>>>>> include/hw/ssi/ssi.h | 2 ++ >>>>>> hw/ssi/ssi.c | 15 +++++++++++++++ >>>>>> 2 files changed, 17 insertions(+) >>>>>> >>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >>>>>> index ffd3a34ba4..c7beabdb09 100644 >>>>>> --- a/include/hw/ssi/ssi.h >>>>>> +++ b/include/hw/ssi/ssi.h >>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, >>>>>> const char *name); >>>>>> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); >>>> >>>> Also, this helper should (preferably) return a SSIPeripheral type. >>> >>> Well, having a DeviceState is handy for the callers (today) and >>> ssi_create_peripheral() returns a DeviceState. Let's keep it that >>> way. >> >> Yes I know it is handy :) I'm not against your patch; besides other >> APIs do that. I'm wondering about QOM design here. Having Foo device, >> should FOO API return the common qdev abstract type (DeviceState) or >> a Foo type? Either ways we keep QOM-casting around, but I still tend >> to consider FOO API returning Foo pointer provides some type check >> safety, and also provides the API user hints about what is used. >> Need more coffee. > > It is used in two code paths today: > > ... > DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0); > BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL; > ... > and > ... > DeviceState *dev = ssi_get_cs(s->spi, i); > if (dev) { > qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, > 0); > ... > > > Changing the interface is not a radical change, it will add two QOM-casts. > I can give it a try in v2. Hold on, lets see what others think first.
Am 31. Mai 2023 07:39:32 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 31/5/23 08:36, Cédric Le Goater wrote: >> On 5/31/23 08:17, Philippe Mathieu-Daudé wrote: >>> +QOM tinkerers >>> >>> On 31/5/23 07:59, Cédric Le Goater wrote: >>>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote: >>>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: >>>>>> On 8/5/23 09:58, Cédric Le Goater wrote: >>>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its >>>>>>> address/cs. It will be useful for the board to wire the CS lines. >>>>>>> >>>>>>> Cc: Alistair Francis <alistair@alistair23.me> >>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>>>> --- >>>>>>> include/hw/ssi/ssi.h | 2 ++ >>>>>>> hw/ssi/ssi.c | 15 +++++++++++++++ >>>>>>> 2 files changed, 17 insertions(+) >>>>>>> >>>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >>>>>>> index ffd3a34ba4..c7beabdb09 100644 >>>>>>> --- a/include/hw/ssi/ssi.h >>>>>>> +++ b/include/hw/ssi/ssi.h >>>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); >>>>>>> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >>>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); >>>>> >>>>> Also, this helper should (preferably) return a SSIPeripheral type. >>>> >>>> Well, having a DeviceState is handy for the callers (today) and >>>> ssi_create_peripheral() returns a DeviceState. Let's keep it that >>>> way. >>> >>> Yes I know it is handy :) I'm not against your patch; besides other >>> APIs do that. I'm wondering about QOM design here. Having Foo device, >>> should FOO API return the common qdev abstract type (DeviceState) or >>> a Foo type? Either ways we keep QOM-casting around, but I still tend >>> to consider FOO API returning Foo pointer provides some type check >>> safety, and also provides the API user hints about what is used. >>> Need more coffee. >> >> It is used in two code paths today: >> >> ... >> DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0); >> BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL; Looks like m25p80_get_blk() should take a more specific argument as well, like Phil suggested already. Wouldn't that avoid the QOM cast here? Best regards, Bernhard >> ... >> and >> ... >> DeviceState *dev = ssi_get_cs(s->spi, i); >> if (dev) { >> qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); >> ... >> >> >> Changing the interface is not a radical change, it will add two QOM-casts. >> I can give it a try in v2. > >Hold on, lets see what others think first.
On 6/5/23 07:57, Bernhard Beschow wrote: > > > Am 31. Mai 2023 07:39:32 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >> On 31/5/23 08:36, Cédric Le Goater wrote: >>> On 5/31/23 08:17, Philippe Mathieu-Daudé wrote: >>>> +QOM tinkerers >>>> >>>> On 31/5/23 07:59, Cédric Le Goater wrote: >>>>> On 5/30/23 23:15, Philippe Mathieu-Daudé wrote: >>>>>> On 30/5/23 22:34, Philippe Mathieu-Daudé wrote: >>>>>>> On 8/5/23 09:58, Cédric Le Goater wrote: >>>>>>>> Simple routine to retrieve a DeviceState object on a SPI bus using its >>>>>>>> address/cs. It will be useful for the board to wire the CS lines. >>>>>>>> >>>>>>>> Cc: Alistair Francis <alistair@alistair23.me> >>>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>>>>>> --- >>>>>>>> include/hw/ssi/ssi.h | 2 ++ >>>>>>>> hw/ssi/ssi.c | 15 +++++++++++++++ >>>>>>>> 2 files changed, 17 insertions(+) >>>>>>>> >>>>>>>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h >>>>>>>> index ffd3a34ba4..c7beabdb09 100644 >>>>>>>> --- a/include/hw/ssi/ssi.h >>>>>>>> +++ b/include/hw/ssi/ssi.h >>>>>>>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); >>>>>>>> uint32_t ssi_transfer(SSIBus *bus, uint32_t val); >>>>>>>> +DeviceState *ssi_get_cs(SSIBus *bus, int addr); >>>>>> >>>>>> Also, this helper should (preferably) return a SSIPeripheral type. >>>>> >>>>> Well, having a DeviceState is handy for the callers (today) and >>>>> ssi_create_peripheral() returns a DeviceState. Let's keep it that >>>>> way. >>>> >>>> Yes I know it is handy :) I'm not against your patch; besides other >>>> APIs do that. I'm wondering about QOM design here. Having Foo device, >>>> should FOO API return the common qdev abstract type (DeviceState) or >>>> a Foo type? Either ways we keep QOM-casting around, but I still tend >>>> to consider FOO API returning Foo pointer provides some type check >>>> safety, and also provides the API user hints about what is used. >>>> Need more coffee. >>> >>> It is used in two code paths today: >>> >>> ... >>> DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0); >>> BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL; > > Looks like m25p80_get_blk() should take a more specific argument as well, like Phil suggested already. Wouldn't that avoid the QOM cast here? Exposing the m25p80 internals could be painful. I'd rather keep the definitions where they are under m25p80.c. Thanks, C. > > Best regards, > Bernhard > >>> ... >>> and >>> ... >>> DeviceState *dev = ssi_get_cs(s->spi, i); >>> if (dev) { >>> qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0); >>> ... >>> >>> >>> Changing the interface is not a radical change, it will add two QOM-casts. >>> I can give it a try in v2. >> >> Hold on, lets see what others think first.
diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h index ffd3a34ba4..c7beabdb09 100644 --- a/include/hw/ssi/ssi.h +++ b/include/hw/ssi/ssi.h @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name); uint32_t ssi_transfer(SSIBus *bus, uint32_t val); +DeviceState *ssi_get_cs(SSIBus *bus, int addr); + #endif diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c index 9fffe4f27a..a25e064417 100644 --- a/hw/ssi/ssi.c +++ b/hw/ssi/ssi.c @@ -27,6 +27,21 @@ struct SSIBus { #define TYPE_SSI_BUS "SSI" OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS) +DeviceState *ssi_get_cs(SSIBus *bus, int addr) +{ + BusState *b = BUS(bus); + BusChild *kid; + + QTAILQ_FOREACH(kid, &b->children, sibling) { + SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child); + if (kid_ssi->addr == addr) { + return kid->child; + } + } + + return NULL; +} + static const TypeInfo ssi_bus_info = { .name = TYPE_SSI_BUS, .parent = TYPE_BUS,
Simple routine to retrieve a DeviceState object on a SPI bus using its address/cs. It will be useful for the board to wire the CS lines. Cc: Alistair Francis <alistair@alistair23.me> Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ssi/ssi.h | 2 ++ hw/ssi/ssi.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+)