| Message ID | 20230508075859.3326566-10-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: fixes and extensions | expand |
On 8/5/23 09:58, Cédric Le Goater wrote: > It will help in getting rid of some drive_get(IF_MTD) calls by > retrieving the BlockBackend directly from the m25p80 device. > > Cc: Alistair Francis <alistair@alistair23.me> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/block/flash.h | 4 ++++ > hw/block/m25p80.c | 6 ++++++ > 2 files changed, 10 insertions(+) > > diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h > index 7198953702..de93756cbe 100644 > --- a/include/hw/block/flash.h > +++ b/include/hw/block/flash.h > @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); > void ecc_reset(ECCState *s); > extern const VMStateDescription vmstate_ecc_state; > > +/* m25p80.c */ > + > +BlockBackend *m25p80_get_blk(DeviceState *dev); - Option 1, declare QOM typedef and use proper type: #define TYPE_M25P80 "m25p80-generic" OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80) BlockBackend *m25p80_get_blk(Flash *dev); - Option 2, preliminary patch renaming 'Flash' type to 'M25P80' then option 1 again - Option 3: no change. With the QOM style we try to enforce, I'd go for #2. Still, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > #endif > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index dc5ffbc4ff..afc3fdf4d6 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -25,6 +25,7 @@ > #include "qemu/units.h" > #include "sysemu/block-backend.h" > #include "hw/block/block.h" > +#include "hw/block/flash.h" > #include "hw/qdev-properties.h" > #include "hw/qdev-properties-system.h" > #include "hw/ssi/ssi.h" > @@ -1830,3 +1831,8 @@ static void m25p80_register_types(void) > } > > type_init(m25p80_register_types) > + > +BlockBackend *m25p80_get_blk(DeviceState *dev) > +{ > + return M25P80(dev)->blk; > +}
On 5/30/23 23:14, Philippe Mathieu-Daudé wrote: > On 8/5/23 09:58, Cédric Le Goater wrote: >> It will help in getting rid of some drive_get(IF_MTD) calls by >> retrieving the BlockBackend directly from the m25p80 device. >> >> Cc: Alistair Francis <alistair@alistair23.me> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/block/flash.h | 4 ++++ >> hw/block/m25p80.c | 6 ++++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >> index 7198953702..de93756cbe 100644 >> --- a/include/hw/block/flash.h >> +++ b/include/hw/block/flash.h >> @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >> void ecc_reset(ECCState *s); >> extern const VMStateDescription vmstate_ecc_state; >> +/* m25p80.c */ >> + >> +BlockBackend *m25p80_get_blk(DeviceState *dev); > > - Option 1, declare QOM typedef and use proper type: > > #define TYPE_M25P80 "m25p80-generic" > OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80) > > BlockBackend *m25p80_get_blk(Flash *dev); > > - Option 2, preliminary patch renaming 'Flash' type to > 'M25P80' then option 1 again That's a large change and we would need to introduce a m25p80.h with these definitions. Given that the caller needs a DeviceState in the end, may be not worth the extra hassle. How would you rename 'Flash' ? 'SpiFlash' ? Thanks, C. > > - Option 3: no change. > > With the QOM style we try to enforce, I'd go for #2. > > Still, > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >> #endif >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index dc5ffbc4ff..afc3fdf4d6 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -25,6 +25,7 @@ >> #include "qemu/units.h" >> #include "sysemu/block-backend.h" >> #include "hw/block/block.h" >> +#include "hw/block/flash.h" >> #include "hw/qdev-properties.h" >> #include "hw/qdev-properties-system.h" >> #include "hw/ssi/ssi.h" >> @@ -1830,3 +1831,8 @@ static void m25p80_register_types(void) >> } >> type_init(m25p80_register_types) >> + >> +BlockBackend *m25p80_get_blk(DeviceState *dev) >> +{ >> + return M25P80(dev)->blk; >> +} >
On 31/5/23 08:48, Cédric Le Goater wrote: > On 5/30/23 23:14, Philippe Mathieu-Daudé wrote: >> On 8/5/23 09:58, Cédric Le Goater wrote: >>> It will help in getting rid of some drive_get(IF_MTD) calls by >>> retrieving the BlockBackend directly from the m25p80 device. >>> >>> Cc: Alistair Francis <alistair@alistair23.me> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> include/hw/block/flash.h | 4 ++++ >>> hw/block/m25p80.c | 6 ++++++ >>> 2 files changed, 10 insertions(+) >>> >>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >>> index 7198953702..de93756cbe 100644 >>> --- a/include/hw/block/flash.h >>> +++ b/include/hw/block/flash.h >>> @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >>> void ecc_reset(ECCState *s); >>> extern const VMStateDescription vmstate_ecc_state; >>> +/* m25p80.c */ >>> + >>> +BlockBackend *m25p80_get_blk(DeviceState *dev); >> >> - Option 1, declare QOM typedef and use proper type: >> >> #define TYPE_M25P80 "m25p80-generic" >> OBJECT_DECLARE_TYPE(Flash, M25P80Class, M25P80) >> >> BlockBackend *m25p80_get_blk(Flash *dev); >> >> - Option 2, preliminary patch renaming 'Flash' type to >> 'M25P80' then option 1 again > > That's a large change Yes, it can be done later if you rather. > and we would need to introduce a m25p80.h with > these definitions. FlashPartInfo is used as pointer so can be forward-declared. Then we only need to move M25P80_INTERNAL_DATA_BUFFER_SZ. For 5 lines, "hw/block/flash.h" is good enough IMO, keeping all the rest to m25p80.c. > Given that the caller needs a DeviceState in the > end, may be not worth the extra hassle. > > How would you rename 'Flash' ? 'SpiFlash' ? Since you ask, my preference is SpiNorFlash :) But again, this can be done later on top. Thanks, Phil.
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h index 7198953702..de93756cbe 100644 --- a/include/hw/block/flash.h +++ b/include/hw/block/flash.h @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); void ecc_reset(ECCState *s); extern const VMStateDescription vmstate_ecc_state; +/* m25p80.c */ + +BlockBackend *m25p80_get_blk(DeviceState *dev); + #endif diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index dc5ffbc4ff..afc3fdf4d6 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -25,6 +25,7 @@ #include "qemu/units.h" #include "sysemu/block-backend.h" #include "hw/block/block.h" +#include "hw/block/flash.h" #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" #include "hw/ssi/ssi.h" @@ -1830,3 +1831,8 @@ static void m25p80_register_types(void) } type_init(m25p80_register_types) + +BlockBackend *m25p80_get_blk(DeviceState *dev) +{ + return M25P80(dev)->blk; +}
It will help in getting rid of some drive_get(IF_MTD) calls by retrieving the BlockBackend directly from the m25p80 device. Cc: Alistair Francis <alistair@alistair23.me> Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/block/flash.h | 4 ++++ hw/block/m25p80.c | 6 ++++++ 2 files changed, 10 insertions(+)