[PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

L2 registers are accessed using a select register and data
register pair. To prevent multiple concurrent writes to the
select register by independent drivers, the write to the
select register and the associated access of the data register
are protected with a lock. All drivers accessing the L2
registers use the set and get functions provided by
l2-accessors to ensure correct reads and writes to L2 registers.

Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
---
 drivers/soc/qcom/Kconfig              |  9 +++++
 drivers/soc/qcom/Makefile             |  1 +
 drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
 4 files changed, 103 insertions(+)
 create mode 100644 drivers/soc/qcom/l2-accessors.c
 create mode 100644 include/linux/soc/qcom/l2-accessors.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 461b387..c8498cd 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -10,6 +10,15 @@ config QCOM_GSBI
           functions for connecting the underlying serial UART, SPI, and I2C
           devices to the output pins.
 
+config QCOM_L2_ACCESSORS
+	bool "Qualcomm Technologies L2-cache accessors"
+	depends on ARCH_QCOM
+	help
+	  Say y here to enable support for the Qualcomm Technologies
+	  L2 accessors.
+	  Provides support for accessing registers in the L2 cache
+	  for Qualcomm Technologies chips.
+
 config QCOM_PM
 	bool "Qualcomm Power Management"
 	depends on ARCH_QCOM && !ARM64
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index fdd664e..6ef29b9 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
+obj-$(CONFIG_QCOM_L2_ACCESSORS) += l2-accessors.o
 obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_SMD) +=	smd.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
diff --git a/drivers/soc/qcom/l2-accessors.c b/drivers/soc/qcom/l2-accessors.c
new file mode 100644
index 0000000..fbb69bd
--- /dev/null
+++ b/drivers/soc/qcom/l2-accessors.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2014-2016 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/spinlock.h>
+#include <linux/module.h>
+#include <linux/soc/qcom/l2-accessors.h>
+#include <asm/cputype.h>
+#include <asm/sysreg.h>
+
+#define	L2CPUSRSELR_EL1	S3_3_c15_c0_6
+#define	L2CPUSRDR_EL1	S3_3_c15_c0_7
+
+static DEFINE_RAW_SPINLOCK(l2_access_lock);
+
+/**
+ * set_l2_indirect_reg: write value to an L2 register
+ * @reg: Address of L2 register.
+ * @value: Value to be written to register.
+ *
+ * Use architecturally required barriers for ordering between system register
+ * accesses
+ */
+void set_l2_indirect_reg(u64 reg, u64 val)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg(reg, L2CPUSRSELR_EL1);
+	isb();
+	write_sysreg(val, L2CPUSRDR_EL1);
+	isb();
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
+}
+EXPORT_SYMBOL(set_l2_indirect_reg);
+
+/**
+ * get_l2_indirect_reg: read an L2 register value
+ * @reg: Address of L2 register.
+ *
+ * Use architecturally required barriers for ordering between system register
+ * accesses
+ */
+u64 get_l2_indirect_reg(u64 reg)
+{
+	u64 val;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg(reg, L2CPUSRSELR_EL1);
+	isb();
+	val = read_sysreg(L2CPUSRDR_EL1);
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
+
+	return val;
+}
+EXPORT_SYMBOL(get_l2_indirect_reg);
diff --git a/include/linux/soc/qcom/l2-accessors.h b/include/linux/soc/qcom/l2-accessors.h
new file mode 100644
index 0000000..563c114
--- /dev/null
+++ b/include/linux/soc/qcom/l2-accessors.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __QCOM_L2_ACCESSORS_H
+#define __QCOM_L2_ACCESSORS_H
+
+#ifdef CONFIG_QCOM_L2_ACCESSORS
+void set_l2_indirect_reg(u64 reg_addr, u64 val);
+u64 get_l2_indirect_reg(u64 reg_addr);
+#else
+static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {}
+static inline u64 get_l2_indirect_reg(u64 reg_addr)
+{
+	return 0;
+}
+#endif
+#endif
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Stephen Boyd]

