net: emaclite: Fixes, enable driver for other archs

net: emaclite: Fixes, enable driver for other archs

[Samuel Obuch]

These patches solve a few issues with the driver observed on a RISC-V platform.

[PATCH 1/3] net: emaclite: fix broken build

[Samuel Obuch]

Function ioremap_nocache seems to be defined only for mips and microblaze
architectures. Therefore, the function call in the emaclite driver causes
this driver to be unusable with other architectures, for example riscv.

Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
---
 drivers/net/xilinx_emaclite.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index 6c9f1f7c27..5cd88e04fe 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c
@@ -615,8 +615,12 @@ static int emaclite_of_to_plat(struct udevice *dev)
 	int offset = 0;
 
 	pdata->iobase = dev_read_addr(dev);
+#if defined(CONFIG_MICROBLAZE) || defined(CONFIG_MIPS)
 	emaclite->regs = (struct emaclite_regs *)ioremap_nocache(pdata->iobase,
 								 0x10000);
+#else
+	emaclite->regs = (struct emaclite_regs *)pdata->iobase;
+#endif
 
 	emaclite->phyaddr = -1;
 
-- 
2.31.1

[PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Samuel Obuch]

Use __raw_read* and __raw_write* functions to ensure read/write
is passed to the memory-mapped regions, as non-volatile accesses
may get optimised out.

Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
---
 drivers/net/xilinx_emaclite.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index 5cd88e04fe..de7a2dee0b 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c
@@ -113,12 +113,12 @@ static void xemaclite_alignedread(u32 *srcptr, void *destptr, u32 bytecount)
 	/* Word aligned buffer, no correction needed. */
 	to32ptr = (u32 *) destptr;
 	while (bytecount > 3) {
-		*to32ptr++ = *from32ptr++;
+		*to32ptr++ = __raw_readl(from32ptr++);
 		bytecount -= 4;
 	}
 	to8ptr = (u8 *) to32ptr;
 
-	alignbuffer = *from32ptr++;
+	alignbuffer = __raw_readl(from32ptr++);
 	from8ptr = (u8 *) &alignbuffer;
 
 	for (i = 0; i < bytecount; i++)
@@ -136,8 +136,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
 
 	from32ptr = (u32 *) srcptr;
 	while (bytecount > 3) {
-
-		*to32ptr++ = *from32ptr++;
+		__raw_writel(*from32ptr++, to32ptr++);
 		bytecount -= 4;
 	}
 
@@ -148,7 +147,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
 	for (i = 0; i < bytecount; i++)
 		*to8ptr++ = *from8ptr++;
 
-	*to32ptr++ = alignbuffer;
+	__raw_writel(alignbuffer, to32ptr++);
 }
 
 static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
-- 
2.31.1

[PATCH 3/3] net: emaclite: fix handling for IP packets with specific lengths

[Samuel Obuch]

The maximum length is capped similarly to the emaclite_send function.
Avoid integer underflow for values of ip->ip_len < 30, the minimum
length of an IP packet is 21 bytes.

Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
---
 drivers/net/xilinx_emaclite.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index de7a2dee0b..21c450ec46 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c
@@ -518,6 +518,8 @@ try_again:
 		length = ntohs(ip->ip_len);
 		length += ETHER_HDR_SIZE + ETH_FCS_LEN;
 		debug("IP Packet %x\n", length);
+		if (length > PKTSIZE)
+			length = PKTSIZE;
 		break;
 	default:
 		debug("Other Packet\n");
@@ -526,7 +528,7 @@ try_again:
 	}
 
 	/* Read the rest of the packet which is longer then first read */
-	if (length != first_read)
+	if (length > first_read)
 		xemaclite_alignedread(addr + first_read,
 				      etherrxbuff + first_read,
 				      length - first_read);
-- 
2.31.1

Re: [PATCH 1/3] net: emaclite: fix broken build

[Ramon Fried]

