Re: [PATCH RFC v3 5/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Vinod Koul <vkoul@kernel.org>]

On 25-07-24, 08:38, Josua Mayer wrote:
> Am 25.07.24 um 08:43 schrieb Vinod Koul:
> > On 20-07-24, 16:19, Josua Mayer wrote:

> >> +struct mvebu_cp110_utmi_port;
> > why forward declare and not move the structs instead?
> Seemed like the smaller change / more readable as diff.
> I can move the struct instead!

Yes and that can be preparatory as well :-)
 
> >
> >>  	reg &= ~(PLL_REFDIV_MASK | PLL_FBDIV_MASK | PLL_SEL_LPFR_MASK);
> >>  	reg |= (PLL_REFDIV_VAL << PLL_REFDIV_OFFSET) |
> >>  	       (PLL_FBDIV_VAL << PLL_FBDIV_OFFSET);
> >> -	writel(reg, PORT_REGS(port) + UTMI_PLL_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_PLL_CTRL_REG);
> >>  
> >>  	/* Impedance Calibration Threshold Setting */
> >> -	reg = readl(PORT_REGS(port) + UTMI_CAL_CTRL_REG);
> >> +	reg = readl(port->regs + UTMI_CAL_CTRL_REG);
> >>  	reg &= ~IMPCAL_VTH_MASK;
> >>  	reg |= IMPCAL_VTH_VAL << IMPCAL_VTH_OFFSET;
> >> -	writel(reg, PORT_REGS(port) + UTMI_CAL_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_CAL_CTRL_REG);
> >>  
> >>  	/* Set LS TX driver strength coarse control */
> >> -	reg = readl(PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
> >> +	reg = readl(port->regs + UTMI_TX_CH_CTRL_REG);
> >>  	reg &= ~TX_AMP_MASK;
> >>  	reg |= TX_AMP_VAL << TX_AMP_OFFSET;
> >> -	writel(reg, PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_TX_CH_CTRL_REG);
> >>  
> >>  	/* Disable SQ and enable analog squelch detect */
> >> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
> >> +	reg = readl(port->regs + UTMI_RX_CH_CTRL0_REG);
> >>  	reg &= ~SQ_DET_EN;
> >>  	reg |= SQ_ANA_DTC_SEL;
> >> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
> >> +	writel(reg, port->regs + UTMI_RX_CH_CTRL0_REG);
> >>  
> >>  	/*
> >>  	 * Set External squelch calibration number and
> >>  	 * enable the External squelch calibration
> >>  	 */
> >> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
> >> +	reg = readl(port->regs + UTMI_RX_CH_CTRL1_REG);
> >>  	reg &= ~SQ_AMP_CAL_MASK;
> >>  	reg |= (SQ_AMP_CAL_VAL << SQ_AMP_CAL_OFFSET) | SQ_AMP_CAL_EN;
> >> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
> >> +	writel(reg, port->regs + UTMI_RX_CH_CTRL1_REG);
> >>  
> >>  	/*
> >>  	 * Set Control VDAT Reference Voltage - 0.325V and
> >>  	 * Control VSRC Reference Voltage - 0.6V
> >>  	 */
> >> -	reg = readl(PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
> >> +	reg = readl(port->regs + UTMI_CHGDTC_CTRL_REG);
> >>  	reg &= ~(VDAT_MASK | VSRC_MASK);
> >>  	reg |= (VDAT_VAL << VDAT_OFFSET) | (VSRC_VAL << VSRC_OFFSET);
> >> -	writel(reg, PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_CHGDTC_CTRL_REG);
> >>  }
> >>  
> >>  static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
> >> @@ -166,22 +178,38 @@ static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
> >>  	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
> >>  	struct mvebu_cp110_utmi *utmi = port->priv;
> >>  	int i;
> >> +	int reg;
> >>  
> >>  	/* Power down UTMI PHY port */
> >> -	regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> >> -			  UTMI_PHY_CFG_PU_MASK);
> >> +	if (!IS_ERR(port->regs_cfg)) {
> >> +		reg = readl(port->regs_cfg);
> >> +		reg &= ~(UTMI_PHY_CFG_PU_MASK);
> >> +		writel(reg, port->regs_cfg);
> >> +	} else
> >> +		regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> >> +				  UTMI_PHY_CFG_PU_MASK);
> > why are we doing both raw register read/write and regmap ops... that
> > does not sound correct to me
> Indeed.
> 
> The next question would be why for Armada 8K / CP110 syscon was chosen.
> From what I could find the registers accessed by utmi driver
> are not accessed by other drivers.
> 
> I am adding raw register access as an alternative,
> 
> to keep supporting old device-tree specifying syscon handle.
> 
> I considered writing helper functions for the if-not-error-syscon-else-raw,
> but between set_bits, clear_bits, global and per-port regs
> would have ended up with too many.

Aha, please add the comment so we know why it was done like this few
years later

-- 
~Vinod