| Message ID | 20250121-dts_qcs615-v3-2-fa4496950d8a@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable ethernet on qcs615 | expand |
On 21/01/2025 08:54, Yijie Yang wrote: > The Qualcomm board always chooses the MAC to provide the delay instead of > the PHY, which is completely opposite to the suggestion of the Linux > kernel. How does the Linux kernel suggest it? > The usage of phy-mode in legacy DTS was also incorrect. Change the > phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID > to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to > the definition. > To address the ABI compatibility issue between the kernel and DTS caused by > this change, handle the compatible string 'qcom,qcs404-evb-4000' in the > code, as it is the only legacy board that mistakenly uses the 'rgmii' > phy-mode. > > Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> > --- > .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos) > static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) > { > struct device *dev = ðqos->pdev->dev; > - int phase_shift; > + int phase_shift = 0; > int loopback; > > /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */ > - if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID || > - ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID) > - phase_shift = 0; > - else > + if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) > phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN; > > /* Disable loopback mode */ > @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev) > ret = of_get_phy_mode(np, ðqos->phy_mode); > if (ret) > return dev_err_probe(dev, ret, "Failed to get phy mode\n"); > + > + root = of_find_node_by_path("/"); > + if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000")) First, just check if machine is compatible, don't open code it. Second, drivers should really, really not rely on the machine. I don't think how this resolves ABI break for other users at all. You need to check what the ABI is here and do not break it. Best regards, Krzysztof
> To address the ABI compatibility issue between the kernel and DTS caused by > this change, handle the compatible string 'qcom,qcs404-evb-4000' in the > code, as it is the only legacy board that mistakenly uses the 'rgmii' > phy-mode. Are you saying every other board DT got this correct? How do you know that? Was this SoC never shipped to anybody, so the only possible board is the QCOM RDK which never left the lab? I doubt this is true, so you are probably breaking out of tree boards. We care less about out of tree boards, but we also don't needlessly break them. Andrew
On 2025-01-21 20:57, Russell King (Oracle) wrote: > On Tue, Jan 21, 2025 at 01:47:55PM +0100, Krzysztof Kozlowski wrote: >> On 21/01/2025 08:54, Yijie Yang wrote: >>> The Qualcomm board always chooses the MAC to provide the delay instead of >>> the PHY, which is completely opposite to the suggestion of the Linux >>> kernel. > > You still need to explain why it's preferable to match this in the mainline > kernel. Does it not work when you use the phylib maintainers suggestion > (if that is so, you need to state as much.) Okay, I will include that explanation in the next version. > >> How does the Linux kernel suggest it? > > It's what phylib maintainers prefer, as documented in many emails from > Andrew Lunn and in Documentation/networking/phy.rst: > > "Whenever possible, use the PHY side RGMII delay for these reasons: > > * PHY devices may offer sub-nanosecond granularity in how they allow a > receiver/transmitter side delay (e.g: 0.5, 1.0, 1.5ns) to be specified. Such > precision may be required to account for differences in PCB trace lengths > > * PHY devices are typically qualified for a large range of applications > (industrial, medical, automotive...), and they provide a constant and > reliable delay across temperature/pressure/voltage ranges > > * PHY device drivers in PHYLIB being reusable by nature, being able to > configure correctly a specified delay enables more designs with similar delay > requirements to be operated correctly > " > >>> The usage of phy-mode in legacy DTS was also incorrect. Change the >>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID >>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to >>> the definition. > > If the delays dependent on the phy-mode are going to be implemented at > the MAC end, then changing the PHY mode indicated to phylink and phylib > to PHY_INTERFACE_MODE_RGMII is the right thing to be doing. However, > as mentioned in the documentation and by Andrew, this is discouraged. > Adding delay by the MAC side is generally discouraged, but the current driver configuration sequence was designed to do so. We need to follow this approach until we can rewrite it, correct?
On 2025-01-21 21:17, Maxime Chevallier wrote: > Hi, > > On Tue, 21 Jan 2025 15:54:54 +0800 > Yijie Yang <quic_yijiyang@quicinc.com> wrote: > >> The Qualcomm board always chooses the MAC to provide the delay instead of >> the PHY, which is completely opposite to the suggestion of the Linux >> kernel. The usage of phy-mode in legacy DTS was also incorrect. Change the >> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID >> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to >> the definition. >> To address the ABI compatibility issue between the kernel and DTS caused by >> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the >> code, as it is the only legacy board that mistakenly uses the 'rgmii' >> phy-mode. >> >> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> >> --- >> .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos) >> static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) >> { >> struct device *dev = ðqos->pdev->dev; >> - int phase_shift; >> + int phase_shift = 0; >> int loopback; >> >> /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */ >> - if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID || >> - ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID) >> - phase_shift = 0; >> - else >> + if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) >> phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN; > > So this looks like a driver modification to deal with errors in > devicetree, and these modifications don't seem to be correct. > > You should set RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN (i.e. adding a delay > n the TX line) when the PHY does not add internal delays on that line > (so, when the mode is rgmii or rgmii-rxid. The previous logic looks > correct in that regard. > > Can you elaborate a bit more on the issue you are seeing ? On what > hardware is this happening ? What's the RGMII setup used (i.e. which > PHY, which mode, is there any delay lines on the PCB ?) As discussed following the first patch, the previous method of using 'rgmii' in DTS while adding delay via the MAC was incorrect. We need to correct this misuse in both the DTS and the driver. For new boards, the phy-mode should be 'rgmii-id', while legacy boards will remain 'rgmii'. Both configurations will still enable RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN and allow the MAC to add the delay, ensuring the behavior remains consistent before and after the change. > > Thanks, > > Maxime
On 22/01/2025 09:56, Yijie Yang wrote: > > > On 2025-01-21 20:47, Krzysztof Kozlowski wrote: >> On 21/01/2025 08:54, Yijie Yang wrote: >>> The Qualcomm board always chooses the MAC to provide the delay instead of >>> the PHY, which is completely opposite to the suggestion of the Linux >>> kernel. >> >> >> How does the Linux kernel suggest it? >> >>> The usage of phy-mode in legacy DTS was also incorrect. Change the >>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID >>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to >>> the definition. >>> To address the ABI compatibility issue between the kernel and DTS caused by >>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the >>> code, as it is the only legacy board that mistakenly uses the 'rgmii' >>> phy-mode. >>> >>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> >>> --- >>> .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 18 +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c >>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos) >>> static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) >>> { >>> struct device *dev = ðqos->pdev->dev; >>> - int phase_shift; >>> + int phase_shift = 0; >>> int loopback; >>> >>> /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */ >>> - if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID || >>> - ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID) >>> - phase_shift = 0; >>> - else >>> + if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) >>> phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN; >>> >>> /* Disable loopback mode */ >>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev) >>> ret = of_get_phy_mode(np, ðqos->phy_mode); >>> if (ret) >>> return dev_err_probe(dev, ret, "Failed to get phy mode\n"); >>> + >>> + root = of_find_node_by_path("/"); >>> + if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000")) >> >> >> First, just check if machine is compatible, don't open code it. >> >> Second, drivers should really, really not rely on the machine. I don't >> think how this resolves ABI break for other users at all. > > As detailed in the commit description, some boards mistakenly use the > 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and That's a kind of an ABI now, assuming it worked fine. > added the delay for it. This code aims to prevent breaking these boards > while correcting the erroneous parsing. This issue is similar to the one > discussed in another thread: > https://lore.kernel.org/all/20241225-support_10m100m-v1-2-4b52ef48b488@quicinc.com/ > >> >> You need to check what the ABI is here and do not break it. > > If board compatible string matching is not recommended, can I address And what if affected board does not match the compatible, because you did not include it here? I said this code does not prevent breaking ABI and you did not address it. To my comment you need to consider ABI, you repeat the same, so kind of something irrelevant. I have feelings you want to push your code, just like in the past several of your colleagues. Learning from past mistakes I will not engage in such discussions, so please really rethink the concept and comments you received here. That's a NAK. Best regards, Krzysztof
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos) static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) { struct device *dev = ðqos->pdev->dev; - int phase_shift; + int phase_shift = 0; int loopback; /* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */ - if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID || - ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID) - phase_shift = 0; - else + if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN; /* Disable loopback mode */ @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev) ret = of_get_phy_mode(np, ðqos->phy_mode); if (ret) return dev_err_probe(dev, ret, "Failed to get phy mode\n"); + + root = of_find_node_by_path("/"); + if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000")) + ethqos->phy_mode = PHY_INTERFACE_MODE_RGMII_ID; + else if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII) + return dev_err_probe(dev, -EINVAL, "Invalid phy-mode rgmii\n"); + of_node_put(root); + + if (plat_dat->phy_interface == PHY_INTERFACE_MODE_RGMII_ID) + plat_dat->phy_interface = PHY_INTERFACE_MODE_RGMII; + switch (ethqos->phy_mode) { case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID:
The Qualcomm board always chooses the MAC to provide the delay instead of the PHY, which is completely opposite to the suggestion of the Linux kernel. The usage of phy-mode in legacy DTS was also incorrect. Change the phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to the definition. To address the ABI compatibility issue between the kernel and DTS caused by this change, handle the compatible string 'qcom,qcs404-evb-4000' in the code, as it is the only legacy board that mistakenly uses the 'rgmii' phy-mode. Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> --- .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)