On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>
> Function ioremap_nocache seems to be defined only for mips and microblaze
> architectures. Therefore, the function call in the emaclite driver causes
> this driver to be unusable with other architectures, for example riscv.
>
> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
> ---
>  drivers/net/xilinx_emaclite.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
> index 6c9f1f7c27..5cd88e04fe 100644
> --- a/drivers/net/xilinx_emaclite.c
> +++ b/drivers/net/xilinx_emaclite.c
> @@ -615,8 +615,12 @@ static int emaclite_of_to_plat(struct udevice *dev)
>         int offset = 0;
>
>         pdata->iobase = dev_read_addr(dev);
> +#if defined(CONFIG_MICROBLAZE) || defined(CONFIG_MIPS)
>         emaclite->regs = (struct emaclite_regs *)ioremap_nocache(pdata->iobase,
>                                                                  0x10000);
> +#else
> +       emaclite->regs = (struct emaclite_regs *)pdata->iobase;
> +#endif
>
>         emaclite->phyaddr = -1;
>
> --
> 2.31.1
>
Hm...
Well, this isn't right,The right solution is to replace
ioremap_nocache() with ioremap().
This way it will work both for MIPS and other architectures.
I can do it myself, you can fix your patch. let me know.
Thanks,
Ramon.

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Ramon Fried]

On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>
> Use __raw_read* and __raw_write* functions to ensure read/write
> is passed to the memory-mapped regions, as non-volatile accesses
> may get optimised out.
>
> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
> ---
>  drivers/net/xilinx_emaclite.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
> index 5cd88e04fe..de7a2dee0b 100644
> --- a/drivers/net/xilinx_emaclite.c
> +++ b/drivers/net/xilinx_emaclite.c
> @@ -113,12 +113,12 @@ static void xemaclite_alignedread(u32 *srcptr, void *destptr, u32 bytecount)
>         /* Word aligned buffer, no correction needed. */
>         to32ptr = (u32 *) destptr;
>         while (bytecount > 3) {
> -               *to32ptr++ = *from32ptr++;
> +               *to32ptr++ = __raw_readl(from32ptr++);
>                 bytecount -= 4;
>         }
>         to8ptr = (u8 *) to32ptr;
>
> -       alignbuffer = *from32ptr++;
> +       alignbuffer = __raw_readl(from32ptr++);
>         from8ptr = (u8 *) &alignbuffer;
>
>         for (i = 0; i < bytecount; i++)
> @@ -136,8 +136,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
>
>         from32ptr = (u32 *) srcptr;
>         while (bytecount > 3) {
> -
> -               *to32ptr++ = *from32ptr++;
> +               __raw_writel(*from32ptr++, to32ptr++);
>                 bytecount -= 4;
>         }
>
> @@ -148,7 +147,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
>         for (i = 0; i < bytecount; i++)
>                 *to8ptr++ = *from8ptr++;
>
> -       *to32ptr++ = alignbuffer;
> +       __raw_writel(alignbuffer, to32ptr++);
>  }
>
>  static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
> --
> 2.31.1
>
I think that using readl/writel is fine, no need for raw_...

Re: [PATCH 3/3] net: emaclite: fix handling for IP packets with specific lengths

[Ramon Fried]

On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>
> The maximum length is capped similarly to the emaclite_send function.
> Avoid integer underflow for values of ip->ip_len < 30, the minimum
> length of an IP packet is 21 bytes.
>
> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
> ---
>  drivers/net/xilinx_emaclite.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
> index de7a2dee0b..21c450ec46 100644
> --- a/drivers/net/xilinx_emaclite.c
> +++ b/drivers/net/xilinx_emaclite.c
> @@ -518,6 +518,8 @@ try_again:
>                 length = ntohs(ip->ip_len);
>                 length += ETHER_HDR_SIZE + ETH_FCS_LEN;
>                 debug("IP Packet %x\n", length);
> +               if (length > PKTSIZE)
> +                       length = PKTSIZE;
>                 break;
>         default:
>                 debug("Other Packet\n");
> @@ -526,7 +528,7 @@ try_again:
>         }
>
>         /* Read the rest of the packet which is longer then first read */
> -       if (length != first_read)
> +       if (length > first_read)
>                 xemaclite_alignedread(addr + first_read,
>                                       etherrxbuff + first_read,
>                                       length - first_read);
> --
> 2.31.1
>
+ Michal

Re: [PATCH 1/3] net: emaclite: fix broken build

[Michal Simek]

On 8/6/22 19:31, Ramon Fried wrote:
> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>>
>> Function ioremap_nocache seems to be defined only for mips and microblaze
>> architectures. Therefore, the function call in the emaclite driver causes
>> this driver to be unusable with other architectures, for example riscv.
>>
>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
>> ---
>>   drivers/net/xilinx_emaclite.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
>> index 6c9f1f7c27..5cd88e04fe 100644
>> --- a/drivers/net/xilinx_emaclite.c
>> +++ b/drivers/net/xilinx_emaclite.c
>> @@ -615,8 +615,12 @@ static int emaclite_of_to_plat(struct udevice *dev)
>>          int offset = 0;
>>
>>          pdata->iobase = dev_read_addr(dev);
>> +#if defined(CONFIG_MICROBLAZE) || defined(CONFIG_MIPS)
>>          emaclite->regs = (struct emaclite_regs *)ioremap_nocache(pdata->iobase,
>>                                                                   0x10000);
>> +#else
>> +       emaclite->regs = (struct emaclite_regs *)pdata->iobase;
>> +#endif
>>
>>          emaclite->phyaddr = -1;
>>
>> --
>> 2.31.1
>>
> Hm...
> Well, this isn't right,The right solution is to replace
> ioremap_nocache() with ioremap().
> This way it will work both for MIPS and other architectures.
> I can do it myself, you can fix your patch. let me know.

Microblaze doesn't define it now. But I agree that using ioremap which has 
implicit nocache is the right way to go.
It means please create the first patch which creates ioremap for microblaze,
Then second to replace ioremap_nocache() in emaclite driver to ioremap. And 
third to remove ioremap_nocache from microblaze io.h.

Thanks,
Michal

Re: [PATCH 1/3] net: emaclite: fix broken build

[Michal Simek]

Hi,

On 8/8/22 09:35, Michal Simek wrote:
> 
> 
> On 8/6/22 19:31, Ramon Fried wrote:
>> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>>>
>>> Function ioremap_nocache seems to be defined only for mips and microblaze
>>> architectures. Therefore, the function call in the emaclite driver causes
>>> this driver to be unusable with other architectures, for example riscv.
>>>
>>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
>>> ---
>>>   drivers/net/xilinx_emaclite.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
>>> index 6c9f1f7c27..5cd88e04fe 100644
>>> --- a/drivers/net/xilinx_emaclite.c
>>> +++ b/drivers/net/xilinx_emaclite.c
>>> @@ -615,8 +615,12 @@ static int emaclite_of_to_plat(struct udevice *dev)
>>>          int offset = 0;
>>>
>>>          pdata->iobase = dev_read_addr(dev);
>>> +#if defined(CONFIG_MICROBLAZE) || defined(CONFIG_MIPS)
>>>          emaclite->regs = (struct emaclite_regs *)ioremap_nocache(pdata->iobase,
>>>                                                                   0x10000);
>>> +#else
>>> +       emaclite->regs = (struct emaclite_regs *)pdata->iobase;
>>> +#endif
>>>
>>>          emaclite->phyaddr = -1;
>>>
>>> -- 
>>> 2.31.1
>>>
>> Hm...
>> Well, this isn't right,The right solution is to replace
>> ioremap_nocache() with ioremap().
>> This way it will work both for MIPS and other architectures.
>> I can do it myself, you can fix your patch. let me know.
> 
> Microblaze doesn't define it now. But I agree that using ioremap which has 
> implicit nocache is the right way to go.
> It means please create the first patch which creates ioremap for microblaze,
> Then second to replace ioremap_nocache() in emaclite driver to ioremap. And 
> third to remove ioremap_nocache from microblaze io.h.

I did closer look when I looked at other patches. You should switch to 
linux/io.h which automatically create ioremap if not defined by architecture.

And ioremap_nocache for microblaze can be removed later.

diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
index 48aee77ab509..3299eefd999f 100644
--- a/drivers/net/xilinx_emaclite.c
+++ b/drivers/net/xilinx_emaclite.c
@@ -21,7 +21,7 @@
  #include <linux/delay.h>
  #include <linux/errno.h>
  #include <linux/kernel.h>
-#include <asm/io.h>
+#include <linux/io.h>

M

Re: [PATCH 3/3] net: emaclite: fix handling for IP packets with specific lengths

[Michal Simek]

On 8/6/22 19:35, Ramon Fried wrote:
> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>>
>> The maximum length is capped similarly to the emaclite_send function.
>> Avoid integer underflow for values of ip->ip_len < 30, the minimum
>> length of an IP packet is 21 bytes.
>>
>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
>> ---
>>   drivers/net/xilinx_emaclite.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
>> index de7a2dee0b..21c450ec46 100644
>> --- a/drivers/net/xilinx_emaclite.c
>> +++ b/drivers/net/xilinx_emaclite.c
>> @@ -518,6 +518,8 @@ try_again:
>>                  length = ntohs(ip->ip_len);
>>                  length += ETHER_HDR_SIZE + ETH_FCS_LEN;
>>                  debug("IP Packet %x\n", length);
>> +               if (length > PKTSIZE)
>> +                       length = PKTSIZE;
>>                  break;
>>          default:
>>                  debug("Other Packet\n");
>> @@ -526,7 +528,7 @@ try_again:
>>          }
>>
>>          /* Read the rest of the packet which is longer then first read */
>> -       if (length != first_read)
>> +       if (length > first_read)
>>                  xemaclite_alignedread(addr + first_read,
>>                                        etherrxbuff + first_read,
>>                                        length - first_read);
>> --
>> 2.31.1
>>
> + Michal

This looks good.
Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Michal Simek]

On 8/6/22 19:33, Ramon Fried wrote:
> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>>
>> Use __raw_read* and __raw_write* functions to ensure read/write
>> is passed to the memory-mapped regions, as non-volatile accesses
>> may get optimised out.
>>
>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
>> ---
>>   drivers/net/xilinx_emaclite.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
>> index 5cd88e04fe..de7a2dee0b 100644
>> --- a/drivers/net/xilinx_emaclite.c
>> +++ b/drivers/net/xilinx_emaclite.c
>> @@ -113,12 +113,12 @@ static void xemaclite_alignedread(u32 *srcptr, void *destptr, u32 bytecount)
>>          /* Word aligned buffer, no correction needed. */
>>          to32ptr = (u32 *) destptr;
>>          while (bytecount > 3) {
>> -               *to32ptr++ = *from32ptr++;
>> +               *to32ptr++ = __raw_readl(from32ptr++);
>>                  bytecount -= 4;
>>          }
>>          to8ptr = (u8 *) to32ptr;
>>
>> -       alignbuffer = *from32ptr++;
>> +       alignbuffer = __raw_readl(from32ptr++);
>>          from8ptr = (u8 *) &alignbuffer;
>>
>>          for (i = 0; i < bytecount; i++)
>> @@ -136,8 +136,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
>>
>>          from32ptr = (u32 *) srcptr;
>>          while (bytecount > 3) {
>> -
>> -               *to32ptr++ = *from32ptr++;
>> +               __raw_writel(*from32ptr++, to32ptr++);
>>                  bytecount -= 4;
>>          }
>>
>> @@ -148,7 +147,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
>>          for (i = 0; i < bytecount; i++)
>>                  *to8ptr++ = *from8ptr++;
>>
>> -       *to32ptr++ = alignbuffer;
>> +       __raw_writel(alignbuffer, to32ptr++);
>>   }
>>
>>   static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
>> --
>> 2.31.1
>>
> I think that using readl/writel is fine, no need for raw_...

For microblaze that should be fine but let me ask my team to rest it on ARM.
I think that __raw version are safer because this IP can also run on big endian 
systems and I think that was the reason why readl/writel wasn't used in past.

Thanks,
Michal

Re: net: emaclite: Fixes, enable driver for other archs

[Michal Simek]

Hi,

On 7/13/22 15:52, Samuel Obuch wrote:
> These patches solve a few issues with the driver observed on a RISC-V platform.

How does that system look like that you are using standard Xilinx IPs?
Do you use any other standard Xilinx IP like console?

Is it available somewhere that I can run it too?

Thanks,
Michal

Re: net: emaclite: Fixes, enable driver for other archs

[Samuel Obuch]

Hi,

its a soft-core in an FPGA. Other than the ethernet, I think we only use
Xilinx bridges.

Unfortunately theres no publicly available version.

S.

On Mon, Aug 8, 2022 at 10:08 AM Michal Simek <michal.simek@amd.com> wrote:

> Hi,
>
> On 7/13/22 15:52, Samuel Obuch wrote:
> > These patches solve a few issues with the driver observed on a RISC-V
> platform.
>
> How does that system look like that you are using standard Xilinx IPs?
> Do you use any other standard Xilinx IP like console?
>
> Is it available somewhere that I can run it too?
>
> Thanks,
> Michal
>

Re: net: emaclite: Fixes, enable driver for other archs

[Michal Simek]

Hi Samuel,

po 8. 8. 2022 v 10:34 odesílatel Samuel Obuch <samuel.obuch@codasip.com> napsal:
>
> Hi,
>
> its a soft-core in an FPGA. Other than the ethernet, I think we only use
> Xilinx bridges.
>
> Unfortunately theres no publicly available version.
>
> S.
>
> On Mon, Aug 8, 2022 at 10:08 AM Michal Simek <michal.simek@amd.com> wrote:
>
> > Hi,
> >
> > On 7/13/22 15:52, Samuel Obuch wrote:
> > > These patches solve a few issues with the driver observed on a RISC-V
> > platform.
> >
> > How does that system look like that you are using standard Xilinx IPs?
> > Do you use any other standard Xilinx IP like console?
> >
> > Is it available somewhere that I can run it too?


Are you going to send a new version of this patchset?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Jan Remes]

On Mon, Aug 8, 2022 at 10:05 AM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 8/6/22 19:33, Ramon Fried wrote:
> > On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
> >>
> >> Use __raw_read* and __raw_write* functions to ensure read/write
> >> is passed to the memory-mapped regions, as non-volatile accesses
> >> may get optimised out.
> >>
> >> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
> >> ---
> >>   drivers/net/xilinx_emaclite.c | 9 ++++-----
> >>   1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
> >> index 5cd88e04fe..de7a2dee0b 100644
> >> --- a/drivers/net/xilinx_emaclite.c
> >> +++ b/drivers/net/xilinx_emaclite.c
> >> @@ -113,12 +113,12 @@ static void xemaclite_alignedread(u32 *srcptr, void *destptr, u32 bytecount)
> >>          /* Word aligned buffer, no correction needed. */
> >>          to32ptr = (u32 *) destptr;
> >>          while (bytecount > 3) {
> >> -               *to32ptr++ = *from32ptr++;
> >> +               *to32ptr++ = __raw_readl(from32ptr++);
> >>                  bytecount -= 4;
> >>          }
> >>          to8ptr = (u8 *) to32ptr;
> >>
> >> -       alignbuffer = *from32ptr++;
> >> +       alignbuffer = __raw_readl(from32ptr++);
> >>          from8ptr = (u8 *) &alignbuffer;
> >>
> >>          for (i = 0; i < bytecount; i++)
> >> @@ -136,8 +136,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
> >>
> >>          from32ptr = (u32 *) srcptr;
> >>          while (bytecount > 3) {
> >> -
> >> -               *to32ptr++ = *from32ptr++;
> >> +               __raw_writel(*from32ptr++, to32ptr++);
> >>                  bytecount -= 4;
> >>          }
> >>
> >> @@ -148,7 +147,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
> >>          for (i = 0; i < bytecount; i++)
> >>                  *to8ptr++ = *from8ptr++;
> >>
> >> -       *to32ptr++ = alignbuffer;
> >> +       __raw_writel(alignbuffer, to32ptr++);
> >>   }
> >>
> >>   static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
> >> --
> >> 2.31.1
> >>
> > I think that using readl/writel is fine, no need for raw_...
>
> For microblaze that should be fine but let me ask my team to rest it on ARM.
> I think that __raw version are safer because this IP can also run on big endian
> systems and I think that was the reason why readl/writel wasn't used in past.
>
> Thanks,
> Michal

Hi Michal,

Do you have any new information on this? For the v2, it would be good
to have this resolved.

Thanks,
Jan

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Michal Simek]

On 9/19/22 19:03, Jan Remes wrote:
> On Mon, Aug 8, 2022 at 10:05 AM Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 8/6/22 19:33, Ramon Fried wrote:
>>> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com> wrote:
>>>>
>>>> Use __raw_read* and __raw_write* functions to ensure read/write
>>>> is passed to the memory-mapped regions, as non-volatile accesses
>>>> may get optimised out.
>>>>
>>>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
>>>> ---
>>>>    drivers/net/xilinx_emaclite.c | 9 ++++-----
>>>>    1 file changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
>>>> index 5cd88e04fe..de7a2dee0b 100644
>>>> --- a/drivers/net/xilinx_emaclite.c
>>>> +++ b/drivers/net/xilinx_emaclite.c
>>>> @@ -113,12 +113,12 @@ static void xemaclite_alignedread(u32 *srcptr, void *destptr, u32 bytecount)
>>>>           /* Word aligned buffer, no correction needed. */
>>>>           to32ptr = (u32 *) destptr;
>>>>           while (bytecount > 3) {
>>>> -               *to32ptr++ = *from32ptr++;
>>>> +               *to32ptr++ = __raw_readl(from32ptr++);
>>>>                   bytecount -= 4;
>>>>           }
>>>>           to8ptr = (u8 *) to32ptr;
>>>>
>>>> -       alignbuffer = *from32ptr++;
>>>> +       alignbuffer = __raw_readl(from32ptr++);
>>>>           from8ptr = (u8 *) &alignbuffer;
>>>>
>>>>           for (i = 0; i < bytecount; i++)
>>>> @@ -136,8 +136,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
>>>>
>>>>           from32ptr = (u32 *) srcptr;
>>>>           while (bytecount > 3) {
>>>> -
>>>> -               *to32ptr++ = *from32ptr++;
>>>> +               __raw_writel(*from32ptr++, to32ptr++);
>>>>                   bytecount -= 4;
>>>>           }
>>>>
>>>> @@ -148,7 +147,7 @@ static void xemaclite_alignedwrite(void *srcptr, u32 *destptr, u32 bytecount)
>>>>           for (i = 0; i < bytecount; i++)
>>>>                   *to8ptr++ = *from8ptr++;
>>>>
>>>> -       *to32ptr++ = alignbuffer;
>>>> +       __raw_writel(alignbuffer, to32ptr++);
>>>>    }
>>>>
>>>>    static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
>>>> --
>>>> 2.31.1
>>>>
>>> I think that using readl/writel is fine, no need for raw_...
>>
>> For microblaze that should be fine but let me ask my team to rest it on ARM.
>> I think that __raw version are safer because this IP can also run on big endian
>> systems and I think that was the reason why readl/writel wasn't used in past.
>>
>> Thanks,
>> Michal
> 
> Hi Michal,
> 
> Do you have any new information on this? For the v2, it would be good
> to have this resolved.

we are not testing emaclite on any ARM design. But in Linux you can find in this 
driver.

#ifdef __BIG_ENDIAN
#define xemaclite_readl         ioread32be
#define xemaclite_writel        iowrite32be
#else
#define xemaclite_readl         ioread32
#define xemaclite_writel        iowrite32
#endif

If you keep __raw variants it will ensure that native endian access is doing to 
be used.
On ARM IIRC readl/writel also have barriers. Origin patch is also using raw 
variant that's why I expect it is working on your system.

Thanks,
Michal

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Samuel Obuch]

Hi, I tested both versions to be sure, but the results are as can be
expected:

1. both __raw_readl/__raw_writel and readl/writel functions work ok on
riscv - only the original nonvolatile accesses were problematic
2. the additional barrier in readl/writel functions can introduce
noticeable slowdown - e.g. with our setup, download speed over tftp for the
same image decreased from 460 KiB/s to 118 KiB/s

Can we conclude to keep the raw versions in the patch?

Thanks,
Samuel

On Tue, Sep 20, 2022 at 4:23 PM Michal Simek <michal.simek@amd.com> wrote:

