| Message ID | 1483532844-20649-3-git-send-email-stefan.herbrechtsmeier@weidmueller.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.de> > > If available use the clock framework to set the tx clock rate of the > zynq ethernet controller. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > --- > > arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- > drivers/net/zynq_gem.c | 31 ++++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > index 1db2bd6..bd633a6 100644 > --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > @@ -8,11 +8,6 @@ > #ifndef _ASM_ARCH_SYS_PROTO_H > #define _ASM_ARCH_SYS_PROTO_H > > -/* Setup clk for network */ > -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned long clk_rate) > -{ > -} > - > int zynq_slcr_get_mio_pin_status(const char *periph); > > unsigned int zynqmp_get_silicon_version(void); > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c > index 9bfb89f..9118e49 100644 > --- a/drivers/net/zynq_gem.c > +++ b/drivers/net/zynq_gem.c > @@ -9,6 +9,7 @@ > * SPDX-License-Identifier: GPL-2.0+ > */ > > +#include <clk.h> > #include <common.h> > #include <dm.h> > #include <net.h> > @@ -180,6 +181,9 @@ struct zynq_gem_priv { > struct phy_device *phydev; > int phy_of_handle; > struct mii_dev *bus; > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > + struct clk tx_clk; > +#endif > }; > > static inline int mdio_wait(struct zynq_gem_regs *regs) > @@ -364,6 +368,9 @@ static int zynq_gem_init(struct udevice *dev) > u32 i, nwconfig; > int ret; > unsigned long clk_rate = 0; > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > + unsigned long clk_rate_rounded; > +#endif > struct zynq_gem_priv *priv = dev_get_priv(dev); > struct zynq_gem_regs *regs = priv->iobase; > struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; > @@ -466,8 +473,22 @@ static int zynq_gem_init(struct udevice *dev) > break; > } > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); > + if (IS_ERR_VALUE(clk_rate_rounded)) { > + dev_err(dev, "failed to set tx clock rate\n"); > + return clk_rate_rounded; > + } > + > + ret = clk_enable(&priv->tx_clk); > + if (ret && ret != -ENOSYS) { > + dev_err(dev, "failed to enable tx clock\n"); > + return ret; > + } > +#else > zynq_slcr_gem_clk_setup((ulong)priv->iobase != > ZYNQ_GEM_BASEADDR0, clk_rate); > +#endif > > setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | > ZYNQ_GEM_NWCTRL_TXEN_MASK); > @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct udevice *dev) > struct eth_pdata *pdata = dev_get_platdata(dev); > struct zynq_gem_priv *priv = dev_get_priv(dev); > const char *phy_mode; > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > + int ret; > +#endif > > pdata->iobase = (phys_addr_t)dev_get_addr(dev); > priv->iobase = (struct zynq_gem_regs *)pdata->iobase; > @@ -699,6 +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice *dev) > } > priv->interface = pdata->phy_interface; > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > + ret = clk_get_by_index(dev, 2, &priv->tx_clk); > + if (ret < 0) { > + dev_err(dev, "failed to get clock\n"); > + return ret; > + } > +#endif > > printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv->iobase, > priv->phyaddr, phy_string_for_interface(priv->interface)); > Can you please rebase it on the top of this? https://patchwork.ozlabs.org/patch/706419/ The reason why this patch was written in this way is that we don't have full zynqmp clk driver yet but we are using fixed clock where you can't run set_rate which fails. The patch above will be merged soon that's why please rebase it on the top of it. Here is branch with this patch. http://git.denx.de/?p=u-boot/u-boot-microblaze.git;a=summary Thanks, Michal
> -----Ursprüngliche Nachricht----- > Von: Michal Simek [mailto:monstr@monstr.eu] > Gesendet: Dienstag, 10. Januar 2017 14:59 > An: Herbrechtsmeier, Stefan; u-boot@lists.denx.de > Cc: Herbrechtsmeier, Stefan; Albert Aribaud; Michal Simek; Joe > Hershberger > Betreff: Re: [PATCH 2/8] net: zynq: Add clk framework support to zynq > ethernet driver > > On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: > > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.de> > > > > If available use the clock framework to set the tx clock rate of the > > zynq ethernet controller. > > > > Signed-off-by: Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier@weidmueller.com> > > --- > > > > arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- > > drivers/net/zynq_gem.c | 31 > ++++++++++++++++++++++++++++ > > 2 files changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > > b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > > index 1db2bd6..bd633a6 100644 > > --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > > +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > > @@ -8,11 +8,6 @@ > > #ifndef _ASM_ARCH_SYS_PROTO_H > > #define _ASM_ARCH_SYS_PROTO_H > > > > -/* Setup clk for network */ > > -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned long > > clk_rate) -{ -} > > - > > int zynq_slcr_get_mio_pin_status(const char *periph); > > > > unsigned int zynqmp_get_silicon_version(void); diff --git > > a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index > > 9bfb89f..9118e49 100644 > > --- a/drivers/net/zynq_gem.c > > +++ b/drivers/net/zynq_gem.c > > @@ -9,6 +9,7 @@ > > * SPDX-License-Identifier: GPL-2.0+ > > */ > > > > +#include <clk.h> > > #include <common.h> > > #include <dm.h> > > #include <net.h> > > @@ -180,6 +181,9 @@ struct zynq_gem_priv { > > struct phy_device *phydev; > > int phy_of_handle; > > struct mii_dev *bus; > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > > + struct clk tx_clk; > > +#endif > > }; > > > > static inline int mdio_wait(struct zynq_gem_regs *regs) @@ -364,6 > > +368,9 @@ static int zynq_gem_init(struct udevice *dev) > > u32 i, nwconfig; > > int ret; > > unsigned long clk_rate = 0; > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > > + unsigned long clk_rate_rounded; > > +#endif > > struct zynq_gem_priv *priv = dev_get_priv(dev); > > struct zynq_gem_regs *regs = priv->iobase; > > struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; @@ - > 466,8 > > +473,22 @@ static int zynq_gem_init(struct udevice *dev) > > break; > > } > > > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > > + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); > > + if (IS_ERR_VALUE(clk_rate_rounded)) { > > + dev_err(dev, "failed to set tx clock rate\n"); > > + return clk_rate_rounded; > > + } > > + > > + ret = clk_enable(&priv->tx_clk); > > + if (ret && ret != -ENOSYS) { > > + dev_err(dev, "failed to enable tx clock\n"); > > + return ret; > > + } > > +#else > > zynq_slcr_gem_clk_setup((ulong)priv->iobase != > > ZYNQ_GEM_BASEADDR0, clk_rate); > > +#endif > > > > setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | > > ZYNQ_GEM_NWCTRL_TXEN_MASK); > > @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct > udevice *dev) > > struct eth_pdata *pdata = dev_get_platdata(dev); > > struct zynq_gem_priv *priv = dev_get_priv(dev); > > const char *phy_mode; > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > > + int ret; > > +#endif > > > > pdata->iobase = (phys_addr_t)dev_get_addr(dev); > > priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ -699,6 > > +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice > *dev) > > } > > priv->interface = pdata->phy_interface; > > > > +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > > + ret = clk_get_by_index(dev, 2, &priv->tx_clk); > > + if (ret < 0) { > > + dev_err(dev, "failed to get clock\n"); > > + return ret; > > + } > > +#endif > > > > printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv- > >iobase, > > priv->phyaddr, phy_string_for_interface(priv->interface)); > > > > Can you please rebase it on the top of this? > https://patchwork.ozlabs.org/patch/706419/ > > The reason why this patch was written in this way is that we don't have > full zynqmp clk driver yet but we are using fixed clock where you can't > run set_rate which fails. > The patch assumes that the EMIO_ENET_TX_CLK is always fixed. Is this true? Why don't you add a set_rate() function to the fixed clock? Additionally the patch is missing an enable function call. I don't think the emio configuration should be keep in the Ethernet driver as it is part of the clock configuration and doesn't influent the network interface. Regards, Stefan Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 - Komplementärin: Weidmüller Interface Führungsgesellschaft mbH - Sitz: Detmold - Amtsgericht Lemgo HRB 3924; Geschäftsführer: José Carlos Álvarez Tobar, Elke Eckstein, Dr. Peter Köhler, Jörg Timmermann; USt-ID-Nr. DE124599660
On 10.1.2017 15:46, Stefan.Herbrechtsmeier@weidmueller.com wrote: >> -----Ursprüngliche Nachricht----- >> Von: Michal Simek [mailto:monstr@monstr.eu] >> Gesendet: Dienstag, 10. Januar 2017 14:59 >> An: Herbrechtsmeier, Stefan; u-boot@lists.denx.de >> Cc: Herbrechtsmeier, Stefan; Albert Aribaud; Michal Simek; Joe >> Hershberger >> Betreff: Re: [PATCH 2/8] net: zynq: Add clk framework support to zynq >> ethernet driver >> >> On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: >>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.de> >>> >>> If available use the clock framework to set the tx clock rate of the >>> zynq ethernet controller. >>> >>> Signed-off-by: Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier@weidmueller.com> >>> --- >>> >>> arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- >>> drivers/net/zynq_gem.c | 31 >> ++++++++++++++++++++++++++++ >>> 2 files changed, 31 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>> b/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>> index 1db2bd6..bd633a6 100644 >>> --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>> +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>> @@ -8,11 +8,6 @@ >>> #ifndef _ASM_ARCH_SYS_PROTO_H >>> #define _ASM_ARCH_SYS_PROTO_H >>> >>> -/* Setup clk for network */ >>> -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned long >>> clk_rate) -{ -} >>> - >>> int zynq_slcr_get_mio_pin_status(const char *periph); >>> >>> unsigned int zynqmp_get_silicon_version(void); diff --git >>> a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index >>> 9bfb89f..9118e49 100644 >>> --- a/drivers/net/zynq_gem.c >>> +++ b/drivers/net/zynq_gem.c >>> @@ -9,6 +9,7 @@ >>> * SPDX-License-Identifier: GPL-2.0+ >>> */ >>> >>> +#include <clk.h> >>> #include <common.h> >>> #include <dm.h> >>> #include <net.h> >>> @@ -180,6 +181,9 @@ struct zynq_gem_priv { >>> struct phy_device *phydev; >>> int phy_of_handle; >>> struct mii_dev *bus; >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>> + struct clk tx_clk; >>> +#endif >>> }; >>> >>> static inline int mdio_wait(struct zynq_gem_regs *regs) @@ -364,6 >>> +368,9 @@ static int zynq_gem_init(struct udevice *dev) >>> u32 i, nwconfig; >>> int ret; >>> unsigned long clk_rate = 0; >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>> + unsigned long clk_rate_rounded; >>> +#endif >>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>> struct zynq_gem_regs *regs = priv->iobase; >>> struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; @@ - >> 466,8 >>> +473,22 @@ static int zynq_gem_init(struct udevice *dev) >>> break; >>> } >>> >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>> + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); >>> + if (IS_ERR_VALUE(clk_rate_rounded)) { >>> + dev_err(dev, "failed to set tx clock rate\n"); >>> + return clk_rate_rounded; >>> + } >>> + >>> + ret = clk_enable(&priv->tx_clk); >>> + if (ret && ret != -ENOSYS) { >>> + dev_err(dev, "failed to enable tx clock\n"); >>> + return ret; >>> + } >>> +#else >>> zynq_slcr_gem_clk_setup((ulong)priv->iobase != >>> ZYNQ_GEM_BASEADDR0, clk_rate); >>> +#endif >>> >>> setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | >>> ZYNQ_GEM_NWCTRL_TXEN_MASK); >>> @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct >> udevice *dev) >>> struct eth_pdata *pdata = dev_get_platdata(dev); >>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>> const char *phy_mode; >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>> + int ret; >>> +#endif >>> >>> pdata->iobase = (phys_addr_t)dev_get_addr(dev); >>> priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ -699,6 >>> +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice >> *dev) >>> } >>> priv->interface = pdata->phy_interface; >>> >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>> + ret = clk_get_by_index(dev, 2, &priv->tx_clk); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to get clock\n"); >>> + return ret; >>> + } >>> +#endif >>> >>> printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv- >>> iobase, >>> priv->phyaddr, phy_string_for_interface(priv->interface)); >>> >> >> Can you please rebase it on the top of this? >> https://patchwork.ozlabs.org/patch/706419/ >> >> The reason why this patch was written in this way is that we don't have >> full zynqmp clk driver yet but we are using fixed clock where you can't >> run set_rate which fails. >> > The patch assumes that the EMIO_ENET_TX_CLK is always fixed. Is this true? > > Why don't you add a set_rate() function to the fixed clock? It would be a solution but I am not sure if fixed clock was designed for this. I would expect that fixed clock only provided clock which you can't change. It means we need to have something as transition when u-boot has real clk driver for zynqmp. > > Additionally the patch is missing an enable function call. correct. > > I don't think the emio configuration should be keep in the Ethernet driver as it is part of the clock configuration and doesn't influent the network interface. Please correct me if I am wrong. Are you saying that with these patches especially 8/8 clock for emio will handled automatically? Thanks, Michal
> -----Ursprüngliche Nachricht----- > Von: Michal Simek [mailto:monstr@monstr.eu] > Gesendet: Dienstag, 10. Januar 2017 16:23 > > On 10.1.2017 15:46, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >> -----Ursprüngliche Nachricht----- > >> Von: Michal Simek [mailto:monstr@monstr.eu] > >> Gesendet: Dienstag, 10. Januar 2017 14:59 > >> An: Herbrechtsmeier, Stefan; u-boot@lists.denx.de > >> Cc: Herbrechtsmeier, Stefan; Albert Aribaud; Michal Simek; Joe > >> Hershberger > >> Betreff: Re: [PATCH 2/8] net: zynq: Add clk framework support to > zynq > >> ethernet driver > >> > >> On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: > >>> From: Stefan Herbrechtsmeier > <stefan.herbrechtsmeier@weidmueller.de> > >>> > >>> If available use the clock framework to set the tx clock rate of > the > >>> zynq ethernet controller. > >>> > >>> Signed-off-by: Stefan Herbrechtsmeier > >>> <stefan.herbrechtsmeier@weidmueller.com> > >>> --- > >>> > >>> arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- > >>> drivers/net/zynq_gem.c | 31 > >> ++++++++++++++++++++++++++++ > >>> 2 files changed, 31 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>> b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>> index 1db2bd6..bd633a6 100644 > >>> --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>> +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>> @@ -8,11 +8,6 @@ > >>> #ifndef _ASM_ARCH_SYS_PROTO_H > >>> #define _ASM_ARCH_SYS_PROTO_H > >>> > >>> -/* Setup clk for network */ > >>> -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned > >>> long > >>> clk_rate) -{ -} > >>> - > >>> int zynq_slcr_get_mio_pin_status(const char *periph); > >>> > >>> unsigned int zynqmp_get_silicon_version(void); diff --git > >>> a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index > >>> 9bfb89f..9118e49 100644 > >>> --- a/drivers/net/zynq_gem.c > >>> +++ b/drivers/net/zynq_gem.c > >>> @@ -9,6 +9,7 @@ > >>> * SPDX-License-Identifier: GPL-2.0+ > >>> */ > >>> > >>> +#include <clk.h> > >>> #include <common.h> > >>> #include <dm.h> > >>> #include <net.h> > >>> @@ -180,6 +181,9 @@ struct zynq_gem_priv { > >>> struct phy_device *phydev; > >>> int phy_of_handle; > >>> struct mii_dev *bus; > >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>> + struct clk tx_clk; > >>> +#endif > >>> }; > >>> > >>> static inline int mdio_wait(struct zynq_gem_regs *regs) @@ -364,6 > >>> +368,9 @@ static int zynq_gem_init(struct udevice *dev) > >>> u32 i, nwconfig; > >>> int ret; > >>> unsigned long clk_rate = 0; > >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>> + unsigned long clk_rate_rounded; > >>> +#endif > >>> struct zynq_gem_priv *priv = dev_get_priv(dev); > >>> struct zynq_gem_regs *regs = priv->iobase; > >>> struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; @@ - > >> 466,8 > >>> +473,22 @@ static int zynq_gem_init(struct udevice *dev) > >>> break; > >>> } > >>> > >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>> + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); > >>> + if (IS_ERR_VALUE(clk_rate_rounded)) { > >>> + dev_err(dev, "failed to set tx clock rate\n"); > >>> + return clk_rate_rounded; > >>> + } > >>> + > >>> + ret = clk_enable(&priv->tx_clk); > >>> + if (ret && ret != -ENOSYS) { > >>> + dev_err(dev, "failed to enable tx clock\n"); > >>> + return ret; > >>> + } > >>> +#else > >>> zynq_slcr_gem_clk_setup((ulong)priv->iobase != > >>> ZYNQ_GEM_BASEADDR0, clk_rate); > >>> +#endif > >>> > >>> setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | > >>> ZYNQ_GEM_NWCTRL_TXEN_MASK); > >>> @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct > >> udevice *dev) > >>> struct eth_pdata *pdata = dev_get_platdata(dev); > >>> struct zynq_gem_priv *priv = dev_get_priv(dev); > >>> const char *phy_mode; > >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>> + int ret; > >>> +#endif > >>> > >>> pdata->iobase = (phys_addr_t)dev_get_addr(dev); > >>> priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ -699,6 > >>> +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice > >> *dev) > >>> } > >>> priv->interface = pdata->phy_interface; > >>> > >>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>> + ret = clk_get_by_index(dev, 2, &priv->tx_clk); > >>> + if (ret < 0) { > >>> + dev_err(dev, "failed to get clock\n"); > >>> + return ret; > >>> + } > >>> +#endif > >>> > >>> printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv- > >>> iobase, > >>> priv->phyaddr, phy_string_for_interface(priv->interface)); > >>> > >> > >> Can you please rebase it on the top of this? > >> https://patchwork.ozlabs.org/patch/706419/ > >> > >> The reason why this patch was written in this way is that we don't > >> have full zynqmp clk driver yet but we are using fixed clock where > >> you can't run set_rate which fails. > >> > > The patch assumes that the EMIO_ENET_TX_CLK is always fixed. Is this > true? > > > > Why don't you add a set_rate() function to the fixed clock? > > It would be a solution but I am not sure if fixed clock was designed > for this. I would expect that fixed clock only provided clock which you > can't change. I think the current clock framework is not optimal as it doesn't abstract the clock framework from the drivers. I don't know why a not clock driver want to know that an internal clock ops isn't implemented. I would expect that the driver should not care what clock provides the rate. The clk_set_rate() function should always return the nearest possible clock rate and the clk_enable() shouldn't failed with a missing ops function. Otherwise this functions should be mandatory. > It means we need to have something as transition when u- > boot has real clk driver for zynqmp. We could use the same fall back for the clk_set_rate() as we use for the clk_enable() and ignore the -ENOSYS error. > > > > Additionally the patch is missing an enable function call. > > correct. > > > > > I don't think the emio configuration should be keep in the Ethernet > driver as it is part of the clock configuration and doesn't influent > the network interface. > > Please correct me if I am wrong. Are you saying that with these patches > especially 8/8 clock for emio will handled automatically? What do you mean by 8/8 clock? The Ethernet driver should not care how the clock is connected and only set the required clock rate. The clock driver should route the function calls to the correct clock. Regards, Stefan Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 - Komplementärin: Weidmüller Interface Führungsgesellschaft mbH - Sitz: Detmold - Amtsgericht Lemgo HRB 3924; Geschäftsführer: José Carlos Álvarez Tobar, Elke Eckstein, Dr. Peter Köhler, Jörg Timmermann; USt-ID-Nr. DE124599660
On 11.1.2017 08:30, Stefan.Herbrechtsmeier@weidmueller.com wrote: >> -----Ursprüngliche Nachricht----- >> Von: Michal Simek [mailto:monstr@monstr.eu] >> Gesendet: Dienstag, 10. Januar 2017 16:23 >> >> On 10.1.2017 15:46, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>>> -----Ursprüngliche Nachricht----- >>>> Von: Michal Simek [mailto:monstr@monstr.eu] >>>> Gesendet: Dienstag, 10. Januar 2017 14:59 >>>> An: Herbrechtsmeier, Stefan; u-boot@lists.denx.de >>>> Cc: Herbrechtsmeier, Stefan; Albert Aribaud; Michal Simek; Joe >>>> Hershberger >>>> Betreff: Re: [PATCH 2/8] net: zynq: Add clk framework support to >> zynq >>>> ethernet driver >>>> >>>> On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: >>>>> From: Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier@weidmueller.de> >>>>> >>>>> If available use the clock framework to set the tx clock rate of >> the >>>>> zynq ethernet controller. >>>>> >>>>> Signed-off-by: Stefan Herbrechtsmeier >>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>> --- >>>>> >>>>> arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- >>>>> drivers/net/zynq_gem.c | 31 >>>> ++++++++++++++++++++++++++++ >>>>> 2 files changed, 31 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>> b/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>> index 1db2bd6..bd633a6 100644 >>>>> --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>> +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>> @@ -8,11 +8,6 @@ >>>>> #ifndef _ASM_ARCH_SYS_PROTO_H >>>>> #define _ASM_ARCH_SYS_PROTO_H >>>>> >>>>> -/* Setup clk for network */ >>>>> -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned >>>>> long >>>>> clk_rate) -{ -} >>>>> - >>>>> int zynq_slcr_get_mio_pin_status(const char *periph); >>>>> >>>>> unsigned int zynqmp_get_silicon_version(void); diff --git >>>>> a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index >>>>> 9bfb89f..9118e49 100644 >>>>> --- a/drivers/net/zynq_gem.c >>>>> +++ b/drivers/net/zynq_gem.c >>>>> @@ -9,6 +9,7 @@ >>>>> * SPDX-License-Identifier: GPL-2.0+ >>>>> */ >>>>> >>>>> +#include <clk.h> >>>>> #include <common.h> >>>>> #include <dm.h> >>>>> #include <net.h> >>>>> @@ -180,6 +181,9 @@ struct zynq_gem_priv { >>>>> struct phy_device *phydev; >>>>> int phy_of_handle; >>>>> struct mii_dev *bus; >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>> + struct clk tx_clk; >>>>> +#endif >>>>> }; >>>>> >>>>> static inline int mdio_wait(struct zynq_gem_regs *regs) @@ -364,6 >>>>> +368,9 @@ static int zynq_gem_init(struct udevice *dev) >>>>> u32 i, nwconfig; >>>>> int ret; >>>>> unsigned long clk_rate = 0; >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>> + unsigned long clk_rate_rounded; >>>>> +#endif >>>>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>>>> struct zynq_gem_regs *regs = priv->iobase; >>>>> struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; @@ - >>>> 466,8 >>>>> +473,22 @@ static int zynq_gem_init(struct udevice *dev) >>>>> break; >>>>> } >>>>> >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>> + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); >>>>> + if (IS_ERR_VALUE(clk_rate_rounded)) { >>>>> + dev_err(dev, "failed to set tx clock rate\n"); >>>>> + return clk_rate_rounded; >>>>> + } >>>>> + >>>>> + ret = clk_enable(&priv->tx_clk); >>>>> + if (ret && ret != -ENOSYS) { >>>>> + dev_err(dev, "failed to enable tx clock\n"); >>>>> + return ret; >>>>> + } >>>>> +#else >>>>> zynq_slcr_gem_clk_setup((ulong)priv->iobase != >>>>> ZYNQ_GEM_BASEADDR0, clk_rate); >>>>> +#endif >>>>> >>>>> setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | >>>>> ZYNQ_GEM_NWCTRL_TXEN_MASK); >>>>> @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct >>>> udevice *dev) >>>>> struct eth_pdata *pdata = dev_get_platdata(dev); >>>>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>>>> const char *phy_mode; >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>> + int ret; >>>>> +#endif >>>>> >>>>> pdata->iobase = (phys_addr_t)dev_get_addr(dev); >>>>> priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ -699,6 >>>>> +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice >>>> *dev) >>>>> } >>>>> priv->interface = pdata->phy_interface; >>>>> >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>> + ret = clk_get_by_index(dev, 2, &priv->tx_clk); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "failed to get clock\n"); >>>>> + return ret; >>>>> + } >>>>> +#endif >>>>> >>>>> printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv- >>>>> iobase, >>>>> priv->phyaddr, phy_string_for_interface(priv->interface)); >>>>> >>>> >>>> Can you please rebase it on the top of this? >>>> https://patchwork.ozlabs.org/patch/706419/ >>>> >>>> The reason why this patch was written in this way is that we don't >>>> have full zynqmp clk driver yet but we are using fixed clock where >>>> you can't run set_rate which fails. >>>> >>> The patch assumes that the EMIO_ENET_TX_CLK is always fixed. Is this >> true? >>> >>> Why don't you add a set_rate() function to the fixed clock? >> >> It would be a solution but I am not sure if fixed clock was designed >> for this. I would expect that fixed clock only provided clock which you >> can't change. > > I think the current clock framework is not optimal as it doesn't abstract the clock framework from the drivers. I don't know why a not clock driver want to know that an internal clock ops isn't implemented. > > I would expect that the driver should not care what clock provides the rate. The clk_set_rate() function should always return the nearest possible clock rate and the clk_enable() shouldn't failed with a missing ops function. Otherwise this functions should be mandatory. I can't see an issue if clk_set_rate simply failed for clocks which can't change. It is up to driver to handle this if this is fine or not. In ZynqMP some clocks can be locked because they are used for something more important than non secure sw. > >> It means we need to have something as transition when u- >> boot has real clk driver for zynqmp. > > We could use the same fall back for the clk_set_rate() as we use for the clk_enable() and ignore the -ENOSYS error. Not a problem with using -ENOSYS which is just a signal that clk driver doesn't care about it. > >>> >>> Additionally the patch is missing an enable function call. >> >> correct. >> >>> >>> I don't think the emio configuration should be keep in the Ethernet >> driver as it is part of the clock configuration and doesn't influent >> the network interface. >> >> Please correct me if I am wrong. Are you saying that with these patches >> especially 8/8 clock for emio will handled automatically? > > What do you mean by 8/8 clock? I meant patch 8/8 in this series. > > The Ethernet driver should not care how the clock is connected and only set the required clock rate. The clock driver should route the function calls to the correct clock. ok. I will try to get testcases for emio to make sure that they are still working correctly. Thanks, Michal
> -----Ursprüngliche Nachricht----- > Von: Michal Simek [mailto:michal.simek@xilinx.com] > Gesendet: Mittwoch, 11. Januar 2017 09:22 > > On 11.1.2017 08:30, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >> -----Ursprüngliche Nachricht----- > >> Von: Michal Simek [mailto:monstr@monstr.eu] > >> Gesendet: Dienstag, 10. Januar 2017 16:23 > >> > >> On 10.1.2017 15:46, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >>>> -----Ursprüngliche Nachricht----- > >>>> Von: Michal Simek [mailto:monstr@monstr.eu] > >>>> Gesendet: Dienstag, 10. Januar 2017 14:59 > >>>> An: Herbrechtsmeier, Stefan; u-boot@lists.denx.de > >>>> Cc: Herbrechtsmeier, Stefan; Albert Aribaud; Michal Simek; Joe > >>>> Hershberger > >>>> Betreff: Re: [PATCH 2/8] net: zynq: Add clk framework support to > >> zynq > >>>> ethernet driver > >>>> > >>>> On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: > >>>>> From: Stefan Herbrechtsmeier > >> <stefan.herbrechtsmeier@weidmueller.de> > >>>>> > >>>>> If available use the clock framework to set the tx clock rate of > >> the > >>>>> zynq ethernet controller. > >>>>> > >>>>> Signed-off-by: Stefan Herbrechtsmeier > >>>>> <stefan.herbrechtsmeier@weidmueller.com> > >>>>> --- > >>>>> > >>>>> arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- > >>>>> drivers/net/zynq_gem.c | 31 > >>>> ++++++++++++++++++++++++++++ > >>>>> 2 files changed, 31 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>>>> b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>>>> index 1db2bd6..bd633a6 100644 > >>>>> --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>>>> +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h > >>>>> @@ -8,11 +8,6 @@ > >>>>> #ifndef _ASM_ARCH_SYS_PROTO_H > >>>>> #define _ASM_ARCH_SYS_PROTO_H > >>>>> > >>>>> -/* Setup clk for network */ > >>>>> -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned > >>>>> long > >>>>> clk_rate) -{ -} > >>>>> - > >>>>> int zynq_slcr_get_mio_pin_status(const char *periph); > >>>>> > >>>>> unsigned int zynqmp_get_silicon_version(void); diff --git > >>>>> a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index > >>>>> 9bfb89f..9118e49 100644 > >>>>> --- a/drivers/net/zynq_gem.c > >>>>> +++ b/drivers/net/zynq_gem.c > >>>>> @@ -9,6 +9,7 @@ > >>>>> * SPDX-License-Identifier: GPL-2.0+ > >>>>> */ > >>>>> > >>>>> +#include <clk.h> > >>>>> #include <common.h> > >>>>> #include <dm.h> > >>>>> #include <net.h> > >>>>> @@ -180,6 +181,9 @@ struct zynq_gem_priv { > >>>>> struct phy_device *phydev; > >>>>> int phy_of_handle; > >>>>> struct mii_dev *bus; > >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>>>> + struct clk tx_clk; > >>>>> +#endif > >>>>> }; > >>>>> > >>>>> static inline int mdio_wait(struct zynq_gem_regs *regs) @@ - > 364,6 > >>>>> +368,9 @@ static int zynq_gem_init(struct udevice *dev) > >>>>> u32 i, nwconfig; > >>>>> int ret; > >>>>> unsigned long clk_rate = 0; > >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>>>> + unsigned long clk_rate_rounded; > >>>>> +#endif > >>>>> struct zynq_gem_priv *priv = dev_get_priv(dev); > >>>>> struct zynq_gem_regs *regs = priv->iobase; > >>>>> struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; > @@ - > >>>> 466,8 > >>>>> +473,22 @@ static int zynq_gem_init(struct udevice *dev) > >>>>> break; > >>>>> } > >>>>> > >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>>>> + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); > >>>>> + if (IS_ERR_VALUE(clk_rate_rounded)) { > >>>>> + dev_err(dev, "failed to set tx clock rate\n"); > >>>>> + return clk_rate_rounded; > >>>>> + } > >>>>> + > >>>>> + ret = clk_enable(&priv->tx_clk); > >>>>> + if (ret && ret != -ENOSYS) { > >>>>> + dev_err(dev, "failed to enable tx clock\n"); > >>>>> + return ret; > >>>>> + } > >>>>> +#else > >>>>> zynq_slcr_gem_clk_setup((ulong)priv->iobase != > >>>>> ZYNQ_GEM_BASEADDR0, clk_rate); > >>>>> +#endif > >>>>> > >>>>> setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | > >>>>> ZYNQ_GEM_NWCTRL_TXEN_MASK); > >>>>> @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct > >>>> udevice *dev) > >>>>> struct eth_pdata *pdata = dev_get_platdata(dev); > >>>>> struct zynq_gem_priv *priv = dev_get_priv(dev); > >>>>> const char *phy_mode; > >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>>>> + int ret; > >>>>> +#endif > >>>>> > >>>>> pdata->iobase = (phys_addr_t)dev_get_addr(dev); > >>>>> priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ - > 699,6 > >>>>> +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice > >>>> *dev) > >>>>> } > >>>>> priv->interface = pdata->phy_interface; > >>>>> > >>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) > >>>>> + ret = clk_get_by_index(dev, 2, &priv->tx_clk); > >>>>> + if (ret < 0) { > >>>>> + dev_err(dev, "failed to get clock\n"); > >>>>> + return ret; > >>>>> + } > >>>>> +#endif > >>>>> > >>>>> printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", > (ulong)priv- > >>>>> iobase, > >>>>> priv->phyaddr, > >>>>> phy_string_for_interface(priv->interface)); > >>>>> > >>>> > >>>> Can you please rebase it on the top of this? > >>>> https://patchwork.ozlabs.org/patch/706419/ > >>>> > >>>> The reason why this patch was written in this way is that we don't > >>>> have full zynqmp clk driver yet but we are using fixed clock where > >>>> you can't run set_rate which fails. > >>>> > >>> The patch assumes that the EMIO_ENET_TX_CLK is always fixed. Is > this > >> true? > >>> > >>> Why don't you add a set_rate() function to the fixed clock? > >> > >> It would be a solution but I am not sure if fixed clock was designed > >> for this. I would expect that fixed clock only provided clock which > >> you can't change. > > > > I think the current clock framework is not optimal as it doesn't > abstract the clock framework from the drivers. I don't know why a not > clock driver want to know that an internal clock ops isn't implemented. > > > > I would expect that the driver should not care what clock provides > > the rate. The clk_set_rate() function should always return the nearest > > possible clock rate and the clk_enable() shouldn't failed with a > > missing ops function. > > Otherwise this functions should be mandatory. > > I can't see an issue if clk_set_rate simply failed for clocks which > can't change. It is up to driver to handle this if this is fine or not. > In ZynqMP some clocks can be locked because they are used for something > more important than non secure sw. In this case the clock should return the current rate and the not clock driver could decide if it could work with this clock rate or not. > > > >> It means we need to have something as transition when u- boot has > >> real clk driver for zynqmp. > > > > We could use the same fall back for the clk_set_rate() as we use for > > the clk_enable() and ignore the -ENOSYS error. > > Not a problem with using -ENOSYS which is just a signal that clk driver > doesn't care about it. Okay. Will you update your patch? > >>> > >>> Additionally the patch is missing an enable function call. > >> > >> correct. > >> > >>> > >>> I don't think the emio configuration should be keep in the Ethernet > >> driver as it is part of the clock configuration and doesn't influent > >> the network interface. > >> > >> Please correct me if I am wrong. Are you saying that with these > >> patches especially 8/8 clock for emio will handled automatically? > > > > What do you mean by 8/8 clock? > > I meant patch 8/8 in this series. Okay. After patch 8/8 the emio clock should be handled automatically like in Linux kernel. > > > > The Ethernet driver should not care how the clock is connected and > only set the required clock rate. The clock driver should route the > function calls to the correct clock. > > ok. I will try to get testcases for emio to make sure that they are > still working correctly. Thanks. Regards, Stefan Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 - Komplementärin: Weidmüller Interface Führungsgesellschaft mbH - Sitz: Detmold - Amtsgericht Lemgo HRB 3924; Geschäftsführer: José Carlos Álvarez Tobar, Elke Eckstein, Dr. Peter Köhler, Jörg Timmermann; USt-ID-Nr. DE124599660
On 11.1.2017 10:55, Stefan.Herbrechtsmeier@weidmueller.com wrote: >> -----Ursprüngliche Nachricht----- >> Von: Michal Simek [mailto:michal.simek@xilinx.com] >> Gesendet: Mittwoch, 11. Januar 2017 09:22 >> >> On 11.1.2017 08:30, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>>> -----Ursprüngliche Nachricht----- >>>> Von: Michal Simek [mailto:monstr@monstr.eu] >>>> Gesendet: Dienstag, 10. Januar 2017 16:23 >>>> >>>> On 10.1.2017 15:46, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>>>>> -----Ursprüngliche Nachricht----- >>>>>> Von: Michal Simek [mailto:monstr@monstr.eu] >>>>>> Gesendet: Dienstag, 10. Januar 2017 14:59 >>>>>> An: Herbrechtsmeier, Stefan; u-boot@lists.denx.de >>>>>> Cc: Herbrechtsmeier, Stefan; Albert Aribaud; Michal Simek; Joe >>>>>> Hershberger >>>>>> Betreff: Re: [PATCH 2/8] net: zynq: Add clk framework support to >>>> zynq >>>>>> ethernet driver >>>>>> >>>>>> On 4.1.2017 13:27, stefan.herbrechtsmeier@weidmueller.com wrote: >>>>>>> From: Stefan Herbrechtsmeier >>>> <stefan.herbrechtsmeier@weidmueller.de> >>>>>>> >>>>>>> If available use the clock framework to set the tx clock rate of >>>> the >>>>>>> zynq ethernet controller. >>>>>>> >>>>>>> Signed-off-by: Stefan Herbrechtsmeier >>>>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>>>> --- >>>>>>> >>>>>>> arch/arm/include/asm/arch-zynqmp/sys_proto.h | 5 ----- >>>>>>> drivers/net/zynq_gem.c | 31 >>>>>> ++++++++++++++++++++++++++++ >>>>>>> 2 files changed, 31 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>>>> b/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>>>> index 1db2bd6..bd633a6 100644 >>>>>>> --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>>>> +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h >>>>>>> @@ -8,11 +8,6 @@ >>>>>>> #ifndef _ASM_ARCH_SYS_PROTO_H >>>>>>> #define _ASM_ARCH_SYS_PROTO_H >>>>>>> >>>>>>> -/* Setup clk for network */ >>>>>>> -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned >>>>>>> long >>>>>>> clk_rate) -{ -} >>>>>>> - >>>>>>> int zynq_slcr_get_mio_pin_status(const char *periph); >>>>>>> >>>>>>> unsigned int zynqmp_get_silicon_version(void); diff --git >>>>>>> a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index >>>>>>> 9bfb89f..9118e49 100644 >>>>>>> --- a/drivers/net/zynq_gem.c >>>>>>> +++ b/drivers/net/zynq_gem.c >>>>>>> @@ -9,6 +9,7 @@ >>>>>>> * SPDX-License-Identifier: GPL-2.0+ >>>>>>> */ >>>>>>> >>>>>>> +#include <clk.h> >>>>>>> #include <common.h> >>>>>>> #include <dm.h> >>>>>>> #include <net.h> >>>>>>> @@ -180,6 +181,9 @@ struct zynq_gem_priv { >>>>>>> struct phy_device *phydev; >>>>>>> int phy_of_handle; >>>>>>> struct mii_dev *bus; >>>>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>>>> + struct clk tx_clk; >>>>>>> +#endif >>>>>>> }; >>>>>>> >>>>>>> static inline int mdio_wait(struct zynq_gem_regs *regs) @@ - >> 364,6 >>>>>>> +368,9 @@ static int zynq_gem_init(struct udevice *dev) >>>>>>> u32 i, nwconfig; >>>>>>> int ret; >>>>>>> unsigned long clk_rate = 0; >>>>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>>>> + unsigned long clk_rate_rounded; >>>>>>> +#endif >>>>>>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>>>>>> struct zynq_gem_regs *regs = priv->iobase; >>>>>>> struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; >> @@ - >>>>>> 466,8 >>>>>>> +473,22 @@ static int zynq_gem_init(struct udevice *dev) >>>>>>> break; >>>>>>> } >>>>>>> >>>>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>>>> + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); >>>>>>> + if (IS_ERR_VALUE(clk_rate_rounded)) { >>>>>>> + dev_err(dev, "failed to set tx clock rate\n"); >>>>>>> + return clk_rate_rounded; >>>>>>> + } >>>>>>> + >>>>>>> + ret = clk_enable(&priv->tx_clk); >>>>>>> + if (ret && ret != -ENOSYS) { >>>>>>> + dev_err(dev, "failed to enable tx clock\n"); >>>>>>> + return ret; >>>>>>> + } >>>>>>> +#else >>>>>>> zynq_slcr_gem_clk_setup((ulong)priv->iobase != >>>>>>> ZYNQ_GEM_BASEADDR0, clk_rate); >>>>>>> +#endif >>>>>>> >>>>>>> setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | >>>>>>> ZYNQ_GEM_NWCTRL_TXEN_MASK); >>>>>>> @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct >>>>>> udevice *dev) >>>>>>> struct eth_pdata *pdata = dev_get_platdata(dev); >>>>>>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>>>>>> const char *phy_mode; >>>>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>>>> + int ret; >>>>>>> +#endif >>>>>>> >>>>>>> pdata->iobase = (phys_addr_t)dev_get_addr(dev); >>>>>>> priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ - >> 699,6 >>>>>>> +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice >>>>>> *dev) >>>>>>> } >>>>>>> priv->interface = pdata->phy_interface; >>>>>>> >>>>>>> +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) >>>>>>> + ret = clk_get_by_index(dev, 2, &priv->tx_clk); >>>>>>> + if (ret < 0) { >>>>>>> + dev_err(dev, "failed to get clock\n"); >>>>>>> + return ret; >>>>>>> + } >>>>>>> +#endif >>>>>>> >>>>>>> printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", >> (ulong)priv- >>>>>>> iobase, >>>>>>> priv->phyaddr, >>>>>>> phy_string_for_interface(priv->interface)); >>>>>>> >>>>>> >>>>>> Can you please rebase it on the top of this? >>>>>> https://patchwork.ozlabs.org/patch/706419/ >>>>>> >>>>>> The reason why this patch was written in this way is that we don't >>>>>> have full zynqmp clk driver yet but we are using fixed clock where >>>>>> you can't run set_rate which fails. >>>>>> >>>>> The patch assumes that the EMIO_ENET_TX_CLK is always fixed. Is >> this >>>> true? >>>>> >>>>> Why don't you add a set_rate() function to the fixed clock? >>>> >>>> It would be a solution but I am not sure if fixed clock was designed >>>> for this. I would expect that fixed clock only provided clock which >>>> you can't change. >>> >>> I think the current clock framework is not optimal as it doesn't >> abstract the clock framework from the drivers. I don't know why a not >> clock driver want to know that an internal clock ops isn't implemented. >>> >>> I would expect that the driver should not care what clock provides >>> the rate. The clk_set_rate() function should always return the nearest >>> possible clock rate and the clk_enable() shouldn't failed with a >>> missing ops function. >>> Otherwise this functions should be mandatory. >> >> I can't see an issue if clk_set_rate simply failed for clocks which >> can't change. It is up to driver to handle this if this is fine or not. >> In ZynqMP some clocks can be locked because they are used for something >> more important than non secure sw. > > In this case the clock should return the current rate and the not clock driver could decide if it could work with this clock rate or not. I would discuss this with clk guys. I was checking existing code and a lot of drivers are only checking error value not rate which is return. But based on clk.h driver should check if clk rate is sufficient for that particular case. >>> >>>> It means we need to have something as transition when u- boot has >>>> real clk driver for zynqmp. >>> >>> We could use the same fall back for the clk_set_rate() as we use for >>> the clk_enable() and ignore the -ENOSYS error. >> >> Not a problem with using -ENOSYS which is just a signal that clk driver >> doesn't care about it. > > Okay. Will you update your patch? I have sent PR to Tom some hours ago. It is not big deal to patch it. If you can do it as the part of your rebase that will be great. If you want me to do I can send another patch just for this one. >>>>> >>>>> Additionally the patch is missing an enable function call. >>>> >>>> correct. >>>> >>>>> >>>>> I don't think the emio configuration should be keep in the Ethernet >>>> driver as it is part of the clock configuration and doesn't influent >>>> the network interface. >>>> >>>> Please correct me if I am wrong. Are you saying that with these >>>> patches especially 8/8 clock for emio will handled automatically? >>> >>> What do you mean by 8/8 clock? >> >> I meant patch 8/8 in this series. > > Okay. After patch 8/8 the emio clock should be handled automatically like in Linux kernel. good. Thanks, Michal
diff --git a/arch/arm/include/asm/arch-zynqmp/sys_proto.h b/arch/arm/include/asm/arch-zynqmp/sys_proto.h index 1db2bd6..bd633a6 100644 --- a/arch/arm/include/asm/arch-zynqmp/sys_proto.h +++ b/arch/arm/include/asm/arch-zynqmp/sys_proto.h @@ -8,11 +8,6 @@ #ifndef _ASM_ARCH_SYS_PROTO_H #define _ASM_ARCH_SYS_PROTO_H -/* Setup clk for network */ -static inline void zynq_slcr_gem_clk_setup(u32 gem_id, unsigned long clk_rate) -{ -} - int zynq_slcr_get_mio_pin_status(const char *periph); unsigned int zynqmp_get_silicon_version(void); diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index 9bfb89f..9118e49 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -9,6 +9,7 @@ * SPDX-License-Identifier: GPL-2.0+ */ +#include <clk.h> #include <common.h> #include <dm.h> #include <net.h> @@ -180,6 +181,9 @@ struct zynq_gem_priv { struct phy_device *phydev; int phy_of_handle; struct mii_dev *bus; +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) + struct clk tx_clk; +#endif }; static inline int mdio_wait(struct zynq_gem_regs *regs) @@ -364,6 +368,9 @@ static int zynq_gem_init(struct udevice *dev) u32 i, nwconfig; int ret; unsigned long clk_rate = 0; +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) + unsigned long clk_rate_rounded; +#endif struct zynq_gem_priv *priv = dev_get_priv(dev); struct zynq_gem_regs *regs = priv->iobase; struct emac_bd *dummy_tx_bd = &priv->tx_bd[TX_FREE_DESC]; @@ -466,8 +473,22 @@ static int zynq_gem_init(struct udevice *dev) break; } +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) + clk_rate_rounded = clk_set_rate(&priv->tx_clk, clk_rate); + if (IS_ERR_VALUE(clk_rate_rounded)) { + dev_err(dev, "failed to set tx clock rate\n"); + return clk_rate_rounded; + } + + ret = clk_enable(&priv->tx_clk); + if (ret && ret != -ENOSYS) { + dev_err(dev, "failed to enable tx clock\n"); + return ret; + } +#else zynq_slcr_gem_clk_setup((ulong)priv->iobase != ZYNQ_GEM_BASEADDR0, clk_rate); +#endif setbits_le32(®s->nwctrl, ZYNQ_GEM_NWCTRL_RXEN_MASK | ZYNQ_GEM_NWCTRL_TXEN_MASK); @@ -678,6 +699,9 @@ static int zynq_gem_ofdata_to_platdata(struct udevice *dev) struct eth_pdata *pdata = dev_get_platdata(dev); struct zynq_gem_priv *priv = dev_get_priv(dev); const char *phy_mode; +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) + int ret; +#endif pdata->iobase = (phys_addr_t)dev_get_addr(dev); priv->iobase = (struct zynq_gem_regs *)pdata->iobase; @@ -699,6 +723,13 @@ static int zynq_gem_ofdata_to_platdata(struct udevice *dev) } priv->interface = pdata->phy_interface; +#if defined(CONFIG_CLK) || defined(CONFIG_SPL_CLK) + ret = clk_get_by_index(dev, 2, &priv->tx_clk); + if (ret < 0) { + dev_err(dev, "failed to get clock\n"); + return ret; + } +#endif printf("ZYNQ GEM: %lx, phyaddr %x, interface %s\n", (ulong)priv->iobase, priv->phyaddr, phy_string_for_interface(priv->interface));