| Message ID | ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add reset support to EN7581 clk driver | expand |
On Thu, 13 Jun 2024 14:47:03 +0200, Lorenzo Bianconi wrote: > Introduce reset capability to EN7581 device-tree clock binding > documentation. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- > .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ > 2 files changed, 90 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Il 13/06/24 14:47, Lorenzo Bianconi ha scritto: > Introduce reset capability to EN7581 device-tree clock binding > documentation. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- > .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ > 2 files changed, 90 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h > > diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > index 3f4266637733..84353fd09428 100644 > --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > @@ -35,7 +35,7 @@ properties: > > reg: > minItems: 2 > - maxItems: 3 > + maxItems: 4 > > "#clock-cells": > description: > @@ -43,6 +43,10 @@ properties: > clocks. > const: 1 > > + '#reset-cells': > + description: ID of the controller reset line > + const: 1 > + > required: > - compatible > - reg > @@ -60,6 +64,8 @@ allOf: > - description: scu base address > - description: misc scu base address > > + '#reset-cells': false > + > - if: > properties: > compatible: > @@ -70,6 +76,7 @@ allOf: > items: > - description: scu base address > - description: misc scu base address > + - description: reset base address Are you sure that the indentation is correct? :-) After fixing the indentation, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > - description: pb scu base address
On Thu, Jun 27, 2024 at 11:33:47AM +0200, AngeloGioacchino Del Regno wrote: > Il 13/06/24 14:47, Lorenzo Bianconi ha scritto: > > Introduce reset capability to EN7581 device-tree clock binding > > documentation. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- > > .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ > > 2 files changed, 90 insertions(+), 1 deletion(-) > > create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h > > > > diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > index 3f4266637733..84353fd09428 100644 > > --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > @@ -35,7 +35,7 @@ properties: > > reg: > > minItems: 2 > > - maxItems: 3 > > + maxItems: 4 > > "#clock-cells": > > description: > > @@ -43,6 +43,10 @@ properties: > > clocks. > > const: 1 > > + '#reset-cells': > > + description: ID of the controller reset line > > + const: 1 > > + > > required: > > - compatible > > - reg > > @@ -60,6 +64,8 @@ allOf: > > - description: scu base address > > - description: misc scu base address > > + '#reset-cells': false > > + > > - if: > > properties: > > compatible: > > @@ -70,6 +76,7 @@ allOf: > > items: > > - description: scu base address > > - description: misc scu base address > > + - description: reset base address > > Are you sure that the indentation is correct? :-) > > After fixing the indentation, > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > - description: pb scu base address The indentation actually looks okay when I apply this locally, but how is it backwards compatible to add this register in the middle of the list??
Il 27/06/24 11:47, Conor Dooley ha scritto: > On Thu, Jun 27, 2024 at 11:33:47AM +0200, AngeloGioacchino Del Regno wrote: >> Il 13/06/24 14:47, Lorenzo Bianconi ha scritto: >>> Introduce reset capability to EN7581 device-tree clock binding >>> documentation. >>> >>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >>> --- >>> .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- >>> .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ >>> 2 files changed, 90 insertions(+), 1 deletion(-) >>> create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h >>> >>> diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml >>> index 3f4266637733..84353fd09428 100644 >>> --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml >>> +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml >>> @@ -35,7 +35,7 @@ properties: >>> reg: >>> minItems: 2 >>> - maxItems: 3 >>> + maxItems: 4 >>> "#clock-cells": >>> description: >>> @@ -43,6 +43,10 @@ properties: >>> clocks. >>> const: 1 >>> + '#reset-cells': >>> + description: ID of the controller reset line >>> + const: 1 >>> + >>> required: >>> - compatible >>> - reg >>> @@ -60,6 +64,8 @@ allOf: >>> - description: scu base address >>> - description: misc scu base address >>> + '#reset-cells': false >>> + >>> - if: >>> properties: >>> compatible: >>> @@ -70,6 +76,7 @@ allOf: >>> items: >>> - description: scu base address >>> - description: misc scu base address >>> + - description: reset base address >> >> Are you sure that the indentation is correct? :-) >> >> After fixing the indentation, >> >> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> >>> - description: pb scu base address > > The indentation actually looks okay when I apply this locally, but how is > it backwards compatible to add this register in the middle of the list?? It's not, and this is actually something done on purpose - there is no DT using this binding yet (here, nor uboot), and Lorenzo acknowledged the mistake before it was too late... At least this time, it wasn't a misattention :-P Btw, as far as I know, the reset base address is in between misc scu and pb scu, that's why it was put there in the middle. Cheers! Angelo
On Thu, Jun 27, 2024 at 11:53:00AM +0200, AngeloGioacchino Del Regno wrote: > Il 27/06/24 11:47, Conor Dooley ha scritto: > > On Thu, Jun 27, 2024 at 11:33:47AM +0200, AngeloGioacchino Del Regno wrote: > > > Il 13/06/24 14:47, Lorenzo Bianconi ha scritto: > > > > Introduce reset capability to EN7581 device-tree clock binding > > > > documentation. > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > --- > > > > .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- > > > > .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ > > > > 2 files changed, 90 insertions(+), 1 deletion(-) > > > > create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > > > index 3f4266637733..84353fd09428 100644 > > > > --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > > > +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > > > @@ -35,7 +35,7 @@ properties: > > > > reg: > > > > minItems: 2 > > > > - maxItems: 3 > > > > + maxItems: 4 > > > > "#clock-cells": > > > > description: > > > > @@ -43,6 +43,10 @@ properties: > > > > clocks. > > > > const: 1 > > > > + '#reset-cells': > > > > + description: ID of the controller reset line > > > > + const: 1 > > > > + > > > > required: > > > > - compatible > > > > - reg > > > > @@ -60,6 +64,8 @@ allOf: > > > > - description: scu base address > > > > - description: misc scu base address > > > > + '#reset-cells': false > > > > + > > > > - if: > > > > properties: > > > > compatible: > > > > @@ -70,6 +76,7 @@ allOf: > > > > items: > > > > - description: scu base address > > > > - description: misc scu base address > > > > + - description: reset base address > > > > > > Are you sure that the indentation is correct? :-) > > > > > > After fixing the indentation, > > > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > > > > > - description: pb scu base address > > > > The indentation actually looks okay when I apply this locally, but how is > > it backwards compatible to add this register in the middle of the list?? > > It's not, and this is actually something done on purpose - there is no DT using > this binding yet (here, nor uboot), and Lorenzo acknowledged the mistake before > it was too late... > > At least this time, it wasn't a misattention :-P The rationale for this being okay really should be in the commit message... > Btw, as far as I know, the reset base address is in between misc scu and pb scu, > that's why it was put there in the middle. ...but I don't really see why this needs to be done incompatibly at all in the first place. Just put it at the end of the list, there's no rule that the order of reg properties needs to match the MMIO addresses.
Il 27/06/24 11:57, Conor Dooley ha scritto: > On Thu, Jun 27, 2024 at 11:53:00AM +0200, AngeloGioacchino Del Regno wrote: >> Il 27/06/24 11:47, Conor Dooley ha scritto: >>> On Thu, Jun 27, 2024 at 11:33:47AM +0200, AngeloGioacchino Del Regno wrote: >>>> Il 13/06/24 14:47, Lorenzo Bianconi ha scritto: >>>>> Introduce reset capability to EN7581 device-tree clock binding >>>>> documentation. >>>>> >>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >>>>> --- >>>>> .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- >>>>> .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ >>>>> 2 files changed, 90 insertions(+), 1 deletion(-) >>>>> create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml >>>>> index 3f4266637733..84353fd09428 100644 >>>>> --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml >>>>> +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml >>>>> @@ -35,7 +35,7 @@ properties: >>>>> reg: >>>>> minItems: 2 >>>>> - maxItems: 3 >>>>> + maxItems: 4 >>>>> "#clock-cells": >>>>> description: >>>>> @@ -43,6 +43,10 @@ properties: >>>>> clocks. >>>>> const: 1 >>>>> + '#reset-cells': >>>>> + description: ID of the controller reset line >>>>> + const: 1 >>>>> + >>>>> required: >>>>> - compatible >>>>> - reg >>>>> @@ -60,6 +64,8 @@ allOf: >>>>> - description: scu base address >>>>> - description: misc scu base address >>>>> + '#reset-cells': false >>>>> + >>>>> - if: >>>>> properties: >>>>> compatible: >>>>> @@ -70,6 +76,7 @@ allOf: >>>>> items: >>>>> - description: scu base address >>>>> - description: misc scu base address >>>>> + - description: reset base address >>>> >>>> Are you sure that the indentation is correct? :-) >>>> >>>> After fixing the indentation, >>>> >>>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>>> >>>>> - description: pb scu base address >>> >>> The indentation actually looks okay when I apply this locally, but how is >>> it backwards compatible to add this register in the middle of the list?? >> >> It's not, and this is actually something done on purpose - there is no DT using >> this binding yet (here, nor uboot), and Lorenzo acknowledged the mistake before >> it was too late... >> >> At least this time, it wasn't a misattention :-P > > The rationale for this being okay really should be in the commit > message... > >> Btw, as far as I know, the reset base address is in between misc scu and pb scu, >> that's why it was put there in the middle. > > ...but I don't really see why this needs to be done incompatibly at all > in the first place. Just put it at the end of the list, there's no rule > that the order of reg properties needs to match the MMIO addresses. > It's just a perfection thing... but whatever, if that's unacceptable for you, putting it at the end of the list isn't a huge deal anyway. Your call - it's okay for me either way. Cheers, Angelo
On Thu, Jun 27, 2024 at 12:03:38PM +0200, AngeloGioacchino Del Regno wrote: > Il 27/06/24 11:57, Conor Dooley ha scritto: > > On Thu, Jun 27, 2024 at 11:53:00AM +0200, AngeloGioacchino Del Regno wrote: > > > Il 27/06/24 11:47, Conor Dooley ha scritto: > > > > On Thu, Jun 27, 2024 at 11:33:47AM +0200, AngeloGioacchino Del Regno wrote: > > > > > Il 13/06/24 14:47, Lorenzo Bianconi ha scritto: > > > > > > Introduce reset capability to EN7581 device-tree clock binding > > > > > > documentation. > > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > --- > > > > > > .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- > > > > > > .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ > > > > > > 2 files changed, 90 insertions(+), 1 deletion(-) > > > > > > create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > > > > > index 3f4266637733..84353fd09428 100644 > > > > > > --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > > > > > +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml > > > > > > @@ -35,7 +35,7 @@ properties: > > > > > > reg: > > > > > > minItems: 2 > > > > > > - maxItems: 3 > > > > > > + maxItems: 4 > > > > > > "#clock-cells": > > > > > > description: > > > > > > @@ -43,6 +43,10 @@ properties: > > > > > > clocks. > > > > > > const: 1 > > > > > > + '#reset-cells': > > > > > > + description: ID of the controller reset line > > > > > > + const: 1 > > > > > > + > > > > > > required: > > > > > > - compatible > > > > > > - reg > > > > > > @@ -60,6 +64,8 @@ allOf: > > > > > > - description: scu base address > > > > > > - description: misc scu base address > > > > > > + '#reset-cells': false > > > > > > + > > > > > > - if: > > > > > > properties: > > > > > > compatible: > > > > > > @@ -70,6 +76,7 @@ allOf: > > > > > > items: > > > > > > - description: scu base address > > > > > > - description: misc scu base address > > > > > > + - description: reset base address > > > > > > > > > > Are you sure that the indentation is correct? :-) > > > > > > > > > > After fixing the indentation, > > > > > > > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > > > > > > > > > - description: pb scu base address > > > > > > > > The indentation actually looks okay when I apply this locally, but how is > > > > it backwards compatible to add this register in the middle of the list?? > > > > > > It's not, and this is actually something done on purpose - there is no DT using > > > this binding yet (here, nor uboot), and Lorenzo acknowledged the mistake before > > > it was too late... > > > > > > At least this time, it wasn't a misattention :-P > > > > The rationale for this being okay really should be in the commit > > message... > > > > > Btw, as far as I know, the reset base address is in between misc scu and pb scu, > > > that's why it was put there in the middle. > > > > ...but I don't really see why this needs to be done incompatibly at all > > in the first place. Just put it at the end of the list, there's no rule > > that the order of reg properties needs to match the MMIO addresses. > > > > It's just a perfection thing... but whatever, if that's unacceptable for you, > putting it at the end of the list isn't a huge deal anyway. > > Your call - it's okay for me either way. It has not been in a release yet, so it's not a hard block for me, however, I would want to see a commit message update and a Fixes: tag. The patch should also appear in 6.10, rather than being 6.11 material.
diff --git a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml index 3f4266637733..84353fd09428 100644 --- a/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml +++ b/Documentation/devicetree/bindings/clock/airoha,en7523-scu.yaml @@ -35,7 +35,7 @@ properties: reg: minItems: 2 - maxItems: 3 + maxItems: 4 "#clock-cells": description: @@ -43,6 +43,10 @@ properties: clocks. const: 1 + '#reset-cells': + description: ID of the controller reset line + const: 1 + required: - compatible - reg @@ -60,6 +64,8 @@ allOf: - description: scu base address - description: misc scu base address + '#reset-cells': false + - if: properties: compatible: @@ -70,6 +76,7 @@ allOf: items: - description: scu base address - description: misc scu base address + - description: reset base address - description: pb scu base address additionalProperties: false @@ -83,3 +90,19 @@ examples: <0x1fb00000 0x1000>; #clock-cells = <1>; }; + + - | + soc { + #address-cells = <2>; + #size-cells = <2>; + + scuclk: clock-controller@1fa20000 { + compatible = "airoha,en7581-scu"; + reg = <0x0 0x1fa20000 0x0 0x400>, + <0x0 0x1fb00000 0x0 0x90>, + <0x0 0x1fb00830 0x0 0x8>, + <0x0 0x1fbe3400 0x0 0xfc>; + #clock-cells = <1>; + #reset-cells = <1>; + }; + }; diff --git a/include/dt-bindings/reset/airoha,en7581-reset.h b/include/dt-bindings/reset/airoha,en7581-reset.h new file mode 100644 index 000000000000..6544a1790b83 --- /dev/null +++ b/include/dt-bindings/reset/airoha,en7581-reset.h @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024 AIROHA Inc + * Author: Lorenzo Bianconi <lorenzo@kernel.org> + */ + +#ifndef __DT_BINDINGS_RESET_CONTROLLER_AIROHA_EN7581_H_ +#define __DT_BINDINGS_RESET_CONTROLLER_AIROHA_EN7581_H_ + +/* RST_CTRL2 */ +#define EN7581_XPON_PHY_RST 0 +#define EN7581_CPU_TIMER2_RST 1 +#define EN7581_HSUART_RST 2 +#define EN7581_UART4_RST 3 +#define EN7581_UART5_RST 4 +#define EN7581_I2C2_RST 5 +#define EN7581_XSI_MAC_RST 6 +#define EN7581_XSI_PHY_RST 7 +#define EN7581_NPU_RST 8 +#define EN7581_I2S_RST 9 +#define EN7581_TRNG_RST 10 +#define EN7581_TRNG_MSTART_RST 11 +#define EN7581_DUAL_HSI0_RST 12 +#define EN7581_DUAL_HSI1_RST 13 +#define EN7581_HSI_RST 14 +#define EN7581_DUAL_HSI0_MAC_RST 15 +#define EN7581_DUAL_HSI1_MAC_RST 16 +#define EN7581_HSI_MAC_RST 17 +#define EN7581_WDMA_RST 18 +#define EN7581_WOE0_RST 19 +#define EN7581_WOE1_RST 20 +#define EN7581_HSDMA_RST 21 +#define EN7581_TDMA_RST 22 +#define EN7581_EMMC_RST 23 +#define EN7581_SOE_RST 24 +#define EN7581_PCIE2_RST 25 +#define EN7581_XFP_MAC_RST 26 +#define EN7581_USB_HOST_P1_RST 27 +#define EN7581_USB_HOST_P1_U3_PHY_RST 28 +/* RST_CTRL1 */ +#define EN7581_PCM1_ZSI_ISI_RST 29 +#define EN7581_FE_PDMA_RST 30 +#define EN7581_FE_QDMA_RST 31 +#define EN7581_PCM_SPIWP_RST 32 +#define EN7581_CRYPTO_RST 33 +#define EN7581_TIMER_RST 34 +#define EN7581_PCM1_RST 35 +#define EN7581_UART_RST 36 +#define EN7581_GPIO_RST 37 +#define EN7581_GDMA_RST 38 +#define EN7581_I2C_MASTER_RST 39 +#define EN7581_PCM2_ZSI_ISI_RST 40 +#define EN7581_SFC_RST 41 +#define EN7581_UART2_RST 42 +#define EN7581_GDMP_RST 43 +#define EN7581_FE_RST 44 +#define EN7581_USB_HOST_P0_RST 45 +#define EN7581_GSW_RST 46 +#define EN7581_SFC2_PCM_RST 47 +#define EN7581_PCIE0_RST 48 +#define EN7581_PCIE1_RST 49 +#define EN7581_CPU_TIMER_RST 50 +#define EN7581_PCIE_HB_RST 51 +#define EN7581_XPON_MAC_RST 52 + +#endif /* __DT_BINDINGS_RESET_CONTROLLER_AIROHA_EN7581_H_ */
Introduce reset capability to EN7581 device-tree clock binding documentation. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- .../bindings/clock/airoha,en7523-scu.yaml | 25 ++++++- .../dt-bindings/reset/airoha,en7581-reset.h | 66 +++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 include/dt-bindings/reset/airoha,en7581-reset.h