> Date: Wed, 5 Apr 2023 16:09:21 +1000
> From: David Gwynne <[email protected]>
>
> On Tue, Apr 04, 2023 at 10:59:56PM +1000, David Gwynne wrote:
> >
> >
> > > On 4 Apr 2023, at 20:37, Mark Kettenis <[email protected]> wrote:
> > >
> > >> Date: Tue, 4 Apr 2023 09:49:40 +1000
> > >> From: David Gwynne <[email protected]>
> > >>
> > >> 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@
> 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
>
