[Linux-stm32] [PATCH 2/3] net: stmmac: Add NXP S32 SoC family support
Marion & Christophe JAILLET
christophe.jaillet at wanadoo.fr
Sun Mar 17 07:47:55 UTC 2024
(adding back the mails and ML that got truncated by my mailer in my
first message. Sorry about that)
Le 17/03/2024 à 00:29, Wadim Mueller a écrit :
> On Sat, Mar 16, 2024 at 10:46:57AM +0100, Christophe JAILLET wrote:
>> Le 15/03/2024 à 23:27, Wadim Mueller a écrit :
>>> Add support for NXP S32 SoC family's GMAC to the stmmac network driver. This driver implementation is based on the patchset originally contributed by Chester Lin [1], which itself draws heavily from NXP's downstream implementation [2]. The patchset was never merged.
>>>
>>> The S32G2/3 SoCs feature multiple Ethernet interfaces (PFE0, PFE1, PFE2, and GMAC) which can be routed through a SerDes Subsystem, supporting various interfaces such as SGMII and RGMII. However, the current Glue Code lacks support for SerDes routing and pinctrl handling, relying solely on correct settings in U-Boot. Clock settings for this SoC are managed by the ATF Firmware.
>>>
>>> Changes made compared to [1]:
>>>
>>> Rebased onto Linux 6.8-rc7
>>> Consolidated into a single commit
>>> Minor adjustments in naming and usage of dev_err()/dev_info()
>>>
>>> Test Environment:
>>> The driver has been successfully tested on the official S32G-VNP-RDB3 Reference Design Board from NXP, utilizing an S32G3 SoC. The firmware and U-Boot used were from the BSP39 Release. The official BSP39 Ubuntu 22.04 Release was successfully booted. A network stress test using iperf [3] was also executed without issues.
>>>
>>> [1]https://patchwork.kernel.org/project/netdevbpf/patch/20221031101052.14956-6-clin@suse.com/#25068228
>>> [2]https://github.com/nxp-auto-linux/linux/blob/release/bsp39.0-5.15.129-rt/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c
>>> [3]https://linux.die.net/man/1/iperf
>>> [4]https://github.com/nxp-auto-linux/u-boot
>>> [5]https://github.com/nxp-auto-linux/arm-trusted-firmware
>>>
>>> Signed-off-by: Wadim Mueller<wafgo01 at gmail.com>
>>> ---
>>> drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 +
>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
>>> drivers/net/ethernet/stmicro/stmmac/common.h | 3 +
>>> .../net/ethernet/stmicro/stmmac/dwmac-s32.c | 313 ++++++++++++++++++
>>> .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 9 +
>>> .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 3 +
>>> drivers/net/ethernet/stmicro/stmmac/hwif.h | 5 +
>>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +
>>> include/linux/stmmac.h | 9 +
>>> 9 files changed, 362 insertions(+)
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
>>>
>> ...
>>
>>> + /* DMA cache coherency settings */
>>> + if (of_dma_is_coherent(pdev->dev.of_node)) {
>>> + ret = s32_config_cache_coherency(pdev, plat_dat);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + /* tx clock */
>>> + gmac->tx_clk = devm_clk_get(&pdev->dev, tx_clk);
>> Hi,
>>
>> If this make sense, here and ...
>>
>>> + if (IS_ERR(gmac->tx_clk)) {
>>> + dev_info(&pdev->dev, "tx clock not found\n");
>>> + gmac->tx_clk = NULL;
>>> + }
>>> +
>>> + /* rx clock */
>>> + gmac->rx_clk = devm_clk_get(&pdev->dev, rx_clk);
>> there could be devm_clk_get_optional_enabled() in order to remove
>> s32_gmac_exit() and simplify s32_gmac_init().
>>
> good point. If I understood you right the
> following patch would implement your proposal?
Yes, that's the idea.
Some other questions/remarks below.
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> @@ -48,24 +48,7 @@ static int s32_gmac_init(struct platform_device *pdev, void *priv)
s32_gmac_init() could now return void as it always succeed.
> {
> struct s32_priv_data *gmac = priv;
> u32 intf_sel;
> - int ret;
> -
> - if (gmac->tx_clk) {
> - ret = clk_prepare_enable(gmac->tx_clk);
> - if (ret) {
> - dev_err(&pdev->dev, "Can't set tx clock\n");
> - return ret;
> - }
> - }
> -
> - if (gmac->rx_clk) {
> - ret = clk_prepare_enable(gmac->rx_clk);
> - if (ret) {
> - dev_err(&pdev->dev, "Can't set rx clock\n");
> - return ret;
> - }
> - }
> -
> /* set interface mode */
> if (gmac->ctrl_sts) {
> switch (gmac->intf_mode) {
> @@ -102,16 +85,6 @@ static int s32_gmac_init(struct platform_device *pdev, void *priv)
> return 0;
> }
>
> -static void s32_gmac_exit(struct platform_device *pdev, void *priv)
> -{
> - struct s32_priv_data *gmac = priv;
> -
> - if (gmac->tx_clk)
> - clk_disable_unprepare(gmac->tx_clk);
> -
> - if (gmac->rx_clk)
> - clk_disable_unprepare(gmac->rx_clk);
> -}
>
> static void s32_fix_speed(void *priv, unsigned int speed, unsigned int mode)
> {
> @@ -236,14 +209,14 @@ static int s32_dwmac_probe(struct platform_device *pdev)
> }
>
> /* tx clock */
> - gmac->tx_clk = devm_clk_get(&pdev->dev, tx_clk);
> + gmac->tx_clk = devm_clk_get_optional_enabled(&pdev->dev, tx_clk);
> if (IS_ERR(gmac->tx_clk)) {
> dev_info(&pdev->dev, "tx clock not found\n");
> gmac->tx_clk = NULL;
I wonder if this should be return PTR_ERR(gmac->tx_clk); here, in order
to handle the -EPROBE_DEFER case.
with the _optional(), if we get an error, then the clock was found but
something is wrong. So I guess that we should return the error instead
of (silently) continuing.
> }
>
> /* rx clock */
> - gmac->rx_clk = devm_clk_get(&pdev->dev, rx_clk);
> + gmac->rx_clk = devm_clk_get_optional_enabled(&pdev->dev, rx_clk);
> if (IS_ERR(gmac->rx_clk)) {
> dev_info(&pdev->dev, "rx clock not found\n");
> gmac->rx_clk = NULL;
Same.
CJ
> @@ -258,7 +231,6 @@ static int s32_dwmac_probe(struct platform_device *pdev)
> plat_dat->pmt = 1;
>
> plat_dat->init = s32_gmac_init;
> - plat_dat->exit = s32_gmac_exit;
> plat_dat->fix_mac_speed = s32_fix_speed;
>
> /* safety feature config */
> @@ -267,7 +239,7 @@ static int s32_dwmac_probe(struct platform_device *pdev)
>
> if (!plat_dat->safety_feat_cfg) {
> dev_err(&pdev->dev, "allocate safety_feat_cfg failed\n");
> - goto err_gmac_exit;
> + return -ENODEV;
> }
>
> plat_dat->safety_feat_cfg->tsoee = 1;
> @@ -280,15 +252,7 @@ static int s32_dwmac_probe(struct platform_device *pdev)
> plat_dat->safety_feat_cfg->prtyen = 1;
> plat_dat->safety_feat_cfg->tmouten = 1;
>
> - ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> - if (ret)
> - goto err_gmac_exit;
> -
> - return 0;
> -
> -err_gmac_exit:
> - s32_gmac_exit(pdev, plat_dat->bsp_priv);
> - return ret;
> + return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> }
>
>> CJ
>>
>>> + if (IS_ERR(gmac->rx_clk)) {
>>> + dev_info(&pdev->dev, "rx clock not found\n");
>>> + gmac->rx_clk = NULL;
>>> + }
>> ...
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://st-md-mailman.stormreply.com/pipermail/linux-stm32/attachments/20240317/9a281f78/attachment.html>
More information about the Linux-stm32
mailing list