| Message ID | 20170417115747.7300-3-icenowy@aosc.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote: > Allwinner A64 SoC features a NMI controller, which is usually connected > to the AXP PMIC. > > Add support for it. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > Acked-by: Chen-Yu Tsai <wens@csie.org> > --- > Changes in v2: > - Added Chen-Yu's ACK. > > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > index 05ec9fc5e81f..53c18ca372ea 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > @@ -403,6 +403,14 @@ > <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > }; > > + nmi_intc: interrupt-controller@01f00c0c { > + compatible = "allwinner,sun6i-a31-sc-nmi"; > + interrupt-controller; > + #interrupt-cells = <2>; > + reg = <0x01f00c0c 0x38>; The base address is not correct, and there's uncertainty on whether this is this particular controller or not. Did you even test this? Maxime
于 2017年4月18日 GMT+08:00 下午3:00:16, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote: >> Allwinner A64 SoC features a NMI controller, which is usually >connected >> to the AXP PMIC. >> >> Add support for it. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> Acked-by: Chen-Yu Tsai <wens@csie.org> >> --- >> Changes in v2: >> - Added Chen-Yu's ACK. >> >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> index 05ec9fc5e81f..53c18ca372ea 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> @@ -403,6 +403,14 @@ >> <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; >> }; >> >> + nmi_intc: interrupt-controller@01f00c0c { >> + compatible = "allwinner,sun6i-a31-sc-nmi"; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + reg = <0x01f00c0c 0x38>; > >The base address is not correct, and there's uncertainty on whether >this is this particular controller or not. Did you even test this? Tested by axp20x-pek. > >Maxime
On Tue, Apr 18, 2017 at 06:56:43PM +0800, Icenowy Zheng wrote: > > > 于 2017年4月18日 GMT+08:00 下午3:00:16, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: > >On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote: > >> Allwinner A64 SoC features a NMI controller, which is usually > >connected > >> to the AXP PMIC. > >> > >> Add support for it. > >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > >> Acked-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> Changes in v2: > >> - Added Chen-Yu's ACK. > >> > >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> index 05ec9fc5e81f..53c18ca372ea 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> @@ -403,6 +403,14 @@ > >> <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > >> }; > >> > >> + nmi_intc: interrupt-controller@01f00c0c { > >> + compatible = "allwinner,sun6i-a31-sc-nmi"; > >> + interrupt-controller; > >> + #interrupt-cells = <2>; > >> + reg = <0x01f00c0c 0x38>; > > > >The base address is not correct, and there's uncertainty on whether > >this is this particular controller or not. Did you even test this? > > Tested by axp20x-pek. Still, the base address is wrong, which is yet another hint that this is not the same interrupt controller, and just works by accident. Maxime
在 2017-04-20 13:58，Maxime Ripard 写道： > On Tue, Apr 18, 2017 at 06:56:43PM +0800, Icenowy Zheng wrote: >> >> >> 于 2017年4月18日 GMT+08:00 下午3:00:16, Maxime Ripard >> <maxime.ripard@free-electrons.com> 写到: >> >On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote: >> >> Allwinner A64 SoC features a NMI controller, which is usually >> >connected >> >> to the AXP PMIC. >> >> >> >> Add support for it. >> >> >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> >> Acked-by: Chen-Yu Tsai <wens@csie.org> >> >> --- >> >> Changes in v2: >> >> - Added Chen-Yu's ACK. >> >> >> >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++ >> >> 1 file changed, 8 insertions(+) >> >> >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> index 05ec9fc5e81f..53c18ca372ea 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> @@ -403,6 +403,14 @@ >> >> <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; >> >> }; >> >> >> >> + nmi_intc: interrupt-controller@01f00c0c { >> >> + compatible = "allwinner,sun6i-a31-sc-nmi"; >> >> + interrupt-controller; >> >> + #interrupt-cells = <2>; >> >> + reg = <0x01f00c0c 0x38>; >> > >> >The base address is not correct, and there's uncertainty on whether >> >this is this particular controller or not. Did you even test this? >> >> Tested by axp20x-pek. > > Still, the base address is wrong, which is yet another hint that this > is not the same interrupt controller, and just works by accident. No, it's the same as other post-sun6i device trees. See other post-sun6i device trees: (or maybe they're all wrong, but as we have no document for it, we should temporarily keep them) sun6i-a31.dtsi ``` nmi_intc: interrupt-controller@01f00c0c { compatible = "allwinner,sun6i-a31-sc-nmi"; interrupt-controller; #interrupt-cells = <2>; reg = <0x01f00c0c 0x38>; interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; }; ``` sun8i-a23-a33.dtsi ``` nmi_intc: interrupt-controller@01f00c0c { compatible = "allwinner,sun6i-a31-sc-nmi"; interrupt-controller; #interrupt-cells = <2>; reg = <0x01f00c0c 0x38>; interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; }; ``` But according to the BSP device tree, the base address should be 0x01f00c00. Should I send some patch to fix all of them? (but it will break device tree compatibility) > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
On Thu, Apr 20, 2017 at 03:03:38PM +0800, icenowy@aosc.io wrote: > 在 2017-04-20 13:58，Maxime Ripard 写道： > > On Tue, Apr 18, 2017 at 06:56:43PM +0800, Icenowy Zheng wrote: > > > > > > > > > 于 2017年4月18日 GMT+08:00 下午3:00:16, Maxime Ripard > > > <maxime.ripard@free-electrons.com> 写到: > > > >On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote: > > > >> Allwinner A64 SoC features a NMI controller, which is usually > > > >connected > > > >> to the AXP PMIC. > > > >> > > > >> Add support for it. > > > >> > > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > >> Acked-by: Chen-Yu Tsai <wens@csie.org> > > > >> --- > > > >> Changes in v2: > > > >> - Added Chen-Yu's ACK. > > > >> > > > >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++ > > > >> 1 file changed, 8 insertions(+) > > > >> > > > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > > >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > > >> index 05ec9fc5e81f..53c18ca372ea 100644 > > > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > > > >> @@ -403,6 +403,14 @@ > > > >> <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; > > > >> }; > > > >> > > > >> + nmi_intc: interrupt-controller@01f00c0c { > > > >> + compatible = "allwinner,sun6i-a31-sc-nmi"; > > > >> + interrupt-controller; > > > >> + #interrupt-cells = <2>; > > > >> + reg = <0x01f00c0c 0x38>; > > > > > > > >The base address is not correct, and there's uncertainty on whether > > > >this is this particular controller or not. Did you even test this? > > > > > > Tested by axp20x-pek. > > > > Still, the base address is wrong, which is yet another hint that this > > is not the same interrupt controller, and just works by accident. > > No, it's the same as other post-sun6i device trees. > See other post-sun6i device trees: (or maybe they're all wrong, but > as we have no document for it, we should temporarily keep them) > > sun6i-a31.dtsi > ``` > nmi_intc: interrupt-controller@01f00c0c { > compatible = "allwinner,sun6i-a31-sc-nmi"; > interrupt-controller; > #interrupt-cells = <2>; > reg = <0x01f00c0c 0x38>; > interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; > }; > ``` > > sun8i-a23-a33.dtsi > ``` > nmi_intc: interrupt-controller@01f00c0c { > compatible = "allwinner,sun6i-a31-sc-nmi"; > interrupt-controller; > #interrupt-cells = <2>; > reg = <0x01f00c0c 0x38>; > interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; > }; > ``` > > But according to the BSP device tree, the base address should be > 0x01f00c00. Should I send some patch to fix all of them? (but it will > break device tree compatibility) I'm really not a big fan of "if we see something that is broken, just let it rot" to be honest. We have no idea how this controller works exactly, just like we have no idea if it is exactly the same controller or not. The only thing we have today is the memory map, and it tells us that it has more registers than what you express here. Because of the DT backward compatibility, you have to think of it the other way around: what will happen if it turns out we need to setup any register outside of that region you described in the DT, in something like a year or so? We can't, really. While if you have the full memory region from the beginning, then you just have to add a single writel in your driver. Maxime
在 2017-04-24 15:17，Maxime Ripard 写道： > On Thu, Apr 20, 2017 at 03:03:38PM +0800, icenowy@aosc.io wrote: >> 在 2017-04-20 13:58，Maxime Ripard 写道： >> > On Tue, Apr 18, 2017 at 06:56:43PM +0800, Icenowy Zheng wrote: >> > > >> > > >> > > 于 2017年4月18日 GMT+08:00 下午3:00:16, Maxime Ripard >> > > <maxime.ripard@free-electrons.com> 写到: >> > > >On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote: >> > > >> Allwinner A64 SoC features a NMI controller, which is usually >> > > >connected >> > > >> to the AXP PMIC. >> > > >> >> > > >> Add support for it. >> > > >> >> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> > > >> Acked-by: Chen-Yu Tsai <wens@csie.org> >> > > >> --- >> > > >> Changes in v2: >> > > >> - Added Chen-Yu's ACK. >> > > >> >> > > >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++ >> > > >> 1 file changed, 8 insertions(+) >> > > >> >> > > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> > > >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> > > >> index 05ec9fc5e81f..53c18ca372ea 100644 >> > > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> > > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> > > >> @@ -403,6 +403,14 @@ >> > > >> <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; >> > > >> }; >> > > >> >> > > >> + nmi_intc: interrupt-controller@01f00c0c { >> > > >> + compatible = "allwinner,sun6i-a31-sc-nmi"; >> > > >> + interrupt-controller; >> > > >> + #interrupt-cells = <2>; >> > > >> + reg = <0x01f00c0c 0x38>; >> > > > >> > > >The base address is not correct, and there's uncertainty on whether >> > > >this is this particular controller or not. Did you even test this? >> > > >> > > Tested by axp20x-pek. >> > >> > Still, the base address is wrong, which is yet another hint that this >> > is not the same interrupt controller, and just works by accident. >> >> No, it's the same as other post-sun6i device trees. >> See other post-sun6i device trees: (or maybe they're all wrong, but >> as we have no document for it, we should temporarily keep them) >> >> sun6i-a31.dtsi >> ``` >> nmi_intc: interrupt-controller@01f00c0c { >> compatible = "allwinner,sun6i-a31-sc-nmi"; >> interrupt-controller; >> #interrupt-cells = <2>; >> reg = <0x01f00c0c 0x38>; >> interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; >> }; >> ``` >> >> sun8i-a23-a33.dtsi >> ``` >> nmi_intc: interrupt-controller@01f00c0c { >> compatible = "allwinner,sun6i-a31-sc-nmi"; >> interrupt-controller; >> #interrupt-cells = <2>; >> reg = <0x01f00c0c 0x38>; >> interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; >> }; >> ``` >> >> But according to the BSP device tree, the base address should be >> 0x01f00c00. Should I send some patch to fix all of them? (but it will >> break device tree compatibility) > > I'm really not a big fan of "if we see something that is broken, just > let it rot" to be honest. > > We have no idea how this controller works exactly, just like we have > no idea if it is exactly the same controller or not. > > The only thing we have today is the memory map, and it tells us that > it has more registers than what you express here. > > Because of the DT backward compatibility, you have to think of it the > other way around: what will happen if it turns out we need to setup > any register outside of that region you described in the DT, in > something like a year or so? > > We can't, really. While if you have the full memory region from the > beginning, then you just have to add a single writel in your driver. So things are now already broken, and we may need to fix also A31 and A23/33. How should we do this? > > Maxime > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 05ec9fc5e81f..53c18ca372ea 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -403,6 +403,14 @@ <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>; }; + nmi_intc: interrupt-controller@01f00c0c { + compatible = "allwinner,sun6i-a31-sc-nmi"; + interrupt-controller; + #interrupt-cells = <2>; + reg = <0x01f00c0c 0x38>; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; + }; + r_ccu: clock@1f01400 { compatible = "allwinner,sun50i-a64-r-ccu"; reg = <0x01f01400 0x100>;