| Message ID | 20230125113529.13952-4-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DSS support for AM625 SoC | expand |
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
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index c1c4faccbddc..b55ccbcaa67f 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -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; diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h index 30fb44158347..971f2856f015 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.h +++ b/drivers/gpu/drm/tidss/tidss_dispc.h @@ -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; diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c index 2dac8727d2f4..8558dc6999d8 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.c +++ b/drivers/gpu/drm/tidss/tidss_drv.c @@ -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, }, { }
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(+)