linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)
       [not found] ` <f182b10f318db7cb09216c0176a5b26656d9ef49.1613541226.git.schowdhu@codeaurora.org>
@ 2021-02-17  8:08   ` kernel test robot
  2021-02-18  6:59   ` Vinod Koul
  1 sibling, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-02-17  8:08 UTC (permalink / raw)
  To: Souradeep Chowdhury, Andy Gross, Bjorn Andersson, Rob Herring
  Cc: devicetree, Sai Prakash Ranjan, Rajendra Nayak, linux-arm-msm,
	linux-kernel, Sibi Sankar, kbuild-all, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 5717 bytes --]

Hi Souradeep,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.11 next-20210216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Souradeep-Chowdhury/Add-driver-support-for-Data-Capture-and-Compare-Engine-DCC-for-SM8150/20210217-145428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/98f7664a4e41764c0d2111d6b17905d974aad65d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Souradeep-Chowdhury/Add-driver-support-for-Data-Capture-and-Compare-Engine-DCC-for-SM8150/20210217-145428
        git checkout 98f7664a4e41764c0d2111d6b17905d974aad65d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/soc/qcom/dcc.c: In function '_dcc_ll_cfg_default':
>> drivers/soc/qcom/dcc.c:360:4: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
     360 |    if (ret)
         |    ^~
   drivers/soc/qcom/dcc.c:362:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
     362 |     cfg->sram_offset += 4;
         |     ^~~
   At top level:
   drivers/soc/qcom/dcc.c:787:12: warning: 'dcc_add_write' defined but not used [-Wunused-function]
     787 | static int dcc_add_write(struct dcc_drvdata *drvdata, unsigned int addr,
         |            ^~~~~~~~~~~~~
   drivers/soc/qcom/dcc.c:750:12: warning: 'dcc_rd_mod_wr_add' defined but not used [-Wunused-function]
     750 | static int dcc_rd_mod_wr_add(struct dcc_drvdata *drvdata, unsigned int mask,
         |            ^~~~~~~~~~~~~~~~~
   drivers/soc/qcom/dcc.c:733:12: warning: 'dcc_add_loop' defined but not used [-Wunused-function]
     733 | static int dcc_add_loop(struct dcc_drvdata *drvdata, unsigned long loop_cnt)
         |            ^~~~~~~~~~~~
   drivers/soc/qcom/dcc.c:631:12: warning: 'dcc_config_add' defined but not used [-Wunused-function]
     631 | static int dcc_config_add(struct dcc_drvdata *drvdata, unsigned int addr,
         |            ^~~~~~~~~~~~~~
   drivers/soc/qcom/dcc.c:574:12: warning: 'dcc_enable' defined but not used [-Wunused-function]
     574 | static int dcc_enable(struct dcc_drvdata *drvdata)
         |            ^~~~~~~~~~


vim +/if +360 drivers/soc/qcom/dcc.c

   338	
   339	static int _dcc_ll_cfg_default(struct dcc_drvdata *drvdata,
   340	struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg, u32 *pos, u32 *total_len)
   341	{
   342		int ret = 0;
   343		u32 off;
   344	
   345		cfg->addr = (entry->base >> 4) & BM(0, 27);
   346	
   347		if (entry->apb_bus)
   348			cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_READ_IND | DCC_APB_IND;
   349		else
   350			cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_READ_IND | DCC_AHB_IND;
   351	
   352		off = entry->offset/4;
   353	
   354		*total_len += entry->len * 4;
   355	
   356		if (!cfg->prev_addr || cfg->prev_addr != cfg->addr || cfg->prev_off > off) {
   357			/* Check if we need to write prev link entry */
   358			if (cfg->link) {
   359				ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
 > 360				if (ret)
   361					return ret;
   362					cfg->sram_offset += 4;
   363			}
   364			dev_dbg(drvdata->dev, "DCC: sram address 0x%x\n", cfg->sram_offset);
   365	
   366			/* Write address */
   367			ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
   368	
   369			if (ret)
   370				return ret;
   371	
   372			cfg->sram_offset += 4;
   373	
   374			/* Reset link and prev_off */
   375			cfg->link = 0;
   376			cfg->prev_off = 0;
   377		}
   378	
   379		if ((off - cfg->prev_off) > 0xFF || entry->len > MAX_DCC_LEN) {
   380			dev_err(drvdata->dev, "DCC: Programming error Base: 0x%x, offset 0x%x\n",
   381			entry->base, entry->offset);
   382			ret = -EINVAL;
   383			return ret;
   384		}
   385	
   386		if (cfg->link) {
   387			/*
   388			 * link already has one offset-length so new
   389			 * offset-length needs to be placed at
   390			 * bits [29:15]
   391			 */
   392			*pos = 15;
   393	
   394			/* Clear bits [31:16] */
   395			cfg->link &= BM(0, 14);
   396		} else {
   397			/*
   398			 * link is empty, so new offset-length needs
   399			 * to be placed at bits [15:0]
   400			 */
   401			*pos = 0;
   402			cfg->link = 1 << 15;
   403		}
   404	
   405		/* write new offset-length pair to correct position */
   406		cfg->link |= (((off-cfg->prev_off) & BM(0, 7)) | ((entry->len << 8) & BM(8, 14))) << *pos;
   407	
   408		cfg->link |= DCC_LINK_DESCRIPTOR;
   409	
   410		if (*pos) {
   411			ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
   412			if (ret)
   413				return ret;
   414			cfg->sram_offset += 4;
   415			cfg->link = 0;
   416		}
   417	
   418		cfg->prev_off  = off + entry->len - 1;
   419		cfg->prev_addr = cfg->addr;
   420		return ret;
   421	}
   422	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67240 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V0 1/6] dt-bindings: Added the yaml bindings for DCC
       [not found] ` <5da43657817066e0ffe1e24cfb17104138515452.1613541226.git.schowdhu@codeaurora.org>
