[coreboot-gerrit] Change in coreboot[master]: soc/intel/apollolake: Add CA ODT config to memory

[Ravishankar Sarawadi (Code Review)]

Ravishankar Sarawadi has posted comments on this change. ( https://review.coreboot.org/19397 )

Change subject: soc/intel/apollolake: Add CA ODT config to memory
......................................................................


Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/19397/3/src/soc/intel/apollolake/include/soc/meminit.h
File src/soc/intel/apollolake/include/soc/meminit.h:

Line 76: 	LP4_CAODT_A_B_HIGH_LOW = 0,   /* default */
> You should shift these values by 1 in the enum.
I have defined CA ODT bitmask as 0x1 which will be used to set Ch#_OdtConfig value, no need to shift here i think. 

Enum is only listing CA ODT bit definitions i.e.
0 - ODT_AB_HIGH_LOW,
1 - ODT_AB_HIGH_HIGH. 

MRC will set LPDDR4 MR11 value i.e.  (CAODT_A_B_HIGH_LOW ->40 Ohm and CAODT_A_B_HIGH_HIGH->60 Ohm final impedance) based on CA ODT bit setting. 

Could you please explain the need of shift by 1?

I think, to avoid confusion, I should rename enum members to:

enum {
	ODT_A_B_HIGH_LOW = 0,
	ODT_A_B_HIGH_HIGH = 1,
} 

same as FSP_M_CONFIG.


https://review.coreboot.org/#/c/19397/3/src/soc/intel/apollolake/meminit.c
File src/soc/intel/apollolake/meminit.c:

Line 259: 	cfg->Ch3_OdtConfig |= lpcfg->ca_odt_config;
> But what's the value in that UPD defaults? We can't just rely on OR'ing in 
UPD default should be 0, as it is placed in bss that is supposed to be cleared.

I will revert to have default setting to have Ch#OdtConfig=0x2 i.e. CA ODT being ODT_AB_HIGH_HIGH.

Then mainboard change #19449 is not needed. Mainboard will overwrite defaults only if not applicable.


-- 
To view, visit https://review.coreboot.org/19397
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a2f7636b46492a9d08472a0752cdf1f86a72e15
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov at intel.com>
Gerrit-Reviewer: Freddy Paul <freddy.paul at intel.com>
Gerrit-Reviewer: Kane Chen <kane.chen at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi at intel.com>
Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes