Hi Shenwei,
On 8/2/23 16:27, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Johannes Zink <[email protected]>
>> Sent: Wednesday, August 2, 2023 1:26 AM
>> To: Shenwei Wang <[email protected]>; Russell King
>> <[email protected]>; David S. Miller <[email protected]>; Eric
>> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
>> Abeni <[email protected]>; Maxime Coquelin
>> <[email protected]>; Shawn Guo <[email protected]>; Sascha
>> Hauer <[email protected]>; Neil Armstrong <[email protected]>;
>> Kevin Hilman <[email protected]>; Vinod Koul <[email protected]>; Chen-
>> Yu Tsai <[email protected]>; Jernej Skrabec <[email protected]>; Samuel
>> Holland <[email protected]>
>> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
>> <[email protected]>; Jose Abreu <[email protected]>;
>> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
>> <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
>> <[email protected]>; Martin Blumenstingl
>> <[email protected]>; Bhupesh Sharma
>> <[email protected]>; Nobuhiro Iwamatsu
>> <[email protected]>; Simon Horman
>> <[email protected]>; Andrew Halaney <[email protected]>;
>> Bartosz Golaszewski <[email protected]>; Wong Vee Khee
>> <[email protected]>; Revanth Kumar Uppala <[email protected]>; Jochen
>> Henneberg <[email protected]>; [email protected]; linux-
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; Frank Li <[email protected]>
>> 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 <[email protected]>
>>>> Sent: Tuesday, August 1, 2023 7:48 AM
>>>> To: Shenwei Wang <[email protected]>; Russell King
>>>> <[email protected]>; David S. Miller <[email protected]>; Eric
>>>> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
>>>> Paolo Abeni <[email protected]>; Maxime Coquelin
>>>> <[email protected]>; Shawn Guo <[email protected]>;
>> Sascha
>>>> Hauer <[email protected]>; Neil Armstrong
>>>> <[email protected]>; Kevin Hilman <[email protected]>;
>>>> Vinod Koul <[email protected]>; Chen- Yu Tsai <[email protected]>; Jernej
>>>> Skrabec <[email protected]>; Samuel Holland
>>>> <[email protected]>
>>>> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
>>>> <[email protected]>; Jose Abreu <[email protected]>;
>>>> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
>>>> <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
>>>> <[email protected]>; Martin Blumenstingl
>>>> <[email protected]>; Bhupesh Sharma
>>>> <[email protected]>; Nobuhiro Iwamatsu
>>>> <[email protected]>; Simon Horman
>>>> <[email protected]>; Andrew Halaney <[email protected]>;
>>>> Bartosz Golaszewski <[email protected]>; Wong Vee Khee
>>>> <[email protected]>; Revanth Kumar Uppala <[email protected]>;
>>>> Jochen Henneberg <[email protected]>;
>>>> [email protected]; linux- [email protected];
>>>> [email protected];
>>>> [email protected]; [email protected];
>>>> [email protected]; Frank Li <[email protected]>
>>>> 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/[email protected]
>>
>>
>>> 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 <[email protected]>
>>>>> Reviewed-by: Frank Li <[email protected]>
>>>>> ---
>>>>> .../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 |
> -----Original Message-----
> From: Johannes Zink <[email protected]>
> Sent: Wednesday, August 2, 2023 9:40 AM
> To: Shenwei Wang <[email protected]>; Russell King
> <[email protected]>; David S. Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; Shawn Guo <[email protected]>; Sascha
> Hauer <[email protected]>; Neil Armstrong <[email protected]>;
> Kevin Hilman <[email protected]>; Vinod Koul <[email protected]>; Chen-
> Yu Tsai <[email protected]>; Jernej Skrabec <[email protected]>; Samuel
> Holland <[email protected]>
> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
> <[email protected]>; Martin Blumenstingl
> <[email protected]>; Bhupesh Sharma
> <[email protected]>; Nobuhiro Iwamatsu
> <[email protected]>; Simon Horman
> <[email protected]>; Andrew Halaney <[email protected]>;
> Bartosz Golaszewski <[email protected]>; Wong Vee Khee
> <[email protected]>; Revanth Kumar Uppala <[email protected]>; Jochen
> Henneberg <[email protected]>; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Frank Li <[email protected]>
> 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/2/23 16:27, Shenwei Wang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Johannes Zink <[email protected]>
> >> Sent: Wednesday, August 2, 2023 1:26 AM
> >> To: Shenwei Wang <[email protected]>; Russell King
> >> <[email protected]>; David S. Miller <[email protected]>; Eric
> >> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> >> Paolo Abeni <[email protected]>; Maxime Coquelin
> >> <[email protected]>; Shawn Guo <[email protected]>;
> Sascha
> >> Hauer <[email protected]>; Neil Armstrong
> >> <[email protected]>; Kevin Hilman <[email protected]>;
> >> Vinod Koul <[email protected]>; Chen- Yu Tsai <[email protected]>; Jernej
> >> Skrabec <[email protected]>; Samuel Holland
> >> <[email protected]>
> >> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
> >> <[email protected]>; Jose Abreu <[email protected]>;
> >> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> >> <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
> >> <[email protected]>; Martin Blumenstingl
> >> <[email protected]>; Bhupesh Sharma
> >> <[email protected]>; Nobuhiro Iwamatsu
> >> <[email protected]>; Simon Horman
> >> <[email protected]>; Andrew Halaney <[email protected]>;
> >> Bartosz Golaszewski <[email protected]>; Wong Vee Khee
> >> <[email protected]>; Revanth Kumar Uppala <[email protected]>;
> >> Jochen Henneberg <[email protected]>;
> >> [email protected]; linux- [email protected];
> >> [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; Frank Li <[email protected]>
> >> 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 <[email protected]>
> >>>> Sent: Tuesday, August 1, 2023 7:48 AM
> >>>> To: Shenwei Wang <[email protected]>; Russell King
> >>>> <[email protected]>; David S. Miller <[email protected]>;
> >>>> Eric Dumazet <[email protected]>; Jakub Kicinski
> >>>> <[email protected]>; Paolo Abeni <[email protected]>; Maxime Coquelin
> >>>> <[email protected]>; Shawn Guo <[email protected]>;
> >> Sascha
> >>>> Hauer <[email protected]>; Neil Armstrong
> >>>> <[email protected]>; Kevin Hilman <[email protected]>;
> >>>> Vinod Koul <[email protected]>; Chen- Yu Tsai <[email protected]>;
> >>>> Jernej Skrabec <[email protected]>; Samuel Holland
> >>>> <[email protected]>
> >>>> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
> >>>> <[email protected]>; Jose Abreu <[email protected]>;
> >>>> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> >>>> <[email protected]>; dl-linux-imx <[email protected]>; Jerome
> >>>> Brunet <[email protected]>; Martin Blumenstingl
> >>>> <[email protected]>; Bhupesh Sharma
> >>>> <[email protected]>; Nobuhiro Iwamatsu
> >>>> <[email protected]>; Simon Horman
> >>>> <[email protected]>; Andrew Halaney
> <[email protected]>;
> >>>> Bartosz Golaszewski <[email protected]>; Wong Vee Khee
> >>>> <[email protected]>; Revanth Kumar Uppala <[email protected]>;
> >>>> Jochen Henneberg <[email protected]>;
> >>>> [email protected]; linux- [email protected];
> >>>> [email protected];
> >>>> [email protected]; [email protected];
> >>>> [email protected]; Frank Li <[email protected]>
> >>>> 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.
>
It has the general fix_speed function there named imx_dwmac_fix_speed.
This one is the special for this mx93 fix.
Thanks,
Shenwei
> 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/[email protected]
> >>
> >>
> >>> 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 <[email protected]>
> >>>>> Reviewed-by: Frank Li <[email protected]>
> >>>>> ---
> >>>>> .../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%7Ccfd142f0d60a4
> 61
> >> 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%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cdc64404f8c2c4e
> b87a7808
> >>
> db93666ec9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63826584
> 03801
> >>
> 74614%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJ
> >>
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oxLnb3ppqjhMti
> cQH7P
> >> lfRbIlYJ2R1Z8Tg7Bz2vC%2F%2Bc%3D&reserved=0
> >>
> 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%3
> D&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%7Cdc64404f8
> c2c4eb87a7808db93666ec9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638265840380174614%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=r8tFe0Ts3ev2c7lg3MK0Qc40101d7W%2BEwnpmvMDwjho%3D&res
> erved=0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 |
Hi Shenwei,
[snip]
>>>
>>>> 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.
>>
>
> It has the general fix_speed function there named imx_dwmac_fix_speed.
> This one is the special for this mx93 fix.
I think I might have misunderstood your last statement or I failed to express
my point. If you need to replace the dwmac_fix_speed() on mx93, because this
SoC implementation requires doing so (the usual reason for doing something like
this is something like reset quirks because of screwed up IP Core integration),
then your approach is imho valid.
But if I got your last comment right, your changes should apply to EQOS MAC in
general (but you want to only enable it for mx93 at the moment). In this case
this quirk will later be as the fix_mac_speed function for other hardware as
well, in which case the name ..._mx93 is misleading, and imho rather a
descriptive name should be used (i.e. have the name describe what it does
rather than for what hardware it is implemented).
Except if the maintainers have a strong opinion that the ..._mx93 suffix
version is exactly how you should proceed...
Best regards
Johannes
>
> Thanks,
> Shenwei
> [snip]
--
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 |
> -----Original Message-----
> From: Johannes Zink <[email protected]>
> Sent: Thursday, August 3, 2023 1:36 AM
> To: Shenwei Wang <[email protected]>; Russell King
> <[email protected]>; David S. Miller <[email protected]>; Eric
> Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo
> Abeni <[email protected]>; Maxime Coquelin
> <[email protected]>; Shawn Guo <[email protected]>; Sascha
> Hauer <[email protected]>; Neil Armstrong <[email protected]>;
> Kevin Hilman <[email protected]>; Vinod Koul <[email protected]>; Chen-
> Yu Tsai <[email protected]>; Jernej Skrabec <[email protected]>; Samuel
> Holland <[email protected]>
> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
> <[email protected]>; Jose Abreu <[email protected]>;
> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
> <[email protected]>; Martin Blumenstingl
> <[email protected]>; Bhupesh Sharma
> <[email protected]>; Nobuhiro Iwamatsu
> <[email protected]>; Simon Horman
> > It has the general fix_speed function there named imx_dwmac_fix_speed.
> > This one is the special for this mx93 fix.
>
> I think I might have misunderstood your last statement or I failed to express my
> point. If you need to replace the dwmac_fix_speed() on mx93, because this SoC
> implementation requires doing so (the usual reason for doing something like this
> is something like reset quirks because of screwed up IP Core integration), then
> your approach is imho valid.
>
> But if I got your last comment right, your changes should apply to EQOS MAC in
> general (but you want to only enable it for mx93 at the moment). In this case
> this quirk will later be as the fix_mac_speed function for other hardware as well,
> in which case the name ..._mx93 is misleading, and imho rather a descriptive
> name should be used (i.e. have the name describe what it does rather than for
> what hardware it is implemented).
>
The idea of supporting the SJA1105 is general, but the implementation depends on the
specific SoC. This patch provides the implementation for the MX93 SoC. To support the
MX8x SoCs, the implementation would need to be rewritten based on the current state
of the dwmac-imx driver.
I agree the function name is somewhat ugly. Maybe a better name could be:
mx93_dwmac_fix_speed()
The assumption is that the process of pausing the clock can still be considered part of fixing the speed.
Thanks,
Shenwei
> Except if the maintainers have a strong opinion that the ..._mx93 suffix version
> is exactly how you should proceed...
>
> Best regards
> Johannes
>
> >
> > Thanks,
> > Shenwei
> > [snip]
>
>
> --
> Pengutronix e.K. | Johannes Zink |
> Steuerwalder Str. 21 |
> https://www.pe/
> ngutronix.de%2F&data=05%7C01%7Cshenwei.wang%40nxp.com%7Cd328d89d
> 14e847270d5a08db93ebff01%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638266414027048795%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=dkE9Es7kl3uNYx8zJYZt2r6933dSNtYDpQzCEagAV3M%3D&reserved
> =0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 |