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

[Pali Rohár]

On Monday 27 December 2021 13:50:10 Marek Behún wrote:
> On Mon, 27 Dec 2021 16:01:50 +0530
> Vinod Koul <vkoul at kernel.org> wrote:
> 
> > 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 at 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
> 
> Cool I didn't know about this macro, I always wanted something like
> this. Will resend with this updated.
> 
> Thx.
> 
> Marek

Perfect! Anyway, I think that there is a typo in name. It is REFerence
FREQuency SELect, not freFuency.