Hi Andre, Le Wed 23 Apr 25, 10:52, Andre Przywara a écrit : > For some odd reason we are very picky about the value of the EMAC clock > register from the syscon block, insisting on a certain reset value and > only doing read-modify-write operations on that register, even though we > pretty much know the register layout. > This already led to a basically redundant variant entry for the H6, which > only differs by that value. We will have the same situation with the new > A523 SoC, which again is compatible to the A64, but has a different syscon > reset value. > > Drop any assumptions about that value, and set or clear the bits that we > want to program, from scratch (starting with a value of 0). For the > remove() implementation, we just turn on the POWERDOWN bit, and deselect > the internal PHY, which mimics the existing code. I was confused about why this existed as well and think this change makes a lot of sense! I just tested it on my V3s Lichee Pi Zero dock, which uses the internal EPHY (configured via this register) and it all looks good to me. Tested-by: Paul Kocialkowski Reviewed-by: Paul Kocialkowski Note that my previous patch fixing the PHY address retrieval conflicts with this, so you might want to spin up a new version, or it might be adapted when this patch is picked-up. It's very straightforward to resolve. Thanks for this cleanup! Paul > Signed-off-by: Andre Przywara > --- > Hi, > > if anyone can shed some light on why we had this value and its handling > in the first place, I would be grateful. I don't really get its purpose, > and especially the warning message about the reset value seems odd. > I briefly tested this on A523, H3, H6, but would be glad to see more > testing on this. > > Cheers, > Andre > > .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 47 ++----------------- > 1 file changed, 4 insertions(+), 43 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > index 85723a78793ab..0f8d29763a909 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > @@ -31,10 +31,6 @@ > */ > > /* struct emac_variant - Describe dwmac-sun8i hardware variant > - * @default_syscon_value: The default value of the EMAC register in syscon > - * This value is used for disabling properly EMAC > - * and used as a good starting value in case of the > - * boot process(uboot) leave some stuff. > * @syscon_field reg_field for the syscon's gmac register > * @soc_has_internal_phy: Does the MAC embed an internal PHY > * @support_mii: Does the MAC handle MII > @@ -48,7 +44,6 @@ > * value of zero indicates this is not supported. > */ > struct emac_variant { > - u32 default_syscon_value; > const struct reg_field *syscon_field; > bool soc_has_internal_phy; > bool support_mii; > @@ -94,7 +89,6 @@ static const struct reg_field sun8i_ccu_reg_field = { > }; > > static const struct emac_variant emac_variant_h3 = { > - .default_syscon_value = 0x58000, > .syscon_field = &sun8i_syscon_reg_field, > .soc_has_internal_phy = true, > .support_mii = true, > @@ -105,14 +99,12 @@ static const struct emac_variant emac_variant_h3 = { > }; > > static const struct emac_variant emac_variant_v3s = { > - .default_syscon_value = 0x38000, > .syscon_field = &sun8i_syscon_reg_field, > .soc_has_internal_phy = true, > .support_mii = true > }; > > static const struct emac_variant emac_variant_a83t = { > - .default_syscon_value = 0, > .syscon_field = &sun8i_syscon_reg_field, > .soc_has_internal_phy = false, > .support_mii = true, > @@ -122,7 +114,6 @@ static const struct emac_variant emac_variant_a83t = { > }; > > static const struct emac_variant emac_variant_r40 = { > - .default_syscon_value = 0, > .syscon_field = &sun8i_ccu_reg_field, > .support_mii = true, > .support_rgmii = true, > @@ -130,7 +121,6 @@ static const struct emac_variant emac_variant_r40 = { > }; > > static const struct emac_variant emac_variant_a64 = { > - .default_syscon_value = 0, > .syscon_field = &sun8i_syscon_reg_field, > .soc_has_internal_phy = false, > .support_mii = true, > @@ -141,7 +131,6 @@ static const struct emac_variant emac_variant_a64 = { > }; > > static const struct emac_variant emac_variant_h6 = { > - .default_syscon_value = 0x50000, > .syscon_field = &sun8i_syscon_reg_field, > /* The "Internal PHY" of H6 is not on the die. It's on the > * co-packaged AC200 chip instead. > @@ -933,25 +922,11 @@ static int sun8i_dwmac_set_syscon(struct device *dev, > struct sunxi_priv_data *gmac = plat->bsp_priv; > struct device_node *node = dev->of_node; > int ret; > - u32 reg, val; > - > - ret = regmap_field_read(gmac->regmap_field, &val); > - if (ret) { > - dev_err(dev, "Fail to read from regmap field.\n"); > - return ret; > - } > - > - reg = gmac->variant->default_syscon_value; > - if (reg != val) > - dev_warn(dev, > - "Current syscon value is not the default %x (expect %x)\n", > - val, reg); > + u32 reg = 0, val; > > if (gmac->variant->soc_has_internal_phy) { > if (of_property_read_bool(node, "allwinner,leds-active-low")) > reg |= H3_EPHY_LED_POL; > - else > - reg &= ~H3_EPHY_LED_POL; > > /* Force EPHY xtal frequency to 24MHz. */ > reg |= H3_EPHY_CLK_SEL; > @@ -965,11 +940,6 @@ static int sun8i_dwmac_set_syscon(struct device *dev, > * address. No need to mask it again. > */ > reg |= 1 << H3_EPHY_ADDR_SHIFT; > - } else { > - /* For SoCs without internal PHY the PHY selection bit should be > - * set to 0 (external PHY). > - */ > - reg &= ~H3_EPHY_SELECT; > } > > if (!of_property_read_u32(node, "allwinner,tx-delay-ps", &val)) { > @@ -980,8 +950,6 @@ static int sun8i_dwmac_set_syscon(struct device *dev, > val /= 100; > dev_dbg(dev, "set tx-delay to %x\n", val); > if (val <= gmac->variant->tx_delay_max) { > - reg &= ~(gmac->variant->tx_delay_max << > - SYSCON_ETXDC_SHIFT); > reg |= (val << SYSCON_ETXDC_SHIFT); > } else { > dev_err(dev, "Invalid TX clock delay: %d\n", > @@ -998,8 +966,6 @@ static int sun8i_dwmac_set_syscon(struct device *dev, > val /= 100; > dev_dbg(dev, "set rx-delay to %x\n", val); > if (val <= gmac->variant->rx_delay_max) { > - reg &= ~(gmac->variant->rx_delay_max << > - SYSCON_ERXDC_SHIFT); > reg |= (val << SYSCON_ERXDC_SHIFT); > } else { > dev_err(dev, "Invalid RX clock delay: %d\n", > @@ -1008,11 +974,6 @@ static int sun8i_dwmac_set_syscon(struct device *dev, > } > } > > - /* Clear interface mode bits */ > - reg &= ~(SYSCON_ETCS_MASK | SYSCON_EPIT); > - if (gmac->variant->support_rmii) > - reg &= ~SYSCON_RMII_EN; > - > switch (plat->mac_interface) { > case PHY_INTERFACE_MODE_MII: > /* default */ > @@ -1039,9 +1000,9 @@ static int sun8i_dwmac_set_syscon(struct device *dev, > > static void sun8i_dwmac_unset_syscon(struct sunxi_priv_data *gmac) > { > - u32 reg = gmac->variant->default_syscon_value; > - > - regmap_field_write(gmac->regmap_field, reg); > + if (gmac->variant->soc_has_internal_phy) > + regmap_field_write(gmac->regmap_field, > + (H3_EPHY_SHUTDOWN | H3_EPHY_SELECT)); > } > > static void sun8i_dwmac_exit(struct platform_device *pdev, void *priv) > -- > 2.25.1 > > -- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux.