[PATCH 0/3] Minor SST optimizations

[PATCH 0/3] Minor SST optimizations

[Srinivas Pandruvada]

Contains some minor changes to use SST for non dynamic use cases
for display purpose and remove hardcoded size for map.

Srinivas Pandruvada (3):
  platform/x86: ISST: Use fuse enabled mask instead of allowed levels
  platform/x86: ISST: Allow level 0 to be not present
  platform/x86: intel_speed_select_if: Remove hardcoded map size

 drivers/platform/x86/intel/speed_select_if/isst_if_mmio.c  | 7 +++++--
 .../platform/x86/intel/speed_select_if/isst_tpmi_core.c    | 5 +----
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH 1/3] platform/x86: ISST: Use fuse enabled mask instead of allowed levels

[Srinivas Pandruvada]

Allowed level mask is a mask of levels, which are currently allowed
to dynamically switch. But even dynamic switching is not allowed,
user should be able to check all parameters for selection via BIOS.

So when passing the level mask for display to user space, use fuse
enabled mask, which has all levels.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
index 37f17e229419..48465636aadb 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
@@ -712,7 +712,7 @@ static int isst_if_get_perf_level(void __user *argp)
 		return -EINVAL;
 
 	perf_level.max_level = power_domain_info->max_level;
-	perf_level.level_mask = power_domain_info->pp_header.allowed_level_mask;
+	perf_level.level_mask = power_domain_info->pp_header.level_en_mask;
 	perf_level.feature_rev = power_domain_info->pp_header.feature_rev;
 	_read_pp_info("current_level", perf_level.current_level, SST_PP_STATUS_OFFSET,
 		      SST_PP_LEVEL_START, SST_PP_LEVEL_WIDTH, SST_MUL_FACTOR_NONE)
-- 
2.41.0

[PATCH 2/3] platform/x86: ISST: Allow level 0 to be not present

[Srinivas Pandruvada]

It is possible that SST level 0 or base level is not present in some
configurations. So don't set level 0 mask in level_en_mask by default.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
index 48465636aadb..e6d84ce0e7a5 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
@@ -372,9 +372,6 @@ static int sst_main(struct auxiliary_device *auxdev, struct tpmi_per_power_domai
 	/* Read PP header */
 	*((u64 *)&pd_info->pp_header) = readq(pd_info->sst_base + pd_info->sst_header.pp_offset);
 
-	/* Force level_en_mask level 0 */
-	pd_info->pp_header.level_en_mask |= 0x01;
-
 	mask = 0x01;
 	levels = 0;
 	for (i = 0; i < 8; ++i) {
-- 
2.41.0

[PATCH 3/3] platform/x86: intel_speed_select_if: Remove hardcoded map size

[Srinivas Pandruvada]

The driver is using 256 as the size while calling devm_ioremap(). The
maximum offset is already part of struct isst_mmio_range. Use the
maximum offset (end field of the struct) plus 4 as the map size to remove
hardcoded value of 256.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/platform/x86/intel/speed_select_if/isst_if_mmio.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/speed_select_if/isst_if_mmio.c b/drivers/platform/x86/intel/speed_select_if/isst_if_mmio.c
index ff49025ec085..be709e0c0c00 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_if_mmio.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_if_mmio.c
@@ -114,13 +114,16 @@ static int isst_if_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pcu_base &= GENMASK(10, 0);
 	base_addr = (u64)mmio_base << 23 | (u64) pcu_base << 12;
-	punit_dev->punit_mmio = devm_ioremap(&pdev->dev, base_addr, 256);
+
+	punit_dev->mmio_range = (struct isst_mmio_range *) ent->driver_data;
+
+	punit_dev->punit_mmio = devm_ioremap(&pdev->dev, base_addr,
+					     punit_dev->mmio_range[1].end + sizeof(u32));
 	if (!punit_dev->punit_mmio)
 		return -ENOMEM;
 
 	mutex_init(&punit_dev->mutex);
 	pci_set_drvdata(pdev, punit_dev);
-	punit_dev->mmio_range = (struct isst_mmio_range *) ent->driver_data;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.cmd_size = sizeof(struct isst_if_io_reg);
-- 
2.41.0

Re: [PATCH 3/3] platform/x86: intel_speed_select_if: Remove hardcoded map size

[Andy Shevchenko]

On Mon, Sep 25, 2023 at 12:45:55PM -0700, Srinivas Pandruvada wrote:
> The driver is using 256 as the size while calling devm_ioremap(). The
> maximum offset is already part of struct isst_mmio_range. Use the
> maximum offset (end field of the struct) plus 4 as the map size to remove
> hardcoded value of 256.

...

> +	punit_dev->mmio_range = (struct isst_mmio_range *) ent->driver_data;
> +
> +	punit_dev->punit_mmio = devm_ioremap(&pdev->dev, base_addr,
> +					     punit_dev->mmio_range[1].end + sizeof(u32));

Can we rather fix the mmio_range driver data to have end be actually not the
offset of the last dword? (Better maybe to keep length there.)

With help of

	struct resource r;
	...
	r = DEFINE_RES_MEM(base_addr, mmio_range.beg + mmio_range.len);

you can switch to devm_ioremap_resource() API.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] platform/x86: intel_speed_select_if: Remove hardcoded map size

[srinivas pandruvada]

On Tue, 2023-09-26 at 16:16 +0300, Andy Shevchenko wrote:
> On Mon, Sep 25, 2023 at 12:45:55PM -0700, Srinivas Pandruvada wrote:
> > The driver is using 256 as the size while calling devm_ioremap().
> > The
> > maximum offset is already part of struct isst_mmio_range. Use the
> > maximum offset (end field of the struct) plus 4 as the map size to
> > remove
> > hardcoded value of 256.
> 
> ...
> 
> > +       punit_dev->mmio_range = (struct isst_mmio_range *) ent-
> > >driver_data;
> > +
> > +       punit_dev->punit_mmio = devm_ioremap(&pdev->dev, base_addr,
> > +                                            punit_dev-
> > >mmio_range[1].end + sizeof(u32));
> 
> Can we rather fix the mmio_range driver data to have end be actually
> not the
> offset of the last dword? (Better maybe to keep length there.)
> 
We can. But that has to be separate patch on top as there are other
places this range is used.


> With help of
> 
>         struct resource r;
>         ...
>         r = DEFINE_RES_MEM(base_addr, mmio_range.beg +
> mmio_range.len);
> 
> you can switch to devm_ioremap_resource() API.
What is the advantage of creating a resource and then call
devm_ioremap_resource()?

Thanks,
Srinivas

> 

Re: [PATCH 3/3] platform/x86: intel_speed_select_if: Remove hardcoded map size

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 08:04:46AM -0700, srinivas pandruvada wrote:
> On Tue, 2023-09-26 at 16:16 +0300, Andy Shevchenko wrote:
> > On Mon, Sep 25, 2023 at 12:45:55PM -0700, Srinivas Pandruvada wrote:
> > > The driver is using 256 as the size while calling devm_ioremap().
> > > The
> > > maximum offset is already part of struct isst_mmio_range. Use the
> > > maximum offset (end field of the struct) plus 4 as the map size to
> > > remove
> > > hardcoded value of 256.

...

> > > +       punit_dev->mmio_range = (struct isst_mmio_range *) ent-
> > > >driver_data;
> > > +
> > > +       punit_dev->punit_mmio = devm_ioremap(&pdev->dev, base_addr,
> > > +                                            punit_dev-
> > > >mmio_range[1].end + sizeof(u32));
> > 
> > Can we rather fix the mmio_range driver data to have end be actually
> > not the
> > offset of the last dword? (Better maybe to keep length there.)
> > 
> We can. But that has to be separate patch on top as there are other
> places this range is used.

Still you can add a third member for now and then clean up it later as it's all
in one file.

...

> > With help of
> > 
> >         struct resource r;
> >         ...
> >         r = DEFINE_RES_MEM(base_addr, mmio_range.beg +
> > mmio_range.len);
> > 
> > you can switch to devm_ioremap_resource() API.
> What is the advantage of creating a resource and then call
> devm_ioremap_resource()?

It manages resource via global resource management and also prints an error
messages in case of errors.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] platform/x86: ISST: Use fuse enabled mask instead of allowed levels

[Ilpo Järvinen]

On Mon, 25 Sep 2023, Srinivas Pandruvada wrote:

> Allowed level mask is a mask of levels, which are currently allowed
> to dynamically switch. But even dynamic switching is not allowed,

even if ?

> user should be able to check all parameters for selection via BIOS.

I think you're lacking a negation in the above paragraph because it sounds 
like there's an internal contradiction in it. Can you please take a look.

-- 
 i.


> So when passing the level mask for display to user space, use fuse
> enabled mask, which has all levels.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> index 37f17e229419..48465636aadb 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> @@ -712,7 +712,7 @@ static int isst_if_get_perf_level(void __user *argp)
>  		return -EINVAL;
>  
>  	perf_level.max_level = power_domain_info->max_level;
> -	perf_level.level_mask = power_domain_info->pp_header.allowed_level_mask;
> +	perf_level.level_mask = power_domain_info->pp_header.level_en_mask;
>  	perf_level.feature_rev = power_domain_info->pp_header.feature_rev;
>  	_read_pp_info("current_level", perf_level.current_level, SST_PP_STATUS_OFFSET,
>  		      SST_PP_LEVEL_START, SST_PP_LEVEL_WIDTH, SST_MUL_FACTOR_NONE)
> 

Re: [PATCH 2/3] platform/x86: ISST: Allow level 0 to be not present

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On Mon, 25 Sep 2023, Srinivas Pandruvada wrote:

> It is possible that SST level 0 or base level is not present in some
> configurations. So don't set level 0 mask in level_en_mask by default.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> index 48465636aadb..e6d84ce0e7a5 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> @@ -372,9 +372,6 @@ static int sst_main(struct auxiliary_device *auxdev, struct tpmi_per_power_domai
>  	/* Read PP header */
>  	*((u64 *)&pd_info->pp_header) = readq(pd_info->sst_base + pd_info->sst_header.pp_offset);
>  
> -	/* Force level_en_mask level 0 */
> -	pd_info->pp_header.level_en_mask |= 0x01;
> -
>  	mask = 0x01;
>  	levels = 0;
>  	for (i = 0; i < 8; ++i) {
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH 1/3] platform/x86: ISST: Use fuse enabled mask instead of allowed levels

[srinivas pandruvada]

On Fri, 2023-09-29 at 15:14 +0300, Ilpo Järvinen wrote:
> On Mon, 25 Sep 2023, Srinivas Pandruvada wrote:
> 
> > Allowed level mask is a mask of levels, which are currently allowed
> > to dynamically switch. But even dynamic switching is not allowed,
> 
> even if ?
OK

> 
> > user should be able to check all parameters for selection via BIOS.
> 
> I think you're lacking a negation in the above paragraph because it
> sounds 
> like there's an internal contradiction in it. Can you please take a
> look.

I can try to improve.

>