2019-11-21 08:19:40

by James Qian Wang

Subject: [PATCH v2 0/2] drm/komeda: Add new product "D32" support

Hi All:

This series enables new product "D32" support

v2: Rebase

james qian wang (Arm Technology China) (2):
drm/komeda: Update the chip identify
drm/komeda: Enable new product D32 support

.../drm/arm/display/include/malidp_product.h | 3 +-
.../arm/display/komeda/d71/d71_component.c | 2 +-
.../gpu/drm/arm/display/komeda/d71/d71_dev.c | 70 +++++++++++++------
.../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++
.../gpu/drm/arm/display/komeda/komeda_dev.c | 60 ++++++++--------
.../gpu/drm/arm/display/komeda/komeda_dev.h | 14 +---
.../gpu/drm/arm/display/komeda/komeda_drv.c | 10 +--
7 files changed, 102 insertions(+), 70 deletions(-)

--
2.20.1

2019-11-21 08:19:47

by James Qian Wang

Subject: [PATCH v2 2/2] drm/komeda: Enable new product D32 support

D32 is simple version of D71, the difference is:
- Only has one pipeline
- Drop the periph block and merge it to GCU

v2: Rebase.

Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
---
.../drm/arm/display/include/malidp_product.h | 3 +-
.../arm/display/komeda/d71/d71_component.c | 2 +-
.../gpu/drm/arm/display/komeda/d71/d71_dev.c | 43 ++++++++++++-------
.../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++++
.../gpu/drm/arm/display/komeda/komeda_drv.c | 1 +
5 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
index 96e2e4016250..dbd3d4765065 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_product.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
@@ -18,7 +18,8 @@
#define MALIDP_CORE_ID_STATUS(__core_id) (((__u32)(__core_id)) & 0xFF)

/* Mali-display product IDs */
-#define MALIDP_D71_PRODUCT_ID 0x0071
+#define MALIDP_D71_PRODUCT_ID 0x0071
+#define MALIDP_D32_PRODUCT_ID 0x0032

union komeda_config_id {
struct {
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 6dadf4413ef3..c7f7e9c545c7 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -1274,7 +1274,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,

ctrlr = to_ctrlr(c);

- ctrlr->supports_dual_link = true;
+ ctrlr->supports_dual_link = d71->supports_dual_link;

return 0;
}
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index 9b3bf353b6cc..2d429e310e5b 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -371,23 +371,33 @@ static int d71_enum_resources(struct komeda_dev *mdev)
goto err_cleanup;
}

- /* probe PERIPH */
+ /* Only the legacy HW has the periph block, the newer merges the periph
+ * into GCU
+ */
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));
- err = -EINVAL;
- goto err_cleanup;
+ if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH)
+ d71->periph_addr = NULL;
+
+ if (d71->periph_addr) {
+ /* probe PERIPHERAL in legacy HW */
+ 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);
+ d71->integrates_tbu = !!(value & PERIPH_TBU_EN);
+ } else {
+ value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID0);
+ d71->max_line_size = GCU_MAX_LINE_SIZE(value);
+ d71->max_vsize = GCU_MAX_NUM_LINES(value);
+
+ value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID1);
+ d71->num_rich_layers = GCU_NUM_RICH_LAYERS(value);
+ d71->supports_dual_link = GCU_DISPLAY_SPLIT_EN(value);
+ d71->integrates_tbu = GCU_DISPLAY_TBU_EN(value);
}

- 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),
&d71_pipeline_funcs);
@@ -415,7 +425,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
}

/* loop the register blks and probe */
- i = 2; /* exclude GCU and PERIPH */
+ i = 1; /* exclude GCU */
offset = D71_BLOCK_SIZE; /* skip GCU */
while (i < d71->num_blocks) {
blk_base = mdev->reg_base + (offset >> 2);
@@ -425,9 +435,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
err = d71_probe_block(d71, &blk, blk_base);
if (err)
goto err_cleanup;
- i++;
}

+ i++;
offset += D71_BLOCK_SIZE;
}

@@ -603,6 +613,7 @@ d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)

