| Message ID | 20250525-ipq5018-ge-phy-v1-0-ddab8854e253@outlook.com |
|---|---|
| Headers | show |
| Series | Add support for the IPQ5018 Internal GE PHY | expand |
On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote: > From: George Moussalem <george.moussalem@outlook.com> > > The MISC reset is supposed to trigger a resets across the MDC, DSP, and > RX & TX clocks of the IPQ5018 internal GE PHY. So let's set the bitmask > of the reset definition accordingly in the GCC as per the downstream > driver. > > Link: https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/00743c3e82fa87cba4460e7a2ba32f473a9ce932 > > Signed-off-by: George Moussalem <george.moussalem@outlook.com> > --- > drivers/clk/qcom/gcc-ipq5018.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c > index 70f5dcb96700f55da1fb19fc893d22350a7e63bf..02d6f08f389f24eccc961b9a4271288c6b635bbc 100644 > --- a/drivers/clk/qcom/gcc-ipq5018.c > +++ b/drivers/clk/qcom/gcc-ipq5018.c > @@ -3660,7 +3660,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = { > [GCC_WCSS_AXI_S_ARES] = { 0x59008, 6 }, > [GCC_WCSS_Q6_BCR] = { 0x18004, 0 }, > [GCC_WCSSAON_RESET] = { 0x59010, 0}, > - [GCC_GEPHY_MISC_ARES] = { 0x56004, 0 }, > + [GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = 0xf }, The computer tells me there aren't any bits beyond this mask.. Does this actually fix anything? Konrad
On 5/27/25 1:14 PM, George Moussalem wrote: > Hi Konrad, > > On 5/27/25 15:00, Konrad Dybcio wrote: >> On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote: >>> From: George Moussalem <george.moussalem@outlook.com> >>> >>> The MISC reset is supposed to trigger a resets across the MDC, DSP, and >>> RX & TX clocks of the IPQ5018 internal GE PHY. So let's set the bitmask >>> of the reset definition accordingly in the GCC as per the downstream >>> driver. >>> >>> Link: https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/commit/00743c3e82fa87cba4460e7a2ba32f473a9ce932 >>> >>> Signed-off-by: George Moussalem <george.moussalem@outlook.com> >>> --- >>> drivers/clk/qcom/gcc-ipq5018.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c >>> index 70f5dcb96700f55da1fb19fc893d22350a7e63bf..02d6f08f389f24eccc961b9a4271288c6b635bbc 100644 >>> --- a/drivers/clk/qcom/gcc-ipq5018.c >>> +++ b/drivers/clk/qcom/gcc-ipq5018.c >>> @@ -3660,7 +3660,7 @@ static const struct qcom_reset_map gcc_ipq5018_resets[] = { >>> [GCC_WCSS_AXI_S_ARES] = { 0x59008, 6 }, >>> [GCC_WCSS_Q6_BCR] = { 0x18004, 0 }, >>> [GCC_WCSSAON_RESET] = { 0x59010, 0}, >>> - [GCC_GEPHY_MISC_ARES] = { 0x56004, 0 }, >>> + [GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = 0xf }, >> >> The computer tells me there aren't any bits beyond this mask.. >> >> Does this actually fix anything? > > The mask is documented in the referenced downstream driver and allows for consolidating: > > resets = <&gcc GCC_GEPHY_MDC_SW_ARES>, > <&gcc GCC_GEPHY_DSP_HW_ARES>, > <&gcc GCC_GEPHY_RX_ARES>, > <&gcc GCC_GEPHY_TX_ARES>; > to: > > resets = <&gcc GCC_MISC_ARES>; > > to conform to this bindings restriction in ethernet-phy.yaml > > resets: > maxItems: 1 > > Effectively, there's no functional change. So we can also list all the resets in the device tree, whatever is preferred. + Kathiravan are there any recommendations from the hw team on which one to use? As far as I can tell, the _MISC one simply pulls all the aforementioned resets, like George described.. it seems weird that it would be designed like this Konrad
On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote: > From: George Moussalem <george.moussalem@outlook.com> > > The IPQ5018 SoC contains an internal GE PHY, always at phy address 7. > As such, let's add the GE PHY node to the SoC dtsi. > > In addition, the GE PHY outputs both the RX and TX clocks to the GCC > which gate controls them and routes them back to the PHY itself. > So let's create two DT fixed clocks and register them in the GCC node. > > Signed-off-by: George Moussalem <george.moussalem@outlook.com> > --- > arch/arm64/boot/dts/qcom/ipq5018.dtsi | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi > index 03ebc3e305b267c98a034c41ce47a39269afce75..ff2de44f9b85993fb2d426f85676f7d54c5cf637 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi > @@ -16,6 +16,18 @@ / { > #size-cells = <2>; > > clocks { > + gephy_rx_clk: gephy-rx-clk { > + compatible = "fixed-clock"; > + clock-frequency = <125000000>; > + #clock-cells = <0>; > + }; > + > + gephy_tx_clk: gephy-tx-clk { > + compatible = "fixed-clock"; > + clock-frequency = <125000000>; > + #clock-cells = <0>; > + }; > + > sleep_clk: sleep-clk { > compatible = "fixed-clock"; > #clock-cells = <0>; > @@ -192,6 +204,17 @@ mdio0: mdio@88000 { > clock-names = "gcc_mdio_ahb_clk"; > > status = "disabled"; > + > + ge_phy: ethernet-phy@7 { drop the label unless it needs to be passed somewhere Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> Konrad
On 5/27/25 17:34, Konrad Dybcio wrote: > On 5/25/25 7:56 PM, George Moussalem via B4 Relay wrote: >> From: George Moussalem <george.moussalem@outlook.com> >> >> The IPQ5018 SoC contains an internal GE PHY, always at phy address 7. >> As such, let's add the GE PHY node to the SoC dtsi. >> >> In addition, the GE PHY outputs both the RX and TX clocks to the GCC >> which gate controls them and routes them back to the PHY itself. >> So let's create two DT fixed clocks and register them in the GCC node. >> >> Signed-off-by: George Moussalem <george.moussalem@outlook.com> >> --- >> arch/arm64/boot/dts/qcom/ipq5018.dtsi | 27 +++++++++++++++++++++++++-- >> 1 file changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi >> index 03ebc3e305b267c98a034c41ce47a39269afce75..ff2de44f9b85993fb2d426f85676f7d54c5cf637 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi >> @@ -16,6 +16,18 @@ / { >> #size-cells = <2>; >> >> clocks { >> + gephy_rx_clk: gephy-rx-clk { >> + compatible = "fixed-clock"; >> + clock-frequency = <125000000>; >> + #clock-cells = <0>; >> + }; >> + >> + gephy_tx_clk: gephy-tx-clk { >> + compatible = "fixed-clock"; >> + clock-frequency = <125000000>; >> + #clock-cells = <0>; >> + }; >> + >> sleep_clk: sleep-clk { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> @@ -192,6 +204,17 @@ mdio0: mdio@88000 { >> clock-names = "gcc_mdio_ahb_clk"; >> >> status = "disabled"; >> + >> + ge_phy: ethernet-phy@7 { > > drop the label unless it needs to be passed somewhere it is needed for boards where the qcom,dac-preset-short-cable property needs to be set. Thanks for the quick review! > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > Konrad George
On Sun, 25 May 2025 21:56:03 +0400, George Moussalem wrote: > The IPQ5018 SoC contains an internal Gigabit Ethernet PHY with its > output pins that provide an MDI interface to either an external switch > in a PHY to PHY link architecture or directly to an attached RJ45 > connector. > > The PHY supports 10/100/1000 mbps link modes, CDT, auto-negotiation and > 802.3az EEE. > > The LDO controller found in the IPQ5018 SoC needs to be enabled to drive > power 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. > > In a phy to phy architecture, DAC values need to be set to accommodate > for the short cable length. > > Signed-off-by: George Moussalem <george.moussalem@outlook.com> > > Signed-off-by: George Moussalem <george.moussalem@outlook.com> > --- > George Moussalem (5): > dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support > clk: qcom: gcc-ipq5018: fix GE PHY reset > net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support > arm64: dts: qcom: ipq5018: add MDIO buses > arm64: dts: qcom: ipq5018: Add GE PHY to internal mdio bus > > .../devicetree/bindings/net/qca,ar803x.yaml | 23 +++ > arch/arm64/boot/dts/qcom/ipq5018.dtsi | 51 ++++- > drivers/clk/qcom/gcc-ipq5018.c | 2 +- > drivers/net/phy/qcom/Kconfig | 2 +- > drivers/net/phy/qcom/at803x.c | 221 ++++++++++++++++++++- > 5 files changed, 287 insertions(+), 12 deletions(-) > --- > base-commit: ebfff09f63e3efb6b75b0328b3536d3ce0e26565 > change-id: 20250430-ipq5018-ge-phy-db654afa4ced > > Best regards, > -- > George Moussalem <george.moussalem@outlook.com> > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade This patch series was applied (using b4) to base: Base: base-commit ebfff09f63e3efb6b75b0328b3536d3ce0e26565 not known, ignoring Base: attempting to guess base-commit... Base: remotes/arm-soc/qcom/dt64-11-g43fefd6c7129 (exact match) If this is not the correct base, please add 'base-commit' tag (or use b4 which does this automatically) New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for 20250525-ipq5018-ge-phy-v1-0-ddab8854e253@outlook.com: arch/arm64/boot/dts/qcom/ipq5018-rdp432-c2.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml# arch/arm64/boot/dts/qcom/ipq5018-tplink-archer-ax55-v1.dtb: ethernet-phy@7: clocks: [[7, 36], [7, 37]] is too long from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#