Hi Laurent,
On Tue, Jun 19, 2018 at 4:15 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Sunday, 17 June 2018 03:08:02 EEST Marek Vasut wrote:
> > On 06/16/2018 05:44 PM, Laurent Pinchart wrote:
> > > 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.
Pass a "table"? Or an FDT containing the /memory nodes?
The latter allows for easier future extension.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
