netdev - Re: [PATCH net-next 3/3] net: stmmac: Add RK3566/RK3568 SoC support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Apr 2021 00:33:12 +0800
From:   Chen-Yu Tsai <wens213@...il.com>
To:     Ezequiel Garcia <ezequiel@...labora.com>
Cc:     Heiko Stübner <heiko@...ech.de>,
        Peter Geis <pgwipeout@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        David Wu <david.wu@...k-chips.com>, kernel@...labora.com
Subject: Re: [PATCH net-next 3/3] net: stmmac: Add RK3566/RK3568 SoC support

On Thu, Apr 15, 2021 at 12:14 AM Ezequiel Garcia <ezequiel@...labora.com> wrote:
>
> Hi Peter, Heiko,
>
> On Wed, 2021-04-14 at 13:15 +0200, Heiko Stübner wrote:
> > Am Mittwoch, 14. April 2021, 13:03:25 CEST schrieb Peter Geis:
> > > On Tue, Apr 13, 2021 at 7:37 PM Ezequiel Garcia <ezequiel@...labora.com> wrote:
> > > > > > +static void rk3566_set_to_rmii(struct rk_priv_data *bsp_priv)
> > > > > > +{
> > > > > > +       struct device *dev = &bsp_priv->pdev->dev;
> > > > > > +
> > > > > > +       if (IS_ERR(bsp_priv->grf)) {
> > > > > > +               dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
> > > > > > +               return;
> > > > > > +       }
> > > > > > +
> > > > > > +       regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON1,
> > > > > > +                    RK3568_GMAC_PHY_INTF_SEL_RMII);
> > > > > > +}
> > > > > > +
> > > > > > +static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
> > > > > > +                               int tx_delay, int rx_delay)
> > > > > > +{
> > > > > > +       struct device *dev = &bsp_priv->pdev->dev;
> > > > > > +
> > > > > > +       if (IS_ERR(bsp_priv->grf)) {
> > > > > > +               dev_err(dev, "Missing rockchip,grf property\n");
> > > > > > +               return;
> > > > > > +       }
> > > > > > +
> > > > > > +       regmap_write(bsp_priv->grf, RK3568_GRF_GMAC0_CON1,
> > > > > > +                    RK3568_GMAC_PHY_INTF_SEL_RGMII |
> > > > > > +                    RK3568_GMAC_RXCLK_DLY_ENABLE |
> > > > > > +                    RK3568_GMAC_TXCLK_DLY_ENABLE);
> > > > > > +
> > > > > > +       regmap_write(bsp_priv->grf, RK3568_GRF_GMAC0_CON0,
> > > > > > +                    RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> > > > > > +                    RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> > > > > > +
> > > > > > +       regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON1,
> > > > > > +                    RK3568_GMAC_PHY_INTF_SEL_RGMII |
> > > > > > +                    RK3568_GMAC_RXCLK_DLY_ENABLE |
> > > > > > +                    RK3568_GMAC_TXCLK_DLY_ENABLE);
> > > > > > +
> > > > > > +       regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON0,
> > > > > > +                    RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> > > > > > +                    RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> > > > >
> > > > > Since there are two GMACs on the rk3568, and either, or, or both may
> > > > > be enabled in various configurations, we should only configure the
> > > > > controller we are currently operating.
> > >
> > > Perhaps we should have match data (such as reg = <0>, or against the
> > > address) to identify the individual controllers.
> >
> > Hmm, "reg" will be used by the actual mmio address of the controller,
> > so matching against that should be the way I guess.
> >
> > We're already doing something similar for dsi:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1170
> >
>
> I have to admit, I'm not a fan of hardcoding the registers in the kernel.
>
> David Wu solved this in the downstream kernel by using bus_id,
> which parses the devicetree "ethernet@0" node, i.e.:
>
>   plat->bus_id = of_alias_get_id(np, "ethernet");

What happens when one adds another ethernet controller (USB or PCIe) to
the board and wants to change the numbering order?

Or maybe only the second ethernet controller is routed on some board
and the submitter / vendor wants that one to be ethernet0, because
it's the only usable controller?

This seems even more fragile than hardcoding the registers.

Regards
ChenYu