Re: [PATCH v4 2/4] phy: ralink: Add PHY driver for MT7621 PCIe PHY

[Vinod Koul <vkoul@kernel.org>]

On 31-10-20, 13:22, Sergio Paracuellos wrote:

> +#define RG_PE1_PIPE_REG				0x02c
> +#define RG_PE1_PIPE_RST				BIT(12)
> +#define RG_PE1_PIPE_CMD_FRC			BIT(4)
> +
> +#define RG_P0_TO_P1_WIDTH			0x100
> +#define RG_PE1_H_LCDDS_REG			0x49c
> +#define RG_PE1_H_LCDDS_PCW			GENMASK(30, 0)
> +#define RG_PE1_H_LCDDS_PCW_VAL(x)		((0x7fffffff & (x)) << 0)

Pls use FIELD_{GET|PREP} instead of coding like this, you already
defined the mask, use it to set and get the reg field ;)

> +
> +#define RG_PE1_FRC_H_XTAL_REG			0x400
> +#define RG_PE1_FRC_H_XTAL_TYPE			BIT(8)
> +#define RG_PE1_H_XTAL_TYPE			GENMASK(10, 9)
> +#define RG_PE1_H_XTAL_TYPE_VAL(x)		((0x3 & (x)) << 9)
> +
> +#define RG_PE1_FRC_PHY_REG			0x000
> +#define RG_PE1_FRC_PHY_EN			BIT(4)
> +#define RG_PE1_PHY_EN				BIT(5)
> +
> +#define RG_PE1_H_PLL_REG			0x490
> +#define RG_PE1_H_PLL_BC				GENMASK(23, 22)
> +#define RG_PE1_H_PLL_BC_VAL(x)			((0x3 & (x)) << 22)
> +#define RG_PE1_H_PLL_BP				GENMASK(21, 18)
> +#define RG_PE1_H_PLL_BP_VAL(x)			((0xf & (x)) << 18)
> +#define RG_PE1_H_PLL_IR				GENMASK(15, 12)
> +#define RG_PE1_H_PLL_IR_VAL(x)			((0xf & (x)) << 12)
> +#define RG_PE1_H_PLL_IC				GENMASK(11, 8)
> +#define RG_PE1_H_PLL_IC_VAL(x)			((0xf & (x)) << 8)
> +#define RG_PE1_H_PLL_PREDIV			GENMASK(7, 6)
> +#define RG_PE1_H_PLL_PREDIV_VAL(x)		((0x3 & (x)) << 6)
> +#define RG_PE1_PLL_DIVEN			GENMASK(3, 1)
> +#define RG_PE1_PLL_DIVEN_VAL(x)			((0x7 & (x)) << 1)
> +
> +#define RG_PE1_H_PLL_FBKSEL_REG			0x4bc
> +#define RG_PE1_H_PLL_FBKSEL			GENMASK(5, 4)
> +#define RG_PE1_H_PLL_FBKSEL_VAL(x)		((0x3 & (x)) << 4)
> +
> +#define	RG_PE1_H_LCDDS_SSC_PRD_REG		0x4a4
> +#define RG_PE1_H_LCDDS_SSC_PRD			GENMASK(15, 0)
> +#define RG_PE1_H_LCDDS_SSC_PRD_VAL(x)		((0xffff & (x)) << 0)
> +
> +#define RG_PE1_H_LCDDS_SSC_DELTA_REG		0x4a8
> +#define RG_PE1_H_LCDDS_SSC_DELTA		GENMASK(11, 0)
> +#define RG_PE1_H_LCDDS_SSC_DELTA_VAL(x)		((0xfff & (x)) << 0)
> +#define RG_PE1_H_LCDDS_SSC_DELTA1		GENMASK(27, 16)
> +#define RG_PE1_H_LCDDS_SSC_DELTA1_VAL(x)	((0xff & (x)) << 16)
> +
> +#define RG_PE1_LCDDS_CLK_PH_INV_REG		0x4a0
> +#define RG_PE1_LCDDS_CLK_PH_INV			BIT(5)
> +
> +#define RG_PE1_H_PLL_BR_REG			0x4ac
> +#define RG_PE1_H_PLL_BR				GENMASK(18, 16)
> +#define RG_PE1_H_PLL_BR_VAL(x)			((0x7 & (x)) << 16)
> +
> +#define	RG_PE1_MSTCKDIV_REG			0x414
> +#define RG_PE1_MSTCKDIV				GENMASK(7, 6)
> +#define RG_PE1_MSTCKDIV_VAL(x)			((0x3 & (x)) << 6)
> +
> +#define RG_PE1_FRC_MSTCKDIV			BIT(5)
> +
> +#define XTAL_MODE_SEL_SHIFT			6

Bonus you dont need to define shifts if you use stuff defined in
bitfield.h

