Re: [coreboot] GPIO PADRSTCFG conflict in datasheet

[Youness Alaoui]

Hi,

I just found this file here :
https://github.com/IntelFsp/FSP/blob/Skylake/SkylakeFspBinPkg/Include/GpioCo...
It defines the reset values for GPIO in this enum :
typedef enum {
GpioResetDefault = 0x0, ///< Leave value of pad reset unmodified
GpioResetPwrGood = 0x1, ///< GPP: RSMRST; GPD: DSW_PWROK; (PadRstCfg =
00b = "Powergood")
GpioResetDeep = 0x3, ///< Deep GPIO Reset (PadRstCfg = 01b = "Deep GPIO Reset")
GpioResetNormal = 0x5, ///< GPIO Reset (PadRstCfg = 10b = "GPIO Reset" )
GpioResetResume = 0x7 ///< GPP: Reserved; GPD: RSMRST; (PadRstCfg =
11b = "Resume Reset" )
} GPIO_RESET_CONFIG;

We can see it specifically says that 00b is "GPP: RSMRST; GPD:
DSW_PWROK" and 11b is "GPP: Reserved; GPD: RSMRST". So I'm leaning
towards the datasheet not having a typo, but rather they just have
different values for RSMRST depending on whether it's GPP or GPD, so I
vote for changing the define in coreboot so it stops being confusing
for those who want to use RSMRST.

I'll send a patch shortly to do it as I suggested in my previous
email, unless someone else has a better idea.

Thanks,
Youness.

On Thu, May 18, 2017 at 2:56 PM, Youness Alaoui
 wrote:
> Hi,
>
> I'm working on a skylake port and I've noticed something with the PADRSTCFG
> field of the GPIO Pad configuration. If you look at the 100-series datasheet
> volume 2
> (https://www-ssl.intel.com/content/www/us/en/chipsets/100-series-chipset-data...).
> The Pad configuration DW0 for GPP_A, GPP_B, GPP_C, etc.. (page 1179, 1249)
> up to GPP_I (page 1319) has bits 31:30 (PADRSTCFG) defined like this :
> 00 = RSMRST#
> 01 = Host deep reset.
> 10 = PLTRST#
> 11 = Reserved
>
> But the Pad configuration DW0 for GPD pads (page 1296) has it defined like
> this :
> 00 = DSW_PWROK
> 01 = Host deep reset.
> 10 = PLTRST#
> 11 = RSMRST#
>
> The problem here is that while the DEEP and PLTRST values are the same, the
> RSMRST value is 0 for GPP gpio pads, and 3 for GPD gpio pads. In the skylake
> and apollolake gpio_defs.h file, it is defined like this (with different
> naming for apollolake but same values) :
> #define  PADRSTCFG_DSW_PWROK 0
> #define  PADRSTCFG_DEEP 1
> #define  PADRSTCFG_PLTRST 2
> #define  PADRSTCFG_RSMRST 3
>
> These defines are only valid for GPD pads, if someone was to use a RSMRST
> reset config on a GPP pad, they would be setting the pad to a 'reserved'
> configuration, which could have unexpected behavior.
> There are two possibilities here, either the datasheet is wrong (and PWROK
> should be 3 so the RSMRST value is consistent for both GPP and GPD), or the
> datasheet is correct, and in both cases, the value in coreboot is wrong.
>
> If someone from Intel is reading this list, or if someone here knows someone
> inside Intel who could confirm whether or not the datasheet has an error,
> that would be great.
> In the meantime, I would maybe suggest to change the defines to  :
> #define PADRSTCFG_GPD_RSMRST    3
> #define PADRSTCFG_GPP_RSMRST    0
>
> This would affect both skylake and apollolake, and note that the only
> skylake+ boards that use RSMRST seem to be the kblrvp in the rvp7 variant
> which sets it for GPP_A12 and GPP_E3 (which means those two gpios have the
> wrong setting due to the wrong define).
>
> What do you think ?
>
> Thanks,
> Youness.