Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy

[Dilip Kota]

On 2/19/2020 6:14 PM, Andy Shevchenko wrote:
> On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:
> > Combophy subsystem provides PHYs for various
> > controllers like PCIe, SATA and EMAC.
> ...
>
> > +static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};
> + blank line
Typo, will fix it.
> > +#define CLK_100MHZ		100000000
> > +#define CLK_156_25MHZ		156250000
> ...
>
> > +enum {
> > +	PHY_0 = 0,
> Aren't enum:s start with 0 by the standard?
> Ditto for all enum:s.
> (Or, if it represents value from hardware, perhaps makes sense to put a comment
>   to each of such enum and then all values must be explicit)
Values are related to h/w registers, will add the description in the 
comments.
> > +	PHY_1,
> > +	PHY_MAX_NUM,
> > +};
> ...
>
> > +struct intel_cbphy_iphy {
> > +	struct phy		*phy;
> > +	struct device		*dev;
> Can dev be derived from phy? Or phy from dev?
I see, there is no need of storing phy. Will remove it in the next patch 
version.
> > +	bool			enable;
> > +	struct intel_combo_phy	*parent;
> > +	struct reset_control	*app_rst;
> > +	u32			id;
> > +};
> ...
>
> > +static int intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
> > +{
> > +	struct intel_combo_phy *cbphy = iphy->parent;
> > +	u32 val, bitn;
> > +
> > +	bitn = cbphy->phy_mode * 2 + iphy->id;
> Why not
>
> 	u32 mask = BIT(cbphy->phy_mode * 2 + iphy->id);
> 	u32 val;
Looks more better, i will update it.
> > +	/* Register: 0 is enable, 1 is disable */
> > +	val =  set ? 0 : BIT(bitn);
> 	val = set ? 0 : mask;
>
> (why double space?)
Typo error. Will correct it.
> > +
> > +	return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
> > +				 BIT(bitn), val);
> 	return regmap_update_bits(..., mask, val);
>
> ?
Still it is taking more than 80 characters with mask, need to be in 2 lines

return regmap_update_bits(...,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mask, val);

> > +}
> > +
> > +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
> > +{
> > +	struct intel_combo_phy *cbphy = iphy->parent;
> > +	const u32 pad_dis_cfg_off = 0x174;
> > +	u32 val, bitn;
> > +
> > +	bitn = cbphy->id * 2 + iphy->id;
> > +
> > +	/* Register: 0 is enable, 1 is disable */
> > +	val = set ? 0 : BIT(bitn);
> > +
> > +	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn),
> > +				 val);
> Ditto.
Here it can with go in single line with mask,
> > +}
> ...
>
> > +static int intel_cbphy_iphy_cfg(struct intel_cbphy_iphy *iphy,
> > +				int (*phy_cfg)(struct intel_cbphy_iphy *))
> > +{
> > +	struct intel_combo_phy *cbphy = iphy->parent;
> > +	struct intel_cbphy_iphy *sphy;
> > +	int ret;
> > +
> > +	ret = phy_cfg(iphy);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (cbphy->aggr_mode == PHY_DL_MODE) {
> 	if (x != y)
> 		return 0;
>
> > +		sphy = &cbphy->iphy[PHY_1];
> > +		ret = phy_cfg(sphy);
> > +	}
> > +
> > +	return ret;
> 	return phy_cfg(...);
>
> > +}
> ...
>
> > +	switch (mode) {
> > +	case PHY_PCIE_MODE:
> > +		cb_mode = (aggr == PHY_DL_MODE) ?
> > +			  PCIE_DL_MODE : PCIE0_PCIE1_MODE;
> I think one line is okay here.

its taking 82 characters.

> > +		break;
> > +
> > +	case PHY_XPCS_MODE:
> > +		cb_mode = (aggr == PHY_DL_MODE) ? RXAUI_MODE : XPCS0_XPCS1_MODE;
> > +		break;
> > +
> > +	case PHY_SATA_MODE:
> > +		if (aggr == PHY_DL_MODE) {
> > +			dev_err(dev, "CBPHY%u mode:%u not support dual lane!\n",
> > +				cbphy->id, mode);
> > +			return -EINVAL;
> > +		}
> > +
> > +		cb_mode = SATA0_SATA1_MODE;
> > +		break;
> > +
> > +	default:
> > +		dev_err(dev, "CBPHY%u mode:%u not supported!\n",
> > +			cbphy->id, mode);
> > +		return -EINVAL;
> > +	}
> ...
>
>
> > +	if (!atomic_read(&cbphy->init_cnt)) {
> Here it can be 0.
>
> > +		ret = clk_prepare_enable(cbphy->core_clk);
> > +		if (ret) {
> > +			dev_err(cbphy->dev, "Clock enable failed!\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = clk_set_rate(cbphy->core_clk, cbphy->clk_rate);
> > +		if (ret) {
> > +			dev_err(cbphy->dev, "Clock freq set to %lu failed!\n",
> > +				cbphy->clk_rate);
> > +			goto clk_err;
> > +		}
> > +
> > +		intel_cbphy_rst_assert(cbphy);
> > +		ret = intel_cbphy_set_mode(cbphy);
> > +		if (ret)
> > +			goto clk_err;
> > +	}
> > +
> > +	ret = intel_cbphy_iphy_enable(iphy, true);
> > +	if (ret) {
> > +		dev_err(dev, "Failed enabling Phy core\n");
> > +		goto clk_err;
> > +	}
> > +
> > +	if (!atomic_read(&cbphy->init_cnt))
> Here it can be 1.
True,
I will fix this.
Thanks for pointing it.
> > +		intel_cbphy_rst_deassert(cbphy);
> Is it correct way to go?
>
> > +	ret = reset_control_deassert(iphy->app_rst);
> > +	if (ret) {
> > +		dev_err(dev, "PHY(%u:%u) phy deassert failed!\n",
> > +			COMBO_PHY_ID(iphy), PHY_ID(iphy));
> > +		goto clk_err;
> > +	}
> ...
>
> > +		ret = intel_cbphy_iphy_cfg(iphy,
> > +					   intel_cbphy_pcie_en_pad_refclk);
> One line is fine here.
It is taking 81 characters, so kept in 2 lines.
> > +		if (ret)
> > +			return ret;
> ...
>
> > +		ret = intel_cbphy_iphy_cfg(iphy,
> > +					   intel_cbphy_pcie_dis_pad_refclk);
> Ditto.
82 characters here.
> > +		if (ret)
> > +			return ret;
> ...
>
> > +		return ret;
> > +	}
> > +
> > +	iphy->enable = true;
> > +	platform_set_drvdata(pdev, iphy);
> > +
> > +	return 0;
> > +}
> ...
>
> > +	if (cbphy->aggr_mode == PHY_DL_MODE) {
> > +		if (!iphy0->enable || !iphy1->enable) {
> 	if (a) {
> 		if (b) {
> 			...
> 		}
> 	}
>
> is the same as
> 	if (a && b) {
> 		...
> 	}
>
> We have it many times discussed internally.
Will fix it.
> > +			dev_err(cbphy->dev,
> > +				"Dual lane mode but lane0: %s, lane1: %s\n",
> > +				iphy0->enable ? "on" : "off",
> > +				iphy1->enable ? "on" : "off");
> > +			return -EINVAL;
> > +		}
> > +	}
> ...
>
> > +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> > +						 "intel,syscfg", NULL, 1, 0,
> > +						 &ref);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	fwnode_handle_put(ref.fwnode);
> Why here?

Instructed to do:

" Caller is responsible to call fwnode_handle_put() on the returned  
args->fwnode pointer"

> > +	cbphy->id = ref.args[0];
> > +	cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);
> You rather need to have fwnode_to_regmap(). It's easy to add as a preparatory patch.
Sure, I will add it.
> > +
> > +	ret = fwnode_property_get_reference_args(dev_fwnode(dev), "intel,hsio",
> > +						 NULL, 1, 0, &ref);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	fwnode_handle_put(ref.fwnode);
> > +	cbphy->bid = ref.args[0];
> > +	cbphy->hsiocfg = device_node_to_regmap(ref.fwnode->dev->of_node);
> Ditto.
>
> > +	if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {
> Hmm... Why to mix device_property_*() vs. fwnode_property_*() ?
device_property_* are wrapper functions to fwnode_property_*().
Calling the fwnode_property_*() ending up doing the same work of 
device_property_*().

If the best practice is to maintain symmetry, will call fwnode_property_*().

> > +		cbphy->phy_mode = prop;
> > +		if (cbphy->phy_mode >= PHY_MAX_MODE) {
> > +			dev_err(dev, "PHY mode: %u is invalid\n",
> > +				cbphy->phy_mode);
> > +			return -EINVAL;
> > +		}
> > +	}
> ...
>
> > +	.owner =	THIS_MODULE,
> Do we still need this?
Present in all the PHY drivers,
Please let me know if it need to be removed.

Regards,
Dilip

>