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 <an...@basename.se>
> >
> > 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 <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 :(.
> >
> > 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.
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;
}
