On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:Typo, will fix it.
Combophy subsystem provides PHYs for various...
controllers like PCIe, SATA and EMAC.
+static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};+ blank line
Values are related to h/w registers, will add the description in the comments.
+#define CLK_100MHZ 100000000...
+#define CLK_156_25MHZ 156250000
+enum {Aren't enum:s start with 0 by the standard?
+ PHY_0 = 0,
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)
I see, there is no need of storing phy. Will remove it in the next patch version.
+ PHY_1,...
+ PHY_MAX_NUM,
+};
+struct intel_cbphy_iphy {Can dev be derived from phy? Or phy from dev?
+ struct phy *phy;
+ struct device *dev;
Looks more better, i will update it.
+ 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)Why not
+{
+ struct intel_combo_phy *cbphy = iphy->parent;
+ u32 val, bitn;
+
+ bitn = cbphy->phy_mode * 2 + iphy->id;
u32 mask = BIT(cbphy->phy_mode * 2 + iphy->id);
u32 val;
Typo error. Will correct it.
+ /* Register: 0 is enable, 1 is disable */val = set ? 0 : mask;
+ val = set ? 0 : BIT(bitn);
(why double space?)
Still it is taking more than 80 characters with mask, need to be in 2 lines
+return regmap_update_bits(..., mask, val);
+ return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
+ BIT(bitn), val);
?
Here it can with go in single line with mask,
+}Ditto.
+
+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);
+}...
+static int intel_cbphy_iphy_cfg(struct intel_cbphy_iphy *iphy,if (x != y)
+ 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) {
return 0;
+ sphy = &cbphy->iphy[PHY_1];return phy_cfg(...);
+ ret = phy_cfg(sphy);
+ }
+
+ return ret;
+}...
+ switch (mode) {I think one line is okay here.
+ case PHY_PCIE_MODE:
+ cb_mode = (aggr == PHY_DL_MODE) ?
+ PCIE_DL_MODE : PCIE0_PCIE1_MODE;
True,
+ 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);Here it can be 1.
+ 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))
It is taking 81 characters, so kept in 2 lines.
+ 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,One line is fine here.
+ intel_cbphy_pcie_en_pad_refclk);
82 characters here.
+ if (ret)...
+ return ret;
+ ret = intel_cbphy_iphy_cfg(iphy,Ditto.
+ intel_cbphy_pcie_dis_pad_refclk);
Will fix it.
+ if (ret)...
+ return ret;
+ return ret;...
+ }
+
+ iphy->enable = true;
+ platform_set_drvdata(pdev, iphy);
+
+ return 0;
+}
+ if (cbphy->aggr_mode == PHY_DL_MODE) {if (a) {
+ if (!iphy0->enable || !iphy1->enable) {
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),Why here?
+ "intel,syscfg", NULL, 1, 0,
+ &ref);
+ if (ret < 0)
+ return ret;
+
+ fwnode_handle_put(ref.fwnode);
Sure, I will add it.
+ cbphy->id = ref.args[0];You rather need to have fwnode_to_regmap(). It's easy to add as a preparatory patch.
+ cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);
device_property_* are wrapper functions to fwnode_property_*().
+Ditto.
+ 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);
+ if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {Hmm... Why to mix device_property_*() vs. fwnode_property_*() ?
Present in all the PHY drivers,
+ 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?