On Sat, 13 Jan 2024 at 12:42, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote: > > 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@xxxxxxxxxx> > > --- > > [...] > > > > +#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. For this (and several other comments), see https://lore.kernel.org/linux-arm-msm/20240109-phy-qmp-merge-common-v1-0-572899a14318@xxxxxxxxxx/ > > [...] > > > +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 Yes. I just felt that it is easier to follow for copy & modify rather than just rushing all the changes in. I can squash one of the next patches that clear single-lane support out of this driver.. > > > + 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 regmap will still require posting through read, will it not? > > > + > > +/* 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? Yes and yes. > > > +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)? Nice catch. > > [...] > > The rest looks to be boilerplate that's already present in at least > one more driver.. > > Konrad -- With best wishes Dmitry
