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

[Vinod Koul <vkoul@kernel.org>]

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy