[PATCH v4 0/2] Fix LPSS clock divider for XPS 9530

[PATCH v4 0/2] Fix LPSS clock divider for XPS 9530

[Aleksandrs Vinarskis]

Dell XPS 9530 (2023) uses spi-pxa2xx with clock-divider enabled from
intel-lpss with the ratio of 1:32767 (Dell's firmware bug). This caps SPI
controller's speed at very low value of 3051Hz, which makes the interface
practically unusable. Since either spi-pxa2xx or intel-lpss should have
clock divider enabled, not both, and SPI controller can have higher speed
than requested by the device, it is preffered to disable intel-lpss
clock-divider, and let SPI controller handle it.

Fixing this issue directly in Dell firmware by setting lpss divider to 1:1
would've been the ideal solution, but is unlikely to ever happen.

Particular driver already implements customized solution for handling buggy
ACPI tables for select Microsoft devices. This patch series converts it to
a more generic quirk table, and adds a new quirk QUIRK_CLOCK_DIVIDER_UNITY
which forces clock divider to be set to 1:1. In the future, devices with
similar bug (if any) can be easily added to the same pci id lookup table.

Changes since v3:
- Altered "{ PCI.." style of quirk table
- Added CC to  Hans de Goede

Changes since v2:
- Added missing periods, moved variable declaration
- Altered "}, {" style of quirk table
- Confirmed to compile for 32-bit without warnings

Changes since v1:
- Applied suggestions by Andy Shevchenko

Aleksandrs Vinarskis (2):
  mfd: intel-lpss: Switch to generalized quirk table
  mfd: intel-lpss: Introduce QUIRK_CLOCK_DIVIDER_UNITY for XPS 9530

 drivers/mfd/intel-lpss-pci.c | 28 ++++++++++++++++++++--------
 drivers/mfd/intel-lpss.c     |  9 ++++++++-
 drivers/mfd/intel-lpss.h     | 14 +++++++++++++-
 3 files changed, 41 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH v4 1/2] mfd: intel-lpss: Switch to generalized quirk table

[Aleksandrs Vinarskis]

Introduce generic quirk table, and port existing walkaround for select
Microsoft devices to it. This is a preparation for
QUIRK_CLOCK_DIVIDER_UNITY.

Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
---
 drivers/mfd/intel-lpss-pci.c | 23 +++++++++++++++--------
 drivers/mfd/intel-lpss.c     |  2 +-
 drivers/mfd/intel-lpss.h     |  9 ++++++++-
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index 4621d3950b8f..07713a2f694f 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -23,12 +23,17 @@
 
 #include "intel-lpss.h"
 
-/* Some DSDTs have an unused GEXP ACPI device conflicting with I2C4 resources */
-static const struct pci_device_id ignore_resource_conflicts_ids[] = {
-	/* Microsoft Surface Go (version 1) I2C4 */
-	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1182), },
-	/* Microsoft Surface Go 2 I2C4 */
-	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1237), },
+static const struct pci_device_id quirk_ids[] = {
+	{
+		/* Microsoft Surface Go (version 1) I2C4 */
+		PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1182),
+		.driver_data = QUIRK_IGNORE_RESOURCE_CONFLICTS,
+	},
+	{
+		/* Microsoft Surface Go 2 I2C4 */
+		PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1237),
+		.driver_data = QUIRK_IGNORE_RESOURCE_CONFLICTS,
+	},
 	{ }
 };
 
