From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lina Iyer Subject: Re: [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC Date: Mon, 25 Aug 2014 18:31:53 -0600 Message-ID: <20140826003153.GA72292@ilina-mac.local> References: <1408486537-6358-1-git-send-email-lina.iyer@linaro.org> <1408486537-6358-5-git-send-email-lina.iyer@linaro.org> <53FBC971.1070102@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Received: from mail-ie0-f169.google.com ([209.85.223.169]:36040 "EHLO mail-ie0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756738AbaHZAcA (ORCPT ); Mon, 25 Aug 2014 20:32:00 -0400 Received: by mail-ie0-f169.google.com with SMTP id rd18so10715640iec.14 for ; Mon, 25 Aug 2014 17:32:00 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53FBC971.1070102@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd Cc: daniel.lezcano@linaro.org, khilman@linaro.org, davidb@codeaurora.org, galak@codeaurora.org, linux-arm-msm@vger.kernel.org, lorenzo.pieralisi@arm.com, msivasub@codeaurora.org, Praveen Chidamabram , Murali Nalajala On Mon, Aug 25, 2014 at 04:40:33PM -0700, Stephen Boyd wrote: >On 08/19/14 15:15, Lina Iyer wrote: >> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt >> new file mode 100644 >> index 0000000..318e024 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt > >We already have a binding document for SAW. Please add stuff there >because SPM is just a component of SAW. > I agree that SPM is just a component of the SAW. But the document there seems to indicate regulator details, totally unrelated to the actual SAW hardware functionality. >> @@ -0,0 +1,47 @@ >> +* Subsystem Power Manager (SAW2) >> + >> +S4 generation of MSMs have SPM hardware blocks to control the Application >> +Processor Sub-System power. These SPM blocks run individual state machine >> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI >> +instruction is executed by the core. >> + >> +The devicetree representation of the SPM block should be: >> + >> +Required properties >> + >> +- compatible: Could be one of - >> + "qcom,spm-v2.1" >> + "qcom,spm-v3.0" >> +- reg: The physical address and the size of the SPM's memory mapped registers >> +- qcom,cpu: phandle for the CPU that the SPM block is attached to. >> + This field is required on only for SPMs that control the CPU. > >We have a phandle from the CPU/L2 to the SAW, so this isn't necessary. > Sorry, I dont understand. Care to explain pls? Its necessary to know what SPM instance controls which CPU or L2, so as to pick the right SPM device to configure. >> +- qcom,saw2-cfg: SAW2 configuration register > >Why? Compatible should indicate where this register is. > There are multiple versions of saw handled by the same driver and distinguished by the version register. These SAW revisions differ in the register offset organization. The variable holds the value to be configured in the saw2-cfg register. I will update the documentation to be more clear. >> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command in the SPM >> + sequence > >This is actually three values packed into one register for three >different selectable delays, right? We don't typically do register jam >tables in DT. Perhaps it should be split out into 3 different >properties. Or maybe it shouldn't be specified in DT at all and should >be determined algorithmically from the command sequences? From what I >can tell most of the sequences don't even use these delays. > Not at all sequences use the delays. These cannot be determined algorithmatically, They may be added to the sequence for changes in hardware. Let me revisit the sequences to see if they need to be set with the current sequence in use. >> +- qcom,saw2-spm-ctl: The SPM control register > >Why? Compatible should indicate where this register is. > See above. >> + >> +Optional properties >> + >> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence >> +- qcom,saw2-spm-cmd-ret: The Retention command sequence >> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence >> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This sequence may >> + turn off other SoC components. >> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch) command >> + sequence. This sequence will retain the memory but turn off the logic. > >I wonder if these should be properties of the idle states? That way the >driver isn't searching for them by name in DT, instead it knows what >state is associated with what sequence that the SPM needs to have >programmed. > I see the relation you are seeing. But its not a property of the idle state. Its an SoC specific property that the idle uses to indicate a state. Better off lying here. I doubt there would be a good support for holding SoC specific stuff in the ARM idle-states nodes. >> +- >> +Example: >> + spm@f9089000 { >> + compatible = "qcom,spm-v2.1"; >> + #address-cells = <1>; >> + #size-cells = <1>; > >Why is this in the example? Are there subnodes? > No, there arent. I guess I carried over from downstream. >> + reg = <0xf9089000 0x1000>; >> + qcom,cpu = <&CPU0>; >> + qcom,saw2-cfg = <0x1>; >> + qcom,saw2-spm-dly= <0x20000400>; >> + qcom,saw2-spm-ctl = <0x1>; >> + qcom,saw2-spm-cmd-wfi = [03 0b 0f]; >> + qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92 >> + a0 b0 03 68 70 3b 92 a0 b0 >> + 82 2b 50 10 30 02 22 30 0f]; >> + }; >> diff --git a/drivers/soc/qcom/spm-devices.c b/drivers/soc/qcom/spm-devices.c >> new file mode 100644 >> index 0000000..2175a81 >> --- /dev/null >> +++ b/drivers/soc/qcom/spm-devices.c >> @@ -0,0 +1,198 @@ >> +/* Copyright (c) 2011-2014, 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#include "spm-drv.h" >> + >> +/** >> + * All related information for an SPM device >> + * Helps manage the collective. >> + */ >> +struct msm_spm_device { >> + bool initialized; >> + struct msm_spm_driver_data drv; >> +}; >> + >> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device); >> + >> +/** >> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode >> + * @mode: SPM LPM mode to enter >> + */ >> +int msm_spm_set_low_power_mode(unsigned int mode) >> +{ >> + struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device); >> + int ret = -EINVAL; >> + >> + if (!dev->initialized) >> + return -ENXIO; >> + >> + if (mode == MSM_SPM_MODE_DISABLED) >> + ret = msm_spm_drv_set_spm_enable(&dev->drv, false); >> + else if (!msm_spm_drv_set_spm_enable(&dev->drv, true)) >> + ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(msm_spm_set_low_power_mode); >> + >> +static int get_cpu_id(struct device_node *node) >> +{ >> + struct device_node *cpu_node; >> + u32 cpu; >> + int ret = -EINVAL; >> + char *key = "qcom,cpu"; >> + >> + cpu_node = of_parse_phandle(node, key, 0); >> + if (cpu_node) { >> + for_each_possible_cpu(cpu) { >> + if (of_get_cpu_node(cpu, NULL) == cpu_node) >> + return cpu; >> + } >> + } >> + return ret; >> +} >> + >> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev) >> +{ >> + struct msm_spm_device *dev = NULL; >> + int cpu = get_cpu_id(pdev->dev.of_node); >> + >> + if ((cpu >= 0) && cpu < num_possible_cpus()) >> + dev = &per_cpu(msm_cpu_spm_device, cpu); >> + >> + return dev; >> +} >> + >> +static int msm_spm_dev_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + int i; >> + struct device_node *node = pdev->dev.of_node; >> + char *key; >> + uint32_t val; >> + struct msm_spm_mode modes[MSM_SPM_MODE_NR]; >> + struct msm_spm_device *spm_dev; >> + struct resource *res; >> + uint32_t mode_count = 0; >> + >> + struct spm_of { >> + char *key; >> + uint32_t id; >> + }; >> + >> + /* SPM Configuration registers */ >> + struct spm_of spm_of_data[] = { >> + {"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG}, >> + {"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL}, >> + {"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY}, >> + }; >> + >> + /* SPM sleep sequences */ >> + struct spm_of mode_of_data[] = { >> + {"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING}, >> + {"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE}, >> + {"qcom,saw2-spm-cmd-ret", MSM_SPM_MODE_RETENTION}, >> + }; >> + >> + /* Get the right SPM device */ >> + spm_dev = msm_spm_get_device(pdev); >> + if (IS_ERR_OR_NULL(spm_dev)) >> + return -EINVAL; >> + >> + /* Get the SAW start address */ >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + ret = -EINVAL; >> + goto fail; >> + } >> + spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start, >> + resource_size(res)); >> + if (!spm_dev->drv.reg_base_addr) { >> + ret = -ENOMEM; >> + goto fail; >> + } >> + >> + /* Read the SPM configuration register values */ >> + for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) { >> + ret = of_property_read_u32(node, spm_of_data[i].key, &val); >> + if (ret) >> + continue; >> + spm_dev->drv.reg_shadow[spm_of_data[i].id] = val; >> + } >> + >> + /* Read the byte arrays for the SPM sleep sequences */ >> + for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) { >> + modes[mode_count].start_addr = 0; >> + key = mode_of_data[i].key; >> + modes[mode_count].cmd = >> + (uint8_t *)of_get_property(node, key, &val); >> + if (!modes[mode_count].cmd) >> + continue; >> + modes[mode_count].mode = mode_of_data[i].id; >> + mode_count++; >> + } >> + >> + spm_dev->drv.modes = devm_kcalloc(&pdev->dev, mode_count, >> + sizeof(modes[0]), GFP_KERNEL); >> + if (!spm_dev->drv.modes) >> + return -ENOMEM; >> + spm_dev->drv.num_modes = mode_count; >> + memcpy(spm_dev->drv.modes, &modes[0], sizeof(modes[0]) * mode_count); >> + >> + /* Initialize the hardware */ >> + ret = msm_spm_drv_init(&spm_dev->drv); >> + if (ret) { >> + kfree(spm_dev->drv.modes); >> + return ret; >> + } >> + >> + spm_dev->initialized = true; >> + return ret; >> + >> +fail: >> + dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret); >> + return ret; >> +} >> + >> +static struct of_device_id msm_spm_match_table[] = { > >const. > Will change. > >-- >Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >hosted by The Linux Foundation >