diff mbox series

[1/2] soundwire: slave: introduce DMI quirks for HP Spectre x360 Convertible

Message ID 20210204204837.27876-2-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series soundwire: add DMI quirks to work-around broken _ADR | expand

Commit Message

Pierre-Louis Bossart Feb. 4, 2021, 8:48 p.m. UTC
HP Spectre x360 Convertible devices expose invalid _ADR fields in the
DSDT, which prevents codec drivers from probing. A possible solution
is to override the DSDT, but that's just too painful for users.

This patch suggests a simple DMI-based quirk to remap the existing
invalid ADR information into valid ones.

BugLink: https://github.com/thesofproject/linux/issues/2700
Co-developed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <bard.liao@intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---
 drivers/soundwire/slave.c | 50 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Vinod Koul Feb. 5, 2021, 7:16 a.m. UTC | #1
Hi Pierre,

On 04-02-21, 14:48, Pierre-Louis Bossart wrote:
> HP Spectre x360 Convertible devices expose invalid _ADR fields in the
> DSDT, which prevents codec drivers from probing. A possible solution
> is to override the DSDT, but that's just too painful for users.
> 
> This patch suggests a simple DMI-based quirk to remap the existing
> invalid ADR information into valid ones.

While I agree with the approach, I do not like the implementation. The
quirks in firmware should not reside in core code. Drivers are the right
place, of course we need to add callbacks into driver for this.

So I did a quick hacking and added the below patch, I think you can add
the quirks in Intel driver based on DMI for this.

-- >8 --
Pierre-Louis Bossart Feb. 5, 2021, 3:15 p.m. UTC | #2
Thanks for the review Vinod.

> On 04-02-21, 14:48, Pierre-Louis Bossart wrote:
>> HP Spectre x360 Convertible devices expose invalid _ADR fields in the
>> DSDT, which prevents codec drivers from probing. A possible solution
>> is to override the DSDT, but that's just too painful for users.
>>
>> This patch suggests a simple DMI-based quirk to remap the existing
>> invalid ADR information into valid ones.
> 
> While I agree with the approach, I do not like the implementation. The
> quirks in firmware should not reside in core code. Drivers are the right
> place, of course we need to add callbacks into driver for this.
> 
> So I did a quick hacking and added the below patch, I think you can add
> the quirks in Intel driver based on DMI for this.

I thought about this, but the Intel driver is about the *master* 
configuration. It's not really about slave-related _ADR. If and when the 
IP configuration changes, it'll be problematic to have such quirks in 
the middle.

At the end of the day, the problem is an ACPI one, not an Intel master 
one, and I put the code where it's protected by CONFIG_ACPI.

I don't mind doing the change, but the notion of conflating Intel master 
and list of slave quirks isn't without its own problems.

An alternate solution would be to break the ACPI and OF slave 
initialization into two separate files (slave-acpi.c and slave-of.c), 
that way there is a cleaner split.

Or we put all those quirks in a dedicated slave-dmi-quirks.c and use 
your solution. That may be more manageable since the list of quirks is 
historically likely to grow.

It's really ugly in all cases.

I try to look at the positive side of things: if we have quirks to 
handle it's an indicator that more platforms are moving to SoundWire...

I hope though that it doesn't reach the level of Baytrail where most of 
the machine driver is a dictionary of quirks.

