Re: [PATCH v7 08/12] phy: qcom-qmp-pcie: Add support for SM8550 g3x2 and g4x2 PCIEs

From: Johan Hovold <johan@kernel.org>
To: Abel Vesa <abel.vesa@linaro.org>
Cc: "Andy Gross" <agross@kernel.org>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Johan Hovold" <johan+linaro@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Neil Armstrong" <neil.armstrong@linaro.org>,
	"Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>
Subject: Re: [PATCH v7 08/12] phy: qcom-qmp-pcie: Add support for SM8550 g3x2 and g4x2 PCIEs
Date: Wed, 8 Feb 2023 17:35:39 +0100	[thread overview]
Message-ID: <Y+PPWymsIfYW9OHX@hovoldconsulting.com> (raw)
In-Reply-To: <Y+EJDofgt6I/abyp@linaro.org>

On Mon, Feb 06, 2023 at 04:05:02PM +0200, Abel Vesa wrote:
> On 23-02-03 10:33:14, Johan Hovold wrote:
> > On Fri, Feb 03, 2023 at 10:18:03AM +0200, Abel Vesa wrote:
> > > Add the SM8550 both g4 and g3 configurations. In addition, there is a
> > > new "lane shared" table that needs to be configured for g4, along with
> > > the No-CSR list of resets.
> > 
> > Could you add a comment about the new nocsr reset and how it is used
> > here?
> >  
> > > Co-developed-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > 
> > > This patchset relies on the following patchset:
> > > https://lore.kernel.org/all/20230117224148.1914627-1-abel.vesa@linaro.org/
> > > 
> > > The v6 of this patch is:
> > > https://lore.kernel.org/all/20230202123902.3831491-9-abel.vesa@linaro.org/
> > > 
> > > Changes since v6:
> > >  * none
> > > 
> > > Changes since v5:
> > >  * renmaed the no-CSR reset to "phy_nocsr" as discussed off-list with
> > >    Bjorn and Johan
> > > 
> > > Changes since v4:
> > >  * dropped _serdes infix from ln_shrd table name and from every ln_shrd
> > >    variable name
> > >  * added hyphen between "no CSR" in both places
> > >  * dropped has_ln_shrd_serdes_tbl
> > >  * reordered qmp_pcie_offsets_v6_20 by struct members
> > >  * added rollback for no-CSR reset in qmp_pcie_init fail path
> > >  * moved ln_shrd offset calculation after port_b
> > > 
> > > Changes since v3:
> > >  * added Dmitry's R-b tag
> > > 
> > > Changes since v2:
> > >  * none
> > > 
> > > Changes since v1:
> > >  * split all the offsets into separate patches, like Vinod suggested
> > > 
> > > 
> > >  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 367 ++++++++++++++++++++++-
> > >  1 file changed, 365 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > index 907f3f236f05..ff6c0b526fde 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > @@ -1506,6 +1506,234 @@ static const struct qmp_phy_init_tbl sm8450_qmp_gen4x2_pcie_ep_pcs_misc_tbl[] =
> > >  	QMP_PHY_INIT_CFG(QPHY_V5_20_PCS_PCIE_OSC_DTCT_MODE2_CONFIG5, 0x08),
> > >  };
> > 
> [...]
> > 
> > >  
> > > @@ -2214,6 +2469,68 @@ static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = {
> > >  	.phy_status		= PHYSTATUS_4_20,
> > >  };
> > >  
> > > +static const struct qmp_phy_cfg sm8550_qmp_gen3x2_pciephy_cfg = {
> > > +	.lanes = 2,
> > > +
> > > +	.offsets		= &qmp_pcie_offsets_v5,
> > 
> > Did you really intend to use the v5 offsets here? It seems you use v6.20
> > defines in the tables below. This may work but it looks a little strange
> > and does not match how we name and use these resources for the other
> > SoCs (e.g. reusing structures and defines from older IP revisions is
> > fine, but not necessarily the other way round).
> 
> So here is what is happening here. The actual IP block version is 6 for
> the g3x2. The offsets of the tables are the same as on v5, but the
> actual offsets of some of the registers within those tables are
> entirely different. Now, if you compare the PCS PCIe offsets (v5 vs v6)
> you'll notice that all v6 registers currently added are the same as v5
> (both names and values). With that in mind, we still need to keep the v6
> offsets for the case when a new register, that might not be in v5, might
> be added later on. As for the table offsets, since they look the same we
> should probably not add a dedicated v6 one.

Ok, makes sense.

Johan