On Sun, 17 Nov 2024 at 13:51, Krishna Kurapati
<quic_kriskura@xxxxxxxxxxx> wrote:
>
>
>
> On 11/15/2024 9:29 PM, Dmitry Baryshkov wrote:
> > On Fri, Nov 15, 2024 at 02:45:45PM +0530, Krishna Kurapati wrote:
> >> Commit [1] introduced DP support to QMP driver. While doing so, the
> >> dp and usb configuration structures were added to a combo_phy_cfg
> >> structure. During probe, the match data is used to parse and identify the
> >> dp and usb configs separately. While doing so, the usb_cfg variable
> >> represents the configuration parameters for USB part of the phy (whether
> >> it is DP-Cobo or Uni). during probe, one corner case of parsing usb_cfg
> >> for Uni PHYs is left incomplete and it is left as NULL. This NULL variable
> >> further percolates down to qmp_phy_create() call essentially getting
> >> de-referenced and causing a crash.
> >
> > The UNI PHY platforms don't have usb3-phy subnode. As such the usb_cfg
> > variable should not be used in the for_each_available_child_of_node()
> > loop.
> >
> > Please provide details for the platform on which you observe the crash
> > and the backtrace.
> >
>
> I got this error when I started working on multiport support (begining
> of 2023). Initially I tried testing on my code on 5.15, hence this patch
> was raised on the same.
>
> The 2 qmp phys on sa8195 and sa8295 (based on sc8280xp) are uni phy and
> the following was the DT node that worked out for me on 5.15 codebase:
>
>
> usb_1_qmpphy: ssphy@88eb000 {
> compatible = "qcom,sm8150-qmp-usb3-uni-phy";
> reg = <0x088eb000 0x200>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> //status = "disabled";
> clocks = <&gcc GCC_USB3_MP_PHY_AUX_CLK>,
> <&rpmhcc RPMH_CXO_CLK>,
> <&gcc GCC_USB3_SEC_CLKREF_CLK>,
> <&gcc GCC_USB3_MP_PHY_COM_AUX_CLK>;
> clock-names = "aux", "ref_clk_src", "ref", "com_aux";
>
> resets = <&gcc GCC_USB3UNIPHY_PHY_MP0_BCR>,
> <&gcc GCC_USB3_UNIPHY_MP0_BCR>;
> reset-names = "phy", "common";
>
> //vdda-phy-supply = <&L3C>;
> vdda-pll-supply = <&L5E>;
>
> usb_1_ssphy: usb3-phy@88eb200 {
As this is a UNI PHY and not a combo PHY, the child node should be
just phy@, not usb3-phy@. See
Documentation/devicetree/bindings/phy/qcom,qmp-phy.yaml
> reg = <0x088eb200 0x200>,
> <0x088eb400 0x200>,
> <0x088eb800 0x800>,
> <0x088eb600 0x200>;
> #clock-cells = <0>;
> #phy-cells = <0>;
> clocks = <&gcc GCC_USB3_MP_PHY_PIPE_0_CLK>;
> clock-names = "pipe0";
> clock-output-names = "usb3_uni_phy_pipe_clk_src";
> };
> };
>
>
> I was hitting the bug when I write the DT above way on top of 5.15 baseline.
>
> In 5.15.y, the SM8150 usb_2_qmpphy dT is as follows:
>
> usb_2_qmpphy: phy@88eb000 {
> compatible = "qcom,sm8150-qmp-usb3-uni-phy";
> reg = <0 0x088eb000 0 0x200>;
> status = "disabled";
> #address-cells = <2>;
> #size-cells = <2>;
> ranges;
>
> clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
> <&rpmhcc RPMH_CXO_CLK>,
> <&gcc GCC_USB3_SEC_CLKREF_CLK>,
> <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
> clock-names = "aux", "ref_clk_src", "ref",
> "com_aux";
>
> resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
> <&gcc GCC_USB3_PHY_SEC_BCR>;
> reset-names = "phy", "common";
>
> usb_2_ssphy: phy@88eb200 {
Just as I wrote, this one correctly uses phy@
> reg = <0 0x088eb200 0 0x200>,
> <0 0x088eb400 0 0x200>,
> <0 0x088eb800 0 0x800>,
> <0 0x088eb600 0 0x200>;
> #clock-cells = <0>;
> #phy-cells = <0>;
> clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
> clock-names = "pipe0";
> clock-output-names =
> "usb3_uni_phy_pipe_clk_src";
> };
> };
>
> IIRC, when I tried using the above sm8150 dt on 5.15.y, the phy_create
> was (either not getting called) or crashing. Probably because
> "of_node_name_eq()" didn't find either "dp-phy" or "usb3-phy" and cfg
> variable was NULL.
Unless somebody backported some patch in an incorrect way, the SM8150
DT entry is correct, while SA8xxx is not,
>
> I can try reproducing the issue and get back again in a week.
Yes, please.
>
> Apologies if I have misunderstood something and this patch doesn't make
> sense. Let me know if I have made any mistake anywhere (either in my DT)
> or in understanding.
>
> Regards,
> Krishna,
--
With best wishes
Dmitry
