Message-ID: <20240325154150.00003bde@Huawei.com>
Date: Mon, 25 Mar 2024 15:41:50 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Shimrra Shai <shimmyshai00@...il.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>
Subject: Re: [PATCH 0/0] (proposed?) Add ACPI binding to Rockchip RK3xxx I2C
 bus

On Fri, 22 Mar 2024 10:51:46 -0500
Shimrra Shai <shimmyshai00@...il.com> wrote:

> 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?:

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
> 

Powered by blists - more mailing lists