| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aAqG_83YL0drm6sd@Red>
Date: Thu, 24 Apr 2025 20:46:23 +0200
From: Corentin Labbe <clabbe.montjoie@...il.com>
To: Andre Przywara <andre.przywara@....com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>, Yixun Lan <dlan@...too.org>,
Maxime Ripard <mripard@...nel.org>, netdev@...r.kernel.org,
linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next] net: stmmac: sun8i: drop unneeded default
syscon value
Le Wed, Apr 23, 2025 at 10:52:22AM +0100, 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.
>
> Signed-off-by: Andre Przywara <andre.przywara@....com>
> ---
> 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
>
Hello
I tested this patch on:
sun20i-d1-nezha
sun50i-a64-bananapi-m64
sun8i-a83t-bananapi-m3
sun50i-h6-orangepi-one-plus
sun8i-h3-orangepi-pc
sun50i-h6-pine-h64
sun8i-h2-plus-orangepi-r1
sun8i-h2-plus-orangepi-zero
sun8i-r40-bananapi-m2-ultra
and didnt see any regression.
So you have:
Acked-by: Corentin LABBE <clabbe.montjoie@...il.com>
Tested-by: Corentin LABBE <clabbe.montjoie@...il.com>
Thanks
> .../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
>
Powered by blists - more mailing lists