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

[Samuel Obuch <samuel.obuch@codasip.com>]

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
>
>
>