On 06-Feb-23 19:12, Tomi Valkeinen wrote:
> On 05/02/2023 15:42, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 03-Feb-23 20:42, Tomi Valkeinen wrote:
>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>> The newer version of DSS (AM625-DSS) has 2 OLDI TXes at its disposal.
>>>> These can be configured to support the following modes:
>>>>
>>>> 1. OLDI_SINGLE_LINK_SINGLE_MODE
>>>> Single Output over OLDI 0.
>>>> +------+ +---------+ +-------+
>>>> | | | | | |
>>>> | CRTC +------->+ ENCODER +----->| PANEL |
>>>> | | | | | |
>>>> +------+ +---------+ +-------+
>>>>
>>>> 2. OLDI_SINGLE_LINK_CLONE_MODE
>>>> Duplicate Output over OLDI 0 and 1.
>>>> +------+ +---------+ +-------+
>>>> | | | | | |
>>>> | CRTC +---+--->| ENCODER +----->| PANEL |
>>>> | | | | | | |
>>>> +------+ | +---------+ +-------+
>>>> |
>>>> | +---------+ +-------+
>>>> | | | | |
>>>> +--->| ENCODER +----->| PANEL |
>>>> | | | |
>>>> +---------+ +-------+
>>>>
>>>> 3. OLDI_DUAL_LINK_MODE
>>>> Combined Output over OLDI 0 and 1.
>>>> +------+ +---------+ +-------+
>>>> | | | +----->| |
>>>> | CRTC +------->+ ENCODER | | PANEL |
>>>> | | | +----->| |
>>>> +------+ +---------+ +-------+
>>>>
>>>> Following the above pathways for different modes, 2 encoder/panel-bridge
>>>> pipes get created for clone mode, and 1 pipe in cases of single link and
>>>> dual link mode.
>>>>
>>>> Add support for confguring the OLDI modes using OF and LVDS DRM helper
>>>> functions.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
>>>> ---
>>>> drivers/gpu/drm/tidss/tidss_dispc.c | 24 ++-
>>>> drivers/gpu/drm/tidss/tidss_dispc.h | 12 ++
>>>> drivers/gpu/drm/tidss/tidss_drv.h | 3 +
>>>> drivers/gpu/drm/tidss/tidss_encoder.c | 4 +-
>>>> drivers/gpu/drm/tidss/tidss_encoder.h | 3 +-
>>>> drivers/gpu/drm/tidss/tidss_kms.c | 221 ++++++++++++++++++++++++--
>>>> 6 files changed, 245 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index b55ccbcaa67f..37a73e309330 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -88,6 +88,8 @@ const struct dispc_features dispc_k2g_feats = {
>>>> .subrev = DISPC_K2G,
>>>> + .has_oldi = false,
>>>> +
>>>> .common = "common",
>>>> .common_regs = tidss_k2g_common_regs,
>>>> @@ -166,6 +168,8 @@ const struct dispc_features dispc_am625_feats = {
>>>> .subrev = DISPC_AM625,
>>>> + .has_oldi = true,
>>>> +
>>>> .common = "common",
>>>> .common_regs = tidss_am65x_common_regs,
>>>> @@ -218,6 +222,8 @@ const struct dispc_features dispc_am65x_feats = {
>>>> .subrev = DISPC_AM65X,
>>>> + .has_oldi = true,
>>>> +
>>>> .common = "common",
>>>> .common_regs = tidss_am65x_common_regs,
>>>> @@ -309,6 +315,8 @@ const struct dispc_features dispc_j721e_feats = {
>>>> .subrev = DISPC_J721E,
>>>> + .has_oldi = false,
>>>> +
>>>> .common = "common_m",
>>>> .common_regs = tidss_j721e_common_regs,
>>>> @@ -361,6 +369,8 @@ struct dispc_device {
>>>> struct dss_vp_data vp_data[TIDSS_MAX_VPS];
>>>> + enum dispc_oldi_modes oldi_mode;
>>>> +
>>>> u32 *fourccs;
>>>> u32 num_fourccs;
>>>> @@ -1963,6 +1973,12 @@ const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len)
>>>> return dispc->fourccs;
>>>> }
>>>> +void dispc_set_oldi_mode(struct dispc_device *dispc,
>>>> + enum dispc_oldi_modes oldi_mode)
>>>> +{
>>>> + dispc->oldi_mode = oldi_mode;
>>>> +}
>>>> +
>>>> static s32 pixinc(int pixels, u8 ps)
>>>> {
>>>> if (pixels == 1)
>>>> @@ -2647,7 +2663,7 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>>>> REG_GET(dispc, DSS_SYSSTATUS, 2, 2),
>>>> REG_GET(dispc, DSS_SYSSTATUS, 3, 3));
>>>> - if (dispc->feat->subrev == DISPC_AM65X)
>>>> + if (dispc->feat->has_oldi)
>>>> dev_dbg(dispc->dev, "OLDI RESETDONE %d,%d,%d\n",
>>>> REG_GET(dispc, DSS_SYSSTATUS, 5, 5),
>>>> REG_GET(dispc, DSS_SYSSTATUS, 6, 6),
>>>> @@ -2688,7 +2704,7 @@ static int dispc_iomap_resource(struct platform_device *pdev, const char *name,
>>>> return 0;
>>>> }
>>>> -static int dispc_init_am65x_oldi_io_ctrl(struct device *dev,
>>>> +static int dispc_init_am6xx_oldi_io_ctrl(struct device *dev,
>>>> struct dispc_device *dispc)
>>>> {
>>>> dispc->oldi_io_ctrl =
>>>> @@ -2827,8 +2843,8 @@ int dispc_init(struct tidss_device *tidss)
>>>> dispc->vp_data[i].gamma_table = gamma_table;
>>>> }
>>>> - if (feat->subrev == DISPC_AM65X) {
>>>> - r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
>>>> + if (feat->has_oldi) {
>>>> + r = dispc_init_am6xx_oldi_io_ctrl(dev, dispc);
>>>> if (r)
>>>> return r;
>>>> }
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> index 971f2856f015..880bc7de68b3 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>>> @@ -64,6 +64,15 @@ enum dispc_dss_subrevision {
>>>> DISPC_J721E,
>>>> };
>>>> +enum dispc_oldi_modes {
>>>> + OLDI_MODE_SINGLE_LINK, /* Single output over OLDI 0. */
>>>> + OLDI_MODE_CLONE_SINGLE_LINK, /* Cloned output over OLDI 0 and 1. */
>>>> + OLDI_MODE_DUAL_LINK, /* Combined output over OLDI 0 and 1. */
>>>> + OLDI_MODE_OFF, /* OLDI TXes not connected in OF. */
>>>> + OLDI_MODE_UNSUPPORTED, /* Unsupported OLDI configuration in OF. */
>>>> + OLDI_MODE_UNAVAILABLE, /* OLDI TXes not available in SoC. */
>>>> +};
>>>> +
>>>> struct dispc_features {
>>>> int min_pclk_khz;
>>>> int max_pclk_khz[DISPC_PORT_MAX_BUS_TYPE];
>>>> @@ -72,6 +81,8 @@ struct dispc_features {
>>>> enum dispc_dss_subrevision subrev;
>>>> + bool has_oldi;
>>>> +
>>>> const char *common;
>>>> const u16 *common_regs;
>>>> u32 num_vps;
>>>> @@ -131,6 +142,7 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane,
>>>> u32 hw_videoport);
>>>> int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable);
>>>> const u32 *dispc_plane_formats(struct dispc_device *dispc, unsigned int *len);
>>>> +void dispc_set_oldi_mode(struct dispc_device *dispc, enum dispc_oldi_modes oldi_mode);
>>>> int dispc_init(struct tidss_device *tidss);
>>>> void dispc_remove(struct tidss_device *tidss);
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
>>>> index 0ce7ee5ccd5b..58892f065c16 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>>>> @@ -13,6 +13,9 @@
>>>> #define TIDSS_MAX_PLANES 4
>>>> #define TIDSS_MAX_OUTPUT_PORTS 4
>>>> +/* For AM625-DSS with 2 OLDI TXes */
>>>> +#define TIDSS_MAX_BRIDGES_PER_PIPE 2
>>>> +
>>>> typedef u32 dispc_irq_t;
>>>> struct tidss_device {
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
>>>> index 0d4865e9c03d..bd2a7358d7b0 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
>>>> @@ -70,7 +70,8 @@ static const struct drm_encoder_funcs encoder_funcs = {
>>>> };
>>>> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>> - u32 encoder_type, u32 possible_crtcs)
>>>> + u32 encoder_type, u32 possible_crtcs,
>>>> + u32 possible_clones)
>>>> {
>>>> struct drm_encoder *enc;
>>>> int ret;
>>>> @@ -80,6 +81,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>> return ERR_PTR(-ENOMEM);
>>>> enc->possible_crtcs = possible_crtcs;
>>>> + enc->possible_clones = possible_clones;
>>>> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
>>>> encoder_type, NULL);
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
>>>> index ace877c0e0fd..01c62ba3ef16 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_encoder.h
>>>> +++ b/drivers/gpu/drm/tidss/tidss_encoder.h
>>>> @@ -12,6 +12,7 @@
>>>> struct tidss_device;
>>>> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>>>> - u32 encoder_type, u32 possible_crtcs);
>>>> + u32 encoder_type, u32 possible_crtcs,
>>>> + u32 possible_clones);
>>>> #endif
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
>>>> index d449131935d2..8322ee6310bf 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_kms.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
>>>> @@ -13,6 +13,7 @@
>>>> #include <drm/drm_of.h>
>>>> #include <drm/drm_panel.h>
>>>> #include <drm/drm_vblank.h>
>>>> +#include <linux/of.h>
>>>> #include "tidss_crtc.h"
>>>> #include "tidss_dispc.h"
>>>> @@ -104,26 +105,129 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>>>> .atomic_commit = drm_atomic_helper_commit,
>>>> };
>>>> +static enum dispc_oldi_modes tidss_get_oldi_mode(struct tidss_device *tidss)
>>>> +{
>>>> + int pixel_order;
>>>> + enum dispc_oldi_modes oldi_mode;
>>>> + struct device_node *oldi0_port, *oldi1_port;
>>>> +
>>>> + /*
>>>> + * For am625-dss, the OLDI ports are expected at port reg = 0 and 2,
>>>> + * and for am65x-dss, the OLDI port is expected only at port reg = 0.
>>>> + */
>>>> + const u32 portnum_oldi0 = 0, portnum_oldi1 = 2;
>>>> +
>>>> + oldi0_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi0);
>>>> + oldi1_port = of_graph_get_port_by_id(tidss->dev->of_node, portnum_oldi1);
>>>> +
>>>> + if (!(oldi0_port || oldi1_port)) {
>>>> + /* Keep OLDI TXes OFF if neither OLDI port is present. */
>>>> + oldi_mode = OLDI_MODE_OFF;
>>>> + } else if (oldi0_port && !oldi1_port) {
>>>> + /*
>>>> + * OLDI0 port found, but not OLDI1 port. Setting single
>>>> + * link output mode.
>>>> + */
>>>> + oldi_mode = OLDI_MODE_SINGLE_LINK;
>>>> + } else if (!oldi0_port && oldi1_port) {
>>>> + /*
>>>> + * The 2nd OLDI TX cannot be operated alone. This use case is
>>>> + * not supported in the HW. Since the pins for OLDIs 0 and 1 are
>>>> + * separate, one could theoretically set a clone mode over OLDIs
>>>> + * 0 and 1 and just simply not use the OLDI 0. This is a hacky
>>>> + * way to enable only OLDI TX 1 and hence is not officially
>>>> + * supported.
>>>> + */
>>>> + dev_warn(tidss->dev,
>>>> + "Single Mode over OLDI 1 is not supported in HW.\n");
>>>> + oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> + } else {
>>>> + /*
>>>> + * OLDI Ports found for both the OLDI TXes. The DSS is to be
>>>> + * configured in either Dual Link or Clone Mode.
>>>> + */
>>>> + pixel_order = drm_of_lvds_get_dual_link_pixel_order(oldi0_port,
>>>> + oldi1_port);
>>>> + switch (pixel_order) {
>>>> + case -EINVAL:
>>>> + /*
>>>> + * The dual link properties were not found in at least
>>>> + * one of the sink nodes. Since 2 OLDI ports are present
>>>> + * in the DT, it can be safely assumed that the required
>>>> + * configuration is Clone Mode.
>>>> + */
>>>> + oldi_mode = OLDI_MODE_CLONE_SINGLE_LINK;
>>>> + break;
>>>> +
>>>> + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS:
>>>> + /*
>>>> + * Note that the OLDI TX 0 transmits the odd set of
>>>> + * pixels while the OLDI TX 1 transmits the even set.
>>>> + * This is a fixed configuration in the HW and an cannot
>>>> + * be change via SW.
>>>> + */
>>>> + dev_warn(tidss->dev,
>>>> + "EVEN-ODD Dual-Link Mode is not supported in HW.\n");
>>>> + oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> + break;
>>>> +
>>>> + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS:
>>>> + oldi_mode = OLDI_MODE_DUAL_LINK;
>>>> + break;
>>>> +
>>>> + default:
>>>> + oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> + of_node_put(oldi0_port);
>>>> + of_node_put(oldi1_port);
>>>> +
>>>> + return oldi_mode;
>>>> +}
>>>> +
>>>> static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>>>> {
>>>> struct device *dev = tidss->dev;
>>>> unsigned int fourccs_len;
>>>> const u32 *fourccs = dispc_plane_formats(tidss->dispc, &fourccs_len);
>>>> - unsigned int i;
>>>> + unsigned int i, j;
>>>> struct pipe {
>>>> u32 hw_videoport;
>>>> - struct drm_bridge *bridge;
>>>> + struct drm_bridge *bridge[TIDSS_MAX_BRIDGES_PER_PIPE];
>>>> u32 enc_type;
>>>> + u32 num_bridges;
>>>> };
>>>> const struct dispc_features *feat = tidss->feat;
>>>> u32 output_ports = feat->num_output_ports;
>>>> u32 max_planes = feat->num_planes;
>>>> - struct pipe pipes[TIDSS_MAX_VPS];
>>>> + struct pipe pipes[TIDSS_MAX_VPS] = {0};
>>>> +
>>>> u32 num_pipes = 0;
>>>> u32 crtc_mask;
>>>> + enum dispc_oldi_modes oldi_mode = OLDI_MODE_UNAVAILABLE;
>>>> + u32 num_oldi = 0;
>>>> + u32 num_encoders = 0;
>>>> + u32 oldi_pipe_index = 0;
>>>> +
>>>> + if (feat->has_oldi) {
>>>> + oldi_mode = tidss_get_oldi_mode(tidss);
>>>> +
>>>> + if ((oldi_mode == OLDI_MODE_DUAL_LINK ||
>>>> + oldi_mode == OLDI_MODE_CLONE_SINGLE_LINK) &&
>>>> + feat->subrev == DISPC_AM65X) {
>>>> + dev_warn(tidss->dev,
>>>> + "am65x-dss does not support this OLDI mode.\n");
>>>> +
>>>> + oldi_mode = OLDI_MODE_UNSUPPORTED;
>>>> + }
>>>
>>> Shouldn't OLDI_MODE_UNSUPPORTED be handled as an error? It means the DT
>>> is faulty, doesn't it? Maybe it could even be renamed to
>>> OLDI_MODE_ERROR. Or tidss_get_oldi_mode() could return a negative error
>>> code.
>>>
>>
>> The idea was to let the framework continue configuring the 2nd videoport
>> for DPI, even if the OLDI DT is wrong. But I have come across more
>> examples recently where that is not the case. DT error for one pipe has
>> resulted in returning of an error code.
>>
>> Will make the change.
>
> My opinion is that the DT has to be correct. If it isn't, just fail and
> exit as soon as possible. There shouldn't be any reasons for the drivers
> to be trying to cope with a broken DT.
Understood! Will error out in those cases!
>
>>>> +
>>>> + dispc_set_oldi_mode(tidss->dispc, oldi_mode);
>>>> + }
>>>
>>> Would it be better to move the above dispc_set_oldi_mode() to be outside
>>> the if block? Then oldi mode would be set to OLDI_MODE_UNAVAILABLE on
>>> SoCs that don't have OLDI.
>>
>> Ahh, yes! Will make the change.
>>
>>>
>>> tidss_get_oldi_mode and dispc_set_oldi_mode sound like opposites, but
>>> they're totally different things. Maybe tidss_get_oldi_mode should
>>> rather be something about parsing oldi dt properties or such.
>>
>> Okay! Is 'tidss_parse_oldi_properties' acceptable? This is just
>> something I came up with now. I can think of more if this is not good.
>
> Sounds fine.
>
Okay!
Regards
Aradhya
