| Message ID | 20250525-ipq5018-ge-phy-v1-1-ddab8854e253@outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for the IPQ5018 Internal GE PHY | expand |
On Sun, May 25, 2025 at 09:56:04PM +0400, George Moussalem via B4 Relay wrote: > From: George Moussalem <george.moussalem@outlook.com> > > Document the IPQ5018 Internal Gigabit Ethernet PHY found in the IPQ5018 > SoC. Its output pins provide an MDI interface to either an external > switch in a PHY to PHY link scenario or is directly attached to an RJ45 > connector. > > In a phy to phy architecture, DAC values need to be set to accommodate > for the short cable length. As such, add an optional property to do so. > > In addition, the LDO controller found in the IPQ5018 SoC needs to be > enabled to driver low voltages to the CMN Ethernet Block (CMN BLK) which > the GE PHY depends on. The LDO must be enabled in TCSR by writing to a > specific register. So, adding a property that takes a phandle to the > TCSR node and the register offset. > > Signed-off-by: George Moussalem <george.moussalem@outlook.com> > --- > .../devicetree/bindings/net/qca,ar803x.yaml | 23 ++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml > index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644 > --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml > +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml > @@ -60,6 +60,29 @@ properties: > minimum: 1 > maximum: 255 > > + qca,dac: > + description: > + Values for MDAC and EDAC to adjust amplitude, bias current settings, > + and error detection and correction algorithm. Only set in a PHY to PHY > + link architecture to accommodate for short cable length. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + items: > + - items: > + - description: value for MDAC. Expected 0x10, if set > + - description: value for EDAC. Expected 0x10, if set DT is not a collection of magic values to be poked into registers. A bias current should be mA, amplitude probably in mV, and error detection as an algorithm. Andrew
On 25/05/2025 19:56, George Moussalem via B4 Relay wrote: > diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml > index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644 > --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml > +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml > @@ -60,6 +60,29 @@ properties: > minimum: 1 > maximum: 255 > > + qca,dac: > + description: > + Values for MDAC and EDAC to adjust amplitude, bias current settings, > + and error detection and correction algorithm. Only set in a PHY to PHY > + link architecture to accommodate for short cable length. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + items: > + - items: > + - description: value for MDAC. Expected 0x10, if set > + - description: value for EDAC. Expected 0x10, if set If this is fixed to 0x10, then this is fully deducible from compatible. Drop entire property. > + - maxItems: 1 > + > + qca,eth-ldo-enable: qcom,tcsr-syscon to match property already used. > + description: > + Register in TCSR to enable the LDO controller to supply > + low voltages to the common ethernet block (CMN BLK). > + $ref: /schemas/types.yaml#/definitions/phandle-array > + items: > + - items: > + - description: phandle of TCSR syscon > + - description: offset of TCSR register to enable the LDO controller > + - maxItems: 1 You listed two items, but second is just one item? Drop. Best regards, Krzysztof
Hi Andrew, On 5/25/25 23:35, Andrew Lunn wrote: > On Sun, May 25, 2025 at 09:56:04PM +0400, George Moussalem via B4 Relay wrote: >> From: George Moussalem <george.moussalem@outlook.com> >> >> Document the IPQ5018 Internal Gigabit Ethernet PHY found in the IPQ5018 >> SoC. Its output pins provide an MDI interface to either an external >> switch in a PHY to PHY link scenario or is directly attached to an RJ45 >> connector. >> >> In a phy to phy architecture, DAC values need to be set to accommodate >> for the short cable length. As such, add an optional property to do so. >> >> In addition, the LDO controller found in the IPQ5018 SoC needs to be >> enabled to driver low voltages to the CMN Ethernet Block (CMN BLK) which >> the GE PHY depends on. The LDO must be enabled in TCSR by writing to a >> specific register. So, adding a property that takes a phandle to the >> TCSR node and the register offset. >> >> Signed-off-by: George Moussalem <george.moussalem@outlook.com> >> --- >> .../devicetree/bindings/net/qca,ar803x.yaml | 23 ++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml >> index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644 >> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml >> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml >> @@ -60,6 +60,29 @@ properties: >> minimum: 1 >> maximum: 255 >> >> + qca,dac: >> + description: >> + Values for MDAC and EDAC to adjust amplitude, bias current settings, >> + and error detection and correction algorithm. Only set in a PHY to PHY >> + link architecture to accommodate for short cable length. >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + items: >> + - items: >> + - description: value for MDAC. Expected 0x10, if set >> + - description: value for EDAC. Expected 0x10, if set > > DT is not a collection of magic values to be poked into registers. > > A bias current should be mA, amplitude probably in mV, and error > detection as an algorithm. I couldn't agree more but I just don't know what these values are exactly as they aren't documented anywhere. I'm working off the downstream QCA-SSDK codelinaro repo. My *best guess* for the MDAC value is that it halves the amplitude and current for short cable length, but I don't know what algorithm is used for error detection and correction. What I do know is that values must be written in a phy to phy link architecture as the 'cable length' is short, a few cm at most. Without setting these values, the link doesn't work. With the lack of proper documentation, I could move the values to the driver itself and convert it to a boolean property such as qca,phy-to-phy-dac or something.. Any suggestions? > > Andrew Best regards, George
On 5/26/25 08:17, Krzysztof Kozlowski wrote: > On 25/05/2025 19:56, George Moussalem via B4 Relay wrote: >> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml >> index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644 >> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml >> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml >> @@ -60,6 +60,29 @@ properties: >> minimum: 1 >> maximum: 255 >> >> + qca,dac: >> + description: >> + Values for MDAC and EDAC to adjust amplitude, bias current settings, >> + and error detection and correction algorithm. Only set in a PHY to PHY >> + link architecture to accommodate for short cable length. >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + items: >> + - items: >> + - description: value for MDAC. Expected 0x10, if set >> + - description: value for EDAC. Expected 0x10, if set > > If this is fixed to 0x10, then this is fully deducible from compatible. > Drop entire property. as mentioned to Andrew, I can move the required values to the driver itself, but a property would still be required to indicate that this PHY is connected to an external PHY (ex. qca8337 switch). In that case, the values need to be set. Otherwise, not.. Would qcom,phy-to-phy-dac (boolean) do? > >> + - maxItems: 1 >> + >> + qca,eth-ldo-enable: > > qcom,tcsr-syscon to match property already used. to make sure I understand correctly, rename it to qcom,tcsr-syscon? > >> + description: >> + Register in TCSR to enable the LDO controller to supply >> + low voltages to the common ethernet block (CMN BLK). >> + $ref: /schemas/types.yaml#/definitions/phandle-array >> + items: >> + - items: >> + - description: phandle of TCSR syscon >> + - description: offset of TCSR register to enable the LDO controller >> + - maxItems: 1 > You listed two items, but second is just one item? Drop. What is expected is one item that has two values, in this case: <&tcsr 0x019475c4> I could move the offset to the driver itself as it's a fixed offset, so ultimately the property would become: qcom,tcsr-syscon = <&tscr>; agreed? > > Best regards, > Krzysztof > Best regards, George
On 26/05/2025 08:43, George Moussalem wrote: >>> + qca,dac: >>> + description: >>> + Values for MDAC and EDAC to adjust amplitude, bias current settings, >>> + and error detection and correction algorithm. Only set in a PHY to PHY >>> + link architecture to accommodate for short cable length. >>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>> + items: >>> + - items: >>> + - description: value for MDAC. Expected 0x10, if set >>> + - description: value for EDAC. Expected 0x10, if set >> >> If this is fixed to 0x10, then this is fully deducible from compatible. >> Drop entire property. > > as mentioned to Andrew, I can move the required values to the driver > itself, but a property would still be required to indicate that this PHY > is connected to an external PHY (ex. qca8337 switch). In that case, the > values need to be set. Otherwise, not.. > > Would qcom,phy-to-phy-dac (boolean) do? Seems fine to me. > >> >>> + - maxItems: 1 >>> + >>> + qca,eth-ldo-enable: >> >> qcom,tcsr-syscon to match property already used. > > to make sure I understand correctly, rename it to qcom,tcsr-syscon? Yes > >> >>> + description: >>> + Register in TCSR to enable the LDO controller to supply >>> + low voltages to the common ethernet block (CMN BLK). >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + items: >>> + - items: >>> + - description: phandle of TCSR syscon >>> + - description: offset of TCSR register to enable the LDO controller >>> + - maxItems: 1 >> You listed two items, but second is just one item? Drop. > > What is expected is one item that has two values, in this case: <&tcsr > 0x019475c4> I know. > > I could move the offset to the driver itself as it's a fixed offset, so > ultimately the property would become: > > qcom,tcsr-syscon = <&tscr>; > > agreed? No. Just fix the syntax. Best regards, Krzysztof
> I couldn't agree more but I just don't know what these values are exactly as > they aren't documented anywhere. I'm working off the downstream QCA-SSDK > codelinaro repo. My *best guess* for the MDAC value is that it halves the > amplitude and current for short cable length, but I don't know what > algorithm is used for error detection and correction. > > What I do know is that values must be written in a phy to phy link > architecture as the 'cable length' is short, a few cm at most. Without > setting these values, the link doesn't work. > > With the lack of proper documentation, I could move the values to the driver > itself and convert it to a boolean property such as qca,phy-to-phy-dac or > something.. Making it a boolean property is good. Please include in the binding the behaviour when the bool is not present. What you are really describing here is a sort cable, not phy-to-phy, since a PHY is always connected to another PHY. So i think the property name should be about the sort cable/distance. Andrew
On 5/26/25 17:34, Andrew Lunn wrote: >> I couldn't agree more but I just don't know what these values are exactly as >> they aren't documented anywhere. I'm working off the downstream QCA-SSDK >> codelinaro repo. My *best guess* for the MDAC value is that it halves the >> amplitude and current for short cable length, but I don't know what >> algorithm is used for error detection and correction. >> >> What I do know is that values must be written in a phy to phy link >> architecture as the 'cable length' is short, a few cm at most. Without >> setting these values, the link doesn't work. >> >> With the lack of proper documentation, I could move the values to the driver >> itself and convert it to a boolean property such as qca,phy-to-phy-dac or >> something.. > > Making it a boolean property is good. Please include in the binding > the behaviour when the bool is not present. Thanks, will do. > > What you are really describing here is a sort cable, not phy-to-phy, > since a PHY is always connected to another PHY. So i think the > property name should be about the sort cable/distance. The two common archictures across IPQ5018 boards are: 1. IPQ5018 PHY --> MDI --> RJ45 connector 2. IPQ5018 PHY --> MDI --> External PHY (ex. a PHY of a qca8337 switch) Only in scenario 2 (phy to phy architecture), DAC values need to be set to accommodate for the short cable length. Nothing needs to be set in scenario 1 when connected directly to an RJ45 connector. if not, qcom,phy-to-phy-dac, perhaps qcom,dac-short-cable or any other suggestions? > > Andrew Best regards, George
On 5/27/25 17:00, Konrad Dybcio wrote: > On 5/27/25 2:13 PM, George Moussalem wrote: >> >> >> On 5/27/25 15:31, Konrad Dybcio wrote: >>> On 5/27/25 1:28 PM, George Moussalem wrote: >>>> Hi Konrad, >>>> >>>> On 5/27/25 14:59, Konrad Dybcio wrote: >>>>> On 5/26/25 2:55 PM, Krzysztof Kozlowski wrote: >>>>>> On 26/05/2025 08:43, George Moussalem wrote: >>>>>>>>> + qca,dac: >>>>>>>>> + description: >>>>>>>>> + Values for MDAC and EDAC to adjust amplitude, bias current settings, >>>>>>>>> + and error detection and correction algorithm. Only set in a PHY to PHY >>>>>>>>> + link architecture to accommodate for short cable length. >>>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>>>>>>>> + items: >>>>>>>>> + - items: >>>>>>>>> + - description: value for MDAC. Expected 0x10, if set >>>>>>>>> + - description: value for EDAC. Expected 0x10, if set >>>>>>>> >>>>>>>> If this is fixed to 0x10, then this is fully deducible from compatible. >>>>>>>> Drop entire property. >>>>>>> >>>>>>> as mentioned to Andrew, I can move the required values to the driver >>>>>>> itself, but a property would still be required to indicate that this PHY >>>>>>> is connected to an external PHY (ex. qca8337 switch). In that case, the >>>>>>> values need to be set. Otherwise, not.. >>>>>>> >>>>>>> Would qcom,phy-to-phy-dac (boolean) do? >>>>>> >>>>>> Seems fine to me. >>>>> >>>>> Can the driver instead check for a phy reference? >>>> >>>> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2. >>> >>> I'm not sure how this is all wired up. Do you have an example of a DT >>> with both configurations you described in your reply to Andrew? >> >> Sure, for IPQ5018 GE PHY connected to a QCA8337 switch (phy to phy): >> Link: https://github.com/openwrt/openwrt/blob/main/target/linux/qualcommax/files/arch/arm64/boot/dts/qcom/ipq5018-spnmx56.dts >> In this scenario, the IPQ5018 single UNIPHY is freed up and can be used with an external PHY such as QCA8081 to offer up to 2.5 gbps connectivity, see diagram below: >> >> * ================================================================= >> * _______________________ _______________________ >> * | IPQ5018 | | QCA8337 | >> * | +------+ +--------+ | | +--------+ +------+ | >> * | | MAC0 |---| GE Phy |-+--- MDI ---+ | Phy4 |---| MAC5 | | >> * | +------+ +--------+ | | +--------+ +------+ | >> * | | |_______________________| >> * | | _______________________ >> * | | | QCA8081 | >> * | +------+ +--------+ | | +--------+ +------+ | >> * | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy |---| RJ45 | | >> * | +------+ +--------+ | | +--------+ +------+ | >> * |_______________________| |_______________________| >> * >> * ================================================================= >> >> The other use case is when an external switch or PHY, if any, is connected to the IPQ5018 UNIPHY over SGMII(+), freeing up the GE PHY which can optionally be connected to an RJ45 connector. I haven't worked on such board yet where the GE PHY is directly connected to RJ45, but I believe the Linksys MX6200 has this architecture (which I'll look into soon). >> >> * ================================================================= >> * _______________________ ____________ >> * | IPQ5018 | | | >> * | +------+ +--------+ | | +--------+ | >> * | | MAC0 |---| GE Phy |-+--- MDI ---+ | RJ45 | + >> * | +------+ +--------+ | | +--------+ | >> * | | |____________| >> * | | _______________________ >> * | | | QCA8081 Phy | >> * | +------+ +--------+ | | +--------+ +------+ | >> * | | MAC1 |---| Uniphy |-+-- SGMII+--+ | Phy |---| RJ45 | | >> * | +------+ +--------+ | | +--------+ +------+ | >> * |_______________________| |_______________________| >> * >> * ================================================================= > > So - with keeping in mind that I'm not a big networking guy - can we test > for whether there's an ethernet-switch present under the MDIO host and > decide based on that? AFAIK and unless I stand corrected by others, we can detect the presence of a phy or switch, but we can't detect how it's wired up. It could be connected to the GE PHY or the UNIPHY. Hence, the need for a property. > > Konrad George
On 5/27/25 3:08 PM, Andrew Lunn wrote: >>>>>>> Would qcom,phy-to-phy-dac (boolean) do? >>>>>> >>>>>> Seems fine to me. >>>>> >>>>> Can the driver instead check for a phy reference? >>>> >>>> Do you mean using the existing phy-handle DT property or create a new DT property called 'qcom,phy-reference'? Either way, can add it for v2. >>> >>> I'm not sure how this is all wired up. Do you have an example of a DT >>> with both configurations you described in your reply to Andrew? > > When a SoC interface is connected to a switch, the SoC interface has > no real knowledge it is actually connected to a switch. All it knows > is it has a link peer, and it does not know if that peer is 1cm away > or 100m. It does nothing different. > > The switch has a phandle to the SoC interface, so it knows a bit more, > but it would be a bit around about for the SoC interface to search the > whole device tree to see if there is a switch with a phandle pointing > to it. So for me, a bool property indicating a short 'cable' is the > better solution. OK does this sound like a generic enough problem to contemplate something common, or should we go with something like qcom,dac-preset-short-cable Konrad
diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml index 3acd09f0da863137f8a05e435a1fd28a536c2acd..a9e94666ff0af107db4f358b144bf8644c6597e8 100644 --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml @@ -60,6 +60,29 @@ properties: minimum: 1 maximum: 255 + qca,dac: + description: + Values for MDAC and EDAC to adjust amplitude, bias current settings, + and error detection and correction algorithm. Only set in a PHY to PHY + link architecture to accommodate for short cable length. + $ref: /schemas/types.yaml#/definitions/uint32-array + items: + - items: + - description: value for MDAC. Expected 0x10, if set + - description: value for EDAC. Expected 0x10, if set + - maxItems: 1 + + qca,eth-ldo-enable: + description: + Register in TCSR to enable the LDO controller to supply + low voltages to the common ethernet block (CMN BLK). + $ref: /schemas/types.yaml#/definitions/phandle-array + items: + - items: + - description: phandle of TCSR syscon + - description: offset of TCSR register to enable the LDO controller + - maxItems: 1 + vddio-supply: description: | RGMII I/O voltage regulator (see regulator/regulator.yaml).