[PATCH v11 2/5] drm/bridge: add support for sn65dsi86 bridge driver

spanda at codeaurora.org spanda at codeaurora.org
Mon Jun 18 05:51:46 UTC 2018


On 2018-06-15 16:59, Andrzej Hajda wrote:
> Hi Sandeep,
> 
> Thanks for the patch, I hope it will be merged soon.
> 
> On 15.06.2018 08:43, Sandeep Panda wrote:
>> Add support for TI's sn65dsi86 dsi2edp bridge chip.
>> The chip converts DSI transmitted signal to eDP signal,
>> which is fed to the connected eDP panel.
>> 
>> This chip can be controlled via either i2c interface or
>> dsi interface. Currently in driver all the control registers
>> are being accessed through i2c interface only.
>> Also as of now HPD support has not been added to bridge
>> chip driver.
>> 
>> Changes in v1:
>>  - Split the dt-bindings and the driver support into separate patches
>>    (Andrzej Hajda).
>>  - Use of gpiod APIs to parse and configure gpios instead of obsolete 
>> ones
>>    (Andrzej Hajda).
>>  - Use macros to define the register offsets (Andrzej Hajda).
>> 
>> Changes in v2:
>>  - Separate out edp panel specific HW resource handling from bridge
>>    driver and create a separate edp panel drivers to handle panel
>>    specific mode information and HW resources (Sean Paul).
>>  - Replace pr_* APIs to DRM_* APIs to log error or debug information
>>    (Sean Paul).
>>  - Remove some of the unnecessary structure/variable from driver (Sean
>>    Paul).
>>  - Rename the function and structure prefix "sn65dsi86" to 
>> "ti_sn_bridge"
>>    (Sean Paul / Rob Herring).
>>  - Remove most of the hard-coding and modified the bridge init 
>> sequence
>>    based on current mode (Sean Paul).
>>  - Remove the existing function to retrieve the EDID data and
>>    implemented this as an i2c_adapter and use drm_get_edid() (Sean 
>> Paul).
>>  - Remove the dummy irq handler implementation, will add back the
>>    proper irq handling later (Sean Paul).
>>  - Capture the required enable gpios in a single array based on dt 
>> entry
>>    instead of having individual descriptor for each gpio (Sean Paul).
>> 
>> Changes in v3:
>>  - Remove usage of irq_gpio and replace it as "interrupts" property 
>> (Rob
>>    Herring).
>>  - Remove the unnecessary header file inclusions (Sean Paul).
>>  - Rearrange the header files in alphabetical order (Sean Paul).
>>  - Use regmap interface to perform i2c transactions.
>>  - Update Copyright/License field and address other review comments
>>    (Jordan Crouse).
>> 
>> Changes in v4:
>>  - Update License/Copyright (Sean Paul).
>>  - Add Kconfig and Makefile changes (Sean Paul).
>>  - Drop i2c gpio handling from this bridge driver, since i2c sda/scl 
>> gpios
>>    will be handled by i2c master.
>>  - Update required supplies names.
>>  - Remove unnecessary goto statements (Sean Paul).
>>  - Add mutex lock to power_ctrl API to avoid race conditions (Sean
>>    Paul).
>>  - Add support to parse reference clk frequency from dt(optional).
>>  - Update the bridge chip enable/disable sequence.
>> 
>> Changes in v5:
>>  - Fixed Kbuild test service reported warnings.
>> 
>> Changes in v6:
>>  - Use PM runtime based ref-counting instead of local ref_count 
>> mechanism
>>    (Stephen Boyd).
>>  - Clean up some debug logs and indentations (Sean Paul).
>>  - Simplify dp rate calculation (Sean Paul).
>>  - Add support to configure refclk based on input REFCLK pin or DACP/N
>>    pin (Stephen Boyd).
>> 
>> Changes in v7:
>>  - Use static supply entries instead of dynamic allocation (Andrzej
>>    Hajda).
>>  - Defer bridge driver probe if panel is not probed (Andrzej Hajda).
>>  - Update of_graph APIs for correct node reference management. 
>> (Andrzej
>>    Hajda).
>>  - Remove local display_mode object (Andrzej Hajda).
>>  - Remove version id check function from driver.
>> 
>> Changes in v8:
>>  - Move dsi register/attach function to bridge driver probe (Andrzej
>>    Hajda).
>>  - Introduce a new helper function to write 16bit words into 
>> consecutive
>>    registers (Andrzej Hajda).
>>  - Remove unnecessary macros (Andrzej Hajda).
>> 
>> Changes in v9:
>>  - Remove dsi register/attach from bridge probe, since dsi dev 
>> register
>>    completion also waits for any panel or bridge to get added. This 
>> creates
>>    deadlock situation when bridge driver calls dsi dev register and
>>    attach before bridge add, in its probe function.
>>  - Fix issues faced during testing of bridge driver on actual HW.
>>  - Remove unnecessary initializations (Stephen Boyd).
>>  - Use local refclk lut size instead of global macro (Sean Paul).
>> 
>> Changes in v10:
>>  - Use refclk to determine if continuous dsi clock is needed or not.
>> 
>> Changes in v11:
>>  - Read DPPLL_SRC register to determine continuous clock instead of
>>    using refclk handle (Stephen Boyd).
>> 
>> Signed-off-by: Sandeep Panda <spanda at codeaurora.org>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig        |   9 +
>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 696 
>> ++++++++++++++++++++++++++++++++++
>>  3 files changed, 706 insertions(+)
>>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> 
>> diff --git a/drivers/gpu/drm/bridge/Kconfig 
>> b/drivers/gpu/drm/bridge/Kconfig
>> index 3b99d5a..8153150 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -108,6 +108,15 @@ config DRM_TI_TFP410
>>  	---help---
>>  	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
>> 
>> +config DRM_TI_SN65DSI86
>> +	tristate "TI SN65DSI86 DSI to eDP bridge"
>> +	depends on OF
>> +	select DRM_KMS_HELPER
>> +	select REGMAP_I2C
>> +	select DRM_PANEL
>> +	---help---
>> +	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>> +
>>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>> 
>>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>> b/drivers/gpu/drm/bridge/Makefile
>> index 373eb28..a7ca000 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -11,5 +11,6 @@ obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>> +obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>>  obj-y += synopsys/
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
>> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> new file mode 100644
>> index 0000000..5473456
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -0,0 +1,696 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_panel.h>
>> +#include <linux/clk.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +/* Link Training specific registers */
>> +#define SN_DEVICE_REV_REG			0x08
>> +#define SN_HPD_DISABLE_REG			0x5C
>> +#define SN_DPPLL_SRC_REG			0x0A
>> +#define SN_DSI_LANES_REG			0x10
>> +#define SN_DSIA_CLK_FREQ_REG			0x12
>> +#define SN_ENH_FRAME_REG			0x5A
>> +#define SN_SSC_CONFIG_REG			0x93
>> +#define SN_DATARATE_CONFIG_REG			0x94
>> +#define SN_PLL_ENABLE_REG			0x0D
>> +#define SN_SCRAMBLE_CONFIG_REG			0x95
>> +#define SN_AUX_WDATA0_REG			0x64
>> +#define SN_AUX_ADDR_19_16_REG			0x74
>> +#define SN_AUX_ADDR_15_8_REG			0x75
>> +#define SN_AUX_ADDR_7_0_REG			0x76
>> +#define SN_AUX_LENGTH_REG			0x77
>> +#define SN_AUX_CMD_REG				0x78
>> +#define SN_ML_TX_MODE_REG			0x96
>> +/* video config specific registers */
>> +#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
>> +#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
>> +#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
>> +#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
>> +#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
>> +#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
>> +#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
>> +#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
>> +#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
>> +#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
>> +#define SN_DATA_FORMAT_REG			0x5B
>> +
>> +#define MIN_DSI_CLK_FREQ_MHZ	40
>> +
>> +/* fudge factor required to account for 8b/10b encoding */
>> +#define DP_CLK_FUDGE_NUM	10
>> +#define DP_CLK_FUDGE_DEN	8
>> +
>> +#define DPPLL_CLK_SRC_REFCLK	0
>> +#define DPPLL_CLK_SRC_DSICLK	1
>> +
>> +#define SN_REFCLK_FREQ_OFFSET	1
>> +#define SN_DSIA_LANE_OFFSET	3
>> +#define SN_DP_LANE_OFFSET	4
>> +#define SN_DP_DATA_RATE_OFFSET	5
>> +#define SN_SYNC_POLARITY_OFFSET	7
>> +
>> +#define SN_ENABLE_VID_STREAM_BIT	BIT(3)
>> +#define SN_REFCLK_FREQ_BITS		(BIT(3) | BIT(2) | BIT(1))
>> +#define SN_DSIA_NUM_LANES_BITS		(BIT(4) | BIT(3))
>> +#define SN_DP_NUM_LANES_BITS		(BIT(5) | BIT(4))
>> +#define SN_DP_DATA_RATE_BITS		(BIT(7) | BIT(6) | BIT(5))
> 
> For contiguous bitmasks you could use GENMASK(h, l), but it is up to 
> you.
Ok will update in next patch.
> 
>> +#define SN_HPD_DISABLE_BIT		BIT(0)
>> +
>> +#define SN_REGULATOR_SUPPLY_NUM		4
>> +
>> +struct ti_sn_bridge {
>> +	struct device			*dev;
>> +	struct regmap			*regmap;
>> +	struct drm_bridge		bridge;
>> +	struct drm_connector		connector;
>> +	struct device_node		*host_node;
>> +	struct mipi_dsi_device		*dsi;
>> +	struct clk			*refclk;
>> +	struct drm_panel		*panel;
>> +	struct gpio_desc		*enable_gpio;
>> +	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
>> +	struct i2c_adapter		*ddc;
>> +};
>> +
>> +static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
>> +	{ .range_min = 0, .range_max = 0xFF },
>> +};
>> +
>> +static const struct regmap_access_table ti_sn_bridge_volatile_table = 
>> {
>> +	.yes_ranges = ti_sn_bridge_volatile_ranges,
>> +	.n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
>> +};
>> +
>> +static const struct regmap_config ti_sn_bridge_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.volatile_table = &ti_sn_bridge_volatile_table,
>> +	.cache_type = REGCACHE_NONE,
>> +};
>> +
>> +static void ti_sn_bridge_write_u16(struct ti_sn_bridge *pdata,
>> +				   unsigned int reg, u16 val)
>> +{
>> +	regmap_write(pdata->regmap, reg, val & 0xFF);
>> +	regmap_write(pdata->regmap, reg + 1, val >> 8);
>> +}
>> +
>> +static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
>> +{
>> +	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = regulator_bulk_enable(SN_REGULATOR_SUPPLY_NUM, 
>> pdata->supplies);
>> +	if (ret) {
>> +		DRM_ERROR("failed to enable supplies %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	gpiod_set_value(pdata->enable_gpio, 1);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
>> +{
>> +	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	gpiod_set_value(pdata->enable_gpio, 0);
>> +
>> +	ret = regulator_bulk_disable(SN_REGULATOR_SUPPLY_NUM, 
>> pdata->supplies);
>> +	if (ret)
>> +		DRM_ERROR("failed to disable supplies %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
>> +};
>> +
>> +/* Connector funcs */
>> +static struct ti_sn_bridge *
>> +connector_to_ti_sn_bridge(struct drm_connector *connector)
>> +{
>> +	return container_of(connector, struct ti_sn_bridge, connector);
>> +}
>> +
>> +static int ti_sn_bridge_connector_get_modes(struct drm_connector 
>> *connector)
>> +{
>> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
>> +	struct edid *edid;
>> +	u32 num_modes;
>> +
>> +	if (pdata->panel) {
>> +		DRM_DEBUG_KMS("get mode from connected drm_panel\n");
>> +		return drm_panel_get_modes(pdata->panel);
>> +	}
>> +
>> +	if (!pdata->ddc)
>> +		return 0;
> I think it should not happen, ie probe should fail if both panel and 
> ddc
> are absent.
Ok i will remove this check in next patchset.
>> +
>> +	pm_runtime_get_sync(pdata->dev);
>> +	edid = drm_get_edid(connector, pdata->ddc);
>> +	pm_runtime_put_sync(pdata->dev);
> 
> Minor nit, you can use here pm_runtime_put, I guess you do not care 
> when
> it will be turned off.

Ok.
> 
>> +	if (!edid)
>> +		return 0;
>> +
>> +	drm_mode_connector_update_edid_property(connector, edid);
>> +	num_modes = drm_add_edid_modes(connector, edid);
>> +	kfree(edid);
>> +
>> +	return num_modes;
>> +}
>> +
>> +static enum drm_mode_status
>> +ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
>> +			     struct drm_display_mode *mode)
>> +{
>> +	/* maximum supported resolution is 4K at 60 fps */
>> +	if (mode->clock > 594000)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	return MODE_OK;
>> +}
>> +
>> +static struct drm_connector_helper_funcs 
>> ti_sn_bridge_connector_helper_funcs = {
>> +	.get_modes = ti_sn_bridge_connector_get_modes,
>> +	.mode_valid = ti_sn_bridge_connector_mode_valid,
>> +};
>> +
>> +static enum drm_connector_status
>> +ti_sn_bridge_connector_detect(struct drm_connector *connector, bool 
>> force)
>> +{
>> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
>> +
>> +	/**
>> +	 * TODO: Currently if drm_panel is present, then always
>> +	 * return the status as connected. Need to add support to detect
>> +	 * device state for no panel(hot pluggable) scenarios.
>> +	 */
>> +	if (pdata->panel)
>> +		return connector_status_connected;
>> +	else
>> +		return connector_status_unknown;
>> +}
>> +
>> +static const struct drm_connector_funcs ti_sn_bridge_connector_funcs 
>> = {
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.detect = ti_sn_bridge_connector_detect,
>> +	.destroy = drm_connector_cleanup,
>> +	.reset = drm_atomic_helper_connector_reset,
>> +	.atomic_duplicate_state = 
>> drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge 
>> *bridge)
>> +{
>> +	return container_of(bridge, struct ti_sn_bridge, bridge);
>> +}
>> +
>> +static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
>> +{
>> +	unsigned int i;
>> +	const char * const ti_sn_bridge_supply_names[] = {
>> +		"vcca", "vcc", "vccio", "vpll",
>> +	};
>> +
>> +	for (i = 0; i < SN_REGULATOR_SUPPLY_NUM; i++)
>> +		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
>> +
>> +	return devm_regulator_bulk_get(pdata->dev, SN_REGULATOR_SUPPLY_NUM,
>> +				       pdata->supplies);
>> +}
>> +
>> +static int ti_sn_bridge_attach(struct drm_bridge *bridge)
>> +{
>> +	int ret, val;
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +	struct mipi_dsi_host *host;
>> +	struct mipi_dsi_device *dsi;
>> +	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
>> +						   .channel = 0,
>> +						   .node = NULL,
>> +						 };
>> +
>> +	ret = drm_connector_init(bridge->dev, &pdata->connector,
>> +				 &ti_sn_bridge_connector_funcs,
>> +				 DRM_MODE_CONNECTOR_eDP);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to initialize connector with drm\n");
>> +		return ret;
>> +	}
>> +
>> +	drm_connector_helper_add(&pdata->connector,
>> +				 &ti_sn_bridge_connector_helper_funcs);
>> +	drm_mode_connector_attach_encoder(&pdata->connector, 
>> bridge->encoder);
>> +
>> +	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
>> +	if (!host) {
>> +		DRM_ERROR("failed to find dsi host\n");
>> +		ret = -ENODEV;
>> +		goto err_dsi_host;
>> +	}
> 
> In case of late host probing it will fail, instead of probe deferring.
> If I remember I have asked you to move this code to probe, as deferring
> is only possible there.
> 

I moved the this code to probe, and returned probe defer if host not 
found in patchset v9.
This caused a dead lock when i was testing on actual HW. The reason 
being,
DSI host probe completion was waiting for any drm_panel/drm_bridge to 
get
added to the global panel/bridge list, and here in bridge driver we are 
waiting for
DSI host probe completion to happen so that host node gets added to the 
global host list.
Also i found other up-streamed bridge drivers are also doing the same in 
bridge_attach.

>> +
>> +	dsi = mipi_dsi_device_register_full(host, &info);
>> +	if (IS_ERR(dsi)) {
>> +		DRM_ERROR("failed to create dsi device\n");
>> +		ret = PTR_ERR(dsi);
>> +		goto err_dsi_host;
>> +	}
>> +
>> +	/* TODO: setting to 4 lanes always for now */
>> +	dsi->lanes = 4;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
>> MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>> +			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
>> +
>> +	/* check if continuous dsi clock is required or not */
>> +	pm_runtime_get_sync(pdata->dev);
>> +	regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
>> +	pm_runtime_put_sync(pdata->dev);
>> +	if (!(val & DPPLL_CLK_SRC_DSICLK))
>> +		dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
>> +
>> +	ret = mipi_dsi_attach(dsi);
>> +	if (ret < 0) {
>> +		DRM_ERROR("failed to attach dsi to host\n");
>> +		goto err_dsi_attach;
>> +	}
>> +	pdata->dsi = dsi;
>> +
>> +	/* attach panel to bridge */
>> +	if (pdata->panel)
>> +		drm_panel_attach(pdata->panel, &pdata->connector);
>> +
>> +	return 0;
>> +
>> +err_dsi_attach:
>> +	mipi_dsi_device_unregister(dsi);
>> +err_dsi_host:
>> +	drm_connector_cleanup(&pdata->connector);
>> +	return ret;
>> +}
>> +
>> +static void ti_sn_bridge_disable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +
>> +	if (pdata->panel) {
>> +		drm_panel_disable(pdata->panel);
>> +		drm_panel_unprepare(pdata->panel);
>> +	}
>> +
>> +	/* disable video stream */
>> +	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
>> +			   SN_ENABLE_VID_STREAM_BIT, 0);
>> +	/* semi auto link training mode OFF */
>> +	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
>> +	/* disable DP PLL */
>> +	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
> 
> Maybe in your particular case it does not matter but generally
> drm_panel_disable should be called 1st, but drm_panel_unprepare should
> be called somewhere after disabling video stream.

