[PATCH v2 0/2] Add macb support for SiFive FU540-C000

[PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Yash Shah]

On FU540, the management IP block is tightly coupled with the Cadence
MACB IP block. It manages many of the boundary signals from the MACB IP
This patchset controls the tx_clk input signal to the MACB IP. It
switches between the local TX clock (125MHz) and PHY TX clocks. This
is necessary to toggle between 1Gb and 100/10Mb speeds.

Future patches may add support for monitoring or controlling other IP
boundary signals.

This patchset is mostly based on work done by
Wesley Terpstra <wesley@sifive.com>

This patchset is based on Linux v5.2-rc1 and tested on HiFive Unleashed
board with additional board related patches needed for testing can be
found at dev/yashs/ethernet branch of:
https://github.com/yashshah7/riscv-linux.git

Change History:
V2:
- Change compatible string from "cdns,fu540-macb" to "sifive,fu540-macb"
- Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
  driver. This is needed because on FU540, the macb driver depends on
  SiFive GPIO driver.
- Avoid writing the result of a comparison to a register.
- Fix the issue of probe fail on reloading the module reported by:
  Andreas Schwab <schwab@suse.de>

Yash Shah (2):
  macb: bindings doc: add sifive fu540-c000 binding
  macb: Add support for SiFive FU540-C000

 Documentation/devicetree/bindings/net/macb.txt |   3 +
 drivers/net/ethernet/cadence/Kconfig           |   6 ++
 drivers/net/ethernet/cadence/macb_main.c       | 129 +++++++++++++++++++++++++
 3 files changed, 138 insertions(+)

-- 
1.9.1

[PATCH v2 1/2] macb: bindings doc: add sifive fu540-c000 binding

[Yash Shah]

Add the compatibility string documentation for SiFive FU540-C0000
interface.
On the FU540, this driver also needs to read and write registers in a
management IP block that monitors or drives boundary signals for the
GEMGXL IP block that are not directly mapped to GEMGXL registers.
Therefore, add additional range to "reg" property for SiFive GEMGXL
management IP registers.

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 Documentation/devicetree/bindings/net/macb.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index 9c5e944..63c73fa 100644
--- a/Documentation/devicetree/bindings/net/macb.txt
+++ b/Documentation/devicetree/bindings/net/macb.txt
@@ -15,8 +15,11 @@ Required properties:
   Use "atmel,sama5d4-gem" for the GEM IP (10/100) available on Atmel sama5d4 SoCs.
   Use "cdns,zynq-gem" Xilinx Zynq-7xxx SoC.
   Use "cdns,zynqmp-gem" for Zynq Ultrascale+ MPSoC.
+  Use "sifive,fu540-macb" for SiFive FU540-C000 SoC.
   Or the generic form: "cdns,emac".
 - reg: Address and length of the register set for the device
+	For "sifive,fu540-macb", second range is required to specify the
+	address and length of the registers for GEMGXL Management block.
 - interrupts: Should contain macb interrupt
 - phy-mode: See ethernet.txt file in the same directory.
 - clock-names: Tuple listing input clock names.
-- 
1.9.1

Re: [PATCH v2 1/2] macb: bindings doc: add sifive fu540-c000 binding

[Paul Walmsley]

On Mon, 17 Jun 2019, Yash Shah wrote:

> Add the compatibility string documentation for SiFive FU540-C0000
> interface.
> On the FU540, this driver also needs to read and write registers in a
> management IP block that monitors or drives boundary signals for the
> GEMGXL IP block that are not directly mapped to GEMGXL registers.
> Therefore, add additional range to "reg" property for SiFive GEMGXL
> management IP registers.
> 
> Signed-off-by: Yash Shah <yash.shah@sifive.com>

Reviewed-by: Paul Walmsley <paul.walmsley@sifive.com>


- Paul

[PATCH v2 2/2] macb: Add support for SiFive FU540-C000

[Yash Shah]

The management IP block is tightly coupled with the Cadence MACB IP
block on the FU540, and manages many of the boundary signals from the
MACB IP. This patch only controls the tx_clk input signal to the MACB
IP. Future patches may add support for monitoring or controlling other
IP boundary signals.

Signed-off-by: Yash Shah <yash.shah@sifive.com>
---
 drivers/net/ethernet/cadence/Kconfig     |   6 ++
 drivers/net/ethernet/cadence/macb_main.c | 129 +++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+)

diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index b998401..d478fae 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -48,4 +48,10 @@ config MACB_PCI
 	  To compile this driver as a module, choose M here: the module
 	  will be called macb_pci.
 
+config MACB_SIFIVE_FU540
+	bool "Cadence MACB/GEM support for SiFive FU540 SoC"
+	depends on MACB && GPIO_SIFIVE
+	help
+	  Enable the Cadence MACB/GEM support for SiFive FU540 SoC.
+
 endif # NET_VENDOR_CADENCE
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c049410..275b5e8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -10,6 +10,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/crc32.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -40,6 +41,15 @@
 #include <linux/pm_runtime.h>
 #include "macb.h"
 
