by Vinod Koul

[permalink] [raw]

Subject: Re: [RFC PATCH v4 3/4] phy: rockchip: add naneng combo phy for RK3568

On 08-12-21, 19:54, Johan Jonker wrote:
> From: Yifeng Zhao <[email protected]>
>
> This patch implements a combo phy driver for Rockchip SoCs
> with NaNeng IP block. This phy can be used as pcie-phy, usb3-phy,
> sata-phy or sgmii-phy.
>
> Signed-off-by: Yifeng Zhao <[email protected]>
> Signed-off-by: Johan Jonker <[email protected]>
> ---
>
> TEST COMPILED ONLY!
> Driver not verified with hardware!
> ---
>
> Changed V4:
> restyle
> add devm_reset_control_array_get()
> remove clk structure
> change refclk DT parse
> change dev_err message
> add dot to phrase
> add ext_refclk variable
> add enable_ssc variable
> rename rockchip_combphy_param_write
> remove param_read
> replace rockchip-naneng-combphy driver name
> ---
> drivers/phy/rockchip/Kconfig | 8 +
> drivers/phy/rockchip/Makefile | 1 +
> .../rockchip/phy-rockchip-naneng-combphy.c | 608 ++++++++++++++++++
> 3 files changed, 617 insertions(+)
> create mode 100644 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>
> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> index e812adad7..9022e395c 100644
> --- a/drivers/phy/rockchip/Kconfig
> +++ b/drivers/phy/rockchip/Kconfig
> @@ -66,6 +66,14 @@ config PHY_ROCKCHIP_INNO_DSIDPHY
> Enable this to support the Rockchip MIPI/LVDS/TTL PHY with
> Innosilicon IP block.
>
> +config PHY_ROCKCHIP_NANENG_COMBO_PHY
> + tristate "Rockchip NANENG COMBO PHY Driver"
> + depends on ARCH_ROCKCHIP && OF
> + select GENERIC_PHY
> + help
> + Enable this to support the Rockchip PCIe/USB3.0/SATA/QSGMII
> + combo PHY with NaNeng IP block.
> +
> config PHY_ROCKCHIP_PCIE
> tristate "Rockchip PCIe PHY Driver"
> depends on (ARCH_ROCKCHIP && OF) || COMPILE_TEST
> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
> index f0eec212b..a5041efb5 100644
> --- a/drivers/phy/rockchip/Makefile
> +++ b/drivers/phy/rockchip/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_PHY_ROCKCHIP_INNO_CSIDPHY) += phy-rockchip-inno-csidphy.o
> obj-$(CONFIG_PHY_ROCKCHIP_INNO_DSIDPHY) += phy-rockchip-inno-dsidphy.o
> obj-$(CONFIG_PHY_ROCKCHIP_INNO_HDMI) += phy-rockchip-inno-hdmi.o
> obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
> +obj-$(CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY) += phy-rockchip-naneng-combphy.o
> obj-$(CONFIG_PHY_ROCKCHIP_PCIE) += phy-rockchip-pcie.o
> obj-$(CONFIG_PHY_ROCKCHIP_TYPEC) += phy-rockchip-typec.o
> obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> new file mode 100644
> index 000000000..56aaeed62
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -0,0 +1,608 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip PIPE USB3.0 PCIE SATA combphy driver
> + *
> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <dt-bindings/phy/phy.h>
> +
> +#define BIT_WRITEABLE_SHIFT 16
> +#define REF_CLOCK_24MHz 24000000

How about 24 * MEGA (see units.h)

> +#define REF_CLOCK_25MHz 25000000
> +#define REF_CLOCK_100MHz 100000000

empty line here pls

> +/* RK3568 T22 COMBO PHY REG */
> +#define RK3568_T22_PHYREG5 0x14
> +#define T22_PHYREG5_PLL_DIV_MASK GENMASK(7, 6)
> +#define T22_PHYREG5_PLL_DIV_SHIFT 6
> +#define T22_PHYREG5_PLL_DIV_2 1
> +
> +#define RK3568_T22_PHYREG6 0x18
> +#define T22_PHYREG6_TX_RTERM_MASK GENMASK(7, 4)
> +#define T22_PHYREG6_TX_RTERM_SHIFT 4
> +#define T22_PHYREG6_TX_RTERM_50OHM 0x8
> +#define T22_PHYREG6_RX_RTERM_MASK GENMASK(3, 0)
> +#define T22_PHYREG6_RX_RTERM_SHIFT 0

You can avoid using SHIFT macros by defining register masks using BIT,
GENMASK and then use FIELD_PREP() and FIELD_GET() to set and get
register values

> +#define T22_PHYREG6_RX_RTERM_44OHM 0xF
> +
> +#define RK3568_T22_PHYREG7 0x1C
> +#define T22_PHYREG7_SSC_EN BIT(4)
> +
> +#define RK3568_T22_PHYREG10 0x28
> +#define T22_PHYREG10_SU_TRIM_0_7 0xF0

GENMASK() ?

> +
> +#define RK3568_T22_PHYREG11 0x2C
> +#define T22_PHYREG11_PLL_LPF_ADJ 0x4

BIT(), here and other places applicable

> +
> +#define RK3568_T22_PHYREG12 0x30
> +#define T22_PHYREG12_RESISTER_MASK GENMASK(5, 4)
> +#define T22_PHYREG12_RESISTER_SHIFT 0x4
> +#define T22_PHYREG12_RESISTER_HIGH_Z 0x3
> +#define T22_PHYREG12_CKRCV_AMP0 BIT(7)
> +
> +#define RK3568_T22_PHYREG13 0x34
> +#define T22_PHYREG13_CKRCV_AMP1 BIT(0)
> +
> +#define RK3568_T22_PHYREG14 0x38
> +#define T22_PHYREG14_CTLE_EN BIT(0)
> +#define T22_PHYREG14_SSC_CNT_MASK GENMASK(7, 6)
> +#define T22_PHYREG14_SSC_CNT_SHIFT 6
> +#define T22_PHYREG14_SSC_CNT_VALUE 0x1
> +
> +#define RK3568_T22_PHYREG15 0x3C
> +#define T22_PHYREG15_SSC_CNT_VALUE 0x5f
> +
> +#define RK3568_T22_PHYREG17 0x44
> +#define T22_PHYREG17_PLL_LOOP 0x32
> +
> +#define RK3568_T22_PHYREG31 0x7C
> +#define T22_PHYREG31_SSC_MASK GENMASK(7, 4)
> +#define T22_PHYREG31_SSC_DIR_SHIFT 4
> +#define T22_PHYREG31_SSC_UPWARD 0
> +#define T22_PHYREG31_SSC_DOWNWARD 1
> +#define T22_PHYREG31_SSC_OFFSET_SHIFT 6
> +#define T22_PHYREG31_SSC_OFFSET_500PPM 1
> +
> +#define RK3568_T22_PHYREG32 0x80
> +#define T22_PHYREG32_PLL_KVCO_MASK GENMASK(4, 2)
> +#define T22_PHYREG32_PLL_KVCO_SHIFT 2
> +#define T22_PHYREG32_PLL_KVCO_VALUE 2
> +
> +struct rockchip_combphy_priv;
> +
> +struct combphy_reg {
> + u16 offset;
> + u16 bitend;
> + u16 bitstart;
> + u16 disable;
> + u16 enable;
> +};
> +
> +struct rockchip_combphy_grfcfg {
> + struct combphy_reg pcie_mode_set;
> + struct combphy_reg usb_mode_set;
> + struct combphy_reg sgmii_mode_set;
> + struct combphy_reg qsgmii_mode_set;
> + struct combphy_reg pipe_rxterm_set;
> + struct combphy_reg pipe_txelec_set;
> + struct combphy_reg pipe_txcomp_set;
> + struct combphy_reg pipe_clk_25m;
> + struct combphy_reg pipe_clk_100m;
> + struct combphy_reg pipe_phymode_sel;
> + struct combphy_reg pipe_rate_sel;
> + struct combphy_reg pipe_rxterm_sel;
> + struct combphy_reg pipe_txelec_sel;
> + struct combphy_reg pipe_txcomp_sel;
> + struct combphy_reg pipe_clk_ext;
> + struct combphy_reg pipe_sel_usb;
> + struct combphy_reg pipe_sel_qsgmii;
> + struct combphy_reg pipe_phy_status;
> + struct combphy_reg con0_for_pcie;
> + struct combphy_reg con1_for_pcie;
> + struct combphy_reg con2_for_pcie;
> + struct combphy_reg con3_for_pcie;
> + struct combphy_reg con0_for_sata;
> + struct combphy_reg con1_for_sata;
> + struct combphy_reg con2_for_sata;
> + struct combphy_reg con3_for_sata;
> + struct combphy_reg pipe_con0_for_sata;
> + struct combphy_reg pipe_sgmii_mac_sel;
> + struct combphy_reg pipe_xpcs_phy_ready;
> + struct combphy_reg u3otg0_port_en;
> + struct combphy_reg u3otg1_port_en;
> +};
> +
> +struct rockchip_combphy_cfg {
> + const struct rockchip_combphy_grfcfg *grfcfg;
> + int (*combphy_cfg)(struct rockchip_combphy_priv *priv);
> +};
> +
> +struct rockchip_combphy_priv {
> + u8 mode;
> + void __iomem *mmio;
> + int num_clks;
> + struct clk_bulk_data *clks;
> + struct device *dev;
> + struct regmap *pipe_grf;
> + struct regmap *phy_grf;
> + struct phy *phy;
> + struct reset_control *phy_rst;
> + const struct rockchip_combphy_cfg *cfg;
> + bool enable_ssc;
> + bool ext_refclk;
> + struct clk *refclk;
> +};
> +
> +static int rockchip_combphy_param_write(struct regmap *base,
> + const struct combphy_reg *reg, bool en)

pls align to previous line open brace (hint, checkpatch with --strict
will find these for you)

> +{
> + u32 val, mask, tmp;
> +
> + tmp = en ? reg->enable : reg->disable;
> + mask = GENMASK(reg->bitend, reg->bitstart);
> + val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
> +
> + return regmap_write(base, reg->offset, val);
> +}
> +
> +static u32 rockchip_combphy_is_ready(struct rockchip_combphy_priv *priv)
> +{
> + const struct rockchip_combphy_grfcfg *cfg = priv->cfg->grfcfg;
> + u32 mask, val;
> +
> + mask = GENMASK(cfg->pipe_phy_status.bitend,
> + cfg->pipe_phy_status.bitstart);
> +
> + regmap_read(priv->phy_grf, cfg->pipe_phy_status.offset, &val);
> + val = (val & mask) >> cfg->pipe_phy_status.bitstart;
> +
> + return val;
> +}
> +
> +static int rockchip_combphy_set_mode(struct rockchip_combphy_priv *priv)
> +{
> + int ret = 0;
> +
> + switch (priv->mode) {
> + case PHY_TYPE_PCIE:
> + case PHY_TYPE_USB3:
> + case PHY_TYPE_SATA:
> + case PHY_TYPE_SGMII:
> + case PHY_TYPE_QSGMII:
> + if (priv->cfg->combphy_cfg)
> + ret = priv->cfg->combphy_cfg(priv);
> + break;
> + default:
> + dev_err(priv->dev, "incompatible PHY type\n");
> + return -EINVAL;
> + }
> +
> + if (ret)
> + dev_err(priv->dev, "failed to init phy for phy mode %x\n", priv->mode);
> +
> + return ret;
> +}
> +
> +static int rockchip_combphy_init(struct phy *phy)
> +{
> + struct rockchip_combphy_priv *priv = phy_get_drvdata(phy);
> + const struct rockchip_combphy_grfcfg *cfg = priv->cfg->grfcfg;
> + u32 val;
> + int ret;
> +
> + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> + if (ret) {
> + dev_err(priv->dev, "failed to enable clks\n");
> + return ret;
> + }
> +
> + ret = rockchip_combphy_set_mode(priv);
> + if (ret)
> + goto err_clk;
> +
> + ret = reset_control_deassert(priv->phy_rst);
> + if (ret)
> + goto err_clk;
> +
> + if (priv->mode == PHY_TYPE_USB3) {
> + ret = readx_poll_timeout_atomic(rockchip_combphy_is_ready,
> + priv, val,
> + val == cfg->pipe_phy_status.enable,
> + 10, 1000);
> + if (ret)
> + dev_warn(priv->dev, "wait phy status ready timeout\n");
> + }
> +
> + return 0;
> +
> +err_clk:
> + clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> +
> + return ret;
> +}
> +
> +static int rockchip_combphy_exit(struct phy *phy)
> +{
> + struct rockchip_combphy_priv *priv = phy_get_drvdata(phy);
> +
> + clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> + reset_control_assert(priv->phy_rst);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops rochchip_combphy_ops = {
> + .init = rockchip_combphy_init,
> + .exit = rockchip_combphy_exit,
> + .owner = THIS_MODULE,
> +};
> +
> +static struct phy *rockchip_combphy_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> + struct rockchip_combphy_priv *priv = dev_get_drvdata(dev);
> +
> + if (args->args_count != 1) {
> + dev_err(dev, "invalid number of arguments\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (priv->mode != PHY_NONE && priv->mode != args->args[0])
> + dev_warn(dev, "phy type select %d overwriting type %d\n",
> + args->args[0], priv->mode);
> +
> + priv->mode = args->args[0];
> +
> + return priv->phy;
> +}
> +
> +static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy_priv *priv)
> +{
> + const struct rockchip_combphy_cfg *phy_cfg = priv->cfg;
> + int mac_id;
> + int i;
> +
> + priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks);
> + if (priv->num_clks < 1)
> + return -EINVAL;
> +
> + priv->refclk = NULL;
> + for (i = 0; i < priv->num_clks; i++) {
> + if (!strncmp(priv->clks[i].id, "ref", 3)) {
> + priv->refclk = priv->clks[i].clk;
> + break;
> + }
> + }
> +
> + if (!priv->refclk) {
> + dev_err(dev, "no refclk found\n");
> + return -EINVAL;
> + }
> +
> + priv->pipe_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,pipe-grf");
> + if (IS_ERR(priv->pipe_grf)) {
> + dev_err(dev, "failed to find peri_ctrl pipe-grf regmap\n");
> + return PTR_ERR(priv->pipe_grf);
> + }
> +
> + priv->phy_grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,pipe-phy-grf");
> + if (IS_ERR(priv->phy_grf)) {
> + dev_err(dev, "failed to find peri_ctrl pipe-phy-grf regmap\n");
> + return PTR_ERR(priv->phy_grf);
> + }
> +
> + if (device_property_present(dev, "rockchip,dis-u3otg0-port"))
> + rockchip_combphy_param_write(priv->pipe_grf, &phy_cfg->grfcfg->u3otg0_port_en,
> + false);
> +
> + if (device_property_present(dev, "rockchip,dis-u3otg1-port"))
> + rockchip_combphy_param_write(priv->pipe_grf, &phy_cfg->grfcfg->u3otg1_port_en,
> + false);
> +
> + priv->enable_ssc = device_property_present(dev, "rockchip,enable-ssc");
> +
> + priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
> +
> + if (!device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &mac_id) &&
> + (mac_id > 0))
> + rockchip_combphy_param_write(priv->pipe_grf, &phy_cfg->grfcfg->pipe_sgmii_mac_sel,
> + true);
> + else
> + rockchip_combphy_param_write(priv->pipe_grf, &phy_cfg->grfcfg->pipe_sgmii_mac_sel,
> + false);
> +
> + priv->phy_rst = devm_reset_control_array_get(dev, false, false);
> + if (IS_ERR(priv->phy_rst))
> + return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");
> +
> + return 0;
> +}
> +
> +static int rockchip_combphy_probe(struct platform_device *pdev)
> +{
> + struct phy_provider *phy_provider;
> + struct device *dev = &pdev->dev;
> + struct rockchip_combphy_priv *priv;
> + const struct rockchip_combphy_cfg *phy_cfg;
> + struct resource *res;
> + int ret;
> +
> + phy_cfg = of_device_get_match_data(dev);
> + if (!phy_cfg) {
> + dev_err(dev, "no OF match data provided\n");
> + return -EINVAL;
> + }
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->mmio = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> + if (IS_ERR(priv->mmio)) {
> + ret = PTR_ERR(priv->mmio);
> + return ret;
> + }
> +
> + priv->dev = dev;
> + priv->mode = PHY_NONE;
> + priv->cfg = phy_cfg;
> +
> + ret = rockchip_combphy_parse_dt(dev, priv);
> + if (ret)
> + return ret;
> +
> + ret = reset_control_assert(priv->phy_rst);
> + if (ret) {
> + dev_err(dev, "failed to reset phy\n");
> + return ret;
> + }
> +
> + priv->phy = devm_phy_create(dev, NULL, &rochchip_combphy_ops);
> + if (IS_ERR(priv->phy)) {
> + dev_err(dev, "failed to create combphy\n");
> + return PTR_ERR(priv->phy);
> + }
> +
> + dev_set_drvdata(dev, priv);
> + phy_set_drvdata(priv->phy, priv);
> +
> + phy_provider = devm_of_phy_provider_register(dev, rockchip_combphy_xlate);
> +
> + return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
> +{
> + const struct rockchip_combphy_grfcfg *cfg = priv->cfg->grfcfg;
> + unsigned long rate;
> + u32 val;
> +
> + switch (priv->mode) {
> + case PHY_TYPE_PCIE:
> + /* Set SSC downward spread spectrum. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG31);
> + val &= ~T22_PHYREG31_SSC_MASK;
> + val |= T22_PHYREG31_SSC_DOWNWARD << T22_PHYREG31_SSC_DIR_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG31);
> +
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con0_for_pcie, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
> + break;

empty line here helps readability

> + case PHY_TYPE_USB3:
> + /* Set SSC downward spread spectrum. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG31);
> + val &= ~T22_PHYREG31_SSC_MASK;
> + val |= T22_PHYREG31_SSC_DOWNWARD << T22_PHYREG31_SSC_DIR_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG31);

perhaps a updatel() macro to help with this?

> +
> + /* Enable adaptive CTLE for USB3.0 Rx. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG14);
> + val |= T22_PHYREG14_CTLE_EN;
> + writel(val, priv->mmio + RK3568_T22_PHYREG14);
> +
> + /* Set PLL KVCO fine tuning signals. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG32);
> + val &= ~T22_PHYREG32_PLL_KVCO_MASK;
> + val |= T22_PHYREG32_PLL_KVCO_VALUE << T22_PHYREG32_PLL_KVCO_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG32);
> +
> + /* Enable controlling random jitter. */
> + writel(T22_PHYREG11_PLL_LPF_ADJ, priv->mmio + RK3568_T22_PHYREG11);
> +
> + /* Set PLL input clock divider 1/2. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG5);
> + val &= ~T22_PHYREG5_PLL_DIV_MASK;
> + val |= T22_PHYREG5_PLL_DIV_2 << T22_PHYREG5_PLL_DIV_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG5);
> +
> + writel(T22_PHYREG17_PLL_LOOP, priv->mmio + RK3568_T22_PHYREG17);
> + writel(T22_PHYREG10_SU_TRIM_0_7, priv->mmio + RK3568_T22_PHYREG10);
> +
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_usb, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_txcomp_sel, false);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_txelec_sel, false);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->usb_mode_set, true);
> + break;
> + case PHY_TYPE_SATA:
> + /* Enable adaptive CTLE for SATA Rx. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG14);
> + val |= T22_PHYREG14_CTLE_EN;
> + writel(val, priv->mmio + RK3568_T22_PHYREG14);
> + /*
> + * Set tx_rterm=50ohm and rx_rterm=44ohm for SATA.
> + * 0: 60ohm, 8: 50ohm 15: 44ohm (by step abort 1ohm)
> + */
> + val = T22_PHYREG6_TX_RTERM_50OHM << T22_PHYREG6_TX_RTERM_SHIFT;
> + val |= T22_PHYREG6_RX_RTERM_44OHM << T22_PHYREG6_RX_RTERM_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG6);
> +
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con0_for_sata, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con1_for_sata, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con2_for_sata, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->con3_for_sata, true);
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_con0_for_sata, true);
> + break;
> + case PHY_TYPE_SGMII:
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->sgmii_mode_set, true);
> + break;
> + case PHY_TYPE_QSGMII:
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_rate_sel, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_sel_qsgmii, true);
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->qsgmii_mode_set, true);
> + break;
> + default:
> + dev_err(priv->dev, "incompatible PHY type\n");
> + return -EINVAL;
> + }
> +
> + rate = clk_get_rate(priv->refclk);
> +
> + switch (rate) {
> + case REF_CLOCK_24MHz:
> + if (priv->mode == PHY_TYPE_USB3 || priv->mode == PHY_TYPE_SATA) {
> + /* Set ssc_cnt[9:0]=0101111101 & 31.5KHz. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG14);
> + val &= ~T22_PHYREG14_SSC_CNT_MASK;
> + val |= T22_PHYREG14_SSC_CNT_VALUE << T22_PHYREG14_SSC_CNT_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG14);
> + writel(T22_PHYREG15_SSC_CNT_VALUE, priv->mmio + RK3568_T22_PHYREG15);
> + }
> + break;
> + case REF_CLOCK_25MHz:
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_25m, true);
> + break;
> + case REF_CLOCK_100MHz:
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_100m, true);
> + if (priv->mode == PHY_TYPE_PCIE) {
> + /* PLL KVCO fine tuning. */
> + val = readl(priv->mmio + RK3568_T22_PHYREG32);
> + val &= ~T22_PHYREG32_PLL_KVCO_MASK;
> + val |= T22_PHYREG32_PLL_KVCO_VALUE << T22_PHYREG32_PLL_KVCO_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG32);
> +
> + /* Enable controlling random jitter. */
> + writel(T22_PHYREG11_PLL_LPF_ADJ, priv->mmio + RK3568_T22_PHYREG11);
> +
> + val = readl(priv->mmio + RK3568_T22_PHYREG5);
> + val &= ~T22_PHYREG5_PLL_DIV_MASK;
> + val |= T22_PHYREG5_PLL_DIV_2 << T22_PHYREG5_PLL_DIV_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG5);
> +
> + writel(T22_PHYREG17_PLL_LOOP, priv->mmio + RK3568_T22_PHYREG17);
> + writel(T22_PHYREG10_SU_TRIM_0_7, priv->mmio + RK3568_T22_PHYREG10);
> + } else if (priv->mode == PHY_TYPE_SATA) {
> + /* downward spread spectrum +500ppm */
> + val = readl(priv->mmio + RK3568_T22_PHYREG31);
> + val &= ~T22_PHYREG31_SSC_MASK;
> + val |= T22_PHYREG31_SSC_DOWNWARD << T22_PHYREG31_SSC_DIR_SHIFT;
> + val |= T22_PHYREG31_SSC_OFFSET_500PPM << T22_PHYREG31_SSC_OFFSET_SHIFT;
> + writel(val, priv->mmio + RK3568_T22_PHYREG31);
> + }
> + break;
> + default:
> + dev_err(priv->dev, "unsupported rate: %lu\n", rate);
> + return -EINVAL;
> + }
> +
> + if (priv->ext_refclk) {
> + rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_clk_ext, true);
> + if (priv->mode == PHY_TYPE_PCIE && rate == REF_CLOCK_100MHz) {
> + val = readl(priv->mmio + RK3568_T22_PHYREG12);
> + val &= ~T22_PHYREG12_RESISTER_MASK;
> + val |= T22_PHYREG12_RESISTER_HIGH_Z << T22_PHYREG12_RESISTER_SHIFT;
> + val |= T22_PHYREG12_CKRCV_AMP0;
> + writel(val, priv->mmio + RK3568_T22_PHYREG12);
> +
> + val = readl(priv->mmio + RK3568_T22_PHYREG13);
> + val |= T22_PHYREG13_CKRCV_AMP1;
> + writel(val, priv->mmio + RK3568_T22_PHYREG13);
> + }
> + }
> +
> + if (priv->enable_ssc) {
> + val = readl(priv->mmio + RK3568_T22_PHYREG7);
> + val |= T22_PHYREG7_SSC_EN;
> + writel(val, priv->mmio + RK3568_T22_PHYREG7);
> + }
> +
> + return 0;
> +}
> +
> +static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
> + /* pipe-phy-grf */
> + .pcie_mode_set = { 0x0000, 5, 0, 0x00, 0x11 },
> + .usb_mode_set = { 0x0000, 5, 0, 0x00, 0x04 },
> + .sgmii_mode_set = { 0x0000, 5, 0, 0x00, 0x01 },
> + .qsgmii_mode_set = { 0x0000, 5, 0, 0x00, 0x21 },
> + .pipe_rxterm_set = { 0x0000, 12, 12, 0x00, 0x01 },
> + .pipe_txelec_set = { 0x0004, 1, 1, 0x00, 0x01 },
> + .pipe_txcomp_set = { 0x0004, 4, 4, 0x00, 0x01 },
> + .pipe_clk_25m = { 0x0004, 14, 13, 0x00, 0x01 },
> + .pipe_clk_100m = { 0x0004, 14, 13, 0x00, 0x02 },
> + .pipe_phymode_sel = { 0x0008, 1, 1, 0x00, 0x01 },
> + .pipe_rate_sel = { 0x0008, 2, 2, 0x00, 0x01 },
> + .pipe_rxterm_sel = { 0x0008, 8, 8, 0x00, 0x01 },
> + .pipe_txelec_sel = { 0x0008, 12, 12, 0x00, 0x01 },
> + .pipe_txcomp_sel = { 0x0008, 15, 15, 0x00, 0x01 },
> + .pipe_clk_ext = { 0x000c, 9, 8, 0x02, 0x01 },
> + .pipe_sel_usb = { 0x000c, 14, 13, 0x00, 0x01 },
> + .pipe_sel_qsgmii = { 0x000c, 15, 13, 0x00, 0x07 },
> + .pipe_phy_status = { 0x0034, 6, 6, 0x01, 0x00 },
> + .con0_for_pcie = { 0x0000, 15, 0, 0x00, 0x1000 },
> + .con1_for_pcie = { 0x0004, 15, 0, 0x00, 0x0000 },
> + .con2_for_pcie = { 0x0008, 15, 0, 0x00, 0x0101 },
> + .con3_for_pcie = { 0x000c, 15, 0, 0x00, 0x0200 },
> + .con0_for_sata = { 0x0000, 15, 0, 0x00, 0x0119 },
> + .con1_for_sata = { 0x0004, 15, 0, 0x00, 0x0040 },
> + .con2_for_sata = { 0x0008, 15, 0, 0x00, 0x80c3 },
> + .con3_for_sata = { 0x000c, 15, 0, 0x00, 0x4407 },
> + /* pipe-grf */
> + .pipe_con0_for_sata = { 0x0000, 15, 0, 0x00, 0x2220 },
> + .pipe_sgmii_mac_sel = { 0x0040, 1, 1, 0x00, 0x01 },
> + .pipe_xpcs_phy_ready = { 0x0040, 2, 2, 0x00, 0x01 },
> + .u3otg0_port_en = { 0x0104, 15, 0, 0x0181, 0x1100 },
> + .u3otg1_port_en = { 0x0144, 15, 0, 0x0181, 0x1100 },
> +};
> +
> +static const struct rockchip_combphy_cfg rk3568_combphy_cfgs = {
> + .grfcfg = &rk3568_combphy_grfcfgs,
> + .combphy_cfg = rk3568_combphy_cfg,
> +};
> +
> +static const struct of_device_id rockchip_combphy_of_match[] = {
> + {
> + .compatible = "rockchip,rk3568-naneng-combphy",
> + .data = &rk3568_combphy_cfgs,
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, rockchip_combphy_of_match);
> +
> +static struct platform_driver rockchip_combphy_driver = {
> + .probe = rockchip_combphy_probe,
> + .driver = {
> + .name = "rockchip-naneng-combphy",
> + .of_match_table = rockchip_combphy_of_match,
> + },
> +};
> +module_platform_driver(rockchip_combphy_driver);
> +
> +MODULE_DESCRIPTION("Rockchip NANENG COMBPHY driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.20.1

--
~Vinod

2021-12-14 12:20:31

by Johan Jonker

[permalink] [raw]

Subject: Re: [RFC PATCH v4 2/4] dt-bindings: phy: rockchip: Add Naneng combo PHY bindings

Hi,

On 12/14/21 9:58 AM, Vinod Koul wrote:
> On 08-12-21, 19:54, Johan Jonker wrote:
>> From: Yifeng Zhao <[email protected]>
>>
>> Add the compatible strings for the Naneng combo PHY found on rockchip SoC.
>

> Why is this series still tagged RFC..?

The phy DT nodes are urgent in need for other USB3, SATA and PCIe follow
up series. When the author doesn't respond for some time I can kick the
can a bit if it's for 'little' YAML, C style or DT changes. For larger
changes it's better to have the hardware tested as well, so I
carefully/politely marked it RFC as I don't know the author's intentions.

>
>>
>> Signed-off-by: Yifeng Zhao <[email protected]>
>> Signed-off-by: Johan Jonker <[email protected]>
>> ---
>>
>> Changed V4:
>> restyle
>> remove some minItems
>> add more properties
>> remove reset-names
>> move #phy-cells
>> add rockchip,rk3568-pipe-grf
>> add rockchip,rk3568-pipe-phy-grf
>> ---
>> .../phy/phy-rockchip-naneng-combphy.yaml | 127 ++++++++++++++++++
>> 1 file changed, 127 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
>> new file mode 100644
>> index 000000000..d309e2008
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
>> @@ -0,0 +1,127 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/phy-rockchip-naneng-combphy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Rockchip SoC Naneng Combo Phy Device Tree Bindings
>> +
>> +maintainers:
>> + - Heiko Stuebner <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - rockchip,rk3568-naneng-combphy
>> +
>> + reg:
>> + maxItems: 1
>> +

>> + clocks:
>> + items:
>> + - description: reference clock
>> + - description: apb clock
>> + - description: pipe clock
>
> no maxItems or minItems for this?

Documentation/devicetree/bindings/processed-schema.json

"clocks": {
"items": [
{},
{},
{}
],
"type": "array",
"minItems": 3,
"maxItems": 3,
"additionalItems": false
},

With 3 items the properties minItems and maxItems are automatically
added. Only when the number of clocks varies for example between 1 and 3
one should add minItems.

>
>> +
>> + clock-names:
>> + items:
>> + - const: ref
>> + - const: apb
>> + - const: pipe
>> +
>> + resets:
>> + items:
>> + - description: exclusive apb reset line
>> + - description: exclusive PHY reset line
>
> Ditto?
>
>> +

>> + rockchip,dis-u3otg0-port:
>> + type: boolean
>> + description:
>> + Disable the u3otg0 port.
>
> why not make it explicit and say rockchip,disable-u3otg0-port
>
> Also why should this port be disabled?

From Rockchip RK3568 Datasheet V1.0-20201210 page 16-17:

Multi-PHY0 support one of the following interfaces
USB3.0 OTG
SATA0

Multi-PHY1 support one of the following interfaces
USB3.0 Host
SATA1
QSGMII/SGMII

Multi-PHY2 support one of the following interfaces
PCIe2.1
SATA2
QSGMII/SGMII

===

Rockchip RK3568 TRM Part1 V1.0-20210111
page 233-234

PIPE_GRF_USB3OTG0_CON1
Address: Operational Base + offset (0x0104)

usb3otg0_host_u3_port_disable
USB 3.0 SS Port Disable control.
1'b0: Port Enabled
1'b1: Port Disabled

page 235-236

PIPE_GRF_USB3OTG1_CON1
Address: Operational Base + offset (0x0144)

usb3otg1_host_u3_port_disable
USB 3.0 SS Port Disable control.
1'b0: Port Enabled
1'b1: Port Disabled

===

https://www.cnx-software.com/2020/12/01/rockchip-rk3568-processor-to-power-edge-computing-and-nvr-applications/
https://eji4evk5kxx.exactdn.com/wp-content/uploads/2020/12/RK3568-multiplexed-sata-usb-3.0-pcie.jpg?lossy=1&ssl=1

===

USB3.0 OTG, USB3.0 HOT, SATA3.0, PCIE2.1, QSGMII are all multiplexed via
three Serdes lanes.
The driver in it's current state doesn't keep track of which phy[0-2]
node it probes I think. Nodes can be probed in random order, so it's not
able to tell if usb3otg0_host_u3_port_disable or
usb3otg1_host_u3_port_disable should be used. That why the author
probably choose to use a property.

Please advise if we need more complex logic, state locking, etc.
(Any example from the kernel source for that?)

(with more complexity I sould better pass this serie to somebody else)

Johan

>
>> +
>> + rockchip,dis-u3otg1-port:
>> + type: boolean
>> + description:
>> + Disable the u3otg1 port.
>
> ditto
>
>> +
>> + rockchip,enable-ssc:
>> + type: boolean
>> + description:
>> + In U3 and SATA mode the SSC option is already disabled by default.
>> + In PCIE mode the option SSC can be enabled.
>> + If Spread Spectrum Clocking (SSC) is used it is
>> + required that a common reference clock is used by the link partners.
>> + Most commercially available platforms with PCIe backplanes use
>> + SSC to reduce EMI.
>> +
>> + rockchip,ext-refclk:
>> + type: boolean
>> + description:
>> + Many PCIe connections, especially backplane connections,
>> + require a synchronous reference clock between the two link partners.
>> + To achieve this a common clock source, referred to as REFCLK in
>> + the PCI Express Card Electromechanical Specification,
>> + should be used by both ends of the PCIe link.
>> + The PCIe PHY provides 100MHz differential clock output
>> + (optional with SSC) in RC mode for system applications.
>> +
>> + rockchip,pipe-grf:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description:
>> + Some additional phy settings are accessed through GRF regs.
>> +
>> + rockchip,pipe-phy-grf:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description:
>> + Some additional pipe settings are accessed through GRF regs.
>> +
>> + rockchip,sgmii-mac-sel:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1]
>> + default: 0
>> + description:
>> + Select gmac0 or gmac1 to be used as SGMII controller.
>> +
>> + "#phy-cells":
>> + const: 1
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-names
>> + - resets
>> + - rockchip,pipe-grf
>> + - rockchip,pipe-phy-grf
>> + - "#phy-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/clock/rk3568-cru.h>
>> +
>> + pipegrf: syscon@fdc50000 {
>> + compatible = "rockchip,rk3568-pipe-grf", "syscon";
>> + reg = <0xfdc50000 0x1000>;
>> + };
>> +
>> + pipe_phy_grf0: syscon@fdc70000 {
>> + compatible = "rockchip,rk3568-pipe-phy-grf", "syscon";
>> + reg = <0xfdc70000 0x1000>;
>> + };
>> +
>> + combphy0: phy@fe820000 {
>> + compatible = "rockchip,rk3568-naneng-combphy";
>> + reg = <0xfe820000 0x100>;
>> + clocks = <&pmucru CLK_PCIEPHY0_REF>,
>> + <&cru PCLK_PIPEPHY0>,
>> + <&cru PCLK_PIPE>;
>> + clock-names = "ref", "apb", "pipe";
>> + assigned-clocks = <&pmucru CLK_PCIEPHY0_REF>;
>> + assigned-clock-rates = <100000000>;
>> + resets = <&cru SRST_P_PIPEPHY0>, <&cru SRST_PIPEPHY0>;
>> + rockchip,pipe-grf = <&pipegrf>;
>> + rockchip,pipe-phy-grf = <&pipe_phy_grf0>;
>> + #phy-cells = <1>;
>> + };
>> --
>> 2.20.1
>