@ 2021-02-17 11:02   ` Vinod Koul
  2021-02-17 20:24   ` Rob Herring
  1 sibling, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2021-02-17 11:02 UTC (permalink / raw)
  To: Souradeep Chowdhury
  Cc: devicetree, Sai Prakash Ranjan, Rajendra Nayak, linux-arm-msm,
	linux-kernel, Rob Herring, Bjorn Andersson, Andy Gross,
	Sibi Sankar, linux-arm-kernel

On 17-02-21, 12:18, Souradeep Chowdhury wrote:
> Documentation for Data Capture and Compare(DCC) device tree bindings
> in yaml format.
> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> ---
>  .../devicetree/bindings/arm/msm/qcom,dcc.yaml      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> new file mode 100644
> index 0000000..8f09578
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/msm/qcom,dcc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Data Capture and Compare
> +
> +maintainers:
> +  - Souradeep Chowdhury <schowdhu@codeaurora.org>
> +
> +description: |
> +    DCC (Data Capture and Compare) is a DMA engine which is used to save
> +    configuration data or system memory contents during catastrophic failure
> +    or SW trigger.DCC is used to capture and store data for debugging purpose

space after .

> +
> +
> +properties:
> +  compatible:
> +    items:
> +    - enum:
> +      - qcom,sm8150-dcc
> +    - const: qcom,dcc
> +
> +  reg:
> +    items:
> +      - description: DCC base register region
> +      - description: DCC RAM base register region
> +
> +  reg-names:
> +    items:
> +      - const: dcc-base
> +      - const: dcc-ram-base

drop dcc from names

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    dcc@010a2000{
> +                compatible = "qcom,sm8150-dcc";

should this not be:
                compatible = "qcom,sm8150-dcc", "qcom,dcc";

> +                reg = <0 0x010a2000 0  0x1000>,
> +                      <0 0x010ae000 0  0x2000>;
> +                reg-names = "dcc-base", "dcc-ram-base";
> +    };
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V0 2/6] arm64: dts: qcom: sm8150: Add Data Capture and Compare(DCC) support node
       [not found] ` <893022aecd4ba354adb57bd463206dd93fc19886.1613541226.git.schowdhu@codeaurora.org>
@ 2021-02-17 11:03   ` Vinod Koul
  0 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2021-02-17 11:03 UTC (permalink / raw)
  To: Souradeep Chowdhury
  Cc: devicetree, Sai Prakash Ranjan, Rajendra Nayak, linux-arm-msm,
	linux-kernel, Rob Herring, Bjorn Andersson, Andy Gross,
	Sibi Sankar, linux-arm-kernel

On 17-02-21, 12:18, Souradeep Chowdhury wrote:
> Add the DCC(Data Capture and Compare) device tree node entry along with
> the addresses for register regions.

This should be last patch..

> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8150.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> index e5bb17b..3198bd3 100644
> --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
> @@ -654,6 +654,13 @@
>  			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>  		};
>  
> +		dcc@010a2000{

no leading zero here and space before {

> +			compatible = "qcom,sm8150-dcc", "qcom,dcc";
> +			reg = <0x0 0x010a2000 0x0 0x1000>,
> +				<0x0 0x010ad000 0x0 0x3000>;

pls align this to preceding line

> +			reg-names = "dcc-base", "dcc-ram-base";
> +		};
> +
>  		ufs_mem_hc: ufshc@1d84000 {
>  			compatible = "qcom,sm8150-ufshc", "qcom,ufshc",
>  				     "jedec,ufs-2.0";
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V0 1/6] dt-bindings: Added the yaml bindings for DCC
       [not found] ` <5da43657817066e0ffe1e24cfb17104138515452.1613541226.git.schowdhu@codeaurora.org>
  2021-02-17 11:02   ` [PATCH V0 1/6] dt-bindings: Added the yaml bindings for DCC Vinod Koul
@ 2021-02-17 20:24   ` Rob Herring
  1 sibling, 0 replies; 5+ messages in thread
From: Rob Herring @ 2021-02-17 20:24 UTC (permalink / raw)
  To: Souradeep Chowdhury
  Cc: devicetree, Sai Prakash Ranjan, Rajendra Nayak, linux-kernel,
	Andy Gross, Bjorn Andersson, Rob Herring, Sibi Sankar,
	linux-arm-msm, linux-arm-kernel

On Wed, 17 Feb 2021 12:18:22 +0530, Souradeep Chowdhury wrote:
> Documentation for Data Capture and Compare(DCC) device tree bindings
> in yaml format.
> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> ---
>  .../devicetree/bindings/arm/msm/qcom,dcc.yaml      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml:21:5: [warning] wrong indentation: expected 6 but found 4 (indentation)
./Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml:22:7: [warning] wrong indentation: expected 8 but found 6 (indentation)

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/arm/msm/qcom,dcc.example.dts:19.21-24.11: Warning (unit_address_format): /example-0/dcc@010a2000: unit name should not have leading 0s
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/msm/qcom,dcc.example.dt.yaml: example-0: dcc@010a2000:reg:0: [0, 17440768, 0, 4096] is too long
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/msm/qcom,dcc.example.dt.yaml: example-0: dcc@010a2000:reg:1: [0, 17489920, 0, 8192] is too long
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/msm/qcom,dcc.example.dt.yaml: dcc@010a2000: compatible: ['qcom,sm8150-dcc'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/arm/msm/qcom,dcc.yaml

See https://patchwork.ozlabs.org/patch/1441182

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)
       [not found] ` <f182b10f318db7cb09216c0176a5b26656d9ef49.1613541226.git.schowdhu@codeaurora.org>
  2021-02-17  8:08   ` [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC) kernel test robot
@ 2021-02-18  6:59   ` Vinod Koul
  1 sibling, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2021-02-18  6:59 UTC (permalink / raw)
  To: Souradeep Chowdhury
  Cc: devicetree, Sai Prakash Ranjan, Rajendra Nayak, linux-arm-msm,
	linux-kernel, Rob Herring, Bjorn Andersson, Andy Gross,
	Sibi Sankar, linux-arm-kernel

On 17-02-21, 12:18, Souradeep Chowdhury wrote:
> The DCC is a DMA Engine designed to capture and store data
> during system crash or software triggers.The DCC operates
                                        ^^^
Space after . (quite a few here, pls fix them)

> based on link list entries which provides it with data and
> addresses and the function it needs to perform.These functions
> are read,write and loop.Added the basic driver in this patch
> which contains a probe method which instantiates all the link
> list data specific to a SoC.Methods have also been added to
> handle all the functionalities specific to a linked list.Each
> DCC has it's own SRAM which needs to be instantiated at probe
> time as well.

So help me understand, in case of system crash how will this be used..?

> 
> Signed-off-by: Souradeep Chowdhury <schowdhu@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig  |    8 +
>  drivers/soc/qcom/Makefile |    1 +
>  drivers/soc/qcom/dcc.c    | 1055 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1064 insertions(+)
>  create mode 100644 drivers/soc/qcom/dcc.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 79b568f..8819e0b 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -69,6 +69,14 @@ config QCOM_LLCC
>  	  SDM845. This provides interfaces to clients that use the LLCC.
>  	  Say yes here to enable LLCC slice driver.
>  
> +config QCOM_DCC
> +	tristate "Qualcomm Technologies, Inc. Data Capture and Compare engine driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  This option enables driver for Data Capture and Compare engine. DCC
> +	  driver provides interface to configure DCC block and read back
> +	  captured data from DCC's internal SRAM.
> +
>  config QCOM_KRYO_L2_ACCESSORS
>  	bool
>  	depends on ARCH_QCOM && ARM64 || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index ad675a6..1b00870 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -26,3 +26,4 @@ obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>  obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
>  obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=	kryo-l2-accessors.o
> +obj-$(CONFIG_QCOM_DCC) += dcc.o
> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
> new file mode 100644
> index 0000000..d67452b
> --- /dev/null
> +++ b/drivers/soc/qcom/dcc.c
> @@ -0,0 +1,1055 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/cdev.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#define TIMEOUT_US		100
> +
> +#define BM(lsb, msb)		((BIT(msb) - BIT(lsb)) + BIT(msb))
> +#define BMVAL(val, lsb, msb)	((val & BM(lsb, msb)) >> lsb)
> +#define BVAL(val, n)		((val & BIT(n)) >> n)

Pls use macros available in bitfield.h rather than inventing your own..

> +
> +#define dcc_writel(drvdata, val, off)					\
> +	writel((val), drvdata->base + dcc_offset_conv(drvdata, off))
> +#define dcc_readl(drvdata, off)						\
> +	readl(drvdata->base + dcc_offset_conv(drvdata, off))
> +
> +#define dcc_sram_readl(drvdata, off)					\
> +	readl(drvdata->ram_base + off)
> +
> +/* DCC registers */
> +#define DCC_HW_INFO					0x04
> +#define DCC_LL_NUM_INFO					0x10
> +#define DCC_STATUS					0x1C
> +#define DCC_LL_LOCK(m)					(0x34 + 0x80 * m)
> +#define DCC_LL_CFG(m)					(0x38 + 0x80 * m)
> +#define DCC_LL_BASE(m)					(0x3c + 0x80 * m)
> +#define DCC_FD_BASE(m)					(0x40 + 0x80 * m)
> +#define DCC_LL_TIMEOUT(m)				(0x44 + 0x80 * m)
> +#define DCC_LL_INT_ENABLE(m)				(0x4C + 0x80 * m)
> +#define DCC_LL_INT_STATUS(m)				(0x50 + 0x80 * m)
> +#define DCC_LL_SW_TRIGGER(m)				(0x60 + 0x80 * m)
> +#define DCC_LL_BUS_ACCESS_STATUS(m)			(0x64 + 0x80 * m)
> +
> +#define DCC_MAP_LEVEL1			0x18
> +#define DCC_MAP_LEVEL2			0x34
> +#define DCC_MAP_LEVEL3			0x4C
> +
> +#define DCC_MAP_OFFSET1			0x10
> +#define DCC_MAP_OFFSET2			0x18
> +#define DCC_MAP_OFFSET3			0x1C
> +#define DCC_MAP_OFFSET4			0x8
> +
> +#define DCC_FIX_LOOP_OFFSET		16
> +#define DCC_VER_INFO_BIT		9
> +
> +#define DCC_READ        0
> +#define DCC_WRITE       1
> +#define DCC_LOOP        2
> +#define DCC_READ_WRITE  3
> +
> +#define MAX_DCC_OFFSET				(0xFF * 4)
> +#define MAX_DCC_LEN				0x7F
> +#define MAX_LOOP_CNT				0xFF
> +
> +#define DCC_ADDR_DESCRIPTOR			0x00
> +#define DCC_LOOP_DESCRIPTOR			(BIT(30))
> +#define DCC_RD_MOD_WR_DESCRIPTOR		(BIT(31))
> +#define DCC_LINK_DESCRIPTOR			(BIT(31) | BIT(30))

we have GENMASK() for this

> +
> +#define DCC_READ_IND				0x00
> +#define DCC_WRITE_IND				(BIT(28))
> +
> +#define DCC_AHB_IND				0x00
> +#define DCC_APB_IND				BIT(29)
> +
> +#define DCC_MAX_LINK_LIST			8
> +#define DCC_INVALID_LINK_LIST			0xFF
> +
> +#define DCC_VER_MASK1				0x7F
> +#define DCC_VER_MASK2				0x3F

Genmask for these too...

> +
> +#define DCC_RD_MOD_WR_ADDR                      0xC105E
> +
> +struct qcom_dcc_config {
> +	const int dcc_ram_offset;
> +};
> +
> +static const struct qcom_dcc_config sm8150_cfg = {
> +	.dcc_ram_offset				= 0x5000,
> +};

maybe move it down near compatible table?

> +
> +enum dcc_descriptor_type {
> +	DCC_ADDR_TYPE,
> +	DCC_LOOP_TYPE,
> +	DCC_READ_WRITE_TYPE,
> +	DCC_WRITE_TYPE
> +};
> +
> +enum dcc_mem_map_ver {
> +	DCC_MEM_MAP_VER1,
> +	DCC_MEM_MAP_VER2,
> +	DCC_MEM_MAP_VER3
> +};
> +
> +struct dcc_config_entry {
> +	u32				base;
> +	u32				offset;
> +	u32				len;
> +	u32				index;
> +	u32				loop_cnt;
> +	u32				write_val;
> +	u32				mask;
> +	bool				apb_bus;
> +	enum dcc_descriptor_type	desc_type;
> +	struct list_head		list;
> +};
> +
> +struct dcc_drvdata {
> +	void __iomem		*base;
> +	u32			reg_size;
> +	struct device		*dev;
> +	struct mutex		mutex;
> +	void __iomem		*ram_base;
> +	u32			ram_size;
> +	u32			ram_offset;
> +	enum dcc_mem_map_ver	mem_map_ver;
> +	u32			ram_cfg;
> +	u32			ram_start;
> +	bool			*enable;
> +	bool			*configured;
> +	bool			interrupt_disable;
> +	char			*sram_node;
> +	struct cdev		sram_dev;
> +	struct class		*sram_class;
> +	struct list_head	*cfg_head;
> +	u32			*nr_config;
> +	u32			nr_link_list;
> +	u8			curr_list;
> +	u8			loopoff;
> +};
> +
> +struct dcc_cfg_attr {
> +	u32	addr;
> +	u32	prev_addr;
> +	u32	prev_off;
> +	u32	link;
> +	u32	sram_offset;
> +};
> +
> +struct dcc_cfg_loop_attr {
> +	u32	loop;
> +	bool	loop_start;
> +	u32	loop_cnt;
> +	u32	loop_len;
> +	u32	loop_off;
> +};
> +
> +static u32 dcc_offset_conv(struct dcc_drvdata *drvdata, u32 off)
> +{
> +	if (drvdata->mem_map_ver == DCC_MEM_MAP_VER1) {
> +		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL3)
> +			return (off - DCC_MAP_OFFSET3);
> +		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL2)
> +			return (off - DCC_MAP_OFFSET2);
> +		else if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL1)
> +			return (off - DCC_MAP_OFFSET1);
> +	} else if (drvdata->mem_map_ver == DCC_MEM_MAP_VER2) {
> +		if ((off & DCC_VER_MASK1) >= DCC_MAP_LEVEL2)
> +			return (off - DCC_MAP_OFFSET4);
> +	}
> +	return off;
> +}
> +
> +static int dcc_sram_writel(struct dcc_drvdata *drvdata,
> +					u32 val, u32 off)
> +{
> +	if (unlikely(off > (drvdata->ram_size - 4)))
> +		return -EINVAL;
> +
> +	writel((val), drvdata->ram_base + off);
> +
> +	return 0;
> +}
> +
> +static int _dcc_ll_cfg_read_write(struct dcc_drvdata *drvdata,
> +struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg)

this looks a bit hard to read, can you make it better (also you can go
upto 100 chars now), do you checkpatch with --strict option to get
better alignment of code


> +{
> +	int ret = 0;

Superfluous init?

> +
> +	if (cfg->link) {
> +		/* write new offset = 1 to continue
> +		 * processing the list

kernel uses:
        /*
         * this is a 
         * multi line comment style
         */

> +		 */
> +
> +		ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +		if (ret)
> +			return ret;
> +		cfg->sram_offset += 4;
> +		/* Reset link and prev_off */
> +		cfg->addr = 0x00;
> +		cfg->link = 0;
> +		cfg->prev_off = 0;

memset cfg first?

> +		cfg->prev_addr = cfg->addr;
> +	}
> +
> +	cfg->addr = DCC_RD_MOD_WR_DESCRIPTOR;
> +
> +	ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
> +

drop this empty line

> +	if (ret)
> +		return ret;
> +
> +	cfg->sram_offset += 4;
> +
> +	ret = dcc_sram_writel(drvdata, entry->mask, cfg->sram_offset);
> +
> +	if (ret)
> +		return ret;
> +
> +	cfg->sram_offset += 4;
> +
> +	ret = dcc_sram_writel(drvdata, entry->write_val, cfg->sram_offset);
> +
> +	if (ret)
> +		return ret;
> +
> +	cfg->sram_offset += 4;
> +
> +	cfg->addr = 0;
> +
> +	return ret;
> +}
> +
> +static int _dcc_ll_cfg_loop(struct dcc_drvdata *drvdata, struct dcc_config_entry *entry,
> +struct dcc_cfg_attr *cfg, struct dcc_cfg_loop_attr *cfg_loop, u32 *total_len)

