| Message ID | 20230122225107.62464-5-samuel@sholland.org |
|---|---|
| State | Accepted |
| Commit | 3cfb1e69142a5cde251e43fa373d8c5a2b5cfae4 |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | net: sun8i-emac: Allwinner D1 Support | expand |
On 22/01/2023 22:51, Samuel Holland wrote: > While R40 puts the EMAC syscon register at a different address from > other variants, the relevant portion of the register's layout is the > same. Factor out the register offset so the same code can be shared > by all variants. This matches what the Linux driver does. > > This change provides two benefits beyond the simplification: > - R40 boards now respect the RX delays from the devicetree > - This resolves a warning on architectures where readl/writel > expect the address to have a pointer type, not phys_addr_t. > Nice cleanup, and makes it obvious that the R40 isn't such a special snowflake after all. > Signed-off-by: Samuel Holland <samuel@sholland.org> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > > Changes in v2: > - Add a structure for driver data, and put the syscon offset there > > drivers/net/sun8i_emac.c | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c > index 36cc2498b5..231aac19e3 100644 > --- a/drivers/net/sun8i_emac.c > +++ b/drivers/net/sun8i_emac.c > @@ -137,6 +137,7 @@ enum emac_variant_id { > > struct emac_variant { > enum emac_variant_id variant; > + uint syscon_offset; > bool soc_has_internal_phy; > bool support_rmii; > }; > @@ -168,7 +169,7 @@ struct emac_eth_dev { > > const struct emac_variant *variant; > void *mac_reg; > - phys_addr_t sysctl_reg; > + void *sysctl_reg; > struct phy_device *phydev; > struct mii_dev *bus; > struct clk tx_clk; > @@ -323,18 +324,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata, > { > u32 reg; > > - if (priv->variant->variant == R40_GMAC) { > - /* Select RGMII for R40 */ > - reg = readl(priv->sysctl_reg + 0x164); > - reg |= SC_ETCS_INT_GMII | > - SC_EPIT | > - (CONFIG_GMAC_TX_DELAY << SC_ETXDC_OFFSET); > - > - writel(reg, priv->sysctl_reg + 0x164); > - return 0; > - } > - > - reg = readl(priv->sysctl_reg + 0x30); > + reg = readl(priv->sysctl_reg); > > reg = sun8i_emac_set_syscon_ephy(priv, reg); > > @@ -370,7 +360,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata, > reg |= ((pdata->rx_delay_ps / 100) << SC_ERXDC_OFFSET) > & SC_ERXDC_MASK; > > - writel(reg, priv->sysctl_reg + 0x30); > + writel(reg, priv->sysctl_reg); > > return 0; > } > @@ -793,6 +783,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) > struct sun8i_eth_pdata *sun8i_pdata = dev_get_plat(dev); > struct eth_pdata *pdata = &sun8i_pdata->eth_pdata; > struct emac_eth_dev *priv = dev_get_priv(dev); > + phys_addr_t syscon_base; > const fdt32_t *reg; > int node = dev_of_offset(dev); > int offset = 0; > @@ -838,13 +829,15 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) > __func__); > return -EINVAL; > } > - priv->sysctl_reg = fdt_translate_address((void *)gd->fdt_blob, > - offset, reg); > - if (priv->sysctl_reg == FDT_ADDR_T_NONE) { > + > + syscon_base = fdt_translate_address((void *)gd->fdt_blob, offset, reg); > + if (syscon_base == FDT_ADDR_T_NONE) { > debug("%s: Cannot find syscon base address\n", __func__); > return -EINVAL; > } > > + priv->sysctl_reg = (void *)syscon_base + priv->variant->syscon_offset; > + > pdata->phy_interface = -1; > priv->phyaddr = -1; > priv->use_internal_phy = false; > @@ -903,25 +896,30 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) > > static const struct emac_variant emac_variant_a83t = { > .variant = A83T_EMAC, > + .syscon_offset = 0x30, > }; > > static const struct emac_variant emac_variant_h3 = { > .variant = H3_EMAC, > + .syscon_offset = 0x30, > .soc_has_internal_phy = true, > .support_rmii = true, > }; > > static const struct emac_variant emac_variant_r40 = { > .variant = R40_GMAC, > + .syscon_offset = 0x164, > }; > > static const struct emac_variant emac_variant_a64 = { > .variant = A64_EMAC, > + .syscon_offset = 0x30, > .support_rmii = true, > }; > > static const struct emac_variant emac_variant_h6 = { > .variant = H6_EMAC, > + .syscon_offset = 0x30, > .support_rmii = true, > }; >
On Mon, Jan 23, 2023 at 7:24 PM Andre Przywara <andre.przywara@arm.com> wrote: > > On 22/01/2023 22:51, Samuel Holland wrote: > > While R40 puts the EMAC syscon register at a different address from > > other variants, the relevant portion of the register's layout is the > > same. Factor out the register offset so the same code can be shared > > by all variants. This matches what the Linux driver does. > > > > This change provides two benefits beyond the simplification: > > - R40 boards now respect the RX delays from the devicetree > > - This resolves a warning on architectures where readl/writel > > expect the address to have a pointer type, not phys_addr_t. > > > > Nice cleanup, and makes it obvious that the R40 isn't such a special > snowflake after all. > > > Signed-off-by: Samuel Holland <samuel@sholland.org> > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Cheers, > Andre > > > > --- > > > > Changes in v2: > > - Add a structure for driver data, and put the syscon offset there > > > > drivers/net/sun8i_emac.c | 32 +++++++++++++++----------------- > > 1 file changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c > > index 36cc2498b5..231aac19e3 100644 > > --- a/drivers/net/sun8i_emac.c > > +++ b/drivers/net/sun8i_emac.c > > @@ -137,6 +137,7 @@ enum emac_variant_id { > > > > struct emac_variant { > > enum emac_variant_id variant; > > + uint syscon_offset; > > bool soc_has_internal_phy; > > bool support_rmii; > > }; > > @@ -168,7 +169,7 @@ struct emac_eth_dev { > > > > const struct emac_variant *variant; > > void *mac_reg; > > - phys_addr_t sysctl_reg; > > + void *sysctl_reg; > > struct phy_device *phydev; > > struct mii_dev *bus; > > struct clk tx_clk; > > @@ -323,18 +324,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata, > > { > > u32 reg; > > > > - if (priv->variant->variant == R40_GMAC) { > > - /* Select RGMII for R40 */ > > - reg = readl(priv->sysctl_reg + 0x164); > > - reg |= SC_ETCS_INT_GMII | > > - SC_EPIT | > > - (CONFIG_GMAC_TX_DELAY << SC_ETXDC_OFFSET); > > - > > - writel(reg, priv->sysctl_reg + 0x164); > > - return 0; > > - } > > - > > - reg = readl(priv->sysctl_reg + 0x30); > > + reg = readl(priv->sysctl_reg); > > > > reg = sun8i_emac_set_syscon_ephy(priv, reg); > > > > @@ -370,7 +360,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata, > > reg |= ((pdata->rx_delay_ps / 100) << SC_ERXDC_OFFSET) > > & SC_ERXDC_MASK; > > > > - writel(reg, priv->sysctl_reg + 0x30); > > + writel(reg, priv->sysctl_reg); > > > > return 0; > > } > > @@ -793,6 +783,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) > > struct sun8i_eth_pdata *sun8i_pdata = dev_get_plat(dev); > > struct eth_pdata *pdata = &sun8i_pdata->eth_pdata; > > struct emac_eth_dev *priv = dev_get_priv(dev); > > + phys_addr_t syscon_base; > > const fdt32_t *reg; > > int node = dev_of_offset(dev); > > int offset = 0; > > @@ -838,13 +829,15 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) > > __func__); > > return -EINVAL; > > } > > - priv->sysctl_reg = fdt_translate_address((void *)gd->fdt_blob, > > - offset, reg); > > - if (priv->sysctl_reg == FDT_ADDR_T_NONE) { > > + > > + syscon_base = fdt_translate_address((void *)gd->fdt_blob, offset, reg); > > + if (syscon_base == FDT_ADDR_T_NONE) { > > debug("%s: Cannot find syscon base address\n", __func__); > > return -EINVAL; > > } > > > > + priv->sysctl_reg = (void *)syscon_base + priv->variant->syscon_offset; > > + > > pdata->phy_interface = -1; > > priv->phyaddr = -1; > > priv->use_internal_phy = false; > > @@ -903,25 +896,30 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) > > > > static const struct emac_variant emac_variant_a83t = { > > .variant = A83T_EMAC, > > + .syscon_offset = 0x30, > > }; > > > > static const struct emac_variant emac_variant_h3 = { > > .variant = H3_EMAC, > > + .syscon_offset = 0x30, > > .soc_has_internal_phy = true, > > .support_rmii = true, > > }; > > > > static const struct emac_variant emac_variant_r40 = { > > .variant = R40_GMAC, > > + .syscon_offset = 0x164, > > }; > > > > static const struct emac_variant emac_variant_a64 = { > > .variant = A64_EMAC, > > + .syscon_offset = 0x30, > > .support_rmii = true, > > }; > > > > static const struct emac_variant emac_variant_h6 = { > > .variant = H6_EMAC, > > + .syscon_offset = 0x30, > > .support_rmii = true, > > }; > > > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c index 36cc2498b5..231aac19e3 100644 --- a/drivers/net/sun8i_emac.c +++ b/drivers/net/sun8i_emac.c @@ -137,6 +137,7 @@ enum emac_variant_id { struct emac_variant { enum emac_variant_id variant; + uint syscon_offset; bool soc_has_internal_phy; bool support_rmii; }; @@ -168,7 +169,7 @@ struct emac_eth_dev { const struct emac_variant *variant; void *mac_reg; - phys_addr_t sysctl_reg; + void *sysctl_reg; struct phy_device *phydev; struct mii_dev *bus; struct clk tx_clk; @@ -323,18 +324,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata, { u32 reg; - if (priv->variant->variant == R40_GMAC) { - /* Select RGMII for R40 */ - reg = readl(priv->sysctl_reg + 0x164); - reg |= SC_ETCS_INT_GMII | - SC_EPIT | - (CONFIG_GMAC_TX_DELAY << SC_ETXDC_OFFSET); - - writel(reg, priv->sysctl_reg + 0x164); - return 0; - } - - reg = readl(priv->sysctl_reg + 0x30); + reg = readl(priv->sysctl_reg); reg = sun8i_emac_set_syscon_ephy(priv, reg); @@ -370,7 +360,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata, reg |= ((pdata->rx_delay_ps / 100) << SC_ERXDC_OFFSET) & SC_ERXDC_MASK; - writel(reg, priv->sysctl_reg + 0x30); + writel(reg, priv->sysctl_reg); return 0; } @@ -793,6 +783,7 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) struct sun8i_eth_pdata *sun8i_pdata = dev_get_plat(dev); struct eth_pdata *pdata = &sun8i_pdata->eth_pdata; struct emac_eth_dev *priv = dev_get_priv(dev); + phys_addr_t syscon_base; const fdt32_t *reg; int node = dev_of_offset(dev); int offset = 0; @@ -838,13 +829,15 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) __func__); return -EINVAL; } - priv->sysctl_reg = fdt_translate_address((void *)gd->fdt_blob, - offset, reg); - if (priv->sysctl_reg == FDT_ADDR_T_NONE) { + + syscon_base = fdt_translate_address((void *)gd->fdt_blob, offset, reg); + if (syscon_base == FDT_ADDR_T_NONE) { debug("%s: Cannot find syscon base address\n", __func__); return -EINVAL; } + priv->sysctl_reg = (void *)syscon_base + priv->variant->syscon_offset; + pdata->phy_interface = -1; priv->phyaddr = -1; priv->use_internal_phy = false; @@ -903,25 +896,30 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev) static const struct emac_variant emac_variant_a83t = { .variant = A83T_EMAC, + .syscon_offset = 0x30, }; static const struct emac_variant emac_variant_h3 = { .variant = H3_EMAC, + .syscon_offset = 0x30, .soc_has_internal_phy = true, .support_rmii = true, }; static const struct emac_variant emac_variant_r40 = { .variant = R40_GMAC, + .syscon_offset = 0x164, }; static const struct emac_variant emac_variant_a64 = { .variant = A64_EMAC, + .syscon_offset = 0x30, .support_rmii = true, }; static const struct emac_variant emac_variant_h6 = { .variant = H6_EMAC, + .syscon_offset = 0x30, .support_rmii = true, };
While R40 puts the EMAC syscon register at a different address from other variants, the relevant portion of the register's layout is the same. Factor out the register offset so the same code can be shared by all variants. This matches what the Linux driver does. This change provides two benefits beyond the simplification: - R40 boards now respect the RX delays from the devicetree - This resolves a warning on architectures where readl/writel expect the address to have a pointer type, not phys_addr_t. Signed-off-by: Samuel Holland <samuel@sholland.org> --- Changes in v2: - Add a structure for driver data, and put the syscon offset there drivers/net/sun8i_emac.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)