Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Johannes Zink <j.zink@pengutronix.de>]

Hi Shenwei,

On 8/2/23 16:27, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Johannes Zink <j.zink@pengutronix.de>
>> Sent: Wednesday, August 2, 2023 1:26 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
>> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
>> Abeni <pabeni@redhat.com>; Maxime Coquelin
>> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; Sascha
>> Hauer <s.hauer@pengutronix.de>; Neil Armstrong <neil.armstrong@linaro.org>;
>> Kevin Hilman <khilman@baylibre.com>; Vinod Koul <vkoul@kernel.org>; Chen-
>> Yu Tsai <wens@csie.org>; Jernej Skrabec <jernej.skrabec@gmail.com>; Samuel
>> Holland <samuel@sholland.org>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>> <jbrunet@baylibre.com>; Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen
>> Henneberg <jh@henneberg-systemdesign.com>; netdev@vger.kernel.org; linux-
>> stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>> Subject: Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the
>> TXC clock in fixed-link
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report this
>> email' button
>>
>>
>> Hi Shenwei,
>>
>> On 8/1/23 19:10, Shenwei Wang wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Johannes Zink <j.zink@pengutronix.de>
>>>> Sent: Tuesday, August 1, 2023 7:48 AM
>>>> To: Shenwei Wang <shenwei.wang@nxp.com>; Russell King
>>>> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Eric
>>>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
>>>> Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
>>>> <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
>> Sascha
>>>> Hauer <s.hauer@pengutronix.de>; Neil Armstrong
>>>> <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>;
>>>> Vinod Koul <vkoul@kernel.org>; Chen- Yu Tsai <wens@csie.org>; Jernej
>>>> Skrabec <jernej.skrabec@gmail.com>; Samuel Holland
>>>> <samuel@sholland.org>
>>>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
>>>> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
>>>> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>>>> <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet
>>>> <jbrunet@baylibre.com>; Martin Blumenstingl
>>>> <martin.blumenstingl@googlemail.com>; Bhupesh Sharma
>>>> <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu
>>>> <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman
>>>> <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>;
>>>> Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee
>>>> <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>;
>>>> Jochen Henneberg <jh@henneberg-systemdesign.com>;
>>>> netdev@vger.kernel.org; linux- stm32@st-md-mailman.stormreply.com;
>>>> linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
>>>> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
>>>> Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause
>>>> the TXC clock in fixed-link
>>>>
>>>> Caution: This is an external email. Please take care when clicking
>>>> links or opening attachments. When in doubt, report the message using
>>>> the 'Report this email' button
>>>>
>>>>
>>>> Hi Shenwei,
>>>>
>>>> thanks for your patch.
>>>>
>>>> On 7/31/23 18:19, Shenwei Wang wrote:
>>>>> When using a fixed-link setup, certain devices like the SJA1105
>>>>> require a small pause in the TXC clock line to enable their internal
>>>>> tunable delay line (TDL).
>>>>
>>>> If this is only required for some devices, is it safe to enforce this
>>>> behaviour unconditionally for any kind of fixed link devices
>>>> connected to the MX93 EQOS or could this possibly break for other devices?
>>>>
>>>
>>> It won't impact normal devices. The link layer hasn't built up yet.
>>>
>>
>> As Russel suggested in [1] - maybe you could rephrase your commit message for
>> your v4 to point this out to future reviewers (apparently multiple people have
>> had questions about this...)  and have this fact also recorded in the git log later
>> on.
>>
> 
> Okay.
> 
>> Also: does this only apply to i.MX93, or would we have to test and enable it on
>> e.g. i.MX8MP as well?
>>
> 
> Yes, it is required when the EQOS MAC is selected. However, this patch just enables
> The feature on i.MX93.

If this behaviour is required on all EQOS, I think the name 
imx_dwmac_fix_speed_mx93() is misleading. It should either be 
imx_dwmac_fix_speed() if applicable to all imx implementations, or 
dwmac_fix_speed() (and moved to a non-gluecode file) if applicable for all 
implementations in general.

You can then add a second patch for enabling it for the i.mx93 in the gluecode 
driver.

Johannes


> 
> Thanks,
> Shenwei
> 
>> Thanks
>> Johannes
>>
>> [1] ZMk/xqRP67zXHNrf@shell.armlinux.org.uk
>>
>>
>>> Thanks,
>>> Shenwei
>>>
>>>> Best regards
>>>> Johannes
>>>>
>>>>>
>>>>> To satisfy this requirement, this patch temporarily disables the TX
>>>>> clock, and restarts it after a required period. This provides the
>>>>> required silent interval on the clock line for SJA1105 to complete
>>>>> the frequency transition and enable the internal TDLs.
>>>>>
>>>>> So far we have only enabled this feature on the i.MX93 platform.
>>>>>
>>>>> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
>>>>> Reviewed-by: Frank Li <frank.li@nxp.com>
>>>>> ---
>>>>>     .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 42
>> +++++++++++++++++++
>>>>>     1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> index 53ee5a42c071..2e4173d099f3 100644
>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
>>>>> @@ -32,6 +32,7 @@
>>>>>     #define GPR_ENET_QOS_RGMII_EN               (0x1 << 21)
>>>>>
>>>>>     #define MX93_GPR_ENET_QOS_INTF_MODE_MASK    GENMASK(3, 0)
>>>>> +#define MX93_GPR_ENET_QOS_INTF_MASK          GENMASK(3, 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_MII              (0x0 << 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RMII             (0x4 << 1)
>>>>>     #define MX93_GPR_ENET_QOS_INTF_SEL_RGMII    (0x1 << 1)
>>>>> @@ -40,6 +41,7 @@
>>>>>     #define DMA_BUS_MODE                        0x00001000
>>>>>     #define DMA_BUS_MODE_SFT_RESET              (0x1 << 0)
>>>>>     #define RMII_RESET_SPEED            (0x3 << 14)
>>>>> +#define CTRL_SPEED_MASK                      GENMASK(15, 14)
>>>>>
>>>>>     struct imx_dwmac_ops {
>>>>>         u32 addr_width;
>>>>> @@ -56,6 +58,7 @@ struct imx_priv_data {
>>>>>         struct regmap *intf_regmap;
>>>>>         u32 intf_reg_off;
>>>>>         bool rmii_refclk_ext;
>>>>> +     void __iomem *base_addr;
>>>>>
>>>>>         const struct imx_dwmac_ops *ops;
>>>>>         struct plat_stmmacenet_data *plat_dat; @@ -212,6 +215,42 @@
>>>>> static void imx_dwmac_fix_speed(void *priv, uint speed, uint mode)
>>>>>                 dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
>>>>>     }
>>>>>
>>>>> +static void imx_dwmac_fix_speed_mx93(void *priv, uint speed, uint
>>>>> +mode) {
>>>>> +     struct imx_priv_data *dwmac = priv;
>>>>> +     int ctrl, old_ctrl, iface;
>>>>> +
>>>>> +     imx_dwmac_fix_speed(priv, speed, mode);
>>>>> +
>>>>> +     if (!dwmac || mode != MLO_AN_FIXED)
>>>>> +             return;
>>>>> +
>>>>> +     if (regmap_read(dwmac->intf_regmap, dwmac->intf_reg_off, &iface))
>>>>> +             return;
>>>>> +
>>>>> +     iface &= MX93_GPR_ENET_QOS_INTF_MASK;
>>>>> +     if (iface != MX93_GPR_ENET_QOS_INTF_SEL_RGMII)
>>>>> +             return;
>>>>> +
>>>>> +     old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
>>>>> +     ctrl = old_ctrl & ~CTRL_SPEED_MASK;
>>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, 0);
>>>>> +     writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
>>>>> +
>>>>> +     /* Ensure the settings for CTRL are applied and avoid CPU/Compiler
>>>>> +      * reordering.
>>>>> +      */
>>>>> +     wmb();
>>>>> +
>>>>> +     usleep_range(10, 20);
>>>>> +     iface |= MX93_GPR_ENET_QOS_CLK_GEN_EN;
>>>>> +     regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
>>>>> +                        MX93_GPR_ENET_QOS_INTF_MODE_MASK, iface);
>>>>> +
>>>>> +     writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG); }
>>>>> +
>>>>>     static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
>>>>>     {
>>>>>         struct plat_stmmacenet_data *plat_dat = priv; @@ -317,8
>>>>> +356,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
>>>>>         plat_dat->exit = imx_dwmac_exit;
>>>>>         plat_dat->clks_config = imx_dwmac_clks_config;
>>>>>         plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
>>>>> +     if (of_machine_is_compatible("fsl,imx93"))
>>>>> +             plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
>>>>>         plat_dat->bsp_priv = dwmac;
>>>>>         dwmac->plat_dat = plat_dat;
>>>>> +     dwmac->base_addr = stmmac_res.addr;
>>>>>
>>>>>         ret = imx_dwmac_clks_config(dwmac, true);
>>>>>         if (ret)
>>>>
>>>> --
>>>> Pengutronix e.K.                | Johannes Zink                  |
>>>> Steuerwalder Str. 21            |
>>>> https://www/
>>>> .pe%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d60a461
>> ee01408
>>>>
>> db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826554
>> 36335
>>>>
>> 61986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJ
>>>>
>> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CV10o1M%2BOj
>> DPOaH5C
>>>> y%2Fka%2B0aOMs0IaVapMH7aa3RnTI%3D&reserved=0
>>>>
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7C761fbb75c
>>>>
>> 1c24cfe091508db928d8ade%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>>>
>> 0%7C638264908852977732%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>>>>
>> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
>>>>
>> 7C&sdata=2l2zNfIaNnRJENmERehNae8g%2F%2BQqlxD2YRx7ksY2X%2BE%3D&r
>>>> eserved=0    |
>>>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>>>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
>>>
>>>
>>
>> --
>> Pengutronix e.K.                | Johannes Zink                  |
>> Steuerwalder Str. 21            |
>> https://www.pe/
>> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Ccfd142f0d
>> 60a461ee01408db9321578d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
>> C0%7C638265543633561986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
>> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
>> %7C&sdata=yKzNPsHqD%2FxU%2FRmzLn4JSQjmuT9tU8SabLxHyGTTmms%3D&r
>> eserved=0    |
>> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
>> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel