| Message ID | 1548846209-16406-1-git-send-email-dkos@cadence.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: add support for Cadence MHDP DPI/DP bridge. | expand |
Hi, On 30/01/2019 13:03, Damian Kos wrote: > Hello! > > This is the series of patches that will add support for the Cadence's DPI/DP > bridge. Please note that this is a preliminary version of the driver and there > will be more patches in the future with updates, fixes and improvements. > Please keep that in mind when looking at FIXME/TODO/XXX comments. > > Initially, MHDP driver was developed as a DRM bridge driver and was planed to > be placed in drivers/gpu/drm/bridge/mhdp.c. However, there was already > a driver for Cadence's DP controller developed by RockChip, but that driver > uses the different DRM framework and looks like a part of a bigger system. > Both controllers (including firmware) are quite different internally > (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's > etc.) but they have similar register map, except for Framer/Streamer (which is > noticeably different), so they appear similar. > > The following patches contain: > - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and > modifying it a bit (mostly new prefixes for functions and data types) so it > can be used by two, higher level, drivers. > - Modifying existing RockChip's DP driver to use the common code after changes > made to it (use the new cdns_mhdp_device structure and new function names). > - Modifying DRM helpers a bit. Some are required for new driver, some are > updates from DP 1.2 to 1.3 or 1.4. > - Adding documentation for device tree bindings. > - Adding preliminary Cadence DPI/DP bridge driver. > > Some of the things that will be added later on include (but are not limited > to): > - DSC support > - FEC support > - HDCP support A few random comments/questions after a quick look at the patches. The names of the source files and the kernel Kconfig are only about "Cadence DP". But the DT bindings is for cdns,mhdp8546, and the resulting module file is mhdp8546.ko. I think more consistency here would be good. I presume the part number (or family? are there other similar parts with similar part numbers?) is relevant, so it should be in the Kconfig option and help text, and probably in the file names too. The module name should have "cdns" prefix there, similar to the source files and the cdns-dsi.ko. Or maybe the same driver will handle all Cadence DP parts, in which case generic filenames are fine, but then the resulting kernel module should also be just "cdns-mhdp.ko". I see some audio functions in the code, but it's not mentioned in the DT bindings. I'm not an audio guy, but the display bridges with audio support I have seen have had DT bindings for the audio source too. Is audio supported in the current driver? Tomi
Damian, ping. On 31/01/2019 14:08, Tomi Valkeinen wrote: > Hi, > > On 30/01/2019 13:03, Damian Kos wrote: >> Hello! >> >> This is the series of patches that will add support for the Cadence's DPI/DP >> bridge. Please note that this is a preliminary version of the driver and there >> will be more patches in the future with updates, fixes and improvements. >> Please keep that in mind when looking at FIXME/TODO/XXX comments. >> >> Initially, MHDP driver was developed as a DRM bridge driver and was planed to >> be placed in drivers/gpu/drm/bridge/mhdp.c. However, there was already >> a driver for Cadence's DP controller developed by RockChip, but that driver >> uses the different DRM framework and looks like a part of a bigger system. >> Both controllers (including firmware) are quite different internally >> (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's >> etc.) but they have similar register map, except for Framer/Streamer (which is >> noticeably different), so they appear similar. >> >> The following patches contain: >> - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and >> modifying it a bit (mostly new prefixes for functions and data types) so it >> can be used by two, higher level, drivers. >> - Modifying existing RockChip's DP driver to use the common code after changes >> made to it (use the new cdns_mhdp_device structure and new function names). >> - Modifying DRM helpers a bit. Some are required for new driver, some are >> updates from DP 1.2 to 1.3 or 1.4. >> - Adding documentation for device tree bindings. >> - Adding preliminary Cadence DPI/DP bridge driver. >> >> Some of the things that will be added later on include (but are not limited >> to): >> - DSC support >> - FEC support >> - HDCP support > > A few random comments/questions after a quick look at the patches. > > The names of the source files and the kernel Kconfig are only about > "Cadence DP". But the DT bindings is for cdns,mhdp8546, and the > resulting module file is mhdp8546.ko. I think more consistency here > would be good. > > I presume the part number (or family? are there other similar parts with > similar part numbers?) is relevant, so it should be in the Kconfig > option and help text, and probably in the file names too. The module > name should have "cdns" prefix there, similar to the source files and > the cdns-dsi.ko. > > Or maybe the same driver will handle all Cadence DP parts, in which case > generic filenames are fine, but then the resulting kernel module should > also be just "cdns-mhdp.ko". > > I see some audio functions in the code, but it's not mentioned in the DT > bindings. I'm not an audio guy, but the display bridges with audio > support I have seen have had DT bindings for the audio source too. Is > audio supported in the current driver? > > Tomi >
>Damian, ping. > >On 31/01/2019 14:08, Tomi Valkeinen wrote: >> Hi, >> >> On 30/01/2019 13:03, Damian Kos wrote: >>> Hello! >>> >>> This is the series of patches that will add support for the Cadence's DPI/DP >>> bridge. Please note that this is a preliminary version of the driver and there >>> will be more patches in the future with updates, fixes and improvements. >>> Please keep that in mind when looking at FIXME/TODO/XXX comments. >>> >>> Initially, MHDP driver was developed as a DRM bridge driver and was planed to >>> be placed in drivers/gpu/drm/bridge/mhdp.c. However, there was already >>> a driver for Cadence's DP controller developed by RockChip, but that driver >>> uses the different DRM framework and looks like a part of a bigger system. >>> Both controllers (including firmware) are quite different internally >>> (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's >>> etc.) but they have similar register map, except for Framer/Streamer (which is >>> noticeably different), so they appear similar. >>> >>> The following patches contain: >>> - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and >>> modifying it a bit (mostly new prefixes for functions and data types) so it >>> can be used by two, higher level, drivers. >>> - Modifying existing RockChip's DP driver to use the common code after changes >>> made to it (use the new cdns_mhdp_device structure and new function names). >>> - Modifying DRM helpers a bit. Some are required for new driver, some are >>> updates from DP 1.2 to 1.3 or 1.4. >>> - Adding documentation for device tree bindings. >>> - Adding preliminary Cadence DPI/DP bridge driver. >>> >>> Some of the things that will be added later on include (but are not limited >>> to): >>> - DSC support >>> - FEC support >>> - HDCP support >> >> A few random comments/questions after a quick look at the patches. >> >> The names of the source files and the kernel Kconfig are only about >> "Cadence DP". But the DT bindings is for cdns,mhdp8546, and the >> resulting module file is mhdp8546.ko. I think more consistency here >> would be good. >> >> I presume the part number (or family? are there other similar parts with >> similar part numbers?) is relevant, so it should be in the Kconfig >> option and help text, and probably in the file names too. The module >> name should have "cdns" prefix there, similar to the source files and >> the cdns-dsi.ko. >> >> Or maybe the same driver will handle all Cadence DP parts, in which case >> generic filenames are fine, but then the resulting kernel module should >> also be just "cdns-mhdp.ko". We would be very happy to have a separate driver for mhdp8546 instead of mixing it with Rockchip's driver. Unfortunately we need to have one driver for both IP's, so I'll just change mhdp8546.ko to cdns-mhdp.ko. (Unless, maintainers give us a green light for the dedicated driver?) >> >> I see some audio functions in the code, but it's not mentioned in the DT >> bindings. I'm not an audio guy, but the display bridges with audio >> support I have seen have had DT bindings for the audio source too. Is >> audio supported in the current driver? >> As far as I know, audio is working, but I can't double check it right now due to heavy load on my back right now. I'll try to find time for that in the next week. But, like I've mentioned earlier I don't see a reason why audio code (that exists in the mainline) wouldn't work. I'm not an audio guy myself. When updating dt bindings I took the dt bindings for other bridges as an example, but I did not see any bindings for audio in them. I didn't check all of them. Can you please let me know which bridges are you were referring to? In our test environment we didn't use any audio driver as audio generator is so simple that it is part of the register space of IP and we didn't even need to include this device in .dts file. Audio (like the video) is configured and enabled by code that is not included in the patch (for obvious reasons). So, in short, yes, driver supports audio and we will include description for audio input port in the dt bindings. >> Tomi >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki Regards, Damian -----Original Message----- From: Tomi Valkeinen <tomi.valkeinen@ti.com> Sent: Wednesday, March 20, 2019 10:34 To: Damian Kos <dkos@cadence.com> Cc: David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Andrzej Hajda <a.hajda@samsung.com>; Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <maxime.ripard@bootlin.com>; Sean Paul <sean@poorly.run>; Sandy Huang <hjc@rock-chips.com>; Heiko Stübner <heiko@sntech.de>; dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org; jbergsagel@ti.com; quentin.schulz@bootlin.com; Piotr Sroka <piotrs@cadence.com>; Rafal Ciepiela <rafalc@cadence.com> Subject: Re: [PATCH v7 0/4] drm: add support for Cadence MHDP DPI/DP bridge. EXTERNAL MAIL Damian, ping. On 31/01/2019 14:08, Tomi Valkeinen wrote: > Hi, > > On 30/01/2019 13:03, Damian Kos wrote: >> Hello! >> >> This is the series of patches that will add support for the Cadence's DPI/DP >> bridge. Please note that this is a preliminary version of the driver and there >> will be more patches in the future with updates, fixes and improvements. >> Please keep that in mind when looking at FIXME/TODO/XXX comments. >> >> Initially, MHDP driver was developed as a DRM bridge driver and was planed to >> be placed in drivers/gpu/drm/bridge/mhdp.c. However, there was already >> a driver for Cadence's DP controller developed by RockChip, but that driver >> uses the different DRM framework and looks like a part of a bigger system. >> Both controllers (including firmware) are quite different internally >> (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's >> etc.) but they have similar register map, except for Framer/Streamer (which is >> noticeably different), so they appear similar. >> >> The following patches contain: >> - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and >> modifying it a bit (mostly new prefixes for functions and data types) so it >> can be used by two, higher level, drivers. >> - Modifying existing RockChip's DP driver to use the common code after changes >> made to it (use the new cdns_mhdp_device structure and new function names). >> - Modifying DRM helpers a bit. Some are required for new driver, some are >> updates from DP 1.2 to 1.3 or 1.4. >> - Adding documentation for device tree bindings. >> - Adding preliminary Cadence DPI/DP bridge driver. >> >> Some of the things that will be added later on include (but are not limited >> to): >> - DSC support >> - FEC support >> - HDCP support > > A few random comments/questions after a quick look at the patches. > > The names of the source files and the kernel Kconfig are only about > "Cadence DP". But the DT bindings is for cdns,mhdp8546, and the > resulting module file is mhdp8546.ko. I think more consistency here > would be good. > > I presume the part number (or family? are there other similar parts with > similar part numbers?) is relevant, so it should be in the Kconfig > option and help text, and probably in the file names too. The module > name should have "cdns" prefix there, similar to the source files and > the cdns-dsi.ko. > > Or maybe the same driver will handle all Cadence DP parts, in which case > generic filenames are fine, but then the resulting kernel module should > also be just "cdns-mhdp.ko". > > I see some audio functions in the code, but it's not mentioned in the DT > bindings. I'm not an audio guy, but the display bridges with audio > support I have seen have had DT bindings for the audio source too. Is > audio supported in the current driver? > > Tomi >
On 27/03/2019 16:58, Damian Kos wrote: > We would be very happy to have a separate driver for mhdp8546 instead of > mixing it with Rockchip's driver. Unfortunately we need to have one driver > for both IP's, so I'll just change mhdp8546.ko to cdns-mhdp.ko. (Unless, > maintainers give us a green light for the dedicated driver?) Ok. I don't know about the differences, so I can't say much here. But I do think that you shouldn't combine drivers for two different IPs unless they are very similar. And if two different IPs have a block that's the same/similar, the code for that block can be extracted to a "library" used by both drivers. >>> I see some audio functions in the code, but it's not mentioned in the DT >>> bindings. I'm not an audio guy, but the display bridges with audio >>> support I have seen have had DT bindings for the audio source too. Is >>> audio supported in the current driver? >>> > As far as I know, audio is working, but I can't double check it right now > due to heavy load on my back right now. I'll try to find time for that in the > next week. But, like I've mentioned earlier I don't see a reason why audio > code (that exists in the mainline) wouldn't work. > I'm not an audio guy myself. When updating dt bindings I took the dt bindings > for other bridges as an example, but I did not see any bindings for audio in > them. I didn't check all of them. Can you please let me know which bridges > are you were referring to? In our test environment we didn't use any audio > driver as audio generator is so simple that it is part of the register space > of IP and we didn't even need to include this device in .dts file. Audio (like > the video) is configured and enabled by code that is not included in the patch > (for obvious reasons). > So, in short, yes, driver supports audio and we will include description for > audio input port in the dt bindings. Ok. Jyri Sarha <jsarha@ti.com> recently sent "drm/bridge: sii902x: HDMI-audio support and some fixes" series, and I think tda998x_drv.c also has some audio support. But I'm no audio guy either =). I was just curious about the status of the audio. Tomi
Hello! This is the series of patches that will add support for the Cadence's DPI/DP bridge. Please note that this is a preliminary version of the driver and there will be more patches in the future with updates, fixes and improvements. Please keep that in mind when looking at FIXME/TODO/XXX comments. Initially, MHDP driver was developed as a DRM bridge driver and was planed to be placed in drivers/gpu/drm/bridge/mhdp.c. However, there was already a driver for Cadence's DP controller developed by RockChip, but that driver uses the different DRM framework and looks like a part of a bigger system. Both controllers (including firmware) are quite different internally (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's etc.) but they have similar register map, except for Framer/Streamer (which is noticeably different), so they appear similar. The following patches contain: - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and modifying it a bit (mostly new prefixes for functions and data types) so it can be used by two, higher level, drivers. - Modifying existing RockChip's DP driver to use the common code after changes made to it (use the new cdns_mhdp_device structure and new function names). - Modifying DRM helpers a bit. Some are required for new driver, some are updates from DP 1.2 to 1.3 or 1.4. - Adding documentation for device tree bindings. - Adding preliminary Cadence DPI/DP bridge driver. Some of the things that will be added later on include (but are not limited to): - DSC support - FEC support - HDCP support Changes in v7: - Overal code cleanup - Patched memory leak after EDID read - Fixed SPDX-License-Identifier tags - Sorted includes - In patch 4/6: Cleanup and fix in mhdp_transfer Replaced byteshifts with {put|get}_unaligned_be{32|24|16} (added 24bit one) - In patch 5/6: Removed cdns_mhdp_mst_set_threshold (not needed anymore as HW does that) Updated cdns_mhdp_mst_set_rate_governing Replaced request_irq with devm_request_irq And then merged with 4/6 (as many changes in this patch altered the SST part) - In patch 6/6: Fix in checking devm_phy_get result Updated bindings doc And then merged with 4/6 (as this should be in the driver in the first place) Changes in v6: - Added Reviewed-bys for already reviewed patches - Dropped patch v5-0003 (that made dp_get_lane_status helper public) - Added patch with MST support - Added patch with Cadence SD0801 PHY init Changes in v5: - Fixed Kconfig in drm/rockchip again - Moved cdn-dp-reg.h (cdns-mhdp-common.h) to include/drm/bridge instead of drivers/gpu/drm/bridge/ - Updated the mhdp_validate_cr function Changes in v4: - Fixed Kconfig in drm/rockchip - Fixed Signed-offs - dp_link_status() is no longer public since it's used only in drm_dp_helper.c - Replaced EXTRA_CFLAGS with ccflags-y in drm/rockchip Makefile Changes in v3: - Corrected dt-bindings document - Enabled some clocks at startup (since FW doesn't do that anymore). - Changed Firmware file name to match the file on Linux Firmware repo. - Added SST audio support - Made common functions (in cdns-mhdp-common.*) public. Changes in v2: - Added actual description of what the patch contains, what is it for and what's going on here in general. - New structure. Now we have one common low level driver + two high level drivers - one for RockChip with minimum changes and one, more general, for Cadence. - Dropped some changes made to DRM helpers. - Updated the device tree bindings document. Damian Kos (1): drm/rockchip: prepare common code for cdns and rk dpi/dp driver Quentin Schulz (3): drm/dp: fix link probing for devices supporting DP 1.4+ dt-bindings: drm/bridge: Document Cadence MHDP bridge bindings drm: bridge: add support for Cadence MHDP DPI/DP bridge .../bindings/display/bridge/cdns,mhdp.txt | 47 + drivers/gpu/drm/bridge/Kconfig | 9 + drivers/gpu/drm/bridge/Makefile | 3 + drivers/gpu/drm/bridge/cdns-mhdp-common.c | 1103 ++++++++++++++ drivers/gpu/drm/bridge/cdns-mhdp-mst.c | 549 +++++++ drivers/gpu/drm/bridge/cdns-mhdp.c | 1349 +++++++++++++++++ drivers/gpu/drm/bridge/cdns-mhdp.h | 209 +++ drivers/gpu/drm/drm_dp_helper.c | 30 +- drivers/gpu/drm/rockchip/Kconfig | 4 +- drivers/gpu/drm/rockchip/Makefile | 2 +- drivers/gpu/drm/rockchip/cdn-dp-core.c | 234 +-- drivers/gpu/drm/rockchip/cdn-dp-core.h | 43 +- drivers/gpu/drm/rockchip/cdn-dp-reg.c | 969 ------------ include/drm/bridge/cdns-mhdp-cbs.h | 29 + .../drm/bridge/cdns-mhdp-common.h | 176 ++- 15 files changed, 3606 insertions(+), 1150 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp.txt create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp-common.c create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp-mst.c create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp.c create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp.h delete mode 100644 drivers/gpu/drm/rockchip/cdn-dp-reg.c create mode 100644 include/drm/bridge/cdns-mhdp-cbs.h rename drivers/gpu/drm/rockchip/cdn-dp-reg.h => include/drm/bridge/cdns-mhdp-common.h (77%)