+/* This structure is only used for MACB on SiFive FU540 devices */
+struct sifive_fu540_macb_mgmt {
+	void __iomem *reg;
+	unsigned long rate;
+	struct clk_hw hw;
+};
+
+static struct sifive_fu540_macb_mgmt *mgmt;
+
 #define MACB_RX_BUFFER_SIZE	128
 #define RX_BUFFER_MULTIPLE	64  /* bytes */
 
@@ -3903,6 +3913,116 @@ static int at91ether_init(struct platform_device *pdev)
 	return 0;
 }
 
+static unsigned long fu540_macb_tx_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	return mgmt->rate;
+}
+
+static long fu540_macb_tx_round_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long *parent_rate)
+{
+	if (WARN_ON(rate < 2500000))
+		return 2500000;
+	else if (rate == 2500000)
+		return 2500000;
+	else if (WARN_ON(rate < 13750000))
+		return 2500000;
+	else if (WARN_ON(rate < 25000000))
+		return 25000000;
+	else if (rate == 25000000)
+		return 25000000;
+	else if (WARN_ON(rate < 75000000))
+		return 25000000;
+	else if (WARN_ON(rate < 125000000))
+		return 125000000;
+	else if (rate == 125000000)
+		return 125000000;
+
+	WARN_ON(rate > 125000000);
+
+	return 125000000;
+}
+
+static int fu540_macb_tx_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long parent_rate)
+{
+	rate = fu540_macb_tx_round_rate(hw, rate, &parent_rate);
+	if (rate != 125000000)
+		iowrite32(1, mgmt->reg);
+	else
+		iowrite32(0, mgmt->reg);
+	mgmt->rate = rate;
+
+	return 0;
+}
+
+static const struct clk_ops fu540_c000_ops = {
+	.recalc_rate = fu540_macb_tx_recalc_rate,
+	.round_rate = fu540_macb_tx_round_rate,
+	.set_rate = fu540_macb_tx_set_rate,
+};
+
+static int fu540_c000_clk_init(struct platform_device *pdev, struct clk **pclk,
+			       struct clk **hclk, struct clk **tx_clk,
+			       struct clk **rx_clk, struct clk **tsu_clk)
+{
+	struct clk_init_data init;
+	int err = 0;
+
+	err = macb_clk_init(pdev, pclk, hclk, tx_clk, rx_clk, tsu_clk);
+	if (err)
+		return err;
+
+	mgmt = devm_kzalloc(&pdev->dev, sizeof(*mgmt), GFP_KERNEL);
+	if (!mgmt)
+		return -ENOMEM;
+
+	init.name = "sifive-gemgxl-mgmt";
+	init.ops = &fu540_c000_ops;
+	init.flags = 0;
+	init.num_parents = 0;
+
+	mgmt->rate = 0;
+	mgmt->hw.init = &init;
+
+	*tx_clk = clk_register(NULL, &mgmt->hw);
+	if (IS_ERR(*tx_clk))
+		return PTR_ERR(*tx_clk);
+
+	err = clk_prepare_enable(*tx_clk);
+	if (err)
+		dev_err(&pdev->dev, "failed to enable tx_clk (%u)\n", err);
+	else
+		dev_info(&pdev->dev, "Registered clk switch '%s'\n", init.name);
+
+	return 0;
+}
+
+static int fu540_c000_init(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -ENODEV;
+
+	mgmt->reg = ioremap(res->start, resource_size(res));
+	if (!mgmt->reg)
+		return -ENOMEM;
+
+	return macb_init(pdev);
+}
+
+static const struct macb_config fu540_c000_config = {
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
+		MACB_CAPS_GEM_HAS_PTP,
+	.dma_burst_length = 16,
+	.clk_init = fu540_c000_clk_init,
+	.init = fu540_c000_init,
+	.jumbo_max_len = 10240,
+};
+
 static const struct macb_config at91sam9260_config = {
 	.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
 	.clk_init = macb_clk_init,
@@ -3992,6 +4112,9 @@ static int at91ether_init(struct platform_device *pdev)
 	{ .compatible = "cdns,emac", .data = &emac_config },
 	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
 	{ .compatible = "cdns,zynq-gem", .data = &zynq_config },
+#ifdef CONFIG_MACB_SIFIVE_FU540
+	{ .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
+#endif
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, macb_dt_ids);
@@ -4199,6 +4322,9 @@ static int macb_probe(struct platform_device *pdev)
 
 err_disable_clocks:
 	clk_disable_unprepare(tx_clk);
+#ifdef CONFIG_MACB_SIFIVE_FU540
+	clk_unregister(tx_clk);
+#endif
 	clk_disable_unprepare(hclk);
 	clk_disable_unprepare(pclk);
 	clk_disable_unprepare(rx_clk);
@@ -4233,6 +4359,9 @@ static int macb_remove(struct platform_device *pdev)
 		pm_runtime_dont_use_autosuspend(&pdev->dev);
 		if (!pm_runtime_suspended(&pdev->dev)) {
 			clk_disable_unprepare(bp->tx_clk);
+#ifdef CONFIG_MACB_SIFIVE_FU540
+			clk_unregister(bp->tx_clk);
+#endif
 			clk_disable_unprepare(bp->hclk);
 			clk_disable_unprepare(bp->pclk);
 			clk_disable_unprepare(bp->rx_clk);
-- 
1.9.1

Re: [PATCH v2 2/2] macb: Add support for SiFive FU540-C000

[Andrew Lunn]

On Mon, Jun 17, 2019 at 09:49:27AM +0530, Yash Shah wrote:
> The management IP block is tightly coupled with the Cadence MACB IP
> block on the FU540, and manages many of the boundary signals from the
> MACB IP. This patch only controls the tx_clk input signal to the MACB
> IP. Future patches may add support for monitoring or controlling other
> IP boundary signals.
> 
> Signed-off-by: Yash Shah <yash.shah@sifive.com>
> ---
>  drivers/net/ethernet/cadence/Kconfig     |   6 ++
>  drivers/net/ethernet/cadence/macb_main.c | 129 +++++++++++++++++++++++++++++++
>  2 files changed, 135 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
> index b998401..d478fae 100644
> --- a/drivers/net/ethernet/cadence/Kconfig
> +++ b/drivers/net/ethernet/cadence/Kconfig
> @@ -48,4 +48,10 @@ config MACB_PCI
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called macb_pci.
>  
> +config MACB_SIFIVE_FU540
> +	bool "Cadence MACB/GEM support for SiFive FU540 SoC"
> +	depends on MACB && GPIO_SIFIVE
> +	help
> +	  Enable the Cadence MACB/GEM support for SiFive FU540 SoC.
> +
>  endif # NET_VENDOR_CADENCE
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c049410..275b5e8 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -10,6 +10,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/crc32.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -40,6 +41,15 @@
>  #include <linux/pm_runtime.h>
>  #include "macb.h"
>  
> +/* This structure is only used for MACB on SiFive FU540 devices */
> +struct sifive_fu540_macb_mgmt {
> +	void __iomem *reg;
> +	unsigned long rate;
> +	struct clk_hw hw;
> +};
> +
> +static struct sifive_fu540_macb_mgmt *mgmt;
> +
>  #define MACB_RX_BUFFER_SIZE	128
>  #define RX_BUFFER_MULTIPLE	64  /* bytes */
>  
> @@ -3903,6 +3913,116 @@ static int at91ether_init(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static unsigned long fu540_macb_tx_recalc_rate(struct clk_hw *hw,
> +					       unsigned long parent_rate)
> +{
> +	return mgmt->rate;
> +}
> +
> +static long fu540_macb_tx_round_rate(struct clk_hw *hw, unsigned long rate,
> +				     unsigned long *parent_rate)
> +{
> +	if (WARN_ON(rate < 2500000))
> +		return 2500000;
> +	else if (rate == 2500000)
> +		return 2500000;
> +	else if (WARN_ON(rate < 13750000))
> +		return 2500000;
> +	else if (WARN_ON(rate < 25000000))
> +		return 25000000;
> +	else if (rate == 25000000)
> +		return 25000000;
> +	else if (WARN_ON(rate < 75000000))
> +		return 25000000;
> +	else if (WARN_ON(rate < 125000000))
> +		return 125000000;
> +	else if (rate == 125000000)
> +		return 125000000;
> +
> +	WARN_ON(rate > 125000000);
> +
> +	return 125000000;
> +}
> +
> +static int fu540_macb_tx_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long parent_rate)
> +{
> +	rate = fu540_macb_tx_round_rate(hw, rate, &parent_rate);
> +	if (rate != 125000000)
> +		iowrite32(1, mgmt->reg);
> +	else
> +		iowrite32(0, mgmt->reg);
> +	mgmt->rate = rate;
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops fu540_c000_ops = {
> +	.recalc_rate = fu540_macb_tx_recalc_rate,
> +	.round_rate = fu540_macb_tx_round_rate,
> +	.set_rate = fu540_macb_tx_set_rate,
> +};
> +
> +static int fu540_c000_clk_init(struct platform_device *pdev, struct clk **pclk,
> +			       struct clk **hclk, struct clk **tx_clk,
> +			       struct clk **rx_clk, struct clk **tsu_clk)
> +{
> +	struct clk_init_data init;
> +	int err = 0;
> +
> +	err = macb_clk_init(pdev, pclk, hclk, tx_clk, rx_clk, tsu_clk);
> +	if (err)
> +		return err;
> +
> +	mgmt = devm_kzalloc(&pdev->dev, sizeof(*mgmt), GFP_KERNEL);
> +	if (!mgmt)
> +		return -ENOMEM;
> +
> +	init.name = "sifive-gemgxl-mgmt";
> +	init.ops = &fu540_c000_ops;
> +	init.flags = 0;
> +	init.num_parents = 0;
> +
> +	mgmt->rate = 0;
> +	mgmt->hw.init = &init;
> +
> +	*tx_clk = clk_register(NULL, &mgmt->hw);
> +	if (IS_ERR(*tx_clk))
> +		return PTR_ERR(*tx_clk);
> +
> +	err = clk_prepare_enable(*tx_clk);
> +	if (err)
> +		dev_err(&pdev->dev, "failed to enable tx_clk (%u)\n", err);
> +	else
> +		dev_info(&pdev->dev, "Registered clk switch '%s'\n", init.name);
> +
> +	return 0;
> +}
> +
> +static int fu540_c000_init(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res)
> +		return -ENODEV;
> +
> +	mgmt->reg = ioremap(res->start, resource_size(res));
> +	if (!mgmt->reg)
> +		return -ENOMEM;
> +
> +	return macb_init(pdev);
> +}
> +
> +static const struct macb_config fu540_c000_config = {
> +	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> +		MACB_CAPS_GEM_HAS_PTP,
> +	.dma_burst_length = 16,
> +	.clk_init = fu540_c000_clk_init,
> +	.init = fu540_c000_init,
> +	.jumbo_max_len = 10240,
> +};
> +
>  static const struct macb_config at91sam9260_config = {
>  	.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
>  	.clk_init = macb_clk_init,
> @@ -3992,6 +4112,9 @@ static int at91ether_init(struct platform_device *pdev)
>  	{ .compatible = "cdns,emac", .data = &emac_config },
>  	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
>  	{ .compatible = "cdns,zynq-gem", .data = &zynq_config },
> +#ifdef CONFIG_MACB_SIFIVE_FU540
> +	{ .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
> +#endif

This #ifdef should not be needed.

>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, macb_dt_ids);
> @@ -4199,6 +4322,9 @@ static int macb_probe(struct platform_device *pdev)
>  
>  err_disable_clocks:
>  	clk_disable_unprepare(tx_clk);
> +#ifdef CONFIG_MACB_SIFIVE_FU540
> +	clk_unregister(tx_clk);
> +#endif

So long as tx_clk is NULL, you can call clk_unregister(). So please
remove the #ifdef.


>  	clk_disable_unprepare(hclk);
>  	clk_disable_unprepare(pclk);
>  	clk_disable_unprepare(rx_clk);
> @@ -4233,6 +4359,9 @@ static int macb_remove(struct platform_device *pdev)
>  		pm_runtime_dont_use_autosuspend(&pdev->dev);
>  		if (!pm_runtime_suspended(&pdev->dev)) {
>  			clk_disable_unprepare(bp->tx_clk);
> +#ifdef CONFIG_MACB_SIFIVE_FU540
> +			clk_unregister(bp->tx_clk);
> +#endif

Same here.

In general try to avoid #ifdef in C code.

   Andrew

Re: [PATCH v2 2/2] macb: Add support for SiFive FU540-C000

[Yash Shah]

On Mon, Jun 17, 2019 at 9:28 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Jun 17, 2019 at 09:49:27AM +0530, Yash Shah wrote:
...
> >  static const struct macb_config at91sam9260_config = {
> >       .caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
> >       .clk_init = macb_clk_init,
> > @@ -3992,6 +4112,9 @@ static int at91ether_init(struct platform_device *pdev)
> >       { .compatible = "cdns,emac", .data = &emac_config },
> >       { .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
> >       { .compatible = "cdns,zynq-gem", .data = &zynq_config },
> > +#ifdef CONFIG_MACB_SIFIVE_FU540
> > +     { .compatible = "sifive,fu540-macb", .data = &fu540_c000_config },
> > +#endif
>
> This #ifdef should not be needed.
>
> >       { /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, macb_dt_ids);
> > @@ -4199,6 +4322,9 @@ static int macb_probe(struct platform_device *pdev)
> >
> >  err_disable_clocks:
> >       clk_disable_unprepare(tx_clk);
> > +#ifdef CONFIG_MACB_SIFIVE_FU540
> > +     clk_unregister(tx_clk);
> > +#endif
>
> So long as tx_clk is NULL, you can call clk_unregister(). So please
> remove the #ifdef.
>
>
> >       clk_disable_unprepare(hclk);
> >       clk_disable_unprepare(pclk);
> >       clk_disable_unprepare(rx_clk);
> > @@ -4233,6 +4359,9 @@ static int macb_remove(struct platform_device *pdev)
> >               pm_runtime_dont_use_autosuspend(&pdev->dev);
> >               if (!pm_runtime_suspended(&pdev->dev)) {
> >                       clk_disable_unprepare(bp->tx_clk);
> > +#ifdef CONFIG_MACB_SIFIVE_FU540
> > +                     clk_unregister(bp->tx_clk);
> > +#endif
>
> Same here.
>
> In general try to avoid #ifdef in C code.

Will remove all the #ifdef in v3.
Thanks for your comments.

- Yash

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Yash Shah]

On Mon, Jun 17, 2019 at 9:49 AM Yash Shah <yash.shah@sifive.com> wrote:
>
> On FU540, the management IP block is tightly coupled with the Cadence
> MACB IP block. It manages many of the boundary signals from the MACB IP
> This patchset controls the tx_clk input signal to the MACB IP. It
> switches between the local TX clock (125MHz) and PHY TX clocks. This
> is necessary to toggle between 1Gb and 100/10Mb speeds.
>
> Future patches may add support for monitoring or controlling other IP
> boundary signals.
>
> This patchset is mostly based on work done by
> Wesley Terpstra <wesley@sifive.com>
>
> This patchset is based on Linux v5.2-rc1 and tested on HiFive Unleashed
> board with additional board related patches needed for testing can be
> found at dev/yashs/ethernet branch of:

Correction in branch name: dev/yashs/ethernet_v2

> https://github.com/yashshah7/riscv-linux.git
>
> Change History:
> V2:
> - Change compatible string from "cdns,fu540-macb" to "sifive,fu540-macb"
> - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
>   driver. This is needed because on FU540, the macb driver depends on
>   SiFive GPIO driver.
> - Avoid writing the result of a comparison to a register.
> - Fix the issue of probe fail on reloading the module reported by:
>   Andreas Schwab <schwab@suse.de>
>
> Yash Shah (2):
>   macb: bindings doc: add sifive fu540-c000 binding
>   macb: Add support for SiFive FU540-C000
>
>  Documentation/devicetree/bindings/net/macb.txt |   3 +
>  drivers/net/ethernet/cadence/Kconfig           |   6 ++
>  drivers/net/ethernet/cadence/macb_main.c       | 129 +++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>
> --
> 1.9.1
>

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Andreas Schwab]

On Jun 17 2019, Yash Shah <yash.shah@sifive.com> wrote:

> - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
>   driver. This is needed because on FU540, the macb driver depends on
>   SiFive GPIO driver.

This of course requires that the GPIO driver is upstreamed first.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Paul Walmsley]

Hi Yash,

On Mon, 17 Jun 2019, Andreas Schwab wrote:

> On Jun 17 2019, Yash Shah <yash.shah@sifive.com> wrote:
> 
> > - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
> >   driver. This is needed because on FU540, the macb driver depends on
> >   SiFive GPIO driver.
> 
> This of course requires that the GPIO driver is upstreamed first.

What's the impact of enabling CONFIG_MACB_SIFIVE_FU540 when the GPIO 
driver isn't present?  (After modifying the Kconfig "depends" line 
appropriately.)

Looks to me that it shouldn't have an impact unless the DT string is 
present, and even then, the impact might simply be that the MACB driver 
may not work?


- Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Andreas Schwab]

On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:

> Looks to me that it shouldn't have an impact unless the DT string is 
> present, and even then, the impact might simply be that the MACB driver 
> may not work?

If the macb driver doesn't work you have an unusable system, of course.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Paul Walmsley]

On Mon, 17 Jun 2019, Andreas Schwab wrote:

> On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> 
> > Looks to me that it shouldn't have an impact unless the DT string is 
> > present, and even then, the impact might simply be that the MACB driver 
> > may not work?
> 
> If the macb driver doesn't work you have an unusable system, of course.

Why?

- Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Andreas Schwab]

On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:

> On Mon, 17 Jun 2019, Andreas Schwab wrote:
>
>> On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
>> 
>> > Looks to me that it shouldn't have an impact unless the DT string is 
>> > present, and even then, the impact might simply be that the MACB driver 
>> > may not work?
>> 
>> If the macb driver doesn't work you have an unusable system, of course.
>
> Why?

Because a system is useless without network.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Paul Walmsley]

On Mon, 17 Jun 2019, Andreas Schwab wrote:

> On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> 
> > On Mon, 17 Jun 2019, Andreas Schwab wrote:
> >
> >> On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> >> 
> >> > Looks to me that it shouldn't have an impact unless the DT string is 
> >> > present, and even then, the impact might simply be that the MACB driver 
> >> > may not work?
> >> 
> >> If the macb driver doesn't work you have an unusable system, of course.
> >
> > Why?
> 
> Because a system is useless without network.

From an upstream Linux point of view, Yash's patches should be an 
improvement over the current mainline kernel situation, since there's 
currently no upstream support for the (SiFive-specific) TX clock switch 
register.  With the right DT data, and a bootloader that handles the PHY 
reset, I think networking should work after his patches are upstream -- 
although I myself haven't tried this yet.


- Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Troy Benjegerdes]

> On Jun 17, 2019, at 6:34 AM, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> 
> On Mon, 17 Jun 2019, Andreas Schwab wrote:
> 
>> On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
>> 
>>> On Mon, 17 Jun 2019, Andreas Schwab wrote:
>>> 
>>>> On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
>>>> 
>>>>> Looks to me that it shouldn't have an impact unless the DT string is 
>>>>> present, and even then, the impact might simply be that the MACB driver 
>>>>> may not work?
>>>> 
>>>> If the macb driver doesn't work you have an unusable system, of course.
>>> 
>>> Why?
>> 
>> Because a system is useless without network.
> 
> From an upstream Linux point of view, Yash's patches should be an 
> improvement over the current mainline kernel situation, since there's 
> currently no upstream support for the (SiFive-specific) TX clock switch 
> register.  With the right DT data, and a bootloader that handles the PHY 
> reset, I think networking should work after his patches are upstream -- 
> although I myself haven't tried this yet.
> 

Have we documented this tx clock switch register in something with a
direct URL link (rather than a PDF)?

I’d like to update freedom-u-sdk (or yocto) to create bootable images
with a working U-boot (upstream or not, I don’t care, as long as it works),
and what I have right now is the old legacy HiFive U-boot[1] and a 4.19
kernel with a bunch of extra patches.

The legacy M-mode U-boot handles the phy reset already, and I’ve been
able to load upstream S-mode uboot as a payload via TFTP, and then 
load and boot a 4.19 kernel. 

It would be nice to get this all working with 5.x, however there are still
several missing pieces to really have it work well.


[1] https://github.com/sifive/HiFive_U-Boot

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Paul Walmsley]

On Mon, 17 Jun 2019, Troy Benjegerdes wrote:

> Have we documented this tx clock switch register in something with a
> direct URL link (rather than a PDF)?

The SiFive FU540 user manual PDF is the canonical public reference:

https://static.dev.sifive.com/FU540-C000-v1.0.pdf

This practice aligns with other SoC vendors, who also release PDFs.

The relevant Ethernet documentation, including register maps, is in 
Chapter 19.


- Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Alistair Francis]

On Mon, 2019-06-17 at 09:14 -0500, Troy Benjegerdes wrote:
> > On Jun 17, 2019, at 6:34 AM, Paul Walmsley <
> > paul.walmsley@sifive.com> wrote:
> > 
> > On Mon, 17 Jun 2019, Andreas Schwab wrote:
> > 
> > > On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > > 
> > > > On Mon, 17 Jun 2019, Andreas Schwab wrote:
> > > > 
> > > > > On Jun 17 2019, Paul Walmsley <paul.walmsley@sifive.com>
> > > > > wrote:
> > > > > 
> > > > > > Looks to me that it shouldn't have an impact unless the DT
> > > > > > string is 
> > > > > > present, and even then, the impact might simply be that the
> > > > > > MACB driver 
> > > > > > may not work?
> > > > > 
> > > > > If the macb driver doesn't work you have an unusable system,
> > > > > of course.
> > > > 
> > > > Why?
> > > 
> > > Because a system is useless without network.
> > 
> > From an upstream Linux point of view, Yash's patches should be an 
> > improvement over the current mainline kernel situation, since
> > there's 
> > currently no upstream support for the (SiFive-specific) TX clock
> > switch 
> > register.  With the right DT data, and a bootloader that handles
> > the PHY 
> > reset, I think networking should work after his patches are
> > upstream -- 
> > although I myself haven't tried this yet.
> > 
> 
> Have we documented this tx clock switch register in something with a
> direct URL link (rather than a PDF)?
> 
> I’d like to update freedom-u-sdk (or yocto) to create bootable images
> with a working U-boot (upstream or not, I don’t care, as long as it
> works),
> and what I have right now is the old legacy HiFive U-boot[1] and a
> 4.19
> kernel with a bunch of extra patches.

