| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5936016.qkgZygMIky@e123338-lin>
Date: Mon, 2 Dec 2019 11:07:54 +0000
From: Mihail Atanassov <Mihail.Atanassov@....com>
To: "james qian wang (Arm Technology China)" <james.qian.wang@....com>
CC: nd <nd@....com>, Liviu Dudau <Liviu.Dudau@....com>,
"airlied@...ux.ie" <airlied@...ux.ie>,
Brian Starkey <Brian.Starkey@....com>,
"maarten.lankhorst@...ux.intel.com"
<maarten.lankhorst@...ux.intel.com>,
"Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
"Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
"Thomas Sun (Arm Technology China)" <thomas.Sun@....com>,
"Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
"Tiannan Zhu (Arm Technology China)" <Tiannan.Zhu@....com>,
nd <nd@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
Ben Davis <Ben.Davis@....com>,
"Oscar Zhang (Arm Technology China)" <Oscar.Zhang@....com>,
"Channing Chen (Arm Technology China)" <Channing.Chen@....com>
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) <james.qian.wang@....com>
> ---
> .../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 <mihail.atanassov@....com>
--
Mihail
Powered by blists - more mailing lists