[PATCH v2] ddr: marvell: a38x: Add support for DDR4 from Marvell mv-ddr-marvell repository
Tony Dinh
mibodhi at gmail.com
Tue Jan 17 22:02:46 CET 2023
Hi Pali,
On Tue, Jan 17, 2023 at 12:35 AM Pali Rohár <pali at kernel.org> wrote:
>
> Hello! Thank you for update. It is much better.
>
> On Monday 16 January 2023 21:34:39 Tony Dinh wrote:
> > This syncs drivers/ddr/marvell/a38x/ with the master branch of repository
> > https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell.git
> >
> > up to the commit "mv_ddr: a3700: Use the right size for memset to not overflow"
> > d5acc10c287e40cc2feeb28710b92e45c93c702c
> >
> > This patch was created by following steps:
> >
> > 1. Replace all a38x files in U-Boot tree by files from upstream github
> > Marvell mv-ddr-marvell repository.
> >
> > 2. Run following command to omit portions not relevant for a38x, ddr3, and ddr4:
> >
> > files=drivers/ddr/marvell/a38x/*
> > sed 's/#if defined(CONFIG_ARMADA_38X) || defined(CONFIG_ARMADA_39X)/#ifdef TRUE/' -i $files
> > unifdef -m -UMV_DDR -UMV_DDR_ATF -UCONFIG_APN806 \
> > -UCONFIG_MC_STATIC -UCONFIG_MC_STATIC_PRINT -UCONFIG_PHY_STATIC \
> > -UCONFIG_PHY_STATIC_PRINT -UCONFIG_CUSTOMER_BOARD_SUPPORT \
> > -UCONFIG_A3700 -UA3900 -UA80X0 -UA70X0 -DTRUE $files
>
> Do not forget to also update commit message.
Yes, patman extracts and creates the patch description from the commit.
>
> And btw, commit messages has on each line some leading spaces which is
> not probably intended.
That was intentional to make the commit description (and patch
description) more readable. Is it not recommended?
>
> > 3. Manually change license to SPDX-License-Identifier
> > (upstream license in upstream github repository contains long license
> > texts and U-Boot is using just SPDX-License-Identifier.
> >
> > After applying this patch, a38x ddr3 ddr4 code in upstream Marvell github
> > repository and in U-Boot would be fully identical. So in future applying
> > above steps could be used to sync code again.
> >
> > The only change in this patch are:
> > - Removal of common board_topology_map code using ifdefs in mv_ddr_brd.c
> > - Some fixes with include files.
> > - Some basic type defines (original from ATF headers) in mv_ddr_plat.c
> >
> > Reference:
> > "ddr: marvell: a38x: Sync code with Marvell mv-ddr-marvell repository"
> > https://source.denx.de/u-boot/u-boot/-/commit/107c3391b95bcc2ba09a876da4fa0c31b6c1e460
> >
> > Signed-off-by: Tony Dinh <mibodhi at gmail.com>
> > ---
> >
> > Changes in v2:
> > - Modified the filter scrip to explicitly include ARMADA_38X code
> > and exclude ARMADA_39X code; also remove 64BIT code. Reran it on
> > drivers/ddr/marvell/a38x/
> > - Updated script
> > files=drivers/ddr/marvell/a38x/*
> > sed 's/#if defined(CONFIG_ARMADA_38X) || defined(CONFIG_ARMADA_39X)/#ifdef TRUE/' -i $files
>
> You do not need this sed anymore. CONFIG_ARMADA_39X is explicitly
> removed and CONFIG_ARMADA_38X already handled by unifdef.
Thanks, I was not sure if unifdef works in that "OR" condition. I will
update the commit message.
>
> > unifdef -m -UMV_DDR -UMV_DDR_ATF -UCONFIG_APN806 \
> > -UCONFIG_MC_STATIC -UCONFIG_MC_STATIC_PRINT -UCONFIG_PHY_STATIC \
> > -UCONFIG_PHY_STATIC_PRINT -UCONFIG_CUSTOMER_BOARD_SUPPORT \
> > -UCONFIG_A3700 -UA3900 -UA80X0 -UA70X0 -DCONFIG_ARMADA_38X -UCONFIG_ARMADA_39X \
> > -UCONFIG_64BIT $files
> > - Remove more dead code files
> > - Correct SPDX license header
> >
> > drivers/ddr/marvell/a38x/Makefile | 8 +
> > drivers/ddr/marvell/a38x/ddr3_debug.c | 120 +
> > drivers/ddr/marvell/a38x/ddr3_init.c | 25 +
> > drivers/ddr/marvell/a38x/ddr3_init.h | 14 +
> > drivers/ddr/marvell/a38x/ddr3_logging_def.h | 27 +
> > drivers/ddr/marvell/a38x/ddr3_training.c | 131 +
> > drivers/ddr/marvell/a38x/ddr3_training_bist.c | 12 +
> > .../a38x/ddr3_training_centralization.c | 4 +
> > drivers/ddr/marvell/a38x/ddr3_training_db.c | 212 ++
> > drivers/ddr/marvell/a38x/ddr3_training_ip.h | 17 +
> > .../ddr/marvell/a38x/ddr3_training_ip_db.h | 61 +
> > .../marvell/a38x/ddr3_training_ip_engine.c | 145 +
> > .../ddr/marvell/a38x/ddr3_training_ip_flow.h | 5 +
> > .../ddr/marvell/a38x/ddr3_training_leveling.c | 135 +
> > drivers/ddr/marvell/a38x/dram_if.h | 13 -
> > drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.c | 674 +++++
> > drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.h | 59 +
> > drivers/ddr/marvell/a38x/mv_ddr4_training.c | 565 ++++
> > drivers/ddr/marvell/a38x/mv_ddr4_training.h | 32 +
> > .../a38x/mv_ddr4_training_calibration.c | 2336 +++++++++++++++++
> > .../a38x/mv_ddr4_training_calibration.h | 26 +
> > .../ddr/marvell/a38x/mv_ddr4_training_db.c | 545 ++++
> > .../marvell/a38x/mv_ddr4_training_leveling.c | 441 ++++
> > .../marvell/a38x/mv_ddr4_training_leveling.h | 11 +
> > drivers/ddr/marvell/a38x/mv_ddr_plat.c | 249 ++
> > drivers/ddr/marvell/a38x/mv_ddr_plat.h | 11 +
> > drivers/ddr/marvell/a38x/mv_ddr_regs.h | 59 +
> > drivers/ddr/marvell/a38x/mv_ddr_topology.h | 72 +
> > 28 files changed, 5996 insertions(+), 13 deletions(-)
> > delete mode 100644 drivers/ddr/marvell/a38x/dram_if.h
>
> I see that you are removing some existing file. If it is not needed
> neither for DDR3 nor for DDR4 then please remove it in separate commit
> or patch. So we do not mix different things into one commit.
Instead of making a different commit, will it work if we list the
files being removed in this commit message? It is part of removing
dead code.
>
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.c
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.h
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training.c
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training.h
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.h
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_db.c
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_leveling.c
> > create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_leveling.h
> ...
> > diff --git a/drivers/ddr/marvell/a38x/mv_ddr_plat.c b/drivers/ddr/marvell/a38x/mv_ddr_plat.c
> > index 7c7bce73a3..16d177b42f 100644
> > --- a/drivers/ddr/marvell/a38x/mv_ddr_plat.c
> > +++ b/drivers/ddr/marvell/a38x/mv_ddr_plat.c
> > @@ -12,6 +12,11 @@
> > #define DDR_INTERFACES_NUM 1
> > #define DDR_INTERFACE_OCTETS_NUM 5
> >
> > +/* These were defined in ATF area that was stripped out */
> > +#define MV_STATUS int
> > +#define MV_U32 u32
> > +#define MV_U8 u8
> > +
>
> This is something new which you added? Because I do not see it in
> Marvell code.
Yes, those were in the original code after the initial copying from
the Marvell repo.
# grep -E '(MV_U32|MV_STATUS|MV_U8)' *.[ch] a38x/*.[ch]
ddr_init.c:MV_U32 ddr_init(void)
mv_ddr_atf_wrapper.h:#define MV_STATUS int
mv_ddr_atf_wrapper.h:#define MV_U8 u8
mv_ddr_atf_wrapper.h:#define MV_U32 u32
a38x/mv_ddr_plat.c:MV_STATUS mv_ddr4_calibration_validate(MV_U32 dev_num)
a38x/mv_ddr_plat.c: MV_STATUS status = MV_OK;
a38x/mv_ddr_plat.c: MV_U8 if_id = 0;
a38x/mv_ddr_plat.c: MV_U32 read_data[MAX_INTERFACE_NUM];
a38x/mv_ddr_plat.c: MV_U32 cal_n = 0, cal_p = 0;
Those 3 are defined in mv_ddr_atf_wrapper.h and used in mv_ddr_plat.c
(after we ran the filter script, this file is the only place that
needs those 3 defines). Since we removed the ATF code, we need to
define them here in mv_ddr_plat.c. Is this OK or do you have any
suggestions for a better approach?
Thanks,
Tony
More information about the U-Boot
mailing list