Re: svn commit: r299944 - in head/sys: arm64/arm64 conf

2016-05-18 Thread Zbigniew Bodek 
Hello Andrew,

Thanks for the comments. Please check in-line below.

Kind regards
zbb

2016-05-17 14:19 GMT+02:00 Andrew Turner :

> On Mon, 16 May 2016 18:08:49 +0200
> Zbigniew Bodek  wrote:
>
> > Hello Andrew,
> >
> > I think committing this code should be preceded by at least brief
> > review. Few remarks to the code found on the first glance below.
>
> See below for comments.
>
> >
> > Kind regards
> > zbb
> >
> > 2016-05-16 16:07 GMT+02:00 Andrew Turner :
> >
> > > Author: andrew
> > > Date: Mon May 16 14:07:43 2016
> > > New Revision: 299944
> > > URL: https://svnweb.freebsd.org/changeset/base/299944
> > >
> > > Log:
> > >   Add intrng support to the GICv3 driver. It lacks ITS support so
> > > won't handle
> > >   MSI or MSI-X interrupts, however this is enought to boot FreeBSD
> > > under the
> > >   ARM Foundation Model with a GICv3 interrupt controller.
> > >
> > >   Approved by:  ABT Systems Ltd
> > >   Relnotes: yes
> > >   Sponsored by: The FreeBSD Foundation
> ...
> > > +#ifdef INTRNG
> > > +int
> > > +arm_gic_v3_intr(void *arg)
> > > +{
> > > +   struct gic_v3_softc *sc = arg;
> > > +   struct gic_v3_irqsrc *gi;
> > > +   uint64_t active_irq;
> > > +   struct trapframe *tf;
> > > +   bool first;
> > > +
> > > +   first = true;
> > > +
> > > +   while (1) {
> > > +   if (CPU_MATCH_ERRATA_CAVIUM_THUNDER_1_1) {
> > > +   /*
> > > +* Hardware:Cavium ThunderX
> > > +* Chip revision:   Pass 1.0 (early
> > > version)
> > > +*  Pass 1.1
> > > (production)
> > > +* ERRATUM: 22978, 23154
> > > +*/
> > > +   __asm __volatile(
> > > +   "nop;nop;nop;nop;nop;nop;nop;nop;   \n"
> > > +   "mrs %0, ICC_IAR1_EL1   \n"
> > > +   "nop;nop;nop;nop;   \n"
> > > +   "dsb sy \n"
> > > +   : "=" (active_irq));
> > > +   } else {
> > > +   active_irq = gic_icc_read(IAR1);
> > > +   }
> > > +
> > > +   if (__predict_false(active_irq >= sc->gic_nirqs))
> > > +   return (FILTER_HANDLED);
> > >
> >
> > IMHO this is not true. Active IRQ could be much bigger than number of
> > supported IRQs. We are asking for debugging in the future when we
> > enable ITS.
>
> It is correct, the ITS change to this file is missing so we are unable
> to enable ITS interrupts.
>
>
In general it is not correct in terms of GICv3 but in that particular case.
So it would be good to have a "ARM64TODO" comment here.


> ...
> > > +
> > > +#ifdef FDT
> > > +static int
> > > +gic_map_fdt(device_t dev, u_int ncells, pcell_t *cells, u_int
> > > *irqp,
> > > +enum intr_polarity *polp, enum intr_trigger *trigp)
> > >
> >
> > All other functions are called gic_v3 and this one is just gic_
> > Why can't we move as much FDT code to the dedicated file which is
> > gic_v3_fdt.c?
>
> Unfortunately due to the current code in subr_intr.c this is needed for
> simplicity. It it my understanding there is work to fix this, so for
> now I would prefer to keep this.
>

OK.


>
> ...
> > > +   /* Set the trigger and polarity */
> > > +   if (irq <= GIC_LAST_PPI)
> > > +   reg = gic_r_read(sc, 4,
> > > +   GICR_SGI_BASE_SIZE + GICD_ICFGR(irq));
> > > +   else
> > > +   reg = gic_d_read(sc, 4, GICD_ICFGR(irq));
> > > +   if (trig == INTR_TRIGGER_LEVEL)
> > > +   reg &= ~(2 << ((irq % 16) * 2));
> > > +   else
> > > +   reg |= 2 << ((irq % 16) * 2);
> > >
> >
> > The rule of not using magic numbers does not apply here ;-) ?
>
> This is partially why this is still disabled by default, the code still
> needs a little polish, however it will be needed to help with a future
> review for changes to subr_intr.c.
>

So why do we integrate this WIP code now? Is there anyone else using INTRNG
on ARM64 beside you who need to test it?
I guess this is a matter of opinion but it would be much more convenient
(at least for us) if we get INTRNG in a single patch, review it, test it
and then change the existing code.


> ...
> > > +static void
> > > +gic_v3_disable_intr(device_t dev, struct intr_irqsrc *isrc)
> > > +{
> > > +   struct gic_v3_softc *sc;
> > > +   struct gic_v3_irqsrc *gi;
> > > +   u_int irq;
> > > +
> > > +   sc = device_get_softc(dev);
> > > +   gi = (struct gic_v3_irqsrc *)isrc;
> > > +   irq = gi->gi_irq;
> > > +
> > > +   if (irq <= GIC_LAST_PPI) {
> > > +   /* SGIs and PPIs in corresponding Re-Distributor

Re: svn commit: r299944 - in head/sys: arm64/arm64 conf

2016-05-17 Thread Andrew Turner 
On Mon, 16 May 2016 18:08:49 +0200
Zbigniew Bodek  wrote:

> Hello Andrew,
> 
> I think committing this code should be preceded by at least brief
> review. Few remarks to the code found on the first glance below.

See below for comments.

> 
> Kind regards
> zbb
> 
> 2016-05-16 16:07 GMT+02:00 Andrew Turner :
> 
> > Author: andrew
> > Date: Mon May 16 14:07:43 2016
> > New Revision: 299944
> > URL: https://svnweb.freebsd.org/changeset/base/299944
> >
> > Log:
> >   Add intrng support to the GICv3 driver. It lacks ITS support so
> > won't handle
> >   MSI or MSI-X interrupts, however this is enought to boot FreeBSD
> > under the
> >   ARM Foundation Model with a GICv3 interrupt controller.
> >
> >   Approved by:  ABT Systems Ltd
> >   Relnotes: yes
> >   Sponsored by: The FreeBSD Foundation
...
> > +#ifdef INTRNG
> > +int
> > +arm_gic_v3_intr(void *arg)
> > +{
> > +   struct gic_v3_softc *sc = arg;
> > +   struct gic_v3_irqsrc *gi;
> > +   uint64_t active_irq;
> > +   struct trapframe *tf;
> > +   bool first;
> > +
> > +   first = true;
> > +
> > +   while (1) {
> > +   if (CPU_MATCH_ERRATA_CAVIUM_THUNDER_1_1) {
> > +   /*
> > +* Hardware:Cavium ThunderX
> > +* Chip revision:   Pass 1.0 (early
> > version)
> > +*  Pass 1.1
> > (production)
> > +* ERRATUM: 22978, 23154
> > +*/
> > +   __asm __volatile(
> > +   "nop;nop;nop;nop;nop;nop;nop;nop;   \n"
> > +   "mrs %0, ICC_IAR1_EL1   \n"
> > +   "nop;nop;nop;nop;   \n"
> > +   "dsb sy \n"
> > +   : "=" (active_irq));
> > +   } else {
> > +   active_irq = gic_icc_read(IAR1);
> > +   }
> > +
> > +   if (__predict_false(active_irq >= sc->gic_nirqs))
> > +   return (FILTER_HANDLED);
> >  
> 
> IMHO this is not true. Active IRQ could be much bigger than number of
> supported IRQs. We are asking for debugging in the future when we
> enable ITS.

It is correct, the ITS change to this file is missing so we are unable
to enable ITS interrupts.

...
> > +
> > +#ifdef FDT
> > +static int
> > +gic_map_fdt(device_t dev, u_int ncells, pcell_t *cells, u_int
> > *irqp,
> > +enum intr_polarity *polp, enum intr_trigger *trigp)
> >  
> 
> All other functions are called gic_v3 and this one is just gic_
> Why can't we move as much FDT code to the dedicated file which is
> gic_v3_fdt.c?

Unfortunately due to the current code in subr_intr.c this is needed for
simplicity. It it my understanding there is work to fix this, so for
now I would prefer to keep this.

...
> > +   /* Set the trigger and polarity */
> > +   if (irq <= GIC_LAST_PPI)
> > +   reg = gic_r_read(sc, 4,
> > +   GICR_SGI_BASE_SIZE + GICD_ICFGR(irq));
> > +   else
> > +   reg = gic_d_read(sc, 4, GICD_ICFGR(irq));
> > +   if (trig == INTR_TRIGGER_LEVEL)
> > +   reg &= ~(2 << ((irq % 16) * 2));
> > +   else
> > +   reg |= 2 << ((irq % 16) * 2);
> >  
> 
> The rule of not using magic numbers does not apply here ;-) ?

This is partially why this is still disabled by default, the code still
needs a little polish, however it will be needed to help with a future
review for changes to subr_intr.c.

...
> > +static void
> > +gic_v3_disable_intr(device_t dev, struct intr_irqsrc *isrc)
> > +{
> > +   struct gic_v3_softc *sc;
> > +   struct gic_v3_irqsrc *gi;
> > +   u_int irq;
> > +
> > +   sc = device_get_softc(dev);
> > +   gi = (struct gic_v3_irqsrc *)isrc;
> > +   irq = gi->gi_irq;
> > +
> > +   if (irq <= GIC_LAST_PPI) {
> > +   /* SGIs and PPIs in corresponding Re-Distributor */
> > +   gic_r_write(sc, 4, GICR_SGI_BASE_SIZE +
> > GICD_ICENABLER(irq),
> > +   GICD_I_MASK(irq));
> > +   gic_v3_wait_for_rwp(sc, REDIST);
> > +   } else if (irq >= GIC_FIRST_SPI && irq <= GIC_LAST_SPI) {
> > +   /* SPIs in distributor */
> > +   gic_d_write(sc, 4, GICD_ICENABLER(irq),
> > GICD_I_MASK(irq));
> > +   gic_v3_wait_for_rwp(sc, DIST);
> >  
> 
> In gic_v3_setup_intr() we need spin lock and here we don't ?

The locking was based on the existing intrng gic driver, I think they
are both bogus.

...

> > +static void
> > +gic_v3_ipi_send(device_t dev, struct intr_irqsrc *isrc, cpuset_t
> > cpus,
> > +u_int ipi)
> >  
> 
> What exactly is the functional difference between the current
> implementation and this one?
>

Re: svn commit: r299944 - in head/sys: arm64/arm64 conf

2016-05-16 Thread Ed Maste 
On 16 May 2016 at 12:08, Zbigniew Bodek  wrote:
> Hello Andrew,
>
> I think committing this code should be preceded by at least brief review.

I agree, review makes sense especially in the case of GICv3 and other
files that originated in the ThunderX work or are closely tied to that
platform.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r299944 - in head/sys: arm64/arm64 conf

2016-05-16 Thread Zbigniew Bodek 
Hello Andrew,

I think committing this code should be preceded by at least brief review.
Few remarks to the code found on the first glance below.

Kind regards
zbb

2016-05-16 16:07 GMT+02:00 Andrew Turner :

> Author: andrew
> Date: Mon May 16 14:07:43 2016
> New Revision: 299944
> URL: https://svnweb.freebsd.org/changeset/base/299944
>
> Log:
>   Add intrng support to the GICv3 driver. It lacks ITS support so won't
> handle
>   MSI or MSI-X interrupts, however this is enought to boot FreeBSD under
> the
>   ARM Foundation Model with a GICv3 interrupt controller.
>
>   Approved by:  ABT Systems Ltd
>   Relnotes: yes
>   Sponsored by: The FreeBSD Foundation
>
> Modified:
>   head/sys/arm64/arm64/gic_v3.c
>   head/sys/arm64/arm64/gic_v3_fdt.c
>   head/sys/arm64/arm64/gic_v3_var.h
>   head/sys/conf/files.arm64
>
> Modified: head/sys/arm64/arm64/gic_v3.c
>
> ==
> --- head/sys/arm64/arm64/gic_v3.c   Mon May 16 13:39:04 2016
> (r299943)
> +++ head/sys/arm64/arm64/gic_v3.c   Mon May 16 14:07:43 2016
> (r299944)
> @@ -1,7 +1,10 @@
>  /*-
> - * Copyright (c) 2015 The FreeBSD Foundation
> + * Copyright (c) 2015-2016 The FreeBSD Foundation
>   * All rights reserved.
>   *
> + * This software was developed by Andrew Turner under
> + * the sponsorship of the FreeBSD Foundation.
> + *
>   * This software was developed by Semihalf under
>   * the sponsorship of the FreeBSD Foundation.
>   *
> @@ -27,6 +30,8 @@
>   * SUCH DAMAGE.
>   */
>
> +#include "opt_platform.h"
> +
>  #include 
>  __FBSDID("$FreeBSD$");
>
> @@ -58,6 +63,28 @@ __FBSDID("$FreeBSD$");
>  #include "gic_v3_reg.h"
>  #include "gic_v3_var.h"
>
> +#ifdef INTRNG
> +static pic_disable_intr_t gic_v3_disable_intr;
> +static pic_enable_intr_t gic_v3_enable_intr;
> +static pic_map_intr_t gic_v3_map_intr;
> +static pic_setup_intr_t gic_v3_setup_intr;
> +static pic_teardown_intr_t gic_v3_teardown_intr;
> +static pic_post_filter_t gic_v3_post_filter;
> +static pic_post_ithread_t gic_v3_post_ithread;
> +static pic_pre_ithread_t gic_v3_pre_ithread;
> +static pic_bind_intr_t gic_v3_bind_intr;
> +#ifdef SMP
> +static pic_init_secondary_t gic_v3_init_secondary;
> +static pic_ipi_send_t gic_v3_ipi_send;
> +static pic_ipi_setup_t gic_v3_ipi_setup;
> +#endif
> +
> +static u_int gic_irq_cpu;
> +#ifdef SMP
> +static u_int sgi_to_ipi[GIC_LAST_SGI - GIC_FIRST_SGI + 1];
> +static u_int sgi_first_unused = GIC_FIRST_SGI;
> +#endif
> +#else
>  /* Device and PIC methods */
>  static int gic_v3_bind(device_t, u_int, u_int);
>  static void gic_v3_dispatch(device_t, struct trapframe *);
> @@ -68,11 +95,29 @@ static void gic_v3_unmask_irq(device_t,
>  static void gic_v3_init_secondary(device_t);
>  static void gic_v3_ipi_send(device_t, cpuset_t, u_int);
>  #endif
> +#endif
>
>  static device_method_t gic_v3_methods[] = {
> /* Device interface */
> DEVMETHOD(device_detach,gic_v3_detach),
>
> +#ifdef INTRNG
> +   /* Interrupt controller interface */
> +   DEVMETHOD(pic_disable_intr, gic_v3_disable_intr),
> +   DEVMETHOD(pic_enable_intr,  gic_v3_enable_intr),
> +   DEVMETHOD(pic_map_intr, gic_v3_map_intr),
> +   DEVMETHOD(pic_setup_intr,   gic_v3_setup_intr),
> +   DEVMETHOD(pic_teardown_intr,gic_v3_teardown_intr),
> +   DEVMETHOD(pic_post_filter,  gic_v3_post_filter),
> +   DEVMETHOD(pic_post_ithread, gic_v3_post_ithread),
> +   DEVMETHOD(pic_pre_ithread,  gic_v3_pre_ithread),
> +#ifdef SMP
> +   DEVMETHOD(pic_bind_intr,gic_v3_bind_intr),
> +   DEVMETHOD(pic_init_secondary,   gic_v3_init_secondary),
> +   DEVMETHOD(pic_ipi_send, gic_v3_ipi_send),
> +   DEVMETHOD(pic_ipi_setup,gic_v3_ipi_setup),
> +#endif
> +#else
> /* PIC interface */
> DEVMETHOD(pic_bind, gic_v3_bind),
> DEVMETHOD(pic_dispatch, gic_v3_dispatch),
> @@ -83,6 +128,8 @@ static device_method_t gic_v3_methods[]
> DEVMETHOD(pic_init_secondary,   gic_v3_init_secondary),
> DEVMETHOD(pic_ipi_send, gic_v3_ipi_send),
>  #endif
> +#endif
> +
> /* End */
> DEVMETHOD_END
>  };
> @@ -144,6 +191,10 @@ gic_v3_attach(device_t dev)
> int rid;
> int err;
> size_t i;
> +#ifdef INTRNG
> +   u_int irq;
> +   const char *name;
> +#endif
>
> sc = device_get_softc(dev);
> sc->gic_registered = FALSE;
> @@ -192,6 +243,36 @@ gic_v3_attach(device_t dev)
> if (sc->gic_nirqs > GIC_I_NUM_MAX)
> sc->gic_nirqs = GIC_I_NUM_MAX;
>
> +#ifdef INTRNG
> +   sc->gic_irqs = malloc(sizeof(*sc->gic_irqs) * sc->gic_nirqs,
> +   M_GIC_V3, M_WAITOK | M_ZERO);
> +   name = device_get_nameunit(dev);
> +   for (irq = 0; irq < sc->gic_nirqs; irq++) {
> +   struct intr_irqsrc *isrc;
> +
> +   sc->gic_irqs[irq].gi_irq = irq;
> +