Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220607213543.4057620-1-bjorn.andersson@linaro.org>
 <20220607213543.4057620-4-bjorn.andersson@linaro.org> <d9658f54-e594-8f0e-071e-ef627285d281@linaro.org>
 <Yp/ZkxNltUgE79nC@ripper>
In-Reply-To: <Yp/ZkxNltUgE79nC@ripper>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Wed, 8 Jun 2022 02:28:54 +0300
Message-ID: <CAA8EJppTa88Rbo5R1SbycfvpPMAB18DXPd_iGJ_M+swE9KBv2Q@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] phy: qcom-qmp: Add USB4 5NM QMP combo PHY registers
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        Manu Gautam <mgautam@codeaurora.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 8 Jun 2022 at 02:02, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Tue 07 Jun 15:24 PDT 2022, Dmitry Baryshkov wrote:
>
> > On 08/06/2022 00:35, Bjorn Andersson wrote:
> > > Add all registers defines from qcom,usb4-5nm-qmp-combo.h of the msm-5.4
> > > kernel. Offsets are adjusted to be relative to each sub-block, as we
> > > describe the individual pieces in the upstream kernel and "v5_5NM" are
> > > injected in the defines to not collide with existing constants.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >
> > > Changes since v1:
> > > - New patch
> > >
> > >   .../qualcomm/phy-qcom-usb4-5nm-qmp-combo.h    | 1547 +++++++++++++++++
> > >   1 file changed, 1547 insertions(+)
> > >   create mode 100644 drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h b/drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h
> > > new file mode 100644
> > > index 000000000000..7be8a50269ec
> > > --- /dev/null
> > > +++ b/drivers/phy/qualcomm/phy-qcom-usb4-5nm-qmp-combo.h
> > > @@ -0,0 +1,1547 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef PHY_QCOM_V5_5NM_QMP_COMBO_USB4_H
> > > +#define PHY_QCOM_V5_5NM_QMP_COMBO_USB4_H
> > > +
> > > +/* USB4-USB3-DP Combo PHY register offsets */
> > > +/* Module: USB43DP_COM_USB43DP_COM_USB4_USB3_DP_COM */
> > > +#define USB43DP_V5_5NM_COM_PHY_MODE_CTRL                           0x00
> > > +#define USB43DP_V5_5NM_COM_SW_RESET                                        0x04
> > > +#define USB43DP_V5_5NM_COM_POWER_DOWN_CTRL                         0x08
> > > +#define USB43DP_V5_5NM_COM_SWI_CTRL                                        0x0c
> > > +#define USB43DP_V5_5NM_COM_TYPEC_CTRL                                      0x10
> > > +#define USB43DP_V5_5NM_COM_TYPEC_PWRDN_CTRL                                0x14
> > > +#define USB43DP_V5_5NM_COM_DP_BIST_CFG_0                           0x18
> > > +#define USB43DP_V5_5NM_COM_RESET_OVRD_CTRL1                                0x1c
> > > +#define USB43DP_V5_5NM_COM_RESET_OVRD_CTRL2                                0x20
> > > +#define USB43DP_V5_5NM_COM_DBG_CLK_MUX_CTRL                                0x24
> > > +#define USB43DP_V5_5NM_COM_TYPEC_STATUS                                    0x28
> > > +#define USB43DP_V5_5NM_COM_PLACEHOLDER_STATUS                              0x2c
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID0                                    0x30
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID1                                    0x34
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID2                                    0x38
> > > +#define USB43DP_V5_5NM_COM_REVISION_ID3                                    0x3c
> >
> > QPHY_V5_DP_COM_foo ?
> >
>
> My first version of the QMP patch used V5 defines and USB worked
> sometimes. So I hacked up a thing to dump the phy sequences of the
> downstream and upstream kernels, compared the magic numbers and then
> tried to fit suitable constants.
>
> But it obviously was a waste of time and I would have to make up a
> different naming scheme for the ones that doesn't match the existing
> constants - when we could just use the autogenerated files that exist in
> the downstream kernels.
>
> [..]
> > > +#define USB43DP_V5_5NM_QSERDES_TXA_DEBUG_BUS1                              0xf0
> > > +#define USB43DP_V5_5NM_QSERDES_TXA_DEBUG_BUS2                              0xf4
> > > +#define USB43DP_V5_5NM_QSERDES_TXA_DEBUG_BUS3                              0xf8
> > > +#define USB43DP_V5_5NM_QSERDES_TXA_TX_BKUP_RO_BUS                  0xfc
> >
> > QSERDES_V5_20_TX_foo ? This looks compatible with the 4 registers that we
> > have in the header, but I can not verify the rest of registers
> >
>
> Exactly the point I was making in my reply to the other patch.
>
> Per the documentation this is version 5.0.0, but these register offsets
> happens to match the 5.20 defines that we have...
>
> > > +
> > > +/* Module: USB43DP_QSERDES_RXA_USB43DP_QSERDES_RXA_USB4_USB3_DP_QMP_RX */
> [..]
> > > +#define USB43DP_V5_5NM_QSERDES_RXA_RX_BKUP_READ_BUS3_STATUS                0x3e8
>
> And these, doesn't match either V5 or V5_20.

