diff mbox series

[RESEND,v10,1/2] dt-bindings: phy: Document Samsung UFS PHY bindings

Message ID 20200624235631.11232-1-alim.akhtar@samsung.com
State Not Applicable, archived
Headers show
Series [RESEND,v10,1/2] dt-bindings: phy: Document Samsung UFS PHY bindings | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success

Commit Message

Alim Akhtar June 24, 2020, 11:56 p.m. UTC 
This patch documents Samsung UFS PHY device tree bindings

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
This is just a rebase on phy-next, was part of series [1]
which adds ufs host contoller driver.
[1]  https://lkml.org/lkml/2020/5/27/1697

 .../bindings/phy/samsung,ufs-phy.yaml         | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/samsung,ufs-phy.yaml

Comments

Vinod Koul July 1, 2020, 6:53 a.m. UTC | #1 
Hi Alim,

On 25-06-20, 05:26, Alim Akhtar wrote:

> +int samsung_ufs_phy_wait_for_lock_acq(struct phy *phy)

static ?

> +{
> +	struct samsung_ufs_phy *ufs_phy = get_samsung_ufs_phy(phy);
> +	const unsigned int timeout_us = 100000;
> +	const unsigned int sleep_us = 10;
> +	u32 val;
> +	int err;
> +
> +	err = readl_poll_timeout(
> +			ufs_phy->reg_pma + PHY_APB_ADDR(PHY_PLL_LOCK_STATUS),
> +			val, (val & PHY_PLL_LOCK_BIT), sleep_us, timeout_us);
> +	if (err) {
> +		dev_err(ufs_phy->dev,
> +			"failed to get phy pll lock acquisition %d\n", err);
> +		goto out;
> +	}
> +
> +	err = readl_poll_timeout(
> +			ufs_phy->reg_pma + PHY_APB_ADDR(PHY_CDR_LOCK_STATUS),
> +			val, (val & PHY_CDR_LOCK_BIT), sleep_us, timeout_us);
> +	if (err) {
> +		dev_err(ufs_phy->dev,
> +			"failed to get phy cdr lock acquisition %d\n", err);
> +		goto out;

this one can be dropped

> +	}
> +
> +out:
> +	return err;
> +}
> +
> +int samsung_ufs_phy_calibrate(struct phy *phy)

static?

> +{
> +	struct samsung_ufs_phy *ufs_phy = get_samsung_ufs_phy(phy);
> +	struct samsung_ufs_phy_cfg **cfgs = ufs_phy->cfg;
> +	const struct samsung_ufs_phy_cfg *cfg;
> +	int i;
> +	int err = 0;

err before i would make it look better

> +
> +	if (unlikely(ufs_phy->ufs_phy_state < CFG_PRE_INIT ||
> +		     ufs_phy->ufs_phy_state >= CFG_TAG_MAX)) {
> +		dev_err(ufs_phy->dev, "invalid phy config index %d\n",
> +							ufs_phy->ufs_phy_state);

single line now?

> +		return -EINVAL;
> +	}
> +
> +	if (ufs_phy->is_pre_init)
> +		ufs_phy->is_pre_init = false;

that sounds bit strange, you clear it if set? Can you explain what is
going on here, and add comments

> +static int samsung_ufs_phy_symbol_clk_init(struct samsung_ufs_phy *phy)
> +{
> +	int ret = 0;

superfluous init

> +
> +	phy->tx0_symbol_clk = devm_clk_get(phy->dev, "tx0_symbol_clk");
> +	if (IS_ERR(phy->tx0_symbol_clk)) {
> +		dev_err(phy->dev, "failed to get tx0_symbol_clk clock\n");
> +		goto out;
> +	}
> +
> +	phy->rx0_symbol_clk = devm_clk_get(phy->dev, "rx0_symbol_clk");
> +	if (IS_ERR(phy->rx0_symbol_clk)) {
> +		dev_err(phy->dev, "failed to get rx0_symbol_clk clock\n");
> +		goto out;
> +	}
> +
> +	phy->rx1_symbol_clk = devm_clk_get(phy->dev, "rx1_symbol_clk");
> +	if (IS_ERR(phy->rx0_symbol_clk)) {
> +		dev_err(phy->dev, "failed to get rx1_symbol_clk clock\n");
> +		goto out;
> +	}
> +
> +	ret = clk_prepare_enable(phy->tx0_symbol_clk);
> +	if (ret) {
> +		dev_err(phy->dev, "%s: tx0_symbol_clk enable failed %d\n",
> +				__func__, ret);
> +		goto out;
> +	}
> +	ret = clk_prepare_enable(phy->rx0_symbol_clk);
> +	if (ret) {
> +		dev_err(phy->dev, "%s: rx0_symbol_clk enable failed %d\n",
> +				__func__, ret);

so we keep tx0_symbol_clk enabled when bailing out?

> +		goto out;
> +	}
> +	ret = clk_prepare_enable(phy->rx1_symbol_clk);
> +	if (ret) {
> +		dev_err(phy->dev, "%s: rx1_symbol_clk enable failed %d\n",
> +				__func__, ret);

here as well

> +static int samsung_ufs_phy_init(struct phy *phy)
> +{
> +	struct samsung_ufs_phy *_phy = get_samsung_ufs_phy(phy);
> +	int ret;
> +
> +	_phy->lane_cnt = phy->attrs.bus_width;
> +	_phy->ufs_phy_state = CFG_PRE_INIT;
> +
> +	/**
> +	 * In ufs, PHY need to be calibrated at different stages / state
> +	 * mainly before Linkstartup, after Linkstartup, before power
> +	 * mode change and after power mode change.
> +	 * Below state machine initialize the initial state to handle
> +	 * PHY calibration at various stages of UFS initialization and power
> +	 * mode changes
> +	 */
> +	_phy->is_pre_init = true;
> +	_phy->is_post_init = false;
> +	_phy->is_pre_pmc = false;
> +	_phy->is_post_pmc = false;

hmm why not have phy_state and assign that
pre_init/post_init/pre_pmc/post_pmc states?

> +static int samsung_ufs_phy_set_mode(struct phy *generic_phy,
> +					enum phy_mode mode, int submode)

pls align this to preceding line opening brace (tip: checkpatch with
--strict can tell you about these)
Alim Akhtar July 3, 2020, 1:27 a.m. UTC | #2 
Hi Vinod

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: 01 July 2020 12:23
> To: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: robh+dt@kernel.org; krzk@kernel.org; kwmad.kim@samsung.com;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; kishon@ti.com
> Subject: Re: [RESEND PATCH v10 2/2] phy: samsung-ufs: add UFS PHY driver
for
> samsung SoC
> 
> Hi Alim,
> 
> On 25-06-20, 05:26, Alim Akhtar wrote:
> 
> > +int samsung_ufs_phy_wait_for_lock_acq(struct phy *phy)
> 
> static ?
> 
Sure, already got warning email from Kobot. Will fix this.
> > +{
> > +	struct samsung_ufs_phy *ufs_phy = get_samsung_ufs_phy(phy);
> > +	const unsigned int timeout_us = 100000;
> > +	const unsigned int sleep_us = 10;
> > +	u32 val;
> > +	int err;
> > +
> > +	err = readl_poll_timeout(
> > +			ufs_phy->reg_pma +
> PHY_APB_ADDR(PHY_PLL_LOCK_STATUS),
> > +			val, (val & PHY_PLL_LOCK_BIT), sleep_us,
timeout_us);
> > +	if (err) {
> > +		dev_err(ufs_phy->dev,
> > +			"failed to get phy pll lock acquisition %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	err = readl_poll_timeout(
> > +			ufs_phy->reg_pma +
> PHY_APB_ADDR(PHY_CDR_LOCK_STATUS),
> > +			val, (val & PHY_CDR_LOCK_BIT), sleep_us,
timeout_us);
> > +	if (err) {
> > +		dev_err(ufs_phy->dev,
> > +			"failed to get phy cdr lock acquisition %d\n", err);
> > +		goto out;
> 
> this one can be dropped
> 
Sure, will update.
> > +	}
> > +
> > +out:
> > +	return err;
> > +}
> > +
> > +int samsung_ufs_phy_calibrate(struct phy *phy)
> 
> static?
> 
Will fix
> > +{
> > +	struct samsung_ufs_phy *ufs_phy = get_samsung_ufs_phy(phy);
> > +	struct samsung_ufs_phy_cfg **cfgs = ufs_phy->cfg;
> > +	const struct samsung_ufs_phy_cfg *cfg;
> > +	int i;
> > +	int err = 0;
> 
> err before i would make it look better
> 
sure
> > +
> > +	if (unlikely(ufs_phy->ufs_phy_state < CFG_PRE_INIT ||
> > +		     ufs_phy->ufs_phy_state >= CFG_TAG_MAX)) {
> > +		dev_err(ufs_phy->dev, "invalid phy config index %d\n",
> > +							ufs_phy-
> >ufs_phy_state);
> 
> single line now?
> 
Yes, 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ufs_phy->is_pre_init)
> > +		ufs_phy->is_pre_init = false;
> 
> that sounds bit strange, you clear it if set? Can you explain what is
going on
> here, and add comments
> 
Hmm, yes right, this is not needed, let me change this and will add a
comment.
The idea here is, before exiting phy calibration in one state, change the
state to next state,
So that when next time calibrate() is called, it will do the phy settings
for the next stage.

> > +static int samsung_ufs_phy_symbol_clk_init(struct samsung_ufs_phy
> > +*phy) {
> > +	int ret = 0;
> 
> superfluous init
> 
ok
> > +
> > +	phy->tx0_symbol_clk = devm_clk_get(phy->dev, "tx0_symbol_clk");
> > +	if (IS_ERR(phy->tx0_symbol_clk)) {
> > +		dev_err(phy->dev, "failed to get tx0_symbol_clk clock\n");
> > +		goto out;
> > +	}
> > +
> > +	phy->rx0_symbol_clk = devm_clk_get(phy->dev, "rx0_symbol_clk");
> > +	if (IS_ERR(phy->rx0_symbol_clk)) {
> > +		dev_err(phy->dev, "failed to get rx0_symbol_clk clock\n");
> > +		goto out;
> > +	}
> > +
> > +	phy->rx1_symbol_clk = devm_clk_get(phy->dev, "rx1_symbol_clk");
> > +	if (IS_ERR(phy->rx0_symbol_clk)) {
> > +		dev_err(phy->dev, "failed to get rx1_symbol_clk clock\n");
> > +		goto out;
> > +	}
> > +
> > +	ret = clk_prepare_enable(phy->tx0_symbol_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: tx0_symbol_clk enable failed %d\n",
> > +				__func__, ret);
> > +		goto out;
> > +	}
> > +	ret = clk_prepare_enable(phy->rx0_symbol_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: rx0_symbol_clk enable failed %d\n",
> > +				__func__, ret);
> 
> so we keep tx0_symbol_clk enabled when bailing out?
> 
Will add a clk_disable_unprepare()
> > +		goto out;
> > +	}
> > +	ret = clk_prepare_enable(phy->rx1_symbol_clk);
> > +	if (ret) {
> > +		dev_err(phy->dev, "%s: rx1_symbol_clk enable failed %d\n",
> > +				__func__, ret);
> 
> here as well
> 
Will add a clk_disable_unprepare()
> > +static int samsung_ufs_phy_init(struct phy *phy) {
> > +	struct samsung_ufs_phy *_phy = get_samsung_ufs_phy(phy);
> > +	int ret;
> > +
> > +	_phy->lane_cnt = phy->attrs.bus_width;
> > +	_phy->ufs_phy_state = CFG_PRE_INIT;
> > +
> > +	/**
> > +	 * In ufs, PHY need to be calibrated at different stages / state
> > +	 * mainly before Linkstartup, after Linkstartup, before power
> > +	 * mode change and after power mode change.
> > +	 * Below state machine initialize the initial state to handle
> > +	 * PHY calibration at various stages of UFS initialization and power
> > +	 * mode changes
> > +	 */
> > +	_phy->is_pre_init = true;
> > +	_phy->is_post_init = false;
> > +	_phy->is_pre_pmc = false;
> > +	_phy->is_post_pmc = false;
> 
> hmm why not have phy_state and assign that
> pre_init/post_init/pre_pmc/post_pmc states?
> 
These are not needed, ufs_phy_state is enough to handle various stages.
Thanks, will remove and simplify this logic.

> > +static int samsung_ufs_phy_set_mode(struct phy *generic_phy,
> > +					enum phy_mode mode, int submode)
> 
> pls align this to preceding line opening brace (tip: checkpatch with
--strict can
> tell you about these)
Sure, will fix this, thanks for the tip.
> --
> ~Vinod
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/samsung,ufs-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,ufs-phy.yaml
new file mode 100644
index 000000000000..636cc501b54f
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/samsung,ufs-phy.yaml
@@ -0,0 +1,75 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/samsung,ufs-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung SoC series UFS PHY Device Tree Bindings
+
+maintainers:
+  - Alim Akhtar <alim.akhtar@samsung.com>
+
+properties:
+  "#phy-cells":
+    const: 0
+
+  compatible:
+    enum:
+      - samsung,exynos7-ufs-phy
+
+  reg:
+    maxItems: 1
+
+  reg-names:
+    items:
+      - const: phy-pma
+
+  clocks:
+    items:
+      - description: PLL reference clock
+      - description: symbol clock for input symbol ( rx0-ch0 symbol clock)
+      - description: symbol clock for input symbol ( rx1-ch1 symbol clock)
+      - description: symbol clock for output symbol ( tx0 symbol clock)
+
+  clock-names:
+    items:
+      - const: ref_clk
+      - const: rx1_symbol_clk
+      - const: rx0_symbol_clk
+      - const: tx0_symbol_clk
+
+  samsung,pmu-syscon:
+    $ref: '/schemas/types.yaml#/definitions/phandle'
+    description: phandle for PMU system controller interface, used to
+                 control pmu registers bits for ufs m-phy
+
+required:
+  - "#phy-cells"
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - samsung,pmu-syscon
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/exynos7-clk.h>
+
+    ufs_phy: ufs-phy@15571800 {
+        compatible = "samsung,exynos7-ufs-phy";
+        reg = <0x15571800 0x240>;
+        reg-names = "phy-pma";
+        samsung,pmu-syscon = <&pmu_system_controller>;
+        #phy-cells = <0>;
+        clocks = <&clock_fsys1 SCLK_COMBO_PHY_EMBEDDED_26M>,
+                 <&clock_fsys1 PHYCLK_UFS20_RX1_SYMBOL_USER>,
+                 <&clock_fsys1 PHYCLK_UFS20_RX0_SYMBOL_USER>,
+                 <&clock_fsys1 PHYCLK_UFS20_TX0_SYMBOL_USER>;
+        clock-names = "ref_clk", "rx1_symbol_clk",
+                      "rx0_symbol_clk", "tx0_symbol_clk";
+
+    };
+...