> 
> -- >8 --
> 
>  From 20af8100025637ea5e295877d28f3afb9dbd4814 Mon Sep 17 00:00:00 2001
> From: Vinod Koul <vkoul@kernel.org>
> Date: Fri, 5 Feb 2021 12:38:21 +0530
> Subject: [PATCH] soundwire: add override addr ops
> 
> Some firmware can have buggy _ADR values causing the scanning of devices
> to fail. Add the override_ops which if implemented by master driver
> would be invoked instead of reading _ADR. The drivers can implement
> quirks based on DMI etc to override the addr values.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>   -compile tested only, pls validate
> 
>   drivers/soundwire/slave.c     | 25 +++++++++++++++++++++----
>   include/linux/soundwire/sdw.h |  3 +++
>   2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index a08f4081c1c4..2b0d646c5c2f 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -91,12 +91,11 @@ int sdw_slave_add(struct sdw_bus *bus,
>   
>   #if IS_ENABLED(CONFIG_ACPI)
>   
> -static bool find_slave(struct sdw_bus *bus,
> -		       struct acpi_device *adev,
> -		       struct sdw_slave_id *id)
> +static bool _find_slave(struct sdw_bus *bus,
> +		        struct acpi_device *adev,
> +		        struct sdw_slave_id *id)
>   {
>   	unsigned long long addr;
> -	unsigned int link_id;
>   	acpi_status status;
>   
>   	status = acpi_evaluate_integer(adev->handle,
> @@ -108,6 +107,24 @@ static bool find_slave(struct sdw_bus *bus,
>   		return false;
>   	}
>   
> +	return addr;
> +}
> +
> +static bool find_slave(struct sdw_bus *bus,
> +		       struct acpi_device *adev,
> +		       struct sdw_slave_id *id)
> +{
> +	unsigned long long addr;
> +	unsigned int link_id;
> +
> +	if (bus->ops->override_adr)
> +		addr = bus->ops->override_adr(bus);
> +	else
> +		addr = _find_slave(bus, adev, id);
> +
> +	if (!addr)
> +		return false;
> +
>   	/* Extract link id from ADR, Bit 51 to 48 (included) */
>   	link_id = SDW_DISCO_LINK_ID(addr);
>   
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index f0b01b728640..d0ad3404ca95 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -804,6 +804,8 @@ struct sdw_defer {
>   /**
>    * struct sdw_master_ops - Master driver ops
>    * @read_prop: Read Master properties
> + * @override_adr: Override reading address from firmware and read from
> + * driver instead (quirk for buggy firmware)
>    * @xfer_msg: Transfer message callback
>    * @xfer_msg_defer: Defer version of transfer message callback
>    * @reset_page_addr: Reset the SCP page address registers
> @@ -813,6 +815,7 @@ struct sdw_defer {
>    */
>   struct sdw_master_ops {
>   	int (*read_prop)(struct sdw_bus *bus);
> +	unsigned long long (*override_adr)(struct sdw_bus *bus);
>   
>   	enum sdw_command_response (*xfer_msg)
>   			(struct sdw_bus *bus, struct sdw_msg *msg);
>
Vinod Koul Feb. 6, 2021, 10:55 a.m. UTC | #3
Hi Pierre,

On 05-02-21, 09:15, Pierre-Louis Bossart wrote:
> Thanks for the review Vinod.
> 
> > On 04-02-21, 14:48, Pierre-Louis Bossart wrote:
> > > HP Spectre x360 Convertible devices expose invalid _ADR fields in the
> > > DSDT, which prevents codec drivers from probing. A possible solution
> > > is to override the DSDT, but that's just too painful for users.
> > > 
> > > This patch suggests a simple DMI-based quirk to remap the existing
> > > invalid ADR information into valid ones.
> > 
> > While I agree with the approach, I do not like the implementation. The
> > quirks in firmware should not reside in core code. Drivers are the right
> > place, of course we need to add callbacks into driver for this.
> > 
> > So I did a quick hacking and added the below patch, I think you can add
> > the quirks in Intel driver based on DMI for this.
> 
> I thought about this, but the Intel driver is about the *master*
> configuration. It's not really about slave-related _ADR. If and when the IP
> configuration changes, it'll be problematic to have such quirks in the
> middle.

Okay, but ADR is not really a slave configuration either. I feel it is system
wide description..

> At the end of the day, the problem is an ACPI one, not an Intel master one,
> and I put the code where it's protected by CONFIG_ACPI.

Right, to be more specific it is a buggy firmware implementation and not
following of the specs by device vendors.

> I don't mind doing the change, but the notion of conflating Intel master and
> list of slave quirks isn't without its own problems.

Same is true for soundwire core (slave/slave-apci or whatever). The
issue I have is that this will sure become big, so I would like this to
be moved away from all soundwire core files. The right place IMO for
this is respective drivers, intel/codec/machine please do take your
pick. My attempt here was to move this to Intel driver here as I felt
that was the right place for platform issues here.. do feel free to move
any other place except core files...

> 
> An alternate solution would be to break the ACPI and OF slave initialization
> into two separate files (slave-acpi.c and slave-of.c), that way there is a
> cleaner split.
> 
> Or we put all those quirks in a dedicated slave-dmi-quirks.c and use your
> solution. That may be more manageable since the list of quirks is
> historically likely to grow.
> 
> It's really ugly in all cases.
> 
> I try to look at the positive side of things: if we have quirks to handle
> it's an indicator that more platforms are moving to SoundWire...
> 
> I hope though that it doesn't reach the level of Baytrail where most of the
> machine driver is a dictionary of quirks.

Hope that it wont, but having seen things, I think it may come to it!
diff mbox series

Patch

diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 180f38bd003b..abdc1ae94e9c 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -2,6 +2,7 @@ 
 // Copyright(c) 2015-17 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/of.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_type.h>
@@ -91,10 +92,44 @@  int sdw_slave_add(struct sdw_bus *bus,
 
 #if IS_ENABLED(CONFIG_ACPI)
 
+struct adr_remap {
+	u64 adr;
+	u64 remapped_adr;
+};
+
+/*
+ * HP Spectre 360 Convertible devices do not expose the correct _ADR
+ * in the DSDT.
+ * Remap the bad _ADR values to the ones reported by hardware
+ */
+static const struct adr_remap hp_spectre_360[] = {
+	{
+		0x000010025D070100,
+		0x000020025D071100
+	},
+	{
+		0x000110025d070100,
+		0x000120025D130800
+	},
+	{}
+};
+
+static const struct dmi_system_id adr_remap_quirk_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Spectre x360 Convertible"),
+		},
+		.driver_data = (void *)hp_spectre_360,
+	},
+	{}
+};
+
 static bool find_slave(struct sdw_bus *bus,
 		       struct acpi_device *adev,
 		       struct sdw_slave_id *id)
 {
+	const struct dmi_system_id *dmi_id;
 	unsigned long long addr;
 	unsigned int link_id;
 	acpi_status status;
@@ -108,6 +143,21 @@  static bool find_slave(struct sdw_bus *bus,
 		return false;
 	}
 
+	/* check if any address remap quirk applies */
+	dmi_id = dmi_first_match(adr_remap_quirk_table);
+	if (dmi_id) {
+		struct adr_remap *map = dmi_id->driver_data;
+
+		for (map = dmi_id->driver_data; map->adr; map++) {
+			if (map->adr == addr) {
+				dev_dbg(bus->dev, "remapped _ADR 0x%llx as 0x%llx\n",
+					addr, map->remapped_adr);
+				addr = map->remapped_adr;
+				break;
+			}
+		}
+	}
+
 	/* Extract link id from ADR, Bit 51 to 48 (included) */
 	link_id = SDW_DISCO_LINK_ID(addr);