> From: joshua stein <j...@jcs.org>
> Date: Wed, 7 May 2025 21:52:24 -0500
>
> Before MII reads/writes, make sure it's not busy like the docs say
> to.
Why? We already have a check at the end of dwge_mii_readreg() and
dwge_mii_writereg() to make sure the request completed. So unless
we're issuing concurrent reads or writes on the MII bus the busy bit
should still be cleared when we do the next read or write. But if we
issue concurrent reads or writes, we have bigger problems to worry
about. I don't think this fixes anything and only adds bloat to the
kernel.
> ---
> sys/dev/fdt/if_dwge.c | 64 ++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/sys/dev/fdt/if_dwge.c b/sys/dev/fdt/if_dwge.c
> index 94705b1c1cf..68eaae44f96 100644
> --- a/sys/dev/fdt/if_dwge.c
> +++ b/sys/dev/fdt/if_dwge.c
> @@ -378,6 +378,7 @@ dwge_match(struct device *parent, void *cfdata, void *aux)
> return (OF_is_compatible(faa->fa_node, "allwinner,sun7i-a20-gmac") ||
> OF_is_compatible(faa->fa_node, "amlogic,meson-axg-dwmac") ||
> OF_is_compatible(faa->fa_node, "amlogic,meson-g12a-dwmac") ||
> + OF_is_compatible(faa->fa_node, "rockchip,rk3128-gmac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3288-gmac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3308-mac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3328-gmac") ||
> @@ -423,7 +424,8 @@ dwge_attach(struct device *parent, struct device *self,
> void *aux)
> clock_set_assigned(faa->fa_node);
> clock_enable(faa->fa_node, "stmmaceth");
> reset_deassert(faa->fa_node, "stmmaceth");
> - if (OF_is_compatible(faa->fa_node, "rockchip,rk3288-gmac") ||
> + if (OF_is_compatible(faa->fa_node, "rockchip,rk3128-gmac") ||
> + OF_is_compatible(faa->fa_node, "rockchip,rk3288-gmac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3308-mac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3328-gmac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3399-gmac")) {
> @@ -530,7 +532,8 @@ dwge_attach(struct device *parent, struct device *self,
> void *aux)
> /* Do hardware specific initializations. */
> if (OF_is_compatible(faa->fa_node, "allwinner,sun7i-a20-gmac"))
> dwge_setup_allwinner(sc);
> - if (OF_is_compatible(faa->fa_node, "rockchip,rk3288-gmac") ||
> + if (OF_is_compatible(faa->fa_node, "rockchip,rk3128-gmac") ||
> + OF_is_compatible(faa->fa_node, "rockchip,rk3288-gmac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3308-mac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3328-gmac") ||
> OF_is_compatible(faa->fa_node, "rockchip,rk3399-gmac"))
> @@ -862,6 +865,17 @@ dwge_mii_readreg(struct device *self, int phy, int reg)
> struct dwge_softc *sc = (void *)self;
> int n;
>
> + for (n = 0; n < 1000; n++) {
> + if ((dwge_read(sc, GMAC_GMII_ADDR) & GMAC_GMII_ADDR_GB) == 0)
> + break;
> + if (n == 999) {
> + printf("%s: mii_read timeout: GW busy\n",
> + sc->sc_dev.dv_xname);
> + return 0;
> + }
> + delay(10);
> + }
> +
Please drop this chunk.
> dwge_write(sc, GMAC_GMII_ADDR,
> sc->sc_clk << GMAC_GMII_ADDR_CR_SHIFT |
> phy << GMAC_GMII_ADDR_PA_SHIFT |
> @@ -883,6 +897,17 @@ dwge_mii_writereg(struct device *self, int phy, int reg,
> int val)
> struct dwge_softc *sc = (void *)self;
> int n;
>
> + for (n = 0; n < 1000; n++) {
> + if ((dwge_read(sc, GMAC_GMII_ADDR) & GMAC_GMII_ADDR_GB) == 0)
> + break;
> + if (n == 999) {
> + printf("%s: mii_write timeout: GW busy\n",
> + sc->sc_dev.dv_xname);
> + return;
> + }
> + delay(10);
> + }
> +
Likewise.
> dwge_write(sc, GMAC_GMII_DATA, val);
> dwge_write(sc, GMAC_GMII_ADDR,
> sc->sc_clk << GMAC_GMII_ADDR_CR_SHIFT |
> @@ -1583,9 +1608,32 @@ dwge_setup_allwinner(struct dwge_softc *sc)
> }
>
> /*
> - * Rockchip RK3288/RK3399.
> + * Rockchip RK3128/RK3288/RK3399.
> */
>
> +/* RK3128 registers */
> +#define RK3128_GRF_MAC_CON0 0x0168
> +#define RK3128_GMAC_TXCLK_DLY_ENABLE (1 << 14 << 16 | 1 << 14)
> +#define RK3128_GMAC_TXCLK_DLY_DISABLE (1 << 14 << 16)
> +#define RK3128_GMAC_RXCLK_DLY_ENABLE (1 << 15 << 16 | 1 << 15)
> +#define RK3128_GMAC_RXCLK_DLY_DISABLE (1 << 15 << 16)
> +#define RK3128_GMAC_CLK_RX_DL_CFG(val) (0x7f << 7 << 16 | (val) << 7)
> +#define RK3128_GMAC_CLK_TX_DL_CFG(val) (0x7f << 0 << 16 | (val) << 0)
These defines are unused. I suppose these are needed for RGMII
support, which your board doesn't use since it only has a 100base-TX
interface?
I suppose we can keep these, but if you do, can you rename the
RK3128_GMAC_xxCLK_DLY_ENABLE defines to RK3128_GMAC_xxCLK_DLY_ENA and
drop the RK3128_GMAC_xxCLK_DLY_DISABLE defines for consistency with
the other rockchip SoCs?
> +
> +#define RK3128_GRF_MAC_CON1 0x016c
> +#define RK3128_GMAC_PHY_INTF_SEL_RGMII \
> + ((1 << 6 | 1 << 7 | 1 << 8) << 16 | 1 << 6)
Can you write that as ((0x7 << 6) << 16 | (0x1 << 6)) for consistency?
Also makes it fit on one line probably?
> +#define RK3128_GMAC_PHY_INTF_SEL_RMII \
> + ((1 << 6 | 1 << 7 | 1 << 8) << 16 | 1 << 8)
Likewise, ((0x7 << 6) << 16 | (0x4 << 6))
> +#define RK3128_GMAC_FLOW_CTRL (1 << 9 << 16 | 1 << 9)
> +#define RK3128_GMAC_FLOW_CTRL_CLR (1 << 9 << 16)
These two are not used and since we don't support flow control at all
in the driver I think dropping these for now is best.
> +#define RK3128_GMAC_SPEED_10M (1 << 10 << 16)
> +#define RK3128_GMAC_SPEED_100M (1 << 10 << 16 | 1 << 10)
> +#define RK3128_GMAC_RMII_CLK_25M (1 << 11 << 16 | 1 << 11)
> +#define RK3128_GMAC_RMII_CLK_2_5M (1 << 11 << 16)
> +#define RK3128_GMAC_RMII_MODE (1 << 14 << 16 | 1 << 14)
> +#define RK3128_GMAC_RMII_MODE_CLR (1 << 14 << 16)
These should probably be RK3128_GMAC_MODE_RMII and
RK3128_GMAC_MODE_MII. But it doesn't look like MII is actually
supported on this SoC, so the MII mode should probably be dropped.
> +
> /* RK3308 registers */
> #define RK3308_GRF_MAC_CON0 0x04a0
> #define RK3308_MAC_SPEED_100M ((0x1 << 0) << 16 | (0x1 << 0))
> @@ -1657,7 +1705,15 @@ dwge_setup_rockchip(struct dwge_softc *sc)
> tx_delay = OF_getpropint(sc->sc_node, "tx_delay", 0x30);
> rx_delay = OF_getpropint(sc->sc_node, "rx_delay", 0x10);
>
> - if (OF_is_compatible(sc->sc_node, "rockchip,rk3288-gmac")) {
> + if (OF_is_compatible(sc->sc_node, "rockchip,rk3128-gmac")) {
> + /* Use RMII interface. */
> + regmap_write_4(rm, RK3128_GRF_MAC_CON1,
> + RK3128_GMAC_PHY_INTF_SEL_RMII | RK3128_GMAC_SPEED_100M);
> +
> + sc->sc_clk_sel = RK3128_GRF_MAC_CON1;
> + sc->sc_clk_sel_2_5 = RK3128_GMAC_RMII_CLK_2_5M;
> + sc->sc_clk_sel_25 = RK3128_GMAC_RMII_CLK_25M;
I wonder if these should include RK3128_GMAC_SPEED_10M and
RK3128_GMAC_SPEED_100M. Did you test whether the interface works at
10base-T?
> + } else if (OF_is_compatible(sc->sc_node, "rockchip,rk3288-gmac")) {
> /* Use RGMII interface. */
> regmap_write_4(rm, RK3288_GRF_SOC_CON1,
> RK3288_GMAC_PHY_INTF_SEL_RGMII | RK3288_RMII_MODE_MII);
> --
> 2.47.1
>
>
