[05/10] net: designware: Add a post-started hook

Message ID 1443692893-19905-6-git-send-email-sjoerd.simons@collabora.co.uk
State New
Headers show

Commit Message

Sjoerd Simons Oct. 1, 2015, 9:48 a.m.
Add the ability for e.g. drivers subclassing to register a function to
be called after ethernet initialisation. This is useful if e.g. the
driver needs to change configuration based on the negotiated speed.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 drivers/net/designware.c | 11 ++++++++++-
 drivers/net/designware.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Bin Meng Oct. 1, 2015, 11:27 a.m. | #1
Hi Sjoerd,

On Thu, Oct 1, 2015 at 5:48 PM, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Add the ability for e.g. drivers subclassing to register a function to
> be called after ethernet initialisation. This is useful if e.g. the
> driver needs to change configuration based on the negotiated speed.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  drivers/net/designware.c | 11 ++++++++++-
>  drivers/net/designware.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 0b7adc9..da27041 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -564,8 +564,17 @@ int designware_initialize(ulong base_addr, u32 interface)
>  static int designware_eth_start(struct udevice *dev)
>  {
>         struct eth_pdata *pdata = dev_get_platdata(dev);
> +       struct dw_eth_dev *priv = dev_get_priv(dev);
> +       int ret;
>
> -       return _dw_eth_init(dev->priv, pdata->enetaddr);
> +       ret = _dw_eth_init(priv, pdata->enetaddr);
> +       if (ret)
> +               return ret;
> +
> +       if (priv->started)
> +               ret = priv->started(dev);

It looks to me a better approach to set up the MAC clock is to insert
the hook in dw_adjust_link(). And see below ..

> +
> +       return ret;
>  }
>
>  static int designware_eth_send(struct udevice *dev, void *packet, int length)
> diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> index 47e727b..b45599b 100644
> --- a/drivers/net/designware.h
> +++ b/drivers/net/designware.h
> @@ -236,6 +236,10 @@ struct dw_eth_dev {
>  #endif
>         struct phy_device *phydev;
>         struct mii_dev *bus;
> +
> +#ifdef CONFIG_DM_ETH
> +       int (*started)(struct udevice *dev);
> +#endif

We can name this as something like (*clk_set) to be clearer.
(*started) is not that intuitive.

>  };
>
>  #ifdef CONFIG_DM_ETH
> --

Regards,
Bin
Simon Glass Oct. 3, 2015, 2:30 p.m. | #2
Hi Sjoerd,

On 1 October 2015 at 10:48, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote:
> Add the ability for e.g. drivers subclassing to register a function to
> be called after ethernet initialisation. This is useful if e.g. the
> driver needs to change configuration based on the negotiated speed.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  drivers/net/designware.c | 11 ++++++++++-
>  drivers/net/designware.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 0b7adc9..da27041 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -564,8 +564,17 @@ int designware_initialize(ulong base_addr, u32 interface)
>  static int designware_eth_start(struct udevice *dev)
>  {
>         struct eth_pdata *pdata = dev_get_platdata(dev);
> +       struct dw_eth_dev *priv = dev_get_priv(dev);
> +       int ret;
>
> -       return _dw_eth_init(dev->priv, pdata->enetaddr);
> +       ret = _dw_eth_init(priv, pdata->enetaddr);
> +       if (ret)
> +               return ret;
> +
> +       if (priv->started)
> +               ret = priv->started(dev);
> +
> +       return ret;
>  }
>
>  static int designware_eth_send(struct udevice *dev, void *packet, int length)
> diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> index 47e727b..b45599b 100644
> --- a/drivers/net/designware.h
> +++ b/drivers/net/designware.h
> @@ -236,6 +236,10 @@ struct dw_eth_dev {
>  #endif
>         struct phy_device *phydev;
>         struct mii_dev *bus;
> +
> +#ifdef CONFIG_DM_ETH
> +       int (*started)(struct udevice *dev);
> +#endif

With driver model we should not need to add such hooks. is this needed
because we don't have a PHY uclass yet?

>  };
>
>  #ifdef CONFIG_DM_ETH
> --
> 2.5.3
>

Regards,
Simon
Sjoerd Simons Oct. 5, 2015, 8:55 a.m. | #3
On Sat, 2015-10-03 at 15:30 +0100, Simon Glass wrote:
> Hi Sjoerd,

