Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 22 Apr 2022 20:43:18 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Stephen Boyd <sboyd@kernel.org>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Rob Herring <robh+dt@kernel.org>,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        quic_tdas@quicinc.com
Subject: Re: [PATCH v2 1/2] dt-bindings: clock: Add Qualcomm SC8280XP GCC
 bindings
Message-ID: <YmN11qt/PqogYruQ@ripper>
References: <20220422230013.1332993-1-bjorn.andersson@linaro.org>
 <20220423014824.912ACC385A0@smtp.kernel.org>
 <YmNsYSxLtwLpw98t@ripper>
 <20220423031350.01299C385A0@smtp.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220423031350.01299C385A0@smtp.kernel.org>
Precedence: bulk

On Fri 22 Apr 20:13 PDT 2022, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2022-04-22 20:02:57)
> > On Fri 22 Apr 18:48 PDT 2022, Stephen Boyd wrote:
> > 
> > > Quoting Bjorn Andersson (2022-04-22 16:00:12)
> > > > Add binding for the Qualcomm SC8280XP Global Clock controller.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > 
> > > Why no cover letter?
> > > 
> > 
> > I didn't have anything useful to write in it. Will provide you one in
> > the future...
> 
> Thanks!
> 
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: XO reference clock
> > > 
> > > "clock" is redundant in all these descriptions. Please remove.
> > > 
> > 
> > You don't think it's a little bit odd to have description such as
> > "Sleep", "PCIe 2a pipe" or First EMAC controller reference"?
> > 
> > I mean I agree that it's obviously clocks we're talking about, but to me
> > that makes it seems like the descriptions are cut short, just for the
> > sake of avoiding "clock".
> 
> Alright, keeping clock is OK as long as
> 
> > 
> > > > +      - description: Sleep clock
> > > > +      - description: UFS memory first RX symbol clock
> > > > +      - description: UFS memory second RX symbol clock
> > > > +      - description: UFS memory first TX symbol clock
> > > > +      - description: UFS card first RX symbol clock
> > > > +      - description: UFS card second RX symbol clock
> > > > +      - description: UFS card first TX symbol clock
> > > > +      - description: Primary USB SuperSpeed pipe clock
> > > > +      - description: gcc_usb4_phy_pipegmux_clk_src
> 
> there is a better name for this and the other non-word descriptions.
> 
> USB4 phy pipe gmux clock source?
> 

Sounds good, I'll make sure to fill these out.

> > > > +      - description: gcc_usb4_phy_dp_gmux_clk_src
> > > > +      - description: gcc_usb4_phy_sys_pipegmux_clk_src
> > > > +      - description: usb4_phy_gcc_usb4_pcie_pipe_clk
> > > > +      - description: usb4_phy_gcc_usb4rtr_max_pipe_clk
> > > > +      - description: Primary USB4 RX0 clock
> > > > +      - description: Primary USB4 RX1 clock
> > > > +      - description: Secondary USB SuperSpeed pipe clock
> > > > +      - description: gcc_usb4_1_phy_pipegmux_clk_src
> > > > +      - description: gcc_usb4_1_phy_dp_gmux_clk_src
> > > > +      - description: gcc_usb4_1_phy_sys_pipegmux_clk_src
> > > > +      - description: usb4_1_phy_gcc_usb4_pcie_pipe_clk
> > > > +      - description: usb4_1_phy_gcc_usb4rtr_max_pipe_clk
> > > > +      - description: Secondary USB4 RX0 clock
> > > > +      - description: Secondary USB4 RX0 clock
> > > > +      - description: Multiport USB first SupserSpeed pipe clock
> > > > +      - description: Multiport USB second SuperSpeed pipe clock
> > > > +      - description: PCIe 2a pipe clock
> > > > +      - description: PCIe 2b pipe clock
> > > > +      - description: PCIe 3a pipe clock
> > > > +      - description: PCIe 3b pipe clock
> > > > +      - description: PCIe 4 pipe clock
> > > > +      - description: First EMAC controller reference clock
> > > > +      - description: Second EMAC controller reference clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: bi_tcxo
> > > > +      - const: sleep_clk
> > > 
> > > And "_clk" postfix is redundant in all these strings. Remove?
> > > 
> > 
> > In this case I think they should include _clk, as they actually matches
> > the clock names in the documentation.
> > 
> 
> I'd really rather not have clock-names at all because we spend a bunch
> of time comparing strings with them when we could just as easily use
> a number.

I know that you would like to get rid of the clock-names for the clock
controllers. I've looked at it since and while it will be faster to
execute I still feel that it's going to be harder to write and maintain.

E.g. look at gcc_pcie_4_pipe_clk_src, its parents today are
pcie_4_pipe_clk and bi_tcxo. Something I can reason about being correct
or not.

If we ditch the clock-names I will have:

static const struct clk_parent_data gcc_parent_data_14[] = {
        { .index = 30 },
        { .index = 0 },
};

Generally we would perhaps use some compile time constant, but that
won't work here because we're talking about the index in the clocks
array in the yaml.


But perhaps I'm missing something that would make this manageable?

Regards,
Bjorn