> Date: Wed, 05 Apr 2023 09:09:22 +0200 > From: Mark Kettenis <mark.kette...@xs4all.nl> > > > Date: Wed, 5 Apr 2023 16:09:21 +1000 > > From: David Gwynne <da...@gwynne.id.au> > > > > On Tue, Apr 04, 2023 at 10:59:56PM +1000, David Gwynne wrote: > > > > > > > > > > On 4 Apr 2023, at 20:37, Mark Kettenis <mark.kette...@xs4all.nl> wrote: > > > > > > > >> Date: Tue, 4 Apr 2023 09:49:40 +1000 > > > >> From: David Gwynne <da...@gwynne.id.au> > > > >> > > > >> i did this when i was trying to figure out why TX wasn't working on the > > > >> nanopi r5s before figuring out that problem was because we didn't have > > > >> rkiovd. > > > >> > > > >> at the very least it should future proof dwqe against more phy setups, > > > >> and provides a decent example of how to interpret those fdt properties. > > > >> > > > >> ok? > > > > > > > > Oh wow. I always thought those properties are purely about enabling > > > > the internal delays of the PHY and came in addition to the delays > > > > configured in the MAC. But you're right, that isn't what the device > > > > tree bindings say and isn't what the Linux driver implements. So this > > > > is indeed the right thing to do. And it will make the rock-3a work > > > > once I correctly implement configuring the internal delay for > > > > rgephy(4). > > > > > > > > That said, I think that once again you are doing too much DT checking. > > > > Many of the options you're now erroring out on are optional. And > > > > there actually is a reasonable chance that the interface will work if > > > > they're absent, especially if the firmware already initializes the > > > > SoC-specific GRF registers. I don't think we should even print any > > > > warnings. > > > > > > having stared inside some boot loaders recently i have mixed feelings > > > about this. i guess they can only get better from here though, right? > > > > > > > If you make dwqe_rockchip_setup() specific to the rk3568 (no objection > > > > to that), you should probably rename it to dwqe_rk3568_setup(). > > > > > > alright. > > > > > > > And a style issue; please don't add parentheses around trivial > > > > function return values in code that doesn't already use that style. > > > > We moved away from that style years ago... > > > > > > yeah, sorry. muscle memory. > > > > > > i'll fix these up tomorrow and send it around again. > > > > this still works for me on the r5s. e25 doesnt use onboard gmac. > > Just realized that this risks leaving the task uninitialized. But > that risk is already there. > > ok kettenis@
BTW, I have an almost complete diff to wire up the MIIF_RXID/MIIF_TXID flags stuff in dwqe(4) that I'll finish once this diff is in. > > Index: if_dwqe_fdt.c > > =================================================================== > > RCS file: /cvs/src/sys/dev/fdt/if_dwqe_fdt.c,v > > retrieving revision 1.5 > > diff -u -p -r1.5 if_dwqe_fdt.c > > --- if_dwqe_fdt.c 26 Mar 2023 21:44:46 -0000 1.5 > > +++ if_dwqe_fdt.c 5 Apr 2023 06:06:57 -0000 > > @@ -63,7 +63,7 @@ > > > > int dwqe_fdt_match(struct device *, void *, void *); > > void dwqe_fdt_attach(struct device *, struct device *, void *); > > -void dwqe_setup_rockchip(struct dwqe_softc *); > > +void dwqe_setup_rk3568(struct dwqe_softc *); > > void dwqe_mii_statchg_rk3568(struct device *); > > void dwqe_mii_statchg_rk3588(struct device *); > > > > @@ -138,7 +138,7 @@ dwqe_fdt_attach(struct device *parent, s > > > > /* Do hardware specific initializations. */ > > if (OF_is_compatible(faa->fa_node, "rockchip,rk3568-gmac")) > > - dwqe_setup_rockchip(sc); > > + dwqe_setup_rk3568(sc); > > > > /* Power up PHY. */ > > phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0); > > @@ -266,41 +266,62 @@ dwqe_reset_phy(struct dwqe_softc *sc, ui > > #define RK3568_GRF_GMACx_CON1(x) (0x0384 + (x) * 0x8) > > #define RK3568_GMAC_PHY_INTF_SEL_RGMII ((0x7 << 4) << 16 | > > (0x1 << 4)) > > #define RK3568_GMAC_PHY_INTF_SEL_RMII ((0x7 << 4) << 16 | > > (0x4 << 4)) > > -#define RK3568_GMAC_TXCLK_DLY_ENA ((1 << 0) << 16 | (1 << 0)) > > -#define RK3568_GMAC_RXCLK_DLY_ENA ((1 << 1) << 16 | (1 << 1)) > > +#define RK3568_GMAC_TXCLK_DLY_SET(_v) ((1 << 0) << 16 | ((_v) > > << 0)) > > +#define RK3568_GMAC_RXCLK_DLY_SET(_v) ((1 << 1) << 16 | ((_v) > > << 1)) > > > > void dwqe_mii_statchg_rk3568_task(void *); > > > > void > > -dwqe_setup_rockchip(struct dwqe_softc *sc) > > +dwqe_setup_rk3568(struct dwqe_softc *sc) > > { > > + char phy_mode[32]; > > struct regmap *rm; > > uint32_t grf; > > int tx_delay, rx_delay; > > + uint32_t iface; > > > > grf = OF_getpropint(sc->sc_node, "rockchip,grf", 0); > > rm = regmap_byphandle(grf); > > if (rm == NULL) > > return; > > > > + if (OF_getprop(sc->sc_node, "phy-mode", > > + phy_mode, sizeof(phy_mode)) <= 0) > > + return; > > + > > 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,rk3568-gmac")) { > > - /* Program clock delay lines. */ > > - regmap_write_4(rm, RK3568_GRF_GMACx_CON0(sc->sc_gmac_id), > > - RK3568_GMAC_CLK_TX_DL_CFG(tx_delay) | > > - RK3568_GMAC_CLK_RX_DL_CFG(rx_delay)); > > - > > - /* Use RGMII interface and enable clock delay. */ > > - regmap_write_4(rm, RK3568_GRF_GMACx_CON1(sc->sc_gmac_id), > > - RK3568_GMAC_PHY_INTF_SEL_RGMII | > > - RK3568_GMAC_TXCLK_DLY_ENA | > > - RK3568_GMAC_RXCLK_DLY_ENA); > > + if (strcmp(phy_mode, "rgmii") == 0) { > > + iface = RK3568_GMAC_PHY_INTF_SEL_RGMII; > > + } else if (strcmp(phy_mode, "rgmii-id") == 0) { > > + iface = RK3568_GMAC_PHY_INTF_SEL_RGMII; > > + /* id is "internal delay" */ > > + tx_delay = rx_delay = 0; > > + } else if (strcmp(phy_mode, "rgmii-rxid") == 0) { > > + iface = RK3568_GMAC_PHY_INTF_SEL_RGMII; > > + rx_delay = 0; > > + } else if (strcmp(phy_mode, "rgmii-txid") == 0) { > > + iface = RK3568_GMAC_PHY_INTF_SEL_RGMII; > > + tx_delay = 0; > > + } else if (strcmp(phy_mode, "rmii") == 0) { > > + iface = RK3568_GMAC_PHY_INTF_SEL_RMII; > > + tx_delay = rx_delay = 0; > > + } else > > + return; > > > > - task_set(&sc->sc_statchg_task, > > - dwqe_mii_statchg_rk3568_task, sc); > > - } > > + /* Program clock delay lines. */ > > + regmap_write_4(rm, RK3568_GRF_GMACx_CON0(sc->sc_gmac_id), > > + RK3568_GMAC_CLK_TX_DL_CFG(tx_delay) | > > + RK3568_GMAC_CLK_RX_DL_CFG(rx_delay)); > > + > > + /* Set interface and enable/disable clock delay. */ > > + regmap_write_4(rm, RK3568_GRF_GMACx_CON1(sc->sc_gmac_id), iface | > > + RK3568_GMAC_TXCLK_DLY_SET(tx_delay > 0 ? 1 : 0) | > > + RK3568_GMAC_RXCLK_DLY_SET(rx_delay > 0 ? 1 : 0)); > > + > > + task_set(&sc->sc_statchg_task, > > + dwqe_mii_statchg_rk3568_task, sc); > > } > > > > void > > > >