<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>(adding back the mails and ML that got truncated by my mailer in
my first message. Sorry about that)</p>
<p><br>
</p>
<div class="moz-cite-prefix">Le 17/03/2024 à 00:29, Wadim Mueller a
écrit :<br>
</div>
<blockquote type="cite"
cite="mid:20240316232928.GA108054@bhlegrsu.conti.de">
<pre class="moz-quote-pre" wrap="">On Sat, Mar 16, 2024 at 10:46:57AM +0100, Christophe JAILLET wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Le 15/03/2024 à 23:27, Wadim Mueller a écrit :
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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] <a class="moz-txt-link-freetext" href="https://patchwork.kernel.org/project/netdevbpf/patch/20221031101052.14956-6-clin@suse.com/#25068228">https://patchwork.kernel.org/project/netdevbpf/patch/20221031101052.14956-6-clin@suse.com/#25068228</a>
[2] <a class="moz-txt-link-freetext" href="https://github.com/nxp-auto-linux/linux/blob/release/bsp39.0-5.15.129-rt/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c">https://github.com/nxp-auto-linux/linux/blob/release/bsp39.0-5.15.129-rt/drivers/net/ethernet/stmicro/stmmac/dwmac-s32cc.c</a>
[3] <a class="moz-txt-link-freetext" href="https://linux.die.net/man/1/iperf">https://linux.die.net/man/1/iperf</a>
[4] <a class="moz-txt-link-freetext" href="https://github.com/nxp-auto-linux/u-boot">https://github.com/nxp-auto-linux/u-boot</a>
[5] <a class="moz-txt-link-freetext" href="https://github.com/nxp-auto-linux/arm-trusted-firmware">https://github.com/nxp-auto-linux/arm-trusted-firmware</a>
Signed-off-by: Wadim Mueller <a class="moz-txt-link-rfc2396E" href="mailto:wafgo01@gmail.com"><wafgo01@gmail.com></a>
---
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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
...
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ /* 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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Hi,
If this make sense, here and ...
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
there could be devm_clk_get_optional_enabled() in order to remove
s32_gmac_exit() and simplify s32_gmac_init().
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
good point. If I understood you right the
following patch would implement your proposal?</pre>
</blockquote>
<p><br>
</p>
<p>Yes, that's the idea.<br>
Some other questions/remarks below.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20240316232928.GA108054@bhlegrsu.conti.de">
<pre class="moz-quote-pre" wrap="">
--- 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><span
style="padding: 0px; tab-size: 8;"
class="hljs diff colorediffs language-diff"><span
class="hljs-addition">
s32_gmac_init() could now return void as it always succeed.
</span></span></pre>
<blockquote type="cite"
cite="mid:20240316232928.GA108054@bhlegrsu.conti.de">
<pre class="moz-quote-pre" wrap=""> {
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;
</pre>
</blockquote>
<p><br>
</p>
<p>I wonder if this should be return PTR_ERR(gmac->tx_clk); here,
in order to handle the -EPROBE_DEFER case.<br>
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.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20240316232928.GA108054@bhlegrsu.conti.de">
<pre class="moz-quote-pre" wrap=""> }
/* 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;</pre>
</blockquote>
<p><br>
</p>
<p>Same.</p>
<p><br>
</p>
<p>CJ<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20240316232928.GA108054@bhlegrsu.conti.de">
<pre class="moz-quote-pre" wrap="">
@@ -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);
}
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">CJ
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (IS_ERR(gmac->rx_clk)) {
+ dev_info(&pdev->dev, "rx clock not found\n");
+ gmac->rx_clk = NULL;
+ }
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
...
</pre>
</blockquote>
</blockquote>
</body>
</html>