Message ID | 20210422091202.396956-9-green.wan@sifive.com |
---|---|
State | Superseded |
Delegated to: | Andes |
Headers | show |
Series | Add FU740 chip and HiFive Unmatched board support | expand |
(resending to list after subscribing) Hi, On Thu, Apr 22, 2021 at 10:15 AM Green Wan <green.wan@sifive.com> wrote: > > From: David Abdurachmanov <david.abdurachmanov@sifive.com> > > Add fu740 support to macb ethernet driver > > There is a PLL HW quirk in FU740. The VSC8541XMV-02 specification > requires 125 +/-0.0125 Mhz. But the most close value can be output > by PLL is 125.125 MHz and out of VSC8541XMV-02 spec. > In the Linux kernel driver for this drivers/net/ethernet/cadence/macb_main.c it is not marked as compatible with "sifive,fu740-c000-gem" and it does not have a similar fix (and appears to use 125.0 MHz). Should a similar fix be contributed to the Linux kernel? As otherwise at the moment, one cannot pass the dtb from u-boot to linux, as that leads to loss of network since the kernel doesn't know about "sifive,fu740-c000-gem". If linux kernel contribution is not forthcoming, would it be possible to have u-boot dtb to be compatible with _both_ "sifive,fu540-c000-gem" and "sifive,fu740-c000-gem" on unmatched boards, such that if one (mistakenly) uses u-boots dtb with vanilla linux kernel networking still works? And then adjust the test to check for "sifive,fu740-c000-gem" compatible string first. > Signed-off-by: David Abdurachmanov <david.abdurachmanov@sifive.com> > Signed-off-by: Green Wan <green.wan@sifive.com> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > --- > drivers/net/macb.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > index 57ea45e2dc..bf70525c54 100644 > --- a/drivers/net/macb.c > +++ b/drivers/net/macb.c > @@ -591,8 +591,17 @@ static int macb_sifive_clk_init(struct udevice *dev, ulong rate) > * 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 > + * > + * FU740 have a PLL HW quirk. The 125.125 Mhz is actually out of > + * VSC8541XMV-02 specification. The tolerance level is +/-100ppm. > + * Which means the range should be in between 125MHz +/-0.0125. > + * But the most close value can be output by PLL is 125.125 MHz. > */ > - writel(rate != 125000000, gemgxl_regs); > + if (device_is_compatible(dev, "sifive,fu540-c000-gem")) > + writel(rate != 125000000, gemgxl_regs); > + else if (device_is_compatible(dev, "sifive,fu740-c000-gem")) > + writel(rate != 125125000, gemgxl_regs); > + > return 0; > } > > @@ -1507,6 +1516,8 @@ static const struct udevice_id macb_eth_ids[] = { > { .compatible = "cdns,zynq-gem" }, > { .compatible = "sifive,fu540-c000-gem", > .data = (ulong)&sifive_config }, > + { .compatible = "sifive,fu740-c000-gem", > + .data = (ulong)&sifive_config }, > { .compatible = "microchip,mpfs-mss-gem", > .data = (ulong)µchip_config }, > { } > -- > 2.31.0 > -- Regards, Dimitri.
Hi Dimitri, Thanks for looking into this. On Tue, May 4, 2021 at 5:33 PM Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote: > > Hi, > > On Thu, Apr 22, 2021 at 10:15 AM Green Wan <green.wan@sifive.com> wrote: > > > > From: David Abdurachmanov <david.abdurachmanov@sifive.com> > > > > Add fu740 support to macb ethernet driver > > > > There is a PLL HW quirk in FU740. The VSC8541XMV-02 specification > > requires 125 +/-0.0125 Mhz. But the most close value can be output > > by PLL is 125.125 MHz and out of VSC8541XMV-02 spec. > > > > In the Linux kernel driver for this > drivers/net/ethernet/cadence/macb_main.c it is not marked as > compatible with "sifive,fu740-c000-gem" and it does not have a similar > fix (and appears to use 125.0 MHz). > Should a similar fix be contributed to the Linux kernel? You're right. We also notice some refinement should be made here. We're working on the way to solve it. > > As otherwise at the moment, one cannot pass the dtb from u-boot to > linux, as that leads to loss of network since the kernel doesn't know > about "sifive,fu740-c000-gem". If linux kernel contribution is not > forthcoming, would it be possible to have u-boot dtb to be compatible > with _both_ "sifive,fu540-c000-gem" and "sifive,fu740-c000-gem" on > unmatched boards, such that if one (mistakenly) uses u-boots dtb with > vanilla linux kernel networking still works? And then adjust the test > to check for "sifive,fu740-c000-gem" compatible string first. > Not sure whether I get it correct. I think the best case is to have Linux driver support both compatible string. And I'm a bit reluctant to handle incorrect DTB passed situations. It might end up with propagating issues and harder to trace back the root cause. I'll check with colleagues and see if we can resolve that concern. Thanks, > > Signed-off-by: David Abdurachmanov <david.abdurachmanov@sifive.com> > > Signed-off-by: Green Wan <green.wan@sifive.com> > > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > drivers/net/macb.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > > index 57ea45e2dc..bf70525c54 100644 > > --- a/drivers/net/macb.c > > +++ b/drivers/net/macb.c > > @@ -591,8 +591,17 @@ static int macb_sifive_clk_init(struct udevice *dev, ulong rate) > > * 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 > > + * > > + * FU740 have a PLL HW quirk. The 125.125 Mhz is actually out of > > + * VSC8541XMV-02 specification. The tolerance level is +/-100ppm. > > + * Which means the range should be in between 125MHz +/-0.0125. > > + * But the most close value can be output by PLL is 125.125 MHz. > > */ > > - writel(rate != 125000000, gemgxl_regs); > > + if (device_is_compatible(dev, "sifive,fu540-c000-gem")) > > + writel(rate != 125000000, gemgxl_regs); > > + else if (device_is_compatible(dev, "sifive,fu740-c000-gem")) > > + writel(rate != 125125000, gemgxl_regs); > > + > > return 0; > > } > > > > @@ -1507,6 +1516,8 @@ static const struct udevice_id macb_eth_ids[] = { > > { .compatible = "cdns,zynq-gem" }, > > { .compatible = "sifive,fu540-c000-gem", > > .data = (ulong)&sifive_config }, > > + { .compatible = "sifive,fu740-c000-gem", > > + .data = (ulong)&sifive_config }, > > { .compatible = "microchip,mpfs-mss-gem", > > .data = (ulong)µchip_config }, > > { } > > -- > > 2.31.0 > > > > > -- > Regards, > > Dimitri.
On Wed, May 5, 2021 at 4:15 AM Green Wan <green.wan@sifive.com> wrote: > > Hi Dimitri, > > Thanks for looking into this. > > On Tue, May 4, 2021 at 5:33 PM Dimitri John Ledkov > <dimitri.ledkov@canonical.com> wrote: > > > > Hi, > > > > On Thu, Apr 22, 2021 at 10:15 AM Green Wan <green.wan@sifive.com> wrote: > > > > > > From: David Abdurachmanov <david.abdurachmanov@sifive.com> > > > > > > Add fu740 support to macb ethernet driver > > > > > > There is a PLL HW quirk in FU740. The VSC8541XMV-02 specification > > > requires 125 +/-0.0125 Mhz. But the most close value can be output > > > by PLL is 125.125 MHz and out of VSC8541XMV-02 spec. > > > > > > > In the Linux kernel driver for this > > drivers/net/ethernet/cadence/macb_main.c it is not marked as > > compatible with "sifive,fu740-c000-gem" and it does not have a similar > > fix (and appears to use 125.0 MHz). > > Should a similar fix be contributed to the Linux kernel? > > You're right. We also notice some refinement should be made here. > We're working on the way to solve it. > > > > > As otherwise at the moment, one cannot pass the dtb from u-boot to > > linux, as that leads to loss of network since the kernel doesn't know > > about "sifive,fu740-c000-gem". If linux kernel contribution is not > > forthcoming, would it be possible to have u-boot dtb to be compatible > > with _both_ "sifive,fu540-c000-gem" and "sifive,fu740-c000-gem" on > > unmatched boards, such that if one (mistakenly) uses u-boots dtb with > > vanilla linux kernel networking still works? And then adjust the test > > to check for "sifive,fu740-c000-gem" compatible string first. > > > > Not sure whether I get it correct. I think the best case is to have > Linux driver support both compatible string. That is my desired outcome too. Please just do that. > And I'm a bit reluctant > to handle incorrect DTB passed situations. It might end up with > propagating issues and harder to trace back the root cause. I'll check > with colleagues and see if we can resolve that concern. > Writing / handling / handing-over incorrect DTB is not nice. > Thanks, > > > > Signed-off-by: David Abdurachmanov <david.abdurachmanov@sifive.com> > > > Signed-off-by: Green Wan <green.wan@sifive.com> > > > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > > --- > > > drivers/net/macb.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > > > index 57ea45e2dc..bf70525c54 100644 > > > --- a/drivers/net/macb.c > > > +++ b/drivers/net/macb.c > > > @@ -591,8 +591,17 @@ static int macb_sifive_clk_init(struct udevice *dev, ulong rate) > > > * 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 > > > + * > > > + * FU740 have a PLL HW quirk. The 125.125 Mhz is actually out of > > > + * VSC8541XMV-02 specification. The tolerance level is +/-100ppm. > > > + * Which means the range should be in between 125MHz +/-0.0125. > > > + * But the most close value can be output by PLL is 125.125 MHz. > > > */ > > > - writel(rate != 125000000, gemgxl_regs); > > > + if (device_is_compatible(dev, "sifive,fu540-c000-gem")) > > > + writel(rate != 125000000, gemgxl_regs); > > > + else if (device_is_compatible(dev, "sifive,fu740-c000-gem")) > > > + writel(rate != 125125000, gemgxl_regs); > > > + > > > return 0; > > > } > > > > > > @@ -1507,6 +1516,8 @@ static const struct udevice_id macb_eth_ids[] = { > > > { .compatible = "cdns,zynq-gem" }, > > > { .compatible = "sifive,fu540-c000-gem", > > > .data = (ulong)&sifive_config }, > > > + { .compatible = "sifive,fu740-c000-gem", > > > + .data = (ulong)&sifive_config }, > > > { .compatible = "microchip,mpfs-mss-gem", > > > .data = (ulong)µchip_config }, > > > { } > > > -- > > > 2.31.0 > > > > > > > > > -- > > Regards, > > > > Dimitri.
Hi, On Thu, Apr 22, 2021 at 5:14 PM Green Wan <green.wan@sifive.com> wrote: > > From: David Abdurachmanov <david.abdurachmanov@sifive.com> > > Add fu740 support to macb ethernet driver > > There is a PLL HW quirk in FU740. The VSC8541XMV-02 specification > requires 125 +/-0.0125 Mhz. But the most close value can be output > by PLL is 125.125 MHz and out of VSC8541XMV-02 spec. > > Signed-off-by: David Abdurachmanov <david.abdurachmanov@sifive.com> > Signed-off-by: Green Wan <green.wan@sifive.com> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > --- > drivers/net/macb.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > It looks this patch was never applied. Both U-Boot and Linux drivers do not have the "sifive,fu740-c000-gem" codes. Not sure what the current upstream status of this? Regards, Bin
diff --git a/drivers/net/macb.c b/drivers/net/macb.c index 57ea45e2dc..bf70525c54 100644 --- a/drivers/net/macb.c +++ b/drivers/net/macb.c @@ -591,8 +591,17 @@ static int macb_sifive_clk_init(struct udevice *dev, ulong rate) * 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 + * + * FU740 have a PLL HW quirk. The 125.125 Mhz is actually out of + * VSC8541XMV-02 specification. The tolerance level is +/-100ppm. + * Which means the range should be in between 125MHz +/-0.0125. + * But the most close value can be output by PLL is 125.125 MHz. */ - writel(rate != 125000000, gemgxl_regs); + if (device_is_compatible(dev, "sifive,fu540-c000-gem")) + writel(rate != 125000000, gemgxl_regs); + else if (device_is_compatible(dev, "sifive,fu740-c000-gem")) + writel(rate != 125125000, gemgxl_regs); + return 0; } @@ -1507,6 +1516,8 @@ static const struct udevice_id macb_eth_ids[] = { { .compatible = "cdns,zynq-gem" }, { .compatible = "sifive,fu540-c000-gem", .data = (ulong)&sifive_config }, + { .compatible = "sifive,fu740-c000-gem", + .data = (ulong)&sifive_config }, { .compatible = "microchip,mpfs-mss-gem", .data = (ulong)µchip_config }, { }