On 05/20/2016 12:13 PM, Neil Leeder wrote:
> diff --git a/drivers/soc/qcom/l2-accessors.c b/drivers/soc/qcom/l2-accessors.c
> new file mode 100644
> index 0000000..fbb69bd
> --- /dev/null
> +++ b/drivers/soc/qcom/l2-accessors.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (c) 2014-2016 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/spinlock.h>
> +#include <linux/module.h>

export.h instead?

> +#include <linux/soc/qcom/l2-accessors.h>
> +#include <asm/cputype.h>
> +#include <asm/sysreg.h>

Is there a patch to add sysreg.h to arch/arm? It would be nice to use
one l2 accessor API on arm64 and arm.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

On 5/20/2016 05:19 PM, Stephen Boyd wrote:
> On 05/20/2016 12:13 PM, Neil Leeder wrote:
>> diff --git a/drivers/soc/qcom/l2-accessors.c b/drivers/soc/qcom/l2-accessors.c
>> new file mode 100644
>> index 0000000..fbb69bd
>> --- /dev/null
>> +++ b/drivers/soc/qcom/l2-accessors.c
>> @@ -0,0 +1,66 @@
>> +/*
>> + * Copyright (c) 2014-2016 The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/spinlock.h>
>> +#include <linux/module.h>
> 
> export.h instead?

OK, will address in v2

> 
>> +#include <linux/soc/qcom/l2-accessors.h>
>> +#include <asm/cputype.h>
>> +#include <asm/sysreg.h>
> 
> Is there a patch to add sysreg.h to arch/arm? It would be nice to use
> one l2 accessor API on arm64 and arm.
> 

Sounds like a good thing for the next person who submits a krait L2 patch to consider.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Stephen Boyd]

On 05/23/2016 08:43 AM, Neil Leeder wrote:
>
> On 5/20/2016 05:19 PM, Stephen Boyd wrote:
>
>>
>> Is there a patch to add sysreg.h to arch/arm? It would be nice to use
>> one l2 accessor API on arm64 and arm.
>>
> Sounds like a good thing for the next person who submits a krait L2 patch to consider.
>

Heh, ok. If it isn't supported in this patch then we need to make the
config depend on ARM64 so that this doesn't fail to compile on ARM.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Mark Rutland]

On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:
> L2 registers are accessed using a select register and data
> register pair. To prevent multiple concurrent writes to the
> select register by independent drivers, the write to the
> select register and the associated access of the data register
> are protected with a lock. All drivers accessing the L2
> registers use the set and get functions provided by
> l2-accessors to ensure correct reads and writes to L2 registers.

What will this be used for? (i.e. which drivers want to touch the L2
registers?).

Generally we expect FW to configure the caches and interconnect
appropriately.

> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig              |  9 +++++
>  drivers/soc/qcom/Makefile             |  1 +
>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
>  4 files changed, 103 insertions(+)
>  create mode 100644 drivers/soc/qcom/l2-accessors.c
>  create mode 100644 include/linux/soc/qcom/l2-accessors.h

These are awfully generic file names (and function names). Which SoCs
does this apply to?

It would be good to give these more specific names.

> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 461b387..c8498cd 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -10,6 +10,15 @@ config QCOM_GSBI
>            functions for connecting the underlying serial UART, SPI, and I2C
>            devices to the output pins.
>  
> +config QCOM_L2_ACCESSORS
> +	bool "Qualcomm Technologies L2-cache accessors"
> +	depends on ARCH_QCOM
> +	help
> +	  Say y here to enable support for the Qualcomm Technologies
> +	  L2 accessors.
> +	  Provides support for accessing registers in the L2 cache
> +	  for Qualcomm Technologies chips.
> +
>  config QCOM_PM
>  	bool "Qualcomm Power Management"
>  	depends on ARCH_QCOM && !ARM64
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index fdd664e..6ef29b9 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
> +obj-$(CONFIG_QCOM_L2_ACCESSORS) += l2-accessors.o
>  obj-$(CONFIG_QCOM_PM)	+=	spm.o
>  obj-$(CONFIG_QCOM_SMD) +=	smd.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
> diff --git a/drivers/soc/qcom/l2-accessors.c b/drivers/soc/qcom/l2-accessors.c
> new file mode 100644
> index 0000000..fbb69bd
> --- /dev/null
> +++ b/drivers/soc/qcom/l2-accessors.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (c) 2014-2016 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/spinlock.h>
> +#include <linux/module.h>
> +#include <linux/soc/qcom/l2-accessors.h>
> +#include <asm/cputype.h>
> +#include <asm/sysreg.h>
> +
> +#define	L2CPUSRSELR_EL1	S3_3_c15_c0_6
> +#define	L2CPUSRDR_EL1	S3_3_c15_c0_7
> +
> +static DEFINE_RAW_SPINLOCK(l2_access_lock);
> +
> +/**
> + * set_l2_indirect_reg: write value to an L2 register
> + * @reg: Address of L2 register.
> + * @value: Value to be written to register.
> + *
> + * Use architecturally required barriers for ordering between system register
> + * accesses
> + */
> +void set_l2_indirect_reg(u64 reg, u64 val)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&l2_access_lock, flags);
> +	write_sysreg(reg, L2CPUSRSELR_EL1);
> +	isb();
> +	write_sysreg(val, L2CPUSRDR_EL1);
> +	isb();
> +	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
> +}
> +EXPORT_SYMBOL(set_l2_indirect_reg);
> +
> +/**
> + * get_l2_indirect_reg: read an L2 register value
> + * @reg: Address of L2 register.
> + *
> + * Use architecturally required barriers for ordering between system register
> + * accesses
> + */
> +u64 get_l2_indirect_reg(u64 reg)
> +{
> +	u64 val;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&l2_access_lock, flags);
> +	write_sysreg(reg, L2CPUSRSELR_EL1);
> +	isb();
> +	val = read_sysreg(L2CPUSRDR_EL1);
> +	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(get_l2_indirect_reg);
> diff --git a/include/linux/soc/qcom/l2-accessors.h b/include/linux/soc/qcom/l2-accessors.h
> new file mode 100644
> index 0000000..563c114
> --- /dev/null
> +++ b/include/linux/soc/qcom/l2-accessors.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +

> +#ifndef __QCOM_L2_ACCESSORS_H
> +#define __QCOM_L2_ACCESSORS_H
> +
> +#ifdef CONFIG_QCOM_L2_ACCESSORS
> +void set_l2_indirect_reg(u64 reg_addr, u64 val);
> +u64 get_l2_indirect_reg(u64 reg_addr);
> +#else
> +static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {}
> +static inline u64 get_l2_indirect_reg(u64 reg_addr)
> +{
> +	return 0;
> +}

Surely it would be better to error out on any unintentional use of these
at build time?

Thanks,
Mark.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

On 5/23/2016 01:04 PM, Stephen Boyd wrote:
> On 05/23/2016 08:43 AM, Neil Leeder wrote:
>>
>> On 5/20/2016 05:19 PM, Stephen Boyd wrote:
>>
>>>
>>> Is there a patch to add sysreg.h to arch/arm? It would be nice to use
>>> one l2 accessor API on arm64 and arm.
>>>
>> Sounds like a good thing for the next person who submits a krait L2 patch to consider.
>>
> 
> Heh, ok. If it isn't supported in this patch then we need to make the
> config depend on ARM64 so that this doesn't fail to compile on ARM.
> 

Ok, I'll add that in v2 too.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

On 5/23/2016 01:25 PM, Mark Rutland wrote:
> On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:
>> L2 registers are accessed using a select register and data
>> register pair. To prevent multiple concurrent writes to the
>> select register by independent drivers, the write to the
>> select register and the associated access of the data register
>> are protected with a lock. All drivers accessing the L2
>> registers use the set and get functions provided by
>> l2-accessors to ensure correct reads and writes to L2 registers.
> 
> What will this be used for? (i.e. which drivers want to touch the L2
> registers?).
> 
> Generally we expect FW to configure the caches and interconnect
> appropriately.

The primary use is in the L2 PMU driver, which will be posted shortly.

> 
>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
>> ---
>>  drivers/soc/qcom/Kconfig              |  9 +++++
>>  drivers/soc/qcom/Makefile             |  1 +
>>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
>>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
>>  4 files changed, 103 insertions(+)
>>  create mode 100644 drivers/soc/qcom/l2-accessors.c
>>  create mode 100644 include/linux/soc/qcom/l2-accessors.h
> 
> These are awfully generic file names (and function names). Which SoCs
> does this apply to?
> 
> It would be good to give these more specific names.

It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs.
Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd
prefer not to add redundancy by adding another qcom in there.

>> diff --git a/include/linux/soc/qcom/l2-accessors.h b/include/linux/soc/qcom/l2-accessors.h
>> new file mode 100644
>> index 0000000..563c114
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/l2-accessors.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
> 
>> +#ifndef __QCOM_L2_ACCESSORS_H
>> +#define __QCOM_L2_ACCESSORS_H
>> +
>> +#ifdef CONFIG_QCOM_L2_ACCESSORS
>> +void set_l2_indirect_reg(u64 reg_addr, u64 val);
>> +u64 get_l2_indirect_reg(u64 reg_addr);
>> +#else
>> +static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {}
>> +static inline u64 get_l2_indirect_reg(u64 reg_addr)
>> +{
>> +	return 0;
>> +}
> 
> Surely it would be better to error out on any unintentional use of these
> at build time?

This allows building code which is common to ARM SoCs and QCOM SoCs without having to ifdef out the
QCOM-specific pieces.

> 
> Thanks,
> Mark.
> 

Thanks,
Neil

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Christopher Covington]

On 05/23/2016 02:22 PM, Neil Leeder wrote:
> 
> On 5/23/2016 01:25 PM, Mark Rutland wrote:
>> On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:
>>> L2 registers are accessed using a select register and data
>>> register pair. To prevent multiple concurrent writes to the
>>> select register by independent drivers, the write to the
>>> select register and the associated access of the data register
>>> are protected with a lock. All drivers accessing the L2
>>> registers use the set and get functions provided by
>>> l2-accessors to ensure correct reads and writes to L2 registers.
>>
>> What will this be used for? (i.e. which drivers want to touch the L2
>> registers?).
>>
>> Generally we expect FW to configure the caches and interconnect
>> appropriately.
> 
> The primary use is in the L2 PMU driver, which will be posted shortly.
> 
>>
>>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
>>> ---
>>>  drivers/soc/qcom/Kconfig              |  9 +++++
>>>  drivers/soc/qcom/Makefile             |  1 +
>>>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
>>>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
>>>  4 files changed, 103 insertions(+)
>>>  create mode 100644 drivers/soc/qcom/l2-accessors.c
>>>  create mode 100644 include/linux/soc/qcom/l2-accessors.h
>>
>> These are awfully generic file names (and function names). Which SoCs
>> does this apply to?
>>
>> It would be good to give these more specific names.
> 
> It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs.
> Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd
> prefer not to add redundancy by adding another qcom in there.

They ended up calling the pinctrl file qcom/pinctrl/qdf2xxx.c.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8f1338cd80648adf5434798f5393ad7c55d10848

Regards,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Timur Tabi]

