| Message ID | 20241225-support_10m100m-v1-2-4b52ef48b488@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support tuning the RX sampling swap of the MAC. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 25/12/2024 11:04, Yijie Yang wrote: > static int qcom_ethqos_probe(struct platform_device *pdev) > { > - struct device_node *np = pdev->dev.of_node; > + struct device_node *np = pdev->dev.of_node, *root; > const struct ethqos_emac_driver_data *data; > struct plat_stmmacenet_data *plat_dat; > struct stmmac_resources stmmac_res; > @@ -810,6 +805,15 @@ 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,sa8540p-ride")) Nope, your drivers are not supposed to poke root compatibles. Drop and fix your driver to behave correctly for all existing devices. > + ethqos->needs_rx_prog_swap = true; > + else > + ethqos->needs_rx_prog_swap = Best regards, Krzysztof
On 2024-12-25 19:37, Krzysztof Kozlowski wrote: > On 25/12/2024 11:04, Yijie Yang wrote: > >> static int qcom_ethqos_probe(struct platform_device *pdev) >> { >> - struct device_node *np = pdev->dev.of_node; >> + struct device_node *np = pdev->dev.of_node, *root; >> const struct ethqos_emac_driver_data *data; >> struct plat_stmmacenet_data *plat_dat; >> struct stmmac_resources stmmac_res; >> @@ -810,6 +805,15 @@ 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,sa8540p-ride")) > > > Nope, your drivers are not supposed to poke root compatibles. Drop and > fix your driver to behave correctly for all existing devices. > Since this change introduces a new flag in the DTS, we must maintain ABI compatibility with the kernel. The new flag is specific to the board, so I need to ensure root nodes are matched to allow older boards to continue functioning as before. I'm happy to adopt that approach if there are any more elegant solutions. >> + ethqos->needs_rx_prog_swap = true; >> + else >> + ethqos->needs_rx_prog_swap = > Best regards, > Krzysztof
On Thu, Dec 26, 2024 at 10:29:45AM +0800, Yijie Yang wrote: > > > On 2024-12-25 19:37, Krzysztof Kozlowski wrote: > > On 25/12/2024 11:04, Yijie Yang wrote: > > > > > static int qcom_ethqos_probe(struct platform_device *pdev) > > > { > > > - struct device_node *np = pdev->dev.of_node; > > > + struct device_node *np = pdev->dev.of_node, *root; > > > const struct ethqos_emac_driver_data *data; > > > struct plat_stmmacenet_data *plat_dat; > > > struct stmmac_resources stmmac_res; > > > @@ -810,6 +805,15 @@ 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,sa8540p-ride")) > > > > > > Nope, your drivers are not supposed to poke root compatibles. Drop and > > fix your driver to behave correctly for all existing devices. > > > > Since this change introduces a new flag in the DTS, we must maintain ABI > compatibility with the kernel. The new flag is specific to the board, so I > need to ensure root nodes are matched to allow older boards to continue > functioning as before. I'm happy to adopt that approach if there are any > more elegant solutions. Why is it specific to this board? Does the board have a PHY which is broken and requires this property? What we are missing are the details needed to help you get to the correct way to solve the problem you are facing. Andrew
On 26/12/2024 03:29, Yijie Yang wrote: > > > On 2024-12-25 19:37, Krzysztof Kozlowski wrote: >> On 25/12/2024 11:04, Yijie Yang wrote: >> >>> static int qcom_ethqos_probe(struct platform_device *pdev) >>> { >>> - struct device_node *np = pdev->dev.of_node; >>> + struct device_node *np = pdev->dev.of_node, *root; >>> const struct ethqos_emac_driver_data *data; >>> struct plat_stmmacenet_data *plat_dat; >>> struct stmmac_resources stmmac_res; >>> @@ -810,6 +805,15 @@ 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,sa8540p-ride")) >> >> >> Nope, your drivers are not supposed to poke root compatibles. Drop and >> fix your driver to behave correctly for all existing devices. >> > > Since this change introduces a new flag in the DTS, we must maintain ABI > compatibility with the kernel. The new flag is specific to the board, so It's not, I don't see it specific to the board in the bindings. > I need to ensure root nodes are matched to allow older boards to > continue functioning as before. I'm happy to adopt that approach if > there are any more elegant solutions. I don't think you understood the problem. Why you are not handling this for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp? 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..cf9633489975bc89480d065ae723a92723a12b04 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c @@ -120,6 +120,7 @@ struct qcom_ethqos { bool rgmii_config_loopback_en; bool has_emac_ge_3; bool needs_sgmii_loopback; + bool needs_rx_prog_swap; }; static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset) @@ -401,6 +402,7 @@ 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 rx_prog_swap = 0; int phase_shift; int loopback; @@ -421,6 +423,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) else loopback = 0; + if (ethqos->needs_rx_prog_swap) + rx_prog_swap = RGMII_CONFIG2_RX_PROG_SWAP; + /* Select RGMII, write 0 to interface select */ rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL, 0, RGMII_IO_MACRO_CONFIG); @@ -484,14 +489,8 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) BIT(6), RGMII_IO_MACRO_CONFIG); rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, 0, RGMII_IO_MACRO_CONFIG2); - - if (ethqos->has_emac_ge_3) - rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, - RGMII_CONFIG2_RX_PROG_SWAP, - RGMII_IO_MACRO_CONFIG2); - else - rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, - 0, RGMII_IO_MACRO_CONFIG2); + rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, rx_prog_swap, + RGMII_IO_MACRO_CONFIG2); /* Write 0x5 to PRG_RCLK_DLY_CODE */ rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE, @@ -525,13 +524,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) RGMII_IO_MACRO_CONFIG); rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, 0, RGMII_IO_MACRO_CONFIG2); - if (ethqos->has_emac_ge_3) - rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, - RGMII_CONFIG2_RX_PROG_SWAP, - RGMII_IO_MACRO_CONFIG2); - else - rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, - 0, RGMII_IO_MACRO_CONFIG2); + rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, rx_prog_swap, + RGMII_IO_MACRO_CONFIG2); + /* Write 0x5 to PRG_RCLK_DLY_CODE */ rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE, (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG); @@ -782,7 +777,7 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv) static int qcom_ethqos_probe(struct platform_device *pdev) { - struct device_node *np = pdev->dev.of_node; + struct device_node *np = pdev->dev.of_node, *root; const struct ethqos_emac_driver_data *data; struct plat_stmmacenet_data *plat_dat; struct stmmac_resources stmmac_res; @@ -810,6 +805,15 @@ 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,sa8540p-ride")) + ethqos->needs_rx_prog_swap = true; + else + ethqos->needs_rx_prog_swap = + of_property_read_bool(np, "qcom,rx-prog-swap"); + of_node_put(root); + switch (ethqos->phy_mode) { case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID:
The sampling timing of the MAC varies per board due to differences in hardware layout or peer PHY, even within the same version. For instance, the EMAC version on the qcs615-ride board is below 3, but it requires swapping the RX timing to enable 10M/100M speeds. Therefore, this setting should be adjustable rather than solely determined by the version. The RGMII_CONFIG2_RX_PROG_SWAP bit is used to switch the RX sampling timing between the rising edge and the falling edge of the clock. Consequently, the condition for setting this bit should be revised to ensure correct data sampling. The compatible string matching for 'qcom,sa8540p-ride' in this change is intended to ensure ABI compatibility for DTS files that do not include the new 'qcom,rx-prog-swap' property, allowing legacy boards to function as before. This board is the only one among legacy boards using RGMII and needs to enable RX swap. Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> --- .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 36 ++++++++++++---------- 1 file changed, 20 insertions(+), 16 deletions(-)