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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZqIxAf2yPL32HLaP@matsya>
Date: Thu, 25 Jul 2024 16:33:29 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Josua Mayer <josua@...id-run.com>
Cc: Kishon Vijay Abraham I <kishon@...nel.org>,
	Andrew Lunn <andrew@...n.ch>,
	Gregory Clement <gregory.clement@...tlin.com>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	Konstantin Porotchkin <kostap@...vell.com>,
	Yazan Shhady <yazan.shhady@...id-run.com>,
	"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH RFC v3 5/6] phy: mvebu-cp110-utmi: add support for
 armada-380 utmi phys

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