[v7,3/6] drm/tidss: Add support for AM625 DSS
Commit Message
Add support for the DSS controller on TI's new AM625 SoC in the tidss
driver.
The first video port (VP0) in am625-dss can output OLDI signals through
2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
type.
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
drivers/gpu/drm/tidss/tidss_dispc.c | 57 +++++++++++++++++++++++++++++
drivers/gpu/drm/tidss/tidss_dispc.h | 2 +
drivers/gpu/drm/tidss/tidss_drv.c | 1 +
3 files changed, 60 insertions(+)
Comments
On 25/01/2023 13:35, Aradhya Bhatia wrote:
> Add support for the DSS controller on TI's new AM625 SoC in the tidss
> driver.
>
> The first video port (VP0) in am625-dss can output OLDI signals through
> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
> type.
Not a big thing here as you add support for a new SoC, but the ordering
of the patches is not optimal. Here you add the AM625 DSS support, but
then you continue actually adding the DSS support (well, mainly OLDI) in
the following patches.
I think patch 6 could be before this patch. Parts of patch 4 could also
be before this patch. The AM65X renames from patch 5 could be before
this patch.
I'm mainly thinking of a case where someone uses AM625 and is bisecting
a problem. What happens if his board uses OLDI, and he happens to hit
one of these patches during bisect? If the display just stays black, but
otherwise everything works fine, then no problem. But if it crashes or
starts spamming sync losts or such or gives errors, it's not so nice.
Tomi
On 03-Feb-23 21:03, Tomi Valkeinen wrote:
> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>> driver.
>>
>> The first video port (VP0) in am625-dss can output OLDI signals through
>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>> type.
>
> Not a big thing here as you add support for a new SoC, but the ordering
> of the patches is not optimal. Here you add the AM625 DSS support, but
> then you continue actually adding the DSS support (well, mainly OLDI) in
> the following patches.
>
> I think patch 6 could be before this patch. Parts of patch 4 could also
> be before this patch. The AM65X renames from patch 5 could be before
> this patch.
I can move whole of Patch 6 and even of Patch 4 before this one. I have
mentioned 'AM625-DSS' in a couple comments which I can make generic,
and the rest everything is SoC-agnostic.
I haven't tried this, but my concern is if we break patch 5 into 2
separate patches,
i. AM65X rename plus SoC based switch case, and
ii. Addition of AM625 SoC case
then I might have to overwrite some changes implemented during (i) in
(ii). I don't suppose that would be okay, would it?
Also, is it important to keep the compatible-addition patches of
DT-binding and driver next to each other in the series? Or should
the DT-binding patches should be the first ones? Just curious! =)
>
> I'm mainly thinking of a case where someone uses AM625 and is bisecting
> a problem. What happens if his board uses OLDI, and he happens to hit
> one of these patches during bisect? If the display just stays black, but
> otherwise everything works fine, then no problem. But if it crashes or
> starts spamming sync losts or such or gives errors, it's not so nice.
>
You are right! This certainly makes sense.
Regards
Aradhya
On 05/02/2023 16:31, Aradhya Bhatia wrote:
>
>
> On 03-Feb-23 21:03, Tomi Valkeinen wrote:
>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>>> driver.
>>>
>>> The first video port (VP0) in am625-dss can output OLDI signals through
>>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>>> type.
>>
>> Not a big thing here as you add support for a new SoC, but the ordering
>> of the patches is not optimal. Here you add the AM625 DSS support, but
>> then you continue actually adding the DSS support (well, mainly OLDI) in
>> the following patches.
>>
>> I think patch 6 could be before this patch. Parts of patch 4 could also
>> be before this patch. The AM65X renames from patch 5 could be before
>> this patch.
>
> I can move whole of Patch 6 and even of Patch 4 before this one. I have
> mentioned 'AM625-DSS' in a couple comments which I can make generic,
> and the rest everything is SoC-agnostic.
>
> I haven't tried this, but my concern is if we break patch 5 into 2
> separate patches,
>
> i. AM65X rename plus SoC based switch case, and
> ii. Addition of AM625 SoC case
>
> then I might have to overwrite some changes implemented during (i) in
> (ii). I don't suppose that would be okay, would it?
I'm not sure I follow here. Wouldn't (i) be a valid patch in its own?
Nothing wrong in expanding that later (even if you end up changing a lot
of it).
That said, I don't think this is a very important topic. There are only
a few commits in the history that might be problematic. A simple fix
would be to add all the features first, and only last add the compatible
string for am625.
Or do all the changes for am625 in a single patch, and try to implement
all the generic restructuring work before that.
Here we do have to change the vp-to-output mapping management, so maybe
the second option won't be simple enough, and it's better to do the
am625 changes in pieces, as in the first option.
So, it's really up to you. Just wanted to raise this possible issue so
that you are aware of it and can do any easy fixes (if there are such).
> Also, is it important to keep the compatible-addition patches of
> DT-binding and driver next to each other in the series? Or should
> the DT-binding patches should be the first ones? Just curious! =)
I believe the convention is to have the DT-binding changes before you
add the compatible string to the driver (if I recall right checkpatch or
some other checking tool complains if you add a driver for a compatible
that doesn't have a DT binding). Generic restructurings could be before
the DT patch, of course, but usually I like to keep the DT binding
changes at the very beginning of the series.
Tomi
Hi Tomi,
On 06-Feb-23 16:28, Tomi Valkeinen wrote:
> On 05/02/2023 16:31, Aradhya Bhatia wrote:
>>
>>
>> On 03-Feb-23 21:03, Tomi Valkeinen wrote:
>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>>>> driver.
>>>>
>>>> The first video port (VP0) in am625-dss can output OLDI signals through
>>>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>>>> type.
>>>
>>> Not a big thing here as you add support for a new SoC, but the ordering
>>> of the patches is not optimal. Here you add the AM625 DSS support, but
>>> then you continue actually adding the DSS support (well, mainly OLDI) in
>>> the following patches.
>>>
>>> I think patch 6 could be before this patch. Parts of patch 4 could also
>>> be before this patch. The AM65X renames from patch 5 could be before
>>> this patch.
>>
>> I can move whole of Patch 6 and even of Patch 4 before this one. I have
>> mentioned 'AM625-DSS' in a couple comments which I can make generic,
>> and the rest everything is SoC-agnostic.
>>
>> I haven't tried this, but my concern is if we break patch 5 into 2
>> separate patches,
>>
>> i. AM65X rename plus SoC based switch case, and
>> ii. Addition of AM625 SoC case
>>
>> then I might have to overwrite some changes implemented during (i) in
>> (ii). I don't suppose that would be okay, would it?
>
> I'm not sure I follow here. Wouldn't (i) be a valid patch in its own?
> Nothing wrong in expanding that later (even if you end up changing a lot
> of it).
>
(i) would be a valid patch, but implementing (ii) would over-write
certain changes done in (i), albeit small changes in terms of brackets
and indents. That didn't feel right initially and hence the question.
> That said, I don't think this is a very important topic. There are only
> a few commits in the history that might be problematic. A simple fix
> would be to add all the features first, and only last add the compatible
> string for am625.
>
> Or do all the changes for am625 in a single patch, and try to implement
> all the generic restructuring work before that.
>
> Here we do have to change the vp-to-output mapping management, so maybe
> the second option won't be simple enough, and it's better to do the
> am625 changes in pieces, as in the first option.
>
Yeah, the first option does seem a little less complicated. Will try to
re-order this as much clearly as possible.
> So, it's really up to you. Just wanted to raise this possible issue so
> that you are aware of it and can do any easy fixes (if there are such).
>
>> Also, is it important to keep the compatible-addition patches of
>> DT-binding and driver next to each other in the series? Or should
>> the DT-binding patches should be the first ones? Just curious! =)
>
> I believe the convention is to have the DT-binding changes before you
> add the compatible string to the driver (if I recall right checkpatch or
> some other checking tool complains if you add a driver for a compatible
> that doesn't have a DT binding). Generic restructurings could be before
> the DT patch, of course, but usually I like to keep the DT binding
> changes at the very beginning of the series.
>
Okay, I will keep the compatible-append in the binding as the first
patch in the series, before the other general structurings.
Thank you!
Regards
Aradhya
@@ -140,6 +140,58 @@ static const u16 tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
[DISPC_SECURE_DISABLE_OFF] = 0xac,
};
+const struct dispc_features dispc_am625_feats = {
+ .max_pclk_khz = {
+ [DISPC_PORT_DPI] = 165000,
+ [DISPC_PORT_OLDI] = 165000,
+ },
+
+ .scaling = {
+ .in_width_max_5tap_rgb = 1280,
+ .in_width_max_3tap_rgb = 2560,
+ .in_width_max_5tap_yuv = 2560,
+ .in_width_max_3tap_yuv = 4096,
+ .upscale_limit = 16,
+ .downscale_limit_5tap = 4,
+ .downscale_limit_3tap = 2,
+ /*
+ * The max supported pixel inc value is 255. The value
+ * of pixel inc is calculated like this: 1+(xinc-1)*bpp.
+ * The maximum bpp of all formats supported by the HW
+ * is 8. So the maximum supported xinc value is 32,
+ * because 1+(32-1)*8 < 255 < 1+(33-1)*4.
+ */
+ .xinc_max = 32,
+ },
+
+ .subrev = DISPC_AM625,
+
+ .common = "common",
+ .common_regs = tidss_am65x_common_regs,
+
+ .num_vps = 2,
+ .vp_name = { "vp1", "vp2" },
+ .ovr_name = { "ovr1", "ovr2" },
+ .vpclk_name = { "vp1", "vp2" },
+
+ .vp_feat = { .color = {
+ .has_ctm = true,
+ .gamma_size = 256,
+ .gamma_type = TIDSS_GAMMA_8BIT,
+ },
+ },
+
+ .num_planes = 2,
+ /* note: vid is plane_id 0 and vidl1 is plane_id 1 */
+ .vid_name = { "vid", "vidl1" },
+ .vid_lite = { false, true, },
+ .vid_order = { 1, 0 },
+
+ /* 3rd output port is not representative of a 3rd pipeline */
+ .num_output_ports = 3,
+ .output_port_bus_type = { DISPC_PORT_OLDI, DISPC_PORT_DPI, DISPC_PORT_OLDI },
+};
+
const struct dispc_features dispc_am65x_feats = {
.max_pclk_khz = {
[DISPC_PORT_DPI] = 165000,
@@ -783,6 +835,7 @@ dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc)
switch (dispc->feat->subrev) {
case DISPC_K2G:
return dispc_k2g_read_and_clear_irqstatus(dispc);
+ case DISPC_AM625:
case DISPC_AM65X:
case DISPC_J721E:
return dispc_k3_read_and_clear_irqstatus(dispc);
@@ -798,6 +851,7 @@ void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
case DISPC_K2G:
dispc_k2g_set_irqenable(dispc, mask);
break;
+ case DISPC_AM625:
case DISPC_AM65X:
case DISPC_J721E:
dispc_k3_set_irqenable(dispc, mask);
@@ -1288,6 +1342,7 @@ void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane,
dispc_k2g_ovr_set_plane(dispc, hw_plane, hw_videoport,
x, y, layer);
break;
+ case DISPC_AM625:
case DISPC_AM65X:
dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport,
x, y, layer);
@@ -2210,6 +2265,7 @@ static void dispc_plane_init(struct dispc_device *dispc)
case DISPC_K2G:
dispc_k2g_plane_init(dispc);
break;
+ case DISPC_AM625:
case DISPC_AM65X:
case DISPC_J721E:
dispc_k3_plane_init(dispc);
@@ -2316,6 +2372,7 @@ static void dispc_vp_write_gamma_table(struct dispc_device *dispc,
case DISPC_K2G:
dispc_k2g_vp_write_gamma_table(dispc, hw_videoport);
break;
+ case DISPC_AM625:
case DISPC_AM65X:
dispc_am65x_vp_write_gamma_table(dispc, hw_videoport);
break;
@@ -59,6 +59,7 @@ enum dispc_port_bus_type {
enum dispc_dss_subrevision {
DISPC_K2G,
+ DISPC_AM625,
DISPC_AM65X,
DISPC_J721E,
};
@@ -87,6 +88,7 @@ struct dispc_features {
};
extern const struct dispc_features dispc_k2g_feats;
+extern const struct dispc_features dispc_am625_feats;
extern const struct dispc_features dispc_am65x_feats;
extern const struct dispc_features dispc_j721e_feats;
@@ -232,6 +232,7 @@ static void tidss_shutdown(struct platform_device *pdev)
static const struct of_device_id tidss_of_table[] = {
{ .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, },
+ { .compatible = "ti,am625-dss", .data = &dispc_am625_feats, },
{ .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, },
{ .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, },
{ }