Ok i wil move panel unprepare to end in next patchset.
> 
>> +}
>> +
>> +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
>> +{
>> +	u32 bit_rate_khz, clk_freq_khz;
>> +	struct drm_display_mode *mode =
>> +		&pdata->bridge.encoder->crtc->state->adjusted_mode;
>> +
>> +	bit_rate_khz = mode->clock *
>> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> +	clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2);
>> +
>> +	return clk_freq_khz;
>> +}
>> +
>> +/* clk frequencies supported by bridge in Hz in case derived from 
>> REFCLK pin */
>> +static const u32 ti_sn_bridge_refclk_lut[] = {
>> +	12000000,
>> +	19200000,
>> +	26000000,
>> +	27000000,
>> +	38400000,
>> +};
>> +
>> +/* clk frequencies supported by bridge in Hz in case derived from 
>> DACP/N pin */
>> +static const u32 ti_sn_bridge_dsiclk_lut[] = {
>> +	468000000,
>> +	384000000,
>> +	416000000,
>> +	486000000,
>> +	460800000,
>> +};
>> +
>> +static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
>> +{
>> +	int i;
>> +	u32 refclk_rate;
>> +	const u32 *refclk_lut;
>> +	size_t refclk_lut_size;
>> +
>> +	if (pdata->refclk) {
>> +		refclk_rate = clk_get_rate(pdata->refclk);
>> +		refclk_lut = ti_sn_bridge_refclk_lut;
>> +		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut);
>> +		clk_prepare_enable(pdata->refclk);
>> +	} else {
>> +		refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000;
>> +		refclk_lut = ti_sn_bridge_dsiclk_lut;
>> +		refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut);
>> +	}
>> +
>> +	/* for i equals to refclk_lut_size means default frequency */
>> +	for (i = 0; i < refclk_lut_size; i++)
>> +		if (refclk_lut[i] == refclk_rate)
>> +			break;
>> +
>> +	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG,
>> +			   SN_REFCLK_FREQ_BITS, i << SN_REFCLK_FREQ_OFFSET);
>> +}
>> +
>> +/**
>> + * LUT index corresponds to register value and
>> + * LUT values corresponds to dp data rate supported
>> + * by the bridge in Mbps unit.
>> + */
>> +static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
>> +	0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
>> +};
>> +
>> +static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
>> +{
>> +	unsigned int bit_rate_mhz, clk_freq_mhz, dp_rate_mhz;
>> +	unsigned int val, i;
>> +	struct drm_display_mode *mode =
>> +		&pdata->bridge.encoder->crtc->state->adjusted_mode;
>> +
>> +	/* set DSIA clk frequency */
>> +	bit_rate_mhz = (mode->clock / 1000) *
>> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> +	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>> +
>> +	/* for each increment in val, frequency increases by 5MHz */
>> +	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
>> +		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>> +
>> +	/* set DP data rate */
>> +	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * 
>> DP_CLK_FUDGE_NUM) /
>> +							DP_CLK_FUDGE_DEN;
>> +	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i++)
>> +		if (ti_sn_bridge_dp_rate_lut[i] > dp_rate_mhz)
>> +			break;
>> +
>> +	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>> +			   SN_DP_DATA_RATE_BITS, i << SN_DP_DATA_RATE_OFFSET);
>> +}
>> +
>> +static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge 
>> *pdata)
>> +{
>> +	struct drm_display_mode *mode =
>> +		&pdata->bridge.encoder->crtc->state->adjusted_mode;
>> +	u8 hsync_polarity = 0, vsync_polarity = 0;
>> +
>> +	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
>> +		hsync_polarity = 0x1;
> 
> You can assign:
> 
> hsync_polarity = BIT(SN_SYNC_POLARITY_OFFSET);
> 
> and then use it directly in regmap_write.
> 
>> +	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>> +		vsync_polarity = 0x1;
> 
> The same here.
> 
Ok.
>> +
>> +	ti_sn_bridge_write_u16(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
>> +			       mode->hdisplay);
>> +	ti_sn_bridge_write_u16(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
>> +			       mode->vdisplay);
>> +	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
>> +		     (mode->hsync_end - mode->hsync_start) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
>> +		     (((mode->hsync_end - mode->hsync_start) >> 8) & 0x7F)
>> +		     | (hsync_polarity << SN_SYNC_POLARITY_OFFSET));
>> +	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
>> +		     (mode->vsync_end - mode->vsync_start) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
>> +		    (((mode->vsync_end - mode->vsync_start) >> 8) & 0x7F)
>> +		    | (vsync_polarity << SN_SYNC_POLARITY_OFFSET));
>> +
>> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
>> +		     (mode->htotal - mode->hsync_end) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_BACK_PORCH_REG,
>> +		     (mode->vtotal - mode->vsync_end) & 0xFF);
>> +
>> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
>> +		     (mode->hsync_start - mode->hdisplay) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_FRONT_PORCH_REG,
>> +		     (mode->vsync_start - mode->vdisplay) & 0xFF);
>> +
>> +	usleep_range(10000, 10500); /* 10ms delay recommended by spec */
>> +}
>> +
>> +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +	unsigned int val;
>> +
>> +	if (pdata->panel) {
>> +		drm_panel_prepare(pdata->panel);
>> +		/* in case drm_panel is connected then HPD is not supported */
>> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>> +				   SN_HPD_DISABLE_BIT, SN_HPD_DISABLE_BIT);
> 
> I think HPD should be disabled much earlier, I guess somewhere in
> initialization/probe.
> 
As per the driver code, we are enabling the bridge power supplies and 
gpio
only in pre-enable function, i can move this HDP disable to pre-enable.

>> +	}
>> +
>> +	/* DSI_A lane config */
>> +	val = (4 - pdata->dsi->lanes) << SN_DSIA_LANE_OFFSET;
>> +	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
>> +			   SN_DSIA_NUM_LANES_BITS, val);
>> +
>> +	/* DP lane config */
>> +	val = (pdata->dsi->lanes - 1) << SN_DP_LANE_OFFSET;
>> +	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG,
>> +			   SN_DP_NUM_LANES_BITS, val);
>> +
>> +	/* set dsi/dp clk frequency value */
>> +	ti_sn_bridge_set_dsi_dp_rate(pdata);
>> +
>> +	/* enable DP PLL */
>> +	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>> +	usleep_range(10000, 10500); /* 10ms delay recommended by spec */
>> +
>> +	/**
>> +	 * The SN65DSI86 only supports ASSR Display Authentication method 
>> and
>> +	 * this method is enabled by default. An eDP panel must support this
>> +	 * authentication method. We need to enable this method in the eDP 
>> panel
>> +	 * at DisplayPort address 0x0010A prior to link training.
>> +	 */
>> +	regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);
>> +	regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);
>> +	regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);
>> +	regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);
>> +	regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);
>> +	regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);
>> +	usleep_range(10000, 10500); /* 10ms delay recommended by spec */
>> +
>> +	/* Semi auto link training mode */
>> +	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
>> +	msleep(20); /* 20ms delay recommended by spec */
>> +
>> +	/* config video parameters */
>> +	ti_sn_bridge_set_video_timings(pdata);
>> +
>> +	/* enable video stream */
>> +	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
>> +			   SN_ENABLE_VID_STREAM_BIT, SN_ENABLE_VID_STREAM_BIT);
>> +
>> +	if (pdata->panel)
>> +		drm_panel_enable(pdata->panel);
>> +}
>> +
>> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +
>> +	pm_runtime_get_sync(pdata->dev);
>> +
>> +	/* configure bridge ref_clk */
>> +	ti_sn_bridge_set_refclk_freq(pdata);
>> +}
>> +
>> +static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +
>> +	if (pdata->refclk)
>> +		clk_disable_unprepare(pdata->refclk);
>> +
>> +	pm_runtime_put_sync(pdata->dev);
>> +}
>> +
>> +static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>> +	.attach = ti_sn_bridge_attach,
>> +	.pre_enable = ti_sn_bridge_pre_enable,
>> +	.enable = ti_sn_bridge_enable,
>> +	.disable = ti_sn_bridge_disable,
>> +	.post_disable = ti_sn_bridge_post_disable,
>> +};
>> +
>> +static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>> +{
>> +	struct device_node *np = pdata->dev->of_node;
>> +
>> +	pdata->host_node = of_graph_get_remote_node(np, 0, 0);
>> +
>> +	if (!pdata->host_node) {
>> +		DRM_ERROR("remote dsi host node not found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_sn_bridge_probe(struct i2c_client *client,
>> +	 const struct i2c_device_id *id)
>> +{
>> +	struct ti_sn_bridge *pdata;
>> +	struct device_node *ddc_node;
>> +	int ret;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +		DRM_ERROR("device doesn't support I2C\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn_bridge),
>> +			     GFP_KERNEL);
>> +	if (!pdata)
>> +		return -ENOMEM;
>> +
>> +	pdata->regmap = devm_regmap_init_i2c(client,
>> +					     &ti_sn_bridge_regmap_config);
>> +	if (IS_ERR(pdata->regmap)) {
>> +		DRM_ERROR("regmap i2c init failed\n");
>> +		return PTR_ERR(pdata->regmap);
>> +	}
>> +
>> +	pdata->dev = &client->dev;
>> +	dev_set_drvdata(&client->dev, pdata);
>> +
>> +	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
>> +					    GPIOD_OUT_LOW);
>> +	if (IS_ERR(pdata->enable_gpio)) {
>> +		DRM_ERROR("failed to get enable gpio from DT\n");
>> +		ret = PTR_ERR(pdata->enable_gpio);
>> +		return ret;
>> +	}
>> +
>> +	ret = ti_sn_bridge_parse_regulators(pdata);
>> +	if (ret) {
>> +		DRM_ERROR("failed to parse regulators\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
>> +					  &pdata->panel, NULL);
>> +	if (ret) {
>> +		DRM_ERROR("could not find any drm panel node\n");
>> +		return ret;
>> +	}
>> +
>> +	pdata->refclk = devm_clk_get(pdata->dev, "refclk");
>> +	if (IS_ERR(pdata->refclk)) {
>> +		ret = PTR_ERR(pdata->refclk);
>> +		if (ret == -EPROBE_DEFER)
>> +			return ret;
>> +		DRM_DEBUG_KMS("refclk not found\n");
>> +		pdata->refclk = NULL;
>> +	}
>> +
>> +	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
>> +	if (ddc_node) {
>> +		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
>> +		of_node_put(ddc_node);
>> +		if (!pdata->ddc) {
>> +			DRM_DEBUG_KMS("failed to read ddc node\n");
>> +			return -EPROBE_DEFER;
>> +		}
>> +	} else {
>> +		DRM_DEBUG_KMS("no ddc property found\n");
>> +	}
> 
> I guess there should be exclusively panel or ddc, please fail if
> both/none are present.
> 
Just curious, can't an edp panel will have EDID flashed?
> 
> Regards
> Andrzej
> 
>> +
>> +	ret = ti_sn_bridge_parse_dsi_host(pdata);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pm_runtime_enable(pdata->dev);
>> +
>> +	i2c_set_clientdata(client, pdata);
>> +
>> +	pdata->bridge.funcs = &ti_sn_bridge_funcs;
>> +	pdata->bridge.of_node = client->dev.of_node;
>> +
>> +	drm_bridge_add(&pdata->bridge);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_sn_bridge_remove(struct i2c_client *client)
>> +{
>> +	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
>> +
>> +	if (!pdata)
>> +		return -EINVAL;
>> +
>> +	if (pdata->host_node)
>> +		of_node_put(pdata->host_node);
>> +
>> +	pm_runtime_disable(pdata->dev);
>> +
>> +	if (pdata->dsi) {
>> +		mipi_dsi_detach(pdata->dsi);
>> +		mipi_dsi_device_unregister(pdata->dsi);
>> +	}
>> +
>> +	drm_bridge_remove(&pdata->bridge);
>> +	i2c_put_adapter(pdata->ddc);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct i2c_device_id ti_sn_bridge_id[] = {
>> +	{ "ti,sn65dsi86", 0},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
>> +
>> +static const struct of_device_id ti_sn_bridge_match_table[] = {
>> +	{.compatible = "ti,sn65dsi86"},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
>> +
>> +static struct i2c_driver ti_sn_bridge_driver = {
>> +	.driver = {
>> +		.name = "ti_sn65dsi86",
>> +		.of_match_table = ti_sn_bridge_match_table,
>> +		.pm = &ti_sn_bridge_pm_ops,
>> +	},
>> +	.probe = ti_sn_bridge_probe,
>> +	.remove = ti_sn_bridge_remove,
>> +	.id_table = ti_sn_bridge_id,
>> +};
>> +
>> +module_i2c_driver(ti_sn_bridge_driver);
>> +MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
>> +MODULE_LICENSE("GPL v2");


More information about the dri-devel mailing list