From: Krzysztof Kozlowski <krzk@kernel.org>
To: Souradeep Chowdhury <quic_schowdhu@quicinc.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Rob Herring <robh+dt@kernel.org>, Alex Elder <elder@ieee.org>
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
devicetree@vger.kernel.org,
Sai Prakash Ranjan <quic_saipraka@quicinc.com>,
Sibi Sankar <quic_sibis@quicinc.com>,
Rajendra Nayak <quic_rjendra@quicinc.com>,
vkoul@kernel.org
Subject: Re: [PATCH V13 2/7] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)
Date: Fri, 23 Sep 2022 21:42:47 +0200 [thread overview]
Message-ID: <fa33589b-7fe1-7be2-8d80-4c81183126c9@kernel.org> (raw)
In-Reply-To: <44ca04316e8b67f1662d304d8535236d82710bda.1663642052.git.quic_schowdhu@quicinc.com>
On 20/09/2022 05:56, Souradeep Chowdhury wrote:
> The DCC is a DMA Engine designed to capture and store data
> during system crash or software triggers. The DCC operates
> based on user inputs via the debugfs interface. The user gives
> addresses as inputs and these addresses are stored in the
(...)
> +
> +#define DCC_RD_MOD_WR_ADDR 0xC105E
> +
> +/*DCC debugfs directory*/
> +static struct dentry *dcc_dbg;
> +
> +enum dcc_descriptor_type {
> + DCC_READ_TYPE,
> + DCC_LOOP_TYPE,
> + DCC_READ_WRITE_TYPE,
> + DCC_WRITE_TYPE
> +};
> +
> +struct dcc_config_entry {
> + u32 base;
> + u32 offset;
> + u32 len;
> + u32 loop_cnt;
> + u32 write_val;
> + u32 mask;
> + bool apb_bus;
> + enum dcc_descriptor_type desc_type;
> + struct list_head list;
> +};
> +
> +/**
> + * struct dcc_drvdata - configuration information related to a dcc device
> + * @base: Base Address of the dcc device
> + * @dev: The device attached to the driver data
> + * @mutex: Lock to protect access and manipulation of dcc_drvdata
> + * @ram_base: Base address for the SRAM dedicated for the dcc device
> + * @ram_size: Total size of the SRAM dedicated for the dcc device
> + * @ram_offset: Offset to the SRAM dedicated for dcc device
> + * @mem_map_ver: Memory map version of DCC hardware
> + * @ram_cfg: Used for address limit calculation for dcc
> + * @ram_start: Starting address of DCC SRAM
> + * @sram_dev: Micellaneous device equivalent of dcc SRAM
> + * @cfg_head: Points to the head of the linked list of addresses
> + * @dbg_dir: The dcc debugfs directory under which all the debugfs files are placed
> + * @nr_link_list: Total number of linkedlists supported by the DCC configuration
> + * @loopoff: Loop offset bits range for the addresses
All these entres have messed up spacing.
> + * @enable: This contains an array of linkedlist enable flags
No, this is not an array of linked lists... It's a pointer to bool. This
is not way to store linked lists.
> +
> +static int dcc_probe(struct platform_device *pdev)
> +{
> + u32 val;
> + int ret = 0, i, size;
> + struct device *dev = &pdev->dev;
> + struct dcc_drvdata *dcc;
> + struct resource *res;
> +
> + dcc = devm_kzalloc(dev, sizeof(*dcc), GFP_KERNEL);
> + if (!dcc)
> + return -ENOMEM;
> +
> + dcc->dev = &pdev->dev;
> + platform_set_drvdata(pdev, dcc);
> +
> + dcc->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(dcc->base))
> + return PTR_ERR(dcc->base);
> +
> + dcc->ram_base = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
> + if (IS_ERR(dcc->ram_base))
> + return PTR_ERR(dcc->ram_base);
> +
> + dcc->ram_size = resource_size(res);
> +
> + dcc->ram_offset = (size_t)of_device_get_match_data(&pdev->dev);
> +
> + val = dcc_readl(dcc, DCC_HW_INFO);
> +
> + if (FIELD_GET(DCC_VER_INFO_MASK, val)) {
> + dcc->mem_map_ver = 3;
> + dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
> + if (dcc->nr_link_list == 0)
> + return -EINVAL;
> + } else if ((val & DCC_VER_MASK2) == DCC_VER_MASK2) {
> + dcc->mem_map_ver = 2;
> + dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
> + if (dcc->nr_link_list == 0)
> + return -EINVAL;
> + } else {
> + dcc->mem_map_ver = 1;
> + dcc->nr_link_list = DCC_MAX_LINK_LIST;
> + }
> +
> + /* Either set the fixed loop offset or calculate it
Start with /*
(see coding style)
> + * from ram_size.Max consecutive addresses the
> + * dcc can loop is equivalent to the ram size
> + */
> + if (val & DCC_LOOP_OFFSET_MASK)
> + dcc->loopoff = DCC_FIX_LOOP_OFFSET;
> + else
> + dcc->loopoff = get_bitmask_order((dcc->ram_size +
> + dcc->ram_offset) / 4 - 1);
> +
> + mutex_init(&dcc->mutex);
> + /* Allocate space for all entries at once */
> + size = sizeof(*dcc->enable) + sizeof(*dcc->cfg_head);
This is quite confusing way of handling lists - some parts of drvdata
are list, some are not.
> +
> + dcc->enable = devm_kcalloc(dev, dcc->nr_link_list, size, GFP_KERNEL);
> + if (!dcc->enable)
> + return -ENOMEM;
> +
> + dcc->cfg_head = (struct list_head *)(dcc->enable + dcc->nr_link_list);
That's unusual way to iterate over list...
> +
> + for (i = 0; i < dcc->nr_link_list; i++)
> + INIT_LIST_HEAD(&dcc->cfg_head[i]);
> +
> + ret = dcc_sram_dev_init(dcc);
> + if (ret) {
> + dev_err(dcc->dev, "DCC: sram node not registered.\n");
> + return ret;
> + }
> +
> + ret = dcc_create_debug_dir(dcc);
> + if (ret) {
> + dev_err(dcc->dev, "DCC: debugfs files not created.\n");
debugfs failures are not reasons to fail probe. Also no need to print
errors.
> + dcc_sram_dev_exit(dcc);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int dcc_remove(struct platform_device *pdev)
> +{
> + struct dcc_drvdata *drvdata = platform_get_drvdata(pdev);
> +
> + dcc_delete_debug_dir(drvdata);
> +
> + dcc_sram_dev_exit(drvdata);
> +
No need for blank lines between each calls.
> + dcc_config_reset(drvdata);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id dcc_match_table[] = {
> + { .compatible = "qcom,sm8150-dcc", .data = (void *)0x5000 },
> + { .compatible = "qcom,sc7280-dcc", .data = (void *)0x12000 },
> + { .compatible = "qcom,sc7180-dcc", .data = (void *)0x6000 },
> + { .compatible = "qcom,sdm845-dcc", .data = (void *)0x6000 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, dcc_match_table);
> +
> +static struct platform_driver dcc_driver = {
> + .probe = dcc_probe,
> + .remove = dcc_remove,
> + .driver = {
> + .name = "qcom-dcc",
> + .of_match_table = dcc_match_table,
> + },
> +};
> +
> +module_platform_driver(dcc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Qualcomm Technologies Inc. DCC driver");
> +
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-09-23 19:44 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-20 3:56 [PATCH V13 0/7] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC) Souradeep Chowdhury
2022-09-20 3:56 ` [PATCH V13 1/7] dt-bindings: Added the yaml bindings for DCC Souradeep Chowdhury
2022-09-23 19:27 ` Krzysztof Kozlowski
2022-09-24 11:15 ` Souradeep Chowdhury
2022-09-27 19:51 ` Alex Elder
2022-09-28 17:14 ` Souradeep Chowdhury
2022-09-20 3:56 ` [PATCH V13 2/7] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC) Souradeep Chowdhury
2022-09-23 19:42 ` Krzysztof Kozlowski [this message]
2022-09-24 11:44 ` Souradeep Chowdhury
2022-09-23 19:50 ` Krzysztof Kozlowski
2022-09-24 11:46 ` Souradeep Chowdhury
2022-09-20 3:57 ` [PATCH V13 3/7] MAINTAINERS: Add the entry for DCC(Data Capture and Compare) driver support Souradeep Chowdhury
2022-09-23 20:03 ` Krzysztof Kozlowski
2022-09-24 11:47 ` Souradeep Chowdhury
2022-09-20 3:57 ` [PATCH V13 4/7] arm64: dts: qcom: sm8150: Add Data Capture and Compare(DCC) support node Souradeep Chowdhury
2022-09-20 3:57 ` [PATCH V13 5/7] arm64: dts: qcom: sc7280: " Souradeep Chowdhury
2022-09-20 3:57 ` [PATCH V13 6/7] arm64: dts: qcom: sc7180: " Souradeep Chowdhury
2022-09-20 3:57 ` [PATCH V13 7/7] arm64: dts: qcom: sdm845: " Souradeep Chowdhury
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa33589b-7fe1-7be2-8d80-4c81183126c9@kernel.org \
--to=krzk@kernel.org \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=elder@ieee.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=quic_rjendra@quicinc.com \
--cc=quic_saipraka@quicinc.com \
--cc=quic_schowdhu@quicinc.com \
--cc=quic_sibis@quicinc.com \
--cc=robh+dt@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).