| Message ID | 37d5a3ce941bc89f6d73b8e7bf746952a0074da2.1252699478.git.armbru@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster <armbru@redhat.com> wrote: > xilinx.h defines a couple of static inline functions for creating > devices. While that's a fair technique for hot functions, device > initialization is about as cold as it gets. Define them in the device > source files instead, and keep only declarations in the header. If I understood the qdev plan correctly, this is going to wrong direction. These functions should reside near the instantiation, not in the device code. The current approach looks OK if there are going to be more users of the devices.
On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: > On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster <armbru@redhat.com> wrote: > > xilinx.h defines a couple of static inline functions for creating > > devices. While that's a fair technique for hot functions, device > > initialization is about as cold as it gets. Define them in the device > > source files instead, and keep only declarations in the header. > > If I understood the qdev plan correctly, this is going to wrong > direction. These functions should reside near the instantiation, not > in the device code. The current approach looks OK if there are going > to be more users of the devices. I agree that they shouldn't be in the device source. The reason they ended up in a header and not with the petalogix board was that in my tree there are multiple boards using these functions to easy instantiate devices. Cheers
On 09/12/09 12:10, Edgar E. Iglesias wrote: > On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: >> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com> wrote: >>> xilinx.h defines a couple of static inline functions for creating >>> devices. While that's a fair technique for hot functions, device >>> initialization is about as cold as it gets. Define them in the device >>> source files instead, and keep only declarations in the header. >> >> If I understood the qdev plan correctly, this is going to wrong >> direction. These functions should reside near the instantiation, not >> in the device code. The current approach looks OK if there are going >> to be more users of the devices. The functions should go away ;) Some day the information carried by those code snippeds should come from a machine description file, then we'll don't need them any more. > I agree that they shouldn't be in the device source. > The reason they ended up in a header and not with the petalogix board > was that in my tree there are multiple boards using these functions > to easy instantiate devices. They have to be somewhere. Having them in a header file is unclean. Having them in the board-specific code isn't practical when multiple boards share the code. I'd stick them to the device source code as well. Also note that this is common practice elsewhere in the tree. cheers Gerd
On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote: > On 09/12/09 12:10, Edgar E. Iglesias wrote: >> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: >>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com> >>> wrote: >>>> xilinx.h defines a couple of static inline functions for creating >>>> devices. While that's a fair technique for hot functions, device >>>> initialization is about as cold as it gets. Define them in the device >>>> source files instead, and keep only declarations in the header. >>> >>> If I understood the qdev plan correctly, this is going to wrong >>> direction. These functions should reside near the instantiation, not >>> in the device code. The current approach looks OK if there are going >>> to be more users of the devices. > > The functions should go away ;) > > Some day the information carried by those code snippeds should come from a > machine description file, then we'll don't need them any more. I agree. > >> I agree that they shouldn't be in the device source. >> The reason they ended up in a header and not with the petalogix board >> was that in my tree there are multiple boards using these functions >> to easy instantiate devices. > > They have to be somewhere. Having them in a header file is unclean. Having > them in the board-specific code isn't practical when multiple boards share > the code. I'd stick them to the device source code as well. Also note > that this is common practice elsewhere in the tree. I disagree. But if ppl feel very strongly about this, I can remove them and deal with the code duplication in my tree. Afterall, it's unlikely that upsteam qemu gets more xilinx boards before some kind of device tree driven board support is there. Cheers
On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote: >> On 09/12/09 12:10, Edgar E. Iglesias wrote: >>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: >>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com> >>>> wrote: >>>>> xilinx.h defines a couple of static inline functions for creating >>>>> devices. While that's a fair technique for hot functions, device >>>>> initialization is about as cold as it gets. Define them in the device >>>>> source files instead, and keep only declarations in the header. >>>> >>>> If I understood the qdev plan correctly, this is going to wrong >>>> direction. These functions should reside near the instantiation, not >>>> in the device code. The current approach looks OK if there are going >>>> to be more users of the devices. >> >> The functions should go away ;) >> >> Some day the information carried by those code snippeds should come from a >> machine description file, then we'll don't need them any more. > > I agree. > >> >>> I agree that they shouldn't be in the device source. >>> The reason they ended up in a header and not with the petalogix board >>> was that in my tree there are multiple boards using these functions >>> to easy instantiate devices. >> >> They have to be somewhere. Having them in a header file is unclean. Having >> them in the board-specific code isn't practical when multiple boards share >> the code. I'd stick them to the device source code as well. Also note >> that this is common practice elsewhere in the tree. > > I disagree. > > But if ppl feel very strongly about this, I can remove them and deal with > the code duplication in my tree. Afterall, it's unlikely that upsteam > qemu gets more xilinx boards before some kind of device tree driven > board support is there. I also disagree. Why would the functions in a header file be unclean? The common practice is wrong.
Blue Swirl <blauwirbel@gmail.com> writes: > On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias > <edgar.iglesias@gmail.com> wrote: >> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote: >>> On 09/12/09 12:10, Edgar E. Iglesias wrote: >>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: >>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com> >>>>> wrote: >>>>>> xilinx.h defines a couple of static inline functions for creating >>>>>> devices. While that's a fair technique for hot functions, device >>>>>> initialization is about as cold as it gets. Define them in the device >>>>>> source files instead, and keep only declarations in the header. >>>>> >>>>> If I understood the qdev plan correctly, this is going to wrong >>>>> direction. These functions should reside near the instantiation, not >>>>> in the device code. The current approach looks OK if there are going >>>>> to be more users of the devices. >>> >>> The functions should go away ;) >>> >>> Some day the information carried by those code snippeds should come from a >>> machine description file, then we'll don't need them any more. >> >> I agree. >> >>> >>>> I agree that they shouldn't be in the device source. >>>> The reason they ended up in a header and not with the petalogix board >>>> was that in my tree there are multiple boards using these functions >>>> to easy instantiate devices. >>> >>> They have to be somewhere. Having them in a header file is unclean. Having >>> them in the board-specific code isn't practical when multiple boards share >>> the code. I'd stick them to the device source code as well. Also note >>> that this is common practice elsewhere in the tree. >> >> I disagree. >> >> But if ppl feel very strongly about this, I can remove them and deal with >> the code duplication in my tree. Afterall, it's unlikely that upsteam >> qemu gets more xilinx boards before some kind of device tree driven >> board support is there. > > I also disagree. Why would the functions in a header file be unclean? > The common practice is wrong. The common practice is to put code in .c files, and stick to declarations needed by several .c files in .h files. It's okay to deviate from that common practice if there's a good reason --- we do that in places. However, in this case, I couldn't see a reason, so I moved the code from the header to the place where such code lives for pretty much every other qdevified device: the device code. Doing it the same way everywhere means that maintainers have an easier time to predict where stuff is, how it's named, and so forth. Don't disappoint reasonable maintainer expectations without need. I don't want to make a big deal out of this. If the code must remain in the header, expectations be damned, I'll respin the series and move on.
On Mon, Sep 14, 2009 at 9:59 PM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias >> <edgar.iglesias@gmail.com> wrote: >>> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote: >>>> On 09/12/09 12:10, Edgar E. Iglesias wrote: >>>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote: >>>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com> >>>>>> wrote: >>>>>>> xilinx.h defines a couple of static inline functions for creating >>>>>>> devices. While that's a fair technique for hot functions, device >>>>>>> initialization is about as cold as it gets. Define them in the device >>>>>>> source files instead, and keep only declarations in the header. >>>>>> >>>>>> If I understood the qdev plan correctly, this is going to wrong >>>>>> direction. These functions should reside near the instantiation, not >>>>>> in the device code. The current approach looks OK if there are going >>>>>> to be more users of the devices. >>>> >>>> The functions should go away ;) >>>> >>>> Some day the information carried by those code snippeds should come from a >>>> machine description file, then we'll don't need them any more. >>> >>> I agree. >>> >>>> >>>>> I agree that they shouldn't be in the device source. >>>>> The reason they ended up in a header and not with the petalogix board >>>>> was that in my tree there are multiple boards using these functions >>>>> to easy instantiate devices. >>>> >>>> They have to be somewhere. Having them in a header file is unclean. Having >>>> them in the board-specific code isn't practical when multiple boards share >>>> the code. I'd stick them to the device source code as well. Also note >>>> that this is common practice elsewhere in the tree. >>> >>> I disagree. >>> >>> But if ppl feel very strongly about this, I can remove them and deal with >>> the code duplication in my tree. Afterall, it's unlikely that upsteam >>> qemu gets more xilinx boards before some kind of device tree driven >>> board support is there. >> >> I also disagree. Why would the functions in a header file be unclean? >> The common practice is wrong. > > The common practice is to put code in .c files, and stick to > declarations needed by several .c files in .h files. It's okay to > deviate from that common practice if there's a good reason --- we do > that in places. However, in this case, I couldn't see a reason, so I > moved the code from the header to the place where such code lives for > pretty much every other qdevified device: the device code. Understandable, since we don't see the source code of the other xilinx boards. > Doing it the same way everywhere means that maintainers have an easier > time to predict where stuff is, how it's named, and so forth. Don't > disappoint reasonable maintainer expectations without need. It's still wrong, however disappointing that may be. But with enough cleanups and guidance, the expectations will be directed to right path. > I don't want to make a big deal out of this. If the code must remain in > the header, expectations be damned, I'll respin the series and move on. Pushing the device instantiation code higher makes the device code a bit simpler. Also the internal state can be kept private, provided that the interface is designed properly. For example, slavio_timer.c does not export internal state and all functions are static. The former init function is now in sun4m.c. Actually, while I'm quite happy with the device level design (and I think other targets should also be worked to this direction, not the opposite), the next level design is not so clear since we don't have the machine config or something.
diff --git a/hw/xilinx.h b/hw/xilinx.h index 070679c..ea91be1 100644 --- a/hw/xilinx.h +++ b/hw/xilinx.h @@ -1,50 +1,12 @@ +#ifndef XILINX_H +#define XILINX_H -/* OPB Interrupt Controller. */ qemu_irq *microblaze_pic_init_cpu(CPUState *env); - -static inline DeviceState * -xilinx_intc_create(target_phys_addr_t base, qemu_irq irq, int kind_of_intr) -{ - DeviceState *dev; - - dev = qdev_create(NULL, "xilinx,intc"); - qdev_prop_set_uint32(dev, "kind-of-intr", kind_of_intr); - qdev_init(dev); - sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); - sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); - return dev; -} - -/* OPB Timer/Counter. */ -static inline DeviceState * -xilinx_timer_create(target_phys_addr_t base, qemu_irq irq, int nr, int freq) -{ - DeviceState *dev; - - dev = qdev_create(NULL, "xilinx,timer"); - qdev_prop_set_uint32(dev, "nr-timers", nr); - qdev_prop_set_uint32(dev, "frequency", freq); - qdev_init(dev); - sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); - sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); - return dev; -} - -/* XPS Ethernet Lite MAC. */ -static inline DeviceState * -xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq, - int txpingpong, int rxpingpong) -{ - DeviceState *dev; - - qemu_check_nic_model(nd, "xilinx-ethlite"); - - dev = qdev_create(NULL, "xilinx,ethlite"); - dev->nd = nd; - qdev_prop_set_uint32(dev, "txpingpong", txpingpong); - qdev_prop_set_uint32(dev, "rxpingpong", rxpingpong); - qdev_init(dev); - sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); - sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); - return dev; -} +DeviceState *xilinx_intc_create(target_phys_addr_t base, qemu_irq irq, + int kind_of_intr); +DeviceState *xilinx_timer_create(target_phys_addr_t base, qemu_irq irq, + int nr, int freq); +DeviceState *xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, + qemu_irq irq, int txpingpong, int rxpingpong); + +#endif diff --git a/hw/xilinx_ethlite.c b/hw/xilinx_ethlite.c index 9b0074c..1710149 100644 --- a/hw/xilinx_ethlite.c +++ b/hw/xilinx_ethlite.c @@ -25,6 +25,7 @@ #include "sysbus.h" #include "hw.h" #include "net.h" +#include "xilinx.h" #define D(x) #define R_TX_BUF0 0 @@ -201,6 +202,24 @@ static ssize_t eth_rx(VLANClientState *vc, const uint8_t *buf, size_t size) return size; } +DeviceState * +xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq, + int txpingpong, int rxpingpong) +{ + DeviceState *dev; + + qemu_check_nic_model(nd, "xilinx-ethlite"); + + dev = qdev_create(NULL, "xilinx,ethlite"); + dev->nd = nd; + qdev_prop_set_uint32(dev, "txpingpong", txpingpong); + qdev_prop_set_uint32(dev, "rxpingpong", rxpingpong); + qdev_init(dev); + sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); + sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); + return dev; +} + static void eth_cleanup(VLANClientState *vc) { struct xlx_ethlite *s = vc->opaque; diff --git a/hw/xilinx_intc.c b/hw/xilinx_intc.c index 8ef6474..dfb8442 100644 --- a/hw/xilinx_intc.c +++ b/hw/xilinx_intc.c @@ -24,6 +24,7 @@ #include "sysbus.h" #include "hw.h" +#include "xilinx.h" #define D(x) @@ -145,6 +146,19 @@ static void irq_handler(void *opaque, int irq, int level) update_irq(p); } +DeviceState * +xilinx_intc_create(target_phys_addr_t base, qemu_irq irq, int kind_of_intr) +{ + DeviceState *dev; + + dev = qdev_create(NULL, "xilinx,intc"); + qdev_prop_set_uint32(dev, "kind-of-intr", kind_of_intr); + qdev_init(dev); + sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); + sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); + return dev; +} + static int xilinx_intc_init(SysBusDevice *dev) { struct xlx_pic *p = FROM_SYSBUS(typeof (*p), dev); diff --git a/hw/xilinx_timer.c b/hw/xilinx_timer.c index e2d9541..26df4b0 100644 --- a/hw/xilinx_timer.c +++ b/hw/xilinx_timer.c @@ -25,6 +25,7 @@ #include "sysbus.h" #include "sysemu.h" #include "qemu-timer.h" +#include "xilinx.h" #define D(x) @@ -189,6 +190,20 @@ static void timer_hit(void *opaque) timer_update_irq(t); } +DeviceState * +xilinx_timer_create(target_phys_addr_t base, qemu_irq irq, int nr, int freq) +{ + DeviceState *dev; + + dev = qdev_create(NULL, "xilinx,timer"); + qdev_prop_set_uint32(dev, "nr-timers", nr); + qdev_prop_set_uint32(dev, "frequency", freq); + qdev_init(dev); + sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); + sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq); + return dev; +} + static int xilinx_timer_init(SysBusDevice *dev) { struct timerblock *t = FROM_SYSBUS(typeof (*t), dev);
xilinx.h defines a couple of static inline functions for creating devices. While that's a fair technique for hot functions, device initialization is about as cold as it gets. Define them in the device source files instead, and keep only declarations in the header. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/xilinx.h | 58 ++++++++------------------------------------------ hw/xilinx_ethlite.c | 19 ++++++++++++++++ hw/xilinx_intc.c | 14 ++++++++++++ hw/xilinx_timer.c | 15 +++++++++++++ 4 files changed, 58 insertions(+), 48 deletions(-)