* [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses
@ 2021-09-16 7:12 Fabio Aiuto
2021-09-16 11:12 ` Hans de Goede
2021-09-19 9:33 ` Chanwoo Choi
0 siblings, 2 replies; 5+ messages in thread
From: Fabio Aiuto @ 2021-09-16 7:12 UTC (permalink / raw)
To: Chanwoo Choi; +Cc: MyungJoo Ham, Hans de Goede, linux-kernel, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses
2021-09-16 7:12 [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses Fabio Aiuto
@ 2021-09-16 11:12 ` Hans de Goede
2021-09-17 8:04 ` Fabio Aiuto
2021-09-19 9:33 ` Chanwoo Choi
1 sibling, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2021-09-16 11:12 UTC (permalink / raw)
To: Fabio Aiuto, Chanwoo Choi; +Cc: MyungJoo Ham, linux-kernel
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses
2021-09-16 11:12 ` Hans de Goede
@ 2021-09-17 8:04 ` Fabio Aiuto
0 siblings, 0 replies; 5+ messages in thread
From: Fabio Aiuto @ 2021-09-17 8:04 UTC (permalink / raw)
To: Hans de Goede; +Cc: Chanwoo Choi, MyungJoo Ham, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses
2021-09-16 7:12 [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses Fabio Aiuto
2021-09-16 11:12 ` Hans de Goede
@ 2021-09-19 9:33 ` Chanwoo Choi
2021-09-20 7:03 ` Fabio Aiuto
1 sibling, 1 reply; 5+ messages in thread
From: Chanwoo Choi @ 2021-09-19 9:33 UTC (permalink / raw)
To: Fabio Aiuto, Chanwoo Choi; +Cc: MyungJoo Ham, Hans de Goede, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-20 7:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 7:12 [PATCH v2] extcon: extcon-axp288: use P-Unit semaphore lock for register accesses Fabio Aiuto
2021-09-16 11:12 ` Hans de Goede
2021-09-17 8:04 ` Fabio Aiuto
2021-09-19 9:33 ` Chanwoo Choi
2021-09-20 7:03 ` Fabio Aiuto
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.