switch (product_id) {
case MALIDP_D71_PRODUCT_ID:
+ case MALIDP_D32_PRODUCT_ID:
funcs = &d71_chip_funcs;
break;
default:
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
index 1727dc993909..81de6a23e7f3 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
@@ -72,6 +72,19 @@
#define GCU_CONTROL_MODE(x) ((x) & 0x7)
#define GCU_CONTROL_SRST BIT(16)

+/* GCU_CONFIGURATION registers */
+#define GCU_CONFIGURATION_ID0 0x100
+#define GCU_CONFIGURATION_ID1 0x104
+
+/* GCU configuration */
+#define GCU_MAX_LINE_SIZE(x) ((x) & 0xFFFF)
+#define GCU_MAX_NUM_LINES(x) ((x) >> 16)
+#define GCU_NUM_RICH_LAYERS(x) ((x) & 0x7)
+#define GCU_NUM_PIPELINES(x) (((x) >> 3) & 0x7)
+#define GCU_NUM_SCALERS(x) (((x) >> 6) & 0x7)
+#define GCU_DISPLAY_SPLIT_EN(x) (((x) >> 16) & 0x1)
+#define GCU_DISPLAY_TBU_EN(x) (((x) >> 17) & 0x1)
+
/* GCU opmode */
#define INACTIVE_MODE 0
#define TBU_CONNECT_MODE 1
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index b7a1097c45c4..ad38bbc7431e 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -125,6 +125,7 @@ static int komeda_platform_remove(struct platform_device *pdev)

static const struct of_device_id komeda_of_match[] = {
{ .compatible = "arm,mali-d71", .data = d71_identify, },
+ { .compatible = "arm,mali-d32", .data = d71_identify, },
{},
};

--
2.20.1

2019-11-21 08:19:56

by James Qian Wang

Subject: [PATCH v2 1/2] drm/komeda: Update the chip identify

1. Drop komeda-CORE product id comparison and put it into the d71_identify
2. Update pipeline node DT-binding:
(a). Skip the needless pipeline DT node.
(b). Return fail if the essential pipeline DT node is missing.

With these changes, for one family chips no need to change the DT.

v2: Rebase

Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
---
.../gpu/drm/arm/display/komeda/d71/d71_dev.c | 27 +++++++--
.../gpu/drm/arm/display/komeda/komeda_dev.c | 60 ++++++++++---------
.../gpu/drm/arm/display/komeda/komeda_dev.h | 14 +----
.../gpu/drm/arm/display/komeda/komeda_drv.c | 9 +--
4 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index 822b23a1ce75..9b3bf353b6cc 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -594,10 +594,27 @@ static const struct komeda_dev_funcs d71_chip_funcs = {
const struct komeda_dev_funcs *
d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
{
- chip->arch_id = malidp_read32(reg_base, GLB_ARCH_ID);
- chip->core_id = malidp_read32(reg_base, GLB_CORE_ID);
- chip->core_info = malidp_read32(reg_base, GLB_CORE_INFO);
- chip->bus_width = D71_BUS_WIDTH_16_BYTES;
+ const struct komeda_dev_funcs *funcs;
+ u32 product_id;

- return &d71_chip_funcs;
+ chip->core_id = malidp_read32(reg_base, GLB_CORE_ID);
+
+ product_id = MALIDP_CORE_ID_PRODUCT_ID(chip->core_id);
+
+ switch (product_id) {
+ case MALIDP_D71_PRODUCT_ID:
+ funcs = &d71_chip_funcs;
+ break;
+ default:
+ funcs = NULL;
+ DRM_ERROR("Unsupported product: 0x%x\n", product_id);
+ }
+
+ if (funcs) {
+ chip->arch_id = malidp_read32(reg_base, GLB_ARCH_ID);
+ chip->core_info = malidp_read32(reg_base, GLB_CORE_INFO);
+ chip->bus_width = D71_BUS_WIDTH_16_BYTES;
+ }
+
+ return funcs;
}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index 4dd4699d4e3d..8e0bce46555b 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -116,22 +116,14 @@ static struct attribute_group komeda_sysfs_attr_group = {
.attrs = komeda_sysfs_entries,
};

-static int komeda_parse_pipe_dt(struct komeda_dev *mdev, struct device_node *np)
+static int komeda_parse_pipe_dt(struct komeda_pipeline *pipe)
{
- struct komeda_pipeline *pipe;
+ struct device_node *np = pipe->of_node;
struct clk *clk;
- u32 pipe_id;
- int ret = 0;
-
- ret = of_property_read_u32(np, "reg", &pipe_id);
- if (ret != 0 || pipe_id >= mdev->n_pipelines)
- return -EINVAL;
-
- pipe = mdev->pipelines[pipe_id];

clk = of_clk_get_by_name(np, "pxclk");
if (IS_ERR(clk)) {
- DRM_ERROR("get pxclk for pipeline %d failed!\n", pipe_id);
+ DRM_ERROR("get pxclk for pipeline %d failed!\n", pipe->id);
return PTR_ERR(clk);
}
pipe->pxlclk = clk;
@@ -145,7 +137,6 @@ static int komeda_parse_pipe_dt(struct komeda_dev *mdev, struct device_node *np)
of_graph_get_port_by_id(np, KOMEDA_OF_PORT_OUTPUT);

pipe->dual_link = pipe->of_output_links[0] && pipe->of_output_links[1];
- pipe->of_node = of_node_get(np);

return 0;
}
@@ -154,7 +145,9 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
{
struct platform_device *pdev = to_platform_device(dev);
struct device_node *child, *np = dev->of_node;
- int ret;
+ struct komeda_pipeline *pipe;
+ u32 pipe_id = U32_MAX;
+ int ret = -1;

mdev->irq = platform_get_irq(pdev, 0);
if (mdev->irq < 0) {
@@ -169,31 +162,44 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
ret = 0;

for_each_available_child_of_node(np, child) {
- if (of_node_cmp(child->name, "pipeline") == 0) {
- ret = komeda_parse_pipe_dt(mdev, child);
- if (ret) {
- DRM_ERROR("parse pipeline dt error!\n");
- of_node_put(child);
- break;
+ if (of_node_name_eq(child, "pipeline")) {
+ of_property_read_u32(child, "reg", &pipe_id);
+ if (pipe_id >= mdev->n_pipelines) {
+ DRM_WARN("Skip the redundant DT node: pipeline-%u.\n",
+ pipe_id);
+ continue;
}
+ mdev->pipelines[pipe_id]->of_node = of_node_get(child);
}
}

+ for (pipe_id = 0; pipe_id < mdev->n_pipelines; pipe_id++) {
+ pipe = mdev->pipelines[pipe_id];
+
+ if (!pipe->of_node) {
+ DRM_ERROR("Omit DT node for pipeline-%d.\n", pipe->id);
+ return -EINVAL;
+ }
+ ret = komeda_parse_pipe_dt(pipe);
+ if (ret)
+ return ret;
+ }
+
mdev->side_by_side = !of_property_read_u32(np, "side_by_side_master",
&mdev->side_by_side_master);

- return ret;
+ return 0;
}

struct komeda_dev *komeda_dev_create(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- const struct komeda_product_data *product;
+ komeda_identify_func komeda_identify;
struct komeda_dev *mdev;
int err = 0;

- product = of_device_get_match_data(dev);
- if (!product)
+ komeda_identify = of_device_get_match_data(dev);
+ if (!komeda_identify)
return ERR_PTR(-ENODEV);

mdev = devm_kzalloc(dev, sizeof(*mdev), GFP_KERNEL);
@@ -221,11 +227,9 @@ struct komeda_dev *komeda_dev_create(struct device *dev)

clk_prepare_enable(mdev->aclk);

- mdev->funcs = product->identify(mdev->reg_base, &mdev->chip);
- if (!komeda_product_match(mdev, product->product_id)) {
- DRM_ERROR("DT configured %x mismatch with real HW %x.\n",
- product->product_id,
- MALIDP_CORE_ID_PRODUCT_ID(mdev->chip.core_id));
+ mdev->funcs = komeda_identify(mdev->reg_base, &mdev->chip);
+ if (!mdev->funcs) {
+ DRM_ERROR("Failed to identify the HW.\n");
err = -ENODEV;
goto disable_clk;
}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
index 471604b42431..dacdb00153e9 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
@@ -58,11 +58,6 @@
| KOMEDA_EVENT_MODE \
)

-/* malidp device id */
-enum {
- MALI_D71 = 0,
-};
-
/* pipeline DT ports */
enum {
KOMEDA_OF_PORT_OUTPUT = 0,
@@ -76,12 +71,6 @@ struct komeda_chip_info {
u32 bus_width;
};

-struct komeda_product_data {
- u32 product_id;
- const struct komeda_dev_funcs *(*identify)(u32 __iomem *reg,
- struct komeda_chip_info *info);
-};
-
struct komeda_dev;

struct komeda_events {
@@ -243,6 +232,9 @@ komeda_product_match(struct komeda_dev *mdev, u32 target)
return MALIDP_CORE_ID_PRODUCT_ID(mdev->chip.core_id) == target;
}

+typedef const struct komeda_dev_funcs *
+(*komeda_identify_func)(u32 __iomem *reg, struct komeda_chip_info *chip);
+
const struct komeda_dev_funcs *
d71_identify(u32 __iomem *reg, struct komeda_chip_info *chip);

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index d6c2222c5d33..b7a1097c45c4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -123,15 +123,8 @@ static int komeda_platform_remove(struct platform_device *pdev)
return 0;
}

-static const struct komeda_product_data komeda_products[] = {
- [MALI_D71] = {
- .product_id = MALIDP_D71_PRODUCT_ID,
- .identify = d71_identify,
- },
-};
-
static const struct of_device_id komeda_of_match[] = {
- { .compatible = "arm,mali-d71", .data = &komeda_products[MALI_D71], },
+ { .compatible = "arm,mali-d71", .data = d71_identify, },
{},
};

--
2.20.1

2019-12-02 11:09:40

by Mihail Atanassov

Subject: Re: [PATCH v2 1/2] drm/komeda: Update the chip identify

On Thursday, 21 November 2019 08:17:39 GMT james qian wang (Arm Technology China) wrote:
> 1. Drop komeda-CORE product id comparison and put it into the d71_identify
> 2. Update pipeline node DT-binding:
> (a). Skip the needless pipeline DT node.
> (b). Return fail if the essential pipeline DT node is missing.
>
> With these changes, for one family chips no need to change the DT.
>
> v2: Rebase
>
> Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> ---
> .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 27 +++++++--
> .../gpu/drm/arm/display/komeda/komeda_dev.c | 60 ++++++++++---------
> .../gpu/drm/arm/display/komeda/komeda_dev.h | 14 +----
> .../gpu/drm/arm/display/komeda/komeda_drv.c | 9 +--
> 4 files changed, 58 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> index 822b23a1ce75..9b3bf353b6cc 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> @@ -594,10 +594,27 @@ static const struct komeda_dev_funcs d71_chip_funcs = {
> const struct komeda_dev_funcs *
> d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
> {
> - chip->arch_id = malidp_read32(reg_base, GLB_ARCH_ID);
> - chip->core_id = malidp_read32(reg_base, GLB_CORE_ID);
> - chip->core_info = malidp_read32(reg_base, GLB_CORE_INFO);
> - chip->bus_width = D71_BUS_WIDTH_16_BYTES;
> + const struct komeda_dev_funcs *funcs;
> + u32 product_id;
>
> - return &d71_chip_funcs;
> + chip->core_id = malidp_read32(reg_base, GLB_CORE_ID);
> +
> + product_id = MALIDP_CORE_ID_PRODUCT_ID(chip->core_id);
> +
> + switch (product_id) {
> + case MALIDP_D71_PRODUCT_ID:
> + funcs = &d71_chip_funcs;
> + break;
> + default:
> + funcs = NULL;

[bikeshed] I'd just 'return NULL;' after printing the error...

> + DRM_ERROR("Unsupported product: 0x%x\n", product_id);
> + }
> +
> + if (funcs) {

... and save myself the branch and indent level here.

> + chip->arch_id = malidp_read32(reg_base, GLB_ARCH_ID);
> + chip->core_info = malidp_read32(reg_base, GLB_CORE_INFO);
> + chip->bus_width = D71_BUS_WIDTH_16_BYTES;
> + }
> +
> + return funcs;
> }
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index 4dd4699d4e3d..8e0bce46555b 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -116,22 +116,14 @@ static struct attribute_group komeda_sysfs_attr_group = {
> .attrs = komeda_sysfs_entries,
> };
>
> -static int komeda_parse_pipe_dt(struct komeda_dev *mdev, struct device_node *np)
> +static int komeda_parse_pipe_dt(struct komeda_pipeline *pipe)
> {
> - struct komeda_pipeline *pipe;
> + struct device_node *np = pipe->of_node;
> struct clk *clk;
> - u32 pipe_id;
> - int ret = 0;
> -
> - ret = of_property_read_u32(np, "reg", &pipe_id);
> - if (ret != 0 || pipe_id >= mdev->n_pipelines)
> - return -EINVAL;
> -
> - pipe = mdev->pipelines[pipe_id];
>
> clk = of_clk_get_by_name(np, "pxclk");
> if (IS_ERR(clk)) {
> - DRM_ERROR("get pxclk for pipeline %d failed!\n", pipe_id);
> + DRM_ERROR("get pxclk for pipeline %d failed!\n", pipe->id);
> return PTR_ERR(clk);
> }
> pipe->pxlclk = clk;
> @@ -145,7 +137,6 @@ static int komeda_parse_pipe_dt(struct komeda_dev *mdev, struct device_node *np)
> of_graph_get_port_by_id(np, KOMEDA_OF_PORT_OUTPUT);
>
> pipe->dual_link = pipe->of_output_links[0] && pipe->of_output_links[1];
> - pipe->of_node = of_node_get(np);
>
> return 0;
> }
> @@ -154,7 +145,9 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct device_node *child, *np = dev->of_node;
> - int ret;
> + struct komeda_pipeline *pipe;
> + u32 pipe_id = U32_MAX;
> + int ret = -1;
>
> mdev->irq = platform_get_irq(pdev, 0);
> if (mdev->irq < 0) {
> @@ -169,31 +162,44 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
> ret = 0;
>
> for_each_available_child_of_node(np, child) {
> - if (of_node_cmp(child->name, "pipeline") == 0) {
> - ret = komeda_parse_pipe_dt(mdev, child);
> - if (ret) {
> - DRM_ERROR("parse pipeline dt error!\n");
> - of_node_put(child);
> - break;
> + if (of_node_name_eq(child, "pipeline")) {
> + of_property_read_u32(child, "reg", &pipe_id);
> + if (pipe_id >= mdev->n_pipelines) {
> + DRM_WARN("Skip the redundant DT node: pipeline-%u.\n",
> + pipe_id);
> + continue;
> }
> + mdev->pipelines[pipe_id]->of_node = of_node_get(child);
> }
> }
>
> + for (pipe_id = 0; pipe_id < mdev->n_pipelines; pipe_id++) {
> + pipe = mdev->pipelines[pipe_id];
> +
> + if (!pipe->of_node) {
> + DRM_ERROR("Omit DT node for pipeline-%d.\n", pipe->id);

[nit] "Omit DT node" doesn't sound like an error condition. How about:

"pipeline-%d doesn't have a DT node."

> + return -EINVAL;
> + }
> + ret = komeda_parse_pipe_dt(pipe);
> + if (ret)
> + return ret;
> + }
> +
> mdev->side_by_side = !of_property_read_u32(np, "side_by_side_master",

Looks like this isn't based off drm-misc-next, and is instead based on
https://patchwork.freedesktop.org/patch/341867/

> &mdev->side_by_side_master);
>
> - return ret;
> + return 0;
> }
>
> struct komeda_dev *komeda_dev_create(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> - const struct komeda_product_data *product;
> + komeda_identify_func komeda_identify;
> struct komeda_dev *mdev;
> int err = 0;
>
> - product = of_device_get_match_data(dev);
> - if (!product)
> + komeda_identify = of_device_get_match_data(dev);
> + if (!komeda_identify)
> return ERR_PTR(-ENODEV);
>
> mdev = devm_kzalloc(dev, sizeof(*mdev), GFP_KERNEL);
> @@ -221,11 +227,9 @@ struct komeda_dev *komeda_dev_create(struct device *dev)
>
> clk_prepare_enable(mdev->aclk);
>
> - mdev->funcs = product->identify(mdev->reg_base, &mdev->chip);
> - if (!komeda_product_match(mdev, product->product_id)) {
> - DRM_ERROR("DT configured %x mismatch with real HW %x.\n",
> - product->product_id,
> - MALIDP_CORE_ID_PRODUCT_ID(mdev->chip.core_id));
> + mdev->funcs = komeda_identify(mdev->reg_base, &mdev->chip);
> + if (!mdev->funcs) {
> + DRM_ERROR("Failed to identify the HW.\n");
> err = -ENODEV;
> goto disable_clk;
> }
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index 471604b42431..dacdb00153e9 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -58,11 +58,6 @@
> | KOMEDA_EVENT_MODE \
> )
>
> -/* malidp device id */
> -enum {
> - MALI_D71 = 0,
> -};
> -
> /* pipeline DT ports */
> enum {
> KOMEDA_OF_PORT_OUTPUT = 0,
> @@ -76,12 +71,6 @@ struct komeda_chip_info {
> u32 bus_width;
> };
>
> -struct komeda_product_data {
> - u32 product_id;
> - const struct komeda_dev_funcs *(*identify)(u32 __iomem *reg,
> - struct komeda_chip_info *info);
> -};
> -
> struct komeda_dev;
>
> struct komeda_events {
> @@ -243,6 +232,9 @@ komeda_product_match(struct komeda_dev *mdev, u32 target)
> return MALIDP_CORE_ID_PRODUCT_ID(mdev->chip.core_id) == target;
> }
>
> +typedef const struct komeda_dev_funcs *
> +(*komeda_identify_func)(u32 __iomem *reg, struct komeda_chip_info *chip);
> +
> const struct komeda_dev_funcs *
> d71_identify(u32 __iomem *reg, struct komeda_chip_info *chip);
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index d6c2222c5d33..b7a1097c45c4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -123,15 +123,8 @@ static int komeda_platform_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct komeda_product_data komeda_products[] = {
> - [MALI_D71] = {
> - .product_id = MALIDP_D71_PRODUCT_ID,
> - .identify = d71_identify,
> - },
> -};
> -
> static const struct of_device_id komeda_of_match[] = {
> - { .compatible = "arm,mali-d71", .data = &komeda_products[MALI_D71], },
> + { .compatible = "arm,mali-d71", .data = d71_identify, },
> {},
> };
>
>

With the above two fixed (i.e. feel free to ignore the bikeshed),
Reviewed-by: Mihail Atanassov <[email protected]>

--
Mihail

2019-12-02 11:09:54

by Mihail Atanassov

Subject: Re: [PATCH v2 2/2] drm/komeda: Enable new product D32 support

On Thursday, 21 November 2019 08:17:45 GMT james qian wang (Arm Technology China) wrote:
> D32 is simple version of D71, the difference is:
> - Only has one pipeline
> - Drop the periph block and merge it to GCU
>
> v2: Rebase.
>
> Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> ---
> .../drm/arm/display/include/malidp_product.h | 3 +-
> .../arm/display/komeda/d71/d71_component.c | 2 +-
> .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 43 ++++++++++++-------
> .../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++++
> .../gpu/drm/arm/display/komeda/komeda_drv.c | 1 +
> 5 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> index 96e2e4016250..dbd3d4765065 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> @@ -18,7 +18,8 @@
> #define MALIDP_CORE_ID_STATUS(__core_id) (((__u32)(__core_id)) & 0xFF)
>
> /* Mali-display product IDs */
> -#define MALIDP_D71_PRODUCT_ID 0x0071
> +#define MALIDP_D71_PRODUCT_ID 0x0071
> +#define MALIDP_D32_PRODUCT_ID 0x0032
>
> union komeda_config_id {
> struct {
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index 6dadf4413ef3..c7f7e9c545c7 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -1274,7 +1274,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
>
> ctrlr = to_ctrlr(c);
>
> - ctrlr->supports_dual_link = true;
> + ctrlr->supports_dual_link = d71->supports_dual_link;
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> index 9b3bf353b6cc..2d429e310e5b 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> @@ -371,23 +371,33 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> goto err_cleanup;
> }
>
> - /* probe PERIPH */
> + /* Only the legacy HW has the periph block, the newer merges the periph
> + * into GCU
> + */
> 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));
> - err = -EINVAL;
> - goto err_cleanup;
> + if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH)
> + d71->periph_addr = NULL;
> +
> + if (d71->periph_addr) {
> + /* probe PERIPHERAL in legacy HW */
> + 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);
> + d71->integrates_tbu = !!(value & PERIPH_TBU_EN);
> + } else {
> + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID0);
> + d71->max_line_size = GCU_MAX_LINE_SIZE(value);
> + d71->max_vsize = GCU_MAX_NUM_LINES(value);
> +
> + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID1);
> + d71->num_rich_layers = GCU_NUM_RICH_LAYERS(value);
> + d71->supports_dual_link = GCU_DISPLAY_SPLIT_EN(value);
> + d71->integrates_tbu = GCU_DISPLAY_TBU_EN(value);
> }
>
> - 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),
> &d71_pipeline_funcs);
> @@ -415,7 +425,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> }
>
> /* loop the register blks and probe */
> - i = 2; /* exclude GCU and PERIPH */
> + i = 1; /* exclude GCU */
> offset = D71_BLOCK_SIZE; /* skip GCU */
> while (i < d71->num_blocks) {
> blk_base = mdev->reg_base + (offset >> 2);
> @@ -425,9 +435,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> err = d71_probe_block(d71, &blk, blk_base);
> if (err)
> goto err_cleanup;
> - i++;
> }
>
> + i++;

This change doesn't make much sense if you want to count how many
blocks are available, since you're now counting the reserved ones, too.

On the counting note, the prior code rides on the assumption the periph
block is last in the map, and I don't see how you still handle not
counting it in the D71 case.

> offset += D71_BLOCK_SIZE;
> }
>
> @@ -603,6 +613,7 @@ d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
>
> switch (product_id) {
> case MALIDP_D71_PRODUCT_ID:
> + case MALIDP_D32_PRODUCT_ID:
> funcs = &d71_chip_funcs;
> break;
> default:
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> index 1727dc993909..81de6a23e7f3 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> @@ -72,6 +72,19 @@
> #define GCU_CONTROL_MODE(x) ((x) & 0x7)
> #define GCU_CONTROL_SRST BIT(16)
>
> +/* GCU_CONFIGURATION registers */
> +#define GCU_CONFIGURATION_ID0 0x100
> +#define GCU_CONFIGURATION_ID1 0x104
> +
> +/* GCU configuration */
> +#define GCU_MAX_LINE_SIZE(x) ((x) & 0xFFFF)
> +#define GCU_MAX_NUM_LINES(x) ((x) >> 16)
> +#define GCU_NUM_RICH_LAYERS(x) ((x) & 0x7)
> +#define GCU_NUM_PIPELINES(x) (((x) >> 3) & 0x7)
> +#define GCU_NUM_SCALERS(x) (((x) >> 6) & 0x7)
> +#define GCU_DISPLAY_SPLIT_EN(x) (((x) >> 16) & 0x1)
> +#define GCU_DISPLAY_TBU_EN(x) (((x) >> 17) & 0x1)
> +
> /* GCU opmode */
> #define INACTIVE_MODE 0
> #define TBU_CONNECT_MODE 1
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index b7a1097c45c4..ad38bbc7431e 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -125,6 +125,7 @@ static int komeda_platform_remove(struct platform_device *pdev)
>
> static const struct of_device_id komeda_of_match[] = {
> { .compatible = "arm,mali-d71", .data = d71_identify, },
> + { .compatible = "arm,mali-d32", .data = d71_identify, },
> {},
> };
>
>


--
Mihail

2019-12-03 06:47:32

by James Qian Wang

Subject: Re: [PATCH v2 2/2] drm/komeda: Enable new product D32 support

On Mon, Dec 02, 2019 at 11:07:52AM +0000, Mihail Atanassov wrote:
> On Thursday, 21 November 2019 08:17:45 GMT james qian wang (Arm Technology China) wrote:
> > D32 is simple version of D71, the difference is:
> > - Only has one pipeline
> > - Drop the periph block and merge it to GCU
> >
> > v2: Rebase.
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> > ---
> > .../drm/arm/display/include/malidp_product.h | 3 +-
> > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 43 ++++++++++++-------
> > .../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++++
> > .../gpu/drm/arm/display/komeda/komeda_drv.c | 1 +
> > 5 files changed, 44 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > index 96e2e4016250..dbd3d4765065 100644
> > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > @@ -18,7 +18,8 @@
> > #define MALIDP_CORE_ID_STATUS(__core_id) (((__u32)(__core_id)) & 0xFF)
> >
> > /* Mali-display product IDs */
> > -#define MALIDP_D71_PRODUCT_ID 0x0071
> > +#define MALIDP_D71_PRODUCT_ID 0x0071
> > +#define MALIDP_D32_PRODUCT_ID 0x0032
> >
> > union komeda_config_id {
> > struct {
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > index 6dadf4413ef3..c7f7e9c545c7 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > @@ -1274,7 +1274,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> >
> > ctrlr = to_ctrlr(c);
> >
> > - ctrlr->supports_dual_link = true;
> > + ctrlr->supports_dual_link = d71->supports_dual_link;
> >
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > index 9b3bf353b6cc..2d429e310e5b 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > @@ -371,23 +371,33 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > goto err_cleanup;
> > }
> >
> > - /* probe PERIPH */
> > + /* Only the legacy HW has the periph block, the newer merges the periph
> > + * into GCU
> > + */
> > 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));
> > - err = -EINVAL;
> > - goto err_cleanup;
> > + if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH)
> > + d71->periph_addr = NULL;
> > +
> > + if (d71->periph_addr) {
> > + /* probe PERIPHERAL in legacy HW */
> > + 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);
> > + d71->integrates_tbu = !!(value & PERIPH_TBU_EN);
> > + } else {
> > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID0);
> > + d71->max_line_size = GCU_MAX_LINE_SIZE(value);
> > + d71->max_vsize = GCU_MAX_NUM_LINES(value);
> > +
> > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID1);
> > + d71->num_rich_layers = GCU_NUM_RICH_LAYERS(value);
> > + d71->supports_dual_link = GCU_DISPLAY_SPLIT_EN(value);
> > + d71->integrates_tbu = GCU_DISPLAY_TBU_EN(value);
> > }
> >
> > - 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),
> > &d71_pipeline_funcs);
> > @@ -415,7 +425,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > }
> >
> > /* loop the register blks and probe */
> > - i = 2; /* exclude GCU and PERIPH */
> > + i = 1; /* exclude GCU */
> > offset = D71_BLOCK_SIZE; /* skip GCU */
> > while (i < d71->num_blocks) {
> > blk_base = mdev->reg_base + (offset >> 2);
> > @@ -425,9 +435,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > err = d71_probe_block(d71, &blk, blk_base);
> > if (err)
> > goto err_cleanup;
> > - i++;
> > }
> >
> > + i++;
>
> This change doesn't make much sense if you want to count how many
> blocks are available, since you're now counting the reserved ones, too.

