Message-Id: <20240322155146.22755-1-shimmyshai00@gmail.com>
Date: Fri, 22 Mar 2024 10:51:46 -0500
From: Shimrra Shai <shimmyshai00@...il.com>
To: jonathan.cameron@...wei.com
Cc: heiko@...ech.de,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	linux-rockchip@...ts.infradead.org,
	max.schwarz@...ine.de,
	niyas.sait@...wei.com,
	shimmyshai00@...il.com
Subject: Re: Re: [PATCH 0/0] (proposed?) Add ACPI binding to Rockchip RK3xxx I2C bus

Jonathan Cameron <Jonathan.Cameron@...wei.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

Powered by blists - more mailing lists