| Message ID | 20210204204837.27876-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: add DMI quirks to work-around broken _ADR | expand |
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 -- 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);
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); >
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!
On 2/6/21 4:55 AM, Vinod Koul wrote: > 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.. It's a partial description only useful for enumeration. the _ADR field is the means by which we probe a slave driver, which happens when we add a device in slave.c. It doesn't carry any other information beyond enumeration. the only part you could view as system-dependent is the link_id and the unique_id, but that's far from a complete description. More specifically, it doesn't tell you if the device is left or right, if it's meant to be used in an 'aggregated' way or if the different devices are independent. In fact the entire existing DisCo specification only describes 'devices' (in the SoundWire sense), not how they are supposed to be connected and used in a system. We will have to come-up with a new spec for this so that we can have a true 'platform'/system description allowing us to use a generic machine driver (similar to Morimoto-san's generic graph) > >> 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. It's an OEM/BIOS vendor/integrator issue. >> 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... thanks, I will combine your suggestion to add an 'override' callback but stick these quirks in a separate files. That will avoid collisions and make the code more manageable.
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);