RE: [EXT] Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
> -Original Message-
> From: Laurent Pinchart
> Sent: Wednesday, June 3, 2020 7:29 AM
> To: Emil Velikov
> Cc: Sandor Yu ; Andrzej Hajda ;
> Neil Armstrong ; Jonas Karlman
> ; Jernej Skrabec ; Heiko Stübner
> ; Sandy Huang ;
> d...@cadence.com; ML dri-devel ;
> linux-rockchip ; Linux-Kernel@Vger.
> Kernel. Org ; LAKML
> ; dl-linux-imx
> Subject: [EXT] Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns
> and rk dpi/dp driver
>
> Caution: EXT Email
>
> On Tue, Jun 02, 2020 at 02:55:52PM +0100, Emil Velikov wrote:
> > On Mon, 1 Jun 2020 at 07:29, wrote:
> > >
> > > From: Sandor Yu
> > >
> > > - Extracted common fields from cdn_dp_device to a new
> cdns_mhdp_device
> > > structure which will be used by two separate drivers later on.
> > > - Moved some datatypes (audio_format, audio_info,
> vic_pxl_encoding_format,
> > > video_info) from cdn-dp-core.c to cdn-dp-reg.h.
> > > - Changed prefixes from cdn_dp to cdns_mhdp
> > > cdn -> cdns to match the other Cadence's drivers
> > > dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
> > > this registers map can be a HDMI (which is internally different,
> > > but the interface for commands, events is pretty much the same).
> > > - Modified cdn-dp-core.c to use the new driver structure and new function
> > > names.
> > > - writel and readl are replaced by cdns_mhdp_bus_write and
> > > cdns_mhdp_bus_read.
> > >
> > The high-level idea is great - split, refactor and reuse the existing
> > drivers.
> >
> > Although looking at the patches themselves - they seems to be doing
> > multiple things at once.
> > As indicated by the extensive list in the commit log.
> >
> > I would suggest splitting those up a bit, roughly in line of the
> > itemisation as per the commit message.
> >
> > Here is one hand wavy way to chunk this patch:
> > 1) use put_unalligned*
> > 2) 'use local variable dev' style of changes (as seem in
> > cdn_dp_clk_enable)
> > 3) add writel/readl wrappers
> > 4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal.
> > The cdn-dp-reg.h function names/signatures will stay the same.
> > 5) finalize the helpers - use mhdp directly, rename
>
> I second this, otherwise review is very hard.
>
I will split the patch later, thanks.
> > Examples:
> > 4)
> > static int cdn_dp_mailbox_read(struct cdn_dp_device *dp) {
> > +" struct cdns_mhdp_device *mhdp = dp->mhdp;
> >int val, ret;
> >
> > - ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
> > + ret = readx_poll_timeout(readl, mhdp->regs_base +
> > + MAILBOX_EMPTY_ADDR,
> > ...
> >return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff; }
> >
> > 5)
> > -static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
> > +static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
> > {
> > - struct cdns_mhdp_device *mhdp = dp->mhdp;
> >int val, ret;
> > ...
> > - return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
> > + return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
> > }
>
> --
> Regards,
>
> Laurent Pinchart
Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
On Tue, Jun 02, 2020 at 02:55:52PM +0100, Emil Velikov wrote:
> On Mon, 1 Jun 2020 at 07:29, wrote:
> >
> > From: Sandor Yu
> >
> > - Extracted common fields from cdn_dp_device to a new cdns_mhdp_device
> > structure which will be used by two separate drivers later on.
> > - Moved some datatypes (audio_format, audio_info, vic_pxl_encoding_format,
> > video_info) from cdn-dp-core.c to cdn-dp-reg.h.
> > - Changed prefixes from cdn_dp to cdns_mhdp
> > cdn -> cdns to match the other Cadence's drivers
> > dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
> > this registers map can be a HDMI (which is internally different,
> > but the interface for commands, events is pretty much the same).
> > - Modified cdn-dp-core.c to use the new driver structure and new function
> > names.
> > - writel and readl are replaced by cdns_mhdp_bus_write and
> > cdns_mhdp_bus_read.
> >
> The high-level idea is great - split, refactor and reuse the existing drivers.
>
> Although looking at the patches themselves - they seems to be doing
> multiple things at once.
> As indicated by the extensive list in the commit log.
>
> I would suggest splitting those up a bit, roughly in line of the
> itemisation as per the commit message.
>
> Here is one hand wavy way to chunk this patch:
> 1) use put_unalligned*
> 2) 'use local variable dev' style of changes (as seem in cdn_dp_clk_enable)
> 3) add writel/readl wrappers
> 4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal.
> The cdn-dp-reg.h function names/signatures will stay the same.
> 5) finalize the helpers - use mhdp directly, rename
I second this, otherwise review is very hard.
> Examples:
> 4)
> static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
> {
> +" struct cdns_mhdp_device *mhdp = dp->mhdp;
>int val, ret;
>
> - ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
> + ret = readx_poll_timeout(readl, mhdp->regs_base + MAILBOX_EMPTY_ADDR,
> ...
>return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
> }
>
> 5)
> -static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
> +static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
> {
> - struct cdns_mhdp_device *mhdp = dp->mhdp;
>int val, ret;
> ...
> - return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
> + return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
HI Sandor Yu
On Mon, 1 Jun 2020 at 07:29, wrote:
>
> From: Sandor Yu
>
> - Extracted common fields from cdn_dp_device to a new cdns_mhdp_device
> structure which will be used by two separate drivers later on.
> - Moved some datatypes (audio_format, audio_info, vic_pxl_encoding_format,
> video_info) from cdn-dp-core.c to cdn-dp-reg.h.
> - Changed prefixes from cdn_dp to cdns_mhdp
> cdn -> cdns to match the other Cadence's drivers
> dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
> this registers map can be a HDMI (which is internally different,
> but the interface for commands, events is pretty much the same).
> - Modified cdn-dp-core.c to use the new driver structure and new function
> names.
> - writel and readl are replaced by cdns_mhdp_bus_write and
> cdns_mhdp_bus_read.
>
The high-level idea is great - split, refactor and reuse the existing drivers.
Although looking at the patches themselves - they seems to be doing
multiple things at once.
As indicated by the extensive list in the commit log.
I would suggest splitting those up a bit, roughly in line of the
itemisation as per the commit message.
Here is one hand wavy way to chunk this patch:
1) use put_unalligned*
2) 'use local variable dev' style of changes (as seem in cdn_dp_clk_enable)
3) add writel/readl wrappers
4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal.
The cdn-dp-reg.h function names/signatures will stay the same.
5) finalize the helpers - use mhdp directly, rename
HTH
Emil
Examples:
4)
static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
{
+" struct cdns_mhdp_device *mhdp = dp->mhdp;
int val, ret;
- ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
+ ret = readx_poll_timeout(readl, mhdp->regs_base + MAILBOX_EMPTY_ADDR,
...
return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
}
5)
-static int cdn_dp_mailbox_read(struct cdn_dp_device *dp)
+static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
{
- struct cdns_mhdp_device *mhdp = dp->mhdp;
int val, ret;
...
- return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
+ return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
}
[PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
From: Sandor Yu
- Extracted common fields from cdn_dp_device to a new cdns_mhdp_device
structure which will be used by two separate drivers later on.
- Moved some datatypes (audio_format, audio_info, vic_pxl_encoding_format,
video_info) from cdn-dp-core.c to cdn-dp-reg.h.
- Changed prefixes from cdn_dp to cdns_mhdp
cdn -> cdns to match the other Cadence's drivers
dp -> mhdp to distinguish it from a "just a DP" as the IP underneath
this registers map can be a HDMI (which is internally different,
but the interface for commands, events is pretty much the same).
- Modified cdn-dp-core.c to use the new driver structure and new function
names.
- writel and readl are replaced by cdns_mhdp_bus_write and
cdns_mhdp_bus_read.
Signed-off-by: Damian Kos
Signed-off-by: Sandor Yu
---
drivers/gpu/drm/rockchip/cdn-dp-core.c | 240 ++--
drivers/gpu/drm/rockchip/cdn-dp-core.h | 44 +--
drivers/gpu/drm/rockchip/cdn-dp-reg.c | 488 +
drivers/gpu/drm/rockchip/cdn-dp-reg.h | 133 +--
4 files changed, 483 insertions(+), 422 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c
b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index eed594bd38d3..b6aa21779608 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -26,7 +26,7 @@
#include "rockchip_drm_vop.h"
#define connector_to_dp(c) \
- container_of(c, struct cdn_dp_device, connector)
+ container_of(c, struct cdn_dp_device, mhdp.connector.base)
#define encoder_to_dp(c) \
container_of(c, struct cdn_dp_device, encoder)
@@ -61,17 +61,18 @@ MODULE_DEVICE_TABLE(of, cdn_dp_dt_ids);
static int cdn_dp_grf_write(struct cdn_dp_device *dp,
unsigned int reg, unsigned int val)
{
+ struct device *dev = dp->mhdp.dev;
int ret;
ret = clk_prepare_enable(dp->grf_clk);
if (ret) {
- DRM_DEV_ERROR(dp->dev, "Failed to prepare_enable grf clock\n");
+ DRM_DEV_ERROR(dev, "Failed to prepare_enable grf clock\n");
return ret;
}
ret = regmap_write(dp->grf, reg, val);
if (ret) {
- DRM_DEV_ERROR(dp->dev, "Could not write to GRF: %d\n", ret);
+ DRM_DEV_ERROR(dev, "Could not write to GRF: %d\n", ret);
return ret;
}
@@ -82,24 +83,25 @@ static int cdn_dp_grf_write(struct cdn_dp_device *dp,
static int cdn_dp_clk_enable(struct cdn_dp_device *dp)
{
+ struct device *dev = dp->mhdp.dev;
int ret;
unsigned long rate;
ret = clk_prepare_enable(dp->pclk);
if (ret < 0) {
- DRM_DEV_ERROR(dp->dev, "cannot enable dp pclk %d\n", ret);
+ DRM_DEV_ERROR(dev, "cannot enable dp pclk %d\n", ret);
goto err_pclk;
}
ret = clk_prepare_enable(dp->core_clk);
if (ret < 0) {
- DRM_DEV_ERROR(dp->dev, "cannot enable core_clk %d\n", ret);
+ DRM_DEV_ERROR(dev, "cannot enable core_clk %d\n", ret);
goto err_core_clk;
}
- ret = pm_runtime_get_sync(dp->dev);
+ ret = pm_runtime_get_sync(dev);
if (ret < 0) {
- DRM_DEV_ERROR(dp->dev, "cannot get pm runtime %d\n", ret);
+ DRM_DEV_ERROR(dev, "cannot get pm runtime %d\n", ret);
goto err_pm_runtime_get;
}
@@ -112,18 +114,18 @@ static int cdn_dp_clk_enable(struct cdn_dp_device *dp)
rate = clk_get_rate(dp->core_clk);
if (!rate) {
- DRM_DEV_ERROR(dp->dev, "get clk rate failed\n");
+ DRM_DEV_ERROR(dev, "get clk rate failed\n");
ret = -EINVAL;
goto err_set_rate;
}
- cdn_dp_set_fw_clk(dp, rate);
- cdn_dp_clock_reset(dp);
+ cdns_mhdp_set_fw_clk(>mhdp, rate);
+ cdns_mhdp_clock_reset(>mhdp);
return 0;
err_set_rate:
- pm_runtime_put(dp->dev);
+ pm_runtime_put(dev);
err_pm_runtime_get:
clk_disable_unprepare(dp->core_clk);
err_core_clk:
@@ -134,7 +136,7 @@ static int cdn_dp_clk_enable(struct cdn_dp_device *dp)
static void cdn_dp_clk_disable(struct cdn_dp_device *dp)
{
- pm_runtime_put_sync(dp->dev);
+ pm_runtime_put_sync(dp->mhdp.dev);
clk_disable_unprepare(dp->pclk);
clk_disable_unprepare(dp->core_clk);
}
@@ -167,7 +169,7 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device *dp,
u8 *sink_count)
u8 value;
*sink_count = 0;
- ret = cdn_dp_dpcd_read(dp, DP_SINK_COUNT, , 1);
+ ret = drm_dp_dpcd_read(>mhdp.dp.aux, DP_SINK_COUNT, , 1);
if (ret)
return ret;
@@ -191,12 +193,13 @@ static struct cdn_dp_port *cdn_dp_connected_port(struct
cdn_dp_device *dp)
static bool cdn_dp_check_sink_connection(struct cdn_dp_device *dp)
{
+ struct device *dev = dp->mhdp.dev;
unsigned long timeout =
