Re: [Xen-devel] [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Stefano,

On 07/04/2015 10:40, Stefano Stabellini wrote:
> On Mon, 6 Apr 2015, Julien Grall wrote:
> > Hi Pranav,
> >
> > Thank you for the patch.
> >
> > On 06/04/2015 10:54, Pranavkumar Sawargaonkar wrote:
> > > In old X-Gene Storm firmware and DT, secure mode addresses have been
> > > mentioned in GICv2 node. In this case maintenance interrupt is used
> > > instead of EOI HW method.
> > >
> > > This patch checks the GIC Distributor Base Address to enable EOI quirk
> > > for old firmware.
> > >
> > > Ref:
> > > http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
> > >
> > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
> > > ---
> > >    xen/arch/arm/platforms/xgene-storm.c |   37
> > > +++++++++++++++++++++++++++++++++-
> > >    1 file changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/platforms/xgene-storm.c
> > > b/xen/arch/arm/platforms/xgene-storm.c
> > > index eee650e..dd7cbfc 100644
> > > --- a/xen/arch/arm/platforms/xgene-storm.c
> > > +++ b/xen/arch/arm/platforms/xgene-storm.c
> > > @@ -22,6 +22,7 @@
> > >    #include <asm/platform.h>
> > >    #include <xen/stdbool.h>
> > >    #include <xen/vmap.h>
> > > +#include <xen/device_tree.h>
> > >    #include <asm/io.h>
> > >    #include <asm/gic.h>
> > >
> > > @@ -35,9 +36,41 @@ static u64 reset_addr, reset_size;
> > >    static u32 reset_mask;
> > >    static bool reset_vals_valid = false;
> > >
> > > +#define XGENE_SEC_GICV2_DIST_ADDR    0x78010000
> > > +static u32 quirk_guest_pirq_need_eoi;
> > This variable will mostly be read. So, I would add __read_mostly.
> >
> > > +
> > > +static void xgene_check_pirq_eoi(void)
> > If I'm not mistaken, this function is only called during Xen initialization.
> > So, I would add __init.
> >
> > > +{
> > > +    struct dt_device_node *node;
> > > +    int res;
> > > +    paddr_t dbase;
> > > +
> > > +    dt_for_each_device_node( dt_host, node )
> > > +    {
> > It would be better to create a new callback for platform specific GIC
> > initialization and use dt_interrupt_controller.
> >
> > This would avoid to have this loop and rely on there is always only one
> > interrupt controller in the DT.
> That is true, however we do know that on this SoC there is only one GIC,
> so it might be acceptable to rely on this knowledge: this is already
> very platform specific code.
If we keep the loop, I would add a comment on the above the loop 
explaining that we rely on there is always only GIC.
Although, I was wondering if we could simply rely on the node path to 
get the DT node?
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel