Message-ID: <67f0651e-3c6e-5ff6-0913-6b193b581764@microchip.com>
Date:   Thu, 8 Sep 2022 21:44:43 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <prabhakar.mahadev-lad.rj@...renesas.com>,
        <paul.walmsley@...ive.com>, <palmer@...belt.com>,
        <aou@...s.berkeley.edu>
CC:     <atishp@...osinc.com>, <apatel@...tanamicro.com>,
        <geert+renesas@...der.be>, <linux-riscv@...ts.infradead.org>,
        <linux-renesas-soc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <prabhakar.csengg@...il.com>,
        <biju.das.jz@...renesas.com>, <heiko@...ech.de>
Subject: Re: [RFC PATCH 0/2] AX45MP: Add support to non-coherent DMA

Hey,

I had a quick run through this today so if there's discussion
about this next week I at least will have some idea of what I
am talking about...

(I ended up not doing a quick run...)

On 06/09/2022 11:21, Lad Prabhakar wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi All,
> 
> On the Andes AX45MP core, cache coherency is a specification option so it
> may not be supported. In this case DMA will fail. To get around with this
> issue this patch series  does the below:

You say "may not be supported" - is it or is it not supported by the
core on your SoC? Do some of the cheaper SKUs not support it?

From what Biju has said, you need non-coherent DMA for your eMMC, USB
and ethernet controllers to work? To me, that seems like something that
would be quite important to point out here..


> Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> block that allows dynamic adjustment of memory attributes in the runtime.
> It contains a configurable amount of PMA entries implemented as CSR
> registers to control the attributes of memory locations in interest. PMA
> regions are passed from the cpu core node which are configured as
> non-cacheable and non-bufferable with the SBI call.
> 
>         ax45mp: cpu@0 {
>                 compatible = "andestech,ax45mp", "riscv";
>                 device_type = "cpu";
>                 ....
>                 pma-regions = <0x0 0x00000000 0x0 0x14000000>,
>                               <0x0 0x20000000 0x0 0x10000000>,
>                               <0x0 0x58000000 0x0 0x08000000>;
>                 ....
>         };
> 
> We provide callbacks to synchronize specific content between memory and
> cache. We allocate a global DMA coherent pool (which is marked as
> non-cached using PMA) so that DMA memory allocations happens from this
> pool and we implement the below callbacks:
> 
> - arch_sync_dma_for_device()
> - arch_sync_dma_for_cpu()

These two already exist in arch/riscv/mm/dma-noncoherent.c using the
alternatives mechanism.

> - arch_dma_alloc()
> - arch_dma_free()
> 
> Below are the configs that are enabled:
> 
> - DMA_GLOBAL_POOL
> - ARCH_HAS_SYNC_DMA_FOR_CPU
> - ARCH_HAS_SYNC_DMA_FOR_DEVICE

For these two see:
arch/riscv/Kconfig & RISCV_DMA_NONCOHERENT

> 
>         l2cache: cache-controller@...00000 {
>                 compatible = "andestech,ax45mp-cache", "cache";
>                 cache-size = <0x40000>;
>                 cache-line-size = <64>;
>                 cache-sets = <1024>;
>                 cache-unified;
>                 reg = <0x0 0x13400000 0x0 0x100000>;
>         };
> 
> Due to the above approach custom SBI calls have been implemented. The
> above implementation is in preparation for adding support for Renesas
> RZ/Five SoC which uses the AX45MP core. As with the above approach the
> kernel image might not be generic so that it can be used on other
> platforms, so sending it as an RFC (without DT binding patches).
> 
> OpenSBI implementation isn't upstreamed yet, public repo for access is
> available at [0].

When you say "isn't upstreamed yet", what is the actual status? Where in
the process are you or have you not started that yet? Does openSBI even
allow custom extensions to be upstreamed?

> 
> [0] https://github.com/renesas-rz/rz_opensbi/tree/work/OpenSBI-PMA
> 
> Cheers,
> Prabhakar
> 
> Lad Prabhakar (2):
>   riscv: vendors: andes: Add support to configure the PMA regions
>   riscv: vendors: andes: Add support for non-cohernet dma
> 

Anyway, a couple of drive-by comments, having made the wild assumption
that this can be accepted upstream.

>  arch/riscv/Kbuild                             |   2 +
>  arch/riscv/include/asm/sbi.h                  |   1 +
>  arch/riscv/vendors/Makefile                   |   3 +
>  arch/riscv/vendors/andes/Makefile             |   4 +
>  arch/riscv/vendors/andes/ax45mp_cache.c       | 296 ++++++++++++++++++

Surely this should be in drivers/soc/andestech, just like the SiFive L2
controller is in drivers/soc/sifive rather in a subdirectory of the
arch?

>  arch/riscv/vendors/andes/ax45mp_nocache_dma.c |  65 ++++

This looks like it should be implemented as errata/alternatives just
like the non-coherent stuff on the D1 is done.

>  arch/riscv/vendors/andes/include/proc.h       |   9 +

And I think this would fall away if implemented as errata/alternatives.

>  arch/riscv/vendors/andes/include/sbi.h        |  27 ++
>  arch/riscv/vendors/andes/ax45mp.c             |  93 ++++++

idk where this would go though, if it is even something that is
acceptable, given the policy I linked the other day from:
https://www.kernel.org/doc/html/latest/riscv/patch-acceptance.html#submit-checklist-addendum

There is SiFive specific errata but it is implemented using mimpid etc
rather than compatible/dt. As I said in my initial mails, I am quite
interested in vendor SBI extensions in the kernel. If you did check out
the link I sent, our stuff is a world away from yours - it's isolated to
a driver where we are using SBI ECALLs to communicate with other harts
which are running something other than Linux in AMP configurations.
Pretty much we can do everything we want to do without touching a
single line of code in arch/riscv, so although the statement in that doc
applies to both of us here it does not apply evenly :s

It's all a bit unclear to me what the story is here, because obviously
you are doing things that Zicbo* is meant to do (just like the D1), but
your hardware's design and initial tapeout predates the existance of
Zicbom. Makes me start to wonder, what happens for <insert idea> that
eventually becomes an extension? Where does the line get draw for "you
did something that is not a ratified extension, therefore you are not
permitted upstream"? A line obviously does have to be drawn *somewhere*
and the easiest place to draw that line is "non ratified extensions are
a no-go". But then again, why allow the D1 but not you?

Obviously this is not a runner for someone not using an FPGA or similar,
but the InterHart Communication IP that we are using the SBI ECALLs for
is a fabric core, so we (in theory) could re-write it so that instead of
using an ECALL which routes communication via the e51 "monitor" core we
_could_ write directly to the registers of the IHC block. There's clear
security/isolation benefits for doing things via an ECALL which is why
that method was chosen but if we opened for the direct writes/reads the
driver would be upstream acceptable...

Don't get me wrong, I completely understand why a policy of not allowing
extensions that have not been ratified makes sense - *but* at the same
time if touching arch code is not required it does not feel very much
different to me than adding a driver for a fabric core in the first
place. I mentioned this sort of thing a while back on IRC and Jess made
the point that similar sorts of things are done by some of the Qualcomm
for their remoteproc as we would be doing for ours with the IHC. In your
case, if all of your ECALLs are in drivers/soc - the maintainership
burden for any churn would be on you/Geert etc rather than on the RISC-V
maintainer.

TL;DR of that is maybe a more nuanced policy of "no non-ratified
extensions that touch arch/riscv" could be a possibility but I would
completely understand if a "what's sauce for the goose is sauce for the
gander" approach was taken here and a blanket ban remains in place.

As I have said a bunch of times, this is all just my 2 cents etc and I
am as much of a punter here as you are... but maybe since I am in the
same sort of boat I at least have a fleshed out opinion. ¯\_()_/¯

Hopefully either Palmer can weigh in here or we do get a BoF & the
chance to have a chat about this sort of thing & maybe have a more
nuanced policy - or at the very least something that makes it clearer
that vendor extensions are a complete no-go upstream.

Conor.

>  9 files changed, 500 insertions(+)
>  create mode 100644 arch/riscv/vendors/Makefile
>  create mode 100644 arch/riscv/vendors/andes/Makefile
>  create mode 100644 arch/riscv/vendors/andes/ax45mp.c
>  create mode 100644 arch/riscv/vendors/andes/ax45mp_cache.c
>  create mode 100644 arch/riscv/vendors/andes/ax45mp_nocache_dma.c
>  create mode 100644 arch/riscv/vendors/andes/include/proc.h
>  create mode 100644 arch/riscv/vendors/andes/include/sbi.h
> 
> --
> 2.25.1
> 

Powered by blists - more mailing lists