This should be considered a dirty hack. The proper solution would be
extracting write_reg logic to a separate regmap driver. Leaving only
"write BIT(2) to address 0x6" to the PHY driver.
The initial commit is already doing things wrong. The following patches
adding hi3798mv100 support is also very confusing. The name of the
enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
different across SoCs. But actually it's the bus (i.e. how to write to a
given address) which is different, not the PHY.
Signed-off-by: Yang Xiwen <[email protected]>
---
Yang Xiwen (4):
dt-binding: phy: hisi-inno-usb2: convert to YAML
phy: hisilicon: enable clocks for every ports
phy: hisi-inno-usb2: add support for direct MMIO
dt-binding: phy: hisi-inno-usb2: add compatible of hisilicon,hi3798mv200-usb2-phy
.../bindings/phy/hisilicon,inno-usb2-phy.yaml | 125 +++++++++++++++++++++
.../devicetree/bindings/phy/phy-hisi-inno-usb2.txt | 71 ------------
drivers/phy/hisilicon/phy-hisi-inno-usb2.c | 57 ++++++----
3 files changed, 161 insertions(+), 92 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240216-inno-phy-a2d872f6b74b
Best regards,
--
Yang Xiwen <[email protected]>
From: Yang Xiwen <[email protected]>
Also rename to hisilicon,inno-usb2-phy.yaml and add this name to
compatible lists.
Signed-off-by: Yang Xiwen <[email protected]>
---
.../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 <[email protected]>
+
+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>;
- };
- };
-};
--
2.43.0
From: Yang Xiwen <[email protected]>
It is accessed by direct MMIO, making "ranges" property mandatory for
it.
Signed-off-by: Yang Xiwen <[email protected]>
---
.../devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
index 73256eee10f9..d702878b8e6e 100644
--- a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
@@ -16,6 +16,7 @@ properties:
- enum:
- hisilicon,hi3798cv200-usb2-phy
- hisilicon,hi3798mv100-usb2-phy
+ - hisilicon,hi3798mv200-usb2-phy
- const: hisilicon,inno-usb2-phy
reg:
@@ -64,6 +65,15 @@ required:
- '#size-cells'
- resets
+if:
+ properties:
+ compatible:
+ contains:
+ const: hisilicon,hi3798mv200-inno-usb2-phy
+then:
+ required:
+ - ranges
+
additionalProperties: false
examples:
--
2.43.0
On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[email protected]>
>
> It is accessed by direct MMIO, making "ranges" property mandatory for
> it.
>
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 <[email protected]>
> ---
> .../devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> index 73256eee10f9..d702878b8e6e 100644
> --- a/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,inno-usb2-phy.yaml
> @@ -16,6 +16,7 @@ properties:
> - enum:
> - hisilicon,hi3798cv200-usb2-phy
> - hisilicon,hi3798mv100-usb2-phy
> + - hisilicon,hi3798mv200-usb2-phy
> - const: hisilicon,inno-usb2-phy
>
> reg:
> @@ -64,6 +65,15 @@ required:
> - '#size-cells'
> - resets
>
> +if:
allOf: above.
> + properties:
> + compatible:
> + contains:
> + const: hisilicon,hi3798mv200-inno-usb2-phy
> +then:
> + required:
> + - ranges
So please test your DTS first.
>
Best regards,
Krzysztof
On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
> This should be considered a dirty hack. The proper solution would be
> extracting write_reg logic to a separate regmap driver. Leaving only
> "write BIT(2) to address 0x6" to the PHY driver.
>
> The initial commit is already doing things wrong. The following patches
> adding hi3798mv100 support is also very confusing. The name of the
> enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
> different across SoCs. But actually it's the bus (i.e. how to write to a
> given address) which is different, not the PHY.
I have many bounces from your emails. Please do not Cc unrelated,
non-working hisilicon emails.
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
>> +
>> +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
>
--
Regards,
Yang Xiwen
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
>
--
Regards,
Yang Xiwen
On 2/17/2024 6:18 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> This should be considered a dirty hack. The proper solution would be
>> extracting write_reg logic to a separate regmap driver. Leaving only
>> "write BIT(2) to address 0x6" to the PHY driver.
>>
>> The initial commit is already doing things wrong. The following patches
>> adding hi3798mv100 support is also very confusing. The name of the
>> enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
>> different across SoCs. But actually it's the bus (i.e. how to write to a
>> given address) which is different, not the PHY.
> I have many bounces from your emails. Please do not Cc unrelated,
> non-working hisilicon emails.
I can't recall why Tian is Cced here. Will remove next time.
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
On 2/17/2024 6:18 PM, Krzysztof Kozlowski wrote:
> On 16/02/2024 16:21, Yang Xiwen via B4 Relay wrote:
>> This should be considered a dirty hack. The proper solution would be
>> extracting write_reg logic to a separate regmap driver. Leaving only
>> "write BIT(2) to address 0x6" to the PHY driver.
>>
>> The initial commit is already doing things wrong. The following patches
>> adding hi3798mv100 support is also very confusing. The name of the
>> enumeration "PHY_TYPE_x" is very misleading as if it's the phy which is
>> different across SoCs. But actually it's the bus (i.e. how to write to a
>> given address) which is different, not the PHY.
> I have many bounces from your emails. Please do not Cc unrelated,
> non-working hisilicon emails.
All except one email addresses are added by `b4 prep --auto-to-cc`.
Anyway, i'll remove the email address that does not work in next version.
>
> Best regards,
> Krzysztof
>
--
Regards,
Yang Xiwen
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
>> +
>> +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
>
--
Regards,
Yang Xiwen
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> .../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 <[email protected]>
>>> +
>>> +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 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 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
>
--
Regards,
Yang Xiwen