RE: [PATCH v3 11/16] aspeed/intc: Add AST2700 support

2024-05-21 Thread Jamin Lin
Hi Cedric,

It is a partial block diagram.
Thanks-Jamin

GIC132
  ETH1+---+
+>+0 3|
  ETH2|  4|
+>+1 5|
  ETH3|  6|
+>+219|  INTC  
GIC
  UART0   | 20|+--+
+>+721||  |
+--+
  UART1   | 22||orgate0 +> 
output_pin0+--->+GIC128|
+>+823||  ||
  |
  UART2   | 24||orgate1 +> 
output_pin1+--->+GIC129|
+>+925||  ||
  |
  UART3   | 26||orgate2 +> 
output_pin2+--->+GIC130|
+->10   27||  ||
  |
  UART5   | 28||orgate3 +> 
output_pin3+--->+GIC131|
+>+11   29||  ||
  |
  UART6   |   +--->+orgate4 +> 
output_pin4+--->+GIC132|
+>+12   30||  ||
  |
  UART7   | 31||orgate5 +> 
output_pin5+--->+GIC133|
+>+13 ||  ||
  |
  UART8   |  OR[0:31] ||orgate6 +> 
output_pin6+--->+GIC134|
-->14 ||  ||
  |
  UART9   |   ||orgate7 +> 
output_pin7+--->+GIC135|
->+15 ||  ||
  |
  UART10  |   ||orgate8 +> 
output_pin8+--->+GIC136|
->+16 ||  |
+--+
  UART11  |   |+--+
+>+17 |
  UART12  |   |
+->18 |
  |   |
  |   |
  |   |
  +---+  