>
>
> On 9/19/22 19:03, Jan Remes wrote:
> > On Mon, Aug 8, 2022 at 10:05 AM Michal Simek <michal.simek@amd.com>
> wrote:
> >>
> >>
> >>
> >> On 8/6/22 19:33, Ramon Fried wrote:
> >>> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com>
> wrote:
> >>>>
> >>>> Use __raw_read* and __raw_write* functions to ensure read/write
> >>>> is passed to the memory-mapped regions, as non-volatile accesses
> >>>> may get optimised out.
> >>>>
> >>>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
> >>>> ---
> >>>>    drivers/net/xilinx_emaclite.c | 9 ++++-----
> >>>>    1 file changed, 4 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/xilinx_emaclite.c
> b/drivers/net/xilinx_emaclite.c
> >>>> index 5cd88e04fe..de7a2dee0b 100644
> >>>> --- a/drivers/net/xilinx_emaclite.c
> >>>> +++ b/drivers/net/xilinx_emaclite.c
> >>>> @@ -113,12 +113,12 @@ static void xemaclite_alignedread(u32 *srcptr,
> void *destptr, u32 bytecount)
> >>>>           /* Word aligned buffer, no correction needed. */
> >>>>           to32ptr = (u32 *) destptr;
> >>>>           while (bytecount > 3) {
> >>>> -               *to32ptr++ = *from32ptr++;
> >>>> +               *to32ptr++ = __raw_readl(from32ptr++);
> >>>>                   bytecount -= 4;
> >>>>           }
> >>>>           to8ptr = (u8 *) to32ptr;
> >>>>
> >>>> -       alignbuffer = *from32ptr++;
> >>>> +       alignbuffer = __raw_readl(from32ptr++);
> >>>>           from8ptr = (u8 *) &alignbuffer;
> >>>>
> >>>>           for (i = 0; i < bytecount; i++)
> >>>> @@ -136,8 +136,7 @@ static void xemaclite_alignedwrite(void *srcptr,
> u32 *destptr, u32 bytecount)
> >>>>
> >>>>           from32ptr = (u32 *) srcptr;
> >>>>           while (bytecount > 3) {
> >>>> -
> >>>> -               *to32ptr++ = *from32ptr++;
> >>>> +               __raw_writel(*from32ptr++, to32ptr++);
> >>>>                   bytecount -= 4;
> >>>>           }
> >>>>
> >>>> @@ -148,7 +147,7 @@ static void xemaclite_alignedwrite(void *srcptr,
> u32 *destptr, u32 bytecount)
> >>>>           for (i = 0; i < bytecount; i++)
> >>>>                   *to8ptr++ = *from8ptr++;
> >>>>
> >>>> -       *to32ptr++ = alignbuffer;
> >>>> +       __raw_writel(alignbuffer, to32ptr++);
> >>>>    }
> >>>>
> >>>>    static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
> >>>> --
> >>>> 2.31.1
> >>>>
> >>> I think that using readl/writel is fine, no need for raw_...
> >>
> >> For microblaze that should be fine but let me ask my team to rest it on
> ARM.
> >> I think that __raw version are safer because this IP can also run on
> big endian
> >> systems and I think that was the reason why readl/writel wasn't used in
> past.
> >>
> >> Thanks,
> >> Michal
> >
> > Hi Michal,
> >
> > Do you have any new information on this? For the v2, it would be good
> > to have this resolved.
>
> we are not testing emaclite on any ARM design. But in Linux you can find
> in this
> driver.
>
> #ifdef __BIG_ENDIAN
> #define xemaclite_readl         ioread32be
> #define xemaclite_writel        iowrite32be
> #else
> #define xemaclite_readl         ioread32
> #define xemaclite_writel        iowrite32
> #endif
>
> If you keep __raw variants it will ensure that native endian access is
> doing to
> be used.
> On ARM IIRC readl/writel also have barriers. Origin patch is also using
> raw
> variant that's why I expect it is working on your system.
>
> Thanks,
> Michal
>
>
>

Re: [PATCH 1/3] net: emaclite: fix broken build

[Samuel Obuch]

Hi, using linux/io.h and ioremap works well for us, thanks. I will update
the patch.

S

On Mon, Aug 8, 2022 at 9:44 AM Michal Simek <michal.simek@amd.com> wrote:

