Change in coreboot[master]: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus
Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus
This chip driver adds ACPI identifiers for multiplexed I2C bus that are selected using GPIO. The multiplexed bus device defines the address to select the I2C lines.
BUG=b:169444894 TEST=Build and boot to OS in waddledee. Ensure that the ACPI identifiers are added in appropriate context. Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA0) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, Zero) // _ADR: Address } }
Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA1) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, One) // _ADR: Address } }
Change-Id: If8b983bc8ce212ce05fe6b7f01a6d9092468e582 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/i2c/gpiomux/Makefile.inc A src/drivers/i2c/gpiomux/bus/Makefile.inc A src/drivers/i2c/gpiomux/bus/bus.c A src/drivers/i2c/gpiomux/bus/chip.h 4 files changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/46144/1
diff --git a/src/drivers/i2c/gpiomux/Makefile.inc b/src/drivers/i2c/gpiomux/Makefile.inc index fec8d5b..5c328cb 100644 --- a/src/drivers/i2c/gpiomux/Makefile.inc +++ b/src/drivers/i2c/gpiomux/Makefile.inc @@ -1 +1,2 @@ subdirs-$(CONFIG_DRIVERS_I2C_GPIO_MUX) += mux +subdirs-$(CONFIG_DRIVERS_I2C_GPIO_MUX) += bus diff --git a/src/drivers/i2c/gpiomux/bus/Makefile.inc b/src/drivers/i2c/gpiomux/bus/Makefile.inc new file mode 100644 index 0000000..1c462dc --- /dev/null +++ b/src/drivers/i2c/gpiomux/bus/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_I2C_GPIO_MUX) += bus.c diff --git a/src/drivers/i2c/gpiomux/bus/bus.c b/src/drivers/i2c/gpiomux/bus/bus.c new file mode 100644 index 0000000..96763e9 --- /dev/null +++ b/src/drivers/i2c/gpiomux/bus/bus.c @@ -0,0 +1,69 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> +#include <console/console.h> +#include <device/device.h> +#include <device/path.h> +#include <stdlib.h> +#include <string.h> +#include "chip.h" + +static const char *i2c_gpiomux_bus_acpi_name(const struct device *dev) +{ + struct drivers_i2c_gpiomux_bus_config *config = dev->chip_info; + char *name; + + if (config->name) + return config->name; + + name = malloc(ACPI_NAME_BUFFER_SIZE); + snprintf(name, ACPI_NAME_BUFFER_SIZE, "MXA%01.1X", dev->path.generic.id); + name[ACPI_NAME_BUFFER_SIZE - 1] = '\0'; + config->name = name; + return name; +} + +static void i2c_gpiomux_bus_fill_ssdt_generator(const struct device *dev) +{ + const char *scope = acpi_device_scope(dev); + const char *path = acpi_device_path(dev); + + if (!dev->enabled || !scope) + return; + + /* Device */ + acpigen_write_scope(scope); + acpigen_write_device(acpi_device_name(dev)); + + acpigen_write_STA(acpi_device_status(dev)); + acpigen_write_name_integer("_ADR", dev->path.generic.id); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", path, dev->chip_ops->name, dev_path(dev)); +} + +static struct device_operations i2c_gpiomux_bus_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .scan_bus = scan_static_bus, + .acpi_name = i2c_gpiomux_bus_acpi_name, + .acpi_fill_ssdt = i2c_gpiomux_bus_fill_ssdt_generator, +}; + +static void i2c_gpiomux_bus_enable(struct device *dev) +{ + struct drivers_i2c_gpiomux_bus_config *config = dev->chip_info; + + if (!config) + return; + + dev->ops = &i2c_gpiomux_bus_ops; +} + +struct chip_operations drivers_i2c_gpiomux_bus_ops = { + CHIP_NAME("I2C GPIO MUX Bus Device") + .enable_dev = i2c_gpiomux_bus_enable +}; diff --git a/src/drivers/i2c/gpiomux/bus/chip.h b/src/drivers/i2c/gpiomux/bus/chip.h new file mode 100644 index 0000000..784a55b --- /dev/null +++ b/src/drivers/i2c/gpiomux/bus/chip.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __I2C_GPIOMUX_BUS_CHIP_H__ +#define __I2C_GPIOMUX_BUS_CHIP_H__ + +struct drivers_i2c_gpiomux_bus_config { + char *name; +}; + +#endif /* __I2C_GPIOMUX_BUS_CHIP_H__ */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... File src/drivers/i2c/gpiomux/bus/bus.c:
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... PS1, Line 14: dev->chip_info config_of(dev)?
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... PS1, Line 22: name[ACPI_NAME_BUFFER_SIZE - 1] = '\0'; snprintf() already takes care of doing this.
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... PS1, Line 60: if (!config) : return; : I don't understand the intent of this check.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46144
to look at the new patch set (#2).
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus
This chip driver adds ACPI identifiers for multiplexed I2C bus that are selected using GPIO. The multiplexed bus device defines the address to select the I2C lines. These ACPI identifiers are consumed by the i2c-mux-gpio kernel driver: https://www.kernel.org/doc/html/latest/i2c/muxes/i2c-mux-gpio.html
BUG=b:169444894 TEST=Build and boot to OS in waddledee. Ensure that the ACPI identifiers are added in appropriate context. Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA0) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, Zero) // _ADR: Address } }
Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA1) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, One) // _ADR: Address } }
Change-Id: If8b983bc8ce212ce05fe6b7f01a6d9092468e582 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/i2c/gpiomux/Makefile.inc A src/drivers/i2c/gpiomux/bus/Makefile.inc A src/drivers/i2c/gpiomux/bus/bus.c A src/drivers/i2c/gpiomux/bus/chip.h 4 files changed, 78 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/46144/2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... File src/drivers/i2c/gpiomux/bus/bus.c:
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... PS1, Line 14: dev->chip_info
config_of(dev)?
Done. Here and in the mux driver: https://review.coreboot.org/c/coreboot/+/45911/7/src/drivers/i2c/gpiomux/mux...
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... PS1, Line 22: name[ACPI_NAME_BUFFER_SIZE - 1] = '\0';
snprintf() already takes care of doing this.
Done. Here and in the mux driver: https://review.coreboot.org/c/coreboot/+/45911/7/src/drivers/i2c/gpiomux/mux...
https://review.coreboot.org/c/coreboot/+/46144/1/src/drivers/i2c/gpiomux/bus... PS1, Line 60: if (!config) : return; :
I don't understand the intent of this check.
Done. My bad. The new dev check is to satisfy the coverity.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bus... File src/drivers/i2c/gpiomux/bus/bus.c:
https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bus... PS2, Line 20: malloc Just curious: You used a static buffer in earlier CL, but malloc here. Why the difference in choice?
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46144
to look at the new patch set (#3).
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus
This chip driver adds ACPI identifiers for multiplexed I2C bus that are selected using GPIO. The multiplexed bus device defines the address to select the I2C lines. These ACPI identifiers are consumed by the i2c-mux-gpio kernel driver: https://www.kernel.org/doc/html/latest/i2c/muxes/i2c-mux-gpio.html
BUG=b:169444894 TEST=Build and boot to OS in waddledee. Ensure that the ACPI identifiers are added in appropriate context. Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA0) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, Zero) // _ADR: Address } }
Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA1) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, One) // _ADR: Address } }
Change-Id: If8b983bc8ce212ce05fe6b7f01a6d9092468e582 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/drivers/i2c/gpiomux/Makefile.inc A src/drivers/i2c/gpiomux/bus/Makefile.inc A src/drivers/i2c/gpiomux/bus/bus.c A src/drivers/i2c/gpiomux/bus/chip.h 4 files changed, 71 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/46144/3
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bus... File src/drivers/i2c/gpiomux/bus/bus.c:
https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bus... PS2, Line 20: malloc
Just curious: You used a static buffer in earlier CL, but malloc here. […]
I wasn't sure if the names of one of the mux adapters always overrides the other one because of the static buffer. Hence used buffer from the heap.
I verified that the names are distinct and are not overwritten. Hence updated to the static buffer here.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46144 )
Change subject: drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus ......................................................................
drivers/i2c/gpiomux: Add chip driver for multiplexed I2C bus
This chip driver adds ACPI identifiers for multiplexed I2C bus that are selected using GPIO. The multiplexed bus device defines the address to select the I2C lines. These ACPI identifiers are consumed by the i2c-mux-gpio kernel driver: https://www.kernel.org/doc/html/latest/i2c/muxes/i2c-mux-gpio.html
BUG=b:169444894 TEST=Build and boot to OS in waddledee. Ensure that the ACPI identifiers are added in appropriate context. Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA0) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, Zero) // _ADR: Address } }
Scope (_SB.PCI0.I2C3.MUX0) { Device (MXA1) { Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_ADR, One) // _ADR: Address } }
Change-Id: If8b983bc8ce212ce05fe6b7f01a6d9092468e582 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46144 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/i2c/gpiomux/Makefile.inc A src/drivers/i2c/gpiomux/bus/Makefile.inc A src/drivers/i2c/gpiomux/bus/bus.c A src/drivers/i2c/gpiomux/bus/chip.h 4 files changed, 71 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/i2c/gpiomux/Makefile.inc b/src/drivers/i2c/gpiomux/Makefile.inc index fec8d5b..5c328cb 100644 --- a/src/drivers/i2c/gpiomux/Makefile.inc +++ b/src/drivers/i2c/gpiomux/Makefile.inc @@ -1 +1,2 @@ subdirs-$(CONFIG_DRIVERS_I2C_GPIO_MUX) += mux +subdirs-$(CONFIG_DRIVERS_I2C_GPIO_MUX) += bus diff --git a/src/drivers/i2c/gpiomux/bus/Makefile.inc b/src/drivers/i2c/gpiomux/bus/Makefile.inc new file mode 100644 index 0000000..1c462dc --- /dev/null +++ b/src/drivers/i2c/gpiomux/bus/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_I2C_GPIO_MUX) += bus.c diff --git a/src/drivers/i2c/gpiomux/bus/bus.c b/src/drivers/i2c/gpiomux/bus/bus.c new file mode 100644 index 0000000..66aef8e --- /dev/null +++ b/src/drivers/i2c/gpiomux/bus/bus.c @@ -0,0 +1,60 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> +#include <console/console.h> +#include <device/device.h> +#include <device/path.h> +#include <stdlib.h> +#include <string.h> +#include "chip.h" + +static const char *i2c_gpiomux_bus_acpi_name(const struct device *dev) +{ + static char name[ACPI_NAME_BUFFER_SIZE]; + + snprintf(name, ACPI_NAME_BUFFER_SIZE, "MXA%01.1X", dev->path.generic.id); + return name; +} + +static void i2c_gpiomux_bus_fill_ssdt(const struct device *dev) +{ + const char *scope = acpi_device_scope(dev); + const char *path = acpi_device_path(dev); + + if (!dev || !dev->enabled || !scope || !path) + return; + + /* Device */ + acpigen_write_scope(scope); + acpigen_write_device(acpi_device_name(dev)); + + acpigen_write_STA(acpi_device_status(dev)); + acpigen_write_ADR(dev->path.generic.id); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", path, dev->chip_ops->name, dev_path(dev)); +} + +static struct device_operations i2c_gpiomux_bus_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .scan_bus = scan_static_bus, + .acpi_name = i2c_gpiomux_bus_acpi_name, + .acpi_fill_ssdt = i2c_gpiomux_bus_fill_ssdt, +}; + +static void i2c_gpiomux_bus_enable(struct device *dev) +{ + if (!dev) + return; + + dev->ops = &i2c_gpiomux_bus_ops; +} + +struct chip_operations drivers_i2c_gpiomux_bus_ops = { + CHIP_NAME("I2C GPIO MUX Bus Device") + .enable_dev = i2c_gpiomux_bus_enable +}; diff --git a/src/drivers/i2c/gpiomux/bus/chip.h b/src/drivers/i2c/gpiomux/bus/chip.h new file mode 100644 index 0000000..2baf334 --- /dev/null +++ b/src/drivers/i2c/gpiomux/bus/chip.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __I2C_GPIOMUX_BUS_CHIP_H__ +#define __I2C_GPIOMUX_BUS_CHIP_H__ + +struct drivers_i2c_gpiomux_bus_config { +}; + +#endif /* __I2C_GPIOMUX_BUS_CHIP_H__ */
-
Furquan Shaikh (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Patrick Georgi (Code Review)