[v2,1/2] dt-bindings: phy: Add YAML schemas for Intel Combophy

Combophy subsystem provides PHY support to various
controllers, viz. PCIe, SATA and EMAC.
Adding YAML schemas for the same.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
Changes on v2:

 Add custom 'select'
 Pass hardware instance entries with phandles and
   remove cell-index and bid entries
 Clock, register address space, are same for the children.
   So move them to parent node.
 Two PHY instances cannot run in different modes,
   so move the phy-mode entry to parent node.
 Add second child entry in the DT example.

 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
 1 file changed, 138 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml

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?

On Wed, 19 Feb 2020 11:31:29 +0800, Dilip Kota wrote:
> Combophy subsystem provides PHY support to various
> controllers, viz. PCIe, SATA and EMAC.
> Adding YAML schemas for the same.
> 
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
> Changes on v2:
> 
>  Add custom 'select'
>  Pass hardware instance entries with phandles and
>    remove cell-index and bid entries
>  Clock, register address space, are same for the children.
>    So move them to parent node.
>  Two PHY instances cannot run in different modes,
>    so move the phy-mode entry to parent node.
>  Add second child entry in the DT example.
> 
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml: properties:intel,phy-mode: {'$ref': '/schemas/types.yaml#/definitions/uint32', 'minimum': 0, 'maximum': 2, 'description': 'Configure the mode of the PHY.\n  0 - PCIe\n  1 - xpcs\n  2 - sata\n'} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml: properties:intel,phy-mode: 'not' is a required property

Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/phy/intel,combo-phy.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/phy/intel,combo-phy.example.dts] Error 1
Makefile:1263: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1240526
Please check and re-submit.

On Tue, Feb 18, 2020 at 9:31 PM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>
> Combophy subsystem provides PHY support to various
> controllers, viz. PCIe, SATA and EMAC.
> Adding YAML schemas for the same.
>
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
> Changes on v2:
>
>  Add custom 'select'
>  Pass hardware instance entries with phandles and
>    remove cell-index and bid entries
>  Clock, register address space, are same for the children.
>    So move them to parent node.
>  Two PHY instances cannot run in different modes,
>    so move the phy-mode entry to parent node.
>  Add second child entry in the DT example.
>
>  .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> new file mode 100644
> index 000000000000..8e65a2a71e7f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
> @@ -0,0 +1,138 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Combophy Subsystem
> +
> +maintainers:
> +  - Dilip Kota <eswara.kota@linux.intel.com>
> +
> +description: |
> +  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
> +  controllers. A single Combophy provides two PHY instances.
> +
> +# We need a select here so we don't match all nodes with 'simple-bus'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: intel,combo-phy
> +  required:
> +    - compatible
> +
> +properties:
> +  $nodename:
> +    pattern: "^combophy@[0-9]+$"

Unit addresses are hex.

> +
> +  compatible:
> +    items:
> +      - const: intel,combo-phy

Needs to be an SoC specific compatible.

> +      - const: simple-bus
> +
> +  clocks:
> +    description: |
> +      List of phandle and clock specifier pairs as listed
> +      in clock-names property. Configure the clocks according
> +      to the PHY mode.

How many?

No need to redefine a common property name, drop description. Plus,
where's clock-names?

> +
> +  reg:
> +    items:
> +      - description: ComboPhy core registers
> +      - description: PCIe app core control registers
> +
> +  reg-names:
> +    items:
> +      - const: core
> +      - const: app
> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: phy
> +      - const: core
> +
> +  intel,syscfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Chip configuration registers handle and ComboPhy instance id
> +
> +  intel,hsio:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: HSIO registers handle and ComboPhy instance id on NOC
> +
> +  intel,aggregation:
> +    description: |
> +      Specify the flag to confiure ComboPHY in dual lane mode.
> +    type: boolean
> +
> +  intel,phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2
> +    description: |
> +      Configure the mode of the PHY.
> +        0 - PCIe
> +        1 - xpcs
> +        2 - sata

Doesn't this need to be per PHY? Or the 2 PHYs have to be the same mode?

Use the types defined in include/dt-bindings/phy/phy.h. You'll need to
add XPCS which maybe should be more specific to distinguish 1G, 10G,
etc. Also, we typically put the mode into the 'phys' cells so the mode
lives with the client node.

> +
> +patternProperties:
> +  "^cb[0-9]phy@[0-9]+$":

^phy@...

> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: intel,phydev
> +
> +      "#phy-cells":
> +        const: 0
> +
> +      resets:
> +        description: |
> +          reset handle according to the PHY mode.
> +          See ../reset/reset.txt for details.
> +
> +    required:
> +      - compatible
> +      - "#phy-cells"
> +
> +required:
> +  - compatible
> +  - clocks
> +  - reg
> +  - reg-names
> +  - intel,syscfg
> +  - intel,hsio
> +  - intel,phy-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    combophy@0 {
> +        compatible = "intel,combo-phy", "simple-bus";
> +        clocks = <&cgu0 1>;
> +        reg = <0xd0a00000 0x40000>,
> +              <0xd0a40000 0x1000>;
> +        reg-names = "core", "app";
> +        resets = <&rcu0 0x50 6>,
> +                <&rcu0 0x50 17>;
> +        reset-names = "phy", "core";
> +        intel,syscfg = <&sysconf 0>;
> +        intel,hsio = <&hsiol 0>;
> +        intel,phy-mode = <0>;
> +
> +        cb0phy0:cb0phy@0 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 23>;
> +        };
> +
> +        cb0phy1:cb0phy@1 {
> +            compatible = "intel,phydev";
> +            #phy-cells = <0>;
> +            resets = <&rcu0 0x50 24>;
> +        };
> +    };
> --
> 2.11.0
>

On 2/19/2020 10:42 PM, Rob Herring wrote:
> On Tue, Feb 18, 2020 at 9:31 PM Dilip Kota<eswara.kota@linux.intel.com>  wrote:
>> Combophy subsystem provides PHY support to various
>> controllers, viz. PCIe, SATA and EMAC.
>> Adding YAML schemas for the same.
>>
>> Signed-off-by: Dilip Kota<eswara.kota@linux.intel.com>
>> ---
>> Changes on v2:
>>
>>   Add custom 'select'
>>   Pass hardware instance entries with phandles and
>>     remove cell-index and bid entries
>>   Clock, register address space, are same for the children.
>>     So move them to parent node.
>>   Two PHY instances cannot run in different modes,
>>     so move the phy-mode entry to parent node.
>>   Add second child entry in the DT example.
>>
>>   .../devicetree/bindings/phy/intel,combo-phy.yaml   | 138 +++++++++++++++++++++
>>   1 file changed, 138 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>> new file mode 100644
>> index 000000000000..8e65a2a71e7f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
>> @@ -0,0 +1,138 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id:http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Intel Combophy Subsystem
>> +
>> +maintainers:
>> +  - Dilip Kota<eswara.kota@linux.intel.com>
>> +
>> +description: |
>> +  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
>> +  controllers. A single Combophy provides two PHY instances.
>> +
>> +# We need a select here so we don't match all nodes with 'simple-bus'
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: intel,combo-phy
>> +  required:
>> +    - compatible
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^combophy@[0-9]+$"
> Unit addresses are hex.
Will fix it.
>
>> +
>> +  compatible:
>> +    items:
>> +      - const: intel,combo-phy
> Needs to be an SoC specific compatible.
Sure, will update it to intel, combophy-lgm
>
>> +      - const: simple-bus
>> +
>> +  clocks:
>> +    description: |
>> +      List of phandle and clock specifier pairs as listed
>> +      in clock-names property. Configure the clocks according
>> +      to the PHY mode.
> How many?
>
> No need to redefine a common property name, drop description. Plus,
> where's clock-names?
Its only one clock, i will add maxItems:1 and remove the description.
>
>> +
>> +  reg:
>> +    items:
>> +      - description: ComboPhy core registers
>> +      - description: PCIe app core control registers
>> +
>> +  reg-names:
>> +    items:
>> +      - const: core
>> +      - const: app
>> +
>> +  resets:
>> +    maxItems: 2
>> +
>> +  reset-names:
>> +    items:
>> +      - const: phy
>> +      - const: core
>> +
>> +  intel,syscfg:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Chip configuration registers handle and ComboPhy instance id
>> +
>> +  intel,hsio:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: HSIO registers handle and ComboPhy instance id on NOC
>> +
>> +  intel,aggregation:
>> +    description: |
>> +      Specify the flag to confiure ComboPHY in dual lane mode.
>> +    type: boolean
>> +
>> +  intel,phy-mode:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 2
>> +    description: |
>> +      Configure the mode of the PHY.
>> +        0 - PCIe
>> +        1 - xpcs
>> +        2 - sata
> Doesn't this need to be per PHY? Or the 2 PHYs have to be the same mode?
>
> Use the types defined in include/dt-bindings/phy/phy.h. You'll need to
Sure, will define the types in header file.
> add XPCS which maybe should be more specific to distinguish 1G, 10G,
> etc. Also, we typically put the mode into the 'phys' cells so the mode
> lives with the client node.
Two PHYs must be in same mode.
actual mode configuration is done for the ComboPhy to work as a two 
individual PHYs in PCIe/XPCS/SATA mode or as a single PHY providing dual 
physical lanes in PCIe/XPCS.
And mode configuration is dependent on the 'intel,aggregation' flag.
So, placed the phy-mode here itself, to make sure all the mode related 
parameters are at one location. Also, avoids setting individual PHYs in 
different modes.

>> +
>> +patternProperties:
>> +  "^cb[0-9]phy@[0-9]+$":
> ^phy@...

Sure, will fix it.

Regards,
Dilip

>

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

>

On Thu, Feb 20, 2020 at 05:58:41PM +0800, Dilip Kota wrote:
> On 2/19/2020 6:14 PM, Andy Shevchenko wrote:
> > On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:

...

> > 	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);

It's up to maintainer, I was talking about use of temporary variable for mask.

> > > +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,

Here I meant all changes from previous function, yes, temporary variable mask
in particular.

> > > +}

...

> > > +	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.

Up to maintainer, but I consider the two lines approach is worse to read.

> > > +		break;

...

> > > +		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.

Ditto.

> > > +		if (ret)
> > > +			return ret;

...

> > > +		ret = intel_cbphy_iphy_cfg(iphy,
> > > +					   intel_cbphy_pcie_dis_pad_refclk);
> > Ditto.
> 82 characters here.

Ditto.

> > > +		if (ret)
> > > +			return ret;

...

> > > +	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"

Right...

> > > +	cbphy->id = ref.args[0];
> > > +	cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);

...and here you called unreferenced one. Is it okay?
If it's still being referenced, that is fine, but otherwise it may gone already.


> > > +	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_*().

The best practice to keep consistency as much as possible.
If you call two similar APIs in one scope, it's not okay.