From: Yinbo Zhu <zhuyinbo@loongson.cn>
To: WANG Xuerui <kernel@xen0n.name>, Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Linus Walleij <linus.walleij@linaro.org>,
zhuyinbo@loongson.cn, Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Jiaxun Yang <jiaxun.yang@flygoat.com>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
Juxin Gao <gaojuxin@loongson.cn>, Bibo Mao <maobibo@loongson.cn>,
Yanteng Si <siyanteng@loongson.cn>,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, loongarch@lists.linux.dev,
linux-mips@vger.kernel.org, Arnaud Patard <apatard@mandriva.com>,
Huacai Chen <chenhuacai@kernel.org>,
lvjianmin <lvjianmin@loongson.cn>,
zhanghongchen <zhanghongchen@loongson.cn>,
Liu Peibao <liupeibao@loongson.cn>
Subject: Re: [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support
Date: Tue, 15 Nov 2022 21:07:05 +0800 [thread overview]
Message-ID: <9a448680-0bb6-c4f0-93d2-29a86fede2d4@loongson.cn> (raw)
In-Reply-To: <fd5cc541-dfc6-d1cf-0865-669b11ce2e7a@xen0n.name>
在 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@loongson.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@loongson.cn>
>>>> Signed-off-by: zhanghongchen <zhanghongchen@loongson.cn>
>>>> Signed-off-by: Liu Peibao <liupeibao@loongson.cn>
>>>> Signed-off-by: Juxin Gao <gaojuxin@loongson.cn>
>>>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.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.
>
>>>
>>> [snip]
>>>
>>> I will hold off reviewing the rest of the patch until we get that
>>> clarified.
>>>
>>> Bartosz
>>>
>>
>
next prev parent reply other threads:[~2022-11-15 13:07 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-14 9:53 [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support Yinbo Zhu
2022-11-14 9:53 ` [PATCH v2 2/2] dt-bindings: gpio: add loongson series gpio Yinbo Zhu
2022-11-18 12:40 ` Krzysztof Kozlowski
2022-11-15 9:05 ` [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support Bartosz Golaszewski
2022-11-15 9:53 ` Yinbo Zhu
2022-11-15 10:17 ` WANG Xuerui
2022-11-15 13:07 ` Yinbo Zhu [this message]
2022-11-15 13:35 ` Bartosz Golaszewski
2022-11-16 3:31 ` Yinbo Zhu
2022-11-15 10:20 ` Thomas Bogendoerfer
2022-11-16 3:34 ` Yinbo Zhu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9a448680-0bb6-c4f0-93d2-29a86fede2d4@loongson.cn \
--to=zhuyinbo@loongson.cn \
--cc=apatard@mandriva.com \
--cc=brgl@bgdev.pl \
--cc=chenhuacai@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gaojuxin@loongson.cn \
--cc=jiaxun.yang@flygoat.com \
--cc=kernel@xen0n.name \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=liupeibao@loongson.cn \
--cc=loongarch@lists.linux.dev \
--cc=lvjianmin@loongson.cn \
--cc=maobibo@loongson.cn \
--cc=robh+dt@kernel.org \
--cc=siyanteng@loongson.cn \
--cc=tsbogend@alpha.franken.de \
--cc=zhanghongchen@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).