[PATCH v2] ddr: marvell: a38x: Add support for DDR4 from Marvell mv-ddr-marvell repository
Tony Dinh
mibodhi at gmail.com
Wed Jan 18 02:18:43 CET 2023
Hi Pali,
On Tue, Jan 17, 2023 at 1:25 PM Pali Rohár <pali at kernel.org> wrote:
>
> 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.
I see. Yes, I did miss that! I only described the changes in v2 description.
> > >
> > > 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.
When I do git log I also see an extra 4 or 8 spaces on each line:) so
not sure what we are seeing here. But yes it seems some of the
indentation is inconsistent. Will fix that.
>
> > >
> > > > 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 :-)
There is only one existing file removed (dram_if.h). The other are new
files that become dead code after we run the filter script. Do you
think those should also be kept in this patch, and then we'll have a
cleanup patch later? The alternative is I can just list the
new-but-deleted files in the commit description so they can be kept
tracked of (easier for the future code sync if we know what should be
candidates for removal).
>
> > >
> > > > 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.
Cool.
> 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.
Would you do that when you have time after DDR4 gets merged?
Thanks,
Tony
More information about the U-Boot
mailing list