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

Vinod Koul
2021-12-27T10:32:03Z

On 23-12-21, 14:21, Pali Rohár wrote:
> On Thursday 23 December 2021 18:10:52 Vinod Koul wrote:
> > 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?
>
> Hello Vinod! I'm not sure to which macro refers your question as all
> *_MASK defines are now using GENMASK() macros.
>
> Definitions which do not have mask _MASK are not masks, but rather
> particular values for corresponding _MASK constant.
>
> So, for example: REF_FREF_SEL_MASK is 5 bit mask for fref_sel and
> fref_sel can contain exactly one of the following values:
> REF_FREF_SEL_SERDES_25MHZ, REF_FREF_SEL_SERDES_40MHZ,
> REF_FREF_SEL_SERDES_50MHZ, REF_FREF_SEL_PCIE_USB3_25MHZ,
> REF_FREF_SEL_PCIE_USB3_40MHZ. So you want to set serdes ref freq to
> 50 MHz you need to clear bits [4:0] (macro REF_FREF_SEL_MASK) and then
> set to value 0x4 (macro REF_FREF_SEL_SERDES_50MHZ) to bits [4:0].
>
> It is clear now? Or did you mean something different?

Thanks for explanation.

Looking at this, i think this should be defined as constants as defined
in the spec:
#define REF_FREF_SEL_SERDES_25MHZ x
#define REF_FREF_SEL_SERDES_40MHZ y
... so on

and then use FIELD_PREP(MASK, constant) ie,
FIELD_PREP(REF_FREF_SEL_MASK, REF_FREF_SEL_SERDES_25MHZ)

That sounds more readable and less error prone to me :)

Thanks
--
~Vinod