Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 68 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47235/1
diff --git a/src/drivers/i2c/rx6110sa/chip.h b/src/drivers/i2c/rx6110sa/chip.h index 46ef7f1..feb9133 100644 --- a/src/drivers/i2c/rx6110sa/chip.h +++ b/src/drivers/i2c/rx6110sa/chip.h @@ -3,6 +3,7 @@ #include "rx6110sa.h"
struct drivers_i2c_rx6110sa_config { + unsigned int bus_speed; /* Bus clock in Hz (default 400 kHz)*/ /* The day (of the week) is indicated by 7 bits, bit 0 to bit 6. */ unsigned char user_weekday; /* User day of the week to set */ unsigned char user_day; /* User day to set */ @@ -23,4 +24,9 @@ unsigned char bks_on; unsigned char bks_off; unsigned char iocut_en; /* Disable backup of I/O circuit. */ + + const char *hid; /* ACPI _HID (required) */ + const char *desc; /* ACPI device description */ + const char *name; /* ACPI device name (optional) */ + }; diff --git a/src/drivers/i2c/rx6110sa/rx6110sa.c b/src/drivers/i2c/rx6110sa/rx6110sa.c index ca39bdb..e758728 100644 --- a/src/drivers/i2c/rx6110sa/rx6110sa.c +++ b/src/drivers/i2c/rx6110sa/rx6110sa.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> #include <device/i2c_bus.h> #include <device/device.h> #include <version.h> @@ -163,11 +165,67 @@ rx6110sa_write(dev, CTRL_REG, reg); }
+#if CONFIG(HAVE_ACPI_TABLES) +static void rx6110sa_fill_ssdt(const struct device *dev) +{ + struct drivers_i2c_rx6110sa_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + if (!dev->enabled || !scope) + return; + + if (!config->hid) { + printk(BIOS_ERR, "ERROR: HID for %s required\n", dev_path(dev)); + return; + } + struct acpi_i2c i2c = { + .address = dev->path.i2c.device, + .mode_10bit = dev->path.i2c.mode_10bit, + .speed = config->bus_speed ? : I2C_SPEED_FAST, + .resource = scope, + }; + + /* Device */ + acpigen_write_scope(scope); + acpigen_write_device(acpi_device_name(dev)); + acpigen_write_name_string("_HID", config->hid); + acpigen_write_name_string("_DDN", config->desc); + acpigen_write_STA(acpi_device_status(dev)); + + /* Resources */ + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpi_device_write_i2c(&i2c); + + acpigen_write_resourcetemplate_footer(); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), + dev->chip_ops->name, dev_path(dev)); +} + +/* Use name specified in config or take a fixed name. */ +static const char *rx6110sa_acpi_name(const struct device *dev) +{ + struct drivers_i2c_rx6110sa_config *config = dev->chip_info; + + if (config->name && strlen(config->name) == RX6110SA_ACPI_NAME_LEN) + return config->name; + else + return RX6110SA_ACPI_NAME; +} +#endif + static struct device_operations rx6110sa_ops = { .read_resources = noop_read_resources, .set_resources = noop_set_resources, .init = rx6110sa_init, - .final = rx6110sa_final + .final = rx6110sa_final, +#if CONFIG(HAVE_ACPI_TABLES) + .acpi_name = rx6110sa_acpi_name, + .acpi_fill_ssdt = rx6110sa_fill_ssdt, +#endif };
static void rx6110sa_enable(struct device *dev) diff --git a/src/drivers/i2c/rx6110sa/rx6110sa.h b/src/drivers/i2c/rx6110sa/rx6110sa.h index 187bad4..91685e9 100644 --- a/src/drivers/i2c/rx6110sa/rx6110sa.h +++ b/src/drivers/i2c/rx6110sa/rx6110sa.h @@ -7,6 +7,9 @@ #define RX6110SA_SLAVE_ADR 0x32 #define RX6110SA_I2C_CONTROLLER 0
+#define RX6110SA_ACPI_NAME "ERX6" +#define RX6110SA_ACPI_NAME_LEN 4 + /* Register layout */ #define SECOND_REG 0x10 #define MINUTE_REG 0x11
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 1:
start siemens-bot
Johannes Hahn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 1: Code-Review+1
Tested successful on another Siemens platform using this rtc.
Hello build bot (Jenkins), siemens-bot, Johannes Hahn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47235
to look at the new patch set (#2).
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 56 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47235/2
Hello build bot (Jenkins), siemens-bot, Johannes Hahn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47235
to look at the new patch set (#3).
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 56 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47235/3
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 3: Code-Review+1
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 180: .speed = config->bus_speed ? : I2C_SPEED_FAST, so 1hz is fast and 0hz is not?
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 205: /* Use name specified in config or take a fixed name. */ it always used a fixed name, no?
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 180: .speed = config->bus_speed ? : I2C_SPEED_FAST,
so 1hz is fast and 0hz is not?
If there is nothing defined in config->bus_speed (bus_speed=0), then the maximum supported bus speed of the RTC (I2C_SPEED_FAST = 400000,) is stored here. Otherwise the devicetree-value will be used to allow mainboard to override the speed if needed. Should I add a check to make sure the devicetree value is a valid one?
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 205: /* Use name specified in config or take a fixed name. */
it always used a fixed name, no?
Oh, left-over from the previous patchset, will remove.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 180: .speed = config->bus_speed ? : I2C_SPEED_FAST,
If there is nothing defined in config->bus_speed (bus_speed=0), then the maximum supported bus speed […]
Maybe add some code somewhere to throw a warning if it's an unexpected value and fall back to slow?
Hello build bot (Jenkins), Mario Scheithauer, siemens-bot, Johannes Hahn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47235
to look at the new patch set (#4).
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 70 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47235/4
Hello build bot (Jenkins), Mario Scheithauer, siemens-bot, Johannes Hahn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47235
to look at the new patch set (#5).
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 70 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47235/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 5: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 180: .speed = config->bus_speed ? : I2C_SPEED_FAST,
Maybe add some code somewhere to throw a warning if it's an unexpected value and fall back to slow?
Using an enum is a good idea, too
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 176: !dev->enabled CB:47148 made this check unnecessary
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 215: BIOS_INFO nit: I'd use BIOS_DEBUG
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG@11 PS5, Line 11: If there are any, please paste the Linux kernel message, detecting the ACPI interface.
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 185: printk(BIOS_WARNING, "%s: Unsupported bus speed in devicetree!\n", Please extend the message tro mention, that it falls back to standard speed.
Johannes Hahn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG@11 PS5, Line 11:
If there are any, please paste the Linux kernel message, detecting the ACPI interface.
The Linux driver patch should output the following dmesg message: "rx6110 i2c-RX6110SA:00: rtc core: registered RX6110SA:00 as rtcX" "rtc rtcX: Update timer was detected" Where X is the following number assigned by Linux.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG@11 PS5, Line 11:
The Linux driver patch should output the following dmesg message: […]
Paul, do you want me to add these lines to the commit message?
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 176: !dev->enabled
CB:47148 made this check unnecessary
Oh, I was not aware of this change, thank you. I will remove this check here.
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 185: printk(BIOS_WARNING, "%s: Unsupported bus speed in devicetree!\n",
Please extend the message tro mention, that it falls back to standard speed.
I had that in the first place but the line got really long. Will think about a shorter way and add the hint.
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 215: BIOS_INFO
nit: I'd use BIOS_DEBUG
I would rather go with BIOS_INFO then as BIOS_DEBUG produces a large output where things get lost easily.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 215: BIOS_INFO
I would rather go with BIOS_INFO then as BIOS_DEBUG produces a large output where things get lost ea […]
Ack
Hello build bot (Jenkins), Mario Scheithauer, Angel Pons, siemens-bot, Johannes Hahn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47235
to look at the new patch set (#6).
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
If the Linux kernel is able to find the RTC in ACPI scope, you should see the following lines in dmesg, where [n] is an enumerated number:
rx6110 i2c-RX6110SA:00: rtc core: registered RX6110SA:00 as rtc[n] rtc rtc[n]: Update timer was detected
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 70 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/47235/6
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG@11 PS5, Line 11:
Paul, do you want me to add these lines to the commit message?
If you could add it, that’d be great for a TEST= line. If not, it’s only a nit.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 6: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47235/5//COMMIT_MSG@11 PS5, Line 11:
If you could add it, that’d be great for a TEST= line. If not, it’s only a nit.
Done
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/3/src/drivers/i2c/rx6110sa/rx... PS3, Line 180: .speed = config->bus_speed ? : I2C_SPEED_FAST,
Using an enum is a good idea, too
Done
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... File src/drivers/i2c/rx6110sa/rx6110sa.c:
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 176: !dev->enabled
Oh, I was not aware of this change, thank you. […]
Done
https://review.coreboot.org/c/coreboot/+/47235/5/src/drivers/i2c/rx6110sa/rx... PS5, Line 185: printk(BIOS_WARNING, "%s: Unsupported bus speed in devicetree!\n",
I had that in the first place but the line got really long. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 6: Code-Review+1
Werner Zeh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
drivers/i2c/rx6110sa: Add basic ACPI support
This patch adds basic ACPI support for the RTC so that the OS is able to use this RTC via the ACPI interface.
If the Linux kernel is able to find the RTC in ACPI scope, you should see the following lines in dmesg, where [n] is an enumerated number:
rx6110 i2c-RX6110SA:00: rtc core: registered RX6110SA:00 as rtc[n] rtc rtc[n]: Update timer was detected
Change-Id: I9b319e3088e6511592075b055f8fa3e2aedaa209 Signed-off-by: Werner Zeh werner.zeh@siemens.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47235 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/drivers/i2c/rx6110sa/chip.h M src/drivers/i2c/rx6110sa/rx6110sa.c M src/drivers/i2c/rx6110sa/rx6110sa.h 3 files changed, 70 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/drivers/i2c/rx6110sa/chip.h b/src/drivers/i2c/rx6110sa/chip.h index 46ef7f1..4df9580 100644 --- a/src/drivers/i2c/rx6110sa/chip.h +++ b/src/drivers/i2c/rx6110sa/chip.h @@ -3,6 +3,7 @@ #include "rx6110sa.h"
struct drivers_i2c_rx6110sa_config { + unsigned int bus_speed; /* Bus clock in Hz (default 400 kHz)*/ /* The day (of the week) is indicated by 7 bits, bit 0 to bit 6. */ unsigned char user_weekday; /* User day of the week to set */ unsigned char user_day; /* User day to set */ diff --git a/src/drivers/i2c/rx6110sa/rx6110sa.c b/src/drivers/i2c/rx6110sa/rx6110sa.c index ca39bdb..2b8b9b2 100644 --- a/src/drivers/i2c/rx6110sa/rx6110sa.c +++ b/src/drivers/i2c/rx6110sa/rx6110sa.c @@ -1,7 +1,10 @@ /* SPDX-License-Identifier: GPL-2.0-only */
-#include <device/i2c_bus.h> +#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> #include <device/device.h> +#include <device/i2c.h> +#include <device/i2c_bus.h> #include <version.h> #include <console/console.h> #include <bcd.h> @@ -163,11 +166,71 @@ rx6110sa_write(dev, CTRL_REG, reg); }
+#if CONFIG(HAVE_ACPI_TABLES) +static void rx6110sa_fill_ssdt(const struct device *dev) +{ + struct drivers_i2c_rx6110sa_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + enum i2c_speed bus_speed; + + if (!scope) + return; + + switch (config->bus_speed) { + case I2C_SPEED_STANDARD: + case I2C_SPEED_FAST: + bus_speed = config->bus_speed; + break; + default: + printk(BIOS_INFO, "%s: Bus speed unsupported, fall back to %d kHz!\n", + dev_path(dev), I2C_SPEED_STANDARD / 1000); + bus_speed = I2C_SPEED_STANDARD; + break; + } + + struct acpi_i2c i2c = { + .address = dev->path.i2c.device, + .mode_10bit = dev->path.i2c.mode_10bit, + .speed = bus_speed, + .resource = scope, + }; + + /* Device */ + acpigen_write_scope(scope); + acpigen_write_device(acpi_device_name(dev)); + acpigen_write_name_string("_HID", RX6110SA_HID_NAME); + acpigen_write_name_string("_DDN", RX6110SA_HID_DESC); + acpigen_write_STA(acpi_device_status(dev)); + + /* Resources */ + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpi_device_write_i2c(&i2c); + + acpigen_write_resourcetemplate_footer(); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), + dev->chip_ops->name, dev_path(dev)); +} + +static const char *rx6110sa_acpi_name(const struct device *dev) +{ + return RX6110SA_ACPI_NAME; +} +#endif + static struct device_operations rx6110sa_ops = { .read_resources = noop_read_resources, .set_resources = noop_set_resources, .init = rx6110sa_init, - .final = rx6110sa_final + .final = rx6110sa_final, +#if CONFIG(HAVE_ACPI_TABLES) + .acpi_name = rx6110sa_acpi_name, + .acpi_fill_ssdt = rx6110sa_fill_ssdt, +#endif };
static void rx6110sa_enable(struct device *dev) diff --git a/src/drivers/i2c/rx6110sa/rx6110sa.h b/src/drivers/i2c/rx6110sa/rx6110sa.h index 187bad4..73f43b3 100644 --- a/src/drivers/i2c/rx6110sa/rx6110sa.h +++ b/src/drivers/i2c/rx6110sa/rx6110sa.h @@ -7,6 +7,10 @@ #define RX6110SA_SLAVE_ADR 0x32 #define RX6110SA_I2C_CONTROLLER 0
+#define RX6110SA_ACPI_NAME "ERX6" +#define RX6110SA_HID_NAME "RX6110SA" +#define RX6110SA_HID_DESC "Real Time Clock" + /* Register layout */ #define SECOND_REG 0x10 #define MINUTE_REG 0x11
Andy Shevchenko has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
What the heck is going on here?!?!?! Gyus, stop this, please! Can somebody in Coreboot community read the ACPI spec (in particular chapter 6.1.5) and forbid such changes?
This MUST be reverted!
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
What the heck is going on here?!?!?! Gyus, stop this, please! Can somebody in Coreboot community read the ACPI spec (in particular chapter 6.1.5) and forbid such changes?
This MUST be reverted!
Could you please calm down and explain *why* you think this change is wrong?
Andy Shevchenko has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
What the heck is going on here?!?!?! Gyus, stop this, please! Can somebody in Coreboot community read the ACPI spec (in particular chapter 6.1.5) and forbid such changes?
This MUST be reverted!
Could you please calm down and explain *why* you think this change is wrong?
Sure. Sorry for hysteria from my side, because I fed up with abused ACPI IDs in Linux kernel [1][2] (and more). Here we have a chance to do the right things right. what I'm complaining about is solely this (the rest of the patch is absolutely fine with me):
#define RX6110SA_HID_NAME "RX6110SA"
The problem here is that this is completely "invented" ACPI HID against ACPI specification [3], chapter 6.1.5 and against ACPI/PNP registry [4].
The component is produced by Seiko Epson company which has it's own record in the registry [4] 'SEC' and according to specifications [3] the ID should be something like SEChhhh (where h is capitalized hex digit) issued by the vendor. Above doesn't correspond this and must be fixed.
[1]: https://lore.kernel.org/linux-rtc/20201116142859.31257-1-andriy.shevchenko@l... [2]: https://lore.kernel.org/linux-rtc/20201112130734.331094-1-ch@denx.de/ [3]: https://uefi.org/sites/default/files/resources/ACPI_Spec_6_3_A_Oct_6_2020.pd... [4]: https://www.uefi.org/PNP_ACPI_Registry
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
What the heck is going on here?!?!?! Gyus, stop this, please! Can somebody in Coreboot community read the ACPI spec (in particular chapter 6.1.5) and forbid such changes?
This MUST be reverted!
Could you please calm down and explain *why* you think this change is wrong?
Sure. Sorry for hysteria from my side, because I fed up with abused ACPI IDs in Linux kernel [1][2] (and more). Here we have a chance to do the right things right. what I'm complaining about is solely this (the rest of the patch is absolutely fine with me):
#define RX6110SA_HID_NAME "RX6110SA"
The problem here is that this is completely "invented" ACPI HID against ACPI specification [3], chapter 6.1.5 and against ACPI/PNP registry [4].
The component is produced by Seiko Epson company which has it's own record in the registry [4] 'SEC' and according to specifications [3] the ID should be something like SEChhhh (where h is capitalized hex digit) issued by the vendor. Above doesn't correspond this and must be fixed.
Thank you for the excellent description of the issue at hand. I agree that using an invented ACPI ID isn't a good idea, but I don't know where one would find the correct ID to use. I see that you described the options here: https://lore.kernel.org/linux-rtc/CAHp75Vdxj0tgn6P8Nfi5mMd=e9Q1+hzt4bquzB93z...
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
Patch Set 7:
What the heck is going on here?!?!?! Gyus, stop this, please! Can somebody in Coreboot community read the ACPI spec (in particular chapter 6.1.5) and forbid such changes?
This MUST be reverted!
Could you please calm down and explain *why* you think this change is wrong?
Sure. Sorry for hysteria from my side, because I fed up with abused ACPI IDs in Linux kernel [1][2] (and more). Here we have a chance to do the right things right. what I'm complaining about is solely this (the rest of the patch is absolutely fine with me):
#define RX6110SA_HID_NAME "RX6110SA"
The problem here is that this is completely "invented" ACPI HID against ACPI specification [3], chapter 6.1.5 and against ACPI/PNP registry [4].
The component is produced by Seiko Epson company which has it's own record in the registry [4] 'SEC' and according to specifications [3] the ID should be something like SEChhhh (where h is capitalized hex digit) issued by the vendor. Above doesn't correspond this and must be fixed.
Thank you for the excellent description of the issue at hand. I agree that using an invented ACPI ID isn't a good idea, but I don't know where one would find the correct ID to use. I see that you described the options here: https://lore.kernel.org/linux-rtc/CAHp75Vdxj0tgn6P8Nfi5mMd=e9Q1+hzt4bquzB93z...
Hey Andy.
Thank you for the pointer. We had an internal discussion regarding this HID for the RTC _before_ pushing this patch with exact the same arguments you have pointed out, namely the ACPI spec. At this point in time we had a deep search for the right ID but were not able to find anything (just like you already have discovered that there is none). We then took a closer look to other examples like that and figured out, that there are different HIDs out there, even in Linux kernel, which does not match the spec naming convention. And yes, you are right, finding a bad example should not be used as a justification to do it wrong once again. The point where I underestimated this issue is: I never though or was aware of that the kernel folks already started to clean up that mess in the kernel. Sorry for that! Here is what I propose in this situation: We have already reached out to Seico Epson requesting a proper HID created by the manufacturer of the RTC. As soon as we get a reaction there, I will correct the HID here to match the one from Seico Epson with a patch. For now I do not see an urgent case to revert this patch as, as long there is no one using this ID in the kernel, it stays unused in the system. But at least we can test with this ID. Any objections from your side regarding this plan?
Andy Shevchenko has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Sure. Sorry for hysteria from my side, because I fed up with abused ACPI IDs in Linux kernel [1][2] (and more). Here we have a chance to do the right things right. what I'm complaining about is solely this (the rest of the patch is absolutely fine with me):
#define RX6110SA_HID_NAME "RX6110SA"
The problem here is that this is completely "invented" ACPI HID against ACPI specification [3], chapter 6.1.5 and against ACPI/PNP registry [4].
The component is produced by Seiko Epson company which has it's own record in the registry [4] 'SEC' and according to specifications [3] the ID should be something like SEChhhh (where h is capitalized hex digit) issued by the vendor. Above doesn't correspond this and must be fixed.
Thank you for the excellent description of the issue at hand. I agree that using an invented ACPI ID isn't a good idea, but I don't know where one would find the correct ID to use. I see that you described the options here: https://lore.kernel.org/linux-rtc/CAHp75Vdxj0tgn6P8Nfi5mMd=e9Q1+hzt4bquzB93z...
Thank you for the pointer. We had an internal discussion regarding this HID for the RTC _before_ pushing this patch with exact the same arguments you have pointed out, namely the ACPI spec. At this point in time we had a deep search for the right ID but were not able to find anything (just like you already have discovered that there is none). We then took a closer look to other examples like that and figured out, that there are different HIDs out there, even in Linux kernel, which does not match the spec naming convention. And yes, you are right, finding a bad example should not be used as a justification to do it wrong once again.
Thanks for being aware of this.
The point where I underestimated this issue is: I never though or was aware of that the kernel folks already started to clean up that mess in the kernel. Sorry for that! Here is what I propose in this situation: We have already reached out to Seico Epson requesting a proper HID created by the manufacturer of the RTC. As soon as we get a reaction there, I will correct the HID here to match the one from Seico Epson with a patch. For now I do not see an urgent case to revert this patch as, as long there is no one using this ID in the kernel, it stays unused in the system. But at least we can test with this ID. Any objections from your side regarding this plan?
I would prefer to show explicitly that there is no HID assigned (current HID may give an impression that people are fine to use it). So, something like RX6110SA -> XXXX0000 will be good to do now. Otherwise no objections. (Of course you may switch over to PRP0001 which is legal use with DT properties and compatible string, but highly discouraged for market ready products)
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Sure. Sorry for hysteria from my side, because I fed up with abused ACPI IDs in Linux kernel [1][2] (and more). Here we have a chance to do the right things right. what I'm complaining about is solely this (the rest of the patch is absolutely fine with me):
#define RX6110SA_HID_NAME "RX6110SA"
The problem here is that this is completely "invented" ACPI HID against ACPI specification [3], chapter 6.1.5 and against ACPI/PNP registry [4].
The component is produced by Seiko Epson company which has it's own record in the registry [4] 'SEC' and according to specifications [3] the ID should be something like SEChhhh (where h is capitalized hex digit) issued by the vendor. Above doesn't correspond this and must be fixed.
Thank you for the excellent description of the issue at hand. I agree that using an invented ACPI ID isn't a good idea, but I don't know where one would find the correct ID to use. I see that you described the options here: https://lore.kernel.org/linux-rtc/CAHp75Vdxj0tgn6P8Nfi5mMd=e9Q1+hzt4bquzB93z...
Thank you for the pointer. We had an internal discussion regarding this HID for the RTC _before_ pushing this patch with exact the same arguments you have pointed out, namely the ACPI spec. At this point in time we had a deep search for the right ID but were not able to find anything (just like you already have discovered that there is none). We then took a closer look to other examples like that and figured out, that there are different HIDs out there, even in Linux kernel, which does not match the spec naming convention. And yes, you are right, finding a bad example should not be used as a justification to do it wrong once again.
Thanks for being aware of this.
The point where I underestimated this issue is: I never though or was aware of that the kernel folks already started to clean up that mess in the kernel. Sorry for that! Here is what I propose in this situation: We have already reached out to Seico Epson requesting a proper HID created by the manufacturer of the RTC. As soon as we get a reaction there, I will correct the HID here to match the one from Seico Epson with a patch. For now I do not see an urgent case to revert this patch as, as long there is no one using this ID in the kernel, it stays unused in the system. But at least we can test with this ID. Any objections from your side regarding this plan?
I would prefer to show explicitly that there is no HID assigned (current HID may give an impression that people are fine to use it). So, something like RX6110SA -> XXXX0000 will be good to do now. Otherwise no objections. (Of course you may switch over to PRP0001 which is legal use with DT properties and compatible string, but highly discouraged for market ready products)
OK, let me first get a feeling what Seico Epson can deliver and when. If it will turn out that we have a long run in front of us to get the right HID for the RTC, I will push a patch to change the HID to something different clearly stating that it is not ready for productive usage for now. If in turn there is a chance that we will get the needed HID soon (let's say within two weeks or so) I would like to wait for it. Does that sound reasonable?
Andy Shevchenko has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Here is what I propose in this situation: We have already reached out to Seico Epson requesting a proper HID created by the manufacturer of the RTC. As soon as we get a reaction there, I will correct the HID here to match the one from Seico Epson with a patch. For now I do not see an urgent case to revert this patch as, as long there is no one using this ID in the kernel, it stays unused in the system. But at least we can test with this ID. Any objections from your side regarding this plan?
I would prefer to show explicitly that there is no HID assigned (current HID may give an impression that people are fine to use it). So, something like RX6110SA -> XXXX0000 will be good to do now. Otherwise no objections. (Of course you may switch over to PRP0001 which is legal use with DT properties and compatible string, but highly discouraged for market ready products)
OK, let me first get a feeling what Seico Epson can deliver and when. If it will turn out that we have a long run in front of us to get the right HID for the RTC, I will push a patch to change the HID to something different clearly stating that it is not ready for productive usage for now. If in turn there is a chance that we will get the needed HID soon (let's say within two weeks or so) I would like to wait for it. Does that sound reasonable?
Yes, I agree. Thank you! (Just to point out that as one of the last resort Siemens can allocate ID from their pool)
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47235 )
Change subject: drivers/i2c/rx6110sa: Add basic ACPI support ......................................................................
Patch Set 7:
Patch Set 7:
Here is what I propose in this situation: We have already reached out to Seico Epson requesting a proper HID created by the manufacturer of the RTC. As soon as we get a reaction there, I will correct the HID here to match the one from Seico Epson with a patch. For now I do not see an urgent case to revert this patch as, as long there is no one using this ID in the kernel, it stays unused in the system. But at least we can test with this ID. Any objections from your side regarding this plan?
I would prefer to show explicitly that there is no HID assigned (current HID may give an impression that people are fine to use it). So, something like RX6110SA -> XXXX0000 will be good to do now. Otherwise no objections. (Of course you may switch over to PRP0001 which is legal use with DT properties and compatible string, but highly discouraged for market ready products)
OK, let me first get a feeling what Seico Epson can deliver and when. If it will turn out that we have a long run in front of us to get the right HID for the RTC, I will push a patch to change the HID to something different clearly stating that it is not ready for productive usage for now. If in turn there is a chance that we will get the needed HID soon (let's say within two weeks or so) I would like to wait for it. Does that sound reasonable?
Yes, I agree. Thank you! (Just to point out that as one of the last resort Siemens can allocate ID from their pool)
In order to avoid that this wrong HID will be part of our next release planned for the end of this week I have pushed a patch (https://review.coreboot.org/c/coreboot/+/47706) to temporarily omit the HID completely. Once we have a valid ID, I will change the code accordingly.
-
Andy Shevchenko (Code Review) -
Angel Pons (Code Review)
-
Johannes Hahn (Code Review)
-
Mario Scheithauer (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Werner Zeh (Code Review)