On October 24, 2024 thus sayeth Neha Malcom Francis:
> Hi Bryan
>
> On 23/10/24 20:09, Bryan Brattlof wrote:
> > On October 21, 2024 thus sayeth Santhosh Kumar K:
> > > As R5 is a 32 bit processor, the RAM banks' base and size calculation
> > > is restricted to 32 bits, which results in wrong values if bank's base
> > > is greater than 32 bits or bank's size is greater than or equal to 4GB.
> > >
> > > So, add k3_ddrss_ddr_bank_base_size_calc() to get the base address and
> > > size of RAM's banks from the device tree memory node, and store in a
> > > 64 bit device private data which can be used for ECC reserved memory
> > > calculation, Setting ECC range and Fixing up bank size in device tree
> > > when ECC is enabled.
> > >
> > > Signed-off-by: Santhosh Kumar K <s...@ti.com>
> > > ---
> > > drivers/ram/k3-ddrss/k3-ddrss.c | 72 ++++++++++++++++++++++++++-------
> > > 1 file changed, 57 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c
> > > b/drivers/ram/k3-ddrss/k3-ddrss.c
> > > index f2ab8cf2b49b..bd6783ebc2cd 100644
> > > --- a/drivers/ram/k3-ddrss/k3-ddrss.c
> > > +++ b/drivers/ram/k3-ddrss/k3-ddrss.c
> > > @@ -147,6 +147,9 @@ struct k3_ddrss_desc {
> > > struct k3_ddrss_ecc_region
> > > ecc_regions[K3_DDRSS_MAX_ECC_REGIONS];
> > > u64 ecc_reserved_space;
> > > bool ti_ecc_enabled;
> > > + unsigned long long ddr_bank_base[CONFIG_NR_DRAM_BANKS];
> > > + unsigned long long ddr_bank_size[CONFIG_NR_DRAM_BANKS];
> > > + unsigned long long ddr_ram_size;
> >
> > Should we use the u64 typedef here?
> >
> > > };
> > > struct reginitdata {
> > > @@ -668,11 +671,54 @@ static void k3_ddrss_lpddr4_preload_full_mem(struct
> > > k3_ddrss_desc *ddrss,
> > > printf("ECC: priming DDR completed in %lu msec\n",
> > > get_timer(done));
> > > }
> > > +static void k3_ddrss_ddr_bank_base_size_calc(struct k3_ddrss_desc *ddrss)
> > > +{
> > > + int bank, na, ns, len, parent;
> > > + const fdt32_t *ptr, *end;
> > > +
> > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > + ddrss->ddr_bank_base[bank] = 0;
> > > + ddrss->ddr_bank_size[bank] = 0;
> > > + }
> > > +
> > > + ofnode mem = ofnode_null();
> > > +
> > > + do {
> > > + mem = ofnode_by_prop_value(mem, "device_type", "memory", 7);
> > > + } while (!ofnode_is_enabled(mem));
> > > +
> > > + const void *fdt = ofnode_to_fdt(mem);
> > > + int node = ofnode_to_offset(mem);
> > > + const char *property = "reg";
> > > +
> > > + parent = fdt_parent_offset(fdt, node);
> > > + na = fdt_address_cells(fdt, parent);
> > > + ns = fdt_size_cells(fdt, parent);
> > > + ptr = fdt_getprop(fdt, node, property, &len);
> > > + end = ptr + len / sizeof(*ptr);
> > > +
> > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > + if (ptr + na + ns <= end) {
> > > + if (CONFIG_IS_ENABLED(OF_TRANSLATE))
> > > + ddrss->ddr_bank_base[bank] =
> > > fdt_translate_address(fdt, node, ptr);
> > > + else
> > > + ddrss->ddr_bank_base[bank] =
> > > fdtdec_get_number(ptr, na);
> > > +
> > > + ddrss->ddr_bank_size[bank] =
> > > fdtdec_get_number(&ptr[na], ns);
> > > + }
> > > +
> > > + ptr += na + ns;
> > > + }
> > > +
> > > + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++)
> > > + ddrss->ddr_ram_size += ddrss->ddr_bank_size[bank];
> > > +}
> > > +
> > > static void k3_ddrss_lpddr4_ecc_calc_reserved_mem(struct k3_ddrss_desc
> > > *ddrss)
> > > {
> > > fdtdec_setup_mem_size_base_lowest();
> > > - ddrss->ecc_reserved_space = gd->ram_size;
> > > + ddrss->ecc_reserved_space = ddrss->ddr_ram_size;
> > > do_div(ddrss->ecc_reserved_space, 9);
> > > /* Round to clean number */
> > > @@ -696,7 +742,7 @@ static void k3_ddrss_lpddr4_ecc_init(struct
> > > k3_ddrss_desc *ddrss)
> > > /* Preload the full memory with 0's using the BIST engine of
> > > * the LPDDR4 controller.
> > > */
> > > - k3_ddrss_lpddr4_preload_full_mem(ddrss, gd->ram_size, 0);
> > > + k3_ddrss_lpddr4_preload_full_mem(ddrss, ddrss->ddr_ram_size, 0);
> > > /* Clear Error Count Register */
> > > writel(0x1, base + DDRSS_ECC_1B_ERR_CNT_REG);
> > > @@ -741,6 +787,8 @@ static int k3_ddrss_probe(struct udevice *dev)
> > > k3_lpddr4_start(ddrss);
> > > + k3_ddrss_ddr_bank_base_size_calc(ddrss);
> > > +
> > > if (ddrss->ti_ecc_enabled) {
> > > if (!ddrss->ddrss_ss_cfg) {
> > > printf("%s: ss_cfg is required if ecc is
> > > enabled but not provided.",
> > > @@ -761,30 +809,24 @@ static int k3_ddrss_probe(struct udevice *dev)
> > > int k3_ddrss_ddr_fdt_fixup(struct udevice *dev, void *blob, struct
> > > bd_info *bd)
> > > {
> > > - struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> > > - u64 start[CONFIG_NR_DRAM_BANKS];
> > > - u64 size[CONFIG_NR_DRAM_BANKS];
> > > int bank;
> > > + struct k3_ddrss_desc *ddrss = dev_get_priv(dev);
> > > if (ddrss->ecc_reserved_space == 0)
> > > return 0;
> > > for (bank = CONFIG_NR_DRAM_BANKS - 1; bank >= 0; bank--) {
> > > - if (ddrss->ecc_reserved_space > bd->bi_dram[bank].size) {
> > > - ddrss->ecc_reserved_space -= bd->bi_dram[bank].size;
> > > - bd->bi_dram[bank].size = 0;
> > > + if (ddrss->ecc_reserved_space > ddrss->ddr_bank_size[bank]) {
> > > + ddrss->ecc_reserved_space -= ddrss->ddr_bank_size[bank];
> > > + ddrss->ddr_bank_size[bank] = 0;
> > > } else {
> > > - bd->bi_dram[bank].size -= ddrss->ecc_reserved_space;
> > > + ddrss->ddr_bank_size[bank] -= ddrss->ecc_reserved_space;
> > > break;
> > > }
> > > }
> > > - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> > > - start[bank] = bd->bi_dram[bank].start;
> > > - size[bank] = bd->bi_dram[bank].size;
> > > - }
> > > -
> > > - return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
> > > + return fdt_fixup_memory_banks(blob, ddrss->ddr_bank_base,
> > > + ddrss->ddr_bank_size,
> > > CONFIG_NR_DRAM_BANKS);
> > > }
> >
> > Have we looked into passing this information via the global data pointer
> > rather than fixing up the device tree on each boot phase?
> >
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20180926215520.87168-23-...@chromium.org/
> >
> > I don't see a clear path to pass GD from SPL to SPL but there is a TPL
> > to SPL which I think we should be able to copy.
>
> But we also make use of this to fix up the device tree that passes onto
> kernel as well, I need to look into this patch though. But from a first
> glance looks like we will be able to pass on the information from SPL->SPL
> (with modification as you said) and SPL->U-Boot but U-Boot->Kernel would
> still require this function to be present.
>
I agree. If this scheme works out we can place this kernel fixup
function with the other OPN fixups we do though later on in the boot.
https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-k3/common_fdt.c