That's because for D32 num_blocks includes the reserved blocks.

> On the counting note, the prior code rides on the assumption the periph
> block is last in the map, and I don't see how you still handle not
> counting it in the D71 case.

Since D71 has one reserved block, and now we count reserved block.

So now no matter D32 or D71:
num_blocks = n_valid_block + n_reserved_block + GCU.

Thanks
James
>
> > offset += D71_BLOCK_SIZE;
> > }
> >
> > @@ -603,6 +613,7 @@ d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
> >
> > switch (product_id) {
> > case MALIDP_D71_PRODUCT_ID:
> > + case MALIDP_D32_PRODUCT_ID:
> > funcs = &d71_chip_funcs;
> > break;
> > default:
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > index 1727dc993909..81de6a23e7f3 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > @@ -72,6 +72,19 @@
> > #define GCU_CONTROL_MODE(x) ((x) & 0x7)
> > #define GCU_CONTROL_SRST BIT(16)
> >
> > +/* GCU_CONFIGURATION registers */
> > +#define GCU_CONFIGURATION_ID0 0x100
> > +#define GCU_CONFIGURATION_ID1 0x104
> > +
> > +/* GCU configuration */
> > +#define GCU_MAX_LINE_SIZE(x) ((x) & 0xFFFF)
> > +#define GCU_MAX_NUM_LINES(x) ((x) >> 16)
> > +#define GCU_NUM_RICH_LAYERS(x) ((x) & 0x7)
> > +#define GCU_NUM_PIPELINES(x) (((x) >> 3) & 0x7)
> > +#define GCU_NUM_SCALERS(x) (((x) >> 6) & 0x7)
> > +#define GCU_DISPLAY_SPLIT_EN(x) (((x) >> 16) & 0x1)
> > +#define GCU_DISPLAY_TBU_EN(x) (((x) >> 17) & 0x1)
> > +
> > /* GCU opmode */
> > #define INACTIVE_MODE 0
> > #define TBU_CONNECT_MODE 1
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > index b7a1097c45c4..ad38bbc7431e 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > @@ -125,6 +125,7 @@ static int komeda_platform_remove(struct platform_device *pdev)
> >
> > static const struct of_device_id komeda_of_match[] = {
> > { .compatible = "arm,mali-d71", .data = d71_identify, },
> > + { .compatible = "arm,mali-d32", .data = d71_identify, },
> > {},
> > };
> >
> >
>
>
> --
> Mihail
>
>

2019-12-03 06:54:50

by James Qian Wang

Subject: Re: [PATCH v2 1/2] drm/komeda: Update the chip identify

On Mon, Dec 02, 2019 at 11:07:54AM +0000, Mihail Atanassov wrote:
> On Thursday, 21 November 2019 08:17:39 GMT james qian wang (Arm Technology China) wrote:
> > 1. Drop komeda-CORE product id comparison and put it into the d71_identify
> > 2. Update pipeline node DT-binding:
> > (a). Skip the needless pipeline DT node.
> > (b). Return fail if the essential pipeline DT node is missing.
> >
> > With these changes, for one family chips no need to change the DT.
> >
> > v2: Rebase
> >
> > Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> > ---
> > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 27 +++++++--
> > .../gpu/drm/arm/display/komeda/komeda_dev.c | 60 ++++++++++---------
> > .../gpu/drm/arm/display/komeda/komeda_dev.h | 14 +----
> > .../gpu/drm/arm/display/komeda/komeda_drv.c | 9 +--
> > 4 files changed, 58 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > index 822b23a1ce75..9b3bf353b6cc 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > @@ -594,10 +594,27 @@ static const struct komeda_dev_funcs d71_chip_funcs = {
> > const struct komeda_dev_funcs *
> > d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
> > {
> > - chip->arch_id = malidp_read32(reg_base, GLB_ARCH_ID);
> > - chip->core_id = malidp_read32(reg_base, GLB_CORE_ID);
> > - chip->core_info = malidp_read32(reg_base, GLB_CORE_INFO);
> > - chip->bus_width = D71_BUS_WIDTH_16_BYTES;
> > + const struct komeda_dev_funcs *funcs;
> > + u32 product_id;
> >
> > - return &d71_chip_funcs;
> > + chip->core_id = malidp_read32(reg_base, GLB_CORE_ID);
> > +
> > + product_id = MALIDP_CORE_ID_PRODUCT_ID(chip->core_id);
> > +
> > + switch (product_id) {
> > + case MALIDP_D71_PRODUCT_ID:
> > + funcs = &d71_chip_funcs;
> > + break;
> > + default:
> > + funcs = NULL;
>
> [bikeshed] I'd just 'return NULL;' after printing the error...

Good idea, and then no need to check the func in the following code.

> > + DRM_ERROR("Unsupported product: 0x%x\n", product_id);
> > + }
> > +
> > + if (funcs) {
>
> ... and save myself the branch and indent level here.
>
> > + chip->arch_id = malidp_read32(reg_base, GLB_ARCH_ID);
> > + chip->core_info = malidp_read32(reg_base, GLB_CORE_INFO);
> > + chip->bus_width = D71_BUS_WIDTH_16_BYTES;
> > + }
> > +
> > + return funcs;
> > }
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > index 4dd4699d4e3d..8e0bce46555b 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> > @@ -116,22 +116,14 @@ static struct attribute_group komeda_sysfs_attr_group = {
> > .attrs = komeda_sysfs_entries,
> > };
> >
> > -static int komeda_parse_pipe_dt(struct komeda_dev *mdev, struct device_node *np)
> > +static int komeda_parse_pipe_dt(struct komeda_pipeline *pipe)
> > {
> > - struct komeda_pipeline *pipe;
> > + struct device_node *np = pipe->of_node;
> > struct clk *clk;
> > - u32 pipe_id;
> > - int ret = 0;
> > -
> > - ret = of_property_read_u32(np, "reg", &pipe_id);
> > - if (ret != 0 || pipe_id >= mdev->n_pipelines)
> > - return -EINVAL;
> > -
> > - pipe = mdev->pipelines[pipe_id];
> >
> > clk = of_clk_get_by_name(np, "pxclk");
> > if (IS_ERR(clk)) {
> > - DRM_ERROR("get pxclk for pipeline %d failed!\n", pipe_id);
> > + DRM_ERROR("get pxclk for pipeline %d failed!\n", pipe->id);
> > return PTR_ERR(clk);
> > }
> > pipe->pxlclk = clk;
> > @@ -145,7 +137,6 @@ static int komeda_parse_pipe_dt(struct komeda_dev *mdev, struct device_node *np)
> > of_graph_get_port_by_id(np, KOMEDA_OF_PORT_OUTPUT);
> >
> > pipe->dual_link = pipe->of_output_links[0] && pipe->of_output_links[1];
> > - pipe->of_node = of_node_get(np);
> >
> > return 0;
> > }
> > @@ -154,7 +145,9 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> > struct device_node *child, *np = dev->of_node;
> > - int ret;
> > + struct komeda_pipeline *pipe;
> > + u32 pipe_id = U32_MAX;
> > + int ret = -1;
> >
> > mdev->irq = platform_get_irq(pdev, 0);
> > if (mdev->irq < 0) {
> > @@ -169,31 +162,44 @@ static int komeda_parse_dt(struct device *dev, struct komeda_dev *mdev)
> > ret = 0;
> >
> > for_each_available_child_of_node(np, child) {
> > - if (of_node_cmp(child->name, "pipeline") == 0) {
> > - ret = komeda_parse_pipe_dt(mdev, child);
> > - if (ret) {
> > - DRM_ERROR("parse pipeline dt error!\n");
> > - of_node_put(child);
> > - break;
> > + if (of_node_name_eq(child, "pipeline")) {
> > + of_property_read_u32(child, "reg", &pipe_id);
> > + if (pipe_id >= mdev->n_pipelines) {
> > + DRM_WARN("Skip the redundant DT node: pipeline-%u.\n",
> > + pipe_id);
> > + continue;
> > }
> > + mdev->pipelines[pipe_id]->of_node = of_node_get(child);
> > }
> > }
> >
> > + for (pipe_id = 0; pipe_id < mdev->n_pipelines; pipe_id++) {
> > + pipe = mdev->pipelines[pipe_id];
> > +
> > + if (!pipe->of_node) {
> > + DRM_ERROR("Omit DT node for pipeline-%d.\n", pipe->id);
>
> [nit] "Omit DT node" doesn't sound like an error condition. How about:
>
> "pipeline-%d doesn't have a DT node."

Will do it in the next version.

>
> > + return -EINVAL;
> > + }
> > + ret = komeda_parse_pipe_dt(pipe);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > mdev->side_by_side = !of_property_read_u32(np, "side_by_side_master",
>
> Looks like this isn't based off drm-misc-next, and is instead based on
> https://patchwork.freedesktop.org/patch/341867/
>
> > &mdev->side_by_side_master);

OK, will rebase this series directly to drm-misc-next.

> >
> > - return ret;
> > + return 0;
> > }
> >
> > struct komeda_dev *komeda_dev_create(struct device *dev)
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> > - const struct komeda_product_data *product;
> > + komeda_identify_func komeda_identify;
> > struct komeda_dev *mdev;
> > int err = 0;
> >
> > - product = of_device_get_match_data(dev);
> > - if (!product)
> > + komeda_identify = of_device_get_match_data(dev);
> > + if (!komeda_identify)
> > return ERR_PTR(-ENODEV);
> >
> > mdev = devm_kzalloc(dev, sizeof(*mdev), GFP_KERNEL);
> > @@ -221,11 +227,9 @@ struct komeda_dev *komeda_dev_create(struct device *dev)
> >
> > clk_prepare_enable(mdev->aclk);
> >
> > - mdev->funcs = product->identify(mdev->reg_base, &mdev->chip);
> > - if (!komeda_product_match(mdev, product->product_id)) {
> > - DRM_ERROR("DT configured %x mismatch with real HW %x.\n",
> > - product->product_id,
> > - MALIDP_CORE_ID_PRODUCT_ID(mdev->chip.core_id));
> > + mdev->funcs = komeda_identify(mdev->reg_base, &mdev->chip);
> > + if (!mdev->funcs) {
> > + DRM_ERROR("Failed to identify the HW.\n");
> > err = -ENODEV;
> > goto disable_clk;
> > }
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > index 471604b42431..dacdb00153e9 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > @@ -58,11 +58,6 @@
> > | KOMEDA_EVENT_MODE \
> > )
> >
> > -/* malidp device id */
> > -enum {
> > - MALI_D71 = 0,
> > -};
> > -
> > /* pipeline DT ports */
> > enum {
> > KOMEDA_OF_PORT_OUTPUT = 0,
> > @@ -76,12 +71,6 @@ struct komeda_chip_info {
> > u32 bus_width;
> > };
> >
> > -struct komeda_product_data {
> > - u32 product_id;
> > - const struct komeda_dev_funcs *(*identify)(u32 __iomem *reg,
> > - struct komeda_chip_info *info);
> > -};
> > -
> > struct komeda_dev;
> >
> > struct komeda_events {
> > @@ -243,6 +232,9 @@ komeda_product_match(struct komeda_dev *mdev, u32 target)
> > return MALIDP_CORE_ID_PRODUCT_ID(mdev->chip.core_id) == target;
> > }
> >
> > +typedef const struct komeda_dev_funcs *
> > +(*komeda_identify_func)(u32 __iomem *reg, struct komeda_chip_info *chip);
> > +
> > const struct komeda_dev_funcs *
> > d71_identify(u32 __iomem *reg, struct komeda_chip_info *chip);
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > index d6c2222c5d33..b7a1097c45c4 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > @@ -123,15 +123,8 @@ static int komeda_platform_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > -static const struct komeda_product_data komeda_products[] = {
> > - [MALI_D71] = {
> > - .product_id = MALIDP_D71_PRODUCT_ID,
> > - .identify = d71_identify,
> > - },
> > -};
> > -
> > static const struct of_device_id komeda_of_match[] = {
> > - { .compatible = "arm,mali-d71", .data = &komeda_products[MALI_D71], },
> > + { .compatible = "arm,mali-d71", .data = d71_identify, },
> > {},
> > };
> >
> >
>
> With the above two fixed (i.e. feel free to ignore the bikeshed),
> Reviewed-by: Mihail Atanassov <[email protected]>
>
> --
> Mihail
>
>

2019-12-03 10:02:35

by Mihail Atanassov

Subject: Re: [PATCH v2 2/2] drm/komeda: Enable new product D32 support

On Tuesday, 3 December 2019 06:46:06 GMT james qian wang (Arm Technology China) wrote:
> On Mon, Dec 02, 2019 at 11:07:52AM +0000, Mihail Atanassov wrote:
> > On Thursday, 21 November 2019 08:17:45 GMT james qian wang (Arm Technology China) wrote:
> > > D32 is simple version of D71, the difference is:
> > > - Only has one pipeline
> > > - Drop the periph block and merge it to GCU
> > >
> > > v2: Rebase.
> > >
> > > Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> > > ---
> > > .../drm/arm/display/include/malidp_product.h | 3 +-
> > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 43 ++++++++++++-------
> > > .../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++++
> > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 1 +
> > > 5 files changed, 44 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > index 96e2e4016250..dbd3d4765065 100644
> > > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > @@ -18,7 +18,8 @@
> > > #define MALIDP_CORE_ID_STATUS(__core_id) (((__u32)(__core_id)) & 0xFF)
> > >
> > > /* Mali-display product IDs */
> > > -#define MALIDP_D71_PRODUCT_ID 0x0071
> > > +#define MALIDP_D71_PRODUCT_ID 0x0071
> > > +#define MALIDP_D32_PRODUCT_ID 0x0032
> > >
> > > union komeda_config_id {
> > > struct {
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > index 6dadf4413ef3..c7f7e9c545c7 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > @@ -1274,7 +1274,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> > >
> > > ctrlr = to_ctrlr(c);
> > >
> > > - ctrlr->supports_dual_link = true;
> > > + ctrlr->supports_dual_link = d71->supports_dual_link;
> > >
> > > return 0;
> > > }
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > index 9b3bf353b6cc..2d429e310e5b 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > @@ -371,23 +371,33 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > goto err_cleanup;
> > > }
> > >
> > > - /* probe PERIPH */
> > > + /* Only the legacy HW has the periph block, the newer merges the periph
> > > + * into GCU
> > > + */
> > > 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));
> > > - err = -EINVAL;
> > > - goto err_cleanup;
> > > + if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH)
> > > + d71->periph_addr = NULL;
> > > +
> > > + if (d71->periph_addr) {
> > > + /* probe PERIPHERAL in legacy HW */
> > > + 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);
> > > + d71->integrates_tbu = !!(value & PERIPH_TBU_EN);
> > > + } else {
> > > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID0);
> > > + d71->max_line_size = GCU_MAX_LINE_SIZE(value);
> > > + d71->max_vsize = GCU_MAX_NUM_LINES(value);
> > > +
> > > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID1);
> > > + d71->num_rich_layers = GCU_NUM_RICH_LAYERS(value);
> > > + d71->supports_dual_link = GCU_DISPLAY_SPLIT_EN(value);
> > > + d71->integrates_tbu = GCU_DISPLAY_TBU_EN(value);
> > > }
> > >
> > > - 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),
> > > &d71_pipeline_funcs);
> > > @@ -415,7 +425,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > }
> > >
> > > /* loop the register blks and probe */
> > > - i = 2; /* exclude GCU and PERIPH */
> > > + i = 1; /* exclude GCU */
> > > offset = D71_BLOCK_SIZE; /* skip GCU */
> > > while (i < d71->num_blocks) {
> > > blk_base = mdev->reg_base + (offset >> 2);
> > > @@ -425,9 +435,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > err = d71_probe_block(d71, &blk, blk_base);
> > > if (err)
> > > goto err_cleanup;
> > > - i++;
> > > }
> > >
> > > + i++;
> >
> > This change doesn't make much sense if you want to count how many
> > blocks are available, since you're now counting the reserved ones, too.
>
> That's because for D32 num_blocks includes the reserved blocks.
>
> > On the counting note, the prior code rides on the assumption the periph
> > block is last in the map, and I don't see how you still handle not
> > counting it in the D71 case.
>
> Since D71 has one reserved block, and now we count reserved block.
>
> So now no matter D32 or D71:
> num_blocks = n_valid_block + n_reserved_block + GCU.

