| Message ID | 1443692893-19905-6-git-send-email-sjoerd.simons@collabora.co.uk |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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(-)