| Message ID | 20240321173447.15660-1-shimmyshai00@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [0/0] (proposed?) Add ACPI binding to Rockchip RK3xxx I2C bus | expand |
On Thu, 21 Mar 2024 12:34:47 -0500 Shimrra Shai <shimmyshai00@gmail.com> wrote: > Hi! I have been contributing to a firmware project for Rockchip RK3588- > based board using the TianoCore EDK2 firmware development kit, at > https://github.com/edk2-porting/edk2-rk3588. I am seeking to get it > possible to seamlessly boot mainline kernels using it on these platforms, > especially given how that mainline support for this platform has progressed > far since its debut. Since this is not a firmware from a board vendor, I'm > actually not really sure how to best interface or integrate its development > with the Linux kernel community. Particularly the question of Device Tree > Blob (DTB) vs. ACPI binding for configuration bindings, for which previous > discussions have always seemed to center around situations where the > firmware was not controllable by the kernel developer, but that is not the > case with this situation here and now. However, I've found, particularly > given the firmware has an HDMI driver that the currnet kernel does not, > that I am able to get a quicker path to mainline boot on this platform > going through the ACPI route, which inspires the following patch. > > In particular, I found one small issue that obstructs immediate direct > booting of the off-the-shelf downloaded 6.8.x kernel series on my platform > (Firefly ITX-3588J, with no mainline device tree blob support) which is > that the I2C bus lacks code to configure it in the ACPI boot mode. The > firmware came with ACPI bindings for it, nonetheless, and so this patch > acts to enable and document those bindings for future use. It would be good to highlight in this description what is missing for doing a standard ACPI binding and not using any specific hacks in the driver (get clocks as normal etc). There are ACPI clock bindings, but Linux doesn't support the yet (I think?) See ACPICA commit https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a I've seen prototype code but was a while back. I'd like to see that work compled rather than having every driver need to paper over the hole. The alias is a different question that needs to be addressed. If this is a common pattern, push it up in to the i2c core, not a specific driver. I see there is already code related to that in i2c_add_adapter - that just wants an ACPI option. Jonathan > > Signed-off-by: Shimrra Shai <shimmyshai00@gmail.com> > --- > > Documentation/firmware-guide/acpi/dsd/soc/general.rst | 32 ++++++++ > Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst | 50 ++++++++++++ > drivers/i2c/busses/i2c-rk3x.c | 105 +++++++++++++++++++++++--- > 3 files changed, 177 insertions(+), 10 deletions(-) > > diff --git a/Documentation/firmware-guide/acpi/dsd/soc/general.rst b/Documentation/firmware-guide/acpi/dsd/soc/general.rst > new file mode 100644 > index 000000000..d21ccad2d > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/soc/general.rst > @@ -0,0 +1,32 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================================== > +System-on-Chip (SoC) facility descriptions > +========================================== > + > +These documents describe how to create ACPI profiles for devices compsed into composed Spell check the whole thing. > +system-on-chip (SoC) architectures. Currently, we only describe the options > +availed in the _DSD block for select platforms; other considerations are not > +yet applied. The inspiration for these documents are based around the advent > +(as of March 2024) of hefty, non-Apple ARM SoC systems such as the Rockchip > +RK3588 and newer chips that have desktop-like performance and thus are prime > +candidates for a UEFI-based desktop-like boot system with the goal being to > +deliver the same user-friendly ease of loading operating systems as on the > +Intel x86 sphere. Open-source UEFI-based firmware engines, such as TianoCore > +[1], mean it is possible on such platforms for the open-source developer to > +control both firmware and kernel simultaneously, which is not the case for > +the situation with Intel-based PC boards where boards are provided with pre- > +baked, vendor-selected and opaque firmwares. > + > +The description of ACPI usage here is not meant to suggest that ACPI replace > +Deviec Tree altogether for such SoCs; rather, we recognize that given they > +often will have a variety of applications that may include embedded usage where > +that more hard-wired boot loader setups such as U-Boot still shine, the > +maintenance of ACPI and DTB-based configuration options should be in parallel, > +and it may be possible for the same firmware to deploy both options. > + > +References > +========== > + > +[1] EDK2-RK3588 port of TianoCore EDK2 firmware. > + https://github.com/edk2-porting/edk2-rk3588 > diff --git a/Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst b/Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst > new file mode 100644 > index 000000000..47ff69db4 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst > @@ -0,0 +1,50 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +================= > +I2C buses on SoCs > +================= > + > +This document describes the ACPI _DSD parameters currently being employed for > +Inter-Integrated Circuit (I2C) buses in the Linux kernel. It is based off the > +conventions used in the Rockchip RK3588 firmware project [1], which is also the > +first SoC documented here. > + > +General considerations > +====================== > + > +For general use, we recommend indicating I2C busses in the ACPI firmware table > +in the following manner. First, they should be named I2Cx, where "x" is the bus > +index, and that index should also be used for the _UID component, e.g. on > +Rockchip RK3588 (see below), we use: > + > + Device (I2Cx) { > + Name (_HID, "RKCP3001") > + Name (_CID, "PRP0001") That implies that the kernel can cope with the device tree wrapped up in ACPI path. If that's the case, why do you need RKCP3001 as you can match on the compatible? > + Name (_UID, x) > + Name (_CCA, 0) > + > + ... > + } > + > +Interrupts should be specified in the usual ACPI manner. Parameters specific to > +the Rockchip and I2C devices are indicated in the _DSD block as given below. > +The parameters are a curated selection from the Device Tree Blob (DTB) > +representation. > + > +_DSD parameters for different SoCs > +================================== > + > +Rockchip RK3588 > +--------------- > + > +The following parameters are accepted for the I2C on Rockchip RK3588. > + > +- i2c,clk-rate Describe the pclk rate for the I2C bus, in Hz. > +- rockchip,bclk Describe the bclk rate for the I2C bus, in Hz. > + > +References > +========== > + > +[1] EDK2-RK3588 port of TianoCore EDK2 firmware. > + https://github.com/edk2-porting/edk2-rk3588 > + > diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c > index 086fdf262..5a5e1d551 100644 > --- a/drivers/i2c/busses/i2c-rk3x.c > +++ b/drivers/i2c/busses/i2c-rk3x.c > @@ -19,6 +19,9 @@ > #include <linux/of_irq.h> > #include <linux/spinlock.h> > #include <linux/clk.h> > +#ifdef CONFIG_ACPI > +#include <linux/clk-provider.h> > +#endif > #include <linux/wait.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > @@ -1235,6 +1238,15 @@ static const struct of_device_id rk3x_i2c_match[] = { > }; > MODULE_DEVICE_TABLE(of, rk3x_i2c_match); > > +#ifdef CONFIG_ACPI > +/* for RK3588 and at least when loaded with EDK2-RK3588 Tianocore firmware */ > +static const struct acpi_device_id rk3x_i2c_acpi_match[] = { > + { .id = "RKCP3001", .driver_data = (kernel_ulong_t)&rk3399_soc_data }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, rk3x_i2c_acpi_match); > +#endif > + > static int rk3x_i2c_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -1243,6 +1255,10 @@ static int rk3x_i2c_probe(struct platform_device *pdev) > int ret = 0; > int bus_nr; > u32 value; > +#ifdef CONFIG_ACPI > + u64 value64; > + char clk_name[20]; > +#endif > int irq; > unsigned long clk_rate; > > @@ -1250,8 +1266,12 @@ static int rk3x_i2c_probe(struct platform_device *pdev) > if (!i2c) > return -ENOMEM; > > - match = of_match_node(rk3x_i2c_match, np); > - i2c->soc_data = match->data; > + if (acpi_disabled) { > + match = of_match_node(rk3x_i2c_match, np); > + i2c->soc_data = match->data; > + } else { > + i2c->soc_data = device_get_match_data(&pdev->dev); > + } > > /* use common interface to get I2C timing properties */ > i2c_parse_fw_timings(&pdev->dev, &i2c->t, true); > @@ -1266,6 +1286,9 @@ static int rk3x_i2c_probe(struct platform_device *pdev) > > i2c->dev = &pdev->dev; > > + if (!acpi_disabled) > + ACPI_COMPANION_SET(&i2c->adap.dev, ACPI_COMPANION(&pdev->dev)); > + > spin_lock_init(&i2c->lock); > init_waitqueue_head(&i2c->wait); > > @@ -1273,8 +1296,25 @@ static int rk3x_i2c_probe(struct platform_device *pdev) > if (IS_ERR(i2c->regs)) > return PTR_ERR(i2c->regs); > > - /* Try to set the I2C adapter number from dt */ > - bus_nr = of_alias_get_id(np, "i2c"); > + if (acpi_disabled) { > + /* Try to set the I2C adapter number from dt */ > + bus_nr = of_alias_get_id(np, "i2c"); > + } else { > + ret = acpi_dev_uid_to_integer(ACPI_COMPANION(&pdev->dev), > + &value64); > + if (ret) { > + dev_err(&pdev->dev, "Cannot retrieve UID\n"); > + return ret; > + } > + > + if (value64 < INT_MAX) { > + bus_nr = (int) value64; > + } else { > + /* shouldn't happen!!! */ > + dev_err(&pdev->dev, "Too big UID\n"); > + return -EINVAL; > + } > + } > > /* > * Switch to new interface if the SoC also offers the old one. > @@ -1325,13 +1365,58 @@ static int rk3x_i2c_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, i2c); > > - if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) { > - /* Only one clock to use for bus clock and peripheral clock */ > - i2c->clk = devm_clk_get(&pdev->dev, NULL); > - i2c->pclk = i2c->clk; > + if (acpi_disabled) { > + if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) { > + /* Only one clock to use for bus clock and peripheral > + * clock > + */ > + i2c->clk = devm_clk_get(&pdev->dev, NULL); > + i2c->pclk = i2c->clk; > + } else { > + i2c->clk = devm_clk_get(&pdev->dev, "i2c"); > + i2c->pclk = devm_clk_get(&pdev->dev, "pclk"); > + } > } else { > - i2c->clk = devm_clk_get(&pdev->dev, "i2c"); > - i2c->pclk = devm_clk_get(&pdev->dev, "pclk"); > + if (i2c->soc_data->calc_timings != rk3x_i2c_v0_calc_timings) { > + u32 bclkrate = 0; > + u32 pclkrate = 0; > + > + device_property_read_u32(&pdev->dev, "rockchip,bclk", > + &bclkrate); > + device_property_read_u32(&pdev->dev, "i2c,clk-rate", > + &pclkrate); > + > + if (bclkrate != 0) { > + sprintf(clk_name, "rockchip,i2c-clk.%d", bus_nr); > + i2c->clk = clk_register_fixed_rate(&pdev->dev, > + clk_name, > + NULL, > + 0, bclkrate); > + dev_dbg(&pdev->dev, > + "registered i2c clk at %u Hz\n", > + bclkrate); > + } > + > + if (pclkrate != 0) { > + sprintf(clk_name, "rockchip,i2c-pclk.%d", > + bus_nr); > + i2c->pclk = clk_register_fixed_rate(&pdev->dev, > + clk_name, > + NULL, 0, > + pclkrate); > + dev_dbg(&pdev->dev, > + "registered i2c pclk at %u Hz\n", > + pclkrate); > + } > + } else { > + /* NB: currently not expected w/UEFI firmware given > + * these are not super performant RK3xxx > + */ > + dev_err(&pdev->dev, > + "ACPI not supported for this RK3xxx\n"); > + > + return -EINVAL; > + } > } > > if (IS_ERR(i2c->clk)) > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: For > It would be good to highlight in this description what is missing for > doing a standard ACPI binding and not using any specific hacks in the > driver (get clocks as normal etc). > > There are ACPI clock bindings, but Linux doesn't support the yet (I think?) > See ACPICA commit > https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a > > I've seen prototype code but was a while back. I'd like to see that > work compled rather than having every driver need to paper over the hole. > > The alias is a different question that needs to be addressed. > If this is a common pattern, push it up in to the i2c core, not > a specific driver. I see there is already code related to that > in i2c_add_adapter - that just wants an ACPI option. > > Jonathan and > That implies that the kernel can cope with the device tree wrapped up in > ACPI path. If that's the case, why do you need RKCP3001 as you can > match on the compatible? This was all based on how the people working on the firmware project wrote the ACPI binding. That said, since I've got a foot in it too I can definitely submit them an updated binding. The binding for the I2C in the project looks like this: Device (I2C1) { Name (_HID, "RKCP3001") Name (_CID, "PRP0001") Name (_UID, 1) Name (_CCA, 0) Method (_CRS, 0x0, Serialized) { Name (RBUF, ResourceTemplate() { Memory32Fixed (ReadWrite, 0xfea90000, 0x1000) Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 350 } }) Return (RBUF) } Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package (2) { "i2c,clk-rate", 198000000 }, Package (2) { "rockchip,bclk", 198000000 }, Package (2) { "#address-cells", 1 }, Package (2) { "#size-cells", 0 }, } }) } (there are others, e.g. I2C2, I2C3, etc. one for each I2C bus, with correspondingly different _UID and some different numbers for interrupts etc.) From what I'm gathering from reading the documentation at https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html which is admittedly quite terse and doesn't provide nearly enough examples for my liking, and given I have not been able to find a "in the wild" ACPI using ClockInput, I presume a better binding would be like this, correct?: Device (I2C1) { Name (_HID, "RKCP3001") /* _CID is gone now because we are no longer assuming mirror of DT */ Name (_UID, 1) Name (_CCA, 0) Method (_CRS, 0x0, Serialized) { Name (RBUF, ResourceTemplate() { Memory32Fixed (ReadWrite, 0xfea90000, 0x1000) Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 350 } ClockInput (198000000, 1, Hz, Fixed, "PCLK", 0) ClockInput (198000000, 1, Hz, Fixed, "BCLK", 0) }) Return (RBUF) } Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package (2) { "#address-cells", 1 }, Package (2) { "#size-cells", 0 }, } }) Device (PCLK) { Name (_ADR, 0x0) } Device (BCLK) { Name (_ADR, 0x1) } } Note now I added two device nodes for the clocks and use ClockInput to describe the frequencies. I'm unsure though about the device node part, however; I know that the .DTB has a central node for the CRU (the actual clock generator on the RK3588), so should we instead have a top-level Device node "CRU_" and reference the ClockInputs to that, e.g. something like ClockInput (198000000, 1, Hz, Fixed, "CRU_", I2C1_PCLK) ClockInput (198000000, 1, Hz, Fixed, "CRU_", I2C1_BCLK) ? (Note the obvious analogy to rk3588s.dtsi for the labels, which would be #defined constants.) Though in this case I'd ask if someone here would be kind enough to supply the structure for the top-level CRU binding so that I don't have to guess the "best" form for the kernel like the makers of the firmware were doing which is what led to this disagreement in the first place. FWIW, I'd also be willing to help lend a hand to finish out the support for the ClockInput binding in the ACPI reader subsystem. There already seems to be some support, e.g. in drivers/acpi/acpica/rsinfo.c and a few other places, but I'm not sure what else is needed to get it going and would need to study that subsystem in much more depth. - Shimmy
Hey Jonathan,
Since I haven't heard a reply from you so far, and I've also done some more
research on the ClockInput options and stuff I'm soon going to ask the
firmware developers to commit the following updated ACPI bindings unless I
get comment there is still something wrong with them. I'm posting about it
here because this is going to have to be the base for any revisions to
these kernel patches I propose later and so I really want to make sure I
get both "sides" of this right (i.e. both the firmware and kernel). If I
don't hear back within a few days I'm going to just send this as-is and
see if it gets accepted by the firmware team.
Note also that the kernel CRU driver currently won't be able to do anything
with all the info in the CRU binding right now, but the UEFI firmware also
pre-initializes the unit so it isn't super necessary I think, I just
thought to add the binding so it can be referenced elsewhere in the tables.
I believe this is the "proper" way from comparing the descriptions of the
ClockInput tag and "clocks" DTB option; the DTB clocks options all
reference the CRU (which makes sense from description of the physical
hardware itself), so I am fairly confident this is "best", but if anyone
objects on this side I still want to hear it as soon as possible.
ACPI binding for the CRU:
Device (CRU_) {
/* coining a new _HID is unnecessary so long as compatible string exists */
Name (_CID, "PRP0001")
Name (_UID, 0)
Name (_CCA, 0)
Method (_CRS, 0x0, Serialized) {
Name (RBUF, ResourceTemplate() {
Memory32Fixed (ReadWrite, 0xfd7c0000, 0x5c000)
ClockInput (1100000000, 1, Hz, Fixed, "CLK0", 0)
ClockInput (786432000, 1, Hz, Fixed, "CLK1", 0)
ClockInput (850000000, 1, Hz, Fixed, "CLK2", 0)
ClockInput (1188000000, 1, Hz, Fixed, "CLK3", 0)
ClockInput (702000000, 1, Hz, Fixed, "CLK4", 0)
ClockInput (400000000, 1, Hz, Fixed, "CLK5", 0)
ClockInput (500000000, 1, Hz, Fixed, "CLK6", 0)
ClockInput (800000000, 1, Hz, Fixed, "CLK7", 0)
ClockInput (100000000, 1, Hz, Fixed, "CLK8", 0)
ClockInput (400000000, 1, Hz, Fixed, "CLK9", 0)
ClockInput (100000000, 1, Hz, Fixed, "CLKA", 0)
ClockInput (200000000, 1, Hz, Fixed, "CLKB", 0)
ClockInput (500000000, 1, Hz, Fixed, "CLKC", 0)
ClockInput (375000000, 1, Hz, Fixed, "CLKD", 0)
ClockInput (150000000, 1, Hz, Fixed, "CLKE", 0)
ClockInput (200000000, 1, Hz, Fixed, "CLKF", 0)
})
Return (RBUF)
}
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) { "compatible", "rockchip,rk3588-cru" },
}
})
// PLL_PPLL
Device (CLK0) {
Name (_ADR, 0x0)
}
// PLL_AUPLL
Device (CLK1) {
Name (_ADR, 0x1)
}
// PLL_NPLL
Device (CLK2) {
Name (_ADR, 0x2)
}
// PLL_GPLL
Device (CLK3) {
Name (_ADR, 0x3)
}
// ACLK_CENTER_ROOT
Device (CLK4) {
Name (_ADR, 0x4)
}
// HCLK_CENTER_ROOT
Device (CLK5) {
Name (_ADR, 0x5)
}
// ACLK_CENTER_LOW_ROOT
Device (CLK6) {
Name (_ADR, 0x6)
}
// ACLK_TOP_ROOT
Device (CLK7) {
Name (_ADR, 0x7)
}
// PCLK_TOP_ROOT
Device (CLK8) {
Name (_ADR, 0x8)
}
// ACLK_LOW_TOP_ROOT
Device (CLK9) {
Name (_ADR, 0x9)
}
// PCLK_PMU0_ROOT
Device (CLKA) {
Name (_ADR, 0xa)
}
// HCLK_PMU_CM0_ROOT
Device (CLKB) {
Name (_ADR, 0xb)
}
// ACLK_VOP
Device (CLKC) {
Name (_ADR, 0xc)
}
// ACLK_BUS_ROOT
Device (CLKD) {
Name (_ADR, 0xd)
}
// CLK_150M_SRC
Device (CLKE) {
Name (_ADR, 0xe)
}
// CLK_GPU
Device (CLKF) {
Name (_ADR, 0xf)
}
}
ACPI bindings for I2C (I2C1 as example):
Device (I2C1) {
Name (_HID, "RKCP3001")
/* _CID is no longer required */
Name (_UID, 1)
Name (_CCA, 0)
Method (_CRS, 0x0, Serialized) {
Name (RBUF, ResourceTemplate() {
Memory32Fixed (ReadWrite, 0xfea90000, 0x1000)
Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 350 }
ClockInput (198000000, 1, Hz, Fixed, "\_SB_.CRU_", PCLK_I2C1)
ClockInput (198000000, 1, Hz, Fixed, "\_SB_.CRU_", BCLK_I2C1)
})
Return (RBUF)
}
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) { "#address-cells", 1 },
Package (2) { "#size-cells", 0 },
}
})
}
---
Shimmy
On Fri, 22 Mar 2024 10:51:46 -0500 Shimrra Shai <shimmyshai00@gmail.com> wrote: > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > > For > > > It would be good to highlight in this description what is missing for > > doing a standard ACPI binding and not using any specific hacks in the > > driver (get clocks as normal etc). > > > > There are ACPI clock bindings, but Linux doesn't support the yet (I think?) > > See ACPICA commit > > https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a > > > > I've seen prototype code but was a while back. I'd like to see that > > work compled rather than having every driver need to paper over the hole. > > > > The alias is a different question that needs to be addressed. > > If this is a common pattern, push it up in to the i2c core, not > > a specific driver. I see there is already code related to that > > in i2c_add_adapter - that just wants an ACPI option. > > > > Jonathan > > and > > > That implies that the kernel can cope with the device tree wrapped up in > > ACPI path. If that's the case, why do you need RKCP3001 as you can > > match on the compatible? > > This was all based on how the people working on the firmware project wrote > the ACPI binding. That said, since I've got a foot in it too I can > definitely submit them an updated binding. The binding for the I2C in the > project looks like this: > > Device (I2C1) { > Name (_HID, "RKCP3001") > Name (_CID, "PRP0001") > Name (_UID, 1) > Name (_CCA, 0) > > Method (_CRS, 0x0, Serialized) { > Name (RBUF, ResourceTemplate() { > Memory32Fixed (ReadWrite, 0xfea90000, 0x1000) > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 350 } > }) > Return (RBUF) > } > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package (2) { "i2c,clk-rate", 198000000 }, > Package (2) { "rockchip,bclk", 198000000 }, > Package (2) { "#address-cells", 1 }, > Package (2) { "#size-cells", 0 }, > } > }) > } > > (there are others, e.g. I2C2, I2C3, etc. one for each I2C bus, with > correspondingly different _UID and some different numbers for interrupts > etc.) > > From what I'm gathering from reading the documentation at > https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html > which is admittedly quite terse and doesn't provide nearly enough examples > for my liking, and given I have not been able to find a "in the wild" ACPI > using ClockInput, I presume a better binding would be like this, correct?: Niyas is in the CC but is OoO for next few days I think. He wrote the ACPICA patches for this and can provide better answers than me. > > Device (I2C1) { > Name (_HID, "RKCP3001") > /* _CID is gone now because we are no longer assuming mirror of DT */ > Name (_UID, 1) > Name (_CCA, 0) > > Method (_CRS, 0x0, Serialized) { > Name (RBUF, ResourceTemplate() { > Memory32Fixed (ReadWrite, 0xfea90000, 0x1000) > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 350 } > ClockInput (198000000, 1, Hz, Fixed, "PCLK", 0) > ClockInput (198000000, 1, Hz, Fixed, "BCLK", 0) > }) > Return (RBUF) > } > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package (2) { "#address-cells", 1 }, > Package (2) { "#size-cells", 0 }, > } > }) > > Device (PCLK) { > Name (_ADR, 0x0) > } > > Device (BCLK) { > Name (_ADR, 0x1) > } > } > > Note now I added two device nodes for the clocks and use ClockInput to > describe the frequencies. I'm unsure though about the device node part, > however; I know that the .DTB has a central node for the CRU (the actual > clock generator on the RK3588), so should we instead have a top-level > Device node "CRU_" and reference the ClockInputs to that, e.g. something > like > > ClockInput (198000000, 1, Hz, Fixed, "CRU_", I2C1_PCLK) > ClockInput (198000000, 1, Hz, Fixed, "CRU_", I2C1_BCLK) > > ? (Note the obvious analogy to rk3588s.dtsi for the labels, which would be > #defined constants.) Though in this case I'd ask if someone here would be > kind enough to supply the structure for the top-level CRU binding so that I > don't have to guess the "best" form for the kernel like the makers of the > firmware were doing which is what led to this disagreement in the first > place. > > FWIW, I'd also be willing to help lend a hand to finish out the support for > the ClockInput binding in the ACPI reader subsystem. There already seems to > be some support, e.g. in drivers/acpi/acpica/rsinfo.c and a few other > places, but I'm not sure what else is needed to get it going and would need > to study that subsystem in much more depth. That would be great. It's been on my backlog to take a better look at this, but it won't happen any time soon. Niyas, could you forward what you had to Shimrra? (I can find it if you don't have it any more). Thanks, Jonathan > > - Shimmy >
> Niyas, could you forward what you had to Shimrra? (I can find it if you don't have it any more).
I am on holiday so only had a chance to skim through the email. By the way, the half-baked patch is here [1].
The patch only supports fixed clock sources and require further changes to support multiple clock sources.
I will have a look at the discussions when I am back. Hopefully the patch gives some idea. Feel free to take up the patch and fill any missing bits.
[1] https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch
Thanks
Niyas
Hey Jonathan, I just heard back from the firmware team. Apparently, there is rationale behind including the clock bindings as part of the _DSD_ - it is because apparently MS Windows can also run on this platform and it expects those bindings to be there to configure the clocks on this platform. If they are changed, it will break that capability, rendering it kind of moot to use the ACPI boot which is intended to be universal across OSes. (I also wonder if NetBSD uses the same bindings as well; that OS can boot via ACPI boot without much issue on this platform.) Shimrra
diff --git a/Documentation/firmware-guide/acpi/dsd/soc/general.rst b/Documentation/firmware-guide/acpi/dsd/soc/general.rst new file mode 100644 index 000000000..d21ccad2d --- /dev/null +++ b/Documentation/firmware-guide/acpi/dsd/soc/general.rst @@ -0,0 +1,32 @@ +.. SPDX-License-Identifier: GPL-2.0 + +========================================== +System-on-Chip (SoC) facility descriptions +========================================== + +These documents describe how to create ACPI profiles for devices compsed into +system-on-chip (SoC) architectures. Currently, we only describe the options +availed in the _DSD block for select platforms; other considerations are not +yet applied. The inspiration for these documents are based around the advent +(as of March 2024) of hefty, non-Apple ARM SoC systems such as the Rockchip +RK3588 and newer chips that have desktop-like performance and thus are prime +candidates for a UEFI-based desktop-like boot system with the goal being to +deliver the same user-friendly ease of loading operating systems as on the +Intel x86 sphere. Open-source UEFI-based firmware engines, such as TianoCore +[1], mean it is possible on such platforms for the open-source developer to +control both firmware and kernel simultaneously, which is not the case for +the situation with Intel-based PC boards where boards are provided with pre- +baked, vendor-selected and opaque firmwares. + +The description of ACPI usage here is not meant to suggest that ACPI replace +Deviec Tree altogether for such SoCs; rather, we recognize that given they +often will have a variety of applications that may include embedded usage where +that more hard-wired boot loader setups such as U-Boot still shine, the +maintenance of ACPI and DTB-based configuration options should be in parallel, +and it may be possible for the same firmware to deploy both options. + +References +========== + +[1] EDK2-RK3588 port of TianoCore EDK2 firmware. + https://github.com/edk2-porting/edk2-rk3588 diff --git a/Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst b/Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst new file mode 100644 index 000000000..47ff69db4 --- /dev/null +++ b/Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst @@ -0,0 +1,50 @@ +.. SPDX-License-Identifier: GPL-2.0 + +================= +I2C buses on SoCs +================= + +This document describes the ACPI _DSD parameters currently being employed for +Inter-Integrated Circuit (I2C) buses in the Linux kernel. It is based off the +conventions used in the Rockchip RK3588 firmware project [1], which is also the +first SoC documented here. + +General considerations +====================== + +For general use, we recommend indicating I2C busses in the ACPI firmware table +in the following manner. First, they should be named I2Cx, where "x" is the bus +index, and that index should also be used for the _UID component, e.g. on +Rockchip RK3588 (see below), we use: + + Device (I2Cx) { + Name (_HID, "RKCP3001") + Name (_CID, "PRP0001") + Name (_UID, x) + Name (_CCA, 0) + + ... + } + +Interrupts should be specified in the usual ACPI manner. Parameters specific to +the Rockchip and I2C devices are indicated in the _DSD block as given below. +The parameters are a curated selection from the Device Tree Blob (DTB) +representation. + +_DSD parameters for different SoCs +================================== + +Rockchip RK3588 +--------------- + +The following parameters are accepted for the I2C on Rockchip RK3588. + +- i2c,clk-rate Describe the pclk rate for the I2C bus, in Hz. +- rockchip,bclk Describe the bclk rate for the I2C bus, in Hz. + +References +========== + +[1] EDK2-RK3588 port of TianoCore EDK2 firmware. + https://github.com/edk2-porting/edk2-rk3588 + diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 086fdf262..5a5e1d551 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -19,6 +19,9 @@ #include <linux/of_irq.h> #include <linux/spinlock.h> #include <linux/clk.h> +#ifdef CONFIG_ACPI +#include <linux/clk-provider.h> +#endif #include <linux/wait.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> @@ -1235,6 +1238,15 @@ static const struct of_device_id rk3x_i2c_match[] = { }; MODULE_DEVICE_TABLE(of, rk3x_i2c_match); +#ifdef CONFIG_ACPI +/* for RK3588 and at least when loaded with EDK2-RK3588 Tianocore firmware */ +static const struct acpi_device_id rk3x_i2c_acpi_match[] = { + { .id = "RKCP3001", .driver_data = (kernel_ulong_t)&rk3399_soc_data }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, rk3x_i2c_acpi_match); +#endif + static int rk3x_i2c_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -1243,6 +1255,10 @@ static int rk3x_i2c_probe(struct platform_device *pdev) int ret = 0; int bus_nr; u32 value; +#ifdef CONFIG_ACPI + u64 value64; + char clk_name[20]; +#endif int irq; unsigned long clk_rate; @@ -1250,8 +1266,12 @@ static int rk3x_i2c_probe(struct platform_device *pdev) if (!i2c) return -ENOMEM; - match = of_match_node(rk3x_i2c_match, np); - i2c->soc_data = match->data; + if (acpi_disabled) { + match = of_match_node(rk3x_i2c_match, np); + i2c->soc_data = match->data; + } else { + i2c->soc_data = device_get_match_data(&pdev->dev); + } /* use common interface to get I2C timing properties */ i2c_parse_fw_timings(&pdev->dev, &i2c->t, true); @@ -1266,6 +1286,9 @@ static int rk3x_i2c_probe(struct platform_device *pdev) i2c->dev = &pdev->dev; + if (!acpi_disabled) + ACPI_COMPANION_SET(&i2c->adap.dev, ACPI_COMPANION(&pdev->dev)); + spin_lock_init(&i2c->lock); init_waitqueue_head(&i2c->wait); @@ -1273,8 +1296,25 @@ static int rk3x_i2c_probe(struct platform_device *pdev) if (IS_ERR(i2c->regs)) return PTR_ERR(i2c->regs); - /* Try to set the I2C adapter number from dt */ - bus_nr = of_alias_get_id(np, "i2c"); + if (acpi_disabled) { + /* Try to set the I2C adapter number from dt */ + bus_nr = of_alias_get_id(np, "i2c"); + } else { + ret = acpi_dev_uid_to_integer(ACPI_COMPANION(&pdev->dev), + &value64); + if (ret) { + dev_err(&pdev->dev, "Cannot retrieve UID\n"); + return ret; + } + + if (value64 < INT_MAX) { + bus_nr = (int) value64; + } else { + /* shouldn't happen!!! */ + dev_err(&pdev->dev, "Too big UID\n"); + return -EINVAL; + } + } /* * Switch to new interface if the SoC also offers the old one. @@ -1325,13 +1365,58 @@ static int rk3x_i2c_probe(struct platform_device *pdev) platform_set_drvdata(pdev, i2c); - if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) { - /* Only one clock to use for bus clock and peripheral clock */ - i2c->clk = devm_clk_get(&pdev->dev, NULL); - i2c->pclk = i2c->clk; + if (acpi_disabled) { + if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) { + /* Only one clock to use for bus clock and peripheral + * clock + */ + i2c->clk = devm_clk_get(&pdev->dev, NULL); + i2c->pclk = i2c->clk; + } else { + i2c->clk = devm_clk_get(&pdev->dev, "i2c"); + i2c->pclk = devm_clk_get(&pdev->dev, "pclk"); + } } else { - i2c->clk = devm_clk_get(&pdev->dev, "i2c"); - i2c->pclk = devm_clk_get(&pdev->dev, "pclk"); + if (i2c->soc_data->calc_timings != rk3x_i2c_v0_calc_timings) { + u32 bclkrate = 0; + u32 pclkrate = 0; + + device_property_read_u32(&pdev->dev, "rockchip,bclk", + &bclkrate); + device_property_read_u32(&pdev->dev, "i2c,clk-rate", + &pclkrate); + + if (bclkrate != 0) { + sprintf(clk_name, "rockchip,i2c-clk.%d", bus_nr); + i2c->clk = clk_register_fixed_rate(&pdev->dev, + clk_name, + NULL, + 0, bclkrate); + dev_dbg(&pdev->dev, + "registered i2c clk at %u Hz\n", + bclkrate); + } + + if (pclkrate != 0) { + sprintf(clk_name, "rockchip,i2c-pclk.%d", + bus_nr); + i2c->pclk = clk_register_fixed_rate(&pdev->dev, + clk_name, + NULL, 0, + pclkrate); + dev_dbg(&pdev->dev, + "registered i2c pclk at %u Hz\n", + pclkrate); + } + } else { + /* NB: currently not expected w/UEFI firmware given + * these are not super performant RK3xxx + */ + dev_err(&pdev->dev, + "ACPI not supported for this RK3xxx\n"); + + return -EINVAL; + } } if (IS_ERR(i2c->clk))
Hi! I have been contributing to a firmware project for Rockchip RK3588- based board using the TianoCore EDK2 firmware development kit, at https://github.com/edk2-porting/edk2-rk3588. I am seeking to get it possible to seamlessly boot mainline kernels using it on these platforms, especially given how that mainline support for this platform has progressed far since its debut. Since this is not a firmware from a board vendor, I'm actually not really sure how to best interface or integrate its development with the Linux kernel community. Particularly the question of Device Tree Blob (DTB) vs. ACPI binding for configuration bindings, for which previous discussions have always seemed to center around situations where the firmware was not controllable by the kernel developer, but that is not the case with this situation here and now. However, I've found, particularly given the firmware has an HDMI driver that the currnet kernel does not, that I am able to get a quicker path to mainline boot on this platform going through the ACPI route, which inspires the following patch. In particular, I found one small issue that obstructs immediate direct booting of the off-the-shelf downloaded 6.8.x kernel series on my platform (Firefly ITX-3588J, with no mainline device tree blob support) which is that the I2C bus lacks code to configure it in the ACPI boot mode. The firmware came with ACPI bindings for it, nonetheless, and so this patch acts to enable and document those bindings for future use. Signed-off-by: Shimrra Shai <shimmyshai00@gmail.com> --- Documentation/firmware-guide/acpi/dsd/soc/general.rst | 32 ++++++++ Documentation/firmware-guide/acpi/dsd/soc/soc-i2c.rst | 50 ++++++++++++ drivers/i2c/busses/i2c-rk3x.c | 105 +++++++++++++++++++++++--- 3 files changed, 177 insertions(+), 10 deletions(-)