Re: [RFC PATCH 0/2] AX45MP: Add support to non-coherent DMA

[Atish Patra]

On Thu, Sep 8, 2022 at 2:45 PM <Conor.Dooley@xxxxxxxxxxxxx> wrote:
>
> 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@13400000 {
> >                 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.
>

RISC-V BoF is scheduled on Wednesday.

https://lpc.events/event/16/contributions/1389/

> 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
> >
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish