Re: [PATCH V2 11/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[Marek Vasut <marex@denx.de>]

On 2/18/22 19:38, Lucas Stach wrote:

[...]

>> @@ -502,8 +548,10 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>>   				/*
>>   				 * refclk * mul / (ext_pre_div * pre_div)
>>   				 * should be in the 150 to 650 MHz range
>> +				 * for (e)DP
>>   				 */
>> -				if ((clk > 650000000) || (clk < 150000000))
>> +				if ((tc->bridge.type != DRM_MODE_CONNECTOR_DPI) &&
>> +				    ((clk > 650000000) || (clk < 150000000)))
>>   					continue;
> 
> Is there any indication what the bounds are for DPI mode? Can we
> replace this with a better check, instead of just disabling it?

Apparently the DPI pixel PLL output is limited to 100 MHz, so yes, this 
can be improved.

[...]

>> +static int tc_dpi_stream_enable(struct tc_data *tc)
>> +{
>> +	int ret;
>> +	u32 value;
>> +
>> +	dev_dbg(tc->dev, "enable video stream\n");
>> +
>> +	/* Setup PLL */
>> +	ret = tc_set_syspllparam(tc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Pixel PLL must always be enabled for DPI mode */
>> +	ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
>> +			    1000 * tc->mode.clock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 3);
>> +	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 3);
>> +	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 3);
>> +	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 3);
> 
> Hm, those hardcoded always seem kind of fishy, as AFAIK those
> parameters are dependent on land frequency and some other things. But
> I'm also not sure if we have all the information available to
> dynamically calculate them.

We have multiple copies of the same ^ code in other TC358nnn drivers 
too, it seems like some de-duplication could be done too. I have this 
feeling the Toshiba bridges are all glued together from two (or more) 
halves and there is large potential for de-duplication in all those TC 
drivers. But does anyone really have all the chips to test, except for 
Toshiba ?

>> +	regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
>> +	regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
>> +	regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
>> +	regmap_write(tc->regmap, PPI_LPTXTIMECNT, LPX_PERIOD);
>> +
>> +	value = ((LANEENABLE_L0EN << tc->dsi_lanes) - LANEENABLE_L0EN) |
>> +		LANEENABLE_CLEN;
>> +	regmap_write(tc->regmap, PPI_LANEENABLE, value);
>> +	regmap_write(tc->regmap, DSI_LANEENABLE, value);
>> +
>> +	ret = tc_set_common_video_mode(tc, &tc->mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = tc_set_dpi_video_mode(tc, &tc->mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set input interface */
>> +	value = DP0_AUDSRC_NO_INPUT;
>> +	if (tc_test_pattern)
>> +		value |= DP0_VIDSRC_COLOR_BAR;
>> +	else
>> +		value |= DP0_VIDSRC_DSI_RX;
>> +	ret = regmap_write(tc->regmap, SYSCTRL, value);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msleep(100);
> 
> What is that used for? PLL stabilization? Some other purpose?

Yes, except that should've been microseconds ... fixed

[...]

>> +static int tc_mipi_dsi_host_attach(struct tc_data *tc)
>> +{
>> +	struct device *dev = tc->dev;
>> +	struct device_node *host_node;
>> +	struct device_node *endpoint;
>> +	struct mipi_dsi_device *dsi;
>> +	struct mipi_dsi_host *host;
>> +	const struct mipi_dsi_device_info info = {
>> +		.type = "tc358767",
>> +		.channel = 0,
>> +		.node = NULL,
>> +	};
>> +	int ret;
>> +
>> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>> +	tc->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> 
> The data-lanes property isn't documented in the DT binding. Please add.

Fixed in a separate patch.

[...]

>> @@ -1828,15 +2145,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   		tc->have_irq = true;
>>   	}
>>   
>> -	ret = tc_aux_link_setup(tc);
>> -	if (ret)
>> -		return ret;
>> +	if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */
>> +		ret = tc_aux_link_setup(tc);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	tc->bridge.of_node = dev->of_node;
>>   	drm_bridge_add(&tc->bridge);
>>   
>>   	i2c_set_clientdata(client, tc);
>>   
>> +	if (tc->bridge.type == DRM_MODE_CONNECTOR_DPI) { /* DPI output */
>> +		ret = tc_mipi_dsi_host_attach(tc);
>> +		if (ret)
>> +			return ret;
>> +	}
> 
> If tc_mipi_dsi_host_attach fails the drm bridge registered a few lines
> above isn't cleaned up properly.

Fixed