Yes, I guessed so.

>
> [..]
> > > +#define USB43DP_V5_5NM_QSERDES_TXB_TX_BKUP_RO_BUS                  0xfc
> >
> > What is the difference between _TXA_ and _TXB_ ?
> >
>
> Nothing, I just don't want us to mess around with these files if we can
> get them dumped from the register documentation.

Well, you still had the register offsets adjusted, hadn't you? I think
we can also apply sed to convert the names and then check if they
match the existing headers or not. If they do not, create a new
prefix, repeat, etc.

>
> > > +
> [..]
> > > +
> > > +/* Module: USB3_PCS_MISC_USB3_PCS_MISC_USB3_PCS_MISC */
> > > +#define USB3_V5_5NM_PCS_MISC_TYPEC_CTRL                                    0x00
> > > +#define USB3_V5_5NM_PCS_MISC_TYPEC_PWRDN_CTRL                              0x04
> > > +#define USB3_V5_5NM_PCS_MISC_PCS_MISC_CONFIG1                              0x08
> > > +#define USB3_V5_5NM_PCS_MISC_CLAMP_ENABLE                          0x0c
> > > +#define USB3_V5_5NM_PCS_MISC_TYPEC_STATUS                          0x10
> > > +#define USB3_V5_5NM_PCS_MISC_PLACEHOLDER_STATUS                            0x14
> >
> > QPHY_V4_PCS_MISC (or v5)
> >
>
> Perhaps, but then we're just making up those prefixes and hoping for the
> best.
>
> [..]
> > > +#define USB3_V5_5NM_PCS_EQ_CONFIG2                                 0x1e0
> > > +#define USB3_V5_5NM_PCS_EQ_CONFIG3                                 0x1e4
> > > +#define USB3_V5_5NM_PCS_EQ_CONFIG4                                 0x1E8
> > > +#define USB3_V5_5NM_PCS_EQ_CONFIG5                                 0x1EC
> >
> > This looks like both QPHY_V4_PCS and QPHY_V5_PCS. Most probably we should
> > merge them together and add these defines.
> >
>
> Exactly, all these defines looks like defines we already have and if you
> pick the wrong one you end up with things not working - or in my case
> something that worked sometimes.
>
> > > +
> > > +/* Module: USB3_PCS_USB3_USB3_PCS_USB3_USB3_PCS_USB3 */
> [..]
> > > +#define USB3_V5_5NM_PCS_USB3_RXTERMINATION_DLY_SEL                 0x60
> >
> > Again, QPHY_V5_PCS_USB w/o the 0x300 offset
> >
>
> Yeah, that extra region needs to be added to the binding and driver.

We can add it to the driver first (and just make it as an offset from pcs).

-- 
With best wishes
Dmitry