Re: [PATCH v4 12/37] serial: start making SerialMM a sysbus device

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

On Fri, Nov 22, 2019 at 2:11 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 21 Nov 2019 at 18:51, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau
> > > <marcandre.lureau@gmail.com> wrote:
> > > >
> > > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > >
> > > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau
> > > > > <marcandre.lureau@redhat.com> wrote:
> > > > > >
> > > > > > Memory mapped serial device is in fact a sysbus device. The following
> > > > > > patches will make use of sysbus facilities for resource and
> > > > > > registration. In particular, "serial-mm: use sysbus facilities" will
> > > > > > move internal serial realization to serial_mm_realize callback to
> > > > > > follow qdev best practices.
> > > > >
> > > > > What goes wrong if you just put the realize of smm->serial in
> > > > > the right place to start with ?
> > > >
> > > > You mean squash the following patches?
> > >
> > > No, I meant just having this patch have
> > >
> > > static void serial_mm_realize(DeviceState *dev, Error **errp)
> > > {
> > >     SerialMM *smm = SERIAL_MM(dev);
> > >     SerialState *s = &smm->serial;
> > >
> > >     object_property_set_bool(OBJECT(dev), true, "realized", &err);
> > >     if (err) {
> > >         error_propagate(errp, err);
> > >         return;
> > >     }
> > > }
> > >
> > > and
> > >
> > > + dc->realize = serial_mm_realize;
> > >
> > > rather than manually doing the qdev_init_nofail()
> > > in serial_mm_init(). This seems to me like an integral
> > > part of the change to doing the init of the subdevice in the
> > > init method, so it would be better unless there's a reason
> > > why it breaks something. The rest of patch 15 (which is
> > > what currently makes the equivalent change to realize) is all
> > > about passing through the properties and exposing the
> > > sysbus MMIO/irq regions and should stay a separate patch.
> > >
> > > (setting the 'realized' property is better in a realize method
> > > than using qdev_init_nofail() because it means we can propagate
> > > any error outward rather than killing qemu.)
> >
> > Ok, but I implemented realize and moved serial init in "serial: make
> > SerialIO a sysbus device".
>
> That patch is for the TYPE_SERIAL_IO device, isn't it? It
> changes both serial_io_instance_init and serial_io_realize
> to do the instance init and realize of the TYPE_SERIAL device,
> so it doesn't have the same "only doing half a change" issue
> that this patch has for TYPE_SERIAL_MM.
>

Ok, I got it, thanks.




-- 
Marc-André Lureau