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
> +#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)
> + 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?
> + 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;
> + /* Register: 0 is enable, 1 is disable */
> + val = set ? 0 : BIT(bitn);
val = set ? 0 : mask;
(why double space?)
> +
> + return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
> + BIT(bitn), val);
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.
> +}
...
> +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.
> + 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.
> + 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.
> + if (ret)
> + return ret;
...
> + ret = intel_cbphy_iphy_cfg(iphy,
> + intel_cbphy_pcie_dis_pad_refclk);
Ditto.
> + 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.
> + 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?
> + 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.
> +
> + 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_*() ?
> + 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?
--
With Best Regards,
Andy Shevchenko
