Re: [PATCH 3/3] phy: rockchip: Add Samsung HDMI/DP Combo PHY driver

From: Jonas Karlman <jonas@kwiboo.se>
To: Andy Yan <andy.yan@rock-chips.com>,
	Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
	Sascha Hauer <s.hauer@pengutronix.de>
Cc: Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Johan Jonker <jbx6244@gmail.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	Algea Cao <algea.cao@rock-chips.com>,
	linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	kernel@collabora.com, Sandor Yu <Sandor.yu@nxp.com>,
	Maxime Ripard <mripard@kernel.org>
Subject: Re: [PATCH 3/3] phy: rockchip: Add Samsung HDMI/DP Combo PHY driver
Date: Wed, 24 Jan 2024 10:16:29 +0100	[thread overview]
Message-ID: <ba598f02-5f74-4cb8-97e3-c9369e6db0d8@kwiboo.se> (raw)
In-Reply-To: <b0a3da6d-84e2-44c2-9adf-421e306a2acf@rock-chips.com>

On 2024-01-24 08:30, Andy Yan wrote:
> Hi Cristian:
> 
> On 1/24/24 10:42, Andy Yan wrote:
>> Hi Cristian:
>>
>> On 1/24/24 08:58, Cristian Ciocaltea wrote:
>>> 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@rock-chips.com>
>>>>> Signed-off-by: Algea Cao <algea.cao@rock-chips.com>
>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>> ---
>>>>>   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@rock-chips.com>
>>>>> + * Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>> + */
>>>>> +#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?
> 
> How about add a PHY_MODE_HDMI to enum phy_mode, and pass this custom data by extend phy_configure_opts
> or the submode of phy_set_mode_ext ?

Please see the patch "phy: Add HDMI configuration options" [1] from
Sandor Yu. Also the series "drm/connector: Create HDMI Connector
infrastructure" [2] from Maxime Ripard may be of interest.

[1] https://lore.kernel.org/all/19070c125268cfe900021dea6e7e8379b89c630e.1704785836.git.Sandor.yu@nxp.com/
[2] https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@kernel.org/

Regards,
Jonas

>>
>> I'm fine with it.
>> It would be very appreciated if some linux-phy or drm bridge experts can give
>> some suggestions about how to pass different custom phy modes.
>>
>>>
>>>> Sascha
>>>
>>> Thanks for the review,
>>> Cristian