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
