linux-kernel - Re: Re: [PATCH v7 2/2] phy: eswin: Create eswin directory and add EIC7700 SATA PHY driver

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: <aWzhw65QEoLj2XE7@vaman>
Date: Sun, 18 Jan 2026 19:06:03 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Yulin Lu <luyulin@...incomputing.com>
Cc: neil.armstrong@...aro.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, p.zabel@...gutronix.de,
	linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, ningyu@...incomputing.com,
	zhengyu@...incomputing.com, linmin@...incomputing.com,
	huangyifeng@...incomputing.com, fenglin@...incomputing.com,
	lianghujun@...incomputing.com
Subject: Re: Re: [PATCH v7 2/2] phy: eswin: Create eswin directory and add
 EIC7700 SATA PHY driver

On 16-01-26, 16:50, Yulin Lu wrote:
> > > +static int eic7700_sata_phy_init(struct phy *phy)
> > > +{
> > > +	struct eic7700_sata_phy *sata_phy = phy_get_drvdata(phy);
> > > +	u32 val;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(sata_phy->clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	regmap_write(sata_phy->regmap, SATA_REF_CTRL1, SATA_CLK_RST_SOURCE_PHY);
> > > +
> > > +	val = FIELD_PREP(SATA_P0_PHY_TX_AMPLITUDE_GEN1_MASK, 0x42) |
> > > +	      FIELD_PREP(SATA_P0_PHY_TX_AMPLITUDE_GEN2_MASK, 0x46) |
> > > +	      FIELD_PREP(SATA_P0_PHY_TX_AMPLITUDE_GEN3_MASK, 0x73);
> > > +	regmap_write(sata_phy->regmap, SATA_PHY_CTRL0, val);
> > > +
> > > +	val = FIELD_PREP(SATA_P0_PHY_TX_PREEMPH_GEN1_MASK, 0x5) |
> > > +	      FIELD_PREP(SATA_P0_PHY_TX_PREEMPH_GEN2_MASK, 0x5) |
> > > +	      FIELD_PREP(SATA_P0_PHY_TX_PREEMPH_GEN3_MASK, 0x8);
> > 
> > Where are the magic values you are writing coming from..?
> > 
> 
> Hi Vinod,
> 
> These values set the TX preemphasis and amplitude parameters for the SATA PHY.
> The actual numbers come from eye‑diagram tuning results on different hardware
> development boards.
> The current code reflects the settings for the Sifive HiFive Premier P550 board.
> In the next patch I plan to move these into the devicetree (DTS).
> Would that be acceptable?

So this would change wrt each board the device is...? Maybe DT should be
better choice. Please check with DT folks on the approach

> 
> > > +	regmap_write(sata_phy->regmap, SATA_PHY_CTRL1, val);
> > > +
> > > +	val = FIELD_PREP(SATA_LOS_LEVEL_MASK, 0x9) |
> > > +	      FIELD_PREP(SATA_LOS_BIAS_MASK, 0x2);
> > > +	regmap_write(sata_phy->regmap, SATA_LOS_IDEN, val);
> > > +
> > > +	val = SATA_M_CSYSREQ | SATA_S_CSYSREQ;
> > > +	regmap_write(sata_phy->regmap, SATA_AXI_LP_CTRL, val);
> > > +
> > > +	val = SATA_REF_REPEATCLK_EN | SATA_REF_USE_PAD;
> > > +	regmap_write(sata_phy->regmap, SATA_REF_CTRL, val);
> > > +
> > > +	val = FIELD_PREP(SATA_MPLL_MULTIPLIER_MASK, 0x3c);
> > > +	regmap_write(sata_phy->regmap, SATA_MPLL_CTRL, val);
> > > +
> > > +	usleep_range(15, 20);
> > > +
> > > +	ret = reset_control_deassert(sata_phy->rst);
> > > +	if (ret)
> > > +		goto disable_clk;
> > > +
> > > +	ret = wait_for_phy_ready(sata_phy->regmap, SATA_P0_PHY_STAT,
> > > +				 SATA_P0_PHY_READY, 1);
> > > +	if (ret < 0) {
> > > +		dev_err(&sata_phy->phy->dev, "PHY READY check failed\n");
> > > +		goto disable_clk;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +disable_clk:
> > > +	clk_disable_unprepare(sata_phy->clk);
> > > +	return ret;
> > > +}
> > > +
> > > +static int eic7700_sata_phy_exit(struct phy *phy)
> > > +{
> > > +	struct eic7700_sata_phy *sata_phy = phy_get_drvdata(phy);
> > > +	int ret;
> > > +
> > > +	ret = reset_control_assert(sata_phy->rst);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	clk_disable_unprepare(sata_phy->clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct phy_ops eic7700_sata_phy_ops = {
> > > +	.init		= eic7700_sata_phy_init,
> > > +	.exit		= eic7700_sata_phy_exit,
> > > +	.owner		= THIS_MODULE,
> > > +};
> > > +
> > > +static int eic7700_sata_phy_probe(struct platform_device *pdev)
> > > +{
> > > +	struct eic7700_sata_phy *sata_phy;
> > > +	struct phy_provider *phy_provider;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct resource *res;
> > > +	void __iomem *regs;
> > > +
> > > +	sata_phy = devm_kzalloc(dev, sizeof(*sata_phy), GFP_KERNEL);
> > > +	if (!sata_phy)
> > > +		return -ENOMEM;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res)
> > > +		return -ENOENT;
> > > +
> > > +	regs = devm_ioremap(dev, res->start, resource_size(res));
> > > +	if (IS_ERR(regs))
> > > +		return PTR_ERR(regs);
> > 
> > devm_platform_get_and_ioremap_resource() please
> > 
> 
> As explained in my “v6 → v5” changes in the cover‑letter:
> “Map the I/O resource with platform_get_resource and devm_ioremap
> instead of the devm_platform_ioremap_resource API,
> because the address region of the SATA‑PHY falls into the region of
> the HSP clock & reset that has already been obtained by the HSP
> clock‑and‑reset driver.”
> The HSP clock-and-reset driver uses devm_platform_get_and_ioremap_resource(),
> meaning this region has already been requested.
> The HSP clock-and-reset driver is also currently being upstreamed.

Worth adding a comment here for that

-- 
~Vinod