Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
On Wed, 2018-05-30 at 17:03 +0530, Rajneesh Bhardwaj wrote:
> On Wed, May 30, 2018 at 03:53:12AM -0700, David E. Box wrote:
>
> Hi Dave,
>
> > Hi Rajneesh,
> >
> > On Mon, 2018-05-28 at 12:30 +0530, Rajneesh Bhardwaj wrote:
> > > On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
> > >
> > > Thanks for sending this, Dave. Few comments below.
> > >
> > > > Adds debugfs access to registers in the Cannon Point PCH PMC
> > > > that
> > > > are
> > >
> > > Please use Cannonlake PCH.
> > >
> > > > useful for debugging #SLP_S0 signal assertion and other low
> > > > power
> > > > related
> > >
> > > assertion failures and other low power debug.
> > >
> > > > activities. Device pm states are latched in these registers
> > > > whenever the
> > > > package enters C10 and can be read from slp_s0_debug_status.
> > > > The pm
> > > > states may also be latched by writing 1 to slp_s0_dbg_latch
> > > > which
> > > > will
> > > > immediately capture the current state on the next read of
> > > > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > > > spacing.
> > > >
> > >
> > > Initially, unless the latch bit is not set the debugfs file does
> > > not
> > > show
> > > any information as expected but once you write Y to latch file,
> > > the
> > > debugfs
> > > file continues to show blockers even though you write N back
> > > there or
> > > unload
> > > - reload the driver. Please fix this.
> >
> > See comments below
> >
> > >
> > > > Signed-off-by: David E. Box
> > > > ---
> > > >
> > > > V2:
> > > > Per Andy Shevchenko:
> > > > - Clear latch bit after use
> > > > - Pass pmc_dev as parameter
> > > > - Use DEFINE_SHOW_ATTRIBUTE macro
> > > >
> > > > drivers/platform/x86/intel_pmc_core.c | 112
> > > > ++
> > > > drivers/platform/x86/intel_pmc_core.h | 27 +---
> > > > 2 files changed, 132 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > > b/drivers/platform/x86/intel_pmc_core.c
> > > > index 43bbe74..ed40999 100644
> > > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > > @@ -196,9 +196,64 @@ static const struct pmc_bit_map
> > > > cnp_pfear_map[] = {
> > > > {}
> > > > };
> > > >
> > > > +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> > > > + {"AUDIO_D3",BIT(0)},
> > > > + {"OTG_D3", BIT(1)},
> > > > + {"XHCI_D3", BIT(2)},
> > > > + {"LPIO_D3", BIT(3)},
> > > > + {"SDX_D3", BIT(4)},
> > > > + {"SATA_D3", BIT(5)},
> > > > + {"UFS0_D3", BIT(6)},
> > > > + {"UFS1_D3", BIT(7)},
> > > > + {"EMMC_D3", BIT(8)},
> > > > +};
> > > > +
> > > > +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> > > > + {"SDIO_PLL_OFF",BIT(0)},
> > > > + {"USB2_PLL_OFF",BIT(1)},
> > > > + {"AUDIO_PLL_OFF", BIT(2)},
> > > > + {"OC_PLL_OFF", BIT(3)},
> > >
> > > Please rename to ISCLK_OC as it is the preferred nomenclature for
> > > debug.
> >
> > Okay
> >
> > >
> > > > + {"MAIN_PLL_OFF",BIT(4)},
> > >
> > > Ditto, please use ISCLK_MAIN.
> > >
> > > > + {"XOSC_OFF",BIT(5)},
> > > > + {"LPC_CLKS_GATED", BIT(6)},
> > > > + {"PCIE_CLKREQS_IDLE", BIT(7)},
> > > > + {"AUDIO_ROSC_OFF", BIT(8)},
> > > > + {"HPET_XOSC_CLK_REQ", BIT(9)},
> > > > + {"PMC_ROSC_SLOW_CLK", BIT(10)},
> > > > + {"AON2_ROSC_GATED", BIT(11)},
> > > > + {"CLKACKS_DEASSERTED", BIT(12)},
> > > > +};
> > > > +
> > > > +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> > > > + {"MPHY_CORE_GATED", BIT(0)},
> > > > + {"CSME_GATED", BIT(1)},
> > > > + {"USB2_SUS_GATED", BIT(2)},
> > > > + {"DYN_FLEX_IO_IDLE",BIT(3)},
> > > > + {"GBE_NO_LINK", BIT(4)},
> > > > + {"THERM_SEN_DISABLED", BIT(5)},
> > > > + {"PCIE_LOW_POWER", BIT(6)},
> > > > + {"ISH_VNNAON_REQ_ACT", BIT(7)},
> > > > + {"ISH_VNN_REQ_ACT", BIT(8)},
> > > > + {"CNV_VNNAON_REQ_ACT", BIT(9)},
> > > > + {"CNV_VNN_REQ_ACT", BIT(10)},
> > > > + {"NPK_VNNON_REQ_ACT", BIT(11)},
> > > > + {"PMSYNC_STATE_IDLE", BIT(12)},
> > > > + {"ALST_GT_THRES", BIT(13)},
> > > > + {"PMC_ARC_PG_READY",BIT(14)},
> > > > +};
> > > > +
> > > > +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> > > > + {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> > > > + {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> > > > + {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> > > > +};
> > > > +
> > > > static const struct pmc_reg_map cnp_reg_map = {
> > > > .pfear_sts = cnp_pfear_map,
> > > >
Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
On Fri, Jun 1, 2018 at 2:04 AM, David E. Box
wrote:
> On Thu, 2018-05-31 at 21:38 +0300, Andy Shevchenko wrote:
>> On Fri, May 25, 2018 at 4:10 AM, David E. Box
>> wrote:
>> > +struct slps0_dbg_map {
>> > + const struct pmc_bit_map *slps0_dbg_sts;
>> > + int size;
>> > +};
>>
>> Didn't pay attention to this earlier. Why do we have a separate size
>> member? What does it define?
>
> It holds the size of the pmc_bit_map array, assigned as shown here:
>
> +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> + {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> + {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> + {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> +};
I see.
Please drop this and use a terminator instead like it's done for the
rest map tables.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
On Thu, 2018-05-31 at 21:38 +0300, Andy Shevchenko wrote:
> On Fri, May 25, 2018 at 4:10 AM, David E. Box
> wrote:
> > Adds debugfs access to registers in the Cannon Point PCH PMC that
> > are
> > useful for debugging #SLP_S0 signal assertion and other low power
> > related
> > activities. Device pm states are latched in these registers
> > whenever the
> > package enters C10 and can be read from slp_s0_debug_status. The pm
> > states may also be latched by writing 1 to slp_s0_dbg_latch which
> > will
> > immediately capture the current state on the next read of
> > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > spacing.
> >
>
> Thanks for an update. My comments below.
>
> As far as I understand there is still ongoing discussion about the
> approach when and how to show data. So I'll wait a bit for a
> settlement between you, guys.
>
> > +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool
> > reset)
> > +{
> > + const struct pmc_reg_map *map = pmcdev->map;
> > + u32 fd;
> > +
> > + mutex_lock(&pmcdev->lock);
> > +
> > + if (!reset && !slps0_dbg_latch)
> > + goto out_unlock;
> > +
> > + fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> > + reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> > + (fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
>
> I would rather use classical pattern here, i.e.
>
> if (reset)
> fd ...
> else
> fd ...
>
> > + pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> > +
> > + slps0_dbg_latch = 0;
> > +
> > +out_unlock:
> > + mutex_unlock(&pmcdev->lock);
> > +}
> > + struct pmc_dev *pmcdev = s ? s->private : &pmc;
>
> I'm not sure why do we need such condition.
>
> Simple
>
> ... pmcdev = s->private;
>
> is enough.
>
> > /* Cannonlake Power Management Controller register offsets */
> > -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C
> > -#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C
> > -#define CNP_PMC_PM_CFG_OFFSET 0x1818
> > +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C
> > +#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C
> > +#define CNP_PMC_PM_CFG_OFFSET 0x1818
>
> I have hard time to understand what is the difference here.
> Either do another clean up patch (white spaces vs. tabs?) or leave it
> untouched.
>
> > /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> > -#define CNP_PMC_HOST_PPFEAR0A 0x1D90
> > +#define CNP_PMC_HOST_PPFEAR0A 0x1D90
> >
> > -#define CNP_PMC_MMIO_REG_LEN 0x2000
> > -#define CNP_PPFEAR_NUM_ENTRIES 8
> > -#define CNP_PMC_READ_DISABLE_BIT 22
> > +#define CNP_PMC_MMIO_REG_LEN 0x2000
> > +#define CNP_PPFEAR_NUM_ENTRIES 8
> > +#define CNP_PMC_READ_DISABLE_BIT 22
>
> Ditto.
>
> > +struct slps0_dbg_map {
> > + const struct pmc_bit_map *slps0_dbg_sts;
> > + int size;
> > +};
>
> Didn't pay attention to this earlier. Why do we have a separate size
> member? What does it define?
It holds the size of the pmc_bit_map array, assigned as shown here:
+static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
+ {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
+ {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
+ {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
+};
Okay on all other comments
Dave
Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
On Fri, May 25, 2018 at 4:10 AM, David E. Box
wrote:
> Adds debugfs access to registers in the Cannon Point PCH PMC that are
> useful for debugging #SLP_S0 signal assertion and other low power related
> activities. Device pm states are latched in these registers whenever the
> package enters C10 and can be read from slp_s0_debug_status. The pm
> states may also be latched by writing 1 to slp_s0_dbg_latch which will
> immediately capture the current state on the next read of
> slp_s0_debug_status. Also while in intel_pmc_core.h clean up spacing.
>
Thanks for an update. My comments below.
As far as I understand there is still ongoing discussion about the
approach when and how to show data. So I'll wait a bit for a
settlement between you, guys.
> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> +{
> + const struct pmc_reg_map *map = pmcdev->map;
> + u32 fd;
> +
> + mutex_lock(&pmcdev->lock);
> +
> + if (!reset && !slps0_dbg_latch)
> + goto out_unlock;
> +
> + fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> + reset ? (fd &= ~CNP_PMC_LATCH_SLPS0_EVENTS) :
> + (fd |= CNP_PMC_LATCH_SLPS0_EVENTS);
I would rather use classical pattern here, i.e.
if (reset)
fd ...
else
fd ...
> + pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> +
> + slps0_dbg_latch = 0;
> +
> +out_unlock:
> + mutex_unlock(&pmcdev->lock);
> +}
> + struct pmc_dev *pmcdev = s ? s->private : &pmc;
I'm not sure why do we need such condition.
Simple
... pmcdev = s->private;
is enough.
> /* Cannonlake Power Management Controller register offsets */
> -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C
> -#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C
> -#define CNP_PMC_PM_CFG_OFFSET 0x1818
> +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C
> +#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C
> +#define CNP_PMC_PM_CFG_OFFSET 0x1818
I have hard time to understand what is the difference here.
Either do another clean up patch (white spaces vs. tabs?) or leave it untouched.
> /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */
> -#define CNP_PMC_HOST_PPFEAR0A 0x1D90
> +#define CNP_PMC_HOST_PPFEAR0A 0x1D90
>
> -#define CNP_PMC_MMIO_REG_LEN 0x2000
> -#define CNP_PPFEAR_NUM_ENTRIES 8
> -#define CNP_PMC_READ_DISABLE_BIT 22
> +#define CNP_PMC_MMIO_REG_LEN 0x2000
> +#define CNP_PPFEAR_NUM_ENTRIES 8
> +#define CNP_PMC_READ_DISABLE_BIT 22
Ditto.
> +struct slps0_dbg_map {
> + const struct pmc_bit_map *slps0_dbg_sts;
> + int size;
> +};
Didn't pay attention to this earlier. Why do we have a separate size
member? What does it define?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
On Wed, May 30, 2018 at 03:53:12AM -0700, David E. Box wrote:
Hi Dave,
> Hi Rajneesh,
>
> On Mon, 2018-05-28 at 12:30 +0530, Rajneesh Bhardwaj wrote:
> > On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
> >
> > Thanks for sending this, Dave. Few comments below.
> >
> > > Adds debugfs access to registers in the Cannon Point PCH PMC that
> > > are
> >
> > Please use Cannonlake PCH.
> >
> > > useful for debugging #SLP_S0 signal assertion and other low power
> > > related
> >
> > assertion failures and other low power debug.
> >
> > > activities. Device pm states are latched in these registers
> > > whenever the
> > > package enters C10 and can be read from slp_s0_debug_status. The pm
> > > states may also be latched by writing 1 to slp_s0_dbg_latch which
> > > will
> > > immediately capture the current state on the next read of
> > > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > > spacing.
> > >
> >
> > Initially, unless the latch bit is not set the debugfs file does not
> > show
> > any information as expected but once you write Y to latch file, the
> > debugfs
> > file continues to show blockers even though you write N back there or
> > unload
> > - reload the driver. Please fix this.
>
> See comments below
>
> >
> > > Signed-off-by: David E. Box
> > > ---
> > >
> > > V2:
> > > Per Andy Shevchenko:
> > > - Clear latch bit after use
> > > - Pass pmc_dev as parameter
> > > - Use DEFINE_SHOW_ATTRIBUTE macro
> > >
> > > drivers/platform/x86/intel_pmc_core.c | 112
> > > ++
> > > drivers/platform/x86/intel_pmc_core.h | 27 +---
> > > 2 files changed, 132 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > b/drivers/platform/x86/intel_pmc_core.c
> > > index 43bbe74..ed40999 100644
> > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > @@ -196,9 +196,64 @@ static const struct pmc_bit_map
> > > cnp_pfear_map[] = {
> > > {}
> > > };
> > >
> > > +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> > > + {"AUDIO_D3",BIT(0)},
> > > + {"OTG_D3", BIT(1)},
> > > + {"XHCI_D3", BIT(2)},
> > > + {"LPIO_D3", BIT(3)},
> > > + {"SDX_D3", BIT(4)},
> > > + {"SATA_D3", BIT(5)},
> > > + {"UFS0_D3", BIT(6)},
> > > + {"UFS1_D3", BIT(7)},
> > > + {"EMMC_D3", BIT(8)},
> > > +};
> > > +
> > > +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> > > + {"SDIO_PLL_OFF",BIT(0)},
> > > + {"USB2_PLL_OFF",BIT(1)},
> > > + {"AUDIO_PLL_OFF", BIT(2)},
> > > + {"OC_PLL_OFF", BIT(3)},
> >
> > Please rename to ISCLK_OC as it is the preferred nomenclature for
> > debug.
>
> Okay
>
> >
> > > + {"MAIN_PLL_OFF",BIT(4)},
> >
> > Ditto, please use ISCLK_MAIN.
> >
> > > + {"XOSC_OFF",BIT(5)},
> > > + {"LPC_CLKS_GATED", BIT(6)},
> > > + {"PCIE_CLKREQS_IDLE", BIT(7)},
> > > + {"AUDIO_ROSC_OFF", BIT(8)},
> > > + {"HPET_XOSC_CLK_REQ", BIT(9)},
> > > + {"PMC_ROSC_SLOW_CLK", BIT(10)},
> > > + {"AON2_ROSC_GATED", BIT(11)},
> > > + {"CLKACKS_DEASSERTED", BIT(12)},
> > > +};
> > > +
> > > +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> > > + {"MPHY_CORE_GATED", BIT(0)},
> > > + {"CSME_GATED", BIT(1)},
> > > + {"USB2_SUS_GATED", BIT(2)},
> > > + {"DYN_FLEX_IO_IDLE",BIT(3)},
> > > + {"GBE_NO_LINK", BIT(4)},
> > > + {"THERM_SEN_DISABLED", BIT(5)},
> > > + {"PCIE_LOW_POWER", BIT(6)},
> > > + {"ISH_VNNAON_REQ_ACT", BIT(7)},
> > > + {"ISH_VNN_REQ_ACT", BIT(8)},
> > > + {"CNV_VNNAON_REQ_ACT", BIT(9)},
> > > + {"CNV_VNN_REQ_ACT", BIT(10)},
> > > + {"NPK_VNNON_REQ_ACT", BIT(11)},
> > > + {"PMSYNC_STATE_IDLE", BIT(12)},
> > > + {"ALST_GT_THRES", BIT(13)},
> > > + {"PMC_ARC_PG_READY",BIT(14)},
> > > +};
> > > +
> > > +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> > > + {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> > > + {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> > > + {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> > > +};
> > > +
> > > static const struct pmc_reg_map cnp_reg_map = {
> > > .pfear_sts = cnp_pfear_map,
> > > .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> > > + .slps0_dbg_maps = cnp_slps0_dbg_maps,
> > > + .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> > > + .slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
> > > .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> > > .regmap_length = CNP_PMC_MMIO_REG_LEN,
> > > .ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> > > @@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
> > > }
> > >
> > > #if IS_ENABLED(CONFIG_DEBUG_FS)
> > > +static bool slps0_dbg_latch;
> > > +
> > > static void pmc_core_display_map(struct seq_file *s, int index,
> > >u8 pf_reg, con
Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
Hi Rajneesh,
On Mon, 2018-05-28 at 12:30 +0530, Rajneesh Bhardwaj wrote:
> On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
>
> Thanks for sending this, Dave. Few comments below.
>
> > Adds debugfs access to registers in the Cannon Point PCH PMC that
> > are
>
> Please use Cannonlake PCH.
>
> > useful for debugging #SLP_S0 signal assertion and other low power
> > related
>
> assertion failures and other low power debug.
>
> > activities. Device pm states are latched in these registers
> > whenever the
> > package enters C10 and can be read from slp_s0_debug_status. The pm
> > states may also be latched by writing 1 to slp_s0_dbg_latch which
> > will
> > immediately capture the current state on the next read of
> > slp_s0_debug_status. Also while in intel_pmc_core.h clean up
> > spacing.
> >
>
> Initially, unless the latch bit is not set the debugfs file does not
> show
> any information as expected but once you write Y to latch file, the
> debugfs
> file continues to show blockers even though you write N back there or
> unload
> - reload the driver. Please fix this.
See comments below
>
> > Signed-off-by: David E. Box
> > ---
> >
> > V2:
> > Per Andy Shevchenko:
> > - Clear latch bit after use
> > - Pass pmc_dev as parameter
> > - Use DEFINE_SHOW_ATTRIBUTE macro
> >
> > drivers/platform/x86/intel_pmc_core.c | 112
> > ++
> > drivers/platform/x86/intel_pmc_core.h | 27 +---
> > 2 files changed, 132 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 43bbe74..ed40999 100644
> > --- a/drivers/platform/x86/intel_pmc_core.c
> > +++ b/drivers/platform/x86/intel_pmc_core.c
> > @@ -196,9 +196,64 @@ static const struct pmc_bit_map
> > cnp_pfear_map[] = {
> > {}
> > };
> >
> > +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> > + {"AUDIO_D3",BIT(0)},
> > + {"OTG_D3", BIT(1)},
> > + {"XHCI_D3", BIT(2)},
> > + {"LPIO_D3", BIT(3)},
> > + {"SDX_D3", BIT(4)},
> > + {"SATA_D3", BIT(5)},
> > + {"UFS0_D3", BIT(6)},
> > + {"UFS1_D3", BIT(7)},
> > + {"EMMC_D3", BIT(8)},
> > +};
> > +
> > +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> > + {"SDIO_PLL_OFF",BIT(0)},
> > + {"USB2_PLL_OFF",BIT(1)},
> > + {"AUDIO_PLL_OFF", BIT(2)},
> > + {"OC_PLL_OFF", BIT(3)},
>
> Please rename to ISCLK_OC as it is the preferred nomenclature for
> debug.
Okay
>
> > + {"MAIN_PLL_OFF",BIT(4)},
>
> Ditto, please use ISCLK_MAIN.
>
> > + {"XOSC_OFF",BIT(5)},
> > + {"LPC_CLKS_GATED", BIT(6)},
> > + {"PCIE_CLKREQS_IDLE", BIT(7)},
> > + {"AUDIO_ROSC_OFF", BIT(8)},
> > + {"HPET_XOSC_CLK_REQ", BIT(9)},
> > + {"PMC_ROSC_SLOW_CLK", BIT(10)},
> > + {"AON2_ROSC_GATED", BIT(11)},
> > + {"CLKACKS_DEASSERTED", BIT(12)},
> > +};
> > +
> > +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> > + {"MPHY_CORE_GATED", BIT(0)},
> > + {"CSME_GATED", BIT(1)},
> > + {"USB2_SUS_GATED", BIT(2)},
> > + {"DYN_FLEX_IO_IDLE",BIT(3)},
> > + {"GBE_NO_LINK", BIT(4)},
> > + {"THERM_SEN_DISABLED", BIT(5)},
> > + {"PCIE_LOW_POWER", BIT(6)},
> > + {"ISH_VNNAON_REQ_ACT", BIT(7)},
> > + {"ISH_VNN_REQ_ACT", BIT(8)},
> > + {"CNV_VNNAON_REQ_ACT", BIT(9)},
> > + {"CNV_VNN_REQ_ACT", BIT(10)},
> > + {"NPK_VNNON_REQ_ACT", BIT(11)},
> > + {"PMSYNC_STATE_IDLE", BIT(12)},
> > + {"ALST_GT_THRES", BIT(13)},
> > + {"PMC_ARC_PG_READY",BIT(14)},
> > +};
> > +
> > +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> > + {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> > + {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> > + {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> > +};
> > +
> > static const struct pmc_reg_map cnp_reg_map = {
> > .pfear_sts = cnp_pfear_map,
> > .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> > + .slps0_dbg_maps = cnp_slps0_dbg_maps,
> > + .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> > + .slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
> > .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> > .regmap_length = CNP_PMC_MMIO_REG_LEN,
> > .ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> > @@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
> > }
> >
> > #if IS_ENABLED(CONFIG_DEBUG_FS)
> > +static bool slps0_dbg_latch;
> > +
> > static void pmc_core_display_map(struct seq_file *s, int index,
> > u8 pf_reg, const struct
> > pmc_bit_map *pf_map)
> > {
> > @@ -481,6 +538,52 @@ static const struct file_operations
> > pmc_core_ltr_ignore_ops = {
> > .release= single_release,
> > };
> >
> > +static void pmc_core_slps0_dbg_latch(struct pmc_dev
Re: [PATCH V2] platform/x86: intel_pmc_core: Add CNP SLPS0 debug registers
On Thu, May 24, 2018 at 06:10:56PM -0700, David E. Box wrote:
Thanks for sending this, Dave. Few comments below.
> Adds debugfs access to registers in the Cannon Point PCH PMC that are
Please use Cannonlake PCH.
> useful for debugging #SLP_S0 signal assertion and other low power related
assertion failures and other low power debug.
> activities. Device pm states are latched in these registers whenever the
> package enters C10 and can be read from slp_s0_debug_status. The pm
> states may also be latched by writing 1 to slp_s0_dbg_latch which will
> immediately capture the current state on the next read of
> slp_s0_debug_status. Also while in intel_pmc_core.h clean up spacing.
>
Initially, unless the latch bit is not set the debugfs file does not show
any information as expected but once you write Y to latch file, the debugfs
file continues to show blockers even though you write N back there or unload
- reload the driver. Please fix this.
> Signed-off-by: David E. Box
> ---
>
> V2:
> Per Andy Shevchenko:
> - Clear latch bit after use
> - Pass pmc_dev as parameter
> - Use DEFINE_SHOW_ATTRIBUTE macro
>
> drivers/platform/x86/intel_pmc_core.c | 112
> ++
> drivers/platform/x86/intel_pmc_core.h | 27 +---
> 2 files changed, 132 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c
> b/drivers/platform/x86/intel_pmc_core.c
> index 43bbe74..ed40999 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -196,9 +196,64 @@ static const struct pmc_bit_map cnp_pfear_map[] = {
> {}
> };
>
> +static const struct pmc_bit_map cnp_slps0_dbg0_map[] = {
> + {"AUDIO_D3",BIT(0)},
> + {"OTG_D3", BIT(1)},
> + {"XHCI_D3", BIT(2)},
> + {"LPIO_D3", BIT(3)},
> + {"SDX_D3", BIT(4)},
> + {"SATA_D3", BIT(5)},
> + {"UFS0_D3", BIT(6)},
> + {"UFS1_D3", BIT(7)},
> + {"EMMC_D3", BIT(8)},
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg1_map[] = {
> + {"SDIO_PLL_OFF",BIT(0)},
> + {"USB2_PLL_OFF",BIT(1)},
> + {"AUDIO_PLL_OFF", BIT(2)},
> + {"OC_PLL_OFF", BIT(3)},
Please rename to ISCLK_OC as it is the preferred nomenclature for debug.
> + {"MAIN_PLL_OFF",BIT(4)},
Ditto, please use ISCLK_MAIN.
> + {"XOSC_OFF",BIT(5)},
> + {"LPC_CLKS_GATED", BIT(6)},
> + {"PCIE_CLKREQS_IDLE", BIT(7)},
> + {"AUDIO_ROSC_OFF", BIT(8)},
> + {"HPET_XOSC_CLK_REQ", BIT(9)},
> + {"PMC_ROSC_SLOW_CLK", BIT(10)},
> + {"AON2_ROSC_GATED", BIT(11)},
> + {"CLKACKS_DEASSERTED", BIT(12)},
> +};
> +
> +static const struct pmc_bit_map cnp_slps0_dbg2_map[] = {
> + {"MPHY_CORE_GATED", BIT(0)},
> + {"CSME_GATED", BIT(1)},
> + {"USB2_SUS_GATED", BIT(2)},
> + {"DYN_FLEX_IO_IDLE",BIT(3)},
> + {"GBE_NO_LINK", BIT(4)},
> + {"THERM_SEN_DISABLED", BIT(5)},
> + {"PCIE_LOW_POWER", BIT(6)},
> + {"ISH_VNNAON_REQ_ACT", BIT(7)},
> + {"ISH_VNN_REQ_ACT", BIT(8)},
> + {"CNV_VNNAON_REQ_ACT", BIT(9)},
> + {"CNV_VNN_REQ_ACT", BIT(10)},
> + {"NPK_VNNON_REQ_ACT", BIT(11)},
> + {"PMSYNC_STATE_IDLE", BIT(12)},
> + {"ALST_GT_THRES", BIT(13)},
> + {"PMC_ARC_PG_READY",BIT(14)},
> +};
> +
> +static const struct slps0_dbg_map cnp_slps0_dbg_maps[] = {
> + {cnp_slps0_dbg0_map, ARRAY_SIZE(cnp_slps0_dbg0_map)},
> + {cnp_slps0_dbg1_map, ARRAY_SIZE(cnp_slps0_dbg1_map)},
> + {cnp_slps0_dbg2_map, ARRAY_SIZE(cnp_slps0_dbg2_map)},
> +};
> +
> static const struct pmc_reg_map cnp_reg_map = {
> .pfear_sts = cnp_pfear_map,
> .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET,
> + .slps0_dbg_maps = cnp_slps0_dbg_maps,
> + .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET,
> + .slps0_dbg_num = ARRAY_SIZE(cnp_slps0_dbg_maps),
> .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET,
> .regmap_length = CNP_PMC_MMIO_REG_LEN,
> .ppfear0_offset = CNP_PMC_HOST_PPFEAR0A,
> @@ -252,6 +307,8 @@ static int pmc_core_check_read_lock_bit(void)
> }
>
> #if IS_ENABLED(CONFIG_DEBUG_FS)
> +static bool slps0_dbg_latch;
> +
> static void pmc_core_display_map(struct seq_file *s, int index,
>u8 pf_reg, const struct pmc_bit_map *pf_map)
> {
> @@ -481,6 +538,52 @@ static const struct file_operations
> pmc_core_ltr_ignore_ops = {
> .release= single_release,
> };
>
> +static void pmc_core_slps0_dbg_latch(struct pmc_dev *pmcdev, bool reset)
> +{
> + const struct pmc_reg_map *map = pmcdev->map;
> + u32 fd;
> +
> + mutex_lock(&pmcdev->lock);
> +
> + if (!reset && !slps0_dbg_latch)
> + goto out_unlock;
> +
> + fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> +