> >  static int designware_eth_send(struct udevice *dev, void *packet,
> > int length)
> > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > index 47e727b..b45599b 100644
> > --- a/drivers/net/designware.h
> > +++ b/drivers/net/designware.h
> > @@ -236,6 +236,10 @@ struct dw_eth_dev {
> >  #endif
> >         struct phy_device *phydev;
> >         struct mii_dev *bus;
> > +
> > +#ifdef CONFIG_DM_ETH
> > +       int (*started)(struct udevice *dev);
> > +#endif
> 
> With driver model we should not need to add such hooks. is this
> needed
> because we don't have a PHY uclass yet?

Essentially I need to be able to configure one of the GMAC clocks
depending on the result of the phy negotiation. Looking at the linux
kernel this seems a rather common item for the dw gmac interface.

I guess, I could do without that hook by exporting all functions
required to fill the eth_ops struct and override the start function.
Would you prefer that?

I guess a PHY uclass would also help here, assuming such a uclass would
provide a callback for link state changes (e.g. like of_phy_connect in
Linux).

> >  };
> > 
> >  #ifdef CONFIG_DM_ETH
> > --
> > 2.5.3
> > 
> 
> Regards,
> Simon
Joe Hershberger Oct. 5, 2015, 4:39 p.m. | #4
Hi Sjoerd,

On Mon, Oct 5, 2015 at 3:55 AM, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
>
> On Sat, 2015-10-03 at 15:30 +0100, Simon Glass wrote:
> > Hi Sjoerd,
>
> > >  static int designware_eth_send(struct udevice *dev, void *packet,
> > > int length)
> > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > > index 47e727b..b45599b 100644
> > > --- a/drivers/net/designware.h
> > > +++ b/drivers/net/designware.h
> > > @@ -236,6 +236,10 @@ struct dw_eth_dev {
> > >  #endif
> > >         struct phy_device *phydev;
> > >         struct mii_dev *bus;
> > > +
> > > +#ifdef CONFIG_DM_ETH
> > > +       int (*started)(struct udevice *dev);
> > > +#endif
> >
> > With driver model we should not need to add such hooks. is this
> > needed
> > because we don't have a PHY uclass yet?
>
> Essentially I need to be able to configure one of the GMAC clocks
> depending on the result of the phy negotiation. Looking at the linux
> kernel this seems a rather common item for the dw gmac interface.
>
> I guess, I could do without that hook by exporting all functions
> required to fill the eth_ops struct and override the start function.
> Would you prefer that?

I prefer the hook.

> I guess a PHY uclass would also help here, assuming such a uclass would
> provide a callback for link state changes (e.g. like of_phy_connect in
> Linux).

I agree that once we have the phy uclass then we should handle it much
more like Linux, but for now the hook seems cleaner.

> > >  };
> > >
> > >  #ifdef CONFIG_DM_ETH
> > > --
> > > 2.5.3
> > >
> >
> > Regards,
> > Simon
>
> --
> Sjoerd Simons
> Collabora Ltd.
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Bin Meng Oct. 7, 2015, 9:45 a.m. | #5
Hi,

On Tue, Oct 6, 2015 at 12:39 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Sjoerd,
>
> On Mon, Oct 5, 2015 at 3:55 AM, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
>>
>> On Sat, 2015-10-03 at 15:30 +0100, Simon Glass wrote:
>> > Hi Sjoerd,
>>
>> > >  static int designware_eth_send(struct udevice *dev, void *packet,
>> > > int length)
>> > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
>> > > index 47e727b..b45599b 100644
>> > > --- a/drivers/net/designware.h
>> > > +++ b/drivers/net/designware.h
>> > > @@ -236,6 +236,10 @@ struct dw_eth_dev {
>> > >  #endif
>> > >         struct phy_device *phydev;
>> > >         struct mii_dev *bus;
>> > > +
>> > > +#ifdef CONFIG_DM_ETH
>> > > +       int (*started)(struct udevice *dev);
>> > > +#endif
>> >
>> > With driver model we should not need to add such hooks. is this
>> > needed
>> > because we don't have a PHY uclass yet?
>>
>> Essentially I need to be able to configure one of the GMAC clocks
>> depending on the result of the phy negotiation. Looking at the linux
>> kernel this seems a rather common item for the dw gmac interface.
>>
>> I guess, I could do without that hook by exporting all functions
>> required to fill the eth_ops struct and override the start function.
>> Would you prefer that?
>
> I prefer the hook.
>
>> I guess a PHY uclass would also help here, assuming such a uclass would
>> provide a callback for link state changes (e.g. like of_phy_connect in
>> Linux).
>

From what I read to this codes, the work done in the hook is to
program the MAC's clock, not the PHY. So not sure if the PHY uclass
can help here.

> I agree that once we have the phy uclass then we should handle it much
> more like Linux, but for now the hook seems cleaner.
>

Has anyone looked at my suggestion about where we should insert the hook?

Regards,
Bin
Joe Hershberger Oct. 9, 2015, 1:40 p.m. | #6
Hi Bin,

On Wed, Oct 7, 2015 at 4:45 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi,
>
> On Tue, Oct 6, 2015 at 12:39 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Sjoerd,
>>
>> On Mon, Oct 5, 2015 at 3:55 AM, Sjoerd Simons
>> <sjoerd.simons@collabora.co.uk> wrote:
>>>
>>> On Sat, 2015-10-03 at 15:30 +0100, Simon Glass wrote:
>>> > Hi Sjoerd,
>>>
>>> > >  static int designware_eth_send(struct udevice *dev, void *packet,
>>> > > int length)
>>> > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
>>> > > index 47e727b..b45599b 100644
>>> > > --- a/drivers/net/designware.h
>>> > > +++ b/drivers/net/designware.h
>>> > > @@ -236,6 +236,10 @@ struct dw_eth_dev {
>>> > >  #endif
>>> > >         struct phy_device *phydev;
>>> > >         struct mii_dev *bus;
>>> > > +
>>> > > +#ifdef CONFIG_DM_ETH
>>> > > +       int (*started)(struct udevice *dev);
>>> > > +#endif
>>> >
>>> > With driver model we should not need to add such hooks. is this
>>> > needed
>>> > because we don't have a PHY uclass yet?
>>>
>>> Essentially I need to be able to configure one of the GMAC clocks
>>> depending on the result of the phy negotiation. Looking at the linux
>>> kernel this seems a rather common item for the dw gmac interface.
>>>
>>> I guess, I could do without that hook by exporting all functions
>>> required to fill the eth_ops struct and override the start function.
>>> Would you prefer that?
>>
>> I prefer the hook.
>>
>>> I guess a PHY uclass would also help here, assuming such a uclass would
>>> provide a callback for link state changes (e.g. like of_phy_connect in
>>> Linux).
>>
>
> From what I read to this codes, the work done in the hook is to
> program the MAC's clock, not the PHY. So not sure if the PHY uclass
> can help here.
>
>> I agree that once we have the phy uclass then we should handle it much
>> more like Linux, but for now the hook seems cleaner.
>>
>
> Has anyone looked at my suggestion about where we should insert the hook?

I looked at this today and agree that for the DW driver, that is a
more appropriate place to put this hook (especially if renamed as you
proposed).

Ultimately I still think it will be better to have a hook added to
UCLASS_ETH that is invoked by UCLASS_ETH_PHY on link change. This way
it is available to other eth drivers as well. I know that we will also
need this for the macb driver.

-Joe

Patch

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 0b7adc9..da27041 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -564,8 +564,17 @@  int designware_initialize(ulong base_addr, u32 interface)
 static int designware_eth_start(struct udevice *dev)
 {
 	struct eth_pdata *pdata = dev_get_platdata(dev);
+	struct dw_eth_dev *priv = dev_get_priv(dev);
+	int ret;
 
-	return _dw_eth_init(dev->priv, pdata->enetaddr);
+	ret = _dw_eth_init(priv, pdata->enetaddr);
+	if (ret)
+		return ret;
+
+	if (priv->started)
+		ret = priv->started(dev);
+
+	return ret;
 }
 
 static int designware_eth_send(struct udevice *dev, void *packet, int length)
diff --git a/drivers/net/designware.h b/drivers/net/designware.h
index 47e727b..b45599b 100644
--- a/drivers/net/designware.h
+++ b/drivers/net/designware.h
@@ -236,6 +236,10 @@  struct dw_eth_dev {
 #endif
 	struct phy_device *phydev;
 	struct mii_dev *bus;
+
+#ifdef CONFIG_DM_ETH
+	int (*started)(struct udevice *dev);
+#endif
 };
 
 #ifdef CONFIG_DM_ETH