Re: [PATCH v1 33/43] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

[Richard Henderson]

On 4/19/22 00:27, yangxiaojuan wrote:
> On 2022/4/18 下午10:39, Richard Henderson wrote:
> > > > Is there a good reason that this function is treating hi and lo separately, as opposed 
> > > > to simply doing all of the computation on uint64_t?
> > >
> > > In the part of linux kernel, pch pic driver use 32 bits for reading and writing.
> > > e.g in the drivers/irqchip/irq-loongson-pch-pic.c， pch_pic_mask_irq() function use 
> > > writel() to write pch_pic mask reg.
> >
> > So?  update_irq is not writel.
> >
> > I expect that you should have done something (manual deposit64 or .impl.min_access_size 
> > = 8) with the actual writel, but by the time we arrive in this subroutine we have a 
> > complete uint64_t.
>
> In the linux kernel pch_pic driver, pch_pic_unmask_irq() use writel to config 
> PCH_PIC_CLR reg:
>
> writel(BIT(PIC_REG_BIT(d->hwirq)),
>                      priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
>
> And writel() need u32 value.
>
> static inline void writel(u32 value, volatile void __iomem *addr)
> {
>      __io_bw();
>      __raw_writel((u32 __force)__cpu_to_le32(value), addr);
>      __io_aw();
> }

Are we talking past one another?
I said just above: it does not matter what the kernel is doing.


> The emulate of PCH_PIC_CLR in qemu LoongArchPCHPIC struct member is intirr_lo/hi(we devide 
> 64bits reg to two 32bits reg to match the linux kernel), it will be changed when we config 
> clear reg or handler irq.
>
> static void loongarch_pch_pic_low_writew(void *opaque, hwaddr addr,
>                                       uint64_t data, unsigned size)
> {
> ...
> case PCH_PIC_INT_CLEAR_LO:
>      if (s->intedge_lo & data) {
>          s->intirr_lo &= (~data);
>          pch_pic_update_irq(s, data, 0, 0);
>          s->intisr_lo &= (~data);
>       }
>      break;
> case PCH_PIC_INT_CLEAR_HI:
>      if (s->intedge_hi & data) {
>          s->intirr_hi &= (~data);
>          pch_pic_update_irq(s, data, 0, 1);
>          s->intisr_hi &= (~data);
>       }
>      break;

One can just as easily do

    case PCH_PIC_INT_CLEAR_LO:
        data = (uint32_t)data;
        goto do_clear;
    case PCH_PIC_INT_CLEAR_HI:
        data <<= 32;
    do_clear:
        s->intrr &= ~data;
        pch_pic_update_irq(s...);
        s->intrs &= ~data;

with the values internal to qemu be represented with uint64_t instead of a pair of 
uint32_t.  Which would in fact be *much* clearer to read, and would seem to cut down the 
number of code lines required by half.


r~