Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
On Mon, Sep 11, 2017 at 10:19:20PM +0200, Andrew Lunn wrote: > > Even with CLK_BUS_EPHY/RST_BUS_EPHY enabled, the MAC reset timeout. > > So no the CLK/RST are really for the PHY. > > Thanks for trying that. > > You said it was probably during scanning of the bus it times out. What > address is causing the timeout? 0 or 1? If the internal bus can only > have one PHY on it, maybe we need to set bus->phy_mask to 0x1? > I have added a trace in begin and end of stmmac_mdio_read() [ 18.145451] libphy: stmmac: probed [ 18.148398] libphy: mdio_mux: probed [ 18.148650] dwmac-sun8i 1c3.ethernet: Switch mux to internal PHY [ 18.248751] dwmac-sun8i 1c3.ethernet: EMAC reset timeout [ 18.249297] libphy: mdio_mux: probed [ 18.249362] dwmac-sun8i 1c3.ethernet: Switch mux to external PHY [ 18.249391] stmmac_mdio_read 0 2 [ 18.249598] stmmac_mdio_read 0 2 1c [ 18.249623] stmmac_mdio_read 0 3 [ 18.249811] stmmac_mdio_read 0 3 c915 [ 20.737271] EXT4-fs (mmcblk0p1): re-mounted. Opts: (null) [ 31.294868] stmmac_mdio_read 0 0 [ 31.295311] stmmac_mdio_read 0 0 1140 It seems that the timeout is unrelated to MDIO bus. Regards
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
> Even with CLK_BUS_EPHY/RST_BUS_EPHY enabled, the MAC reset timeout. > So no the CLK/RST are really for the PHY. Thanks for trying that. You said it was probably during scanning of the bus it times out. What address is causing the timeout? 0 or 1? If the internal bus can only have one PHY on it, maybe we need to set bus->phy_mask to 0x1? Andrew
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
On Mon, Sep 11, 2017 at 06:11:24PM +0200, Andrew Lunn wrote:
> On Fri, Sep 08, 2017 at 04:28:25PM +0200, Corentin Labbe wrote:
> > On Fri, Sep 08, 2017 at 04:17:36PM +0200, Andrew Lunn wrote:
> > > > > Do you know why the reset times out/fails?
> > > > >
> > > >
> > > > Because there are nothing connected to it.
> > >
> > > That should not be an issue. A read should just return 0x. And it
> > > should return 0x fast. The timing of the MDIO protocol is fixed. A
> > > read or a write takes a fixed number of cycles, independent of if
> > > there is a device there or not. The bus data line has a pullup, so if
> > > you try to access a missing device, you automatically read 0x.
> > >
> >
> > Perhaps, but the reality is that with nothing connected to it, the reset of
> > the MAC timeout.
> > Certainly, the MAC does not support finding no PHY.
>
> Are you sure this is not because of the clock and reset?
>
> + #address-cells = <1>;
> + #size-cells = <0>;
> + int_mii_phy: ethernet-phy@1 {
> + compatible =
> "ethernet-phy-ieee802.3-c22";
> + reg = <1>;
> + clocks = <&ccu CLK_BUS_EPHY>;
> + resets = <&ccu RST_BUS_EPHY>;
>
> The way you describe it here, the clock and reset are for the PHY. But
> maybe it is actually for the bus? I can understand a bus timing out if
> it has no clock, or it is held in reset. Try enabling the clock and
> reset when the internal bus is selected, not when the PHY on the bus
> is selected.
>
Even with CLK_BUS_EPHY/RST_BUS_EPHY enabled, the MAC reset timeout.
So no the CLK/RST are really for the PHY.
Regards
PS: patch and result with "integrated CLK/RST always on"
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -659,7 +659,7 @@ static int mdio_mux_syscon_switch_fn(int current_child, int
desired_child,
struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
u32 reg, val;
int ret = 0;
- bool need_reset = false;
+ bool need_reset = true;
if (current_child ^ desired_child) {
regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
@@ -824,7 +824,7 @@ static int sun8i_dwmac_power_internal_phy(struct
stmmac_priv *priv)
int ret;
if (!gmac->use_internal_phy)
- return 0;
+ dev_info(priv->device, "IPHY BYPASS\n");
ret = clk_prepare_enable(gmac->ephy_clk);
if (ret) {
[ 18.057162] dwmac-sun8i 1c3.ethernet: Will use external PHY
[ 18.183789] dwmac-sun8i 1c3.ethernet: IPHY BYPASS
[ 18.184136] dwmac-sun8i 1c3.ethernet: Chain mode enabled
[ 18.184158] dwmac-sun8i 1c3.ethernet: No HW DMA feature register
supported
[ 18.184175] dwmac-sun8i 1c3.ethernet: Normal descriptors
[ 18.184192] dwmac-sun8i 1c3.ethernet: RX Checksum Offload Engine
supported
[ 18.184214] dwmac-sun8i 1c3.ethernet: COE Type 2
[ 18.184231] dwmac-sun8i 1c3.ethernet: TX Checksum insertion supported
[ 18.185491] libphy: stmmac: probed
[ 18.188481] libphy: mdio_mux: probed
[ 18.188831] dwmac-sun8i 1c3.ethernet: Switch mux to internal PHY
[ 18.288981] dwmac-sun8i 1c3.ethernet: EMAC reset timeout
[ 18.289559] libphy: mdio_mux: probed
[ 18.289629] dwmac-sun8i 1c3.ethernet: Switch mux to external PHY
[ 20.578316] EXT4-fs (mmcblk0p1): re-mounted. Opts: (null)
[ 31.240650] RTL8211E Gigabit Ethernet 0.1:00: attached PHY driver [RTL8211E
Gigabit Ethernet] (mii_bus:phy_addr=0.1:00, irq=POLL)
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
On Fri, Sep 08, 2017 at 04:28:25PM +0200, Corentin Labbe wrote:
> On Fri, Sep 08, 2017 at 04:17:36PM +0200, Andrew Lunn wrote:
> > > > Do you know why the reset times out/fails?
> > > >
> > >
> > > Because there are nothing connected to it.
> >
> > That should not be an issue. A read should just return 0x. And it
> > should return 0x fast. The timing of the MDIO protocol is fixed. A
> > read or a write takes a fixed number of cycles, independent of if
> > there is a device there or not. The bus data line has a pullup, so if
> > you try to access a missing device, you automatically read 0x.
> >
>
> Perhaps, but the reality is that with nothing connected to it, the reset of
> the MAC timeout.
> Certainly, the MAC does not support finding no PHY.
Are you sure this is not because of the clock and reset?
+ #address-cells = <1>;
+ #size-cells = <0>;
+ int_mii_phy: ethernet-phy@1 {
+ compatible =
"ethernet-phy-ieee802.3-c22";
+ reg = <1>;
+ clocks = <&ccu CLK_BUS_EPHY>;
+ resets = <&ccu RST_BUS_EPHY>;
The way you describe it here, the clock and reset are for the PHY. But
maybe it is actually for the bus? I can understand a bus timing out if
it has no clock, or it is held in reset. Try enabling the clock and
reset when the internal bus is selected, not when the PHY on the bus
is selected.
Andrew
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
On Fri, Sep 08, 2017 at 04:17:36PM +0200, Andrew Lunn wrote: > > > Do you know why the reset times out/fails? > > > > > > > Because there are nothing connected to it. > > That should not be an issue. A read should just return 0x. And it > should return 0x fast. The timing of the MDIO protocol is fixed. A > read or a write takes a fixed number of cycles, independent of if > there is a device there or not. The bus data line has a pullup, so if > you try to access a missing device, you automatically read 0x. > Perhaps, but the reality is that with nothing connected to it, the reset of the MAC timeout. Certainly, the MAC does not support finding no PHY. So, to prevent an error message, and a "freeze" of the net process, the need_reset trick is necessary. Regards Corentin Labbe
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
> > Do you know why the reset times out/fails? > > > > Because there are nothing connected to it. That should not be an issue. A read should just return 0x. And it should return 0x fast. The timing of the MDIO protocol is fixed. A read or a write takes a fixed number of cycles, independent of if there is a device there or not. The bus data line has a pullup, so if you try to access a missing device, you automatically read 0x. Andrew
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
On Fri, Sep 08, 2017 at 04:00:20PM +0200, Andrew Lunn wrote:
> > > > +static int mdio_mux_syscon_switch_fn(int current_child, int
> > > > desired_child,
> > > > +void *data)
> > > > +{
> > > > + struct stmmac_priv *priv = data;
> > > > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > > > + u32 reg, val;
> > > > + int ret = 0;
> > > > + bool need_reset = false;
> > > > +
> > > > + if (current_child ^ desired_child) {
> > > > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > > > + switch (desired_child) {
> > > > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > > > + dev_info(priv->device, "Switch mux to internal
> > > > PHY");
> > > > + val = (reg & ~H3_EPHY_MUX_MASK) |
> > > > H3_EPHY_SELECT;
> > > > + if (gmac->use_internal_phy)
> > > > + need_reset = true;
> > > > + break;
> > >
> > > This i don't get. Why do you need use_internal_phy? Isn't that
> > > implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> > > use an external PHY on the internal MDIO bus?
> > >
> >
> > On my H3 box with external PHY, the MDIO mux library first select (for scan
> > ?) the internal MDIO.
> > Without use_internal_phy usage, this board will launch a reset to use the
> > internal MDIO... and this reset timeout/fail.
>
> Do you know why the reset times out/fails?
>
Because there are nothing connected to it.
I got also reset timeout on integrated MDIO when the integrated PHY is not
powered.
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
> > > +static int mdio_mux_syscon_switch_fn(int current_child, int
> > > desired_child,
> > > + void *data)
> > > +{
> > > + struct stmmac_priv *priv = data;
> > > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > > + u32 reg, val;
> > > + int ret = 0;
> > > + bool need_reset = false;
> > > +
> > > + if (current_child ^ desired_child) {
> > > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > > + switch (desired_child) {
> > > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > > + dev_info(priv->device, "Switch mux to internal PHY");
> > > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> > > + if (gmac->use_internal_phy)
> > > + need_reset = true;
> > > + break;
> >
> > This i don't get. Why do you need use_internal_phy? Isn't that
> > implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> > use an external PHY on the internal MDIO bus?
> >
>
> On my H3 box with external PHY, the MDIO mux library first select (for scan
> ?) the internal MDIO.
> Without use_internal_phy usage, this board will launch a reset to use the
> internal MDIO... and this reset timeout/fail.
Do you know why the reset times out/fails?
Andrew
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
On Fri, Sep 08, 2017 at 03:05:20PM +0200, Andrew Lunn wrote:
> > +#define DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID 0
> > +#define DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID 1
> >
> > /* H3/A64 specific bits */
> > #define SYSCON_RMII_EN BIT(13) /* 1: enable RMII (overrides
> > EPIT) */
> > @@ -634,6 +639,76 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
> > return 0;
> > }
> >
> > +/* MDIO multiplexing switch function
> > + * This function is called by the mdio-mux layer when it thinks the mdio
> > bus
> > + * multiplexer needs to switch.
> > + * 'current_child' is the current value of the mux register
> > + * 'desired_child' is the value of the 'reg' property of the target child
> > MDIO
> > + * node.
> > + * The first time this function is called, current_child == -1.
> > + * If current_child == desired_child, then the mux is already set to the
> > + * correct bus.
> > + *
> > + * Note that we do not use reg/mask like mdio-mux-mmioreg because we need
> > to
> > + * know easily which bus is used (reset must be done only for desired bus).
> > + */
> > +static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
> > +void *data)
> > +{
> > + struct stmmac_priv *priv = data;
> > + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > + u32 reg, val;
> > + int ret = 0;
> > + bool need_reset = false;
> > +
> > + if (current_child ^ desired_child) {
> > + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> > + switch (desired_child) {
> > + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> > + dev_info(priv->device, "Switch mux to internal PHY");
> > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> > + if (gmac->use_internal_phy)
> > + need_reset = true;
> > + break;
>
> This i don't get. Why do you need use_internal_phy? Isn't that
> implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
> use an external PHY on the internal MDIO bus?
>
On my H3 box with external PHY, the MDIO mux library first select (for scan ?)
the internal MDIO.
Without use_internal_phy usage, this board will launch a reset to use the
internal MDIO... and this reset timeout/fail.
After the MDIO mux select the external MDIO.
> > + case DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID:
> > + dev_info(priv->device, "Switch mux to external PHY");
> > + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SHUTDOWN;
> > + if (!gmac->use_internal_phy)
> > + need_reset = true;
> > + break;
>
> And is it possible to use the internal PHY on the external bus?
>
I need to check that.
Regards
Re: [PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
> +#define DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID 0
> +#define DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID 1
>
> /* H3/A64 specific bits */
> #define SYSCON_RMII_EN BIT(13) /* 1: enable RMII (overrides
> EPIT) */
> @@ -634,6 +639,76 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
> return 0;
> }
>
> +/* MDIO multiplexing switch function
> + * This function is called by the mdio-mux layer when it thinks the mdio bus
> + * multiplexer needs to switch.
> + * 'current_child' is the current value of the mux register
> + * 'desired_child' is the value of the 'reg' property of the target child
> MDIO
> + * node.
> + * The first time this function is called, current_child == -1.
> + * If current_child == desired_child, then the mux is already set to the
> + * correct bus.
> + *
> + * Note that we do not use reg/mask like mdio-mux-mmioreg because we need to
> + * know easily which bus is used (reset must be done only for desired bus).
> + */
> +static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
> + void *data)
> +{
> + struct stmmac_priv *priv = data;
> + struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> + u32 reg, val;
> + int ret = 0;
> + bool need_reset = false;
> +
> + if (current_child ^ desired_child) {
> + regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
> + switch (desired_child) {
> + case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
> + dev_info(priv->device, "Switch mux to internal PHY");
> + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
> + if (gmac->use_internal_phy)
> + need_reset = true;
> + break;
This i don't get. Why do you need use_internal_phy? Isn't that
implicit from DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID? Is it even possible to
use an external PHY on the internal MDIO bus?
> + case DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID:
> + dev_info(priv->device, "Switch mux to external PHY");
> + val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SHUTDOWN;
> + if (!gmac->use_internal_phy)
> + need_reset = true;
> + break;
And is it possible to use the internal PHY on the external bus?
Andrew
[PATCH v5 10/10] net: stmmac: dwmac-sun8i: Handle integrated/external MDIOs
The Allwinner H3 SoC have two distinct MDIO bus, only one could be
active at the same time.
The selection of the active MDIO bus are done via some bits in the EMAC
register of the system controller.
This patch implement this MDIO switch via a custom MDIO-mux.
Signed-off-by: Corentin Labbe
---
drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 +
drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 116 +++---
2 files changed, 104 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig
b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 97035766c291..e28c0d2c58e9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -159,6 +159,7 @@ config DWMAC_SUN8I
tristate "Allwinner sun8i GMAC support"
default ARCH_SUNXI
depends on OF && (ARCH_SUNXI || COMPILE_TEST)
+ select MDIO_BUS_MUX
---help---
Support for Allwinner H3 A83T A64 EMAC ethernet controllers.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 672553b652bd..ddd5695886ac 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -71,6 +72,7 @@ struct sunxi_priv_data {
const struct emac_variant *variant;
struct regmap *regmap;
bool use_internal_phy;
+ void *mux_handle;
};
static const struct emac_variant emac_variant_h3 = {
@@ -195,6 +197,9 @@ static const struct emac_variant emac_variant_a64 = {
#define H3_EPHY_LED_POLBIT(17) /* 1: active low, 0: active
high */
#define H3_EPHY_SHUTDOWN BIT(16) /* 1: shutdown, 0: power up */
#define H3_EPHY_SELECT BIT(15) /* 1: internal PHY, 0: external PHY */
+#define H3_EPHY_MUX_MASK (H3_EPHY_SHUTDOWN | H3_EPHY_SELECT)
+#define DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID 0
+#define DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID 1
/* H3/A64 specific bits */
#define SYSCON_RMII_EN BIT(13) /* 1: enable RMII (overrides EPIT) */
@@ -634,6 +639,76 @@ static int sun8i_dwmac_reset(struct stmmac_priv *priv)
return 0;
}
+/* MDIO multiplexing switch function
+ * This function is called by the mdio-mux layer when it thinks the mdio bus
+ * multiplexer needs to switch.
+ * 'current_child' is the current value of the mux register
+ * 'desired_child' is the value of the 'reg' property of the target child MDIO
+ * node.
+ * The first time this function is called, current_child == -1.
+ * If current_child == desired_child, then the mux is already set to the
+ * correct bus.
+ *
+ * Note that we do not use reg/mask like mdio-mux-mmioreg because we need to
+ * know easily which bus is used (reset must be done only for desired bus).
+ */
+static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
+void *data)
+{
+ struct stmmac_priv *priv = data;
+ struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
+ u32 reg, val;
+ int ret = 0;
+ bool need_reset = false;
+
+ if (current_child ^ desired_child) {
+ regmap_read(gmac->regmap, SYSCON_EMAC_REG, ®);
+ switch (desired_child) {
+ case DWMAC_sUN8I_MDIO_MUX_INTERNAL_ID:
+ dev_info(priv->device, "Switch mux to internal PHY");
+ val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SELECT;
+ if (gmac->use_internal_phy)
+ need_reset = true;
+ break;
+ case DWMAC_sUN8I_MDIO_MUX_EXTERNAL_ID:
+ dev_info(priv->device, "Switch mux to external PHY");
+ val = (reg & ~H3_EPHY_MUX_MASK) | H3_EPHY_SHUTDOWN;
+ if (!gmac->use_internal_phy)
+ need_reset = true;
+ break;
+ default:
+ dev_err(priv->device, "Invalid child id %x\n",
desired_child);
+ return -EINVAL;
+ }
+ regmap_write(gmac->regmap, SYSCON_EMAC_REG, val);
+ /* After changing syscon value, the MAC need reset or it will
use
+* the last value (and so the last PHY set).
+* Reset is necessary only when we reach the needed MDIO,
+* it timeout in other case.
+*/
+ if (need_reset)
+ ret = sun8i_dwmac_reset(priv);
+ else
+ dev_dbg(priv->device, "skipped reset\n");
+ }
+ return ret;
+}
+
+static int sun8i_dwmac_register_mdio_mux(struct stmmac_priv *priv)
+{
+ int ret;
+ struct device_node *mdio_mux;
+ struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
+
+ mdio_mu
