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

[PATCH v2] extcon: extcon-axp288: use low level P-Unit semaphore lock for axp288 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:
	- add a new depends on in Kconfig
	- add Tested-by Reviewed-by tags
 
 drivers/extcon/Kconfig         |  2 +-
 drivers/extcon/extcon-axp288.c | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

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..460402b14ef2 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,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 	unsigned int cable = info->previous_cable;
 	bool vbus_attach = false;
 
+	iosf_mbi_block_punit_i2c_access();
+
 	vbus_attach = axp288_get_vbus_attach(info);
 	if (!vbus_attach)
 		goto no_vbus;
@@ -253,6 +256,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 +280,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");
 
@@ -307,11 +314,14 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
 
 static void axp288_extcon_enable(struct axp288_extcon_info *info)
 {
+	iosf_mbi_block_punit_i2c_access();
 	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();
 }
 
 static void axp288_put_role_sw(void *data)
@@ -384,10 +394,14 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		}
 	}
 
+	iosf_mbi_block_punit_i2c_access();
+
 	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);
-- 
2.20.1

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

[Chanwoo Choi]

Hi,

On 21. 9. 10. 오후 4:34, 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:
> 	- add a new depends on in Kconfig
> 	- add Tested-by Reviewed-by tags
>   
>   drivers/extcon/Kconfig         |  2 +-
>   drivers/extcon/extcon-axp288.c | 14 ++++++++++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> 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..460402b14ef2 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,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>   	unsigned int cable = info->previous_cable;
>   	bool vbus_attach = false;
>   
> +	iosf_mbi_block_punit_i2c_access();

You have to check the return value of iosf_mbi_block_punit_i2c_access()

> +
>   	vbus_attach = axp288_get_vbus_attach(info);
>   	if (!vbus_attach)
>   		goto no_vbus;
> @@ -253,6 +256,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 +280,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");
>   
> @@ -307,11 +314,14 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>   
>   static void axp288_extcon_enable(struct axp288_extcon_info *info)
>   {
> +	iosf_mbi_block_punit_i2c_access();

ditto.

>   	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();
>   }
>   
>   static void axp288_put_role_sw(void *data)
> @@ -384,10 +394,14 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> +	iosf_mbi_block_punit_i2c_access();

ditto.

> +
>   	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);
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

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

[Chanwoo Choi]

Hi,

If not critical, you better to write the patch title within 75 char.

Regards,
Chanwoo Choi

On 21. 9. 10. 오후 4:34, 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:
> 	- add a new depends on in Kconfig
> 	- add Tested-by Reviewed-by tags
>   
>   drivers/extcon/Kconfig         |  2 +-
>   drivers/extcon/extcon-axp288.c | 14 ++++++++++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> 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..460402b14ef2 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,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>   	unsigned int cable = info->previous_cable;
>   	bool vbus_attach = false;
>   
> +	iosf_mbi_block_punit_i2c_access();
> +
>   	vbus_attach = axp288_get_vbus_attach(info);
>   	if (!vbus_attach)
>   		goto no_vbus;
> @@ -253,6 +256,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 +280,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");
>   
> @@ -307,11 +314,14 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>   
>   static void axp288_extcon_enable(struct axp288_extcon_info *info)
>   {
> +	iosf_mbi_block_punit_i2c_access();
>   	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();
>   }
>   
>   static void axp288_put_role_sw(void *data)
> @@ -384,10 +394,14 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> +	iosf_mbi_block_punit_i2c_access();
> +
>   	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);
> 

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

[Fabio Aiuto]

Hello Chanwoo,

On Thu, Sep 16, 2021 at 02:12:26AM +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 21. 9. 10. 오후 4:34, 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:
> > 	- add a new depends on in Kconfig
> > 	- add Tested-by Reviewed-by tags
> >   drivers/extcon/Kconfig         |  2 +-
> >   drivers/extcon/extcon-axp288.c | 14 ++++++++++++++
> >   2 files changed, 15 insertions(+), 1 deletion(-)

<snip>

> >   #define PS_STAT_VBUS_TRIGGER		BIT(0)
> > @@ -215,6 +216,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
> >   	unsigned int cable = info->previous_cable;
> >   	bool vbus_attach = false;
> > +	iosf_mbi_block_punit_i2c_access();
> 
> You have to check the return value of iosf_mbi_block_punit_i2c_access()

ok will send a v2, with all the fixes you pointed out.

<snip>

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

thank you,

fabio

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

[Fabio Aiuto]

Hello Chanwoo,

On Thu, Sep 16, 2021 at 02:14:35AM +0900, Chanwoo Choi wrote:
> Hi,
> 
> If not critical, you better to write the patch title within 75 char.

no problem, will fix this

> 
> Regards,
> Chanwoo Choi

thank you,

fabio