On 1/22/24 14:14, Sascha Hauer wrote:
> On Fri, Jan 19, 2024 at 09:38:03PM +0200, Cristian Ciocaltea wrote:
>> Add driver for the Rockchip HDMI/eDP TX Combo PHY found on RK3588 SoC.
>>
>> The PHY is based on a Samsung IP block and supports HDMI 2.1 TMDS, FRL
>> and eDP links. The maximum data rate is 12Gbps (HDMI 2.1 FRL), while
>> the minimum is 250Mbps (HDMI 2.1 TMDS).
>>
>> Co-developed-by: Algea Cao <algea.cao@xxxxxxxxxxxxxx>
>> Signed-off-by: Algea Cao <algea.cao@xxxxxxxxxxxxxx>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
>> ---
>> drivers/phy/rockchip/Kconfig | 8 +
>> drivers/phy/rockchip/Makefile | 1 +
>> .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 2045 +++++++++++++++++
>> 3 files changed, 2054 insertions(+)
>> create mode 100644 drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>
>> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
>> index 94360fc96a6f..95666ac6aa3b 100644
>> --- a/drivers/phy/rockchip/Kconfig
>> +++ b/drivers/phy/rockchip/Kconfig
>> @@ -83,6 +83,14 @@ config PHY_ROCKCHIP_PCIE
>> help
>> Enable this to support the Rockchip PCIe PHY.
>>
>> +config PHY_ROCKCHIP_SAMSUNG_HDPTX
>> + tristate "Rockchip Samsung HDMI/DP Combo PHY driver"
>> + depends on (ARCH_ROCKCHIP || COMPILE_TEST) && OF
>> + select GENERIC_PHY
>> + help
>> + Enable this to support the Rockchip HDMI/DP Combo PHY
>> + with Samsung IP block.
>> +
>> config PHY_ROCKCHIP_SNPS_PCIE3
>> tristate "Rockchip Snps PCIe3 PHY Driver"
>> depends on (ARCH_ROCKCHIP && OF) || COMPILE_TEST
>> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
>> index 7eab129230d1..3d911304e654 100644
>> --- a/drivers/phy/rockchip/Makefile
>> +++ b/drivers/phy/rockchip/Makefile
>> @@ -8,6 +8,7 @@ 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_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
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> new file mode 100644
>> index 000000000000..d8171ea5ce2b
>> --- /dev/null
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -0,0 +1,2045 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2021-2022 Rockchip Electronics Co., Ltd.
>> + * Copyright (c) 2024 Collabora Ltd.
>> + *
>> + * Author: Algea Cao <algea.cao@xxxxxxxxxxxxxx>
>> + * Author: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/rational.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#define GRF_HDPTX_CON0 0x00
>> +#define HDPTX_I_PLL_EN BIT(7)
>> +#define HDPTX_I_BIAS_EN BIT(6)
>> +#define HDPTX_I_BGR_EN BIT(5)
>> +#define GRF_HDPTX_STATUS 0x80
>> +#define HDPTX_O_PLL_LOCK_DONE BIT(3)
>> +#define HDPTX_O_PHY_CLK_RDY BIT(2)
>> +#define HDPTX_O_PHY_RDY BIT(1)
>> +#define HDPTX_O_SB_RDY BIT(0)
>> +
>> +#define CMN_REG0000 0x0000
>
> These register names are not particularly helpful. Maybe use a
>
> #define CMN_REG(x) ((x) * 4)
>
> Instead?
Yes, sounds good.
>> +
>> +static int hdptx_lcpll_frl_mode_config(struct rockchip_hdptx_phy *hdptx,
>> + u32 rate)
>> +{
>> + u32 bit_rate = rate & DATA_RATE_MASK;
>> + u8 color_depth = (rate & COLOR_DEPTH_MASK) ? 1 : 0;
>> + const struct lcpll_config *cfg = lcpll_cfg;
>> +
>> + for (; cfg->bit_rate != ~0; cfg++)
>> + if (bit_rate == cfg->bit_rate)
>> + break;
>
> You could use ARRAY_SIZE() to iterate over the array and save the extra
> entry at the end. Likewise for the other arrays used in the driver.
Sure, will do.
>> +
>> + if (cfg->bit_rate == ~0)
>> + return -EINVAL;
>> +
>
>> +static int rockchip_hdptx_phy_power_on(struct phy *phy)
>> +{
>> + struct rockchip_hdptx_phy *hdptx = phy_get_drvdata(phy);
>> + int bus_width = phy_get_bus_width(hdptx->phy);
>> + int bit_rate = bus_width & DATA_RATE_MASK;
>
> What is going on here? bus_width is set to 8 in probe() using
> phy_set_bus_width(), but the value you pull out of phy_get_bus_width()
> is expected to contain the bit_rate and several other flags.
>
> It looks like you are tunneling flags from some other driver using this
> field. Isn't there a better way to accomplish this? If not, I think this
> needs some explanation.
Indeed, sorry for missing a comment here. The flags are set by the
bridge driver to enable 10-bit color depth, FRL and EARC. So far I
couldn't find an alternative approach to pass custom data using the PHY API.
> At least the variable should be renamed. it's called "bus_width" and it's
> passed to functions like hdptx_lcpll_frl_mode_config() which has this
> parameter named "rate" which is quite confusing.
I think for the initial support it's not really necessary to implement
all those features. Andy, should we drop them until a better solution
is found?
> Sascha
Thanks for the review,
Cristian
