| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b0a3da6d-84e2-44c2-9adf-421e306a2acf@rock-chips.com>
Date: Wed, 24 Jan 2024 15:30:34 +0800
From: Andy Yan <andy.yan@...k-chips.com>
To: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>,
Sascha Hauer <s.hauer@...gutronix.de>
Cc: Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I
<kishon@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
Philipp Zabel <p.zabel@...gutronix.de>, Johan Jonker <jbx6244@...il.com>,
Sebastian Reichel <sebastian.reichel@...labora.com>,
Algea Cao <algea.cao@...k-chips.com>, linux-phy@...ts.infradead.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
kernel@...labora.com
Subject: Re: [PATCH 3/3] phy: rockchip: Add Samsung HDMI/DP Combo PHY driver
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@...k-chips.com>
>>>> Signed-off-by: Algea Cao <algea.cao@...k-chips.com>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.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@...k-chips.com>
>>>> + * Author: Cristian Ciocaltea <cristian.ciocaltea@...labora.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 ?
>
> 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
Powered by blists - more mailing lists