[U-Boot] [PATCH v2 07/12] net: gmac_rk3288: Add RK3288 GMAC driver

[Simon Glass]

Hi Sjeord,

On 11 March 2016 at 14:56, Sjoerd Simons <sjoerd.simons at collabora.co.uk> wrote:
> On Mon, 2016-02-29 at 19:03 -0700, Simon Glass wrote:
>> Hi Sjoerd,
>>
>> On 28 February 2016 at 14:25, Sjoerd Simons
>> <sjoerd.simons at collabora.co.uk> wrote:
>> >
>> >
>> > diff --git a/drivers/net/gmac_rk3288.c b/drivers/net/gmac_rk3288.c
>> > new file mode 100644
>> > index 0000000..5400b2c
>> > --- /dev/null
>> > +++ b/drivers/net/gmac_rk3288.c
>> > @@ -0,0 +1,125 @@
>> >
>> > +       priv->fix_mac_speed = gmac_rk3288_fix_mac_speed;
>> > +
>> > +       return designware_eth_probe(dev);
>> This presumably called gmac_rk3288_fix_mac_speed(). Is it possible to
>> split the init so that you can call gmac_rk3288_fix_mac_speed()
>> directly here?
>
> No it gets called down the line from dw eth_ops start function once the
> ethernet driver is started and the phy speed is known so the mac can be
> adjust to match.
>
> So to do it in the driver, it would need to override the eth start op
> from its parent. Possible by exporting more of the designwares
> implementation so specific ops can be overridden.
>
> Seems less elegant then having a hook in the semantically right spot
> (e.g. after the link speed is determined)?
>
> Ooi what's your worry with function pointers in DM ?

I'm trying to avoid them. It is effectively an unofficial API that
doesn't go through DM. It suggests there is an extra layer of
software, but not quite...it is messy.

In the same vein I'm not keen on weak functions.

It looks to me as if the designware driver, which is currently
stand-alone, is being extended into a slightly different driver by
your patch. We really avoid that because we'll end up with more and
more fiddling for each SoC to make it work the way we want. This is
the kind of thing that driver model aims to avoid.

In that case here's my suggestion:
- Turn designware.c into a code library instead of a driver, and
export various functions
- Add a new dw_mac.c or something, which has the compatible strings
and the U_BOOT_DRIVER() declaration, and uses the
designware_eth_start(), etc. calls from designware.c
- Make your new driver do the same, except use your own start function

I see at the end of _dw_eth_init() there is a writel() that might need
to happen after your link adjustment. In that case this could be moved
into a separate function perhaps.

Regards,
Simon