| Message ID | 20230615193302.2507338-2-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | TPMI debugfs suport | expand |
On Thu, 15 Jun 2023, Srinivas Pandruvada wrote: > Some of the PM features can be locked or disabled. In that case, write > interface can be locked. > > This status is read via a mailbox. There is one TPMI ID which provides > base address for interface and data register for mail box operation. > The mailbox operations is defined in the TPMI specification. Refer to > https://github.com/intel/tpmi_power_management/ for TPMI specifications. > > An API is exposed to feature drivers to read feature control status. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > As suggested by Dan Williams changed ioremap to devm_ioremap() after > review by Andy. > > drivers/platform/x86/intel/tpmi.c | 147 ++++++++++++++++++++++++++++++ > include/linux/intel_tpmi.h | 2 + > 2 files changed, 149 insertions(+) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index a5227951decc..9545e9cdb924 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -47,8 +47,11 @@ > */ > > #include <linux/auxiliary_bus.h> > +#include <linux/bitfield.h> > +#include <linux/delay.h> > #include <linux/intel_tpmi.h> > #include <linux/io.h> > +#include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/pci.h> > > @@ -98,6 +101,7 @@ struct intel_tpmi_pm_feature { > * @feature_count: Number of TPMI of TPMI instances pointed by tpmi_features > * @pfs_start: Start of PFS offset for the TPMI instances in this device > * @plat_info: Stores platform info which can be used by the client drivers > + * @tpmi_control_mem: Memory mapped IO for getting control information > * > * Stores the information for all TPMI devices enumerated from a single PCI device. > */ > @@ -107,6 +111,7 @@ struct intel_tpmi_info { > int feature_count; > u64 pfs_start; > struct intel_tpmi_plat_info plat_info; > + void __iomem *tpmi_control_mem; > }; > > /** > @@ -139,6 +144,7 @@ enum intel_tpmi_id { > TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */ > TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */ > TPMI_ID_SST = 5, /* Speed Select Technology */ > + TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */ > TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */ > }; > > @@ -175,6 +181,144 @@ struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int > } > EXPORT_SYMBOL_NS_GPL(tpmi_get_resource_at_index, INTEL_TPMI); > > +/* TPMI Control Interface */ > + > +#define TPMI_CONTROL_STATUS_OFFSET 0x00 > +#define TPMI_COMMAND_OFFSET 0x08 > +#define TPMI_DATA_OFFSET 0x0C > +/* > + * Spec is calling for max 1 seconds to get ownership at the worst > + * case. Read at 10 ms timeouts and repeat up to 1 second. > + */ > +#define TPMI_CONTROL_TIMEOUT_US (10 * USEC_PER_MSEC) > +#define TPMI_CONTROL_TIMEOUT_MAX_US USEC_PER_SEC > + > +#define TPMI_RB_TIMEOUT_US (10 * USEC_PER_MSEC) > +#define TPMI_RB_TIMEOUT_MAX_US USEC_PER_SEC > + > +#define TPMI_OWNER_NONE 0 > +#define TPMI_OWNER_IN_BAND 1 > + > +#define TPMI_GENMASK_OWNER GENMASK_ULL(5, 4) > +#define TPMI_GENMASK_STATUS GENMASK_ULL(15, 8) > + > +#define TPMI_GET_STATE_CMD 0x10 > +#define TPMI_GET_STATE_CMD_DATA_OFFSET 8 > +#define TPMI_CMD_DATA_OFFSET 32 > +#define TPMI_CMD_PKT_LEN_OFFSET 16 > +#define TPMI_CMD_PKT_LEN 2 > +#define TPMI_CONTROL_RB_BIT 0 > +#define TPMI_CONTROL_CPL_BIT 6 > +#define TPMI_CMD_STATUS_SUCCESS 0x40 > +#define TPMI_GET_STATUS_BIT_ENABLE 0 > +#define TPMI_GET_STATUS_BIT_LOCKED 31 > + > +/* Mutex to complete get feature status without interruption */ > +static DEFINE_MUTEX(tpmi_dev_lock); > + > +static int tpmi_wait_for_owner(struct intel_tpmi_info *tpmi_info, u8 owner) > +{ > + u64 control; > + > + return read_poll_timeout(readq, control, owner == FIELD_GET(TPMI_GENMASK_OWNER, control), > + TPMI_CONTROL_TIMEOUT_US, TPMI_CONTROL_TIMEOUT_MAX_US, false, > + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); > +} > + > +static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int feature_id, > + int *locked, int *disabled) > +{ > + u64 control, data; > + int ret; > + > + if (!tpmi_info->tpmi_control_mem) > + return -EFAULT; > + > + mutex_lock(&tpmi_dev_lock); > + > + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_NONE); > + if (ret) > + goto err_unlock; > + > + /* set command id to 0x10 for TPMI_GET_STATE */ > + data = TPMI_GET_STATE_CMD; > + /* 32 bits for DATA offset and +8 for feature_id field */ > + data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET + TPMI_GET_STATE_CMD_DATA_OFFSET)); This looks like you should add the GENMASK_ULL() for the fields and use FIELD_PREP() instead of adding all those OFFSET defines + custom shifting. > + > + /* Write at command offset for qword access */ > + writeq(data, tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET); > + > + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND); > + if (ret) > + goto err_unlock; > + > + /* Set Run Busy and packet length of 2 dwords */ > + writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN << TPMI_CMD_PKT_LEN_OFFSET), Define using BIT_ULL(0) instead. Use FIELD_PREP(). I'd drop _BIT from the define name but I leave it up to you, it just makes your lines longer w/o much added value. > + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); > + > + ret = read_poll_timeout(readq, control, !(control & BIT_ULL(TPMI_CONTROL_RB_BIT)), > + TPMI_RB_TIMEOUT_US, TPMI_RB_TIMEOUT_MAX_US, false, > + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); > + if (ret) > + goto done_proc; > + > + control = FIELD_GET(TPMI_GENMASK_STATUS, control); > + if (control != TPMI_CMD_STATUS_SUCCESS) { > + ret = -EBUSY; > + goto done_proc; > + } > + > + data = readq(tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET); > + data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for TPMI_DATA */ Define the field with GENMASK() and use FIELD_GET(). > + > + *disabled = 0; > + *locked = 0; > + > + if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE))) Put BIT_ULL() into the define. Perhaps drop _BIT_ from the name. > + *disabled = 1; > + > + if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED)) Ditto. > + *locked = 1; > + > + ret = 0; > + > +done_proc: > + /* SET CPL "completion"bit */ Missing space. > + writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT), BIT_ULL() to define. > + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); > + > +err_unlock: > + mutex_unlock(&tpmi_dev_lock); > + > + return ret; > +} > + > +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, > + int *locked, int *disabled) > +{ > + struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(auxdev->dev.parent); > + struct intel_tpmi_info *tpmi_info = auxiliary_get_drvdata(&intel_vsec_dev->auxdev); > + > + return tpmi_read_feature_status(tpmi_info, feature_id, locked, disabled); > +} > +EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI); > + > +static void tpmi_set_control_base(struct auxiliary_device *auxdev, > + struct intel_tpmi_info *tpmi_info, > + struct intel_tpmi_pm_feature *pfs) > +{ > + void __iomem *mem; > + u16 size; > + > + size = pfs->pfs_header.num_entries * pfs->pfs_header.entry_size * 4; Can this overflow u16? Where does pfs_header content originate from? If from HW, how is it the input validated?
On Fri, 2023-06-16 at 10:13 +0300, Ilpo Järvinen wrote: > On Thu, 15 Jun 2023, Srinivas Pandruvada wrote: > > > [...] > > + /* set command id to 0x10 for TPMI_GET_STATE */ > > + data = TPMI_GET_STATE_CMD; > > + /* 32 bits for DATA offset and +8 for feature_id field */ > > + data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET + > > TPMI_GET_STATE_CMD_DATA_OFFSET)); > > This looks like you should add the GENMASK_ULL() for the fields and > use > FIELD_PREP() instead of adding all those OFFSET defines + custom > shifting. You mean, I should change one shift instruction, to FIELD_PREP() which will use three instructions to shift, sub and AND? ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); > > > + > > + /* Write at command offset for qword access */ > > + writeq(data, tpmi_info->tpmi_control_mem + > > TPMI_COMMAND_OFFSET); > > + > > + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND); > > + if (ret) > > + goto err_unlock; > > + > > + /* Set Run Busy and packet length of 2 dwords */ > > + writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN << > > TPMI_CMD_PKT_LEN_OFFSET), > > Define using BIT_ULL(0) instead. Use FIELD_PREP(). This code will run only on X86 64 bit, not a common device driver which will run in any architecture. Please let me know why FIELD_PREP() is better. > > I'd drop _BIT from the define name but I leave it up to you, it just > makes your lines longer w/o much added value. > > > + tpmi_info->tpmi_control_mem + > > TPMI_CONTROL_STATUS_OFFSET); > > + > > + ret = read_poll_timeout(readq, control, !(control & > > BIT_ULL(TPMI_CONTROL_RB_BIT)), > > + TPMI_RB_TIMEOUT_US, > > TPMI_RB_TIMEOUT_MAX_US, false, > > + tpmi_info->tpmi_control_mem + > > TPMI_CONTROL_STATUS_OFFSET); > > + if (ret) > > + goto done_proc; > > + > > + control = FIELD_GET(TPMI_GENMASK_STATUS, control); > > + if (control != TPMI_CMD_STATUS_SUCCESS) { > > + ret = -EBUSY; > > + goto done_proc; > > + } > > + > > + data = readq(tpmi_info->tpmi_control_mem + > > TPMI_COMMAND_OFFSET); > > + data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for > > TPMI_DATA */ > > Define the field with GENMASK() and use FIELD_GET(). > Again 3 instructions instead of 1. > > + > > + *disabled = 0; > > + *locked = 0; > > + > > + if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE))) > > Put BIT_ULL() into the define. Good idea. > > Perhaps drop _BIT_ from the name. I can do that. > > > + *disabled = 1; > > + > > + if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED)) > > Ditto. > > > + *locked = 1; > > + > > + ret = 0; > > + > > +done_proc: > > + /* SET CPL "completion"bit */ > > Missing space. > OK > > + writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT), > > BIT_ULL() to define. > > > + tpmi_info->tpmi_control_mem + > > TPMI_CONTROL_STATUS_OFFSET); > > + > > +err_unlock: > > + mutex_unlock(&tpmi_dev_lock); > > + > > + return ret; > > +} > > + > > +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int > > feature_id, > > + int *locked, int *disabled) > > +{ > > + struct intel_vsec_device *intel_vsec_dev = > > dev_to_ivdev(auxdev->dev.parent); > > + struct intel_tpmi_info *tpmi_info = > > auxiliary_get_drvdata(&intel_vsec_dev->auxdev); > > + > > + return tpmi_read_feature_status(tpmi_info, feature_id, > > locked, disabled); > > +} > > +EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI); > > + > > +static void tpmi_set_control_base(struct auxiliary_device *auxdev, > > + struct intel_tpmi_info > > *tpmi_info, > > + struct intel_tpmi_pm_feature > > *pfs) > > +{ > > + void __iomem *mem; > > + u16 size; > > + > > + size = pfs->pfs_header.num_entries * pfs- > > >pfs_header.entry_size * 4; > > Can this overflow u16? Where does pfs_header content originate from? We can add a check, but this is coming from a trusted and validated x86 core (Not an add on IP), which not only driver uses but all PM IP in the hardware. Thanks, Srinivas > If > from HW, how is it the input validated? >
diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c index a5227951decc..9545e9cdb924 100644 --- a/drivers/platform/x86/intel/tpmi.c +++ b/drivers/platform/x86/intel/tpmi.c @@ -47,8 +47,11 @@ */ #include <linux/auxiliary_bus.h> +#include <linux/bitfield.h> +#include <linux/delay.h> #include <linux/intel_tpmi.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/pci.h> @@ -98,6 +101,7 @@ struct intel_tpmi_pm_feature { * @feature_count: Number of TPMI of TPMI instances pointed by tpmi_features * @pfs_start: Start of PFS offset for the TPMI instances in this device * @plat_info: Stores platform info which can be used by the client drivers + * @tpmi_control_mem: Memory mapped IO for getting control information * * Stores the information for all TPMI devices enumerated from a single PCI device. */ @@ -107,6 +111,7 @@ struct intel_tpmi_info { int feature_count; u64 pfs_start; struct intel_tpmi_plat_info plat_info; + void __iomem *tpmi_control_mem; }; /** @@ -139,6 +144,7 @@ enum intel_tpmi_id { TPMI_ID_PEM = 1, /* Power and Perf excursion Monitor */ TPMI_ID_UNCORE = 2, /* Uncore Frequency Scaling */ TPMI_ID_SST = 5, /* Speed Select Technology */ + TPMI_CONTROL_ID = 0x80, /* Special ID for getting feature status */ TPMI_INFO_ID = 0x81, /* Special ID for PCI BDF and Package ID information */ }; @@ -175,6 +181,144 @@ struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int } EXPORT_SYMBOL_NS_GPL(tpmi_get_resource_at_index, INTEL_TPMI); +/* TPMI Control Interface */ + +#define TPMI_CONTROL_STATUS_OFFSET 0x00 +#define TPMI_COMMAND_OFFSET 0x08 +#define TPMI_DATA_OFFSET 0x0C +/* + * Spec is calling for max 1 seconds to get ownership at the worst + * case. Read at 10 ms timeouts and repeat up to 1 second. + */ +#define TPMI_CONTROL_TIMEOUT_US (10 * USEC_PER_MSEC) +#define TPMI_CONTROL_TIMEOUT_MAX_US USEC_PER_SEC + +#define TPMI_RB_TIMEOUT_US (10 * USEC_PER_MSEC) +#define TPMI_RB_TIMEOUT_MAX_US USEC_PER_SEC + +#define TPMI_OWNER_NONE 0 +#define TPMI_OWNER_IN_BAND 1 + +#define TPMI_GENMASK_OWNER GENMASK_ULL(5, 4) +#define TPMI_GENMASK_STATUS GENMASK_ULL(15, 8) + +#define TPMI_GET_STATE_CMD 0x10 +#define TPMI_GET_STATE_CMD_DATA_OFFSET 8 +#define TPMI_CMD_DATA_OFFSET 32 +#define TPMI_CMD_PKT_LEN_OFFSET 16 +#define TPMI_CMD_PKT_LEN 2 +#define TPMI_CONTROL_RB_BIT 0 +#define TPMI_CONTROL_CPL_BIT 6 +#define TPMI_CMD_STATUS_SUCCESS 0x40 +#define TPMI_GET_STATUS_BIT_ENABLE 0 +#define TPMI_GET_STATUS_BIT_LOCKED 31 + +/* Mutex to complete get feature status without interruption */ +static DEFINE_MUTEX(tpmi_dev_lock); + +static int tpmi_wait_for_owner(struct intel_tpmi_info *tpmi_info, u8 owner) +{ + u64 control; + + return read_poll_timeout(readq, control, owner == FIELD_GET(TPMI_GENMASK_OWNER, control), + TPMI_CONTROL_TIMEOUT_US, TPMI_CONTROL_TIMEOUT_MAX_US, false, + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); +} + +static int tpmi_read_feature_status(struct intel_tpmi_info *tpmi_info, int feature_id, + int *locked, int *disabled) +{ + u64 control, data; + int ret; + + if (!tpmi_info->tpmi_control_mem) + return -EFAULT; + + mutex_lock(&tpmi_dev_lock); + + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_NONE); + if (ret) + goto err_unlock; + + /* set command id to 0x10 for TPMI_GET_STATE */ + data = TPMI_GET_STATE_CMD; + /* 32 bits for DATA offset and +8 for feature_id field */ + data |= ((u64)feature_id << (TPMI_CMD_DATA_OFFSET + TPMI_GET_STATE_CMD_DATA_OFFSET)); + + /* Write at command offset for qword access */ + writeq(data, tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET); + + ret = tpmi_wait_for_owner(tpmi_info, TPMI_OWNER_IN_BAND); + if (ret) + goto err_unlock; + + /* Set Run Busy and packet length of 2 dwords */ + writeq(BIT_ULL(TPMI_CONTROL_RB_BIT) | (TPMI_CMD_PKT_LEN << TPMI_CMD_PKT_LEN_OFFSET), + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); + + ret = read_poll_timeout(readq, control, !(control & BIT_ULL(TPMI_CONTROL_RB_BIT)), + TPMI_RB_TIMEOUT_US, TPMI_RB_TIMEOUT_MAX_US, false, + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); + if (ret) + goto done_proc; + + control = FIELD_GET(TPMI_GENMASK_STATUS, control); + if (control != TPMI_CMD_STATUS_SUCCESS) { + ret = -EBUSY; + goto done_proc; + } + + data = readq(tpmi_info->tpmi_control_mem + TPMI_COMMAND_OFFSET); + data >>= TPMI_CMD_DATA_OFFSET; /* Upper 32 bits are for TPMI_DATA */ + + *disabled = 0; + *locked = 0; + + if (!(data & BIT_ULL(TPMI_GET_STATUS_BIT_ENABLE))) + *disabled = 1; + + if (data & BIT_ULL(TPMI_GET_STATUS_BIT_LOCKED)) + *locked = 1; + + ret = 0; + +done_proc: + /* SET CPL "completion"bit */ + writeq(BIT_ULL(TPMI_CONTROL_CPL_BIT), + tpmi_info->tpmi_control_mem + TPMI_CONTROL_STATUS_OFFSET); + +err_unlock: + mutex_unlock(&tpmi_dev_lock); + + return ret; +} + +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, + int *locked, int *disabled) +{ + struct intel_vsec_device *intel_vsec_dev = dev_to_ivdev(auxdev->dev.parent); + struct intel_tpmi_info *tpmi_info = auxiliary_get_drvdata(&intel_vsec_dev->auxdev); + + return tpmi_read_feature_status(tpmi_info, feature_id, locked, disabled); +} +EXPORT_SYMBOL_NS_GPL(tpmi_get_feature_status, INTEL_TPMI); + +static void tpmi_set_control_base(struct auxiliary_device *auxdev, + struct intel_tpmi_info *tpmi_info, + struct intel_tpmi_pm_feature *pfs) +{ + void __iomem *mem; + u16 size; + + size = pfs->pfs_header.num_entries * pfs->pfs_header.entry_size * 4; + mem = devm_ioremap(&auxdev->dev, pfs->vsec_offset, size); + if (!mem) + return; + + /* mem is pointing to TPMI CONTROL base */ + tpmi_info->tpmi_control_mem = mem; +} + static const char *intel_tpmi_name(enum intel_tpmi_id id) { switch (id) { @@ -367,6 +511,9 @@ static int intel_vsec_tpmi_init(struct auxiliary_device *auxdev) */ if (pfs->pfs_header.tpmi_id == TPMI_INFO_ID) tpmi_process_info(tpmi_info, pfs); + + if (pfs->pfs_header.tpmi_id == TPMI_CONTROL_ID) + tpmi_set_control_base(auxdev, tpmi_info, pfs); } tpmi_info->pfs_start = pfs_start; diff --git a/include/linux/intel_tpmi.h b/include/linux/intel_tpmi.h index f505788c05da..04d937ad4dc4 100644 --- a/include/linux/intel_tpmi.h +++ b/include/linux/intel_tpmi.h @@ -27,4 +27,6 @@ struct intel_tpmi_plat_info *tpmi_get_platform_data(struct auxiliary_device *aux struct resource *tpmi_get_resource_at_index(struct auxiliary_device *auxdev, int index); int tpmi_get_resource_count(struct auxiliary_device *auxdev); +int tpmi_get_feature_status(struct auxiliary_device *auxdev, int feature_id, int *locked, + int *disabled); #endif