[patch v11 09/12] platform/x86: mlx-platform: Simplify IO access to regmap context

[patch v11 09/12] platform/x86: mlx-platform: Simplify IO access to regmap context

[Vadim Pasternak]

Get rid of struct mlxplat_mlxcpld_regmap_context, which contains only the
field with base address. Instead obtain base memory address by
devm_ioport_map  to local variable and pass it to devm_regmap_init.

Modify mlxplat_mlxcpld_reg_read/write to directly use pointer to context
for IO operations.

Add missed description of pdev_hotplug for struct mlxplat_priv.

Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/platform/x86/mlx-platform.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/mlx-platform.c b/drivers/platform/x86/mlx-platform.c
index 03c9e7a..81a7306 100644
--- a/drivers/platform/x86/mlx-platform.c
+++ b/drivers/platform/x86/mlx-platform.c
@@ -83,6 +83,7 @@
 /* mlxplat_priv - platform private data
  * @pdev_i2c - i2c controller platform device
  * @pdev_mux - array of mux platform devices
+ * @pdev_hotplug - hotplug platform devices
  */
 struct mlxplat_priv {
 	struct platform_device *pdev_i2c;
@@ -298,27 +299,17 @@ struct mlxreg_core_hotplug_platform_data mlxplat_mlxcpld_msn21xx_data = {
 	.mask = MLXPLAT_CPLD_AGGR_MASK_DEF,
 };
 
-struct mlxplat_mlxcpld_regmap_context {
-	void __iomem *base;
-};
-
-static struct mlxplat_mlxcpld_regmap_context mlxplat_mlxcpld_regmap_ctx;
-
 static int
 mlxplat_mlxcpld_reg_read(void *context, unsigned int reg, unsigned int *val)
 {
-	struct mlxplat_mlxcpld_regmap_context *ctx = context;
-
-	*val = ioread8(ctx->base + reg);
+	*val = ioread8(context + reg);
 	return 0;
 }
 
 static int
 mlxplat_mlxcpld_reg_write(void *context, unsigned int reg, unsigned int val)
 {
-	struct mlxplat_mlxcpld_regmap_context *ctx = context;
-
-	iowrite8(val, ctx->base + reg);
+	iowrite8(val, context + reg);
 	return 0;
 }
 
@@ -407,6 +398,7 @@ static const struct dmi_system_id mlxplat_dmi_table[] __initconst = {
 static int __init mlxplat_init(void)
 {
 	struct mlxplat_priv *priv;
+	void __iomem *base;
 	int i, err;
 
 	if (!dmi_check_system(mlxplat_dmi_table))
@@ -446,16 +438,15 @@ static int __init mlxplat_init(void)
 		}
 	}
 
-	mlxplat_mlxcpld_regmap_ctx.base = devm_ioport_map(&mlxplat_dev->dev,
+	base = devm_ioport_map(&mlxplat_dev->dev,
 			       mlxplat_lpc_resources[1].start, 1);
-	if (IS_ERR(mlxplat_mlxcpld_regmap_ctx.base)) {
-		err = PTR_ERR(mlxplat_mlxcpld_regmap_ctx.base);
+	if (IS_ERR(base)) {
+		err = PTR_ERR(base);
 		goto fail_platform_mux_register;
 	}
 
 	mlxplat_hotplug->regmap = devm_regmap_init(&mlxplat_dev->dev, NULL,
-					&mlxplat_mlxcpld_regmap_ctx,
-					&mlxplat_mlxcpld_regmap_config);
+					base, &mlxplat_mlxcpld_regmap_config);
 	if (IS_ERR(mlxplat_hotplug->regmap)) {
 		err = PTR_ERR(mlxplat_hotplug->regmap);
 		goto fail_platform_mux_register;
-- 
2.1.4

[patch v11 10/12] platform/x86: mlx-platform: Extend register map configuration with IO access verification callbacks

[Vadim Pasternak]

Add definitions for hotplug device masks and events offsets.

Extend register map configuration with the sets of writable, readable and
volatile registers to allow verification prior to the access.

Extend register map configuration with cache type field: use flat type.

Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/platform/x86/mlx-platform.c | 71 +++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/platform/x86/mlx-platform.c b/drivers/platform/x86/mlx-platform.c
index 81a7306..95eb9d9 100644
--- a/drivers/platform/x86/mlx-platform.c
+++ b/drivers/platform/x86/mlx-platform.c
@@ -48,9 +48,18 @@
 #define MLXPLAT_CPLD_LPC_I2C_BASE_ADRR		0x2000
 #define MLXPLAT_CPLD_LPC_REG_BASE_ADRR		0x2500
 #define MLXPLAT_CPLD_LPC_REG_AGGR_OFFSET	0x3a
+#define MLXPLAT_CPLD_LPC_REG_AGGR_MASK_OFFSET	0x3b
+#define MLXPLAT_CPLD_LPC_REG_AGGRLO_OFFSET	0x40
+#define MLXPLAT_CPLD_LPC_REG_AGGRLO_MASK_OFFSET	0x41
 #define MLXPLAT_CPLD_LPC_REG_PSU_OFFSET		0x58
+#define MLXPLAT_CPLD_LPC_REG_PSU_EVENT_OFFSET	0x59
+#define MLXPLAT_CPLD_LPC_REG_PSU_MASK_OFFSET	0x5a
 #define MLXPLAT_CPLD_LPC_REG_PWR_OFFSET		0x64
+#define MLXPLAT_CPLD_LPC_REG_PWR_EVENT_OFFSET	0x65
+#define MLXPLAT_CPLD_LPC_REG_PWR_MASK_OFFSET	0x66
 #define MLXPLAT_CPLD_LPC_REG_FAN_OFFSET		0x88
+#define MLXPLAT_CPLD_LPC_REG_FAN_EVENT_OFFSET	0x89
+#define MLXPLAT_CPLD_LPC_REG_FAN_MASK_OFFSET	0x8a
 #define MLXPLAT_CPLD_LPC_IO_RANGE		0x100
 #define MLXPLAT_CPLD_LPC_I2C_CH1_OFF		0xdb
 #define MLXPLAT_CPLD_LPC_I2C_CH2_OFF		0xda
@@ -299,6 +308,64 @@ struct mlxreg_core_hotplug_platform_data mlxplat_mlxcpld_msn21xx_data = {
 	.mask = MLXPLAT_CPLD_AGGR_MASK_DEF,
 };
 
+static bool mlxplat_mlxcpld_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MLXPLAT_CPLD_LPC_REG_AGGR_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGRLO_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_MASK_OFFSET:
+		return true;
+	}
+	return false;
+}
+
+static bool mlxplat_mlxcpld_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MLXPLAT_CPLD_LPC_REG_AGGR_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGR_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGRLO_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGRLO_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_MASK_OFFSET:
+		return true;
+	}
+	return false;
+}
+
+static bool mlxplat_mlxcpld_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MLXPLAT_CPLD_LPC_REG_AGGR_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGR_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGRLO_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_AGGRLO_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PSU_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_PWR_MASK_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_EVENT_OFFSET:
+	case MLXPLAT_CPLD_LPC_REG_FAN_MASK_OFFSET:
+		return true;
+	}
+	return false;
+}
+
 static int
 mlxplat_mlxcpld_reg_read(void *context, unsigned int reg, unsigned int *val)
 {
@@ -317,6 +384,10 @@ static const struct regmap_config mlxplat_mlxcpld_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.max_register = 255,
+	.cache_type = REGCACHE_FLAT,
+	.writeable_reg = mlxplat_mlxcpld_writeable_reg,
+	.readable_reg = mlxplat_mlxcpld_readable_reg,
+	.volatile_reg = mlxplat_mlxcpld_volatile_reg,
 	.reg_read = mlxplat_mlxcpld_reg_read,
 	.reg_write = mlxplat_mlxcpld_reg_write,
 };
