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:   Tue, 29 Nov 2022 16:29:49 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Luca Weiss <luca.weiss@fairphone.com>
Cc:     linux-arm-msm@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Vinod Koul <vkoul@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 1/3] dt-bindings: phy: qcom,qmp-usb3-dp: Add
 sm6350 compatible
Message-ID: <Y4YlbWL5uDXElBbi@hovoldconsulting.com>
References: <20221125092749.46073-1-luca.weiss@fairphone.com>
 <Y4CP9fwhDXsLu57Q@hovoldconsulting.com>
 <COLABNHBQ1DG.1PB8SDY3FW1YY@otso>
 <Y4CWoT52Q8jnm/dF@hovoldconsulting.com>
 <COLE3UWQCQ8R.XY36EY07DDDK@otso>
 <Y4DImjwHQNlWPEKh@hovoldconsulting.com>
 <COLFSBYDJQVH.1Q9V9ISPWR6ZX@otso>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <COLFSBYDJQVH.1Q9V9ISPWR6ZX@otso>
Precedence: bulk

On Fri, Nov 25, 2022 at 03:12:24PM +0100, Luca Weiss wrote:
> On Fri Nov 25, 2022 at 2:52 PM CET, Johan Hovold wrote:
> > On Fri, Nov 25, 2022 at 01:53:25PM +0100, Luca Weiss wrote:
> > > > Parent clocks (ref_clk_src) should not be included in the binding, but
> > > > rather be handled by the clock driver. For example, see:
> > > >
> > > > 	https://lore.kernel.org/all/20221121085058.31213-4-johan+linaro@kernel.org/
> > > > 	https://lore.kernel.org/all/20221115152956.21677-1-quic_shazhuss@quicinc.com/
> > > 
> > > So I assume you mean that I shouldn't do this:
> > > 
> > > clocks = <&gcc GCC_USB3_PRIM_PHY_AUX_CLK>,
> > >      <&rpmhcc RPMH_QLINK_CLK>,
> > >      <&gcc GCC_USB3_PRIM_PHY_COM_AUX_CLK>,
> > >      <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
> > > clock-names = "aux", "ref", "com_aux", "usb3_pipe";
> > > 
> > > But for "ref" use GCC_USB3_PRIM_CLKREF_CLK? That also seems to work
> > > fine, also if RPMH_QLINK_CLK is not used from Linux-side (checked in
> > > debugfs).
> >
> > Exactly. Since the vendor dts describes RPMH_QLINK_CLK as parent of ref,
> > I'd suggest modelling that in the clock driver. Perhaps it has just been
> > left on by the boot firmware. Someone with access to docs may be able
> > explain how it is supposed to be used.
> 
> RPMH_QLINK_CLK is also in msm-4.19 ref_clk_src for
> GCC_UFS_MEM_CLKREF_CLK (ufsphy_mem) and also ref_clk (ufshc_mem).
> 
> Honestly since it works fine without adding this to gcc driver and I
> don't really know much about clk (and have no docs for this) would it be
> okay to just ignore RPMH_QLINK_CLK?

Preferably it should be fixed now as it may be harder to figure out
what's missing in case this causes trouble in some setup later.

But, yeah, the lack of documentation is a pain.

Hopefully Bjorn or Vinod can help out with getting this sorted properly.

> > > And for the driver patch, I've discovered that this phy doesn't have
> > > separate txa/tbx region, so dts was also wrong there. Do you know if
> > > there's a way to test DP phy initialization without having all the USB-C
> > > plumbing in place? Might be good to validate at least phy init works if
> > > we're already touching all of this.
> >
> > Do you mean that it appears to work as sc8280xp with txa/txb shared by
> > both the USB and DP parts?
> 
> Yes, looks like it. Can't find any evidence pointing in any other
> direction at least, everything I've seen shows .txa = 0x1200 & .txb =
> 0x1600.

Ok. I've also only seen indirect references to the DP registers
for the older platforms, but at least of them do have the separate DP TX
regions.

> > I guess you need a proper setup to test it properly. Not sure what
> > you'll be able to learn otherwise, apart from whether it passes basic
> > smoke testing.
> 
> Currently it's not even smoke testing because dp phy is never getting
> enabled because there's no consumer. That's why I guess it was never
> noticed it's wrongly described in dts.

Yeah, people shouldn't be adding (copy-pasted) nodes for peripherals
that they are not able to test (especially given the lack of
documentation), but I guess the USB3-DP case is a bit of a grey area as
the USB part can have been verified. Fortunately, this should be less of
any issue with the new binding scheme.

Johan