Hi Philipp,
On 5 May 2017 at 13:48, Philipp Tomsich
<philipp.toms...@theobroma-systems.com> wrote:
> This commit enables the RK3399 HDMI TX, which is very similar to the
> one found on the RK3288. As requested by Simon, this splits the HDMI
> driver into a SOC-specific portion (rk3399_hdmi.c, rk3288_hdmi.c) and
> a common portion (rk_hdmi.c).
>
> Note that the I2C communication for reading the EDID works well with
> the default settings, but does not with the alternate settings used on
> the RK3288... so this configuration aspect also is part of the
> driverdata.
>
> Having some sort of DTS-based configuration for the regulator
> dependencies would be nice for the future, but for now we simply use
> lists of regulator names (also via driverdata) that we probe.
>
> Signed-off-by: Philipp Tomsich <philipp.toms...@theobroma-systems.com>
> ---
>
> Changes in v3:
> - split into separate drivers for the SOC-specific portion of the driver
> - rebase to sjg/next
>
> Changes in v2:
> - removed DEBUG from the patch (as was done in our production branches,
> but missing from the patch-prep branch)
> - updated SJG's comment (with a TODO for the RK3288) to reflect the
> new code structure
>
> arch/arm/include/asm/arch-rockchip/grf_rk3399.h | 3 +
> drivers/video/rockchip/Makefile | 4 +-
> drivers/video/rockchip/rk3288_hdmi.c | 116
> ++++++++++++++++++++++++
> drivers/video/rockchip/rk3399_hdmi.c | 81 +++++++++++++++++
> drivers/video/rockchip/rk_hdmi.c | 114 +++++------------------
> drivers/video/rockchip/rk_hdmi.h | 32 +++++++
> 6 files changed, 260 insertions(+), 90 deletions(-)
> create mode 100644 drivers/video/rockchip/rk3288_hdmi.c
> create mode 100644 drivers/video/rockchip/rk3399_hdmi.c
> create mode 100644 drivers/video/rockchip/rk_hdmi.h
Again this patch does too much. Please can you split out the refactor
(moving the rk3288 code into its own file) from the patch that adds
the new feature?
>
> diff --git a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h
> b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h
> index eda9956..8d21eb7 100644
> --- a/arch/arm/include/asm/arch-rockchip/grf_rk3399.h
> +++ b/arch/arm/include/asm/arch-rockchip/grf_rk3399.h
> @@ -534,6 +534,9 @@ enum {
> GRF_DSI0_VOP_SEL_MASK = 1 << GRF_DSI0_VOP_SEL_SHIFT,
> GRF_DSI0_VOP_SEL_B = 0,
> GRF_DSI0_VOP_SEL_L = 1,
> + GRF_RK3399_HDMI_VOP_SEL_MASK = 1 << 6,
> + GRF_RK3399_HDMI_VOP_SEL_B = 0 << 6,
> + GRF_RK3399_HDMI_VOP_SEL_L = 1 << 6,
>
> /* GRF_SOC_CON22 */
> GRF_DPHY_TX0_RXMODE_SHIFT = 0,
> diff --git a/drivers/video/rockchip/Makefile b/drivers/video/rockchip/Makefile
> index 495d5f7..872dc0f 100644
> --- a/drivers/video/rockchip/Makefile
> +++ b/drivers/video/rockchip/Makefile
> @@ -11,6 +11,8 @@ obj-$(CONFIG_ROCKCHIP_RK3288) += rk3288_vop.o
> obj-$(CONFIG_ROCKCHIP_RK3399) += rk3399_vop.o
> obj-$(CONFIG_DISPLAY_ROCKCHIP_EDP) += rk_edp.o
> obj-$(CONFIG_DISPLAY_ROCKCHIP_LVDS) += rk_lvds.o
> -obj-$(CONFIG_DISPLAY_ROCKCHIP_HDMI) += rk_hdmi.o
> +obj-hdmi-$(CONFIG_ROCKCHIP_RK3288) += rk3288_hdmi.o
> +obj-hdmi-$(CONFIG_ROCKCHIP_RK3399) += rk3399_hdmi.o
> +obj-$(CONFIG_DISPLAY_ROCKCHIP_HDMI) += rk_hdmi.o $(obj-hdmi-y)
> obj-$(CONFIG_DISPLAY_ROCKCHIP_MIPI) += rk_mipi.o
> endif
> diff --git a/drivers/video/rockchip/rk3288_hdmi.c
> b/drivers/video/rockchip/rk3288_hdmi.c
> new file mode 100644
> index 0000000..eae0dd2
> --- /dev/null
> +++ b/drivers/video/rockchip/rk3288_hdmi.c
> @@ -0,0 +1,116 @@
> +/*
> + * Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <display.h>
> +#include <dm.h>
> +#include <dw_hdmi.h>
> +#include <edid.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/grf_rk3288.h>
> +#include <power/regulator.h>
> +#include "rk_hdmi.h"
We may need all these headers but please drop anything that are not needed.
> +
> +static int rk3288_hdmi_enable(struct udevice *dev, int panel_bpp,
> + const struct display_timing *edid)
> +{
> + struct rk_hdmi_priv *priv = dev_get_priv(dev);
> + struct display_plat *uc_plat = dev_get_uclass_platdata(dev);
> + int vop_id = uc_plat->source_id;
> + struct rk3288_grf *grf = priv->grf;
> +
> + /* hdmi source select hdmi controller */
> + rk_setreg(&grf->soc_con6, 1 << 15);
> +
> + /* hdmi data from vop id */
> + rk_clrsetreg(&grf->soc_con6, 1 << 4, (vop_id == 1) ? (1 << 4) : 0);
> +
> + return 0;
> +}
> +
> +static int rk3288_hdmi_ofdata_to_platdata(struct udevice *dev)
> +{
> + struct rk_hdmi_priv *priv = dev_get_priv(dev);
> + struct dw_hdmi *hdmi = &priv->hdmi;
> +
> + hdmi->i2c_clk_high = 0x7a;
> + hdmi->i2c_clk_low = 0x8d;
> +
> + /*
> + * TODO(s...@chromium.org): The above values don't work - these
> + * ones work better, but generate lots of errors in the data.
> + */
> + hdmi->i2c_clk_high = 0x0d;
> + hdmi->i2c_clk_low = 0x0d;
> +
> + return rk_hdmi_ofdata_to_platdata(dev);
> +}
> +
> +static int rk3288_clk_config(struct udevice *dev)
> +{
> + struct display_plat *uc_plat = dev_get_uclass_platdata(dev);
> + struct clk clk;
> + int ret;
> +
> + /*
> + * Configure the maximum clock to permit whatever resolution the
> + * monitor wants
> + */
> + ret = clk_get_by_index(uc_plat->src_dev, 0, &clk);
> + if (ret >= 0) {
> + ret = clk_set_rate(&clk, 384000000);
> + clk_free(&clk);
> + }
> + if (ret < 0) {
> + debug("%s: Failed to set clock in source device '%s':
> ret=%d\n",
> + __func__, uc_plat->src_dev->name, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const char * const rk3288_regulator_names[] = {
> + "vcc50_hdmi"
> +};
> +
> +static int rk3288_hdmi_probe(struct udevice *dev)
> +{
> + /* Enable VOP clock for RK3288 */
> + rk3288_clk_config(dev);
> +
> + /* Enable regulators required for HDMI */
> + rk_hdmi_probe_regulators(dev, rk3288_regulator_names,
> + ARRAY_SIZE(rk3288_regulator_names));
> +
> + return rk_hdmi_probe(dev);
> +}
> +
> +static const struct dm_display_ops rk3288_hdmi_ops = {
> + .read_edid = rk_hdmi_read_edid,
> + .enable = rk3288_hdmi_enable,
> +};
> +
> +static const struct udevice_id rk3288_hdmi_ids[] = {
> + { .compatible = "rockchip,rk3288-dw-hdmi" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(rk3288_hdmi_rockchip) = {
> + .name = "rk3288_hdmi_rockchip",
> + .id = UCLASS_DISPLAY,
> + .of_match = rk3288_hdmi_ids,
> + .ops = &rk3288_hdmi_ops,
> + .ofdata_to_platdata = rk3288_hdmi_ofdata_to_platdata,
> + .probe = rk3288_hdmi_probe,
> + .priv_auto_alloc_size = sizeof(struct rk_hdmi_priv),
> +};
> diff --git a/drivers/video/rockchip/rk3399_hdmi.c
> b/drivers/video/rockchip/rk3399_hdmi.c
> new file mode 100644
> index 0000000..b1e5097
> --- /dev/null
> +++ b/drivers/video/rockchip/rk3399_hdmi.c
> @@ -0,0 +1,81 @@
> +/*
> + * Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <display.h>
> +#include <dm.h>
> +#include <dw_hdmi.h>
> +#include <edid.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/grf_rk3399.h>
> +#include <power/regulator.h>
> +#include "rk_hdmi.h"
> +
> +static int rk3399_hdmi_enable(struct udevice *dev, int panel_bpp,
> + const struct display_timing *edid)
> +{
> + struct rk_hdmi_priv *priv = dev_get_priv(dev);
> + struct display_plat *uc_plat = dev_get_uclass_platdata(dev);
> + int vop_id = uc_plat->source_id;
> + struct rk3399_grf_regs *grf = priv->grf;
> +
> + /* select the hdmi encoder input data from our source_id */
> + rk_clrsetreg(&grf->soc_con20, GRF_RK3399_HDMI_VOP_SEL_MASK,
> + (vop_id == 1) ? GRF_RK3399_HDMI_VOP_SEL_L : 0);
> +
> + return dw_hdmi_enable(&priv->hdmi, edid);
> +}
> +
> +static int rk3399_hdmi_ofdata_to_platdata(struct udevice *dev)
> +{
> + struct rk_hdmi_priv *priv = dev_get_priv(dev);
> + struct dw_hdmi *hdmi = &priv->hdmi;
> +
> + hdmi->i2c_clk_high = 0x7a;
> + hdmi->i2c_clk_low = 0x8d;
> +
> + return rk_hdmi_ofdata_to_platdata(dev);
> +}
> +
> +static const char * const rk3399_regulator_names[] = {
> + "vcc1v8_hdmi",
> + "vcc0v9_hdmi"
> +};
> +
> +static int rk3399_hdmi_probe(struct udevice *dev)
> +{
> + /* Enable regulators required for HDMI */
> + rk_hdmi_probe_regulators(dev, rk3399_regulator_names,
> + ARRAY_SIZE(rk3399_regulator_names));
> +
> + return rk_hdmi_probe(dev);
> +}
> +
> +static const struct dm_display_ops rk3399_hdmi_ops = {
> + .read_edid = rk_hdmi_read_edid,
> + .enable = rk3399_hdmi_enable,
> +};
> +
> +static const struct udevice_id rk3399_hdmi_ids[] = {
> + { .compatible = "rockchip,rk3399-dw-hdmi" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(rk3399_hdmi_rockchip) = {
> + .name = "rk3399_hdmi_rockchip",
> + .id = UCLASS_DISPLAY,
> + .of_match = rk3399_hdmi_ids,
> + .ops = &rk3399_hdmi_ops,
> + .ofdata_to_platdata = rk3399_hdmi_ofdata_to_platdata,
> + .probe = rk3399_hdmi_probe,
> + .priv_auto_alloc_size = sizeof(struct rk_hdmi_priv),
> +};
> diff --git a/drivers/video/rockchip/rk_hdmi.c
> b/drivers/video/rockchip/rk_hdmi.c
> index db07588..25f84a7 100644
> --- a/drivers/video/rockchip/rk_hdmi.c
> +++ b/drivers/video/rockchip/rk_hdmi.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
> * Copyright (c) 2015 Google, Inc
> * Copyright 2014 Rockchip Inc.
> *
> @@ -16,13 +17,9 @@
> #include <asm/gpio.h>
> #include <asm/io.h>
> #include <asm/arch/clock.h>
> -#include <asm/arch/grf_rk3288.h>
> -#include <power/regulator.h>
> -
> -struct rk_hdmi_priv {
> - struct dw_hdmi hdmi;
> - struct rk3288_grf *grf;
> -};
> +#include <asm/arch/hardware.h>
> +#include "rk_hdmi.h"
> +#include "rk_vop.h" /* for rk_vop_probe_regulators */
>
> static const struct hdmi_phy_config rockchip_phy_config[] = {
> {
> @@ -35,6 +32,9 @@ static const struct hdmi_phy_config rockchip_phy_config[] =
> {
> .mpixelclock = 297000000,
> .sym_ctr = 0x8039, .term = 0x0005, .vlev_ctr = 0x028d,
> }, {
> + .mpixelclock = 584000000,
> + .sym_ctr = 0x8039, .term = 0x0000, .vlev_ctr = 0x019d,
> + }, {
> .mpixelclock = ~0ul,
> .sym_ctr = 0x0000, .term = 0x0000, .vlev_ctr = 0x0000,
> }
> @@ -60,27 +60,25 @@ static const struct hdmi_mpll_config rockchip_mpll_cfg[]
> = {
> .mpixelclock = 148500000,
> .cpce = 0x0051, .gmp = 0x0003, .curr = 0x0000,
> }, {
> + .mpixelclock = 272000000,
> + .cpce = 0x0040, .gmp = 0x0003, .curr = 0x0000,
> + }, {
> + .mpixelclock = 340000000,
> + .cpce = 0x0040, .gmp = 0x0003, .curr = 0x0000,
> + }, {
> .mpixelclock = ~0ul,
> .cpce = 0x0051, .gmp = 0x0003, .curr = 0x0000,
> }
> };
>
> -static int rk_hdmi_read_edid(struct udevice *dev, u8 *buf, int buf_size)
> +int rk_hdmi_read_edid(struct udevice *dev, u8 *buf, int buf_size)
> {
> struct rk_hdmi_priv *priv = dev_get_priv(dev);
>
> return dw_hdmi_read_edid(&priv->hdmi, buf, buf_size);
> }
>
> -static int rk_hdmi_enable(struct udevice *dev, int panel_bpp,
> - const struct display_timing *edid)
> -{
> - struct rk_hdmi_priv *priv = dev_get_priv(dev);
> -
> - return dw_hdmi_enable(&priv->hdmi, edid);
> -}
> -
> -static int rk_hdmi_ofdata_to_platdata(struct udevice *dev)
> +int rk_hdmi_ofdata_to_platdata(struct udevice *dev)
> {
> struct rk_hdmi_priv *priv = dev_get_priv(dev);
> struct dw_hdmi *hdmi = &priv->hdmi;
> @@ -88,15 +86,9 @@ static int rk_hdmi_ofdata_to_platdata(struct udevice *dev)
> hdmi->ioaddr = (ulong)dev_get_addr(dev);
> hdmi->mpll_cfg = rockchip_mpll_cfg;
> hdmi->phy_cfg = rockchip_phy_config;
> - hdmi->i2c_clk_high = 0x7a;
> - hdmi->i2c_clk_low = 0x8d;
> -
> - /*
> - * TODO(s...@chromium.org): The above values don't work - these ones
> - * work better, but generate lots of errors in the data.
> - */
> - hdmi->i2c_clk_high = 0x0d;
> - hdmi->i2c_clk_low = 0x0d;
> +
> + /* hdmi->i2c_clk_{high,low} are set up by the SoC driver */
> +
> hdmi->reg_io_width = 4;
> hdmi->phy_set = dw_hdmi_phy_cfg;
>
> @@ -105,53 +97,17 @@ static int rk_hdmi_ofdata_to_platdata(struct udevice
> *dev)
> return 0;
> }
>
> -static int rk_hdmi_probe(struct udevice *dev)
> +void rk_hdmi_probe_regulators(struct udevice *dev,
> + const char * const *names, int cnt)
> +{
> + rk_vop_probe_regulators(dev, names, cnt);
> +}
> +
> +int rk_hdmi_probe(struct udevice *dev)
> {
> - struct display_plat *uc_plat = dev_get_uclass_platdata(dev);
> struct rk_hdmi_priv *priv = dev_get_priv(dev);
> struct dw_hdmi *hdmi = &priv->hdmi;
> - struct udevice *reg;
> - struct clk clk;
> int ret;
> - int vop_id = uc_plat->source_id;
> -
> - ret = clk_get_by_index(dev, 0, &clk);
> - if (ret >= 0) {
> - ret = clk_set_rate(&clk, 0);
> - clk_free(&clk);
> - }
> - if (ret) {
> - debug("%s: Failed to set hdmi clock: ret=%d\n", __func__,
> ret);
> - return ret;
> - }
> -
> - /*
> - * Configure the maximum clock to permit whatever resolution the
> - * monitor wants
> - */
> - ret = clk_get_by_index(uc_plat->src_dev, 0, &clk);
> - if (ret >= 0) {
> - ret = clk_set_rate(&clk, 384000000);
> - clk_free(&clk);
> - }
> - if (ret < 0) {
> - debug("%s: Failed to set clock in source device '%s':
> ret=%d\n",
> - __func__, uc_plat->src_dev->name, ret);
> - return ret;
> - }
> -
> - ret = regulator_get_by_platname("vcc50_hdmi", ®);
> - if (!ret)
> - ret = regulator_set_enable(reg, true);
> - if (ret)
> - debug("%s: Cannot set regulator vcc50_hdmi\n", __func__);
> -
> - /* hdmi source select hdmi controller */
> - rk_setreg(&priv->grf->soc_con6, 1 << 15);
> -
> - /* hdmi data from vop id */
> - rk_clrsetreg(&priv->grf->soc_con6, 1 << 4,
> - (vop_id == 1) ? (1 << 4) : 0);
>
> ret = dw_hdmi_phy_wait_for_hpd(hdmi);
> if (ret < 0) {
> @@ -164,23 +120,3 @@ static int rk_hdmi_probe(struct udevice *dev)
>
> return 0;
> }
> -
> -static const struct dm_display_ops rk_hdmi_ops = {
> - .read_edid = rk_hdmi_read_edid,
> - .enable = rk_hdmi_enable,
> -};
> -
> -static const struct udevice_id rk_hdmi_ids[] = {
> - { .compatible = "rockchip,rk3288-dw-hdmi" },
> - { }
> -};
> -
> -U_BOOT_DRIVER(hdmi_rockchip) = {
> - .name = "hdmi_rockchip",
> - .id = UCLASS_DISPLAY,
> - .of_match = rk_hdmi_ids,
> - .ops = &rk_hdmi_ops,
> - .ofdata_to_platdata = rk_hdmi_ofdata_to_platdata,
> - .probe = rk_hdmi_probe,
> - .priv_auto_alloc_size = sizeof(struct rk_hdmi_priv),
> -};
> diff --git a/drivers/video/rockchip/rk_hdmi.h
> b/drivers/video/rockchip/rk_hdmi.h
> new file mode 100644
> index 0000000..501ed3a
> --- /dev/null
> +++ b/drivers/video/rockchip/rk_hdmi.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef __RK_HDMI_H__
> +#define __RK_HDMI_H__
> +
/**
* struct rkhdmi_driverdata - comment here
*
* @i2c_clk_high: what this is
...
> +struct rkhdmi_driverdata {
> + /* configuration */
> + u8 i2c_clk_high;
> + u8 i2c_clk_low;
> + const char * const *regulator_names;
> + u32 regulator_names_cnt;
> + /* setters/getters */
> + int (*set_input_vop)(struct udevice *dev);
> + int (*clk_config)(struct udevice *dev);
> +};
> +
> +struct rk_hdmi_priv {
> + struct dw_hdmi hdmi;
> + void *grf;
> +};
> +
> +int rk_hdmi_read_edid(struct udevice *dev, u8 *buf, int buf_size);
> +void rk_hdmi_probe_regulators(struct udevice *dev,
> + const char * const *names, int cnt);
> +int rk_hdmi_ofdata_to_platdata(struct udevice *dev);
> +int rk_hdmi_probe(struct udevice *dev);
Please add function comments
> +
> +#endif
> --
> 1.9.1
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
