| Message ID | 20230211073357.755893-1-sergio.paracuellos@gmail.com |
|---|---|
| Headers | show |
| Series | watchdog: mt7621-wdt: avoid globals and arch dependencies | expand |
On 2/12/23 00:13, Sergio Paracuellos wrote: > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/02/2023 12:01, Sergio Paracuellos wrote: >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: >>>> >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote: >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: >>>>>> >>>>>> Is this mediatek,sysctl property required after your changes on the >>>>>> watchdog code? >>>>> >>>>> I don't really understand the question :-) Yes, it is. Since we have >>>>> introduced a new phandle in the watchdog node to be able to access the >>>>> reset status register through the 'sysc' syscon node. >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we >>>>> are getting the syscon regmap handler via >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç. >>>> >>>> I believe you need to put mediatek,sysctl under "required:". >>> >>> Ah, I understood your question now :-). You meant 'required' property. >>> I need more coffee, I guess :-). I am not sure if you can add >>> properties as required after bindings are already mainlined for >>> compatibility issues. The problem with this SoC is that drivers become >>> mainlined before the device tree was so if things are properly fixed >>> now this kind of issues appear. Let's see Krzysztof and Rob comments >>> for this. >> >> If your driver fails to probe without mediatek,sysctl, you already made >> it required (thus broke the ABI) regardless what dt-binding is saying. >> In such case you should update dt-binding to reflect reality. >> >> Now ABI break is different case. Usually you should not break it without >> valid reasons (e.g. it was never working before). Your commit msg >> suggests that you only improve the code, thus ABI break is not really >> justified. In such case - binding is correct, driver should be reworked >> to accept DTS without the new property. > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need > to add this property required (as Arinc was properly pointing out in > previous mail) since without it the driver is going to fail on probe > (PATCH 5 of the series). I understand the "it was never working > before" argument reason for ABI breaks. What happens if the old driver > code was not ideal and totally dependent on architecture specific > operations when this could be totally avoided and properly make arch > independent agnostic drivers? This driver was added in 2016 [0]. There > was not a device tree file in the kernel for this SoC mainlined until > 2022 [1]. I also personally migrated this watchdog binding in 2022 > from text to YAML and maintained it without changes [2]. When this was > mainlined not all drivers were properly reviewed and the current code > was just maintained as it is. Most users of this SoC are in the > openWRT community where the dtsi of the mainline is not used yet and > they maintain their own mt7621.dtsi files. Also, when a new version of > the openWRT selected kernel is added they also modify and align with > its mt7621.dtsi file without maintaining previous dtb's. If "make the > driver arch independent to be able to be compile tested" and this kind > of arguments are not valid at all I need to know because I have > started to review driver code for this SoC and other drivers also have > the same arch dependency that ideally should be avoided in the same > way. This at the end means to break the ABI again in the future for > those drivers / bindings. So I can just let them be as it is and not > provide any change at all and continue without being compile tested > and other beneficial features to detect future driver breakage. > Problem is that there are (presumably) shipped systems out there with the old devicetree file. The watchdog driver would no longer instantiate on those systems. Guenter
On 12/02/2023 09:13, Sergio Paracuellos wrote: > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 11/02/2023 12:01, Sergio Paracuellos wrote: >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: >>>> >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote: >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: >>>>>> >>>>>> Is this mediatek,sysctl property required after your changes on the >>>>>> watchdog code? >>>>> >>>>> I don't really understand the question :-) Yes, it is. Since we have >>>>> introduced a new phandle in the watchdog node to be able to access the >>>>> reset status register through the 'sysc' syscon node. >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we >>>>> are getting the syscon regmap handler via >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç. >>>> >>>> I believe you need to put mediatek,sysctl under "required:". >>> >>> Ah, I understood your question now :-). You meant 'required' property. >>> I need more coffee, I guess :-). I am not sure if you can add >>> properties as required after bindings are already mainlined for >>> compatibility issues. The problem with this SoC is that drivers become >>> mainlined before the device tree was so if things are properly fixed >>> now this kind of issues appear. Let's see Krzysztof and Rob comments >>> for this. >> >> If your driver fails to probe without mediatek,sysctl, you already made >> it required (thus broke the ABI) regardless what dt-binding is saying. >> In such case you should update dt-binding to reflect reality. >> >> Now ABI break is different case. Usually you should not break it without >> valid reasons (e.g. it was never working before). Your commit msg >> suggests that you only improve the code, thus ABI break is not really >> justified. In such case - binding is correct, driver should be reworked >> to accept DTS without the new property. > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need > to add this property required (as Arinc was properly pointing out in > previous mail) since without it the driver is going to fail on probe > (PATCH 5 of the series). I understand the "it was never working > before" argument reason for ABI breaks. What happens if the old driver > code was not ideal and totally dependent on architecture specific > operations when this could be totally avoided and properly make arch > independent agnostic drivers? It's just an improvement and improvements should be incremental and not break ABI. > This driver was added in 2016 [0]. There > was not a device tree file in the kernel for this SoC mainlined until > 2022 [1]. 2022 march was almost a year ago, so there were kernel releases with this ABI. Also, what about all out of tree DTS? What about other operating systems, bootloaders, firmwares etc? Best regards, Krzysztof
On Mon, Feb 13, 2023 at 9:38 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 12/02/2023 09:13, Sergio Paracuellos wrote: > > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 11/02/2023 12:01, Sergio Paracuellos wrote: > >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: > >>>> > >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote: > >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: > >>>>>> > >>>>>> Is this mediatek,sysctl property required after your changes on the > >>>>>> watchdog code? > >>>>> > >>>>> I don't really understand the question :-) Yes, it is. Since we have > >>>>> introduced a new phandle in the watchdog node to be able to access the > >>>>> reset status register through the 'sysc' syscon node. > >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we > >>>>> are getting the syscon regmap handler via > >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç. > >>>> > >>>> I believe you need to put mediatek,sysctl under "required:". > >>> > >>> Ah, I understood your question now :-). You meant 'required' property. > >>> I need more coffee, I guess :-). I am not sure if you can add > >>> properties as required after bindings are already mainlined for > >>> compatibility issues. The problem with this SoC is that drivers become > >>> mainlined before the device tree was so if things are properly fixed > >>> now this kind of issues appear. Let's see Krzysztof and Rob comments > >>> for this. > >> > >> If your driver fails to probe without mediatek,sysctl, you already made > >> it required (thus broke the ABI) regardless what dt-binding is saying. > >> In such case you should update dt-binding to reflect reality. > >> > >> Now ABI break is different case. Usually you should not break it without > >> valid reasons (e.g. it was never working before). Your commit msg > >> suggests that you only improve the code, thus ABI break is not really > >> justified. In such case - binding is correct, driver should be reworked > >> to accept DTS without the new property. > > > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need > > to add this property required (as Arinc was properly pointing out in > > previous mail) since without it the driver is going to fail on probe > > (PATCH 5 of the series). I understand the "it was never working > > before" argument reason for ABI breaks. What happens if the old driver > > code was not ideal and totally dependent on architecture specific > > operations when this could be totally avoided and properly make arch > > independent agnostic drivers? > > It's just an improvement and improvements should be incremental and not > break ABI. Understood. > > > This driver was added in 2016 [0]. There > > was not a device tree file in the kernel for this SoC mainlined until > > 2022 [1]. > > 2022 march was almost a year ago, so there were kernel releases with > this ABI. > > Also, what about all out of tree DTS? What about other operating > systems, bootloaders, firmwares etc? Pretty clear, thanks. So I guess I have to drop all the changes that are breaking ABI and just maintain those that make no real changes in bindings. > > > Best regards, > Krzysztof Thanks, Sergio Paracuellos >
On Mon, Feb 13, 2023 at 09:59:35AM +0100, Sergio Paracuellos wrote: > On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 2/12/23 00:13, Sergio Paracuellos wrote: > > > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski > > > <krzysztof.kozlowski@linaro.org> wrote: > > >> > > >> On 11/02/2023 12:01, Sergio Paracuellos wrote: > > >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: > > >>>> > > >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote: > > >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: > > >>>>>> > > >>>>>> Is this mediatek,sysctl property required after your changes on the > > >>>>>> watchdog code? > > >>>>> > > >>>>> I don't really understand the question :-) Yes, it is. Since we have > > >>>>> introduced a new phandle in the watchdog node to be able to access the > > >>>>> reset status register through the 'sysc' syscon node. > > >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we > > >>>>> are getting the syscon regmap handler via > > >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç. > > >>>> > > >>>> I believe you need to put mediatek,sysctl under "required:". > > >>> > > >>> Ah, I understood your question now :-). You meant 'required' property. > > >>> I need more coffee, I guess :-). I am not sure if you can add > > >>> properties as required after bindings are already mainlined for > > >>> compatibility issues. The problem with this SoC is that drivers become > > >>> mainlined before the device tree was so if things are properly fixed > > >>> now this kind of issues appear. Let's see Krzysztof and Rob comments > > >>> for this. > > >> > > >> If your driver fails to probe without mediatek,sysctl, you already made > > >> it required (thus broke the ABI) regardless what dt-binding is saying. > > >> In such case you should update dt-binding to reflect reality. > > >> > > >> Now ABI break is different case. Usually you should not break it without > > >> valid reasons (e.g. it was never working before). Your commit msg > > >> suggests that you only improve the code, thus ABI break is not really > > >> justified. In such case - binding is correct, driver should be reworked > > >> to accept DTS without the new property. > > > > > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need > > > to add this property required (as Arinc was properly pointing out in > > > previous mail) since without it the driver is going to fail on probe > > > (PATCH 5 of the series). I understand the "it was never working > > > before" argument reason for ABI breaks. What happens if the old driver > > > code was not ideal and totally dependent on architecture specific > > > operations when this could be totally avoided and properly make arch > > > independent agnostic drivers? This driver was added in 2016 [0]. There > > > was not a device tree file in the kernel for this SoC mainlined until > > > 2022 [1]. I also personally migrated this watchdog binding in 2022 > > > from text to YAML and maintained it without changes [2]. When this was > > > mainlined not all drivers were properly reviewed and the current code > > > was just maintained as it is. Most users of this SoC are in the > > > openWRT community where the dtsi of the mainline is not used yet and > > > they maintain their own mt7621.dtsi files. Also, when a new version of > > > the openWRT selected kernel is added they also modify and align with > > > its mt7621.dtsi file without maintaining previous dtb's. If "make the > > > driver arch independent to be able to be compile tested" and this kind > > > of arguments are not valid at all I need to know because I have > > > started to review driver code for this SoC and other drivers also have > > > the same arch dependency that ideally should be avoided in the same > > > way. This at the end means to break the ABI again in the future for > > > those drivers / bindings. So I can just let them be as it is and not > > > provide any change at all and continue without being compile tested > > > and other beneficial features to detect future driver breakage. > > > > > > > Problem is that there are (presumably) shipped systems out there with > > the old devicetree file. The watchdog driver would no longer instantiate > > on those systems. > > Ok, I will maintain only the PATCH that changes the driver to not use > globals and send v5. > Other options might be to search for the "syscon" node name or to search for the "mediatek,mt7621-sysc" compatible. Guenter > > > > Guenter > > > > Thanks, > Sergio Paracuellos
On Mon, Feb 13, 2023 at 8:36 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Feb 13, 2023 at 09:59:35AM +0100, Sergio Paracuellos wrote: > > On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On 2/12/23 00:13, Sergio Paracuellos wrote: > > > > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski > > > > <krzysztof.kozlowski@linaro.org> wrote: > > > >> > > > >> On 11/02/2023 12:01, Sergio Paracuellos wrote: > > > >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: > > > >>>> > > > >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote: > > > >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote: > > > >>>>>> > > > >>>>>> Is this mediatek,sysctl property required after your changes on the > > > >>>>>> watchdog code? > > > >>>>> > > > >>>>> I don't really understand the question :-) Yes, it is. Since we have > > > >>>>> introduced a new phandle in the watchdog node to be able to access the > > > >>>>> reset status register through the 'sysc' syscon node. > > > >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we > > > >>>>> are getting the syscon regmap handler via > > > >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç. > > > >>>> > > > >>>> I believe you need to put mediatek,sysctl under "required:". > > > >>> > > > >>> Ah, I understood your question now :-). You meant 'required' property. > > > >>> I need more coffee, I guess :-). I am not sure if you can add > > > >>> properties as required after bindings are already mainlined for > > > >>> compatibility issues. The problem with this SoC is that drivers become > > > >>> mainlined before the device tree was so if things are properly fixed > > > >>> now this kind of issues appear. Let's see Krzysztof and Rob comments > > > >>> for this. > > > >> > > > >> If your driver fails to probe without mediatek,sysctl, you already made > > > >> it required (thus broke the ABI) regardless what dt-binding is saying. > > > >> In such case you should update dt-binding to reflect reality. > > > >> > > > >> Now ABI break is different case. Usually you should not break it without > > > >> valid reasons (e.g. it was never working before). Your commit msg > > > >> suggests that you only improve the code, thus ABI break is not really > > > >> justified. In such case - binding is correct, driver should be reworked > > > >> to accept DTS without the new property. > > > > > > > > Thanks for clarification, Krzysztof. Ok, so if this is the case I need > > > > to add this property required (as Arinc was properly pointing out in > > > > previous mail) since without it the driver is going to fail on probe > > > > (PATCH 5 of the series). I understand the "it was never working > > > > before" argument reason for ABI breaks. What happens if the old driver > > > > code was not ideal and totally dependent on architecture specific > > > > operations when this could be totally avoided and properly make arch > > > > independent agnostic drivers? This driver was added in 2016 [0]. There > > > > was not a device tree file in the kernel for this SoC mainlined until > > > > 2022 [1]. I also personally migrated this watchdog binding in 2022 > > > > from text to YAML and maintained it without changes [2]. When this was > > > > mainlined not all drivers were properly reviewed and the current code > > > > was just maintained as it is. Most users of this SoC are in the > > > > openWRT community where the dtsi of the mainline is not used yet and > > > > they maintain their own mt7621.dtsi files. Also, when a new version of > > > > the openWRT selected kernel is added they also modify and align with > > > > its mt7621.dtsi file without maintaining previous dtb's. If "make the > > > > driver arch independent to be able to be compile tested" and this kind > > > > of arguments are not valid at all I need to know because I have > > > > started to review driver code for this SoC and other drivers also have > > > > the same arch dependency that ideally should be avoided in the same > > > > way. This at the end means to break the ABI again in the future for > > > > those drivers / bindings. So I can just let them be as it is and not > > > > provide any change at all and continue without being compile tested > > > > and other beneficial features to detect future driver breakage. > > > > > > > > > > Problem is that there are (presumably) shipped systems out there with > > > the old devicetree file. The watchdog driver would no longer instantiate > > > on those systems. > > > > Ok, I will maintain only the PATCH that changes the driver to not use > > globals and send v5. > > > > Other options might be to search for the "syscon" node name or to search > for the "mediatek,mt7621-sysc" compatible. Thanks for the hint. I didn't know about 'syscon_regmap_lookup_by_compatible()'. I will use this to avoid DTB ABI breakage and allow the driver to be selected for COMPILE_TEST.. Thanks, Guenter. Best regards, Sergio Paracuellos > > Guenter > > > > > > > Guenter > > > > > > > Thanks, > > Sergio Paracuellos
Hi all, This series make an update in the MT7621 SoC's watchdog driver code. This SoC already provides a system controller node to access reset status register needed for the watchdog. Instead of using MIPS architecture dependent operations in header 'asm/mach-ralink/ralink_regs.h' use a phandle to the system controller node and use it through regmap APIs from the code. Driver is also using some globals that are not needed at all if a driver data structure is used along the code. Hence, add all new needed stuff inside a new driver data structure. With this changes driver can be properly compile tested. Thanks in advance for reviewing this! v1 of this series here [0]. v2 os thise series here [1]. v3 os thise series here [2]. Cahnges in v4: - Add a patch to fix a watchdog node warning with 'make dtbs_check' because of a wrong node name. - Collect Guenter 'Reviewed-by' tags for watchdog driver code. - Add a missing 'COMPILE_TEST' to Kconfig which was lost when driver code was split in two patches in v2. Changes in v3: - rename phandler from 'ralink,sysctl' into 'mediatek,sysctl'. - Drop error message added in PATCH 3 that modifies functionality and we only want to maintain current functionaloty by now. Changes in v2: - Remove no needed compatible 'syscon' from bindings. - Rewrite new syscon phandle description in bindings. - Remove 'syscon' from compatible in 'mt7621.dtsi'. - Split PATCH 3 into two different patches: - PATCH 3: removes static globals using a driver data structure. - PATCH 4: remove ralink architecture dependent includes and code. Best regards, Sergio Paracuellos [0]: https://lore.kernel.org/linux-watchdog/20230210065621.598120-1-sergio.paracuellos@gmail.com/T/#t [1]: https://lore.kernel.org/linux-watchdog/20230210121735.639089-1-sergio.paracuellos@gmail.com/T/#t [2]: https://lore.kernel.org/linux-watchdog/20230210173841.705783-1-sergio.paracuellos@gmail.com/T/#t Sergio Paracuellos (5): dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers mips: dts: ralink: mt7621: add phandle to system controller node for watchdog mips: dts: ralink: mt7621: rename watchdog node from 'wdt' into 'watchdog' watchdog: mt7621-wdt: avoid static global declarations watchdog: mt7621-wdt: avoid ralink architecture dependent code .../watchdog/mediatek,mt7621-wdt.yaml | 7 ++ arch/mips/boot/dts/ralink/mt7621.dtsi | 3 +- drivers/watchdog/Kconfig | 4 +- drivers/watchdog/mt7621_wdt.c | 119 ++++++++++++------ 4 files changed, 90 insertions(+), 43 deletions(-)