| Message ID | 20221216070632.11444-6-yanhong.wang@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Ethernet driver for StarFive JH7110 SoC | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Guessing tree name failed |
On 16/12/2022 08:06, Yanhong Wang wrote: > Add support for Motorcomm Technology YT8531 10/100/1000 Ethernet PHY. > The document describe details of clock delay train configuration. > > Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> Missing vendor prefix documentation. I don't think you tested this at all with checkpatch and dt_binding_check. > --- > .../bindings/net/motorcomm,yt8531.yaml | 111 ++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 112 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml > > diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml > new file mode 100644 > index 000000000000..c5b8a09a78bb > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml > @@ -0,0 +1,111 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/motorcomm,yt8531.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Motorcomm YT8531 Gigabit Ethernet PHY > + > +maintainers: > + - Yanhong Wang <yanhong.wang@starfivetech.com> > + Why there is no reference to ethernet-phy.yaml? > +select: > + properties: > + $nodename: > + pattern: "^ethernet-phy(@[a-f0-9]+)?$" I don't think that's correct approach. You know affect all phys. > + > + required: > + - $nodename > + > +properties: > + $nodename: > + pattern: "^ethernet-phy(@[a-f0-9]+)?$" Just reference ethernet-phy.yaml. > + > + reg: > + minimum: 0 > + maximum: 31 > + description: > + The ID number for the PHY. Drop duplicated properties. > + > + rxc_dly_en: No underscores in node names. Missing vendor prefix. Both apply to all your other custom properties, unless they are not custom but generic. Missing ref. > + description: | > + RGMII Receive PHY Clock Delay defined with fixed 2ns.This is used for After every full stop goes space. > + PHY that have configurable RX internal delays. If this property set > + to 1, then automatically add 2ns delay pad for Receive PHY clock. Nope, this is wrong. You wrote now boolean property as enum. > + enum: [0, 1] > + default: 0 > + > + rx_delay_sel: > + description: | > + This is supplement to rxc_dly_en property,and it can > + be specified in 150ps(pico seconds) steps. The effective > + delay is: 150ps * N. Nope. Use proper units and drop all this register stuff. > + minimum: 0 > + maximum: 15 > + default: 0 > + > + tx_delay_sel_fe: > + description: | > + RGMII Transmit PHY Clock Delay defined in pico seconds.This is used for > + PHY's that have configurable TX internal delays when speed is 100Mbps > + or 10Mbps. It can be specified in 150ps steps, the effective delay > + is: 150ps * N. The binding is in very poor shape. Please look carefully in example-schema. All my previous comments apply everywhere. > + minimum: 0 > + maximum: 15 > + default: 15 > + > + tx_delay_sel: > + description: | > + RGMII Transmit PHY Clock Delay defined in pico seconds.This is used for > + PHY's that have configurable TX internal delays when speed is 1000Mbps. > + It can be specified in 150ps steps, the effective delay is: 150ps * N. > + minimum: 0 > + maximum: 15 > + default: 1 > + > + tx_inverted_10: > + description: | > + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII > + Transmit PHY Clock delay train configuration when speed is 10Mbps. > + 0: original 1: inverted > + enum: [0, 1] > + default: 0 > + > + tx_inverted_100: > + description: | > + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII > + Transmit PHY Clock delay train configuration when speed is 100Mbps. > + 0: original 1: inverted > + enum: [0, 1] > + default: 0 > + > + tx_inverted_1000: > + description: | > + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII > + Transmit PHY Clock delay train configuration when speed is 1000Mbps. > + 0: original 1: inverted > + enum: [0, 1] > + default: 0 > + > +required: > + - reg > + > +additionalProperties: true This must be false. After referencing ethernet-phy this should be unevaluatedProperties: false. Best regards, Krzysztof
On 2022/12/16 19:15, Krzysztof Kozlowski wrote: > On 16/12/2022 08:06, Yanhong Wang wrote: >> Add support for Motorcomm Technology YT8531 10/100/1000 Ethernet PHY. >> The document describe details of clock delay train configuration. >> >> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> > > Missing vendor prefix documentation. I don't think you tested this at > all with checkpatch and dt_binding_check. > >> --- >> .../bindings/net/motorcomm,yt8531.yaml | 111 ++++++++++++++++++ >> MAINTAINERS | 1 + >> 2 files changed, 112 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml >> new file mode 100644 >> index 000000000000..c5b8a09a78bb >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml >> @@ -0,0 +1,111 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/motorcomm,yt8531.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Motorcomm YT8531 Gigabit Ethernet PHY >> + >> +maintainers: >> + - Yanhong Wang <yanhong.wang@starfivetech.com> >> + > > Why there is no reference to ethernet-phy.yaml? > >> +select: >> + properties: >> + $nodename: >> + pattern: "^ethernet-phy(@[a-f0-9]+)?$" > > I don't think that's correct approach. You know affect all phys. > >> + >> + required: >> + - $nodename >> + >> +properties: >> + $nodename: >> + pattern: "^ethernet-phy(@[a-f0-9]+)?$" > > Just reference ethernet-phy.yaml. > >> + >> + reg: >> + minimum: 0 >> + maximum: 31 >> + description: >> + The ID number for the PHY. > > Drop duplicated properties. > >> + >> + rxc_dly_en: > > No underscores in node names. Missing vendor prefix. Both apply to all > your other custom properties, unless they are not custom but generic. > > Missing ref. > >> + description: | >> + RGMII Receive PHY Clock Delay defined with fixed 2ns.This is used for > > After every full stop goes space. > >> + PHY that have configurable RX internal delays. If this property set >> + to 1, then automatically add 2ns delay pad for Receive PHY clock. > > Nope, this is wrong. You wrote now boolean property as enum. > >> + enum: [0, 1] >> + default: 0 >> + >> + rx_delay_sel: >> + description: | >> + This is supplement to rxc_dly_en property,and it can >> + be specified in 150ps(pico seconds) steps. The effective >> + delay is: 150ps * N. > > Nope. Use proper units and drop all this register stuff. > >> + minimum: 0 >> + maximum: 15 >> + default: 0 >> + >> + tx_delay_sel_fe: >> + description: | >> + RGMII Transmit PHY Clock Delay defined in pico seconds.This is used for >> + PHY's that have configurable TX internal delays when speed is 100Mbps >> + or 10Mbps. It can be specified in 150ps steps, the effective delay >> + is: 150ps * N. > > The binding is in very poor shape. Please look carefully in > example-schema. All my previous comments apply everywhere. > >> + minimum: 0 >> + maximum: 15 >> + default: 15 >> + >> + tx_delay_sel: >> + description: | >> + RGMII Transmit PHY Clock Delay defined in pico seconds.This is used for >> + PHY's that have configurable TX internal delays when speed is 1000Mbps. >> + It can be specified in 150ps steps, the effective delay is: 150ps * N. >> + minimum: 0 >> + maximum: 15 >> + default: 1 >> + >> + tx_inverted_10: >> + description: | >> + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII >> + Transmit PHY Clock delay train configuration when speed is 10Mbps. >> + 0: original 1: inverted >> + enum: [0, 1] >> + default: 0 >> + >> + tx_inverted_100: >> + description: | >> + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII >> + Transmit PHY Clock delay train configuration when speed is 100Mbps. >> + 0: original 1: inverted >> + enum: [0, 1] >> + default: 0 >> + >> + tx_inverted_1000: >> + description: | >> + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII >> + Transmit PHY Clock delay train configuration when speed is 1000Mbps. >> + 0: original 1: inverted >> + enum: [0, 1] >> + default: 0 >> + >> +required: >> + - reg >> + >> +additionalProperties: true > > This must be false. After referencing ethernet-phy this should be > unevaluatedProperties: false. > > Thanks. Parts of this patch exist already, after discussion unanimity was achieved, i will remove the parts of YT8531 in the next version. > Best regards, > Krzysztof >
On 27/12/2022 10:38, yanhong wang wrote: >> >> This must be false. After referencing ethernet-phy this should be >> unevaluatedProperties: false. >> >> > > Thanks. Parts of this patch exist already, after discussion unanimity was achieved, > i will remove the parts of YT8531 in the next version. I don't understand what does it mean. You sent duplicated patch? If so, please do not... you waste reviewers time. Anyway this entire patch does not meet criteria for submission at all, so please start over from example-schema. Best regards, Krzysztof
On 2022/12/27 17:52, Krzysztof Kozlowski wrote: > On 27/12/2022 10:38, yanhong wang wrote: >>> >>> This must be false. After referencing ethernet-phy this should be >>> unevaluatedProperties: false. >>> >>> >> >> Thanks. Parts of this patch exist already, after discussion unanimity was achieved, >> i will remove the parts of YT8531 in the next version. > > I don't understand what does it mean. You sent duplicated patch? If so, > please do not... you waste reviewers time. > > Anyway this entire patch does not meet criteria for submission at all, > so please start over from example-schema. > Sorry, maybe I didn't make it clear, which led to misunderstanding. Motorcomm Inc is also carrying out the upstream of YT8531, and my patch will be duplicated and conflicted with their submission. By communicating with the developers of Motorcomm Inc, the part of YT8531 will be submitted by Motorcomm Inc, so my submission about YT8531 will be withdrawn. > Best regards, > Krzysztof >
On 28/12/2022 04:23, yanhong wang wrote: > > > On 2022/12/27 17:52, Krzysztof Kozlowski wrote: >> On 27/12/2022 10:38, yanhong wang wrote: >>>> >>>> This must be false. After referencing ethernet-phy this should be >>>> unevaluatedProperties: false. >>>> >>>> >>> >>> Thanks. Parts of this patch exist already, after discussion unanimity was achieved, >>> i will remove the parts of YT8531 in the next version. >> >> I don't understand what does it mean. You sent duplicated patch? If so, >> please do not... you waste reviewers time. >> >> Anyway this entire patch does not meet criteria for submission at all, >> so please start over from example-schema. >> > > Sorry, maybe I didn't make it clear, which led to misunderstanding. Motorcomm Inc is also > carrying out the upstream of YT8531, and my patch will be duplicated and conflicted > with their submission. By communicating with the developers of Motorcomm Inc, the part > of YT8531 will be submitted by Motorcomm Inc, so my submission about YT8531 will be withdrawn. Are they going to apply the feedback received for this series? Best regards, Krzysztof
On 2022/12/28 17:11, Krzysztof Kozlowski wrote: > On 28/12/2022 04:23, yanhong wang wrote: >> >> >> On 2022/12/27 17:52, Krzysztof Kozlowski wrote: >>> On 27/12/2022 10:38, yanhong wang wrote: >>>>> >>>>> This must be false. After referencing ethernet-phy this should be >>>>> unevaluatedProperties: false. >>>>> >>>>> >>>> >>>> Thanks. Parts of this patch exist already, after discussion unanimity was achieved, >>>> i will remove the parts of YT8531 in the next version. >>> >>> I don't understand what does it mean. You sent duplicated patch? If so, >>> please do not... you waste reviewers time. >>> >>> Anyway this entire patch does not meet criteria for submission at all, >>> so please start over from example-schema. >>> >> >> Sorry, maybe I didn't make it clear, which led to misunderstanding. Motorcomm Inc is also >> carrying out the upstream of YT8531, and my patch will be duplicated and conflicted >> with their submission. By communicating with the developers of Motorcomm Inc, the part >> of YT8531 will be submitted by Motorcomm Inc, so my submission about YT8531 will be withdrawn. > > Are they going to apply the feedback received for this series? > Yes, they support not only YT8531, but also other models of their company. > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml new file mode 100644 index 000000000000..c5b8a09a78bb --- /dev/null +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml @@ -0,0 +1,111 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/motorcomm,yt8531.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Motorcomm YT8531 Gigabit Ethernet PHY + +maintainers: + - Yanhong Wang <yanhong.wang@starfivetech.com> + +select: + properties: + $nodename: + pattern: "^ethernet-phy(@[a-f0-9]+)?$" + + required: + - $nodename + +properties: + $nodename: + pattern: "^ethernet-phy(@[a-f0-9]+)?$" + + reg: + minimum: 0 + maximum: 31 + description: + The ID number for the PHY. + + rxc_dly_en: + description: | + RGMII Receive PHY Clock Delay defined with fixed 2ns.This is used for + PHY that have configurable RX internal delays. If this property set + to 1, then automatically add 2ns delay pad for Receive PHY clock. + enum: [0, 1] + default: 0 + + rx_delay_sel: + description: | + This is supplement to rxc_dly_en property,and it can + be specified in 150ps(pico seconds) steps. The effective + delay is: 150ps * N. + minimum: 0 + maximum: 15 + default: 0 + + tx_delay_sel_fe: + description: | + RGMII Transmit PHY Clock Delay defined in pico seconds.This is used for + PHY's that have configurable TX internal delays when speed is 100Mbps + or 10Mbps. It can be specified in 150ps steps, the effective delay + is: 150ps * N. + minimum: 0 + maximum: 15 + default: 15 + + tx_delay_sel: + description: | + RGMII Transmit PHY Clock Delay defined in pico seconds.This is used for + PHY's that have configurable TX internal delays when speed is 1000Mbps. + It can be specified in 150ps steps, the effective delay is: 150ps * N. + minimum: 0 + maximum: 15 + default: 1 + + tx_inverted_10: + description: | + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII + Transmit PHY Clock delay train configuration when speed is 10Mbps. + 0: original 1: inverted + enum: [0, 1] + default: 0 + + tx_inverted_100: + description: | + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII + Transmit PHY Clock delay train configuration when speed is 100Mbps. + 0: original 1: inverted + enum: [0, 1] + default: 0 + + tx_inverted_1000: + description: | + Use original or inverted RGMII Transmit PHY Clock to drive the RGMII + Transmit PHY Clock delay train configuration when speed is 1000Mbps. + 0: original 1: inverted + enum: [0, 1] + default: 0 + +required: + - reg + +additionalProperties: true + +examples: + - | + ethernet { + #address-cells = <1>; + #size-cells = <0>; + + ethernet-phy@0 { + reg = <0>; + + rxc_dly_en = <1>; + tx_delay_sel_fe = <5>; + tx_delay_sel = <0xa>; + tx_inverted_10 = <0x1>; + tx_inverted_100 = <0x1>; + tx_inverted_1000 = <0x1>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 166b0009f63c..1ff68b8524d2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19609,6 +19609,7 @@ F: include/dt-bindings/clock/starfive* STARFIVE DWMAC GLUE LAYER M: Yanhong Wang <yanhong.wang@starfivetech.com> S: Maintained +F: Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml F: Documentation/devicetree/bindings/net/starfive,jh71x0-dwmac.yaml STARFIVE PINCTRL DRIVER
Add support for Motorcomm Technology YT8531 10/100/1000 Ethernet PHY. The document describe details of clock delay train configuration. Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> --- .../bindings/net/motorcomm,yt8531.yaml | 111 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 112 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8531.yaml