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
