[U-Boot] [PATCH v5 6/8] net: macb: Extend MACB driver for SiFive Unleashed board
Anup Patel
anup at brainfault.org
Mon Jun 24 03:47:29 UTC 2019
On Thu, Jun 20, 2019 at 4:01 PM Ramon Fried <rfried.dev at gmail.com> wrote:
>
>
> On 6/20/19 12:55 PM, Anup Patel wrote:
> >
> >> -----Original Message-----
> >> From: Ramon Fried <rfried.dev at gmail.com>
> >> Sent: Thursday, June 20, 2019 2:48 PM
> >> To: Anup Patel <Anup.Patel at wdc.com>
> >> Cc: Rick Chen <rick at andestech.com>; Bin Meng <bmeng.cn at gmail.com>;
> >> Lukas Auer <lukas.auer at aisec.fraunhofer.de>; Simon Glass
> >> <sjg at chromium.org>; Joe Hershberger <joe.hershberger at ni.com>; Palmer
> >> Dabbelt <palmer at sifive.com>; Paul Walmsley <paul.walmsley at sifive.com>;
> >> Troy Benjegerdes <troy.benjegerdes at sifive.com>; Atish Patra
> >> <Atish.Patra at wdc.com>; Alistair Francis <Alistair.Francis at wdc.com>; U-Boot
> >> Mailing List <u-boot at 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 at 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 at 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 at 10010000
Out: serial at 10010000
Err: serial at 10010000
Net: eth0: ethernet at 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 at 10090000: PHY present at 0
ethernet at 10090000: Starting autonegotiation...
ethernet at 10090000: Autonegotiation complete
ethernet at 10090000: link up, 1000Mbps full-duplex (lpa: 0x7c00)
Using ethernet at 10090000 device
ethernet at 10090000: TX timeout
ethernet at 10090000: TX timeout
ethernet at 10090000: TX timeout
ethernet at 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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list