Re: [PATCH v2 15/15] phy: qcom-qmp-pcie: add support for sc8280xp 4-lane PHYs

[Johan Hovold]

On Thu, Oct 20, 2022 at 12:32:13PM +0300, Dmitry Baryshkov wrote:
> On 20/10/2022 09:43, Johan Hovold wrote:
> > On Thu, Oct 20, 2022 at 06:43:47AM +0300, Dmitry Baryshkov wrote:
> >> On 19/10/2022 14:35, Johan Hovold wrote:
> >>> The PCIe2 and PCIe3 controllers and PHYs on SC8280XP can be used in
> >>> 4-lane mode or as separate controllers and PHYs in 2-lane mode (e.g. as
> >>> PCIe2A and PCIe2B).
> >>>
> >>> Add support for fetching the 4-lane configuration from the TCSR and
> >>> programming the lane registers of the second port when in 4-lane mode.
> >>>
> >>> Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>

> > The gen3x4 PHYs can be in either 4-lane or 2-lane mode depending on the
> > TCSR configuration. Port A is programmed identically in both cases
> > except for this serdes register, and in 4-lane mode tx/rx also needs
> > to be programmed for port B.
> >   
> >>> +
> >>>    	/* clock ids to be requested */
> >>>    	const char * const *clk_list;
> >>>    	int num_clks;
> >>> @@ -1518,6 +1527,7 @@ struct qmp_pcie {
> >>>    	struct device *dev;
> >>>    
> >>>    	const struct qmp_phy_cfg *cfg;
> >>> +	bool tcsr_4ln_config;
> >>
> >> As a matter of preference, this seems too specific. I'd rename it to
> >> split_config or split_4ln_config.
> > 
> > I'm afraid those names do not make much sense. This TCSR register
> > controls whether the PHY is in 4-lane mode (instead of 2-lane mode).
> 
> Well, we just need the info that it's 4-lane. It doesn't really matter 
> if this information comes from TCSR, DT or e.g. fuses. I'd say that TCSR 
> is a platform detail. Thus I'm suggesting a more generic name.

No, it's a specific configuration flag for this (and possibly coming
platforms) to control whether the two PHY ports are used as individual x2
PHYs or as a combined x4 PHY.

It's not just about number of lanes and can definitely not come from DT
or somewhere else as that TCSR bit drives a signal that's needed during
programming.

> >>> +static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> >>> +{
> >>> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> >>> +	const struct qmp_pcie_offsets *offs = cfg->offsets;
> >>> +	void __iomem *tx3, *rx3, *tx4, *rx4;
> >>> +
> >>> +	tx3 = qmp->port_b + offs->tx;
> >>> +	rx3 = qmp->port_b + offs->rx;
> >>> +	tx4 = qmp->port_b + offs->tx2;
> >>> +	rx4 = qmp->port_b + offs->rx2;
> >>> +
> >>> +	qmp_pcie_configure_lane(tx3, tbls->tx, tbls->tx_num, 1);
> >>> +	qmp_pcie_configure_lane(rx3, tbls->rx, tbls->rx_num, 1);
> >>> +
> >>> +	qmp_pcie_configure_lane(tx4, tbls->tx, tbls->tx_num, 2);
> >>> +	qmp_pcie_configure_lane(rx4, tbls->rx, tbls->rx_num, 2);
> >>
> >> I'd use BIT(2) and BIT(3) here. This would allow one to make a
> >> difference between programming first pair of lanes and second pair of
> >> lanes if necessary.
> > 
> > No, the tx and tx registers of the second port should be programmed
> > identically to that of the first port.
> 
> As you would prefer. As a matter of fact, we do not have CFG_LANES in 
> the PCIe PHY. Thus I'm surprised that you didn't drop this. I think 
> CFG_LANES usage is limited to sm8250 USB and combo PHY configurations.

It's actually also used by SC8280XP so we cannot drop it (see
sc8280xp_qmp_gen3x2_pcie_tx_tbl) here. Appears to be unused for UFS
currently, though.

Johan