Re: [PATCH 1/2] phy: rockchip: Add Rockchip Synopsys PCIe 3.0 PHY

Hi Shawn,

On Thu, 14 Jan 2021 at 01:15, Shawn Lin shawn.lin@rock-chips.com wrote:

Add the Rockchip Synopsys based PCIe 3.0 PHY driver as part of Generic PHY framework.

Signed-off-by: Shawn Lin shawn.lin@rock-chips.com

drivers/phy/rockchip/Kconfig | 6 + drivers/phy/rockchip/Makefile | 1 + .../phy/rockchip/phy-rockchip-snps-pcie3.c | 146 ++++++++++++++++++ 3 files changed, 153 insertions(+) create mode 100644 drivers/phy/rockchip/phy-rockchip-snps-pcie3.c

Reviewed-by: Simon Glass sjg@chromium.org

nits below

diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig index 2318e71f35..b794cdaf6a 100644 --- a/drivers/phy/rockchip/Kconfig +++ b/drivers/phy/rockchip/Kconfig @@ -18,6 +18,12 @@ config PHY_ROCKCHIP_PCIE help Enable this to support the Rockchip PCIe PHY.

+config PHY_ROCKCHIP_SNPS_PCIE3

•   bool "Rockchip Snps PCIe3 PHY Driver"
  

•   depends on PHY && ARCH_ROCKCHIP
  

•   help
  

•     Support for Rockchip PCIe3 PHY with Synopsys IP block.
  

Can you mention the features here?

config PHY_ROCKCHIP_TYPEC bool "Rockchip TYPEC PHY Driver" depends on ARCH_ROCKCHIP diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile index 44049154f9..f6ad3bf59a 100644 --- a/drivers/phy/rockchip/Makefile +++ b/drivers/phy/rockchip/Makefile @@ -5,4 +5,5 @@

obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o +obj-$(CONFIG_PHY_ROCKCHIP_SNPS_PCIE3) += phy-rockchip-snps-pcie3.o obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c new file mode 100644 index 0000000000..3132f811b9 --- /dev/null +++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Rockchip PCIE3.0 phy driver
• • Copyright (C) 2021 Rockchip Electronics Co., Ltd.
• */

+#include <common.h> +#include <clk.h> +#include <dm.h> +#include <dm/lists.h> +#include <generic-phy.h> +#include <syscon.h> +#include <asm/io.h> +#include <dm/device_compat.h> +#include <regmap.h> +#include <reset-uclass.h>

Check header order

https://www.denx.de/wiki/U-Boot/CodingStyle

+#define GRF_PCIE30PHY_CON1 0x4 +#define GRF_PCIE30PHY_CON6 0x18 +#define GRF_PCIE30PHY_CON9 0x24

+struct rockchip_p3phy_priv {

•   void __iomem *mmio;
  

•   int mode;
  

please add comments for members

•   struct regmap *phy_grf;
  

•   struct reset_ctl p30phy;
  

•   struct clk ref_clk_m;
  

•   struct clk ref_clk_n;
  

•   struct clk pclk;
  

+};

+static int rochchip_p3phy_init(struct phy *phy) +{

•   struct rockchip_p3phy_priv *priv = dev_get_priv(phy->dev);
  

•   int ret;
  

•   ret = clk_enable(&priv->ref_clk_m);
  

•   if (ret < 0 && ret != -ENOSYS)
  

•           return ret;
  

•   ret = clk_enable(&priv->ref_clk_n);
  

•   if (ret < 0 && ret != -ENOSYS)
  

•           goto err_ref;
  

•   ret = clk_enable(&priv->pclk);
  

•   if (ret < 0 && ret != -ENOSYS)
  

•           goto err_pclk;
  

•   reset_assert(&priv->p30phy);
  

•   udelay(1);
  

•   /* Deassert PCIe PMA output clamp mode */
  

•   regmap_write(priv->phy_grf, GRF_PCIE30PHY_CON9,
  

•                (0x1 << 15) | (0x1 << 31));
  

•   reset_deassert(&priv->p30phy);
  

•   udelay(1);
  

•   return 0;
  

+err_pclk:

•   clk_disable(&priv->ref_clk_n);
  

+err_ref:

•   clk_disable(&priv->ref_clk_m);
  

blank line before final return (please fix throughout)

•   return ret;
  

+}

[...]

Regards, Simon