@@ -36,6 +41,7 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev,
 				const struct pci_device_id *id)
 {
 	const struct intel_lpss_platform_info *data = (void *)id->driver_data;
+	const struct pci_device_id *quirk_pci_info;
 	struct intel_lpss_platform_info *info;
 	int ret;
 
@@ -55,8 +61,9 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev,
 	info->mem = pci_resource_n(pdev, 0);
 	info->irq = pci_irq_vector(pdev, 0);
 
-	if (pci_match_id(ignore_resource_conflicts_ids, pdev))
-		info->ignore_resource_conflicts = true;
+	quirk_pci_info = pci_match_id(quirk_ids, pdev);
+	if (quirk_pci_info)
+		info->quirks = quirk_pci_info->driver_data;
 
 	pdev->d3cold_delay = 0;
 
diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index eff423f7dd28..aafa0da5f8db 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -412,7 +412,7 @@ int intel_lpss_probe(struct device *dev,
 		return ret;
 
 	lpss->cell->swnode = info->swnode;
-	lpss->cell->ignore_resource_conflicts = info->ignore_resource_conflicts;
+	lpss->cell->ignore_resource_conflicts = info->quirks & QUIRK_IGNORE_RESOURCE_CONFLICTS;
 
 	intel_lpss_init_dev(lpss);
 
diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
index c1d72b117ed5..2fa9ef916258 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -11,16 +11,23 @@
 #ifndef __MFD_INTEL_LPSS_H
 #define __MFD_INTEL_LPSS_H
 
+#include <linux/bits.h>
 #include <linux/pm.h>
 
+/*
+ * Some DSDTs have an unused GEXP ACPI device conflicting with I2C4 resources.
+ * Set to ignore resource conflicts with ACPI declared SystemMemory regions.
+ */
+#define QUIRK_IGNORE_RESOURCE_CONFLICTS BIT(0)
+
 struct device;
 struct resource;
 struct software_node;
 
 struct intel_lpss_platform_info {
 	struct resource *mem;
-	bool ignore_resource_conflicts;
 	int irq;
+	unsigned int quirks;
 	unsigned long clk_rate;
 	const char *clk_con_id;
 	const struct software_node *swnode;
-- 
2.40.1

Re: [PATCH v4 1/2] mfd: intel-lpss: Switch to generalized quirk table

[Lee Jones]

On Thu, 21 Dec 2023, Aleksandrs Vinarskis wrote:

> Introduce generic quirk table, and port existing walkaround for select
> Microsoft devices to it. This is a preparation for
> QUIRK_CLOCK_DIVIDER_UNITY.
> 
> Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> ---
>  drivers/mfd/intel-lpss-pci.c | 23 +++++++++++++++--------
>  drivers/mfd/intel-lpss.c     |  2 +-
>  drivers/mfd/intel-lpss.h     |  9 ++++++++-
>  3 files changed, 24 insertions(+), 10 deletions(-)

An RB from Andy would make for an easy apply.

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 1/2] mfd: intel-lpss: Switch to generalized quirk table

[Lee Jones]

On Thu, 11 Jan 2024, Lee Jones wrote:

> On Thu, 21 Dec 2023, Aleksandrs Vinarskis wrote:
> 
> > Introduce generic quirk table, and port existing walkaround for select
> > Microsoft devices to it. This is a preparation for
> > QUIRK_CLOCK_DIVIDER_UNITY.
> > 
> > Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
> > ---
> >  drivers/mfd/intel-lpss-pci.c | 23 +++++++++++++++--------
> >  drivers/mfd/intel-lpss.c     |  2 +-
> >  drivers/mfd/intel-lpss.h     |  9 ++++++++-
> >  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> An RB from Andy would make for an easy apply.

Nevermind, I found it!  Old eyes!!

-- 
Lee Jones [李琼斯]

[PATCH v4 2/2] mfd: intel-lpss: Introduce QUIRK_CLOCK_DIVIDER_UNITY for XPS 9530

[Aleksandrs Vinarskis]

Some devices (eg. Dell XPS 9530, 2023) due to a firmware bug have a
misconfigured clock divider, which should've been 1:1. This introduces
quirk which conditionally re-configures the clock divider to 1:1.

Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@gmail.com>
---
 drivers/mfd/intel-lpss-pci.c | 5 +++++
 drivers/mfd/intel-lpss.c     | 7 +++++++
 drivers/mfd/intel-lpss.h     | 5 +++++
 3 files changed, 17 insertions(+)

diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index 07713a2f694f..8c00e0c695c5 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -34,6 +34,11 @@ static const struct pci_device_id quirk_ids[] = {
 		PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1237),
 		.driver_data = QUIRK_IGNORE_RESOURCE_CONFLICTS,
 	},
+	{
+		/* Dell XPS 9530 (2023) */
+		PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x51fb, 0x1028, 0x0beb),
+		.driver_data = QUIRK_CLOCK_DIVIDER_UNITY,
+	},
 	{ }
 };
 
diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index aafa0da5f8db..2a9018112dfc 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -300,6 +300,7 @@ static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
 {
 	char name[32];
 	struct clk *tmp = *clk;
+	int ret;
 
 	snprintf(name, sizeof(name), "%s-enable", devname);
 	tmp = clk_register_gate(NULL, name, __clk_get_name(tmp), 0,
@@ -316,6 +317,12 @@ static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
 		return PTR_ERR(tmp);
 	*clk = tmp;
 
+	if (lpss->info->quirks & QUIRK_CLOCK_DIVIDER_UNITY) {
+		ret = clk_set_rate(tmp, lpss->info->clk_rate);
+		if (ret)
+			return ret;
+	}
+
 	snprintf(name, sizeof(name), "%s-update", devname);
 	tmp = clk_register_gate(NULL, name, __clk_get_name(tmp),
 				CLK_SET_RATE_PARENT, lpss->priv, 31, 0, NULL);
diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
index 2fa9ef916258..6f8f668f4c6f 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -19,6 +19,11 @@
  * Set to ignore resource conflicts with ACPI declared SystemMemory regions.
  */
 #define QUIRK_IGNORE_RESOURCE_CONFLICTS BIT(0)
+/*
+ * Some devices have misconfigured clock divider due to a firmware bug.
+ * Set this to force the clock divider to 1:1 ratio.
+ */
+#define QUIRK_CLOCK_DIVIDER_UNITY		BIT(1)
 
 struct device;
 struct resource;
-- 
2.40.1

Re: [PATCH v4 0/2] Fix LPSS clock divider for XPS 9530

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 07:51:40PM +0100, Aleksandrs Vinarskis wrote:
> Dell XPS 9530 (2023) uses spi-pxa2xx with clock-divider enabled from
> intel-lpss with the ratio of 1:32767 (Dell's firmware bug). This caps SPI
> controller's speed at very low value of 3051Hz, which makes the interface
> practically unusable. Since either spi-pxa2xx or intel-lpss should have
> clock divider enabled, not both, and SPI controller can have higher speed
> than requested by the device, it is preffered to disable intel-lpss
> clock-divider, and let SPI controller handle it.
> 
> Fixing this issue directly in Dell firmware by setting lpss divider to 1:1
> would've been the ideal solution, but is unlikely to ever happen.
> 
> Particular driver already implements customized solution for handling buggy
> ACPI tables for select Microsoft devices. This patch series converts it to
> a more generic quirk table, and adds a new quirk QUIRK_CLOCK_DIVIDER_UNITY
> which forces clock divider to be set to 1:1. In the future, devices with
> similar bug (if any) can be easily added to the same pci id lookup table.

LGTM now,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/2] Fix LPSS clock divider for XPS 9530

[Lee Jones]

On Thu, 21 Dec 2023 19:51:40 +0100, Aleksandrs Vinarskis wrote:
> Dell XPS 9530 (2023) uses spi-pxa2xx with clock-divider enabled from
> intel-lpss with the ratio of 1:32767 (Dell's firmware bug). This caps SPI
> controller's speed at very low value of 3051Hz, which makes the interface
> practically unusable. Since either spi-pxa2xx or intel-lpss should have
> clock divider enabled, not both, and SPI controller can have higher speed
> than requested by the device, it is preffered to disable intel-lpss
> clock-divider, and let SPI controller handle it.
> 
> [...]

Applied, thanks!

[1/2] mfd: intel-lpss: Switch to generalized quirk table
      commit: d43b5230c38ed6001f996eb9262b457713b31ef8
[2/2] mfd: intel-lpss: Introduce QUIRK_CLOCK_DIVIDER_UNITY for XPS 9530
      commit: 106362167f49a0e83f4e6c26f9653f3061575229

--
Lee Jones [李琼斯]

Re: [PATCH v4 0/2] Fix LPSS clock divider for XPS 9530

[Lee Jones]

Please don't drop the list.  Other people may benefit from your questions.

I'm redacting some of the text which may cause conflict and adding back
the original recipient's list.

On Fri, 26 Jan 2024, alex vinarskis wrote:

> > On Thu, 21 Dec 2023 19:51:40 +0100, Aleksandrs Vinarskis wrote:
> > > Dell XPS 9530 (2023) uses spi-pxa2xx with clock-divider enabled from
> > > intel-lpss with the ratio of 1:32767 (Dell's firmware bug). This caps SPI
> > > controller's speed at very low value of 3051Hz, which makes the interface
> > > practically unusable. Since either spi-pxa2xx or intel-lpss should have
> > > clock divider enabled, not both, and SPI controller can have higher speed
> > > than requested by the device, it is preffered to disable intel-lpss
> > > clock-divider, and let SPI controller handle it.
> > >
> > > [...]
> >
> > Applied, thanks!
> 
> Hi,
> 
> I've got a quick question regarding the merging procedure, apologies
> if this was trivial,
> I am still rather new to this:
> 
> I noticed that this series was staged for `for-mfd-next-next`, while
> only `for-mfd-next`
> was merged to 6.8-rc1. Does that imply that following series is not
> planned to be included
> in 6.8, but rather in 6.9?

Yes, that's correct.  The `for-mfd-next-next` branch comes into play
when `for-mfd-next` gets locked down for inclusion and gets merged into
`for-mfd-next` once the merge window has closed.

> If yes, could I ask why? It was under the impression that it was
> submitted in time (at the late stage of 6.7 testing) to make it to 6.8

It was not.  Most maintainers stop merging new patches in the mid to
late -rcs for inclusion into the next merge-window.  I like to have
all new additions soak in Linux Next (-next) for at least 2 to 3 weeks
before sending to Linus.

> (sadly, not in time for LTS > 6.7).

I think you're confusing 2 different things.  I assume you mean the
-rcs.  LTS (Long Term Stable) is for fixes.  Commits end up there
*after* they land in Mainline so long as they conform to the rules set
out in:

  Documentation/process/stable-kernel-rules.rst

> Reason I am asking is
> because this is the last series required to fix audio on XPS 9530
> laptops: [1] was merged in
> 6.7, [2] was merged in 6.8-rc1 and backported to 6.7.1-rc1. Thus me
> and other XPS users
> are extremely eager to see this land in the next kernel release, as
> there was quite some
> community effort to get this done.

Adding support for new devices isn't usually a good enough reason to
submit to the -rcs.  The -rcs are used to fix issues that broke during
the merge-window of the same release and for urgent bug fixes.

This set will be applied to v6.8-rc1.  From there you can apply to have
it backported (or better still, backport and submit it yourself) to
Stable (a.k.a. LTS).  Since it's the LTS' that you will usually find
running on your distro of choice, your audio will likely be fixed with
an upcoming `apt upgrade` or equivalent.

> Once again, apologies if this question is obvious,
> Thanks,
> Alex
> 
> [1] https://lore.kernel.org/all/20231203233006.100558-1-alex.vinarskis@gmail.com/
> [2] https://lore.kernel.org/all/20231221132518.3213-1-sbinding@opensource.cirrus.com/
> 
> >
> > [1/2] mfd: intel-lpss: Switch to generalized quirk table
> >       commit: d43b5230c38ed6001f996eb9262b457713b31ef8
> > [2/2] mfd: intel-lpss: Introduce QUIRK_CLOCK_DIVIDER_UNITY for XPS 9530
> >       commit: 106362167f49a0e83f4e6c26f9653f3061575229
> >
> > --
> > Lee Jones [李琼斯]
> >

-- 
Lee Jones [李琼斯]