[2/3] drm/bridge: add lvds controller support for sam9x7
Commit Message
Add a new LVDS controller driver for sam9x7 which does the following:
- Prepares and enables the LVDS Peripheral clock
- Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
to the global bridge list.
- Identifies its output endpoint as panel and adds it to the encoder
display pipeline
- Enables the LVDS serializer
Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
---
drivers/gpu/drm/bridge/Kconfig | 7 +
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/microchip-lvds.c | 250 ++++++++++++++++++++++++
3 files changed, 258 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/microchip-lvds.c
Comments
On 22/01/2024 09:29, Dharma Balasubiramani wrote:
> Add a new LVDS controller driver for sam9x7 which does the following:
> - Prepares and enables the LVDS Peripheral clock
> - Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
> to the global bridge list.
> - Identifies its output endpoint as panel and adds it to the encoder
> display pipeline
> - Enables the LVDS serializer
>
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---
..
> +
> +static int mchp_lvds_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mchp_lvds *lvds;
> + struct resource *res;
> + struct device_node *port;
> + int ret;
> +
> + if (!dev->of_node)
> + return -ENODEV;
> +
> + lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> + if (!lvds)
> + return -ENOMEM;
> +
> + lvds->dev = dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + lvds->regs = devm_ioremap_resource(lvds->dev, res);
Why not combining these two?
> + if (IS_ERR(lvds->regs))
> + return PTR_ERR(lvds->regs);
> +
> + lvds->pclk = devm_clk_get(lvds->dev, "pclk");
> + if (IS_ERR(lvds->pclk)) {
> + DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
Handle properly deferred probe. What's DRM wrapper over dev_err_probe()?
> + return PTR_ERR(lvds->pclk);
> + }
> +
> + ret = clk_prepare(lvds->pclk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
> + return ret;
> + }
> +
> + port = of_graph_get_remote_node(dev->of_node, 1, 0);
> + if (!port) {
> + DRM_DEV_ERROR(dev,
> + "can't find port point, please init lvds panel port!\n");
> + return -EINVAL;
> + }
> +
> + lvds->panel = of_drm_find_panel(port);
> + of_node_put(port);
> +
> + if (IS_ERR(lvds->panel)) {
> + DRM_DEV_ERROR(dev, "failed to find panel node\n");
> + return -EPROBE_DEFER;
OK, that's for sure wrong. Don't print anything on deferred probe.
> + }
> +
> + lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
> +
> + if (IS_ERR(lvds->panel_bridge))
> + return PTR_ERR(lvds->panel_bridge);
> +
> + lvds->bridge.of_node = dev->of_node;
> + lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
> + lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
> +
> + dev_set_drvdata(dev, lvds);
> + pm_runtime_enable(dev);
> +
> + drm_bridge_add(&lvds->bridge);
> +
> + return 0;
> +}
> +
> +static int mchp_lvds_remove(struct platform_device *pdev)
> +{
> + struct mchp_lvds *lvds = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(&pdev->dev);
> + clk_unprepare(lvds->pclk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mchp_lvds_dt_ids[] = {
> + {
> + .compatible = "microchip,sam9x7-lvds",
> + },
> + {},
> +};
> +
> +struct platform_driver mchp_lvds_driver = {
> + .probe = mchp_lvds_probe,
> + .remove = mchp_lvds_remove,
> + .driver = {
> + .name = "microchip-lvds",
> + .of_match_table = mchp_lvds_dt_ids,
> + },
> +};
> +module_platform_driver(mchp_lvds_driver);
> +
> +MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:microchip-lvds");
You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.
Best regards,
Krzysztof
Hi Krzysztof,
On 22/01/24 9:19 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 22/01/2024 09:29, Dharma Balasubiramani wrote:
>> Add a new LVDS controller driver for sam9x7 which does the following:
>> - Prepares and enables the LVDS Peripheral clock
>> - Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
>> to the global bridge list.
>> - Identifies its output endpoint as panel and adds it to the encoder
>> display pipeline
>> - Enables the LVDS serializer
>>
>> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
>> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
>> ---
>
> ...
>
>> +
>> +static int mchp_lvds_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct mchp_lvds *lvds;
>> + struct resource *res;
>> + struct device_node *port;
>> + int ret;
>> +
>> + if (!dev->of_node)
>> + return -ENODEV;
>> +
>> + lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
>> + if (!lvds)
>> + return -ENOMEM;
>> +
>> + lvds->dev = dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + lvds->regs = devm_ioremap_resource(lvds->dev, res);
>
> Why not combining these two?
It seems reasonable to combine these two lines since the resource
variable (res) is only used at this point. I'll proceed with
consolidating these lines for simplicity.
>
>> + if (IS_ERR(lvds->regs))
>> + return PTR_ERR(lvds->regs);
>> +
>> + lvds->pclk = devm_clk_get(lvds->dev, "pclk");
>> + if (IS_ERR(lvds->pclk)) {
>> + DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
>
> Handle properly deferred probe. What's DRM wrapper over dev_err_probe()?
Sure, I will use dev_err_probe()
return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk), "could not get
pclk_lvds\n");
>
>> + return PTR_ERR(lvds->pclk);
>> + }
>> +
>> + ret = clk_prepare(lvds->pclk);
>> + if (ret < 0) {
>> + DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
>> + return ret;
>> + }
>> +
>> + port = of_graph_get_remote_node(dev->of_node, 1, 0);
>> + if (!port) {
>> + DRM_DEV_ERROR(dev,
>> + "can't find port point, please init lvds panel port!\n");
>> + return -EINVAL;
>> + }
>> +
>> + lvds->panel = of_drm_find_panel(port);
>> + of_node_put(port);
>> +
>> + if (IS_ERR(lvds->panel)) {
>> + DRM_DEV_ERROR(dev, "failed to find panel node\n");
>> + return -EPROBE_DEFER;
>
> OK, that's for sure wrong. Don't print anything on deferred probe.
Sure, I will drop the print here.
>
>> + }
>> +
>> + lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
>> +
>> + if (IS_ERR(lvds->panel_bridge))
>> + return PTR_ERR(lvds->panel_bridge);
>> +
>> + lvds->bridge.of_node = dev->of_node;
>> + lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
>> + lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
>> +
>> + dev_set_drvdata(dev, lvds);
>> + pm_runtime_enable(dev);
>> +
>> + drm_bridge_add(&lvds->bridge);
>> +
>> + return 0;
>> +}
>> +
>> +static int mchp_lvds_remove(struct platform_device *pdev)
>> +{
>> + struct mchp_lvds *lvds = platform_get_drvdata(pdev);
>> +
>> + pm_runtime_disable(&pdev->dev);
>> + clk_unprepare(lvds->pclk);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id mchp_lvds_dt_ids[] = {
>> + {
>> + .compatible = "microchip,sam9x7-lvds",
>> + },
>> + {},
>> +};
>> +
>> +struct platform_driver mchp_lvds_driver = {
>> + .probe = mchp_lvds_probe,
>> + .remove = mchp_lvds_remove,
>> + .driver = {
>> + .name = "microchip-lvds",
>> + .of_match_table = mchp_lvds_dt_ids,
>> + },
>> +};
>> +module_platform_driver(mchp_lvds_driver);
>> +
>> +MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
>> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
>> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:microchip-lvds");
>
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
Okay, I will remove the MODULE_ALIAS and update the mchp_lvds_dt_ids[]
as below along with MODULE_DEVICE_TABLE()
static const struct of_device_id mchp_lvds_dt_ids[] = {
{
.compatible = "microchip,sam9x72-lvds",
},
{
.compatible = "microchip,sam9x75-lvds",
},
{},
};
MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);
--
Thanks,
Dharma B.
>
>
> Best regards,
> Krzysztof
>
Hi Dharma,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.8-rc2 next-20240202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dharma-Balasubiramani/dt-bindings-display-bridge-add-sam9x7-lvds-compatible/20240122-163209
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20240122082947.21645-3-dharma.b%40microchip.com
patch subject: [PATCH 2/3] drm/bridge: add lvds controller support for sam9x7
config: arm-randconfig-r112-20240203 (https://download.01.org/0day-ci/archive/20240203/202402032248.6puqAuzM-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240203/202402032248.6puqAuzM-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402032248.6puqAuzM-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/bridge/microchip-lvds.c:236:24: sparse: sparse: symbol 'mchp_lvds_driver' was not declared. Should it be static?
vim +/mchp_lvds_driver +236 drivers/gpu/drm/bridge/microchip-lvds.c
235
> 236 struct platform_driver mchp_lvds_driver = {
237 .probe = mchp_lvds_probe,
238 .remove = mchp_lvds_remove,
239 .driver = {
240 .name = "microchip-lvds",
241 .of_match_table = mchp_lvds_dt_ids,
242 },
243 };
244 module_platform_driver(mchp_lvds_driver);
245
@@ -173,6 +173,13 @@ config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
to DP++. This is used with the i.MX6 imx-ldb
driver. You are likely to say N here.
+config DRM_MICROCHIP_LVDS_SERIALIZER
+ tristate "Microchip LVDS serailzer support"
+ depends on OF
+ depends on DRM_ATMEL_HLCDC
+ help
+ Support for Microchip's LVDS serializer.
+
config DRM_NWL_MIPI_DSI
tristate "Northwest Logic MIPI DSI Host controller"
depends on DRM
@@ -11,6 +11,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
+obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o
obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o
new file mode 100644
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Manikandan Muralidharan <manikandan.m@microchip.com>
+ * Author: Dharma Balasubiramani <dharma.b@microchip.com>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/component.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_graph.h>
+#include <linux/pinctrl/devinfo.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+
+#define LVDS_POLL_TIMEOUT_MS 1000
+
+/* LVDSC register offsets */
+#define LVDSC_CR 0x00
+#define LVDSC_CFGR 0x04
+#define LVDSC_SR 0x0C
+#define LVDSC_WPMR 0xE4
+
+/* Bitfields in LVDSC_CR (Control Register) */
+#define LVDSC_CR_SER_EN BIT(0)
+
+/* Bitfields in LVDSC_CFGR (Configuration Register) */
+#define LVDSC_CFGR_PIXSIZE_24BITS 0
+#define LVDSC_CFGR_DEN_POL_HIGH 0
+#define LVDSC_CFGR_DC_UNBALANCED 0
+#define LVDSC_CFGR_MAPPING_JEIDA BIT(6)
+
+/*Bitfields in LVDSC_SR */
+#define LVDSC_SR_CS BIT(0)
+
+/* Bitfields in LVDSC_WPMR (Write Protection Mode Register) */
+#define LVDSC_WPMR_WPKEY_MASK GENMASK(31, 8)
+#define LVDSC_WPMR_WPKEY_PSSWD 0x4C5644
+
+struct mchp_lvds {
+ struct device *dev;
+ void __iomem *regs;
+ struct clk *pclk;
+ int format; /* vesa or jeida format */
+ struct drm_panel *panel;
+ struct drm_bridge bridge;
+ struct drm_bridge *panel_bridge;
+};
+
+static inline struct mchp_lvds *bridge_to_lvds(struct drm_bridge *bridge)
+{
+ return container_of(bridge, struct mchp_lvds, bridge);
+}
+
+static inline u32 lvds_readl(struct mchp_lvds *lvds, u32 offset)
+{
+ return readl_relaxed(lvds->regs + offset);
+}
+
+static inline void lvds_writel(struct mchp_lvds *lvds, u32 offset, u32 val)
+{
+ writel_relaxed(val, lvds->regs + offset);
+}
+
+static void lvds_serialiser_on(struct mchp_lvds *lvds)
+{
+ unsigned long timeout = jiffies + msecs_to_jiffies(LVDS_POLL_TIMEOUT_MS);
+
+ /* The LVDSC registers can only be written if WPEN is cleared */
+ lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD &
+ LVDSC_WPMR_WPKEY_MASK));
+
+ /* Wait for the status of configuration registers to be changed */
+ while (lvds_readl(lvds, LVDSC_SR) & LVDSC_SR_CS) {
+ if (time_after(jiffies, timeout)) {
+ dev_err(lvds->dev, "%s: timeout error\n", __func__);
+ return;
+ }
+ usleep_range(1000, 2000);
+ }
+
+ /* Configure the LVDSC */
+ lvds_writel(lvds, LVDSC_CFGR, (LVDSC_CFGR_MAPPING_JEIDA |
+ LVDSC_CFGR_DC_UNBALANCED |
+ LVDSC_CFGR_DEN_POL_HIGH |
+ LVDSC_CFGR_PIXSIZE_24BITS));
+
+ /* Enable the LVDS serializer */
+ lvds_writel(lvds, LVDSC_CR, LVDSC_CR_SER_EN);
+}
+
+static int mchp_lvds_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct mchp_lvds *lvds = bridge_to_lvds(bridge);
+
+ bridge->encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
+
+ return drm_bridge_attach(bridge->encoder, lvds->panel_bridge,
+ bridge, flags);
+}
+
+static void mchp_lvds_enable(struct drm_bridge *bridge)
+{
+ struct mchp_lvds *lvds = bridge_to_lvds(bridge);
+ int ret;
+
+ ret = clk_enable(lvds->pclk);
+ if (ret < 0) {
+ DRM_DEV_ERROR(lvds->dev, "failed to enable lvds pclk %d\n", ret);
+ return;
+ }
+
+ ret = pm_runtime_get_sync(lvds->dev);
+ if (ret < 0) {
+ DRM_DEV_ERROR(lvds->dev, "failed to get pm runtime: %d\n", ret);
+ clk_disable(lvds->pclk);
+ return;
+ }
+
+ lvds_serialiser_on(lvds);
+}
+
+static void mchp_lvds_disable(struct drm_bridge *bridge)
+{
+ struct mchp_lvds *lvds = bridge_to_lvds(bridge);
+
+ pm_runtime_put(lvds->dev);
+ clk_disable(lvds->pclk);
+}
+
+static const struct drm_bridge_funcs mchp_lvds_bridge_funcs = {
+ .attach = mchp_lvds_attach,
+ .enable = mchp_lvds_enable,
+ .disable = mchp_lvds_disable,
+};
+
+static int mchp_lvds_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mchp_lvds *lvds;
+ struct resource *res;
+ struct device_node *port;
+ int ret;
+
+ if (!dev->of_node)
+ return -ENODEV;
+
+ lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
+ if (!lvds)
+ return -ENOMEM;
+
+ lvds->dev = dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ lvds->regs = devm_ioremap_resource(lvds->dev, res);
+ if (IS_ERR(lvds->regs))
+ return PTR_ERR(lvds->regs);
+
+ lvds->pclk = devm_clk_get(lvds->dev, "pclk");
+ if (IS_ERR(lvds->pclk)) {
+ DRM_DEV_ERROR(lvds->dev, "could not get pclk_lvds\n");
+ return PTR_ERR(lvds->pclk);
+ }
+
+ ret = clk_prepare(lvds->pclk);
+ if (ret < 0) {
+ DRM_DEV_ERROR(lvds->dev, "failed to prepare pclk_lvds\n");
+ return ret;
+ }
+
+ port = of_graph_get_remote_node(dev->of_node, 1, 0);
+ if (!port) {
+ DRM_DEV_ERROR(dev,
+ "can't find port point, please init lvds panel port!\n");
+ return -EINVAL;
+ }
+
+ lvds->panel = of_drm_find_panel(port);
+ of_node_put(port);
+
+ if (IS_ERR(lvds->panel)) {
+ DRM_DEV_ERROR(dev, "failed to find panel node\n");
+ return -EPROBE_DEFER;
+ }
+
+ lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
+
+ if (IS_ERR(lvds->panel_bridge))
+ return PTR_ERR(lvds->panel_bridge);
+
+ lvds->bridge.of_node = dev->of_node;
+ lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
+ lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
+
+ dev_set_drvdata(dev, lvds);
+ pm_runtime_enable(dev);
+
+ drm_bridge_add(&lvds->bridge);
+
+ return 0;
+}
+
+static int mchp_lvds_remove(struct platform_device *pdev)
+{
+ struct mchp_lvds *lvds = platform_get_drvdata(pdev);
+
+ pm_runtime_disable(&pdev->dev);
+ clk_unprepare(lvds->pclk);
+
+ return 0;
+}
+
+static const struct of_device_id mchp_lvds_dt_ids[] = {
+ {
+ .compatible = "microchip,sam9x7-lvds",
+ },
+ {},
+};
+
+struct platform_driver mchp_lvds_driver = {
+ .probe = mchp_lvds_probe,
+ .remove = mchp_lvds_remove,
+ .driver = {
+ .name = "microchip-lvds",
+ .of_match_table = mchp_lvds_dt_ids,
+ },
+};
+module_platform_driver(mchp_lvds_driver);
+
+MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
+MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
+MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:microchip-lvds");