> Hi,
>
> On 8/8/22 09:35, Michal Simek wrote:
> >
> >
> > On 8/6/22 19:31, Ramon Fried wrote:
> >> On Wed, Jul 13, 2022 at 5:02 PM Samuel Obuch <samuel.obuch@codasip.com>
> wrote:
> >>>
> >>> Function ioremap_nocache seems to be defined only for mips and
> microblaze
> >>> architectures. Therefore, the function call in the emaclite driver
> causes
> >>> this driver to be unusable with other architectures, for example riscv.
> >>>
> >>> Signed-off-by: Samuel Obuch <samuel.obuch@codasip.com>
> >>> ---
> >>>   drivers/net/xilinx_emaclite.c | 4 ++++
> >>>   1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/net/xilinx_emaclite.c
> b/drivers/net/xilinx_emaclite.c
> >>> index 6c9f1f7c27..5cd88e04fe 100644
> >>> --- a/drivers/net/xilinx_emaclite.c
> >>> +++ b/drivers/net/xilinx_emaclite.c
> >>> @@ -615,8 +615,12 @@ static int emaclite_of_to_plat(struct udevice
> *dev)
> >>>          int offset = 0;
> >>>
> >>>          pdata->iobase = dev_read_addr(dev);
> >>> +#if defined(CONFIG_MICROBLAZE) || defined(CONFIG_MIPS)
> >>>          emaclite->regs = (struct emaclite_regs
> *)ioremap_nocache(pdata->iobase,
> >>>
> 0x10000);
> >>> +#else
> >>> +       emaclite->regs = (struct emaclite_regs *)pdata->iobase;
> >>> +#endif
> >>>
> >>>          emaclite->phyaddr = -1;
> >>>
> >>> --
> >>> 2.31.1
> >>>
> >> Hm...
> >> Well, this isn't right,The right solution is to replace
> >> ioremap_nocache() with ioremap().
> >> This way it will work both for MIPS and other architectures.
> >> I can do it myself, you can fix your patch. let me know.
> >
> > Microblaze doesn't define it now. But I agree that using ioremap which
> has
> > implicit nocache is the right way to go.
> > It means please create the first patch which creates ioremap for
> microblaze,
> > Then second to replace ioremap_nocache() in emaclite driver to ioremap.
> And
> > third to remove ioremap_nocache from microblaze io.h.
>
> I did closer look when I looked at other patches. You should switch to
> linux/io.h which automatically create ioremap if not defined by
> architecture.
>
> And ioremap_nocache for microblaze can be removed later.
>
> diff --git a/drivers/net/xilinx_emaclite.c b/drivers/net/xilinx_emaclite.c
> index 48aee77ab509..3299eefd999f 100644
> --- a/drivers/net/xilinx_emaclite.c
> +++ b/drivers/net/xilinx_emaclite.c
> @@ -21,7 +21,7 @@
>   #include <linux/delay.h>
>   #include <linux/errno.h>
>   #include <linux/kernel.h>
> -#include <asm/io.h>
> +#include <linux/io.h>
>
> M
>

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Michal Simek]

Hi,

On 9/23/22 11:17, Samuel Obuch wrote:
> Hi, I tested both versions to be sure, but the results are as can be expected:
> 
> 1. both __raw_readl/__raw_writel and readl/writel functions work ok on riscv - 
> only the original nonvolatile accesses were problematic
> 2. the additional barrier in readl/writel functions can introduce noticeable 
> slowdown - e.g. with our setup, download speed over tftp for the same image 
> decreased from 460 KiB/s to 118 KiB/s
> 
> Can we conclude to keep the raw versions in the patch?

I prefer raw versions too.

M

Re: [PATCH 2/3] net: emaclite: fix xemaclite_alignedread/write functions

[Ramon Fried]

On Fri, Sep 23, 2022 at 12:36 PM Michal Simek <michal.simek@amd.com> wrote:
>
> Hi,
>
> On 9/23/22 11:17, Samuel Obuch wrote:
> > Hi, I tested both versions to be sure, but the results are as can be expected:
> >
> > 1. both __raw_readl/__raw_writel and readl/writel functions work ok on riscv -
> > only the original nonvolatile accesses were problematic
> > 2. the additional barrier in readl/writel functions can introduce noticeable
> > slowdown - e.g. with our setup, download speed over tftp for the same image
> > decreased from 460 KiB/s to 118 KiB/s
> >
> > Can we conclude to keep the raw versions in the patch?
>
> I prefer raw versions too.
>
>
Let's keep it raw.