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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240819-anthill-episode-9db783c3e200@spud>
Date: Mon, 19 Aug 2024 18:36:00 +0100
From: Conor Dooley <conor@...nel.org>
To: Detlev Casanova <detlev.casanova@...labora.com>
Cc: linux-kernel@...r.kernel.org,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Elaine Zhang <zhangqing@...k-chips.com>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, kernel@...labora.com,
	Sugar Zhang <sugar.zhang@...k-chips.com>
Subject: Re: [PATCH v5 1/2] dt-bindings: clock, reset: Add support for rk3576

On Mon, Aug 19, 2024 at 06:31:09PM +0100, Conor Dooley wrote:
> 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@...k-chips.com>
> > > > > > Signed-off-by: Sugar Zhang <sugar.zhang@...k-chips.com>
> > > > > > Signed-off-by: Detlev Casanova <detlev.casanova@...labora.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@...k-chips.com>
> > > > > > +  - Heiko Stuebner <heiko@...ech.de>
> > > > > > +  - Detlev Casanova <detlev.casanova@...labora.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 

To be clear, I'm don't particularly care in this case, given there's at
least code being shared - but pointless syscon phandles that could be
looked up by compatible are starting to annoy me in general.



Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)