So at least we need that comment dropped in with the code. Future HW
might break your assumption here once more and it will then be useful
to know why this works for both products.

I'd personally abstract that a bit behind a small helper func and
handle different products separately. It's a bit of duplication but
much easier to comprehend. Saved cycles reading code == Good Thing(tm).

>
> Thanks
> James
> >
> > > offset += D71_BLOCK_SIZE;
> > > }
> > >
> > > @@ -603,6 +613,7 @@ d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
> > >
> > > switch (product_id) {
> > > case MALIDP_D71_PRODUCT_ID:
> > > + case MALIDP_D32_PRODUCT_ID:
> > > funcs = &d71_chip_funcs;
> > > break;
> > > default:
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > index 1727dc993909..81de6a23e7f3 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > @@ -72,6 +72,19 @@
> > > #define GCU_CONTROL_MODE(x) ((x) & 0x7)
> > > #define GCU_CONTROL_SRST BIT(16)
> > >
> > > +/* GCU_CONFIGURATION registers */
> > > +#define GCU_CONFIGURATION_ID0 0x100
> > > +#define GCU_CONFIGURATION_ID1 0x104
> > > +
> > > +/* GCU configuration */
> > > +#define GCU_MAX_LINE_SIZE(x) ((x) & 0xFFFF)
> > > +#define GCU_MAX_NUM_LINES(x) ((x) >> 16)
> > > +#define GCU_NUM_RICH_LAYERS(x) ((x) & 0x7)
> > > +#define GCU_NUM_PIPELINES(x) (((x) >> 3) & 0x7)
> > > +#define GCU_NUM_SCALERS(x) (((x) >> 6) & 0x7)
> > > +#define GCU_DISPLAY_SPLIT_EN(x) (((x) >> 16) & 0x1)
> > > +#define GCU_DISPLAY_TBU_EN(x) (((x) >> 17) & 0x1)
> > > +
> > > /* GCU opmode */
> > > #define INACTIVE_MODE 0
> > > #define TBU_CONNECT_MODE 1
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > index b7a1097c45c4..ad38bbc7431e 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > @@ -125,6 +125,7 @@ static int komeda_platform_remove(struct platform_device *pdev)
> > >
> > > static const struct of_device_id komeda_of_match[] = {
> > > { .compatible = "arm,mali-d71", .data = d71_identify, },
> > > + { .compatible = "arm,mali-d32", .data = d71_identify, },
> > > {},
> > > };
> > >
> > >
> >
> >
>


--
Mihail

2019-12-05 08:54:07

by James Qian Wang

Subject: Re: [PATCH v2 2/2] drm/komeda: Enable new product D32 support

On Tue, Dec 03, 2019 at 09:59:57AM +0000, Mihail Atanassov wrote:
> On Tuesday, 3 December 2019 06:46:06 GMT james qian wang (Arm Technology China) wrote:
> > On Mon, Dec 02, 2019 at 11:07:52AM +0000, Mihail Atanassov wrote:
> > > On Thursday, 21 November 2019 08:17:45 GMT james qian wang (Arm Technology China) wrote:
> > > > D32 is simple version of D71, the difference is:
> > > > - Only has one pipeline
> > > > - Drop the periph block and merge it to GCU
> > > >
> > > > v2: Rebase.
> > > >
> > > > Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> > > > ---
> > > > .../drm/arm/display/include/malidp_product.h | 3 +-
> > > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 43 ++++++++++++-------
> > > > .../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++++
> > > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 1 +
> > > > 5 files changed, 44 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > > index 96e2e4016250..dbd3d4765065 100644
> > > > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > > @@ -18,7 +18,8 @@
> > > > #define MALIDP_CORE_ID_STATUS(__core_id) (((__u32)(__core_id)) & 0xFF)
> > > >
> > > > /* Mali-display product IDs */
> > > > -#define MALIDP_D71_PRODUCT_ID 0x0071
> > > > +#define MALIDP_D71_PRODUCT_ID 0x0071
> > > > +#define MALIDP_D32_PRODUCT_ID 0x0032
> > > >
> > > > union komeda_config_id {
> > > > struct {
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > index 6dadf4413ef3..c7f7e9c545c7 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > @@ -1274,7 +1274,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> > > >
> > > > ctrlr = to_ctrlr(c);
> > > >
> > > > - ctrlr->supports_dual_link = true;
> > > > + ctrlr->supports_dual_link = d71->supports_dual_link;
> > > >
> > > > return 0;
> > > > }
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > index 9b3bf353b6cc..2d429e310e5b 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > @@ -371,23 +371,33 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > > goto err_cleanup;
> > > > }
> > > >
> > > > - /* probe PERIPH */
> > > > + /* Only the legacy HW has the periph block, the newer merges the periph
> > > > + * into GCU
> > > > + */
> > > > 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));
> > > > - err = -EINVAL;
> > > > - goto err_cleanup;
> > > > + if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH)
> > > > + d71->periph_addr = NULL;
> > > > +
> > > > + if (d71->periph_addr) {
> > > > + /* probe PERIPHERAL in legacy HW */
> > > > + 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);
> > > > + d71->integrates_tbu = !!(value & PERIPH_TBU_EN);
> > > > + } else {
> > > > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID0);
> > > > + d71->max_line_size = GCU_MAX_LINE_SIZE(value);
> > > > + d71->max_vsize = GCU_MAX_NUM_LINES(value);
> > > > +
> > > > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID1);
> > > > + d71->num_rich_layers = GCU_NUM_RICH_LAYERS(value);
> > > > + d71->supports_dual_link = GCU_DISPLAY_SPLIT_EN(value);
> > > > + d71->integrates_tbu = GCU_DISPLAY_TBU_EN(value);
> > > > }
> > > >
> > > > - 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),
> > > > &d71_pipeline_funcs);
> > > > @@ -415,7 +425,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > > }
> > > >
> > > > /* loop the register blks and probe */
> > > > - i = 2; /* exclude GCU and PERIPH */
> > > > + i = 1; /* exclude GCU */
> > > > offset = D71_BLOCK_SIZE; /* skip GCU */
> > > > while (i < d71->num_blocks) {
> > > > blk_base = mdev->reg_base + (offset >> 2);
> > > > @@ -425,9 +435,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > > err = d71_probe_block(d71, &blk, blk_base);
> > > > if (err)
> > > > goto err_cleanup;
> > > > - i++;
> > > > }
> > > >
> > > > + i++;
> > >
> > > This change doesn't make much sense if you want to count how many
> > > blocks are available, since you're now counting the reserved ones, too.
> >
> > That's because for D32 num_blocks includes the reserved blocks.
> >
> > > On the counting note, the prior code rides on the assumption the periph
> > > block is last in the map, and I don't see how you still handle not
> > > counting it in the D71 case.
> >
> > Since D71 has one reserved block, and now we count reserved block.
> >
> > So now no matter D32 or D71:
> > num_blocks = n_valid_block + n_reserved_block + GCU.
>
> So at least we need that comment dropped in with the code. Future HW
> might break your assumption here once more and it will then be useful
> to know why this works for both products.

OK, will add a comments like.

/* Per HW design: num_blocks = n_valid_block + n_reserved_block + GCU */

And Per HW, all arch-v1.x include (d71/d32/d77) follows this rule,
the old logic which skip the reserved-block actually not right.

> I'd personally abstract that a bit behind a small helper func and
> handle different products separately. It's a bit of duplication but
> much easier to comprehend. Saved cycles reading code == Good Thing(tm).

Our komeda driver has two layers, common layer and chip. current we
only have one chip folder d71 for support arch-v1.x prodcut.
And our future products (arch-v2.x) will have its own chip folder, and
its own chip specific code.

Thanks
James
>
> >
> > Thanks
> > James
> > >
> > > > offset += D71_BLOCK_SIZE;
> > > > }
> > > >
> > > > @@ -603,6 +613,7 @@ d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
> > > >
> > > > switch (product_id) {
> > > > case MALIDP_D71_PRODUCT_ID:
> > > > + case MALIDP_D32_PRODUCT_ID:
> > > > funcs = &d71_chip_funcs;
> > > > break;
> > > > default:
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > > index 1727dc993909..81de6a23e7f3 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > > @@ -72,6 +72,19 @@
> > > > #define GCU_CONTROL_MODE(x) ((x) & 0x7)
> > > > #define GCU_CONTROL_SRST BIT(16)
> > > >
> > > > +/* GCU_CONFIGURATION registers */
> > > > +#define GCU_CONFIGURATION_ID0 0x100
> > > > +#define GCU_CONFIGURATION_ID1 0x104
> > > > +
> > > > +/* GCU configuration */
> > > > +#define GCU_MAX_LINE_SIZE(x) ((x) & 0xFFFF)
> > > > +#define GCU_MAX_NUM_LINES(x) ((x) >> 16)
> > > > +#define GCU_NUM_RICH_LAYERS(x) ((x) & 0x7)
> > > > +#define GCU_NUM_PIPELINES(x) (((x) >> 3) & 0x7)
> > > > +#define GCU_NUM_SCALERS(x) (((x) >> 6) & 0x7)
> > > > +#define GCU_DISPLAY_SPLIT_EN(x) (((x) >> 16) & 0x1)
> > > > +#define GCU_DISPLAY_TBU_EN(x) (((x) >> 17) & 0x1)
> > > > +
> > > > /* GCU opmode */
> > > > #define INACTIVE_MODE 0
> > > > #define TBU_CONNECT_MODE 1
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > > index b7a1097c45c4..ad38bbc7431e 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > > @@ -125,6 +125,7 @@ static int komeda_platform_remove(struct platform_device *pdev)
> > > >
> > > > static const struct of_device_id komeda_of_match[] = {
> > > > { .compatible = "arm,mali-d71", .data = d71_identify, },
> > > > + { .compatible = "arm,mali-d32", .data = d71_identify, },
> > > > {},
> > > > };
> > > >
> > > >
> > >
> > >
> >
>
>
> --
> Mihail
>
>

2019-12-05 12:05:26

by Mihail Atanassov

Subject: Re: [PATCH v2 2/2] drm/komeda: Enable new product D32 support

On Thursday, 5 December 2019 08:53:02 GMT james qian wang (Arm Technology China) wrote:
> On Tue, Dec 03, 2019 at 09:59:57AM +0000, Mihail Atanassov wrote:
> > On Tuesday, 3 December 2019 06:46:06 GMT james qian wang (Arm Technology China) wrote:
> > > On Mon, Dec 02, 2019 at 11:07:52AM +0000, Mihail Atanassov wrote:
> > > > On Thursday, 21 November 2019 08:17:45 GMT james qian wang (Arm Technology China) wrote:
> > > > > D32 is simple version of D71, the difference is:
> > > > > - Only has one pipeline
> > > > > - Drop the periph block and merge it to GCU
> > > > >
> > > > > v2: Rebase.
> > > > >
> > > > > Signed-off-by: James Qian Wang (Arm Technology China) <[email protected]>
> > > > > ---
> > > > > .../drm/arm/display/include/malidp_product.h | 3 +-
> > > > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 43 ++++++++++++-------
> > > > > .../gpu/drm/arm/display/komeda/d71/d71_regs.h | 13 ++++++
> > > > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 1 +
> > > > > 5 files changed, 44 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/arm/display/include/malidp_product.h b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > > > index 96e2e4016250..dbd3d4765065 100644
> > > > > --- a/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > > > +++ b/drivers/gpu/drm/arm/display/include/malidp_product.h
> > > > > @@ -18,7 +18,8 @@
> > > > > #define MALIDP_CORE_ID_STATUS(__core_id) (((__u32)(__core_id)) & 0xFF)
> > > > >
> > > > > /* Mali-display product IDs */
> > > > > -#define MALIDP_D71_PRODUCT_ID 0x0071
> > > > > +#define MALIDP_D71_PRODUCT_ID 0x0071
> > > > > +#define MALIDP_D32_PRODUCT_ID 0x0032
> > > > >
> > > > > union komeda_config_id {
> > > > > struct {
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > > index 6dadf4413ef3..c7f7e9c545c7 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > > @@ -1274,7 +1274,7 @@ static int d71_timing_ctrlr_init(struct d71_dev *d71,
> > > > >
> > > > > ctrlr = to_ctrlr(c);
> > > > >
> > > > > - ctrlr->supports_dual_link = true;
> > > > > + ctrlr->supports_dual_link = d71->supports_dual_link;
> > > > >
> > > > > return 0;
> > > > > }
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > > index 9b3bf353b6cc..2d429e310e5b 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > > @@ -371,23 +371,33 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > > > goto err_cleanup;
> > > > > }
> > > > >
> > > > > - /* probe PERIPH */
> > > > > + /* Only the legacy HW has the periph block, the newer merges the periph
> > > > > + * into GCU
> > > > > + */
> > > > > 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));
> > > > > - err = -EINVAL;
> > > > > - goto err_cleanup;
> > > > > + if (BLOCK_INFO_BLK_TYPE(value) != D71_BLK_TYPE_PERIPH)
> > > > > + d71->periph_addr = NULL;
> > > > > +
> > > > > + if (d71->periph_addr) {
> > > > > + /* probe PERIPHERAL in legacy HW */
> > > > > + 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);
> > > > > + d71->integrates_tbu = !!(value & PERIPH_TBU_EN);
> > > > > + } else {
> > > > > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID0);
> > > > > + d71->max_line_size = GCU_MAX_LINE_SIZE(value);
> > > > > + d71->max_vsize = GCU_MAX_NUM_LINES(value);
> > > > > +
> > > > > + value = malidp_read32(d71->gcu_addr, GCU_CONFIGURATION_ID1);
> > > > > + d71->num_rich_layers = GCU_NUM_RICH_LAYERS(value);
> > > > > + d71->supports_dual_link = GCU_DISPLAY_SPLIT_EN(value);
> > > > > + d71->integrates_tbu = GCU_DISPLAY_TBU_EN(value);
> > > > > }
> > > > >
> > > > > - 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),
> > > > > &d71_pipeline_funcs);
> > > > > @@ -415,7 +425,7 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > > > }
> > > > >
> > > > > /* loop the register blks and probe */
> > > > > - i = 2; /* exclude GCU and PERIPH */
> > > > > + i = 1; /* exclude GCU */
> > > > > offset = D71_BLOCK_SIZE; /* skip GCU */
> > > > > while (i < d71->num_blocks) {
> > > > > blk_base = mdev->reg_base + (offset >> 2);
> > > > > @@ -425,9 +435,9 @@ static int d71_enum_resources(struct komeda_dev *mdev)
> > > > > err = d71_probe_block(d71, &blk, blk_base);
> > > > > if (err)
> > > > > goto err_cleanup;
> > > > > - i++;
> > > > > }
> > > > >
> > > > > + i++;
> > > >
> > > > This change doesn't make much sense if you want to count how many
> > > > blocks are available, since you're now counting the reserved ones, too.
> > >
> > > That's because for D32 num_blocks includes the reserved blocks.
> > >
> > > > On the counting note, the prior code rides on the assumption the periph
> > > > block is last in the map, and I don't see how you still handle not
> > > > counting it in the D71 case.
> > >
> > > Since D71 has one reserved block, and now we count reserved block.
> > >
> > > So now no matter D32 or D71:
> > > num_blocks = n_valid_block + n_reserved_block + GCU.
> >
> > So at least we need that comment dropped in with the code. Future HW
> > might break your assumption here once more and it will then be useful
> > to know why this works for both products.
>
> OK, will add a comments like.
>
> /* Per HW design: num_blocks = n_valid_block + n_reserved_block + GCU */
>
> And Per HW, all arch-v1.x include (d71/d32/d77) follows this rule,
> the old logic which skip the reserved-block actually not right.

