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?
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;
}
