| 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: <fynyo2amqillioxwfyydvztakba5ecwa2qrtdtuoaffyvwc62c@3vizyubfqvsf>
Date: Tue, 5 Nov 2024 15:53:40 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Heiko Stuebner <heiko@...ech.de>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, quentin.schulz@...rry.de,
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,
Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH v2 2/2] phy: rockchip: Add Samsung CSI/DSI Combo DCPHY
driver
Hi,
On Mon, Nov 04, 2024 at 12:11:16PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@...rry.de>
>
> Add phy driver needed to drive either a MIPI DSI output to a DSI display
> or MIPI CSI input from a camera on rk3588.
>
> Right now only the DSI portion is implemented as the whole camera part
> needs more work in general.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@...rry.de>
> ---
Do you have a git branch with the DSI controller driver, so that
this can actually be tested? :)
> drivers/phy/rockchip/Kconfig | 12 +
> drivers/phy/rockchip/Makefile | 1 +
> .../phy/rockchip/phy-rockchip-samsung-dcphy.c | 1654 +++++++++++++++++
> 3 files changed, 1667 insertions(+)
> create mode 100644 drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
>
> diff --git a/drivers/phy/rockchip/Kconfig b/drivers/phy/rockchip/Kconfig
> index 2f7a05f21dc5..2bfb42996503 100644
> --- a/drivers/phy/rockchip/Kconfig
> +++ b/drivers/phy/rockchip/Kconfig
> @@ -83,6 +83,18 @@ config PHY_ROCKCHIP_PCIE
> help
> Enable this to support the Rockchip PCIe PHY.
>
> +config PHY_ROCKCHIP_SAMSUNG_DCPHY
> + tristate "Rockchip Samsung MIPI DCPHY driver"
> + depends on (ARCH_ROCKCHIP || COMPILE_TEST)
> + select GENERIC_PHY
> + select GENERIC_PHY_MIPI_DPHY
> + help
> + Enable this to support the Rockchip MIPI DCPHY with
> + Samsung IP block.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called phy-rockchip-samsung-dcphy
> +
> config PHY_ROCKCHIP_SAMSUNG_HDPTX
> tristate "Rockchip Samsung HDMI/eDP Combo PHY driver"
> depends on (ARCH_ROCKCHIP || COMPILE_TEST) && OF
> diff --git a/drivers/phy/rockchip/Makefile b/drivers/phy/rockchip/Makefile
> index 010a824e32ce..117aaffd037d 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_DCPHY) += phy-rockchip-samsung-dcphy.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
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> new file mode 100644
> index 000000000000..a2f897fa5516
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> @@ -0,0 +1,1654 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) Rockchip Electronics Co.Ltd
> + * Author:
> + * Guochun Huang <hero.huang@...k-chips.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
I think this should be
#include <linux/mod_devicetable.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
[...]
> +static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *samsung)
> +{
> + u32 bias_con2 = 0x3223;
> +
> + regmap_write(samsung->regmap, BIAS_CON0, 0x0010);
> + regmap_write(samsung->regmap, BIAS_CON1, 0x0110);
> + regmap_write(samsung->regmap, BIAS_CON2, bias_con2);
> +
> + /* default output voltage select:
> + * dphy: 400mv
> + * cphy: 530mv
> + */
> + regmap_update_bits(samsung->regmap, BIAS_CON4,
> + I_MUX_SEL_MASK, I_MUX_SEL_400MV);
> +}
> +
> +static void samsung_mipi_dcphy_bias_block_disable(struct samsung_mipi_dcphy *samsung)
> +{
> +}
uhm? :)
[...]
> +static const struct samsung_mipi_dphy_timing *
> +samsung_mipi_dphy_get_timing(struct samsung_mipi_dcphy *samsung)
> +{
> + const struct samsung_mipi_dphy_timing *timings;
> + unsigned int num_timings;
> + unsigned int lane_mbps = div64_ul(samsung->pll.rate, USEC_PER_SEC);
> + unsigned int i;
> +
> + timings = samsung_mipi_dphy_timing_table;
> + num_timings = ARRAY_SIZE(samsung_mipi_dphy_timing_table);
> +
> + for (i = num_timings; i > 0; i--)
> + if (lane_mbps <= timings[i - 1].max_lane_mbps)
> + break;
> +
> + if (i == 0)
> + ++i;
I guess you can just do 'for (i = max; i > 1; i--)' instead of
counting to 0 and then go back to 1?
> +
> + return &timings[i - 1];
> +}
[...]
> +static int samsung_mipi_dphy_power_on(struct samsung_mipi_dcphy *samsung)
> +{
> + int ret;
> +
> + reset_control_assert(samsung->m_phy_rst);
> +
> + samsung_mipi_dcphy_bias_block_enable(samsung);
> + samsung_mipi_dcphy_pll_configure(samsung);
> + samsung_mipi_dphy_clk_lane_timing_init(samsung);
> + samsung_mipi_dphy_data_lane_timing_init(samsung);
> + ret = samsung_mipi_dcphy_pll_enable(samsung);
> + if (ret < 0) {
> + samsung_mipi_dcphy_bias_block_disable(samsung);
> + return ret;
> + }
> +
> + samsung_mipi_dphy_lane_enable(samsung);
> +
> + reset_control_deassert(samsung->m_phy_rst);
> +
> + /* The TSKEWCAL maximum is 100 µsec
> + * at initial calibration.
> + */
> + usleep_range(100, 110);
> +
> + return 0;
> +}
> +
> +static int samsung_mipi_dcphy_power_on(struct phy *phy)
> +{
> + struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> + enum phy_mode mode = phy_get_mode(phy);
> +
> + pm_runtime_get_sync(samsung->dev);
This already happened in samsung_mipi_dcphy_init?
> + reset_control_assert(samsung->apb_rst);
> + udelay(1);
> + reset_control_deassert(samsung->apb_rst);
> +
> + switch (mode) {
> + case PHY_MODE_MIPI_DPHY:
> + return samsung_mipi_dphy_power_on(samsung);
> + default:
> + /* CSI cphy part to be implemented later */
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int samsung_mipi_dcphy_power_off(struct phy *phy)
> +{
> + struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> + enum phy_mode mode = phy_get_mode(phy);
> +
> + switch (mode) {
> + case PHY_MODE_MIPI_DPHY:
> + samsung_mipi_dphy_lane_disable(samsung);
> + break;
> + default:
> + /* CSI cphy part to be implemented later */
> + return -EOPNOTSUPP;
> + }
> +
> + samsung_mipi_dcphy_pll_disable(samsung);
> + samsung_mipi_dcphy_bias_block_disable(samsung);
> +
> + pm_runtime_put(samsung->dev);
> +
> + return 0;
> +}
> +
> +static int samsung_mipi_dcphy_set_mode(struct phy *phy, enum phy_mode mode,
> + int submode)
> +{
> + return 0;
> +}
You can just remove this. phy_set_mode_ext() will return 0 byself if
the callback is NULL.
[...]
> +static int samsung_mipi_dcphy_init(struct phy *phy)
> +{
> + struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +
> + pm_runtime_get_sync(samsung->dev);
return pm_runtime_resume_and_get(samsung->dev);
> + return 0;
> +}
> +
> +static int samsung_mipi_dcphy_exit(struct phy *phy)
> +{
> + struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
> +
> + pm_runtime_put(samsung->dev);
return pm_runtime_put(samsung->dev);
> +
> + return 0;
> +}
[...]
> +static __maybe_unused int samsung_mipi_dcphy_runtime_suspend(struct device *dev)
> +{
> + struct samsung_mipi_dcphy *samsung = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(samsung->pclk);
> + clk_disable_unprepare(samsung->ref_clk);
> +
> + return 0;
> +}
> +
> +static __maybe_unused int samsung_mipi_dcphy_runtime_resume(struct device *dev)
> +{
> + struct samsung_mipi_dcphy *samsung = dev_get_drvdata(dev);
> +
> + clk_prepare_enable(samsung->pclk);
> + clk_prepare_enable(samsung->ref_clk);
> +
> + return 0;
> +}
No error checking for managing the clocks?
[...]
> +static const struct of_device_id samsung_mipi_dcphy_of_match[] = {
> + {
> + .compatible = "rockchip,rk3576-mipi-dcphy",
> + .data = &rk3576_samsung_mipi_dcphy_plat_data,
> + }, {
> + .compatible = "rockchip,rk3588-mipi-dcphy",
> + .data = &rk3588_samsung_mipi_dcphy_plat_data,
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, samsung_mipi_dcphy_of_match);
> +
> +static struct platform_driver samsung_mipi_dcphy_driver = {
> + .driver = {
> + .name = "samsung-mipi-dcphy",
> + .of_match_table = of_match_ptr(samsung_mipi_dcphy_of_match),
drop of_match_ptr(). The way it is used right now just brings
a compiler warning for CONFIG_OF=n.
> + .pm = &samsung_mipi_dcphy_pm_ops,
> + },
> + .probe = samsung_mipi_dcphy_probe,
> +};
> +module_platform_driver(samsung_mipi_dcphy_driver);
> +
> +MODULE_AUTHOR("Guochun Huang<hero.huang@...k-chips.com>");
space before <
> +MODULE_DESCRIPTION("Samsung MIPI DCPHY Driver");
> +MODULE_LICENSE("GPL");
Greetings,
-- Sebastian
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists