[PATCH v1 0/2] pinctrl: intel: Fix PWM initialisation

[PATCH v1 0/2] pinctrl: intel: Fix PWM initialisation

[Andy Shevchenko]

It appears that PWM instantiated from pinctrl-intel is configured
to a wrong flow. This mini-series to fix the issue. Note, patch 1
is comprehensive documentation paragraph to explain what the difference
in the programming flow and what the SoCs are affected.

The issue had been reported privately, hence no Closes tag.
I haven't added the Tested-by, so to make sure that it (still) works
I ask Alexis to give the formal Tag here in a response to the series.

The idea is to route this via pin control tree as there are already two patches
against PWM handling in pinctrl-intel. There is no need to backport that, it's
optional, because it wasn't worked from day 1, and hence no Cc: stable@.

Cc: Alexis GUILLEMET<alexis.guillemet@dunasys.com>

Andy Shevchenko (2):
  pwm: lpss: Clarify the bypass member semantics in struct
    pwm_lpss_boardinfo
  pinctrl: intel: Fix wrong bypass assignment in
    intel_pinctrl_probe_pwm()

 drivers/pinctrl/intel/pinctrl-intel.c      |  1 -
 include/linux/platform_data/x86/pwm-lpss.h | 33 ++++++++++++++++++++--
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.45.1.3035.g276e886db78b

[PATCH v1 1/2] pwm: lpss: Clarify the bypass member semantics in struct pwm_lpss_boardinfo

[Andy Shevchenko]

Instead of an odd comment, cite the documentation, which says more clearly
what's going on with the programming flow on some of the Intel SoCs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/platform_data/x86/pwm-lpss.h | 33 ++++++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
index 752c06b47cc8..f0349edb47f4 100644
--- a/include/linux/platform_data/x86/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -15,9 +15,36 @@ struct pwm_lpss_boardinfo {
 	unsigned int npwm;
 	unsigned long base_unit_bits;
 	/*
-	 * Some versions of the IP may stuck in the state machine if enable
-	 * bit is not set, and hence update bit will show busy status till
-	 * the reset. For the rest it may be otherwise.
+	 * NOTE:
+	 * Intel Broxton, Apollo Lake, and Gemini Lake have different programming flow.
+	 *
+	 * Initial Enable or First Activation
+	 * 1. Program the base unit and on time divisor values.
+	 * 2. Set the software update bit.
+	 * 3. Poll in a loop on the PWMCTRL bit until software update bit is cleared.+
+	 * 4. Enable the PWM output by setting PWM Enable.
+	 * 5. Repeat the above steps for the next PWM Module.
+	 *
+	 * Dynamic update while PWM is Enabled
+	 * 1. Program the base unit and on-time divisor values.
+	 * 2. Set the software update bit.
+	 * 3. Repeat the above steps for the next PWM module.
+	 *
+	 * + After setting PWMCTRL register's SW update bit, hardware automatically
+	 * deasserts the SW update bit after a brief delay. It was observed that
+	 * setting of PWM enable is typically done via read-modify-write of the PWMCTRL
+	 * register. If there is no/little delay between setting software update bit
+	 * and setting enable bit via read-modify-write, it is possible that the read
+	 * could return with software enable as 1. In that case, the last write to set
+	 * enable to 1 could also set sw_update to 1. If this happens, sw_update gets
+	 * stuck and the driver code can hang as it explicitly waits for sw_update bit
+	 * to be 0 after setting the enable bit to 1. To avoid this race condition,
+	 * SW should poll on the software update bit to make sure that it is 0 before
+	 * doing the read-modify-write to set the enable bit to 1.
+	 *
+	 * Also, we noted that if sw_update bit was set in step #1 above then when it
+	 * is set again in step #2, sw_update bit never gets cleared and the flow hangs.
+	 * As such, we need to make sure that sw_update bit is 0 when doing step #1.
 	 */
 	bool bypass;
 	/*
-- 
2.45.1.3035.g276e886db78b

[PATCH v1 2/2] pinctrl: intel: Fix wrong bypass assignment in intel_pinctrl_probe_pwm()

[Andy Shevchenko]

When instantiating PWM, the bypass should be set to false. The field
is used for the selected Intel SoCs that do not have PWM feature enabled
in their pin control IPs.

Fixes: eb78d3604d6b ("pinctrl: intel: Enumerate PWM device when community has a capability")
Reported-by: Alexis GUILLEMET<alexis.guillemet@dunasys.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 5c04079e852b..8d1f4e7a2fe2 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1543,7 +1543,6 @@ static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl,
 		.clk_rate = 19200000,
 		.npwm = 1,
 		.base_unit_bits = 22,
-		.bypass = true,
 	};
 	struct pwm_chip *chip;
 
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 2/2] pinctrl: intel: Fix wrong bypass assignment in intel_pinctrl_probe_pwm()

[Andy Shevchenko]

On Tue, Feb 11, 2025 at 01:11:57PM +0100, Alexis GUILLEMET wrote:
> On Mon, Feb 10, 2025 at 08:44PM +0200, Andy Shevchenko wrote:

> Tested-by: Alexis GUILLEMET<alexis.guillemet@dunasys.com>

Thank you for confirming, I'm going to push this to my review and testing queue soon.
It should appear in the next (v6.15-rc1) Linux kernel.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/2] pinctrl: intel: Fix PWM initialisation

[Mika Westerberg]

On Mon, Feb 10, 2025 at 09:44:49PM +0200, Andy Shevchenko wrote:
> It appears that PWM instantiated from pinctrl-intel is configured
> to a wrong flow. This mini-series to fix the issue. Note, patch 1
> is comprehensive documentation paragraph to explain what the difference
> in the programming flow and what the SoCs are affected.
> 
> The issue had been reported privately, hence no Closes tag.
> I haven't added the Tested-by, so to make sure that it (still) works
> I ask Alexis to give the formal Tag here in a response to the series.
> 
> The idea is to route this via pin control tree as there are already two patches
> against PWM handling in pinctrl-intel. There is no need to backport that, it's
> optional, because it wasn't worked from day 1, and hence no Cc: stable@.
> 
> Cc: Alexis GUILLEMET<alexis.guillemet@dunasys.com>
> 
> Andy Shevchenko (2):
>   pwm: lpss: Clarify the bypass member semantics in struct
>     pwm_lpss_boardinfo
>   pinctrl: intel: Fix wrong bypass assignment in
>     intel_pinctrl_probe_pwm()

Both,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>