| Message ID | 20221216210742.3233382-1-l.stach@pengutronix.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/4] dt-bindings: display: imx: add binding for i.MX8MP HDMI TX | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hi Lucas, On Fri, 2022-12-16 at 22:07 +0100, Lucas Stach wrote: > The HDMI TX controller on the i.MX8MP SoC is a Synopsys designware IP > core with a little bit of SoC integration around it. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > .../bindings/display/imx/fsl,imx8mp-hdmi.yaml | 69 > +++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > > diff --git > a/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > new file mode 100644 > index 000000000000..75ebeaa8c9d5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp- > hdmi.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8mp-hdmi.yaml# Better to put the binding documentation under the display/bridge umbrella as the corresponding linux driver is a DRM bridge driver, not a DRM encoder driver. Regarding the file name, I would use 'fsl,imx8mp-hdmi-tx.yaml' to explicitly tell it's a TX controller(not a RX controller), which matches the chapter name 'HDMI TX controller' in i.MX8mp RM. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Freescale i.MX8MP DWC HDMI TX Encoder > + > +maintainers: > + - Lucas Stach <l.stach@pengutronix.de> > + > +description: | > + The i.MX8MP HDMI transmitter is a Synopsys DesignWare > + HDMI 2.0 TX controller IP. i.MX8mp RM says it is compatible with the HDMI v2.0a spec, so better to mention 2.0a instead of 2.0. > + > +allOf: > + - $ref: ../bridge/synopsys,dw-hdmi.yaml# > + > +properties: > + compatible: > + enum: > + - fsl,imx8mp-hdmi Like the file name, I would use 'fsl,imx8mp-hdmi-tx'. It seems that the i.MX6q DW HDMI TX controller will not easily use this binding since it's corresponding driver is a DRM encoder driver, and no other i.MX SoCs embed the controller, so use const instead of enum(It can be changed to enum when necessary later.)? > + > + reg-io-width: > + const: 1 > + > + clocks: > + maxItems: 5 > + > + clock-names: > + items: > + - const: iahb > + - const: isfr > + - const: fdcc > + - const: cec > + - const: pix > + > + power-domains: > + maxItems: 1 Missing 'ports' property? > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - power-domains > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> Sort the header files alphabetically. I'm cc'ing Sandor to be aware of the patch set. Regards, Liu Ying > + #include <dt-bindings/clock/imx8mp-clock.h> > + #include <dt-bindings/power/imx8mp-power.h> > + > + hdmi@32fd8000 { > + compatible = "fsl,imx8mp-hdmi"; > + reg = <0x32fd8000 0x7eff>; > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MP_CLK_HDMI_APB>, > + <&clk IMX8MP_CLK_HDMI_REF_266M>, > + <&clk IMX8MP_CLK_HDMI_FDCC_TST>, > + <&clk IMX8MP_CLK_32K>, > + <&hdmi_tx_phy>; > + clock-names = "iahb", "isfr", "fdcc", "cec", "pix"; > + power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_HDMI_TX>; > + reg-io-width = <1>; > + };
On Fri, 2022-12-16 at 22:07 +0100, Lucas Stach wrote: > Add a simple wrapper driver for the DWC HDMI bridge driver that > implements the few bits that are necessary to abstract the i.MX8MP > SoC integration. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Marek Vasut <marex@denx.de> > --- > drivers/gpu/drm/bridge/imx/Kconfig | 9 ++ > drivers/gpu/drm/bridge/imx/Makefile | 2 + > drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c | 140 > +++++++++++++++++++++++ > 3 files changed, 151 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c > Can you please provide a changelog since this is v2? > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig > b/drivers/gpu/drm/bridge/imx/Kconfig > index 608f47f41bcd..d828d8bfd893 100644 > --- a/drivers/gpu/drm/bridge/imx/Kconfig > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > @@ -44,4 +44,13 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI > Choose this to enable pixel link to display pixel > interface(PXL2DPI) > found in Freescale i.MX8qxp processor. > > +config DRM_IMX8MP_DW_HDMI_BRIDGE Sort the config names alphabetically please. > + tristate "i.MX8MP HDMI bridge support" To show the prompts in this Kconfig file in a consistent fashion, please add 'Freescale' before 'i.MX8MP'. > + depends on OF > + depends on COMMON_CLK > + select DRM_DW_HDMI > + help > + Choose this to enable support for the internal HDMI encoder > found > + on the i.MX8MP SoC. > + > endif # ARCH_MXC || COMPILE_TEST > diff --git a/drivers/gpu/drm/bridge/imx/Makefile > b/drivers/gpu/drm/bridge/imx/Makefile > index aa90ec8d5433..03b0074ae538 100644 > --- a/drivers/gpu/drm/bridge/imx/Makefile > +++ b/drivers/gpu/drm/bridge/imx/Makefile > @@ -7,3 +7,5 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o > + > +obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi.o Sort the config names alphabetically. > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c > b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c > new file mode 100644 > index 000000000000..06849b817aed > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c > @@ -0,0 +1,140 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * Copyright (C) 2022 Pengutronix, Lucas Stach < > kernel@pengutronix.de> > + */ > + > +#include <drm/bridge/dw_hdmi.h> > +#include <drm/drm_modes.h> > +#include <linux/clk.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> Header files in linux/ come before those in drm/. > + > +struct imx8mp_hdmi { > + struct dw_hdmi_plat_data plat_data; > + struct dw_hdmi *dw_hdmi; > + struct clk *pixclk; > + struct clk *fdcc; > +}; > + > +static enum drm_mode_status > +imx8mp_hdmi_mode_valid(struct dw_hdmi *dw_hdmi, void *data, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + struct imx8mp_hdmi *hdmi = (struct imx8mp_hdmi *)data; > + > + if (mode->clock < 13500) > + return MODE_CLOCK_LOW; > + > + if (mode->clock > 297000) > + return MODE_CLOCK_HIGH; > + > + if (clk_round_rate(hdmi->pixclk, mode->clock * 1000) != > + mode->clock * 1000) > + return MODE_CLOCK_RANGE; > + > + /* We don't support double-clocked and Interlaced modes */ > + if ((mode->flags & DRM_MODE_FLAG_DBLCLK) || > + (mode->flags & DRM_MODE_FLAG_INTERLACE)) > + return MODE_BAD; > + > + return MODE_OK; > +} > + > +static int imx8mp_hdmi_phy_init(struct dw_hdmi *dw_hdmi, void *data, > + const struct drm_display_info *display, > + const struct drm_display_mode *mode) > +{ > + return 0; > +} > + > +static void imx8mp_hdmi_phy_disable(struct dw_hdmi *dw_hdmi, void > *data) > +{ > +} > + > +static void im8mp_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void > *data) > +{ > + /* > + * Just release PHY core from reset, all other power management > is done > + * by the PHY driver. > + */ > + dw_hdmi_phy_gen1_reset(hdmi); > + > + dw_hdmi_phy_setup_hpd(hdmi, data); > +} > + > +static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = { > + .init = imx8mp_hdmi_phy_init, > + .disable = imx8mp_hdmi_phy_disable, > + .setup_hpd = im8mp_hdmi_phy_setup_hpd, > + .read_hpd = dw_hdmi_phy_read_hpd, > + .update_hpd = dw_hdmi_phy_update_hpd, > +}; > + > +static int imx8mp_dw_hdmi_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct dw_hdmi_plat_data *plat_data; > + struct imx8mp_hdmi *hdmi; > + int ret; Please fix this build warning: drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c: In function ‘imx8mp_dw_hdmi_probe’: drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c:80:13: warning: unused variable ‘ret’ [-Wunused-variable] 80 | int ret; | ^~~ > + > + hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL); > + if (!hdmi) > + return -ENOMEM; > + > + plat_data = &hdmi->plat_data; > + > + hdmi->pixclk = devm_clk_get(dev, "pix"); > + if (IS_ERR(hdmi->pixclk)) > + return dev_err_probe(dev, PTR_ERR(hdmi->pixclk), > + "Unable to get pixel clock\n"); > + > + hdmi->fdcc = devm_clk_get_enabled(dev, "fdcc"); > + if (IS_ERR(hdmi->fdcc)) > + return dev_err_probe(dev, PTR_ERR(hdmi->fdcc), > + "Unable to get FDCC clock\n"); Similar to Laurent's comment on v1 here, why does fdcc clock need to be always enabled? > + > + plat_data->mode_valid = imx8mp_hdmi_mode_valid; > + plat_data->phy_ops = &imx8mp_hdmi_phy_ops; > + plat_data->phy_name = "SAMSUNG HDMI TX PHY"; > + plat_data->priv_data = hdmi; Need to set plat_data->phy_force_vendor to be true? Or, you rely on reading the HDMI_CONFIG2_ID register to determine the phy type? > + > + hdmi->dw_hdmi = dw_hdmi_probe(pdev, plat_data); > + if (IS_ERR(hdmi->dw_hdmi)) > + return PTR_ERR(hdmi->dw_hdmi); > + > + platform_set_drvdata(pdev, hdmi); > + > + return 0; > +} > + > +static int imx8mp_dw_hdmi_remove(struct platform_device *pdev) > +{ > + struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev); > + > + dw_hdmi_remove(hdmi->dw_hdmi); > + > + return 0; > +} > + > +static const struct of_device_id imx8mp_dw_hdmi_of_table[] = { > + { .compatible = "fsl,imx8mp-hdmi" }, > + { /* Sentinel */ }, Nitpick: ',' after the sentinel is not needed since it's the last one. Regards, Liu Ying > +}; > +MODULE_DEVICE_TABLE(of, imx8mp_dw_hdmi_of_table); > + > +static struct platform_driver imx8mp_dw_hdmi_platform_driver = { > + .probe = imx8mp_dw_hdmi_probe, > + .remove = imx8mp_dw_hdmi_remove, > + .driver = { > + .name = "imx8mp-dw-hdmi", > + .of_match_table = imx8mp_dw_hdmi_of_table, > + }, > +}; > + > +module_platform_driver(imx8mp_dw_hdmi_platform_driver); > + > +MODULE_DESCRIPTION("i.MX8MP HDMI encoder driver"); > +MODULE_LICENSE("GPL");
On Fri, 2022-12-16 at 22:07 +0100, Lucas Stach wrote: > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It > has a > full timing generator and can switch between different video sources. > On > the i.MX8MP however the only supported source is the LCDIF. The block > just needs to be powered up and told about the polarity of the video > sync signals to act in bypass mode. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Tested-by: Marek Vasut <marex@denx.de> > --- > drivers/gpu/drm/bridge/imx/Kconfig | 7 + > drivers/gpu/drm/bridge/imx/Makefile | 1 + > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 202 > +++++++++++++++++++ > 3 files changed, 210 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig > b/drivers/gpu/drm/bridge/imx/Kconfig > index d828d8bfd893..e6cc4000bccd 100644 > --- a/drivers/gpu/drm/bridge/imx/Kconfig > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > @@ -53,4 +53,11 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE > Choose this to enable support for the internal HDMI encoder > found > on the i.MX8MP SoC. > > +config DRM_IMX8MP_HDMI_PVI Sort the config names alphabetically. > + tristate "i.MX8MP HDMI PVI bridge support" Add 'Freescale' before 'i.MX8MP' to show prompts in a consistent fashion. > + depends on OF select DRM_KMS_HELPER > + help > + Choose this to enable support for the internal HDMI TX > Parallel > + Video Interface found on the i.MX8MP SoC. > + > endif # ARCH_MXC || COMPILE_TEST > diff --git a/drivers/gpu/drm/bridge/imx/Makefile > b/drivers/gpu/drm/bridge/imx/Makefile > index 03b0074ae538..b0fd56550dad 100644 > --- a/drivers/gpu/drm/bridge/imx/Makefile > +++ b/drivers/gpu/drm/bridge/imx/Makefile > @@ -9,3 +9,4 @@ obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp- > pixel-link.o > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o > > obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi.o > +obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o Sort the config names alphabetically. > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > new file mode 100644 > index 000000000000..30d40c21dabb > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > @@ -0,0 +1,202 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * Copyright (C) 2022 Pengutronix, Lucas Stach < > kernel@pengutronix.de> > + */ > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_crtc.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_graph.h> > +#include <linux/pm_runtime.h> Header files in linux/ come before those in drm/. > + > +#define HTX_PVI_CTL 0x0 Nitpick: One more tab between the macro and '0x0'. > +#define PVI_CTL_OP_VSYNC_POL BIT(18) > +#define PVI_CTL_OP_HSYNC_POL BIT(17) > +#define PVI_CTL_OP_DE_POL BIT(16) > +#define PVI_CTL_INP_VSYNC_POL BIT(14) > +#define PVI_CTL_INP_HSYNC_POL BIT(13) > +#define PVI_CTL_INP_DE_POL BIT(12) > +#define PVI_CTL_INPUT_LCDIF BIT(2) > +#define PVI_CTL_EN BIT(0) > + > +struct imx8mp_hdmi_pvi { > + struct drm_bridge bridge; > + struct device *dev; > + struct drm_bridge *next_bridge; > + void __iomem *regs; > +}; > + > +static inline struct imx8mp_hdmi_pvi * > +to_imx8mp_hdmi_pvi(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct imx8mp_hdmi_pvi, bridge); > +} > + > +static int imx8mp_hdmi_pvi_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags > flags) > +{ > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > + > + return drm_bridge_attach(bridge->encoder, pvi->next_bridge, > + bridge, flags); > +} > + > +static void imx8mp_hdmi_pvi_bridge_enable(struct drm_bridge *bridge, > + struct drm_bridge_state > *bridge_state) > +{ > + struct drm_atomic_state *state = bridge_state->base.state; > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > + struct drm_connector_state *conn_state; > + const struct drm_display_mode *mode; > + struct drm_crtc_state *crtc_state; > + struct drm_connector *connector; > + u32 bus_flags, val; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, > bridge->encoder); > + conn_state = drm_atomic_get_new_connector_state(state, > connector); > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state- > >crtc); > + > + if (WARN_ON(pm_runtime_resume_and_get(pvi->dev))) > + return; > + > + mode = &crtc_state->adjusted_mode; > + > + val = PVI_CTL_INPUT_LCDIF; > + > + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > + val |= PVI_CTL_OP_VSYNC_POL | PVI_CTL_INP_VSYNC_POL; > + > + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > + val |= PVI_CTL_OP_HSYNC_POL | PVI_CTL_INP_HSYNC_POL; > + > + if (pvi->next_bridge->timings) > + bus_flags = pvi->next_bridge->timings->input_bus_flags; > + else if (bridge_state) > + bus_flags = bridge_state->input_bus_cfg.flags; > + > + if (bus_flags & DRM_BUS_FLAG_DE_HIGH) > + val |= PVI_CTL_OP_DE_POL | PVI_CTL_INP_DE_POL; Downstream kernel driver sets bit20 & bit21 to insert background color when something wrong happens(maybe, it may mitigate the issue), not sure if this driver should do the same or not. > + > + writel(val, pvi->regs + HTX_PVI_CTL); > + val |= PVI_CTL_EN; > + writel(val, pvi->regs + HTX_PVI_CTL); > +} > + > +static void imx8mp_hdmi_pvi_bridge_disable(struct drm_bridge > *bridge, > + struct drm_bridge_state > *bridge_state) > +{ > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > + > + writel(0x0, pvi->regs + HTX_PVI_CTL); > + > + pm_runtime_put(pvi->dev); > +} > + > +static u32 *imx8mp_hdmi_pvi_bridge_get_input_bus_fmts(struct > drm_bridge *bridge, > + struct drm_bridge_state > *bridge_state, > + struct drm_crtc_state > *crtc_state, > + struct drm_connector_state > *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) Please fix the checkpatch.pl warning with --strict parameter: CHECK: Alignment should match open parenthesis #151: FILE: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:99: +static u32 *imx8mp_hdmi_pvi_bridge_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, > +{ > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > + struct drm_bridge *next_bridge = pvi->next_bridge; > + struct drm_bridge_state *next_state; > + > + if (!next_bridge->funcs->atomic_get_input_bus_fmts) > + return 0; > + > + next_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + next_bridge); > + > + return next_bridge->funcs- > >atomic_get_input_bus_fmts(next_bridge, > + next_state > , > + crtc_state > , > + conn_state > , > + output_fmt > , > + num_input_ > fmts); > +} > + > +static const struct drm_bridge_funcs imx_hdmi_pvi_bridge_funcs = { > + .attach = imx8mp_hdmi_pvi_bridge_attach, > + .atomic_enable = imx8mp_hdmi_pvi_bridge_enable, > + .atomic_disable = imx8mp_hdmi_pvi_bridge_disable, > + .atomic_get_input_bus_fmts = > imx8mp_hdmi_pvi_bridge_get_input_bus_fmts, > + .atomic_duplicate_state = > drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > +}; > + > +static int imx8mp_hdmi_pvi_probe(struct platform_device *pdev) > +{ > + struct device_node *remote; > + struct imx8mp_hdmi_pvi *pvi; > + > + pvi = devm_kzalloc(&pdev->dev, sizeof(*pvi), GFP_KERNEL); > + if (!pvi) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, pvi); > + pvi->dev = &pdev->dev; > + > + pvi->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pvi->regs)) > + return PTR_ERR(pvi->regs); > + > + /* Get the next bridge in the pipeline. */ > + remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1); > + if (!remote) > + return -EINVAL; > + > + pvi->next_bridge = of_drm_find_bridge(remote); > + of_node_put(remote); > + > + if (!pvi->next_bridge) > + return dev_err_probe(&pdev->dev, -EPROBE_DEFER, > + "could not find next bridge\n"); > + > + /* Register the bridge. */ > + pvi->bridge.funcs = &imx_hdmi_pvi_bridge_funcs; > + pvi->bridge.of_node = pdev->dev.of_node; > + pvi->bridge.timings = pvi->next_bridge->timings; > + > + drm_bridge_add(&pvi->bridge); > + > + pm_runtime_enable(&pdev->dev); > + > + return 0; > +} > + > +static int imx8mp_hdmi_pvi_remove(struct platform_device *pdev) > +{ > + struct imx8mp_hdmi_pvi *pvi = platform_get_drvdata(pdev); > + > + drm_bridge_remove(&pvi->bridge); Missing the function call for pm_runtime_disable(). > + > + return 0; > +} > + > +static const struct of_device_id imx8mp_hdmi_pvi_match[] = { > + { > + .compatible = "fsl,imx8mp-hdmi-pvi", > + }, { > + /* sentinel */ > + }, Nitpick: ',' after the sentinel is not needed since it's the last one. Regards, Liu Ying > +}; > +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pvi_match); > + > +static struct platform_driver imx8mp_hdmi_pvi_driver = { > + .probe = imx8mp_hdmi_pvi_probe, > + .remove = imx8mp_hdmi_pvi_remove, > + .driver = { > + .name = "imx-hdmi-pvi", > + .of_match_table = imx8mp_hdmi_pvi_match, > + }, > +}; > +module_platform_driver(imx8mp_hdmi_pvi_driver); > + > +MODULE_DESCRIPTION("i.MX8MP HDMI TX Parallel Video Interface bridge > driver"); > +MODULE_LICENSE("GPL");
On Fri, Dec 16, 2022 at 10:07:39PM +0100, Lucas Stach wrote: > The HDMI TX controller on the i.MX8MP SoC is a Synopsys designware IP > core with a little bit of SoC integration around it. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > .../bindings/display/imx/fsl,imx8mp-hdmi.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > > diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > new file mode 100644 > index 000000000000..75ebeaa8c9d5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8mp-hdmi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Freescale i.MX8MP DWC HDMI TX Encoder > + > +maintainers: > + - Lucas Stach <l.stach@pengutronix.de> > + > +description: | Don't need '|'. > + The i.MX8MP HDMI transmitter is a Synopsys DesignWare > + HDMI 2.0 TX controller IP. > + > +allOf: > + - $ref: ../bridge/synopsys,dw-hdmi.yaml# /schemas/display/bridge/... > + > +properties: > + compatible: > + enum: > + - fsl,imx8mp-hdmi > + > + reg-io-width: > + const: 1 > + > + clocks: > + maxItems: 5 > + > + clock-names: > + items: > + - const: iahb > + - const: isfr > + - const: fdcc > + - const: cec > + - const: pix > + > + power-domains: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - power-domains > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/clock/imx8mp-clock.h> > + #include <dt-bindings/power/imx8mp-power.h> > + > + hdmi@32fd8000 { > + compatible = "fsl,imx8mp-hdmi"; > + reg = <0x32fd8000 0x7eff>; > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MP_CLK_HDMI_APB>, > + <&clk IMX8MP_CLK_HDMI_REF_266M>, > + <&clk IMX8MP_CLK_HDMI_FDCC_TST>, > + <&clk IMX8MP_CLK_32K>, > + <&hdmi_tx_phy>; > + clock-names = "iahb", "isfr", "fdcc", "cec", "pix"; > + power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_HDMI_TX>; > + reg-io-width = <1>; ports? This block isn't connected to anything? Like an 'hdmi-connector'? Rob
On Sat, Dec 17, 2022 at 03:40:24PM +0800, Liu Ying wrote: > Hi Lucas, > > On Fri, 2022-12-16 at 22:07 +0100, Lucas Stach wrote: > > The HDMI TX controller on the i.MX8MP SoC is a Synopsys designware IP > > core with a little bit of SoC integration around it. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > .../bindings/display/imx/fsl,imx8mp-hdmi.yaml | 69 > > +++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > > b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml > > new file mode 100644 > > index 000000000000..75ebeaa8c9d5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp- > > hdmi.yaml > > @@ -0,0 +1,69 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/imx/fsl,imx8mp-hdmi.yaml# > > Better to put the binding documentation under the display/bridge > umbrella as the corresponding linux driver is a DRM bridge driver, not > a DRM encoder driver. Bridge vs. encoder is not a distinction I would make for bindings. It would be better if all the HDMI encoders/bridges were grouped together rather than in vendor silos/directories. But that's a much bigger restructuring and fsl,imx6-hdmi.yaml is already here. > > Regarding the file name, I would use 'fsl,imx8mp-hdmi-tx.yaml' to > explicitly tell it's a TX controller(not a RX controller), which > matches the chapter name 'HDMI TX controller' in i.MX8mp RM. > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Freescale i.MX8MP DWC HDMI TX Encoder > > + > > +maintainers: > > + - Lucas Stach <l.stach@pengutronix.de> > > + > > +description: | > > + The i.MX8MP HDMI transmitter is a Synopsys DesignWare > > + HDMI 2.0 TX controller IP. > > i.MX8mp RM says it is compatible with the HDMI v2.0a spec, so better to > mention 2.0a instead of 2.0. > > > + > > +allOf: > > + - $ref: ../bridge/synopsys,dw-hdmi.yaml# > > + > > +properties: > > + compatible: > > + enum: > > + - fsl,imx8mp-hdmi > > Like the file name, I would use 'fsl,imx8mp-hdmi-tx'. > > It seems that the i.MX6q DW HDMI TX controller will not easily use this > binding since it's corresponding driver is a DRM encoder driver, and no > other i.MX SoCs embed the controller, so use const instead of enum(It > can be changed to enum when necessary later.)? That shouldn't matter for bindings. I do expect the 'ports' will be a bit different, so probably not worth trying to combine the schema. Rob
Hi Lucas, hope I got the latest version of this series. If not, please feel free to point me to the correct one. On Fri, Dec 16, 2022 at 10:07:42PM +0100, Lucas Stach wrote: > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > full timing generator and can switch between different video sources. On > the i.MX8MP however the only supported source is the LCDIF. The block > just needs to be powered up and told about the polarity of the video > sync signals to act in bypass mode. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Tested-by: Marek Vasut <marex@denx.de> I've successfully tested this patch on our custom i.MX8MP board. The test case was basically "cat /dev/urandom > /dev/fb1" with a 800x480 HDMI display. Therefore please feel free to add: Tested-by: Richard Leitner <richard.leitner@skidata.com> > --- > drivers/gpu/drm/bridge/imx/Kconfig | 7 + > drivers/gpu/drm/bridge/imx/Makefile | 1 + > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 202 +++++++++++++++++++ > 3 files changed, 210 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c >
Hi Lucas, On Fri, Dec 16, 2022 at 10:07:40PM +0100, Lucas Stach wrote: > Add a simple wrapper driver for the DWC HDMI bridge driver that > implements the few bits that are necessary to abstract the i.MX8MP > SoC integration. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Marek Vasut <marex@denx.de> I've successfully tested this patch on our custom i.MX8MP board. The test case was basically "cat /dev/urandom > /dev/fb1" with a 800x480 HDMI display. Therefore please feel free to add: Tested-by: Richard Leitner <richard.leitner@skidata.com> > --- > drivers/gpu/drm/bridge/imx/Kconfig | 9 ++ > drivers/gpu/drm/bridge/imx/Makefile | 2 + > drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c | 140 +++++++++++++++++++++++ > 3 files changed, 151 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi.c
On Tue, Mar 7, 2023 at 7:07 AM Richard Leitner <richard.leitner@linux.dev> wrote: > > Hi Lucas, > > hope I got the latest version of this series. If not, please feel free > to point me to the correct one. > > On Fri, Dec 16, 2022 at 10:07:42PM +0100, Lucas Stach wrote: > > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > > full timing generator and can switch between different video sources. On > > the i.MX8MP however the only supported source is the LCDIF. The block > > just needs to be powered up and told about the polarity of the video > > sync signals to act in bypass mode. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > Tested-by: Marek Vasut <marex@denx.de> > > I've successfully tested this patch on our custom i.MX8MP board. The > test case was basically "cat /dev/urandom > /dev/fb1" with a 800x480 > HDMI display. > > Therefore please feel free to add: > > Tested-by: Richard Leitner <richard.leitner@skidata.com> > Lucas, Is there going to be a subsequent rev of this series? It seems to be stuck somewhere without any movement. thanks adam > > --- > > drivers/gpu/drm/bridge/imx/Kconfig | 7 + > > drivers/gpu/drm/bridge/imx/Makefile | 1 + > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 202 +++++++++++++++++++ > > 3 files changed, 210 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > >
On 16.12.22 22:07, Lucas Stach wrote: > Add a simple wrapper driver for the DWC HDMI bridge driver that > implements the few bits that are necessary to abstract the i.MX8MP > SoC integration. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Marek Vasut <marex@denx.de> I tested this on our Kontron BL i.MX8MP board. Feel free to add: Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de>
On 16.12.22 22:07, Lucas Stach wrote: > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > full timing generator and can switch between different video sources. On > the i.MX8MP however the only supported source is the LCDIF. The block > just needs to be powered up and told about the polarity of the video > sync signals to act in bypass mode. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Tested-by: Marek Vasut <marex@denx.de> I tested this on our Kontron BL i.MX8MP board. Feel free to add: Tested-by: Frieder Schrempf <frieder.schrempf@kontron.de> Lucas, any chance that you can revive this series and bring it over the finish line?
Hi Lucas, On Fri, 16 Dec 2022 22:07:40 +0100 Lucas Stach <l.stach@pengutronix.de> wrote: > Add a simple wrapper driver for the DWC HDMI bridge driver that > implements the few bits that are necessary to abstract the i.MX8MP > SoC integration. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Tested-by: Marek Vasut <marex@denx.de> I realized I had sent my Tested-by to v1 when v2 was already out. So, in case you still need some encouragement for keeping on with this series: [Tested on a custom board using modetest on v6.5-rc6] Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Hi Lucas, On Fri, 16 Dec 2022 22:07:42 +0100 Lucas Stach <l.stach@pengutronix.de> wrote: > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > full timing generator and can switch between different video sources. On > the i.MX8MP however the only supported source is the LCDIF. The block > just needs to be powered up and told about the polarity of the video > sync signals to act in bypass mode. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Tested-by: Marek Vasut <marex@denx.de> > --- > drivers/gpu/drm/bridge/imx/Kconfig | 7 + > drivers/gpu/drm/bridge/imx/Makefile | 1 + > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c | 202 +++++++++++++++++++ > 3 files changed, 210 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig > index d828d8bfd893..e6cc4000bccd 100644 > --- a/drivers/gpu/drm/bridge/imx/Kconfig > +++ b/drivers/gpu/drm/bridge/imx/Kconfig > @@ -53,4 +53,11 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE > Choose this to enable support for the internal HDMI encoder found > on the i.MX8MP SoC. > > +config DRM_IMX8MP_HDMI_PVI > + tristate "i.MX8MP HDMI PVI bridge support" > + depends on OF > + help > + Choose this to enable support for the internal HDMI TX Parallel > + Video Interface found on the i.MX8MP SoC. > + > endif # ARCH_MXC || COMPILE_TEST > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile > index 03b0074ae538..b0fd56550dad 100644 > --- a/drivers/gpu/drm/bridge/imx/Makefile > +++ b/drivers/gpu/drm/bridge/imx/Makefile > @@ -9,3 +9,4 @@ obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o > obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o > > obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi.o > +obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > new file mode 100644 > index 000000000000..30d40c21dabb > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > @@ -0,0 +1,202 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * Copyright (C) 2022 Pengutronix, Lucas Stach <kernel@pengutronix.de> > + */ > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_crtc.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_graph.h> > +#include <linux/pm_runtime.h> > + > +#define HTX_PVI_CTL 0x0 Personally I would s/CTL/CTRL/, to be consistent with the manual and thus more search-friendly. > +#define PVI_CTL_OP_VSYNC_POL BIT(18) > +#define PVI_CTL_OP_HSYNC_POL BIT(17) > +#define PVI_CTL_OP_DE_POL BIT(16) > +#define PVI_CTL_INP_VSYNC_POL BIT(14) > +#define PVI_CTL_INP_HSYNC_POL BIT(13) > +#define PVI_CTL_INP_DE_POL BIT(12) > +#define PVI_CTL_INPUT_LCDIF BIT(2) According to the reference manual there is actually a 2-bit field here: HTX_PVI_MODE, using bits 2:1, and whose "LCDIF" value is 0b10. Thus while it obviously won't change the resulting code, it seems more correct to define this as (2 << 1). > +static void imx8mp_hdmi_pvi_bridge_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state) > +{ > + struct drm_atomic_state *state = bridge_state->base.state; > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > + struct drm_connector_state *conn_state; > + const struct drm_display_mode *mode; > + struct drm_crtc_state *crtc_state; > + struct drm_connector *connector; > + u32 bus_flags, val; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > + > + if (WARN_ON(pm_runtime_resume_and_get(pvi->dev))) > + return; > + > + mode = &crtc_state->adjusted_mode; > + > + val = PVI_CTL_INPUT_LCDIF; > + > + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > + val |= PVI_CTL_OP_VSYNC_POL | PVI_CTL_INP_VSYNC_POL; > + > + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > + val |= PVI_CTL_OP_HSYNC_POL | PVI_CTL_INP_HSYNC_POL; > + > + if (pvi->next_bridge->timings) > + bus_flags = pvi->next_bridge->timings->input_bus_flags; > + else if (bridge_state) > + bus_flags = bridge_state->input_bus_cfg.flags; > + > + if (bus_flags & DRM_BUS_FLAG_DE_HIGH) > + val |= PVI_CTL_OP_DE_POL | PVI_CTL_INP_DE_POL; > + > + writel(val, pvi->regs + HTX_PVI_CTL); > + val |= PVI_CTL_EN; > + writel(val, pvi->regs + HTX_PVI_CTL); I guess I'm missing something here: why can't one just write the register once, with the enable bit set? I tried removing the first writel() and everything seems to work just the same. With these fixed: Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> And definitely: [Tested on a custom board using modetest on v6.5-rc6] Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml new file mode 100644 index 000000000000..75ebeaa8c9d5 --- /dev/null +++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml @@ -0,0 +1,69 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/imx/fsl,imx8mp-hdmi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Freescale i.MX8MP DWC HDMI TX Encoder + +maintainers: + - Lucas Stach <l.stach@pengutronix.de> + +description: | + The i.MX8MP HDMI transmitter is a Synopsys DesignWare + HDMI 2.0 TX controller IP. + +allOf: + - $ref: ../bridge/synopsys,dw-hdmi.yaml# + +properties: + compatible: + enum: + - fsl,imx8mp-hdmi + + reg-io-width: + const: 1 + + clocks: + maxItems: 5 + + clock-names: + items: + - const: iahb + - const: isfr + - const: fdcc + - const: cec + - const: pix + + power-domains: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts + - power-domains + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/imx8mp-clock.h> + #include <dt-bindings/power/imx8mp-power.h> + + hdmi@32fd8000 { + compatible = "fsl,imx8mp-hdmi"; + reg = <0x32fd8000 0x7eff>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk IMX8MP_CLK_HDMI_APB>, + <&clk IMX8MP_CLK_HDMI_REF_266M>, + <&clk IMX8MP_CLK_HDMI_FDCC_TST>, + <&clk IMX8MP_CLK_32K>, + <&hdmi_tx_phy>; + clock-names = "iahb", "isfr", "fdcc", "cec", "pix"; + power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_HDMI_TX>; + reg-io-width = <1>; + };
The HDMI TX controller on the i.MX8MP SoC is a Synopsys designware IP core with a little bit of SoC integration around it. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- .../bindings/display/imx/fsl,imx8mp-hdmi.yaml | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi.yaml