Well, given this ^...

>
> > I'd personally abstract that a bit behind a small helper func and
> > handle different products separately. It's a bit of duplication but
> > much easier to comprehend. Saved cycles reading code == Good Thing(tm).

...then my comment here ^ no longer applies. :)

I'd be a bit happier if that fix is split out into its own patch. Mind
doing that?

>
> Our komeda driver has two layers, common layer and chip. current we
> only have one chip folder d71 for support arch-v1.x prodcut.
> And our future products (arch-v2.x) will have its own chip folder, and
> its own chip specific code.

The point is moot now, but I meant adding a few small static functions in
the same file. Nothing to do with layering :).

>
> Thanks
> James
> >
> > >
> > > Thanks
> > > James
> > > >
> > > > > offset += D71_BLOCK_SIZE;
> > > > > }
> > > > >
> > > > > @@ -603,6 +613,7 @@ d71_identify(u32 __iomem *reg_base, struct komeda_chip_info *chip)
> > > > >
> > > > > switch (product_id) {
> > > > > case MALIDP_D71_PRODUCT_ID:
> > > > > + case MALIDP_D32_PRODUCT_ID:
> > > > > funcs = &d71_chip_funcs;
> > > > > break;
> > > > > default:
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > > > index 1727dc993909..81de6a23e7f3 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h
> > > > > @@ -72,6 +72,19 @@
> > > > > #define GCU_CONTROL_MODE(x) ((x) & 0x7)
> > > > > #define GCU_CONTROL_SRST BIT(16)
> > > > >
> > > > > +/* GCU_CONFIGURATION registers */
> > > > > +#define GCU_CONFIGURATION_ID0 0x100
> > > > > +#define GCU_CONFIGURATION_ID1 0x104
> > > > > +
> > > > > +/* GCU configuration */
> > > > > +#define GCU_MAX_LINE_SIZE(x) ((x) & 0xFFFF)
> > > > > +#define GCU_MAX_NUM_LINES(x) ((x) >> 16)
> > > > > +#define GCU_NUM_RICH_LAYERS(x) ((x) & 0x7)
> > > > > +#define GCU_NUM_PIPELINES(x) (((x) >> 3) & 0x7)
> > > > > +#define GCU_NUM_SCALERS(x) (((x) >> 6) & 0x7)
> > > > > +#define GCU_DISPLAY_SPLIT_EN(x) (((x) >> 16) & 0x1)
> > > > > +#define GCU_DISPLAY_TBU_EN(x) (((x) >> 17) & 0x1)
> > > > > +
> > > > > /* GCU opmode */
> > > > > #define INACTIVE_MODE 0
> > > > > #define TBU_CONNECT_MODE 1
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > > > index b7a1097c45c4..ad38bbc7431e 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> > > > > @@ -125,6 +125,7 @@ static int komeda_platform_remove(struct platform_device *pdev)
> > > > >
> > > > > static const struct of_device_id komeda_of_match[] = {
> > > > > { .compatible = "arm,mali-d71", .data = d71_identify, },
> > > > > + { .compatible = "arm,mali-d32", .data = d71_identify, },
> > > > > {},
> > > > > };
> > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
>


--
Mihail