here as well

> +{
> +
> +	int ret = 0;
> +
> +	/* Check if we need to write link of prev entry */
> +	if (cfg->link) {
> +		ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +		if (ret)
> +			return ret;
> +		cfg->sram_offset += 4;
> +	}
> +
> +	if (cfg_loop->loop_start) {
> +		cfg_loop->loop = (cfg->sram_offset - cfg_loop->loop_off) / 4;
> +		cfg_loop->loop |= (cfg_loop->loop_cnt << drvdata->loopoff) &
> +		BM(drvdata->loopoff, 27);
> +		cfg_loop->loop |= DCC_LOOP_DESCRIPTOR;
> +		*total_len += (*total_len - cfg_loop->loop_len) * cfg_loop->loop_cnt;
> +
> +		ret = dcc_sram_writel(drvdata, cfg_loop->loop, cfg->sram_offset);
> +
> +		if (ret)
> +			return ret;
> +		cfg->sram_offset += 4;
> +
> +		cfg_loop->loop_start = false;
> +		cfg_loop->loop_len = 0;
> +		cfg_loop->loop_off = 0;

seems quite similar to last one..? Maybe a helper for common code

> +	} else {
> +		cfg_loop->loop_start = true;
> +		cfg_loop->loop_cnt = entry->loop_cnt - 1;
> +		cfg_loop->loop_len = *total_len;
> +		cfg_loop->loop_off = cfg->sram_offset;
> +	}
> +
> +	/* Reset link and prev_off */
> +
> +	cfg->addr = 0x00;
> +	cfg->link = 0;
> +	cfg->prev_off = 0;
> +	cfg->prev_addr = cfg->addr;
> +
> +	return ret;
> +}
> +
> +static int _dcc_ll_cfg_write(struct dcc_drvdata *drvdata,
> +struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg, u32 *total_len)
> +{
> +	u32 off;
> +	int ret = 0;
> +
> +	if (cfg->link) {
> +		/* write new offset = 1 to continue
> +		 * processing the list
> +		 */
> +		ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +
> +		if (ret)
> +			return ret;
> +
> +		cfg->sram_offset += 4;
> +		/* Reset link and prev_off */
> +		cfg->addr = 0x00;
> +		cfg->prev_off = 0;
> +		cfg->prev_addr = cfg->addr;
> +	}
> +
> +	off = entry->offset/4;
> +	/* write new offset-length pair to correct position */
> +	cfg->link |= ((off & BM(0, 7)) | BIT(15) | ((entry->len << 8) & BM(8, 14)));
> +	cfg->link |= DCC_LINK_DESCRIPTOR;
> +
> +	/* Address type */
> +	cfg->addr = (entry->base >> 4) & BM(0, 27);
> +	if (entry->apb_bus)
> +		cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_WRITE_IND | DCC_APB_IND;
> +	else
> +		cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_WRITE_IND | DCC_AHB_IND;
> +	ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
> +
> +	if (ret)
> +		return ret;
> +	cfg->sram_offset += 4;
> +
> +	ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +	if (ret)
> +		return ret;
> +	cfg->sram_offset += 4;
> +
> +	ret = dcc_sram_writel(drvdata, entry->write_val, cfg->sram_offset);
> +
> +	if (ret)
> +		return ret;
> +
> +	cfg->sram_offset += 4;
> +	cfg->addr = 0x00;
> +	cfg->link = 0;
> +	return ret;
> +}
> +
> +static int _dcc_ll_cfg_default(struct dcc_drvdata *drvdata,
> +struct dcc_config_entry *entry, struct dcc_cfg_attr *cfg, u32 *pos, u32 *total_len)
> +{
> +	int ret = 0;
> +	u32 off;
> +
> +	cfg->addr = (entry->base >> 4) & BM(0, 27);
> +
> +	if (entry->apb_bus)
> +		cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_READ_IND | DCC_APB_IND;
> +	else
> +		cfg->addr |= DCC_ADDR_DESCRIPTOR | DCC_READ_IND | DCC_AHB_IND;
> +
> +	off = entry->offset/4;
> +
> +	*total_len += entry->len * 4;
> +
> +	if (!cfg->prev_addr || cfg->prev_addr != cfg->addr || cfg->prev_off > off) {
> +		/* Check if we need to write prev link entry */
> +		if (cfg->link) {
> +			ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +			if (ret)
> +				return ret;
> +				cfg->sram_offset += 4;
> +		}
> +		dev_dbg(drvdata->dev, "DCC: sram address 0x%x\n", cfg->sram_offset);
> +
> +		/* Write address */
> +		ret = dcc_sram_writel(drvdata, cfg->addr, cfg->sram_offset);
> +
> +		if (ret)
> +			return ret;
> +
> +		cfg->sram_offset += 4;
> +
> +		/* Reset link and prev_off */
> +		cfg->link = 0;
> +		cfg->prev_off = 0;
> +	}
> +
> +	if ((off - cfg->prev_off) > 0xFF || entry->len > MAX_DCC_LEN) {
> +		dev_err(drvdata->dev, "DCC: Programming error Base: 0x%x, offset 0x%x\n",
> +		entry->base, entry->offset);
> +		ret = -EINVAL;
> +		return ret;
> +	}
> +
> +	if (cfg->link) {
> +		/*
> +		 * link already has one offset-length so new
> +		 * offset-length needs to be placed at
> +		 * bits [29:15]
> +		 */
> +		*pos = 15;
> +
> +		/* Clear bits [31:16] */
> +		cfg->link &= BM(0, 14);
> +	} else {
> +		/*
> +		 * link is empty, so new offset-length needs
> +		 * to be placed at bits [15:0]
> +		 */
> +		*pos = 0;
> +		cfg->link = 1 << 15;
> +	}
> +
> +	/* write new offset-length pair to correct position */
> +	cfg->link |= (((off-cfg->prev_off) & BM(0, 7)) | ((entry->len << 8) & BM(8, 14))) << *pos;
> +
> +	cfg->link |= DCC_LINK_DESCRIPTOR;
> +
> +	if (*pos) {
> +		ret = dcc_sram_writel(drvdata, cfg->link, cfg->sram_offset);
> +		if (ret)
> +			return ret;
> +		cfg->sram_offset += 4;
> +		cfg->link = 0;
> +	}
> +
> +	cfg->prev_off  = off + entry->len - 1;
> +	cfg->prev_addr = cfg->addr;
> +	return ret;
> +}
> +
> +static int __dcc_ll_cfg(struct dcc_drvdata *drvdata, int curr_list)
> +{
> +	int ret = 0;
> +	u32 total_len, pos;
> +	struct dcc_config_entry *entry;
> +	struct dcc_cfg_attr cfg;
> +	struct dcc_cfg_loop_attr cfg_loop;
> +
> +	cfg.sram_offset = drvdata->ram_cfg * 4;
> +	cfg.prev_off = 0;
> +	cfg_loop.loop_off = 0;
> +	total_len = 0;
> +	cfg_loop.loop_len = 0;
> +	cfg_loop.loop_cnt = 0;
> +	cfg_loop.loop_start = false;
> +	cfg.prev_addr = 0;
> +	cfg.addr = 0;
> +	cfg.link = 0;

again use memset for these

> +
> +	list_for_each_entry(entry, &drvdata->cfg_head[curr_list], list) {
> +		switch (entry->desc_type) {
> +		case DCC_READ_WRITE_TYPE:
> +		{

checkpatch should have told you this is not typical kernel style, pls
fix this and many other things before we process further

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-02-18  7:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1613541226.git.schowdhu@codeaurora.org>
     [not found] ` <5da43657817066e0ffe1e24cfb17104138515452.1613541226.git.schowdhu@codeaurora.org>
2021-02-17 11:02   ` [PATCH V0 1/6] dt-bindings: Added the yaml bindings for DCC Vinod Koul
2021-02-17 20:24   ` Rob Herring
     [not found] ` <893022aecd4ba354adb57bd463206dd93fc19886.1613541226.git.schowdhu@codeaurora.org>
2021-02-17 11:03   ` [PATCH V0 2/6] arm64: dts: qcom: sm8150: Add Data Capture and Compare(DCC) support node Vinod Koul
     [not found] ` <f182b10f318db7cb09216c0176a5b26656d9ef49.1613541226.git.schowdhu@codeaurora.org>
2021-02-17  8:08   ` [PATCH V0 3/6] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC) kernel test robot
2021-02-18  6:59   ` Vinod Koul

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).