> Hi Cedric,
> 
> > Hello Jamin
> >
> > On 4/16/24 11:18, Jamin Lin wrote:
> > > AST2700 interrupt controller(INTC) provides hardware interrupt
> > > interfaces to interrupt of processors PSP, SSP and TSP. In INTC,
> > > each interrupt of INT 128 to INT136 combines 32 interrupts.
> > >
> > > Introduce a new aspeed_intc class with instance_init and realize handlers.
> > >
> > > So far, this model only supports GICINT128 to GICINT136.
> > > It creates 9 GICINT or-gates to connect 32 interrupts sources from
> > > GICINT128 to GICINT136 as IRQ GPIO-OUTPUT pins.
> > > Then, this model registers IRQ handler with its IRQ GPIO-INPUT pins
> > > which connect to GICINT or-gates. And creates 9 GICINT IRQ
> > > GPIO-OUTPUT pins which connect to GIC device with GIC IRQ 128 to 136.
> > >
> > > If one interrupt source from GICINT128 to GICINT136 set irq, the
> > > OR-GATE irq callback function is called and set irq to INTC by
> > > OR-GATE GPIO-OUTPUT pins. Then, the INTC irq callback function is
> > > called and set irq to GIC by its GICINT IRQ GPIO-OUTPUT pins.
> > > Finally, the GIC irq callback function is called and set irq to CPUs
> > > and CPUs execute Interrupt Service Routine (ISR).
> >
> > It is difficult to understand what the model does :/
> >
> I noticed serial console stuck if I run the following commands. In this 
> situation,
> I need to type keyboard any keys to trigger serial interrupt again.
> The contents of my test script as following.
> Test.sh
> ```
> while true;
> do
>   ifconfig
>   ip addr
>   cat /proc/cpuinfo
>   cat /proc/meminfo
>   systemctl status
>   hexdump /dev/mtd0
> done
> ```
> 
> I refactor INTC model and believe this issues has been fixed because I run 
> this
> scripts stress test INTC model over 3 days.
> Could you please help to review the new changes and give me your
> suggestions?
> 
> To make you more understand this model behavior my briefly instruction as
> following.
> 1.A source trigger interrupt from a orgate, the aspeed_intc_set_irq is 
> called.
> In this function, it checks orgate level index from bit 0 to 31 with INTC 
> enable
> variable to find the valid source interrupt.
> Then, set source bit 1 to status register. Finally, FW known to run which 
> source
> ISR by reading status register.
> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/ir
> qchip/irq-aspeed-intc.c#L30
> For example: According to the data sheet description, serial console(tty12) 
> is in
> gic132 at bit18. Therefore, it is in orgate index 4(132-128) and level index 
> 18.
> s->regs[status_addr] = select;
> trace_aspeed_intc_debug("trigger source interrupt",
>  

RE: [PATCH v3 11/16] aspeed/intc: Add AST2700 support

2024-05-21 Thread Jamin Lin
Hi Cedric,

> Hello Jamin
> 
> On 4/16/24 11:18, Jamin Lin wrote:
> > AST2700 interrupt controller(INTC) provides hardware interrupt
> > interfaces to interrupt of processors PSP, SSP and TSP. In INTC, each
> > interrupt of INT 128 to INT136 combines 32 interrupts.
> >
> > Introduce a new aspeed_intc class with instance_init and realize handlers.
> >
> > So far, this model only supports GICINT128 to GICINT136.
> > It creates 9 GICINT or-gates to connect 32 interrupts sources from
> > GICINT128 to GICINT136 as IRQ GPIO-OUTPUT pins.
> > Then, this model registers IRQ handler with its IRQ GPIO-INPUT pins
> > which connect to GICINT or-gates. And creates 9 GICINT IRQ GPIO-OUTPUT
> > pins which connect to GIC device with GIC IRQ 128 to 136.
> >
> > If one interrupt source from GICINT128 to GICINT136 set irq, the
> > OR-GATE irq callback function is called and set irq to INTC by OR-GATE
> > GPIO-OUTPUT pins. Then, the INTC irq callback function is called and
> > set irq to GIC by its GICINT IRQ GPIO-OUTPUT pins. Finally, the GIC
> > irq callback function is called and set irq to CPUs and CPUs execute
> > Interrupt Service Routine (ISR).
> 
> It is difficult to understand what the model does :/
> 
I noticed serial console stuck if I run the following commands. In this 
situation, I need to type keyboard any keys to trigger serial interrupt again.
The contents of my test script as following.
Test.sh
```
while true;
do
  ifconfig
  ip addr
  cat /proc/cpuinfo
  cat /proc/meminfo
  systemctl status
  hexdump /dev/mtd0
done
```

I refactor INTC model and believe this issues has been fixed because I run this 
scripts stress test INTC model over 3 days. 
Could you please help to review the new changes and give me your suggestions?

To make you more understand this model behavior my briefly instruction as 
following.
1.  A source trigger interrupt from a orgate, the aspeed_intc_set_irq is 
called.
In this function, it checks orgate level index from bit 0 to 31 with INTC 
enable variable to find the valid source interrupt.
Then, set source bit 1 to status register. Finally, FW known to run which 
source ISR by reading status register.
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/irqchip/irq-aspeed-intc.c#L30
 
For example: According to the data sheet description, serial console(tty12) is 
in gic132 at bit18. Therefore, it is in orgate index 4(132-128) and level index 
18.
s->regs[status_addr] = select;
trace_aspeed_intc_debug("trigger source interrupt",
  s->regs[status_addr]);
aspeed_intc_update(s, irq, 1)

2.  Once CPU enters ISR mode, FW masks enable register first, clear source 
bit in status register after it executes source ISR and unmasks enable register.
a.  Mask 
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/irqchip/irq-aspeed-intc.c#L39
b.  After executes source ISR, clear status register at source bit, 
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/irqchip/irq-aspeed-intc.c#L33
c.  Unmask 
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/irqchip/irq-aspeed-intc.c#L46

3.  If status register is 0, it means ALL source ISR execution are done. If 
no pending interrupt, it clear gic else set gic to handle source pending 
interrupts.
/* All source ISR execution are done */
if (!s->regs[addr]) {
  if (s->pending[irq]) {
 /*
  * handle pending source interrupt
  * notify firmware which source interrupt are pending
  * by setting status register
  */
  s->regs[addr] = s->pending[irq];
  s->pending[irq] = 0;
  trace_aspeed_intc_debug("trigger pending interrupt",
   s->regs[addr]);
  aspeed_intc_update(s, irq, 1);
 } else {
   /* clear irq */
   aspeed_intc_update(s, irq, 0);
 }
 }

4.  According to the design of INTC controller, it only have Enable 
register. This register is used for enable source interrupt, mask and unmake 
source interrupt if executing ISR mode.
We don’t know users want to “mask or disable” specific source interrupt because 
it only have one register.
So, I create enable variables to save the source interrupt enable status if 
enable register has been set to 1.

old_enable = s->enable[irq];
s->enable[irq] |= data;

/* enable new source interrupt */
if (old_enable != s->enable[irq]) {
trace_aspeed_intc_debug("enable new source interrupt",
s->enable[irq]);
s->regs[addr] = data;
return;
}

If no new bits set to 1, it means mask/unmask behavior. It set mask status in 
s->mask variables, so aspeed_intc_set_irq function know it is in ISR mode, and 
all source interrupts are saved to pending variables.
After ISR done, this model set gic to handle pending interrupts.

/* mask and unmask source interrupt */
change = s->regs[addr] ^ data;
trace_aspeed_intc_debug("change bit", change);
if (change & data) {
  s->mask[irq] &= 

Re: [PATCH v3 11/16] aspeed/intc: Add AST2700 support

2024-05-07 Thread Cédric Le Goater

Hello Jamin

On 4/16/24 11:18, Jamin Lin wrote:

AST2700 interrupt controller(INTC) provides hardware interrupt interfaces
to interrupt of processors PSP, SSP and TSP. In INTC, each interrupt of
INT 128 to INT136 combines 32 interrupts.

Introduce a new aspeed_intc class with instance_init and realize handlers.

So far, this model only supports GICINT128 to GICINT136.
It creates 9 GICINT or-gates to connect 32 interrupts sources
from GICINT128 to GICINT136 as IRQ GPIO-OUTPUT pins.
Then, this model registers IRQ handler with its IRQ GPIO-INPUT pins which
connect to GICINT or-gates. And creates 9 GICINT IRQ GPIO-OUTPUT pins which
connect to GIC device with GIC IRQ 128 to 136.

If one interrupt source from GICINT128 to GICINT136
set irq, the OR-GATE irq callback function is called and set irq to INTC by
OR-GATE GPIO-OUTPUT pins. Then, the INTC irq callback function is called and
set irq to GIC by its GICINT IRQ GPIO-OUTPUT pins. Finally, the GIC irq
callback function is called and set irq to CPUs and
CPUs execute Interrupt Service Routine (ISR).


It is difficult to understand what the model does :/


Signed-off-by: Troy Lee 
Signed-off-by: Jamin Lin 
---
  hw/intc/aspeed_intc.c | 269 ++
  hw/intc/meson.build   |   1 +
  hw/intc/trace-events  |   6 +
  include/hw/intc/aspeed_intc.h |  35 +
  4 files changed, 311 insertions(+)
  create mode 100644 hw/intc/aspeed_intc.c
  create mode 100644 include/hw/intc/aspeed_intc.h

diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
new file mode 100644
index 00..df31900d56
--- /dev/null
+++ b/hw/intc/aspeed_intc.c
@@ -0,0 +1,269 @@
+/*
+ * ASPEED INTC Controller
+ *
+ * Copyright (C) 2024 ASPEED Technology Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/intc/aspeed_intc.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "hw/intc/arm_gicv3.h"
+#include "trace.h"
+#include "hw/registerfields.h"
+#include "qapi/error.h"
+
+/* INTC Registers */
+REG32(GICINT128_EN, 0x1000)
+REG32(GICINT128_STATUS, 0x1004)
+REG32(GICINT129_EN, 0x1100)
+REG32(GICINT129_STATUS, 0x1104)
+REG32(GICINT130_EN, 0x1200)
+REG32(GICINT130_STATUS, 0x1204)
+REG32(GICINT131_EN, 0x1300)
+REG32(GICINT131_STATUS, 0x1304)
+REG32(GICINT132_EN, 0x1400)
+REG32(GICINT132_STATUS, 0x1404)
+REG32(GICINT133_EN, 0x1500)
+REG32(GICINT133_STATUS, 0x1504)
+REG32(GICINT134_EN, 0x1600)
+REG32(GICINT134_STATUS, 0x1604)
+REG32(GICINT135_EN, 0x1700)
+REG32(GICINT135_STATUS, 0x1704)
+REG32(GICINT136_EN, 0x1800)
+REG32(GICINT136_STATUS, 0x1804)
+
+#define GICINT_EN_BASE R_GICINT128_EN
+
+/*
+ * The address of GICINT128 to GICINT136 are from 0x1000 to 0x1804.
+ * Utilize "address & 0x0f00" to get the gicint_out index and
+ * its gic irq.
+ */
+static void aspeed_intc_update(AspeedINTCState *s, int irq, int level)
+{
+uint32_t gicint_enable_addr = GICINT_EN_BASE + ((0x100 * irq) >> 2);
+uint32_t gicint_status_addr = gicint_enable_addr + (0x4 >> 2);
+
+if (s->trigger[irq]) {
+if (!level && !s->regs[gicint_status_addr]) {
+/* clear irq */
+trace_aspeed_intc_update_irq(irq, 0);
+qemu_set_irq(s->gicint_out[irq], 0);
+s->trigger[irq] = false;
+}
+} else {
+if (s->new_gicint_status[irq]) {
+/* set irq */
+trace_aspeed_intc_update_irq(irq, 1);
+s->regs[gicint_status_addr] = s->new_gicint_status[irq];
+s->new_gicint_status[irq] = 0;
+qemu_set_irq(s->gicint_out[irq], 1);
+s->trigger[irq] = true;
+}
+}


I will trust you on this as I don't understand.



+}
+
+/*
+ * The value of irq should be 0 to ASPEED_INTC_NR_GICS.
+ * The irq 0 indicates GICINT128, irq 1 indicates GICINT129 and so on.
+ */
+static void aspeed_intc_set_irq(void *opaque, int irq, int level)
+{
+AspeedINTCState *s = (AspeedINTCState *)opaque;
+uint32_t gicint_enable_addr = GICINT_EN_BASE + ((0x100 * irq) >> 2);
+uint32_t enable = s->regs[gicint_enable_addr];
+int i;
+
+if (irq > ASPEED_INTC_NR_GICS) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid interrupt number: %d\n",
+  __func__, irq);
+return;
+}
+
+trace_aspeed_intc_set_irq(irq, level);
+
+for (i = 0; i < 32; i++) {
+if (s->gicint_orgate[irq].levels[i]) {
+if (enable & BIT(i)) {
+s->new_gicint_status[irq] |= BIT(i);
+}
+}
+}
+
+aspeed_intc_update(s, irq, level);
+}
+
+static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned int 
size)
+{
+AspeedINTCState *s = ASPEED_INTC(opaque);
+uint32_t addr = offset >> 2;
+uint32_t value = 0;
+
+if (addr >=