| Message ID | 20230807074043.31288-2-zhuyinbo@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: loongson: add firmware offset parse support | expand |
On 09/08/2023 09:28, Yinbo Zhu wrote: >> >>> - gpio-ranges >>> - interrupts >>> >>> @@ -49,11 +82,16 @@ examples: >>> #include <dt-bindings/interrupt-controller/irq.h> >>> >>> gpio0: gpio@1fe00500 { >>> - compatible = "loongson,ls2k-gpio"; >>> + compatible = "loongson,ls2k1000-gpio"; >>> reg = <0x1fe00500 0x38>; >>> ngpios = <64>; >>> #gpio-cells = <2>; >>> gpio-controller; >>> + loongson,gpio-conf-offset = <0>; >>> + loongson,gpio-in-offset = <0x20>; >>> + loongson,gpio-out-offset = <0x10>; >>> + loongson,gpio-ctrl-mode = <0>; >>> + loongson,gpio-inten-offset = <0x30>; >> >> I still think that you just embed the programming model into properties, >> instead of using dedicated compatible for different blocks. It could be >> fine, although I would prefer to check it with your DTS > > Okay, I got it, and if I understand correctly, you seem to agree with > me adding attributes like this. > > And, if using this method that programming model into dts properites, > then when adding a new platform's GPIO, there is no longer a need to > modify the driver because gpio controller is compatible and different > platform can use a same compatible. Uhu, so there we are. You use this method now to avoid new compatibles. No, therefore I do not agree. > >> >> Where is your DTS? > > > Sorry, the dts containing gpio nodes are only available in the product > code and have not been sent to the community yet. Does not help to convince us, but it is your right. With this and above explanation, my answer is no - NAK. Best regards, Krzysztof
On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote: > 在 2023/8/8 下午8:05, Conor Dooley 写道: > > On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote: > > > + loongson,gpio-ctrl-mode: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: > > > + This option indicate this GPIO control mode, where '0' represents > > > + bit control mode and '1' represents byte control mode. > > > > How is one supposed to know which of these modes to use? > > > Byte mode is to access by byte, such as gpio3, the base address of the > gpio controller is offset by 3 bytes as the access address of gpio3. > > The bit mode is the normal mode that like other platform gpio and it is > to access by bit. > > If both modes are supported, it is recommended to prioritize using byte > mode that according to spec. So, sounds like this property should instead be a boolean that notes whether the hardware supports the mode or not, rather than the current enum used to determine software policy. However, from Krzysztof's comments & my own feeling, it really does seem like you should drop the do-everything compatible and introduce things that are soc-specific.
在 2023/8/9 下午9:00, Krzysztof Kozlowski 写道: > On 09/08/2023 09:28, Yinbo Zhu wrote: >>> >>>> - gpio-ranges >>>> - interrupts >>>> >>>> @@ -49,11 +82,16 @@ examples: >>>> #include <dt-bindings/interrupt-controller/irq.h> >>>> >>>> gpio0: gpio@1fe00500 { >>>> - compatible = "loongson,ls2k-gpio"; >>>> + compatible = "loongson,ls2k1000-gpio"; >>>> reg = <0x1fe00500 0x38>; >>>> ngpios = <64>; >>>> #gpio-cells = <2>; >>>> gpio-controller; >>>> + loongson,gpio-conf-offset = <0>; >>>> + loongson,gpio-in-offset = <0x20>; >>>> + loongson,gpio-out-offset = <0x10>; >>>> + loongson,gpio-ctrl-mode = <0>; >>>> + loongson,gpio-inten-offset = <0x30>; >>> >>> I still think that you just embed the programming model into properties, >>> instead of using dedicated compatible for different blocks. It could be >>> fine, although I would prefer to check it with your DTS >> >> Okay, I got it, and if I understand correctly, you seem to agree with >> me adding attributes like this. >> >> And, if using this method that programming model into dts properites, >> then when adding a new platform's GPIO, there is no longer a need to >> modify the driver because gpio controller is compatible and different >> platform can use a same compatible. > > Uhu, so there we are. You use this method now to avoid new compatibles. > No, therefore I do not agree. I don't seem to got it, if the GPIO controllers of two platforms are compatible, shouldn't they use the same compatible? > >> >>> >>> Where is your DTS? >> >> >> Sorry, the dts containing gpio nodes are only available in the product >> code and have not been sent to the community yet. > > Does not help to convince us, but it is your right. With this and above > explanation, my answer is no - NAK. The community work for DTS on the 2K platform is still ongoing. Do I need to add a GPIO DTS node based on the following DTS to request your review? so that you can more conveniently review whether my patch is suitable. 2k1000 https://lore.kernel.org/all/99bdbfc66604b4700e3e22e28c3d27ef7c9c9af7.1686882123.git.zhoubinbin@loongson.cn/ 2k500 https://lore.kernel.org/all/c7087046a725e7a2cfde788185112c150e216f1b.1686882123.git.zhoubinbin@loongson.cn/ 2k2000 https://lore.kernel.org/all/977009099c38177c384fca5a0ee77ebbe50e3ea2.1686882123.git.zhoubinbin@loongson.cn/ Thanks, Yinbo
On 10/08/2023 05:35, Yinbo Zhu wrote: > > > 在 2023/8/9 下午9:00, Krzysztof Kozlowski 写道: >> On 09/08/2023 09:28, Yinbo Zhu wrote: >>>> >>>>> - gpio-ranges >>>>> - interrupts >>>>> >>>>> @@ -49,11 +82,16 @@ examples: >>>>> #include <dt-bindings/interrupt-controller/irq.h> >>>>> >>>>> gpio0: gpio@1fe00500 { >>>>> - compatible = "loongson,ls2k-gpio"; >>>>> + compatible = "loongson,ls2k1000-gpio"; >>>>> reg = <0x1fe00500 0x38>; >>>>> ngpios = <64>; >>>>> #gpio-cells = <2>; >>>>> gpio-controller; >>>>> + loongson,gpio-conf-offset = <0>; >>>>> + loongson,gpio-in-offset = <0x20>; >>>>> + loongson,gpio-out-offset = <0x10>; >>>>> + loongson,gpio-ctrl-mode = <0>; >>>>> + loongson,gpio-inten-offset = <0x30>; >>>> >>>> I still think that you just embed the programming model into properties, >>>> instead of using dedicated compatible for different blocks. It could be >>>> fine, although I would prefer to check it with your DTS >>> >>> Okay, I got it, and if I understand correctly, you seem to agree with >>> me adding attributes like this. >>> >>> And, if using this method that programming model into dts properites, >>> then when adding a new platform's GPIO, there is no longer a need to >>> modify the driver because gpio controller is compatible and different >>> platform can use a same compatible. >> >> Uhu, so there we are. You use this method now to avoid new compatibles. >> No, therefore I do not agree. > > > I don't seem to got it, if the GPIO controllers of two platforms are > compatible, shouldn't they use the same compatible? They can use the same fallback compatible, but you should have specific compatible anyway. However they are not compatible, because programming model is different.- > >> >>> >>>> >>>> Where is your DTS? >>> >>> >>> Sorry, the dts containing gpio nodes are only available in the product >>> code and have not been sent to the community yet. >> >> Does not help to convince us, but it is your right. With this and above >> explanation, my answer is no - NAK. > > > The community work for DTS on the 2K platform is still ongoing. Do I > need to add a GPIO DTS node based on the following DTS to request your > review? so that you can more conveniently review whether my patch is > suitable. > > 2k1000 > https://lore.kernel.org/all/99bdbfc66604b4700e3e22e28c3d27ef7c9c9af7.1686882123.git.zhoubinbin@loongson.cn/ > > 2k500 > https://lore.kernel.org/all/c7087046a725e7a2cfde788185112c150e216f1b.1686882123.git.zhoubinbin@loongson.cn/ > > 2k2000 > https://lore.kernel.org/all/977009099c38177c384fca5a0ee77ebbe50e3ea2.1686882123.git.zhoubinbin@loongson.cn/ If you want to convince us that your properties makes sense, adding GPIO nodes there would be helpful. Best regards, Krzysztof
On Thu, Aug 10, 2023 at 8:19 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote: > > > > 在 2023/8/9 下午11:39, Conor Dooley 写道: > > On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote: > >> 在 2023/8/8 下午8:05, Conor Dooley 写道: > >>> On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote: > > > >>>> + loongson,gpio-ctrl-mode: > >>>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>>> + description: > >>>> + This option indicate this GPIO control mode, where '0' represents > >>>> + bit control mode and '1' represents byte control mode. > >>> > >>> How is one supposed to know which of these modes to use? > >> > >> > >> Byte mode is to access by byte, such as gpio3, the base address of the > >> gpio controller is offset by 3 bytes as the access address of gpio3. > >> > >> The bit mode is the normal mode that like other platform gpio and it is > >> to access by bit. > >> > >> If both modes are supported, it is recommended to prioritize using byte > >> mode that according to spec. > > > > So, sounds like this property should instead be a boolean that notes > > whether the hardware supports the mode or not, rather than the current > > enum used to determine software policy. > > > okay, I got it, I will use boolean, > Why do you want to put it into device-tree so badly? This is not the first driver that would have of_match_data for different variants where you can have a structure that would keep offsets for different models. It's not like you will have hundreds of "compatible" chips anyway, most likely just a few? Bart
在 2023/8/11 下午10:25, Bartosz Golaszewski 写道: > On Thu, Aug 10, 2023 at 8:19 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote: >> >> >> >> 在 2023/8/9 下午11:39, Conor Dooley 写道: >>> On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote: >>>> 在 2023/8/8 下午8:05, Conor Dooley 写道: >>>>> On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote: >>> >>>>>> + loongson,gpio-ctrl-mode: >>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>> + description: >>>>>> + This option indicate this GPIO control mode, where '0' represents >>>>>> + bit control mode and '1' represents byte control mode. >>>>> >>>>> How is one supposed to know which of these modes to use? >>>> >>>> >>>> Byte mode is to access by byte, such as gpio3, the base address of the >>>> gpio controller is offset by 3 bytes as the access address of gpio3. >>>> >>>> The bit mode is the normal mode that like other platform gpio and it is >>>> to access by bit. >>>> >>>> If both modes are supported, it is recommended to prioritize using byte >>>> mode that according to spec. >>> >>> So, sounds like this property should instead be a boolean that notes >>> whether the hardware supports the mode or not, rather than the current >>> enum used to determine software policy. >> >> >> okay, I got it, I will use boolean, >> > > Why do you want to put it into device-tree so badly? This is not the > first driver that would have of_match_data for different variants > where you can have a structure that would keep offsets for different > models. It's not like you will have hundreds of "compatible" chips > anyway, most likely just a few? Using this ways that put offset property into device-tree that can be compatible with future GPIO chips without the need to modify drivers, such as more 2K chips in the future, but use of_match_data and data field of_device_id, which every time a new SoC is released, the GPIO driver needs to be modified once, which is not friendly to us. Thanks, Yinbo
On 14/08/2023 05:39, Yinbo Zhu wrote: > > > 在 2023/8/11 下午10:25, Bartosz Golaszewski 写道: >> On Thu, Aug 10, 2023 at 8:19 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote: >>> >>> >>> >>> 在 2023/8/9 下午11:39, Conor Dooley 写道: >>>> On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote: >>>>> 在 2023/8/8 下午8:05, Conor Dooley 写道: >>>>>> On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote: >>>> >>>>>>> + loongson,gpio-ctrl-mode: >>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>> + description: >>>>>>> + This option indicate this GPIO control mode, where '0' represents >>>>>>> + bit control mode and '1' represents byte control mode. >>>>>> >>>>>> How is one supposed to know which of these modes to use? >>>>> >>>>> >>>>> Byte mode is to access by byte, such as gpio3, the base address of the >>>>> gpio controller is offset by 3 bytes as the access address of gpio3. >>>>> >>>>> The bit mode is the normal mode that like other platform gpio and it is >>>>> to access by bit. >>>>> >>>>> If both modes are supported, it is recommended to prioritize using byte >>>>> mode that according to spec. >>>> >>>> So, sounds like this property should instead be a boolean that notes >>>> whether the hardware supports the mode or not, rather than the current >>>> enum used to determine software policy. >>> >>> >>> okay, I got it, I will use boolean, >>> >> >> Why do you want to put it into device-tree so badly? This is not the >> first driver that would have of_match_data for different variants >> where you can have a structure that would keep offsets for different >> models. It's not like you will have hundreds of "compatible" chips >> anyway, most likely just a few? > > > Using this ways that put offset property into device-tree that can be > compatible with future GPIO chips without the need to modify drivers, That's not an argument for putting into DT. > such as more 2K chips in the future, but use of_match_data and data > field of_device_id, which every time a new SoC is released, the GPIO > driver needs to be modified once, which is not friendly to us. Sorry, "friendly" is again hardly an argument what should or should not be in DT. Best regards, Krzysztof
On Mon, Aug 14, 2023 at 5:39 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote: > > Why do you want to put it into device-tree so badly? This is not the > > first driver that would have of_match_data for different variants > > where you can have a structure that would keep offsets for different > > models. It's not like you will have hundreds of "compatible" chips > > anyway, most likely just a few? > > Using this ways that put offset property into device-tree that can be > compatible with future GPIO chips without the need to modify drivers, > such as more 2K chips in the future, but use of_match_data and data > field of_device_id, which every time a new SoC is released, the GPIO > driver needs to be modified once, which is not friendly to us. The purpose of device tree is to describe the hardware and to configure it for a target system. The purpose of device tree is not to make driver writing easy or convenient. It often does, but that is not the purpose. These offsets are not relevant to the people that need to author and maintain device trees to support and tailor their system. They are only relevant to driver authors and SoC manufacturers. What about just writing these offsets into the driver and use the compatible match to look them up. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml b/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml index fb86e8ce6349..fc51cf40fccd 100644 --- a/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml +++ b/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml @@ -14,6 +14,7 @@ properties: enum: - loongson,ls2k-gpio - loongson,ls7a-gpio + - loongson,ls2k1000-gpio reg: maxItems: 1 @@ -29,6 +30,33 @@ properties: gpio-ranges: true + loongson,gpio-conf-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This option indicate this GPIO configuration register offset address. + + loongson,gpio-out-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This option indicate this GPIO output register offset address. + + loongson,gpio-in-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This option indicate this GPIO input register offset address. + + loongson,gpio-ctrl-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This option indicate this GPIO control mode, where '0' represents + bit control mode and '1' represents byte control mode. + + loongson,gpio-inten-offset: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This option indicate this GPIO interrupt enable register offset + address. + interrupts: minItems: 1 maxItems: 64 @@ -39,6 +67,11 @@ required: - ngpios - "#gpio-cells" - gpio-controller + - loongson,gpio-conf-offset + - loongson,gpio-in-offset + - loongson,gpio-out-offset + - loongson,gpio-ctrl-mode + - loongson,gpio-inten-offset - gpio-ranges - interrupts @@ -49,11 +82,16 @@ examples: #include <dt-bindings/interrupt-controller/irq.h> gpio0: gpio@1fe00500 { - compatible = "loongson,ls2k-gpio"; + compatible = "loongson,ls2k1000-gpio"; reg = <0x1fe00500 0x38>; ngpios = <64>; #gpio-cells = <2>; gpio-controller; + loongson,gpio-conf-offset = <0>; + loongson,gpio-in-offset = <0x20>; + loongson,gpio-out-offset = <0x10>; + loongson,gpio-ctrl-mode = <0>; + loongson,gpio-inten-offset = <0x30>; gpio-ranges = <&pctrl 0 0 15>, <&pctrl 16 16 15>, <&pctrl 32 32 10>,
Loongson GPIO controllers come in multiple variants that are compatible except for certain register offset values. Add support in yaml file for device properties allowing to specify them in DT. Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> --- .../bindings/gpio/loongson,ls-gpio.yaml | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)