-- 
2.1.4

[patch v11 11/12] platform/mellanox: mlxreg-hotplug: Add check for negative adapter number value

[Vadim Pasternak]

Verify before creation of hotplug device if the associated adapter number
is negative. It could be in case hotplug event is not associated with
hotplug device.

Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/platform/mellanox/mlxreg-hotplug.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
index b891ec7..c806451 100644
--- a/drivers/platform/mellanox/mlxreg-hotplug.c
+++ b/drivers/platform/mellanox/mlxreg-hotplug.c
@@ -95,6 +95,14 @@ struct mlxreg_hotplug_priv_data {
 
 static int mlxreg_hotplug_device_create(struct mlxreg_core_data *data)
 {
+	/*
+	 * Return if adapter number is negative. Return if adapter number is
+	 * negative. It could be in case hotplug event is not associated with
+	 * hotplug device.
+	 */
+	if (data->hpdev.nr < 0)
+		return 0;
+
 	data->hpdev.adapter = i2c_get_adapter(data->hpdev.nr);
 	if (!data->hpdev.adapter)
 		return -EFAULT;
-- 
2.1.4

[patch v11 12/12] platform/mellanox: Add validation of return code of hotplug device creation routine

[Vadim Pasternak]

Adding validation of return code of mlxreg_hotplug_device_create. It could
fail in case the requested adapter is not available or if client can not
be connected to the adapter. Error is to be reported in case of bad flow.

Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/platform/mellanox/mlxreg-hotplug.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
index c806451..e852219 100644
--- a/drivers/platform/mellanox/mlxreg-hotplug.c
+++ b/drivers/platform/mellanox/mlxreg-hotplug.c
@@ -263,13 +263,15 @@ mlxreg_hotplug_work_helper(struct mlxreg_hotplug_priv_data *priv,
 			if (item->inversed)
 				mlxreg_hotplug_device_destroy(data);
 			else
-				mlxreg_hotplug_device_create(data);
+				ret = mlxreg_hotplug_device_create(data);
 		} else {
 			if (item->inversed)
-				mlxreg_hotplug_device_create(data);
+				ret = mlxreg_hotplug_device_create(data);
 			else
 				mlxreg_hotplug_device_destroy(data);
 		}
+		if (ret)
+			dev_err(priv->dev, "Failed to create hotplug device.\n");
 	}
 
 	/* Acknowledge event. */
@@ -312,8 +314,11 @@ mlxreg_hotplug_health_work_helper(struct mlxreg_hotplug_priv_data *priv,
 		if (regval == MLXREG_HOTPLUG_HEALTH_MASK) {
 			if ((data->health_cntr++ == MLXREG_HOTPLUG_RST_CNTR) ||
 			    !priv->after_probe) {
-				mlxreg_hotplug_device_create(data);
-				data->attached = true;
+				ret = mlxreg_hotplug_device_create(data);
+				if (ret)
+					dev_err(priv->dev, "Failed to create hotplug device.\n");
+				else
+					data->attached = true;
 			}
 		} else {
 			if (data->attached) {
-- 
2.1.4

Re: [patch v11 09/12] platform/x86: mlx-platform: Simplify IO access to regmap context

[Darren Hart]

On Wed, Jan 24, 2018 at 08:34:55PM +0000, Vadim Pasternak wrote:
> Get rid of struct mlxplat_mlxcpld_regmap_context, which contains only the
> field with base address. Instead obtain base memory address by
> devm_ioport_map  to local variable and pass it to devm_regmap_init.
> 
> Modify mlxplat_mlxcpld_reg_read/write to directly use pointer to context
> for IO operations.
> 
> Add missed description of pdev_hotplug for struct mlxplat_priv.

Separate patch. Keep in mind that if there is a bug, and a patch is reverted -
do you want it all reverted together? In this case, the comment is totally
unrelated to this change. I've made this change.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [patch v11 10/12] platform/x86: mlx-platform: Extend register map configuration with IO access verification callbacks

[Darren Hart]

On Wed, Jan 24, 2018 at 08:34:56PM +0000, Vadim Pasternak wrote:
> Add definitions for hotplug device masks and events offsets.
> 
> Extend register map configuration with the sets of writable, readable and
> volatile registers to allow verification prior to the access.
> 
> Extend register map configuration with cache type field: use flat type.

This says "what" you're doing, but not why. The point of the change log is to
help the reviewer or some future debugger to understand not WHAT you did (we
have that in the code below) but WHY you did it - keeping in mind that we likely
haven't spent the time you have to understand what the right thing is to do
here, so share with us the result of your research.

"Extend register map configuration with cache type field: use flat type, in
order to XXXXXXXXXX".

Please keep in mind for future patches.

...

> +
>  static int
>  mlxplat_mlxcpld_reg_read(void *context, unsigned int reg, unsigned int *val)
>  {
> @@ -317,6 +384,10 @@ static const struct regmap_config mlxplat_mlxcpld_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
>  	.max_register = 255,
> +	.cache_type = REGCACHE_FLAT,

-- 
Darren Hart
VMware Open Source Technology Center

Re: [patch v11 11/12] platform/mellanox: mlxreg-hotplug: Add check for negative adapter number value

[Darren Hart]

On Wed, Jan 24, 2018 at 08:34:57PM +0000, Vadim Pasternak wrote:
> Verify before creation of hotplug device if the associated adapter number
> is negative. It could be in case hotplug event is not associated with
> hotplug device.
> 
> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
> ---
>  drivers/platform/mellanox/mlxreg-hotplug.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
> index b891ec7..c806451 100644
> --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> @@ -95,6 +95,14 @@ struct mlxreg_hotplug_priv_data {
>  
>  static int mlxreg_hotplug_device_create(struct mlxreg_core_data *data)
>  {
> +	/*
> +	 * Return if adapter number is negative. Return if adapter number is
> +	 * negative. It could be in case hotplug event is not associated with

First sentence repeated twice. I've corrected this.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [patch v11 12/12] platform/mellanox: Add validation of return code of hotplug device creation routine

[Darren Hart]

On Wed, Jan 24, 2018 at 08:34:58PM +0000, Vadim Pasternak wrote:
> Adding validation of return code of mlxreg_hotplug_device_create. It could
> fail in case the requested adapter is not available or if client can not
> be connected to the adapter. Error is to be reported in case of bad flow.
> 

I had dropped the removal of the dev_err messages patch as it was based on an
inaccurate assumption that the return codes were checked. We could just include
that change together with this change as they are tightly coupled.

But...


> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
> ---
>  drivers/platform/mellanox/mlxreg-hotplug.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c
> index c806451..e852219 100644
> --- a/drivers/platform/mellanox/mlxreg-hotplug.c
> +++ b/drivers/platform/mellanox/mlxreg-hotplug.c
> @@ -263,13 +263,15 @@ mlxreg_hotplug_work_helper(struct mlxreg_hotplug_priv_data *priv,
>  			if (item->inversed)
>  				mlxreg_hotplug_device_destroy(data);
>  			else
> -				mlxreg_hotplug_device_create(data);
> +				ret = mlxreg_hotplug_device_create(data);
>  		} else {
>  			if (item->inversed)
> -				mlxreg_hotplug_device_create(data);
> +				ret = mlxreg_hotplug_device_create(data);
>  			else
>  				mlxreg_hotplug_device_destroy(data);
>  		}
> +		if (ret)
> +			dev_err(priv->dev, "Failed to create hotplug device.\n");

So we've moved the error message - but we have affected any functional change -
we still don't DO anything (or NOT DO anything) if the create call fails. Is
that the right thing?

>  	}
>  
>  	/* Acknowledge event. */
> @@ -312,8 +314,11 @@ mlxreg_hotplug_health_work_helper(struct mlxreg_hotplug_priv_data *priv,
>  		if (regval == MLXREG_HOTPLUG_HEALTH_MASK) {
>  			if ((data->health_cntr++ == MLXREG_HOTPLUG_RST_CNTR) ||
>  			    !priv->after_probe) {
> -				mlxreg_hotplug_device_create(data);
> -				data->attached = true;
> +				ret = mlxreg_hotplug_device_create(data);
> +				if (ret)
> +					dev_err(priv->dev, "Failed to create hotplug device.\n");

I meant to bring this up - rather than repeat the exact same message as above,
should this read:

"Failed to create hotplug health device.\n" Or something similar? This would
provide the user/developer with more information about what is failing.

> +				else
> +					data->attached = true;

And this does change behavior, but isn't noted in the changelog.

I have pushed my v11 including everything up to and excluding this patch here:

https://github.com/dvhart/linux-pdx86/tree/review-dvhart-mellanox-v11

(also on the original repo at infradead which appears to be back up and running,
just giving it time before switching back 100%)

-- 
Darren Hart
VMware Open Source Technology Center