[RFC,1/1] dt-bindings: clk: Mstar msc313 clkgen mux

Add a devicetree binding yaml for the clkgen muxes used
in various places in MStar/SigmaStar SoCs.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 .../clock/mstar,msc313-clkgen-mux.yaml        | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-clkgen-mux.yaml

Quoting Daniel Palmer (2021-02-12 03:16:49)
> Add a devicetree binding yaml for the clkgen muxes used
> in various places in MStar/SigmaStar SoCs.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---

Don't need a cover letter for a single patch.

>  .../clock/mstar,msc313-clkgen-mux.yaml        | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-clkgen-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/mstar,msc313-clkgen-mux.yaml b/Documentation/devicetree/bindings/clock/mstar,msc313-clkgen-mux.yaml
> new file mode 100644
> index 000000000000..7f2ff72a601f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mstar,msc313-clkgen-mux.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/mstar,msc313-clkgen-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar/Sigmastar MSC313 CLKGEN mux
> +
> +maintainers:
> +  - Daniel Palmer <daniel@thingy.jp>
> +
> +description: |
> +  The MStar/SigmaStar MSC313 and later ARMv7 chips has a number of
> +  "CLKGEN"s. Some in the pm area, some in the standard peripheral area
> +  and some in the "scaler" area. Inside of these CLKGENs there are
> +  muxes that either connect the output to an always on clock
> +  (deglitch) or one of a number of clocks that are selectable via
> +  a mux. Each of these muxes also includes a gate for the output.
> +  Most of these are in a nice block with multiple muxes in a single
> +  register. Some of them are embedded within blocks of unrelated
> +  registers like in the pm area. Some seem to be embedded in registers
> +  with unrelated bits. For this reason a syscon is used to access the
> +  registers.
> +
> +properties:
> +  compatible:
> +    const: mstar,msc313-clkgen-mux
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    description: |
> +      List of clock sources for this mux. If the mux has a deglitch
> +      bit the last entry should be the source of the deglitch clock.
> +
> +  offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset in the register map for the mux register (in bytes).
> +
> +  regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to the register map node.
> +
> +  mstar,gate:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Bit position for the gate bit.
> +
> +  mstar,deglitch:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Bit position of the deglitch bit for muxes that have one.
> +
> +  mstar,mux-shift:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Left shift value for the mux bits.
> +
> +  mstar,mux-width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: The width of the mux bits.
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - offset
> +  - regmap
> +  - mstar,gate
> +  - mstar,mux-shift
> +  - mstar,mux-width
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clkgen_mux_mspi0: clkgen_mux_mspi0 {
> +      compatible = "mstar,msc313-clkgen-mux";
> +      regmap = <&clkgen>;
> +      offset = <0xcc>;
> +      #clock-cells = <1>;
> +      mstar,gate = <0>;
> +      mstar,mux-shift = <2>;
> +      mstar,mux-width = <2>;

It looks like a node-per clk sort of binding which has been rejected
multiple times in the past. If the clks are spread across various
devices then it sounds like the mediatek design where they have many
syscon nodes that also register clks inside those register spaces. In
this case, I would expect the clkgen node to be registering clks. Given
that there isn't a reg property and there's these mstar specific
properties like shift/width it looks really wrong. Please don't do this.

> +      clocks = <&clk_mpll_div2_div2>,
> +               <&clk_mpll_div2_div4>,
> +               <&xtal12>;
> +    };
> --

Hi Stephen,

On Sat, 13 Feb 2021 at 09:11, Stephen Boyd <sboyd@kernel.org> wrote:
> > +examples:
> > +  - |
> > +    clkgen_mux_mspi0: clkgen_mux_mspi0 {
> > +      compatible = "mstar,msc313-clkgen-mux";
> > +      regmap = <&clkgen>;
> > +      offset = <0xcc>;
> > +      #clock-cells = <1>;
> > +      mstar,gate = <0>;
> > +      mstar,mux-shift = <2>;
> > +      mstar,mux-width = <2>;
>
> It looks like a node-per clk sort of binding which has been rejected
> multiple times in the past. If the clks are spread across various
> devices then it sounds like the mediatek design where they have many
> syscon nodes that also register clks inside those register spaces. In
> this case, I would expect the clkgen node to be registering clks. Given
> that there isn't a reg property and there's these mstar specific
> properties like shift/width it looks really wrong. Please don't do this.

Ok. I will rethink this. One of the problems I face here is that there
isn't any documentation for what the clkgen looks like.
I have a list of offsets and bit positions for these muxes but very little else.
Looking at the mediatek clock drivers it seems like they have a driver
that consumes some register areas and then creates all of the muxes
etc within those areas within the driver instead. If that's an
acceptable solution I will go for that.
There would probably be 2 compatible strings right now (one for the pm
area and one for the normal area) and that would take a phandle to the
syscon that holds the registers. Then there would be a big table of
the offsets, masks etc in the driver.

Thanks,

Daniel

On Sat, Feb 13, 2021 at 10:18:14AM +0900, Daniel Palmer wrote:
> Hi Stephen,
> 
> On Sat, 13 Feb 2021 at 09:11, Stephen Boyd <sboyd@kernel.org> wrote:
> > > +examples:
> > > +  - |
> > > +    clkgen_mux_mspi0: clkgen_mux_mspi0 {
> > > +      compatible = "mstar,msc313-clkgen-mux";
> > > +      regmap = <&clkgen>;
> > > +      offset = <0xcc>;
> > > +      #clock-cells = <1>;
> > > +      mstar,gate = <0>;
> > > +      mstar,mux-shift = <2>;
> > > +      mstar,mux-width = <2>;
> >
> > It looks like a node-per clk sort of binding which has been rejected
> > multiple times in the past. If the clks are spread across various
> > devices then it sounds like the mediatek design where they have many
> > syscon nodes that also register clks inside those register spaces. In
> > this case, I would expect the clkgen node to be registering clks. Given
> > that there isn't a reg property and there's these mstar specific
> > properties like shift/width it looks really wrong. Please don't do this.
> 
> Ok. I will rethink this. One of the problems I face here is that there
> isn't any documentation for what the clkgen looks like.

All the more reason to not do a node per clock.

> I have a list of offsets and bit positions for these muxes but very little else.
> Looking at the mediatek clock drivers it seems like they have a driver
> that consumes some register areas and then creates all of the muxes
> etc within those areas within the driver instead. If that's an
> acceptable solution I will go for that.
> There would probably be 2 compatible strings right now (one for the pm
> area and one for the normal area) and that would take a phandle to the
> syscon that holds the registers. Then there would be a big table of
> the offsets, masks etc in the driver.

Ideally, the 'syscon' is just the clock provider or a child node is.

Rob