Re: [PATCH 07/13] phy: qcom: qmp-usb: split USB-C PHY driver

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 13.01.2024 06:42, Dmitry Baryshkov wrote:
> In preparation to adding Type-C handling for MSM8998, QCM2290 and SM6115
> platforms, create new QMP USB-C PHY driver by splitting mentioned
> platforms to a separate file. In future it will also be extended with
> support for the DisplayPort handling. It will also be reused later for
> such platforms as SDM660, SM6125, SM6150.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

[...]


> +#include "phy-qcom-qmp.h"
> +#include "phy-qcom-qmp-pcs-misc-v3.h"
> +
> +/* QPHY_SW_RESET bit */
> +#define SW_RESET				BIT(0)
> +/* QPHY_POWER_DOWN_CONTROL */
> +#define SW_PWRDN				BIT(0)

Most / all of these defines could probably live in a header file.

[...]

> +struct qmp_usbc_offsets {
> +	u16 serdes;
> +	u16 pcs;
> +	u16 pcs_misc;
> +	u16 pcs_usb;
> +	u16 tx;
> +	u16 rx;
> +	/* for PHYs with >= 2 lanes */

So, all PHYs within this driver if I'm following correctly

> +	u16 tx2;
> +	u16 rx2;
> +};
> +

> +static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> +{
> +	u32 reg;
> +
> +	reg = readl(base + offset);
> +	reg |= val;
> +	writel(reg, base + offset);
> +
> +	/* ensure that above write is through */
> +	readl(base + offset);
> +}
> +
> +static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
> +{
> +	u32 reg;
> +
> +	reg = readl(base + offset);
> +	reg &= ~val;
> +	writel(reg, base + offset);
> +
> +	/* ensure that above write is through */
> +	readl(base + offset);
> +}

Maybe you could use regmap to avoid NIH-ing such accessors

> +
> +/* list of clocks required by phy */
> +static const char * const qmp_usbc_phy_clk_l[] = {
> +	"aux", "cfg_ahb", "ref", "com_aux",
> +};
> +
> +/* list of resets */
> +static const char * const usb3phy_legacy_reset_l[] = {
> +	"phy", "common",
> +};
> +
> +static const char * const usb3phy_reset_l[] = {
> +	"phy_phy", "phy",
> +};
> +
> +/* list of regulators */
> +static const char * const qmp_phy_vreg_l[] = {
> +	"vdda-phy", "vdda-pll",
> +};
> +
> +static const struct qmp_usbc_offsets qmp_usbc_offsets_v3_qcm2290 = {
> +	.serdes		= 0x0,
> +	.pcs		= 0xc00,
> +	.pcs_misc	= 0xa00,
> +	.tx		= 0x200,
> +	.rx		= 0x400,
> +	.tx2		= 0x600,
> +	.rx2		= 0x800,
> +};
> +
> +static const struct qmp_phy_cfg msm8998_usb3phy_cfg = {
> +	.lanes			= 2,
> +
> +	.offsets		= &qmp_usbc_offsets_v3_qcm2290,
> +
> +	.serdes_tbl             = msm8998_usb3_serdes_tbl,
> +	.serdes_tbl_num         = ARRAY_SIZE(msm8998_usb3_serdes_tbl),
> +	.tx_tbl                 = msm8998_usb3_tx_tbl,
> +	.tx_tbl_num             = ARRAY_SIZE(msm8998_usb3_tx_tbl),
> +	.rx_tbl                 = msm8998_usb3_rx_tbl,
> +	.rx_tbl_num             = ARRAY_SIZE(msm8998_usb3_rx_tbl),
> +	.pcs_tbl                = msm8998_usb3_pcs_tbl,
> +	.pcs_tbl_num            = ARRAY_SIZE(msm8998_usb3_pcs_tbl),
> +	.vreg_list              = qmp_phy_vreg_l,
> +	.num_vregs              = ARRAY_SIZE(qmp_phy_vreg_l),
> +	.regs                   = qmp_v3_usb3phy_regs_layout,
> +};
> +
> +static const struct qmp_phy_cfg qcm2290_usb3phy_cfg = {
> +	.lanes			= 2,
> +
> +	.offsets		= &qmp_usbc_offsets_v3_qcm2290,
> +
> +	.serdes_tbl		= qcm2290_usb3_serdes_tbl,
> +	.serdes_tbl_num		= ARRAY_SIZE(qcm2290_usb3_serdes_tbl),
> +	.tx_tbl			= qcm2290_usb3_tx_tbl,
> +	.tx_tbl_num		= ARRAY_SIZE(qcm2290_usb3_tx_tbl),
> +	.rx_tbl			= qcm2290_usb3_rx_tbl,
> +	.rx_tbl_num		= ARRAY_SIZE(qcm2290_usb3_rx_tbl),
> +	.pcs_tbl		= qcm2290_usb3_pcs_tbl,
> +	.pcs_tbl_num		= ARRAY_SIZE(qcm2290_usb3_pcs_tbl),
> +	.vreg_list		= qmp_phy_vreg_l,
> +	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
> +	.regs			= qmp_v3_usb3phy_regs_layout_qcm2290,
> +};
> +
> +static void qmp_usbc_configure_lane(void __iomem *base,
> +					const struct qmp_phy_init_tbl tbl[],
> +					int num,
> +					u8 lane_mask)
> +{
> +	int i;
> +	const struct qmp_phy_init_tbl *t = tbl;
> +
> +	if (!t)
> +		return;
> +
> +	for (i = 0; i < num; i++, t++) {
> +		if (!(t->lane_mask & lane_mask))
> +			continue;
> +
> +		writel(t->val, base + t->offset);
> +	}
> +}
> +
> +static void qmp_usbc_configure(void __iomem *base,
> +				   const struct qmp_phy_init_tbl tbl[],
> +				   int num)
> +{
> +	qmp_usbc_configure_lane(base, tbl, num, 0xff);
> +}
> +

Can this be inlined?

> +static int qmp_usbc_serdes_init(struct qmp_usbc *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	void __iomem *serdes = qmp->serdes;
> +	const struct qmp_phy_init_tbl *serdes_tbl = cfg->serdes_tbl;
> +	int serdes_tbl_num = cfg->serdes_tbl_num;
> +
> +	qmp_usbc_configure(serdes, serdes_tbl, serdes_tbl_num);
> +
> +	return 0;
> +}

Can this be inlined?

[...]

> +	/* Tx, Rx, and PCS configurations */
> +	qmp_usbc_configure_lane(tx, cfg->tx_tbl, cfg->tx_tbl_num, 1);
> +	qmp_usbc_configure_lane(rx, cfg->rx_tbl, cfg->rx_tbl_num, 1);
> +
> +	if (cfg->lanes >= 2) {

Again, if (true) IIUC


> +		qmp_usbc_configure_lane(qmp->tx2, cfg->tx_tbl, cfg->tx_tbl_num, 2);
> +		qmp_usbc_configure_lane(qmp->rx2, cfg->rx_tbl, cfg->rx_tbl_num, 2);
> +	}
> +
> +	qmp_usbc_configure(pcs, cfg->pcs_tbl, cfg->pcs_tbl_num);
> +
> +	if (pcs_usb)

if (false)?

[...]

The rest looks to be boilerplate that's already present in at least
one more driver..

Konrad