| Message ID | 20210816192523.1739365-1-msp@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add mt8195 DisplayPort driver | expand |
Hi Markus, A few general things in the following. This is what I look for first in a bridge driver - and I had no time today to review the driver in full. Please address these, then cc: me on next revision where I hopefully have more time. Sam > +static int mtk_dp_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + int ret; > + > + mtk_dp_poweron(mtk_dp); > + > + if (mtk_dp->next_bridge) { > + ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge, > + &mtk_dp->bridge, flags); > + if (ret) { > + drm_warn(mtk_dp->drm_dev, > + "Failed to attach external bridge: %d\n", ret); > + return ret; > + } > + } > + > + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > + drm_err(mtk_dp->drm_dev, > + "Fix bridge driver to make connector optional!"); > + return 0; > + } This driver is only used by mediatek, and I thought all of mediatek is converted so the display driver creates the connector. It would be better to migrate mediatek over to always let the display driver create the connector and drop the connector support in this driver. > + struct drm_bridge_funcs mtk_dp_bridge_funcs = { > + .attach = mtk_dp_bridge_attach, > + .mode_fixup = mtk_dp_bridge_mode_fixup, > + .disable = mtk_dp_bridge_disable, > + .post_disable = mtk_dp_bridge_post_disable, > + .mode_set = mtk_dp_bridge_mode_set, > + .pre_enable = mtk_dp_bridge_pre_enable, > + .enable = mtk_dp_bridge_enable, > + .get_edid = mtk_get_edid, > + .detect = mtk_dp_bdg_detect, > +}; For new drivers please avoid the recently deprecated functions. - Use the atomic versions of pre_enable, enable, disable and post_disable. - Merge mode_set with atomic_enable - as there is no need for the mode_Set operation. - Use atomic_check in favour of mode_fixup, albeit the rules for atomic_check is at best vauge at the moment.
Hi, Markus: On Mon, 2021-08-16 at 21:25 +0200, Markus Schneider-Pargmann wrote: > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports both functional units on the mt8195, the embedded > DisplayPort as well as the external DisplayPort units. It offers > hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up > to 4 lanes. > > This driver is based on an initial version by > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > --- [snip] > + > +static const struct of_device_id mtk_dp_of_match[] = { > + { > + .compatible = "mediatek,mt8195-dp_tx", Where is the binding document of "mediatek,mt8195-dp_tx"? > + .data = &mt8195_dp_driver_data, > + }, > + { > + .compatible = "mediatek,mt8195-edp_tx", Where is the binding document of "mediatek,mt8195-edp_tx"? > + .data = &mt8195_edp_driver_data, > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, mtk_dp_of_match); > + > +struct platform_driver mtk_dp_driver = { > + .probe = mtk_dp_probe, > + .remove = mtk_dp_remove, > + .driver = { > + .name = "mediatek-drm-dp", > + .of_match_table = mtk_dp_of_match, > + .pm = &mtk_dp_pm_ops, > + }, > +}; > + > diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > new file mode 100644 > index 000000000000..83afc79d98ff > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > @@ -0,0 +1,3095 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2019 MediaTek Inc. > + * Copyright (c) 2021 BayLibre > + */ > +#ifndef _MTK_DP_REG_H_ > +#define _MTK_DP_REG_H_ > + > +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 > +# define MTK_DP_SIP_ATF_VIDEO_UNMUTE 0x20 > +# define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE 0x21 > +# define MTK_DP_SIP_ATF_REG_WRITE 0x22 > +# define MTK_DP_SIP_ATF_REG_READ 0x23 > +# define MTK_DP_SIP_ATF_CMD_COUNT 0x24 > + > +#define TOP_OFFSET 0x2000 > +#define ENC0_OFFSET 0x3000 > +#define ENC1_OFFSET 0x3200 > +#define TRANS_OFFSET 0x3400 > +#define AUX_OFFSET 0x3600 > +#define SEC_OFFSET 0x4000 > + > +#define MTK_DP_HPD_DISCONNECT BIT(1) > +#define MTK_DP_HPD_CONNECT BIT(2) > +#define MTK_DP_HPD_INTERRUPT BIT(3) > + > +#define MTK_DP_ENC0_P0_3000 (ENC0_OFFSET + 0x000) > +# define LANE_NUM_DP_ENC0_P0_MASK 0x3 > +# define LANE_NUM_DP_ENC0_P0_SHIFT 0 > +# define VIDEO_MUTE_SW_DP_ENC0_P0_MASK 0x4 > +# define VIDEO_MUTE_SW_DP_ENC0_P0_SHIFT 2 > +# define VIDEO_MUTE_SEL_DP_ENC0_P0_MASK 0x8 > +# define VIDEO_MUTE_SEL_DP_ENC0_P0_SHIFT 3 > +# define ENHANCED_FRAME_EN_DP_ENC0_P0_MASK 0x10 > +# define ENHANCED_FRAME_EN_DP_ENC0_P0_SHIFT 4 > +# define HDCP_FRAME_EN_DP_ENC0_P0_MASK 0x20 > +# define HDCP_FRAME_EN_DP_ENC0_P0_SHIFT 5 > +# define IDP_EN_DP_ENC0_P0_MASK 0x40 Remove useless definition. Regards, CK. > +# define IDP_EN_DP_ENC0_P0_SHIFT 6 > +# define BS_SYMBOL_CNT_RESET_DP_ENC0_P0_MASK 0x80 > +# define BS_SYMBOL_CNT_RESET_DP_ENC0_P0_SHIFT 7 > +# define MIXER_DUMMY_DATA_DP_ENC0_P0_MASK 0xff00 > +# define MIXER_DUMMY_DATA_DP_ENC0_P0_SHIFT 8 > + > + > +#endif /*_MTK_DP_REG_H_*/
Hi Sam, On Mon, Aug 16, 2021 at 11:36:13PM +0200, Sam Ravnborg wrote: > Hi Markus, > > A few general things in the following. This is what I look for first in > a bridge driver - and I had no time today to review the driver in full. > Please address these, then cc: me on next revision where I hopefully > have more time. Thanks for taking the time and giving me the tips, will fix it and send a new version. Best, Markus > > Sam > > > +static int mtk_dp_bridge_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + int ret; > > + > > + mtk_dp_poweron(mtk_dp); > > + > > + if (mtk_dp->next_bridge) { > > + ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge, > > + &mtk_dp->bridge, flags); > > + if (ret) { > > + drm_warn(mtk_dp->drm_dev, > > + "Failed to attach external bridge: %d\n", ret); > > + return ret; > > + } > > + } > > + > > + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > > + drm_err(mtk_dp->drm_dev, > > + "Fix bridge driver to make connector optional!"); > > + return 0; > > + } > > This driver is only used by mediatek, and I thought all of mediatek is > converted so the display driver creates the connector. > > It would be better to migrate mediatek over to always let the display > driver create the connector and drop the connector support in this > driver. > > > > + struct drm_bridge_funcs mtk_dp_bridge_funcs = { > > + .attach = mtk_dp_bridge_attach, > > + .mode_fixup = mtk_dp_bridge_mode_fixup, > > + .disable = mtk_dp_bridge_disable, > > + .post_disable = mtk_dp_bridge_post_disable, > > + .mode_set = mtk_dp_bridge_mode_set, > > + .pre_enable = mtk_dp_bridge_pre_enable, > > + .enable = mtk_dp_bridge_enable, > > + .get_edid = mtk_get_edid, > > + .detect = mtk_dp_bdg_detect, > > +}; > > > For new drivers please avoid the recently deprecated functions. > > - Use the atomic versions of pre_enable, enable, disable and post_disable. > > - Merge mode_set with atomic_enable - as there is no need for the mode_Set > operation. > > - Use atomic_check in favour of mode_fixup, albeit the rules for > atomic_check is at best vauge at the moment. > >
Hi, On Tue, Aug 17, 2021 at 01:36:45PM +0800, CK Hu wrote: > Hi, Markus: > > On Mon, 2021-08-16 at 21:25 +0200, Markus Schneider-Pargmann wrote: > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > It supports both functional units on the mt8195, the embedded > > DisplayPort as well as the external DisplayPort units. It offers > > hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up > > to 4 lanes. > > > > This driver is based on an initial version by > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > --- > > [snip] > > > + > > +static const struct of_device_id mtk_dp_of_match[] = { > > + { > > + .compatible = "mediatek,mt8195-dp_tx", > > Where is the binding document of "mediatek,mt8195-dp_tx"? > > > + .data = &mt8195_dp_driver_data, > > + }, > > + { > > + .compatible = "mediatek,mt8195-edp_tx", > > Where is the binding document of "mediatek,mt8195-edp_tx"? I didn't include the bindings in this RFC, but will include them in the next version. > > > + .data = &mt8195_edp_driver_data, > > + }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, mtk_dp_of_match); > > + > > +struct platform_driver mtk_dp_driver = { > > + .probe = mtk_dp_probe, > > + .remove = mtk_dp_remove, > > + .driver = { > > + .name = "mediatek-drm-dp", > > + .of_match_table = mtk_dp_of_match, > > + .pm = &mtk_dp_pm_ops, > > + }, > > +}; > > + > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > > new file mode 100644 > > index 000000000000..83afc79d98ff > > --- /dev/null > > +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > > @@ -0,0 +1,3095 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (c) 2019 MediaTek Inc. > > + * Copyright (c) 2021 BayLibre > > + */ > > +#ifndef _MTK_DP_REG_H_ > > +#define _MTK_DP_REG_H_ > > + > > +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 > > +# define MTK_DP_SIP_ATF_VIDEO_UNMUTE 0x20 > > +# define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE 0x21 > > +# define MTK_DP_SIP_ATF_REG_WRITE 0x22 > > +# define MTK_DP_SIP_ATF_REG_READ 0x23 > > +# define MTK_DP_SIP_ATF_CMD_COUNT 0x24 > > + > > +#define TOP_OFFSET 0x2000 > > +#define ENC0_OFFSET 0x3000 > > +#define ENC1_OFFSET 0x3200 > > +#define TRANS_OFFSET 0x3400 > > +#define AUX_OFFSET 0x3600 > > +#define SEC_OFFSET 0x4000 > > + > > +#define MTK_DP_HPD_DISCONNECT BIT(1) > > +#define MTK_DP_HPD_CONNECT BIT(2) > > +#define MTK_DP_HPD_INTERRUPT BIT(3) > > + > > +#define MTK_DP_ENC0_P0_3000 (ENC0_OFFSET + 0x000) > > +# define LANE_NUM_DP_ENC0_P0_MASK 0x3 > > +# define LANE_NUM_DP_ENC0_P0_SHIFT 0 > > +# define VIDEO_MUTE_SW_DP_ENC0_P0_MASK 0x4 > > +# define VIDEO_MUTE_SW_DP_ENC0_P0_SHIFT 2 > > +# define VIDEO_MUTE_SEL_DP_ENC0_P0_MASK 0x8 > > +# define VIDEO_MUTE_SEL_DP_ENC0_P0_SHIFT 3 > > +# define ENHANCED_FRAME_EN_DP_ENC0_P0_MASK 0x10 > > +# define ENHANCED_FRAME_EN_DP_ENC0_P0_SHIFT 4 > > +# define HDCP_FRAME_EN_DP_ENC0_P0_MASK 0x20 > > +# define HDCP_FRAME_EN_DP_ENC0_P0_SHIFT 5 > > +# define IDP_EN_DP_ENC0_P0_MASK 0x40 > > Remove useless definition. Ok, will do. Thanks for the feedback. Best, Markus > > Regards, > CK. > > > +# define IDP_EN_DP_ENC0_P0_SHIFT 6 > > +# define BS_SYMBOL_CNT_RESET_DP_ENC0_P0_MASK 0x80 > > +# define BS_SYMBOL_CNT_RESET_DP_ENC0_P0_SHIFT 7 > > +# define MIXER_DUMMY_DATA_DP_ENC0_P0_MASK 0xff00 > > +# define MIXER_DUMMY_DATA_DP_ENC0_P0_SHIFT 8 > > + > > > + > > +#endif /*_MTK_DP_REG_H_*/ >
Hi, Markus: On Mon, 2021-08-16 at 21:25 +0200, Markus Schneider-Pargmann wrote: > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports both functional units on the mt8195, the embedded > DisplayPort as well as the external DisplayPort units. It offers > hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up > to 4 lanes. > > This driver is based on an initial version by > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > --- > drivers/gpu/drm/mediatek/Kconfig | 7 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 3025 ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 3095 +++++++++++++++++++++++++ > 4 files changed, 6129 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig > index 2976d21e9a34..d81eb3521c1c 100644 > --- a/drivers/gpu/drm/mediatek/Kconfig > +++ b/drivers/gpu/drm/mediatek/Kconfig > @@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI > select PHY_MTK_HDMI > help > DRM/KMS HDMI driver for Mediatek SoCs > + > +config MTK_DPTX_SUPPORT > + tristate "DRM DPTX Support for Mediatek SoCs" > + depends on DRM_MEDIATEK > + select GENERIC_PHY Why select GENERIC_PHY? If this is a phy driver, place this driver in drivers/phy/mediatek/ Regards, CK > + help > + DRM/KMS Display Port driver for Mediatek SoCs. > diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile > index dc54a7a69005..6b9d148ab7fe 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -20,3 +20,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \ > mtk_hdmi_ddc.o >