Use proper ACPI clock frequency binding ("ClockInput") for clock frequency settings

[shimmyshai00]

I've been discussing this over with the Linux kernel team, about tweaking the Rockchip kernel drivers (I2C, Ethernet) for ACPI use:

https://lore.kernel.org/linux-kernel/[email protected]/T/#u

In the course of discussion, they offered some pointers there regarding how that the tables should be built, in particular using a "ClockInput" macro binding to handle clock speeds, which was introduced in ACPI 6.5, as opposed to going through the _DSD. This patch proposes such a new table for the I2C, moving the two clock frequencies to the new macro, and also adds a table for the CRU (Clock and Reset Unit) as this is needed to describe the peripheral clocks, just as in the .DTB.

[mariobalanica]

I'm reluctant to merge this until there's some form of support in any OS and the spec gets more clear about how ClockInput is meant to be used. Right now it leaves most implementation details up to personal interpretation, this isn't any better than a custom method.

One critical aspect is that many clocks need to be adjusted at runtime (for audio, display, PHYs, etc.), how would that be accomplished here?
For instance, I used a _DSM method for I2S, GMAC and eMMC clocks in Windows. Not ideal, but it's simple enough and can be supported anywhere with minimal driver changes, while ClockInput also needs significant ACPI kernel changes beyond our control, at least in Windows.

Btw, there's also SCMI. If Linux can be fixed to properly locate clocks from _DSD in the ACPI namespace, then this also becomes an option (although Arm-specific).

[mariobalanica]

Why 0x1 here? It's the first PHY on this bus.

[mariobalanica]

_ADR is meant to be used for buses that have their own enumeration mechanism, this is not the case here. I guess it should have _HID and _UID instead?

[mariobalanica]

This is used by the Windows driver, it shouldn't be removed.

[shimmyshai00]

Aha. This is a good reason I would/should tell the Linux kernel team about, then. They were the ones who proposed I should use the ClockInput binding, but if this was not simply an arbitrary choice on the part of this firmware development team then I will have to let them know about it and I will advocate they accept the first set of patches I sent them which uses those settings as-is and we can cancel this pull request.

[mariobalanica]

Those bindings were added arbitrarily just to get the Windows I2C driver working, without taking into consideration other OSes and future requirements. That driver can be tweaked to read the clock rate differently if there's a better (more standard) way, but ClockInput is currently a no-go for two reasons:

1. The Windows ACPI bus driver does not natively support it.
2. It references CLK objects outside the I2C device's scope. Windows drivers are only allowed to access the scope of the devices they're managing, so at this point we really depend on 1).

Linux has a clock framework and that I2C driver uses it, ideally we should settle on some method that can be integrated into this framework and also allows the clock to be controlled (on/off, set rate), so there won't be any need to patch each and every driver.

The _DSD approach above is only going to work for devices that have one fixed clock, so not many.

Since ClockInput is a mystery, you could suggest using SCMI as I previously mentioned. Essentially there will be a SCMI "PRP0001" device that exposes a bunch of clocks, just like in a device tree, then other devices can reference these clocks by their object path in the namespace (rather than phandles).

Trying to expose a "fixed-clock" device in this manner currently causes Linux to panic. I recall it trying to dereference a null "name"-something string. I imagine this would be the device name (e.g. CLK0) that it doesn't know how to get yet.

[shimmyshai00]

OK, so you are suggesting to deploy both options in the ACPI, then? I.e. use both the Windows boot options and the Linux ClockInput or SCMI options, given replacement by either of the latter will break Windows? Also, I am not sure what you mean by "leaves the implementation details up to personal interpretation" - each setting on a ClockInput binding is described at the very end of the official UEFI documentation here:

https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html

I don't see a lot of room for interpretation, as to why it'd be a "mystery". Though, last two options were admittedly a bit hard to understand, but after reading the kernel docs for the Device Tree options they sound pretty much like a usual DT "clock = <&clock_device NUMBER>" binding, i.e. the first part references the device node and the second the number for the clock source. In the .DTB those all reference to the CRU, hence why I have given the setup as here. And I am less familiar still with how to use SCMI, with no idea if and what "standards" (incl. informal) there are for constructing a correct ACPI binding for it. The kernel developers also suggested to me they were desiring to add code that would support the ClockInput binding, and even provided me a stub code to be expanded to handle variable clocks and the like. This code would in effect bind to the binding in the same way that it would to a DT clock entry and thus interface with the Common Clock Framework.

[mariobalanica]

Also, I am not sure what you mean by "leaves the implementation details up to personal interpretation" - each setting on a ClockInput binding is described at the very end of the official UEFI documentation here:

From the spec:

ResourceSource is a string which uniquely identifies the Device which is the source of the clock signal described by this Resource. If FixedMode is Variable, this argument is required. ResourceSource can be a fully-qualified name, a relative name or a name segment that utilizes the namespace search rules.

What is this device supposed to look like? How does one describe it such that the OS is able to turn the clock on/off or adjust its frequency, for instance?

And I am less familiar still with how to use SCMI, with no idea if and what "standards" (incl. informal) there are for constructing a correct ACPI binding for it.

There isn't any standard binding for it. What I suggested was to patch Linux so that it can accept clock devices with properties encoded in _DSD, similar to how they're described in a device tree.

SCMI is a firmware mechanism that provides extensive support for managing platform clocks, resets, etc. Instead of having a custom clock driver in every OS, you can have a generic SCMI driver (like Linux does) and let the firmware abstract all platform-specific bits. See how CPU clocks are described in rk3588.dtsi for an example.

The kernel developers also suggested to me they were desiring to add code that would support the ClockInput binding, and even provided me a stub code to be expanded to handle variable clocks and the like. This code would in effect bind to the binding in the same way that it would to a DT clock entry and thus interface with the Common Clock Framework.

Do you have a link to the variable clock implementation? I'd like to take a look.

[shimmyshai00]

OK, thanks and yes, that's a good point about the ambiguity as to what the linked device structure should look like. I posted to them essentially what I was trying to send here, i.e. how I imagined the CRU binding to look, but they don't seem to have replied with any comments to me about whether it was in line with what they were thinking or not. Also, as I said, the variable part is still to be added - what was provided me was a stub doing the fixed implementation only. I suggested I might be able to add the variable later:

https://lore.kernel.org/linux-kernel/[email protected]/

There is a linked patch. Nonetheless one can see it tries to adapt the ACPI binding to the Common Clock Framework that is the same as employed for Device Tree, using clk_register_fixed_rate to register the fixed-rate clock.

[mariobalanica]

Does this patch have a Linux counterpart that effectively makes I2C and GMAC work, or is it merely a concept of what the ACPI bindings could look like?

If it's just theoretical and hasn't actually been validated with any OS, then there isn't much point in merging it, since it doesn't add any value or fix anything. CI build also fails, perhaps due to the iASL version?

We can take the discussion to a separate issue here, or maybe the mailing list? Though I won't be able to help much with the Linux side of this, I'm mostly involved with Windows.

[shimmyshai00]

Does this patch have a Linux counterpart that effectively makes I2C and GMAC work, or is it merely a concept of what the ACPI bindings could look like?

If it's just theoretical and hasn't actually been validated with any OS, then there isn't much point in merging it, since it doesn't add any value or fix anything. CI build also fails, perhaps due to the iASL version?

We can take the discussion to a separate issue here, or maybe the mailing list? Though I won't be able to help much with the Linux side of this, I'm mostly involved with Windows.

That's essentially the whole rub - the convo I just linked you to and have referenced here is precisely around the development of such a patch. That's the trick. In terms of how you're seeing it, neither can come "first" - it's a chicken-and-egg problem. The OS code cannot be patched until the clock situation in the FW is sorted, and the clock situation cannot be sorted until something is accepted here that they will like.

FWIW the full thread begins at:

https://lore.kernel.org/linux-kernel/[email protected]/

Note how I mentioned this situation, that it's kind of unusual because "we" (i.e. in that both are open source and modifiable) have control over both the firmware and the kernel at the same time, which is not the situation the kernel developers typically handle in this case which is either that they are trying to support some opaque board's ACPI table, or else are using .DTB.

[mariobalanica]

In terms of how you're seeing it, neither can come "first" - it's a chicken-and-egg problem. The OS code cannot be patched until the clock situation in the FW is sorted, and the clock situation cannot be sorted until something is accepted here that they will like.

Sure, I was wondering if you also have some corresponding WIP patches for Linux to demonstrate the concept.

This needs to happen in parallel so you can have a better view of how the ACPI and OS code will go together, neither can be done exactly first.

Things like these are better discussed as an RFC or issue IMO; pull request essentially means that you have some code you wish to be merged. Maybe it can be marked as draft, but I believe an issue would be better until we have a good idea of what the bindings would look like, and some proof-of-concept in Linux and possibly other OSes.

[shimmyshai00]

Sure, I was wondering if you also have some corresponding WIP patches for Linux to demonstrate the concept.

This needs to happen in parallel so you can have a better view of how the ACPI and OS code will go together, neither can be done exactly first.

Thanks, yeah I should be able to prepare something like that, actually. I.e. with the ClockInput binding in particular given that that is what seems to be being "pushed" over at the kernel house.

Things like these are better discussed as an RFC or issue IMO; pull request essentially means that you have some code you wish to be merged. Maybe it can be marked as draft, but I believe an issue would be better until we have a good idea of what the bindings would look like, and some proof-of-concept in Linux and possibly other OSes.

Sure, I could open one, esp. if I can get something working in terms of code for the kernel side too since I already had a draft patch for using the Windows clock settings and as I said an initial patch was given me for adding the ClockInput binding support to the kernel, I should be able to prepare something to at least proof-of-concept the two of them together. Not sure how to close the pull request though.

[mariobalanica]

I came across this some time ago: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources

At first glance, option 2 seems good since it's generic and abstracts the platform's clock control mechanism (could be MMIO, SCMI or some other transport).

Here's a potential description for the eMMC bus clock:

Spoiler

Scope (\_SB) {
  Device (RCRU) {
    Name (_HID, "ACPI0004")
    Name (_UID, 0x0)
    
    Method (_CRS, 0x0, Serialized) {
      Name (RBUF, ResourceTemplate() {
        Memory32Fixed (ReadWrite, 0xfd7c0000, 0x5c000)
      })
      Return (RBUF)
    }

    // eMMC Core Clock
    Device (EMCC) { 
      // ACPI Clock Device (ID NOT yet assigned)
      Name (_HID, "ACPI####") 
      Name (_UID, 0x0)

      OperationRegion (OCRU, SystemMemory, 0xFD7C0434, 0x4)
      Field (OCRU, DWordAcc, NoLock, WriteAsZeros) {
        C77, 32,
      }

      // Get Clock Rate (Hz)
      Method (_GCR)
      {     
        Local0 = (C77 >> 14) & 0x3 // cclk_emmc_sel
        Local1 = (C77 >> 8) & 0x3f // cclk_emmc_div

        Switch (Local0) {
          Case (0) { // clk_gpll_mux
            Return (Local1 * 1200000000)
          }
          Case (2) { // xin_osc0_func
            Return (Local1 * 24000000)
          }
        }
        // 0 = failure
        Return (0)
      }

      // Set Clock Rate (Hz)
      Method (_SCR, 1)
      {
        Local0 = ToInteger (Arg0)
        
        If (Local0 >= 200000000) {
          C77 = 0xFF000500
          Return (200000000)
        }
        If (Local0 >= 150000000) {
          C77 = 0xFF000700
          Return (150000000)
        }
        If (Local0 >= 100000000) {
          C77 = 0xFF000B00
          Return (100000000)
        }
        If (Local0 >= 50000000) {
          C77 = 0xFF001700
          Return (50000000)
        }
        If (Local0 >= 24000000) {
          C77 = 0xFF008000
          Return (24000000)
        }
        If (Local0 >= 375000) {
          C77 = 0xFF00BF00
          Return (375000)
        }
        // 0 = failure (unless Arg0 is 0)
        Return (0)
      }

      // Get Clock State
      Method (_GCS)
      {
        // 0 = disabled or failure
        // 1 = enabled
        Return (1)
      }

      // Set Clock State
      Method (_SCS, 1)
      {
        // On success, matches the set state.
        // Otherwise assume failure.
        Return (ToInteger (Arg0))
      }
    }
  }

  Device (SDC3) {
    Name (_HID, "RKCP0D40")
    Name (_UID, 3)
    Name (_CCA, 0)

    Method (_CRS, 0x0, Serialized) {
      Name (RBUF, ResourceTemplate() {
        Memory32Fixed (ReadWrite, 0xfe2e0000, 0x10000)
        Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 237 }
        ClockInput (200000000, 1, Hz, Variable, "\\_SB.RCRU.EMCC", 0)
      })
      Return (RBUF)
    }

    Name (_DSD, Package () {
      ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
      Package () {
        Package () { "compatible", "rockchip,rk3588-dwcmshc" },
        Package () { "max-frequency", 200000000 },
        Package () { "bus-width", 8 },
        Package () { "no-sd", 0x1 },
        Package () { "no-sdio", 0x1 },
        Package () { "mmc-hs400-1_8v", 0x1 },
        Package () { "mmc-hs400-enhanced-strobe", 0x1 },
        Package () { "non-removable", 0x1 },
        // Friendly name for the ClockInput resources
        Package () { "clock-names", Package () { "core" } }
      }
    })

    //
    // A child device that represents the non-removable eMMC.
    //
    Device (SDMM)
    {
      Method (_ADR)
      {
        Return (0)
      }
      Method (_RMV) // Is removable
      {
        Return (0) // 0 - fixed
      }
    }
  }
}

But the ResourceSourceIndex argument of ClockInput kinda implies that CRU should be the only clock device, with multiple outputs. This device could then have a method along the lines of: ClockOutput (ResourceSourceIndex, Action, ActionArgumentsPackage) that the OS would call. Failure/success reporting could also be done better, by returning a package.

Clocks also have parent-child relationships, yet another area the spec doesn't cover...

Either way, I'm not sure this is the way to go. Controlling clocks in ASL can get pretty messy (CRU is humongous!) and the language itself is quite limited (only 8 local vars with fixed name, 4 char object names, etc.). It's a maintenance nightmare.

SCMI tackles the problem in a waaaay more elegant way and already has good support in Linux. It just needs to bind to ACPI somehow (_DSD?) and that's about it...