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]

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...