diff mbox series

[u-boot-marvell,RFC] PLEASE TEST: ddr: marvell: a38x: fix BYTE_HOMOGENEOUS_SPLIT_OUT decision

Message ID 20220210162517.8485-1-kabel@kernel.org
State RFC
Delegated to: Stefan Roese
Headers show
Series [u-boot-marvell,RFC] PLEASE TEST: ddr: marvell: a38x: fix BYTE_HOMOGENEOUS_SPLIT_OUT decision | expand

Commit Message

Marek Behún Feb. 10, 2022, 4:25 p.m. UTC 
From: Marek Behún <marek.behun@nic.cz>

In commit 3fc92a215b69 ("ddr: marvell: a38x: fix SPLIT_OUT_MIX state
decision") I ported a cleaned up and changed version of patch
  mv_ddr: a380: fix SPLIT_OUT_MIX state decision

In the port we removed checking for BYTE_HOMOGENEOUS_SPLIT_OUT bit,
because:
- the fix seemed to work without it
- the bit was checked for only at one place out of two, while the second
  bit, BYTE_SPLIT_OUT_MIX, was checked for in both cases
- without the removal it didn't work on Allied Telesis' x530 board

We recently had a chance to test on more boards, and it seems that the
change needs to be opposite: instead of removing the check for
BYTE_HOMOGENEOUS_SPLIT_OUT from the first if() statement, the check
needs to be added also to the second one - it needs to be at both
places.

With this change all the Turris Omnia boards I have had available to
test seem to work, I didn't encounter not even one failed DDR training.

As last time, I am noting that I do not understand what this code is
actually doing, I haven't studied the DDR training algorithm and
I suspect that no one will be able to explain it to U-Boot contributors,
so we are left with this blind poking in the code with testing whether
it works on several boards and hoping it doesn't break anything for
anyone :-(.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/ddr/marvell/a38x/ddr3_training_centralization.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Chris Packham Feb. 15, 2022, 10:21 p.m. UTC | #1 
On Fri, Feb 11, 2022 at 5:25 AM Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> In commit 3fc92a215b69 ("ddr: marvell: a38x: fix SPLIT_OUT_MIX state
> decision") I ported a cleaned up and changed version of patch
>   mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>
> In the port we removed checking for BYTE_HOMOGENEOUS_SPLIT_OUT bit,
> because:
> - the fix seemed to work without it
> - the bit was checked for only at one place out of two, while the second
>   bit, BYTE_SPLIT_OUT_MIX, was checked for in both cases
> - without the removal it didn't work on Allied Telesis' x530 board
>
> We recently had a chance to test on more boards, and it seems that the
> change needs to be opposite: instead of removing the check for
> BYTE_HOMOGENEOUS_SPLIT_OUT from the first if() statement, the check
> needs to be added also to the second one - it needs to be at both
> places.
>
> With this change all the Turris Omnia boards I have had available to
> test seem to work, I didn't encounter not even one failed DDR training.
>
> As last time, I am noting that I do not understand what this code is
> actually doing, I haven't studied the DDR training algorithm and
> I suspect that no one will be able to explain it to U-Boot contributors,
> so we are left with this blind poking in the code with testing whether
> it works on several boards and hoping it doesn't break anything for
> anyone :-(.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Tested-by: Chris Packham <judge.packham@gmail.com>

> ---
>  drivers/ddr/marvell/a38x/ddr3_training_centralization.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> index 42308b6965..be9f985f22 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
> @@ -180,7 +180,8 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
>                                                                [bit_id],
>                                                                EDGE_1);
>                                         if (current_byte_status &
> -                                           BYTE_SPLIT_OUT_MIX) {
> +                                           (BYTE_SPLIT_OUT_MIX |
> +                                            BYTE_HOMOGENEOUS_SPLIT_OUT)) {
>                                                 if (cur_start_win[bit_id] >= 64)
>                                                         cur_start_win[bit_id] -= 64;
>                                                 else
> @@ -197,7 +198,8 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
>                                                                EDGE_1);
>                                         if (cur_end_win[bit_id] >= 64 &&
>                                             (current_byte_status &
> -                                            BYTE_SPLIT_OUT_MIX)) {
> +                                            (BYTE_SPLIT_OUT_MIX |
> +                                             BYTE_HOMOGENEOUS_SPLIT_OUT))) {
>                                                 cur_end_win[bit_id] -= 64;
>                                                 DEBUG_CENTRALIZATION_ENGINE
>                                                         (DEBUG_LEVEL_INFO,
> --
> 2.34.1
>
Stefan Roese Feb. 16, 2022, 8:41 a.m. UTC | #2 
On 2/15/22 23:21, Chris Packham wrote:
> On Fri, Feb 11, 2022 at 5:25 AM Marek Behún <kabel@kernel.org> wrote:
>>
>> From: Marek Behún <marek.behun@nic.cz>
>>
>> In commit 3fc92a215b69 ("ddr: marvell: a38x: fix SPLIT_OUT_MIX state
>> decision") I ported a cleaned up and changed version of patch
>>    mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>>
>> In the port we removed checking for BYTE_HOMOGENEOUS_SPLIT_OUT bit,
>> because:
>> - the fix seemed to work without it
>> - the bit was checked for only at one place out of two, while the second
>>    bit, BYTE_SPLIT_OUT_MIX, was checked for in both cases
>> - without the removal it didn't work on Allied Telesis' x530 board
>>
>> We recently had a chance to test on more boards, and it seems that the
>> change needs to be opposite: instead of removing the check for
>> BYTE_HOMOGENEOUS_SPLIT_OUT from the first if() statement, the check
>> needs to be added also to the second one - it needs to be at both
>> places.
>>
>> With this change all the Turris Omnia boards I have had available to
>> test seem to work, I didn't encounter not even one failed DDR training.
>>
>> As last time, I am noting that I do not understand what this code is
>> actually doing, I haven't studied the DDR training algorithm and
>> I suspect that no one will be able to explain it to U-Boot contributors,
>> so we are left with this blind poking in the code with testing whether
>> it works on several boards and hoping it doesn't break anything for
>> anyone :-(.
>>
>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> 
> Tested-by: Chris Packham <judge.packham@gmail.com>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> 
>> ---
>>   drivers/ddr/marvell/a38x/ddr3_training_centralization.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
>> index 42308b6965..be9f985f22 100644
>> --- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
>> +++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
>> @@ -180,7 +180,8 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
>>                                                                 [bit_id],
>>                                                                 EDGE_1);
>>                                          if (current_byte_status &
>> -                                           BYTE_SPLIT_OUT_MIX) {
>> +                                           (BYTE_SPLIT_OUT_MIX |
>> +                                            BYTE_HOMOGENEOUS_SPLIT_OUT)) {
>>                                                  if (cur_start_win[bit_id] >= 64)
>>                                                          cur_start_win[bit_id] -= 64;
>>                                                  else
>> @@ -197,7 +198,8 @@ static int ddr3_tip_centralization(u32 dev_num, u32 mode)
>>                                                                 EDGE_1);
>>                                          if (cur_end_win[bit_id] >= 64 &&
>>                                              (current_byte_status &
>> -                                            BYTE_SPLIT_OUT_MIX)) {
>> +                                            (BYTE_SPLIT_OUT_MIX |
>> +                                             BYTE_HOMOGENEOUS_SPLIT_OUT))) {
>>                                                  cur_end_win[bit_id] -= 64;
>>                                                  DEBUG_CENTRALIZATION_ENGINE
>>                                                          (DEBUG_LEVEL_INFO,
>> --
>> 2.34.1
>>

Viele Grüße,
Stefan Roese
Marek Behún Feb. 16, 2022, 1:26 p.m. UTC | #3 
On Wed, 16 Feb 2022 09:41:44 +0100
Stefan Roese <sr@denx.de> wrote:

> On 2/15/22 23:21, Chris Packham wrote:
> > On Fri, Feb 11, 2022 at 5:25 AM Marek Behún <kabel@kernel.org> wrote:  
> >>
> >> From: Marek Behún <marek.behun@nic.cz>
> >>
> >> In commit 3fc92a215b69 ("ddr: marvell: a38x: fix SPLIT_OUT_MIX state
> >> decision") I ported a cleaned up and changed version of patch
> >>    mv_ddr: a380: fix SPLIT_OUT_MIX state decision
> >>
> >> In the port we removed checking for BYTE_HOMOGENEOUS_SPLIT_OUT bit,
> >> because:
> >> - the fix seemed to work without it
> >> - the bit was checked for only at one place out of two, while the second
> >>    bit, BYTE_SPLIT_OUT_MIX, was checked for in both cases
> >> - without the removal it didn't work on Allied Telesis' x530 board
> >>
> >> We recently had a chance to test on more boards, and it seems that the
> >> change needs to be opposite: instead of removing the check for
> >> BYTE_HOMOGENEOUS_SPLIT_OUT from the first if() statement, the check
> >> needs to be added also to the second one - it needs to be at both
> >> places.
> >>
> >> With this change all the Turris Omnia boards I have had available to
> >> test seem to work, I didn't encounter not even one failed DDR training.
> >>
> >> As last time, I am noting that I do not understand what this code is
> >> actually doing, I haven't studied the DDR training algorithm and
> >> I suspect that no one will be able to explain it to U-Boot contributors,
> >> so we are left with this blind poking in the code with testing whether
> >> it works on several boards and hoping it doesn't break anything for
> >> anyone :-(.
> >>
> >> Signed-off-by: Marek Behún <marek.behun@nic.cz>  
> > 
> > Tested-by: Chris Packham <judge.packham@gmail.com>  
> 
> Reviewed-by: Stefan Roese <sr@denx.de>

Dear Stefan,

will you apply this (removing the PLEASE TEST: prefix) or should I send
it again without it?

I am going to send another patch which will reset the board immediately
if DDR training fails, so I can send with that one.

Marek
Stefan Roese Feb. 16, 2022, 1:41 p.m. UTC | #4 
On 2/16/22 14:26, Marek Behún wrote:
> On Wed, 16 Feb 2022 09:41:44 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> On 2/15/22 23:21, Chris Packham wrote:
>>> On Fri, Feb 11, 2022 at 5:25 AM Marek Behún <kabel@kernel.org> wrote:
>>>>
>>>> From: Marek Behún <marek.behun@nic.cz>
>>>>
>>>> In commit 3fc92a215b69 ("ddr: marvell: a38x: fix SPLIT_OUT_MIX state
>>>> decision") I ported a cleaned up and changed version of patch
>>>>     mv_ddr: a380: fix SPLIT_OUT_MIX state decision
>>>>
>>>> In the port we removed checking for BYTE_HOMOGENEOUS_SPLIT_OUT bit,
>>>> because:
>>>> - the fix seemed to work without it
>>>> - the bit was checked for only at one place out of two, while the second
>>>>     bit, BYTE_SPLIT_OUT_MIX, was checked for in both cases
>>>> - without the removal it didn't work on Allied Telesis' x530 board
>>>>
>>>> We recently had a chance to test on more boards, and it seems that the
>>>> change needs to be opposite: instead of removing the check for
>>>> BYTE_HOMOGENEOUS_SPLIT_OUT from the first if() statement, the check
>>>> needs to be added also to the second one - it needs to be at both
>>>> places.
>>>>
>>>> With this change all the Turris Omnia boards I have had available to
>>>> test seem to work, I didn't encounter not even one failed DDR training.
>>>>
>>>> As last time, I am noting that I do not understand what this code is
>>>> actually doing, I haven't studied the DDR training algorithm and
>>>> I suspect that no one will be able to explain it to U-Boot contributors,
>>>> so we are left with this blind poking in the code with testing whether
>>>> it works on several boards and hoping it doesn't break anything for
>>>> anyone :-(.
>>>>
>>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>>
>>> Tested-by: Chris Packham <judge.packham@gmail.com>
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Dear Stefan,
> 
> will you apply this (removing the PLEASE TEST: prefix) or should I send
> it again without it?
> 
> I am going to send another patch which will reset the board immediately
> if DDR training fails, so I can send with that one.

Whatever you prefer. Plan is to apply/push some of the pending patches
in the next few days.

Thanks,
Stefan
diff mbox series

Patch

diff --git a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
index 42308b6965..be9f985f22 100644
--- a/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
+++ b/drivers/ddr/marvell/a38x/ddr3_training_centralization.c
@@ -180,7 +180,8 @@  static int ddr3_tip_centralization(u32 dev_num, u32 mode)
 							       [bit_id],
 							       EDGE_1);
 					if (current_byte_status &
-					    BYTE_SPLIT_OUT_MIX) {
+					    (BYTE_SPLIT_OUT_MIX |
+					     BYTE_HOMOGENEOUS_SPLIT_OUT)) {
 						if (cur_start_win[bit_id] >= 64)
 							cur_start_win[bit_id] -= 64;
 						else
@@ -197,7 +198,8 @@  static int ddr3_tip_centralization(u32 dev_num, u32 mode)
 							       EDGE_1);
 					if (cur_end_win[bit_id] >= 64 &&
 					    (current_byte_status &
-					     BYTE_SPLIT_OUT_MIX)) {
+					     (BYTE_SPLIT_OUT_MIX |
+					      BYTE_HOMOGENEOUS_SPLIT_OUT))) {
 						cur_end_win[bit_id] -= 64;
 						DEBUG_CENTRALIZATION_ENGINE
 							(DEBUG_LEVEL_INFO,