Re: [PATCH phy v2 2/6] phy: marvell: phy-mvebu-a3700-comphy: Add native kernel implementation

Vinod Koul
2021-12-23T12:41:01Z

On 08-12-21, 03:40, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>

> +/* COMPHY registers */
> +#define COMPHY_POWER_PLL_CTRL 0x01
> +#define PU_IVREF_BIT BIT(15)
> +#define PU_PLL_BIT BIT(14)
> +#define PU_RX_BIT BIT(13)
> +#define PU_TX_BIT BIT(12)
> +#define PU_TX_INTP_BIT BIT(11)
> +#define PU_DFE_BIT BIT(10)
> +#define RESET_DTL_RX_BIT BIT(9)
> +#define PLL_LOCK_BIT BIT(8)
> +#define REF_FREF_SEL_MASK GENMASK(4, 0)
> +#define REF_FREF_SEL_SERDES_25MHZ (0x1 << 0)
> +#define REF_FREF_SEL_SERDES_40MHZ (0x3 << 0)
> +#define REF_FREF_SEL_SERDES_50MHZ (0x4 << 0)
> +#define REF_FREF_SEL_PCIE_USB3_25MHZ (0x2 << 0)
> +#define REF_FREF_SEL_PCIE_USB3_40MHZ (0x3 << 0)
> +#define COMPHY_MODE_MASK GENMASK(7, 5)
> +#define COMPHY_MODE_SATA (0x0 << 5)
> +#define COMPHY_MODE_PCIE (0x3 << 5)
> +#define COMPHY_MODE_SERDES (0x4 << 5)
> +#define COMPHY_MODE_USB3 (0x5 << 5)

Any reason why these are not using GENMASK. I guess documentation would
define these as BIT x-y right?

> +/* Changes to 40M1G25 mode data required for running 40M3G125 init mode */
> +static struct gbe_phy_init_data_fix gbe_phy_init_fix[] = {
> + {0x005, 0x07CC}, {0x015, 0x0000}, {0x01B, 0x0000}, {0x01D, 0x0000},
> + {0x01E, 0x0000}, {0x01F, 0x0000}, {0x020, 0x0000}, {0x021, 0x0030},
> + {0x026, 0x0888}, {0x04D, 0x0152}, {0x04F, 0xA020}, {0x050, 0x07CC},
> + {0x053, 0xE9CA}, {0x055, 0xBD97}, {0x071, 0x3015}, {0x076, 0x03AA},
> + {0x07C, 0x0FDF}, {0x0C2, 0x3030}, {0x0C3, 0x8000}, {0x0E2, 0x5550},
> + {0x0E3, 0x12A4}, {0x0E4, 0x7D00}, {0x0E6, 0x0C83}, {0x101, 0xFCC0},
> + {0x104, 0x0C10}

empty space after { and before } makes it look neater and it typically
the convention followed at many places

> +/* PHY selector configures with corresponding modes */
> +static int
> +mvebu_a3700_comphy_set_phy_selector(struct mvebu_a3700_comphy_lane *lane)
> +{
> + u32 old, new, clr = 0, set = 0;
> + unsigned long flags;
> +
> + switch (lane->mode) {
> + case PHY_MODE_SATA:
> + /* SATA must be in Lane2 */
> + if (lane->id == 2)
> + clr = COMPHY_SELECTOR_USB3_PHY_SEL_BIT;
> + else
> + goto error;
> + break;
> +
> + case PHY_MODE_ETHERNET:
> + if (lane->id == 0)
> + clr = COMPHY_SELECTOR_USB3_GBE1_SEL_BIT;
> + else if (lane->id == 1)
> + clr = COMPHY_SELECTOR_PCIE_GBE0_SEL_BIT;
> + else
> + goto error;
> + break;
> +
> + case PHY_MODE_USB_HOST_SS:
> + if (lane->id == 2)
> + set = COMPHY_SELECTOR_USB3_PHY_SEL_BIT;
> + else if (lane->id == 0)
> + set = COMPHY_SELECTOR_USB3_GBE1_SEL_BIT;
> + else
> + goto error;
> + break;
> +
> + case PHY_MODE_PCIE:
> + /* PCIE must be in Lane1 */
> + if (lane->id == 1)
> + set = COMPHY_SELECTOR_PCIE_GBE0_SEL_BIT;
> + else
> + goto error;
> + break;
> +
> + default:
> + goto error;
> + }
> +
> + raw_spin_lock_irqsave(&lane->priv->lock, flags);
> +
> + old = readl(lane->priv->comphy_regs + COMPHY_SELECTOR_PHY_REG);
> + new = (old & ~clr) | set;
> + writel(new, lane->priv->comphy_regs + COMPHY_SELECTOR_PHY_REG);
> +
> + raw_spin_unlock_irqrestore(&lane->priv->lock, flags);

any reason to use raw_ variant? why not spin_lock_irqsave()

--
~Vinod