| Message ID | 20200110203942.5745-7-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | Fix Exynos4210 DMA support | expand |
On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote: > > The Exynos4210 serial driver uses an interrupt line to signal if receive > data is available. Connect that interrupt with the DMA controller's > 'peripheral busy' gpio pin. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > hw/arm/exynos4210.c | 39 ++++++++++++++++++++++++++------------- > 1 file changed, 26 insertions(+), 13 deletions(-) > > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c > index c7b5c587b1..498d79ebb2 100644 > --- a/hw/arm/exynos4210.c > +++ b/hw/arm/exynos4210.c > @@ -166,8 +166,8 @@ static uint64_t exynos4210_calc_affinity(int cpu) > return (0x9 << ARM_AFF1_SHIFT) | cpu; > } > > -static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents, > - int width) > +static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq, > + int nevents, int width) > { > SysBusDevice *busdev; > DeviceState *dev; > @@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents, > for (i = 0; i < nevents; i++) { > sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */ > } > + return dev; > } > > static void exynos4210_realize(DeviceState *socdev, Error **errp) > @@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp) > MemoryRegion *system_mem = get_system_memory(); > qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS]; > SysBusDevice *busdev; > - DeviceState *dev; > + DeviceState *dev, *uart[4], *pl330[3]; Rather than having the uart and pl330 pointers be locals, they should be fields in Exynos4210State. (Otherwise technically we leak them, though this is unnoticeable in practice because there's no way to destroy an Exynos4210State.) Patch looks good otherwise. thanks -- PMM
On Fri, Jan 17, 2020 at 01:48:06PM +0000, Peter Maydell wrote: > On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote: > > > > The Exynos4210 serial driver uses an interrupt line to signal if receive > > data is available. Connect that interrupt with the DMA controller's > > 'peripheral busy' gpio pin. > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > hw/arm/exynos4210.c | 39 ++++++++++++++++++++++++++------------- > > 1 file changed, 26 insertions(+), 13 deletions(-) > > > > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c > > index c7b5c587b1..498d79ebb2 100644 > > --- a/hw/arm/exynos4210.c > > +++ b/hw/arm/exynos4210.c > > @@ -166,8 +166,8 @@ static uint64_t exynos4210_calc_affinity(int cpu) > > return (0x9 << ARM_AFF1_SHIFT) | cpu; > > } > > > > -static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents, > > - int width) > > +static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq, > > + int nevents, int width) > > { > > SysBusDevice *busdev; > > DeviceState *dev; > > @@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents, > > for (i = 0; i < nevents; i++) { > > sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */ > > } > > + return dev; > > } > > > > static void exynos4210_realize(DeviceState *socdev, Error **errp) > > @@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp) > > MemoryRegion *system_mem = get_system_memory(); > > qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS]; > > SysBusDevice *busdev; > > - DeviceState *dev; > > + DeviceState *dev, *uart[4], *pl330[3]; > > Rather than having the uart and pl330 pointers be locals, > they should be fields in Exynos4210State. (Otherwise technically > we leak them, though this is unnoticeable in practice because there's > no way to destroy an Exynos4210State.) > Out of curiosity: Is that a new leak because they are now tied together, or is it an existing leak ? I don't find many DeviceState entries in xxxState structures, so find it difficult to determine if/when/why there is such a leak. Thanks, Guenter
On Fri, 17 Jan 2020 at 18:29, Guenter Roeck <linux@roeck-us.net> wrote: > > On Fri, Jan 17, 2020 at 01:48:06PM +0000, Peter Maydell wrote: > > On Fri, 10 Jan 2020 at 20:39, Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > The Exynos4210 serial driver uses an interrupt line to signal if receive > > > data is available. Connect that interrupt with the DMA controller's > > > 'peripheral busy' gpio pin. > > Rather than having the uart and pl330 pointers be locals, > > they should be fields in Exynos4210State. (Otherwise technically > > we leak them, though this is unnoticeable in practice because there's > > no way to destroy an Exynos4210State.) > > > Out of curiosity: Is that a new leak because they are now tied together, > or is it an existing leak ? I don't find many DeviceState entries > in xxxState structures, so find it difficult to determine if/when/why > there is such a leak. Yes, it's an existing leak, though it's more of a conceptual leak than one that valgrind would actually complain about. (The object isn't totally unreachable because there will be references to it in the QOM tree. Keeping track of your child objects only becomes really important if the device is hot-pluggable, because if you can hot-unplug the device you get a real leak if it hasn't cleaned up its child objects.) Aside: I think this code is written this way because although it's a container device it has been incorrectly written against the pattern of a board model. Originally board models did not have any "this is the board" struct that they could keep things in, so the only way to write board model code that created, configured and wired up devices was to have it do it in a way that didn't keep track of the things it created once the board init function returned. This code is part of an SoC "container" device, so it does have a state struct that it could use to hold either embedded device structs or simply pointers to device objects, but the code is written like the old board-model code. (These days even board models have a suitable state struct they can use.) include/hw/arm/armsse.h is an example of a device state struct with a lot of embedded device state structs in it. (Not all device state structs have names ending "State".) thanks -- PMM
On 1/17/20 10:44 AM, Peter Maydell wrote: > On Fri, 17 Jan 2020 at 18:29, Guenter Roeck <linux@roeck-us.net> wrote: [ ... ] >>> Rather than having the uart and pl330 pointers be locals, >>> they should be fields in Exynos4210State. (Otherwise technically >>> we leak them, though this is unnoticeable in practice because there's >>> no way to destroy an Exynos4210State.) >>> >> Out of curiosity: Is that a new leak because they are now tied together, >> or is it an existing leak ? I don't find many DeviceState entries >> in xxxState structures, so find it difficult to determine if/when/why >> there is such a leak. > > Yes, it's an existing leak, though it's more of a conceptual leak Do only the pointers have to be in Exynos4210State, or the entire data structures ? In the armsse code it looks like it is the complete data structures. Also, it seems to me that this means that not only pl330 and uart states are affected, but everything created with qdev_create(). If so, the entire file needs a serious rework, not just its pl330 / uart initialization. Am I missing something ? Thanks, Guenter
On Sat, 18 Jan 2020 at 15:08, Guenter Roeck <linux@roeck-us.net> wrote: > Do only the pointers have to be in Exynos4210State, or the entire > data structures ? In the armsse code it looks like it is the complete > data structures. Either works. Embedding the entire data structure is the more "modern" approach, but we don't generally go to the effort of converting from the older style to the newer. > Also, it seems to me that this means that not only pl330 and uart states > are affected, but everything created with qdev_create(). If so, the entire > file needs a serious rework, not just its pl330 / uart initialization. > Am I missing something ? Yeah, all that stuff is broken, but don't feel you need to fix it. You just brought the pl330 pointers to my attention specifically by declaring locals in this patch, at which point it's just as easy to put those pointers in the state struct where they should be. thanks -- PMM
On 1/18/20 12:02 PM, Peter Maydell wrote: > On Sat, 18 Jan 2020 at 15:08, Guenter Roeck <linux@roeck-us.net> wrote: >> Do only the pointers have to be in Exynos4210State, or the entire >> data structures ? In the armsse code it looks like it is the complete >> data structures. > > Either works. Embedding the entire data structure is the more > "modern" approach, but we don't generally go to the effort of > converting from the older style to the newer. > >> Also, it seems to me that this means that not only pl330 and uart states >> are affected, but everything created with qdev_create(). If so, the entire >> file needs a serious rework, not just its pl330 / uart initialization. >> Am I missing something ? > > Yeah, all that stuff is broken, but don't feel you need to fix it. > You just brought the pl330 pointers to my attention specifically > by declaring locals in this patch, at which point it's just > as easy to put those pointers in the state struct where they > should be. > I'd rather try to fix it all if I am at it; otherwise it feels kind of incomplete. Would you be ok with addressing this separately after the current patch series is accepted ? Thanks, Guenter
On Sun, 19 Jan 2020 at 01:52, Guenter Roeck <linux@roeck-us.net> wrote: > I'd rather try to fix it all if I am at it; otherwise it feels kind > of incomplete. Would you be ok with addressing this separately after > the current patch series is accepted ? Absolutely, if you'd like to clean up the code please feel free. I agree that a separate patchset is probably the best way to go. (Do you mean by that that you'd like me to take your v2 as-is?) thanks -- PMM
On 1/19/20 11:01 AM, Peter Maydell wrote: > On Sun, 19 Jan 2020 at 01:52, Guenter Roeck <linux@roeck-us.net> wrote: >> I'd rather try to fix it all if I am at it; otherwise it feels kind >> of incomplete. Would you be ok with addressing this separately after >> the current patch series is accepted ? > > Absolutely, if you'd like to clean up the code please feel free. > I agree that a separate patchset is probably the best way to go. > (Do you mean by that that you'd like me to take your v2 as-is?) > Sure, if you accept it as-is. Thanks, Guenter
diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c index c7b5c587b1..498d79ebb2 100644 --- a/hw/arm/exynos4210.c +++ b/hw/arm/exynos4210.c @@ -166,8 +166,8 @@ static uint64_t exynos4210_calc_affinity(int cpu) return (0x9 << ARM_AFF1_SHIFT) | cpu; } -static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents, - int width) +static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq, + int nevents, int width) { SysBusDevice *busdev; DeviceState *dev; @@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int nevents, for (i = 0; i < nevents; i++) { sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */ } + return dev; } static void exynos4210_realize(DeviceState *socdev, Error **errp) @@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp) MemoryRegion *system_mem = get_system_memory(); qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS]; SysBusDevice *busdev; - DeviceState *dev; + DeviceState *dev, *uart[4], *pl330[3]; int i, n; for (n = 0; n < EXYNOS4210_NCPUS; n++) { @@ -385,19 +386,19 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp) /*** UARTs ***/ - exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR, + uart[0] = exynos4210_uart_create(EXYNOS4210_UART0_BASE_ADDR, EXYNOS4210_UART0_FIFO_SIZE, 0, serial_hd(0), s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 0)]); - exynos4210_uart_create(EXYNOS4210_UART1_BASE_ADDR, + uart[1] = exynos4210_uart_create(EXYNOS4210_UART1_BASE_ADDR, EXYNOS4210_UART1_FIFO_SIZE, 1, serial_hd(1), s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 1)]); - exynos4210_uart_create(EXYNOS4210_UART2_BASE_ADDR, + uart[2] = exynos4210_uart_create(EXYNOS4210_UART2_BASE_ADDR, EXYNOS4210_UART2_FIFO_SIZE, 2, serial_hd(2), s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 2)]); - exynos4210_uart_create(EXYNOS4210_UART3_BASE_ADDR, + uart[3] = exynos4210_uart_create(EXYNOS4210_UART3_BASE_ADDR, EXYNOS4210_UART3_FIFO_SIZE, 3, serial_hd(3), s->irq_table[exynos4210_get_irq(EXYNOS4210_UART_INT_GRP, 3)]); @@ -445,12 +446,24 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp) s->irq_table[exynos4210_get_irq(28, 3)]); /*** DMA controllers ***/ - pl330_create(EXYNOS4210_PL330_BASE0_ADDR, - s->irq_table[exynos4210_get_irq(21, 0)], 32, 32, 32); - pl330_create(EXYNOS4210_PL330_BASE1_ADDR, - s->irq_table[exynos4210_get_irq(21, 1)], 32, 32, 32); - pl330_create(EXYNOS4210_PL330_BASE2_ADDR, - s->irq_table[exynos4210_get_irq(20, 1)], 1, 31, 64); + pl330[0] = pl330_create(EXYNOS4210_PL330_BASE0_ADDR, + s->irq_table[exynos4210_get_irq(21, 0)], + 32, 32, 32); + pl330[1] = pl330_create(EXYNOS4210_PL330_BASE1_ADDR, + s->irq_table[exynos4210_get_irq(21, 1)], + 32, 32, 32); + pl330[2] = pl330_create(EXYNOS4210_PL330_BASE2_ADDR, + s->irq_table[exynos4210_get_irq(20, 1)], + 1, 31, 64); + + sysbus_connect_irq(SYS_BUS_DEVICE(uart[0]), 1, + qdev_get_gpio_in(pl330[0], 15)); + sysbus_connect_irq(SYS_BUS_DEVICE(uart[1]), 1, + qdev_get_gpio_in(pl330[1], 15)); + sysbus_connect_irq(SYS_BUS_DEVICE(uart[2]), 1, + qdev_get_gpio_in(pl330[0], 17)); + sysbus_connect_irq(SYS_BUS_DEVICE(uart[3]), 1, + qdev_get_gpio_in(pl330[1], 17)); } static void exynos4210_class_init(ObjectClass *klass, void *data)
The Exynos4210 serial driver uses an interrupt line to signal if receive data is available. Connect that interrupt with the DMA controller's 'peripheral busy' gpio pin. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- hw/arm/exynos4210.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)