Re: [PATCH v5 1/2] dt-bindings: clock, reset: Add support for rk3576

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5779 bytes --]

On Mon, Aug 19, 2024 at 01:14:38PM -0400, Detlev Casanova wrote:
> On Monday, 19 August 2024 12:34:15 EDT Conor Dooley wrote:
> > On Mon, Aug 19, 2024 at 10:08:31AM -0400, Detlev Casanova wrote:
> > > Hi Conor,
> > > 
> > > On Thursday, 15 August 2024 11:07:46 EDT Conor Dooley wrote:
> > > > On Wed, Aug 14, 2024 at 06:19:22PM -0400, Detlev Casanova wrote:
> > > > > Add clock and reset ID defines for rk3576.
> > > > > 
> > > > > Compared to the downstream bindings written by Elaine, this uses
> > > > > continous gapless IDs starting at 0. Thus all numbers are
> > > > > different between downstream and upstream, but names are kept
> > > > > exactly the same.
> > > > > 
> > > > > Also add documentation for the rk3576 CRU core.
> > > > > 
> > > > > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> > > > > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> > > > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > > > > ---
> > > > > 
> > > > >  .../bindings/clock/rockchip,rk3576-cru.yaml   |  64 ++
> > > > >  .../dt-bindings/clock/rockchip,rk3576-cru.h   | 592
> > > > >  ++++++++++++++++++
> > > > >  .../dt-bindings/reset/rockchip,rk3576-cru.h   | 564 +++++++++++++++++
> > > > >  3 files changed, 1220 insertions(+)
> > > > >  create mode 100644
> > > > >  Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml
> > > > >  create
> > > > >  mode 100644 include/dt-bindings/clock/rockchip,rk3576-cru.h create
> > > > >  mode
> > > > >  100644 include/dt-bindings/reset/rockchip,rk3576-cru.h>
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml
> > > > > b/Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml new
> > > > > file mode 100644
> > > > > index 0000000000000..d69985e6fa0ce
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml
> > > > > @@ -0,0 +1,64 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/clock/rockchip,rk3576-cru.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Rockchip rk3576 Family Clock and Reset Control Module
> > > > > +
> > > > > +maintainers:
> > > > > +  - Elaine Zhang <zhangqing@rock-chips.com>
> > > > > +  - Heiko Stuebner <heiko@sntech.de>
> > > > > +  - Detlev Casanova <detlev.casanova@collabora.com>
> > > > > +
> > > > > +description:
> > > > > +  The RK3576 clock controller generates the clock and also implements
> > > > > a
> > > > > reset +  controller for SoC peripherals. For example it provides
> > > > > SCLK_UART2 and +  PCLK_UART2, as well as SRST_P_UART2 and SRST_S_UART2
> > > > > for the second UART +  module.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: rockchip,rk3576-cru
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  "#clock-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  "#reset-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  clocks:
> > > > > +    maxItems: 2
> > > > > +
> > > > > +  clock-names:
> > > > > +    items:
> > > > > +      - const: xin24m
> > > > > +      - const: xin32k
> > > > > +
> > > > > +  rockchip,grf:
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +    description: >
> > > > > +      phandle to the syscon managing the "general register files". It
> > > > > is
> > > > > used +      for GRF muxes, if missing any muxes present in the GRF
> > > > > will
> > > > > not be +      available.
> > > > 
> > > > Two questions on this property:
> > > > - you only support one soc, why is this optional?
> > > 
> > > It is optional because only used for some specific clocks. The SoC can
> > > still be used without this, but some devices might not work (Not tested
> > > but USB PHYs might not be working without the GRF)

I would not bother making them optional. I don't think there's any
benefit to doing so when there's only one instance of this controller
on the device, and the grfs will also always be present.

> > > This is also set as optional in similar rockchip CRU bindings (rk3588).
> > > 
> > > > - why can't you look it up by compatible?
> > > 
> > > These bindings are specific to one compatible only. It is very similar to
> > > rk3588 but it looks like all rockchip CRU driver has its own yaml file, so
> > > I followed that trend instead of merging with the rk3588 CRU bindings.
> > I don't think you've answered the question I am asking. Why can you not
> > look up the syscon by the /syscon/'s compatible in the clock driver, and
> > thus remove the property from here?
> 
> I don't think that this is possible.

Undesirable maybe, impossible no.

> That means modifying rockchip/clk.c to 
> lookup the syscon instead of using the `rockchip,grf` phandle. As it is used 
> by other rockchip clock drivers, that means that we'd need to change the 
> syscon's node name for some other SoCs to match what is being looked up.

No, you would not have to change anything for other SoCs, it would
definitely be possible to do compatible based lookups on some devices
and phandle on others, particularly given you're adding a new driver.

> But the GRF nodes have different compatibles:
> - rk3588 uses php_grf (rockchip,rk3588-php-grf)
> - rk3576 uses pmu0_grf (rockchip,rk3576-pmu0-grf)

Ditto this, the new clock driver that you're adding with knowledge of
the clock tree could also contain the new compatible for that soc.

> 
> So an optional rockchip,grf phandle sounds a good solution for this 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]