| Message ID | 20220523104758.29531-1-granquet@baylibre.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add mt8195 DisplayPort driver | expand |
On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > DPINTF is similar to DPI but does not have the exact same feature set > or register layouts. > > DPINTF is the sink of the display pipeline that is connected to the > DisplayPort controller and encoder unit. It takes the same clocks as > DPI. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > .../bindings/display/mediatek/mediatek,dpi.yaml | 13 ++++++++--- > -- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yam > l > b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yam > l > index dd2896a40ff0..6d9f6c11806e 100644 > --- > a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yam > l > +++ > b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yam > l > @@ -4,16 +4,16 @@ > $id: > http://devicetree.org/schemas/display/mediatek/mediatek,dpi.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: mediatek DPI Controller Device Tree Bindings > +title: mediatek DPI/DPINTF Controller > > maintainers: > - CK Hu <ck.hu@mediatek.com> > - Jitao shi <jitao.shi@mediatek.com> > > description: | > - The Mediatek DPI function block is a sink of the display subsystem > and > - provides 8-bit RGB/YUV444 or 8/10/10-bit YUV422 pixel data on a > parallel > - output bus. > + The Mediatek DPI and DPINTF function blocks are a sink of the > display > + subsystem and provides 8-bit RGB/YUV444 or 8/10/10-bit YUV422 > pixel data on a > + parallel output bus. > > properties: > compatible: > @@ -23,6 +23,7 @@ properties: > - mediatek,mt8173-dpi > - mediatek,mt8183-dpi > - mediatek,mt8192-dpi > + - mediatek,mt8195-dpintf > > reg: > maxItems: 1 > @@ -35,12 +36,14 @@ properties: > - description: Pixel Clock > - description: Engine Clock > - description: DPI PLL > + - description: Optional CK CG Clock > > clock-names: > items: > - const: pixel > - const: engine > - const: pll > + - const: ck_cg 'ck_cg' seems not a exact clock names, could you pleas check it again with DE. > > pinctrl-0: true > pinctrl-1: true > @@ -54,7 +57,7 @@ properties: > $ref: /schemas/graph.yaml#/properties/port > description: > Output port node. This port should be connected to the input > port of an > - attached HDMI or LVDS encoder chip. > + attached HDMI, LVDS or DisplayPort encoder chip. > > required: > - compatible
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > 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> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > [snip] > + > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) I don't have a DP spec. I find one in [1] but I'm not sure it's real spec or not. If it's real, in section 3.1.3.2, it describe: 3.1.3.2 Hot Plug/Unplug Detection One signal (HPD) is used by a device (an Upstream device) to detect that a Downstream port on the device has been connected to another device (the Downstream device). Implementation of HPD is optional for an embedded link configuration. At least a “trickle power” must be present both in the Upstream and Downstream devices for a Hot Plug event to be detected. I focus on the statement "Implementation of HPD is optional for an embedded link configuration". I'm not sure what does 'optional' mean. Does it mean eDP panel without HPD signal is possible? If so, I think driver should support eDP panel without HPD signal. Maybe I misunderstanding this spec. Please explain for me. Regards, CK [1] https://glenwing.github.io/docs/DP-1.2.pdf > +{ > + struct mtk_dp *mtk_dp = dev; > + int event; > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > + > + event = mtk_dp_plug_state(mtk_dp) ? connector_status_connected > : > + connector_status_disc > onnected; > + > + if (event < 0) > + return IRQ_HANDLED; > + > + if (mtk_dp->drm_dev) { > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > + } > + > + if (mtk_dp->train_info.cable_state_change) { > + mtk_dp->train_info.cable_state_change = false; > + > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + > + if (!mtk_dp->train_info.cable_plugged_in || > + !mtk_dp_plug_state(mtk_dp)) { > + mtk_dp_video_mute(mtk_dp, true); > + > + mtk_dp_initialize_priv_data(mtk_dp); > + mtk_dp_set_idle_pattern(mtk_dp, true); > + if (mtk_dp->has_fec) > + mtk_dp_fec_enable(mtk_dp, false); > + > + mtk_dp_update_bits(mtk_dp, > MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL, > + DP_PWR_STATE_MASK); > + } else { > + mtk_dp_update_bits(mtk_dp, > MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LA > NE, > + DP_PWR_STATE_MASK); > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + mtk_dp->train_info.link_rate = > + min_t(int, mtk_dp->max_linkrate, > + buf[mtk_dp->max_linkrate]); > + mtk_dp->train_info.lane_count = > + min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(buf)); > + } > + } > + > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > + mtk_dp_hpd_sink_event(mtk_dp); > + } > + > + return IRQ_HANDLED; > +} > +
Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > Similar to HDMI, DP uses audio infoframes as well which are structured > very similar to the HDMI ones. > > This patch adds a helper function to pack the HDMI audio infoframe for > DP, called hdmi_audio_infoframe_pack_for_dp(). > hdmi_audio_infoframe_pack_only() is split into two parts. One of them > packs the payload only and can be used for HDMI and DP. > > Also constify the frame parameter in hdmi_audio_infoframe_check() as > it is passed to hdmi_audio_infoframe_check_only() which expects a const. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > drivers/video/hdmi.c | 82 ++++++++++++++++++++++++++-------- > include/drm/dp/drm_dp_helper.h | 2 + this has been moved... again... this time it's include/drm/display/drm_dp_helper.h > include/linux/hdmi.h | 7 ++- > 3 files changed, 71 insertions(+), 20 deletions(-) >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > 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> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > [snip] > + > +static irqreturn_t mtk_dp_hpd_isr_handler(struct mtk_dp *mtk_dp) > +{ > + bool connected; > + u16 swirq_status = mtk_dp_swirq_get_clear(mtk_dp); > + u8 hwirq_status = mtk_dp_hwirq_get_clear(mtk_dp); > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + > + train_info->irq_status |= hwirq_status | swirq_status; You mix software control variable and irq status here. Break each flag in irq_status to variables in train_info to decouple software control variable and irq status. > + > + if (!train_info->irq_status) > + return IRQ_HANDLED; > + > + connected = mtk_dp_plug_state(mtk_dp); > + if (connected || !train_info->cable_plugged_in) > + train_info->irq_status &= ~MTK_DP_HPD_DISCONNECT; > + else if (!connected || train_info->cable_plugged_in) > + train_info->irq_status &= ~MTK_DP_HPD_CONNECT; > + > + if (!(train_info->irq_status & > + (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT))) > + return IRQ_HANDLED; > + > + if (train_info->irq_status & MTK_DP_HPD_CONNECT) { > + train_info->irq_status &= ~MTK_DP_HPD_CONNECT; > + train_info->cable_plugged_in = true; > + } else { > + train_info->irq_status &= ~MTK_DP_HPD_DISCONNECT; > + train_info->cable_plugged_in = false; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + } > + train_info->cable_state_change = true; > + > + return IRQ_WAKE_THREAD; > +} > +
On Thu, 2022-06-02 at 11:50 +0800, Rex-BC Chen wrote: > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > this series is built around the DisplayPort driver. The dpi/dpintf > > driver and the added helper functions are required for the > > DisplayPort > > driver to work. > > > > This v10 still has some un-answered comments and TODOs for v11. > > > > This has been tested sucessfully on a 5.18-next based "vendor > > branch". > > > > There's a missing dependency in the mediatek clock framework to > > allow > > a > > mux clock to change it's parent automatically on rate change. > > Without this change, the dpi driver won't properly set the clocks > > on > > mode change and thus nothing will be displayed on screen. > > > > Changes from v9: > > - The DP-Phy is back to being a child device of the DP driver (as > > in > > v8) > > - hot plug detection has been added back to Embedded Display > > Port... > > as > > after discussing with mediatek experts, this is needed eventhough > > the > > Embedded Display port is not un-pluggable > > - rebased on linux-next > > - simplified/split train_handler function, as suggested by Rex > > - added comments on the sleep/delays present in the code > > - removed previous patch introducing retries when receiving > > AUX_DEFER > > as > > this is already handled in the dp_aux framework > > - added max-lane and max-linkrate device tree u8 properties instead > > of > > hardcoded #defines > > > > Things that are in my todolist for v11: > > - retrieve CK/DE support from panel driver instead of hardcoding it > > into > > the dpi driver > > - refcount the dp driver "enabled" status for "future proofing" > > - review the drm_dp_helpers for features/functions that have been > > re-implemented in the mediatek dp drivers > > > > Older revisions: > > RFC - > > https://lore.kernel.org/linux-mediatek/20210816192523.1739365-1-msp@baylibre.com/ > > v1 - > > https://lore.kernel.org/linux-mediatek/20210906193529.718845-1-msp@baylibre.com/ > > v2 - > > https://lore.kernel.org/linux-mediatek/20210920084424.231825-1-msp@baylibre.com/ > > v3 - > > https://lore.kernel.org/linux-mediatek/20211001094443.2770169-1-msp@baylibre.com/ > > v4 - > > https://lore.kernel.org/linux-mediatek/20211011094624.3416029-1-msp@baylibre.com/ > > v5 - > > https://lore.kernel.org/all/20211021092707.3562523-1-msp@baylibre.com/ > > v6 - > > https://lore.kernel.org/linux-mediatek/20211110130623.20553-1-granquet@baylibre.com/ > > v7 - > > https://lore.kernel.org/linux-mediatek/20211217150854.2081-1-granquet@baylibre.com/ > > v8 - > > https://lore.kernel.org/linux-mediatek/20220218145437.18563-1-granquet@baylibre.com/ > > v9 - > > https://lore.kernel.org/all/20220327223927.20848-1-granquet@baylibre.com/ > > > > Functional dependencies are: > > - Add Mediatek Soc DRM (vdosys0) support for mt8195 > > > > https://lore.kernel.org/linux-mediatek/20220419094143.9561-2-jason-jh.lin@mediatek.com/ > > - Add MediaTek SoC DRM (vdosys1) support for mt8195 > > > > https://lore.kernel.org/linux-mediatek/20220512053128.31415-1-nancy.lin@mediatek.com/ > > > > > > Guillaume Ranquet (15): > > drm/edid: Convert cea_sad helper struct to kernelDoc > > drm/edid: Add cea_sad helpers for freq/length > > drm/mediatek: dpi: move dpi limits to SoC config > > drm/mediatek: dpi: implement a CK/DE pol toggle in SoC config > > drm/mediatek: dpi: implement a swap_input toggle in SoC config > > drm/mediatek: dpi: move dimension mask to SoC config > > drm/mediatek: dpi: move hvsize_mask to SoC config > > drm/mediatek: dpi: move swap_shift to SoC config > > drm/mediatek: dpi: move the yuv422_en_bit to SoC config > > drm/mediatek: dpi: move the csc_enable bit to SoC config > > drm/mediatek: dpi: Add dpintf support > > drm/mediatek: dpi: Only enable dpi after the bridge is enabled > > drm/meditek: dpi: Add matrix_sel helper > > drm/mediatek: Add mt8195 External DisplayPort support > > drm/mediatek: DP audio support for mt8195 > > > > Jitao Shi (1): > > drm/mediatek: add hpd debounce > > > > Markus Schneider-Pargmann (5): > > dt-bindings: mediatek,dpi: Add DPINTF compatible > > dt-bindings: mediatek,dp: Add Display Port binding > > video/hdmi: Add audio_infoframe packing for DP > > phy: phy-mtk-dp: Add driver for DP phy > > drm/mediatek: Add mt8195 Embedded DisplayPort driver > > > > .../display/mediatek/mediatek,dp.yaml | 99 + > > .../display/mediatek/mediatek,dpi.yaml | 13 +- > > MAINTAINERS | 1 + > > drivers/gpu/drm/drm_edid.c | 74 + > > drivers/gpu/drm/mediatek/Kconfig | 8 + > > drivers/gpu/drm/mediatek/Makefile | 2 + > > drivers/gpu/drm/mediatek/mtk_dp.c | 3419 > > +++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 570 +++ > > drivers/gpu/drm/mediatek/mtk_dpi.c | 272 +- > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 38 + > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 + > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 8 +- > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + > > drivers/phy/mediatek/Kconfig | 8 + > > drivers/phy/mediatek/Makefile | 1 + > > drivers/phy/mediatek/phy-mtk-dp.c | 200 + > > drivers/video/hdmi.c | 82 +- > > include/drm/dp/drm_dp_helper.h | 2 + > > include/drm/drm_edid.h | 26 +- > > include/linux/hdmi.h | 7 +- > > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > > 22 files changed, 4765 insertions(+), 81 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c > > Hello all, > > Due to the resource issue, I will keep upstreaming Guillaume's MT8195 > dp/edp series. > > I will check the comments for v8/v9/v10 and have some discussion with > you. > > Thanks for your all comments. > > BRs, > Bo-Chen > Hello all, Because the patches of dp_intf seem to be almost completed, I want to split this series into two series: dp_intf and edp/dp. It will be easier to review and maintain this series. Thanks! BRs, Bo-Chen
On Wed, 2022-05-25 at 13:55 +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > DPINTF is similar to DPI but does not have the exact same feature > > set > > or register layouts. > > > > DPINTF is the sink of the display pipeline that is connected to the > > DisplayPort controller and encoder unit. It takes the same clocks > > as > > DPI. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > .../bindings/display/mediatek/mediatek,dpi.yaml | 13 > > ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.y > > aml > > b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.y > > aml > > index dd2896a40ff0..6d9f6c11806e 100644 > > --- > > a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.y > > aml > > +++ > > b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.y > > aml > > @@ -4,16 +4,16 @@ > > $id: > > http://devicetree.org/schemas/display/mediatek/mediatek,dpi.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: mediatek DPI Controller Device Tree Bindings > > +title: mediatek DPI/DPINTF Controller > > > > maintainers: > > - CK Hu <ck.hu@mediatek.com> > > - Jitao shi <jitao.shi@mediatek.com> > > > > description: | > > - The Mediatek DPI function block is a sink of the display > > subsystem and > > - provides 8-bit RGB/YUV444 or 8/10/10-bit YUV422 pixel data on a > > parallel > > - output bus. > > + The Mediatek DPI and DPINTF function blocks are a sink of the > > display > > + subsystem and provides 8-bit RGB/YUV444 or 8/10/10-bit YUV422 > > pixel data on a > > + parallel output bus. > > > > properties: > > compatible: > > @@ -23,6 +23,7 @@ properties: > > - mediatek,mt8173-dpi > > - mediatek,mt8183-dpi > > - mediatek,mt8192-dpi > > + - mediatek,mt8195-dpintf > > > > reg: > > maxItems: 1 > > @@ -35,12 +36,14 @@ properties: > > - description: Pixel Clock > > - description: Engine Clock > > - description: DPI PLL > > + - description: Optional CK CG Clock > > > > clock-names: > > items: > > - const: pixel > > - const: engine > > - const: pll > > + - const: ck_cg > > This is my understanding on how the DisplayPort Interface clocks work > on 8195: > > The "engine" clock is for the *VPP Engine's DisplayPort ip/block*, > "pll" is for TVD PLL divider selection > "pixel" is the gate for the pixel clock to the connected display. > > "ck_cg" is useless, as that's the parent of "pixel" (and will always > be)... for > example, on mt8195... check clk/mediatek/clk-mt8195-vdo0.c - the > CLK_VDO0_DP_INTF0_DP_INTF clock already has CLK_TOP_EDP as its > parent, hence > enabling the first will enable the latter. > > That said... you can most probably avoid adding the ck_cg clock, as > if you try > to turn that off while it's in use by its children, you'll be only > decrementing > a refcount, but no "real action" will ever take place. > > > Regards, > Angelo Hello Chunfeng and Angelo, ck_cg is a clock gate, and I try to remove it from drivers but it's failed to enable dp_intf. the block diagram is: 1. 26M->CLK_APMIXED_TVDPLL1(pll)->CLK_TOP_EDP(pixel)- >CLK_VDO0_DP_INTF0_DP_INTF(ck_cg)->dp_intf 2. VDOSYS clock->CLK_VDO0_DP_INTF0(engine)->dp_intf "engine" and "ck_cg" are all clock gates which control the clock source input to dp_intf. Maybe we just need to rename it? If so, what name do you think we should modify? BRs, Bo-Chen > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 2022-05-25 at 14:01 +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > include/drm/drm_edid.h | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > > index 144c495b99c4..37c420423625 100644 > > --- a/include/drm/drm_edid.h > > +++ b/include/drm/drm_edid.h > > @@ -359,12 +359,18 @@ struct edid { > > > > #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)- > > >prod_code[1] << 8)) > > > > -/* Short Audio Descriptor */ > > +/** > > + * struct cea_sad - Short Audio Descriptor. > > Perhaps.... > > * struct cea_sad - CEA Short Audio Descriptor > > ...but that's relative to personal liking and nothing else, it's also > fine as > it is, if you like it more as it is. The ball is yours :-P > > Regardless of any choice about changing the description or not: > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > > Cheers, > Angelo > Hello Angelo, ok, I will do this in next version. BRs, Bo-Chen > > + * @format: See HDMI_AUDIO_CODING_TYPE_*. > > + * @channels: max number of channels - 1. > > + * @freq: See CEA_SAD_FREQ_*. > > + * @byte2: meaning depends on format. > > + */ > > struct cea_sad { > > u8 format; > > - u8 channels; /* max number of channels - 1 */ > > + u8 channels; > > u8 freq; > > - u8 byte2; /* meaning depends on format */ > > + u8 byte2; > > }; > > > > struct drm_encoder; > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Mon, 2022-05-30 at 15:44 +0800, CK Hu wrote: > Hi, Guillaume: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > Adds a bit of flexibility to support SoCs without CK/DE pol support > > It seems that DP_INTF has no CK/DE pol function. If so, could you > explain why DP_INTF has this difference with DPI? > > Regards, > CK > Hello CK, Dp_intf does not support CK/DE polarity because the polarity information is not used for eDP and DP while dp_intf is only for eDP and DP. I will add this in commit message in next version. BRs, Bo-Chen > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 22 +++++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 4746eb342567..545a1337cc89 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -125,6 +125,7 @@ struct mtk_dpi_conf { > > bool edge_sel_en; > > const u32 *output_fmts; > > u32 num_output_fmts; > > + bool is_ck_de_pol; > > const struct mtk_dpi_yc_limit *limit; > > }; > > > > @@ -211,13 +212,20 @@ static void mtk_dpi_config_pol(struct mtk_dpi > > *dpi, > > struct mtk_dpi_polarities *dpi_pol) > > { > > unsigned int pol; > > + unsigned int mask; > > > > - pol = (dpi_pol->ck_pol == MTK_DPI_POLARITY_RISING ? 0 : CK_POL) > > > > > > > - (dpi_pol->de_pol == MTK_DPI_POLARITY_RISING ? 0 : DE_POL) > > > > > > > - (dpi_pol->hsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > > HSYNC_POL) | > > + mask = HSYNC_POL | VSYNC_POL; > > + pol = (dpi_pol->hsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > > HSYNC_POL) | > > (dpi_pol->vsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > > VSYNC_POL); > > - mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, pol, > > - CK_POL | DE_POL | HSYNC_POL | VSYNC_POL); > > + if (dpi->conf->is_ck_de_pol) { > > + mask |= CK_POL | DE_POL; > > + pol |= (dpi_pol->ck_pol == MTK_DPI_POLARITY_RISING ? > > + 0 : CK_POL) | > > + (dpi_pol->de_pol == MTK_DPI_POLARITY_RISING ? > > + 0 : DE_POL); > > + } > > + > > + mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, pol, mask); > > } > > > > static void mtk_dpi_config_3d(struct mtk_dpi *dpi, bool en_3d) > > @@ -799,6 +807,7 @@ static const struct mtk_dpi_conf mt8173_conf = > > { > > .max_clock_khz = 300000, > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -809,6 +818,7 @@ static const struct mtk_dpi_conf mt2701_conf = > > { > > .max_clock_khz = 150000, > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -818,6 +828,7 @@ static const struct mtk_dpi_conf mt8183_conf = > > { > > .max_clock_khz = 100000, > > .output_fmts = mt8183_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -827,6 +838,7 @@ static const struct mtk_dpi_conf mt8192_conf = > > { > > .max_clock_khz = 150000, > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > >
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > 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> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) > +{ > + bool training_done = false; > + short max_retry = 50; > + int ret = 0; > + > + do { > + switch (mtk_dp->train_state) { > + case MTK_DP_TRAIN_STATE_STARTUP: mtk_dp->train_state is initialized as MTK_DP_TRAIN_STATE_STARTUP even though HPD ISR does not exist. Does this mean HPD ISR is redundant? If HPD ISR is not redundant, create a new state MTK_DP_TRAIN_STATE_NONE for init state. > + mtk_dp_state_handler(mtk_dp); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_CHECKCAP; > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKCAP: > + if (mtk_dp_parse_capabilities(mtk_dp)) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_CHECKEDID; > + } else { > + mtk_dp->train_info.check_cap_count++; > + > + if (mtk_dp->train_info.check_cap_count > > > + MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT > ) { > + mtk_dp- > >train_info.check_cap_count = 0; > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_DPIDLE; > + ret = -ETIMEDOUT; > + } > + } > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKEDID: > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_TRAINING_PRE; MTK_DP_TRAIN_STATE_CHECKEDID is a redundant state, drop it. > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > + mtk_dp_state_handler(mtk_dp); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_TRAINING; > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING: > + ret = mtk_dp_train_start(mtk_dp); > + if (ret == 0) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_NORMAL; > + mtk_dp_fec_enable(mtk_dp, mtk_dp- > >has_fec); > + } else if (ret != -EAGAIN) { > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_DPIDLE; > + } > + break; > + case MTK_DP_TRAIN_STATE_NORMAL: > + mtk_dp_state_handler(mtk_dp); > + training_done = true; > + break; > + case MTK_DP_TRAIN_STATE_DPIDLE: When would this case happen? Regards, CK > + break; > + default: > + break; > + } > + > + if (ret) { > + if (ret == -EAGAIN) > + continue; > + /* > + * If we get any other error number, it doesn't > + * make any sense to keep iterating. > + */ > + break; > + } > + } while (!training_done || --max_retry); > + > + return ret; > +}
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > 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> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static int mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > +{ > + ssize_t ret; > + u8 sink_count; > + bool locked; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > + u32 link_status_reg = DP_LANE0_1_STATUS; > + > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, > &sink_count); > + if (ret < 0) { > + drm_err(mtk_dp->drm_dev, "Read sink count failed: > %ld\n", ret); > + return ret; > + } > + > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > link_status, > + sizeof(link_status)); > + if (!ret) { > + drm_err(mtk_dp->drm_dev, "Read link status failed: > %ld\n", > + ret); > + return ret; > + } > + > + locked = drm_dp_channel_eq_ok(link_status, > + mtk_dp->train_info.lane_count); > + if (!locked && mtk_dp->train_state > > MTK_DP_TRAIN_STATE_TRAINING_PRE) Before enter this function, mtk_dp->train_state is set to MTK_DP_TRAIN_STATE_STARTUP, so this never happen, drop this. > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE; > + > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > + drm_dp_dpcd_writeb(&mtk_dp->aux, > DP_DEVICE_SERVICE_IRQ_VECTOR, > + DP_REMOTE_CONTROL_COMMAND_PENDING); > + > + if (DP_GET_SINK_COUNT(sink_count) && > + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID; Why change state from MTK_DP_TRAIN_STATE_STARTUP to MTK_DP_TRAIN_STATE_CHECKEDID? In mtk_dp_train_handler(), mtk_dp_parse_capabilities() is true then change to MTK_DP_TRAIN_STATE_CHECKEDID. Give a reason why these two are different. Regards, CK > + msleep(20); > + } > + > + return 0; > +} > +
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > 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> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > +{ > + struct mtk_dp *mtk_dp = dev; > + int event; > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > + > + event = mtk_dp_plug_state(mtk_dp) ? connector_status_connected > : > + connector_status_disc > onnected; > + > + if (event < 0) > + return IRQ_HANDLED; > + > + if (mtk_dp->drm_dev) { > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > + } > + > + if (mtk_dp->train_info.cable_state_change) { > + mtk_dp->train_info.cable_state_change = false; > + > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + > + if (!mtk_dp->train_info.cable_plugged_in || > + !mtk_dp_plug_state(mtk_dp)) { > + mtk_dp_video_mute(mtk_dp, true); For eDP, when does 'unplug' happen? Explain this or move unplug processing to DP patch. Regards, CK > + > + mtk_dp_initialize_priv_data(mtk_dp); > + mtk_dp_set_idle_pattern(mtk_dp, true); > + if (mtk_dp->has_fec) > + mtk_dp_fec_enable(mtk_dp, false); > + > + mtk_dp_update_bits(mtk_dp, > MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL, > + DP_PWR_STATE_MASK); > + } else { > + mtk_dp_update_bits(mtk_dp, > MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LA > NE, > + DP_PWR_STATE_MASK); > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + mtk_dp->train_info.link_rate = > + min_t(int, mtk_dp->max_linkrate, > + buf[mtk_dp->max_linkrate]); > + mtk_dp->train_info.lane_count = > + min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(buf)); > + } > + } > + > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > + mtk_dp_hpd_sink_event(mtk_dp); > + } > + > + return IRQ_HANDLED; > +} > +
On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > It supports the mt8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > 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> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > > +{ > > + struct mtk_dp *mtk_dp = dev; > > + int event; > > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > + > > + event = mtk_dp_plug_state(mtk_dp) ? connector_status_connected > > : > > + connector_status_disc > > onnected; > > + > > + if (event < 0) > > event is always > 0, isn't it? > Hello CK, ok, I will move this to dp patch. > > + return IRQ_HANDLED; > > + > > + if (mtk_dp->drm_dev) { > > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > I think this ISR would come once. If bridge has not attached, the drm > core would lost this event. Maybe you should enable eDP hardware > after > bridge attached or send this event when attached. > for edp patch, I will move it to (mtk_dp_bridge_attach). for dp patch, I will add it back. > > + } > > + > > + if (mtk_dp->train_info.cable_state_change) { > > Executing this thread imply cable_state_change = true, so drop > cable_state_change. > In mtk_dp_hpd_isr_handler(), there is another irq "MTK_DP_HPD_INTERRUPT" which means the sink devices give a interrupt to source device. it's not about connected status, so I think we still need this. > > + mtk_dp->train_info.cable_state_change = false; > > + > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > + > > + if (!mtk_dp->train_info.cable_plugged_in || > > + !mtk_dp_plug_state(mtk_dp)) { > > I do not like two variable to present one thing. If > > mtk_dp->train_info.cable_plugged_in = false > and > mtk_dp_plug_state(mtk_dp) = ture > > What does this mean? I think this mean 'now' is connected because > cable_plugged_in is old information and mtk_dp_plug_state() is > current > information. > > But I would like to keep cable_plugged_in and drop > mtk_dp_plug_state() > because cable_plugged_in would be changed in isr and it would be the > same as mtk_dp_plug_state(). > > Regards, > CK > ok, I will drop this. BRs, Rex > > + mtk_dp_video_mute(mtk_dp, true); > > + > > + mtk_dp_initialize_priv_data(mtk_dp); > > + mtk_dp_set_idle_pattern(mtk_dp, true); > > + if (mtk_dp->has_fec) > > + mtk_dp_fec_enable(mtk_dp, false); > > + > > + mtk_dp_update_bits(mtk_dp, > > MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL, > > + DP_PWR_STATE_MASK); > > + } else { > > + mtk_dp_update_bits(mtk_dp, > > MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL_LA > > NE, > > + DP_PWR_STATE_MASK); > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > + mtk_dp->train_info.link_rate = > > + min_t(int, mtk_dp->max_linkrate, > > + buf[mtk_dp->max_linkrate]); > > + mtk_dp->train_info.lane_count = > > + min_t(int, mtk_dp->max_lanes, > > + drm_dp_max_lane_count(buf)); > > + } > > + } > > + > > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > > + mtk_dp_hpd_sink_event(mtk_dp); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > >
On Tue, 2022-06-07 at 15:30 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > It supports the mt8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > 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> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux, > > + struct drm_dp_aux_msg *msg) > > +{ > > + struct mtk_dp *mtk_dp; > > + bool is_read; > > + u8 request; > > + size_t accessed_bytes = 0; > > + int ret = 0; > > + > > + mtk_dp = container_of(mtk_aux, struct mtk_dp, aux); > > + > > + if (!mtk_dp->train_info.cable_plugged_in || > > + mtk_dp->train_info.irq_status & MTK_DP_HPD_DISCONNECT) { > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP; > > Changing state here has no any effect, so drop this. > ok, I will drop it. > > + return -EAGAIN; > > + } > > + > > + switch (msg->request) { > > + case DP_AUX_I2C_MOT: > > + case DP_AUX_I2C_WRITE: > > + case DP_AUX_NATIVE_WRITE: > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE | DP_AUX_I2C_MOT: > > + request = msg->request & > > ~DP_AUX_I2C_WRITE_STATUS_UPDATE; > > + is_read = false; > > + break; > > + case DP_AUX_I2C_READ: > > + case DP_AUX_NATIVE_READ: > > + case DP_AUX_I2C_READ | DP_AUX_I2C_MOT: > > + request = msg->request; > > + is_read = true; > > + break; > > + default: > > + drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n", > > + msg->request); > > + return -EINVAL; > > + } > > + > > + if (msg->size == 0) { > > + ret = mtk_dp_aux_do_transfer(mtk_dp, is_read, request, > > + msg->address + > > accessed_bytes, > > + msg->buffer + > > accessed_bytes, 0); > > + } else { > > + while (accessed_bytes < msg->size) { > > + size_t to_access = > > + min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES, > > + msg->size - accessed_bytes); > > + > > + ret = mtk_dp_aux_do_transfer(mtk_dp, > > + is_read, request, > > + msg->address + > > accessed_bytes, > > + msg->buffer + > > accessed_bytes, > > + to_access); > > + > > + if (ret) { > > + drm_info(mtk_dp->drm_dev, > > + "Failed to do AUX transfer: > > %d\n", ret); > > + break; > > + } > > + accessed_bytes += to_access; > > + } > > + } > > + > > + if (ret) { > > + msg->reply = DP_AUX_NATIVE_REPLY_NACK | > > DP_AUX_I2C_REPLY_NACK; > > + return ret; > > + } > > + > > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > > + return msg->size; > > +} > >
Hi, Rex: On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > > Hi, Rex: > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > > > It supports the mt8195, the embedded DisplayPort units. It offers > > > DisplayPort 1.4 with up to 4 lanes. > > > > > > The driver creates a child device for the phy. The child device > > > will > > > never exist without the parent being active. As they are sharing > > > a > > > register range, the parent passes a regmap pointer to the child > > > so > > > that > > > both can work with the same register range. The phy driver sets > > > device > > > data that is read by the parent to get the phy device that can be > > > used > > > to control the phy properties. > > > > > > 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> > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > --- > > > > [snip] > > > > > + > > > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > > > +{ > > > + struct mtk_dp *mtk_dp = dev; > > > + int event; > > > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > > + > > > + event = mtk_dp_plug_state(mtk_dp) ? connector_status_connected > > > : > > > + connector_status_disc > > > onnected; > > > + > > > + if (event < 0) > > > > event is always > 0, isn't it? > > > > Hello CK, > > ok, I will move this to dp patch. > > > > + return IRQ_HANDLED; > > > + > > > + if (mtk_dp->drm_dev) { > > > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > > > I think this ISR would come once. If bridge has not attached, the > > drm > > core would lost this event. Maybe you should enable eDP hardware > > after > > bridge attached or send this event when attached. > > > > for edp patch, I will move it to (mtk_dp_bridge_attach). > for dp patch, I will add it back. I find out that mtk_dp_poweron() is in top of mtk_dp_bridge_attach(). If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(), mtk_dp- >drm_dev would not be NULL here. So we could drop this checking. > > > > + } > > > + > > > + if (mtk_dp->train_info.cable_state_change) { > > > > Executing this thread imply cable_state_change = true, so drop > > cable_state_change. > > > > In mtk_dp_hpd_isr_handler(), there is another irq > "MTK_DP_HPD_INTERRUPT" which means the sink devices give a interrupt > to > source device. it's not about connected status, so I think we still > need this. In bottom of mtk_dp_hpd_isr_handler(), the code is: + train_info->cable_state_change = true; + + return IRQ_WAKE_THREAD; This thread is called only when return IRQ_WAKE_THREAD, and before return IRQ_WAKE_THREAD, train_info->cable_state_change is always set to true. So in this thread, train_info->cable_state_change must be true. Regards, CK > > > > + mtk_dp->train_info.cable_state_change = false; > > > + > > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > > + > > > + if (!mtk_dp->train_info.cable_plugged_in || > > > + !mtk_dp_plug_state(mtk_dp)) { > > > > I do not like two variable to present one thing. If > > > > mtk_dp->train_info.cable_plugged_in = false > > and > > mtk_dp_plug_state(mtk_dp) = ture > > > > What does this mean? I think this mean 'now' is connected because > > cable_plugged_in is old information and mtk_dp_plug_state() is > > current > > information. > > > > But I would like to keep cable_plugged_in and drop > > mtk_dp_plug_state() > > because cable_plugged_in would be changed in isr and it would be > > the > > same as mtk_dp_plug_state(). > > > > Regards, > > CK > > > > ok, I will drop this. > > BRs, > Rex > > > > + mtk_dp_video_mute(mtk_dp, true); > > > + > > > + mtk_dp_initialize_priv_data(mtk_dp); > > > + mtk_dp_set_idle_pattern(mtk_dp, true); > > > + if (mtk_dp->has_fec) > > > + mtk_dp_fec_enable(mtk_dp, false); > > > + > > > + mtk_dp_update_bits(mtk_dp, > > > MTK_DP_TOP_PWR_STATE, > > > + DP_PWR_STATE_BANDGAP_TPLL, > > > + DP_PWR_STATE_MASK); > > > + } else { > > > + mtk_dp_update_bits(mtk_dp, > > > MTK_DP_TOP_PWR_STATE, > > > + DP_PWR_STATE_BANDGAP_TPLL_LA > > > NE, > > > + DP_PWR_STATE_MASK); > > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > > + mtk_dp->train_info.link_rate = > > > + min_t(int, mtk_dp->max_linkrate, > > > + buf[mtk_dp->max_linkrate]); > > > + mtk_dp->train_info.lane_count = > > > + min_t(int, mtk_dp->max_lanes, > > > + drm_dp_max_lane_count(buf)); > > > + } > > > + } > > > + > > > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > > > + mtk_dp_hpd_sink_event(mtk_dp); > > > + } > > > + > > > + return IRQ_HANDLED; > > > +} > > > + > > > > > >
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > It supports the mt8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > 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> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static bool mtk_dp_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, int > lane_num, > + int swing_val, int > preemphasis) The return value is never processed, so let this function to be void. Regards, CK > +{ > + int ret; > + > + u32 lane_shift = lane_num * DP_TX1_VOLT_SWING_SHIFT; > + > + if (lane_num < 0 || lane_num > 3) lane_num < 0 would not happen. lane_num > 3 only if device tree max lane is wrong. So I would like to checkout max lane when parsing device tree instead of checking here. > + return false; > + > + dev_dbg(mtk_dp->dev, > + "link training swing_val= 0x%x, preemphasis = 0x%x\n", > + swing_val, preemphasis); > + > + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_SWING_EMP, > + swing_val << (DP_TX0_VOLT_SWING_SHIFT > + lane_shift), > + DP_TX0_VOLT_SWING_MASK << lane_shift); > + if (ret) > + return ret; > + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_SWING_EMP, > + preemphasis << (DP_TX0_PRE_EMPH_SHIFT > + lane_shift), > + DP_TX0_PRE_EMPH_MASK << lane_shift); > + > + return !ret; > +} > +
On Wed, 2022-06-08 at 17:15 +0800, CK Hu wrote: > Hi, Rex: > > On Wed, 2022-06-08 at 16:43 +0800, Rex-BC Chen wrote: > > On Wed, 2022-06-08 at 10:23 +0800, CK Hu wrote: > > > Hi, Rex: > > > > > > On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote: > > > > On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > > > > > Hi, Rex: > > > > > > > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > > > > > > > This patch adds a DisplayPort driver for the Mediatek > > > > > > mt8195 > > > > > > SoC. > > > > > > > > > > > > It supports the mt8195, the embedded DisplayPort units. It > > > > > > offers > > > > > > DisplayPort 1.4 with up to 4 lanes. > > > > > > > > > > > > The driver creates a child device for the phy. The child > > > > > > device > > > > > > will > > > > > > never exist without the parent being active. As they are > > > > > > sharing > > > > > > a > > > > > > register range, the parent passes a regmap pointer to the > > > > > > child > > > > > > so > > > > > > that > > > > > > both can work with the same register range. The phy driver > > > > > > sets > > > > > > device > > > > > > data that is read by the parent to get the phy device that > > > > > > can > > > > > > be > > > > > > used > > > > > > to control the phy properties. > > > > > > > > > > > > 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> > > > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > > > --- > > > > > > > > > > [snip] > > > > > > > > > > > + > > > > > > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void > > > > > > *dev) > > > > > > +{ > > > > > > + struct mtk_dp *mtk_dp = dev; > > > > > > + int event; > > > > > > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > > > > > + > > > > > > + event = mtk_dp_plug_state(mtk_dp) ? > > > > > > connector_status_connected > > > > > > : > > > > > > + connector_sta > > > > > > tus_disc > > > > > > onnected; > > > > > > + > > > > > > + if (event < 0) > > > > > > > > > > event is always > 0, isn't it? > > > > > > > > > > > > > Hello CK, > > > > > > > > ok, I will move this to dp patch. > > > > > > > > > > + return IRQ_HANDLED; > > > > > > + > > > > > > + if (mtk_dp->drm_dev) { > > > > > > + dev_info(mtk_dp->dev, > > > > > > "drm_helper_hpd_irq_event\n"); > > > > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > > > > > > > > > I think this ISR would come once. If bridge has not attached, > > > > > the > > > > > drm > > > > > core would lost this event. Maybe you should enable eDP > > > > > hardware > > > > > after > > > > > bridge attached or send this event when attached. > > > > > > > > > > > > > for edp patch, I will move it to (mtk_dp_bridge_attach). > > > > for dp patch, I will add it back. > > > > > > I find out that mtk_dp_poweron() is in top of > > > mtk_dp_bridge_attach(). > > > If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(), > > > mtk_dp- > > > > drm_dev would not be NULL here. So we could drop this checking. > > > > > > > > Hello CK, > > > > If we failed to setup phy(ret!=0), we alos need to deattach this > > bridge. > > I don't think it's a good idea just for remove this. > > OK, move mtk_dp_hwirq_enable() out of mtk_dp_poweron() and to the > bottom of mtk_dp_bridge_attach(). irq is not part of power. > I will do this and drop "if (mtk_dp->drm_dev)" > > > > > > > > + } > > > > > > + > > > > > > + if (mtk_dp->train_info.cable_state_change) { > > > > > > > > > > Executing this thread imply cable_state_change = true, so > > > > > drop > > > > > cable_state_change. > > > > > > > > > > > > > In mtk_dp_hpd_isr_handler(), there is another irq > > > > "MTK_DP_HPD_INTERRUPT" which means the sink devices give a > > > > interrupt > > > > to > > > > source device. it's not about connected status, so I think we > > > > still > > > > need this. > > > > > > In bottom of mtk_dp_hpd_isr_handler(), the code is: > > > > > > + train_info->cable_state_change = true; > > > + > > > + return IRQ_WAKE_THREAD; > > > > > > This thread is called only when return IRQ_WAKE_THREAD, and > > > before > > > return IRQ_WAKE_THREAD, train_info->cable_state_change is always > > > set > > > to > > > true. So in this thread, train_info->cable_state_change must be > > > true. > > > > > > > As mentioned, this irq handler function is not only for connected > > status. > > > > this could be return if this irq is interrupt from sink device. > > + if (!(train_info->irq_status & > > + (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT))) > > + return IRQ_HANDLED; > > According to [1], return IRQ_WAKE_THREAD to wake up thread. So return > IRQ_HANDLED would not wake up thread. > > [1] > https://www.kernel.org/doc/htmldocs/kernel-api/API-request-threaded-irq.html > > Regards, > CK > yes, you are right. I will return IRQ_WAKE_THREAD for handle sink interrupt. > > > > BRs, > > Bo-Chen > > > Regards, > > > CK > > > > > > > > > > > > > + mtk_dp->train_info.cable_state_change = false; > > > > > > + > > > > > > + mtk_dp->train_state = > > > > > > MTK_DP_TRAIN_STATE_STARTUP; > > > > > > + > > > > > > + if (!mtk_dp->train_info.cable_plugged_in || > > > > > > + !mtk_dp_plug_state(mtk_dp)) { > > > > > > > > > > I do not like two variable to present one thing. If > > > > > > > > > > mtk_dp->train_info.cable_plugged_in = false > > > > > and > > > > > mtk_dp_plug_state(mtk_dp) = ture > > > > > > > > > > What does this mean? I think this mean 'now' is connected > > > > > because > > > > > cable_plugged_in is old information and mtk_dp_plug_state() > > > > > is > > > > > current > > > > > information. > > > > > > > > > > But I would like to keep cable_plugged_in and drop > > > > > mtk_dp_plug_state() > > > > > because cable_plugged_in would be changed in isr and it would > > > > > be > > > > > the > > > > > same as mtk_dp_plug_state(). > > > > > > > > > > Regards, > > > > > CK > > > > > > > > > > > > > ok, I will drop this. > > > > > > > > BRs, > > > > Rex > > > > > > > > > > + mtk_dp_video_mute(mtk_dp, true); > > > > > > + > > > > > > + mtk_dp_initialize_priv_data(mtk_dp); > > > > > > + mtk_dp_set_idle_pattern(mtk_dp, true); > > > > > > + if (mtk_dp->has_fec) > > > > > > + mtk_dp_fec_enable(mtk_dp, > > > > > > false); > > > > > > + > > > > > > + mtk_dp_update_bits(mtk_dp, > > > > > > MTK_DP_TOP_PWR_STATE, > > > > > > + DP_PWR_STATE_BANDGAP > > > > > > _TPLL, > > > > > > + DP_PWR_STATE_MASK); > > > > > > + } else { > > > > > > + mtk_dp_update_bits(mtk_dp, > > > > > > MTK_DP_TOP_PWR_STATE, > > > > > > + DP_PWR_STATE_BANDGAP > > > > > > _TPLL_LA > > > > > > NE, > > > > > > + DP_PWR_STATE_MASK); > > > > > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, > > > > > > buf); > > > > > > + mtk_dp->train_info.link_rate = > > > > > > + min_t(int, mtk_dp- > > > > > > > max_linkrate, > > > > > > > > > > > > + buf[mtk_dp- > > > > > > > max_linkrate]); > > > > > > > > > > > > + mtk_dp->train_info.lane_count = > > > > > > + min_t(int, mtk_dp->max_lanes, > > > > > > + drm_dp_max_lane_count(buf > > > > > > )); > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + if (mtk_dp->train_info.irq_status & > > > > > > MTK_DP_HPD_INTERRUPT) { > > > > > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > > > > > + mtk_dp->train_info.irq_status &= > > > > > > ~MTK_DP_HPD_INTERRUPT; > > > > > > + mtk_dp_hpd_sink_event(mtk_dp); > > > > > > + } > > > > > > + > > > > > > + return IRQ_HANDLED; > > > > > > +} > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On 23-05-22, 12:47, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This is a new driver that supports the integrated DisplayPort phy for > mediatek SoCs, especially the mt8195. The phy is integrated into the > DisplayPort controller and will be created by the mtk-dp driver. This > driver expects a struct regmap to be able to work on the same registers > as the DisplayPort controller. It sets the device data to be the struct > phy so that the DisplayPort controller can easily work with it. > > The driver does not have any devicetree bindings because the datasheet > does not list the controller and the phy as distinct units. > > The interaction with the controller can be covered by the configure > callback of the phy framework and its displayport parameters. I must admit that I have missed previous iteration of this driver. This is a standalone phy driver, pls split and submit this and we can get this merged... > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > MAINTAINERS | 1 + > drivers/phy/mediatek/Kconfig | 8 ++ > drivers/phy/mediatek/Makefile | 1 + > drivers/phy/mediatek/phy-mtk-dp.c | 200 ++++++++++++++++++++++++++++++ > 4 files changed, 210 insertions(+) > create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4cc47b2dbdc9..bfca96469d80 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6604,6 +6604,7 @@ L: linux-mediatek@lists.infradead.org (moderated for non-subscribers) > S: Supported > F: Documentation/devicetree/bindings/display/mediatek/ > F: drivers/gpu/drm/mediatek/ > +F: drivers/phy/mediatek/phy-mtk-dp.c > F: drivers/phy/mediatek/phy-mtk-hdmi* > F: drivers/phy/mediatek/phy-mtk-mipi* > > diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig > index 55f8e6c048ab..f7ec86059049 100644 > --- a/drivers/phy/mediatek/Kconfig > +++ b/drivers/phy/mediatek/Kconfig > @@ -55,3 +55,11 @@ config PHY_MTK_MIPI_DSI > select GENERIC_PHY > help > Support MIPI DSI for Mediatek SoCs. > + > +config PHY_MTK_DP > + tristate "MediaTek DP-PHY Driver" > + depends on ARCH_MEDIATEK || COMPILE_TEST > + depends on OF > + select GENERIC_PHY > + help > + Support DisplayPort PHY for Mediatek SoCs. > diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile > index ace660fbed3a..4ba1e0650434 100644 > --- a/drivers/phy/mediatek/Makefile > +++ b/drivers/phy/mediatek/Makefile > @@ -3,6 +3,7 @@ > # Makefile for the phy drivers. > # > > +obj-$(CONFIG_PHY_MTK_DP) += phy-mtk-dp.o > obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o > obj-$(CONFIG_PHY_MTK_UFS) += phy-mtk-ufs.o > obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c > new file mode 100644 > index 000000000000..6f29854f0c2f > --- /dev/null > +++ b/drivers/phy/mediatek/phy-mtk-dp.c > @@ -0,0 +1,200 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MediaTek DisplayPort PHY driver > + * > + * Copyright (c) 2021 BayLibre 2022 now > + * Author: Markus Schneider-Pargmann <msp@baylibre.com> use MODULE_AUTHOR() > + */ > + > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define PHY_OFFSET 0x1000 > + > +#define MTK_DP_PHY_DIG_PLL_CTL_1 (PHY_OFFSET + 0x14) > +#define TPLL_SSC_EN BIT(3) > + > +#define MTK_DP_PHY_DIG_BIT_RATE (PHY_OFFSET + 0x3C) > +#define BIT_RATE_RBR 0 > +#define BIT_RATE_HBR 1 > +#define BIT_RATE_HBR2 2 > +#define BIT_RATE_HBR3 3 > + > +#define MTK_DP_PHY_DIG_SW_RST (PHY_OFFSET + 0x38) > +#define DP_GLB_SW_RST_PHYD BIT(0) > + > +#define MTK_DP_LANE0_DRIVING_PARAM_3 (PHY_OFFSET + 0x138) > +#define MTK_DP_LANE1_DRIVING_PARAM_3 (PHY_OFFSET + 0x238) > +#define MTK_DP_LANE2_DRIVING_PARAM_3 (PHY_OFFSET + 0x338) > +#define MTK_DP_LANE3_DRIVING_PARAM_3 (PHY_OFFSET + 0x438) > +#define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT BIT(4) > +#define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT ((BIT(2) | BIT(4)) << 8) Sound like BIT(10) and BIT (12), no? > +#define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT GENMASK(20, 19) > +#define XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT GENMASK(29, 29) > +#define DRIVING_PARAM_3_DEFAULT (XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT) > + > +#define XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT GENMASK(4, 3) > +#define XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT GENMASK(12, 9) > +#define XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT ((BIT(2) | BIT(5)) << 16) Here too > +#define XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT GENMASK(29, 29) > +#define DRIVING_PARAM_4_DEFAULT (XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT) > + > +#define XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT (BIT(3) | BIT(5)) > +#define XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT GENMASK(13, 12) > +#define DRIVING_PARAM_5_DEFAULT (XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT) > + > +#define XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT 0 > +#define XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT GENMASK(10, 10) > +#define XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT GENMASK(19, 19) > +#define XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT GENMASK(28, 28) > +#define DRIVING_PARAM_6_DEFAULT (XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT) > + > +#define XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT 0 > +#define XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT GENMASK(10, 9) > +#define XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT GENMASK(19, 18) > +#define XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT 0 > +#define DRIVING_PARAM_7_DEFAULT (XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT) > + > +#define XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT GENMASK(3, 3) > +#define XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT 0 > +#define DRIVING_PARAM_8_DEFAULT (XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT) > + > +struct mtk_dp_phy { > + struct regmap *regs; > +}; > + > +static int mtk_dp_phy_init(struct phy *phy) > +{ > + struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy); > + u32 driving_params[] = { > + DRIVING_PARAM_3_DEFAULT, > + DRIVING_PARAM_4_DEFAULT, > + DRIVING_PARAM_5_DEFAULT, > + DRIVING_PARAM_6_DEFAULT, > + DRIVING_PARAM_7_DEFAULT, > + DRIVING_PARAM_8_DEFAULT > + }; > + > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE0_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE1_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE2_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE3_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + > + return 0; > +} > + > +static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts) > +{ > + struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy); > + u32 val; > + > + if (opts->dp.set_rate) { > + switch (opts->dp.link_rate) { > + default: > + dev_err(&phy->dev, > + "Implementation error, unknown linkrate %x\n", > + opts->dp.link_rate); > + return -EINVAL; > + case 1620: > + val = BIT_RATE_RBR; > + break; > + case 2700: > + val = BIT_RATE_HBR; > + break; > + case 5400: > + val = BIT_RATE_HBR2; > + break; > + case 8100: > + val = BIT_RATE_HBR3; > + break; > + } > + regmap_write(dp_phy->regs, MTK_DP_PHY_DIG_BIT_RATE, val); > + } > + > + regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_PLL_CTL_1, > + TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0); > + > + return 0; > +} > + > +static int mtk_dp_phy_reset(struct phy *phy) > +{ > + struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy); > + > + regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_SW_RST, > + DP_GLB_SW_RST_PHYD, 0); > + usleep_range(50, 200); > + regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_SW_RST, > + DP_GLB_SW_RST_PHYD, 1); > + > + return 0; > +} > + > +static const struct phy_ops mtk_dp_phy_dev_ops = { > + .init = mtk_dp_phy_init, > + .configure = mtk_dp_phy_configure, > + .reset = mtk_dp_phy_reset, > + .owner = THIS_MODULE, > +}; > + > +static int mtk_dp_phy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_dp_phy *dp_phy; > + struct phy *phy; > + struct regmap *regs; > + > + regs = *(struct regmap **)dev->platform_data; > + if (!regs) > + return dev_err_probe(dev, EINVAL, "No data passed, requires struct regmap**\n"); > + > + dp_phy = devm_kzalloc(dev, sizeof(*dp_phy), GFP_KERNEL); > + if (!dp_phy) > + return -ENOMEM; > + > + dp_phy->regs = regs; > + phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops); > + if (IS_ERR(phy)) > + return dev_err_probe(dev, PTR_ERR(phy), "Failed to create DP PHY\n"); > + > + phy_set_drvdata(phy, dp_phy); > + if (!dev->of_node) > + phy_create_lookup(phy, "dp", dev_name(dev)); > + > + return 0; > +} > + > +struct platform_driver mtk_dp_phy_driver = { > + .probe = mtk_dp_phy_probe, > + .driver = { > + .name = "mediatek-dp-phy", > + }, > +}; > +module_platform_driver(mtk_dp_phy_driver); > + > +MODULE_AUTHOR("Markus Schneider-Pargmann <msp@baylibre.com>"); > +MODULE_DESCRIPTION("MediaTek DP PHY Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.35.1
On Tue, 2022-06-07 at 16:01 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > It supports the mt8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > 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> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static void mtk_dp_state_handler(struct mtk_dp *mtk_dp) > > +{ > > + switch (mtk_dp->state) { > > Does mtk_dp->state has any relation with mtk_dp->train_state. If yes, > mix mtk_dp->state and mtk_dp->train_state into one state. If no, move > calling mtk_dp_state_handler() out of mtk_dp_train_handler(). > > Regards, > CK > Hello CK, OK, I will refine this flow. About the state machine of traning flow, we can review in v11. BRs, Bo-Chen > > + case MTK_DP_STATE_INITIAL: > > + mtk_dp_video_mute(mtk_dp, true); > > + mtk_dp->state = MTK_DP_STATE_IDLE; > > + break; > > + > > + case MTK_DP_STATE_IDLE: > > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > > + mtk_dp->state = MTK_DP_STATE_PREPARE; > > + break; > > + > > + case MTK_DP_STATE_PREPARE: > > + mtk_dp_video_config(mtk_dp); > > + mtk_dp_video_enable(mtk_dp, true); > > + > > + mtk_dp->state = MTK_DP_STATE_NORMAL; > > + break; > > + > > + case MTK_DP_STATE_NORMAL: > > + if (mtk_dp->train_state != MTK_DP_TRAIN_STATE_NORMAL) { > > + mtk_dp_video_mute(mtk_dp, true); > > + mtk_dp->state = MTK_DP_STATE_IDLE; > > + } > > + break; > > + > > + default: > > + break; > > + } > > +} > >
On Tue, 2022-06-07 at 17:04 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC. > > > > It supports the mt8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > 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> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > > +{ > > + struct mtk_dp *mtk_dp = dev; > > + int event; > > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > + > > + event = mtk_dp_plug_state(mtk_dp) ? connector_status_connected > > : > > + connector_status_disc > > onnected; > > + > > + if (event < 0) > > + return IRQ_HANDLED; > > + > > + if (mtk_dp->drm_dev) { > > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > + } > > + > > + if (mtk_dp->train_info.cable_state_change) { > > + mtk_dp->train_info.cable_state_change = false; > > + > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > + > > + if (!mtk_dp->train_info.cable_plugged_in || > > + !mtk_dp_plug_state(mtk_dp)) { > > + mtk_dp_video_mute(mtk_dp, true); > > For eDP, when does 'unplug' happen? Explain this or move unplug > processing to DP patch. > > Regards, > CK > Hello CK, ok, I move them to dp patch. BRs, Bo-Chen > > + > > + mtk_dp_initialize_priv_data(mtk_dp); > > + mtk_dp_set_idle_pattern(mtk_dp, true); > > + if (mtk_dp->has_fec) > > + mtk_dp_fec_enable(mtk_dp, false); > > + > > + mtk_dp_update_bits(mtk_dp, > > MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL, > > + DP_PWR_STATE_MASK); > > + } else { > > + mtk_dp_update_bits(mtk_dp, > > MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL_LA > > NE, > > + DP_PWR_STATE_MASK); > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > + mtk_dp->train_info.link_rate = > > + min_t(int, mtk_dp->max_linkrate, > > + buf[mtk_dp->max_linkrate]); > > + mtk_dp->train_info.lane_count = > > + min_t(int, mtk_dp->max_lanes, > > + drm_dp_max_lane_count(buf)); > > + } > > + } > > + > > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > > + mtk_dp_hpd_sink_event(mtk_dp); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > >