> 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.
cheers,
dlg
>
>
>> 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
>>
>>
