| Message ID | 1530612152-27555-1-git-send-email-dkos@cadence.com |
|---|---|
| Headers | show |
| Series | drm: add support for Cadence MHDP DPI/DP bridge. | expand |
Hi Damien, it's very cool to see collaboration from vendors on this. Am Dienstag, 3. Juli 2018, 12:02:23 CEST schrieb Damian Kos: > It would be really nice to explain a bit about the added controller support in the commit message, so that people reviewing the patch can get a feeling for it. > Signed-off-by: Damian Kos <dkos@cadence.com> > --- > drivers/gpu/drm/rockchip/cdn-dp-core.c | 953 > +++++++++++++++++++++++++++++++- drivers/gpu/drm/rockchip/cdn-dp-core.h | > 25 + > drivers/gpu/drm/rockchip/cdn-dp-reg.c | 2 +- > drivers/gpu/drm/rockchip/cdn-dp-reg.h | 4 + >From the changes below, it looks that this seems to add support for a bridge chip based on that IP block. So it seems like the bridge+glue driver model would be a perfect fit for this, instead of stapling this onto the Rockchip-specific driver. So essentially, you could take the Rockchip cdn-dp driver, move the common parts to drivers/gpu/drm/bridge and then create separate glue drivers for both Rockchip and your external bridge IP block. This would prevent code duplication and also allow your bridge driver to be compiled without the Rockchip drm being present :-) . And also pave the way for future socs using your DP ip block. Nowadays we have quite a number of examples you could take as inspiration for this: - bridge/analogix/* (shared between Exynos and Rockchip right now) - bridge/synopsys/dw-hdmi* (shared between a quite big number of users) - bridge/synopsys/dw-mipi-dsi.c (shared between Rockchip [pending] and stm) Thanks Heiko -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Heiko, Thank you for your feedback! Initially, MHDP driver was developed as a DRM bridge driver and was planned to be placed in drivers/gpu/drm/bridge/cadence/mhdp.c. However, there was already a driver for Cadence's DP controller developed by RockChip, but that driver uses different DRM framework. Both controllers (including firmware) are quite different internally (MST/FEC/DSC support, link training done by driver, additional commands, etc.) but they have very similar register map, except for Framer/Streamer, so they appear similar. We would be more than happy to provide fully separate driver (that was basically pasted in RockChip's driver) for DP DRM bridge. Some parts can definitely shared between these two drivers like code for mailbox and commands sent/received by it, audio init. Moving cdn-dp-* to drivers/gpu/drm/bridge/ was also a plan, but it seems that cdn-dp-core.c use some stuff from drivers/gpu/drm/rockchip/*. cdn-dp-core driver in this case seems like a part of a bigger picture while the driver that we want to upstream is standalone. We'll move/add everything that can be shared by both drivers to drivers/gpu/drm/ and add new DPI/DP bridge driver as you advised and provide a new patch. PS. Should we post patches 01-08 done in DRM helper by Quentin as a separate patch, so that reviewers will not have to go through them every time when we send updated version? Regards, Damian -----Original Message----- From: Heiko Stübner <heiko@sntech.de> Sent: Tuesday, July 3, 2018 13:03 To: Damian Kos <dkos@cadence.com> Cc: David Airlie <airlied@linux.ie>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Gustavo Padovan <gustavo@padovan.org>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Sean Paul <seanpaul@chromium.org>; Sandy Huang <hjc@rock-chips.com>; 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; Lukasz Tyrala <ltyrala@cadence.com>; Przemyslaw Gaj <pgaj@cadence.com>; Scott Telford <stelford@cadence.com> Subject: Re: [PATCH 12/12] drm/rockchip: add support for CDNS MHDP IP controller. EXTERNAL MAIL Hi Damien, it's very cool to see collaboration from vendors on this. Am Dienstag, 3. Juli 2018, 12:02:23 CEST schrieb Damian Kos: > It would be really nice to explain a bit about the added controller support in the commit message, so that people reviewing the patch can get a feeling for it. > Signed-off-by: Damian Kos <dkos@cadence.com> > --- > drivers/gpu/drm/rockchip/cdn-dp-core.c | 953 > +++++++++++++++++++++++++++++++- > +++++++++++++++++++++++++++++++drivers/gpu/drm/rockchip/cdn-dp-core.h > +++++++++++++++++++++++++++++++| > 25 + > drivers/gpu/drm/rockchip/cdn-dp-reg.c | 2 +- > drivers/gpu/drm/rockchip/cdn-dp-reg.h | 4 + >From the changes below, it looks that this seems to add support for a bridge chip based on that IP block. So it seems like the bridge+glue driver model would be a perfect fit for this, instead of stapling this onto the Rockchip-specific driver. So essentially, you could take the Rockchip cdn-dp driver, move the common parts to drivers/gpu/drm/bridge and then create separate glue drivers for both Rockchip and your external bridge IP block. This would prevent code duplication and also allow your bridge driver to be compiled without the Rockchip drm being present :-) . And also pave the way for future socs using your DP ip block. Nowadays we have quite a number of examples you could take as inspiration for this: - bridge/analogix/* (shared between Exynos and Rockchip right now) - bridge/synopsys/dw-hdmi* (shared between a quite big number of users) - bridge/synopsys/dw-mipi-dsi.c (shared between Rockchip [pending] and stm) Thanks Heiko -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 03, 2018 at 11:02:14AM +0100, Damian Kos wrote: > From: Quentin Schulz <quentin.schulz@free-electrons.com> > > We already have functions to get the adjust request voltage and > pre-emphasis for a lane so it makes also sense to be able to set them so > that we can then easily update them via a DPCD write. > > Add helpers for drm_dp_set_adjust_request_pre_emphasis and > drm_dp_set_adjust_request_voltage that respectively set the > pre-emphasis and voltage of a lane in the link status array. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > Signed-off-by: Damian Kos <dkos@cadence.com> Hm usually this is source dependent - some sources only have one adj/pre-emph value for all lanes, some only 2 (for groups of 2), some for all four. That's kinda why we don't have helpers for this stuff. An excellent way to show that your new helpers are useful would be to go through existing drivers and convert them over, where it makes sense. Same kinda holds for patch 1. Thanks, Daniel > --- > drivers/gpu/drm/drm_dp_helper.c | 28 ++++++++++++++++++++++++++++ > include/drm/drm_dp_helper.h | 4 ++++ > 2 files changed, 32 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 3bc2e98..ca2f469 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -120,6 +120,34 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SI > } > EXPORT_SYMBOL(drm_dp_get_adjust_request_pre_emphasis); > > +void drm_dp_set_adjust_request_voltage(u8 link_status[DP_LINK_STATUS_SIZE], > + int lane, u8 volt) > +{ > + int i = DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1); > + int s = ((lane & 1) ? > + DP_ADJUST_VOLTAGE_SWING_LANE1_SHIFT : > + DP_ADJUST_VOLTAGE_SWING_LANE0_SHIFT); > + int idx = i - DP_LANE0_1_STATUS; > + > + link_status[idx] &= ~(DP_ADJUST_VOLTAGE_SWING_LANE0_MASK << s); > + link_status[idx] |= volt << s; > +} > +EXPORT_SYMBOL(drm_dp_set_adjust_request_voltage); > + > +void drm_dp_set_adjust_request_pre_emphasis(u8 link_status[DP_LINK_STATUS_SIZE], > + int lane, u8 pre_emphasis) > +{ > + int i = DP_ADJUST_REQUEST_LANE0_1 + (lane >> 1); > + int s = ((lane & 1) ? > + DP_ADJUST_PRE_EMPHASIS_LANE1_SHIFT : > + DP_ADJUST_PRE_EMPHASIS_LANE0_SHIFT); > + int idx = i - DP_LANE0_1_STATUS; > + > + link_status[idx] &= ~(DP_ADJUST_PRE_EMPHASIS_LANE0_MASK << s); > + link_status[idx] |= pre_emphasis << s; > +} > +EXPORT_SYMBOL(drm_dp_set_adjust_request_pre_emphasis); > + > void drm_dp_link_train_clock_recovery_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { > if (dpcd[DP_TRAINING_AUX_RD_INTERVAL] == 0) > udelay(100); > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index a488af0..6e64b2a 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -946,6 +946,10 @@ u8 drm_dp_get_adjust_request_voltage(const u8 link_status[DP_LINK_STATUS_SIZE], > int lane); > u8 drm_dp_get_adjust_request_pre_emphasis(const u8 link_status[DP_LINK_STATUS_SIZE], > int lane); > +void drm_dp_set_adjust_request_voltage(u8 link_status[DP_LINK_STATUS_SIZE], > + int lane, u8 volt); > +void drm_dp_set_adjust_request_pre_emphasis(u8 link_status[DP_LINK_STATUS_SIZE], > + int lane, u8 pre_emphasis); > > #define DP_BRANCH_OUI_HEADER_SIZE 0xc > #define DP_RECEIVER_CAP_SIZE 0xf > -- > 1.7.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Damian Kos (3): dt-bindings: drm/bridge: Document Cadence MHDP bridge bindings drm/rockchip: added implementation for a few FW commands. drm/rockchip: add support for CDNS MHDP IP controller. Quentin Schulz (9): HACK: increase timeout for drm_atomic_helper_wait_for_vblanks drm/dp: make dp_link_status and dp_get_lane_status usable from outside of the core drm/dp: add helpers for drm_dp_set_adjust_request_pre_emphasis and drm_dp_set_adjust_request_voltage drm/dp: fix training interval formula for DP 1.3+ drm/dp: fix link probing for devices supporting DP 1.4+ drm/dp: fix drm_dp_link_power_* for DP 1.2+ drm/dp: fix drm_dp_link_train_clock_recovery_delay for DP 1.4 drm/dp: add max number of lanes supported drm/dp: add pixel encoding and colorimetry format indicator field in MISC1 .../bindings/display/bridge/cdns,mhdp.txt | 39 + drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_dp_helper.c | 97 ++- drivers/gpu/drm/rockchip/cdn-dp-core.c | 953 +++++++++++++++++++- drivers/gpu/drm/rockchip/cdn-dp-core.h | 25 + drivers/gpu/drm/rockchip/cdn-dp-reg.c | 167 ++++- drivers/gpu/drm/rockchip/cdn-dp-reg.h | 147 +++- include/drm/drm_dp_helper.h | 14 +- 8 files changed, 1400 insertions(+), 44 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp.txt -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html