[PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses

[PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses

[Fabio Aiuto]

use low level P-Unit semaphore lock for axp288 register
accesses directly and for more than one access a time,
to reduce the number of times this semaphore is locked
and released which is an expensive operation.

i2c-bus to the XPower is shared between the kernel and the
SoCs P-Unit. The P-Unit has a semaphore wich the kernel must
lock for axp288 register accesses. When the P-Unit semaphore
is locked CPU and GPU power states cannot change or the system
will freeze.

The P-Unit semaphore lock is already managed inside the regmap
access logic, but for each access the semaphore is locked and
released. So use directly iosf_mbi_(un)block_punit_i2c_access(),
we are safe in doing so because nested calls to the same
semaphore are turned to nops.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
---
Changes in v2:
	- shortened patch title within 75 char
	- added return value check in function
	  iosf_mbi_lock_punit_i2c_access() calls

 drivers/extcon/Kconfig         |  2 +-
 drivers/extcon/extcon-axp288.c | 31 +++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index c69d40ae5619..aab87c9b35c8 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -23,7 +23,7 @@ config EXTCON_ADC_JACK
 
 config EXTCON_AXP288
 	tristate "X-Power AXP288 EXTCON support"
-	depends on MFD_AXP20X && USB_SUPPORT && X86 && ACPI
+	depends on MFD_AXP20X && USB_SUPPORT && X86 && ACPI && IOSF_MBI
 	select USB_ROLE_SWITCH
 	help
 	  Say Y here to enable support for USB peripheral detection
diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index fdb31954cf2b..7c6d5857ff25 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -24,6 +24,7 @@
 
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
+#include <asm/iosf_mbi.h>
 
 /* Power source status register */
 #define PS_STAT_VBUS_TRIGGER		BIT(0)
@@ -215,6 +216,10 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 	unsigned int cable = info->previous_cable;
 	bool vbus_attach = false;
 
+	ret = iosf_mbi_block_punit_i2c_access();
+	if (ret < 0)
+		return ret;
+
 	vbus_attach = axp288_get_vbus_attach(info);
 	if (!vbus_attach)
 		goto no_vbus;
@@ -253,6 +258,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 	}
 
 no_vbus:
+	iosf_mbi_unblock_punit_i2c_access();
+
 	extcon_set_state_sync(info->edev, info->previous_cable, false);
 	if (info->previous_cable == EXTCON_CHG_USB_SDP)
 		extcon_set_state_sync(info->edev, EXTCON_USB, false);
@@ -275,6 +282,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 	return 0;
 
 dev_det_ret:
+	iosf_mbi_unblock_punit_i2c_access();
+
 	if (ret < 0)
 		dev_err(info->dev, "failed to detect BC Mod\n");
 
@@ -305,13 +314,23 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void axp288_extcon_enable(struct axp288_extcon_info *info)
+static int axp288_extcon_enable(struct axp288_extcon_info *info)
 {
+	int ret = 0;
+
+	ret = iosf_mbi_block_punit_i2c_access();
+	if (ret < 0)
+		return ret;
+
 	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
 						BC_GLOBAL_RUN, 0);
 	/* Enable the charger detection logic */
 	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
 					BC_GLOBAL_RUN, BC_GLOBAL_RUN);
+
+	iosf_mbi_unblock_punit_i2c_access();
+
+	return ret;
 }
 
 static void axp288_put_role_sw(void *data)
@@ -384,10 +403,16 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = iosf_mbi_block_punit_i2c_access();
+	if (ret < 0)
+		return ret;
+
 	info->vbus_attach = axp288_get_vbus_attach(info);
 
 	axp288_extcon_log_rsi(info);
 
+	iosf_mbi_unblock_punit_i2c_access();
+
 	/* Initialize extcon device */
 	info->edev = devm_extcon_dev_allocate(&pdev->dev,
 					      axp288_extcon_cables);
@@ -441,7 +466,9 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	}
 
 	/* Start charger cable type detection */
-	axp288_extcon_enable(info);
+	ret = axp288_extcon_enable(info);
+	if (ret < 0)
+		return ret;
 
 	device_init_wakeup(dev, true);
 	platform_set_drvdata(pdev, info);
-- 
2.20.1

Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses

[Hans de Goede]

Hi,

On 9/16/21 9:12 AM, Fabio Aiuto wrote:
> use low level P-Unit semaphore lock for axp288 register
> accesses directly and for more than one access a time,
> to reduce the number of times this semaphore is locked
> and released which is an expensive operation.
> 
> i2c-bus to the XPower is shared between the kernel and the
> SoCs P-Unit. The P-Unit has a semaphore wich the kernel must
> lock for axp288 register accesses. When the P-Unit semaphore
> is locked CPU and GPU power states cannot change or the system
> will freeze.
> 
> The P-Unit semaphore lock is already managed inside the regmap
> access logic, but for each access the semaphore is locked and
> released. So use directly iosf_mbi_(un)block_punit_i2c_access(),
> we are safe in doing so because nested calls to the same
> semaphore are turned to nops.
> 
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> ---
> Changes in v2:
> 	- shortened patch title within 75 char
> 	- added return value check in function
> 	  iosf_mbi_lock_punit_i2c_access() calls


Actually your last version was v2, so this one should have
been v3 (no need to resend it just for that).

Other then that remark this looks good, thank you.

Regards,

Hans


> 
>  drivers/extcon/Kconfig         |  2 +-
>  drivers/extcon/extcon-axp288.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index c69d40ae5619..aab87c9b35c8 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -23,7 +23,7 @@ config EXTCON_ADC_JACK
>  
>  config EXTCON_AXP288
>  	tristate "X-Power AXP288 EXTCON support"
> -	depends on MFD_AXP20X && USB_SUPPORT && X86 && ACPI
> +	depends on MFD_AXP20X && USB_SUPPORT && X86 && ACPI && IOSF_MBI
>  	select USB_ROLE_SWITCH
>  	help
>  	  Say Y here to enable support for USB peripheral detection
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index fdb31954cf2b..7c6d5857ff25 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -24,6 +24,7 @@
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/intel-family.h>
> +#include <asm/iosf_mbi.h>
>  
>  /* Power source status register */
>  #define PS_STAT_VBUS_TRIGGER		BIT(0)
> @@ -215,6 +216,10 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  	unsigned int cable = info->previous_cable;
>  	bool vbus_attach = false;
>  
> +	ret = iosf_mbi_block_punit_i2c_access();
> +	if (ret < 0)
> +		return ret;
> +
>  	vbus_attach = axp288_get_vbus_attach(info);
>  	if (!vbus_attach)
>  		goto no_vbus;
> @@ -253,6 +258,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  	}
>  
>  no_vbus:
> +	iosf_mbi_unblock_punit_i2c_access();
> +
>  	extcon_set_state_sync(info->edev, info->previous_cable, false);
>  	if (info->previous_cable == EXTCON_CHG_USB_SDP)
>  		extcon_set_state_sync(info->edev, EXTCON_USB, false);
> @@ -275,6 +282,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  	return 0;
>  
>  dev_det_ret:
> +	iosf_mbi_unblock_punit_i2c_access();
> +
>  	if (ret < 0)
>  		dev_err(info->dev, "failed to detect BC Mod\n");
>  
> @@ -305,13 +314,23 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void axp288_extcon_enable(struct axp288_extcon_info *info)
> +static int axp288_extcon_enable(struct axp288_extcon_info *info)
>  {
> +	int ret = 0;
> +
> +	ret = iosf_mbi_block_punit_i2c_access();
> +	if (ret < 0)
> +		return ret;
> +
>  	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>  						BC_GLOBAL_RUN, 0);
>  	/* Enable the charger detection logic */
>  	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>  					BC_GLOBAL_RUN, BC_GLOBAL_RUN);
> +
> +	iosf_mbi_unblock_punit_i2c_access();
> +
> +	return ret;
>  }
>  
>  static void axp288_put_role_sw(void *data)
> @@ -384,10 +403,16 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ret = iosf_mbi_block_punit_i2c_access();
> +	if (ret < 0)
> +		return ret;
> +
>  	info->vbus_attach = axp288_get_vbus_attach(info);
>  
>  	axp288_extcon_log_rsi(info);
>  
> +	iosf_mbi_unblock_punit_i2c_access();
> +
>  	/* Initialize extcon device */
>  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
>  					      axp288_extcon_cables);
> @@ -441,7 +466,9 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Start charger cable type detection */
> -	axp288_extcon_enable(info);
> +	ret = axp288_extcon_enable(info);
> +	if (ret < 0)
> +		return ret;
>  
>  	device_init_wakeup(dev, true);
>  	platform_set_drvdata(pdev, info);
> 

Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses

[Fabio Aiuto]

Hello Hans,

On Thu, Sep 16, 2021 at 01:12:53PM +0200, Hans de Goede wrote:
> Hi,
<snip>
> > 
> > Suggested-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> > ---
> > Changes in v2:
> > 	- shortened patch title within 75 char
> > 	- added return value check in function
> > 	  iosf_mbi_lock_punit_i2c_access() calls
> 
> 
> Actually your last version was v2, so this one should have
> been v3 (no need to resend it just for that).

oh sorry for mistake

> 
> Other then that remark this looks good, thank you.

thank you for review

> 
> Regards,
> 
> Hans

@Chanwoo, do you need for me to resend a v3 or
you can take the patch as-is?

thank you,

fabio

Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses

[Chanwoo Choi]

