Re: [coreboot] GPIO PADRSTCFG conflict in datasheet

2017-05-25 Thread Youness Alaoui
Hi,

I just found this file here :
https://github.com/IntelFsp/FSP/blob/Skylake/SkylakeFspBinPkg/Include/GpioConfig.h
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-datasheet-vol-2.html).
> 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_RSMRST3
> #define PADRSTCFG_GPP_RSMRST0
>
> 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.

-- 
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot


[coreboot] GPIO PADRSTCFG conflict in datasheet

2017-05-18 Thread Youness Alaoui
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-datasheet-vol-2.html).
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_RSMRST3
#define PADRSTCFG_GPP_RSMRST0

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.
-- 
coreboot mailing list: coreboot@coreboot.org
https://mail.coreboot.org/mailman/listinfo/coreboot