[U-Boot] [PATCH 3/4] dm: net: macb: Implement link speed change callback

[Bin Meng <bmeng.cn@gmail.com>]

Hi Lukas,

On Mon, May 20, 2019 at 7:29 PM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Thu, 2019-05-16 at 02:12 -0700, Bin Meng wrote:
> > At present the link speed change callback is a nop. According to
> > macb device tree bindings, an optional "tx_clk" is used to clock
> > the ethernet controller's TX_CLK under different link speed.
> >
> > In 10/100 MII mode, transmit logic must be clocked from a free
> > running clock generated by the external PHY. In gigabit GMII mode,
> > the controller, not the external PHY, must generate the 125 MHz
> > transmit clock towards the PHY.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  drivers/net/macb.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> > index b7f404e..7d86fa1 100644
> > --- a/drivers/net/macb.c
> > +++ b/drivers/net/macb.c
> > @@ -497,6 +497,32 @@ static int macb_phy_find(struct macb_device *macb, const char *name)
> >  #ifdef CONFIG_DM_ETH
> >  int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed)
> >  {
> > +#ifdef CONFIG_CLK
> > +     struct clk tx_clk;
> > +     ulong rate;
> > +     int ret;
> > +
> > +     ret = clk_get_by_name(dev, "tx_clk", &tx_clk);
> > +     if (ret)
> > +             return 0;
>
> Can you return errors in ret here?

No, we should ignore DT that does not contain the "tx_clk" source
hence cannot return error here. I will add a comment in v2.

>
> > +
> > +     switch (speed) {
> > +     case _10BASET:
> > +             rate = 2500000;
> > +             break;
> > +     case _100BASET:
> > +             rate = 25000000;
> > +             break;
> > +     case _1000BASET:
> > +     default:
> > +             rate = 125000000;
> > +             break;
>
> The Linux driver simply returns in the default case, without changing
> tx_clk. I am wondering if we should do the same in U-Boot?

Sure.

>
> > +     }
> > +
> > +     if (tx_clk.dev)
> > +             clk_set_rate(&tx_clk, rate);
>
> Can you also check the return value of clk_set_rate() here?

Will add the return value check in v2.

Regards,
Bin