On Mon, 2025-02-17 at 16:48 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
>
>
> Add support for the newer HDMI-TX (Encoder) v2 and DDC v2 IPs
> found in MediaTek's MT8195, MT8188 SoC and their variants, and
> including support for display modes up to 4k60 and for HDMI
> Audio, as per the HDMI 2.0 spec.
>
> HDCP and CEC functionalities are also supported by this hardware,
> but are not included in this commit and that also poses a slight
> difference between the V2 and V1 controllers in how they handle
> Hotplug Detection (HPD).
>
> While the v1 controller was using the CEC controller to check
> HDMI cable connection and disconnection, in this driver the v2
> one does not.
>
> This is due to the fact that on parts with v2 designs, like the
> MT8195 SoC, there is one CEC controller shared between the HDMI
> Transmitter (HDMI-TX) and Receiver (HDMI-RX): before eventually
> adding support to use the CEC HW to wake up the HDMI controllers
> it is necessary to have support for one TX, one RX *and* for both
> at the same time.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> ---
[snip]
> +
> +static inline void mtk_hdmi_v2_hw_reset_av_mute_regs(struct mtk_hdmi *hdmi)
> +{
This function is called only by mtk_hdmi_v2_hw_gcp_avmute(), so merge it into mtk_hdmi_v2_hw_gcp_avmute().
Your point is that function name would show more information about this register setting.
I think you could merge this function and use comment to describe about this register setting.
Function name just show few information. If you want to show more things, comment is better.
If you worry that comment is too many, the comment may be as simple as function name.
It's better to use comment to describe register usage than break them into small pieces functions.
> + /* GCP packet */
> + regmap_clear_bits(hdmi->regs, TOP_CFG01, CP_CLR_MUTE_EN | CP_SET_MUTE_EN);
> + regmap_clear_bits(hdmi->regs, TOP_INFO_RPT, CP_RPT_EN);
> + regmap_clear_bits(hdmi->regs, TOP_INFO_EN, CP_EN | CP_EN_WR);
> +}
> +
> +static inline void mtk_hdmi_v2_hw_gcp_avmute(struct mtk_hdmi *hdmi, bool mute)
> +{
> + mtk_hdmi_v2_hw_reset_av_mute_regs(hdmi);
> +
> + if (mute)
> + regmap_set_bits(hdmi->regs, TOP_CFG01, CP_SET_MUTE_EN);
> + else
> + regmap_set_bits(hdmi->regs, TOP_CFG01, CP_CLR_MUTE_EN);
> +
> + regmap_set_bits(hdmi->regs, TOP_INFO_RPT, CP_RPT_EN);
> + regmap_set_bits(hdmi->regs, TOP_INFO_EN, CP_EN | CP_EN_WR);
> +}
> +
[snip]
> +
> +static void mtk_hdmi_v2_change_video_resolution(struct mtk_hdmi *hdmi)
> +{
> + mtk_hdmi_v2_hw_reset(hdmi);
> + mtk_hdmi_v2_set_sw_hpd(hdmi, true);
> + udelay(2);
> +
> + regmap_write(hdmi->regs, HDCP_TOP_CTRL, 0);
> +
> + /* Enable HDCP reauthentication interrupt */
> + regmap_set_bits(hdmi->regs, TOP_INT_ENABLE00, HDCP2X_RX_REAUTH_REQ_DDCM_INT);
You have mentioned that hardware would process this interrupt internally so we need this interrupt.
Add comment for this because this is not trivial.
> +
> + /* Enable hotplug and pord interrupts */
> + mtk_hdmi_v2_enable_hpd_pord_irq(hdmi, true);
> +
> + /* Force enabling HDCP HPD */
> + regmap_set_bits(hdmi->regs, HDCP2X_CTRL_0, HDCP2X_HPD_OVR);
> + regmap_set_bits(hdmi->regs, HDCP2X_CTRL_0, HDCP2X_HPD_SW);
> +
> + /* Set 8 bits per pixel */
> + regmap_update_bits(hdmi->regs, TOP_CFG00, TMDS_PACK_MODE,
> + FIELD_PREP(TMDS_PACK_MODE, TMDS_PACK_MODE_8BPP));
> + /* Disable generating deepcolor packets */
> + regmap_clear_bits(hdmi->regs, TOP_CFG00, DEEPCOLOR_PKT_EN);
> + /* Disable adding deepcolor information to the general packet */
> + regmap_clear_bits(hdmi->regs, TOP_MISC_CTLR, DEEP_COLOR_ADD);
> +
> + if (hdmi->curr_conn->display_info.is_hdmi)
> + regmap_set_bits(hdmi->regs, TOP_CFG00, HDMI_MODE_HDMI);
> + else
> + regmap_clear_bits(hdmi->regs, TOP_CFG00, HDMI_MODE_HDMI);
> +
> + udelay(5);
> + mtk_hdmi_v2_hw_vid_mute(hdmi, true);
> + mtk_hdmi_v2_hw_aud_mute(hdmi, true);
> + mtk_hdmi_v2_hw_gcp_avmute(hdmi, false);
> +
> + regmap_update_bits(hdmi->regs, TOP_CFG01,
> + NULL_PKT_VSYNC_HIGH_EN | NULL_PKT_EN, NULL_PKT_VSYNC_HIGH_EN);
> + usleep_range(100, 150);
> +
> + /* Enable scrambling if tmds clock is 340MHz or more */
> + mtk_hdmi_v2_enable_scrambling(hdmi, hdmi->mode.clock >= 340 * KILO);
> +
> + /* Disable YUV420 downsampling */
> + mtk_hdmi_yuv420_downsampling(hdmi, false);
> +}
> +
[snip]
> +
> +static void mtk_hdmi_hpd_event(enum hdmi_hpd_state hpd, struct device *dev)
> +{
> + struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
> +
> + if (hdmi && hdmi->bridge.encoder && hdmi->bridge.encoder->dev)
> + drm_helper_hpd_irq_event(hdmi->bridge.encoder->dev);
This function just do one thing and is called only by isr thread, so merge this function into isr thread.
> +}
> +
> +static enum hdmi_hpd_state mtk_hdmi_v2_hpd_pord_status(struct mtk_hdmi *hdmi)
> +{
> + u8 hpd_pin_sta, pord_pin_sta;
> + u32 hpd_status;
> +
> + regmap_read(hdmi->regs, HPD_DDC_STATUS, &hpd_status);
> + hpd_pin_sta = FIELD_GET(HPD_PIN_STA, hpd_status);
> + pord_pin_sta = FIELD_GET(PORD_PIN_STA, hpd_status);
> +
> + if (hpd_pin_sta && pord_pin_sta)
> + return HDMI_PLUG_IN_AND_SINK_POWER_ON;
> + else if (hpd_pin_sta)
> + return HDMI_PLUG_IN_ONLY;
> + else
> + return HDMI_PLUG_OUT;
In v1, cec driver treat '(hpd_pin_sta && pord_pin_sta) is true' as plug in, otherwise plug out.
It seems that v1 could support 3 status but it choose 2 status and make external connector complicated?
Add comment about the benefit for external connector.
> +}
> +
> +static irqreturn_t mtk_hdmi_v2_isr(int irq, void *arg)
> +{
> + struct mtk_hdmi *hdmi = arg;
> + unsigned int irq_sta;
> + int ret = IRQ_HANDLED;
> +
> + regmap_read(hdmi->regs, TOP_INT_STA00, &irq_sta);
> +
> + /* Handle Hotplug Detection interrupt */
> + if (irq_sta & (HTPLG_R_INT | HTPLG_F_INT | PORD_F_INT | PORD_R_INT)) {
if (irq_sta & HPD_PORD_HW_IRQS) {
> + mtk_hdmi_v2_enable_hpd_pord_irq(hdmi, false);
The reason you dynamically enable/disable hpd irq is to prevent isr thread to be interrupted.
But I think the isr thread could be interrupted and nothing would be error.
If the isr thread is resheduled, then it just read the latest plug status again and report it to drm core.
I don't know why you don't want reschedule?
> + ret = IRQ_WAKE_THREAD;
> + }
> +
> + /*
> + * Clear all 32 + 19 interrupts in CLR00 and CLR01: this is important
> + * to avoid unwanted retriggering of any interrupts
> + */
If you does not enable interrupt in TOP_INT_ENABLE01 (I mean software disable these interrupt),
and hardware would still trigger these interrupt, add comment to describe this weird hardware behavior.
I will treat this weird behavior as hardware bug, and expect it would be fixed in next SoC.
Regards,
CK
> + regmap_write(hdmi->regs, TOP_INT_CLR00, GENMASK(31, 0));
> + regmap_write(hdmi->regs, TOP_INT_CLR01, GENMASK(18, 0));
> +
> + /* Restore interrupt clearing registers to zero */
> + regmap_write(hdmi->regs, TOP_INT_CLR00, 0);
> + regmap_write(hdmi->regs, TOP_INT_CLR01, 0);
> +
> + return ret;
> +}
> +
>
