| Message ID | 20240216-inno-phy-v1-1-1ab912f0533f@outlook.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | phy: hisi-inno-phy: add support for hi3798mv200-usb2-phy | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 115 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > Also rename to hisilicon,inno-usb2-phy.yaml and add this name to > compatible lists. Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > .../bindings/phy/hisilicon,inno-usb2-phy.yaml | 115 +++++++++++++++++++++ > .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt | 71 ------------- > 2 files changed, 115 insertions(+), 71 deletions(-) > > diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml > new file mode 100644 > index 000000000000..73256eee10f9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml > @@ -0,0 +1,115 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: HiSilicon HiSTB SoCs INNO USB2 PHY device > + > +maintainers: > + - Yang Xiwen <forbidden405@outlook.com> > + > +properties: > + compatible: > + minItems: 1 No, why? Compatibles must be fixed/constrained. > + items: > + - enum: > + - hisilicon,hi3798cv200-usb2-phy > + - hisilicon,hi3798mv100-usb2-phy This wasn't here before. Not explained in commit msg. > + - const: hisilicon,inno-usb2-phy > + > + reg: > + maxItems: 1 > + description: | Do not need '|' unless you need to preserve formatting. > + Should be the address space for PHY configuration register in peripheral > + controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC. > + Or direct MMIO address space. > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + clocks: > + maxItems: 1 > + description: reference clock > + > + resets: > + maxItems: 1 > + > +patternProperties: > + 'phy@[0-9a-f]+': > + type: object > + additionalProperties: false > + description: individual ports provided by INNO PHY > + > + properties: > + reg: > + maxItems: 1 > + > + '#phy-cells': > + const: 0 > + > + resets: > + maxItems: 1 > + > + required: [reg, '#phy-cells', resets] One item per line. Look at other bindings or example-schema. > + > +required: > + - compatible > + - clocks > + - reg > + - '#address-cells' > + - '#size-cells' > + - resets > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/histb-clock.h> > + > + peripheral-controller@8a20000 { > + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; > + reg = <0x8a20000 0x1000>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x8a20000 0x1000>; Drop the node, not related to this binding. If this binding is supposed to be part of other device in case of MFD devices or some tightly coupled ones, then could be included in the example there. > + > + usb2-phy@120 { > + compatible = "hisilicon,hi3798cv200-usb2-phy"; > + reg = <0x120 0x4>; > + clocks = <&crg HISTB_USB2_PHY1_REF_CLK>; > + resets = <&crg 0xbc 4>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + phy@0 { > + reg = <0>; > + #phy-cells = <0>; > + resets = <&crg 0xbc 8>; > + }; > + > + phy@1 { > + reg = <1>; > + #phy-cells = <0>; > + resets = <&crg 0xbc 9>; > + }; > + }; > + > + usb2-phy@124 { > + compatible = "hisilicon,hi3798cv200-usb2-phy"; You can keep only one example, because they are basically the same. Best regards, Krzysztof
On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote: > On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to >> compatible lists. > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. Will fix in next version > >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> .../bindings/phy/hisilicon,inno-usb2-phy.yaml | 115 +++++++++++++++++++++ >> .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt | 71 ------------- >> 2 files changed, 115 insertions(+), 71 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml >> new file mode 100644 >> index 000000000000..73256eee10f9 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml >> @@ -0,0 +1,115 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device >> + >> +maintainers: >> + - Yang Xiwen <forbidden405@outlook.com> >> + >> +properties: >> + compatible: >> + minItems: 1 > No, why? Compatibles must be fixed/constrained. > >> + items: >> + - enum: >> + - hisilicon,hi3798cv200-usb2-phy >> + - hisilicon,hi3798mv100-usb2-phy > This wasn't here before. Not explained in commit msg. The commit 3940ffc65492 ("phy: hisilicon: Add inno-usb2-phy driver for Hi3798MV100") does not have dt-binding change commit along with. Will explain this in commit log. > >> + - const: hisilicon,inno-usb2-phy >> + >> + reg: >> + maxItems: 1 >> + description: | > Do not need '|' unless you need to preserve formatting. > >> + Should be the address space for PHY configuration register in peripheral >> + controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC. >> + Or direct MMIO address space. >> + >> + '#address-cells': >> + const: 1 >> + >> + '#size-cells': >> + const: 0 >> + >> + clocks: >> + maxItems: 1 >> + description: reference clock >> + >> + resets: >> + maxItems: 1 >> + >> +patternProperties: >> + 'phy@[0-9a-f]+': >> + type: object >> + additionalProperties: false >> + description: individual ports provided by INNO PHY >> + >> + properties: >> + reg: >> + maxItems: 1 >> + >> + '#phy-cells': >> + const: 0 >> + >> + resets: >> + maxItems: 1 >> + >> + required: [reg, '#phy-cells', resets] > One item per line. Look at other bindings or example-schema. > >> + >> +required: >> + - compatible >> + - clocks >> + - reg >> + - '#address-cells' >> + - '#size-cells' >> + - resets >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/histb-clock.h> >> + >> + peripheral-controller@8a20000 { >> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >> + reg = <0x8a20000 0x1000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges = <0x0 0x8a20000 0x1000>; > Drop the node, not related to this binding. If this binding is supposed > to be part of other device in case of MFD devices or some tightly > coupled ones, then could be included in the example there. For CV200, this binding is supposed to be always inside the perictrl device. The PHY address space are accessed from a bus implemented by perictrl. > >> + >> + usb2-phy@120 { >> + compatible = "hisilicon,hi3798cv200-usb2-phy"; >> + reg = <0x120 0x4>; >> + clocks = <&crg HISTB_USB2_PHY1_REF_CLK>; >> + resets = <&crg 0xbc 4>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + phy@0 { >> + reg = <0>; >> + #phy-cells = <0>; >> + resets = <&crg 0xbc 8>; >> + }; >> + >> + phy@1 { >> + reg = <1>; >> + #phy-cells = <0>; >> + resets = <&crg 0xbc 9>; >> + }; >> + }; >> + >> + usb2-phy@124 { >> + compatible = "hisilicon,hi3798cv200-usb2-phy"; > You can keep only one example, because they are basically the same. Will remove > > > Best regards, > Krzysztof >
On 17/02/2024 11:24, Yang Xiwen wrote: >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/histb-clock.h> >>> + >>> + peripheral-controller@8a20000 { >>> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >>> + reg = <0x8a20000 0x1000>; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges = <0x0 0x8a20000 0x1000>; >> Drop the node, not related to this binding. If this binding is supposed >> to be part of other device in case of MFD devices or some tightly >> coupled ones, then could be included in the example there. > For CV200, this binding is supposed to be always inside the perictrl > device. The PHY address space are accessed from a bus implemented by > perictrl. Every node is supposes to be somewhere in something, so with this logic you would start from soc@. What's wrong in putting it in parent schema? Best regards, Krzysztof
On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote: > On 17/02/2024 11:24, Yang Xiwen wrote: > >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/clock/histb-clock.h> >>>> + >>>> + peripheral-controller@8a20000 { >>>> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >>>> + reg = <0x8a20000 0x1000>; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + ranges = <0x0 0x8a20000 0x1000>; >>> Drop the node, not related to this binding. If this binding is supposed >>> to be part of other device in case of MFD devices or some tightly >>> coupled ones, then could be included in the example there. >> For CV200, this binding is supposed to be always inside the perictrl >> device. The PHY address space are accessed from a bus implemented by >> perictrl. > Every node is supposes to be somewhere in something, so with this logic > you would start from soc@. What's wrong in putting it in parent schema? When the ports reg property only has an dummy address (no size), it must be inside the perictrl node, accessed from the bus implemented by perictrl. But when the ports has its own MMIO address space (mv200), it should be located under a simple-bus node instead. So it's either: perictrl@8a20000 { usb2-phy@120: { reg = <0x120 0x4>; // this is the register that controls writes and reads to the phy, implemented by perictrl. (just like SPI/I2C) phy@0: { reg = <0>; // the reg is used as an index }; }; }; or: soc@0 { usb2-phy@f9865000 { // MMIO reg = <0xf9865000 0x1000> port0@0 { reg = <0x0 0x400>; // used as MMIO address space }; }; }; So here is why i include perictrl node in the example. If the ports are not mmio, the phy must be under a perictrl node. Or if the ports has its own address space, it should not be under a perictrl node, but rather an simple-bus node. > > Best regards, > Krzysztof >
On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote: > On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to >> compatible lists. > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. > >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> .../bindings/phy/hisilicon,inno-usb2-phy.yaml | 115 +++++++++++++++++++++ >> .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt | 71 ------------- >> 2 files changed, 115 insertions(+), 71 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml >> new file mode 100644 >> index 000000000000..73256eee10f9 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml >> @@ -0,0 +1,115 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device >> + >> +maintainers: >> + - Yang Xiwen <forbidden405@outlook.com> >> + >> +properties: >> + compatible: >> + minItems: 1 > No, why? Compatibles must be fixed/constrained. Hi3798CV200 has only the first compatible listed in its device tree. But you are right i can add it to hi3798mv200.dtsi so that `minItems` can be removed > >> + items: >> + - enum: >> + - hisilicon,hi3798cv200-usb2-phy >> + - hisilicon,hi3798mv100-usb2-phy > This wasn't here before. Not explained in commit msg. > >> + - const: hisilicon,inno-usb2-phy >> + >> + reg: >> + maxItems: 1 >> + description: | > Do not need '|' unless you need to preserve formatting. > >> + Should be the address space for PHY configuration register in peripheral >> + controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC. >> + Or direct MMIO address space. >> + >> + '#address-cells': >> + const: 1 >> + >> + '#size-cells': >> + const: 0 >> + >> + clocks: >> + maxItems: 1 >> + description: reference clock >> + >> + resets: >> + maxItems: 1 >> + >> +patternProperties: >> + 'phy@[0-9a-f]+': >> + type: object >> + additionalProperties: false >> + description: individual ports provided by INNO PHY >> + >> + properties: >> + reg: >> + maxItems: 1 >> + >> + '#phy-cells': >> + const: 0 >> + >> + resets: >> + maxItems: 1 >> + >> + required: [reg, '#phy-cells', resets] > One item per line. Look at other bindings or example-schema. > >> + >> +required: >> + - compatible >> + - clocks >> + - reg >> + - '#address-cells' >> + - '#size-cells' >> + - resets >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/histb-clock.h> >> + >> + peripheral-controller@8a20000 { >> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >> + reg = <0x8a20000 0x1000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges = <0x0 0x8a20000 0x1000>; > Drop the node, not related to this binding. If this binding is supposed > to be part of other device in case of MFD devices or some tightly > coupled ones, then could be included in the example there. > >> + >> + usb2-phy@120 { >> + compatible = "hisilicon,hi3798cv200-usb2-phy"; >> + reg = <0x120 0x4>; >> + clocks = <&crg HISTB_USB2_PHY1_REF_CLK>; >> + resets = <&crg 0xbc 4>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + phy@0 { >> + reg = <0>; >> + #phy-cells = <0>; >> + resets = <&crg 0xbc 8>; >> + }; >> + >> + phy@1 { >> + reg = <1>; >> + #phy-cells = <0>; >> + resets = <&crg 0xbc 9>; >> + }; >> + }; >> + >> + usb2-phy@124 { >> + compatible = "hisilicon,hi3798cv200-usb2-phy"; > You can keep only one example, because they are basically the same. It is listed here just because cv200 (and the upcoming mv200) actually has two INNO phys in the SoC. And coincidently for both SoCs, one with two ports is wired to USB2 controller(EHCI &OHCI), while the other one with only one port is wired to DWC3 controller. The example here is borrowed directly from hi3798cv200.dtsi. > > Best regards, > Krzysztof >
On 17/02/2024 11:54, Yang Xiwen wrote: > On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote: >> On 17/02/2024 11:24, Yang Xiwen wrote: >> >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/clock/histb-clock.h> >>>>> + >>>>> + peripheral-controller@8a20000 { >>>>> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >>>>> + reg = <0x8a20000 0x1000>; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <1>; >>>>> + ranges = <0x0 0x8a20000 0x1000>; >>>> Drop the node, not related to this binding. If this binding is supposed >>>> to be part of other device in case of MFD devices or some tightly >>>> coupled ones, then could be included in the example there. >>> For CV200, this binding is supposed to be always inside the perictrl >>> device. The PHY address space are accessed from a bus implemented by >>> perictrl. >> Every node is supposes to be somewhere in something, so with this logic >> you would start from soc@. What's wrong in putting it in parent schema? > > When the ports reg property only has an dummy address (no size), it must > be inside the perictrl node, accessed from the bus implemented by perictrl. > > But when the ports has its own MMIO address space (mv200), it should be > located under a simple-bus node instead. > > So it's either: > > perictrl@8a20000 { > > usb2-phy@120: { > > reg = <0x120 0x4>; // this is the register that controls writes > and reads to the phy, implemented by perictrl. (just like SPI/I2C) > > phy@0: { > > reg = <0>; // the reg is used as an index > > }; > > }; > > }; > > or: > > soc@0 { > > usb2-phy@f9865000 { // MMIO > > reg = <0xf9865000 0x1000> > > port0@0 { > > reg = <0x0 0x400>; // used as MMIO address space > > }; > > }; > > }; > > So here is why i include perictrl node in the example. If the ports are > not mmio, the phy must be under a perictrl node. Or if the ports has its > own address space, it should not be under a perictrl node, but rather an > simple-bus node. I don't understand why you keep insisting and discussing this. You are adding other compatibles to this schema example, which usually we try to avoid. You entirely ignored my comment above and pasted DTS which is no related to the topic we discuss here. I did not question whether this can be or cannot be in some node. Best regards, Krzysztof
On 17/02/2024 14:14, Yang Xiwen wrote: > On 2/17/2024 6:14 PM, Krzysztof Kozlowski wrote: >> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote: >>> From: Yang Xiwen <forbidden405@outlook.com> >>> >>> Also rename to hisilicon,inno-usb2-phy.yaml and add this name to >>> compatible lists. >> Please use subject prefixes matching the subsystem. You can get them for >> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory >> your patch is touching. >> >>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >>> --- >>> .../bindings/phy/hisilicon,inno-usb2-phy.yaml | 115 +++++++++++++++++++++ >>> .../devicetree/bindings/phy/phy-hisi-inno-usb2.txt | 71 ------------- >>> 2 files changed, 115 insertions(+), 71 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml >>> new file mode 100644 >>> index 000000000000..73256eee10f9 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml >>> @@ -0,0 +1,115 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: HiSilicon HiSTB SoCs INNO USB2 PHY device >>> + >>> +maintainers: >>> + - Yang Xiwen <forbidden405@outlook.com> >>> + >>> +properties: >>> + compatible: >>> + minItems: 1 >> No, why? Compatibles must be fixed/constrained. > Hi3798CV200 has only the first compatible listed in its device tree. But > you are right i can add it to hi3798mv200.dtsi so that `minItems` can be > removed >> >>> + items: >>> + - enum: >>> + - hisilicon,hi3798cv200-usb2-phy >>> + - hisilicon,hi3798mv100-usb2-phy >> This wasn't here before. Not explained in commit msg. >> >>> + - const: hisilicon,inno-usb2-phy >>> + >>> + reg: >>> + maxItems: 1 >>> + description: | >> Do not need '|' unless you need to preserve formatting. >> >>> + Should be the address space for PHY configuration register in peripheral >>> + controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC. >>> + Or direct MMIO address space. >>> + >>> + '#address-cells': >>> + const: 1 >>> + >>> + '#size-cells': >>> + const: 0 >>> + >>> + clocks: >>> + maxItems: 1 >>> + description: reference clock >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> +patternProperties: >>> + 'phy@[0-9a-f]+': >>> + type: object >>> + additionalProperties: false >>> + description: individual ports provided by INNO PHY >>> + >>> + properties: >>> + reg: >>> + maxItems: 1 >>> + >>> + '#phy-cells': >>> + const: 0 >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> + required: [reg, '#phy-cells', resets] >> One item per line. Look at other bindings or example-schema. >> >>> + >>> +required: >>> + - compatible >>> + - clocks >>> + - reg >>> + - '#address-cells' >>> + - '#size-cells' >>> + - resets >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/histb-clock.h> >>> + >>> + peripheral-controller@8a20000 { >>> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >>> + reg = <0x8a20000 0x1000>; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges = <0x0 0x8a20000 0x1000>; >> Drop the node, not related to this binding. If this binding is supposed >> to be part of other device in case of MFD devices or some tightly >> coupled ones, then could be included in the example there. >> >>> + >>> + usb2-phy@120 { >>> + compatible = "hisilicon,hi3798cv200-usb2-phy"; >>> + reg = <0x120 0x4>; >>> + clocks = <&crg HISTB_USB2_PHY1_REF_CLK>; >>> + resets = <&crg 0xbc 4>; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + phy@0 { >>> + reg = <0>; >>> + #phy-cells = <0>; >>> + resets = <&crg 0xbc 8>; >>> + }; >>> + >>> + phy@1 { >>> + reg = <1>; >>> + #phy-cells = <0>; >>> + resets = <&crg 0xbc 9>; >>> + }; >>> + }; >>> + >>> + usb2-phy@124 { >>> + compatible = "hisilicon,hi3798cv200-usb2-phy"; >> You can keep only one example, because they are basically the same. > > It is listed here just because cv200 (and the upcoming mv200) actually > has two INNO phys in the SoC. And coincidently for both SoCs, one with > two ports is wired to USB2 controller(EHCI &OHCI), while the other one > with only one port is wired to DWC3 controller. The example here is > borrowed directly from hi3798cv200.dtsi. > I see the differences and one difference means they are basically the same. Best regards, Krzysztof
On 2/17/2024 9:39 PM, Krzysztof Kozlowski wrote: > On 17/02/2024 11:54, Yang Xiwen wrote: >> On 2/17/2024 6:29 PM, Krzysztof Kozlowski wrote: >>> On 17/02/2024 11:24, Yang Xiwen wrote: >>> >>>>>> + >>>>>> +examples: >>>>>> + - | >>>>>> + #include <dt-bindings/clock/histb-clock.h> >>>>>> + >>>>>> + peripheral-controller@8a20000 { >>>>>> + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; >>>>>> + reg = <0x8a20000 0x1000>; >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <1>; >>>>>> + ranges = <0x0 0x8a20000 0x1000>; >>>>> Drop the node, not related to this binding. If this binding is supposed >>>>> to be part of other device in case of MFD devices or some tightly >>>>> coupled ones, then could be included in the example there. >>>> For CV200, this binding is supposed to be always inside the perictrl >>>> device. The PHY address space are accessed from a bus implemented by >>>> perictrl. >>> Every node is supposes to be somewhere in something, so with this logic >>> you would start from soc@. What's wrong in putting it in parent schema? >> When the ports reg property only has an dummy address (no size), it must >> be inside the perictrl node, accessed from the bus implemented by perictrl. >> >> But when the ports has its own MMIO address space (mv200), it should be >> located under a simple-bus node instead. >> >> So it's either: >> >> perictrl@8a20000 { >> >> usb2-phy@120: { >> >> reg = <0x120 0x4>; // this is the register that controls writes >> and reads to the phy, implemented by perictrl. (just like SPI/I2C) >> >> phy@0: { >> >> reg = <0>; // the reg is used as an index >> >> }; >> >> }; >> >> }; >> >> or: >> >> soc@0 { >> >> usb2-phy@f9865000 { // MMIO >> >> reg = <0xf9865000 0x1000> >> >> port0@0 { >> >> reg = <0x0 0x400>; // used as MMIO address space >> >> }; >> >> }; >> >> }; >> >> So here is why i include perictrl node in the example. If the ports are >> not mmio, the phy must be under a perictrl node. Or if the ports has its >> own address space, it should not be under a perictrl node, but rather an >> simple-bus node. > I don't understand why you keep insisting and discussing this. You are > adding other compatibles to this schema example, which usually we try to > avoid. You entirely ignored my comment above and pasted DTS which is no > related to the topic we discuss here. I did not question whether this > can be or cannot be in some node. okay, I can remove the parent node if it's preferred. This is simply the most usual example that exists in real dts. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml new file mode 100644 index 000000000000..73256eee10f9 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/hisilicon,inno-usb2-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: HiSilicon HiSTB SoCs INNO USB2 PHY device + +maintainers: + - Yang Xiwen <forbidden405@outlook.com> + +properties: + compatible: + minItems: 1 + items: + - enum: + - hisilicon,hi3798cv200-usb2-phy + - hisilicon,hi3798mv100-usb2-phy + - const: hisilicon,inno-usb2-phy + + reg: + maxItems: 1 + description: | + Should be the address space for PHY configuration register in peripheral + controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC. + Or direct MMIO address space. + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + clocks: + maxItems: 1 + description: reference clock + + resets: + maxItems: 1 + +patternProperties: + 'phy@[0-9a-f]+': + type: object + additionalProperties: false + description: individual ports provided by INNO PHY + + properties: + reg: + maxItems: 1 + + '#phy-cells': + const: 0 + + resets: + maxItems: 1 + + required: [reg, '#phy-cells', resets] + +required: + - compatible + - clocks + - reg + - '#address-cells' + - '#size-cells' + - resets + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/histb-clock.h> + + peripheral-controller@8a20000 { + compatible = "hisilicon,hi3798cv200-perictrl", "syscon", "simple-mfd"; + reg = <0x8a20000 0x1000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0x0 0x8a20000 0x1000>; + + usb2-phy@120 { + compatible = "hisilicon,hi3798cv200-usb2-phy"; + reg = <0x120 0x4>; + clocks = <&crg HISTB_USB2_PHY1_REF_CLK>; + resets = <&crg 0xbc 4>; + #address-cells = <1>; + #size-cells = <0>; + + phy@0 { + reg = <0>; + #phy-cells = <0>; + resets = <&crg 0xbc 8>; + }; + + phy@1 { + reg = <1>; + #phy-cells = <0>; + resets = <&crg 0xbc 9>; + }; + }; + + usb2-phy@124 { + compatible = "hisilicon,hi3798cv200-usb2-phy"; + reg = <0x124 0x4>; + clocks = <&crg HISTB_USB2_PHY2_REF_CLK>; + resets = <&crg 0xbc 6>; + #address-cells = <1>; + #size-cells = <0>; + + phy@0 { + reg = <0>; + #phy-cells = <0>; + resets = <&crg 0xbc 10>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/phy/phy-hisi-inno-usb2.txt b/Documentation/devicetree/bindings/phy/phy-hisi-inno-usb2.txt deleted file mode 100644 index 104953e849e7..000000000000 --- a/Documentation/devicetree/bindings/phy/phy-hisi-inno-usb2.txt +++ /dev/null @@ -1,71 +0,0 @@ -Device tree bindings for HiSilicon INNO USB2 PHY - -Required properties: -- compatible: Should be one of the following strings: - "hisilicon,inno-usb2-phy", - "hisilicon,hi3798cv200-usb2-phy". -- reg: Should be the address space for PHY configuration register in peripheral - controller, e.g. PERI_USB0 for USB 2.0 PHY01 on Hi3798CV200 SoC. -- clocks: The phandle and clock specifier pair for INNO USB2 PHY device - reference clock. -- resets: The phandle and reset specifier pair for INNO USB2 PHY device reset - signal. -- #address-cells: Must be 1. -- #size-cells: Must be 0. - -The INNO USB2 PHY device should be a child node of peripheral controller that -contains the PHY configuration register, and each device supports up to 2 PHY -ports which are represented as child nodes of INNO USB2 PHY device. - -Required properties for PHY port node: -- reg: The PHY port instance number. -- #phy-cells: Defined by generic PHY bindings. Must be 0. -- resets: The phandle and reset specifier pair for PHY port reset signal. - -Refer to phy/phy-bindings.txt for the generic PHY binding properties - -Example: - -perictrl: peripheral-controller@8a20000 { - compatible = "hisilicon,hi3798cv200-perictrl", "simple-mfd"; - reg = <0x8a20000 0x1000>; - #address-cells = <1>; - #size-cells = <1>; - ranges = <0x0 0x8a20000 0x1000>; - - usb2_phy1: usb2-phy@120 { - compatible = "hisilicon,hi3798cv200-usb2-phy"; - reg = <0x120 0x4>; - clocks = <&crg HISTB_USB2_PHY1_REF_CLK>; - resets = <&crg 0xbc 4>; - #address-cells = <1>; - #size-cells = <0>; - - usb2_phy1_port0: phy@0 { - reg = <0>; - #phy-cells = <0>; - resets = <&crg 0xbc 8>; - }; - - usb2_phy1_port1: phy@1 { - reg = <1>; - #phy-cells = <0>; - resets = <&crg 0xbc 9>; - }; - }; - - usb2_phy2: usb2-phy@124 { - compatible = "hisilicon,hi3798cv200-usb2-phy"; - reg = <0x124 0x4>; - clocks = <&crg HISTB_USB2_PHY2_REF_CLK>; - resets = <&crg 0xbc 6>; - #address-cells = <1>; - #size-cells = <0>; - - usb2_phy2_port0: phy@0 { - reg = <0>; - #phy-cells = <0>; - resets = <&crg 0xbc 10>; - }; - }; -};