Re: [PATCH 3/4] drm/tegra: Add VIC support

[Arto Merilainen <amerilainen@nvidia.com>]

On 05/22/2015 01:25 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, May 21, 2015 at 05:40:31PM +0300, Mikko Perttunen wrote:
>> On 05/21/2015 04:20 PM, Arto Merilainen wrote:
> [...]
>>> +static int vic_is_addr_reg(struct device *dev, u32 class, u32 offset, u32 val)
>>> +{
>>> +	struct vic *vic = dev_get_drvdata(dev);
>>> +
>>> +	/* handle host class */
>>> +	if (class == HOST1X_CLASS_HOST1X) {
>>> +		if (offset == 0x2b)
>>> +			return true;
>>> +		return false;
>>
>> "return (offset == 0x2b);" perhaps?
>
> I think this should really be extracted into a separate helper. If we
> ever need to take into account additional offsets we would otherwise
> have to extend every driver rather than just the helper.

I agree, that would be better.

>
> Also I think the 0x2b should be replaced by some symbolic name.
> According to the TRM 0x2b is the host1x class method named
> NV_CLASS_HOST_INDCTRL_0. Oddly enough that doesn't seem to be an address
> register. Instead the address seems to be in the INDOFF2 and INDOFF
> methods (0x2c and 0x2d). I also can't tell from the TRM what exactly
> these are supposed to do.
>
> Arto, can you clarify?

This looks like an unfortunate mistake that got reproduced from gr2d and 
gr3d.

The INDCTRL method is used for indirect register accessing and it allows 
Host1x to read registers of an engine - or write data directly to 
memory. It allow implementing context switch for the clients whose state 
should be not change between jobs from the same application.

>
>>> +	if (IS_ERR(vic->rst)) {
>>> +		dev_err(&pdev->dev, "cannot get reset\n");
>>> +		return PTR_ERR(vic->rst);
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, vic);
>>> +
>>> +	INIT_LIST_HEAD(&vic->client.base.list);
>>> +	vic->client.base.ops = &vic_client_ops;
>>> +	vic->client.base.dev = dev;
>>> +	vic->client.base.class = vic_config->class_id;
>>> +	vic->client.base.syncpts = syncpts;
>>> +	vic->client.base.num_syncpts = 1;
>>> +	vic->dev = dev;
>>> +	vic->config = vic_config;
>>> +
>>> +	INIT_LIST_HEAD(&vic->client.list);
>>> +	vic->client.ops = &vic_ops;
>>> +
>>> +	err = tegra_powergate_sequence_power_up(vic->config->powergate_id,
>>> +						vic->clk, vic->rst);
>>> +	if (err) {
>>> +		dev_err(dev, "cannot turn on the device\n");
>>> +		return err;
>>> +	}
>>> +
>>> +	err = host1x_client_register(&vic->client.base);
>>> +	if (err < 0) {
>>
>> You used 'if (err) {' previously, so maybe also here.
>
> For consistency with other Tegra DRM code these checks should use (at
> least where possible) the (err < 0) notation.
>

Will fix.

- Arto