STM32MP13 is STM32 SOC with 2 GMACs instances
This board have 2 RMII phy:
-Ethernet1: RMII with crystal
-Ethernet2: RMII without crystal
Rework dwmac glue to simplify management for next stm32
Add support for PHY regulator
V2: - Remark from Rob Herring (add Krzysztof's ack in patch 02/11, update in yaml)
Remark from Serge Semin (upate commits msg)
Christophe Roullier (11):
dt-bindings: net: add STM32MP13 compatible in documentation for stm32
dt-bindings: net: add phy-supply property for stm32
net: stmmac: dwmac-stm32: rework glue to simplify management
net: stmmac: dwmac-stm32: add management of stm32mp13
net: stmmac: dwmac-stm32: update config management for phy wo cristal
net: stmmac: dwmac-stm32: clean the way to manage wol irqwake
net: stmmac: dwmac-stm32: support the phy-supply regulator binding
ARM: dts: stm32: add ethernet1 and ethernet2 support on stm32mp13
ARM: dts: stm32: add ethernet1/2 RMII pins for STM32MP13F-DK board
ARM: dts: stm32: add ethernet1 and ethernet2 for STM32MP135F-DK board
ARM: multi_v7_defconfig: Add MCP23S08 pinctrl support
.../devicetree/bindings/net/stm32-dwmac.yaml | 79 +++++-
arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi | 71 ++++++
arch/arm/boot/dts/st/stm32mp131.dtsi | 31 +++
arch/arm/boot/dts/st/stm32mp133.dtsi | 30 +++
arch/arm/boot/dts/st/stm32mp135f-dk.dts | 48 ++++
arch/arm/configs/multi_v7_defconfig | 1 +
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 235 ++++++++++++------
7 files changed, 416 insertions(+), 79 deletions(-)
--
2.25.1
Both instances ethernet based on GMAC SNPS IP on stm32mp13.
GMAC IP version is SNPS 4.20.
Signed-off-by: Christophe Roullier <[email protected]>
---
arch/arm/boot/dts/st/stm32mp131.dtsi | 31 ++++++++++++++++++++++++++++
arch/arm/boot/dts/st/stm32mp133.dtsi | 30 +++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp131.dtsi b/arch/arm/boot/dts/st/stm32mp131.dtsi
index ecfa120827ba..00d258d2032e 100644
--- a/arch/arm/boot/dts/st/stm32mp131.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp131.dtsi
@@ -854,6 +854,37 @@ crc1: crc@58009000 {
status = "disabled";
};
+ ethernet1: ethernet@5800a000 {
+ compatible = "st,stm32mp13-dwmac", "snps,dwmac-4.20a";
+ reg = <0x5800a000 0x2000>;
+ reg-names = "stmmaceth";
+ interrupts-extended = <&intc GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
+ <&exti 68 1>;
+ interrupt-names = "macirq", "eth_wake_irq";
+ clock-names = "stmmaceth",
+ "mac-clk-tx",
+ "mac-clk-rx",
+ "ethstp",
+ "eth-ck";
+ clocks = <&rcc ETH1MAC>,
+ <&rcc ETH1TX>,
+ <&rcc ETH1RX>,
+ <&rcc ETH1STP>,
+ <&rcc ETH1CK_K>;
+ st,syscon = <&syscfg 0x4 0xff0000>;
+ snps,mixed-burst;
+ snps,pbl = <2>;
+ snps,axi-config = <&stmmac_axi_config_1>;
+ snps,tso;
+ status = "disabled";
+
+ stmmac_axi_config_1: stmmac-axi-config {
+ snps,wr_osr_lmt = <0x7>;
+ snps,rd_osr_lmt = <0x7>;
+ snps,blen = <0 0 0 0 16 8 4>;
+ };
+ };
+
usbh_ohci: usb@5800c000 {
compatible = "generic-ohci";
reg = <0x5800c000 0x1000>;
diff --git a/arch/arm/boot/dts/st/stm32mp133.dtsi b/arch/arm/boot/dts/st/stm32mp133.dtsi
index 3e394c8e58b9..2796d15276df 100644
--- a/arch/arm/boot/dts/st/stm32mp133.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp133.dtsi
@@ -68,4 +68,34 @@ channel@18 {
};
};
};
+
+ ethernet2: ethernet@5800e000 {
+ compatible = "st,stm32mp13-dwmac", "snps,dwmac-4.20a";
+ reg = <0x5800e000 0x2000>;
+ reg-names = "stmmaceth";
+ interrupts-extended = <&intc GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "macirq";
+ clock-names = "stmmaceth",
+ "mac-clk-tx",
+ "mac-clk-rx",
+ "ethstp",
+ "eth-ck";
+ clocks = <&rcc ETH2MAC>,
+ <&rcc ETH2TX>,
+ <&rcc ETH2RX>,
+ <&rcc ETH2STP>,
+ <&rcc ETH2CK_K>;
+ st,syscon = <&syscfg 0x4 0xff000000>;
+ snps,mixed-burst;
+ snps,pbl = <2>;
+ snps,axi-config = <&stmmac_axi_config_2>;
+ snps,tso;
+ status = "disabled";
+
+ stmmac_axi_config_2: stmmac-axi-config {
+ snps,wr_osr_lmt = <0x7>;
+ snps,rd_osr_lmt = <0x7>;
+ snps,blen = <0 0 0 0 16 8 4>;
+ };
+ };
};
--
2.25.1
Phandle to a regulator that provides power to the PHY. This
regulator will be managed during the PHY power on/off sequence.
Acked-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Christophe Roullier <[email protected]>
---
Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
index b901a432dfa9..7c3aa181abcb 100644
--- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
@@ -84,6 +84,9 @@ properties:
- description: offset of the control register
- description: field to set mask in register
+ phy-supply:
+ description: PHY regulator
+
st,eth-clk-sel:
description:
set this property in RGMII PHY when you want to select RCC clock instead of ETH_CLK125.
--
2.25.1
Add Ethernet support for STM32MP13.
STM32MP13 is STM32 SOC with 2 GMACs instances.
GMAC IP version is SNPS 4.20.
GMAC IP configure with 1 RX and 1 TX queue.
DMA HW capability register supported
RX Checksum Offload Engine supported
TX Checksum insertion supported
Wake-Up On Lan supported
TSO supported
Signed-off-by: Christophe Roullier <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index 68a02de25ac7..7529a8d15492 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -517,9 +517,30 @@ static struct stm32_ops stm32mp1_dwmac_data = {
}
};
+static struct stm32_ops stm32mp13_dwmac_data = {
+ .set_mode = stm32mp1_set_mode,
+ .suspend = stm32mp1_suspend,
+ .resume = stm32mp1_resume,
+ .parse_data = stm32mp1_parse_data,
+ .clk_rx_enable_in_suspend = true,
+ .syscfg_clr_off = 0x08,
+ .pmcsetr = {
+ .eth1_clk_sel = BIT(16),
+ .eth1_ref_clk_sel = BIT(17),
+ .eth1_selmii = 0,
+ .eth1_sel_rgmii = BIT(21),
+ .eth1_sel_rmii = BIT(23),
+ .eth2_clk_sel = BIT(24),
+ .eth2_ref_clk_sel = BIT(25),
+ .eth2_sel_rgmii = BIT(29),
+ .eth2_sel_rmii = BIT(31)
+ }
+};
+
static const struct of_device_id stm32_dwmac_match[] = {
{ .compatible = "st,stm32-dwmac", .data = &stm32mcu_dwmac_data},
{ .compatible = "st,stm32mp1-dwmac", .data = &stm32mp1_dwmac_data},
+ { .compatible = "st,stm32mp13-dwmac", .data = &stm32mp13_dwmac_data},
{ }
};
MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
--
2.25.1
On STM32 platforms it is no longer needed to use a dedicated wakeup to
wake up system from CStop. This patch removes the dedicated wake up usage
and clean the way to register the wake up irq.
Signed-off-by: Christophe Roullier <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 66 +++++++++----------
1 file changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index e648c4e790a7..5d8f4eac4bc0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -81,7 +81,6 @@ struct stm32_dwmac {
int enable_eth_ck;
int eth_clk_sel_reg;
int eth_ref_clk_sel_reg;
- int irq_pwr_wakeup;
u32 mode_reg; /* MAC glue-logic mode register */
u32 mode_mask;
struct regmap *regmap;
@@ -312,9 +311,7 @@ static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
struct device_node *np = dev->of_node;
- int err = 0;
/* Ethernet PHY have no crystal */
dwmac->ext_phyclk = of_property_read_bool(np, "st,ext-phyclk");
@@ -346,29 +343,24 @@ static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
if (IS_ERR(dwmac->syscfg_clk))
dwmac->syscfg_clk = NULL;
- /* Get IRQ information early to have an ability to ask for deferred
- * probe if needed before we went too far with resource allocation.
- */
- dwmac->irq_pwr_wakeup = platform_get_irq_byname_optional(pdev,
- "stm32_pwr_wakeup");
- if (dwmac->irq_pwr_wakeup == -EPROBE_DEFER)
- return -EPROBE_DEFER;
-
- if (!dwmac->clk_eth_ck && dwmac->irq_pwr_wakeup >= 0) {
- err = device_init_wakeup(&pdev->dev, true);
- if (err) {
- dev_err(&pdev->dev, "Failed to init wake up irq\n");
- return err;
- }
- err = dev_pm_set_dedicated_wake_irq(&pdev->dev,
- dwmac->irq_pwr_wakeup);
- if (err) {
- dev_err(&pdev->dev, "Failed to set wake up irq\n");
- device_init_wakeup(&pdev->dev, false);
- }
- device_set_wakeup_enable(&pdev->dev, false);
+ return 0;
+}
+
+static int stm32_dwmac_wake_init(struct device *dev,
+ struct stmmac_resources *stmmac_res)
+{
+ int err;
+
+ device_set_wakeup_capable(dev, true);
+
+ err = dev_pm_set_wake_irq(dev, stmmac_res->wol_irq);
+ if (err) {
+ dev_err(dev, "Failed to set wake up irq\n");
+ device_set_wakeup_capable(dev, false);
+ return err;
}
- return err;
+
+ return 0;
}
static int stm32_dwmac_probe(struct platform_device *pdev)
@@ -406,11 +398,17 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
return ret;
}
+ if (stmmac_res.wol_irq && !dwmac->clk_eth_ck) {
+ ret = stm32_dwmac_wake_init(&pdev->dev, &stmmac_res);
+ if (ret)
+ goto err_wake_init_disable;
+ }
+
plat_dat->bsp_priv = dwmac;
ret = stm32_dwmac_init(plat_dat, false);
if (ret)
- return ret;
+ goto err_wake_init_disable;
ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
if (ret)
@@ -420,7 +418,11 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
err_clk_disable:
stm32_dwmac_clk_disable(dwmac, false);
-
+err_wake_init_disable:
+ if (stmmac_res.wol_irq && !dwmac->clk_eth_ck) {
+ dev_pm_clear_wake_irq(&pdev->dev);
+ device_set_wakeup_capable(&pdev->dev, false);
+ }
return ret;
}
@@ -428,16 +430,12 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
{
struct net_device *ndev = platform_get_drvdata(pdev);
struct stmmac_priv *priv = netdev_priv(ndev);
- struct stm32_dwmac *dwmac = priv->plat->bsp_priv;
stmmac_dvr_remove(&pdev->dev);
+ stm32_dwmac_clk_disable(priv->plat->bsp_priv, false);
- stm32_dwmac_clk_disable(dwmac, false);
-
- if (dwmac->irq_pwr_wakeup >= 0) {
- dev_pm_clear_wake_irq(&pdev->dev);
- device_init_wakeup(&pdev->dev, false);
- }
+ dev_pm_clear_wake_irq(&pdev->dev);
+ device_init_wakeup(&pdev->dev, false);
}
static int stm32mp1_suspend(struct stm32_dwmac *dwmac)
--
2.25.1
Configure the phy regulator if defined by the "phy-supply" DT phandle.
Signed-off-by: Christophe Roullier <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 51 ++++++++++++++++++-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index 5d8f4eac4bc0..964173d08026 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -14,6 +14,7 @@
#include <linux/of_net.h>
#include <linux/phy.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
#include <linux/pm_wakeirq.h>
#include <linux/regmap.h>
#include <linux/slab.h>
@@ -84,6 +85,7 @@ struct stm32_dwmac {
u32 mode_reg; /* MAC glue-logic mode register */
u32 mode_mask;
struct regmap *regmap;
+ struct regulator *regulator;
u32 speed;
const struct stm32_ops *ops;
struct device *dev;
@@ -305,6 +307,16 @@ static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
if (err)
pr_debug("Warning sysconfig register mask not set\n");
+ dwmac->regulator = devm_regulator_get_optional(dev, "phy");
+ if (IS_ERR(dwmac->regulator)) {
+ if (PTR_ERR(dwmac->regulator) == -EPROBE_DEFER) {
+ dev_dbg(dev, "phy regulator is not available yet, deferred probing\n");
+ return -EPROBE_DEFER;
+ }
+ dev_dbg(dev, "no regulator found\n");
+ dwmac->regulator = NULL;
+ }
+
return 0;
}
@@ -363,6 +375,27 @@ static int stm32_dwmac_wake_init(struct device *dev,
return 0;
}
+static int phy_power_on(struct stm32_dwmac *bsp_priv, bool enable)
+{
+ int ret;
+ struct device *dev = bsp_priv->dev;
+
+ if (!bsp_priv->regulator)
+ return 0;
+
+ if (enable) {
+ ret = regulator_enable(bsp_priv->regulator);
+ if (ret)
+ dev_err(dev, "fail to enable phy-supply\n");
+ } else {
+ ret = regulator_disable(bsp_priv->regulator);
+ if (ret)
+ dev_err(dev, "fail to disable phy-supply\n");
+ }
+
+ return ret;
+}
+
static int stm32_dwmac_probe(struct platform_device *pdev)
{
struct plat_stmmacenet_data *plat_dat;
@@ -410,12 +443,18 @@ static int stm32_dwmac_probe(struct platform_device *pdev)
if (ret)
goto err_wake_init_disable;
- ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+ ret = phy_power_on(plat_dat->bsp_priv, true);
if (ret)
goto err_clk_disable;
+ ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+ if (ret)
+ goto err_gmac_powerdown;
+
return 0;
+err_gmac_powerdown:
+ phy_power_on(plat_dat->bsp_priv, false);
err_clk_disable:
stm32_dwmac_clk_disable(dwmac, false);
err_wake_init_disable:
@@ -436,6 +475,8 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
dev_pm_clear_wake_irq(&pdev->dev);
device_init_wakeup(&pdev->dev, false);
+
+ phy_power_on(priv->plat->bsp_priv, false);
}
static int stm32mp1_suspend(struct stm32_dwmac *dwmac)
@@ -466,6 +507,10 @@ static int stm32_dwmac_suspend(struct device *dev)
if (dwmac->ops->suspend)
ret = dwmac->ops->suspend(dwmac);
+ /* Keep the PHY up if we use Wake-on-Lan. */
+ if (!device_may_wakeup(dwmac->dev))
+ phy_power_on(dwmac, false);
+
return ret;
}
@@ -479,6 +524,10 @@ static int stm32_dwmac_resume(struct device *dev)
if (dwmac->ops->resume)
dwmac->ops->resume(dwmac);
+ /* The PHY was up for Wake-on-Lan. */
+ if (!device_may_wakeup(dwmac->dev))
+ phy_power_on(dwmac, true);
+
ret = stm32_dwmac_init(priv->plat, true);
if (ret)
return ret;
--
2.25.1
Change glue to be more generic and manage easily next stm32 products.
The goal of this commit is to have one stm32mp1_set_mode function which
can manage different STM32 SOC. SOC can have different SYSCFG register
bitfields. so in pmcsetr we defined the bitfields corresponding to the SOC.
Signed-off-by: Christophe Roullier <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------
1 file changed, 51 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index c92dfc4ecf57..68a02de25ac7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -23,10 +23,6 @@
#define SYSCFG_MCU_ETH_MASK BIT(23)
#define SYSCFG_MP1_ETH_MASK GENMASK(23, 16)
-#define SYSCFG_PMCCLRR_OFFSET 0x40
-
-#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16)
-#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17)
/* CLOCK feed to PHY*/
#define ETH_CK_F_25M 25000000
@@ -46,9 +42,6 @@
* RMII | 1 | 0 | 0 | n/a |
*------------------------------------------
*/
-#define SYSCFG_PMCR_ETH_SEL_MII BIT(20)
-#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21)
-#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23)
#define SYSCFG_PMCR_ETH_SEL_GMII 0
#define SYSCFG_MCU_ETH_SEL_MII 0
#define SYSCFG_MCU_ETH_SEL_RMII 1
@@ -90,19 +83,33 @@ struct stm32_dwmac {
int eth_ref_clk_sel_reg;
int irq_pwr_wakeup;
u32 mode_reg; /* MAC glue-logic mode register */
+ u32 mode_mask;
struct regmap *regmap;
u32 speed;
const struct stm32_ops *ops;
struct device *dev;
};
+struct stm32_syscfg_pmcsetr {
+ u32 eth1_clk_sel;
+ u32 eth1_ref_clk_sel;
+ u32 eth1_selmii;
+ u32 eth1_sel_rgmii;
+ u32 eth1_sel_rmii;
+ u32 eth2_clk_sel;
+ u32 eth2_ref_clk_sel;
+ u32 eth2_sel_rgmii;
+ u32 eth2_sel_rmii;
+};
+
struct stm32_ops {
int (*set_mode)(struct plat_stmmacenet_data *plat_dat);
int (*suspend)(struct stm32_dwmac *dwmac);
void (*resume)(struct stm32_dwmac *dwmac);
int (*parse_data)(struct stm32_dwmac *dwmac,
struct device *dev);
- u32 syscfg_eth_mask;
+ u32 syscfg_clr_off;
+ struct stm32_syscfg_pmcsetr pmcsetr;
bool clk_rx_enable_in_suspend;
};
@@ -161,7 +168,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
{
struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
u32 reg = dwmac->mode_reg, clk_rate;
- int val;
+ int val = 0;
clk_rate = clk_get_rate(dwmac->clk_eth_ck);
dwmac->enable_eth_ck = false;
@@ -169,7 +176,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
case PHY_INTERFACE_MODE_MII:
if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk)
dwmac->enable_eth_ck = true;
- val = SYSCFG_PMCR_ETH_SEL_MII;
+ val = dwmac->ops->pmcsetr.eth1_selmii;
pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n");
break;
case PHY_INTERFACE_MODE_GMII:
@@ -177,16 +184,17 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
if (clk_rate == ETH_CK_F_25M &&
(dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
dwmac->enable_eth_ck = true;
- val |= SYSCFG_PMCR_ETH_CLK_SEL;
+ val |= dwmac->ops->pmcsetr.eth1_clk_sel;
}
pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n");
break;
case PHY_INTERFACE_MODE_RMII:
- val = SYSCFG_PMCR_ETH_SEL_RMII;
+ val = dwmac->ops->pmcsetr.eth1_sel_rmii | dwmac->ops->pmcsetr.eth2_sel_rmii;
if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
(dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {
dwmac->enable_eth_ck = true;
- val |= SYSCFG_PMCR_ETH_REF_CLK_SEL;
+ val |= dwmac->ops->pmcsetr.eth1_ref_clk_sel;
+ val |= dwmac->ops->pmcsetr.eth2_ref_clk_sel;
}
pr_debug("SYSCFG init : PHY_INTERFACE_MODE_RMII\n");
break;
@@ -194,11 +202,12 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
- val = SYSCFG_PMCR_ETH_SEL_RGMII;
+ val = dwmac->ops->pmcsetr.eth1_sel_rgmii | dwmac->ops->pmcsetr.eth2_sel_rgmii;
if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) &&
(dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
dwmac->enable_eth_ck = true;
- val |= SYSCFG_PMCR_ETH_CLK_SEL;
+ val |= dwmac->ops->pmcsetr.eth1_clk_sel;
+ val |= dwmac->ops->pmcsetr.eth2_clk_sel;
}
pr_debug("SYSCFG init : PHY_INTERFACE_MODE_RGMII\n");
break;
@@ -210,12 +219,12 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
}
/* Need to update PMCCLRR (clear register) */
- regmap_write(dwmac->regmap, reg + SYSCFG_PMCCLRR_OFFSET,
- dwmac->ops->syscfg_eth_mask);
+ regmap_write(dwmac->regmap, reg + dwmac->ops->syscfg_clr_off,
+ dwmac->mode_mask);
/* Update PMCSETR (set register) */
return regmap_update_bits(dwmac->regmap, reg,
- dwmac->ops->syscfg_eth_mask, val);
+ dwmac->mode_mask, val);
}
static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat)
@@ -241,7 +250,7 @@ static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat)
}
return regmap_update_bits(dwmac->regmap, reg,
- dwmac->ops->syscfg_eth_mask, val << 23);
+ SYSCFG_MCU_ETH_MASK, val << 23);
}
static void stm32_dwmac_clk_disable(struct stm32_dwmac *dwmac, bool suspend)
@@ -286,10 +295,17 @@ static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
return PTR_ERR(dwmac->regmap);
err = of_property_read_u32_index(np, "st,syscon", 1, &dwmac->mode_reg);
+ if (err) {
+ dev_err(dev, "Can't get sysconfig register offset (%d)\n", err);
+ return err;
+ }
+
+ dwmac->mode_mask = SYSCFG_MP1_ETH_MASK;
+ err = of_property_read_u32_index(np, "st,syscon", 2, &dwmac->mode_mask);
if (err)
- dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
+ pr_debug("Warning sysconfig register mask not set\n");
- return err;
+ return 0;
}
static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
@@ -478,8 +494,7 @@ static SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops,
stm32_dwmac_suspend, stm32_dwmac_resume);
static struct stm32_ops stm32mcu_dwmac_data = {
- .set_mode = stm32mcu_set_mode,
- .syscfg_eth_mask = SYSCFG_MCU_ETH_MASK
+ .set_mode = stm32mcu_set_mode
};
static struct stm32_ops stm32mp1_dwmac_data = {
@@ -487,8 +502,19 @@ static struct stm32_ops stm32mp1_dwmac_data = {
.suspend = stm32mp1_suspend,
.resume = stm32mp1_resume,
.parse_data = stm32mp1_parse_data,
- .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK,
- .clk_rx_enable_in_suspend = true
+ .clk_rx_enable_in_suspend = true,
+ .syscfg_clr_off = 0x44,
+ .pmcsetr = {
+ .eth1_clk_sel = BIT(16),
+ .eth1_ref_clk_sel = BIT(17),
+ .eth1_selmii = BIT(20),
+ .eth1_sel_rgmii = BIT(21),
+ .eth1_sel_rmii = BIT(23),
+ .eth2_clk_sel = 0,
+ .eth2_ref_clk_sel = 0,
+ .eth2_sel_rgmii = 0,
+ .eth2_sel_rmii = 0
+ }
};
static const struct of_device_id stm32_dwmac_match[] = {
--
2.25.1
Those pins are used for Ethernet 1 and 2 on STM32MP13F-DK board.
ethernet1: RMII with crystal.
ethernet2: RMII without crystal.
Add analog gpio pin configuration ("sleep") to manage power mode on
stm32mp13.
Signed-off-by: Christophe Roullier <[email protected]>
---
arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi | 71 +++++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi b/arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi
index 32c5d8a1e06a..7f72c62da0a6 100644
--- a/arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi
@@ -13,6 +13,77 @@ pins {
};
};
+ eth1_rmii_pins_a: eth1-rmii-0 {
+ pins1 {
+ pinmux = <STM32_PINMUX('G', 13, AF11)>, /* ETH_RMII_TXD0 */
+ <STM32_PINMUX('G', 14, AF11)>, /* ETH_RMII_TXD1 */
+ <STM32_PINMUX('B', 11, AF11)>, /* ETH_RMII_TX_EN */
+ <STM32_PINMUX('A', 1, AF11)>, /* ETH_RMII_REF_CLK */
+ <STM32_PINMUX('A', 2, AF11)>, /* ETH_MDIO */
+ <STM32_PINMUX('G', 2, AF11)>; /* ETH_MDC */
+ bias-disable;
+ drive-push-pull;
+ slew-rate = <1>;
+ };
+
+ pins2 {
+ pinmux = <STM32_PINMUX('C', 4, AF11)>, /* ETH_RMII_RXD0 */
+ <STM32_PINMUX('C', 5, AF11)>, /* ETH_RMII_RXD1 */
+ <STM32_PINMUX('C', 1, AF10)>; /* ETH_RMII_CRS_DV */
+ bias-disable;
+ };
+
+ };
+
+ eth1_rmii_sleep_pins_a: eth1-rmii-sleep-0 {
+ pins1 {
+ pinmux = <STM32_PINMUX('G', 13, ANALOG)>, /* ETH_RMII_TXD0 */
+ <STM32_PINMUX('G', 14, ANALOG)>, /* ETH_RMII_TXD1 */
+ <STM32_PINMUX('B', 11, ANALOG)>, /* ETH_RMII_TX_EN */
+ <STM32_PINMUX('A', 1, ANALOG)>, /* ETH_RMII_REF_CLK */
+ <STM32_PINMUX('A', 2, ANALOG)>, /* ETH_MDIO */
+ <STM32_PINMUX('G', 2, ANALOG)>, /* ETH_MDC */
+ <STM32_PINMUX('C', 4, ANALOG)>, /* ETH_RMII_RXD0 */
+ <STM32_PINMUX('C', 5, ANALOG)>, /* ETH_RMII_RXD1 */
+ <STM32_PINMUX('C', 1, ANALOG)>; /* ETH_RMII_CRS_DV */
+ };
+ };
+
+ eth2_rmii_pins_a: eth2-rmii-0 {
+ pins1 {
+ pinmux = <STM32_PINMUX('F', 7, AF11)>, /* ETH_RMII_TXD0 */
+ <STM32_PINMUX('G', 11, AF10)>, /* ETH_RMII_TXD1 */
+ <STM32_PINMUX('G', 8, AF13)>, /* ETH_RMII_ETHCK */
+ <STM32_PINMUX('F', 6, AF11)>, /* ETH_RMII_TX_EN */
+ <STM32_PINMUX('B', 2, AF11)>, /* ETH_MDIO */
+ <STM32_PINMUX('G', 5, AF10)>; /* ETH_MDC */
+ bias-disable;
+ drive-push-pull;
+ slew-rate = <1>;
+ };
+
+ pins2 {
+ pinmux = <STM32_PINMUX('F', 4, AF11)>, /* ETH_RMII_RXD0 */
+ <STM32_PINMUX('E', 2, AF10)>, /* ETH_RMII_RXD1 */
+ <STM32_PINMUX('A', 12, AF11)>; /* ETH_RMII_CRS_DV */
+ bias-disable;
+ };
+ };
+
+ eth2_rmii_sleep_pins_a: eth2-rmii-sleep-0 {
+ pins1 {
+ pinmux = <STM32_PINMUX('F', 7, ANALOG)>, /* ETH_RMII_TXD0 */
+ <STM32_PINMUX('G', 11, ANALOG)>, /* ETH_RMII_TXD1 */
+ <STM32_PINMUX('G', 8, ANALOG)>, /* ETH_RMII_ETHCK */
+ <STM32_PINMUX('F', 6, ANALOG)>, /* ETH_RMII_TX_EN */
+ <STM32_PINMUX('B', 2, ANALOG)>, /* ETH_MDIO */
+ <STM32_PINMUX('G', 5, ANALOG)>, /* ETH_MDC */
+ <STM32_PINMUX('F', 4, ANALOG)>, /* ETH_RMII_RXD0 */
+ <STM32_PINMUX('E', 2, ANALOG)>, /* ETH_RMII_RXD1 */
+ <STM32_PINMUX('A', 12, ANALOG)>; /* ETH_RMII_CRS_DV */
+ };
+ };
+
i2c1_pins_a: i2c1-0 {
pins {
pinmux = <STM32_PINMUX('D', 12, AF5)>, /* I2C1_SCL */
--
2.25.1
Some cleaning because some Ethernet PHY configs do not need to add
st,ext-phyclk property.
Change print info message "No phy clock provided" only when debug.
Signed-off-by: Christophe Roullier <[email protected]>
---
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 27 ++++++++++---------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
index 7529a8d15492..e648c4e790a7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
@@ -55,17 +55,17 @@
*| | | 25MHz | 50MHz | |
* ---------------------------------------------------------------------------
*| MII | - | eth-ck | n/a | n/a |
- *| | | st,ext-phyclk | | |
+ *| | | | | |
* ---------------------------------------------------------------------------
*| GMII | - | eth-ck | n/a | n/a |
- *| | | st,ext-phyclk | | |
+ *| | | | | |
* ---------------------------------------------------------------------------
*| RGMII | - | eth-ck | n/a | eth-ck |
- *| | | st,ext-phyclk | | st,eth-clk-sel or|
+ *| | | | | st,eth-clk-sel or|
*| | | | | st,ext-phyclk |
* ---------------------------------------------------------------------------
*| RMII | - | eth-ck | eth-ck | n/a |
- *| | | st,ext-phyclk | st,eth-ref-clk-sel | |
+ *| | | | st,eth-ref-clk-sel | |
*| | | | or st,ext-phyclk | |
* ---------------------------------------------------------------------------
*
@@ -174,23 +174,22 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
dwmac->enable_eth_ck = false;
switch (plat_dat->mac_interface) {
case PHY_INTERFACE_MODE_MII:
- if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk)
+ if (clk_rate == ETH_CK_F_25M)
dwmac->enable_eth_ck = true;
val = dwmac->ops->pmcsetr.eth1_selmii;
pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n");
break;
case PHY_INTERFACE_MODE_GMII:
val = SYSCFG_PMCR_ETH_SEL_GMII;
- if (clk_rate == ETH_CK_F_25M &&
- (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
+ if (clk_rate == ETH_CK_F_25M)
dwmac->enable_eth_ck = true;
- val |= dwmac->ops->pmcsetr.eth1_clk_sel;
- }
pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n");
break;
case PHY_INTERFACE_MODE_RMII:
val = dwmac->ops->pmcsetr.eth1_sel_rmii | dwmac->ops->pmcsetr.eth2_sel_rmii;
- if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
+ if (clk_rate == ETH_CK_F_25M)
+ dwmac->enable_eth_ck = true;
+ if (clk_rate == ETH_CK_F_50M &&
(dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {
dwmac->enable_eth_ck = true;
val |= dwmac->ops->pmcsetr.eth1_ref_clk_sel;
@@ -203,7 +202,9 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
val = dwmac->ops->pmcsetr.eth1_sel_rgmii | dwmac->ops->pmcsetr.eth2_sel_rgmii;
- if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) &&
+ if (clk_rate == ETH_CK_F_25M)
+ dwmac->enable_eth_ck = true;
+ if (clk_rate == ETH_CK_F_125M &&
(dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
dwmac->enable_eth_ck = true;
val |= dwmac->ops->pmcsetr.eth1_clk_sel;
@@ -219,7 +220,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
}
/* Need to update PMCCLRR (clear register) */
- regmap_write(dwmac->regmap, reg + dwmac->ops->syscfg_clr_off,
+ regmap_write(dwmac->regmap, dwmac->ops->syscfg_clr_off,
dwmac->mode_mask);
/* Update PMCSETR (set register) */
@@ -328,7 +329,7 @@ static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
/* Get ETH_CLK clocks */
dwmac->clk_eth_ck = devm_clk_get(dev, "eth-ck");
if (IS_ERR(dwmac->clk_eth_ck)) {
- dev_info(dev, "No phy clock provided...\n");
+ dev_dbg(dev, "No phy clock provided...\n");
dwmac->clk_eth_ck = NULL;
}
--
2.25.1
Add dual Ethernet:
-Ethernet1: RMII with crystal
-Ethernet2: RMII without crystal
PHYs used are SMSC (LAN8742A)
With Ethernet1, we can performed WoL from PHY instead of GMAC point
of view.
(in this case IRQ for WoL is managed as wakeup pin and configured
in OS secure).
Signed-off-by: Christophe Roullier <[email protected]>
---
arch/arm/boot/dts/st/stm32mp135f-dk.dts | 48 +++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp135f-dk.dts b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
index 567e53ad285f..3b8eb0ab9ab9 100644
--- a/arch/arm/boot/dts/st/stm32mp135f-dk.dts
+++ b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
@@ -19,6 +19,8 @@ / {
compatible = "st,stm32mp135f-dk", "st,stm32mp135";
aliases {
+ ethernet0 = ðernet1;
+ ethernet1 = ðernet2;
serial0 = &uart4;
serial1 = &usart1;
serial2 = &uart8;
@@ -141,6 +143,52 @@ &cryp {
status = "okay";
};
+ðernet1 {
+ status = "okay";
+ pinctrl-0 = <ð1_rmii_pins_a>;
+ pinctrl-1 = <ð1_rmii_sleep_pins_a>;
+ pinctrl-names = "default", "sleep";
+ phy-mode = "rmii";
+ max-speed = <100>;
+ phy-handle = <&phy0_eth1>;
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "snps,dwmac-mdio";
+
+ phy0_eth1: ethernet-phy@0 {
+ compatible = "ethernet-phy-id0007.c131";
+ reset-gpios = <&mcp23017 9 GPIO_ACTIVE_LOW>;
+ reg = <0>;
+ wakeup-source;
+ };
+ };
+};
+
+ðernet2 {
+ status = "okay";
+ pinctrl-0 = <ð2_rmii_pins_a>;
+ pinctrl-1 = <ð2_rmii_sleep_pins_a>;
+ pinctrl-names = "default", "sleep";
+ phy-mode = "rmii";
+ max-speed = <100>;
+ phy-handle = <&phy0_eth2>;
+ st,ext-phyclk;
+ phy-supply = <&scmi_v3v3_sw>;
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "snps,dwmac-mdio";
+ phy0_eth2: ethernet-phy@0 {
+ compatible = "ethernet-phy-id0007.c131";
+ reset-gpios = <&mcp23017 10 GPIO_ACTIVE_LOW>;
+ reg = <0>;
+ };
+ };
+};
+
&i2c1 {
pinctrl-names = "default", "sleep";
pinctrl-0 = <&i2c1_pins_a>;
--
2.25.1
Need to enable MCP23S08 I/O expanders to manage Ethernet phy
reset in STM32MP135F-DK board
STMMAC driver defer is not silent, need to put this config in
built-in to avoid huge of Ethernet messages
Signed-off-by: Christophe Roullier <[email protected]>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index b2955dcb5a53..0abbe00372df 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -469,6 +469,7 @@ CONFIG_SPI_XILINX=y
CONFIG_SPI_SPIDEV=y
CONFIG_SPMI=y
CONFIG_PINCTRL_AS3722=y
+CONFIG_PINCTRL_MCP23S08=y
CONFIG_PINCTRL_MICROCHIP_SGPIO=y
CONFIG_PINCTRL_OCELOT=y
CONFIG_PINCTRL_PALMAS=y
--
2.25.1
On Fri, 26 Apr 2024 14:56:56 +0200, Christophe Roullier wrote:
> STM32MP13 is STM32 SOC with 2 GMACs instances
> This board have 2 RMII phy:
> -Ethernet1: RMII with crystal
> -Ethernet2: RMII without crystal
> Rework dwmac glue to simplify management for next stm32
> Add support for PHY regulator
>
> V2: - Remark from Rob Herring (add Krzysztof's ack in patch 02/11, update in yaml)
> Remark from Serge Semin (upate commits msg)
>
> Christophe Roullier (11):
> dt-bindings: net: add STM32MP13 compatible in documentation for stm32
> dt-bindings: net: add phy-supply property for stm32
> net: stmmac: dwmac-stm32: rework glue to simplify management
> net: stmmac: dwmac-stm32: add management of stm32mp13
> net: stmmac: dwmac-stm32: update config management for phy wo cristal
> net: stmmac: dwmac-stm32: clean the way to manage wol irqwake
> net: stmmac: dwmac-stm32: support the phy-supply regulator binding
> ARM: dts: stm32: add ethernet1 and ethernet2 support on stm32mp13
> ARM: dts: stm32: add ethernet1/2 RMII pins for STM32MP13F-DK board
> ARM: dts: stm32: add ethernet1 and ethernet2 for STM32MP135F-DK board
> ARM: multi_v7_defconfig: Add MCP23S08 pinctrl support
>
> .../devicetree/bindings/net/stm32-dwmac.yaml | 79 +++++-
> arch/arm/boot/dts/st/stm32mp13-pinctrl.dtsi | 71 ++++++
> arch/arm/boot/dts/st/stm32mp131.dtsi | 31 +++
> arch/arm/boot/dts/st/stm32mp133.dtsi | 30 +++
> arch/arm/boot/dts/st/stm32mp135f-dk.dts | 48 ++++
> arch/arm/configs/multi_v7_defconfig | 1 +
> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 235 ++++++++++++------
> 7 files changed, 416 insertions(+), 79 deletions(-)
>
> --
> 2.25.1
>
>
>
My bot found new DTB warnings on the .dts files added or changed in this
series.
Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.
If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:
pip3 install dtschema --upgrade
New warnings running 'make CHECK_DTBS=y st/stm32mp135f-dk.dtb' for [email protected]:
arch/arm/boot/dts/st/stm32mp135f-dk.dtb: ethernet@5800e000: Unevaluated properties are not allowed ('st,ext-phyclk' was unexpected)
from schema $id: http://devicetree.org/schemas/net/stm32-dwmac.yaml#
On Fri, Apr 26, 2024 at 02:56:58PM +0200, Christophe Roullier wrote:
> Phandle to a regulator that provides power to the PHY. This
> regulator will be managed during the PHY power on/off sequence.
>
> Acked-by: Krzysztof Kozlowski <[email protected]>
> Signed-off-by: Christophe Roullier <[email protected]>
> ---
> Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> index b901a432dfa9..7c3aa181abcb 100644
> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
> @@ -84,6 +84,9 @@ properties:
> - description: offset of the control register
> - description: field to set mask in register
>
> + phy-supply:
> + description: PHY regulator
This is for which PHY? The serdes phy or ethernet phy? This only makes
sense here if the phy is part of the MAC. Otherwise, it belongs in the
phy node.
Rob
On 4/26/24 2:57 PM, Christophe Roullier wrote:
> On STM32 platforms it is no longer needed to use a dedicated wakeup to
> wake up system from CStop.
This really needs more clarification.
Why was the code needed before ? Maybe because it was used by some of
the older STM32F4/F7/H7 SoCs ? Is it still needed by those SoCs ? Will
this patch break those older SoCs ?
> This patch removes the dedicated wake up usage
> and clean the way to register the wake up irq.
[...]
On 4/26/24 2:57 PM, Christophe Roullier wrote:
> Some cleaning because some Ethernet PHY configs do not need to add
> st,ext-phyclk property.
> Change print info message "No phy clock provided" only when debug.
>
> Signed-off-by: Christophe Roullier <[email protected]>
> ---
> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 27 ++++++++++---------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> index 7529a8d15492..e648c4e790a7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> @@ -55,17 +55,17 @@
> *| | | 25MHz | 50MHz | |
> * ---------------------------------------------------------------------------
> *| MII | - | eth-ck | n/a | n/a |
> - *| | | st,ext-phyclk | | |
> + *| | | | | |
> * ---------------------------------------------------------------------------
> *| GMII | - | eth-ck | n/a | n/a |
> - *| | | st,ext-phyclk | | |
> + *| | | | | |
> * ---------------------------------------------------------------------------
> *| RGMII | - | eth-ck | n/a | eth-ck |
> - *| | | st,ext-phyclk | | st,eth-clk-sel or|
> + *| | | | | st,eth-clk-sel or|
> *| | | | | st,ext-phyclk |
> * ---------------------------------------------------------------------------
> *| RMII | - | eth-ck | eth-ck | n/a |
> - *| | | st,ext-phyclk | st,eth-ref-clk-sel | |
> + *| | | | st,eth-ref-clk-sel | |
> *| | | | or st,ext-phyclk | |
> * ---------------------------------------------------------------------------
> *
> @@ -174,23 +174,22 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
> dwmac->enable_eth_ck = false;
> switch (plat_dat->mac_interface) {
> case PHY_INTERFACE_MODE_MII:
> - if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk)
> + if (clk_rate == ETH_CK_F_25M)
I see two problems here.
First, according to the table above, in MII mode, clk_rate cannot be
anything else but 25 MHz, so the (clk_rate == ETH_CK_F_25M) condition is
always true. Why not drop that condition ?
The "dwmac->ext_phyclk" means "Ethernet PHY have no crystal", which
means the clock are provided by the STM32 RCC clock IP instead, which
means if the dwmac->ext_phyclk is true, dwmac->enable_eth_ck should be
set to true, because dwmac->enable_eth_ck controls the enablement of
these STM32 clock IP generated clock.
Second, as far as I understand it, there is no way to operate this IP
with external clock in MII mode, so this section should always be only:
dwmac->enable_eth_ck = true;
> dwmac->enable_eth_ck = true;
> val = dwmac->ops->pmcsetr.eth1_selmii;
> pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n");
> break;
> case PHY_INTERFACE_MODE_GMII:
> val = SYSCFG_PMCR_ETH_SEL_GMII;
> - if (clk_rate == ETH_CK_F_25M &&
> - (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
> + if (clk_rate == ETH_CK_F_25M)
> dwmac->enable_eth_ck = true;
> - val |= dwmac->ops->pmcsetr.eth1_clk_sel;
> - }
> pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n");
> break;
> case PHY_INTERFACE_MODE_RMII:
> val = dwmac->ops->pmcsetr.eth1_sel_rmii | dwmac->ops->pmcsetr.eth2_sel_rmii;
> - if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
> + if (clk_rate == ETH_CK_F_25M)
> + dwmac->enable_eth_ck = true;
> + if (clk_rate == ETH_CK_F_50M &&
> (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {
This doesn't seem to be equivalent change to the previous code . Here,
if the clock frequency is 25 MHz, the clock are unconditionally enabled.
Before, the code enabled the clock only if clock frequency was 25 MHz
AND one of the "dwmac->eth_ref_clk_sel_reg" or "dwmac->ext_phyclk" was
set (i.e. clock provided by SoC RCC clock IP).
I think it might make this code easier if you drop all of the frequency
test conditionals, which aren't really all that useful, and only enable
the clock if either dwmac->ext_phyclk / dwmac->eth_clk_sel_reg /
dwmac->eth_ref_clk_sel_reg is set , because effectively what this entire
convoluted code is implementing is "if (clock supplied by clock IP i.e.
RCC) enable the clock()" *, right ?
* And it is also toggling the right clock mux bit in PMCSETR.
So, for MII this would be plain:
dwmac->enable_eth_ck = true;
For GMII/RGMII this would be:
if (dwmac->ext_phyclk || dwmac->eth_clk_sel_reg)
dwmac->enable_eth_ck = true;
For RMII this would be:
if (dwmac->ext_phyclk || dwmac->eth_ref_clk_sel_reg)
dwmac->enable_eth_ck = true;
Maybe the clock frequency validation can be retained, but done separately?
> dwmac->enable_eth_ck = true;
> val |= dwmac->ops->pmcsetr.eth1_ref_clk_sel;
> @@ -203,7 +202,9 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
> case PHY_INTERFACE_MODE_RGMII_RXID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> val = dwmac->ops->pmcsetr.eth1_sel_rgmii | dwmac->ops->pmcsetr.eth2_sel_rgmii;
> - if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) &&
> + if (clk_rate == ETH_CK_F_25M)
> + dwmac->enable_eth_ck = true;
> + if (clk_rate == ETH_CK_F_125M &&
> (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
> dwmac->enable_eth_ck = true;
> val |= dwmac->ops->pmcsetr.eth1_clk_sel;
> @@ -219,7 +220,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
> }
>
> /* Need to update PMCCLRR (clear register) */
> - regmap_write(dwmac->regmap, reg + dwmac->ops->syscfg_clr_off,
> + regmap_write(dwmac->regmap, dwmac->ops->syscfg_clr_off,
> dwmac->mode_mask);
>
> /* Update PMCSETR (set register) */
> @@ -328,7 +329,7 @@ static int stm32mp1_parse_data(struct stm32_dwmac *dwmac,
> /* Get ETH_CLK clocks */
> dwmac->clk_eth_ck = devm_clk_get(dev, "eth-ck");
> if (IS_ERR(dwmac->clk_eth_ck)) {
> - dev_info(dev, "No phy clock provided...\n");
> + dev_dbg(dev, "No phy clock provided...\n");
> dwmac->clk_eth_ck = NULL;
> }
>
On 4/26/24 2:57 PM, Christophe Roullier wrote:
> Add dual Ethernet:
> -Ethernet1: RMII with crystal
> -Ethernet2: RMII without crystal
> PHYs used are SMSC (LAN8742A)
>
> With Ethernet1, we can performed WoL from PHY instead of GMAC point
> of view.
> (in this case IRQ for WoL is managed as wakeup pin and configured
> in OS secure).
How does the Linux PHY driver process such a PHY IRQ ?
Or is Linux unaware of the PHY IRQ ? Doesn't that cause issues ?
> diff --git a/arch/arm/boot/dts/st/stm32mp135f-dk.dts b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
> index 567e53ad285f..3b8eb0ab9ab9 100644
> --- a/arch/arm/boot/dts/st/stm32mp135f-dk.dts
> +++ b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
> @@ -19,6 +19,8 @@ / {
> compatible = "st,stm32mp135f-dk", "st,stm32mp135";
>
> aliases {
> + ethernet0 = ðernet1;
> + ethernet1 = ðernet2;
> serial0 = &uart4;
> serial1 = &usart1;
> serial2 = &uart8;
> @@ -141,6 +143,52 @@ &cryp {
> status = "okay";
> };
>
> +ðernet1 {
> + status = "okay";
> + pinctrl-0 = <ð1_rmii_pins_a>;
> + pinctrl-1 = <ð1_rmii_sleep_pins_a>;
> + pinctrl-names = "default", "sleep";
> + phy-mode = "rmii";
> + max-speed = <100>;
> + phy-handle = <&phy0_eth1>;
Keep the list sorted please (is the max-speed even needed? if not, drop it)
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "snps,dwmac-mdio";
> +
> + phy0_eth1: ethernet-phy@0 {
> + compatible = "ethernet-phy-id0007.c131";
> + reset-gpios = <&mcp23017 9 GPIO_ACTIVE_LOW>;
> + reg = <0>;
> + wakeup-source;
> + };
> + };
> +};
> +
> +ðernet2 {
> + status = "okay";
> + pinctrl-0 = <ð2_rmii_pins_a>;
> + pinctrl-1 = <ð2_rmii_sleep_pins_a>;
> + pinctrl-names = "default", "sleep";
> + phy-mode = "rmii";
> + max-speed = <100>;
> + phy-handle = <&phy0_eth2>;
> + st,ext-phyclk;
> + phy-supply = <&scmi_v3v3_sw>;
Sort please
[...]
On 4/26/24 2:57 PM, Christophe Roullier wrote:
> Configure the phy regulator if defined by the "phy-supply" DT phandle.
>
> Signed-off-by: Christophe Roullier <[email protected]>
Maybe this should be separate from this series ?
On 4/26/24 2:56 PM, Christophe Roullier wrote:
> Phandle to a regulator that provides power to the PHY. This
> regulator will be managed during the PHY power on/off sequence.
>
> Acked-by: Krzysztof Kozlowski <[email protected]>
> Signed-off-by: Christophe Roullier <[email protected]>
Maybe this entire regulator business should be separate series from the
MP13 DWMAC ethernet series ?
On 4/26/24 2:56 PM, Christophe Roullier wrote:
> Change glue to be more generic and manage easily next stm32 products.
> The goal of this commit is to have one stm32mp1_set_mode function which
> can manage different STM32 SOC. SOC can have different SYSCFG register
> bitfields. so in pmcsetr we defined the bitfields corresponding to the SOC.
>
> Signed-off-by: Christophe Roullier <[email protected]>
> ---
> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------
> 1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> index c92dfc4ecf57..68a02de25ac7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> @@ -23,10 +23,6 @@
>
> #define SYSCFG_MCU_ETH_MASK BIT(23)
> #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16)
> -#define SYSCFG_PMCCLRR_OFFSET 0x40
> -
> -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16)
> -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17)
>
> /* CLOCK feed to PHY*/
> #define ETH_CK_F_25M 25000000
> @@ -46,9 +42,6 @@
> * RMII | 1 | 0 | 0 | n/a |
> *------------------------------------------
> */
> -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20)
> -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21)
> -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23)
> #define SYSCFG_PMCR_ETH_SEL_GMII 0
> #define SYSCFG_MCU_ETH_SEL_MII 0
> #define SYSCFG_MCU_ETH_SEL_RMII 1
> @@ -90,19 +83,33 @@ struct stm32_dwmac {
> int eth_ref_clk_sel_reg;
> int irq_pwr_wakeup;
> u32 mode_reg; /* MAC glue-logic mode register */
> + u32 mode_mask;
> struct regmap *regmap;
> u32 speed;
> const struct stm32_ops *ops;
> struct device *dev;
> };
>
> +struct stm32_syscfg_pmcsetr {
> + u32 eth1_clk_sel;
> + u32 eth1_ref_clk_sel;
> + u32 eth1_selmii;
> + u32 eth1_sel_rgmii;
> + u32 eth1_sel_rmii;
> + u32 eth2_clk_sel;
> + u32 eth2_ref_clk_sel;
> + u32 eth2_sel_rgmii;
> + u32 eth2_sel_rmii;
> +};
[...]
> @@ -487,8 +502,19 @@ static struct stm32_ops stm32mp1_dwmac_data = {
> .suspend = stm32mp1_suspend,
> .resume = stm32mp1_resume,
> .parse_data = stm32mp1_parse_data,
> - .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK,
> - .clk_rx_enable_in_suspend = true
> + .clk_rx_enable_in_suspend = true,
> + .syscfg_clr_off = 0x44,
> + .pmcsetr = {
> + .eth1_clk_sel = BIT(16),
> + .eth1_ref_clk_sel = BIT(17),
> + .eth1_selmii = BIT(20),
> + .eth1_sel_rgmii = BIT(21),
> + .eth1_sel_rmii = BIT(23),
> + .eth2_clk_sel = 0,
> + .eth2_ref_clk_sel = 0,
> + .eth2_sel_rgmii = 0,
> + .eth2_sel_rmii = 0
> + }
> };
Is this structure really necessary ?
It seems the MP15 single ethernet config bitfield is at offset 16.
MP13 has two bitfields, one at offset 16, the other at offset 24 .
All you need to do is figure out which of the two MACs you are
configuring, and then shift the bitfield mask by 16 or 24, since the
bits are at the same offset for both bitfields.
See the matching upstream U-Boot commit for how this shift can be done:
a440d19c6c91 ("net: dwc_eth_qos: Add DT parsing for STM32MP13xx platform")
Hi,
On 4/26/24 16:47, Marek Vasut wrote:
> On 4/26/24 2:56 PM, Christophe Roullier wrote:
>> Phandle to a regulator that provides power to the PHY. This
>> regulator will be managed during the PHY power on/off sequence.
>>
>> Acked-by: Krzysztof Kozlowski <[email protected]>
>> Signed-off-by: Christophe Roullier <[email protected]>
>
> Maybe this entire regulator business should be separate series from
> the MP13 DWMAC ethernet series ?
I prefer push it with MP13 Ethernet series if possible.
Hi
On 4/26/24 16:53, Marek Vasut wrote:
> On 4/26/24 2:56 PM, Christophe Roullier wrote:
>> Change glue to be more generic and manage easily next stm32 products.
>> The goal of this commit is to have one stm32mp1_set_mode function which
>> can manage different STM32 SOC. SOC can have different SYSCFG register
>> bitfields. so in pmcsetr we defined the bitfields corresponding to
>> the SOC.
>>
>> Signed-off-by: Christophe Roullier <[email protected]>
>> ---
>> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------
>> 1 file changed, 51 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> index c92dfc4ecf57..68a02de25ac7 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> @@ -23,10 +23,6 @@
>> #define SYSCFG_MCU_ETH_MASK BIT(23)
>> #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16)
>> -#define SYSCFG_PMCCLRR_OFFSET 0x40
>> -
>> -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16)
>> -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17)
>> /* CLOCK feed to PHY*/
>> #define ETH_CK_F_25M 25000000
>> @@ -46,9 +42,6 @@
>> * RMII | 1 | 0 | 0 | n/a |
>> *------------------------------------------
>> */
>> -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20)
>> -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21)
>> -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23)
>> #define SYSCFG_PMCR_ETH_SEL_GMII 0
>> #define SYSCFG_MCU_ETH_SEL_MII 0
>> #define SYSCFG_MCU_ETH_SEL_RMII 1
>> @@ -90,19 +83,33 @@ struct stm32_dwmac {
>> int eth_ref_clk_sel_reg;
>> int irq_pwr_wakeup;
>> u32 mode_reg; /* MAC glue-logic mode register */
>> + u32 mode_mask;
>> struct regmap *regmap;
>> u32 speed;
>> const struct stm32_ops *ops;
>> struct device *dev;
>> };
>> +struct stm32_syscfg_pmcsetr {
>> + u32 eth1_clk_sel;
>> + u32 eth1_ref_clk_sel;
>> + u32 eth1_selmii;
>> + u32 eth1_sel_rgmii;
>> + u32 eth1_sel_rmii;
>> + u32 eth2_clk_sel;
>> + u32 eth2_ref_clk_sel;
>> + u32 eth2_sel_rgmii;
>> + u32 eth2_sel_rmii;
>> +};
>
> [...]
>
>> @@ -487,8 +502,19 @@ static struct stm32_ops stm32mp1_dwmac_data = {
>> .suspend = stm32mp1_suspend,
>> .resume = stm32mp1_resume,
>> .parse_data = stm32mp1_parse_data,
>> - .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK,
>> - .clk_rx_enable_in_suspend = true
>> + .clk_rx_enable_in_suspend = true,
>> + .syscfg_clr_off = 0x44,
>> + .pmcsetr = {
>> + .eth1_clk_sel = BIT(16),
>> + .eth1_ref_clk_sel = BIT(17),
>> + .eth1_selmii = BIT(20),
>> + .eth1_sel_rgmii = BIT(21),
>> + .eth1_sel_rmii = BIT(23),
>> + .eth2_clk_sel = 0,
>> + .eth2_ref_clk_sel = 0,
>> + .eth2_sel_rgmii = 0,
>> + .eth2_sel_rmii = 0
>> + }
>> };
>
> Is this structure really necessary ?
>
I prefer to keep this implementation for the moment, as it is working
fine. Maybe at a later stage, I will send some optimizations.
> It seems the MP15 single ethernet config bitfield is at offset 16.
> MP13 has two bitfields, one at offset 16, the other at offset 24 .
>
> All you need to do is figure out which of the two MACs you are
> configuring, and then shift the bitfield mask by 16 or 24, since the
> bits are at the same offset for both bitfields.
>
> See the matching upstream U-Boot commit for how this shift can be done:
> a440d19c6c91 ("net: dwc_eth_qos: Add DT parsing for STM32MP13xx
> platform")
>
Hi
On 4/26/24 17:30, Rob Herring wrote:
> On Fri, Apr 26, 2024 at 02:56:58PM +0200, Christophe Roullier wrote:
>> Phandle to a regulator that provides power to the PHY. This
>> regulator will be managed during the PHY power on/off sequence.
>>
>> Acked-by: Krzysztof Kozlowski <[email protected]>
>> Signed-off-by: Christophe Roullier <[email protected]>
>> ---
>> Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> index b901a432dfa9..7c3aa181abcb 100644
>> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> @@ -84,6 +84,9 @@ properties:
>> - description: offset of the control register
>> - description: field to set mask in register
>>
>> + phy-supply:
>> + description: PHY regulator
> This is for which PHY? The serdes phy or ethernet phy? This only makes
> sense here if the phy is part of the MAC. Otherwise, it belongs in the
> phy node.
>
> Rob
You are right, normally it should be managed in Ethernet PHY (Realtek,
Microchip etc...)
Lots of glue manage this like this. Does it forbidden now ? if yes need
to update PHY driver to manage this property.
On 5/13/24 1:45 PM, Christophe ROULLIER wrote:
> Hi,
Hi,
> On 4/26/24 16:47, Marek Vasut wrote:
>> On 4/26/24 2:56 PM, Christophe Roullier wrote:
>>> Phandle to a regulator that provides power to the PHY. This
>>> regulator will be managed during the PHY power on/off sequence.
>>>
>>> Acked-by: Krzysztof Kozlowski <[email protected]>
>>> Signed-off-by: Christophe Roullier <[email protected]>
>>
>> Maybe this entire regulator business should be separate series from
>> the MP13 DWMAC ethernet series ?
> I prefer push it with MP13 Ethernet series if possible.
This is separate functionality, independent of the MP13 support and not
required for the MP13 support, correct ?
If yes, move it into separate patch(set) to make both series easier to
review.
On 5/13/24 4:06 PM, Christophe ROULLIER wrote:
> Hi
>
> On 4/26/24 17:30, Rob Herring wrote:
>> On Fri, Apr 26, 2024 at 02:56:58PM +0200, Christophe Roullier wrote:
>>> Phandle to a regulator that provides power to the PHY. This
>>> regulator will be managed during the PHY power on/off sequence.
>>>
>>> Acked-by: Krzysztof Kozlowski <[email protected]>
>>> Signed-off-by: Christophe Roullier <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/net/stm32-dwmac.yaml | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>>> b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>>> index b901a432dfa9..7c3aa181abcb 100644
>>> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>>> @@ -84,6 +84,9 @@ properties:
>>> - description: offset of the control register
>>> - description: field to set mask in register
>>> + phy-supply:
>>> + description: PHY regulator
>> This is for which PHY? The serdes phy or ethernet phy? This only makes
>> sense here if the phy is part of the MAC. Otherwise, it belongs in the
>> phy node.
>>
>> Rob
>
> You are right, normally it should be managed in Ethernet PHY (Realtek,
> Microchip etc...)
>
> Lots of glue manage this like this. Does it forbidden now ? if yes need
> to update PHY driver to manage this property.
If the regulator is connected to the PHY, then the supply should be
described in the PHY node and you wouldn't even need these PHY patches
(also see my comment that you should split the PHY regulator part of
this patchset into separate series).
On 5/13/24 2:48 PM, Christophe ROULLIER wrote:
> Hi
>
> On 4/26/24 16:53, Marek Vasut wrote:
>> On 4/26/24 2:56 PM, Christophe Roullier wrote:
>>> Change glue to be more generic and manage easily next stm32 products.
>>> The goal of this commit is to have one stm32mp1_set_mode function which
>>> can manage different STM32 SOC. SOC can have different SYSCFG register
>>> bitfields. so in pmcsetr we defined the bitfields corresponding to
>>> the SOC.
>>>
>>> Signed-off-by: Christophe Roullier <[email protected]>
>>> ---
>>> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------
>>> 1 file changed, 51 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> index c92dfc4ecf57..68a02de25ac7 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> @@ -23,10 +23,6 @@
>>> #define SYSCFG_MCU_ETH_MASK BIT(23)
>>> #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16)
>>> -#define SYSCFG_PMCCLRR_OFFSET 0x40
>>> -
>>> -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16)
>>> -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17)
>>> /* CLOCK feed to PHY*/
>>> #define ETH_CK_F_25M 25000000
>>> @@ -46,9 +42,6 @@
>>> * RMII | 1 | 0 | 0 | n/a |
>>> *------------------------------------------
>>> */
>>> -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20)
>>> -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21)
>>> -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23)
>>> #define SYSCFG_PMCR_ETH_SEL_GMII 0
>>> #define SYSCFG_MCU_ETH_SEL_MII 0
>>> #define SYSCFG_MCU_ETH_SEL_RMII 1
>>> @@ -90,19 +83,33 @@ struct stm32_dwmac {
>>> int eth_ref_clk_sel_reg;
>>> int irq_pwr_wakeup;
>>> u32 mode_reg; /* MAC glue-logic mode register */
>>> + u32 mode_mask;
>>> struct regmap *regmap;
>>> u32 speed;
>>> const struct stm32_ops *ops;
>>> struct device *dev;
>>> };
>>> +struct stm32_syscfg_pmcsetr {
>>> + u32 eth1_clk_sel;
>>> + u32 eth1_ref_clk_sel;
>>> + u32 eth1_selmii;
>>> + u32 eth1_sel_rgmii;
>>> + u32 eth1_sel_rmii;
>>> + u32 eth2_clk_sel;
>>> + u32 eth2_ref_clk_sel;
>>> + u32 eth2_sel_rgmii;
>>> + u32 eth2_sel_rmii;
>>> +};
>>
>> [...]
>>
>>> @@ -487,8 +502,19 @@ static struct stm32_ops stm32mp1_dwmac_data = {
>>> .suspend = stm32mp1_suspend,
>>> .resume = stm32mp1_resume,
>>> .parse_data = stm32mp1_parse_data,
>>> - .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK,
>>> - .clk_rx_enable_in_suspend = true
>>> + .clk_rx_enable_in_suspend = true,
>>> + .syscfg_clr_off = 0x44,
>>> + .pmcsetr = {
>>> + .eth1_clk_sel = BIT(16),
>>> + .eth1_ref_clk_sel = BIT(17),
>>> + .eth1_selmii = BIT(20),
>>> + .eth1_sel_rgmii = BIT(21),
>>> + .eth1_sel_rmii = BIT(23),
>>> + .eth2_clk_sel = 0,
>>> + .eth2_ref_clk_sel = 0,
>>> + .eth2_sel_rgmii = 0,
>>> + .eth2_sel_rmii = 0
>>> + }
>>> };
>>
>> Is this structure really necessary ?
>>
> I prefer to keep this implementation for the moment, as it is working
> fine. Maybe at a later stage, I will send some optimizations.
BIT() and left shift by 8 works all the same, without all this
complexity and new structures, since all you really have on MP13 are two
identical bitfields (one at offset 16, the other at offset 24) with the
same bits in them, so why not make this simple ?
On 4/26/24 17:37, Marek Vasut wrote:
> On 4/26/24 2:57 PM, Christophe Roullier wrote:
>> Some cleaning because some Ethernet PHY configs do not need to add
>> st,ext-phyclk property.
>> Change print info message "No phy clock provided" only when debug.
>>
>> Signed-off-by: Christophe Roullier <[email protected]>
>> ---
>> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 27 ++++++++++---------
>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> index 7529a8d15492..e648c4e790a7 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>> @@ -55,17 +55,17 @@
>> *| | | 25MHz | 50MHz
>> | |
>> *
>> ---------------------------------------------------------------------------
>> *| MII | - | eth-ck | n/a |
>> n/a |
>> - *| | | st,ext-phyclk | | |
>> + *| | | | | |
>> *
>> ---------------------------------------------------------------------------
>> *| GMII | - | eth-ck | n/a |
>> n/a |
>> - *| | | st,ext-phyclk | | |
>> + *| | | | | |
>> *
>> ---------------------------------------------------------------------------
>> *| RGMII | - | eth-ck | n/a |
>> eth-ck |
>> - *| | | st,ext-phyclk | |
>> st,eth-clk-sel or|
>> + *| | | | |
>> st,eth-clk-sel or|
>> *| | | | |
>> st,ext-phyclk |
>> *
>> ---------------------------------------------------------------------------
>> *| RMII | - | eth-ck | eth-ck |
>> n/a |
>> - *| | | st,ext-phyclk | st,eth-ref-clk-sel
>> | |
>> + *| | | | st,eth-ref-clk-sel
>> | |
>> *| | | | or st,ext-phyclk
>> | |
>> *
>> ---------------------------------------------------------------------------
>> *
>> @@ -174,23 +174,22 @@ static int stm32mp1_set_mode(struct
>> plat_stmmacenet_data *plat_dat)
>> dwmac->enable_eth_ck = false;
>> switch (plat_dat->mac_interface) {
>> case PHY_INTERFACE_MODE_MII:
>> - if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk)
>> + if (clk_rate == ETH_CK_F_25M)
>
> I see two problems here.
>
> First, according to the table above, in MII mode, clk_rate cannot be
> anything else but 25 MHz, so the (clk_rate == ETH_CK_F_25M) condition
> is always true. Why not drop that condition ?
Not agree, there is also "Normal" case MII (MII with quartz/cristal)
(first column in the table above), so need to keep this test to check
clk_rate 25MHz.
>
> The "dwmac->ext_phyclk" means "Ethernet PHY have no crystal", which
> means the clock are provided by the STM32 RCC clock IP instead, which
> means if the dwmac->ext_phyclk is true, dwmac->enable_eth_ck should be
> set to true, because dwmac->enable_eth_ck controls the enablement of
> these STM32 clock IP generated clock.
Right
>
> Second, as far as I understand it, there is no way to operate this IP
> with external clock in MII mode, so this section should always be only:
>
> dwmac->enable_eth_ck = true;
Not for case "Normal" MII :-)
>
>> dwmac->enable_eth_ck = true;
>> val = dwmac->ops->pmcsetr.eth1_selmii;
>> pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n");
>> break;
>> case PHY_INTERFACE_MODE_GMII:
>> val = SYSCFG_PMCR_ETH_SEL_GMII;
>> - if (clk_rate == ETH_CK_F_25M &&
>> - (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
>> + if (clk_rate == ETH_CK_F_25M)
>> dwmac->enable_eth_ck = true;
>> - val |= dwmac->ops->pmcsetr.eth1_clk_sel;
>> - }
>> pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n");
>> break;
>> case PHY_INTERFACE_MODE_RMII:
>> val = dwmac->ops->pmcsetr.eth1_sel_rmii |
>> dwmac->ops->pmcsetr.eth2_sel_rmii;
>> - if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
>> + if (clk_rate == ETH_CK_F_25M)
>> + dwmac->enable_eth_ck = true;
>> + if (clk_rate == ETH_CK_F_50M &&
>> (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {
>
> This doesn't seem to be equivalent change to the previous code . Here,
> if the clock frequency is 25 MHz, the clock are unconditionally
> enabled. Before, the code enabled the clock only if clock frequency
> was 25 MHz AND one of the "dwmac->eth_ref_clk_sel_reg" or
> "dwmac->ext_phyclk" was set (i.e. clock provided by SoC RCC clock IP).
You are right, but in STM32MP15/MP13 reference manual it is write that
we need to update SYSCFG (SYSCFG_PMCSETR) register only in "Ethernet
50MHz RMII clock selection":
Bit 17 ETH_REF_CLK_SEL: Ethernet 50MHz RMII clock selection.
Set by software.
0: Writing '0' has no effect, reading '0' means External clock is
used. Need selection of AFMux. Could be used with all PHY
1: Writing '1' set this bit, reading '1' means Internal clock
ETH_CLK1 from RCC is used regardless AFMux. Could be used only with RMII PHY
>
> I think it might make this code easier if you drop all of the
> frequency test conditionals, which aren't really all that useful, and
> only enable the clock if either dwmac->ext_phyclk /
> dwmac->eth_clk_sel_reg / dwmac->eth_ref_clk_sel_reg is set , because
> effectively what this entire convoluted code is implementing is "if
> (clock supplied by clock IP i.e. RCC) enable the clock()" *, right ?
>
> * And it is also toggling the right clock mux bit in PMCSETR.
>
> So, for MII this would be plain:
> dwmac->enable_eth_ck = true;
>
> For GMII/RGMII this would be:
> if (dwmac->ext_phyclk || dwmac->eth_clk_sel_reg)
> dwmac->enable_eth_ck = true;
>
> For RMII this would be:
> if (dwmac->ext_phyclk || dwmac->eth_ref_clk_sel_reg)
> dwmac->enable_eth_ck = true;
>
> Maybe the clock frequency validation can be retained, but done
> separately?
As explained previously, need to keep check of clock frequency in this test.
>
>> dwmac->enable_eth_ck = true;
>> val |= dwmac->ops->pmcsetr.eth1_ref_clk_sel;
>> @@ -203,7 +202,9 @@ static int stm32mp1_set_mode(struct
>> plat_stmmacenet_data *plat_dat)
>> case PHY_INTERFACE_MODE_RGMII_RXID:
>> case PHY_INTERFACE_MODE_RGMII_TXID:
>> val = dwmac->ops->pmcsetr.eth1_sel_rgmii |
>> dwmac->ops->pmcsetr.eth2_sel_rgmii;
>> - if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) &&
>> + if (clk_rate == ETH_CK_F_25M)
>> + dwmac->enable_eth_ck = true;
>> + if (clk_rate == ETH_CK_F_125M &&
>> (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
>> dwmac->enable_eth_ck = true;
>> val |= dwmac->ops->pmcsetr.eth1_clk_sel;
>> @@ -219,7 +220,7 @@ static int stm32mp1_set_mode(struct
>> plat_stmmacenet_data *plat_dat)
>> }
>> /* Need to update PMCCLRR (clear register) */
>> - regmap_write(dwmac->regmap, reg + dwmac->ops->syscfg_clr_off,
>> + regmap_write(dwmac->regmap, dwmac->ops->syscfg_clr_off,
>> dwmac->mode_mask);
>> /* Update PMCSETR (set register) */
>> @@ -328,7 +329,7 @@ static int stm32mp1_parse_data(struct stm32_dwmac
>> *dwmac,
>> /* Get ETH_CLK clocks */
>> dwmac->clk_eth_ck = devm_clk_get(dev, "eth-ck");
>> if (IS_ERR(dwmac->clk_eth_ck)) {
>> - dev_info(dev, "No phy clock provided...\n");
>> + dev_dbg(dev, "No phy clock provided...\n");
>> dwmac->clk_eth_ck = NULL;
>> }
On 4/26/24 17:40, Marek Vasut wrote:
> On 4/26/24 2:57 PM, Christophe Roullier wrote:
>> On STM32 platforms it is no longer needed to use a dedicated wakeup to
>> wake up system from CStop.
>
> This really needs more clarification.
>
> Why was the code needed before ? Maybe because it was used by some of
> the older STM32F4/F7/H7 SoCs ? Is it still needed by those SoCs ? Will
> this patch break those older SoCs ?
Yes you are right, if power mode use in STM32F4/F7/H7 SoC, issue with
this patch, I will abandon it.
>
>> This patch removes the dedicated wake up usage
>> and clean the way to register the wake up irq.
>
> [...]
Hi Marek
On 4/26/24 17:44, Marek Vasut wrote:
> On 4/26/24 2:57 PM, Christophe Roullier wrote:
>> Add dual Ethernet:
>> -Ethernet1: RMII with crystal
>> -Ethernet2: RMII without crystal
>> PHYs used are SMSC (LAN8742A)
>>
>> With Ethernet1, we can performed WoL from PHY instead of GMAC point
>> of view.
>> (in this case IRQ for WoL is managed as wakeup pin and configured
>> in OS secure).
>
> How does the Linux PHY driver process such a PHY IRQ ?
>
> Or is Linux unaware of the PHY IRQ ? Doesn't that cause issues ?
In this case, we want to have an example to wakeup the system from
Standby low power mode (VDDCPU and VDD_CORE off) thanks to a magic
packet detected by the PHY. The PHY then assert his interrupt output signal.
On MP13 DK platform, this PHY signal is connected to a specific GPIO
aka "Wakeup pins" (only 6 wakeup pins an MP13). Those specific GPIOs are
handled by the PWR peripheral which is controlled by the secure OS.
On WoL packet, the Secure OS catches the PHY interrupt and uses
asynchronous notification mechanism to warn Linux (on our platform we
use a PPI). On Linux side, Optee core driver creates an irq
domain/irqchip triggered on the asynchronous notification. Each device
which use a wakeup pin need then to request an IRQ on this "Optee irq
domain".
This OPTEE irq domain will be pushed soon.
cheers
Alex
>
>> diff --git a/arch/arm/boot/dts/st/stm32mp135f-dk.dts
>> b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
>> index 567e53ad285f..3b8eb0ab9ab9 100644
>> --- a/arch/arm/boot/dts/st/stm32mp135f-dk.dts
>> +++ b/arch/arm/boot/dts/st/stm32mp135f-dk.dts
>> @@ -19,6 +19,8 @@ / {
>> compatible = "st,stm32mp135f-dk", "st,stm32mp135";
>> aliases {
>> + ethernet0 = ðernet1;
>> + ethernet1 = ðernet2;
>> serial0 = &uart4;
>> serial1 = &usart1;
>> serial2 = &uart8;
>> @@ -141,6 +143,52 @@ &cryp {
>> status = "okay";
>> };
>> +ðernet1 {
>> + status = "okay";
>> + pinctrl-0 = <ð1_rmii_pins_a>;
>> + pinctrl-1 = <ð1_rmii_sleep_pins_a>;
>> + pinctrl-names = "default", "sleep";
>> + phy-mode = "rmii";
>> + max-speed = <100>;
>> + phy-handle = <&phy0_eth1>;
>
> Keep the list sorted please (is the max-speed even needed? if not, drop it)
>
>> + mdio {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "snps,dwmac-mdio";
>> +
>> + phy0_eth1: ethernet-phy@0 {
>> + compatible = "ethernet-phy-id0007.c131";
>> + reset-gpios = <&mcp23017 9 GPIO_ACTIVE_LOW>;
>> + reg = <0>;
>> + wakeup-source;
>> + };
>> + };
>> +};
>> +
>> +ðernet2 {
>> + status = "okay";
>> + pinctrl-0 = <ð2_rmii_pins_a>;
>> + pinctrl-1 = <ð2_rmii_sleep_pins_a>;
>> + pinctrl-names = "default", "sleep";
>> + phy-mode = "rmii";
>> + max-speed = <100>;
>> + phy-handle = <&phy0_eth2>;
>> + st,ext-phyclk;
>> + phy-supply = <&scmi_v3v3_sw>;
>
> Sort please
>
> [...]
On 5/13/24 5:11 PM, Christophe ROULLIER wrote:
>
> On 4/26/24 17:37, Marek Vasut wrote:
>> On 4/26/24 2:57 PM, Christophe Roullier wrote:
>>> Some cleaning because some Ethernet PHY configs do not need to add
>>> st,ext-phyclk property.
>>> Change print info message "No phy clock provided" only when debug.
>>>
>>> Signed-off-by: Christophe Roullier <[email protected]>
>>> ---
>>> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 27 ++++++++++---------
>>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> index 7529a8d15492..e648c4e790a7 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> @@ -55,17 +55,17 @@
>>> *| | | 25MHz | 50MHz
>>> | |
>>> *
>>> ---------------------------------------------------------------------------
>>> *| MII | - | eth-ck | n/a | n/a |
>>> - *| | | st,ext-phyclk | | |
>>> + *| | | | | |
>>> *
>>> ---------------------------------------------------------------------------
>>> *| GMII | - | eth-ck | n/a | n/a |
>>> - *| | | st,ext-phyclk | | |
>>> + *| | | | | |
>>> *
>>> ---------------------------------------------------------------------------
>>> *| RGMII | - | eth-ck | n/a | eth-ck |
>>> - *| | | st,ext-phyclk | |
>>> st,eth-clk-sel or|
>>> + *| | | | |
>>> st,eth-clk-sel or|
>>> *| | | | |
>>> st,ext-phyclk |
>>> *
>>> ---------------------------------------------------------------------------
>>> *| RMII | - | eth-ck | eth-ck | n/a |
>>> - *| | | st,ext-phyclk | st,eth-ref-clk-sel
>>> | |
>>> + *| | | | st,eth-ref-clk-sel
>>> | |
>>> *| | | | or st,ext-phyclk
>>> | |
>>> *
>>> ---------------------------------------------------------------------------
>>> *
>>> @@ -174,23 +174,22 @@ static int stm32mp1_set_mode(struct
>>> plat_stmmacenet_data *plat_dat)
>>> dwmac->enable_eth_ck = false;
>>> switch (plat_dat->mac_interface) {
>>> case PHY_INTERFACE_MODE_MII:
>>> - if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk)
>>> + if (clk_rate == ETH_CK_F_25M)
>>
>> I see two problems here.
>>
>> First, according to the table above, in MII mode, clk_rate cannot be
>> anything else but 25 MHz, so the (clk_rate == ETH_CK_F_25M) condition
>> is always true. Why not drop that condition ?
> Not agree, there is also "Normal" case MII (MII with quartz/cristal)
> (first column in the table above), so need to keep this test to check
> clk_rate 25MHz.
What other rate is supported in the MII mode ? Isn't the rate always 25
MHz for MII , no matter whether it is generated by RCC or external Xtal ?
>> The "dwmac->ext_phyclk" means "Ethernet PHY have no crystal", which
>> means the clock are provided by the STM32 RCC clock IP instead, which
>> means if the dwmac->ext_phyclk is true, dwmac->enable_eth_ck should be
>> set to true, because dwmac->enable_eth_ck controls the enablement of
>> these STM32 clock IP generated clock.
> Right
>>
>> Second, as far as I understand it, there is no way to operate this IP
>> with external clock in MII mode, so this section should always be only:
>>
>> dwmac->enable_eth_ck = true;
> Not for case "Normal" MII :-)
What happens if that external clock source is not an xtal, but an
oscillator with its own driver described in DT, and enabled e.g. using
GPIO (that's compatible "gpio-gate-clock" for that oscillator) . Then
you do need to enable those clock even in the "normal" (with external
clock source instead of RCC clock source) MII case.
>>> dwmac->enable_eth_ck = true;
>>> val = dwmac->ops->pmcsetr.eth1_selmii;
>>> pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n");
>>> break;
>>> case PHY_INTERFACE_MODE_GMII:
>>> val = SYSCFG_PMCR_ETH_SEL_GMII;
>>> - if (clk_rate == ETH_CK_F_25M &&
>>> - (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) {
>>> + if (clk_rate == ETH_CK_F_25M)
>>> dwmac->enable_eth_ck = true;
>>> - val |= dwmac->ops->pmcsetr.eth1_clk_sel;
>>> - }
>>> pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n");
>>> break;
>>> case PHY_INTERFACE_MODE_RMII:
>>> val = dwmac->ops->pmcsetr.eth1_sel_rmii |
>>> dwmac->ops->pmcsetr.eth2_sel_rmii;
>>> - if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) &&
>>> + if (clk_rate == ETH_CK_F_25M)
>>> + dwmac->enable_eth_ck = true;
>>> + if (clk_rate == ETH_CK_F_50M &&
>>> (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) {
>>
>> This doesn't seem to be equivalent change to the previous code . Here,
>> if the clock frequency is 25 MHz, the clock are unconditionally
>> enabled. Before, the code enabled the clock only if clock frequency
>> was 25 MHz AND one of the "dwmac->eth_ref_clk_sel_reg" or
>> "dwmac->ext_phyclk" was set (i.e. clock provided by SoC RCC clock IP).
>
> You are right, but in STM32MP15/MP13 reference manual it is write that
> we need to update SYSCFG (SYSCFG_PMCSETR) register only in "Ethernet
> 50MHz RMII clock selection":
>
> Bit 17 ETH_REF_CLK_SEL: Ethernet 50MHz RMII clock selection.
>
> Set by software.
>
> 0: Writing '0' has no effect, reading '0' means External clock is
> used. Need selection of AFMux. Could be used with all PHY
>
> 1: Writing '1' set this bit, reading '1' means Internal clock
> ETH_CLK1 from RCC is used regardless AFMux. Could be used only with RMII
> PHY
Look at this:
"
RM0436 Rev 6 Reset and clock control (RCC)
Clock distribution for Ethernet (ETH)
Figure 83. Peripheral clock distribution for Ethernet
Page 575
"
See the mux at bottom left side in the PKCS group.
I suspect that no matter whether the clock are 25 MHz or 50 MHz, they
enter this mux, the mux selects the clock source from either RCC or from
external oscillator, and therefore the original code was correct.
>> I think it might make this code easier if you drop all of the
>> frequency test conditionals, which aren't really all that useful, and
>> only enable the clock if either dwmac->ext_phyclk /
>> dwmac->eth_clk_sel_reg / dwmac->eth_ref_clk_sel_reg is set , because
>> effectively what this entire convoluted code is implementing is "if
>> (clock supplied by clock IP i.e. RCC) enable the clock()" *, right ?
>>
>> * And it is also toggling the right clock mux bit in PMCSETR.
>>
>> So, for MII this would be plain:
>> dwmac->enable_eth_ck = true;
>>
>> For GMII/RGMII this would be:
>> if (dwmac->ext_phyclk || dwmac->eth_clk_sel_reg)
>> dwmac->enable_eth_ck = true;
>>
>> For RMII this would be:
>> if (dwmac->ext_phyclk || dwmac->eth_ref_clk_sel_reg)
>> dwmac->enable_eth_ck = true;
>>
>> Maybe the clock frequency validation can be retained, but done
>> separately?
> As explained previously, need to keep check of clock frequency in this
> test.
I sent 5 patches which split this code up, you were on CC:
[net-next,RFC,PATCH 1/5] net: stmmac: dwmac-stm32: Separate out external
clock rate validation
Maybe you can apply those, fix them up as needed, and then add this
series on top ? I think it would simplify this code a lot, since that
series splits up the clock rate validation / PMCR configuration /
external-internal clock selection into separate steps. What do you think ?
On 5/13/24 6:01 PM, Alexandre TORGUE wrote:
> Hi Marek
Hi,
> On 4/26/24 17:44, Marek Vasut wrote:
>> On 4/26/24 2:57 PM, Christophe Roullier wrote:
>>> Add dual Ethernet:
>>> -Ethernet1: RMII with crystal
>>> -Ethernet2: RMII without crystal
>>> PHYs used are SMSC (LAN8742A)
>>>
>>> With Ethernet1, we can performed WoL from PHY instead of GMAC point
>>> of view.
>>> (in this case IRQ for WoL is managed as wakeup pin and configured
>>> in OS secure).
>>
>> How does the Linux PHY driver process such a PHY IRQ ?
>>
>> Or is Linux unaware of the PHY IRQ ? Doesn't that cause issues ?
>
> In this case, we want to have an example to wakeup the system from
> Standby low power mode (VDDCPU and VDD_CORE off) thanks to a magic
> packet detected by the PHY. The PHY then assert his interrupt output
> signal.
> On MP13 DK platform, this PHY signal is connected to a specific GPIO
> aka "Wakeup pins" (only 6 wakeup pins an MP13). Those specific GPIOs are
> handled by the PWR peripheral which is controlled by the secure OS.
What does configure the PHY for this wakeup mode ?
> On WoL packet, the Secure OS catches the PHY interrupt and uses
> asynchronous notification mechanism to warn Linux (on our platform we
> use a PPI). On Linux side, Optee core driver creates an irq
> domain/irqchip triggered on the asynchronous notification. Each device
> which use a wakeup pin need then to request an IRQ on this "Optee irq
> domain".
>
> This OPTEE irq domain will be pushed soon.
I suspect it might make sense to add this WoL part separately from the
actual ethernet DT nodes, so ethernet could land and the WoL
functionality can be added when it is ready ?
Hi
On 5/16/24 02:23, Marek Vasut wrote:
> On 5/13/24 6:01 PM, Alexandre TORGUE wrote:
>> Hi Marek
>
> Hi,
>
>> On 4/26/24 17:44, Marek Vasut wrote:
>>> On 4/26/24 2:57 PM, Christophe Roullier wrote:
>>>> Add dual Ethernet:
>>>> -Ethernet1: RMII with crystal
>>>> -Ethernet2: RMII without crystal
>>>> PHYs used are SMSC (LAN8742A)
>>>>
>>>> With Ethernet1, we can performed WoL from PHY instead of GMAC point
>>>> of view.
>>>> (in this case IRQ for WoL is managed as wakeup pin and configured
>>>> in OS secure).
>>>
>>> How does the Linux PHY driver process such a PHY IRQ ?
>>>
>>> Or is Linux unaware of the PHY IRQ ? Doesn't that cause issues ?
>>
>> In this case, we want to have an example to wakeup the system from
>> Standby low power mode (VDDCPU and VDD_CORE off) thanks to a magic
>> packet detected by the PHY. The PHY then assert his interrupt output
>> signal.
>> On MP13 DK platform, this PHY signal is connected to a specific GPIO
>> aka "Wakeup pins" (only 6 wakeup pins an MP13). Those specific GPIOs
>> are handled by the PWR peripheral which is controlled by the secure OS.
>
> What does configure the PHY for this wakeup mode ?
Linux device tree.
>
>> On WoL packet, the Secure OS catches the PHY interrupt and uses
>> asynchronous notification mechanism to warn Linux (on our platform we
>> use a PPI). On Linux side, Optee core driver creates an irq
>> domain/irqchip triggered on the asynchronous notification. Each device
>> which use a wakeup pin need then to request an IRQ on this "Optee irq
>> domain".
>>
>> This OPTEE irq domain will be pushed soon.
>
> I suspect it might make sense to add this WoL part separately from the
> actual ethernet DT nodes, so ethernet could land and the WoL
> functionality can be added when it is ready ?
If at the end we want to have this Wol from PHY then I agree we need to
wait. We could push a WoL from MAC for this node before optee driver
patches merge but not sure it makes sens.
Alex
> > I suspect it might make sense to add this WoL part separately from the
> > actual ethernet DT nodes, so ethernet could land and the WoL
> > functionality can be added when it is ready ?
>
> If at the end we want to have this Wol from PHY then I agree we need to
> wait. We could push a WoL from MAC for this node before optee driver patches
> merge but not sure it makes sens.
In general, it is better if the PHY does WoL, since the MAC can then
be powered down. MAC WoL should only be used when the PHY does not
support the requested WoL configuration, but the MAC can. And
sometimes you need to spread it over both the PHY and the MAC.
Andrew
On 5/16/24 14:22, Andrew Lunn wrote:
>>> I suspect it might make sense to add this WoL part separately from the
>>> actual ethernet DT nodes, so ethernet could land and the WoL
>>> functionality can be added when it is ready ?
>>
>> If at the end we want to have this Wol from PHY then I agree we need to
>> wait. We could push a WoL from MAC for this node before optee driver patches
>> merge but not sure it makes sens.
>
> In general, it is better if the PHY does WoL, since the MAC can then
> be powered down. MAC WoL should only be used when the PHY does not
> support the requested WoL configuration, but the MAC can. And
> sometimes you need to spread it over both the PHY and the MAC.
>
thanks Andrew. So lets wait the optee driver missing part.
alex
> Andrew