diff mbox series

[3/3] AMD_SFH: Add DMI quirk table for BIOS-es which don't set the activestatus bits

Message ID 20210128121219.6381-4-hdegoede@redhat.com (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series AMD_SFH: Allow overriding sensor-mask by module-param or DMI-quirk | expand

Commit Message

Hans de Goede Jan. 28, 2021, 12:12 p.m. UTC
Some BIOS-es do not initialize the activestatus bits of the AMD_P2C_MSG3
register. This cause the AMD_SFH driver to not register any sensors even
though the laptops in question do have sensors.

Add a DMI quirk-table for specifying sensor-mask overrides based on
DMI match, to make the sensors work OOTB on these laptop models.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Shah, Nehal-bakulchandra Feb. 15, 2021, 8:24 a.m. UTC | #1
Hi Hans,
On 1/28/2021 5:42 PM, Hans de Goede wrote:
> Some BIOS-es do not initialize the activestatus bits of the AMD_P2C_MSG3
> register. This cause the AMD_SFH driver to not register any sensors even
> though the laptops in question do have sensors.
>
> Add a DMI quirk-table for specifying sensor-mask overrides based on
> DMI match, to make the sensors work OOTB on these laptop models.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
> Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index ab0a9443e252..ddecc84fd6f0 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> @@ -10,6 +10,7 @@
>  #include <linux/bitops.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dmi.h>
>  #include <linux/interrupt.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/module.h>
> @@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
>  	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
>  }
>  
> +static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 13-ag0xxx"),
> +		},
> +		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 15-cp0xxx"),
> +		},
> +		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
> +	},
> +	{ }
> +};
> +
>  int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
>  {
>  	int activestatus, num_of_sensors = 0;
> +	const struct dmi_system_id *dmi_id;
>  	u32 activecontrolstatus;
>  
> +	if (sensor_mask_override == -1) {
> +		dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
> +		if (dmi_id)
> +			sensor_mask_override = (long)dmi_id->driver_data;
> +	}
> +
>  	if (sensor_mask_override >= 0) {
>  		activestatus = sensor_mask_override;
>  	} else {
Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior based) series or ryzen 3000 (Raven based) series? As of now current upstream code does not have support for Ryzen 4000 series
for which work is in progress. However, for Ryzen 3000 based series this patch looks fine and thanks for the contribution.


Regards

Nehal
Richard Neumann Feb. 15, 2021, 9:02 a.m. UTC | #2
My 13" model is a Raven-Ridge device:

$ grep -i model /proc/cpuinfo | head -2
model		: 17
model name	: AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx

Since the accelerometer is also reported working on the 15" device, I'd
assume that the corresponding DMI record is also fine.

However, I don't know whether there are newer models on the market with
the same product names, which is why in my patch I used the board's
serial number for DMI matching instead.
This, however, seems unlikely since the series "13-ag0xxx" is included
in the product name, so we should be fine here.

Am Montag, dem 15.02.2021 um 13:54 +0530 schrieb Shah, Nehal-
bakulchandra:
> Hi Hans,
> On 1/28/2021 5:42 PM, Hans de Goede wrote:
> > Some BIOS-es do not initialize the activestatus bits of the
> > AMD_P2C_MSG3
> > register. This cause the AMD_SFH driver to not register any sensors
> > even
> > though the laptops in question do have sensors.
> > 
> > Add a DMI quirk-table for specifying sensor-mask overrides based on
> > DMI match, to make the sensors work OOTB on these laptop models.
> > 
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
> > Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor
> > fusion hub")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> > b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> > index ab0a9443e252..ddecc84fd6f0 100644
> > --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> > +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/bitops.h>
> >  #include <linux/delay.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/dmi.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> >  #include <linux/module.h>
> > @@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev
> > *privdata)
> >         writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> >  }
> >  
> > +static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
> > +       {
> > +               .matches = {
> > +                       DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360
> > Convertible 13-ag0xxx"),
> > +               },
> > +               .driver_data = (void *)(ACEL_EN | MAGNO_EN),
> > +       },
> > +       {
> > +               .matches = {
> > +                       DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360
> > Convertible 15-cp0xxx"),
> > +               },
> > +               .driver_data = (void *)(ACEL_EN | MAGNO_EN),
> > +       },
> > +       { }
> > +};
> > +
> >  int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8
> > *sensor_id)
> >  {
> >         int activestatus, num_of_sensors = 0;
> > +       const struct dmi_system_id *dmi_id;
> >         u32 activecontrolstatus;
> >  
> > +       if (sensor_mask_override == -1) {
> > +               dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
> > +               if (dmi_id)
> > +                       sensor_mask_override = (long)dmi_id-
> > >driver_data;
> > +       }
> > +
> >         if (sensor_mask_override >= 0) {
> >                 activestatus = sensor_mask_override;
> >         } else {
> Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior
> based) series or ryzen 3000 (Raven based) series? As of now current
> upstream code does not have support for Ryzen 4000 series
> for which work is in progress. However, for Ryzen 3000 based series
> this patch looks fine and thanks for the contribution.
> 
> 
> Regards
> 
> Nehal
> 
>
Hans de Goede Feb. 22, 2021, 11:53 a.m. UTC | #3
Hi,

On 2/15/21 9:24 AM, Shah, Nehal-bakulchandra wrote:
> Hi Hans,
> On 1/28/2021 5:42 PM, Hans de Goede wrote:
>> Some BIOS-es do not initialize the activestatus bits of the AMD_P2C_MSG3
>> register. This cause the AMD_SFH driver to not register any sensors even
>> though the laptops in question do have sensors.
>>
>> Add a DMI quirk-table for specifying sensor-mask overrides based on
>> DMI match, to make the sensors work OOTB on these laptop models.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199715
>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1651886
>> Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>> index ab0a9443e252..ddecc84fd6f0 100644
>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/bitops.h>
>>  #include <linux/delay.h>
>>  #include <linux/dma-mapping.h>
>> +#include <linux/dmi.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io-64-nonatomic-lo-hi.h>
>>  #include <linux/module.h>
>> @@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
>>  	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
>>  }
>>  
>> +static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 13-ag0xxx"),
>> +		},
>> +		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 15-cp0xxx"),
>> +		},
>> +		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
>> +	},
>> +	{ }
>> +};
>> +
>>  int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
>>  {
>>  	int activestatus, num_of_sensors = 0;
>> +	const struct dmi_system_id *dmi_id;
>>  	u32 activecontrolstatus;
>>  
>> +	if (sensor_mask_override == -1) {
>> +		dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
>> +		if (dmi_id)
>> +			sensor_mask_override = (long)dmi_id->driver_data;
>> +	}
>> +
>>  	if (sensor_mask_override >= 0) {
>>  		activestatus = sensor_mask_override;
>>  	} else {
> Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior based) series or ryzen 3000 (Raven based) series? As of now current upstream code does not have support for Ryzen 4000 series
> for which work is in progress. However, for Ryzen 3000 based series this patch looks fine and thanks for the contribution.

Both models added to the dmi_sensor_mask_overrides table here use Raven-Ridge SoCs,
they ship with the following CPU options:

Ryzen 3 2300U
Ryzen 5 2500U
Ryzen 7 2700U

So I think we should be good to go wrt merging this set.

Since this set is basically a bugfix set it would be nice if we can get
this merged into one of the 5.12-rc#s.

Regards,

Hans
Hans de Goede Feb. 24, 2021, 7:30 p.m. UTC | #4
Hi,

On 2/23/21 7:24 AM, Singh, Sandeep wrote:
> Hi Hans,
> 
> On 2/22/2021 5:23 PM, Hans de Goede wrote:
>> [CAUTION: External Email]
>>
>> Hi,
>>
>> On 2/15/21 9:24 AM, Shah, Nehal-bakulchandra wrote:
>>> Hi Hans,
>>> On 1/28/2021 5:42 PM, Hans de Goede wrote:
>>>> Some BIOS-es do not initialize the activestatus bits of the AMD_P2C_MSG3
>>>> register. This cause the AMD_SFH driver to not register any sensors even
>>>> though the laptops in question do have sensors.
>>>>
>>>> Add a DMI quirk-table for specifying sensor-mask overrides based on
>>>> DMI match, to make the sensors work OOTB on these laptop models.
>>>>
>>>> BugLink: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D199715&amp;data=04%7C01%7Csandeep.singh%40amd.com%7Cdb4b6cfd7bad4eaa118008d8d7287aca%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637495916161889961%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=SJvTzigbw9lshqAdo37bSHeAiXh6%2Bs90lP187Pwx%2B%2BU%3D&amp;reserved=0
>>>> BugLink: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1651886&amp;data=04%7C01%7Csandeep.singh%40amd.com%7Cdb4b6cfd7bad4eaa118008d8d7287aca%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637495916161894939%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=iy3BWrCWZsInBhnmeGTgLE3MnP9YuPQspWy8Kwuretw%3D&amp;reserved=0
>>>> Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 24 ++++++++++++++++++++++++
>>>>  1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>>>> index ab0a9443e252..ddecc84fd6f0 100644
>>>> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>>>> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
>>>> @@ -10,6 +10,7 @@
>>>>  #include <linux/bitops.h>
>>>>  #include <linux/delay.h>
>>>>  #include <linux/dma-mapping.h>
>>>> +#include <linux/dmi.h>
>>>>  #include <linux/interrupt.h>
>>>>  #include <linux/io-64-nonatomic-lo-hi.h>
>>>>  #include <linux/module.h>
>>>> @@ -77,11 +78,34 @@ void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
>>>>      writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
>>>>  }
>>>>
>>>> +static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
>>>> +    {
>>>> +            .matches = {
>>>> +                    DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 13-ag0xxx"),
>>>> +            },
>>>> +            .driver_data = (void *)(ACEL_EN | MAGNO_EN),
>>>> +    },
>>>> +    {
>>>> +            .matches = {
>>>> +                    DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 15-cp0xxx"),
>>>> +            },
>>>> +            .driver_data = (void *)(ACEL_EN | MAGNO_EN),
>>>> +    },
>>>> +    { }
>>>> +};
>>>> +
>>>>  int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
>>>>  {
>>>>      int activestatus, num_of_sensors = 0;
>>>> +    const struct dmi_system_id *dmi_id;
>>>>      u32 activecontrolstatus;
>>>>
>>>> +    if (sensor_mask_override == -1) {
>>>> +            dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
>>>> +            if (dmi_id)
>>>> +                    sensor_mask_override = (long)dmi_id->driver_data;
>>>> +    }
>>>> +
>>>>      if (sensor_mask_override >= 0) {
>>>>              activestatus = sensor_mask_override;
>>>>      } else {
>>> Can you please confirm that HP Envy x360  is whether ryzen 4000 (Renior based) series or ryzen 3000 (Raven based) series? As of now current upstream code does not have support for Ryzen 4000 series
>>> for which work is in progress. However, for Ryzen 3000 based series this patch looks fine and thanks for the contribution.
>> Both models added to the dmi_sensor_mask_overrides table here use Raven-Ridge SoCs,
>> they ship with the following CPU options:
>>
>> Ryzen 3 2300U
>> Ryzen 5 2500U
>> Ryzen 7 2700U
>>
>> So I think we should be good to go wrt merging this set.
>>
>> Since this set is basically a bugfix set it would be nice if we can get
>> this merged into one of the 5.12-rc#s.
> 
> Acked-by: Sandeep Singh <sandeep.singh@amd.com>

Is that for just this patch, or the entire series?

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index ab0a9443e252..ddecc84fd6f0 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -10,6 +10,7 @@ 
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmi.h>
 #include <linux/interrupt.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/module.h>
@@ -77,11 +78,34 @@  void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
 	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
 }
 
+static const struct dmi_system_id dmi_sensor_mask_overrides[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 13-ag0xxx"),
+		},
+		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP ENVY x360 Convertible 15-cp0xxx"),
+		},
+		.driver_data = (void *)(ACEL_EN | MAGNO_EN),
+	},
+	{ }
+};
+
 int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
 {
 	int activestatus, num_of_sensors = 0;
+	const struct dmi_system_id *dmi_id;
 	u32 activecontrolstatus;
 
+	if (sensor_mask_override == -1) {
+		dmi_id = dmi_first_match(dmi_sensor_mask_overrides);
+		if (dmi_id)
+			sensor_mask_override = (long)dmi_id->driver_data;
+	}
+
 	if (sensor_mask_override >= 0) {
 		activestatus = sensor_mask_override;
 	} else {