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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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?

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 ?

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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux