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

2019-05-29 Thread Palmer Dabbelt

On Fri, 24 May 2019 06:48:47 PDT (-0700), and...@lunn.ch wrote:

On Fri, May 24, 2019 at 10:22:06AM +0530, Yash Shah wrote:

On Thu, May 23, 2019 at 8:24 PM Andrew Lunn  wrote:
>
> > +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);
> > + iowrite32(rate != 12500, mgmt->reg);
>
> That looks odd. Writing the result of a comparison to a register?

The idea was to write "1" to the register if the value of rate is
anything else than 12500.


I'm not a language lawyer. Is it guaranteed that an expression like
this returns 1? Any value !0 is true, so maybe it actually returns 42?



From Stack Overflow: 
https://stackoverflow.com/questions/18097922/return-value-of-operator-in-c


"C11(ISO/IEC 9899:201x) ยง6.5.8 Relational operators

Each of the operators < (less than), > (greater than), <= (less than or equal
to), and >= (greater than or equal to) shall yield 1 if the specified relation
is true and 0 if it is false. The result has type int."


To make it easier to read, I will change this to below:
- iowrite32(rate != 12500, mgmt->reg);
+ if (rate != 12500)
+ iowrite32(1, mgmt->reg);
+ else
+ iowrite32(0, mgmt->reg);

Hope that's fine. Thanks for your comment


Yes, that is good.

 Andrew


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

2019-05-24 Thread Andrew Lunn
On Fri, May 24, 2019 at 10:22:06AM +0530, Yash Shah wrote:
> On Thu, May 23, 2019 at 8:24 PM Andrew Lunn  wrote:
> >
> > > +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);
> > > + iowrite32(rate != 12500, mgmt->reg);
> >
> > That looks odd. Writing the result of a comparison to a register?
> 
> The idea was to write "1" to the register if the value of rate is
> anything else than 12500.

I'm not a language lawyer. Is it guaranteed that an expression like
this returns 1? Any value !0 is true, so maybe it actually returns 42?

> To make it easier to read, I will change this to below:
> - iowrite32(rate != 12500, mgmt->reg);
> + if (rate != 12500)
> + iowrite32(1, mgmt->reg);
> + else
> + iowrite32(0, mgmt->reg);
> 
> Hope that's fine. Thanks for your comment

Yes, that is good.

 Andrew


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

2019-05-23 Thread Yash Shah
On Thu, May 23, 2019 at 8:24 PM Andrew Lunn  wrote:
>
> > +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);
> > + iowrite32(rate != 12500, mgmt->reg);
>
> That looks odd. Writing the result of a comparison to a register?

The idea was to write "1" to the register if the value of rate is
anything else than 12500.
To make it easier to read, I will change this to below:
- iowrite32(rate != 12500, mgmt->reg);
+ if (rate != 12500)
+ iowrite32(1, mgmt->reg);
+ else
+ iowrite32(0, mgmt->reg);

Hope that's fine. Thanks for your comment
- Yash

>
>  Andrew
>
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


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

2019-05-23 Thread Andrew Lunn
> +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);
> + iowrite32(rate != 12500, mgmt->reg);

That looks odd. Writing the result of a comparison to a register?

 Andrew


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

2019-05-23 Thread 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 
---
 drivers/net/ethernet/cadence/macb_main.c | 118 +++
 1 file changed, 118 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c 
b/drivers/net/ethernet/cadence/macb_main.c
index c049410..a9e5227 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 
+#include 
 #include 
 #include 
 #include 
@@ -40,6 +41,15 @@
 #include 
 #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_SIZE128
 #define RX_BUFFER_MULTIPLE 64  /* bytes */
 
@@ -3903,6 +3913,113 @@ 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 < 250))
+   return 250;
+   else if (rate == 250)
+   return 250;
+   else if (WARN_ON(rate < 1375))
+   return 250;
+   else if (WARN_ON(rate < 2500))
+   return 2500;
+   else if (rate == 2500)
+   return 2500;
+   else if (WARN_ON(rate < 7500))
+   return 2500;
+   else if (WARN_ON(rate < 12500))
+   return 12500;
+   else if (rate == 12500)
+   return 12500;
+
+   WARN_ON(rate > 12500);
+
+   return 12500;
+}
+
+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);
+   iowrite32(rate != 12500, 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,
@@ -3980,6 +4097,7 @@ static int at91ether_init(struct platform_device *pdev)
{ .compatible = "cdns,at32ap7000-macb" },
{ .compatible = "cdns,at91sam9260-macb", .data = &at91sam9260_config },
{ .compatible = "cdns,macb" },
+   { .compatible = "cdns,fu540-macb", .data = &fu540_c000_config },
{