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 <nlee...@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.