Yocto/OpenEmbedded does this today. TFTP boot works with the 2019.04 U-
Boot (+ some patches ontop for SMP support). We use the latest 5.1
stable kernel plus 5 or so patches to boot on the Unleased. Networking,
display and audio are all working with the Microsemi expansion board as
well. Let me know if there is something else missing and I'll add it
in. There are probably documentation fixes that are needed as well.

I was thinking of skipping the 5.2 release though as I thought the DT
stuff wasn't going to make it [1]. I will probably re-evaluate that
decision though when 5.2 comes out as it looks like it's all going to
work :)

With U-boot 2017.09 and Linux 5.2/5.3 we should finally be upstream
only!

> 
> The legacy M-mode U-boot handles the phy reset already, and I’ve been
> able to load upstream S-mode uboot as a payload via TFTP, and then 
> load and boot a 4.19 kernel. 
> 
> It would be nice to get this all working with 5.x, however there are
> still
> several missing pieces to really have it work well.

Let me know what is still missing/doesn't work and I can add it. At the
moment the only known issue I know of is a missing SD card driver in U-
Boot.

1: https://github.com/riscv/meta-riscv/issues/143

Alistair

> 
> 
> [1] https://github.com/sifive/HiFive_U-Boot
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Paul Walmsley]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

On Mon, 17 Jun 2019, Alistair Francis wrote:

> > The legacy M-mode U-boot handles the phy reset already, and I’ve been
> > able to load upstream S-mode uboot as a payload via TFTP, and then 
> > load and boot a 4.19 kernel. 
> > 
> > It would be nice to get this all working with 5.x, however there are
> > still
> > several missing pieces to really have it work well.
> 
> Let me know what is still missing/doesn't work and I can add it. At the
> moment the only known issue I know of is a missing SD card driver in U-
> Boot.

The DT data has changed between the non-upstream data that people 
developed against previously, vs. the DT data that just went upstream 
here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852

and

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205

So Upstream U-Boot is going to need several patches to get things working 
again.  Clock identifiers and Ethernet are two known areas.


- Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Anup Patel]

On Tue, Jun 18, 2019 at 8:56 AM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Mon, 17 Jun 2019, Alistair Francis wrote:
>
> > > The legacy M-mode U-boot handles the phy reset already, and I’ve been
> > > able to load upstream S-mode uboot as a payload via TFTP, and then
> > > load and boot a 4.19 kernel.
> > >
> > > It would be nice to get this all working with 5.x, however there are
> > > still
> > > several missing pieces to really have it work well.
> >
> > Let me know what is still missing/doesn't work and I can add it. At the
> > moment the only known issue I know of is a missing SD card driver in U-
> > Boot.
>
> The DT data has changed between the non-upstream data that people
> developed against previously, vs. the DT data that just went upstream
> here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852
>
> and
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205
>
> So Upstream U-Boot is going to need several patches to get things working
> again.  Clock identifiers and Ethernet are two known areas.

