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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux