| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 20 Feb 2020 17:58:41 +0800
From: Dilip Kota <eswara.kota@...ux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
kishon@...com, robh@...nel.org, cheol.yong.kim@...el.com,
chuanhua.lei@...ux.intel.com, qi-ming.wu@...el.com,
yixin.zhu@...el.com
Subject: Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy
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
>
Powered by blists - more mailing lists