linux-kernel - Re: [PATCH 13/15] phy: qcom-qmp-pcie: add support for pipediv2 clock

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y0692mW6MJneljrP@hovoldconsulting.com>
Date:   Tue, 18 Oct 2022 16:53:14 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/15] phy: qcom-qmp-pcie: add support for pipediv2 clock

On Tue, Oct 18, 2022 at 04:05:29PM +0300, Dmitry Baryshkov wrote:
> On 17/10/2022 17:53, Johan Hovold wrote:
> > Some QMP PHYs have a second fixed-divider pipe clock that needs to be
> > enabled along with the pipe clock.
> > 
> > Add support for an optional "pipediv2" clock.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@...nel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++----
> >   1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 9c8e009033f1..c1d74c06fad1 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -1379,7 +1379,9 @@ struct qmp_pcie {
> >   	void __iomem *rx2;
> >   
> >   	struct clk *pipe_clk;
> > +	struct clk *pipediv2_clk;
> >   	struct clk_bulk_data *clks;
> > +
> >   	struct reset_control_bulk_data *resets;
> >   	struct regulator_bulk_data *vregs;
> >   
> > @@ -1902,6 +1904,36 @@ static int qmp_pcie_exit(struct phy *phy)
> >   	return 0;
> >   }
> >   
> > +static int pipe_clk_enable(struct qmp_pcie *qmp)
> > +{
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(qmp->pipe_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(qmp->pipediv2_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret);
> > +		goto err_disable_pipe_clk;
> > +	}
> 
> Do they have to be enabled in sequence? If not, I'd use a bulk clocks 
> here for the pipe clocks. While it can look like an overkill, it would 
> be a safe net for the possible future changes, which might include 
> additional clocks.

I don't believe the bulk API is a good fit here as we need to support
both the new and old bindings, and for the latter the pipe_clk is looked
up by index rather than name (and that's from the child node too which
limits which APIs you can use further).

The code is clear enough as it stands, and I don't think we need to take
height for a hypothetical third pipe clock just yet.

Johan