Re: [PATCH v4 02/10] phy: rockchip: add usbdp combo phy driver

[Vinod Koul]

On 25-03-24, 15:15, Sebastian Reichel wrote:
> This adds a new USBDP combo PHY with Samsung IP block driver.
> 
> The driver get lane mux and mapping info in 2 ways, supporting
> DisplayPort alternate mode or parsing from DT. When parsing from DT,
> the property "rockchip,dp-lane-mux" provide the DP mux and mapping
> info. This is needed when the PHY is not used with TypeC Alt-Mode.
> For example if the USB3 interface of the PHY is connected to a USB
> Type A connector and the DP interface is connected to a DisplayPort
> connector.
> 
> When do DP link training, need to set lane number, link rate, swing,
> and pre-emphasis via PHY configure interface.
> 
> Co-developed-by: Heiko Stuebner <heiko@xxxxxxxxx>
> Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> Co-developed-by: Zhang Yubing <yubing.zhang@xxxxxxxxxxxxxx>
> Signed-off-by: Zhang Yubing <yubing.zhang@xxxxxxxxxxxxxx>
> Co-developed-by: Frank Wang <frank.wang@xxxxxxxxxxxxxx>
> Signed-off-by: Frank Wang <frank.wang@xxxxxxxxxxxxxx>
> Tested-by: Heiko Stuebner <heiko@xxxxxxxxx>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> ---
>  drivers/phy/rockchip/Kconfig              |   12 +
>  drivers/phy/rockchip/Makefile             |    1 +
>  drivers/phy/rockchip/phy-rockchip-usbdp.c | 1612 +++++++++++++++++++++
>  3 files changed, 1625 insertions(+)
>  create mode 100644 drivers/phy/rockchip/phy-rockchip-usbdp.c
> 
> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> index a34f67bb7e61..c3d62243b474 100644
> --- a/drivers/phy/rockchip/Kconfig
> +++ b/drivers/phy/rockchip/Kconfig
> @@ -115,3 +115,15 @@ config PHY_ROCKCHIP_USB
>  	select GENERIC_PHY
>  	help
>  	  Enable this to support the Rockchip USB 2.0 PHY.
> +
> +config PHY_ROCKCHIP_USBDP
> +	tristate "Rockchip USBDP COMBO PHY Driver"
> +	depends on ARCH_ROCKCHIP && OF
> +	select GENERIC_PHY
> +	select TYPEC
> +	help
> +	  Enable this to support the Rockchip USB3.0/DP combo PHY with
> +	  Samsung IP block. This is required for USB3 support on RK3588.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called phy-rockchip-usbdp
> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
> index 3d911304e654..010a824e32ce 100644
> --- a/drivers/phy/rockchip/Makefile
> +++ b/drivers/phy/rockchip/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_PHY_ROCKCHIP_SAMSUNG_HDPTX)	+= phy-rockchip-samsung-hdptx.o
>  obj-$(CONFIG_PHY_ROCKCHIP_SNPS_PCIE3)	+= phy-rockchip-snps-pcie3.o
>  obj-$(CONFIG_PHY_ROCKCHIP_TYPEC)	+= phy-rockchip-typec.o
>  obj-$(CONFIG_PHY_ROCKCHIP_USB)		+= phy-rockchip-usb.o
> +obj-$(CONFIG_PHY_ROCKCHIP_USBDP)	+= phy-rockchip-usbdp.o
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> new file mode 100644
> index 000000000000..38dc96cfe403
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -0,0 +1,1612 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Rockchip USBDP Combo PHY with Samsung IP block driver
> + *
> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd

we are in 2024!

> + */
> +
> +#include <dt-bindings/phy/phy.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

Do you need both

> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_mux.h>

That seems a bit more of headers, do you need all of them?

> +static int rk_udphy_setup(struct rk_udphy *udphy)
> +{
> +	int ret = 0;

Superfluous init

> +static int rk_udphy_parse_dt(struct rk_udphy *udphy)
> +{
> +	struct device *dev = udphy->dev;
> +	struct device_node *np = dev_of_node(dev);
> +	enum usb_device_speed maximum_speed;
> +	int ret;
> +
> +	udphy->u2phygrf = syscon_regmap_lookup_by_phandle(np, "rockchip,u2phy-grf");
> +	if (IS_ERR(udphy->u2phygrf))
> +		return dev_err_probe(dev, PTR_ERR(udphy->u2phygrf), "failed to get u2phy-grf\n");
> +
> +	udphy->udphygrf = syscon_regmap_lookup_by_phandle(np, "rockchip,usbdpphy-grf");
> +	if (IS_ERR(udphy->udphygrf))
> +		return dev_err_probe(dev, PTR_ERR(udphy->udphygrf), "failed to get usbdpphy-grf\n");
> +
> +	udphy->usbgrf = syscon_regmap_lookup_by_phandle(np, "rockchip,usb-grf");
> +	if (IS_ERR(udphy->usbgrf))
> +		return dev_err_probe(dev, PTR_ERR(udphy->usbgrf), "failed to get usb-grf\n");
> +
> +	udphy->vogrf = syscon_regmap_lookup_by_phandle(np, "rockchip,vo-grf");
> +	if (IS_ERR(udphy->vogrf))
> +		return dev_err_probe(dev, PTR_ERR(udphy->vogrf), "failed to get vo-grf\n");
> +
> +	ret = rk_udphy_parse_lane_mux_data(udphy);
> +	if (ret)
> +		return ret;
> +
> +	udphy->sbu1_dc_gpio = devm_gpiod_get_optional(dev, "sbu1-dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(udphy->sbu1_dc_gpio))
> +		return PTR_ERR(udphy->sbu1_dc_gpio);
> +
> +	udphy->sbu2_dc_gpio = devm_gpiod_get_optional(dev, "sbu2-dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(udphy->sbu2_dc_gpio))
> +		return PTR_ERR(udphy->sbu2_dc_gpio);
> +
> +	if (device_property_present(dev, "maximum-speed")) {
> +		maximum_speed = usb_get_maximum_speed(dev);
> +		udphy->hs = maximum_speed <= USB_SPEED_HIGH ? true : false;
> +	}
> +
> +	ret = rk_udphy_clk_init(udphy, dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rk_udphy_reset_init(udphy, dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return rk_udphy_reset_init()

> +static const struct phy_ops rk_udphy_dp_phy_ops = {
> +	.init		= rk_udphy_dp_phy_init,
> +	.exit		= rk_udphy_dp_phy_exit,
> +	.power_on	= rk_udphy_dp_phy_power_on,
> +	.power_off	= rk_udphy_dp_phy_power_off,
> +	.configure	= rk_udphy_dp_phy_configure,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int rk_udphy_usb3_phy_init(struct phy *phy)
> +{
> +	struct rk_udphy *udphy = phy_get_drvdata(phy);
> +	int ret = 0;

Superfluous init here too

-- 
~Vinod