[3/9] platform/x86/intel/sdsi: Support different GUIDs

  Newer versions of Intel On Demand hardware may have an expanded list of
registers to support new features. The register layout is identified by a
unique GUID that's read during driver probe. Add support for handling
different GUIDs and add support for current GUIDs [1].

[1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
---
 drivers/platform/x86/intel/sdsi.c | 47 +++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)
  

On Tue, Nov 01, 2022 at 12:10:17PM -0700, David E. Box wrote:
> Newer versions of Intel On Demand hardware may have an expanded list of
> registers to support new features. The register layout is identified by a
> unique GUID that's read during driver probe. Add support for handling
> different GUIDs and add support for current GUIDs [1].

> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst

Link: tag?

...

>  #define SDSI_MIN_SIZE_DWORDS		276
> -#define SDSI_SIZE_CONTROL		8
>  #define SDSI_SIZE_MAILBOX		1024
> -#define SDSI_SIZE_REGS			72
> +#define SDSI_SIZE_REGS			80
>  #define SDSI_SIZE_CMD			sizeof(u64)

> +#define SDSI_SIZE_MAILBOX		1024

Why do you need this second time?

...

> +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
> +{
> +	switch (table->guid) {
> +	case SDSI_GUID_V1:
> +		priv->control_size = 8;
> +		priv->registers_size = 72;
> +		break;
> +	case SDSI_GUID_V2:
> +		priv->control_size = 16;
> +		priv->registers_size = 80;

Maybe it makes sense to use previously defined constants here instead of magics?

> +		break;
> +	default:
> +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
  

On Wed, 2022-11-02 at 12:44 +0200, Andy Shevchenko wrote:
> On Tue, Nov 01, 2022 at 12:10:17PM -0700, David E. Box wrote:
> > Newer versions of Intel On Demand hardware may have an expanded list of
> > registers to support new features. The register layout is identified by a
> > unique GUID that's read during driver probe. Add support for handling
> > different GUIDs and add support for current GUIDs [1].
> > [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> 
> Link: tag?

Ack

> 
> ...
> 
> >  #define SDSI_MIN_SIZE_DWORDS		276
> > -#define SDSI_SIZE_CONTROL		8
> >  #define SDSI_SIZE_MAILBOX		1024
> > -#define SDSI_SIZE_REGS			72
> > +#define SDSI_SIZE_REGS			80
> >  #define SDSI_SIZE_CMD			sizeof(u64)
> > +#define SDSI_SIZE_MAILBOX		1024
> 
> Why do you need this second time?

typo

> 
> ...
> 
> > +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table
> > *table)
> > +{
> > +	switch (table->guid) {
> > +	case SDSI_GUID_V1:
> > +		priv->control_size = 8;
> > +		priv->registers_size = 72;
> > +		break;
> > +	case SDSI_GUID_V2:
> > +		priv->control_size = 16;
> > +		priv->registers_size = 80;
> 
> Maybe it makes sense to use previously defined constants here instead of
> magics?

The constant is used for the file size, which since is static is set to the max.
But I'll add defines for these.

> 
> > +		break;
> > +	default:
> > +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
  

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Newer versions of Intel On Demand hardware may have an expanded list of
> registers to support new features. The register layout is identified by a
> unique GUID that's read during driver probe. Add support for handling
> different GUIDs and add support for current GUIDs [1].
> 
> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

With Andy's remarks fixed this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/platform/x86/intel/sdsi.c | 47 +++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index bca05b4dd983..ab1f65919fc5 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -27,10 +27,10 @@
>  #define ACCESS_TYPE_LOCAL		3
>  
>  #define SDSI_MIN_SIZE_DWORDS		276
> -#define SDSI_SIZE_CONTROL		8
>  #define SDSI_SIZE_MAILBOX		1024
> -#define SDSI_SIZE_REGS			72
> +#define SDSI_SIZE_REGS			80
>  #define SDSI_SIZE_CMD			sizeof(u64)
> +#define SDSI_SIZE_MAILBOX		1024
>  
>  /*
>   * Write messages are currently up to the size of the mailbox
> @@ -76,6 +76,9 @@
>  #define DT_TBIR				GENMASK(2, 0)
>  #define DT_OFFSET(v)			((v) & GENMASK(31, 3))
>  
> +#define SDSI_GUID_V1			0x006DD191
> +#define SDSI_GUID_V2			0xF210D9EF
> +
>  enum sdsi_command {
>  	SDSI_CMD_PROVISION_AKC		= 0x04,
>  	SDSI_CMD_PROVISION_CAP		= 0x08,
> @@ -100,6 +103,9 @@ struct sdsi_priv {
>  	void __iomem		*control_addr;
>  	void __iomem		*mbox_addr;
>  	void __iomem		*regs_addr;
> +	int			control_size;
> +	int			maibox_size;
> +	int			registers_size;
>  	u32			guid;
>  	u32			features;
>  };
> @@ -444,6 +450,18 @@ static ssize_t registers_read(struct file *filp, struct kobject *kobj,
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct sdsi_priv *priv = dev_get_drvdata(dev);
>  	void __iomem *addr = priv->regs_addr;
> +	int size =  priv->registers_size;
> +
> +	/*
> +	 * The check below is performed by the sysfs caller based on the static
> +	 * file size. But this may be greater than the actual size which is based
> +	 * on the GUID. So check here again based on actual size before reading.
> +	 */
> +	if (off >= size)
> +		return 0;
> +
> +	if (off + count > size)
> +		count = size - off;
>  
>  	memcpy_fromio(buf, addr + off, count);
>  
> @@ -496,6 +514,24 @@ static const struct attribute_group sdsi_group = {
>  };
>  __ATTRIBUTE_GROUPS(sdsi);
>  
> +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
> +{
> +	switch (table->guid) {
> +	case SDSI_GUID_V1:
> +		priv->control_size = 8;
> +		priv->registers_size = 72;
> +		break;
> +	case SDSI_GUID_V2:
> +		priv->control_size = 16;
> +		priv->registers_size = 80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *parent,
>  				   struct disc_table *disc_table, struct resource *disc_res)
>  {
> @@ -537,7 +573,7 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
>  	if (IS_ERR(priv->control_addr))
>  		return PTR_ERR(priv->control_addr);
>  
> -	priv->mbox_addr = priv->control_addr + SDSI_SIZE_CONTROL;
> +	priv->mbox_addr = priv->control_addr + priv->control_size;
>  	priv->regs_addr = priv->mbox_addr + SDSI_SIZE_MAILBOX;
>  
>  	priv->features = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
> @@ -572,6 +608,11 @@ static int sdsi_probe(struct auxiliary_device *auxdev, const struct auxiliary_de
>  
>  	priv->guid = disc_table.guid;
>  
> +	/* Get guid based layout info */
> +	ret = sdsi_get_layout(priv, &disc_table);
> +	if (ret)
> +		return ret;
> +
>  	/* Map the SDSi mailbox registers */
>  	ret = sdsi_map_mbox_registers(priv, intel_cap_dev->pcidev, &disc_table, disc_res);
>  	if (ret)