[U-Boot] [RFC] ARM: rmobile: create DT memory nodes for R8A7795 3.0 and newer

[Laurent Pinchart]

On Sunday, 17 June 2018 03:08:02 EEST Marek Vasut wrote:
> On 06/16/2018 05:44 PM, Laurent Pinchart wrote:
> > Hi Marek,
> > 
> > On Saturday, 16 June 2018 02:42:30 EEST Marek Vasut wrote:
> >> On 06/16/2018 01:21 AM, Laurent Pinchart wrote:
> >>> On Friday, 15 June 2018 15:00:31 EEST Marek Vasut wrote:
> >>>> On 06/15/2018 01:43 PM, Marek Vasut wrote:
> >>>>> On 06/15/2018 12:37 PM, Ulrich Hecht wrote:
> >>>>>> On Fri, Jun 15, 2018 at 12:09 PM, Marek Vasut  wrote:
> >>>>>>>> +             arm_smccc_smc(ARM_SMCCC_RENESAS_MEMCONF,
> >>>>>>>> +                           0, 0, 0, 0, 0, 0, 0, &res);
> >>>>>>> 
> >>>>>>> Will this call work on platforms without patched ATF ?
> >>>>>>> (I think not, don't you need to handle return value?)
> >>>>>> 
> >>>>>> I have not actually tested that, but if I understand the ATF code
> >>>>>> correctly, unimplemented calls return
> >>>>>> SMC_UNK (0xffffffff), which should be handled by the default case
> >>>>>> (NOP)
> >>>>>> below.
> >>>>> 
> >>>>> Which means the board has a memory size of 0 and fails to boot ?
> >>>>> 
> >>>>>>>> +             switch (res.a0) {
> >>>>>>>> +             case 1:
> >>>>>>>> +                     base[0] = 0x048000000ULL;
> >>>>>>>> +                     size[0] = 0x038000000ULL;
> >>>>>>>> +                     base[1] = 0x500000000ULL;
> >>>>>>>> +                     size[1] = 0x040000000ULL;
> >>>>>>>> +                     base[2] = 0x600000000ULL;
> >>>>>>>> +                     size[2] = 0x040000000ULL;
> >>>>>>>> +                     base[3] = 0x700000000ULL;
> >>>>>>>> +                     size[3] = 0x040000000ULL;
> >>>>>>>> +                     fdt_fixup_memory_banks(blob, base, size, 4);
> >>>>>>>> +                     break;
> >>>>>>>> +             case 2:
> >>>>>>>> +                     base[0] = 0x048000000ULL;
> >>>>>>>> +                     size[0] = 0x078000000ULL;
> >>>>>>>> +                     base[1] = 0x500000000ULL;
> >>>>>>>> +                     size[1] = 0x080000000ULL;
> >>>>>>>> +                     fdt_fixup_memory_banks(blob, base, size, 2);
> >>>>>>>> +                     break;
> >>>>>>>> +             case 3:
> >>>>>>>> +                     base[0] = 0x048000000ULL;
> >>>>>>>> +                     size[0] = 0x078000000ULL;
> >>>>>>>> +                     base[1] = 0x500000000ULL;
> >>>>>>>> +                     size[1] = 0x080000000ULL;
> >>>>>>>> +                     base[2] = 0x600000000ULL;
> >>>>>>>> +                     size[2] = 0x080000000ULL;
> >>>>>>>> +                     base[3] = 0x700000000ULL;
> >>>>>>>> +                     size[3] = 0x080000000ULL;
> >>>>>>>> +                     fdt_fixup_memory_banks(blob, base, size, 4);
> >>>>>>>> +                     break;
> >>>>>>> 
> >>>>>>> Obvious design question is -- since you're adding new SMC call
> >>>>>>> anyway,
> >>>>>>> can't the call just return the memory layout table itself, so that
> >>>>>>> it
> >>>>>>> won't be duplicated both in U-Boot and ATF ?
> >>>>>> 
> >>>>>> My gut feeling was to go with the smallest interface possible.
> >>>>> 
> >>>>> But this doesn't scale. The API here uses some ad-hoc constants to
> >>>>> identify memory layout tables which have to be encoded both in ATF and
> >>>>> U-Boot, both of which must be kept in sync.
> >>>>> 
> >>>>> The ATF already has those memory layout tables, it's only a matter of
> >>>>> passing them to U-Boot. If you do just that, the ad-hoc constants and
> >>>>> encoding of tables into U-Boot goes away and in fact simplifies the
> >>>>> design.
> >>>>> 
> >>>>> Yet, I have to wonder if ATF doesn't already contain some sort of
> >>>>> standard SMC call to get memory topology. It surprises me that it
> >>>>> wouldn't.
> >>>> 
> >>>> In fact, Laurent (CCed) was solving some similar issue with lossy
> >>>> decomp
> >>>> and I think this involved some passing of memory layout information
> >>>> from
> >>>> ATF to U-Boot too, or am I mistaken ?
> >>> 
> >>> That's correct, ATF stores information about the memory layout at a
> >>> fixed
> >>> address in system memory, and U-Boot can read it.
> >> 
> >> Well, that sounds good ! Maybe we can avoid adding SMC call altogether
> >> then? :)
> > 
> > I'd prefer that, yes.
> > 
> > Let's not duplicate the mechanism used to pass FCNL information from ATF
> > to U- Boot, but instead create a data table format that can store all the
> > information we need, in an easily extensible way.
> > 
> > To see how the mechanism is implemented for FCNL, search for 47FD7000 in
> > the Renesas ATF sources
> > (git://github.com/renesas-rcar/arm-trusted-firmware.git).
> For everyone involved, can you explain what FCNL is ? ;-)

FCNL is Frame Compression Near Lossless. It's a way to reduce memory bandwidth 
by transparent compression and decompression of video frames. Compression is 
handled by an IP core called FCP, and decompression is handled by the DRAM 
controller. ATF programs the DRAM controller with ranges of memory addresses 
that will be dynamically decompressed. The registers containing those ranges 
are accessible in secure mode only, so neither U-Boot nor Linux can read them. 
That's why ATF has to pass the information to U-Boot, in order to add the 
ranges as reserved memory in DT.

> Any yes, I agree this sounds good. I had a discussion on the U-Boot IRC
> about passing the memory configuration around and the result is
> basically the same -- pass a table from ATF to U-Boot. If there's
> already something, great.

-- 
Regards,

Laurent Pinchart