On Tue, 2018-05-22 at 12:35 +0300, Andy Shevchenko wrote:
> On Wed, May 16, 2018 at 2:39 AM, David E. Box
> <david.e.box@xxxxxxxxxxxxxxx> 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.
>
> IIRC we discussed with Sathya to refactor the driver(s) to be more
> flexible, in particular switching to regmap API which brings debug
> dump for free.
>
> I don't know if regmap API supports bit labeling though.
>
> Anyway, added Sathya and Heikki to the discussion.
> Also, see my comments below.
We can own refactoring the driver since we will have more updates to
it. We'd like this patch, after fixes, applied first.
>
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > drivers/platform/x86/intel_pmc_core.c | 122
> > ++++++++++++++++++++++++++++++++++
> > drivers/platform/x86/intel_pmc_core.h | 27 ++++++--
> > 2 files changed, 142 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index 43bbe74743d9..043e224e7a21 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)},
> > + {"MAIN_PLL_OFF", BIT(4)},
> > + {"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,62 @@ static const struct file_operations
> > pmc_core_ltr_ignore_ops = {
> > .release = single_release,
> > };
> >
> > +static void pmc_core_slps0_dbg_latch(void)
> > +{
> > + struct pmc_dev *pmcdev = &pmc;
>
> This should be a parameter of the function.
>
> > + const struct pmc_reg_map *map = pmcdev->map;
> > + u32 fd;
> > +
> > + mutex_lock(&pmcdev->lock);
> > +
> > + if (!slps0_dbg_latch)
> > + goto out_unlock;
> > +
> > + fd = pmc_core_reg_read(pmcdev, map->slps0_dbg_offset);
> > + fd |= CNP_PMC_LATCH_SLPS0_EVENTS;
> > + pmc_core_reg_write(pmcdev, map->slps0_dbg_offset, fd);
> > +
> > + slps0_dbg_latch = 0;
> > +
> > +out_unlock:
> > + mutex_unlock(&pmcdev->lock);
> > +}
> > +
> > +static int pmc_core_slps0_dbg_show(struct seq_file *s, void
> > *unused)
> > +{
> > + struct pmc_dev *pmcdev = s ? s->private : &pmc;
> > + const struct slps0_dbg_map *maps = pmcdev->map-
> > >slps0_dbg_maps;
> > + int num_slps0_dbg_regs = pmcdev->map->slps0_dbg_num;
> > + int i, offset;
> > + u32 data;
> > +
> > + pmc_core_slps0_dbg_latch();
> > + offset = pmcdev->map->slps0_dbg_offset;
> > + while (num_slps0_dbg_regs--) {
> > + data = pmc_core_reg_read(pmcdev, offset);
> > + offset += 4;
> > + for (i = 0; i < maps->size; i++)
> > + seq_printf(s, "SLP_S0_DBG: %-32s\tState:
> > %s\n",
> > + maps->slps0_dbg_sts[i].name,
> > + data & maps-
> > >slps0_dbg_sts[i].bit_mask ?
> > + "Yes" : "No");
> > + maps++;
> > + }
>
> Shouldn't it unlatch the values here?
Good catch. It needs to be cleared so that further latch attempts will
take effect. Okay on all other comments.
>
> > + return 0;
> > +}
> > +
> > +static int pmc_core_slps0_dbg_open(struct inode *inode, struct
> > file *file)
> > +{
> > + return single_open(file, pmc_core_slps0_dbg_show, inode-
> > >i_private);
> > +}
> > +
> > +static const struct file_operations pmc_core_slps0_dbg_ops = {
> > + .open = pmc_core_slps0_dbg_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
> > +
>
> The above is just one line macro, i.e. DEFINE_SHOW_ATTRIBUTE().
>
> > static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> > {
> > debugfs_remove_recursive(pmcdev->dbgfs_dir);
> > @@ -514,6 +627,15 @@ static int pmc_core_dbgfs_register(struct
> > pmc_dev *pmcdev)
> > 0444, dir, pmcdev,
> > &pmc_core_mphy_pg_ops);
> >
> > + if (pmcdev->map->slps0_dbg_maps) {
> > + debugfs_create_file("slp_s0_debug_status", 0444,
> > + dir, pmcdev,
> > + &pmc_core_slps0_dbg_ops);
> > +
> > + debugfs_create_bool("slp_s0_dbg_latch", 0644,
> > + dir, &slps0_dbg_latch);
> > + }
> > +
> > return 0;
> > }
> > #else
> > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > b/drivers/platform/x86/intel_pmc_core.h
> > index 5fa5f97870aa..97e4b5a2f4b9 100644
> > --- a/drivers/platform/x86/intel_pmc_core.h
> > +++ b/drivers/platform/x86/intel_pmc_core.h
> > @@ -124,27 +124,35 @@ enum ppfear_regs {
> > #define SPT_PMC_BIT_MPHY_CMN_LANE3 BIT(3)
> >
> > /* 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
> > +#define CNP_PMC_SLPS0_DBG_OFFSET 0x10B4
> > /* 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
> > +#define CNP_PMC_LATCH_SLPS0_EVENTS BIT(31)
> >
> > struct pmc_bit_map {
> > const char *name;
> > u32 bit_mask;
> > };
> >
> > +struct slps0_dbg_map {
> > + const struct pmc_bit_map *slps0_dbg_sts;
> > + int size;
> > +};
> > +
> > /**
> > * struct pmc_reg_map - Structure used to define parameter unique
> > to a
> > PCH family
> > * @pfear_sts: Maps name of IP block to PPFEAR* bit
> > * @mphy_sts: Maps name of MPHY lane to MPHY status lane
> > status bit
> > * @pll_sts: Maps name of PLL to corresponding bit
> > status
> > + * @slps0_dbg_maps: Array of SLP_S0_DBG* registers containing
> > debug info
> > * @slp_s0_offset: PWRMBASE offset to read SLP_S0 residency
> > * @ltr_ignore_offset: PWRMBASE offset to read/write LTR ignore
> > bit
> > * @regmap_length: Length of memory to map from PWRMBASE
> > address to access
> > @@ -153,6 +161,8 @@ struct pmc_bit_map {
> > * PPFEAR
> > * @pm_cfg_offset: PWRMBASE offset to PM_CFG register
> > * @pm_read_disable_bit: Bit index to read PMC_READ_DISABLE
> > + * @slps0_dbg_offset: PWRMBASE offset to SLP_S0_DEBUG_REG*
> > + * @slps0_dbg_num: Number of SLP_S0_DEBUG_REG registers
> > *
> > * Each PCH has unique set of register offsets and bit indexes.
> > This structure
> > * captures them to have a common implementation.
> > @@ -161,6 +171,7 @@ struct pmc_reg_map {
> > const struct pmc_bit_map *pfear_sts;
> > const struct pmc_bit_map *mphy_sts;
> > const struct pmc_bit_map *pll_sts;
> > + const struct slps0_dbg_map *slps0_dbg_maps;
> > const u32 slp_s0_offset;
> > const u32 ltr_ignore_offset;
> > const int regmap_length;
> > @@ -168,6 +179,8 @@ struct pmc_reg_map {
> > const int ppfear_buckets;
> > const u32 pm_cfg_offset;
> > const int pm_read_disable_bit;
> > + const u32 slps0_dbg_offset;
> > + const int slps0_dbg_num;
> > };
> >
> > /**
> > --
> > 2.14.1
> >
>
>
>
