Message-ID: <6ec6d8ed-c9c2-f020-86dc-4a9be6d47718@loongson.cn>
Date:   Wed, 16 Nov 2022 11:31:30 +0800
From:   Yinbo Zhu <zhuyinbo@...ngson.cn>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     WANG Xuerui <kernel@...0n.name>,
        Linus Walleij <linus.walleij@...aro.org>, zhuyinbo@...ngson.cn,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Juxin Gao <gaojuxin@...ngson.cn>,
        Bibo Mao <maobibo@...ngson.cn>,
        Yanteng Si <siyanteng@...ngson.cn>, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        loongarch@...ts.linux.dev, linux-mips@...r.kernel.org,
        Arnaud Patard <apatard@...driva.com>,
        Huacai Chen <chenhuacai@...nel.org>,
        lvjianmin <lvjianmin@...ngson.cn>,
        zhanghongchen <zhanghongchen@...ngson.cn>,
        Liu Peibao <liupeibao@...ngson.cn>
Subject: Re: [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support



在 2022/11/15 下午9:35, Bartosz Golaszewski 写道:
> On Tue, Nov 15, 2022 at 2:07 PM Yinbo Zhu <zhuyinbo@...ngson.cn> wrote:
>>
>>
>>
>> 在 2022/11/15 下午6:17, WANG Xuerui 写道:
>>> Sorry for jumping in, but...
>>>
>>> On 2022/11/15 17:53, Yinbo Zhu wrote:
>>>>
>>>>
>>>> 在 2022/11/15 下午5:05, Bartosz Golaszewski 写道:
>>>>> On Mon, Nov 14, 2022 at 10:53 AM Yinbo Zhu <zhuyinbo@...ngson.cn> wrote:
>>>>>>
>>>>>> The latest Loongson series platform use dts or acpi framework to
>>>>>> register gpio device resources, such as the Loongson-2 series
>>>>>> SoC of LOONGARCH architecture. In order to support dts, acpi and
>>>>>> compatibility with previous platform device resources in driver,
>>>>>> this patch was added.
>>>>>>
>>>>>> Signed-off-by: lvjianmin <lvjianmin@...ngson.cn>
>>>>>> Signed-off-by: zhanghongchen <zhanghongchen@...ngson.cn>
>>>>>> Signed-off-by: Liu Peibao <liupeibao@...ngson.cn>
>>>>>> Signed-off-by: Juxin Gao <gaojuxin@...ngson.cn>
>>>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
>>>>>> ---
>>>>>> Change in v2:
>>>>>>                   1. Fixup of_loongson_gpio_get_props and remove the
>>>>>> parse logic about
>>>>>>                      "loongson,conf_offset", "loongson,out_offset",
>>>>>> "loongson,in_offset",
>>>>>>                      "loongson,gpio_base", "loongson,support_irq"
>>>>>> then kernel driver will
>>>>>>                      initial them that depend compatible except
>>>>>> "loongson,gpio_base".
>>>>>>
>>>>>>    arch/loongarch/include/asm/loongson.h         |  13 +
>>>>>>    .../include/asm/mach-loongson2ef/loongson.h   |  12 +
>>>>>>    .../include/asm/mach-loongson64/loongson.h    |  13 +
>>>>>>    drivers/gpio/Kconfig                          |   6 +-
>>>>>>    drivers/gpio/gpio-loongson.c                  | 422
>>>>>> +++++++++++++++---
>>>>>>    5 files changed, 391 insertions(+), 75 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/include/asm/loongson.h
>>>>>> b/arch/loongarch/include/asm/loongson.h
>>>>>> index 00db93edae1b..383fdda155f0 100644
>>>>>> --- a/arch/loongarch/include/asm/loongson.h
>>>>>> +++ b/arch/loongarch/include/asm/loongson.h
>>>>>> @@ -60,6 +60,19 @@ static inline void xconf_writeq(u64 val64,
>>>>>> volatile void __iomem *addr)
>>>>>>           );
>>>>>>    }
>>>>>>
>>>>>> +/* ============== Data structrues =============== */
>>>>>> +
>>>>>> +/* gpio data */
>>>>>> +struct platform_gpio_data {
>>>>>> +       u32 gpio_conf;
>>>>>> +       u32 gpio_out;
>>>>>> +       u32 gpio_in;
>>>>>> +       u32 support_irq;
>>>>>> +       char *label;
>>>>>> +       int gpio_base;
>>>>>> +       int ngpio;
>>>>>> +};
>>>>>
>>>>> This is a terrible name for an exported structure. You would at least
>>>>> need some kind of a namespace prefix. But even then the need to add a
>>>>> platform data structure is very questionable. We've moved past the
>>>>> need for platform data in the kernel. I don't see anyone setting it up
>>>>> in this series either. Could you provide more explanation on why you
>>>>> would need it and who would use it?
>>>> okay, I will add a namespace prefix, about this platform data was added
>>>> that was to compatible with legacy platforms that do not support dts or
>>>> acpi, then, the mips loongson platform or loongarch loongson platform
>>>
>>> Why are you trying to support "legacy" LoongArch platforms when the
>>> architecture was just upstreamed *this year*?
>> the leagacy gpio driver had support LoongArch, and you can find some
>> gpio register defined in arch/loongarch/include
>> /asm/loongson.h in legacy gpio driver, such as LOONGSON_GPIODATA,
>> The legacy gpio driver is the driver that doesn't include my gpio patch.
>>>
>>>> can implement the gpio device driver to initialize the
>>>> platform_gpio_data structure as needed after exporting the structure.
>>>>>
>>>>>> +
>>>>>>    /* ============== LS7A registers =============== */
>>>>>>    #define LS7A_PCH_REG_BASE              0x10000000UL
>>>>>>    /* LPC regs */
>>>>>> diff --git a/arch/mips/include/asm/mach-loongson2ef/loongson.h
>>>>>> b/arch/mips/include/asm/mach-loongson2ef/loongson.h
>>>>>> index ca039b8dcde3..b261cea4fee1 100644
>>>>>> --- a/arch/mips/include/asm/mach-loongson2ef/loongson.h
>>>>>> +++ b/arch/mips/include/asm/mach-loongson2ef/loongson.h
>>>>>> @@ -315,4 +315,16 @@ extern unsigned long _loongson_addrwincfg_base;
>>>>>>
>>>>>>    #endif /* ! CONFIG_CPU_SUPPORTS_ADDRWINCFG */
>>>>>>
>>>>>> +/* ============== Data structrues =============== */
>>>>>> +
>>>>>> +/* gpio data */
>>>>>> +struct platform_gpio_data {
>>>>>> +       u32 gpio_conf;
>>>>>> +       u32 gpio_out;
>>>>>> +       u32 gpio_in;
>>>>>> +       u32 support_irq;
>>>>>> +       char *label;
>>>>>> +       int gpio_base;
>>>>>> +       int ngpio;
>>>>>> +};
>>>>>
>>>>> No idea why you would need to duplicate it like this either. And why
>>>>> put it in arch/.
>>>> because loongson platform include mips and loongarch, and the gpio
>>>> device data was defined in arch/ in leagcy loongson gpio driver.  so the
>>>> latest loongson gpio drvier add platform_gpio_data in same dir.
>>>
>>> I think at this point it's hopefully clear, that the way forward to
>>> supporting Loongson IP blocks shared between MIPS/LoongArch SoCs is to
>>> start over and do things properly, making the code as platform-agnostic
>>> as possible. Just make sure the drivers can get initialized via DT and
>>> ACPI then you're all set -- the upstream kernel is guaranteed to use one
>>> of the two well-established boot flows for every Loongson chip it
>>> supports. Be it hard-coded DT in arch/mips/boot/dts/loongson, or the
>>> LoongArch ACPI/upcoming DT, no need for hard-coding things in arch/ in
>>> either case.
>> Our old platforms are used by customers, but we will not maintain those
>> platforms. Adding dts/acpi support to those old platforms not only makes
>> no sense, but also affects their use. Because the configuration of
>> dts/acpi requires the support of the firmware team, but in fact, we have
>> no one to maintain those old platforms.
>>
>> in a words, My patch to upstream was supposed to consider dts/acpi in
>> LoongArch platform  But I have to consider the original legacy gpio
>> driver and to compatible with it.
> 
> Which legacy driver are you referring to? Neither gpio-loongson nor
> gpio-loongson1 define any platform data. If it wasn't needed before
> then it's sure we won't be adding it in 2022. If you have board-files
> upstream that need to use this driver then you can do fine with
> regular device properties.
in fact, It is my gpio driver need to be compatible with 
gpio-loongson.c, because I need use this name gpio-loongson.c and doesn't
drop this driver.

The legacy driver also uses a set of common definitions, but does not
use the structure to encapsulate it.  then use platform_device_register
to register device, and my driver is to suppot dts/acpi gpio and need
separate platform device register logic, so gpio device need a platform
data to register device. It's confusing that as for some legacy platform
device drvier to set loongson platform device properties that required
by gpio driver, but if supply a platform data for device drvier it will
clear.

in addition, I have a look about include/linux/platform_data/
I think it is appropriate for me to add gpio platform data here

Thanks

BRs,
Yinbo
> 
> Bartosz
> 

Powered by blists - more mailing lists