linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
@ 2017-12-04 12:32 Adrian Hunter
  2017-12-04 13:48 ` Hans de Goede
  2017-12-04 13:56 ` Carlo Caione
  0 siblings, 2 replies; 11+ messages in thread
From: Adrian Hunter @ 2017-12-04 12:32 UTC (permalink / raw)
  To: Rafael J. Wysocki, Mika Westerberg
  Cc: Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione, Hans de Goede

Some Cherry Trail boards have a dependency between the SDHCI host
controller used for SD cards and an external PMIC accessed via I2C. Add a
device link between the SDHCI host controller (consumer) and the I2C
adapter (supplier).

This patch depends on a fix to devices links, namely commit 0ff26c662d5f
("driver core: Fix device link deferred probe"). And also either,
commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
runtime PM".

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/acpi/acpi_lpss.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 7f2b02cc8ea1..9d82f2f6c327 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -290,6 +290,11 @@ static void bsw_pwm_setup(struct lpss_private_data *pdata)
 	{}
 };
 
+static const struct x86_cpu_id cht_cpu[] = {
+	ICPU(INTEL_FAM6_ATOM_AIRMONT),	/* Braswell, Cherry Trail */
+	{}
+};
+
 #else
 
 #define LPSS_ADDR(desc) (0UL)
@@ -427,6 +432,139 @@ static int register_device_clock(struct acpi_device *adev,
 	return 0;
 }
 
