| Message ID | 20190620064753.32391-7-anup.patel@wdc.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | Update SiFive Unleashed Drivers | expand |
On Thu, Jun 20, 2019 at 9:49 AM Anup Patel <Anup.Patel@wdc.com> wrote: > The SiFive MACB ethernet has a custom TX_CLK_SEL register to select > different TX clock for 1000mbps vs 10/100mbps. > > This patch adds SiFive MACB compatible string and extends the MACB > ethernet driver to change TX clock using TX_CLK_SEL register for > SiFive MACB. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > drivers/net/macb.c | 53 +++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 45 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > index c072f99d8f..6a29ee3064 100644 > --- a/drivers/net/macb.c > +++ b/drivers/net/macb.c > @@ -83,6 +83,9 @@ struct macb_dma_desc { > > struct macb_device { > void *regs; > + void *regs_sifive_gemgxl; This needs needs to be part of the specific config structure. > + > + bool skip_dma_config; Why do you ever need to skip_dma_config ? > unsigned int dma_burst_length; > > unsigned int rx_tail; > @@ -122,7 +125,9 @@ struct macb_device { > }; > > struct macb_config { > + bool skip_dma_config; > unsigned int dma_burst_length; > + bool has_sifive_gemgxl; > }; > > #ifndef CONFIG_DM_ETH > @@ -486,18 +491,11 @@ static int macb_phy_find(struct macb_device *macb, const char *name) > int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed) > { > #ifdef CONFIG_CLK > + struct macb_device *macb = dev_get_priv(dev); > struct clk tx_clk; > ulong rate; > int ret; > > - /* > - * "tx_clk" is an optional clock source for MACB. > - * Ignore if it does not exist in DT. > - */ > - ret = clk_get_by_name(dev, "tx_clk", &tx_clk); > - if (ret) > - return 0; > - > switch (speed) { > case _10BASET: > rate = 2500000; /* 2.5 MHz */ > @@ -513,6 +511,26 @@ int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed) > return 0; > } > > + if (macb->regs_sifive_gemgxl) { > + /* > + * SiFive GEMGXL TX clock operation mode: > + * > + * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic > + * and output clock on GMII output signal GTX_CLK > + * 1 = MII mode. Use MII input signal TX_CLK in TX logic > + */ > + writel(rate != 125000000, macb->regs_sifive_gemgxl); > + return 0; > + } move to dedicated clock init. > + > + /* > + * "tx_clk" is an optional clock source for MACB. > + * Ignore if it does not exist in DT. > + */ > + ret = clk_get_by_name(dev, "tx_clk", &tx_clk); > + if (ret) > + return 0; > + > if (tx_clk.dev) { > ret = clk_set_rate(&tx_clk, rate); > if (ret) > @@ -701,6 +719,9 @@ static void gmac_configure_dma(struct macb_device *macb) > u32 buffer_size; > u32 dmacfg; > > + if (macb->skip_dma_config) > + return; > + Same as before, why do you skip it ? > buffer_size = 128 / RX_BUFFER_MULTIPLE; > dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L); > dmacfg |= GEM_BF(RXBS, buffer_size); > @@ -1178,6 +1199,7 @@ static int macb_eth_probe(struct udevice *dev) > struct macb_device *macb = dev_get_priv(dev); > const char *phy_mode; > __maybe_unused int ret; > + fdt_addr_t addr; > > phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy-mode", > NULL); > @@ -1194,6 +1216,7 @@ static int macb_eth_probe(struct udevice *dev) > if (!macb_config) > macb_config = &default_gem_config; > > + macb->skip_dma_config = macb_config->skip_dma_config; > macb->dma_burst_length = macb_config->dma_burst_length; > #ifdef CONFIG_CLK > ret = macb_enable_clk(dev); > @@ -1201,6 +1224,13 @@ static int macb_eth_probe(struct udevice *dev) > return ret; > #endif > > + if (macb_config->has_sifive_gemgxl) { > + addr = dev_read_addr_index(dev, 1); > + if (addr == FDT_ADDR_T_NONE) > + return -ENODEV; > + macb->regs_sifive_gemgxl = (void __iomem *)addr; > + } This should be all done in a a specific sifive init function. You can define init function and clock init function CB functions in config (Like in Linux): " int (*clk_init)(struct platform_device *pdev, struct clk **pclk, struct clk **hclk, struct clk **tx_clk, struct clk **rx_clk); int (*init)(struct platform_device *pdev); " and add your specific SOC initialization there. This function should be generic as possible. > + > _macb_eth_initialize(macb); > > #if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB) > @@ -1259,6 +1289,12 @@ static const struct macb_config sama5d4_config = { > .dma_burst_length = 4, > }; > > +static const struct macb_config sifive_config = { > + .skip_dma_config = true, > + .dma_burst_length = 0, Don't mention it if it's zero, and by the way, I assume it should be not zero. You're wasting memory cycles, try to find the correct value. (I think 4 is the default in macb) > + .has_sifive_gemgxl = true, can be dropped if callback functions are declared. > +}; > + > static const struct udevice_id macb_eth_ids[] = { > { .compatible = "cdns,macb" }, > { .compatible = "cdns,at91sam9260-macb" }, > @@ -1266,6 +1302,7 @@ static const struct udevice_id macb_eth_ids[] = { > { .compatible = "atmel,sama5d3-gem" }, > { .compatible = "atmel,sama5d4-gem", .data = (ulong)&sama5d4_config }, > { .compatible = "cdns,zynq-gem" }, > + { .compatible = "sifive,fu540-macb", .data = (ulong)&sifive_config }, > { } > }; > > -- > 2.17.1 > Thanks, Ramon.
> -----Original Message----- > From: Ramon Fried <rfried.dev@gmail.com> > Sent: Thursday, June 20, 2019 2:48 PM > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Rick Chen <rick@andestech.com>; Bin Meng <bmeng.cn@gmail.com>; > Lukas Auer <lukas.auer@aisec.fraunhofer.de>; Simon Glass > <sjg@chromium.org>; Joe Hershberger <joe.hershberger@ni.com>; Palmer > Dabbelt <palmer@sifive.com>; Paul Walmsley <paul.walmsley@sifive.com>; > Troy Benjegerdes <troy.benjegerdes@sifive.com>; Atish Patra > <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; U-Boot > Mailing List <u-boot@lists.denx.de> > Subject: Re: [PATCH v5 6/8] net: macb: Extend MACB driver for SiFive > Unleashed board > > > On Thu, Jun 20, 2019 at 9:49 AM Anup Patel <Anup.Patel@wdc.com> wrote: > > The SiFive MACB ethernet has a custom TX_CLK_SEL register to select > > different TX clock for 1000mbps vs 10/100mbps. > > > > This patch adds SiFive MACB compatible string and extends the MACB > > ethernet driver to change TX clock using TX_CLK_SEL register for > > SiFive MACB. > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > --- > > drivers/net/macb.c | 53 > > +++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 45 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c index > > c072f99d8f..6a29ee3064 100644 > > --- a/drivers/net/macb.c > > +++ b/drivers/net/macb.c > > @@ -83,6 +83,9 @@ struct macb_dma_desc { > > > > struct macb_device { > > void *regs; > > + void *regs_sifive_gemgxl; > This needs needs to be part of the specific config structure. Sure, these are SOC specific so let me place it in config structure. > > + > > + bool skip_dma_config; > Why do you ever need to skip_dma_config ? I tried all possible dma_burst_length from 0 to 16 but none of them worked. At the moment, with gmac_configure_dma() in-place this driver has stopped working on SiFive Unleashed board. Instead of skip_dma_config, I think dma_burst_length ==0 should be treated as skip gmac_configure_dma(). > > unsigned int dma_burst_length; I was confused here. You have dma_burst_length here and in config structure as well. Why ? > > > > unsigned int rx_tail; > > @@ -122,7 +125,9 @@ struct macb_device { }; > > > > struct macb_config { > > + bool skip_dma_config; > > unsigned int dma_burst_length; > > + bool has_sifive_gemgxl; > > }; > > > > #ifndef CONFIG_DM_ETH > > @@ -486,18 +491,11 @@ static int macb_phy_find(struct macb_device > > *macb, const char *name) int __weak macb_linkspd_cb(struct udevice > > *dev, unsigned int speed) { #ifdef CONFIG_CLK > > + struct macb_device *macb = dev_get_priv(dev); > > struct clk tx_clk; > > ulong rate; > > int ret; > > > > - /* > > - * "tx_clk" is an optional clock source for MACB. > > - * Ignore if it does not exist in DT. > > - */ > > - ret = clk_get_by_name(dev, "tx_clk", &tx_clk); > > - if (ret) > > - return 0; > > - > > switch (speed) { > > case _10BASET: > > rate = 2500000; /* 2.5 MHz */ > > @@ -513,6 +511,26 @@ int __weak macb_linkspd_cb(struct udevice *dev, > unsigned int speed) > > return 0; > > } > > > > + if (macb->regs_sifive_gemgxl) { > > + /* > > + * SiFive GEMGXL TX clock operation mode: > > + * > > + * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic > > + * and output clock on GMII output signal GTX_CLK > > + * 1 = MII mode. Use MII input signal TX_CLK in TX logic > > + */ > > + writel(rate != 125000000, macb->regs_sifive_gemgxl); > > + return 0; > > + } > move to dedicated clock init. sure, I will add clock_init() callback in macb_config > > + > > + /* > > + * "tx_clk" is an optional clock source for MACB. > > + * Ignore if it does not exist in DT. > > + */ > > + ret = clk_get_by_name(dev, "tx_clk", &tx_clk); > > + if (ret) > > + return 0; > > + > > if (tx_clk.dev) { > > ret = clk_set_rate(&tx_clk, rate); > > if (ret) > > @@ -701,6 +719,9 @@ static void gmac_configure_dma(struct > macb_device *macb) > > u32 buffer_size; > > u32 dmacfg; > > > > + if (macb->skip_dma_config) > > + return; > > + > Same as before, why do you skip it ? Same as above comment. > > buffer_size = 128 / RX_BUFFER_MULTIPLE; > > dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L); > > dmacfg |= GEM_BF(RXBS, buffer_size); @@ -1178,6 +1199,7 @@ > > static int macb_eth_probe(struct udevice *dev) > > struct macb_device *macb = dev_get_priv(dev); > > const char *phy_mode; > > __maybe_unused int ret; > > + fdt_addr_t addr; > > > > phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy- > mode", > > NULL); > > @@ -1194,6 +1216,7 @@ static int macb_eth_probe(struct udevice *dev) > > if (!macb_config) > > macb_config = &default_gem_config; > > > > + macb->skip_dma_config = macb_config->skip_dma_config; > > macb->dma_burst_length = macb_config->dma_burst_length; > > #ifdef CONFIG_CLK > > ret = macb_enable_clk(dev); > > @@ -1201,6 +1224,13 @@ static int macb_eth_probe(struct udevice *dev) > > return ret; > > #endif > > > > + if (macb_config->has_sifive_gemgxl) { > > + addr = dev_read_addr_index(dev, 1); > > + if (addr == FDT_ADDR_T_NONE) > > + return -ENODEV; > > + macb->regs_sifive_gemgxl = (void __iomem *)addr; > > + } > This should be all done in a a specific sifive init function. > You can define init function and clock init function CB functions in config (Like > in Linux): > " > int (*clk_init)(struct platform_device *pdev, struct clk **pclk, struct clk > **hclk, struct clk **tx_clk, struct clk **rx_clk); int (*init)(struct > platform_device *pdev); " > > and add your specific SOC initialization there. > This function should be generic as possible. Sure, I will add init() callback in config structure. > > > + > > _macb_eth_initialize(macb); > > > > #if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB) @@ -1259,6 > > +1289,12 @@ static const struct macb_config sama5d4_config = { > > .dma_burst_length = 4, > > }; > > > > +static const struct macb_config sifive_config = { > > + .skip_dma_config = true, > > + .dma_burst_length = 0, > Don't mention it if it's zero, and by the way, I assume it should be not zero. > You're wasting memory cycles, try to find the correct value. (I think > 4 is the default in macb) Well, SiFive FU540 is a SiFive SOC. I don't know how they have integrated MACB. May be DMA configuration for SiFive MACB is little different. In any case, it was working before so we should atleast get it back to working state by skipping DMA configuration when dma_burst_lenght == 0. > > + .has_sifive_gemgxl = true, > can be dropped if callback functions are declared. Sure. > > +}; > > + > > static const struct udevice_id macb_eth_ids[] = { > > { .compatible = "cdns,macb" }, > > { .compatible = "cdns,at91sam9260-macb" }, @@ -1266,6 +1302,7 > > @@ static const struct udevice_id macb_eth_ids[] = { > > { .compatible = "atmel,sama5d3-gem" }, > > { .compatible = "atmel,sama5d4-gem", .data = > (ulong)&sama5d4_config }, > > { .compatible = "cdns,zynq-gem" }, > > + { .compatible = "sifive,fu540-macb", .data = > > + (ulong)&sifive_config }, > > { } > > }; > > > > -- > > 2.17.1 > > > Thanks, > Ramon. Regards, Anup
On 6/20/19 12:55 PM, Anup Patel wrote: > >> -----Original Message----- >> From: Ramon Fried <rfried.dev@gmail.com> >> Sent: Thursday, June 20, 2019 2:48 PM >> To: Anup Patel <Anup.Patel@wdc.com> >> Cc: Rick Chen <rick@andestech.com>; Bin Meng <bmeng.cn@gmail.com>; >> Lukas Auer <lukas.auer@aisec.fraunhofer.de>; Simon Glass >> <sjg@chromium.org>; Joe Hershberger <joe.hershberger@ni.com>; Palmer >> Dabbelt <palmer@sifive.com>; Paul Walmsley <paul.walmsley@sifive.com>; >> Troy Benjegerdes <troy.benjegerdes@sifive.com>; Atish Patra >> <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; U-Boot >> Mailing List <u-boot@lists.denx.de> >> Subject: Re: [PATCH v5 6/8] net: macb: Extend MACB driver for SiFive >> Unleashed board >> >> >> On Thu, Jun 20, 2019 at 9:49 AM Anup Patel <Anup.Patel@wdc.com> wrote: >>> The SiFive MACB ethernet has a custom TX_CLK_SEL register to select >>> different TX clock for 1000mbps vs 10/100mbps. >>> >>> This patch adds SiFive MACB compatible string and extends the MACB >>> ethernet driver to change TX clock using TX_CLK_SEL register for >>> SiFive MACB. >>> >>> Signed-off-by: Anup Patel <anup.patel@wdc.com> >>> --- >>> drivers/net/macb.c | 53 >>> +++++++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 45 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c index >>> c072f99d8f..6a29ee3064 100644 >>> --- a/drivers/net/macb.c >>> +++ b/drivers/net/macb.c >>> @@ -83,6 +83,9 @@ struct macb_dma_desc { >>> >>> struct macb_device { >>> void *regs; >>> + void *regs_sifive_gemgxl; >> This needs needs to be part of the specific config structure. > Sure, these are SOC specific so let me place it in > config structure. > >>> + >>> + bool skip_dma_config; >> Why do you ever need to skip_dma_config ? > I tried all possible dma_burst_length from 0 to 16 but > none of them worked. > > At the moment, with gmac_configure_dma() in-place > this driver has stopped working on SiFive Unleashed board. > > Instead of skip_dma_config, I think dma_burst_length ==0 > should be treated as skip gmac_configure_dma(). This is interesting, can you please print the dmacfg register before and after gma_configure_dma() ? I tested this on two different boards, but maybe I missed something. > >>> unsigned int dma_burst_length; > I was confused here. You have dma_burst_length here > and in config structure as well. Why ? I only had one configuration property, so I just copied it, but now that you've added more configuration items, it's reasonable to put a pointer to the config structure and use the dma_burst_length directly from there. > >>> unsigned int rx_tail; >>> @@ -122,7 +125,9 @@ struct macb_device { }; >>> >>> struct macb_config { >>> + bool skip_dma_config; >>> unsigned int dma_burst_length; >>> + bool has_sifive_gemgxl; >>> }; >>> >>> #ifndef CONFIG_DM_ETH >>> @@ -486,18 +491,11 @@ static int macb_phy_find(struct macb_device >>> *macb, const char *name) int __weak macb_linkspd_cb(struct udevice >>> *dev, unsigned int speed) { #ifdef CONFIG_CLK >>> + struct macb_device *macb = dev_get_priv(dev); >>> struct clk tx_clk; >>> ulong rate; >>> int ret; >>> >>> - /* >>> - * "tx_clk" is an optional clock source for MACB. >>> - * Ignore if it does not exist in DT. >>> - */ >>> - ret = clk_get_by_name(dev, "tx_clk", &tx_clk); >>> - if (ret) >>> - return 0; >>> - >>> switch (speed) { >>> case _10BASET: >>> rate = 2500000; /* 2.5 MHz */ >>> @@ -513,6 +511,26 @@ int __weak macb_linkspd_cb(struct udevice *dev, >> unsigned int speed) >>> return 0; >>> } >>> >>> + if (macb->regs_sifive_gemgxl) { >>> + /* >>> + * SiFive GEMGXL TX clock operation mode: >>> + * >>> + * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic >>> + * and output clock on GMII output signal GTX_CLK >>> + * 1 = MII mode. Use MII input signal TX_CLK in TX logic >>> + */ >>> + writel(rate != 125000000, macb->regs_sifive_gemgxl); >>> + return 0; >>> + } >> move to dedicated clock init. > sure, I will add clock_init() callback in macb_config > >>> + >>> + /* >>> + * "tx_clk" is an optional clock source for MACB. >>> + * Ignore if it does not exist in DT. >>> + */ >>> + ret = clk_get_by_name(dev, "tx_clk", &tx_clk); >>> + if (ret) >>> + return 0; >>> + >>> if (tx_clk.dev) { >>> ret = clk_set_rate(&tx_clk, rate); >>> if (ret) >>> @@ -701,6 +719,9 @@ static void gmac_configure_dma(struct >> macb_device *macb) >>> u32 buffer_size; >>> u32 dmacfg; >>> >>> + if (macb->skip_dma_config) >>> + return; >>> + >> Same as before, why do you skip it ? > Same as above comment. > >>> buffer_size = 128 / RX_BUFFER_MULTIPLE; >>> dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L); >>> dmacfg |= GEM_BF(RXBS, buffer_size); @@ -1178,6 +1199,7 @@ >>> static int macb_eth_probe(struct udevice *dev) >>> struct macb_device *macb = dev_get_priv(dev); >>> const char *phy_mode; >>> __maybe_unused int ret; >>> + fdt_addr_t addr; >>> >>> phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy- >> mode", >>> NULL); >>> @@ -1194,6 +1216,7 @@ static int macb_eth_probe(struct udevice *dev) >>> if (!macb_config) >>> macb_config = &default_gem_config; >>> >>> + macb->skip_dma_config = macb_config->skip_dma_config; >>> macb->dma_burst_length = macb_config->dma_burst_length; >>> #ifdef CONFIG_CLK >>> ret = macb_enable_clk(dev); >>> @@ -1201,6 +1224,13 @@ static int macb_eth_probe(struct udevice *dev) >>> return ret; >>> #endif >>> >>> + if (macb_config->has_sifive_gemgxl) { >>> + addr = dev_read_addr_index(dev, 1); >>> + if (addr == FDT_ADDR_T_NONE) >>> + return -ENODEV; >>> + macb->regs_sifive_gemgxl = (void __iomem *)addr; >>> + } >> This should be all done in a a specific sifive init function. >> You can define init function and clock init function CB functions in config (Like >> in Linux): >> " >> int (*clk_init)(struct platform_device *pdev, struct clk **pclk, struct clk >> **hclk, struct clk **tx_clk, struct clk **rx_clk); int (*init)(struct >> platform_device *pdev); " >> >> and add your specific SOC initialization there. >> This function should be generic as possible. > Sure, I will add init() callback in config structure. > >>> + >>> _macb_eth_initialize(macb); >>> >>> #if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB) @@ -1259,6 >>> +1289,12 @@ static const struct macb_config sama5d4_config = { >>> .dma_burst_length = 4, >>> }; >>> >>> +static const struct macb_config sifive_config = { >>> + .skip_dma_config = true, >>> + .dma_burst_length = 0, >> Don't mention it if it's zero, and by the way, I assume it should be not zero. >> You're wasting memory cycles, try to find the correct value. (I think >> 4 is the default in macb) > Well, SiFive FU540 is a SiFive SOC. I don't know how they > have integrated MACB. May be DMA configuration for > SiFive MACB is little different. > > In any case, it was working before so we should atleast > get it back to working state by skipping DMA configuration > when dma_burst_lenght == 0. Like I written before, additionally, I'm writing a new patch that increases buffer size to 2K for GEM supported devices, this will go in dma configuration, it's pitty that you will miss this improvement. Let's try to fix gmac_configure_dma() for you. >>> + .has_sifive_gemgxl = true, >> can be dropped if callback functions are declared. > Sure. > >>> +}; >>> + >>> static const struct udevice_id macb_eth_ids[] = { >>> { .compatible = "cdns,macb" }, >>> { .compatible = "cdns,at91sam9260-macb" }, @@ -1266,6 +1302,7 >>> @@ static const struct udevice_id macb_eth_ids[] = { >>> { .compatible = "atmel,sama5d3-gem" }, >>> { .compatible = "atmel,sama5d4-gem", .data = >> (ulong)&sama5d4_config }, >>> { .compatible = "cdns,zynq-gem" }, >>> + { .compatible = "sifive,fu540-macb", .data = >>> + (ulong)&sifive_config }, >>> { } >>> }; >>> >>> -- >>> 2.17.1 >>> >> Thanks, >> Ramon. > Regards, > Anup
On Thu, Jun 20, 2019 at 4:01 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > On 6/20/19 12:55 PM, Anup Patel wrote: > > > >> -----Original Message----- > >> From: Ramon Fried <rfried.dev@gmail.com> > >> Sent: Thursday, June 20, 2019 2:48 PM > >> To: Anup Patel <Anup.Patel@wdc.com> > >> Cc: Rick Chen <rick@andestech.com>; Bin Meng <bmeng.cn@gmail.com>; > >> Lukas Auer <lukas.auer@aisec.fraunhofer.de>; Simon Glass > >> <sjg@chromium.org>; Joe Hershberger <joe.hershberger@ni.com>; Palmer > >> Dabbelt <palmer@sifive.com>; Paul Walmsley <paul.walmsley@sifive.com>; > >> Troy Benjegerdes <troy.benjegerdes@sifive.com>; Atish Patra > >> <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; U-Boot > >> Mailing List <u-boot@lists.denx.de> > >> Subject: Re: [PATCH v5 6/8] net: macb: Extend MACB driver for SiFive > >> Unleashed board > >> > >> > >> On Thu, Jun 20, 2019 at 9:49 AM Anup Patel <Anup.Patel@wdc.com> wrote: > >>> The SiFive MACB ethernet has a custom TX_CLK_SEL register to select > >>> different TX clock for 1000mbps vs 10/100mbps. > >>> > >>> This patch adds SiFive MACB compatible string and extends the MACB > >>> ethernet driver to change TX clock using TX_CLK_SEL register for > >>> SiFive MACB. > >>> > >>> Signed-off-by: Anup Patel <anup.patel@wdc.com> > >>> --- > >>> drivers/net/macb.c | 53 > >>> +++++++++++++++++++++++++++++++++++++++------- > >>> 1 file changed, 45 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/net/macb.c b/drivers/net/macb.c index > >>> c072f99d8f..6a29ee3064 100644 > >>> --- a/drivers/net/macb.c > >>> +++ b/drivers/net/macb.c > >>> @@ -83,6 +83,9 @@ struct macb_dma_desc { > >>> > >>> struct macb_device { > >>> void *regs; > >>> + void *regs_sifive_gemgxl; > >> This needs needs to be part of the specific config structure. > > Sure, these are SOC specific so let me place it in > > config structure. > > > >>> + > >>> + bool skip_dma_config; > >> Why do you ever need to skip_dma_config ? > > I tried all possible dma_burst_length from 0 to 16 but > > none of them worked. > > > > At the moment, with gmac_configure_dma() in-place > > this driver has stopped working on SiFive Unleashed board. > > > > Instead of skip_dma_config, I think dma_burst_length ==0 > > should be treated as skip gmac_configure_dma(). > > This is interesting, can you please print the dmacfg register before and after gma_configure_dma() ? > > I tested this on two different boards, but maybe I missed something. Here's the log with dma_burst_length = 16, ... U-Boot 2019.07-rc4-00014-gd0e7f88c1b-dirty (Jun 24 2019 - 09:00:01 +0530) CPU: rv64imafdc Model: SiFive HiFive Unleashed A00 DRAM: 8 GiB In: serial@10010000 Out: serial@10010000 Err: serial@10010000 Net: eth0: ethernet@10090000 Hit any key to stop autoboot: 0 => setenv ipaddr 10.206.7.133 => setenv netmask 255.255.252.0 => setenv serverip 10.206.4.143 => setenv gateway 10.206.4.1 => ping 10.206.4.143 gmac_configure_dma: before 0x20704 gmac_configure_dma: wrote 0x20750 gmac_configure_dma: after 0x20750 ethernet@10090000: PHY present at 0 ethernet@10090000: Starting autonegotiation... ethernet@10090000: Autonegotiation complete ethernet@10090000: link up, 1000Mbps full-duplex (lpa: 0x7c00) Using ethernet@10090000 device ethernet@10090000: TX timeout ethernet@10090000: TX timeout ethernet@10090000: TX timeout ethernet@10090000: TX timeout ARP Retry count exceeded; starting again I figured, BIG_ENDIAN bit is being set in DMACFG because of CONFIG_SYS_LITTLE_ENDIAN. I think the check should be on __LITTLE_ENDIAN which is a GCC pre-defined macro for little endian system. If I fix this check then dma_burst_length = 16 works fine. I will send a v7 series with a separate patch to fix this patch. You can quash that patch in your series or let be as a separate patch. Regards, Anup > > > > > >>> unsigned int dma_burst_length; > > I was confused here. You have dma_burst_length here > > and in config structure as well. Why ? > > I only had one configuration property, so I just copied it, but now that you've added more configuration items, it's reasonable to put a pointer to the config structure and use the dma_burst_length directly from there. > > > > > >>> unsigned int rx_tail; > >>> @@ -122,7 +125,9 @@ struct macb_device { }; > >>> > >>> struct macb_config { > >>> + bool skip_dma_config; > >>> unsigned int dma_burst_length; > >>> + bool has_sifive_gemgxl; > >>> }; > >>> > >>> #ifndef CONFIG_DM_ETH > >>> @@ -486,18 +491,11 @@ static int macb_phy_find(struct macb_device > >>> *macb, const char *name) int __weak macb_linkspd_cb(struct udevice > >>> *dev, unsigned int speed) { #ifdef CONFIG_CLK > >>> + struct macb_device *macb = dev_get_priv(dev); > >>> struct clk tx_clk; > >>> ulong rate; > >>> int ret; > >>> > >>> - /* > >>> - * "tx_clk" is an optional clock source for MACB. > >>> - * Ignore if it does not exist in DT. > >>> - */ > >>> - ret = clk_get_by_name(dev, "tx_clk", &tx_clk); > >>> - if (ret) > >>> - return 0; > >>> - > >>> switch (speed) { > >>> case _10BASET: > >>> rate = 2500000; /* 2.5 MHz */ > >>> @@ -513,6 +511,26 @@ int __weak macb_linkspd_cb(struct udevice *dev, > >> unsigned int speed) > >>> return 0; > >>> } > >>> > >>> + if (macb->regs_sifive_gemgxl) { > >>> + /* > >>> + * SiFive GEMGXL TX clock operation mode: > >>> + * > >>> + * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic > >>> + * and output clock on GMII output signal GTX_CLK > >>> + * 1 = MII mode. Use MII input signal TX_CLK in TX logic > >>> + */ > >>> + writel(rate != 125000000, macb->regs_sifive_gemgxl); > >>> + return 0; > >>> + } > >> move to dedicated clock init. > > sure, I will add clock_init() callback in macb_config > > > >>> + > >>> + /* > >>> + * "tx_clk" is an optional clock source for MACB. > >>> + * Ignore if it does not exist in DT. > >>> + */ > >>> + ret = clk_get_by_name(dev, "tx_clk", &tx_clk); > >>> + if (ret) > >>> + return 0; > >>> + > >>> if (tx_clk.dev) { > >>> ret = clk_set_rate(&tx_clk, rate); > >>> if (ret) > >>> @@ -701,6 +719,9 @@ static void gmac_configure_dma(struct > >> macb_device *macb) > >>> u32 buffer_size; > >>> u32 dmacfg; > >>> > >>> + if (macb->skip_dma_config) > >>> + return; > >>> + > >> Same as before, why do you skip it ? > > Same as above comment. > > > >>> buffer_size = 128 / RX_BUFFER_MULTIPLE; > >>> dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L); > >>> dmacfg |= GEM_BF(RXBS, buffer_size); @@ -1178,6 +1199,7 @@ > >>> static int macb_eth_probe(struct udevice *dev) > >>> struct macb_device *macb = dev_get_priv(dev); > >>> const char *phy_mode; > >>> __maybe_unused int ret; > >>> + fdt_addr_t addr; > >>> > >>> phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy- > >> mode", > >>> NULL); > >>> @@ -1194,6 +1216,7 @@ static int macb_eth_probe(struct udevice *dev) > >>> if (!macb_config) > >>> macb_config = &default_gem_config; > >>> > >>> + macb->skip_dma_config = macb_config->skip_dma_config; > >>> macb->dma_burst_length = macb_config->dma_burst_length; > >>> #ifdef CONFIG_CLK > >>> ret = macb_enable_clk(dev); > >>> @@ -1201,6 +1224,13 @@ static int macb_eth_probe(struct udevice *dev) > >>> return ret; > >>> #endif > >>> > >>> + if (macb_config->has_sifive_gemgxl) { > >>> + addr = dev_read_addr_index(dev, 1); > >>> + if (addr == FDT_ADDR_T_NONE) > >>> + return -ENODEV; > >>> + macb->regs_sifive_gemgxl = (void __iomem *)addr; > >>> + } > >> This should be all done in a a specific sifive init function. > >> You can define init function and clock init function CB functions in config (Like > >> in Linux): > >> " > >> int (*clk_init)(struct platform_device *pdev, struct clk **pclk, struct clk > >> **hclk, struct clk **tx_clk, struct clk **rx_clk); int (*init)(struct > >> platform_device *pdev); " > >> > >> and add your specific SOC initialization there. > >> This function should be generic as possible. > > Sure, I will add init() callback in config structure. > > > >>> + > >>> _macb_eth_initialize(macb); > >>> > >>> #if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB) @@ -1259,6 > >>> +1289,12 @@ static const struct macb_config sama5d4_config = { > >>> .dma_burst_length = 4, > >>> }; > >>> > >>> +static const struct macb_config sifive_config = { > >>> + .skip_dma_config = true, > >>> + .dma_burst_length = 0, > >> Don't mention it if it's zero, and by the way, I assume it should be not zero. > >> You're wasting memory cycles, try to find the correct value. (I think > >> 4 is the default in macb) > > Well, SiFive FU540 is a SiFive SOC. I don't know how they > > have integrated MACB. May be DMA configuration for > > SiFive MACB is little different. > > > > In any case, it was working before so we should atleast > > get it back to working state by skipping DMA configuration > > when dma_burst_lenght == 0. > > Like I written before, additionally, I'm writing a new patch that increases buffer size to 2K for > GEM supported devices, this will go in dma configuration, it's pitty that you will miss this improvement. > Let's try to fix gmac_configure_dma() for you. > > >>> + .has_sifive_gemgxl = true, > >> can be dropped if callback functions are declared. > > Sure. > > > >>> +}; > >>> + > >>> static const struct udevice_id macb_eth_ids[] = { > >>> { .compatible = "cdns,macb" }, > >>> { .compatible = "cdns,at91sam9260-macb" }, @@ -1266,6 +1302,7 > >>> @@ static const struct udevice_id macb_eth_ids[] = { > >>> { .compatible = "atmel,sama5d3-gem" }, > >>> { .compatible = "atmel,sama5d4-gem", .data = > >> (ulong)&sama5d4_config }, > >>> { .compatible = "cdns,zynq-gem" }, > >>> + { .compatible = "sifive,fu540-macb", .data = > >>> + (ulong)&sifive_config }, > >>> { } > >>> }; > >>> > >>> -- > >>> 2.17.1 > >>> > >> Thanks, > >> Ramon. > > Regards, > > Anup > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
diff --git a/drivers/net/macb.c b/drivers/net/macb.c index c072f99d8f..6a29ee3064 100644 --- a/drivers/net/macb.c +++ b/drivers/net/macb.c @@ -83,6 +83,9 @@ struct macb_dma_desc { struct macb_device { void *regs; + void *regs_sifive_gemgxl; + + bool skip_dma_config; unsigned int dma_burst_length; unsigned int rx_tail; @@ -122,7 +125,9 @@ struct macb_device { }; struct macb_config { + bool skip_dma_config; unsigned int dma_burst_length; + bool has_sifive_gemgxl; }; #ifndef CONFIG_DM_ETH @@ -486,18 +491,11 @@ static int macb_phy_find(struct macb_device *macb, const char *name) int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed) { #ifdef CONFIG_CLK + struct macb_device *macb = dev_get_priv(dev); struct clk tx_clk; ulong rate; int ret; - /* - * "tx_clk" is an optional clock source for MACB. - * Ignore if it does not exist in DT. - */ - ret = clk_get_by_name(dev, "tx_clk", &tx_clk); - if (ret) - return 0; - switch (speed) { case _10BASET: rate = 2500000; /* 2.5 MHz */ @@ -513,6 +511,26 @@ int __weak macb_linkspd_cb(struct udevice *dev, unsigned int speed) return 0; } + if (macb->regs_sifive_gemgxl) { + /* + * SiFive GEMGXL TX clock operation mode: + * + * 0 = GMII mode. Use 125 MHz gemgxlclk from PRCI in TX logic + * and output clock on GMII output signal GTX_CLK + * 1 = MII mode. Use MII input signal TX_CLK in TX logic + */ + writel(rate != 125000000, macb->regs_sifive_gemgxl); + return 0; + } + + /* + * "tx_clk" is an optional clock source for MACB. + * Ignore if it does not exist in DT. + */ + ret = clk_get_by_name(dev, "tx_clk", &tx_clk); + if (ret) + return 0; + if (tx_clk.dev) { ret = clk_set_rate(&tx_clk, rate); if (ret) @@ -701,6 +719,9 @@ static void gmac_configure_dma(struct macb_device *macb) u32 buffer_size; u32 dmacfg; + if (macb->skip_dma_config) + return; + buffer_size = 128 / RX_BUFFER_MULTIPLE; dmacfg = gem_readl(macb, DMACFG) & ~GEM_BF(RXBS, -1L); dmacfg |= GEM_BF(RXBS, buffer_size); @@ -1178,6 +1199,7 @@ static int macb_eth_probe(struct udevice *dev) struct macb_device *macb = dev_get_priv(dev); const char *phy_mode; __maybe_unused int ret; + fdt_addr_t addr; phy_mode = fdt_getprop(gd->fdt_blob, dev_of_offset(dev), "phy-mode", NULL); @@ -1194,6 +1216,7 @@ static int macb_eth_probe(struct udevice *dev) if (!macb_config) macb_config = &default_gem_config; + macb->skip_dma_config = macb_config->skip_dma_config; macb->dma_burst_length = macb_config->dma_burst_length; #ifdef CONFIG_CLK ret = macb_enable_clk(dev); @@ -1201,6 +1224,13 @@ static int macb_eth_probe(struct udevice *dev) return ret; #endif + if (macb_config->has_sifive_gemgxl) { + addr = dev_read_addr_index(dev, 1); + if (addr == FDT_ADDR_T_NONE) + return -ENODEV; + macb->regs_sifive_gemgxl = (void __iomem *)addr; + } + _macb_eth_initialize(macb); #if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB) @@ -1259,6 +1289,12 @@ static const struct macb_config sama5d4_config = { .dma_burst_length = 4, }; +static const struct macb_config sifive_config = { + .skip_dma_config = true, + .dma_burst_length = 0, + .has_sifive_gemgxl = true, +}; + static const struct udevice_id macb_eth_ids[] = { { .compatible = "cdns,macb" }, { .compatible = "cdns,at91sam9260-macb" }, @@ -1266,6 +1302,7 @@ static const struct udevice_id macb_eth_ids[] = { { .compatible = "atmel,sama5d3-gem" }, { .compatible = "atmel,sama5d4-gem", .data = (ulong)&sama5d4_config }, { .compatible = "cdns,zynq-gem" }, + { .compatible = "sifive,fu540-macb", .data = (ulong)&sifive_config }, { } };
The SiFive MACB ethernet has a custom TX_CLK_SEL register to select different TX clock for 1000mbps vs 10/100mbps. This patch adds SiFive MACB compatible string and extends the MACB ethernet driver to change TX clock using TX_CLK_SEL register for SiFive MACB. Signed-off-by: Anup Patel <anup.patel@wdc.com> --- drivers/net/macb.c | 53 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 8 deletions(-)