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