> 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.
If you make dwqe_rockchip_setup() specific to the rk3568 (no objection
to that), you should probably rename it to dwqe_rk3568_setup().
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...
> 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 3 Apr 2023 23:45:05 -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 *);
> +int dwqe_setup_rockchip(struct dwqe_softc *);
> void dwqe_mii_statchg_rk3568(struct device *);
> void dwqe_mii_statchg_rk3588(struct device *);
>
> @@ -137,8 +137,12 @@ dwqe_fdt_attach(struct device *parent, s
> delay(5000);
>
> /* Do hardware specific initializations. */
> - if (OF_is_compatible(faa->fa_node, "rockchip,rk3568-gmac"))
> - dwqe_setup_rockchip(sc);
> + if (OF_is_compatible(faa->fa_node, "rockchip,rk3568-gmac")) {
> + if (dwqe_setup_rockchip(sc) == -1) {
> + /* dwqe_setup_rockchip prints the error and \n */
> + return;
> + }
> + }
>
> /* Power up PHY. */
> phy_supply = OF_getpropint(faa->fa_node, "phy-supply", 0);
> @@ -266,41 +270,70 @@ 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
> +int
> dwqe_setup_rockchip(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 (rm == NULL) {
> + printf(": can't find rockchip,grf\n");
> + return (-1);
> + }
> +
> + if (OF_getprop(sc->sc_node, "phy-mode",
> + phy_mode, sizeof(phy_mode)) <= 0) {
> + printf(": no phy-mode\n");
> + return (-1);
> + }
>
> 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);
> -
> - task_set(&sc->sc_statchg_task,
> - dwqe_mii_statchg_rk3568_task, sc);
> + 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 {
> + printf(": unknown phy-mode %s\n", phy_mode);
> + return (-1);
> }
> +
> + /* 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);
> +
> + return (0);
> }
>
> void
>
>
