On Mon, Jul 18, 2022 at 12:37:00PM +0200, Anton Lindqvist wrote:
> On Fri, Jul 15, 2022 at 06:58:33PM +0200, Mark Kettenis wrote:
> > > Date: Mon, 11 Jul 2022 16:21:39 +0200
> > > From: Anton Lindqvist <[email protected]>
> > >
> > > On Mon, Jul 11, 2022 at 09:02:20AM +0200, Mark Kettenis wrote:
> > > > > Date: Mon, 11 Jul 2022 08:35:44 +0200
> > > > > From: Anton Lindqvist <[email protected]>
> > > > >
> > > > > Hi,
> > > > > Addressing the following nits made it easier for me to grasp the
> > > > > ampintc
> > > > > implementation while reading the specification. There's also one
> > > > > missing
> > > > > NULL check included.
> > > > >
> > > > > Too verbose or does anyone else consider this an improvement?
> > > >
> > > > Your timing is a but unfortunate since I just posted the
> > > > suspend/resume diff that will conflict with this.
> > > >
> > > > Some/all of this may be useful, but not right now :(.
> > >
> > > That was actually the thing that reminded me about this diff :)
> > > I can wait until things have stabilized and send out a new diff.
> >
> > Right. I actually have another change coming, but that one won't
> > conflict.
>
> Here's the rebased diff.
Ping
>
> diff --git sys/arch/arm64/dev/ampintc.c sys/arch/arm64/dev/ampintc.c
> index 45a15dd2d57..c569c88cc2b 100644
> --- sys/arch/arm64/dev/ampintc.c
> +++ sys/arch/arm64/dev/ampintc.c
> @@ -125,6 +125,13 @@
> #define ICPHPIR 0x18
> #define ICPIIR 0xFC
>
> +/* Sofware Generated Interrupts (SGI) */
> +#define SGI_MIN 0
> +#define SGI_MAX 15
> +/* Private Peripheral Interrupts (PPI) */
> +#define PPI_MIN 16
> +#define PPI_MAX 31
> +
> /*
> * what about periph_id and component_id
> */
> @@ -237,7 +244,7 @@ ampintc_attach(struct device *parent, struct device
> *self, void *aux)
> {
> struct ampintc_softc *sc = (struct ampintc_softc *)self;
> struct fdt_attach_args *faa = aux;
> - int i, nintr, ncpu;
> + int i, ncpu, nintr, nlines;
> uint32_t ictr;
> #ifdef MULTIPROCESSOR
> int nipi, ipiirq[3];
> @@ -262,8 +269,9 @@ ampintc_attach(struct device *parent, struct device
> *self, void *aux)
> evcount_attach(&sc->sc_spur, "irq1023/spur", NULL);
>
> ictr = bus_space_read_4(sc->sc_iot, sc->sc_d_ioh, ICD_ICTR);
> - nintr = 32 * ((ictr >> ICD_ICTR_ITL_SH) & ICD_ICTR_ITL_M);
> - nintr += 32; /* ICD_ICTR + 1, irq 0-31 is SGI, 32+ is PPI */
> + nlines = (ictr >> ICD_ICTR_ITL_SH) & ICD_ICTR_ITL_M;
> + /* Account for SGI + PPI = 16 + 16 = 32 */
> + nintr = 32 * (nlines + 1);
> sc->sc_nintr = nintr;
> ncpu = ((ictr >> ICD_ICTR_CPU_SH) & ICD_ICTR_CPU_M) + 1;
> printf(" nirq %d, ncpu %d", nintr, ncpu);
> @@ -302,7 +310,7 @@ ampintc_attach(struct device *parent, struct device
> *self, void *aux)
> * the non-secure OS.
> */
> nipi = 0;
> - for (i = 0; i < 16; i++) {
> + for (i = 0; i <= SGI_MAX; i++) {
> int reg, oldreg;
>
> oldreg = bus_space_read_1(sc->sc_iot, sc->sc_d_ioh,
> @@ -417,7 +425,8 @@ ampintc_activate(struct device *self, int act)
> void
> ampintc_init(struct ampintc_softc *sc)
> {
> - int i;
> + uint32_t ictr;
> + int i, nconfigs, nlines;
>
> /* Disable all interrupts, clear all pending */
> for (i = 0; i < sc->sc_nintr / 32; i++) {
> @@ -432,8 +441,20 @@ ampintc_init(struct ampintc_softc *sc)
> /* target no cpus */
> bus_space_write_1(sc->sc_iot, sc->sc_d_ioh, ICD_IPTRn(i), 0);
> }
> - for (i = 2; i < sc->sc_nintr / 16; i++) {
> - /* irq 32 - N */
> +
> + /*
> + * Reset edge/level configuration for all interrupts where each
> + * interrupt is represented using 2 bits and each configuration register
> + * being 32 bits wide. Each configuration register therefore covers 16
> + * configurations.
> + *
> + * Since SGI are read only and PPI might not be programmable skip the
> + * first 2*(16 + 16)/32 = 2 configuration registers.
> + */
> + ictr = bus_space_read_4(sc->sc_iot, sc->sc_d_ioh, ICD_ICTR);
> + nlines = (ictr >> ICD_ICTR_ITL_SH) & ICD_ICTR_ITL_M;
> + nconfigs = 2 * (nlines + 1);
> + for (i = 2; i < nconfigs; i++) {
> bus_space_write_4(sc->sc_iot, sc->sc_d_ioh,
> ICD_ICRn(i * 16), 0);
> }
> @@ -828,10 +849,10 @@ ampintc_intr_establish(int irqno, int type, int level,
> struct cpu_info *ci,
> if (ci == NULL)
> ci = &cpu_info_primary;
>
> - if (irqno < 16) {
> + if (irqno <= SGI_MAX) {
> /* SGI are only EDGE */
> type = IST_EDGE_RISING;
> - } else if (irqno < 32) {
> + } else if (irqno <= PPI_MAX) {
> /* PPI are only LEVEL */
> type = IST_LEVEL_HIGH;
> }
