From: Phi-bang Nguyen <pnguyen@baylibre.com>
This is a new driver that supports the MIPI CSI CD-PHY version 0.5
The number of PHYs depend on the soc.
Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
[Julien Stephan: use GENMASK]
[Julien Stephan: refactor code]
[Julien Stephan: update device tree support and probe function]
Co-developed-by: Julien Stephan <jstephan@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
MAINTAINERS | 1 +
drivers/phy/mediatek/Kconfig | 8 +
drivers/phy/mediatek/Makefile | 2 +
.../mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h | 58 ++++
drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c | 257 ++++++++++++++++++
5 files changed, 326 insertions(+)
create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 44f0ff11e984..fc2766cb50d3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13103,6 +13103,7 @@ M: Julien Stephan <jstephan@baylibre.com>
M: Andy Hsieh <andy.hsieh@mediatek.com>
S: Supported
F: Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
+F: drivers/phy/mediatek/phy-mtk-mipi-csi-0-5*
MEDIATEK MMC/SD/SDIO DRIVER
M: Chaotian Jing <chaotian.jing@mediatek.com>
diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
index 3125ecb5d119..452bc7ac5ce5 100644
--- a/drivers/phy/mediatek/Kconfig
+++ b/drivers/phy/mediatek/Kconfig
@@ -74,3 +74,11 @@ config PHY_MTK_DP
select GENERIC_PHY
help
Support DisplayPort PHY for MediaTek SoCs.
+
+config PHY_MTK_MIPI_CSI_0_5
+ tristate "MediaTek CSI CD-PHY v 0.5 Driver"
+ depends on ARCH_MEDIATEK && OF
+ select GENERIC_PHY
+ help
+ Enable this to support the MIPI CSI CD-PHY receiver version 0.5.
+ The driver supports multiple CSI cdphy ports simultaneously.
diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
index fb1f8edaffa7..8eb7b8747c67 100644
--- a/drivers/phy/mediatek/Makefile
+++ b/drivers/phy/mediatek/Makefile
@@ -18,3 +18,5 @@ phy-mtk-mipi-dsi-drv-y := phy-mtk-mipi-dsi.o
phy-mtk-mipi-dsi-drv-y += phy-mtk-mipi-dsi-mt8173.o
phy-mtk-mipi-dsi-drv-y += phy-mtk-mipi-dsi-mt8183.o
obj-$(CONFIG_PHY_MTK_MIPI_DSI) += phy-mtk-mipi-dsi-drv.o
+
+obj-$(CONFIG_PHY_MTK_MIPI_CSI_0_5) += phy-mtk-mipi-csi-0-5.o
diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
new file mode 100644
index 000000000000..e9a7f1ab3e2f
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
+#define __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
+
+/*
+ * CSI1 and CSI2 are identical, and similar to CSI0. All CSIx macros are
+ * applicable to the three PHYs. Where differences exist, they are denoted by
+ * macro names using CSI0 and CSI1, the latter being applicable to CSI1 and
+ * CSI2 alike.
+ */
+
+#define MIPI_RX_ANA00_CSIxA 0x0000
+#define RG_CSI0A_CPHY_EN BIT(0)
+#define RG_CSIxA_EQ_PROTECT_EN BIT(1)
+#define RG_CSIxA_BG_LPF_EN BIT(2)
+#define RG_CSIxA_BG_CORE_EN BIT(3)
+#define RG_CSIxA_DPHY_L0_CKMODE_EN BIT(5)
+#define RG_CSIxA_DPHY_L0_CKSEL BIT(6)
+#define RG_CSIxA_DPHY_L1_CKMODE_EN BIT(8)
+#define RG_CSIxA_DPHY_L1_CKSEL BIT(9)
+#define RG_CSIxA_DPHY_L2_CKMODE_EN BIT(11)
+#define RG_CSIxA_DPHY_L2_CKSEL BIT(12)
+
+#define MIPI_RX_ANA18_CSIxA 0x0018
+#define RG_CSI0A_L0_T0AB_EQ_IS GENMASK(5, 4)
+#define RG_CSI0A_L0_T0AB_EQ_BW GENMASK(7, 6)
+#define RG_CSI0A_L1_T1AB_EQ_IS GENMASK(21, 20)
+#define RG_CSI0A_L1_T1AB_EQ_BW GENMASK(23, 22)
+#define RG_CSI0A_L2_T1BC_EQ_IS GENMASK(21, 20)
+#define RG_CSI0A_L2_T1BC_EQ_BW GENMASK(23, 22)
+#define RG_CSI1A_L0_EQ_IS GENMASK(5, 4)
+#define RG_CSI1A_L0_EQ_BW GENMASK(7, 6)
+#define RG_CSI1A_L1_EQ_IS GENMASK(21, 20)
+#define RG_CSI1A_L1_EQ_BW GENMASK(23, 22)
+#define RG_CSI1A_L2_EQ_IS GENMASK(5, 4)
+#define RG_CSI1A_L2_EQ_BW GENMASK(7, 6)
+
+#define MIPI_RX_ANA1C_CSIxA 0x001c
+#define MIPI_RX_ANA20_CSI0A 0x0020
+
+#define MIPI_RX_ANA24_CSIxA 0x0024
+#define RG_CSIxA_RESERVE GENMASK(31, 24)
+
+#define MIPI_RX_ANA40_CSIxA 0x0040
+#define RG_CSIxA_CPHY_FMCK_SEL GENMASK(1, 0)
+#define RG_CSIxA_ASYNC_OPTION GENMASK(7, 4)
+#define RG_CSIxA_CPHY_SPARE GENMASK(31, 16)
+
+#define MIPI_RX_WRAPPER80_CSIxA 0x0080
+#define CSR_CSI_RST_MODE GENMASK(17, 16)
+
+#define MIPI_RX_ANAA8_CSIxA 0x00a8
+#define RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT BIT(0)
+#define RG_CSIxA_DPHY_L1_BYTECK_INVERT BIT(1)
+#define RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT BIT(2)
+
+#endif
diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
new file mode 100644
index 000000000000..ae2d3dc9631d
--- /dev/null
+++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "phy-mtk-io.h"
+#include "phy-mtk-mipi-csi-0-5-rx-reg.h"
+
+#define CSIxB_OFFSET 0x1000
+
+struct mtk_mipi_dphy;
+
+struct mtk_mipi_dphy_port {
+ struct device *dev;
+ void __iomem *base;
+ struct phy *phy;
+ bool is_cdphy;
+};
+
+static int mtk_mipi_phy_power_on(struct phy *phy)
+{
+ struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
+ void __iomem *base = port->base;
+
+ /*
+ * Only DPHY is supported for now,
+ * so set analog phy mode to DPHY in CDPHY compatible PHYs
+ */
+ if (port->is_cdphy) {
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSI0A_CPHY_EN, 0);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSI0A_CPHY_EN, 0);
+ }
+
+ /*
+ * Lane configuration:
+ *
+ * Only 4 data + 1 clock is supported for now with the following mapping:
+ *
+ * CSIxA_LNR0 --> D2
+ * CSIxA_LNR1 --> D0
+ * CSIxA_LNR2 --> C
+ * CSIxB_LNR0 --> D1
+ * CSIxB_LNR1 --> D3
+ */
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L0_CKSEL, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L1_CKSEL, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L2_CKMODE_EN, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L2_CKSEL, 1);
+
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L0_CKSEL, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L1_CKSEL, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L2_CKMODE_EN, 0);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_DPHY_L2_CKSEL, 1);
+
+ /* Byte clock invert */
+ mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
+ RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
+ RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
+ RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
+
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
+ RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
+ RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
+ RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
+
+ /* Start ANA EQ tuning */
+ if (port->is_cdphy) {
+ mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+ RG_CSI0A_L0_T0AB_EQ_IS, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+ RG_CSI0A_L0_T0AB_EQ_BW, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI0A_L1_T1AB_EQ_IS, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI0A_L1_T1AB_EQ_BW, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
+ RG_CSI0A_L2_T1BC_EQ_IS, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
+ RG_CSI0A_L2_T1BC_EQ_BW, 1);
+
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+ RG_CSI0A_L0_T0AB_EQ_IS, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+ RG_CSI0A_L0_T0AB_EQ_BW, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI0A_L1_T1AB_EQ_IS, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI0A_L1_T1AB_EQ_BW, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
+ RG_CSI0A_L2_T1BC_EQ_IS, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
+ RG_CSI0A_L2_T1BC_EQ_BW, 1);
+ } else {
+ mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L0_EQ_IS, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L0_EQ_BW, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L1_EQ_IS, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L1_EQ_BW, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI1A_L2_EQ_IS, 1);
+ mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI1A_L2_EQ_BW, 1);
+
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L0_EQ_IS, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L0_EQ_BW, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L1_EQ_IS, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
+ RG_CSI1A_L1_EQ_BW, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI1A_L2_EQ_IS, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
+ RG_CSI1A_L2_EQ_BW, 1);
+ }
+
+ /* End ANA EQ tuning */
+ mtk_phy_set_bits(base + MIPI_RX_ANA40_CSIxA, 0x90);
+
+ mtk_phy_update_field(base + MIPI_RX_ANA24_CSIxA,
+ RG_CSIxA_RESERVE, 0x40);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA24_CSIxA,
+ RG_CSIxA_RESERVE, 0x40);
+ mtk_phy_update_field(base + MIPI_RX_WRAPPER80_CSIxA,
+ CSR_CSI_RST_MODE, 0);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_WRAPPER80_CSIxA,
+ CSR_CSI_RST_MODE, 0);
+ /* ANA power on */
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_CORE_EN, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_CORE_EN, 1);
+ usleep_range(20, 40);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_LPF_EN, 1);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_LPF_EN, 1);
+
+ return 0;
+}
+
+static int mtk_mipi_phy_power_off(struct phy *phy)
+{
+ struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
+ void __iomem *base = port->base;
+
+ /* Disable MIPI BG. */
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_CORE_EN, 0);
+ mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_LPF_EN, 0);
+
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_CORE_EN, 0);
+ mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
+ RG_CSIxA_BG_LPF_EN, 0);
+
+ return 0;
+}
+
+static const struct phy_ops mtk_dphy_ops = {
+ .power_on = mtk_mipi_phy_power_on,
+ .power_off = mtk_mipi_phy_power_off,
+ .owner = THIS_MODULE,
+};
+
+static int mtk_mipi_dphy_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct phy_provider *phy_provider;
+ struct mtk_mipi_dphy_port *port;
+ struct phy *phy;
+
+ port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, port);
+
+ port->dev = dev;
+
+ port->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(port->base))
+ return PTR_ERR(port->base);
+
+ port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
+
+ phy = devm_phy_create(dev, NULL, &mtk_dphy_ops);
+ if (IS_ERR(phy)) {
+ dev_err(dev, "Failed to create PHY: %ld\n", PTR_ERR(phy));
+ return PTR_ERR(phy);
+ }
+
+ port->phy = phy;
+ phy_set_drvdata(phy, port);
+
+ phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+ if (IS_ERR(phy_provider)) {
+ dev_err(dev, "Failed to register PHY provider: %ld\n",
+ PTR_ERR(phy_provider));
+ return PTR_ERR(phy_provider);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id mtk_mipi_dphy_of_match[] = {
+ {.compatible = "mediatek,phy-mipi-csi-0-5"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, mtk_mipi_dphy_of_match);
+
+static struct platform_driver mipi_dphy_pdrv = {
+ .probe = mtk_mipi_dphy_probe,
+ .driver = {
+ .name = "mtk-mipi-csi-0-5",
+ .of_match_table = mtk_mipi_dphy_of_match,
+ },
+};
+
+module_platform_driver(mipi_dphy_pdrv);
+
+MODULE_DESCRIPTION("MTK mipi csi cdphy driver");
+MODULE_AUTHOR("Louis Kuo <louis.kuo@mediatek.com>");
+MODULE_LICENSE("GPL");
--
2.40.0Il 15/05/23 11:05, Julien Stephan ha scritto:
> From: Phi-bang Nguyen <pnguyen@baylibre.com>
>
> This is a new driver that supports the MIPI CSI CD-PHY version 0.5
>
> The number of PHYs depend on the soc.
>
> Signed-off-by: Louis Kuo <louis.kuo@mediatek.com>
> Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
> [Julien Stephan: use GENMASK]
> [Julien Stephan: refactor code]
> [Julien Stephan: update device tree support and probe function]
> Co-developed-by: Julien Stephan <jstephan@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
> MAINTAINERS | 1 +
> drivers/phy/mediatek/Kconfig | 8 +
> drivers/phy/mediatek/Makefile | 2 +
> .../mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h | 58 ++++
> drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c | 257 ++++++++++++++++++
> 5 files changed, 326 insertions(+)
> create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
> create mode 100644 drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 44f0ff11e984..fc2766cb50d3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13103,6 +13103,7 @@ M: Julien Stephan <jstephan@baylibre.com>
> M: Andy Hsieh <andy.hsieh@mediatek.com>
> S: Supported
> F: Documentation/devicetree/bindings/phy/mediatek,phy-mipi-csi-0-5.yaml
> +F: drivers/phy/mediatek/phy-mtk-mipi-csi-0-5*
>
> MEDIATEK MMC/SD/SDIO DRIVER
> M: Chaotian Jing <chaotian.jing@mediatek.com>
> diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig
> index 3125ecb5d119..452bc7ac5ce5 100644
> --- a/drivers/phy/mediatek/Kconfig
> +++ b/drivers/phy/mediatek/Kconfig
> @@ -74,3 +74,11 @@ config PHY_MTK_DP
> select GENERIC_PHY
> help
> Support DisplayPort PHY for MediaTek SoCs.
> +
> +config PHY_MTK_MIPI_CSI_0_5
> + tristate "MediaTek CSI CD-PHY v 0.5 Driver"
"MediaTek CSI CD-PHY v0.5 Driver"
> + depends on ARCH_MEDIATEK && OF
> + select GENERIC_PHY
> + help
> + Enable this to support the MIPI CSI CD-PHY receiver version 0.5.
> + The driver supports multiple CSI cdphy ports simultaneously.
> diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile
> index fb1f8edaffa7..8eb7b8747c67 100644
> --- a/drivers/phy/mediatek/Makefile
> +++ b/drivers/phy/mediatek/Makefile
> @@ -18,3 +18,5 @@ phy-mtk-mipi-dsi-drv-y := phy-mtk-mipi-dsi.o
> phy-mtk-mipi-dsi-drv-y += phy-mtk-mipi-dsi-mt8173.o
> phy-mtk-mipi-dsi-drv-y += phy-mtk-mipi-dsi-mt8183.o
> obj-$(CONFIG_PHY_MTK_MIPI_DSI) += phy-mtk-mipi-dsi-drv.o
> +
> +obj-$(CONFIG_PHY_MTK_MIPI_CSI_0_5) += phy-mtk-mipi-csi-0-5.o
> diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
> new file mode 100644
> index 000000000000..e9a7f1ab3e2f
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5-rx-reg.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
> +#define __PHY_MTK__MIPI_CSI__C_0_5_RX_REG_H__
What about....
__PHY_MTK_MIPI_CSI_V_0_5_RX_REG_H ?
> +
> +/*
> + * CSI1 and CSI2 are identical, and similar to CSI0. All CSIx macros are
> + * applicable to the three PHYs. Where differences exist, they are denoted by
> + * macro names using CSI0 and CSI1, the latter being applicable to CSI1 and
> + * CSI2 alike.
> + */
> +
> +#define MIPI_RX_ANA00_CSIxA 0x0000
I would rename all those from "CSIx" to "CSIX" (so, just toupper('x')).
> +#define RG_CSI0A_CPHY_EN BIT(0)
..snip..
> +
> +#endif
> diff --git a/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
> new file mode 100644
> index 000000000000..ae2d3dc9631d
> --- /dev/null
> +++ b/drivers/phy/mediatek/phy-mtk-mipi-csi-0-5.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "phy-mtk-io.h"
> +#include "phy-mtk-mipi-csi-0-5-rx-reg.h"
> +
> +#define CSIxB_OFFSET 0x1000
What if we grab two (or three?) iospaces from devicetree?
- base (global)
- csi_a
- csi_b
That would make it possible to maybe eventually extend this driver to more
versions (older or newer) of the CSI PHY IP without putting fixes offsets
inside of platform data structures and such.
> +
> +struct mtk_mipi_dphy;
> +
> +struct mtk_mipi_dphy_port {
> + struct device *dev;
> + void __iomem *base;
> + struct phy *phy;
> + bool is_cdphy;
> +};
> +
> +static int mtk_mipi_phy_power_on(struct phy *phy)
> +{
> + struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
> + void __iomem *base = port->base;
> +
> + /*
> + * Only DPHY is supported for now,
> + * so set analog phy mode to DPHY in CDPHY compatible PHYs
> + */
> + if (port->is_cdphy) {
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSI0A_CPHY_EN, 0);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSI0A_CPHY_EN, 0);
> + }
> +
> + /*
> + * Lane configuration:
> + *
> + * Only 4 data + 1 clock is supported for now with the following mapping:
> + *
> + * CSIxA_LNR0 --> D2
> + * CSIxA_LNR1 --> D0
> + * CSIxA_LNR2 --> C
> + * CSIxB_LNR0 --> D1
> + * CSIxB_LNR1 --> D3
> + */
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L0_CKSEL, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L1_CKSEL, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L2_CKMODE_EN, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L2_CKSEL, 1);
> +
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L0_CKMODE_EN, 0);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L0_CKSEL, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L1_CKMODE_EN, 0);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L1_CKSEL, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L2_CKMODE_EN, 0);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_DPHY_L2_CKSEL, 1);
> +
> + /* Byte clock invert */
> + mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
> + RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
> + RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANAA8_CSIxA,
> + RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
> +
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
> + RG_CSIxA_CDPHY_L0_T0_BYTECK_INVERT, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
> + RG_CSIxA_DPHY_L1_BYTECK_INVERT, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANAA8_CSIxA,
> + RG_CSIxA_CDPHY_L2_T1_BYTECK_INVERT, 1);
> +
> + /* Start ANA EQ tuning */
> + if (port->is_cdphy) {
statid void mtk_phy_csi_analog_eq_tune(struct mtk_mipi_dphy_port *port)
{
if (port->is_cdphy)
mtk_phy_csi_dphy_ana_eq_tune(...)
else
mtk_phy_csi_cphy_ana_eq_tune(...)
/* CPHY/DPHY common "end of tuning" sequence below */
mtk_phy_update_field( ... stuff ...);
}
...then all those calls will also fit in one line due to the reduced
indentation, improving readability and reducing line count.
> + mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> + RG_CSI0A_L0_T0AB_EQ_IS, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> + RG_CSI0A_L0_T0AB_EQ_BW, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI0A_L1_T1AB_EQ_IS, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI0A_L1_T1AB_EQ_BW, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
> + RG_CSI0A_L2_T1BC_EQ_IS, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA20_CSI0A,
> + RG_CSI0A_L2_T1BC_EQ_BW, 1);
> +
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> + RG_CSI0A_L0_T0AB_EQ_IS, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> + RG_CSI0A_L0_T0AB_EQ_BW, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI0A_L1_T1AB_EQ_IS, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI0A_L1_T1AB_EQ_BW, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
> + RG_CSI0A_L2_T1BC_EQ_IS, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA20_CSI0A,
> + RG_CSI0A_L2_T1BC_EQ_BW, 1);
> + } else {
> + mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L0_EQ_IS, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L0_EQ_BW, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L1_EQ_IS, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L1_EQ_BW, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI1A_L2_EQ_IS, 1);
> + mtk_phy_update_field(base + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI1A_L2_EQ_BW, 1);
> +
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L0_EQ_IS, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L0_EQ_BW, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L1_EQ_IS, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA18_CSIxA,
> + RG_CSI1A_L1_EQ_BW, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI1A_L2_EQ_IS, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA1C_CSIxA,
> + RG_CSI1A_L2_EQ_BW, 1);
> + }
> +
> + /* End ANA EQ tuning */
> + mtk_phy_set_bits(base + MIPI_RX_ANA40_CSIxA, 0x90);
> +
> + mtk_phy_update_field(base + MIPI_RX_ANA24_CSIxA,
> + RG_CSIxA_RESERVE, 0x40);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA24_CSIxA,
> + RG_CSIxA_RESERVE, 0x40);
> + mtk_phy_update_field(base + MIPI_RX_WRAPPER80_CSIxA,
> + CSR_CSI_RST_MODE, 0);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_WRAPPER80_CSIxA,
> + CSR_CSI_RST_MODE, 0);
> + /* ANA power on */
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_CORE_EN, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_CORE_EN, 1);
> + usleep_range(20, 40);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_LPF_EN, 1);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_LPF_EN, 1);
> +
> + return 0;
> +}
> +
> +static int mtk_mipi_phy_power_off(struct phy *phy)
> +{
> + struct mtk_mipi_dphy_port *port = phy_get_drvdata(phy);
> + void __iomem *base = port->base;
> +
> + /* Disable MIPI BG. */
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_CORE_EN, 0);
> + mtk_phy_update_field(base + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_LPF_EN, 0);
> +
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_CORE_EN, 0);
> + mtk_phy_update_field(base + CSIxB_OFFSET + MIPI_RX_ANA00_CSIxA,
> + RG_CSIxA_BG_LPF_EN, 0);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops mtk_dphy_ops = {
> + .power_on = mtk_mipi_phy_power_on,
> + .power_off = mtk_mipi_phy_power_off,
> + .owner = THIS_MODULE,
> +};
> +
> +static int mtk_mipi_dphy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct phy_provider *phy_provider;
> + struct mtk_mipi_dphy_port *port;
> + struct phy *phy;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, port);
> +
> + port->dev = dev;
> +
> + port->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(port->base))
> + return PTR_ERR(port->base);
> +
> + port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
This driver doesn't support C-PHY mode, so you either add support for that, or in
my opinion you should simply refuse to probe it, as it is *dysfunctional* for the
unsupported case (and might even introduce unstabilities).
/* At the moment, only D-PHY mode is supported */
if (!port->is_cdphy)
return -EINVAL;
Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy"
is fine.
> +
> + phy = devm_phy_create(dev, NULL, &mtk_dphy_ops);
> + if (IS_ERR(phy)) {
> + dev_err(dev, "Failed to create PHY: %ld\n", PTR_ERR(phy));
> + return PTR_ERR(phy);
> + }
> +
> + port->phy = phy;
> + phy_set_drvdata(phy, port);
> +
> + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> + if (IS_ERR(phy_provider)) {
> + dev_err(dev, "Failed to register PHY provider: %ld\n",
> + PTR_ERR(phy_provider));
> + return PTR_ERR(phy_provider);
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mtk_mipi_dphy_of_match[] = {
> + {.compatible = "mediatek,phy-mipi-csi-0-5"},
leave spaces.
{ .comp... " },
...and always end with
{ /* sentinel */ }
Also, please follow what the other PHY drivers do and use a SoC model,
example:
"mediatek,mt7777-csi-phy", or "mediatek,mt8888-csi-rx"
where the latter would make more sense imo.
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_mipi_dphy_of_match);
> +
> +static struct platform_driver mipi_dphy_pdrv = {
> + .probe = mtk_mipi_dphy_probe,
> + .driver = {
> + .name = "mtk-mipi-csi-0-5",
> + .of_match_table = mtk_mipi_dphy_of_match,
> + },
> +};
> +
remove extra blank line here.
> +module_platform_driver(mipi_dphy_pdrv);
> +
> +MODULE_DESCRIPTION("MTK mipi csi cdphy driver");
"MediaTek MIPI CSI CDPHY Driver"
> +MODULE_AUTHOR("Louis Kuo <louis.kuo@mediatek.com>");
> +MODULE_LICENSE("GPL");
Regards,
Angelo
On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote: > > +#define CSIxB_OFFSET 0x1000 > > What if we grab two (or three?) iospaces from devicetree? > > - base (global) > - csi_a > - csi_b > > That would make it possible to maybe eventually extend this driver to more > versions (older or newer) of the CSI PHY IP without putting fixes offsets > inside of platform data structures and such. > Hi Angelo, The register bank of the CSI port is divided into 2: * from base address to base + 0x1000 (port A) * from base + 0x1000 to base +0x2000 (port B) Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data + 1 clock) and use either port A or port B. For example mt8365 has CSI0 that can be used either in 4D1C mode or in 2 * 2D1C and CSI1 which can use only 4D1C mode 2D1C mode can not be tested and is not implemented in the driver so I guess adding csi_a and csi_b reg value may be confusing? What do you think? Regards, Julien
Il 15/05/23 16:07, Julien Stephan ha scritto:
> On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
>>> +#define CSIxB_OFFSET 0x1000
>>
>> What if we grab two (or three?) iospaces from devicetree?
>>
>> - base (global)
>> - csi_a
>> - csi_b
>>
>> That would make it possible to maybe eventually extend this driver to more
>> versions (older or newer) of the CSI PHY IP without putting fixes offsets
>> inside of platform data structures and such.
>>
> Hi Angelo,
> The register bank of the CSI port is divided into 2:
> * from base address to base + 0x1000 (port A)
> * from base + 0x1000 to base +0x2000 (port B)
> Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
> the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
> 1 clock) and use either port A or port B.
>
> For example mt8365 has CSI0 that can be used either in 4D1C mode or in
> 2 * 2D1C and CSI1 which can use only 4D1C mode
>
> 2D1C mode can not be tested and is not implemented in the driver so
> I guess adding csi_a and csi_b reg value may be confusing?
>
> What do you think?
Ok so we're talking about two data lanes per CSI port... it may still be
beneficial to split the two register regions as
reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
and that was a mistake, sorry!)
....but that would be actually good only if we are expecting to get a CSI
PHY in the future with four data lanes per port.
If you do *not* expect at all such a CSI PHY, or you do *not* expect such
a PHY to ever be compatible with this driver (read as: if you expect such
a PHY to be literally completely different from this one), then it would
not change much to have the registers split in two.
Another case in which it would make sense is if we were to get a PHY that
provides more than two CSI ports: in that case, we'd avoid platform data
machinery to check the number of actual ports in the IP, as we would be
just checking how many register regions we were given from the devicetree,
meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
ports.
Besides, another thing to think about is... yes you cannot test nor implement
2D1C mode in your submission, but this doesn't mean that others won't ever be
interested in this and that other people won't be actually implementing that;
Providing them with the right initial driver structure will surely make things
easier, encouraging other people from the community to spend their precious
time on the topic.
Regards,
Angelo
On Mon, May 15, 2023 at 04:32:42PM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/05/23 16:07, Julien Stephan ha scritto:
> > On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> > > > +#define CSIxB_OFFSET 0x1000
> > >
> > > What if we grab two (or three?) iospaces from devicetree?
> > >
> > > - base (global)
> > > - csi_a
> > > - csi_b
> > >
> > > That would make it possible to maybe eventually extend this driver to more
> > > versions (older or newer) of the CSI PHY IP without putting fixes offsets
> > > inside of platform data structures and such.
> > >
> > Hi Angelo,
> > The register bank of the CSI port is divided into 2:
> > * from base address to base + 0x1000 (port A)
> > * from base + 0x1000 to base +0x2000 (port B)
> > Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
> > the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
> > 1 clock) and use either port A or port B.
> >
> > For example mt8365 has CSI0 that can be used either in 4D1C mode or in
> > 2 * 2D1C and CSI1 which can use only 4D1C mode
> >
> > 2D1C mode can not be tested and is not implemented in the driver so
> > I guess adding csi_a and csi_b reg value may be confusing?
> >
> > What do you think?
>
> Ok so we're talking about two data lanes per CSI port... it may still be
> beneficial to split the two register regions as
>
> reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
> and that was a mistake, sorry!)
>
> ....but that would be actually good only if we are expecting to get a CSI
> PHY in the future with four data lanes per port.
>
> If you do *not* expect at all such a CSI PHY, or you do *not* expect such
> a PHY to ever be compatible with this driver (read as: if you expect such
> a PHY to be literally completely different from this one), then it would
> not change much to have the registers split in two.
>
> Another case in which it would make sense is if we were to get a PHY that
> provides more than two CSI ports: in that case, we'd avoid platform data
> machinery to check the number of actual ports in the IP, as we would be
> just checking how many register regions we were given from the devicetree,
> meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
> ports.
>
> Besides, another thing to think about is... yes you cannot test nor implement
> 2D1C mode in your submission, but this doesn't mean that others won't ever be
> interested in this and that other people won't be actually implementing that;
> Providing them with the right initial driver structure will surely make things
> easier, encouraging other people from the community to spend their precious
> time on the topic.
>
Hi Angelo,
Ok, I see your point, but for future potential upgrade to support A/B
ports I was thinking of something else: adding independent nodes for csixA
and csixB such as:
csi0_rx: phy@11c10000 {
reg = <0 0x11C10000 0 0x2000>;
mediatek,mode = <4D1c>;
...
};
csi0a_rx: phy@11c10000 {
reg = <0 0x11C10000 0 0x1000>;
mediatek,mode = <2D1c>;
...
};
csi0b_rx: phy@11c11000 {
reg = <0 0x11C11000 0 0x1000>;
mediatek,mode = <2D1c>;
...
};
giving the correct register range. One thing I did not mention is that if
csi0_rx is used csi0a_rx and csi0b_rx cannot be used (they share same
physical lanes as csio_rx), but csi0a_rx and csi0b_rx can be used simultaneously.
So platform device will enable only the node(s) it needs and enabling
csi0_rx and csioa/b_rx will fail because they share the same register
region and map will fail and it does not have any sense because you
either have a camera using the whole port or sub port but you cannot have
both plugged in. What do you think about it?
> > > > +#define CSIxB_OFFSET 0x1000
Maybe moving this declaration in phy-mtk-mipi-csi-0-5-rx-reg.h would be
better?
Regards,
Julien
> Regards,
> Angelo
>
Il 16/05/23 11:30, Julien Stephan ha scritto:
> On Mon, May 15, 2023 at 04:32:42PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 15/05/23 16:07, Julien Stephan ha scritto:
>>> On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
>>>>> +#define CSIxB_OFFSET 0x1000
>>>>
>>>> What if we grab two (or three?) iospaces from devicetree?
>>>>
>>>> - base (global)
>>>> - csi_a
>>>> - csi_b
>>>>
>>>> That would make it possible to maybe eventually extend this driver to more
>>>> versions (older or newer) of the CSI PHY IP without putting fixes offsets
>>>> inside of platform data structures and such.
>>>>
>>> Hi Angelo,
>>> The register bank of the CSI port is divided into 2:
>>> * from base address to base + 0x1000 (port A)
>>> * from base + 0x1000 to base +0x2000 (port B)
>>> Some CSI port can be configured in 4D1C mode (4 data + 1 clock) using
>>> the whole register bank from base to base + 0x2000 or in 2D1C mode (2 data +
>>> 1 clock) and use either port A or port B.
>>>
>>> For example mt8365 has CSI0 that can be used either in 4D1C mode or in
>>> 2 * 2D1C and CSI1 which can use only 4D1C mode
>>>
>>> 2D1C mode can not be tested and is not implemented in the driver so
>>> I guess adding csi_a and csi_b reg value may be confusing?
>>>
>>> What do you think?
>>
>> Ok so we're talking about two data lanes per CSI port... it may still be
>> beneficial to split the two register regions as
>>
>> reg-names = "csi-a", "csi-b"; (whoops, I actually used underscores before,
>> and that was a mistake, sorry!)
>>
>> ....but that would be actually good only if we are expecting to get a CSI
>> PHY in the future with four data lanes per port.
>>
>> If you do *not* expect at all such a CSI PHY, or you do *not* expect such
>> a PHY to ever be compatible with this driver (read as: if you expect such
>> a PHY to be literally completely different from this one), then it would
>> not change much to have the registers split in two.
>>
>> Another case in which it would make sense is if we were to get a PHY that
>> provides more than two CSI ports: in that case, we'd avoid platform data
>> machinery to check the number of actual ports in the IP, as we would be
>> just checking how many register regions we were given from the devicetree,
>> meaning that if we got "csi-a", "csi-b", "csi-c", "csi-d", we have four
>> ports.
>>
>> Besides, another thing to think about is... yes you cannot test nor implement
>> 2D1C mode in your submission, but this doesn't mean that others won't ever be
>> interested in this and that other people won't be actually implementing that;
>> Providing them with the right initial driver structure will surely make things
>> easier, encouraging other people from the community to spend their precious
>> time on the topic.
>>
> Hi Angelo,
> Ok, I see your point, but for future potential upgrade to support A/B
> ports I was thinking of something else: adding independent nodes for csixA
> and csixB such as:
>
> csi0_rx: phy@11c10000 {
> reg = <0 0x11C10000 0 0x2000>;
> mediatek,mode = <4D1c>;
> ...
> };
>
> csi0a_rx: phy@11c10000 {
> reg = <0 0x11C10000 0 0x1000>;
> mediatek,mode = <2D1c>;
> ...
> };
> csi0b_rx: phy@11c11000 {
> reg = <0 0x11C11000 0 0x1000>;
> mediatek,mode = <2D1c>;
> ...
> };
>
> giving the correct register range. One thing I did not mention is that if
> csi0_rx is used csi0a_rx and csi0b_rx cannot be used (they share same
> physical lanes as csio_rx), but csi0a_rx and csi0b_rx can be used simultaneously.
> So platform device will enable only the node(s) it needs and enabling
> csi0_rx and csioa/b_rx will fail because they share the same register
> region and map will fail and it does not have any sense because you
> either have a camera using the whole port or sub port but you cannot have
> both plugged in. What do you think about it?
>
Your description of the hardware makes me even more confident in pushing for
having one single node with multiple iospaces.
You could have a node such as:
csi0_rx: phy@11c10000 {
compatible = ....
reg = <0 0x11c10000 0 0x1000>, <0 0x11c20000 0 0x1000>;
reg-names = "csi-a", "csi-b";
/* 4 means 4D1C */
num-lanes = <4>;
or
/* 2 means 2D1C */
num-lanes = <2>;
};
You would then reference the csi0_rx node as:
/* PHY is configured as 4 lanes (4D1C) */
something = <&csi0_rx 0>;
or
/* First two lanes (CSI0 PORT-A) */
something = <&csi0_rx 0>;
/* Second two lanes (CSI0 PORT-B) */
something = <&csi0_rx 1>;
Preferrably, you should (or shall?) use a graph to describe such connections,
anyway.
This is because overriding the number of lanes on a per-board basis becomes
*otherwise* difficult, in the sense of human readability issues, other than
duplicated nodes being a real issue.
Regards,
Angelo
On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote:
> Il 15/05/23 11:05, Julien Stephan ha scritto:
..snip..
> > + port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy");
>
> This driver doesn't support C-PHY mode, so you either add support for that, or in
> my opinion you should simply refuse to probe it, as it is *dysfunctional* for the
> unsupported case (and might even introduce unstabilities).
>
> /* At the moment, only D-PHY mode is supported */
> if (!port->is_cdphy)
> return -EINVAL;
>
> Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy"
> is fine.
>
Hi Angelo,
You are right this driver does not support C-PHY mode, but some of the
PHYs themselves support BOTH C-PHY AND D-PHY. The idea of `is_cdphy` variable
is to know if the CSI port supports BOTH C-PHY AND D-PHY or only DPHY.
For example mt8365 has 2 PHYs: CSI0 and CSI1. CSI1 support only D-PHY,
while CSI0 can be configured in C-PHY or D-PHY. Registers for CD-PHY and
D-PHY are almost identical, except that CD-PHY compatible has some extra
bitfields to configure properly the mode and the lanes (because supporting
trios for CD-PHY).
If C-PHY support is eventually added into the driver, I think we will need
another variable such as `mode` to know the mode. I was also thinking
of adding a phy argument to determine if the mode is C-PHY or D-PHY.
So here, I don't want to stop the probe if `is_cdphy` variable is set to
true. Does it make sense ?
Regards
Julien
.. snip..
> > +
> > + phy = devm_phy_create(dev, NULL, &mtk_dphy_ops);
> > + if (IS_ERR(phy)) {
> > + dev_err(dev, "Failed to create PHY: %ld\n", PTR_ERR(phy));
> > + return PTR_ERR(phy);
> > + }
>
> Regards,
> Angelo
>
Il 15/05/23 15:36, Julien Stephan ha scritto: > On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote: >> Il 15/05/23 11:05, Julien Stephan ha scritto: > ..snip.. >>> + port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy"); >> >> This driver doesn't support C-PHY mode, so you either add support for that, or in >> my opinion you should simply refuse to probe it, as it is *dysfunctional* for the >> unsupported case (and might even introduce unstabilities). >> >> /* At the moment, only D-PHY mode is supported */ >> if (!port->is_cdphy) >> return -EINVAL; >> >> Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy" >> is fine. >> > Hi Angelo, > You are right this driver does not support C-PHY mode, but some of the > PHYs themselves support BOTH C-PHY AND D-PHY. The idea of `is_cdphy` variable > is to know if the CSI port supports BOTH C-PHY AND D-PHY or only DPHY. > For example mt8365 has 2 PHYs: CSI0 and CSI1. CSI1 support only D-PHY, > while CSI0 can be configured in C-PHY or D-PHY. Registers for CD-PHY and > D-PHY are almost identical, except that CD-PHY compatible has some extra > bitfields to configure properly the mode and the lanes (because supporting > trios for CD-PHY). > If C-PHY support is eventually added into the driver, I think we will need > another variable such as `mode` to know the mode. I was also thinking > of adding a phy argument to determine if the mode is C-PHY or D-PHY. > > So here, I don't want to stop the probe if `is_cdphy` variable is set to > true. Does it make sense ? > Comments in the code convinced me that the other PHYs providing only C or D PHY support weren't compatible at all with this driver. I got it now - but at this point can you please add a comment in the code actually clarifying that this driver supports both PHYs providing *only* D-PHY and ones providing selectable C-or-D PHY? That clarified, it would not make sense to stop probing if it's not a CDPHY because as you said there might be a D-only PHY that would be actually supported here. Regards, Angelo
On Mon, May 15, 2023 at 04:22:38PM +0200, AngeloGioacchino Del Regno wrote: > Il 15/05/23 15:36, Julien Stephan ha scritto: > > On Mon, May 15, 2023 at 02:22:52PM +0200, AngeloGioacchino Del Regno wrote: > > > Il 15/05/23 11:05, Julien Stephan ha scritto: > > ..snip.. > > > > + port->is_cdphy = of_property_read_bool(dev->of_node, "mediatek,is_cdphy"); > > > > > > This driver doesn't support C-PHY mode, so you either add support for that, or in > > > my opinion you should simply refuse to probe it, as it is *dysfunctional* for the > > > unsupported case (and might even introduce unstabilities). > > > > > > /* At the moment, only D-PHY mode is supported */ > > > if (!port->is_cdphy) > > > return -EINVAL; > > > > > > Also, please don't use underscores for devicetree properties: "mediatek,is-cdphy" > > > is fine. > > > > > Hi Angelo, > > You are right this driver does not support C-PHY mode, but some of the > > PHYs themselves support BOTH C-PHY AND D-PHY. The idea of `is_cdphy` variable > > is to know if the CSI port supports BOTH C-PHY AND D-PHY or only DPHY. > > For example mt8365 has 2 PHYs: CSI0 and CSI1. CSI1 support only D-PHY, > > while CSI0 can be configured in C-PHY or D-PHY. Registers for CD-PHY and > > D-PHY are almost identical, except that CD-PHY compatible has some extra > > bitfields to configure properly the mode and the lanes (because supporting > > trios for CD-PHY). > > If C-PHY support is eventually added into the driver, I think we will need > > another variable such as `mode` to know the mode. I was also thinking > > of adding a phy argument to determine if the mode is C-PHY or D-PHY. > > > > So here, I don't want to stop the probe if `is_cdphy` variable is set to > > true. Does it make sense ? > > > > Comments in the code convinced me that the other PHYs providing only C or D PHY > support weren't compatible at all with this driver. > > I got it now - but at this point can you please add a comment in the code actually > clarifying that this driver supports both PHYs providing *only* D-PHY and ones > providing selectable C-or-D PHY? > > That clarified, it would not make sense to stop probing if it's not a CDPHY because > as you said there might be a D-only PHY that would be actually supported here. > > Regards, > Angelo > > Ok, I will add a comment in the code to make it more clear. Regards Julien
© 2016 - 2024 Red Hat, Inc.