Re: [Qemu-devel] [PATCH v2 3/7] ppc/pnv: Add XSCOM infrastructure

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]

On Tue, Sep 06, 2016 at 04:42:58PM +0200, Cédric Le Goater wrote:
> On 09/06/2016 02:48 AM, David Gibson wrote:
> > On Mon, Sep 05, 2016 at 05:11:53PM +1000, Benjamin Herrenschmidt wrote:
> >> On Mon, 2016-09-05 at 13:39 +1000, David Gibson wrote:
> >>>> +static XScomDevice *xscom_find_target(XScomState *s, uint32_t
> >>> pcb_addr,
> >>>> +                                      uint32_t *range)
> >>>> +{
> >>>> +    BusChild *bc;
> >>>> +
> >>>> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
> >>>> +        DeviceState *qd = bc->child;
> >>>> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> >>>> +        unsigned int i;
> >>>> +
> >>>> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> >>>> +            if (xd->ranges[i].addr <= pcb_addr &&
> >>>> +                (xd->ranges[i].addr + xd->ranges[i].size) >
> >>> pcb_addr) {
> >>>> +                *range = i;
> >>>> +                return xd;
> >>>> +            }
> >>>> +        }
> >>>> +    }
> >>>
> >>> Hmm.. you could set up a SCOM local address space using the
> >>> infrastructure in memory.c, rather than doing your own dispatch.
> >>
> >> There are pros and cons to this approach. The memory.c stuff comes with
> >> quite a lot of baggage, not all of it very shinny to be honest ;-) I
> >> still *hate* how it forces upon us a whole 128-bit integer arithmetic
> >> library just so that it can represent 1_0000_0000_0000_0000 ... 
> > 
> > Ugh, yeah.  I tried to argue against this when it first came in, but
> > was overruled.
> > 
> >> It would be make more sense to use inclusive start/end instead and
> >> stick to 64-bits.
> >>
> >> That being said, we could do that. We'd have to shift the XSCOM
> >> addresses left by 3 since each address is an 8 bytes reigster and
> >> forbid non-8-bytes accesses.
> > 
> > Ok.  I'm not particularly fussed either way.
> 
> 
> The change does seem too invasive. I can give it a try in next
> version.
> 
> When a memory region is triggered, the impacted device will have
> to convert the address with xscom_to_pcb_addr(). There is some 
> dependency on the chip model because the translation is different. 
> That would be an extra op in the xscom device model I guess. 

Actually, I was still thinking of having an MR for the scom interface
unit, which does the xscom_to_pcb_addr() then re-issues the access in
the PCB address space.

But your suggestion might work too.

> Also, the main purpose of the XscomBus is to loop on the devices 
> to populate the device tree. I am wondering if we could just use 
> a simple list under the chip for that purpose. 
> 
> Thanks,
> 
> C. 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]