Re: [PATCH 1/7] drm/komeda: Add d71_enum_resources and d71_cleanup

[Brian Starkey]

Hi James,

A few minor comments.

On Mon, Dec 24, 2018 at 08:58:46AM +0000, james qian wang (Arm Technology China) wrote:
> D71 consists of a number of Register Blocks, every Block controls a
> specific HW function, every block has a common block_header to represent
> its type and pipeline information.
> 
> GCU (Global Control Unit) is the first Block which describe the global
> information of D71 HW, Like number of block contained and the number of
> pipeline supported.
> 
> So the d71_enum_resources parsed GCU and create pipeline according
> the GCU configuration, and then iterate and detect the blocks that
> indicated by the GCU and block_header.
> 
> And this change also added two struct d71_dev/d71_pipeline to extend
> komeda_dev/komeda_pipeline to add some d71 only members.
> 
> Signed-off-by: James (Qian) Wang <james.qian.wang@xxxxxxx>
> ---

-- snip --

> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> new file mode 100644
> index 000000000000..a43a2410159f
> --- /dev/null
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (C) COPYRIGHT 2018 ARM Limited. All rights reserved.
> + * Author: James.Qian.Wang <james.qian.wang@xxxxxxx>
> + *
> + */
> +#include "d71_dev.h"
> +#include "komeda_kms.h"
> +#include "malidp_io.h"
> +
> +static int d71_layer_init(struct d71_dev *d71,
> +			  struct block_header *blk, u32 __iomem *reg)
> +{
> +	DRM_INFO("Detect D71_Layer.\n");

I think all of these can be DRM_DEBUG.

-- snip --

>  
>  static int d71_enum_resources(struct komeda_dev *mdev)
>  {
> -	/* TODO add enum resources */
> +	struct d71_dev *d71;
> +	struct komeda_pipeline *pipe;
> +	struct block_header blk;
> +	u32 __iomem *blk_base;
> +	u32 i, value, offset;
> +
> +	d71 = devm_kzalloc(mdev->dev, sizeof(*d71), GFP_KERNEL);
> +	if (!d71)
> +		return -ENOMEM;
> +
> +	mdev->chip_data = d71;
> +	d71->mdev = mdev;
> +	d71->gcu_addr = mdev->reg_base;
> +	d71->periph_addr = mdev->reg_base + (D71_BLOCK_OFFSET_PERIPH >> 2);
> +
> +	if (d71_reset(d71)) {
> +		DRM_ERROR("Fail to reset d71 device.\n");
> +		goto err_cleanup;
> +	}
> +
> +	/* probe GCU */
> +	value = malidp_read32(d71->gcu_addr, GLB_CORE_INFO);
> +	d71->num_blocks = value & 0xFF;
> +	d71->num_pipelines = (value >> 8) & 0x7;
> +
> +	if (d71->num_pipelines > D71_MAX_PIPELINE) {
> +		DRM_ERROR("d71 supports %d pipelines, but got: %d.\n",
> +			  D71_MAX_PIPELINE, d71->num_pipelines);
> +		goto err_cleanup;
> +	}
> +
> +	/* probe PERIPH */
> +	value = malidp_read32(d71->periph_addr, BLK_BLOCK_INFO);
> +	if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH) {
> +		DRM_ERROR("access blk periph but got blk: %d.\n",
> +			  BLOCK_INFO_BLK_TYPE(value));
> +		goto err_cleanup;
> +	}
> +
> +	value = malidp_read32(d71->periph_addr, PERIPH_CONFIGURATION_ID);
> +
> +	d71->max_line_size	= value & PERIPH_MAX_LINE_SIZE ? 4096 : 2048;
> +	d71->max_vsize		= 4096;
> +	d71->num_rich_layers	= value & PERIPH_NUM_RICH_LAYERS ? 2 : 1;
> +	d71->supports_dual_link	= value & PERIPH_SPLIT_EN ? true : false;
> +	d71->integrates_tbu	= value & PERIPH_TBU_EN ? true : false;
> +
> +	for (i = 0; i < d71->num_pipelines; i++) {
> +		pipe = komeda_pipeline_add(mdev, sizeof(struct d71_pipeline),
> +					   NULL);
> +		if (!pipe)
> +			goto err_cleanup;
> +
> +		d71->pipes[i] = to_d71_pipeline(pipe);
> +	}
> +
> +	/* loop the register blks and probe */
> +	i = 2; /* exclude GCU and PERIPH */
> +	offset = D71_BLOCK_SIZE; /* skip GCU */
> +	while (i < d71->num_blocks) {
> +		blk_base = mdev->reg_base + (offset >> 2);
> +
> +		d71_read_block_header(blk_base, &blk);
> +		if (BLOCK_INFO_BLK_TYPE(blk.block_info) != D71_BLK_TYPE_RESERVED) {
> +			if (d71_probe_block(d71, &blk, blk_base))
> +				goto err_cleanup;
> +			i++;
> +		}
> +
> +		offset += D71_BLOCK_SIZE;
> +	}
> +
> +	DRM_INFO("total %d (out of %d) blocks are found.\n",
> +		 i, d71->num_blocks);
> +
> +	return 0;
> +
> +err_cleanup:
> +	d71_cleanup(mdev);
>  	return -1;

-1 isn't a useful error code, and you return -ENOMEM if allocation
fails. It would probably be better to return proper codes everywhere
(e.g. you might get -EINVAL back from d71_probe_block, but you don't
propagate it).

-- snip --

> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> new file mode 100644
> index 000000000000..2d5e6d00b42c
> --- /dev/null
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h

...

> +
> +/* Image process registers */
> +#define IPS_DEPTH		0x0D8
> +#define IPS_RGB_RGB_COEFF0	0x130
> +#define IPS_RGB_YUV_COEFF0	0x170
> +
> +#define IPS_DEPTH_MARK		0xF

s/MARK/MASK/

Cheers,
-Brian