| Message ID | 20220608095623.22327-1-tmaimon77@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
On 08/06/2022 11:56, Tomer Maimon wrote: > Add nuvoton,sysgcr syscon property to the reset > node to handle the general control registers. Wrong wrapping. Best regards, Krzysztof
On 08/06/2022 11:56, Tomer Maimon wrote: > Using syscon device tree property instead of > device data to handle the NPCM general control > registers. > Again ignored the comment. > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/reset/reset-npcm.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c > index 2ea4d3136e15..312c3b594b8f 100644 > --- a/drivers/reset/reset-npcm.c > +++ b/drivers/reset/reset-npcm.c > @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev, > } > > static const struct of_device_id npcm_rc_match[] = { > - { .compatible = "nuvoton,npcm750-reset", > - .data = (void *)"nuvoton,npcm750-gcr" }, > + { .compatible = "nuvoton,npcm750-reset"}, > { } > }; > > @@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) > u32 ipsrst1_bits = 0; > u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST; > u32 ipsrst3_bits = 0; > - const char *gcr_dt; > > - gcr_dt = (const char *) > - of_match_device(dev->driver->of_match_table, dev)->data; > - > - gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt); > + gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr"); > if (IS_ERR(gcr_regmap)) { > - dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt); > + dev_err(&pdev->dev, "Failed to find gcr syscon"); > return PTR_ERR(gcr_regmap); Comment still ignored. There is no point in this review if you keep ignoring what we ask to fix. If something is unclear, ask for clarification. Resending without implementing the comment means that you ignore the review which is waste of my time. I am sorry, but this is not acceptable. Best regards, Krzysztof
On 08/06/2022 11:56, Tomer Maimon wrote: > Add binding document and device tree binding > constants for Nuvoton BMC NPCM8XX reset controller. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > .../bindings/reset/nuvoton,npcm-reset.yaml | 13 +- > .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 128 ++++++++++++++++++ > 2 files changed, 140 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h > > diff --git a/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml > index c6bbc1589ab9..93ea81686f58 100644 > --- a/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml > +++ b/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml > @@ -9,9 +9,20 @@ title: Nuvoton NPCM Reset controller > maintainers: > - Tomer Maimon <tmaimon77@gmail.com> > > +description: | > + The NPCM reset controller used to reset various set of peripherals. > + Please refer to reset.txt in this directory for common reset > + controller binding usage. > + > + For list of all valid reset indices see > + <dt-bindings/reset/nuvoton,npcm7xx-reset.h> for Poleg NPCM7XX SoC, > + <dt-bindings/reset/nuvoton,npcm8xx-reset.h> for Arbel NPCM8XX SoC. > + > properties: > compatible: > - const: nuvoton,npcm750-reset > + enum: > + - nuvoton,npcm750-reset # Poleg NPCM7XX SoC > + - nuvoton,npcm845-reset # Arbel NPCM8XX SoC > > reg: > maxItems: 1 > diff --git a/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h > new file mode 100644 > index 000000000000..5b3b74534b50 > --- /dev/null > +++ b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h > @@ -0,0 +1,128 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ Again - ignored comment from v1. > +/* > + * Copyright (c) 2022 Nuvoton Technology corporation. > + * Author: Tomer Maimon <tmaimon77@gmail.com> > + */ > + > +#ifndef _DT_BINDINGS_NPCM8XX_RESET_H > +#define _DT_BINDINGS_NPCM8XX_RESET_H > + > +/* represent reset register offset */ > +#define NPCM8XX_RESET_IPSRST1 0x20 > +#define NPCM8XX_RESET_IPSRST2 0x24 > +#define NPCM8XX_RESET_IPSRST3 0x34 > +#define NPCM8XX_RESET_IPSRST4 0x74 > + > +/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */ Again - ignored comment from v1. My last message was quite clear, wasn't it? https://lore.kernel.org/all/4a69902f-a545-23a1-1430-e5ece16997e9@linaro.org/ You ignored several of previous comments, so: NAK. Best regards, Krzysztof
Hi Krzysztof, Thanks for your comments On Wed, 8 Jun 2022 at 13:07, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 08/06/2022 11:56, Tomer Maimon wrote: > > Add nuvoton,sysgcr syscon property to the reset > > node to handle the general control registers. > > Wrong wrapping. it will be very helpful if you could point me what wrong wrapped in the commit message, is it the explanation or the header? or something else? > > Best regards, > Krzysztof Best regards, Tomer
Hi Krzysztof Sorry but I didn't ignore your comment. For not breaking exciting boards I add the following patch in V2 https://lore.kernel.org/linux-arm-kernel/20220608095623.22327-11-tmaimon77@gmail.com/ On Wed, 8 Jun 2022 at 13:08, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 08/06/2022 11:56, Tomer Maimon wrote: > > Using syscon device tree property instead of > > device data to handle the NPCM general control > > registers. > > > > Again ignored the comment. > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > drivers/reset/reset-npcm.c | 11 +++-------- > > 1 file changed, 3 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c > > index 2ea4d3136e15..312c3b594b8f 100644 > > --- a/drivers/reset/reset-npcm.c > > +++ b/drivers/reset/reset-npcm.c > > @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev, > > } > > > > static const struct of_device_id npcm_rc_match[] = { > > - { .compatible = "nuvoton,npcm750-reset", > > - .data = (void *)"nuvoton,npcm750-gcr" }, > > + { .compatible = "nuvoton,npcm750-reset"}, > > { } > > }; > > > > @@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) > > u32 ipsrst1_bits = 0; > > u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST; > > u32 ipsrst3_bits = 0; > > - const char *gcr_dt; > > > > - gcr_dt = (const char *) > > - of_match_device(dev->driver->of_match_table, dev)->data; > > - > > - gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt); > > + gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr"); > > if (IS_ERR(gcr_regmap)) { > > - dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt); > > + dev_err(&pdev->dev, "Failed to find gcr syscon"); > > return PTR_ERR(gcr_regmap); > > Comment still ignored. > > There is no point in this review if you keep ignoring what we ask to fix. > > If something is unclear, ask for clarification. Resending without > implementing the comment means that you ignore the review which is waste > of my time. > > I am sorry, but this is not acceptable. > > Best regards, > Krzysztof Best regards, Tomer
Hi Krzysztof, Sorry, but I thought the fix is only to add an explanation to the dt-binding file as was done in V2. The NPCM8XX binding is done in the same way as the NPCM7XX and both use the same reset driver and use the same reset method in upstreamed NPCM reset driver. Can you please explain again what you suggest to do? On Wed, 8 Jun 2022 at 13:11, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 08/06/2022 11:56, Tomer Maimon wrote: > > Add binding document and device tree binding > > constants for Nuvoton BMC NPCM8XX reset controller. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > .../bindings/reset/nuvoton,npcm-reset.yaml | 13 +- > > .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 128 ++++++++++++++++++ > > 2 files changed, 140 insertions(+), 1 deletion(-) > > create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h > > > > diff --git a/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml > > index c6bbc1589ab9..93ea81686f58 100644 > > --- a/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml > > +++ b/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.yaml > > @@ -9,9 +9,20 @@ title: Nuvoton NPCM Reset controller > > maintainers: > > - Tomer Maimon <tmaimon77@gmail.com> > > > > +description: | > > + The NPCM reset controller used to reset various set of peripherals. > > + Please refer to reset.txt in this directory for common reset > > + controller binding usage. > > + > > + For list of all valid reset indices see > > + <dt-bindings/reset/nuvoton,npcm7xx-reset.h> for Poleg NPCM7XX SoC, > > + <dt-bindings/reset/nuvoton,npcm8xx-reset.h> for Arbel NPCM8XX SoC. > > + > > properties: > > compatible: > > - const: nuvoton,npcm750-reset > > + enum: > > + - nuvoton,npcm750-reset # Poleg NPCM7XX SoC > > + - nuvoton,npcm845-reset # Arbel NPCM8XX SoC > > > > reg: > > maxItems: 1 > > diff --git a/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h > > new file mode 100644 > > index 000000000000..5b3b74534b50 > > --- /dev/null > > +++ b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h > > @@ -0,0 +1,128 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > Again - ignored comment from v1. > > > +/* > > + * Copyright (c) 2022 Nuvoton Technology corporation. > > + * Author: Tomer Maimon <tmaimon77@gmail.com> > > + */ > > + > > +#ifndef _DT_BINDINGS_NPCM8XX_RESET_H > > +#define _DT_BINDINGS_NPCM8XX_RESET_H > > + > > +/* represent reset register offset */ > > +#define NPCM8XX_RESET_IPSRST1 0x20 > > +#define NPCM8XX_RESET_IPSRST2 0x24 > > +#define NPCM8XX_RESET_IPSRST3 0x34 > > +#define NPCM8XX_RESET_IPSRST4 0x74 > > + > > +/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */ > > Again - ignored comment from v1. My last message was quite clear, wasn't it? > > https://lore.kernel.org/all/4a69902f-a545-23a1-1430-e5ece16997e9@linaro.org/ > > You ignored several of previous comments, so: > > NAK. > > Best regards, > Krzysztof Appreciate your time and comments. Best regards, Tomer
Hi Tomer, On Thu, 9 Jun 2022 at 14:30, Tomer Maimon <tmaimon77@gmail.com> wrote: > > Hi Krzysztof, > > Thanks for your comments > > On Wed, 8 Jun 2022 at 13:07, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 08/06/2022 11:56, Tomer Maimon wrote: > > > Add nuvoton,sysgcr syscon property to the reset > > > node to handle the general control registers. > > > > Wrong wrapping. > it will be very helpful if you could point me what wrong wrapped in > the commit message, is it the explanation or the header? or something > else? The commit message body should be wrapped at 72 chars. You can fit more on the first line if you reflow: Add nuvoton,sysgcr syscon property to the reset node to handle the general control registers. > > > > Best regards, > > Krzysztof > > Best regards, > > Tomer
Hi Benjamin, Thanks a lot for your explanation. will be applied in next patch set Best regards, Tomer On Fri, 10 Jun 2022 at 01:11, Benjamin Fair <benjaminfair@google.com> wrote: > > Hi Tomer, > > On Thu, 9 Jun 2022 at 14:30, Tomer Maimon <tmaimon77@gmail.com> wrote: > > > > Hi Krzysztof, > > > > Thanks for your comments > > > > On Wed, 8 Jun 2022 at 13:07, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > > > > > > On 08/06/2022 11:56, Tomer Maimon wrote: > > > > Add nuvoton,sysgcr syscon property to the reset > > > > node to handle the general control registers. > > > > > > Wrong wrapping. > > it will be very helpful if you could point me what wrong wrapped in > > the commit message, is it the explanation or the header? or something > > else? > > The commit message body should be wrapped at 72 chars. You can fit > more on the first line if you reflow: > > Add nuvoton,sysgcr syscon property to the reset node to handle the > general control registers. > > > > > > > Best regards, > > > Krzysztof > > > > Best regards, > > > > Tomer
On 09/06/2022 23:30, Tomer Maimon wrote: > Hi Krzysztof, > > Thanks for your comments > > On Wed, 8 Jun 2022 at 13:07, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 08/06/2022 11:56, Tomer Maimon wrote: >>> Add nuvoton,sysgcr syscon property to the reset >>> node to handle the general control registers. >> >> Wrong wrapping. > it will be very helpful if you could point me what wrong wrapped in > the commit message, is it the explanation or the header? or something > else? I pointed you last time. I pointed the exact line, exact rule you need to follow. I pointed it three times already and three times I said wrapping is wrong: https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 "The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch." Your wrapping is not at 75 columns and it causes the commit to be less readable, without any reason. Please follow Linux kernel coding style/rules. Best regards, Krzysztof
On 09/06/2022 23:37, Tomer Maimon wrote: > Hi Krzysztof > > Sorry but I didn't ignore your comment. > > For not breaking exciting boards I add the following patch in V2 > https://lore.kernel.org/linux-arm-kernel/20220608095623.22327-11-tmaimon77@gmail.com/ No, it does not solve it. 1. Patchset goes via separate trees (DTS are always separate), so it is not bisectable. One of the branches/trees will have broken DTS. 2. All out of tree DTSes are broken. This is expressed as ABI and - with some reasonable exceptions - you should not break it. https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/ABI.rst You have to keep backwards compatibility, so parse/handle both versions of DTS. Best regards, Krzysztof
On 10/06/2022 00:05, Tomer Maimon wrote: > Hi Krzysztof, > > Sorry, but I thought the fix is only to add an explanation to the > dt-binding file as was done in V2. > > The NPCM8XX binding is done in the same way as the NPCM7XX and both > use the same reset driver and use the same reset method in upstreamed > NPCM reset driver. > > Can you please explain again what you suggest to do? If you want abstract IDs, they must be abstract, so not representing hardware registers. Then they start at 1 and are incremented by 1. Other option is to skip such IDs entirely and use register offsets/addresses directly, like Arnd suggested in linked documents. I think he expressed it clearly, so please read his answers which I linked in previous discussion. There is no single reason to store register addresses/values/offsets as binding headers. These are not bindings. Best regards, Krzysztof
Hi Krzysztof, Thanks for the clarifications, will update the next version. Best regards, Tomer On Fri, 10 Jun 2022 at 12:51, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 09/06/2022 23:30, Tomer Maimon wrote: > > Hi Krzysztof, > > > > Thanks for your comments > > > > On Wed, 8 Jun 2022 at 13:07, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 08/06/2022 11:56, Tomer Maimon wrote: > >>> Add nuvoton,sysgcr syscon property to the reset > >>> node to handle the general control registers. > >> > >> Wrong wrapping. > > it will be very helpful if you could point me what wrong wrapped in > > the commit message, is it the explanation or the header? or something > > else? > > I pointed you last time. I pointed the exact line, exact rule you need > to follow. I pointed it three times already and three times I said > wrapping is wrong: > https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 > > "The body of the explanation, line wrapped at 75 columns, which will be > copied to the permanent changelog to describe this patch." > > Your wrapping is not at 75 columns and it causes the commit to be less > readable, without any reason. Please follow Linux kernel coding style/rules. > > > Best regards, > Krzysztof
Hi Krzysztof, I will make sure to add backward compatibility in the reset driver in the next version. Thanks, Tomer On Fri, 10 Jun 2022 at 12:53, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 09/06/2022 23:37, Tomer Maimon wrote: > > Hi Krzysztof > > > > Sorry but I didn't ignore your comment. > > > > For not breaking exciting boards I add the following patch in V2 > > https://lore.kernel.org/linux-arm-kernel/20220608095623.22327-11-tmaimon77@gmail.com/ > > No, it does not solve it. > 1. Patchset goes via separate trees (DTS are always separate), so it is > not bisectable. One of the branches/trees will have broken DTS. > > 2. All out of tree DTSes are broken. This is expressed as ABI and - with > some reasonable exceptions - you should not break it. > https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/ABI.rst > > You have to keep backwards compatibility, so parse/handle both versions > of DTS. > > > Best regards, > Krzysztof
Hi Krzysztof, Thanks for your clarification. We can remove the dt-binding file and use numbers in the DTS, appreciate if you can answer few additional questions: 1. Do you suggest adding all NPCM reset values to the NPCM reset document or the reset values should describe in the module documentation that uses it? 2. Some of the NPCM7XX document modules describe the reset value they use from the dt-binding for example: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61 If we remove the NPCM8XX dt-binding file should we describe the NPCM8XX values in the NPCM-ADC document file? Best regards, Tomer On Fri, 10 Jun 2022 at 12:55, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 10/06/2022 00:05, Tomer Maimon wrote: > > Hi Krzysztof, > > > > Sorry, but I thought the fix is only to add an explanation to the > > dt-binding file as was done in V2. > > > > The NPCM8XX binding is done in the same way as the NPCM7XX and both > > use the same reset driver and use the same reset method in upstreamed > > NPCM reset driver. > > > > Can you please explain again what you suggest to do? > > If you want abstract IDs, they must be abstract, so not representing > hardware registers. Then they start at 1 and are incremented by 1. > > Other option is to skip such IDs entirely and use register > offsets/addresses directly, like Arnd suggested in linked documents. I > think he expressed it clearly, so please read his answers which I linked > in previous discussion. > > There is no single reason to store register addresses/values/offsets as > binding headers. These are not bindings. > > Best regards, > Krzysztof
On 13/06/2022 02:25, Tomer Maimon wrote: > Hi Krzysztof, > > Thanks for your clarification. > > We can remove the dt-binding file and use numbers in the DTS, > appreciate if you can answer few additional questions: > 1. Do you suggest adding all NPCM reset values to the NPCM reset > document or the reset values should describe in the module > documentation that uses it? What is "NPCM reset document"? Are these reset values anyhow different than interrupts or pins? > 2. Some of the NPCM7XX document modules describe the reset value they > use from the dt-binding for example: > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61 This is NPCM750 > If we remove the NPCM8XX dt-binding file should we describe the > NPCM8XX values in the NPCM-ADC document file? What is NPCM-ADC document file? What do you want to describe there? Again - how is it different than interrupts? Best regards, Krzysztof
Hi Krzysztof, On Wed, 15 Jun 2022 at 20:03, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 13/06/2022 02:25, Tomer Maimon wrote: > > Hi Krzysztof, > > > > Thanks for your clarification. > > > > We can remove the dt-binding file and use numbers in the DTS, > > appreciate if you can answer few additional questions: > > 1. Do you suggest adding all NPCM reset values to the NPCM reset > > document or the reset values should describe in the module > > documentation that uses it? > > What is "NPCM reset document"? Are these reset values anyhow different > than interrupts or pins? No, they represent the same values. > > > 2. Some of the NPCM7XX document modules describe the reset value they > > use from the dt-binding for example: > > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61 > > This is NPCM750 > > > If we remove the NPCM8XX dt-binding file should we describe the > > NPCM8XX values in the NPCM-ADC document file? > > What is NPCM-ADC document file? What do you want to describe there? > Again - how is it different than interrupts? It is not different from the interrupts. I will remove the dt-binding reset include file, the reset property will use numbers and not macro's. > > > > Best regards, > Krzysztof Best regards, Tomer
On 16/06/2022 06:24, Tomer Maimon wrote: > Hi Krzysztof, > > On Wed, 15 Jun 2022 at 20:03, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 13/06/2022 02:25, Tomer Maimon wrote: >>> Hi Krzysztof, >>> >>> Thanks for your clarification. >>> >>> We can remove the dt-binding file and use numbers in the DTS, >>> appreciate if you can answer few additional questions: >>> 1. Do you suggest adding all NPCM reset values to the NPCM reset >>> document or the reset values should describe in the module >>> documentation that uses it? >> >> What is "NPCM reset document"? Are these reset values anyhow different >> than interrupts or pins? > No, they represent the same values. We do not document in the bindings actual pin or interrupt numbers... >> >>> 2. Some of the NPCM7XX document modules describe the reset value they >>> use from the dt-binding for example: >>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61 >> >> This is NPCM750 >> >>> If we remove the NPCM8XX dt-binding file should we describe the >>> NPCM8XX values in the NPCM-ADC document file? >> >> What is NPCM-ADC document file? What do you want to describe there? >> Again - how is it different than interrupts? > It is not different from the interrupts. > I will remove the dt-binding reset include file, the reset property > will use numbers and not macro's. I have no clue what are you referring now... This is NPCM8xx and it has no binding header with reset values. What to remove then? Best regards, Krzysztof
Hi Krzysztof, On Thu, 16 Jun 2022 at 16:39, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 16/06/2022 06:24, Tomer Maimon wrote: > > Hi Krzysztof, > > > > On Wed, 15 Jun 2022 at 20:03, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 13/06/2022 02:25, Tomer Maimon wrote: > >>> Hi Krzysztof, > >>> > >>> Thanks for your clarification. > >>> > >>> We can remove the dt-binding file and use numbers in the DTS, > >>> appreciate if you can answer few additional questions: > >>> 1. Do you suggest adding all NPCM reset values to the NPCM reset > >>> document or the reset values should describe in the module > >>> documentation that uses it? > >> > >> What is "NPCM reset document"? Are these reset values anyhow different > >> than interrupts or pins? > > No, they represent the same values. > > > We do not document in the bindings actual pin or interrupt numbers... > > >> > >>> 2. Some of the NPCM7XX document modules describe the reset value they > >>> use from the dt-binding for example: > >>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/nuvoton%2Cnpcm750-adc.yaml#L61 > >> > >> This is NPCM750 > >> > >>> If we remove the NPCM8XX dt-binding file should we describe the > >>> NPCM8XX values in the NPCM-ADC document file? > >> > >> What is NPCM-ADC document file? What do you want to describe there? > >> Again - how is it different than interrupts? > > It is not different from the interrupts. > > I will remove the dt-binding reset include file, the reset property > > will use numbers and not macro's. > > I have no clue what are you referring now... This is NPCM8xx and it has > no binding header with reset values. What to remove then? I refer nuvoton,npcm8xx-reset.h file, we don't need it. > > > Best regards, > Krzysztof
On 16/06/2022 06:41, Tomer Maimon wrote: >>>> What is NPCM-ADC document file? What do you want to describe there? >>>> Again - how is it different than interrupts? >>> It is not different from the interrupts. >>> I will remove the dt-binding reset include file, the reset property >>> will use numbers and not macro's. >> >> I have no clue what are you referring now... This is NPCM8xx and it has >> no binding header with reset values. What to remove then? > I refer nuvoton,npcm8xx-reset.h file, we don't need it. There is no such file in kernel, I believe. If you refer to the patchset here, then of course it should not be sent. Best regards, Krzysztof
This patchset adds initial support for the Nuvoton Arbel NPCM8XX Board Management controller (BMC) SoC family. The Nuvoton Arbel NPCM8XX SoC is a fourth-generation BMC. The NPCM8XX computing subsystem comprises a quadcore ARM Cortex A35 ARM-V8 architecture. This patchset adds minimal architecture and drivers such as: Clocksource, Clock, Reset, and WD. Some of the Arbel NPCM8XX peripherals are based on Poleg NPCM7XX. This patchset was tested on the Arbel NPCM8XX evaluation board. Addressed comments from: - Arnd Bergmann : https://www.spinics.net/lists/arm-kernel/msg982728.html https://www.spinics.net/lists/linux-serial/msg48179.html - Stephen Boyd: https://www.spinics.net/lists/arm-kernel/msg983594.html - Krzysztof Kozlowski: https://www.spinics.net/lists/kernel/msg4368955.html https://www.spinics.net/lists/arm-kernel/msg982427.html https://lore.kernel.org/all/973d75b8-0eb6-ff5b-6cd2-9b7d7c5cbcaa@linaro.org/ https://www.spinics.net/lists/arm-kernel/msg982778.html https://www.spinics.net/lists/arm-kernel/msg982588.html - Ilpo Järvinen: https://www.spinics.net/lists/kernel/msg4368936.html - Stephen Boyd: https://www.spinics.net/lists/arm-kernel/msg983596.html - Geert Uytterhoeven : https://www.spinics.net/lists/kernel/msg4369514.html Changes since version 1: - NPCM8XX clock driver - Modify dt-binding. - Remove unsed definition and include. - Include alphabetically. - Use clock devm. - NPCM reset driver - Modify dt-binding. - Modify syscon name. - Add syscon support to NPCM7XX dts reset node. - use data structure. - NPCM8XX device tree: - Modify evb compatible name. - Add NPCM7xx compatible. - Remove disable nodes from the EVB DTS. Tomer Maimon (20): clocksource: timer-npcm7xx: Add NPCM845 timer dt-bindings: serial: 8250: Add npcm845 compatible string tty: serial: 8250: Add NPCM845 UART support dt-bindings: watchdog: npcm: Add npcm845 compatible string watchdog: npcm_wdt: Add NPCM845 watchdog support dt-binding: clk: npcm845: Add binding for Nuvoton NPCM8XX Clock clk: npcm8xx: add clock controller dt-bindings: reset: modify to general NPCM name dt-bindings: reset: npcm: add GCR syscon property ARM: dts: nuvoton: add reset syscon property reset: npcm: using syscon instead of device data dt-bindings: reset: npcm: Add support for NPCM8XX reset: npcm: Add NPCM8XX support dt-bindings: arm: npcm: Add maintainer dt-bindings: arm: npcm: Add nuvoton,npcm845 compatible string dt-bindings: arm: npcm: Add nuvoton,npcm845 GCR compatible string arm64: npcm: Add support for Nuvoton NPCM8XX BMC SoC arm64: dts: nuvoton: Add initial NPCM8XX device tree arm64: dts: nuvoton: Add initial NPCM845 EVB device tree arm64: defconfig: Add Nuvoton NPCM family support .../devicetree/bindings/arm/npcm/npcm.yaml | 7 + .../bindings/arm/npcm/nuvoton,gcr.yaml | 2 + .../bindings/clock/nuvoton,npcm845-clk.yaml | 63 ++ ...750-reset.yaml => nuvoton,npcm-reset.yaml} | 21 +- .../devicetree/bindings/serial/8250.yaml | 1 + .../bindings/watchdog/nuvoton,npcm-wdt.txt | 3 +- MAINTAINERS | 3 + arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi | 1 + arch/arm64/Kconfig.platforms | 11 + arch/arm64/boot/dts/Makefile | 1 + arch/arm64/boot/dts/nuvoton/Makefile | 2 + .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 197 +++++ .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 30 + .../boot/dts/nuvoton/nuvoton-npcm845.dtsi | 76 ++ arch/arm64/configs/defconfig | 3 + drivers/clk/Kconfig | 6 + drivers/clk/Makefile | 1 + drivers/clk/clk-npcm8xx.c | 756 ++++++++++++++++++ drivers/clocksource/timer-npcm7xx.c | 1 + drivers/reset/reset-npcm.c | 202 ++++- drivers/tty/serial/8250/8250_of.c | 1 + drivers/watchdog/npcm_wdt.c | 1 + .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 ++ .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 128 +++ 24 files changed, 1530 insertions(+), 37 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml rename Documentation/devicetree/bindings/reset/{nuvoton,npcm750-reset.yaml => nuvoton,npcm-reset.yaml} (58%) create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi create mode 100644 drivers/clk/clk-npcm8xx.c create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h