Hi Vinod,
Thanks for the review.
On Thu, Nov 19, 2020 at 6:31 AM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
>
> 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 ;)
Will change this and properly remove all of these *_VAL defines.
>
> > +
> > +#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
I will also check how to remove this SHIFT.
>
> > +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..?
This is because name was the dummy names are a bit shorter :) but if
it is also necessary I will use directly regmap_ functions.
>
> > +}
> > +
> > +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
True. Will change.
>
> > +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
Will change this trace to debug.
>
> > + } 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
And this one too.
> --
> ~Vinod
Again, thanks for the review. Will change this and send v5 with this
changes and collect ack's and reviewed's from this series.
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
