Messages in this thread | | | Subject | Re: [PATCH v2 1/2] gpio: loongson: add dts/acpi gpio support | From | Yinbo Zhu <> | Date | Tue, 15 Nov 2022 21:07:05 +0800 |
| |
在 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 >>> >> >
| |