| Message ID | 20211022085648.134655-5-padmarao.begari@microchip.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | Update Microchip PolarFire SoC support | expand |
On Fri, Oct 22, 2021 at 02:26:47PM +0530, Padmarao Begari wrote: > Update compatible as per Microchip PolarFire SoC ethernet > device node. > > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> > --- > drivers/net/macb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>
On Fri, Oct 22, 2021 at 4:58 PM Padmarao Begari <padmarao.begari@microchip.com> wrote: > > Update compatible as per Microchip PolarFire SoC ethernet > device node. > > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> > --- > drivers/net/macb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > index 8c6461e717..1b867bd5c2 100644 > --- a/drivers/net/macb.c > +++ b/drivers/net/macb.c > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = { > { .compatible = "cdns,zynq-gem" }, > { .compatible = "sifive,fu540-c000-gem", > .data = (ulong)&sifive_config }, > - { .compatible = "microchip,mpfs-mss-gem", > + { .compatible = "microchip,mpfs-gem", Could you please provide the upstream Linux kernel binding reference? I can't find such string in the Linux kernel. > .data = (ulong)µchip_config }, > { } > }; Regards, Bin
Hi Bin, On Mon, Nov 1, 2021 at 2:15 PM Bin Meng <bmeng.cn@gmail.com> wrote: > On Fri, Oct 22, 2021 at 4:58 PM Padmarao Begari > <padmarao.begari@microchip.com> wrote: > > > > Update compatible as per Microchip PolarFire SoC ethernet > > device node. > > > > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> > > --- > > drivers/net/macb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > > index 8c6461e717..1b867bd5c2 100644 > > --- a/drivers/net/macb.c > > +++ b/drivers/net/macb.c > > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = { > > { .compatible = "cdns,zynq-gem" }, > > { .compatible = "sifive,fu540-c000-gem", > > .data = (ulong)&sifive_config }, > > - { .compatible = "microchip,mpfs-mss-gem", > > + { .compatible = "microchip,mpfs-gem", > > Could you please provide the upstream Linux kernel binding reference? > I can't find such string in the Linux kernel. > > We are not upstreamed Linux bindings yet, soon we will do. The compatible "cdns,macb" is used in Linux for 32-bit and 64-bit DMA transfer and U-Boot for 32-bit DMA transfer. We added this string to support 64-bit DMA transfer of the GEM. Regards Padmarao > > .data = (ulong)µchip_config }, > > { } > > }; > > Regards, > Bin >
Hi Padmarao, On Tue, Nov 2, 2021 at 7:03 PM Padmarao Begari <padmarao.b@gmail.com> wrote: > > Hi Bin, > > On Mon, Nov 1, 2021 at 2:15 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> >> On Fri, Oct 22, 2021 at 4:58 PM Padmarao Begari >> <padmarao.begari@microchip.com> wrote: >> > >> > Update compatible as per Microchip PolarFire SoC ethernet >> > device node. >> > >> > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> >> > --- >> > drivers/net/macb.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c >> > index 8c6461e717..1b867bd5c2 100644 >> > --- a/drivers/net/macb.c >> > +++ b/drivers/net/macb.c >> > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = { >> > { .compatible = "cdns,zynq-gem" }, >> > { .compatible = "sifive,fu540-c000-gem", >> > .data = (ulong)&sifive_config }, >> > - { .compatible = "microchip,mpfs-mss-gem", >> > + { .compatible = "microchip,mpfs-gem", >> >> Could you please provide the upstream Linux kernel binding reference? >> I can't find such string in the Linux kernel. >> > > We are not upstreamed Linux bindings yet, soon we will do. > > The compatible "cdns,macb" is used in Linux for 32-bit and 64-bit DMA transfer and U-Boot for 32-bit DMA transfer. > We added this string to support 64-bit DMA transfer of the GEM. > I suggest we upstream the new compatible string binding first, then update U-Boot. Otherwise U-Boot might be updated again if the compatible string is changed during the upstream review process. Regards, Bin
Hi Bin, On Tue, Nov 2, 2021 at 6:16 PM Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Padmarao, > > On Tue, Nov 2, 2021 at 7:03 PM Padmarao Begari <padmarao.b@gmail.com> > wrote: > > > > Hi Bin, > > > > On Mon, Nov 1, 2021 at 2:15 PM Bin Meng <bmeng.cn@gmail.com> wrote: > >> > >> On Fri, Oct 22, 2021 at 4:58 PM Padmarao Begari > >> <padmarao.begari@microchip.com> wrote: > >> > > >> > Update compatible as per Microchip PolarFire SoC ethernet > >> > device node. > >> > > >> > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> > >> > --- > >> > drivers/net/macb.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > >> > index 8c6461e717..1b867bd5c2 100644 > >> > --- a/drivers/net/macb.c > >> > +++ b/drivers/net/macb.c > >> > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = > { > >> > { .compatible = "cdns,zynq-gem" }, > >> > { .compatible = "sifive,fu540-c000-gem", > >> > .data = (ulong)&sifive_config }, > >> > - { .compatible = "microchip,mpfs-mss-gem", > >> > + { .compatible = "microchip,mpfs-gem", > >> > >> Could you please provide the upstream Linux kernel binding reference? > >> I can't find such string in the Linux kernel. > >> > > > > We are not upstreamed Linux bindings yet, soon we will do. > > > > The compatible "cdns,macb" is used in Linux for 32-bit and 64-bit DMA > transfer and U-Boot for 32-bit DMA transfer. > > We added this string to support 64-bit DMA transfer of the GEM. > > > > I suggest we upstream the new compatible string binding first, then > update U-Boot. Otherwise U-Boot might be updated again if the > compatible string is changed during the upstream review process. > > We are going to use the same compatible strings in the U-Boot and Linux for PolarFire SoC peripherals like "microchip,mpfs-xxx" (xxx means peripheral name, ex. "microchip,mpfs-i2c", "microchip,mpfs-rtc"....etc). Here, we are updating compatible(existing) "microchip,mpfs-mss-gem" with "microchip,mpfs-gem". Regards, > Bin >
Hi Bin, On Wed, Nov 3, 2021 at 5:17 PM Padmarao Begari <padmarao.b@gmail.com> wrote: > Hi Bin, > > On Tue, Nov 2, 2021 at 6:16 PM Bin Meng <bmeng.cn@gmail.com> wrote: > >> Hi Padmarao, >> >> On Tue, Nov 2, 2021 at 7:03 PM Padmarao Begari <padmarao.b@gmail.com> >> wrote: >> > >> > Hi Bin, >> > >> > On Mon, Nov 1, 2021 at 2:15 PM Bin Meng <bmeng.cn@gmail.com> wrote: >> >> >> >> On Fri, Oct 22, 2021 at 4:58 PM Padmarao Begari >> >> <padmarao.begari@microchip.com> wrote: >> >> > >> >> > Update compatible as per Microchip PolarFire SoC ethernet >> >> > device node. >> >> > >> >> > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> >> >> > --- >> >> > drivers/net/macb.c | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c >> >> > index 8c6461e717..1b867bd5c2 100644 >> >> > --- a/drivers/net/macb.c >> >> > +++ b/drivers/net/macb.c >> >> > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] >> = { >> >> > { .compatible = "cdns,zynq-gem" }, >> >> > { .compatible = "sifive,fu540-c000-gem", >> >> > .data = (ulong)&sifive_config }, >> >> > - { .compatible = "microchip,mpfs-mss-gem", >> >> > + { .compatible = "microchip,mpfs-gem", >> >> >> >> Could you please provide the upstream Linux kernel binding reference? >> >> I can't find such string in the Linux kernel. >> >> >> > >> > We are not upstreamed Linux bindings yet, soon we will do. >> > >> > The compatible "cdns,macb" is used in Linux for 32-bit and 64-bit DMA >> transfer and U-Boot for 32-bit DMA transfer. >> > We added this string to support 64-bit DMA transfer of the GEM. >> > >> >> I suggest we upstream the new compatible string binding first, then >> update U-Boot. Otherwise U-Boot might be updated again if the >> compatible string is changed during the upstream review process. >> >> > We are going to use the same compatible strings in the U-Boot and Linux > for PolarFire SoC peripherals like "microchip,mpfs-xxx" (xxx means > peripheral name, ex. "microchip,mpfs-i2c", "microchip,mpfs-rtc"....etc). > Here, we are updating compatible(existing) "microchip,mpfs-mss-gem" with > "microchip,mpfs-gem". > > The U-Boot MACB driver code updated to support 64-bit DMA access using the compatible string "microchip,mpfs-mss-gem" last time and the compatible string "microchip,mpfs-mss-gem" is only for U-Boot for 64-bit DMA access of MACB not for Linux, the Linux works fine with compatible string "cdns,macb" and there is no change in the driver code. Still do we need to upstream Linux kernel bindings for this? Regards Padmarao > Regards, >> Bin >> >
Hi Padmarao, On Thu, Nov 11, 2021 at 2:11 PM <Padmarao.Begari@microchip.com> wrote: > > Hi Bin, > > > > Do we need to upstream Linux kernel bindings for Microchip MACB compatible if there is no change in Linux MACB driver? > > Are the Linux maintainers can approve this? Because the changes only in U-Boot not Linux. > If Linux driver does not need to be updated to support MPFS macb using existing compatible string but U-Boot driver has to, something is wrong on the U-Boot macb driver side. Would you please reconsider the whole changes? Regards, Bin
Hi Bin, On Thu, Nov 11, 2021 at 1:37 PM Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Padmarao, > > On Thu, Nov 11, 2021 at 2:11 PM <Padmarao.Begari@microchip.com> wrote: > > > > Hi Bin, > > > > > > > > Do we need to upstream Linux kernel bindings for Microchip MACB > compatible if there is no change in Linux MACB driver? > > > > Are the Linux maintainers can approve this? Because the changes only in > U-Boot not Linux. > > > > If Linux driver does not need to be updated to support MPFS macb using > existing compatible string but U-Boot driver has to, something is > wrong on the U-Boot macb driver side. > > Would you please reconsider the whole changes? > > We submitted patches(v1, v2) last year for the U-Boot MACB update for 64-bit DMA access same like Linux MACB driver using "#ifdef CONFIG_DMA_ADDR_T_64BIT" but one of the reviewer wanted to check 64-bit DMA support at runtime instead of #ifdef and we updated the macb driver based on the design config debug6 register of MACB hardware which supports 32-bit or 64-bit DMA in patch(v3) but the SiFive FU540 MACB didn't work then the reviewer suggested use compatible string instead of design config register and updated same in patch(v4), these changes were tested and acknowledged them at Patch v6. Below links for patch submitted for "net: macb: Add DMA 64-bit address support for macb" https://www.mail-archive.com/u-boot@lists.denx.de/msg387528.html - Patch v2 https://www.mail-archive.com/u-boot@lists.denx.de/msg389490.html - Patch v3 https://www.mail-archive.com/u-boot@lists.denx.de/msg390611.html - Patch v4 https://www.mail-archive.com/u-boot@lists.denx.de/msg391001.html - Patch v5 https://www.mail-archive.com/u-boot@lists.denx.de/msg391850.html - Patch v6 Regards Padmarao Regards, > Bin >
Hi, not wanting to hijack this too much, but does the mac driver also need some sort of clock handling? Because on the Icicle I have here, I'm running into "TX timeout" errors: RISC-V # dhcp ethernet@20112000: PHY present at 9 ethernet@20112000: Starting autonegotiation... ethernet@20112000: Autonegotiation complete ethernet@20112000: link up, 1000Mbps full-duplex (lpa: 0x3800) BOOTP broadcast 1 ethernet@20112000: TX timeout BOOTP broadcast 2 ethernet@20112000: TX timeout BOOTP broadcast 3 ethernet@20112000: TX timeout BOOTP broadcast 4 ethernet@20112000: TX timeout The sifive variant of the macb distinguishes between speeds in its cllk_init callback, so I guess the Icicle might need that as well? Thanks Heiko Am Freitag, 22. Oktober 2021, 10:56:47 CET schrieb Padmarao Begari: > Update compatible as per Microchip PolarFire SoC ethernet > device node. > > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> > --- > drivers/net/macb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > index 8c6461e717..1b867bd5c2 100644 > --- a/drivers/net/macb.c > +++ b/drivers/net/macb.c > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = { > { .compatible = "cdns,zynq-gem" }, > { .compatible = "sifive,fu540-c000-gem", > .data = (ulong)&sifive_config }, > - { .compatible = "microchip,mpfs-mss-gem", > + { .compatible = "microchip,mpfs-gem", > .data = (ulong)µchip_config }, > { } > }; >
> > If Linux driver does not need to be updated to support MPFS macb using > > existing compatible string but U-Boot driver has to, something is > > wrong on the U-Boot macb driver side. > > > > Would you please reconsider the whole changes? > > > We submitted patches(v1, v2) last year for the U-Boot MACB update for > 64-bit DMA access same like Linux MACB driver using "#ifdef > CONFIG_DMA_ADDR_T_64BIT" but one of the reviewer wanted to check 64-bit DMA > support at runtime instead of #ifdef and we updated the macb driver based > on the design config debug6 register of MACB hardware which supports 32-bit > or 64-bit DMA in patch(v3) but the SiFive FU540 MACB didn't work then the > reviewer suggested use compatible string instead of design config register > and updated same in patch(v4), these changes were tested and acknowledged > them at Patch v6. I agree with Bin here. You shouldn't introduce a new compatible just for u-boot. If you need one, please to it first in linux and get an ACK there. Or at least there should be patches for it pending in linux and it should be likely, that they will be accepted. Please work towards having one binding for u-boot and linux. -michael
> I agree with Bin here. You shouldn't introduce a new compatible just for > u-boot. If you need one, please to it first in linux and get an ACK there. > Or at least there should be patches for it pending in linux and it should > be likely, that they will be accepted. > > Please work towards having one binding for u-boot and linux. > > -michael I think both Michael and Bin are right, but that maybe this has gone circular. IIRC, Linux *doesn't need* any extra bindings because its driver already supports 64-bit DMA. Padmarao's original patch added equivalent 64-bit functionality to the driver in U-Boot, but this was rejected. Instead I think the suggestion was to add a device-tree binding to choose 32 or 64-bit DMA... however, there is no reasonably way of upstreaming this into the Linux device-tree, as Linux doesn't need it... so he is left in a Catch-22. A way forward may be to go back to his original approach and get the U-Boot driver functionality updated so that it works similarly to the Linux driver (and thus can use the same device-tree stanza)?
On 11/11/2021 12:54, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> If Linux driver does not need to be updated to support MPFS macb using >>> existing compatible string but U-Boot driver has to, something is >>> wrong on the U-Boot macb driver side. >>> >>> Would you please reconsider the whole changes? >>> >> We submitted patches(v1, v2) last year for the U-Boot MACB update for >> 64-bit DMA access same like Linux MACB driver using "#ifdef >> CONFIG_DMA_ADDR_T_64BIT" but one of the reviewer wanted to check 64-bit DMA >> support at runtime instead of #ifdef and we updated the macb driver based >> on the design config debug6 register of MACB hardware which supports 32-bit >> or 64-bit DMA in patch(v3) but the SiFive FU540 MACB didn't work then the >> reviewer suggested use compatible string instead of design config register >> and updated same in patch(v4), these changes were tested and acknowledged >> them at Patch v6. > > I agree with Bin here. You shouldn't introduce a new compatible just for > u-boot. If you need one, please to it first in linux and get an ACK there. > Or at least there should be patches for it pending in linux and it should > be likely, that they will be accepted. > > Please work towards having one binding for u-boot and linux. > > -michael > I think the point that Padmarao is trying to make is that we don't need a new compatible for 64-bit DMA in the linux macb driver - we just use "cdns,macb" and enable CONFIG_ARCH_DMA_ADDR_T_64BIT. Padmarao previously submitted patches which would have introduced the same behaviour to u-boot, but after review was told to implement it using a compatible string specific to our board rather than copying the linux approach. Introducing that compatible string in linux would just be creating a superfluous binding, no? Conor.
On Thu, Nov 11, 2021 at 9:17 PM <Ivan.Griffin@microchip.com> wrote: > > > I agree with Bin here. You shouldn't introduce a new compatible just for > > u-boot. If you need one, please to it first in linux and get an ACK there. > > Or at least there should be patches for it pending in linux and it should > > be likely, that they will be accepted. > > > > Please work towards having one binding for u-boot and linux. > > > > -michael > > I think both Michael and Bin are right, but that maybe this has gone circular. > > IIRC, Linux *doesn't need* any extra bindings because its driver already > supports 64-bit DMA. > > Padmarao's original patch added equivalent 64-bit functionality to the > driver in U-Boot, but this was rejected. > I am not sure why it was rejected. Is that because it breaks some other platforms? > Instead I think the suggestion was to add a device-tree binding to choose 32 or > 64-bit DMA... however, there is no reasonably way of upstreaming this into > the Linux device-tree, as Linux doesn't need it... so he is left in a Catch-22. > > A way forward may be to go back to his original approach and get the U-Boot > driver functionality updated so that it works similarly to the Linux driver > (and thus can use the same device-tree stanza)? Let's go back to the original approach and see what happens. Regards, Bin
Hi Bin, On Fri, Nov 12, 2021 at 6:58 AM Bin Meng <bmeng.cn@gmail.com> wrote: > On Thu, Nov 11, 2021 at 9:17 PM <Ivan.Griffin@microchip.com> wrote: > > > > > I agree with Bin here. You shouldn't introduce a new compatible just > for > > > u-boot. If you need one, please to it first in linux and get an ACK > there. > > > Or at least there should be patches for it pending in linux and it > should > > > be likely, that they will be accepted. > > > > > > Please work towards having one binding for u-boot and linux. > > > > > > -michael > > > > I think both Michael and Bin are right, but that maybe this has gone > circular. > > > > IIRC, Linux *doesn't need* any extra bindings because its driver already > > supports 64-bit DMA. > > > > Padmarao's original patch added equivalent 64-bit functionality to the > > driver in U-Boot, but this was rejected. > > > > I am not sure why it was rejected. Is that because it breaks some > other platforms? > > No > > Instead I think the suggestion was to add a device-tree binding to > choose 32 or > > 64-bit DMA... however, there is no reasonably way of upstreaming this > into > > the Linux device-tree, as Linux doesn't need it... so he is left in a > Catch-22. > > > > A way forward may be to go back to his original approach and get the > U-Boot > > driver functionality updated so that it works similarly to the Linux > driver > > (and thus can use the same device-tree stanza)? > > Let's go back to the original approach and see what happens. > > Ok, I will do that and submit it in Patch v2. Regards Padmarao > Regards, > Bin >
Hi, Am Donnerstag, 11. November 2021, 10:41:46 CET schrieb Heiko Stübner: > not wanting to hijack this too much, but does the mac driver also need > some sort of clock handling? > > Because on the Icicle I have here, I'm running into "TX timeout" errors: > > RISC-V # dhcp > ethernet@20112000: PHY present at 9 > ethernet@20112000: Starting autonegotiation... > ethernet@20112000: Autonegotiation complete > ethernet@20112000: link up, 1000Mbps full-duplex (lpa: 0x3800) > BOOTP broadcast 1 > ethernet@20112000: TX timeout > BOOTP broadcast 2 > ethernet@20112000: TX timeout > BOOTP broadcast 3 > ethernet@20112000: TX timeout > BOOTP broadcast 4 > ethernet@20112000: TX timeout > > The sifive variant of the macb distinguishes between speeds in its > cllk_init callback, so I guess the Icicle might need that as well? just for "the archive", i.e. when people read this in the future: I got this solved by updating the HSS to a recent release (2021.11 in this case). So now I do have working network in u-boot. Heiko > Am Freitag, 22. Oktober 2021, 10:56:47 CET schrieb Padmarao Begari: > > Update compatible as per Microchip PolarFire SoC ethernet > > device node. > > > > Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> > > --- > > drivers/net/macb.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/macb.c b/drivers/net/macb.c > > index 8c6461e717..1b867bd5c2 100644 > > --- a/drivers/net/macb.c > > +++ b/drivers/net/macb.c > > @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = { > > { .compatible = "cdns,zynq-gem" }, > > { .compatible = "sifive,fu540-c000-gem", > > .data = (ulong)&sifive_config }, > > - { .compatible = "microchip,mpfs-mss-gem", > > + { .compatible = "microchip,mpfs-gem", > > .data = (ulong)µchip_config }, > > { } > > }; > > > >
diff --git a/drivers/net/macb.c b/drivers/net/macb.c index 8c6461e717..1b867bd5c2 100644 --- a/drivers/net/macb.c +++ b/drivers/net/macb.c @@ -1502,7 +1502,7 @@ static const struct udevice_id macb_eth_ids[] = { { .compatible = "cdns,zynq-gem" }, { .compatible = "sifive,fu540-c000-gem", .data = (ulong)&sifive_config }, - { .compatible = "microchip,mpfs-mss-gem", + { .compatible = "microchip,mpfs-gem", .data = (ulong)µchip_config }, { } };
Update compatible as per Microchip PolarFire SoC ethernet device node. Signed-off-by: Padmarao Begari <padmarao.begari@microchip.com> --- drivers/net/macb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)