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.
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 | 36 ++++++++++++++++++++++++++++--------
drivers/mfd/intel-lpss.c | 9 ++++++++-
drivers/mfd/intel-lpss.h | 5 ++++-
3 files changed, 40 insertions(+), 10 deletions(-)
--
2.40.1
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 <[email protected]>
---
drivers/mfd/intel-lpss-pci.c | 8 ++++++++
drivers/mfd/intel-lpss.c | 7 +++++++
drivers/mfd/intel-lpss.h | 1 +
3 files changed, 16 insertions(+)
diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index cf8006e094f7..2e3b91418415 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -28,6 +28,11 @@ static const struct intel_lpss_platform_info quirk_ignore_resource_conflicts = {
.quirks = QUIRK_IGNORE_RESOURCE_CONFLICTS,
};
+/* Some devices have misconfigured clock divider, which should've been 1:1 */
+static const struct intel_lpss_platform_info quirk_skip_clock_divider = {
+ .quirks = QUIRK_CLOCK_DIVIDER_UNITY,
+};
+
static const struct pci_device_id quirk_ids[] = {
{ /* Microsoft Surface Go (version 1) I2C4 */
PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1182),
@@ -35,6 +40,9 @@ static const struct pci_device_id quirk_ids[] = {
}, { /* Microsoft Surface Go 2 I2C4 */
PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1237),
.driver_data = (kernel_ulong_t)&quirk_ignore_resource_conflicts,
+ }, { /* Dell XPS 9530 (2023) */
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x51fb, 0x1028, 0x0beb),
+ .driver_data = (kernel_ulong_t)&quirk_skip_clock_divider,
},
{}
};
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 3e9d96c372a8..f27834b74cb6 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -14,6 +14,7 @@
#include <linux/pm.h>
#define QUIRK_IGNORE_RESOURCE_CONFLICTS BIT(0)
+#define QUIRK_CLOCK_DIVIDER_UNITY BIT(1)
struct device;
struct resource;
--
2.40.1
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 <[email protected]>
---
drivers/mfd/intel-lpss-pci.c | 28 ++++++++++++++++++++--------
drivers/mfd/intel-lpss.c | 2 +-
drivers/mfd/intel-lpss.h | 4 +++-
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..cf8006e094f7 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -24,12 +24,19 @@
#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 intel_lpss_platform_info quirk_ignore_resource_conflicts = {
+ .quirks = QUIRK_IGNORE_RESOURCE_CONFLICTS,
+};
+
+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 = (kernel_ulong_t)&quirk_ignore_resource_conflicts,
+ }, { /* Microsoft Surface Go 2 I2C4 */
+ PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x9d64, 0x152d, 0x1237),
+ .driver_data = (kernel_ulong_t)&quirk_ignore_resource_conflicts,
+ },
+ {}
};
static int intel_lpss_pci_probe(struct pci_dev *pdev,
@@ -37,6 +44,8 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev,
{
const struct intel_lpss_platform_info *data = (void *)id->driver_data;
struct intel_lpss_platform_info *info;
+ const struct intel_lpss_platform_info *quirk_lpss_info;
+ const struct pci_device_id *quirk_pci_info;
int ret;
ret = pcim_enable_device(pdev);
@@ -55,8 +64,11 @@ 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) {
+ quirk_lpss_info = (void *)quirk_pci_info->driver_data;
+ info->quirks = quirk_lpss_info->quirks;
+ }
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..3e9d96c372a8 100644
--- a/drivers/mfd/intel-lpss.h
+++ b/drivers/mfd/intel-lpss.h
@@ -13,14 +13,16 @@
#include <linux/pm.h>
+#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
On Wed, Dec 20, 2023 at 08:31:47AM +0100, 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.
...
> + quirk_pci_info = pci_match_id(quirk_ids, pdev);
> + if (quirk_pci_info) {
> + quirk_lpss_info = (void *)quirk_pci_info->driver_data;
> + info->quirks = quirk_lpss_info->quirks;
> + }
Just use driver_data as the quirks. No need to have an intermediate data
structure. This will become as simple as
quirk_pci_info = pci_match_id(quirk_ids, pdev);
if (quirk_pci_info)
info->quirks = quirk_pci_info->driver_data;
No ugly castings is a good side effect of the change.
...
>
+ bits.h as you used BIT() below.
> #include <linux/pm.h>
>
> +#define QUIRK_IGNORE_RESOURCE_CONFLICTS BIT(0)
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 20, 2023 at 08:31:48AM +0100, Aleksandrs Vinarskis wrote:
> 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.
(Btw, do you use --histogram when preparing patches? Use it in v2.)
...
> + }, { /* Dell XPS 9530 (2023) */
> + PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x51fb, 0x1028, 0x0beb),
> + .driver_data = (kernel_ulong_t)&quirk_skip_clock_divider,
> },
Should be
}, { /* Dell XPS 9530 (2023) */
PCI_DEVICE_SUB(PCI_VENDOR_ID_INTEL, 0x51fb, 0x1028, 0x0beb),
.driver_data = QUIRK_CLOCK_DIVIDER_UNITY,
},
...
> #define QUIRK_IGNORE_RESOURCE_CONFLICTS BIT(0)
> +#define QUIRK_CLOCK_DIVIDER_UNITY BIT(1)
Each quirk should be documented, see, for example,
https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L593
--
With Best Regards,
Andy Shevchenko
Thanks to Andy Shevchenko for the review and suggestions. Fixed
accordingly.
---
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.
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 | 25 ++++++++++++++++---------
drivers/mfd/intel-lpss.c | 9 ++++++++-
drivers/mfd/intel-lpss.h | 14 +++++++++++++-
3 files changed, 37 insertions(+), 11 deletions(-)
--
2.40.1
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 <[email protected]>
---
V1 -> V2: Documented the quirk
drivers/mfd/intel-lpss-pci.c | 3 +++
drivers/mfd/intel-lpss.c | 7 +++++++
drivers/mfd/intel-lpss.h | 5 +++++
3 files changed, 15 insertions(+)
diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index f8454d802677..951e28ca5e42 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -30,6 +30,9 @@ static const struct pci_device_id quirk_ids[] = {
}, { /* Microsoft Surface Go 2 I2C4 */
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 8a1ffd4f3546..8c032177611e 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
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 <[email protected]>
---
V1 -> V2: Removed unnecessary intermidiate data structure
drivers/mfd/intel-lpss-pci.c | 22 +++++++++++++---------
drivers/mfd/intel-lpss.c | 2 +-
drivers/mfd/intel-lpss.h | 9 ++++++++-
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c
index 4621d3950b8f..f8454d802677 100644
--- a/drivers/mfd/intel-lpss-pci.c
+++ b/drivers/mfd/intel-lpss-pci.c
@@ -23,13 +23,15 @@
#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,
+ },
+ {}
};
static int intel_lpss_pci_probe(struct pci_dev *pdev,
@@ -37,6 +39,7 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev,
{
const struct intel_lpss_platform_info *data = (void *)id->driver_data;
struct intel_lpss_platform_info *info;
+ const struct pci_device_id *quirk_pci_info;
int ret;
ret = pcim_enable_device(pdev);
@@ -55,8 +58,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..8a1ffd4f3546 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
On Wed, Dec 20, 2023 at 09:56:20PM +0100, 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.
Thank you for the update!
Some nit-picks below, after addressing them feel free to add
Reviewed-by: Andy Shevchenko <[email protected]>
...
> - { }
> + {}
No need to change here.
Also I'm not sure about "}, {" style. Lee as the maintainer can clarify
on this.
...
> const struct intel_lpss_platform_info *data = (void *)id->driver_data;
> struct intel_lpss_platform_info *info;
> + const struct pci_device_id *quirk_pci_info;
> int ret;
I would preserve reversed xmas tree order:
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;
...
> + quirk_pci_info = pci_match_id(quirk_ids, pdev);
> + if (quirk_pci_info)
> + info->quirks = quirk_pci_info->driver_data;
Can you confirm that there is no compiler warning if you compile for 32-bit?
...
> +/*
> + * Some DSDTs have an unused GEXP ACPI device conflicting with I2C4 resources
> + * Set to ignore resource conflicts with ACPI declared SystemMemory regions
Please, mind the periods at the ends of sentences.
> + */
...
> + unsigned int quirks;
Depending on the above (32-bit compilation) this might be converted to
unsigned long.
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 20, 2023 at 09:56:21PM +0100, Aleksandrs Vinarskis wrote:
> 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.
Reviewed-by: Andy Shevchenko <[email protected]>
...
> + * Some devices have misconfigured clock divider due to a firmware bug
> + * Set this to force the clock divider to 1:1 ratio
Same comment: Use periods.
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 21, 2023 at 02:41:28PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 20, 2023 at 09:56:20PM +0100, Aleksandrs Vinarskis wrote:
...
> Also I'm not sure about "}, {" style. Lee as the maintainer can clarify
> on this.
JFYI:
$ git grep '^[[:space:]]\+}, {$' -- drivers/mfd/ | wc
250 750 8008
$ git grep '^[[:space:]]\+},$' -- drivers/mfd/ | wc
1898 3796 55230
--
With Best Regards,
Andy Shevchenko