> Date: Mon, 11 Jul 2022 08:35:44 +0200
> From: Anton Lindqvist <an...@basename.se>
>
> 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 :(.
> diff --git sys/arch/arm64/dev/ampintc.c sys/arch/arm64/dev/ampintc.c
> index 3983aa5ed08..6407793e9b8 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
> */
> @@ -233,7 +240,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, nconfigs, nintr, nlines;
> uint32_t ictr;
> #ifdef MULTIPROCESSOR
> int nipi, ipiirq[2];
> @@ -258,8 +265,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);
> @@ -281,10 +289,19 @@ ampintc_attach(struct device *parent, struct device
> *self, void *aux)
> /* target no cpus */
> bus_space_write_1(sc->sc_iot, sc->sc_d_ioh, ICD_IPTRn(i), 0);
> }
> - for (i = 2; i < 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.
> + */
> + 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);
> - }
>
> /* software reset of the part? */
> /* set protection bit (kernel only)? */
> @@ -293,6 +310,8 @@ ampintc_attach(struct device *parent, struct device
> *self, void *aux)
>
> sc->sc_handler = mallocarray(nintr, sizeof(*sc->sc_handler), M_DEVBUF,
> M_ZERO | M_NOWAIT);
> + if (sc->sc_handler == NULL)
> + panic("%s: cannot allocate irq handlers", __func__);
> for (i = 0; i < nintr; i++) {
> TAILQ_INIT(&sc->sc_handler[i].iq_list);
> }
> @@ -313,7 +332,7 @@ ampintc_attach(struct device *parent, struct device
> *self, void *aux)
> * possible that most are not available to 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,
> @@ -762,10 +781,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;
> }
>
>