On 21. 9. 16. 오후 4:12, Fabio Aiuto wrote:
> use low level P-Unit semaphore lock for axp288 register
> accesses directly and for more than one access a time,
> to reduce the number of times this semaphore is locked
> and released which is an expensive operation.
> 
> i2c-bus to the XPower is shared between the kernel and the
> SoCs P-Unit. The P-Unit has a semaphore wich the kernel must
> lock for axp288 register accesses. When the P-Unit semaphore
> is locked CPU and GPU power states cannot change or the system
> will freeze.
> 
> The P-Unit semaphore lock is already managed inside the regmap
> access logic, but for each access the semaphore is locked and
> released. So use directly iosf_mbi_(un)block_punit_i2c_access(),
> we are safe in doing so because nested calls to the same
> semaphore are turned to nops.
> 
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Fabio Aiuto <fabioaiuto83@gmail.com>
> ---
> Changes in v2:
> 	- shortened patch title within 75 char
> 	- added return value check in function
> 	  iosf_mbi_lock_punit_i2c_access() calls
> 
>   drivers/extcon/Kconfig         |  2 +-
>   drivers/extcon/extcon-axp288.c | 31 +++++++++++++++++++++++++++++--
>   2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index c69d40ae5619..aab87c9b35c8 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -23,7 +23,7 @@ config EXTCON_ADC_JACK
>   
>   config EXTCON_AXP288
>   	tristate "X-Power AXP288 EXTCON support"
> -	depends on MFD_AXP20X && USB_SUPPORT && X86 && ACPI
> +	depends on MFD_AXP20X && USB_SUPPORT && X86 && ACPI && IOSF_MBI
>   	select USB_ROLE_SWITCH
>   	help
>   	  Say Y here to enable support for USB peripheral detection
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index fdb31954cf2b..7c6d5857ff25 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -24,6 +24,7 @@
>   
>   #include <asm/cpu_device_id.h>
>   #include <asm/intel-family.h>
> +#include <asm/iosf_mbi.h>
>   
>   /* Power source status register */
>   #define PS_STAT_VBUS_TRIGGER		BIT(0)
> @@ -215,6 +216,10 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>   	unsigned int cable = info->previous_cable;
>   	bool vbus_attach = false;
>   
> +	ret = iosf_mbi_block_punit_i2c_access();
> +	if (ret < 0)
> +		return ret;
> +
>   	vbus_attach = axp288_get_vbus_attach(info);
>   	if (!vbus_attach)n
>   		goto no_vbus;
> @@ -253,6 +258,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>   	}
>   
>   no_vbus:
> +	iosf_mbi_unblock_punit_i2c_access();
> +
>   	extcon_set_state_sync(info->edev, info->previous_cable, false);
>   	if (info->previous_cable == EXTCON_CHG_USB_SDP)
>   		extcon_set_state_sync(info->edev, EXTCON_USB, false);
> @@ -275,6 +282,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>   	return 0;
>   
>   dev_det_ret:
> +	iosf_mbi_unblock_punit_i2c_access();
> +
>   	if (ret < 0)
>   		dev_err(info->dev, "failed to detect BC Mod\n");
>   
> @@ -305,13 +314,23 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>   	return IRQ_HANDLED;
>   }
>   
> -static void axp288_extcon_enable(struct axp288_extcon_info *info)
> +static int axp288_extcon_enable(struct axp288_extcon_info *info)
>   {
> +	int ret = 0;
> +
> +	ret = iosf_mbi_block_punit_i2c_access();
> +	if (ret < 0)
> +		return ret;
> +
>   	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>   						BC_GLOBAL_RUN, 0);
>   	/* Enable the charger detection logic */
>   	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>   					BC_GLOBAL_RUN, BC_GLOBAL_RUN);
> +
> +	iosf_mbi_unblock_punit_i2c_access();
> +
> +	return ret;
>   }
>   
>   static void axp288_put_role_sw(void *data)
> @@ -384,10 +403,16 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> +	ret = iosf_mbi_block_punit_i2c_access();
> +	if (ret < 0)
> +		return ret;
> +
>   	info->vbus_attach = axp288_get_vbus_attach(info);
>   
>   	axp288_extcon_log_rsi(info);
>   
> +	iosf_mbi_unblock_punit_i2c_access();
> +
>   	/* Initialize extcon device */
>   	info->edev = devm_extcon_dev_allocate(&pdev->dev,
>   					      axp288_extcon_cables);
> @@ -441,7 +466,9 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>   	}
>   
>   	/* Start charger cable type detection */
> -	axp288_extcon_enable(info);
> +	ret = axp288_extcon_enable(info);
> +	if (ret < 0)
> +		return ret;
>   
>   	device_init_wakeup(dev, true);
>   	platform_set_drvdata(pdev, info);
> 

Applied it. Thanks.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses

[Fabio Aiuto]

Hello Chanwoo,

On Sun, Sep 19, 2021 at 06:33:47PM +0900, Chanwoo Choi wrote:
<snip>
> 
> Applied it. Thanks.

thanks a lot

> 
> -- 
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

fabio