Christopher Covington wrote:
> They ended up calling the pinctrl file qcom/pinctrl/qdf2xxx.c.
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8f1338cd80648adf5434798f5393ad7c55d10848

To be fair, none of the other drivers in drivers/soc/ specify any 
specific SOCs.  They all have generic file names.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Mark Rutland]

On Mon, May 23, 2016 at 02:22:59PM -0400, Neil Leeder wrote:
> 
> On 5/23/2016 01:25 PM, Mark Rutland wrote:
> > On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:
> >> L2 registers are accessed using a select register and data
> >> register pair. To prevent multiple concurrent writes to the
> >> select register by independent drivers, the write to the
> >> select register and the associated access of the data register
> >> are protected with a lock. All drivers accessing the L2
> >> registers use the set and get functions provided by
> >> l2-accessors to ensure correct reads and writes to L2 registers.
> > 
> > What will this be used for? (i.e. which drivers want to touch the L2
> > registers?).
> > 
> > Generally we expect FW to configure the caches and interconnect
> > appropriately.
> 
> The primary use is in the L2 PMU driver, which will be posted shortly.

Ok.

> >> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> >> ---
> >>  drivers/soc/qcom/Kconfig              |  9 +++++
> >>  drivers/soc/qcom/Makefile             |  1 +
> >>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
> >>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
> >>  4 files changed, 103 insertions(+)
> >>  create mode 100644 drivers/soc/qcom/l2-accessors.c
> >>  create mode 100644 include/linux/soc/qcom/l2-accessors.h
> > 
> > These are awfully generic file names (and function names). Which SoCs
> > does this apply to?
> > 
> > It would be good to give these more specific names.
> 
> It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs.

Per Christopher's comment, it sounds like this applies to QDF24xx.

Given that the code uses IMPLEMENTATION DEFINED system registers, I
presume that this does not apply to MSM8916 which uses Cortex-A53, for
example (though perhaps it does, and I am mistaken).

> Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd
> prefer not to add redundancy by adding another qcom in there.

I'm not asking for another "qcom", but simply the SoC variant or family
(e.g. "qdf24xx" would be fine).

