-
Notifications
You must be signed in to change notification settings - Fork 6.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LCDIF support for RT700 #82635
base: main
Are you sure you want to change the base?
Add LCDIF support for RT700 #82635
Conversation
Hello @KATE-WANG-NXP, and thank you very much for your first pull request to the Zephyr project! |
e99f680
to
723c969
Compare
Hi @KATE-WANG-NXP , please pay attention to the code format, such as: |
723c969
to
c816605
Compare
@@ -32,3 +32,11 @@ properties: | |||
- "24-bit" # 24 Bit | |||
description: | |||
LCD data bus width. The default is set to the reset value of 24-bit | |||
|
|||
version: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not opposed to this approach, but is this an IP revision, or a new IP block? If it is a new IP block, we can actually support multiple compatibles within one driver (and we should, in that case). See here for an example:
zephyr/drivers/i2c/i2c_tca954x.c
Line 204 in e60f040
#define TCA9544A_INIT(n) TCA954x_ROOT_DEFINE(9544, n, 4, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I did not know about this usage before. I think it is an IP revision, for DC8000 there are extra registers to set for a frame configuration compared to DCNano. Actually I did not find any similar use case of adding a new binding for IP revision so I'm not sure it is good practice. I did find using kconfig symbols to isolate extra register configurations but I think it is obsolete?
zephyr/drivers/counter/counter_mcux_snvs.c
Line 279 in e60f040
#if CONFIG_COUNTER_MCUX_SNVS_SRTC_WAKE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is an IP revision, we could add properties to the binding to indicate these registers were supported- we should check though, this IP seems to support a lot more than the instance on the RT595
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes DC8000 has a lot more registers compared with DCNano, because in DC8000 apart form the cursor layer, there are 3 graphical layers instead of 1, to perform color key, or porter duff etc. Since these functions are not used in Zephyr, I was wondering that it would be unnecessary and confusing to add property for each one, so I just added a version binding, similarly treated in MCUXpresso SDK lcdif driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the version binding is probably the right choice- we can split this into a new binding in the future if we end up utilizing the porter-duff blending or similar features
if (0 != (status & kLCDIF_Display0FrameDoneInterrupt)) | ||
{ | ||
/* Handle multi chunk with mipi. */ | ||
#if CONFIG_MIPI_DSI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear why we have dependencies on CONFIG_MIPI_DSI
here?
From what I understand about the DC8000 peripheral, it can support MIPI DBI (8080 and 6800), and it can support MIPI DPI, which would be sent via a DSI phy to the display.
A few questions here:
- The MIPI DPI support would be used for displays that operate in video mode (IE no graphics RAM, require constant refresh). An example would be the RK055HDMIPI4MA0 display
- The DSI PHY on this SOC supports sending MIPI DSI commands directly, right? This would be used for displays like the G1120B0MIPI, correct? Or can the DC8000 still drive those displays?
If I'm understanding the peripheral right, then we probably need something like the following:
- A new compatible for the peripheral- this is clearly different from the DCNANO IP present on the RT500
- A MIPI-DBI driver that supports driving displays in 6800 or 8080 mode
- A MIPI-DSI driver for the MIPI-DPHY (if this IP is reused from the RT500, we can reuse the existing driver)
- Additions to the DCNANO LCDIF driver to support this new DC8000 IP (using the trick I mentioned for multiple compatibles)
Critically, the DC8000 should only have one compatible string. Users can select which driver gets used with the IP based on which Kconfigs they enable. We get into a bit of a strange situation though, since it seems this IP can be used in MIPI-DBI mode, or as a display driver (enabled with CONFIG_DISPLAY
). We probably need to make it so that CONFIG_DISPLAY_MCUX_DCNANO_LCDIF
depends on CONFIG_MIPI_DBI
being disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The DSI PHY on this SOC supports sending MIPI DSI commands directly, right? This would be used for displays like the G1120B0MIPI, correct? Or can the DC8000 still drive those displays?
Yes, on RT700 the panel G1120B0MIPI is driven by DC8000 and MIPI DSI combined.
We can also use the RT500 legacy way to drive G1120B0MIPI by sending MIPI DSI commands and data directly on APB, but since RT700 does not have smartdma, this way can be rather slow, and does not support pixel format convertion, eg. the frame buffer in memory is stored in RGB888 format while the data sent on bus in D16RGB565 format as G1120B0MIPI required.
To use both DC8000 and the MIPI DSI to drive the panel, there are many additional actions to do on the MIPI DSI side. Since the internal interface between these 2 modules have limited buffering, if the data to be sent is too large, the driver will have to chop up the data and sent one piece at a time. So for G1120B0MIPI the CONFIG_MIPI_DSI has to be enabled, for displays in 6800 or 8080 mode, there is no need to enable CONFIG_MIPI_DSI.
- A new compatible for the peripheral- this is clearly different from the DCNANO IP present on the RT500
Yes I created a new compatible nxp,dcnano_lcdif_dbi for this driver drivers/mipi_dbi/mipi_dbi_nxp_dcnano_lcdif.c, take reference from the compatible nxp,dcnano_lcdif of drivers/display/display_mcux_dcnano_lcdif.c(not sure if it is a good practice since they belong to different driver groups).
They all use the same IP DC8000 on RT700, but as you pointed out DC8000 can either be in DBI or in DPI mode so only one compatible at the same time, so in my test example, I change the compatible in the display's overlay(still not sure if it is a good practice since I did not find similar cases).
476b7bb#diff-1b9a0139b8cc287d78c4bb150fc04ccc161c82f8fea192fe9aa1ecc9a177c18aR18
- We probably need to make it so that CONFIG_DISPLAY_MCUX_DCNANO_LCDIF depends on CONFIG_MIPI_DBI being disabled.
Thanks, will update the dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so the DC8000 is sending video data to the MIPI DSI, which then sends it to the RM67162 controller on the G1120B0MIPI? Do we have the ability to perform partial writes using the DC8000? I ask because the RM67162 controller has internal GRAM, so we should be able to do partial refreshes (whereas a standard video mode MIPI DSI controller would not accept partial refreshes)
I'm still not clear where MIPI DBI fits in this chain. For the RM67162, I would expect we would perform partial refreshes via the MIPI DSI API- If we need to integrate the DC8000 into this chain, the best way to do that is likely to use the DC8000 as the Zephyr display driver, and have the RM67162 driver only handle the init phase of the display setup. This is what we do for video mode displays currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add to the above, MIPI DBI (6800/8080) mode should be handled by the MIPI DBI driver, and MIPI-DSI should be handled by the MIPI-DSI driver.
Seeing this peripheral now, I am wondering if we should be merging the MIPI-DBI and MIPI-DSI driver classes at some point, since they do use the same command set (MIPI DCS). That is a broader question though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so the DC8000 is sending video data to the MIPI DSI, which then sends it to the RM67162 controller on the G1120B0MIPI? Yes.
Do we have the ability to perform partial writes using the DC8000? I ask because the RM67162 controller has internal GRAM, so we should be able to do partial refreshes (whereas a standard video mode MIPI DSI controller would not accept partial refreshes) Yes. We can use DC8000 to chose which part of the area on screen to update by setting the coordinates.
I'm still not clear where MIPI DBI fits in this chain. For the RM67162, I would expect we would perform partial refreshes via the MIPI DSI API-
I would prefer this way too so RM67162 driver can still be used as the display driver same as on RT500, but when using the DC8000 and MIPI DSI combined to drive the panel, sending command/data is controlled by DC8000 instead of MIPI-DSI, MIPI-DSI is simply performing like a bridge. So in order to use DC8000 to send DCS command, I added nxp_dcnano_lcdif_dbi driver, which also answers the next comment.
If we need to integrate the DC8000 into this chain, the best way to do that is likely to use the DC8000 as the Zephyr display driver, and have the RM67162 driver only handle the init phase of the display setup. This is what we do for video mode displays currently. Yes that is exactly what I'm dong for RT700 display example for G1120B0MIPI, refering to the HX8394 on RT500, using nxp_dcnano_lcdif_dbi as display driver, only leaving the init to RM67162 driver.
To add to the above, MIPI DBI (6800/8080) mode should be handled by the MIPI DBI driver, and MIPI-DSI should be handled by the MIPI-DSI driver.
Seeing this peripheral now, I am wondering if we should be merging the MIPI-DBI and MIPI-DSI driver classes at some point, since they do use the same command set (MIPI DCS). That is a broader question though. I was thinking about it at some point too, but since the hardware interfaces of MIPI-DBI and MIPI-DSI are different similar to parallel and serial ports, plus MIPI-DSI has DPHY to consider, I would prefer leaving them seperated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you for the clarification!
I think what we need in order to move this PR forwards would be something like the following changes:
- Updates to the DCNANO eLCDIF driver to enable the following:
- Any additional configuration needed for the DC8000 versus the DCNANO
- Changes to the
display_write
function to support partial area updates when using the DC8000
- Any changes needed to the MIPI DSI driver to enable it to act as a bridge for the DCNANO (may not need any, because we do this on the RT500 already for video mode)
- A new MIPI-DBI driver for the DC8000
Do these changes sound reasonable? If so, feel free to ping me to review once they have been made
Also, if the changes to the eLCDIF driver get too large, we should consider splitting the DC8000 into a new binding. The fact the IP has a new name suggests it may also warrant a new binding- as well as the fact it has a lot more features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes are all included in this PR?
Any additional configuration needed for the DC8000 versus the DCNANO
You mean the update DPI mode right? It's in ede2ef3
Changes to the display_write function to support partial area updates when using the DC8000
I believe it shall be done by the DBI driver? Or you mean the lcdif DCNano driver shall support both DPI or DBI mode?
A new MIPI-DBI driver for the DC8000.
It's in c2f74b8
Any changes needed to the MIPI DSI driver to enable it to act as a bridge for the DCNANO (may not need any, because we do this on the RT500 already for video mode)
Its also added in c2f74b8 protected by CONFIG_MIPI_DSI macro. I think it is not appropriate to add it in the MIPI DSI driver since the operations are all very low level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes are all included in this PR?
Any additional configuration needed for the DC8000 versus the DCNANO
You mean the update DPI mode right? It's in ede2ef3
Yup, this looks good- keep in mind I'm not deeply familiar with this hardware, but I think this is the right approach.
Changes to the display_write function to support partial area updates when using the DC8000
I believe it shall be done by the DBI driver? Or you mean the lcdif DCNano driver shall support both DPI or DBI mode?
I think the lcdif DCNano driver should support video mode and command mode via the MIPI-DSI link. Essentially, I'd expect the display_write
implementation in the LCDIF DCNANO driver to support partial writes if using a command mode MIPI DSI display
A new MIPI-DBI driver for the DC8000.
It's in c2f74b8
Any changes needed to the MIPI DSI driver to enable it to act as a bridge for the DCNANO (may not need any, because we do this on the RT500 already for video mode)
Its also added in c2f74b8 protected by CONFIG_MIPI_DSI macro. I think it is not appropriate to add it in the MIPI DSI driver since the operations are all very low level.
I think the changes should be in the MIPI DSI driver- the DBI driver will only be use when we we are running the DCNANO with a display controller using 8080/6800 mode, and the DSI driver will be used when we use a display controller in MIPI DSI video/command mode, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When MIPI-DSI is used as the bridge, what should be done on the MIPI-DSI side is different for command mode from video mode. In video mode, once the MIPI-DSI is initialized, there is nothing more needed for frame update, but in command mode, for each write the DSI buffer/payload/send level has to be reconfigured. So if the lcdif DCNano driver is going to support the command mode, it just has to call those low level SDK driver APIs, like currently my new DCNANO MIPI-DBI driver does. And I think you are right, the DCNANO MIPI-DBI driver shall only handle 8080/6800 mode, and keep the low level MIPI-DSI operation all inside the MIPI-DSI driver. So I'm thinking to support DCNano command mode via the MIPI-DSI link, is it better to update the MIPI-DSI driver, let user to configure whether to use the DCNano command mode for acceleration, since for RT700 the G1120B0MIPI can still only use the MIPI-DSI despite the low speed. If DCNano command mode is used, the MIPI-DSI driver will call DCNANO MIPI-DBI APIs then those low level SDK driver APIs, instead of writing data to DSI APB buffer. In this case the RM67162 driver can still be used as the display driver for G1120B0MIPI.
drivers/display/display_mipi_dbi.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do displays actually exist that only require the DCS command set in order to initialize and display data? Every display I've ever encountered does use the DCS command set for writing to graphics RAM, but also requires a long set of vendor specific commands to set up support for the display properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To drive the panel G1120B0MIPI using both DC8000 and MIPI DSI, the driver display_rm67162.c can not be used, so I created this driver based on display_rm67162.c. I just updated the write and set_pixel_format by using mipi_dbi_write_display to send the data instead of mipi_dsi_dcs_write. Then set CONFIG_DISPLAY_MIPI_DBI=y and change the chosen zephyr display to display_mipi_dbi: 476b7bb#diff-1b9a0139b8cc287d78c4bb150fc04ccc161c82f8fea192fe9aa1ecc9a177c18aR12
The panel initilization function still is rm67162_init. This driver can also be used for panels similar to G1120B0MIPI, for example ZC143AC72MIPI, this panel is a replacement of G1120B0MIPI since it is discontinued.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what I'm confused on here is why we are using the DBI driver here under the hood at all. The RM67162 uses MIPI-DSI. Why can't the display_rm67162 driver be used on the RT700? Is is because the DC8000 is used to drive the display data? In that case (as I said in another comment) the existing RM67162 driver should be used for display init, then the actual display driver (which has display_write called on it) should implement support for the DC8000.
95f60b4
to
4756ad4
Compare
Should be DISPLAY_MCUX_DCNANO_LCDIF instead of DISPLAY_MCUX_ELCDIF. Signed-off-by: Kate Wang <[email protected]>
Update nxp,dcnano-lcdif to support IP change on RT700. There are extra registers need to be configured for the lcdif on RT700. Add new binding item "version" to tell which version of the IP the SoC has. Signed-off-by: Kate Wang <[email protected]>
Introduce NXP NCNano driver using MIPI DBI class. This peripheral supports 8080 and 6800 mode. The driver also supports used with nxp,mipi_dsi_2l driver, for the panel with DPHY bus, such as g1120b0mipi. Signed-off-by: Kate Wang <[email protected]>
…Nano DBI driver for memory write There is no smartdma on RT700, so to perform DCS memory write the CPU has to write APB buffer word by word. But the DCNano in DBI mode can interface with the MIPI-DSI, and send data to MIPI-DSI to transfer, once properly configured. Updated the MCUX MIPI-DSI driver to support using the NXP DCNano DBI driver. Also added new parameter first_write in display_buffer_descriptor to let MIPI_DCS_WRITE_MEMORY_START know to use MIPI_DCS_WRITE_MEMORY_START or MIPI_DCS_WRITE_MEMORY_CONTINUE. Signed-off-by: Kate Wang <[email protected]>
4756ad4
to
abd0e7c
Compare
1.Update nxp_dcnano_lcdif display driver
2.Add new mipi_dbi driver nxp_dcnano_lcdif_dbi
3.Add new display driver for panels which have bl pin and use DCS commands