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

[Hans de Goede <hdegoede@redhat.com>]

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