> +struct mt7621_pci_phy {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct phy *phy;
> +	void __iomem *port_base;
> +	bool has_dual_port;
> +	bool bypass_pipe_rst;
> +};
> +
> +static inline u32 phy_read(struct mt7621_pci_phy *phy, u32 reg)
> +{
> +	u32 val;
> +
> +	regmap_read(phy->regmap, reg, &val);
> +
> +	return val;
> +}
> +
> +static inline void phy_write(struct mt7621_pci_phy *phy, u32 val, u32 reg)
> +{
> +	regmap_write(phy->regmap, reg, val);

Why not use regmap_ calls directly and avoid the dummy wrappers..?

> +}
> +
> +static inline void mt7621_phy_rmw(struct mt7621_pci_phy *phy,
> +				  u32 reg, u32 clr, u32 set)
> +{
> +	u32 val = phy_read(phy, reg);
> +
> +	val &= ~clr;
> +	val |= set;
> +	phy_write(phy, val, reg);

why not use regmap_update_bits() instead

> +static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
> +{
> +	struct device *dev = phy->dev;
> +	u32 xtal_mode;
> +
> +	xtal_mode = (rt_sysc_r32(SYSC_REG_SYSTEM_CONFIG0)
> +		     >> XTAL_MODE_SEL_SHIFT) & XTAL_MODE_SEL_MASK;
> +
> +	/* Set PCIe Port PHY to disable SSC */
> +	/* Debug Xtal Type */
> +	mt7621_phy_rmw(phy, RG_PE1_FRC_H_XTAL_REG,
> +		       RG_PE1_FRC_H_XTAL_TYPE | RG_PE1_H_XTAL_TYPE,
> +		       RG_PE1_FRC_H_XTAL_TYPE | RG_PE1_H_XTAL_TYPE_VAL(0x00));
> +
> +	/* disable port */
> +	mt7621_phy_rmw(phy, RG_PE1_FRC_PHY_REG,
> +		       RG_PE1_PHY_EN, RG_PE1_FRC_PHY_EN);
> +
> +	if (phy->has_dual_port) {
> +		mt7621_phy_rmw(phy, RG_PE1_FRC_PHY_REG + RG_P0_TO_P1_WIDTH,
> +			       RG_PE1_PHY_EN, RG_PE1_FRC_PHY_EN);
> +	}
> +
> +	if (xtal_mode <= 5 && xtal_mode >= 3) { /* 40MHz Xtal */
> +		/* Set Pre-divider ratio (for host mode) */
> +		mt7621_phy_rmw(phy, RG_PE1_H_PLL_REG,
> +			       RG_PE1_H_PLL_PREDIV,
> +			       RG_PE1_H_PLL_PREDIV_VAL(0x01));
> +		dev_info(dev, "Xtal is 40MHz\n");
> +	} else if (xtal_mode >= 6) { /* 25MHz Xal */
> +		mt7621_phy_rmw(phy, RG_PE1_H_PLL_REG,
> +			       RG_PE1_H_PLL_PREDIV,
> +			       RG_PE1_H_PLL_PREDIV_VAL(0x00));
> +		/* Select feedback clock */
> +		mt7621_phy_rmw(phy, RG_PE1_H_PLL_FBKSEL_REG,
> +			       RG_PE1_H_PLL_FBKSEL,
> +			       RG_PE1_H_PLL_FBKSEL_VAL(0x01));
> +		/* DDS NCPO PCW (for host mode) */
> +		mt7621_phy_rmw(phy, RG_PE1_H_LCDDS_SSC_PRD_REG,
> +			       RG_PE1_H_LCDDS_SSC_PRD,
> +			       RG_PE1_H_LCDDS_SSC_PRD_VAL(0x18000000));
> +		/* DDS SSC dither period control */
> +		mt7621_phy_rmw(phy, RG_PE1_H_LCDDS_SSC_PRD_REG,
> +			       RG_PE1_H_LCDDS_SSC_PRD,
> +			       RG_PE1_H_LCDDS_SSC_PRD_VAL(0x18d));
> +		/* DDS SSC dither amplitude control */
> +		mt7621_phy_rmw(phy, RG_PE1_H_LCDDS_SSC_DELTA_REG,
> +			       RG_PE1_H_LCDDS_SSC_DELTA |
> +			       RG_PE1_H_LCDDS_SSC_DELTA1,
> +			       RG_PE1_H_LCDDS_SSC_DELTA_VAL(0x4a) |
> +			       RG_PE1_H_LCDDS_SSC_DELTA1_VAL(0x4a));
> +		dev_info(dev, "Xtal is 25MHz\n");

Debug please

> +	} else { /* 20MHz Xtal */
> +		mt7621_phy_rmw(phy, RG_PE1_H_PLL_REG,
> +			       RG_PE1_H_PLL_PREDIV,
> +			       RG_PE1_H_PLL_PREDIV_VAL(0x00));
> +
> +		dev_info(dev, "Xtal is 20MHz\n");

ditto
-- 
~Vinod