+struct lpss_device_links {
+	const char *supplier_hid;
+	const char *supplier_uid;
+	const char *consumer_hid;
+	const char *consumer_uid;
+	const struct x86_cpu_id *cpus;
+	u32 flags;
+};
+
+static const struct lpss_device_links lpss_device_links[] = {
+	{"808622C1", "7", "80860F14", "3", cht_cpu, DL_FLAG_PM_RUNTIME},
+};
+
+static bool hid_uid_match(const char *hid1, const char *uid1,
+			  const char *hid2, const char *uid2)
+{
+	return !strcmp(hid1, hid2) && uid1 && uid2 && !strcmp(uid1, uid2);
+}
+
+static bool acpi_lpss_is_supplier(struct acpi_device *adev,
+				  const struct lpss_device_links *link)
+{
+	return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
+			     link->supplier_hid, link->supplier_uid);
+}
+
+static bool acpi_lpss_is_consumer(struct acpi_device *adev,
+				  const struct lpss_device_links *link)
+{
+	return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
+			     link->consumer_hid, link->consumer_uid);
+}
+
+struct hid_uid {
+	const char *hid;
+	const char *uid;
+};
+
+static int match_hid_uid(struct device *dev, void *data)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct hid_uid *id = data;
+
+	if (!adev)
+		return 0;
+
+	return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
+			     id->hid, id->uid);
+}
+
+static struct device *acpi_lpss_find_device(const char *hid, const char *uid)
+{
+	struct hid_uid data = {
+		.hid = hid,
+		.uid = uid,
+	};
+
+	return bus_find_device(&platform_bus_type, NULL, &data, match_hid_uid);
+}
+
+static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
+{
+	struct acpi_handle_list dep_devices;
+	acpi_status status;
+	int i;
+
+	if (!acpi_has_method(adev->handle, "_DEP"))
+		return false;
+
+	status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
+					 &dep_devices);
+	if (ACPI_FAILURE(status)) {
+		dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
+		return false;
+	}
+
+	for (i = 0; i < dep_devices.count; i++) {
+		if (dep_devices.handles[i] == handle)
+			return true;
+	}
+
+	return false;
+}
+
+static void acpi_lpss_link_consumer(struct device *dev1,
+				    const struct lpss_device_links *link)
+{
+	struct device *dev2;
+
+	dev2 = acpi_lpss_find_device(link->consumer_hid, link->consumer_uid);
+	if (!dev2)
+		return;
+
+	if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
+		device_link_add(dev2, dev1, link->flags);
+
+	put_device(dev2);
+}
+
+static void acpi_lpss_link_supplier(struct device *dev1,
+				    const struct lpss_device_links *link)
+{
+	struct device *dev2;
+
+	dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid);
+	if (!dev2)
+		return;
+
+	if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
+		device_link_add(dev1, dev2, link->flags);
+
+	put_device(dev2);
+}
+
+static void acpi_lpss_create_device_links(struct acpi_device *adev,
+					  struct platform_device *pdev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lpss_device_links); i++) {
+		const struct lpss_device_links *link = &lpss_device_links[i];
+
+		if (link->cpus && !x86_match_cpu(link->cpus))
+			continue;
+
+		if (acpi_lpss_is_supplier(adev, link))
+			acpi_lpss_link_consumer(&pdev->dev, link);
+
+		if (acpi_lpss_is_consumer(adev, link))
+			acpi_lpss_link_supplier(&pdev->dev, link);
+	}
+}
+
 static int acpi_lpss_create_device(struct acpi_device *adev,
 				   const struct acpi_device_id *id)
 {
@@ -500,6 +638,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
 	adev->driver_data = pdata;
 	pdev = acpi_create_platform_device(adev, dev_desc->properties);
 	if (!IS_ERR_OR_NULL(pdev)) {
+		acpi_lpss_create_device_links(adev, pdev);
 		return 1;
 	}
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 12:32 [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C Adrian Hunter
@ 2017-12-04 13:48 ` Hans de Goede
  2017-12-04 14:30   ` Adrian Hunter
  2017-12-04 13:56 ` Carlo Caione
  1 sibling, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2017-12-04 13:48 UTC (permalink / raw)
  To: Adrian Hunter, Rafael J. Wysocki, Mika Westerberg
  Cc: Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione

Hi,

Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
add something like this to the the probe function:

	struct acpi_device = ACPI_COMPANION(device);

	if (acpi_device->dep_unmet)
		return -EPROBE_DEFER;

No idea if this will work, but if it does work, using the deps described
in the ACPI tables seems like a better solution then hardcoding this.

Regards,

Hans



On 04-12-17 13:32, Adrian Hunter wrote:
> Some Cherry Trail boards have a dependency between the SDHCI host
> controller used for SD cards and an external PMIC accessed via I2C. Add a
> device link between the SDHCI host controller (consumer) and the I2C
> adapter (supplier).
> 
> This patch depends on a fix to devices links, namely commit 0ff26c662d5f
> ("driver core: Fix device link deferred probe"). And also either,
> commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
> probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
> runtime PM".
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>   drivers/acpi/acpi_lpss.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 139 insertions(+)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 7f2b02cc8ea1..9d82f2f6c327 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -290,6 +290,11 @@ static void bsw_pwm_setup(struct lpss_private_data *pdata)
>   	{}
>   };
>   
> +static const struct x86_cpu_id cht_cpu[] = {
> +	ICPU(INTEL_FAM6_ATOM_AIRMONT),	/* Braswell, Cherry Trail */
> +	{}
> +};
> +
>   #else
>   
>   #define LPSS_ADDR(desc) (0UL)
> @@ -427,6 +432,139 @@ static int register_device_clock(struct acpi_device *adev,
>   	return 0;
>   }
>   
> +struct lpss_device_links {
> +	const char *supplier_hid;
> +	const char *supplier_uid;
> +	const char *consumer_hid;
> +	const char *consumer_uid;
> +	const struct x86_cpu_id *cpus;
> +	u32 flags;
> +};
> +
> +static const struct lpss_device_links lpss_device_links[] = {
> +	{"808622C1", "7", "80860F14", "3", cht_cpu, DL_FLAG_PM_RUNTIME},
> +};
> +
> +static bool hid_uid_match(const char *hid1, const char *uid1,
> +			  const char *hid2, const char *uid2)
> +{
> +	return !strcmp(hid1, hid2) && uid1 && uid2 && !strcmp(uid1, uid2);
> +}
> +
> +static bool acpi_lpss_is_supplier(struct acpi_device *adev,
> +				  const struct lpss_device_links *link)
> +{
> +	return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
> +			     link->supplier_hid, link->supplier_uid);
> +}
> +
> +static bool acpi_lpss_is_consumer(struct acpi_device *adev,
> +				  const struct lpss_device_links *link)
> +{
> +	return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
> +			     link->consumer_hid, link->consumer_uid);
> +}
> +
> +struct hid_uid {
> +	const char *hid;
> +	const char *uid;
> +};
> +
> +static int match_hid_uid(struct device *dev, void *data)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct hid_uid *id = data;
> +
> +	if (!adev)
> +		return 0;
> +
> +	return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
> +			     id->hid, id->uid);
> +}
> +
> +static struct device *acpi_lpss_find_device(const char *hid, const char *uid)
> +{
> +	struct hid_uid data = {
> +		.hid = hid,
> +		.uid = uid,
> +	};
> +
> +	return bus_find_device(&platform_bus_type, NULL, &data, match_hid_uid);
> +}
> +
> +static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
> +{
> +	struct acpi_handle_list dep_devices;
> +	acpi_status status;
> +	int i;
> +
> +	if (!acpi_has_method(adev->handle, "_DEP"))
> +		return false;
> +
> +	status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
> +					 &dep_devices);
> +	if (ACPI_FAILURE(status)) {
> +		dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
> +		return false;
> +	}
> +
> +	for (i = 0; i < dep_devices.count; i++) {
> +		if (dep_devices.handles[i] == handle)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void acpi_lpss_link_consumer(struct device *dev1,
> +				    const struct lpss_device_links *link)
> +{
> +	struct device *dev2;
> +
> +	dev2 = acpi_lpss_find_device(link->consumer_hid, link->consumer_uid);
> +	if (!dev2)
> +		return;
> +
> +	if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
> +		device_link_add(dev2, dev1, link->flags);
> +
> +	put_device(dev2);
> +}
> +
> +static void acpi_lpss_link_supplier(struct device *dev1,
> +				    const struct lpss_device_links *link)
> +{
> +	struct device *dev2;
> +
> +	dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid);
> +	if (!dev2)
> +		return;
> +
> +	if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
> +		device_link_add(dev1, dev2, link->flags);
> +
> +	put_device(dev2);
> +}
> +
> +static void acpi_lpss_create_device_links(struct acpi_device *adev,
> +					  struct platform_device *pdev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(lpss_device_links); i++) {
> +		const struct lpss_device_links *link = &lpss_device_links[i];
> +
> +		if (link->cpus && !x86_match_cpu(link->cpus))
> +			continue;
> +
> +		if (acpi_lpss_is_supplier(adev, link))
> +			acpi_lpss_link_consumer(&pdev->dev, link);
> +
> +		if (acpi_lpss_is_consumer(adev, link))
> +			acpi_lpss_link_supplier(&pdev->dev, link);
> +	}
> +}
> +
>   static int acpi_lpss_create_device(struct acpi_device *adev,
>   				   const struct acpi_device_id *id)
>   {
> @@ -500,6 +638,7 @@ static int acpi_lpss_create_device(struct acpi_device *adev,
>   	adev->driver_data = pdata;
>   	pdev = acpi_create_platform_device(adev, dev_desc->properties);
>   	if (!IS_ERR_OR_NULL(pdev)) {
> +		acpi_lpss_create_device_links(adev, pdev);
>   		return 1;
>   	}
>   
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 12:32 [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C Adrian Hunter
  2017-12-04 13:48 ` Hans de Goede
@ 2017-12-04 13:56 ` Carlo Caione
  2017-12-04 15:52   ` Adrian Hunter
  1 sibling, 1 reply; 11+ messages in thread
From: Carlo Caione @ 2017-12-04 13:56 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J. Wysocki, Mika Westerberg, Andy Shevchenko, linux-acpi,
	linux-kernel, Hans de Goede, Linux Upstreaming Team

On Mon, Dec 4, 2017 at 12:32 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Some Cherry Trail boards have a dependency between the SDHCI host
> controller used for SD cards and an external PMIC accessed via I2C. Add a
> device link between the SDHCI host controller (consumer) and the I2C
> adapter (supplier).
>
> This patch depends on a fix to devices links, namely commit 0ff26c662d5f
> ("driver core: Fix device link deferred probe"). And also either,
> commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
> probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
> runtime PM".
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Hey Adrian,
thank you for working on this.

I tried this patch on top of linus HEAD (0ff26c662d5f and 126dbc6b49c8
are already applied) but I'm still experiencing some difficulties with
some SD cards on the cherry-trail laptop I'm working with. You can
find the DSDT in [0].

With an ultra high speed SDR104 SDHC card I get:

  mmc2: Tuning timeout, falling back to fixed sampling clock
  mmc2: new ultra high speed SDR104 SDHC card at address 59b4
  mmcblk2: mmc2:59b4 USD00 15.0 GiB
  mmc2: Timeout waiting for hardware interrupt.
  mmc2: sdhci: ============ SDHCI REGISTER DUMP ===========
  mmc2: sdhci: Sys addr:  0x00000008 | Version:  0x00001002
  mmc2: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000008
  mmc2: sdhci: Argument:  0x00000000 | Trn mode: 0x0000003b
  mmc2: sdhci: Present:   0x01ff0001 | Host ctl: 0x00000017
  mmc2: sdhci: Power:     0x0000000f | Blk gap:  0x00000080
  mmc2: sdhci: Wake-up:   0x00000000 | Clock:    0x00000007
  mmc2: sdhci: Timeout:   0x0000000a | Int stat: 0x00000000
  mmc2: sdhci: Int enab:  0x02ff008b | Sig enab: 0x02ff008b
  mmc2: sdhci: AC12 err:  0x00000000 | Slot int: 0x00000000
  mmc2: sdhci: Caps:      0x0568c8b2 | Caps_1:   0x00000807
  mmc2: sdhci: Cmd:       0x0000123a | Max curr: 0x00000000
  mmc2: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x0077dd7f
  mmc2: sdhci: Resp[2]:   0x325b5900 | Resp[3]:  0x00000000
  mmc2: sdhci: Host ctl2: 0x0000800b
  mmc2: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0x33773200
  mmc2: sdhci: ============================================
  mmcblk2: error -110 sending status command, retrying
  mmcblk2: error -110 sending status command, retrying
  mmcblk2: error -110 sending status command, aborting
  mmc2: Tuning timeout, falling back to fixed sampling clock

For an high speed SDHC card I simply get:

  mmc2: error -110 whilst initialising SD card

Some other cards just work fine, i.e. ultra high speed DDR50.

At this point I'm not sure if the problem I'm seeing is actually
related to the issue you are fixing with this commit (and if now,
sorry to have hijacked this thread).
Any idea on that?

Thank you,

[0] https://gist.github.com/carlocaione/82bff95ababb67dd33f52a86e94ce3ff

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 13:48 ` Hans de Goede
@ 2017-12-04 14:30   ` Adrian Hunter
  2017-12-04 14:33     ` Hans de Goede
  0 siblings, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2017-12-04 14:30 UTC (permalink / raw)
  To: Hans de Goede, Rafael J. Wysocki, Mika Westerberg
  Cc: Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione

On 04/12/17 15:48, Hans de Goede wrote:
> Hi,
> 
> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.

It is using _DEP, see acpi_lpss_dep()

> add something like this to the the probe function:
> 
>     struct acpi_device = ACPI_COMPANION(device);
> 
>     if (acpi_device->dep_unmet)
>         return -EPROBE_DEFER;
> 
> No idea if this will work, but if it does work, using the deps described
> in the ACPI tables seems like a better solution then hardcoding this.

That would not work because there are other devices listed in the _DEP
method so dep_unmet is always true.  So we are left checking _DEP but only
for specific device dependencies.

> 
> Regards,
> 
> Hans
> 
> 
> 
> On 04-12-17 13:32, Adrian Hunter wrote:
>> Some Cherry Trail boards have a dependency between the SDHCI host
>> controller used for SD cards and an external PMIC accessed via I2C. Add a
>> device link between the SDHCI host controller (consumer) and the I2C
>> adapter (supplier).
>>
>> This patch depends on a fix to devices links, namely commit 0ff26c662d5f
>> ("driver core: Fix device link deferred probe"). And also either,
>> commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
>> probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
>> runtime PM".
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>   drivers/acpi/acpi_lpss.c | 139
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 139 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>> index 7f2b02cc8ea1..9d82f2f6c327 100644
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>> @@ -290,6 +290,11 @@ static void bsw_pwm_setup(struct lpss_private_data
>> *pdata)
>>       {}
>>   };
>>   +static const struct x86_cpu_id cht_cpu[] = {
>> +    ICPU(INTEL_FAM6_ATOM_AIRMONT),    /* Braswell, Cherry Trail */
>> +    {}
>> +};
>> +
>>   #else
>>     #define LPSS_ADDR(desc) (0UL)
>> @@ -427,6 +432,139 @@ static int register_device_clock(struct acpi_device
>> *adev,
>>       return 0;
>>   }
>>   +struct lpss_device_links {
>> +    const char *supplier_hid;
>> +    const char *supplier_uid;
>> +    const char *consumer_hid;
>> +    const char *consumer_uid;
>> +    const struct x86_cpu_id *cpus;
>> +    u32 flags;
>> +};
>> +
>> +static const struct lpss_device_links lpss_device_links[] = {
>> +    {"808622C1", "7", "80860F14", "3", cht_cpu, DL_FLAG_PM_RUNTIME},
>> +};
>> +
>> +static bool hid_uid_match(const char *hid1, const char *uid1,
>> +              const char *hid2, const char *uid2)
>> +{
>> +    return !strcmp(hid1, hid2) && uid1 && uid2 && !strcmp(uid1, uid2);
>> +}
>> +
>> +static bool acpi_lpss_is_supplier(struct acpi_device *adev,
>> +                  const struct lpss_device_links *link)
>> +{
>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>> +                 link->supplier_hid, link->supplier_uid);
>> +}
>> +
>> +static bool acpi_lpss_is_consumer(struct acpi_device *adev,
>> +                  const struct lpss_device_links *link)
>> +{
>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>> +                 link->consumer_hid, link->consumer_uid);
>> +}
>> +
>> +struct hid_uid {
>> +    const char *hid;
>> +    const char *uid;
>> +};
>> +
>> +static int match_hid_uid(struct device *dev, void *data)
>> +{
>> +    struct acpi_device *adev = ACPI_COMPANION(dev);
>> +    struct hid_uid *id = data;
>> +
>> +    if (!adev)
>> +        return 0;
>> +
>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>> +                 id->hid, id->uid);
>> +}
>> +
>> +static struct device *acpi_lpss_find_device(const char *hid, const char
>> *uid)
>> +{
>> +    struct hid_uid data = {
>> +        .hid = hid,
>> +        .uid = uid,
>> +    };
>> +
>> +    return bus_find_device(&platform_bus_type, NULL, &data, match_hid_uid);
>> +}
>> +
>> +static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
>> +{
>> +    struct acpi_handle_list dep_devices;
>> +    acpi_status status;
>> +    int i;
>> +
>> +    if (!acpi_has_method(adev->handle, "_DEP"))
>> +        return false;
>> +
>> +    status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
>> +                     &dep_devices);
>> +    if (ACPI_FAILURE(status)) {
>> +        dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
>> +        return false;
>> +    }
>> +
>> +    for (i = 0; i < dep_devices.count; i++) {
>> +        if (dep_devices.handles[i] == handle)
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static void acpi_lpss_link_consumer(struct device *dev1,
>> +                    const struct lpss_device_links *link)
>> +{
>> +    struct device *dev2;
>> +
>> +    dev2 = acpi_lpss_find_device(link->consumer_hid, link->consumer_uid);
>> +    if (!dev2)
>> +        return;
>> +
>> +    if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
>> +        device_link_add(dev2, dev1, link->flags);
>> +
>> +    put_device(dev2);
>> +}
>> +
>> +static void acpi_lpss_link_supplier(struct device *dev1,
>> +                    const struct lpss_device_links *link)
>> +{
>> +    struct device *dev2;
>> +
>> +    dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid);
>> +    if (!dev2)
>> +        return;
>> +
>> +    if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
>> +        device_link_add(dev1, dev2, link->flags);
>> +
>> +    put_device(dev2);
>> +}
>> +
>> +static void acpi_lpss_create_device_links(struct acpi_device *adev,
>> +                      struct platform_device *pdev)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(lpss_device_links); i++) {
>> +        const struct lpss_device_links *link = &lpss_device_links[i];
>> +
>> +        if (link->cpus && !x86_match_cpu(link->cpus))
>> +            continue;
>> +
>> +        if (acpi_lpss_is_supplier(adev, link))
>> +            acpi_lpss_link_consumer(&pdev->dev, link);
>> +
>> +        if (acpi_lpss_is_consumer(adev, link))
>> +            acpi_lpss_link_supplier(&pdev->dev, link);
>> +    }
>> +}
>> +
>>   static int acpi_lpss_create_device(struct acpi_device *adev,
>>                      const struct acpi_device_id *id)
>>   {
>> @@ -500,6 +638,7 @@ static int acpi_lpss_create_device(struct acpi_device
>> *adev,
>>       adev->driver_data = pdata;
>>       pdev = acpi_create_platform_device(adev, dev_desc->properties);
>>       if (!IS_ERR_OR_NULL(pdev)) {
>> +        acpi_lpss_create_device_links(adev, pdev);
>>           return 1;
>>       }
>>  
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 14:30   ` Adrian Hunter
@ 2017-12-04 14:33     ` Hans de Goede
  2017-12-04 14:41       ` Adrian Hunter
  2017-12-04 14:56       ` Rafael J. Wysocki
  0 siblings, 2 replies; 11+ messages in thread
From: Hans de Goede @ 2017-12-04 14:33 UTC (permalink / raw)
  To: Adrian Hunter, Rafael J. Wysocki, Mika Westerberg
  Cc: Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione

Hi,

On 04-12-17 15:30, Adrian Hunter wrote:
> On 04/12/17 15:48, Hans de Goede wrote:
>> Hi,
>>
>> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
> 
> It is using _DEP, see acpi_lpss_dep()
> 
>> add something like this to the the probe function:
>>
>>      struct acpi_device = ACPI_COMPANION(device);
>>
>>      if (acpi_device->dep_unmet)
>>          return -EPROBE_DEFER;
>>
>> No idea if this will work, but if it does work, using the deps described
>> in the ACPI tables seems like a better solution then hardcoding this.
> 
> That would not work because there are other devices listed in the _DEP
> method so dep_unmet is always true.  So we are left checking _DEP but only
> for specific device dependencies.

Ugh, understood thank you for explaining this. Perhaps it is a good idea
to mention in the commit message why acpi_dev->dep_unmet cannot be used
here?

Regards,

Hans


>> On 04-12-17 13:32, Adrian Hunter wrote:
>>> Some Cherry Trail boards have a dependency between the SDHCI host
>>> controller used for SD cards and an external PMIC accessed via I2C. Add a
>>> device link between the SDHCI host controller (consumer) and the I2C
>>> adapter (supplier).
>>>
>>> This patch depends on a fix to devices links, namely commit 0ff26c662d5f
>>> ("driver core: Fix device link deferred probe"). And also either,
>>> commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
>>> probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
>>> runtime PM".
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>    drivers/acpi/acpi_lpss.c | 139
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 139 insertions(+)
>>>
>>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>>> index 7f2b02cc8ea1..9d82f2f6c327 100644
>>> --- a/drivers/acpi/acpi_lpss.c
>>> +++ b/drivers/acpi/acpi_lpss.c
>>> @@ -290,6 +290,11 @@ static void bsw_pwm_setup(struct lpss_private_data
>>> *pdata)
>>>        {}
>>>    };
>>>    +static const struct x86_cpu_id cht_cpu[] = {
>>> +    ICPU(INTEL_FAM6_ATOM_AIRMONT),    /* Braswell, Cherry Trail */
>>> +    {}
>>> +};
>>> +
>>>    #else
>>>      #define LPSS_ADDR(desc) (0UL)
>>> @@ -427,6 +432,139 @@ static int register_device_clock(struct acpi_device
>>> *adev,
>>>        return 0;
>>>    }
>>>    +struct lpss_device_links {
>>> +    const char *supplier_hid;
>>> +    const char *supplier_uid;
>>> +    const char *consumer_hid;
>>> +    const char *consumer_uid;
>>> +    const struct x86_cpu_id *cpus;
>>> +    u32 flags;
>>> +};
>>> +
>>> +static const struct lpss_device_links lpss_device_links[] = {
>>> +    {"808622C1", "7", "80860F14", "3", cht_cpu, DL_FLAG_PM_RUNTIME},
>>> +};
>>> +
>>> +static bool hid_uid_match(const char *hid1, const char *uid1,
>>> +              const char *hid2, const char *uid2)
>>> +{
>>> +    return !strcmp(hid1, hid2) && uid1 && uid2 && !strcmp(uid1, uid2);
>>> +}
>>> +
>>> +static bool acpi_lpss_is_supplier(struct acpi_device *adev,
>>> +                  const struct lpss_device_links *link)
>>> +{
>>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>> +                 link->supplier_hid, link->supplier_uid);
>>> +}
>>> +
>>> +static bool acpi_lpss_is_consumer(struct acpi_device *adev,
>>> +                  const struct lpss_device_links *link)
>>> +{
>>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>> +                 link->consumer_hid, link->consumer_uid);
>>> +}
>>> +
>>> +struct hid_uid {
>>> +    const char *hid;
>>> +    const char *uid;
>>> +};
>>> +
>>> +static int match_hid_uid(struct device *dev, void *data)
>>> +{
>>> +    struct acpi_device *adev = ACPI_COMPANION(dev);
>>> +    struct hid_uid *id = data;
>>> +
>>> +    if (!adev)
>>> +        return 0;
>>> +
>>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>> +                 id->hid, id->uid);
>>> +}
>>> +
>>> +static struct device *acpi_lpss_find_device(const char *hid, const char
>>> *uid)
>>> +{
>>> +    struct hid_uid data = {
>>> +        .hid = hid,
>>> +        .uid = uid,
>>> +    };
>>> +
>>> +    return bus_find_device(&platform_bus_type, NULL, &data, match_hid_uid);
>>> +}
>>> +
>>> +static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
>>> +{
>>> +    struct acpi_handle_list dep_devices;
>>> +    acpi_status status;
>>> +    int i;
>>> +
>>> +    if (!acpi_has_method(adev->handle, "_DEP"))
>>> +        return false;
>>> +
>>> +    status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
>>> +                     &dep_devices);
>>> +    if (ACPI_FAILURE(status)) {
>>> +        dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
>>> +        return false;
>>> +    }
>>> +
>>> +    for (i = 0; i < dep_devices.count; i++) {
>>> +        if (dep_devices.handles[i] == handle)
>>> +            return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +static void acpi_lpss_link_consumer(struct device *dev1,
>>> +                    const struct lpss_device_links *link)
>>> +{
>>> +    struct device *dev2;
>>> +
>>> +    dev2 = acpi_lpss_find_device(link->consumer_hid, link->consumer_uid);
>>> +    if (!dev2)
>>> +        return;
>>> +
>>> +    if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
>>> +        device_link_add(dev2, dev1, link->flags);
>>> +
>>> +    put_device(dev2);
>>> +}
>>> +
>>> +static void acpi_lpss_link_supplier(struct device *dev1,
>>> +                    const struct lpss_device_links *link)
>>> +{
>>> +    struct device *dev2;
>>> +
>>> +    dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid);
>>> +    if (!dev2)
>>> +        return;
>>> +
>>> +    if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
>>> +        device_link_add(dev1, dev2, link->flags);
>>> +
>>> +    put_device(dev2);
>>> +}
>>> +
>>> +static void acpi_lpss_create_device_links(struct acpi_device *adev,
>>> +                      struct platform_device *pdev)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(lpss_device_links); i++) {
>>> +        const struct lpss_device_links *link = &lpss_device_links[i];
>>> +
>>> +        if (link->cpus && !x86_match_cpu(link->cpus))
>>> +            continue;
>>> +
>>> +        if (acpi_lpss_is_supplier(adev, link))
>>> +            acpi_lpss_link_consumer(&pdev->dev, link);
>>> +
>>> +        if (acpi_lpss_is_consumer(adev, link))
>>> +            acpi_lpss_link_supplier(&pdev->dev, link);
>>> +    }
>>> +}
>>> +
>>>    static int acpi_lpss_create_device(struct acpi_device *adev,
>>>                       const struct acpi_device_id *id)
>>>    {
>>> @@ -500,6 +638,7 @@ static int acpi_lpss_create_device(struct acpi_device
>>> *adev,
>>>        adev->driver_data = pdata;
>>>        pdev = acpi_create_platform_device(adev, dev_desc->properties);
>>>        if (!IS_ERR_OR_NULL(pdev)) {
>>> +        acpi_lpss_create_device_links(adev, pdev);
>>>            return 1;
>>>        }
>>>   
>>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 14:33     ` Hans de Goede
@ 2017-12-04 14:41       ` Adrian Hunter
  2017-12-04 15:00         ` Rafael J. Wysocki
  2017-12-04 14:56       ` Rafael J. Wysocki
  1 sibling, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2017-12-04 14:41 UTC (permalink / raw)
  To: Hans de Goede, Rafael J. Wysocki, Mika Westerberg
  Cc: Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione

On 04/12/17 16:33, Hans de Goede wrote:
> Hi,
> 
> On 04-12-17 15:30, Adrian Hunter wrote:
>> On 04/12/17 15:48, Hans de Goede wrote:
>>> Hi,
>>>
>>> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
>>
>> It is using _DEP, see acpi_lpss_dep()
>>
>>> add something like this to the the probe function:
>>>
>>>      struct acpi_device = ACPI_COMPANION(device);
>>>
>>>      if (acpi_device->dep_unmet)
>>>          return -EPROBE_DEFER;
>>>
>>> No idea if this will work, but if it does work, using the deps described
>>> in the ACPI tables seems like a better solution then hardcoding this.
>>
>> That would not work because there are other devices listed in the _DEP
>> method so dep_unmet is always true.  So we are left checking _DEP but only
>> for specific device dependencies.
> 
> Ugh, understood thank you for explaining this. Perhaps it is a good idea
> to mention in the commit message why acpi_dev->dep_unmet cannot be used
> here?

dep_unmet predates device links, but now we have device links, they are
better anyway.

> 
> Regards,
> 
> Hans
> 
> 
>>> On 04-12-17 13:32, Adrian Hunter wrote:
>>>> Some Cherry Trail boards have a dependency between the SDHCI host
>>>> controller used for SD cards and an external PMIC accessed via I2C. Add a
>>>> device link between the SDHCI host controller (consumer) and the I2C
>>>> adapter (supplier).
>>>>
>>>> This patch depends on a fix to devices links, namely commit 0ff26c662d5f
>>>> ("driver core: Fix device link deferred probe"). And also either,
>>>> commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
>>>> probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
>>>> runtime PM".
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>    drivers/acpi/acpi_lpss.c | 139
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 139 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
>>>> index 7f2b02cc8ea1..9d82f2f6c327 100644
>>>> --- a/drivers/acpi/acpi_lpss.c
>>>> +++ b/drivers/acpi/acpi_lpss.c
>>>> @@ -290,6 +290,11 @@ static void bsw_pwm_setup(struct lpss_private_data
>>>> *pdata)
>>>>        {}
>>>>    };
>>>>    +static const struct x86_cpu_id cht_cpu[] = {
>>>> +    ICPU(INTEL_FAM6_ATOM_AIRMONT),    /* Braswell, Cherry Trail */
>>>> +    {}
>>>> +};
>>>> +
>>>>    #else
>>>>      #define LPSS_ADDR(desc) (0UL)
>>>> @@ -427,6 +432,139 @@ static int register_device_clock(struct acpi_device
>>>> *adev,
>>>>        return 0;
>>>>    }
>>>>    +struct lpss_device_links {
>>>> +    const char *supplier_hid;
>>>> +    const char *supplier_uid;
>>>> +    const char *consumer_hid;
>>>> +    const char *consumer_uid;
>>>> +    const struct x86_cpu_id *cpus;
>>>> +    u32 flags;
>>>> +};
>>>> +
>>>> +static const struct lpss_device_links lpss_device_links[] = {
>>>> +    {"808622C1", "7", "80860F14", "3", cht_cpu, DL_FLAG_PM_RUNTIME},
>>>> +};
>>>> +
>>>> +static bool hid_uid_match(const char *hid1, const char *uid1,
>>>> +              const char *hid2, const char *uid2)
>>>> +{
>>>> +    return !strcmp(hid1, hid2) && uid1 && uid2 && !strcmp(uid1, uid2);
>>>> +}
>>>> +
>>>> +static bool acpi_lpss_is_supplier(struct acpi_device *adev,
>>>> +                  const struct lpss_device_links *link)
>>>> +{
>>>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>>> +                 link->supplier_hid, link->supplier_uid);
>>>> +}
>>>> +
>>>> +static bool acpi_lpss_is_consumer(struct acpi_device *adev,
>>>> +                  const struct lpss_device_links *link)
>>>> +{
>>>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>>> +                 link->consumer_hid, link->consumer_uid);
>>>> +}
>>>> +
>>>> +struct hid_uid {
>>>> +    const char *hid;
>>>> +    const char *uid;
>>>> +};
>>>> +
>>>> +static int match_hid_uid(struct device *dev, void *data)
>>>> +{
>>>> +    struct acpi_device *adev = ACPI_COMPANION(dev);
>>>> +    struct hid_uid *id = data;
>>>> +
>>>> +    if (!adev)
>>>> +        return 0;
>>>> +
>>>> +    return hid_uid_match(acpi_device_hid(adev), acpi_device_uid(adev),
>>>> +                 id->hid, id->uid);
>>>> +}
>>>> +
>>>> +static struct device *acpi_lpss_find_device(const char *hid, const char
>>>> *uid)
>>>> +{
>>>> +    struct hid_uid data = {
>>>> +        .hid = hid,
>>>> +        .uid = uid,
>>>> +    };
>>>> +
>>>> +    return bus_find_device(&platform_bus_type, NULL, &data,
>>>> match_hid_uid);
>>>> +}
>>>> +
>>>> +static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle)
>>>> +{
>>>> +    struct acpi_handle_list dep_devices;
>>>> +    acpi_status status;
>>>> +    int i;
>>>> +
>>>> +    if (!acpi_has_method(adev->handle, "_DEP"))
>>>> +        return false;
>>>> +
>>>> +    status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
>>>> +                     &dep_devices);
>>>> +    if (ACPI_FAILURE(status)) {
>>>> +        dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < dep_devices.count; i++) {
>>>> +        if (dep_devices.handles[i] == handle)
>>>> +            return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static void acpi_lpss_link_consumer(struct device *dev1,
>>>> +                    const struct lpss_device_links *link)
>>>> +{
>>>> +    struct device *dev2;
>>>> +
>>>> +    dev2 = acpi_lpss_find_device(link->consumer_hid, link->consumer_uid);
>>>> +    if (!dev2)
>>>> +        return;
>>>> +
>>>> +    if (acpi_lpss_dep(ACPI_COMPANION(dev2), ACPI_HANDLE(dev1)))
>>>> +        device_link_add(dev2, dev1, link->flags);
>>>> +
>>>> +    put_device(dev2);
>>>> +}
>>>> +
>>>> +static void acpi_lpss_link_supplier(struct device *dev1,
>>>> +                    const struct lpss_device_links *link)
>>>> +{
>>>> +    struct device *dev2;
>>>> +
>>>> +    dev2 = acpi_lpss_find_device(link->supplier_hid, link->supplier_uid);
>>>> +    if (!dev2)
>>>> +        return;
>>>> +
>>>> +    if (acpi_lpss_dep(ACPI_COMPANION(dev1), ACPI_HANDLE(dev2)))
>>>> +        device_link_add(dev1, dev2, link->flags);
>>>> +
>>>> +    put_device(dev2);
>>>> +}
>>>> +
>>>> +static void acpi_lpss_create_device_links(struct acpi_device *adev,
>>>> +                      struct platform_device *pdev)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(lpss_device_links); i++) {
>>>> +        const struct lpss_device_links *link = &lpss_device_links[i];
>>>> +
>>>> +        if (link->cpus && !x86_match_cpu(link->cpus))
>>>> +            continue;
>>>> +
>>>> +        if (acpi_lpss_is_supplier(adev, link))
>>>> +            acpi_lpss_link_consumer(&pdev->dev, link);
>>>> +
>>>> +        if (acpi_lpss_is_consumer(adev, link))
>>>> +            acpi_lpss_link_supplier(&pdev->dev, link);
>>>> +    }
>>>> +}
>>>> +
>>>>    static int acpi_lpss_create_device(struct acpi_device *adev,
>>>>                       const struct acpi_device_id *id)
>>>>    {
>>>> @@ -500,6 +638,7 @@ static int acpi_lpss_create_device(struct acpi_device
>>>> *adev,
>>>>        adev->driver_data = pdata;
>>>>        pdev = acpi_create_platform_device(adev, dev_desc->properties);
>>>>        if (!IS_ERR_OR_NULL(pdev)) {
>>>> +        acpi_lpss_create_device_links(adev, pdev);
>>>>            return 1;
>>>>        }
>>>>   
>>>
>>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 14:33     ` Hans de Goede
  2017-12-04 14:41       ` Adrian Hunter
@ 2017-12-04 14:56       ` Rafael J. Wysocki
  1 sibling, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-12-04 14:56 UTC (permalink / raw)
  To: Hans de Goede, Adrian Hunter
  Cc: Rafael J. Wysocki, Mika Westerberg, Andy Shevchenko, linux-acpi,
	linux-kernel, Carlo Caione

On Monday, December 4, 2017 3:33:29 PM CET Hans de Goede wrote:
> Hi,
> 
> On 04-12-17 15:30, Adrian Hunter wrote:
> > On 04/12/17 15:48, Hans de Goede wrote:
> >> Hi,
> >>
> >> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
> > 
> > It is using _DEP, see acpi_lpss_dep()
> > 
> >> add something like this to the the probe function:
> >>
> >>      struct acpi_device = ACPI_COMPANION(device);
> >>
> >>      if (acpi_device->dep_unmet)
> >>          return -EPROBE_DEFER;
> >>
> >> No idea if this will work, but if it does work, using the deps described
> >> in the ACPI tables seems like a better solution then hardcoding this.
> > 
> > That would not work because there are other devices listed in the _DEP
> > method so dep_unmet is always true.  So we are left checking _DEP but only
> > for specific device dependencies.
> 
> Ugh, understood thank you for explaining this. Perhaps it is a good idea
> to mention in the commit message why acpi_dev->dep_unmet cannot be used
> here?

Not just in the commit message, but I'd suggest adding a comment to that effect
next to the definition of lpss_device_links[].

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 14:41       ` Adrian Hunter
@ 2017-12-04 15:00         ` Rafael J. Wysocki
  2017-12-05 14:25           ` Adrian Hunter
  0 siblings, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-12-04 15:00 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Hans de Goede, Rafael J. Wysocki, Mika Westerberg,
	Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione

On Monday, December 4, 2017 3:41:45 PM CET Adrian Hunter wrote:
> On 04/12/17 16:33, Hans de Goede wrote:
> > Hi,
> > 
> > On 04-12-17 15:30, Adrian Hunter wrote:
> >> On 04/12/17 15:48, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
> >>
> >> It is using _DEP, see acpi_lpss_dep()
> >>
> >>> add something like this to the the probe function:
> >>>
> >>>      struct acpi_device = ACPI_COMPANION(device);
> >>>
> >>>      if (acpi_device->dep_unmet)
> >>>          return -EPROBE_DEFER;
> >>>
> >>> No idea if this will work, but if it does work, using the deps described
> >>> in the ACPI tables seems like a better solution then hardcoding this.
> >>
> >> That would not work because there are other devices listed in the _DEP
> >> method so dep_unmet is always true.  So we are left checking _DEP but only
> >> for specific device dependencies.
> > 
> > Ugh, understood thank you for explaining this. Perhaps it is a good idea
> > to mention in the commit message why acpi_dev->dep_unmet cannot be used
> > here?
> 
> dep_unmet predates device links, but now we have device links, they are
> better anyway.

Right (they cover PM too, for example), but it would be good to note why
it is necessary to hardcode the links information in the code.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 13:56 ` Carlo Caione
@ 2017-12-04 15:52   ` Adrian Hunter
  0 siblings, 0 replies; 11+ messages in thread
From: Adrian Hunter @ 2017-12-04 15:52 UTC (permalink / raw)
  To: Carlo Caione
  Cc: Rafael J. Wysocki, Mika Westerberg, Andy Shevchenko, linux-acpi,
	linux-kernel, Hans de Goede, Linux Upstreaming Team

On 04/12/17 15:56, Carlo Caione wrote:
> On Mon, Dec 4, 2017 at 12:32 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Some Cherry Trail boards have a dependency between the SDHCI host
>> controller used for SD cards and an external PMIC accessed via I2C. Add a
>> device link between the SDHCI host controller (consumer) and the I2C
>> adapter (supplier).
>>
>> This patch depends on a fix to devices links, namely commit 0ff26c662d5f
>> ("driver core: Fix device link deferred probe"). And also either,
>> commit 126dbc6b49c8 ("PM: i2c-designware-platdrv: Clean up PM handling in
>> probe"), or patch "PM / runtime: Fix handling of suppliers with disabled
>> runtime PM".
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Hey Adrian,
> thank you for working on this.
> 
> I tried this patch on top of linus HEAD (0ff26c662d5f and 126dbc6b49c8
> are already applied) but I'm still experiencing some difficulties with
> some SD cards on the cherry-trail laptop I'm working with. You can
> find the DSDT in [0].
> 
> With an ultra high speed SDR104 SDHC card I get:

SDR104 works for me but I now see it is not supported on all boards.  I will
send a patch for that i.e. you will end up with no SDR104.

> 
>   mmc2: Tuning timeout, falling back to fixed sampling clock
>   mmc2: new ultra high speed SDR104 SDHC card at address 59b4
>   mmcblk2: mmc2:59b4 USD00 15.0 GiB
>   mmc2: Timeout waiting for hardware interrupt.
>   mmc2: sdhci: ============ SDHCI REGISTER DUMP ===========
>   mmc2: sdhci: Sys addr:  0x00000008 | Version:  0x00001002
>   mmc2: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000008
>   mmc2: sdhci: Argument:  0x00000000 | Trn mode: 0x0000003b
>   mmc2: sdhci: Present:   0x01ff0001 | Host ctl: 0x00000017
>   mmc2: sdhci: Power:     0x0000000f | Blk gap:  0x00000080
>   mmc2: sdhci: Wake-up:   0x00000000 | Clock:    0x00000007
>   mmc2: sdhci: Timeout:   0x0000000a | Int stat: 0x00000000
>   mmc2: sdhci: Int enab:  0x02ff008b | Sig enab: 0x02ff008b
>   mmc2: sdhci: AC12 err:  0x00000000 | Slot int: 0x00000000
>   mmc2: sdhci: Caps:      0x0568c8b2 | Caps_1:   0x00000807
>   mmc2: sdhci: Cmd:       0x0000123a | Max curr: 0x00000000
>   mmc2: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x0077dd7f
>   mmc2: sdhci: Resp[2]:   0x325b5900 | Resp[3]:  0x00000000
>   mmc2: sdhci: Host ctl2: 0x0000800b
>   mmc2: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0x33773200
>   mmc2: sdhci: ============================================
>   mmcblk2: error -110 sending status command, retrying
>   mmcblk2: error -110 sending status command, retrying
>   mmcblk2: error -110 sending status command, aborting
>   mmc2: Tuning timeout, falling back to fixed sampling clock
> 
> For an high speed SDHC card I simply get:
> 
>   mmc2: error -110 whilst initialising SD card

I will investigate that some more.

> 
> Some other cards just work fine, i.e. ultra high speed DDR50.

This patch should help in the DDR50 case when booting with a card already
inserted.  Did it help?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-04 15:00         ` Rafael J. Wysocki
@ 2017-12-05 14:25           ` Adrian Hunter
  2017-12-05 15:05             ` Rafael J. Wysocki
  0 siblings, 1 reply; 11+ messages in thread
From: Adrian Hunter @ 2017-12-05 14:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Hans de Goede, Rafael J. Wysocki, Mika Westerberg,
	Andy Shevchenko, linux-acpi, linux-kernel, Carlo Caione

On 04/12/17 17:00, Rafael J. Wysocki wrote:
> On Monday, December 4, 2017 3:41:45 PM CET Adrian Hunter wrote:
>> On 04/12/17 16:33, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 04-12-17 15:30, Adrian Hunter wrote:
>>>> On 04/12/17 15:48, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
>>>>
>>>> It is using _DEP, see acpi_lpss_dep()
>>>>
>>>>> add something like this to the the probe function:
>>>>>
>>>>>      struct acpi_device = ACPI_COMPANION(device);
>>>>>
>>>>>      if (acpi_device->dep_unmet)
>>>>>          return -EPROBE_DEFER;
>>>>>
>>>>> No idea if this will work, but if it does work, using the deps described
>>>>> in the ACPI tables seems like a better solution then hardcoding this.
>>>>
>>>> That would not work because there are other devices listed in the _DEP
>>>> method so dep_unmet is always true.  So we are left checking _DEP but only
>>>> for specific device dependencies.
>>>
>>> Ugh, understood thank you for explaining this. Perhaps it is a good idea
>>> to mention in the commit message why acpi_dev->dep_unmet cannot be used
>>> here?
>>
>> dep_unmet predates device links, but now we have device links, they are
>> better anyway.
> 
> Right (they cover PM too, for example), but it would be good to note why
> it is necessary to hardcode the links information in the code.

It isn't entirely necessary to hardcode the links information.  For example,
another possibility is to create device links for all LPSS devices with _DEP
methods that point to other LPSS devices i.e. match against
acpi_lpss_device_ids.  The assumptions would be that all LPSS devices have
drivers so it would be safe to create links between them, and that the
nature of the dependency is correctly represented by a device link.

An advantage of that approach would be that it might work for future
dependencies between LPSS devices without having to add entries to a table.
The disadvantage would be the possibility that creating a device link
somehow turns out not to be the right thing to do.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C
  2017-12-05 14:25           ` Adrian Hunter
@ 2017-12-05 15:05             ` Rafael J. Wysocki
  0 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2017-12-05 15:05 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Rafael J. Wysocki, Hans de Goede, Rafael J. Wysocki,
	Mika Westerberg, Andy Shevchenko, ACPI Devel Maling List,
	Linux Kernel Mailing List, Carlo Caione

On Tue, Dec 5, 2017 at 3:25 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/12/17 17:00, Rafael J. Wysocki wrote:
>> On Monday, December 4, 2017 3:41:45 PM CET Adrian Hunter wrote:
>>> On 04/12/17 16:33, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 04-12-17 15:30, Adrian Hunter wrote:
>>>>> On 04/12/17 15:48, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Wouldn't it be easier to use the ACPI _DEP tracking for this, e.g.
>>>>>
>>>>> It is using _DEP, see acpi_lpss_dep()
>>>>>
>>>>>> add something like this to the the probe function:
>>>>>>
>>>>>>      struct acpi_device = ACPI_COMPANION(device);
>>>>>>
>>>>>>      if (acpi_device->dep_unmet)
>>>>>>          return -EPROBE_DEFER;
>>>>>>
>>>>>> No idea if this will work, but if it does work, using the deps described
>>>>>> in the ACPI tables seems like a better solution then hardcoding this.
>>>>>
>>>>> That would not work because there are other devices listed in the _DEP
>>>>> method so dep_unmet is always true.  So we are left checking _DEP but only
>>>>> for specific device dependencies.
>>>>
>>>> Ugh, understood thank you for explaining this. Perhaps it is a good idea
>>>> to mention in the commit message why acpi_dev->dep_unmet cannot be used
>>>> here?
>>>
>>> dep_unmet predates device links, but now we have device links, they are
>>> better anyway.
>>
>> Right (they cover PM too, for example), but it would be good to note why
>> it is necessary to hardcode the links information in the code.
>
> It isn't entirely necessary to hardcode the links information.  For example,
> another possibility is to create device links for all LPSS devices with _DEP
> methods that point to other LPSS devices i.e. match against
> acpi_lpss_device_ids.  The assumptions would be that all LPSS devices have
> drivers so it would be safe to create links between them, and that the
> nature of the dependency is correctly represented by a device link.
>
> An advantage of that approach would be that it might work for future
> dependencies between LPSS devices without having to add entries to a table.
> The disadvantage would be the possibility that creating a device link
> somehow turns out not to be the right thing to do.

OK

To me, hardcoding is fine for the time being, but I would just add the
above information as a comment to explain the choice made.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-12-05 15:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-04 12:32 [PATCH] ACPI / LPSS: Add device link for CHT SD card dependency on I2C Adrian Hunter
2017-12-04 13:48 ` Hans de Goede
2017-12-04 14:30   ` Adrian Hunter
2017-12-04 14:33     ` Hans de Goede
2017-12-04 14:41       ` Adrian Hunter
2017-12-04 15:00         ` Rafael J. Wysocki
2017-12-05 14:25           ` Adrian Hunter
2017-12-05 15:05             ` Rafael J. Wysocki
2017-12-04 14:56       ` Rafael J. Wysocki
2017-12-04 13:56 ` Carlo Caione
2017-12-04 15:52   ` Adrian Hunter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).