| Message ID | 1558611952-13295-1-git-send-email-yash.shah@sifive.com |
|---|---|
| Headers | show |
| Series | net: macb: Add support for SiFive FU540-C000 | expand |
On Mai 23 2019, 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. Doesn't work for me: [ 365.842801] macb: probe of 10090000.ethernet failed with error -17 Andreas.
> +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 != 125000000, mgmt->reg); That looks odd. Writing the result of a comparison to a register? Andrew
Please be consistent in your subsystem prefixes used in your Subject lines. You use "net: macb:" then "net/macb:" Really, plain "macb: " is sufficient. Thank you.
Hi Andreas, On Thu, May 23, 2019 at 6:19 PM Andreas Schwab <schwab@suse.de> wrote: > > On Mai 23 2019, 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. > > Doesn't work for me: > > [ 365.842801] macb: probe of 10090000.ethernet failed with error -17 > Make sure you have applied all the patches needed for testing found at dev/yashs/ethernet branch of: https://github.com/yashshah7/riscv-linux.git In addition to that, make sure in your kernel config GPIO_SIFIVE=y In v2 of this patch, I will add this select GPIO_SIFIVE config in the Cadence Kconfig file. - Yash
On Thu, May 23, 2019 at 8:24 PM Andrew Lunn <andrew@lunn.ch> 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 != 125000000, 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 125000000. To make it easier to read, I will change this to below: - iowrite32(rate != 125000000, mgmt->reg); + if (rate != 125000000) + iowrite32(1, mgmt->reg); + else + iowrite32(0, mgmt->reg); Hope that's fine. Thanks for your comment - Yash > > Andrew > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, May 23, 2019 at 9:58 PM David Miller <davem@davemloft.net> wrote: > > > Please be consistent in your subsystem prefixes used in your Subject lines. > You use "net: macb:" then "net/macb:" Really, plain "macb: " is sufficient. Sure, Will take care of this in the next revision of this patch. Thanks for your comment. - Yash
On Fri, May 24, 2019 at 10:22:06AM +0530, Yash Shah wrote: > On Thu, May 23, 2019 at 8:24 PM Andrew Lunn <andrew@lunn.ch> 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 != 125000000, 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 125000000. 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 != 125000000, mgmt->reg); > + if (rate != 125000000) > + iowrite32(1, mgmt->reg); > + else > + iowrite32(0, mgmt->reg); > > Hope that's fine. Thanks for your comment Yes, that is good. Andrew
On Mai 24 2019, Yash Shah <yash.shah@sifive.com> wrote: > Hi Andreas, > > On Thu, May 23, 2019 at 6:19 PM Andreas Schwab <schwab@suse.de> wrote: >> >> On Mai 23 2019, 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. >> >> Doesn't work for me: >> >> [ 365.842801] macb: probe of 10090000.ethernet failed with error -17 >> > > Make sure you have applied all the patches needed for testing found at > dev/yashs/ethernet branch of: Nope, try reloading the module. Andreas.
On Mon, May 27, 2019 at 1:34 PM Andreas Schwab <schwab@suse.de> wrote: > > On Mai 24 2019, Yash Shah <yash.shah@sifive.com> wrote: > > > Hi Andreas, > > > > On Thu, May 23, 2019 at 6:19 PM Andreas Schwab <schwab@suse.de> wrote: > >> > >> On Mai 23 2019, 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. > >> > >> Doesn't work for me: > >> > >> [ 365.842801] macb: probe of 10090000.ethernet failed with error -17 > >> > > > > Make sure you have applied all the patches needed for testing found at > > dev/yashs/ethernet branch of: > > Nope, try reloading the module. Yes, I could see the error on reloading the module. Thanks for the catch. I will fix this in the next version of this patch. - Yash
On Fri, 24 May 2019 06:48:47 PDT (-0700), andrew@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 <andrew@lunn.ch> 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 != 125000000, 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 125000000. > > 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 != 125000000, mgmt->reg); >> + if (rate != 125000000) >> + iowrite32(1, mgmt->reg); >> + else >> + iowrite32(0, mgmt->reg); >> >> Hope that's fine. Thanks for your comment > > Yes, that is good. > > Andrew