> >> diff --git a/include/linux/soc/qcom/l2-accessors.h b/include/linux/soc/qcom/l2-accessors.h
> >> new file mode 100644
> >> index 0000000..563c114
> >> --- /dev/null
> >> +++ b/include/linux/soc/qcom/l2-accessors.h
> >> @@ -0,0 +1,27 @@
> >> +/*
> >> + * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 and
> >> + * only version 2 as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> > 
> >> +#ifndef __QCOM_L2_ACCESSORS_H
> >> +#define __QCOM_L2_ACCESSORS_H
> >> +
> >> +#ifdef CONFIG_QCOM_L2_ACCESSORS
> >> +void set_l2_indirect_reg(u64 reg_addr, u64 val);
> >> +u64 get_l2_indirect_reg(u64 reg_addr);
> >> +#else
> >> +static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {}
> >> +static inline u64 get_l2_indirect_reg(u64 reg_addr)
> >> +{
> >> +	return 0;
> >> +}
> > 
> > Surely it would be better to error out on any unintentional use of these
> > at build time?
> 
> This allows building code which is common to ARM SoCs and QCOM SoCs without having to ifdef out the
> QCOM-specific pieces.

These shouldn't appear in generic code.

Other than the L2 PMU driver (which presumably depends on or selects
CONFIG_QCOM_L2_ACCESSORS), what code would you have to ifdef?

I don't have a major concern on this, I just don't see where it should
matter.

Thanks,
Mark.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

On 5/24/2016 07:23 AM, Mark Rutland wrote:
> On Mon, May 23, 2016 at 02:22:59PM -0400, Neil Leeder wrote:
>>
>> On 5/23/2016 01:25 PM, Mark Rutland wrote:
>>> On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:

>>>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
>>>> ---
>>>>  drivers/soc/qcom/Kconfig              |  9 +++++
>>>>  drivers/soc/qcom/Makefile             |  1 +
>>>>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
>>>>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
>>>>  4 files changed, 103 insertions(+)
>>>>  create mode 100644 drivers/soc/qcom/l2-accessors.c
>>>>  create mode 100644 include/linux/soc/qcom/l2-accessors.h
>>>
>>> These are awfully generic file names (and function names). Which SoCs
>>> does this apply to?
>>>
>>> It would be good to give these more specific names.
>>
>> It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs.
> 
> Per Christopher's comment, it sounds like this applies to QDF24xx.
> 
> Given that the code uses IMPLEMENTATION DEFINED system registers, I
> presume that this does not apply to MSM8916 which uses Cortex-A53, for
> example (though perhaps it does, and I am mistaken).
> 
>> Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd
>> prefer not to add redundancy by adding another qcom in there.
> 
> I'm not asking for another "qcom", but simply the SoC variant or family
> (e.g. "qdf24xx" would be fine).
> 

It applies to all ARMv8 SoCs with QCOM processors in them. So QDF24xx and mobile 820, but not SoCs
with ARM processors in them such as MSM8916. So neither msm_ nor qdf_ are accurate prefixes. As Timur
pointed out, the majority of source files in drivers/soc/qcom don't have any prefix, which is a
reason why I didn't include one.

>>>> diff --git a/include/linux/soc/qcom/l2-accessors.h b/include/linux/soc/qcom/l2-accessors.h
>>>> new file mode 100644
>>>> index 0000000..563c114
>>>> --- /dev/null
>>>> +++ b/include/linux/soc/qcom/l2-accessors.h
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 and
>>>> + * only version 2 as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>
>>>> +#ifndef __QCOM_L2_ACCESSORS_H
>>>> +#define __QCOM_L2_ACCESSORS_H
>>>> +
>>>> +#ifdef CONFIG_QCOM_L2_ACCESSORS
>>>> +void set_l2_indirect_reg(u64 reg_addr, u64 val);
>>>> +u64 get_l2_indirect_reg(u64 reg_addr);
>>>> +#else
>>>> +static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {}
>>>> +static inline u64 get_l2_indirect_reg(u64 reg_addr)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>
>>> Surely it would be better to error out on any unintentional use of these
>>> at build time?
>>
>> This allows building code which is common to ARM SoCs and QCOM SoCs without having to ifdef out the
>> QCOM-specific pieces.
> 
> These shouldn't appear in generic code.
> 
> Other than the L2 PMU driver (which presumably depends on or selects
> CONFIG_QCOM_L2_ACCESSORS), what code would you have to ifdef?
> 
> I don't have a major concern on this, I just don't see where it should
> matter.

Ok, I agree, I will remove this. Thanks.

> 
> Thanks,
> Mark.
> 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Bjorn Andersson]

On Tue 24 May 12:54 PDT 2016, Neil Leeder wrote:

> 
> 
> On 5/24/2016 07:23 AM, Mark Rutland wrote:
> > On Mon, May 23, 2016 at 02:22:59PM -0400, Neil Leeder wrote:
> >>
> >> On 5/23/2016 01:25 PM, Mark Rutland wrote:
> >>> On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:
> 
> >>>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> >>>> ---
> >>>>  drivers/soc/qcom/Kconfig              |  9 +++++
> >>>>  drivers/soc/qcom/Makefile             |  1 +
> >>>>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
> >>>>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
> >>>>  4 files changed, 103 insertions(+)
> >>>>  create mode 100644 drivers/soc/qcom/l2-accessors.c
> >>>>  create mode 100644 include/linux/soc/qcom/l2-accessors.h
> >>>
> >>> These are awfully generic file names (and function names). Which SoCs
> >>> does this apply to?
> >>>
> >>> It would be good to give these more specific names.
> >>
> >> It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs.
> > 
> > Per Christopher's comment, it sounds like this applies to QDF24xx.
> > 
> > Given that the code uses IMPLEMENTATION DEFINED system registers, I
> > presume that this does not apply to MSM8916 which uses Cortex-A53, for
> > example (though perhaps it does, and I am mistaken).
> > 
> >> Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd
> >> prefer not to add redundancy by adding another qcom in there.
> > 
> > I'm not asking for another "qcom", but simply the SoC variant or family
> > (e.g. "qdf24xx" would be fine).
> > 
> 
> It applies to all ARMv8 SoCs with QCOM processors in them. So QDF24xx
> and mobile 820, but not SoCs with ARM processors in them such as
> MSM8916. So neither msm_ nor qdf_ are accurate prefixes.

What's the code name for the SoC in QDF24xx? The 820 is Kryo, is it the
same core in QDF24xx or does that have some other name.

We should try to pick something adding value, not adding another generic
thing.

> As Timur pointed out, the majority of source files in drivers/soc/qcom
> don't have any prefix, which is a reason why I didn't include one.
> 

There's no reason to add a generic "qcom" to the qcom folder, if
anything we should drop the "qcom" prefix of the only one in there.

Regards,
Bjorn

Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

On 5/26/2016 12:48 AM, Bjorn Andersson wrote:
> On Tue 24 May 12:54 PDT 2016, Neil Leeder wrote:
> 
>>
>>
>> On 5/24/2016 07:23 AM, Mark Rutland wrote:
>>> On Mon, May 23, 2016 at 02:22:59PM -0400, Neil Leeder wrote:
>>>>
>>>> On 5/23/2016 01:25 PM, Mark Rutland wrote:
>>>>> On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote:
>>
>>>>>> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/soc/qcom/Kconfig              |  9 +++++
>>>>>>  drivers/soc/qcom/Makefile             |  1 +
>>>>>>  drivers/soc/qcom/l2-accessors.c       | 66 +++++++++++++++++++++++++++++++++++
>>>>>>  include/linux/soc/qcom/l2-accessors.h | 27 ++++++++++++++
>>>>>>  4 files changed, 103 insertions(+)
>>>>>>  create mode 100644 drivers/soc/qcom/l2-accessors.c
>>>>>>  create mode 100644 include/linux/soc/qcom/l2-accessors.h
>>>>>
>>>>> These are awfully generic file names (and function names). Which SoCs
>>>>> does this apply to?
>>>>>
>>>>> It would be good to give these more specific names.
>>>>
>>>> It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs.
>>>
>>> Per Christopher's comment, it sounds like this applies to QDF24xx.
>>>
>>> Given that the code uses IMPLEMENTATION DEFINED system registers, I
>>> presume that this does not apply to MSM8916 which uses Cortex-A53, for
>>> example (though perhaps it does, and I am mistaken).
>>>
>>>> Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd
>>>> prefer not to add redundancy by adding another qcom in there.
>>>
>>> I'm not asking for another "qcom", but simply the SoC variant or family
>>> (e.g. "qdf24xx" would be fine).
>>>
>>
>> It applies to all ARMv8 SoCs with QCOM processors in them. So QDF24xx
>> and mobile 820, but not SoCs with ARM processors in them such as
>> MSM8916. So neither msm_ nor qdf_ are accurate prefixes.
> 
> What's the code name for the SoC in QDF24xx? The 820 is Kryo, is it the
> same core in QDF24xx or does that have some other name.
> 
> We should try to pick something adding value, not adding another generic
> thing.

There is currently no public name for the QDF24xx core.

> 
>> As Timur pointed out, the majority of source files in drivers/soc/qcom
>> don't have any prefix, which is a reason why I didn't include one.
>>
> 
> There's no reason to add a generic "qcom" to the qcom folder, if
> anything we should drop the "qcom" prefix of the only one in there.
> 
> Regards,
> Bjorn
> 

Neil

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.