Il 28/03/22 00:39, Guillaume Ranquet ha scritto:
> From: Markus Schneider-Pargmann <[email protected]>
>
> 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 shares its iomap range with the mtk-dp-phy driver using
> the regmap/syscon facility.
>
> This driver is based on an initial version by
> Jason-JH.Lin <[email protected]>.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> Signed-off-by: Guillaume Ranquet <[email protected]>
> Reported-by: kernel test robot <[email protected]>
Hello Guillaume,
as you know, there's some more work to do on this driver.
I will also mention here, not on the code, that at this point, your
mtk_dp_aux_transfer() function has something VERY similar to function
drm_dp_dpcd_access(), so I really believe that you can instead use
functions drm_dp_dpcd_read() and drm_dp_dpcd_write(), avoiding code
duplication around.
Please check drivers/gpu/drm/dp/drm_dp.c.
> ---
> drivers/gpu/drm/mediatek/Kconfig | 8 +
> drivers/gpu/drm/mediatek/Makefile | 2 +
> drivers/gpu/drm/mediatek/mtk_dp.c | 2221 ++++++++++++++++++++++++
> drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 +
> 6 files changed, 2801 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..03ffa9b896c3 100644
> --- a/drivers/gpu/drm/mediatek/Kconfig
> +++ b/drivers/gpu/drm/mediatek/Kconfig
> @@ -28,3 +28,11 @@ config DRM_MEDIATEK_HDMI
> select PHY_MTK_HDMI
> help
> DRM/KMS HDMI driver for Mediatek SoCs
> +
> +config MTK_DPTX_SUPPORT
Actually, I think that the best would be DRM_MEDIATEK_DP_TX or DRM_MEDIATEK_DP...
...also, ordering is important, please!
> + tristate "DRM DPTX Support for Mediatek SoCs"
> + depends on DRM_MEDIATEK
> + select PHY_MTK_DP
> + select DRM_DP_HELPER
> + help
> + DRM/KMS Display Port driver for Mediatek SoCs.
> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
> index 29098d7c8307..d86a6406055e 100644
> --- a/drivers/gpu/drm/mediatek/Makefile
> +++ b/drivers/gpu/drm/mediatek/Makefile
> @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
> mtk_hdmi_ddc.o
>
> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
> +
> +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> new file mode 100644
> index 000000000000..7cd8459cf719
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -0,0 +1,2221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + * Copyright (c) 2021 BayLibre
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/dp/drm_dp_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <sound/hdmi-codec.h>
> +#include <video/videomode.h>
> +
> +#include "mtk_dp_reg.h"
> +
> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> +
> +//TODO: platform/device data or dts?
Assuming that your TODO is about the maximum number of lanes,
{ .compatible = "mediatek,mt8195-edp-tx", .data = &some_const_structure },
> +#define MTK_DP_MAX_LANES 4
> +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3
..snip..
> +
> +static int mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 offset, u8 *buf,
> + size_t length)
> +{
> + int i;
> + int ret = 0;
> + int num_regs = (length + 1) / 2;
> +
> + /* 2 bytes per register */
> + for (i = 0; i < num_regs; i++) {
> + u32 val = buf[i * 2] |
> + (i * 2 + 1 < length ? buf[i * 2 + 1] << 8 : 0);
> +
> + ret = mtk_dp_write(mtk_dp, offset + i * 4, val);
> + if (ret)
> + goto out;
if (ret)
return ret;
> + }
> +
> +out:
Remove this label.
> + return ret;
> +}
> +
> +static unsigned long mtk_dp_sip_atf_call(unsigned int cmd, unsigned int para)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(MTK_DP_SIP_CONTROL_AARCH32, cmd, para, 0, 0, 0, 0, 0,
> + &res);
> +
> + pr_debug("[DPTX]%s cmd 0x%x, p1 0x%x, ret 0x%lx-0x%lx", __func__, cmd,
> + para, res.a0, res.a1);
> + return res.a1;
> +}
> +
> +static int mtk_dp_msa_bypass_disable(struct mtk_dp *mtk_dp)
> +{
> + const u16 bits_to_set =
> + BIT(HTOTAL_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(VTOTAL_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(HSTART_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(VSTART_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(HWIDTH_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(VHEIGHT_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(HSP_SEL_DP_ENC0_P0_SHIFT) | BIT(HSW_SEL_DP_ENC0_P0_SHIFT) |
> + BIT(VSP_SEL_DP_ENC0_P0_SHIFT) | BIT(VSW_SEL_DP_ENC0_P0_SHIFT);
> + return mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3030, bits_to_set,
> + bits_to_set);
> +}
> +
> +#define MTK_UPD_BITS_OR_OUT(mtk_dp, offset, val, mask, ret, label) \
> + do {\
> + ret = mtk_dp_update_bits(mtk_dp, offset, val, mask); \
> + if (ret) \
> + goto label; \
> + } while (0)
I'm sorry, no offense - but this macro is a bit ugly...
I think I understand why you have introduced it, but in my opinion this decreases
human readability a lot, I was even about to point out multiple functions that
you had unused labels before checking this macro, as that was totally unexpected...
In my opinion, this should be open-coded everywhere... yes it makes the file a
bit fatter in terms of amount of text, but eh... it's life :)))
> +
....snip....
> +
> +static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp,
> + enum mtk_dp_color_format color_format)
> +{
> + u32 val;
> + int ret;
> +
> + mtk_dp->info.format = color_format;
> +
> + /* Update MISC0 */
> + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034,
> + color_format << DP_TEST_COLOR_FORMAT_SHIFT,
> + DP_TEST_COLOR_FORMAT_MASK);
> +
> + if (ret)
> + return ret;
> +
> + switch (color_format) {
> + case MTK_DP_COLOR_FORMAT_YUV_422:
> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422;
> + break;
> + case MTK_DP_COLOR_FORMAT_YUV_420:
> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420;
> + break;
> + case MTK_DP_COLOR_FORMAT_YONLY:
> + case MTK_DP_COLOR_FORMAT_RAW:
> + case MTK_DP_COLOR_FORMAT_RESERVED:
> + case MTK_DP_COLOR_FORMAT_UNKNOWN:
> + drm_warn(mtk_dp->drm_dev, "Unsupported color format: %d\n",
> + color_format);
> + fallthrough;
Uhm, are you sure that you should fallthrough?
If we get a color format that we don't understand, the best thing that would
happen is that we'd get scrambled colors... which is something unwanted.
The best idea is to return -EINVAL here, hence refuse to go on with an invalid
color format.
> + case MTK_DP_COLOR_FORMAT_RGB_444:
> + case MTK_DP_COLOR_FORMAT_YUV_444:
> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB;
> + break;
> + }
> +
> + return mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val,
> + PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK);
> +}
> +
...snip...
> +
> +static void mtk_dp_initialize_hpd_detect_settings(struct mtk_dp *mtk_dp)
> +{
> + // Debounce threshold
I think I already told you to fix comments style.
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
> + 8 << HPD_DEB_THD_DP_TRANS_P0_SHIFT,
> + HPD_DEB_THD_DP_TRANS_P0_MASK);
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
> + (HPD_INT_THD_DP_TRANS_P0_LOWER_500US |
> + HPD_INT_THD_DP_TRANS_P0_UPPER_1100US)
> + << HPD_INT_THD_DP_TRANS_P0_SHIFT,
> + HPD_INT_THD_DP_TRANS_P0_MASK);
> +
> + // Connect threshold 1.5ms + 5 x 0.1ms = 2ms
> + // Disconnect threshold 1.5ms + 5 x 0.1ms = 2ms
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
> + (5 << HPD_DISC_THD_DP_TRANS_P0_SHIFT) |
> + (5 << HPD_CONN_THD_DP_TRANS_P0_SHIFT),
> + HPD_DISC_THD_DP_TRANS_P0_MASK |
> + HPD_CONN_THD_DP_TRANS_P0_MASK);
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3430,
> + HPD_INT_THD_ECO_DP_TRANS_P0_HIGH_BOUND_EXT,
> + HPD_INT_THD_ECO_DP_TRANS_P0_MASK);
> +}
> +
..snip..
> +
> +static int mtk_dp_power_disable(struct mtk_dp *mtk_dp)
> +{
> + int ret;
> +
> + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0);
> +
> + if (ret)
> + goto out;
> +
> + usleep_range(10, 200);
> + ret = mtk_dp_write(mtk_dp, MTK_DP_0034,
> + DA_CKM_CKTX0_EN_FORCE_EN | DA_CKM_BIAS_LPF_EN_FORCE_VAL |
> + DA_CKM_BIAS_EN_FORCE_VAL |
> + DA_XTP_GLB_LDO_EN_FORCE_VAL |
> + DA_XTP_GLB_AVD10_ON_FORCE_VAL);
> +
> + if (ret)
> + goto out;
> +
> + /* Disable RX */
> + ret = mtk_dp_write(mtk_dp, MTK_DP_1040, 0);
> +
> + if (ret)
> + goto out;
> +
> + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD,
> + 0x550 | BIT(FUSE_SEL_SHIFT) | BIT(MEM_ISO_EN_SHIFT));
> +
> +out:
You are using a goto instead of a return, but you're not reverting any change,
so this goto usage is not justified - hence, wrong.
Please simply return ret when anything fails, and return 0 at the end of this
function.
.......
(blahblah function code)
ret = something()
if (ret)
return ret;
return 0;
}
> + return ret;
> +}
> +
...snip...
> +
> +static int mtk_dp_dt_parse(struct mtk_dp *mtk_dp,
> + struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int ret = 0;
> + void __iomem *base;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + mtk_dp->regs = syscon_node_to_regmap(dev->of_node);
> + if (IS_ERR(mtk_dp->regs))
> + return PTR_ERR(mtk_dp->regs);
> +
> + //TODO: optional clock?
Well, if it's optional, you should use devm_clk_get_optional(), meaning
that......
> + mtk_dp->dp_tx_clk = devm_clk_get(dev, "faxi");
> + if (IS_ERR(mtk_dp->dp_tx_clk)) {
> + ret = PTR_ERR(mtk_dp->dp_tx_clk);
> + dev_info(dev, "Failed to get dptx clock: %d\n", ret);
> + mtk_dp->dp_tx_clk = NULL;
...I shouldn't see this snippet of code, as it should be a bit different... :)
> + }
> +
> + return 0;
> +}
> +
..snip..
> +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_state)
> +{
> + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> +
> + mtk_dp_video_mute(mtk_dp, true);
> + mtk_dp->state = MTK_DP_STATE_IDLE;
> + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
> +
> + mtk_dp->enabled = false;
> + msleep(100);
100 milliseconds is an eternity, why are we sleeping for *so long* here?
Please, either add a comment that fully explains the resons for that, or
remove the sleep entirely: to my eyes, it doesn't look like this sleep is
really needed for anything important because here you are disabling the
bridge and *powering off* the IP entirely.
> + mtk_dp_poweroff(mtk_dp);
> +}
> +
..snip..
> +
> +static enum drm_mode_status
> +mtk_dp_bridge_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_info *info,
> + const struct drm_display_mode *mode)
> +{
> + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> + u32 rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000;
> + u32 bpp = info->color_formats & DRM_COLOR_FORMAT_YCBCR422 ? 16 : 24;
> +
> + if (rx_linkrate * mtk_dp->train_info.lane_count < mode->clock * bpp / 8)
> + return MODE_CLOCK_HIGH;
> +
> + if (mode->clock > 600000)
> + return MODE_CLOCK_HIGH;
> +
> + //TODO: explain magic number: 62?
Please fix this TODO for v10.
> + if ((mode->clock * 1000) / (mode->htotal * mode->vtotal) > 62)
> + return MODE_CLOCK_HIGH;
> +
> + return MODE_OK;
> +}
> +
On Mon, 28 Mar 2022 11:14, AngeloGioacchino Del Regno
<[email protected]> wrote:
>Il 28/03/22 00:39, Guillaume Ranquet ha scritto:
>> From: Markus Schneider-Pargmann <[email protected]>
>>
>> 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 shares its iomap range with the mtk-dp-phy driver using
>> the regmap/syscon facility.
>>
>> This driver is based on an initial version by
>> Jason-JH.Lin <[email protected]>.
>>
>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>> Signed-off-by: Guillaume Ranquet <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>
>Hello Guillaume,
>as you know, there's some more work to do on this driver.
>
>I will also mention here, not on the code, that at this point, your
>mtk_dp_aux_transfer() function has something VERY similar to function
>drm_dp_dpcd_access(), so I really believe that you can instead use
>functions drm_dp_dpcd_read() and drm_dp_dpcd_write(), avoiding code
>duplication around.
>
>Please check drivers/gpu/drm/dp/drm_dp.c.
>
This is already in my TODO list as this has been suggested by Rex earlier.
>> ---
>> drivers/gpu/drm/mediatek/Kconfig | 8 +
>> drivers/gpu/drm/mediatek/Makefile | 2 +
>> drivers/gpu/drm/mediatek/mtk_dp.c | 2221 ++++++++++++++++++++++++
>> drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++
>> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
>> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 +
>> 6 files changed, 2801 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..03ffa9b896c3 100644
>> --- a/drivers/gpu/drm/mediatek/Kconfig
>> +++ b/drivers/gpu/drm/mediatek/Kconfig
>> @@ -28,3 +28,11 @@ config DRM_MEDIATEK_HDMI
>> select PHY_MTK_HDMI
>> help
>> DRM/KMS HDMI driver for Mediatek SoCs
>> +
>> +config MTK_DPTX_SUPPORT
>
>Actually, I think that the best would be DRM_MEDIATEK_DP_TX or DRM_MEDIATEK_DP...
>...also, ordering is important, please!
>
I will update the name.
What do you mean by ordering? do you expect the configs to be ordered
alphabetically?
>> + tristate "DRM DPTX Support for Mediatek SoCs"
>> + depends on DRM_MEDIATEK
>> + select PHY_MTK_DP
>> + select DRM_DP_HELPER
>> + help
>> + DRM/KMS Display Port driver for Mediatek SoCs.
>> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
>> index 29098d7c8307..d86a6406055e 100644
>> --- a/drivers/gpu/drm/mediatek/Makefile
>> +++ b/drivers/gpu/drm/mediatek/Makefile
>> @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
>> mtk_hdmi_ddc.o
>>
>> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
>> +
>> +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>> new file mode 100644
>> index 000000000000..7cd8459cf719
>> --- /dev/null
>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
>> @@ -0,0 +1,2221 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019 MediaTek Inc.
>> + * Copyright (c) 2021 BayLibre
>> + */
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_bridge.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/dp/drm_dp_helper.h>
>> +#include <drm/drm_edid.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_print.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <linux/arm-smccc.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/nvmem-consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <sound/hdmi-codec.h>
>> +#include <video/videomode.h>
>> +
>> +#include "mtk_dp_reg.h"
>> +
>> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
>> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
>> +
>> +//TODO: platform/device data or dts?
>
>Assuming that your TODO is about the maximum number of lanes,
>{ .compatible = "mediatek,mt8195-edp-tx", .data = &some_const_structure },
Yes, exactly that, it'll be in v10.
>
>> +#define MTK_DP_MAX_LANES 4
>> +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3
>
>..snip..
>
>> +
>> +static int mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 offset, u8 *buf,
>> + size_t length)
>> +{
>> + int i;
>> + int ret = 0;
>> + int num_regs = (length + 1) / 2;
>> +
>> + /* 2 bytes per register */
>> + for (i = 0; i < num_regs; i++) {
>> + u32 val = buf[i * 2] |
>> + (i * 2 + 1 < length ? buf[i * 2 + 1] << 8 : 0);
>> +
>> + ret = mtk_dp_write(mtk_dp, offset + i * 4, val);
>> + if (ret)
>> + goto out;
>
>if (ret)
> return ret;
>
>> + }
>> +
>> +out:
>
>Remove this label.
>
>> + return ret;
>> +}
>> +
>> +static unsigned long mtk_dp_sip_atf_call(unsigned int cmd, unsigned int para)
>> +{
>> + struct arm_smccc_res res;
>> +
>> + arm_smccc_smc(MTK_DP_SIP_CONTROL_AARCH32, cmd, para, 0, 0, 0, 0, 0,
>> + &res);
>> +
>> + pr_debug("[DPTX]%s cmd 0x%x, p1 0x%x, ret 0x%lx-0x%lx", __func__, cmd,
>> + para, res.a0, res.a1);
>> + return res.a1;
>> +}
>> +
>> +static int mtk_dp_msa_bypass_disable(struct mtk_dp *mtk_dp)
>> +{
>> + const u16 bits_to_set =
>> + BIT(HTOTAL_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(VTOTAL_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(HSTART_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(VSTART_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(HWIDTH_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(VHEIGHT_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(HSP_SEL_DP_ENC0_P0_SHIFT) | BIT(HSW_SEL_DP_ENC0_P0_SHIFT) |
>> + BIT(VSP_SEL_DP_ENC0_P0_SHIFT) | BIT(VSW_SEL_DP_ENC0_P0_SHIFT);
>> + return mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3030, bits_to_set,
>> + bits_to_set);
>> +}
>> +
>> +#define MTK_UPD_BITS_OR_OUT(mtk_dp, offset, val, mask, ret, label) \
>> + do {\
>> + ret = mtk_dp_update_bits(mtk_dp, offset, val, mask); \
>> + if (ret) \
>> + goto label; \
>> + } while (0)
>
>I'm sorry, no offense - but this macro is a bit ugly...
>
>I think I understand why you have introduced it, but in my opinion this decreases
>human readability a lot, I was even about to point out multiple functions that
>you had unused labels before checking this macro, as that was totally unexpected...
>
>In my opinion, this should be open-coded everywhere... yes it makes the file a
>bit fatter in terms of amount of text, but eh... it's life :)))
>
>
No offense taken... I find the macro ugly too... but I couldn't think
of anything less
ugly... I'll bite the bullet and write all of the code then.
>> +
>
>....snip....
>
>> +
>> +static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp,
>> + enum mtk_dp_color_format color_format)
>> +{
>> + u32 val;
>> + int ret;
>> +
>> + mtk_dp->info.format = color_format;
>> +
>> + /* Update MISC0 */
>> + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034,
>> + color_format << DP_TEST_COLOR_FORMAT_SHIFT,
>> + DP_TEST_COLOR_FORMAT_MASK);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + switch (color_format) {
>> + case MTK_DP_COLOR_FORMAT_YUV_422:
>> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422;
>> + break;
>> + case MTK_DP_COLOR_FORMAT_YUV_420:
>> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420;
>> + break;
>> + case MTK_DP_COLOR_FORMAT_YONLY:
>> + case MTK_DP_COLOR_FORMAT_RAW:
>> + case MTK_DP_COLOR_FORMAT_RESERVED:
>> + case MTK_DP_COLOR_FORMAT_UNKNOWN:
>> + drm_warn(mtk_dp->drm_dev, "Unsupported color format: %d\n",
>> + color_format);
>> + fallthrough;
>
>Uhm, are you sure that you should fallthrough?
>If we get a color format that we don't understand, the best thing that would
>happen is that we'd get scrambled colors... which is something unwanted.
>
>The best idea is to return -EINVAL here, hence refuse to go on with an invalid
>color format.
>
Will do.
>> + case MTK_DP_COLOR_FORMAT_RGB_444:
>> + case MTK_DP_COLOR_FORMAT_YUV_444:
>> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB;
>> + break;
>> + }
>> +
>> + return mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val,
>> + PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK);
>> +}
>> +
>
>...snip...
>
>> +
>> +static void mtk_dp_initialize_hpd_detect_settings(struct mtk_dp *mtk_dp)
>> +{
>> + // Debounce threshold
>
>I think I already told you to fix comments style.
>
>> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
>> + 8 << HPD_DEB_THD_DP_TRANS_P0_SHIFT,
>> + HPD_DEB_THD_DP_TRANS_P0_MASK);
>> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
>> + (HPD_INT_THD_DP_TRANS_P0_LOWER_500US |
>> + HPD_INT_THD_DP_TRANS_P0_UPPER_1100US)
>> + << HPD_INT_THD_DP_TRANS_P0_SHIFT,
>> + HPD_INT_THD_DP_TRANS_P0_MASK);
>> +
>> + // Connect threshold 1.5ms + 5 x 0.1ms = 2ms
>> + // Disconnect threshold 1.5ms + 5 x 0.1ms = 2ms
>> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
>> + (5 << HPD_DISC_THD_DP_TRANS_P0_SHIFT) |
>> + (5 << HPD_CONN_THD_DP_TRANS_P0_SHIFT),
>> + HPD_DISC_THD_DP_TRANS_P0_MASK |
>> + HPD_CONN_THD_DP_TRANS_P0_MASK);
>> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3430,
>> + HPD_INT_THD_ECO_DP_TRANS_P0_HIGH_BOUND_EXT,
>> + HPD_INT_THD_ECO_DP_TRANS_P0_MASK);
>> +}
>> +
>
>..snip..
>
>> +
>> +static int mtk_dp_power_disable(struct mtk_dp *mtk_dp)
>> +{
>> + int ret;
>> +
>> + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0);
>> +
>> + if (ret)
>> + goto out;
>> +
>> + usleep_range(10, 200);
>> + ret = mtk_dp_write(mtk_dp, MTK_DP_0034,
>> + DA_CKM_CKTX0_EN_FORCE_EN | DA_CKM_BIAS_LPF_EN_FORCE_VAL |
>> + DA_CKM_BIAS_EN_FORCE_VAL |
>> + DA_XTP_GLB_LDO_EN_FORCE_VAL |
>> + DA_XTP_GLB_AVD10_ON_FORCE_VAL);
>> +
>> + if (ret)
>> + goto out;
>> +
>> + /* Disable RX */
>> + ret = mtk_dp_write(mtk_dp, MTK_DP_1040, 0);
>> +
>> + if (ret)
>> + goto out;
>> +
>> + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD,
>> + 0x550 | BIT(FUSE_SEL_SHIFT) | BIT(MEM_ISO_EN_SHIFT));
>> +
>> +out:
>
>You are using a goto instead of a return, but you're not reverting any change,
>so this goto usage is not justified - hence, wrong.
>
>Please simply return ret when anything fails, and return 0 at the end of this
>function.
>
> .......
> (blahblah function code)
>
> ret = something()
> if (ret)
> return ret;
>
> return 0;
>}
>
>> + return ret;
>> +}
>> +
>
>...snip...
I'll remove all the gotos that have no unwrapping then.
>
>> +
>> +static int mtk_dp_dt_parse(struct mtk_dp *mtk_dp,
>> + struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + int ret = 0;
>> + void __iomem *base;
>> +
>> + base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> +
>> + mtk_dp->regs = syscon_node_to_regmap(dev->of_node);
>> + if (IS_ERR(mtk_dp->regs))
>> + return PTR_ERR(mtk_dp->regs);
>> +
>> + //TODO: optional clock?
>
>Well, if it's optional, you should use devm_clk_get_optional(), meaning
>that......
>
>> + mtk_dp->dp_tx_clk = devm_clk_get(dev, "faxi");
>> + if (IS_ERR(mtk_dp->dp_tx_clk)) {
>> + ret = PTR_ERR(mtk_dp->dp_tx_clk);
>> + dev_info(dev, "Failed to get dptx clock: %d\n", ret);
>> + mtk_dp->dp_tx_clk = NULL;
>
>...I shouldn't see this snippet of code, as it should be a bit different... :)
>
It is still unclear to me if this should be optional or not, hence the 'TODO'
>> + }
>> +
>> + return 0;
>> +}
>> +
>
>..snip..
>
>> +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>> + struct drm_bridge_state *old_state)
>> +{
>> + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
>> +
>> + mtk_dp_video_mute(mtk_dp, true);
>> + mtk_dp->state = MTK_DP_STATE_IDLE;
>> + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
>> +
>> + mtk_dp->enabled = false;
>
>> + msleep(100);
>
>100 milliseconds is an eternity, why are we sleeping for *so long* here?
>
>Please, either add a comment that fully explains the resons for that, or
>remove the sleep entirely: to my eyes, it doesn't look like this sleep is
>really needed for anything important because here you are disabling the
>bridge and *powering off* the IP entirely.
>
This sleep can be removed.
I will also make an effort to document all the other delays in the code for v10.
>> + mtk_dp_poweroff(mtk_dp);
>> +}
>> +
>
>..snip..
>
>> +
>> +static enum drm_mode_status
>> +mtk_dp_bridge_mode_valid(struct drm_bridge *bridge,
>> + const struct drm_display_info *info,
>> + const struct drm_display_mode *mode)
>> +{
>> + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
>> + u32 rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000;
>> + u32 bpp = info->color_formats & DRM_COLOR_FORMAT_YCBCR422 ? 16 : 24;
>> +
>> + if (rx_linkrate * mtk_dp->train_info.lane_count < mode->clock * bpp / 8)
>> + return MODE_CLOCK_HIGH;
>> +
>> + if (mode->clock > 600000)
>> + return MODE_CLOCK_HIGH;
>> +
>> + //TODO: explain magic number: 62?
>
>Please fix this TODO for v10.
>
>> + if ((mode->clock * 1000) / (mode->htotal * mode->vtotal) > 62)
>> + return MODE_CLOCK_HIGH;
>> +
>> + return MODE_OK;
>> +}
>> +
>
>
Thx a lot for your review,
Guillaume.
Il 12/04/22 11:46, Guillaume Ranquet ha scritto:
> On Mon, 28 Mar 2022 11:14, AngeloGioacchino Del Regno
> <[email protected]> wrote:
>> Il 28/03/22 00:39, Guillaume Ranquet ha scritto:
>>> From: Markus Schneider-Pargmann <[email protected]>
>>>
>>> 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 shares its iomap range with the mtk-dp-phy driver using
>>> the regmap/syscon facility.
>>>
>>> This driver is based on an initial version by
>>> Jason-JH.Lin <[email protected]>.
>>>
>>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>>> Signed-off-by: Guillaume Ranquet <[email protected]>
>>> Reported-by: kernel test robot <[email protected]>
>>
>> Hello Guillaume,
>> as you know, there's some more work to do on this driver.
>>
>> I will also mention here, not on the code, that at this point, your
>> mtk_dp_aux_transfer() function has something VERY similar to function
>> drm_dp_dpcd_access(), so I really believe that you can instead use
>> functions drm_dp_dpcd_read() and drm_dp_dpcd_write(), avoiding code
>> duplication around.
>>
>> Please check drivers/gpu/drm/dp/drm_dp.c.
>>
>
> This is already in my TODO list as this has been suggested by Rex earlier.
>
>>> ---
>>> drivers/gpu/drm/mediatek/Kconfig | 8 +
>>> drivers/gpu/drm/mediatek/Makefile | 2 +
>>> drivers/gpu/drm/mediatek/mtk_dp.c | 2221 ++++++++++++++++++++++++
>>> drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++
>>> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
>>> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 +
>>> 6 files changed, 2801 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..03ffa9b896c3 100644
>>> --- a/drivers/gpu/drm/mediatek/Kconfig
>>> +++ b/drivers/gpu/drm/mediatek/Kconfig
>>> @@ -28,3 +28,11 @@ config DRM_MEDIATEK_HDMI
>>> select PHY_MTK_HDMI
>>> help
>>> DRM/KMS HDMI driver for Mediatek SoCs
>>> +
>>> +config MTK_DPTX_SUPPORT
>>
>> Actually, I think that the best would be DRM_MEDIATEK_DP_TX or DRM_MEDIATEK_DP...
>> ...also, ordering is important, please!
>>
> I will update the name.
> What do you mean by ordering? do you expect the configs to be ordered
> alphabetically?
>
Yes, correct. Configuration options shall be alphabetically ordered.
Obviously, the same applies for Makefile :)
>>> + tristate "DRM DPTX Support for Mediatek SoCs"
>>> + depends on DRM_MEDIATEK
>>> + select PHY_MTK_DP
>>> + select DRM_DP_HELPER
>>> + help
>>> + DRM/KMS Display Port driver for Mediatek SoCs.
>>> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
>>> index 29098d7c8307..d86a6406055e 100644
>>> --- a/drivers/gpu/drm/mediatek/Makefile
>>> +++ b/drivers/gpu/drm/mediatek/Makefile
>>> @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
>>> mtk_hdmi_ddc.o
>>>
>>> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
>>> +
>>> +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> new file mode 100644
>>> index 000000000000..7cd8459cf719
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
..snip..
>>> +
>>> +#define MTK_UPD_BITS_OR_OUT(mtk_dp, offset, val, mask, ret, label) \
>>> + do {\
>>> + ret = mtk_dp_update_bits(mtk_dp, offset, val, mask); \
>>> + if (ret) \
>>> + goto label; \
>>> + } while (0)
>>
>> I'm sorry, no offense - but this macro is a bit ugly...
>>
>> I think I understand why you have introduced it, but in my opinion this decreases
>> human readability a lot, I was even about to point out multiple functions that
>> you had unused labels before checking this macro, as that was totally unexpected...
>>
>> In my opinion, this should be open-coded everywhere... yes it makes the file a
>> bit fatter in terms of amount of text, but eh... it's life :)))
>>
>>
> No offense taken... I find the macro ugly too... but I couldn't think
> of anything less
> ugly... I'll bite the bullet and write all of the code then.
>
I can't think of anything shorter, either.. so yep, sometimes writing 'em all
is simply the best option.
>>> +
>>
>> ....snip....
>>
>>> +
>>> +static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp,
>>> + enum mtk_dp_color_format color_format)
>>> +{
..snip..
>>> +
>>> +static int mtk_dp_dt_parse(struct mtk_dp *mtk_dp,
>>> + struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + int ret = 0;
>>> + void __iomem *base;
>>> +
>>> + base = devm_platform_ioremap_resource(pdev, 0);
>>> + if (IS_ERR(base))
>>> + return PTR_ERR(base);
>>> +
>>> + mtk_dp->regs = syscon_node_to_regmap(dev->of_node);
>>> + if (IS_ERR(mtk_dp->regs))
>>> + return PTR_ERR(mtk_dp->regs);
>>> +
>>> + //TODO: optional clock?
>>
>> Well, if it's optional, you should use devm_clk_get_optional(), meaning
>> that......
>>
>>> + mtk_dp->dp_tx_clk = devm_clk_get(dev, "faxi");
>>> + if (IS_ERR(mtk_dp->dp_tx_clk)) {
>>> + ret = PTR_ERR(mtk_dp->dp_tx_clk);
>>> + dev_info(dev, "Failed to get dptx clock: %d\n", ret);
>>> + mtk_dp->dp_tx_clk = NULL;
>>
>> ...I shouldn't see this snippet of code, as it should be a bit different... :)
>>
> It is still unclear to me if this should be optional or not, hence the 'TODO'
>
Thinking about this a bit more - this driver is for MT8195 only, for now... and
the DP cannot work without the AXI clock being up... logically.
I'd say, at this point, to make this mandatory.
If any newer platform will make this optional, we can either assign clk26m (ugh!)
or we can get back to this one and convert this to devm_clk_get_optional().
Don't forget that we can also manage this in devicetree bindings, where we can
for example declare a property to be optional for one or more compatibles, so
there's no problem if we'd be doing such a change in the future, I think.
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> ..snip..
>>
>>> +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>>> + struct drm_bridge_state *old_state)
>>> +{
>>> + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
>>> +
>>> + mtk_dp_video_mute(mtk_dp, true);
>>> + mtk_dp->state = MTK_DP_STATE_IDLE;
>>> + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
>>> +
>>> + mtk_dp->enabled = false;
>>
>>> + msleep(100);
>>
>> 100 milliseconds is an eternity, why are we sleeping for *so long* here?
>>
>> Please, either add a comment that fully explains the resons for that, or
>> remove the sleep entirely: to my eyes, it doesn't look like this sleep is
>> really needed for anything important because here you are disabling the
>> bridge and *powering off* the IP entirely.
>>
> This sleep can be removed.
> I will also make an effort to document all the other delays in the code for v10.
>
Thanks for that.
(snip)
>>> +
>>
>>
>
> Thx a lot for your review,
> Guillaume.
You're welcome!
Cheers,
Angelo