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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