Done.

I just send-out few patches to fix U-Boot SiFive Clock driver.

The U-Boot SiFive Clock driver fix series can be found in
riscv_unleashed_clk_sync_v1 branch of:
https://github.com/avpatel/u-boot.git

Users will also require OpenSBI DTB fix which can be found
in sifive_unleashed_dtb_fix_v1 branch of:
https://github.com/avpatel/opensbi.git

With above fixes, we can now use same DTB for both U-Boot
and Linux kernel (5.2-rc1). Although, users are free to pass a
different DTB to Linux kernel via TFTP.

I have tested SiFive serial and Cadance MACB ethernet on
both U-Boot and Linux (5.2-rc1)

Regards,
Anup

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Troy Benjegerdes]

> On Jun 18, 2019, at 4:32 AM, Anup Patel <anup@brainfault.org> wrote:
> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852

I added your patches, along with two of mine, and rebased them
to the latest U-boot master, and put them on the ‘to-upstream’ branch
at https://github.com/sifive/u-boot/tree/to-upstream

I am most interested in review of the patch that adds the DTS files
from Linux to U-boot, along with a ‘-u-boot.dtsi’ file which includes
several extra things, most notably an ethernet entry [1] which does
not match the new proposed changes for the MacB driver that Yash
is working on.

How close are we to consensus on the new “sifive,fu540-macb”
device tree entry format? Is this something that is stable enough to
start basing some work in M-mode U-boot on yet, or do we expect
more changes?

[1] https://github.com/sifive/u-boot/commit/35e4168e36139722f30143a0ca0aa8637dd3ee04#diff-27d2d375ddac52f1bca71594075e1be4R93

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Atish Patra]

On Tue, 2019-06-18 at 15:02 +0530, Anup Patel wrote:
> On Tue, Jun 18, 2019 at 8:56 AM Paul Walmsley <
> paul.walmsley@sifive.com> wrote:
> > On Mon, 17 Jun 2019, Alistair Francis wrote:
> > 
> > > > The legacy M-mode U-boot handles the phy reset already, and
> > > > I’ve been
> > > > able to load upstream S-mode uboot as a payload via TFTP, and
> > > > then
> > > > load and boot a 4.19 kernel.
> > > > 
> > > > It would be nice to get this all working with 5.x, however
> > > > there are
> > > > still
> > > > several missing pieces to really have it work well.
> > > 
> > > Let me know what is still missing/doesn't work and I can add it.
> > > At the
> > > moment the only known issue I know of is a missing SD card driver
> > > in U-
> > > Boot.
> > 
> > The DT data has changed between the non-upstream data that people
> > developed against previously, vs. the DT data that just went
> > upstream
> > here:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852
> > 
> > and
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205
> > 
> > So Upstream U-Boot is going to need several patches to get things
> > working
> > again.  Clock identifiers and Ethernet are two known areas.
> 
> Done.
> 
> I just send-out few patches to fix U-Boot SiFive Clock driver.
> 
> The U-Boot SiFive Clock driver fix series can be found in
> riscv_unleashed_clk_sync_v1 branch of:
> https://github.com/avpatel/u-boot.git
> 
> Users will also require OpenSBI DTB fix which can be found
> in sifive_unleashed_dtb_fix_v1 branch of:
> https://github.com/avpatel/opensbi.git
> 

Additionally, user can also use FW_PAYLOAD_FDT_PATH argument during
build to include the dtb built from kernel.

example:
make -j 32 PLATFORM=sifive/fu540 FW_PAYLOAD_PATH=<u-boot path>/u-
boot.bin FW_PAYLOAD_FDT_PATH=<kernel_dtb_path>


> With above fixes, we can now use same DTB for both U-Boot
> and Linux kernel (5.2-rc1). Although, users are free to pass a
> different DTB to Linux kernel via TFTP.
> 
> I have tested SiFive serial and Cadance MACB ethernet on
> both U-Boot and Linux (5.2-rc1)
> 
> Regards,
> Anup
-- 
Regards,
Atish

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Alistair Francis]

On Mon, 2019-06-17 at 20:26 -0700, Paul Walmsley wrote:
> On Mon, 17 Jun 2019, Alistair Francis wrote:
> 
> > > The legacy M-mode U-boot handles the phy reset already, and I’ve
> > > been
> > > able to load upstream S-mode uboot as a payload via TFTP, and
> > > then 
> > > load and boot a 4.19 kernel. 
> > > 
> > > It would be nice to get this all working with 5.x, however there
> > > are
> > > still
> > > several missing pieces to really have it work well.
> > 
> > Let me know what is still missing/doesn't work and I can add it. At
> > the
> > moment the only known issue I know of is a missing SD card driver
> > in U-
> > Boot.
> 
> The DT data has changed between the non-upstream data that people 
> developed against previously, vs. the DT data that just went
> upstream 
> here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852
> 
> and
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205
> 
> So Upstream U-Boot is going to need several patches to get things
> working 
> again.  Clock identifiers and Ethernet are two known areas.

Yep, Anup has sent patches to U-Boot and OpenSBI.

Alistair

> 
> 
> - Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Yash Shah]

On Mon, Jun 17, 2019 at 3:28 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> Hi Yash,
>
> On Mon, 17 Jun 2019, Andreas Schwab wrote:
>
> > On Jun 17 2019, Yash Shah <yash.shah@sifive.com> wrote:
> >
> > > - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
> > >   driver. This is needed because on FU540, the macb driver depends on
> > >   SiFive GPIO driver.
> >
> > This of course requires that the GPIO driver is upstreamed first.
>
> What's the impact of enabling CONFIG_MACB_SIFIVE_FU540 when the GPIO
> driver isn't present?  (After modifying the Kconfig "depends" line
> appropriately.)
>
> Looks to me that it shouldn't have an impact unless the DT string is
> present, and even then, the impact might simply be that the MACB driver
> may not work?

Yes, there won't be an impact other than MACB driver not working.
In any case, without GPIO driver, PHY won't get reset and the network
interface won't come up.

>
>
> - Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Paul Walmsley]

On Mon, 17 Jun 2019, Yash Shah wrote:

> On Mon, Jun 17, 2019 at 3:28 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> > On Mon, 17 Jun 2019, Andreas Schwab wrote:
> >
> > > On Jun 17 2019, Yash Shah <yash.shah@sifive.com> wrote:
> > >
> > > > - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
> > > >   driver. This is needed because on FU540, the macb driver depends on
> > > >   SiFive GPIO driver.
> > >
> > > This of course requires that the GPIO driver is upstreamed first.
> >
> > What's the impact of enabling CONFIG_MACB_SIFIVE_FU540 when the GPIO
> > driver isn't present?  (After modifying the Kconfig "depends" line
> > appropriately.)
> >
> > Looks to me that it shouldn't have an impact unless the DT string is
> > present, and even then, the impact might simply be that the MACB driver
> > may not work?
> 
> Yes, there won't be an impact other than MACB driver not working.

OK.  In that case, there doesn't seem much point to adding the Kconfig 
option.  Could you please post a new version without it?

> In any case, without GPIO driver, PHY won't get reset and the network
> interface won't come up.

Naturally, in the medium term, we want Linux to handle the reset.  But if 
there's no GPIO driver present, and the bootloader handles the PHY reset 
before the kernel starts, would the network driver work in that case?
 

- Paul

Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000

[Yash Shah]

On Mon, Jun 17, 2019 at 3:58 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Mon, 17 Jun 2019, Yash Shah wrote:
>
> > On Mon, Jun 17, 2019 at 3:28 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> >
> > > On Mon, 17 Jun 2019, Andreas Schwab wrote:
> > >
> > > > On Jun 17 2019, Yash Shah <yash.shah@sifive.com> wrote:
> > > >
> > > > > - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
> > > > >   driver. This is needed because on FU540, the macb driver depends on
> > > > >   SiFive GPIO driver.
> > > >
> > > > This of course requires that the GPIO driver is upstreamed first.
> > >
> > > What's the impact of enabling CONFIG_MACB_SIFIVE_FU540 when the GPIO
> > > driver isn't present?  (After modifying the Kconfig "depends" line
> > > appropriately.)
> > >
> > > Looks to me that it shouldn't have an impact unless the DT string is
> > > present, and even then, the impact might simply be that the MACB driver
> > > may not work?
> >
> > Yes, there won't be an impact other than MACB driver not working.
>
> OK.  In that case, there doesn't seem much point to adding the Kconfig
> option.  Could you please post a new version without it?

Sure, will do that.

>
> > In any case, without GPIO driver, PHY won't get reset and the network
> > interface won't come up.
>
> Naturally, in the medium term, we want Linux to handle the reset.  But if
> there's no GPIO driver present, and the bootloader handles the PHY reset
> before the kernel starts, would the network driver work in that case?

Yes, if bootloader handles the PHY reset then the network driver will
work in that case.
I will post a new version without the GPIO driver dependency.

>
>
> - Paul