From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932618AbcEXTy4 (ORCPT ); Tue, 24 May 2016 15:54:56 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:54415 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752967AbcEXTyy (ORCPT ); Tue, 24 May 2016 15:54:54 -0400 Subject: Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers To: Mark Rutland References: <1463771587-7438-1-git-send-email-nleeder@codeaurora.org> <20160523172527.GK4976@leverpostej> <20160524112326.GC25374@leverpostej> Cc: David Brown , Andy Gross , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Mark Langsdorf , Mark Salter , Jon Masters , Timur Tabi , cov@codeaurora.org, ashwin.chaugule@linaro.org, sboyd@codeaurora.org, nleeder@codeaurora.org From: Neil Leeder Message-ID: Date: Tue, 24 May 2016 15:54:50 -0400 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160524112326.GC25374@leverpostej> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> --- >>>> 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.