Re: [PATCH V5 2/3] dt-bindings: update riscv plic compatible string

[Anup Patel]

On Sun, Oct 24, 2021 at 3:05 PM Guo Ren <guoren@xxxxxxxxxx> wrote:
>
> On Sun, Oct 24, 2021 at 5:18 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
> >
> > On Sun, Oct 24, 2021 at 2:31 PM Guo Ren <guoren@xxxxxxxxxx> wrote:
> > >
> > > On Sun, Oct 24, 2021 at 3:35 PM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Sun, Oct 24, 2021 at 7:03 AM <guoren@xxxxxxxxxx> wrote:
> > > > >
> > > > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > > >
> > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > bindings to support allwinner d1 SOC which contains c906 core.
> > > > >
> > > > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > > > Cc: Anup Patel <anup@xxxxxxxxxxxxxx>
> > > > > Cc: Atish Patra <atish.patra@xxxxxxx>
> > > > > Cc: Heiko Stuebner <heiko@xxxxxxxxx>
> > > > > Cc: Rob Herring <robh@xxxxxxxxxx>
> > > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > > > Cc: Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>
> > > > >
> > > > > ---
> > > > >
> > > > > Changes since V5:
> > > > >  - Add DT list
> > > > >  - Fixup compatible string
> > > > >  - Remove allwinner-d1 compatible
> > > > >  - make dt_binding_check
> > > > >
> > > > > Changes since V4:
> > > > >  - Update description in errata style
> > > > >  - Update enum suggested by Anup, Heiko, Samuel
> > > > >
> > > > > Changes since V3:
> > > > >  - Rename "c9xx" to "c900"
> > > > >  - Add thead,c900-plic in the description section
> > > > > ---
> > > > >  .../interrupt-controller/sifive,plic-1.0.0.yaml   | 15 ++++++++++++---
> > > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> > > > > index 08d5a57ce00f..18b97bfd7954 100644
> > > > > --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> > > > > @@ -35,6 +35,10 @@ description:
> > > > >    contains a specific memory layout, which is documented in chapter 8 of the
> > > > >    SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
> > > > >
> > > > > +  The thead,c900-plic couldn't complete masked irq source which has been disabled in
> > > > > +  enable register. Add thead_plic_chip which fix up c906-plic irq source completion
> > > > > +  problem by unmask/mask wrapper.
> > > > > +
> > > >
> > > > This is an incomplete description about how T-HEAD PLIC is different from
> > > > RISC-V PLIC.
> > > >
> > > > I would suggest the following:
> > > >
> > > > The T-HEAD C9xx SoC implements a modified/custom T-HEAD PLIC specification
> > > > which will mask current IRQ upon read to CLAIM register and will unmask the IRQ
> > > > upon write to CLAIM register. The thead,c900-plic compatible string
> > > > represents the
> > > > custom T-HEAD PLIC specification.
> > > The patch fixup the problem that when "thead,c900-plic" couldn't
> > > complete masked irq source which has been disabled.
> > >
> > > This patch is different from the last one in that there is no
> > > relationship with the auto-mask feature.
> >
> > This patch adds compatible string for T-HEAD PLIC so it
> > should describe how T-HEAD PLIC is different from RISC-V
> > PLIC. The DT bindings document describes HW and not
> > the software work-around implemented using DT bindings.
> >
> > Your irqchip patch uses T-HEAD PLIC compatible string to
> > implement a work-around.
> >
> > In other words, this patch is different from the irqchip patch.
>
> How about below:
>
> The thead,c900-plic compatible string represents the custom T-HEAD
> PLIC specification.
>  - It couldn't complete masked irq source which has been disabled in
> enable register. Add thead_plic_chip which fix up c906-plic irq source
> completion problem by unmask/mask wrapper.

This first bullet is not required because it describes how it is used
in irqchip driver to fix issues. This info has to go in your driver fix
patch.

>  - It implements a modified/custom T-HEAD PLIC specification which
> will mask current IRQ upon read to CLAIM register and will unmask the
> IRQ upon write to CLAIM register. But the feature wasn't utilized by
> software.

Please don't advertise non-compliance with RISC-V PLIC spec as feature.

What I had suggest before seems better.

Regards,
Anup

>
> >
> > Regards,
> > Anup
> >
> > >
> > > >
> > > > Regards,
> > > > Anup
> > > >
> > > > >  maintainers:
> > > > >    - Sagar Kadam <sagar.kadam@xxxxxxxxxx>
> > > > >    - Paul Walmsley  <paul.walmsley@xxxxxxxxxx>
> > > > > @@ -42,11 +46,16 @@ maintainers:
> > > > >
> > > > >  properties:
> > > > >    compatible:
> > > > > -    items:
> > > > > +   oneOf:
> > > > > +    - items:
> > > > >        - enum:
> > > > > -          - sifive,fu540-c000-plic
> > > > > -          - canaan,k210-plic
> > > > > +        - sifive,fu540-c000-plic
> > > > > +        - canaan,k210-plic
> > > > >        - const: sifive,plic-1.0.0
> > > > > +    - items:
> > > > > +      - enum:
> > > > > +        - allwinner,sun20i-d1-plic
> > > > > +      - const: thead,c900-plic
> > > > >
> > > > >    reg:
> > > > >      maxItems: 1
> > > > > --
> > > > > 2.25.1
> > > > >
> > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> > >
> > > ML: https://lore.kernel.org/linux-csky/
>
>
>
> --
> Best Regards
>  Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/