Re: [PATCH] arm64: dts: amlogic: Fix IR Controller register area for Meson-GX/AXG/G12 series SoCs

[Zelong Dong <Zelong.Dong@amlogic.com>]

在 2023/9/29 12:00, Neil Armstrong 写道:
> Hi,
> 
> Le 22/09/2023 à 10:05, zelong dong a écrit :
>> From: Zelong Dong <zelong.dong@amlogic.com>
>>
>> Amloic Meson-6 SoC only has one IR Controller, since then, there are 2 IR
>> Controllers in every SoCs, one is Legacy IR Controller (same as 
>> Meson-6's one),
>> the new one is Multi-Format IR Controller (abbreviated "MF-IR",
>> which supports more than one IR Protocol). MF-IR was updated several 
>> times,
>> so different SoCs could be got different register sizes.
>>
>> There are all IR Controller register areas from upstream's SoCs:
>>                     Legacy IR          MF-IR
>> Meson-6               | <0xc8100480 0x20> |     NULL     |
>> Meson-8/8B/8M2           | <0xc8100480 0x20> | <0xc8100580 0x30> |
>> Meson-GXBB             | <0xc8100480 0x20> | <0xc8100580 0x44> |
>> Meson-GXM/GXL            | <0xc8100480 0x20> | <0xc8100580 0x54> |
>> Meson-AXG/G12A/G12B/SM1           | <0xff808000 0x20> | <0xff808040 0x58> |
>>
>> About Meson-IR driver, MF-IR was supported from Meson-8, which was 
>> distinguished
>> by compatible string (of_device_is_compatible(node, 
>> "amlogic,meson6-ir")),
>> and only one register (macro 'IR_DEC_REG2') was added because 
>> controller worked
>> in raw decoder mode, later registers are unused, so we recommend that 
>> register
>> size should be 0x24 if MF-IR is in use.
>>
>> There are 2 ways to fix.
>>
>> MF-IR is in use:
>>    Meson-8/8B/8M2/GXBB/GXM/GXL: <0xc8100580 0x24>
>>    Meson-AXG/G12A/G12B/SM1:     <0xff808040 0x24>
>>
>> Legacy IR is in use:
>>    Meson-8/8B/8M2/GXBB/GXM/GXL: <0xc8100480 0x20>
>>    Meson-AXG/G12A/G12B/SM1:     <0xff808000 0x20>
> 
> This is slighly confusing, so you mean since Meson-8 there's 2 IR 
> controllers
> which have the same register mapping, and both works with the 
> "amlogic,meson6-ir" compatible ?

They are not the same register mapping but similar.
Compatible "amlogic,meson6-ir" only works with legacy IR, other 
compatibles work with MF-IR.
Please refer to 
https://lore.kernel.org/all/20160820095424.636-5-martin.blumenstingl@googlemail.com/

> 
> So why should we switch to the MF-IR address ? what does it exactly 
> change ?

In raw decoder mode, it's not necessarily to switch to MF-IR, you could 
use "amlogic,meson6-ir" with legacy IR register area.
This patch is for fix the wrong relation between compatible and register 
area. If compatible is "amlogic,meson-gxbb-ir", the register should be 
from MF-IR.

> 
> If we want to be clean:
> - both should be added, legacy IR with current address + reg size change
> - a new compatible should be added for the MF IR, with 
> "amlogic,meson6-ir" as fallback to take in account they are compatible
> - a new node should be added for the MR IR
> - DTs should be switch to the MF IR label instead of the legacy IR
> 
> With this we would be able to take advantage of the MR IF 
> functionalities while keeping driver functionnal
> with old DTs and new DTs with old kernels.

Legacy IR and MF-IR share the same IRQ number, so I recommend that chose 
one to use. Do they both need to be added?
In a word, this patch is not for new function, just fix the wrong 
register addr.
> 
> Thanks,
> Neil
> 
>>
>> Fixes: 2bfe8412c5388a ("arm64: dts: meson-g12a: Add IR nodes")
>> Fixes: 7bd46a79aad549 ("ARM64: dts: meson-axg: enable IR controller")
>> Fixes: c58d77855f0069 ("ARM64: dts: meson-gxbb: Add Infrared Remote 
>> Controller decoder")
>> Link: 
>> https://lore.kernel.org/all/20160820095424.636-5-martin.blumenstingl@googlemail.com/
>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>> ---
>>   arch/arm64/boot/dts/amlogic/meson-axg.dtsi        | 4 ++--
>>   arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 4 ++--
>>   arch/arm64/boot/dts/amlogic/meson-gx.dtsi         | 2 +-
>>   3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi 
>> b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
>> index 768d0ed78dbe..dd8c58e74322 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
>> @@ -1705,9 +1705,9 @@ pwm_AO_ab: pwm@7000 {
>>                               status = "disabled";
>>                       };
>>
>> -                     ir: ir@8000 {
>> +                     ir: ir@8040 {
>>                               compatible = "amlogic,meson-gxbb-ir";
>> -                             reg = <0x0 0x8000 0x0 0x20>;
>> +                             reg = <0x0 0x8040 0x0 0x24>;
>>                               interrupts = <GIC_SPI 196 
>> IRQ_TYPE_EDGE_RISING>;
>>                               status = "disabled";
>>                       };
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi 
>> b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
>> index ff68b911b729..e12cea5fa889 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
>> @@ -2084,9 +2084,9 @@ pwm_AO_ab: pwm@7000 {
>>                               status = "disabled";
>>                       };
>>
>> -                     ir: ir@8000 {
>> +                     ir: ir@8040 {
>>                               compatible = "amlogic,meson-gxbb-ir";
>> -                             reg = <0x0 0x8000 0x0 0x20>;
>> +                             reg = <0x0 0x8040 0x0 0x24>;
>>                               interrupts = <GIC_SPI 196 
>> IRQ_TYPE_EDGE_RISING>;
>>                               status = "disabled";
>>                       };
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi 
>> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> index 2673f0dbafe7..0c04e34a0923 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> @@ -506,7 +506,7 @@ pwm_AO_ab: pwm@550 {
>>
>>                       ir: ir@580 {
>>                               compatible = "amlogic,meson-gx-ir", 
>> "amlogic,meson-gxbb-ir";
>> -                             reg = <0x0 0x00580 0x0 0x40>;
>> +                             reg = <0x0 0x00580 0x0 0x24>;
>>                               interrupts = <GIC_SPI 196 
>> IRQ_TYPE_EDGE_RISING>;
>>                               status = "disabled";
>>                       };
>