Change in coreboot[master]: drivers/i2c/generic: Support power resource as parent in device tree

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35075 )

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

drivers/i2c/generic: Support power resource as parent in device tree

This allows supporting an independent power resource for I2C generic
devices, so that the power resource configuration can be refactored
out of individual drivers and coalesced into a separate one.

Change-Id: I7554ab4fef88ffea0d3423ea27bb18a56b7f4de5
Signed-off-by: Tim Wawrzynczak 
---
M src/drivers/i2c/generic/generic.c
1 file changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/35075/1

diff --git a/src/drivers/i2c/generic/generic.c b/src/drivers/i2c/generic/generic.c
index 27ff9ff..a3411b0 100644
--- a/src/drivers/i2c/generic/generic.c
+++ b/src/drivers/i2c/generic/generic.c
@@ -23,6 +23,7 @@
 #include <stdint.h>
 #include <string.h>
 #include "chip.h"
+#include "drivers/generic/power_resource/chip.h"

#if CONFIG(HAVE_ACPI_TABLES)

@@ -59,7 +60,18 @@
    		void (*callback)(struct device *dev),
    		struct drivers_i2c_generic_config *config)
 {
-	const char *scope = acpi_device_scope(dev);
+	const int parent_is_power_resource =
+		CONFIG(DRIVERS_GENERIC_POWER_RESOURCE) &&
+		(dev->bus && dev->bus->children == dev &&
+		dev->bus->dev->chip_ops == &drivers_generic_power_resource_ops);
+	/*
+	 * Because the power resource is a "pseudo-device", we don't want it
+	 * to be included in the ACPI path
+	 */
+	const char *pr_scope = parent_is_power_resource ?
+		acpi_device_scope(dev->bus->dev) :
+		NULL;
+	const char *scope = (pr_scope == NULL) ? acpi_device_scope(dev) : pr_scope;
    struct acpi_i2c i2c = {
    	.address = dev->path.i2c.device,
    	.mode_10bit = dev->path.i2c.mode_10bit,
@@ -163,7 +175,9 @@
    		       acpi_device_path(dev));

acpi_add_device_power_resource("PRIC");
-
+	} else if (parent_is_power_resource) {
+		/* Parent device is a power resource, so set up _PR0/_PR3 */
+		acpi_add_device_power_resource(acpi_device_path(dev->bus->dev));
    }

/* Callback if any. */

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35075 )

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35075/2/src/drivers/i2c/generic/gen... 
File src/drivers/i2c/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/35075/2/src/drivers/i2c/generic/gen... 
PS2, Line 68: 	 * Because the power resource is a "pseudo-device", we don't want it
Why not?
It has it's own chip driver and it is a "device".

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35075 )

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35075/2/src/drivers/i2c/generic/gen... 
File src/drivers/i2c/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/35075/2/src/drivers/i2c/generic/gen... 
PS2, Line 68: 	 * Because the power resource is a "pseudo-device", we don't want it
> Why not? […]

I had issues getting the kernel to find the I2C device when the I2CSerialBus object had a path of _SB.PCI0.PR00.I2C0 for example.  Maybe I did something else wrong, but I could not get the kernel to enumerate it unless it was _SB.PCI0.I2C0

[Tim Wawrzynczak (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Duncan Laurie, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35075

to look at the new patch set (#3).

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

drivers/i2c/generic: Support power resource as parent in device tree

This allows supporting an independent power resource for I2C generic
devices, so that the power resource configuration can be refactored
out of individual drivers and coalesced into a separate one.
This helps enable shared power resources in the future.

Instead of:

device pci 00.0 on
    chip drivers/i2c/generic
    	register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPIO0)"
    	register "reset_enable_delay_ms" = "100"
    	...
    	device i2c 00 on end
    end

chip drivers/i2c/generic
    	register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPIO0)"
    	register "reset_enable_delay_ms" = "2"
    	...
    	device i2c 01 on end
    end
end

You can then write:

device pci 00.0 on
    chip drivers/generic/power_resource
    	register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPIO0)"
    	register "reset_enable_delay_ms" = "100"
    	device generic 0 on
    		chip drivers/i2c/generic
    			register ...
    			device i2c 00 on end
    		end
    		chip drivers/i2c/generic
    			register ...
    			device i2c 01 on end
    		end
    	end
    end
end

Change-Id: I7554ab4fef88ffea0d3423ea27bb18a56b7f4de5
Signed-off-by: Tim Wawrzynczak 
---
M src/drivers/i2c/generic/generic.c
1 file changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/35075/3

[Tim Wawrzynczak (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Julius Werner, Subrata Banik, Duncan Laurie, Paul Menzel, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35075

to look at the new patch set (#6).

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

drivers/i2c/generic: Support power resource as parent in device tree

This allows supporting an independent power resource for I2C generic
devices, so that the power resource configuration can be refactored
out of individual drivers and coalesced into a separate one.
This helps enable shared power resources in the future.

Instead of:

device pci 00.0 on
    chip drivers/i2c/generic
    	register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPIO0)"
    	register "reset_enable_delay_ms" = "100"
    	...
    	device i2c 00 on end
    end

chip drivers/i2c/generic
    	register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPIO0)"
    	register "reset_enable_delay_ms" = "2"
    	...
    	device i2c 01 on end
    end
end

You can then write:

device pci 00.0 on
    chip drivers/generic/power_resource
    	register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPIO0)"
    	register "reset_enable_delay_ms" = "100"
    	device generic 0 on
    		chip drivers/i2c/generic
    			register ...
    			device i2c 00 on end
    		end
    		chip drivers/i2c/generic
    			register ...
    			device i2c 01 on end
    		end
    	end
    end
end

Change-Id: I7554ab4fef88ffea0d3423ea27bb18a56b7f4de5
Signed-off-by: Tim Wawrzynczak 
---
M src/drivers/i2c/generic/generic.c
1 file changed, 20 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/35075/6

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35075 )

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35075/2/src/drivers/i2c/generic/gen... 
File src/drivers/i2c/generic/generic.c:

https://review.coreboot.org/c/coreboot/+/35075/2/src/drivers/i2c/generic/gen... 
PS2, Line 68: 	 * Because the power resource is a "pseudo-device", we don't want it
> I had issues getting the kernel to find the I2C device when the I2CSerialBus object had a path of _ […]

Reorganized this part, the power resource is now scoped under the device it controls power signals for.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35075 )

Change subject: drivers/i2c/generic: Support power resource as parent in device tree
......................................................................

Abandoned

needs more thought