* [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
@ 2022-01-12 16:06 Marek Behún
2022-01-13 6:29 ` Stefan Roese
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Marek Behún @ 2022-01-12 16:06 UTC (permalink / raw)
To: Stefan Roese, Mario Six, Dennis Gilmore, Kostya Porotchkin
Cc: Pali Rohár, u-boot, Moti Boskula, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This is a cleaned up and fixed version of a patch
mv_ddr: a380: fix SPLIT_OUT_MIX state decision
in each pattern cycle the bus state can be changed
in order to avoide it, need to back to the same bus state on each
pattern cycle
by
Moti Boskula <motib@marvell.com>
The original patch is not in Marvell's mv-ddr-marvell repository. It was
gives to us by Marvell to fix an issues with DDR training on some
boards, but it cannot be applied as is to mv-ddr-marvell, because it is
a very dirty draft patch that would certainly break other things, mainly
DDR4 training code in mv-ddr-marvell, since it changes common functions.
I have cleaned up the patch and removed stuff that seemed unnecessary
(when removed, it still fixed things). Note that I don't understand
completely what the code does exactly, since I haven't studied the DDR
training code extensively (and I suspect that no one besides some few
people in Marvell understand the code completely).
Anyway after the cleanup the patch still fixes isssues with DDR training
on the failing boards.
There was also a problem with the original patch on some of the Allied
Telesis' x530 boards, reported by Chris Packham. I have asked Chris to
send me some logs, and managed to fix it:
- if you look at the change, you'll notice that it introduces
subtraction of cur_start_win[] and cur_end_win[] members, depending on
a bit set in the current_byte_status variable
- the original patch subtracted cur_start_win[] if either
BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
was set
- from Chris Packham logs I discovered that the x530 board where the
original patch introduced DDR training failure, only the
BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
current_byte_status variable
- this led me to the hypothesis that both cur_start_win[] and
cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at all
- this hypothesis also gains credibility when considering the commit
title ("fix SPLIT_OUT_MIX state decision")
Hopefully this will fix things without breaking anything else.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
.../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
index 648b37ef6f..42308b6965 100644
--- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
+++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
@@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
u32 if_id, pattern_id, bit_id;
u8 bus_id;
+ u8 current_byte_status;
u8 cur_start_win[BUS_WIDTH_IN_BITS];
u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
u8 cur_end_win[BUS_WIDTH_IN_BITS];
@@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
result[search_dir_id][7]));
}
+ current_byte_status =
+ mv_ddr_tip_sub_phy_byte_status_get(if_id,
+ bus_id);
+
for (bit_id = 0; bit_id < BUS_WIDTH_IN_BITS;
bit_id++) {
/* check if this code is valid for 2 edge, probably not :( */
@@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
[HWS_LOW2HIGH]
[bit_id],
EDGE_1);
+ if (current_byte_status &
+ BYTE_SPLIT_OUT_MIX) {
+ if (cur_start_win[bit_id] >= 64)
+ cur_start_win[bit_id] -= 64;
+ else
+ cur_start_win[bit_id] = 0;
+ DEBUG_CENTRALIZATION_ENGINE
+ (DEBUG_LEVEL_INFO,
+ ("pattern %d IF %d pup %d bit %d subtract 64 adll from start\n",
+ pattern_id, if_id, bus_id, bit_id));
+ }
cur_end_win[bit_id] =
GET_TAP_RESULT(result
[HWS_HIGH2LOW]
[bit_id],
EDGE_1);
+ if (cur_end_win[bit_id] >= 64 &&
+ (current_byte_status &
+ BYTE_SPLIT_OUT_MIX)) {
+ cur_end_win[bit_id] -= 64;
+ DEBUG_CENTRALIZATION_ENGINE
+ (DEBUG_LEVEL_INFO,
+ ("pattern %d IF %d pup %d bit %d subtract 64 adll from end\n",
+ pattern_id, if_id, bus_id, bit_id));
+ }
+
/* window length */
current_window[bit_id] =
cur_end_win[bit_id] -
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
2022-01-12 16:06 [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision Marek Behún
@ 2022-01-13 6:29 ` Stefan Roese
2022-01-13 7:13 ` Chris Packham
2022-01-14 15:38 ` Stefan Roese
2022-01-17 6:52 ` [EXT] " Moti Buskila
2 siblings, 1 reply; 7+ messages in thread
From: Stefan Roese @ 2022-01-13 6:29 UTC (permalink / raw)
To: Marek Behún, Mario Six, Dennis Gilmore, Kostya Porotchkin
Cc: Pali Rohár, u-boot, Moti Boskula, Marek Behún, Chris Packham
+Cc Chris
On 1/12/22 17:06, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
>
> This is a cleaned up and fixed version of a patch
> mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>
> in each pattern cycle the bus state can be changed
> in order to avoide it, need to back to the same bus state on each
> pattern cycle
> by
> Moti Boskula <motib@marvell.com>
>
> The original patch is not in Marvell's mv-ddr-marvell repository. It was
> gives to us by Marvell to fix an issues with DDR training on some
> boards, but it cannot be applied as is to mv-ddr-marvell, because it is
> a very dirty draft patch that would certainly break other things, mainly
> DDR4 training code in mv-ddr-marvell, since it changes common functions.
>
> I have cleaned up the patch and removed stuff that seemed unnecessary
> (when removed, it still fixed things). Note that I don't understand
> completely what the code does exactly, since I haven't studied the DDR
> training code extensively (and I suspect that no one besides some few
> people in Marvell understand the code completely).
>
> Anyway after the cleanup the patch still fixes isssues with DDR training
> on the failing boards.
>
> There was also a problem with the original patch on some of the Allied
> Telesis' x530 boards, reported by Chris Packham. I have asked Chris to
> send me some logs, and managed to fix it:
> - if you look at the change, you'll notice that it introduces
> subtraction of cur_start_win[] and cur_end_win[] members, depending on
> a bit set in the current_byte_status variable
> - the original patch subtracted cur_start_win[] if either
> BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
> subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
> was set
> - from Chris Packham logs I discovered that the x530 board where the
> original patch introduced DDR training failure, only the
> BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
> patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
> current_byte_status variable
> - this led me to the hypothesis that both cur_start_win[] and
> cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
> set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at all
> - this hypothesis also gains credibility when considering the commit
> title ("fix SPLIT_OUT_MIX state decision")
>
> Hopefully this will fix things without breaking anything else.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Stefan Roese <sr@denx.de>
I would be good though, if Chris (and others) could also add a comment
(RB or TB tag).
Thanks,
Stefan
> ---
> .../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> index 648b37ef6f..42308b6965 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> @@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
> u32 if_id, pattern_id, bit_id;
> u8 bus_id;
> + u8 current_byte_status;
> u8 cur_start_win[BUS_WIDTH_IN_BITS];
> u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
> u8 cur_end_win[BUS_WIDTH_IN_BITS];
> @@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> result[search_dir_id][7]));
> }
>
> + current_byte_status =
> + mv_ddr_tip_sub_phy_byte_status_get(if_id,
> + bus_id);
> +
> for (bit_id = 0; bit_id < BUS_WIDTH_IN_BITS;
> bit_id++) {
> /* check if this code is valid for 2 edge, probably not :( */
> @@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> [HWS_LOW2HIGH]
> [bit_id],
> EDGE_1);
> + if (current_byte_status &
> + BYTE_SPLIT_OUT_MIX) {
> + if (cur_start_win[bit_id] >= 64)
> + cur_start_win[bit_id] -= 64;
> + else
> + cur_start_win[bit_id] = 0;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from start\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> cur_end_win[bit_id] =
> GET_TAP_RESULT(result
> [HWS_HIGH2LOW]
> [bit_id],
> EDGE_1);
> + if (cur_end_win[bit_id] >= 64 &&
> + (current_byte_status &
> + BYTE_SPLIT_OUT_MIX)) {
> + cur_end_win[bit_id] -= 64;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from end\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> +
> /* window length */
> current_window[bit_id] =
> cur_end_win[bit_id] -
>
Viele Grüße,
Stefan Roese
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
2022-01-13 6:29 ` Stefan Roese
@ 2022-01-13 7:13 ` Chris Packham
0 siblings, 0 replies; 7+ messages in thread
From: Chris Packham @ 2022-01-13 7:13 UTC (permalink / raw)
To: Stefan Roese
Cc: Marek Behún, Mario Six, Dennis Gilmore, Kostya Porotchkin,
Pali Rohár, u-boot, Moti Boskula, Marek Behún
On Thu, 13 Jan 2022, 7:29 PM Stefan Roese, <sr@denx.de> wrote:
> +Cc Chris
>
> On 1/12/22 17:06, Marek Behún wrote:
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > This is a cleaned up and fixed version of a patch
> > mv_ddr: a380: fix SPLIT_OUT_MIX state decision
> >
> > in each pattern cycle the bus state can be changed
> > in order to avoide it, need to back to the same bus state on each
> > pattern cycle
> > by
> > Moti Boskula <motib@marvell.com>
> >
> > The original patch is not in Marvell's mv-ddr-marvell repository. It was
> > gives to us by Marvell to fix an issues with DDR training on some
> > boards, but it cannot be applied as is to mv-ddr-marvell, because it is
> > a very dirty draft patch that would certainly break other things, mainly
> > DDR4 training code in mv-ddr-marvell, since it changes common functions.
> >
> > I have cleaned up the patch and removed stuff that seemed unnecessary
> > (when removed, it still fixed things). Note that I don't understand
> > completely what the code does exactly, since I haven't studied the DDR
> > training code extensively (and I suspect that no one besides some few
> > people in Marvell understand the code completely).
> >
> > Anyway after the cleanup the patch still fixes isssues with DDR training
> > on the failing boards.
> >
> > There was also a problem with the original patch on some of the Allied
> > Telesis' x530 boards, reported by Chris Packham. I have asked Chris to
> > send me some logs, and managed to fix it:
> > - if you look at the change, you'll notice that it introduces
> > subtraction of cur_start_win[] and cur_end_win[] members, depending on
> > a bit set in the current_byte_status variable
> > - the original patch subtracted cur_start_win[] if either
> > BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
> > subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
> > was set
> > - from Chris Packham logs I discovered that the x530 board where the
> > original patch introduced DDR training failure, only the
> > BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
> > patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
> > current_byte_status variable
> > - this led me to the hypothesis that both cur_start_win[] and
> > cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
> > set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at all
> > - this hypothesis also gains credibility when considering the commit
> > title ("fix SPLIT_OUT_MIX state decision")
> >
> > Hopefully this will fix things without breaking anything else.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
>
> Reviewed-by: Stefan Roese <sr@denx.de>
>
> I would be good though, if Chris (and others) could also add a comment
> (RB or TB tag).
>
Yes this is the same patch that Marek sent me privately.
Tested-by: Chris Packham <judge.packham@gmail.com>
> Thanks,
> Stefan
>
> > ---
> > .../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> > index 648b37ef6f..42308b6965 100644
> > --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> > +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> > @@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32
> mode)
> > enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
> > u32 if_id, pattern_id, bit_id;
> > u8 bus_id;
> > + u8 current_byte_status;
> > u8 cur_start_win[BUS_WIDTH_IN_BITS];
> > u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
> > u8 cur_end_win[BUS_WIDTH_IN_BITS];
> > @@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32
> mode)
> >
> result[search_dir_id][7]));
> > }
> >
> > + current_byte_status =
> > +
> mv_ddr_tip_sub_phy_byte_status_get(if_id,
> > +
> bus_id);
> > +
> > for (bit_id = 0; bit_id <
> BUS_WIDTH_IN_BITS;
> > bit_id++) {
> > /* check if this code is valid for
> 2 edge, probably not :( */
> > @@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num,
> u32 mode)
> >
> [HWS_LOW2HIGH]
> > [bit_id],
> > EDGE_1);
> > + if (current_byte_status &
> > + BYTE_SPLIT_OUT_MIX) {
> > + if (cur_start_win[bit_id]
> >= 64)
> > +
> cur_start_win[bit_id] -= 64;
> > + else
> > +
> cur_start_win[bit_id] = 0;
> > + DEBUG_CENTRALIZATION_ENGINE
> > + (DEBUG_LEVEL_INFO,
> > + ("pattern %d IF
> %d pup %d bit %d subtract 64 adll from start\n",
> > + pattern_id,
> if_id, bus_id, bit_id));
> > + }
> > cur_end_win[bit_id] =
> > GET_TAP_RESULT(result
> >
> [HWS_HIGH2LOW]
> > [bit_id],
> > EDGE_1);
> > + if (cur_end_win[bit_id] >= 64 &&
> > + (current_byte_status &
> > + BYTE_SPLIT_OUT_MIX)) {
> > + cur_end_win[bit_id] -= 64;
> > + DEBUG_CENTRALIZATION_ENGINE
> > + (DEBUG_LEVEL_INFO,
> > + ("pattern %d IF
> %d pup %d bit %d subtract 64 adll from end\n",
> > + pattern_id,
> if_id, bus_id, bit_id));
> > + }
> > +
> > /* window length */
> > current_window[bit_id] =
> > cur_end_win[bit_id] -
> >
>
> Viele Grüße,
> Stefan Roese
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
2022-01-12 16:06 [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision Marek Behún
2022-01-13 6:29 ` Stefan Roese
@ 2022-01-14 15:38 ` Stefan Roese
2022-01-17 6:52 ` [EXT] " Moti Buskila
2 siblings, 0 replies; 7+ messages in thread
From: Stefan Roese @ 2022-01-14 15:38 UTC (permalink / raw)
To: Marek Behún, Mario Six, Dennis Gilmore, Kostya Porotchkin
Cc: Pali Rohár, u-boot, Moti Boskula, Marek Behún
On 1/12/22 17:06, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
>
> This is a cleaned up and fixed version of a patch
> mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>
> in each pattern cycle the bus state can be changed
> in order to avoide it, need to back to the same bus state on each
> pattern cycle
> by
> Moti Boskula <motib@marvell.com>
>
> The original patch is not in Marvell's mv-ddr-marvell repository. It was
> gives to us by Marvell to fix an issues with DDR training on some
> boards, but it cannot be applied as is to mv-ddr-marvell, because it is
> a very dirty draft patch that would certainly break other things, mainly
> DDR4 training code in mv-ddr-marvell, since it changes common functions.
>
> I have cleaned up the patch and removed stuff that seemed unnecessary
> (when removed, it still fixed things). Note that I don't understand
> completely what the code does exactly, since I haven't studied the DDR
> training code extensively (and I suspect that no one besides some few
> people in Marvell understand the code completely).
>
> Anyway after the cleanup the patch still fixes isssues with DDR training
> on the failing boards.
>
> There was also a problem with the original patch on some of the Allied
> Telesis' x530 boards, reported by Chris Packham. I have asked Chris to
> send me some logs, and managed to fix it:
> - if you look at the change, you'll notice that it introduces
> subtraction of cur_start_win[] and cur_end_win[] members, depending on
> a bit set in the current_byte_status variable
> - the original patch subtracted cur_start_win[] if either
> BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
> subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
> was set
> - from Chris Packham logs I discovered that the x530 board where the
> original patch introduced DDR training failure, only the
> BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
> patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
> current_byte_status variable
> - this led me to the hypothesis that both cur_start_win[] and
> cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
> set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at all
> - this hypothesis also gains credibility when considering the commit
> title ("fix SPLIT_OUT_MIX state decision")
>
> Hopefully this will fix things without breaking anything else.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
Applied to u-boot-marvell/master
Thanks,
Stefan
> ---
> .../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> index 648b37ef6f..42308b6965 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> @@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
> u32 if_id, pattern_id, bit_id;
> u8 bus_id;
> + u8 current_byte_status;
> u8 cur_start_win[BUS_WIDTH_IN_BITS];
> u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
> u8 cur_end_win[BUS_WIDTH_IN_BITS];
> @@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> result[search_dir_id][7]));
> }
>
> + current_byte_status =
> + mv_ddr_tip_sub_phy_byte_status_get(if_id,
> + bus_id);
> +
> for (bit_id = 0; bit_id < BUS_WIDTH_IN_BITS;
> bit_id++) {
> /* check if this code is valid for 2 edge, probably not :( */
> @@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> [HWS_LOW2HIGH]
> [bit_id],
> EDGE_1);
> + if (current_byte_status &
> + BYTE_SPLIT_OUT_MIX) {
> + if (cur_start_win[bit_id] >= 64)
> + cur_start_win[bit_id] -= 64;
> + else
> + cur_start_win[bit_id] = 0;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from start\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> cur_end_win[bit_id] =
> GET_TAP_RESULT(result
> [HWS_HIGH2LOW]
> [bit_id],
> EDGE_1);
> + if (cur_end_win[bit_id] >= 64 &&
> + (current_byte_status &
> + BYTE_SPLIT_OUT_MIX)) {
> + cur_end_win[bit_id] -= 64;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from end\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> +
> /* window length */
> current_window[bit_id] =
> cur_end_win[bit_id] -
>
Viele Grüße,
Stefan Roese
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
2022-01-12 16:06 [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision Marek Behún
2022-01-13 6:29 ` Stefan Roese
2022-01-14 15:38 ` Stefan Roese
@ 2022-01-17 6:52 ` Moti Buskila
2022-01-17 14:23 ` Marek Behún
2 siblings, 1 reply; 7+ messages in thread
From: Moti Buskila @ 2022-01-17 6:52 UTC (permalink / raw)
To: Marek Behún, Stefan Roese, Mario Six, Dennis Gilmore,
Kostya Porotchkin
Cc: Pali Rohár, u-boot, Marek Behún
Hi Assaf,
I've got this email a few days ago.
Is it related to what you’ve send me?
Thanks
-----Original Message-----
From: Marek Behún <kabel@kernel.org>
Sent: Wednesday, January 12, 2022 6:07 PM
To: Stefan Roese <sr@denx.de>; Mario Six <mario.six@gdsys.cc>; Dennis Gilmore <dgilmore@redhat.com>; Kostya Porotchkin <kostap@marvell.com>
Cc: Pali Rohár <pali@kernel.org>; u-boot@lists.denx.de; Moti Buskila <motib@marvell.com>; Marek Behún <marek.behun@nic.cz>
Subject: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
External Email
----------------------------------------------------------------------
From: Marek Behún <marek.behun@nic.cz>
This is a cleaned up and fixed version of a patch
mv_ddr: a380: fix SPLIT_OUT_MIX state decision
in each pattern cycle the bus state can be changed
in order to avoide it, need to back to the same bus state on each
pattern cycle
by
Moti Boskula <motib@marvell.com>
The original patch is not in Marvell's mv-ddr-marvell repository. It was gives to us by Marvell to fix an issues with DDR training on some boards, but it cannot be applied as is to mv-ddr-marvell, because it is a very dirty draft patch that would certainly break other things, mainly
DDR4 training code in mv-ddr-marvell, since it changes common functions.
I have cleaned up the patch and removed stuff that seemed unnecessary (when removed, it still fixed things). Note that I don't understand completely what the code does exactly, since I haven't studied the DDR training code extensively (and I suspect that no one besides some few people in Marvell understand the code completely).
Anyway after the cleanup the patch still fixes isssues with DDR training on the failing boards.
There was also a problem with the original patch on some of the Allied Telesis' x530 boards, reported by Chris Packham. I have asked Chris to send me some logs, and managed to fix it:
- if you look at the change, you'll notice that it introduces
subtraction of cur_start_win[] and cur_end_win[] members, depending on
a bit set in the current_byte_status variable
- the original patch subtracted cur_start_win[] if either
BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
was set
- from Chris Packham logs I discovered that the x530 board where the
original patch introduced DDR training failure, only the
BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
current_byte_status variable
- this led me to the hypothesis that both cur_start_win[] and
cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at all
- this hypothesis also gains credibility when considering the commit
title ("fix SPLIT_OUT_MIX state decision")
Hopefully this will fix things without breaking anything else.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
.../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
index 648b37ef6f..42308b6965 100644
--- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
+++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
@@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
u32 if_id, pattern_id, bit_id;
u8 bus_id;
+ u8 current_byte_status;
u8 cur_start_win[BUS_WIDTH_IN_BITS];
u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
u8 cur_end_win[BUS_WIDTH_IN_BITS];
@@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
result[search_dir_id][7]));
}
+ current_byte_status =
+ mv_ddr_tip_sub_phy_byte_status_get(if_id,
+ bus_id);
+
for (bit_id = 0; bit_id < BUS_WIDTH_IN_BITS;
bit_id++) {
/* check if this code is valid for 2 edge, probably not :( */ @@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
[HWS_LOW2HIGH]
[bit_id],
EDGE_1);
+ if (current_byte_status &
+ BYTE_SPLIT_OUT_MIX) {
+ if (cur_start_win[bit_id] >= 64)
+ cur_start_win[bit_id] -= 64;
+ else
+ cur_start_win[bit_id] = 0;
+ DEBUG_CENTRALIZATION_ENGINE
+ (DEBUG_LEVEL_INFO,
+ ("pattern %d IF %d pup %d bit %d subtract 64 adll from start\n",
+ pattern_id, if_id, bus_id, bit_id));
+ }
cur_end_win[bit_id] =
GET_TAP_RESULT(result
[HWS_HIGH2LOW]
[bit_id],
EDGE_1);
+ if (cur_end_win[bit_id] >= 64 &&
+ (current_byte_status &
+ BYTE_SPLIT_OUT_MIX)) {
+ cur_end_win[bit_id] -= 64;
+ DEBUG_CENTRALIZATION_ENGINE
+ (DEBUG_LEVEL_INFO,
+ ("pattern %d IF %d pup %d bit %d subtract 64 adll from end\n",
+ pattern_id, if_id, bus_id, bit_id));
+ }
+
/* window length */
current_window[bit_id] =
cur_end_win[bit_id] -
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
2022-01-17 6:52 ` [EXT] " Moti Buskila
@ 2022-01-17 14:23 ` Marek Behún
2022-01-17 15:07 ` Moti Buskila
0 siblings, 1 reply; 7+ messages in thread
From: Marek Behún @ 2022-01-17 14:23 UTC (permalink / raw)
To: Moti Buskila
Cc: Stefan Roese, Mario Six, Dennis Gilmore, Kostya Porotchkin,
Pali Rohár, u-boot, Marek Behún
Hello Moti,
since you're the author of the original version of this patch, could
you please review it and if it is okay, put it into mv-ddr-marvell?
Thanks.
Marek
On Mon, 17 Jan 2022 06:52:08 +0000
Moti Buskila <motib@marvell.com> wrote:
> Hi Assaf,
> I've got this email a few days ago.
> Is it related to what you’ve send me?
> Thanks
>
>
> -----Original Message-----
> From: Marek Behún <kabel@kernel.org>
> Sent: Wednesday, January 12, 2022 6:07 PM
> To: Stefan Roese <sr@denx.de>; Mario Six <mario.six@gdsys.cc>; Dennis Gilmore <dgilmore@redhat.com>; Kostya Porotchkin <kostap@marvell.com>
> Cc: Pali Rohár <pali@kernel.org>; u-boot@lists.denx.de; Moti Buskila <motib@marvell.com>; Marek Behún <marek.behun@nic.cz>
> Subject: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
>
> External Email
>
> ----------------------------------------------------------------------
> From: Marek Behún <marek.behun@nic.cz>
>
> This is a cleaned up and fixed version of a patch
> mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>
> in each pattern cycle the bus state can be changed
> in order to avoide it, need to back to the same bus state on each
> pattern cycle
> by
> Moti Boskula <motib@marvell.com>
>
> The original patch is not in Marvell's mv-ddr-marvell repository. It was gives to us by Marvell to fix an issues with DDR training on some boards, but it cannot be applied as is to mv-ddr-marvell, because it is a very dirty draft patch that would certainly break other things, mainly
> DDR4 training code in mv-ddr-marvell, since it changes common functions.
>
> I have cleaned up the patch and removed stuff that seemed unnecessary (when removed, it still fixed things). Note that I don't understand completely what the code does exactly, since I haven't studied the DDR training code extensively (and I suspect that no one besides some few people in Marvell understand the code completely).
>
> Anyway after the cleanup the patch still fixes isssues with DDR training on the failing boards.
>
> There was also a problem with the original patch on some of the Allied Telesis' x530 boards, reported by Chris Packham. I have asked Chris to send me some logs, and managed to fix it:
> - if you look at the change, you'll notice that it introduces
> subtraction of cur_start_win[] and cur_end_win[] members, depending on
> a bit set in the current_byte_status variable
> - the original patch subtracted cur_start_win[] if either
> BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
> subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
> was set
> - from Chris Packham logs I discovered that the x530 board where the
> original patch introduced DDR training failure, only the
> BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
> patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
> current_byte_status variable
> - this led me to the hypothesis that both cur_start_win[] and
> cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
> set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at all
> - this hypothesis also gains credibility when considering the commit
> title ("fix SPLIT_OUT_MIX state decision")
>
> Hopefully this will fix things without breaking anything else.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> .../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> index 648b37ef6f..42308b6965 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> @@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
> u32 if_id, pattern_id, bit_id;
> u8 bus_id;
> + u8 current_byte_status;
> u8 cur_start_win[BUS_WIDTH_IN_BITS];
> u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
> u8 cur_end_win[BUS_WIDTH_IN_BITS];
> @@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> result[search_dir_id][7]));
> }
>
> + current_byte_status =
> + mv_ddr_tip_sub_phy_byte_status_get(if_id,
> + bus_id);
> +
> for (bit_id = 0; bit_id < BUS_WIDTH_IN_BITS;
> bit_id++) {
> /* check if this code is valid for 2 edge, probably not :( */ @@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> [HWS_LOW2HIGH]
> [bit_id],
> EDGE_1);
> + if (current_byte_status &
> + BYTE_SPLIT_OUT_MIX) {
> + if (cur_start_win[bit_id] >= 64)
> + cur_start_win[bit_id] -= 64;
> + else
> + cur_start_win[bit_id] = 0;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from start\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> cur_end_win[bit_id] =
> GET_TAP_RESULT(result
> [HWS_HIGH2LOW]
> [bit_id],
> EDGE_1);
> + if (cur_end_win[bit_id] >= 64 &&
> + (current_byte_status &
> + BYTE_SPLIT_OUT_MIX)) {
> + cur_end_win[bit_id] -= 64;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from end\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> +
> /* window length */
> current_window[bit_id] =
> cur_end_win[bit_id] -
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
2022-01-17 14:23 ` Marek Behún
@ 2022-01-17 15:07 ` Moti Buskila
0 siblings, 0 replies; 7+ messages in thread
From: Moti Buskila @ 2022-01-17 15:07 UTC (permalink / raw)
To: Marek Behún, Assaf Hoffman
Cc: Stefan Roese, Mario Six, Dennis Gilmore, Kostya Porotchkin,
Pali Rohár, u-boot, Marek Behún
Hi Marek
This mail was send by accident.
Assaf H, is taking care of that.
Thanks
-----Original Message-----
From: Marek Behún <kabel@kernel.org>
Sent: Monday, January 17, 2022 4:23 PM
To: Moti Buskila <motib@marvell.com>
Cc: Stefan Roese <sr@denx.de>; Mario Six <mario.six@gdsys.cc>; Dennis Gilmore <dgilmore@redhat.com>; Kostya Porotchkin <kostap@marvell.com>; Pali Rohár <pali@kernel.org>; u-boot@lists.denx.de; Marek Behún <marek.behun@nic.cz>
Subject: Re: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision
Hello Moti,
since you're the author of the original version of this patch, could you please review it and if it is okay, put it into mv-ddr-marvell?
Thanks.
Marek
On Mon, 17 Jan 2022 06:52:08 +0000
Moti Buskila <motib@marvell.com> wrote:
> Hi Assaf,
> I've got this email a few days ago.
> Is it related to what you’ve send me?
> Thanks
>
>
> -----Original Message-----
> From: Marek Behún <kabel@kernel.org>
> Sent: Wednesday, January 12, 2022 6:07 PM
> To: Stefan Roese <sr@denx.de>; Mario Six <mario.six@gdsys.cc>; Dennis
> Gilmore <dgilmore@redhat.com>; Kostya Porotchkin <kostap@marvell.com>
> Cc: Pali Rohár <pali@kernel.org>; u-boot@lists.denx.de; Moti Buskila
> <motib@marvell.com>; Marek Behún <marek.behun@nic.cz>
> Subject: [EXT] [PATCH u-boot-marvell] ddr: marvell: a38x: fix
> SPLIT_OUT_MIX state decision
>
> External Email
>
> ----------------------------------------------------------------------
> From: Marek Behún <marek.behun@nic.cz>
>
> This is a cleaned up and fixed version of a patch
> mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>
> in each pattern cycle the bus state can be changed
> in order to avoide it, need to back to the same bus state on each
> pattern cycle
> by
> Moti Boskula <motib@marvell.com>
>
> The original patch is not in Marvell's mv-ddr-marvell repository. It
> was gives to us by Marvell to fix an issues with DDR training on some
> boards, but it cannot be applied as is to mv-ddr-marvell, because it
> is a very dirty draft patch that would certainly break other things,
> mainly
> DDR4 training code in mv-ddr-marvell, since it changes common functions.
>
> I have cleaned up the patch and removed stuff that seemed unnecessary (when removed, it still fixed things). Note that I don't understand completely what the code does exactly, since I haven't studied the DDR training code extensively (and I suspect that no one besides some few people in Marvell understand the code completely).
>
> Anyway after the cleanup the patch still fixes isssues with DDR training on the failing boards.
>
> There was also a problem with the original patch on some of the Allied Telesis' x530 boards, reported by Chris Packham. I have asked Chris to send me some logs, and managed to fix it:
> - if you look at the change, you'll notice that it introduces
> subtraction of cur_start_win[] and cur_end_win[] members, depending on
> a bit set in the current_byte_status variable
> - the original patch subtracted cur_start_win[] if either
> BYTE_SPLIT_OUT_MIX or BYTE_HOMOGENEOUS_SPLIT_OUT bits were set, but
> subtracted cur_end_win[] only if the first one (BYTE_SPLIT_OUT_MIX)
> was set
> - from Chris Packham logs I discovered that the x530 board where the
> original patch introduced DDR training failure, only the
> BYTE_HOMOGENEOUS_SPLIT_OUT bit was set, and on our boards where the
> patch is needed only the BYTE_SPLIT_OUT_MIX is set in the
> current_byte_status variable
> - this led me to the hypothesis that both cur_start_win[] and
> cur_end_win[] should be subtracted only if BYTE_SPLIT_OUT_MIX bit is
> set, the BYTE_HOMOGENEOUS_SPLIT_OUT bit shouldn't be considered at
> all
> - this hypothesis also gains credibility when considering the commit
> title ("fix SPLIT_OUT_MIX state decision")
>
> Hopefully this will fix things without breaking anything else.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> .../a38x/ddr3_training_centralization.c | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> index 648b37ef6f..42308b6965 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> @@ -55,6 +55,7 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> enum hws_training_ip_stat training_result[MAX_INTERFACE_NUM];
> u32 if_id, pattern_id, bit_id;
> u8 bus_id;
> + u8 current_byte_status;
> u8 cur_start_win[BUS_WIDTH_IN_BITS];
> u8 centralization_result[MAX_INTERFACE_NUM][BUS_WIDTH_IN_BITS];
> u8 cur_end_win[BUS_WIDTH_IN_BITS];
> @@ -166,6 +167,10 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> result[search_dir_id][7]));
> }
>
> + current_byte_status =
> + mv_ddr_tip_sub_phy_byte_status_get(if_id,
> + bus_id);
> +
> for (bit_id = 0; bit_id < BUS_WIDTH_IN_BITS;
> bit_id++) {
> /* check if this code is valid for 2 edge, probably not :( */ @@ -174,11 +179,32 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
> [HWS_LOW2HIGH]
> [bit_id],
> EDGE_1);
> + if (current_byte_status &
> + BYTE_SPLIT_OUT_MIX) {
> + if (cur_start_win[bit_id] >= 64)
> + cur_start_win[bit_id] -= 64;
> + else
> + cur_start_win[bit_id] = 0;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from start\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> cur_end_win[bit_id] =
> GET_TAP_RESULT(result
> [HWS_HIGH2LOW]
> [bit_id],
> EDGE_1);
> + if (cur_end_win[bit_id] >= 64 &&
> + (current_byte_status &
> + BYTE_SPLIT_OUT_MIX)) {
> + cur_end_win[bit_id] -= 64;
> + DEBUG_CENTRALIZATION_ENGINE
> + (DEBUG_LEVEL_INFO,
> + ("pattern %d IF %d pup %d bit %d subtract 64 adll from end\n",
> + pattern_id, if_id, bus_id, bit_id));
> + }
> +
> /* window length */
> current_window[bit_id] =
> cur_end_win[bit_id] -
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-17 15:56 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12 16:06 [PATCH u-boot-marvell] ddr: marvell: a38x: fix SPLIT_OUT_MIX state decision Marek Behún
2022-01-13 6:29 ` Stefan Roese
2022-01-13 7:13 ` Chris Packham
2022-01-14 15:38 ` Stefan Roese
2022-01-17 6:52 ` [EXT] " Moti Buskila
2022-01-17 14:23 ` Marek Behún
2022-01-17 15:07 ` Moti Buskila
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.