Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 19 Feb 2020 12:14:35 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy |
| |
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
| ![\](/images/icornerr.gif) |