Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47576 )
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
nb/intel/sandybridge: Rename `read_training` function
Given that it sets the receive enable mode bit in the GDCRTRAININGMOD register, it's clear that this is about receive enable calibration.
Change-Id: Iaefc8905adf2878bec3b43494dc53530064a9f5d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_native.c 3 files changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/47576/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index f20181e..9da5a97 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1307,7 +1307,7 @@ * Once the controller has detected this pattern a bit in the result register is set for the * current phase shift. */ -int read_training(ramctr_timing *ctrl) +int receive_enable_calibration(ramctr_timing *ctrl) { int channel, slotrank, lane; int err; diff --git a/src/northbridge/intel/sandybridge/raminit_common.h b/src/northbridge/intel/sandybridge/raminit_common.h index de74af5..5227536 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.h +++ b/src/northbridge/intel/sandybridge/raminit_common.h @@ -400,7 +400,7 @@ void dram_zones(ramctr_timing *ctrl, int training); void dram_memorymap(ramctr_timing *ctrl, int me_uma_size); void dram_jedecreset(ramctr_timing *ctrl); -int read_training(ramctr_timing *ctrl); +int receive_enable_calibration(ramctr_timing *ctrl); int write_training(ramctr_timing *ctrl); int command_training(ramctr_timing *ctrl); int discover_edges(ramctr_timing *ctrl); diff --git a/src/northbridge/intel/sandybridge/raminit_native.c b/src/northbridge/intel/sandybridge/raminit_native.c index 53017eb..20009cb 100644 --- a/src/northbridge/intel/sandybridge/raminit_native.c +++ b/src/northbridge/intel/sandybridge/raminit_native.c @@ -680,7 +680,7 @@ /* Prepare for memory training */ prepare_training(ctrl);
- err = read_training(ctrl); + err = receive_enable_calibration(ctrl); if (err) return err;
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47576 )
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47576/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47576/2/src/northbridge/intel/sandy... PS2, Line 1315: Compensate the skew between DQS and DQs. : * : * To ease PCB design, a small skew between Data Strobe signals and Data Signals is allowed. : * The controller has to measure and compensate this skew for every byte-lane. By delaying : * either all DQ signals or DQS signal, a full phase shift can be introduced. It is assumed : * that one byte-lane's DQs signals have the same routing delay. : * : * To measure the actual skew, the DRAM is placed in "read leveling" mode. In read leveling : * mode the DRAM-chip outputs an alternating periodic pattern. The memory controller iterates : * over all possible values to do a full phase shift and issues read commands. With DQS and : * DQ in phase the data being read is expected to alternate on every byte: : * : * 0xFF 0x00 0xFF ... : * : * Once the controller has detected this pattern a bit in the result register is set for the : * current phase shift. Is this still accurate?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47576 )
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47576/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47576/2/src/northbridge/intel/sandy... PS2, Line 1315: Compensate the skew between DQS and DQs. : * : * To ease PCB design, a small skew between Data Strobe signals and Data Signals is allowed. : * The controller has to measure and compensate this skew for every byte-lane. By delaying : * either all DQ signals or DQS signal, a full phase shift can be introduced. It is assumed : * that one byte-lane's DQs signals have the same routing delay. : * : * To measure the actual skew, the DRAM is placed in "read leveling" mode. In read leveling : * mode the DRAM-chip outputs an alternating periodic pattern. The memory controller iterates : * over all possible values to do a full phase shift and issues read commands. With DQS and : * DQ in phase the data being read is expected to alternate on every byte: : * : * 0xFF 0x00 0xFF ... : * : * Once the controller has detected this pattern a bit in the result register is set for the : * current phase shift.
Is this still accurate?
Still not sure. I'd rather rewrite the comments after having cleaned up the whole thing, though.
Hello build bot (Jenkins), Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47576
to look at the new patch set (#3).
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
nb/intel/sandybridge: Rename `read_training` function
Given that it sets the receive enable mode bit in the GDCRTRAININGMOD register, it's clear that this is about receive enable calibration.
Remove a potentially-outdated comment. Proper documentation will be written once code refactoring and various improvements are complete.
Change-Id: Iaefc8905adf2878bec3b43494dc53530064a9f5d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_native.c 3 files changed, 3 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/47576/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47576 )
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47576/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47576/2/src/northbridge/intel/sandy... PS2, Line 1315: Compensate the skew between DQS and DQs. : * : * To ease PCB design, a small skew between Data Strobe signals and Data Signals is allowed. : * The controller has to measure and compensate this skew for every byte-lane. By delaying : * either all DQ signals or DQS signal, a full phase shift can be introduced. It is assumed : * that one byte-lane's DQs signals have the same routing delay. : * : * To measure the actual skew, the DRAM is placed in "read leveling" mode. In read leveling : * mode the DRAM-chip outputs an alternating periodic pattern. The memory controller iterates : * over all possible values to do a full phase shift and issues read commands. With DQS and : * DQ in phase the data being read is expected to alternate on every byte: : * : * 0xFF 0x00 0xFF ... : * : * Once the controller has detected this pattern a bit in the result register is set for the : * current phase shift.
Still not sure. I'd rather rewrite the comments after having cleaned up the whole thing, though.
Gone
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47576 )
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
Patch Set 6: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47576 )
Change subject: nb/intel/sandybridge: Rename `read_training` function ......................................................................
nb/intel/sandybridge: Rename `read_training` function
Given that it sets the receive enable mode bit in the GDCRTRAININGMOD register, it's clear that this is about receive enable calibration.
Remove a potentially-outdated comment. Proper documentation will be written once code refactoring and various improvements are complete.
Change-Id: Iaefc8905adf2878bec3b43494dc53530064a9f5d Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47576 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/sandybridge/raminit_common.c M src/northbridge/intel/sandybridge/raminit_common.h M src/northbridge/intel/sandybridge/raminit_native.c 3 files changed, 3 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 5aaafdd..5332e24 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -1316,25 +1316,7 @@ printram("4028 -= %d;\n", logic_delay_min); }
-/* - * Compensate the skew between DQS and DQs. - * - * To ease PCB design, a small skew between Data Strobe signals and Data Signals is allowed. - * The controller has to measure and compensate this skew for every byte-lane. By delaying - * either all DQ signals or DQS signal, a full phase shift can be introduced. It is assumed - * that one byte-lane's DQs signals have the same routing delay. - * - * To measure the actual skew, the DRAM is placed in "read leveling" mode. In read leveling - * mode the DRAM-chip outputs an alternating periodic pattern. The memory controller iterates - * over all possible values to do a full phase shift and issues read commands. With DQS and - * DQ in phase the data being read is expected to alternate on every byte: - * - * 0xFF 0x00 0xFF ... - * - * Once the controller has detected this pattern a bit in the result register is set for the - * current phase shift. - */ -int read_training(ramctr_timing *ctrl) +int receive_enable_calibration(ramctr_timing *ctrl) { int channel, slotrank, lane; int err; diff --git a/src/northbridge/intel/sandybridge/raminit_common.h b/src/northbridge/intel/sandybridge/raminit_common.h index 4a7e806..10dd59d 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.h +++ b/src/northbridge/intel/sandybridge/raminit_common.h @@ -400,7 +400,7 @@ void dram_zones(ramctr_timing *ctrl, int training); void dram_memorymap(ramctr_timing *ctrl, int me_uma_size); void dram_jedecreset(ramctr_timing *ctrl); -int read_training(ramctr_timing *ctrl); +int receive_enable_calibration(ramctr_timing *ctrl); int write_training(ramctr_timing *ctrl); int command_training(ramctr_timing *ctrl); int read_mpr_training(ramctr_timing *ctrl); diff --git a/src/northbridge/intel/sandybridge/raminit_native.c b/src/northbridge/intel/sandybridge/raminit_native.c index 454ba01..e0b5a3d 100644 --- a/src/northbridge/intel/sandybridge/raminit_native.c +++ b/src/northbridge/intel/sandybridge/raminit_native.c @@ -680,7 +680,7 @@ /* Prepare for memory training */ prepare_training(ctrl);
- err = read_training(ctrl); + err = receive_enable_calibration(ctrl); if (err) return err;
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Felix Held (Code Review)