[PATCH v2] ddr: marvell: a38x: Add support for DDR4 from Marvell mv-ddr-marvell repository
Pali Rohár
pali at kernel.org
Tue Jan 17 22:25:37 CET 2023
Hello!
On Tuesday 17 January 2023 13:02:46 Tony Dinh wrote:
> 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.
My reaction was just because you forgot to update undef for a39x.
> >
> > 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?
I'm not sure if we are talking about the same thing. When I read this
your patch I saw that every time, even the first one "This sync drivers/..."
has 4 spaces before word "This". And I'm not sure if this is just my
email client or not and there is some reason for it. Look at indentation
of line "Signed-off-by:" and line "Reference:". Should not be those two
lines at same indentation level? Or I did not understand it? :D
I agree that adding indentation inside of 1. 2. 3. parts is fully
recommended as it makes text more readable.
> >
> > > 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.
It should work if at least one of the option in OR condition is
specified with -D on command line. But if you are unsure then it is
better to test it (should be quite easy and fast).
> >
> > > 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.
I do not know. I'm always trying to put different thing into different
commits. Reason is that if in some case it would be needed to revert
commit then unrelated cleanup does not need to be reverted :-)
> >
> > > 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?
Hmm... You found another bug in Marvell code:
$ git grep mv_ddr4_calibration_validate
a38x/mv_ddr_plat.c:MV_STATUS mv_ddr4_calibration_validate(MV_U32 dev_num)
apn806/mv_ddr_plat.c:int mv_ddr4_calibration_validate(u32 dev_num)
mv_ddr4_training.c: return mv_ddr4_calibration_validate(dev_num);
mv_ddr4_training.h:int mv_ddr4_calibration_validate(u32 dev_num);
That function mv_ddr4_calibration_validate() should return int type (as
defined in header file) and not MV_STATUS type. So rather fix return
type of the function to match what is in header file. Also there is
mismatch with its argument u32 vs MV_u32!
Next, MV_U8 is used only at one place (a37xx defines it moreover locally):
$ git grep MV_U8
a3700/mv_ddr_a3700_wrapper.h:#define MV_U8 u8
a38x/mv_ddr_plat.c: MV_U8 if_id = 0;
mv_ddr_atf_wrapper.h:#define MV_U8 u8
So replace MV_U8 directly by u8. And same for MV_U32 for a38x code.
And ideally, send a pull request to Marvell repo with these fixes (and
also with floating point), so code can be synced easily also again in
future.
More information about the U-Boot
mailing list