* Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
From: Boris Brezillon @ 2018-05-24 12:41 UTC (permalink / raw)
To: Stefan Agner
Cc: dwmw2, computersforpeace, marek.vasut, robh+dt, mark.rutland,
thierry.reding, mturquette, sboyd, dev, miquel.raynal, richard,
marcel, krzk, digetx, benjamin.lindqvist, jonathanh, pdeschrijver,
pgaikwad, mirza.krak, linux-mtd, linux-tegra, devicetree,
linux-kernel, linux-clk
In-Reply-To: <20180524142356.0fc68797@bbrezillon>
On Thu, 24 May 2018 14:23:56 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> On Thu, 24 May 2018 13:09:53 +0200
> Stefan Agner <stefan@agner.ch> wrote:
>
> > On 24.05.2018 10:56, Boris Brezillon wrote:
> > > On Thu, 24 May 2018 10:46:27 +0200
> > > Stefan Agner <stefan@agner.ch> wrote:
> > >
> > >> Hi Boris,
> > >>
> > >> Thanks for the initial review! One small question below:
> > >>
> > >> On 23.05.2018 16:18, Boris Brezillon wrote:
> > >> > Hi Stefan,
> > >> >
> > >> > On Tue, 22 May 2018 14:07:06 +0200
> > >> > Stefan Agner <stefan@agner.ch> wrote:
> > >> >> +
> > >> >> +struct tegra_nand {
> > >> >> + void __iomem *regs;
> > >> >> + struct clk *clk;
> > >> >> + struct gpio_desc *wp_gpio;
> > >> >> +
> > >> >> + struct nand_chip chip;
> > >> >> + struct device *dev;
> > >> >> +
> > >> >> + struct completion command_complete;
> > >> >> + struct completion dma_complete;
> > >> >> + bool last_read_error;
> > >> >> +
> > >> >> + dma_addr_t data_dma;
> > >> >> + void *data_buf;
> > >> >> + dma_addr_t oob_dma;
> > >> >> + void *oob_buf;
> > >> >> +
> > >> >> + int cur_chip;
> > >> >> +};
> > >> >
> > >> > This struct should be split in 2 structures: one representing the NAND
> > >> > controller and one representing the NAND chip:
> > >> >
> > >> > struct tegra_nand_controller {
> > >> > struct nand_hw_control base;
> > >> > void __iomem *regs;
> > >> > struct clk *clk;
> > >> > struct device *dev;
> > >> > struct completion command_complete;
> > >> > struct completion dma_complete;
> > >> > bool last_read_error;
> > >> > int cur_chip;
> > >> > };
> > >> >
> > >> > struct tegra_nand {
> > >> > struct nand_chip base;
> > >> > dma_addr_t data_dma;
> > >> > void *data_buf;
> > >> > dma_addr_t oob_dma;
> > >> > void *oob_buf;
> > >> > };
> > >>
> > >> Is there a particular reason why you would leave DMA buffers in the chip
> > >> structure? It seems that is more a controller thing...
> > >
> > > The size of those buffers is likely to be device dependent, so if you
> > > have several NANDs connected to the controller, you'll either have to
> > > have one buffer at the controller level which is max(all-chip-buf-size)
> > > or a buffer per device.
> > >
> > > Also, do you really need these buffers? The core already provide some
> > > which are suitable for DMA (chip->oob_poi and chip->data_buf).
> > >
> >
> > Good question, I am not sure, that was existing code.
> >
> > Are you sure data_buf it is DMA capable?
> >
> > nand_scan_tail allocates with kmalloc:
> >
> > chip->data_buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
>
> Yes, kmalloc() allocates DMA-able buffers, so those are DMA-safe.
Hm, that's not exactly true. It depends on the dma_mask attached to the
device.
^ permalink raw reply
* Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-24 12:51 UTC (permalink / raw)
To: Ilia Lin, vireshk, nm, sboyd, robh, mark.rutland, rjw
Cc: Sudeep Holla, linux-pm, devicetree, linux-kernel
In-Reply-To: <1527152242-31281-2-git-send-email-ilialin@codeaurora.org>
Hi Ilia,
On 24/05/18 09:57, Ilia Lin wrote:
> In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> the CPU frequency subset and voltage value of each OPP varies
> based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> defines the voltage and frequency value based on the msm-id in SMEM
> and speedbin blown in the efuse combination.
> The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> to provide the OPP framework with required information.
> This is used to determine the voltage and frequency value for each OPP of
> operating-points-v2 table when it is parsed by the OPP framework.
>
> Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> ---
> drivers/cpufreq/Kconfig.arm | 10 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> drivers/cpufreq/qcom-cpufreq-kryo.c | 194 +++++++++++++++++++++++++++++++++++
> 4 files changed, 208 insertions(+)
> create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index de55c7d..0bfd40e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -124,6 +124,16 @@ config ARM_OMAP2PLUS_CPUFREQ
> depends on ARCH_OMAP2PLUS
> default ARCH_OMAP2PLUS
>
> +config ARM_QCOM_CPUFREQ_KRYO
> + bool "Qualcomm Kryo based CPUFreq"
> + depends on QCOM_QFPROM
> + depends on QCOM_SMEM
> + select PM_OPP
> + help
> + This adds the CPUFreq driver for Qualcomm Kryo SoC based boards.
> +
> + If in doubt, say N.
> +
> config ARM_S3C_CPUFREQ
> bool
> help
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 8d24ade..fb4a2ec 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_MACH_MVEBU_V7) += mvebu-cpufreq.o
> obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
> obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o
> obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 3b585e4..77d6ab8 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -118,6 +118,9 @@
>
> { .compatible = "nvidia,tegra124", },
>
> + { .compatible = "qcom,apq8096", },
> + { .compatible = "qcom,msm8996", },
> +
> { .compatible = "st,stih407", },
> { .compatible = "st,stih410", },
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c b/drivers/cpufreq/qcom-cpufreq-kryo.c
> new file mode 100644
> index 0000000..9fe379c
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
> @@ -0,0 +1,194 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +/*
> + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors,
> + * the CPU frequency subset and voltage value of each OPP varies
> + * based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables
> + * defines the voltage and frequency value based on the msm-id in SMEM
> + * and speedbin blown in the efuse combination.
> + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC
> + * to provide the OPP framework with required information.
> + * This is used to determine the voltage and frequency value for each OPP of
> + * operating-points-v2 table when it is parsed by the OPP framework.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#define MSM_ID_SMEM 137
> +
> +enum _msm_id {
> + MSM8996V3 = 0xF6ul,
> + APQ8096V3 = 0x123ul,
> + MSM8996SG = 0x131ul,
> + APQ8096SG = 0x138ul,
> +};
> +
> +enum _msm8996_version {
> + MSM8996_V3,
> + MSM8996_SG,
> + NUM_OF_MSM8996_VERSIONS,
> +};
> +
> +static enum _msm8996_version __init qcom_cpufreq_kryo_get_msm_id(void)
> +{
> + size_t len;
> + u32 *msm_id;
> + enum _msm8996_version version;
> +
> + msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
> + /* The first 4 bytes are format, next to them is the actual msm-id */
> + msm_id++;
> +
> + switch ((enum _msm_id)*msm_id) {
> + case MSM8996V3:
> + case APQ8096V3:
> + version = MSM8996_V3;
> + break;
> + case MSM8996SG:
> + case APQ8096SG:
> + version = MSM8996_SG;
> + break;
> + default:
> + version = NUM_OF_MSM8996_VERSIONS;
> + }
> +
> + return version;
> +}
> +
> +static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
> +{
> + struct opp_table *opp_tables[NR_CPUS] = {0};
> + struct platform_device *cpufreq_dt_pdev;
> + enum _msm8996_version msm8996_version;
> + struct nvmem_cell *speedbin_nvmem;
> + struct device_node *np;
> + struct device *cpu_dev;
> + unsigned cpu;
> + u8 *speedbin;
> + u32 versions;
> + size_t len;
> + int ret;
> +
> + cpu_dev = get_cpu_device(0);
> + if (NULL == cpu_dev)
> + return -ENODEV;
> +
> + msm8996_version = qcom_cpufreq_kryo_get_msm_id();
> + if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
> + dev_err(cpu_dev, "Not Snapdragon 820/821!");
> + return -ENODEV;
> + }
> +
> + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> + if (IS_ERR(np))
> + return PTR_ERR(np);
> +
> + if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
> + of_node_put(np);
> + return -ENOENT;
> + }
> +
> + speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> + of_node_put(np);
> + if (IS_ERR(speedbin_nvmem)) {
> + dev_err(cpu_dev, "Could not get nvmem cell: %ld\n",
> + PTR_ERR(speedbin_nvmem));
> + return PTR_ERR(speedbin_nvmem);
> + }
> +
> + speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> + nvmem_cell_put(speedbin_nvmem);
> +
> + switch (msm8996_version) {
> + case MSM8996_V3:
> + versions = 1 << (unsigned int)(*speedbin);
> + break;
> + case MSM8996_SG:
> + versions = 1 << ((unsigned int)(*speedbin) + 4);
> + break;
> + default:
> + BUG();
> + break;
> + }
> +
> + for_each_possible_cpu(cpu) {
> + cpu_dev = get_cpu_device(cpu);
> + if (NULL == cpu_dev) {
> + ret = -ENODEV;
> + goto free_opp;
> + }
> +
> + opp_tables[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> + &versions, 1);
> + if (IS_ERR(opp_tables[cpu])) {
> + ret = PTR_ERR(opp_tables[cpu]);
> + dev_err(cpu_dev, "Failed to set supported hardware\n");
> + goto free_opp;
> + }
> + }
> +
> + cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
> + NULL, 0);
> + if (!IS_ERR(cpufreq_dt_pdev))
> + return 0;
> +
> + ret = PTR_ERR(cpufreq_dt_pdev);
> + dev_err(cpu_dev, "Failed to register platform device\n");
> +
> +free_opp:
> + for_each_possible_cpu(cpu) {
> + if (IS_ERR_OR_NULL(opp_tables[cpu]))
> + break;
> + dev_pm_opp_put_supported_hw(opp_tables[cpu]);
> + }
> +
> + return ret;
> +}
> +
> +static struct platform_driver qcom_cpufreq_kryo_driver = {
> + .probe = qcom_cpufreq_kryo_probe,
> + .driver = {
> + .name = "qcom-cpufreq-kryo",
> + },
> +};
> +
> +/*
> + * Since the driver depends on smem and nvmem drivers, which may
> + * return EPROBE_DEFER, all the real activity is done in the probe,
> + * which may be defered as well. The init here is only registering
> + * the driver and the platform device.
> + */
> +static int __init qcom_cpufreq_kryo_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&qcom_cpufreq_kryo_driver);
> + if (unlikely(ret < 0))
> + return ret;
> +
> + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
> + "qcom-cpufreq-kryo", -1, NULL, 0));
You simply can't do this unconditionally here. This will blow up on
platforms where this driver is not supposed to work. The probe will be
called on non-QCOM or non-Kryo QCOM platforms and I reckon it will
crash trying to execute something in qcom_smem_get.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers"
From: Qiang Yu @ 2018-05-24 12:54 UTC (permalink / raw)
To: Christian König
Cc: Simon Shields, devicetree, Connor Abbott, Marek Vasut,
Neil Armstrong, Andrei Paulau, dri-devel, Vasily Khoruzhick,
Erico Nunes
In-Reply-To: <569c175b-9e1e-a462-9d87-b64d6675ca88@amd.com>
On Thu, May 24, 2018 at 5:41 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 24.05.2018 um 11:24 schrieb Qiang Yu:
>>
>> On Thu, May 24, 2018 at 2:46 PM, Christian König
>> <christian.koenig@amd.com> wrote:
>> [SNIP]
>>>
>>> Because of this we have a separate tracking in amdgpu so that we not only
>>> know who is using which BO, who is using which VM.
>>
>> amdgpu's VM implementation seems too complicated for this simple mali GPU,
>> but I may investigate it more to see if I can make it better.
>
>
> Yeah, completely agree.
>
> The VM handling in amdgpu is really complicated because we had to tune it
> for multiple use cases. E.g. partial resident textures, delayed updates etc
> etc....
>
> But you should at least be able to take the lessons learned we had with that
> VM code and not make the same mistakes again.
>
>>> We intentionally removed the preclose callback to prevent certain use
>>> cases,
>>> bringing it back to allow your use case looks rather fishy to me.
>>
>> Seems other drivers do either the deffer or wait way to adopt the drop
>> of preclose. I can do the same as you suggested, but just not understand
>> why
>> we make our life harder. Can I know what's the case you want to prevent?
>
>
> I think what matters most for your case is the issue is that drivers should
> handle closing a BO because userspace said so in the same way it handles
> closing a BO because of a process termination, but see below.
>
>>> BTW: What exactly is the issue with using the postclose callback?
>>
>> The issue is, when Ctrl+C to terminate an application, if no wait or
>> deffer
>> unmap, buffer just gets unmapped before task is done, so kernel driver
>> gets MMU fault and HW reset to recover the GPU.
>
>
> Yeah, that sounds like exactly one of the reasons we had the callback in the
> first place and worked on to removing it.
>
> See the intention is to have reliable handling, e.g. use the same code path
> for closing a BO because of an IOCTL and closing a BO because of process
> termination.
>
> In other words what happens when userspace closes a BO while the GPU is
> still using it? Would you then run into a GPU reset as well?
Yes, also a MMU fault and GPU reset when user space driver error usage like
this. I think I don't need to avoid this case because it's user error
usage which deserve a GPU reset, but process termination is not. But you
remind me they indeed share the same code path if remove preclose now.
Regards,
Qiang
>
> I mean it's your driver stack, so I'm not against it as long as you can live
> with it. But it's exactly the thing we wanted to avoid here.
Seems
>
> Regards,
> Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH V1 3/3] mmc: host: Register changes for sdcc V5
From: Vijay Viswanath @ 2018-05-24 13:00 UTC (permalink / raw)
To: Evan Green
Cc: adrian.hunter, Ulf Hansson, robh+dt, mark.rutland, linux-mmc,
linux-kernel, shawn.lin, linux-arm-msm, georgi.djakov, devicetree,
asutoshd, stummala, venkatg, jeremymc, Bjorn Andersson, riteshh,
vbadigan, Doug Anderson, sayalil
In-Reply-To: <CAE=gft4DOJTuZDQDX1PL0fXHKPBsgWRPsO1bgunfO0VZ+pa6zQ@mail.gmail.com>
On 5/22/2018 11:42 PM, Evan Green wrote:
> Hi Vijay. Thanks for this patch.
>
> On Thu, May 17, 2018 at 3:30 AM Vijay Viswanath <vviswana@codeaurora.org>
> wrote:
>
>> From: Sayali Lokhande <sayalil@codeaurora.org>
>
>> For SDCC version 5.0.0 and higher, new compatible string
>> "qcom,sdhci-msm-v5" is added.
>
>> Based on the msm variant, pick the relevant variant data and
>> use it for register read/write to msm specific registers.
>
>> Signed-off-by: Sayali Lokhande <sayalil@codeaurora.org>
>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>> ---
>> .../devicetree/bindings/mmc/sdhci-msm.txt | 5 +-
>> drivers/mmc/host/sdhci-msm.c | 344
> +++++++++++++--------
>> 2 files changed, 222 insertions(+), 127 deletions(-)
>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index bfdcdc4..c2b7b2b 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -4,7 +4,10 @@ This file documents differences between the core
> properties in mmc.txt
>> and the properties used by the sdhci-msm driver.
>
>> Required properties:
>> -- compatible: Should contain "qcom,sdhci-msm-v4".
>> +- compatible: Should contain "qcom,sdhci-msm-v4" or "qcom,sdhci-msm-v5".
>> + For SDCC version 5.0.0, MCI registers are removed from
> SDCC
>> + interface and some registers are moved to HC. New
> compatible
>> + string is added to support this change -
> "qcom,sdhci-msm-v5".
>> - reg: Base address and length of the register in the following order:
>> - Host controller register map (required)
>> - SD Core register map (required)
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index bb2bb59..408e6b2 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -33,16 +33,11 @@
>> #define CORE_MCI_GENERICS 0x70
>> #define SWITCHABLE_SIGNALING_VOLTAGE BIT(29)
>
>> -#define CORE_HC_MODE 0x78
>
> Remove CORE_MCI_VERSION as well.
>
Missed it. Will remove
>> #define HC_MODE_EN 0x1
>> #define CORE_POWER 0x0
>> #define CORE_SW_RST BIT(7)
>> #define FF_CLK_SW_RST_DIS BIT(13)
>
>> -#define CORE_PWRCTL_STATUS 0xdc
>> -#define CORE_PWRCTL_MASK 0xe0
>> -#define CORE_PWRCTL_CLEAR 0xe4
>> -#define CORE_PWRCTL_CTL 0xe8
>> #define CORE_PWRCTL_BUS_OFF BIT(0)
>> #define CORE_PWRCTL_BUS_ON BIT(1)
>> #define CORE_PWRCTL_IO_LOW BIT(2)
>> @@ -63,17 +58,13 @@
>> #define CORE_CDR_EXT_EN BIT(19)
>> #define CORE_DLL_PDN BIT(29)
>> #define CORE_DLL_RST BIT(30)
>> -#define CORE_DLL_CONFIG 0x100
>> #define CORE_CMD_DAT_TRACK_SEL BIT(0)
>> -#define CORE_DLL_STATUS 0x108
>
>> -#define CORE_DLL_CONFIG_2 0x1b4
>> #define CORE_DDR_CAL_EN BIT(0)
>> #define CORE_FLL_CYCLE_CNT BIT(18)
>> #define CORE_DLL_CLOCK_DISABLE BIT(21)
>
>> -#define CORE_VENDOR_SPEC 0x10c
>> -#define CORE_VENDOR_SPEC_POR_VAL 0xa1c
>> +#define CORE_VENDOR_SPEC_POR_VAL 0xa1c
>> #define CORE_CLK_PWRSAVE BIT(1)
>> #define CORE_HC_MCLK_SEL_DFLT (2 << 8)
>> #define CORE_HC_MCLK_SEL_HS400 (3 << 8)
>> @@ -111,17 +102,14 @@
>> #define CORE_CDC_SWITCH_BYPASS_OFF BIT(0)
>> #define CORE_CDC_SWITCH_RC_EN BIT(1)
>
>> -#define CORE_DDR_200_CFG 0x184
>> #define CORE_CDC_T4_DLY_SEL BIT(0)
>> #define CORE_CMDIN_RCLK_EN BIT(1)
>> #define CORE_START_CDC_TRAFFIC BIT(6)
>> -#define CORE_VENDOR_SPEC3 0x1b0
>> +
>> #define CORE_PWRSAVE_DLL BIT(3)
>
>> -#define CORE_DDR_CONFIG 0x1b8
>> #define DDR_CONFIG_POR_VAL 0x80040853
>
>> -#define CORE_VENDOR_SPEC_CAPABILITIES0 0x11c
>
>> #define INVALID_TUNING_PHASE -1
>> #define SDHCI_MSM_MIN_CLOCK 400000
>> @@ -380,10 +368,14 @@ static inline int msm_dll_poll_ck_out_en(struct
> sdhci_host *host, u8 poll)
>> u32 wait_cnt = 50;
>> u8 ck_out_en;
>> struct mmc_host *mmc = host->mmc;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + const struct sdhci_msm_offset *msm_offset =
>> + msm_host->offset;
>
> I notice this pattern is pasted all over the place in order to get to the
> offsets. Maybe a macro or inlined function would be cleaner to get you to
> directly to the sdhci_msm_offset struct from sdhci_host, rather than this
> blob of paste soup everywhere. In some places you do seem to use the
> intermediate locals, so those cases wouldn't need to use the new helper.
>
>
>> /* Poll for CK_OUT_EN bit. max. poll time = 50us */
>> - ck_out_en = !!(readl_relaxed(host->ioaddr + CORE_DLL_CONFIG) &
>> - CORE_CK_OUT_EN);
>> + ck_out_en = !!(readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config) & CORE_CK_OUT_EN);
>
>> while (ck_out_en != poll) {
>> if (--wait_cnt == 0) {
>> @@ -393,8 +385,8 @@ static inline int msm_dll_poll_ck_out_en(struct
> sdhci_host *host, u8 poll)
>> }
>> udelay(1);
>
>> - ck_out_en = !!(readl_relaxed(host->ioaddr +
> CORE_DLL_CONFIG) &
>> - CORE_CK_OUT_EN);
>> + ck_out_en = !!(readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config) & CORE_CK_OUT_EN);
>> }
>
>> return 0;
>> @@ -410,16 +402,20 @@ static int msm_config_cm_dll_phase(struct
> sdhci_host *host, u8 phase)
>> unsigned long flags;
>> u32 config;
>> struct mmc_host *mmc = host->mmc;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + const struct sdhci_msm_offset *msm_offset =
>> + msm_host->offset;
>
>> if (phase > 0xf)
>> return -EINVAL;
>
>> spin_lock_irqsave(&host->lock, flags);
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>> config &= ~(CORE_CDR_EN | CORE_CK_OUT_EN);
>> config |= (CORE_CDR_EXT_EN | CORE_DLL_EN);
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
>
>> /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '0' */
>> rc = msm_dll_poll_ck_out_en(host, 0);
>> @@ -430,24 +426,24 @@ static int msm_config_cm_dll_phase(struct
> sdhci_host *host, u8 phase)
>> * Write the selected DLL clock output phase (0 ... 15)
>> * to CDR_SELEXT bit field of DLL_CONFIG register.
>> */
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>> config &= ~CDR_SELEXT_MASK;
>> config |= grey_coded_phase_table[phase] << CDR_SELEXT_SHIFT;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>> config |= CORE_CK_OUT_EN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
>
>> /* Wait until CK_OUT_EN bit of DLL_CONFIG register becomes '1' */
>> rc = msm_dll_poll_ck_out_en(host, 1);
>> if (rc)
>> goto err_out;
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>> config |= CORE_CDR_EN;
>> config &= ~CORE_CDR_EXT_EN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
>
> Nit: host->ioaddr + msm_offset->core_dll_config might benefit from having
> its own local, since you use it so much in this function. Same goes for
> where I've noted below...
>
core_dll_config is very much used. But having a local for it feels like
a bad idea. As different versions come up, the most used register may
change. So it would be better to stick to a consistent approach to
accessing every register.
>> goto out;
>
>> err_out:
>> @@ -573,6 +569,10 @@ static int msm_find_most_appropriate_phase(struct
> sdhci_host *host,
>> static inline void msm_cm_dll_set_freq(struct sdhci_host *host)
>> {
>> u32 mclk_freq = 0, config;
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + const struct sdhci_msm_offset *msm_offset =
>> + msm_host->offset;
>
>> /* Program the MCLK value to MCLK_FREQ bit field */
>> if (host->clock <= 112000000)
>> @@ -592,10 +592,10 @@ static inline void msm_cm_dll_set_freq(struct
> sdhci_host *host)
>> else if (host->clock <= 200000000)
>> mclk_freq = 7;
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
> msm_offset->core_dll_config);
>> config &= ~CMUX_SHIFT_PHASE_MASK;
>> config |= mclk_freq << CMUX_SHIFT_PHASE_SHIFT;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
> msm_offset->core_dll_config);
>> }
>
>> /* Initialize the DLL (Programmable Delay Line) */
>> @@ -607,6 +607,8 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>> int wait_cnt = 50;
>> unsigned long flags;
>> u32 config;
>> + const struct sdhci_msm_offset *msm_offset =
>> + msm_host->offset;
>
>> spin_lock_irqsave(&host->lock, flags);
>
>> @@ -615,34 +617,43 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>> * tuning is in progress. Keeping PWRSAVE ON may
>> * turn off the clock.
>> */
>> - config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
>> + config = readl_relaxed(host->ioaddr +
> msm_offset->core_vendor_spec);
>> config &= ~CORE_CLK_PWRSAVE;
>> - writel_relaxed(config, host->ioaddr + CORE_VENDOR_SPEC);
>> + writel_relaxed(config, host->ioaddr +
> msm_offset->core_vendor_spec);
>
>> if (msm_host->use_14lpp_dll_reset) {
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config &= ~CORE_CK_OUT_EN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> config |= CORE_DLL_CLOCK_DISABLE;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> }
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config |= CORE_DLL_RST;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config |= CORE_DLL_PDN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>> msm_cm_dll_set_freq(host);
>
>> if (msm_host->use_14lpp_dll_reset &&
>> !IS_ERR_OR_NULL(msm_host->xo_clk)) {
>> u32 mclk_freq = 0;
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> config &= CORE_FLL_CYCLE_CNT;
>> if (config)
>> mclk_freq = DIV_ROUND_CLOSEST_ULL((host->clock *
> 8),
>> @@ -651,40 +662,52 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>> mclk_freq = DIV_ROUND_CLOSEST_ULL((host->clock *
> 4),
>> clk_get_rate(msm_host->xo_clk));
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> config &= ~(0xFF << 10);
>> config |= mclk_freq << 10;
>
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> /* wait for 5us before enabling DLL clock */
>> udelay(5);
>> }
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config &= ~CORE_DLL_RST;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config &= ~CORE_DLL_PDN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>
>> if (msm_host->use_14lpp_dll_reset) {
>> msm_cm_dll_set_freq(host);
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG_2);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> config &= ~CORE_DLL_CLOCK_DISABLE;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG_2);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config_2);
>> }
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config |= CORE_DLL_EN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>
>> - config = readl_relaxed(host->ioaddr + CORE_DLL_CONFIG);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_dll_config);
>> config |= CORE_CK_OUT_EN;
>> - writel_relaxed(config, host->ioaddr + CORE_DLL_CONFIG);
>> + writel_relaxed(config, host->ioaddr +
>> + msm_offset->core_dll_config);
>
>
> ...here. A local for host->ioaddr + msm_offset->core_dll_config would save
> you a lot of split lines.
>
>> @@ -1272,12 +1327,17 @@ static void sdhci_msm_dump_pwr_ctrl_regs(struct
> sdhci_host *host)
>> {
>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> + const struct sdhci_msm_offset *msm_offset =
>> + msm_host->offset;
>
>> pr_err("%s: PWRCTL_STATUS: 0x%08x | PWRCTL_MASK: 0x%08x |
> PWRCTL_CTL: 0x%08x\n",
>> - mmc_hostname(host->mmc),
>> - readl_relaxed(msm_host->core_mem +
> CORE_PWRCTL_STATUS),
>> - readl_relaxed(msm_host->core_mem +
> CORE_PWRCTL_MASK),
>> - readl_relaxed(msm_host->core_mem +
> CORE_PWRCTL_CTL));
>> + mmc_hostname(host->mmc),
>> + msm_host->var_ops->msm_readl_relaxed(host,
>
> There's a weird extra space here.
>
>> + msm_offset->core_pwrctl_status),
>> + msm_host->var_ops->msm_readl_relaxed(host,
>> + msm_offset->core_pwrctl_mask),
>> + msm_host->var_ops->msm_readl_relaxed(host,
>> + msm_offset->core_pwrctl_ctl));
>
> I think the idea of function pointers is fine, but overall the use of them
> everywhere sure is ugly. It makes it really hard to actually see what's
> happening. I wonder if things might look a lot cleaner with a helper
> function here. Then instead of:
>
> msm_host->var_ops->msm_readl_relaxed(host, msm_offset->core_pwrctl_ctl);
>
> You could have
>
> msm_core_read(host, msm_offset->core_pwrctl_ctl);
>
if we use a helper function, then we will have to pass msm_host into it
as well. Otherwise there would be the hassle of deriving msm_host
address from sdhci_host.
How about using a MACRO here instead for readability ?
>> @@ -1553,7 +1619,8 @@ static void sdhci_msm_set_regulator_caps(struct
> sdhci_msm_host *msm_host)
>> */
>> u32 io_level = msm_host->curr_io_level;
>
>> - config = readl_relaxed(host->ioaddr + CORE_VENDOR_SPEC);
>> + config = readl_relaxed(host->ioaddr +
>> + msm_offset->core_vendor_spec);
>
> Remove the second space before the readl_relaxed.
>
>> @@ -1710,32 +1793,40 @@ static int sdhci_msm_probe(struct platform_device
> *pdev)
>> dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
>> }
>
>> - core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> - msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
> core_memres);
>> + if (!msm_host->mci_removed) {
>> + core_memres = platform_get_resource(pdev, IORESOURCE_MEM,
> 1);
>> + msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> + core_memres);
>
>> - if (IS_ERR(msm_host->core_mem)) {
>> - dev_err(&pdev->dev, "Failed to remap registers\n");
>> - ret = PTR_ERR(msm_host->core_mem);
>> - goto clk_disable;
>> + if (IS_ERR(msm_host->core_mem)) {
>> + dev_err(&pdev->dev, "Failed to remap
> registers\n");
>> + ret = PTR_ERR(msm_host->core_mem);
>> + goto clk_disable;
>> + }
>> }
>
>> /* Reset the vendor spec register to power on reset state */
>> writel_relaxed(CORE_VENDOR_SPEC_POR_VAL,
>> - host->ioaddr + CORE_VENDOR_SPEC);
>> -
>> - /* Set HC_MODE_EN bit in HC_MODE register */
>> - writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
>> -
>> - config = readl_relaxed(msm_host->core_mem + CORE_HC_MODE);
>> - config |= FF_CLK_SW_RST_DIS;
>> - writel_relaxed(config, msm_host->core_mem + CORE_HC_MODE);
>> + host->ioaddr + msm_offset->core_vendor_spec);
>> +
>> + if (!msm_host->mci_removed) {
>> + /* Set HC_MODE_EN bit in HC_MODE register */
>> + msm_host->var_ops->msm_writel_relaxed(HC_MODE_EN, host,
>> + msm_offset->core_hc_mode);
>> + config = msm_host->var_ops->msm_readl_relaxed(host,
>> + msm_offset->core_hc_mode);
>> + config |= FF_CLK_SW_RST_DIS;
>> + msm_host->var_ops->msm_writel_relaxed(config, host,
>> + msm_offset->core_hc_mode);
>> + }
>
>> host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
>> dev_dbg(&pdev->dev, "Host Version: 0x%x Vendor Version 0x%x\n",
>> host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
>> SDHCI_VENDOR_VER_SHIFT));
>
>> - core_version = readl_relaxed(msm_host->core_mem +
> CORE_MCI_VERSION);
>> + core_version = msm_host->var_ops->msm_readl_relaxed(host,
>> + msm_offset->core_mci_version);
>
> Another double space after the =. Perhaps this was a find/replace error?
> Look out for more of these that I missed.
>
> -Evan
>
Thanks for pointing these out. Will check for such issues in all 3 patches.
Thanks,
Vijay
^ permalink raw reply
* RE: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: ilialin @ 2018-05-24 13:03 UTC (permalink / raw)
To: 'Sudeep Holla', vireshk, nm, sboyd, robh, mark.rutland,
rjw
Cc: linux-pm, devicetree, linux-kernel
In-Reply-To: <860be68b-cac0-9efc-b3c7-cc75b391a4c3@arm.com>
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Thursday, May 24, 2018 15:52
> To: Ilia Lin <ilialin@codeaurora.org>; vireshk@kernel.org; nm@ti.com;
> sboyd@kernel.org; robh@kernel.org; mark.rutland@arm.com;
> rjw@rjwysocki.net
> Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-pm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
>
> Hi Ilia,
>
>
> On 24/05/18 09:57, Ilia Lin wrote:
> > In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO
> > processors, the CPU frequency subset and voltage value of each OPP
> > varies based on the silicon variant in use. Qualcomm Process Voltage
> > Scaling Tables defines the voltage and frequency value based on the
> > msm-id in SMEM and speedbin blown in the efuse combination.
> > The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the
> > SoC to provide the OPP framework with required information.
> > This is used to determine the voltage and frequency value for each OPP
> > of
> > operating-points-v2 table when it is parsed by the OPP framework.
> >
> > Signed-off-by: Ilia Lin <ilialin@codeaurora.org>
> > ---
> > drivers/cpufreq/Kconfig.arm | 10 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/cpufreq-dt-platdev.c | 3 +
> > drivers/cpufreq/qcom-cpufreq-kryo.c | 194
> > +++++++++++++++++++++++++++++++++++
> > 4 files changed, 208 insertions(+)
> > create mode 100644 drivers/cpufreq/qcom-cpufreq-kryo.c
> >
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index de55c7d..0bfd40e 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -124,6 +124,16 @@ config ARM_OMAP2PLUS_CPUFREQ
> > depends on ARCH_OMAP2PLUS
> > default ARCH_OMAP2PLUS
> >
> > +config ARM_QCOM_CPUFREQ_KRYO
> > + bool "Qualcomm Kryo based CPUFreq"
> > + depends on QCOM_QFPROM
> > + depends on QCOM_SMEM
> > + select PM_OPP
> > + help
> > + This adds the CPUFreq driver for Qualcomm Kryo SoC based boards.
> > +
> > + If in doubt, say N.
> > +
> > config ARM_S3C_CPUFREQ
> > bool
> > help
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index
> > 8d24ade..fb4a2ec 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -65,6 +65,7 @@ obj-$(CONFIG_MACH_MVEBU_V7) +=
> mvebu-cpufreq.o
> > obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
> > obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> > obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> > +obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-
> kryo.o
> > obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o
> > obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> > b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 3b585e4..77d6ab8 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -118,6 +118,9 @@
> >
> > { .compatible = "nvidia,tegra124", },
> >
> > + { .compatible = "qcom,apq8096", },
> > + { .compatible = "qcom,msm8996", },
> > +
> > { .compatible = "st,stih407", },
> > { .compatible = "st,stih410", },
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c
> > b/drivers/cpufreq/qcom-cpufreq-kryo.c
> > new file mode 100644
> > index 0000000..9fe379c
> > --- /dev/null
> > +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
> > @@ -0,0 +1,194 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +/*
> > + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO
> > +processors,
> > + * the CPU frequency subset and voltage value of each OPP varies
> > + * based on the silicon variant in use. Qualcomm Process Voltage
> > +Scaling Tables
> > + * defines the voltage and frequency value based on the msm-id in
> > +SMEM
> > + * and speedbin blown in the efuse combination.
> > + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from
> > +the SoC
> > + * to provide the OPP framework with required information.
> > + * This is used to determine the voltage and frequency value for each
> > +OPP of
> > + * operating-points-v2 table when it is parsed by the OPP framework.
> > + */
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/slab.h>
> > +#include <linux/soc/qcom/smem.h>
> > +
> > +#define MSM_ID_SMEM 137
> > +
> > +enum _msm_id {
> > + MSM8996V3 = 0xF6ul,
> > + APQ8096V3 = 0x123ul,
> > + MSM8996SG = 0x131ul,
> > + APQ8096SG = 0x138ul,
> > +};
> > +
> > +enum _msm8996_version {
> > + MSM8996_V3,
> > + MSM8996_SG,
> > + NUM_OF_MSM8996_VERSIONS,
> > +};
> > +
> > +static enum _msm8996_version __init
> > +qcom_cpufreq_kryo_get_msm_id(void)
> > +{
> > + size_t len;
> > + u32 *msm_id;
> > + enum _msm8996_version version;
> > +
> > + msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> MSM_ID_SMEM, &len);
> > + /* The first 4 bytes are format, next to them is the actual msm-id */
> > + msm_id++;
> > +
> > + switch ((enum _msm_id)*msm_id) {
> > + case MSM8996V3:
> > + case APQ8096V3:
> > + version = MSM8996_V3;
> > + break;
> > + case MSM8996SG:
> > + case APQ8096SG:
> > + version = MSM8996_SG;
> > + break;
> > + default:
> > + version = NUM_OF_MSM8996_VERSIONS;
> > + }
> > +
> > + return version;
> > +}
> > +
> > +static int qcom_cpufreq_kryo_probe(struct platform_device *pdev) {
> > + struct opp_table *opp_tables[NR_CPUS] = {0};
> > + struct platform_device *cpufreq_dt_pdev;
> > + enum _msm8996_version msm8996_version;
> > + struct nvmem_cell *speedbin_nvmem;
> > + struct device_node *np;
> > + struct device *cpu_dev;
> > + unsigned cpu;
> > + u8 *speedbin;
> > + u32 versions;
> > + size_t len;
> > + int ret;
> > +
> > + cpu_dev = get_cpu_device(0);
> > + if (NULL == cpu_dev)
> > + return -ENODEV;
> > +
> > + msm8996_version = qcom_cpufreq_kryo_get_msm_id();
> > + if (NUM_OF_MSM8996_VERSIONS == msm8996_version) {
> > + dev_err(cpu_dev, "Not Snapdragon 820/821!");
> > + return -ENODEV;
> > + }
> > +
> > + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> > + if (IS_ERR(np))
> > + return PTR_ERR(np);
> > +
> > + if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) {
> > + of_node_put(np);
> > + return -ENOENT;
> > + }
> > +
> > + speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> > + of_node_put(np);
> > + if (IS_ERR(speedbin_nvmem)) {
> > + dev_err(cpu_dev, "Could not get nvmem cell: %ld\n",
> > + PTR_ERR(speedbin_nvmem));
> > + return PTR_ERR(speedbin_nvmem);
> > + }
> > +
> > + speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> > + nvmem_cell_put(speedbin_nvmem);
> > +
> > + switch (msm8996_version) {
> > + case MSM8996_V3:
> > + versions = 1 << (unsigned int)(*speedbin);
> > + break;
> > + case MSM8996_SG:
> > + versions = 1 << ((unsigned int)(*speedbin) + 4);
> > + break;
> > + default:
> > + BUG();
> > + break;
> > + }
> > +
> > + for_each_possible_cpu(cpu) {
> > + cpu_dev = get_cpu_device(cpu);
> > + if (NULL == cpu_dev) {
> > + ret = -ENODEV;
> > + goto free_opp;
> > + }
> > +
> > + opp_tables[cpu] =
> dev_pm_opp_set_supported_hw(cpu_dev,
> > + &versions, 1);
> > + if (IS_ERR(opp_tables[cpu])) {
> > + ret = PTR_ERR(opp_tables[cpu]);
> > + dev_err(cpu_dev, "Failed to set supported
> hardware\n");
> > + goto free_opp;
> > + }
> > + }
> > +
> > + cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -
> 1,
> > + NULL, 0);
> > + if (!IS_ERR(cpufreq_dt_pdev))
> > + return 0;
> > +
> > + ret = PTR_ERR(cpufreq_dt_pdev);
> > + dev_err(cpu_dev, "Failed to register platform device\n");
> > +
> > +free_opp:
> > + for_each_possible_cpu(cpu) {
> > + if (IS_ERR_OR_NULL(opp_tables[cpu]))
> > + break;
> > + dev_pm_opp_put_supported_hw(opp_tables[cpu]);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static struct platform_driver qcom_cpufreq_kryo_driver = {
> > + .probe = qcom_cpufreq_kryo_probe,
> > + .driver = {
> > + .name = "qcom-cpufreq-kryo",
> > + },
> > +};
> > +
> > +/*
> > + * Since the driver depends on smem and nvmem drivers, which may
> > + * return EPROBE_DEFER, all the real activity is done in the probe,
> > + * which may be defered as well. The init here is only registering
> > + * the driver and the platform device.
> > + */
> > +static int __init qcom_cpufreq_kryo_init(void) {
> > + int ret;
> > +
> > + ret = platform_driver_register(&qcom_cpufreq_kryo_driver);
> > + if (unlikely(ret < 0))
> > + return ret;
> > +
> > + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
> > + "qcom-cpufreq-kryo", -1, NULL, 0));
>
>
> You simply can't do this unconditionally here. This will blow up on platforms
> where this driver is not supposed to work. The probe will be called on non-
> QCOM or non-Kryo QCOM platforms and I reckon it will crash trying to
> execute something in qcom_smem_get.
What do you mean by 'unconditionally'?
The driver depends on the smem and nvmem drivers, which depend on ARCH_QCOM:
+ depends on QCOM_QFPROM
+ depends on QCOM_SMEM
And if SMEM read in the probe returns something other than Kryo, it will exit.
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* [PATCH 0/3] Dts nodes for Keystone2 hw_rng driver
From: Vitaly Andrianov @ 2018-05-24 13:12 UTC (permalink / raw)
To: ssantosh, robh+dt, mark.rutland, linux-arm-kernel, devicetree,
linux-kernel
Cc: vitalya, m-karicheri2
This series adds dts nodes for Keystone2 hw_rng driver
Vitaly Andrianov (3):
ARM: dts: k2hk: add dts node for k2hk hw_rng driver
ARM: dts: k2l: add dts node for k2l hw_rng driver
ARM: dts: k2e: add dts node for k2e hw_rng driver
arch/arm/boot/dts/keystone-k2e-netcp.dtsi | 20 ++++++++++++++++++++
arch/arm/boot/dts/keystone-k2hk-netcp.dtsi | 20 ++++++++++++++++++++
arch/arm/boot/dts/keystone-k2l-netcp.dtsi | 20 ++++++++++++++++++++
3 files changed, 60 insertions(+)
--
2.7.4
^ permalink raw reply
* [PATCH 1/3] ARM: dts: k2hk: add dts node for k2hk hw_rng driver
From: Vitaly Andrianov @ 2018-05-24 13:12 UTC (permalink / raw)
To: ssantosh, robh+dt, mark.rutland, linux-arm-kernel, devicetree,
linux-kernel
Cc: vitalya, m-karicheri2, Tero Kristo
In-Reply-To: <1527167538-29837-1-git-send-email-vitalya@ti.com>
This patch adds dts node for k2hk hw_random generator driver
Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
[t-kristo@ti.com: added missing addresses from node identifiers]
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
arch/arm/boot/dts/keystone-k2hk-netcp.dtsi | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2hk-netcp.dtsi b/arch/arm/boot/dts/keystone-k2hk-netcp.dtsi
index b88c068..e203145 100644
--- a/arch/arm/boot/dts/keystone-k2hk-netcp.dtsi
+++ b/arch/arm/boot/dts/keystone-k2hk-netcp.dtsi
@@ -228,3 +228,23 @@ netcp: netcp@2000000 {
};
};
};
+
+sa_subsys: subsys@20c0000 {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x20c0000 0x40000>;
+
+ sa_config: subsys@0 {
+ compatible = "syscon";
+ reg = <0x0 0x100>;
+ };
+
+ rng@24000 {
+ compatible = "ti,keystone-rng";
+ reg = <0x24000 0x1000>;
+ ti,syscon-sa-cfg = <&sa_config>;
+ clocks = <&clksa>;
+ clock-names = "fck";
+ };
+};
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3] ARM: dts: k2l: add dts node for k2l hw_rng driver
From: Vitaly Andrianov @ 2018-05-24 13:12 UTC (permalink / raw)
To: ssantosh, robh+dt, mark.rutland, linux-arm-kernel, devicetree,
linux-kernel
Cc: vitalya, m-karicheri2, Tero Kristo
In-Reply-To: <1527167538-29837-1-git-send-email-vitalya@ti.com>
This patch adds dts node for k2l hw_random generator driver
Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
[t-kristo@ti.com: added missing addresses from node identifiers]
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
arch/arm/boot/dts/keystone-k2l-netcp.dtsi | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2l-netcp.dtsi b/arch/arm/boot/dts/keystone-k2l-netcp.dtsi
index 9ec8422..a2e47ba 100644
--- a/arch/arm/boot/dts/keystone-k2l-netcp.dtsi
+++ b/arch/arm/boot/dts/keystone-k2l-netcp.dtsi
@@ -208,3 +208,23 @@ netcp: netcp@26000000 {
};
};
};
+
+sa_subsys: subsys@26080000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "simple-bus";
+ ranges = <0 0x26080000 0x40000>;
+
+ sa_config: subsys@0 {
+ compatible = "syscon";
+ reg = <0x0 0x100>;
+ };
+
+ rng@24000 {
+ compatible = "ti,keystone-rng";
+ reg = <0x24000 0x1000>;
+ ti,syscon-sa-cfg = <&sa_config>;
+ clocks = <&clksa>;
+ clock-names = "fck";
+ };
+};
--
2.7.4
^ permalink raw reply related
* [PATCH 3/3] ARM: dts: k2e: add dts node for k2e hw_rng driver
From: Vitaly Andrianov @ 2018-05-24 13:12 UTC (permalink / raw)
To: ssantosh, robh+dt, mark.rutland, linux-arm-kernel, devicetree,
linux-kernel
Cc: vitalya, m-karicheri2
In-Reply-To: <1527167538-29837-1-git-send-email-vitalya@ti.com>
This patch adds dts node for k2e hw_random generator driver
Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
arch/arm/boot/dts/keystone-k2e-netcp.dtsi | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2e-netcp.dtsi b/arch/arm/boot/dts/keystone-k2e-netcp.dtsi
index a17311c..1db17ec 100644
--- a/arch/arm/boot/dts/keystone-k2e-netcp.dtsi
+++ b/arch/arm/boot/dts/keystone-k2e-netcp.dtsi
@@ -225,3 +225,23 @@ netcp: netcp@24000000 {
};
};
};
+
+sa_subsys: subsys@24080000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "simple-bus";
+ ranges = <0 0x24080000 0x40000>;
+
+ sa_config: subsys@0 {
+ compatible = "syscon";
+ reg = <0x0 0x100>;
+ };
+
+ rng@24000 {
+ compatible = "ti,keystone-rng";
+ reg = <0x24000 0x1000>;
+ ti,syscon-sa-cfg = <&sa_config>;
+ clocks = <&clksa>;
+ clock-names = "fck";
+ };
+};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v3 0/7] Add support for QCA8334 switch
From: Andrew Lunn @ 2018-05-24 13:17 UTC (permalink / raw)
To: Michal Vokáč
Cc: Florian Fainelli, netdev, linux-kernel, devicetree,
vivien.didelot, mark.rutland, robh+dt, davem, michal.vokac
In-Reply-To: <f8bf325c-77a5-d44b-34c3-93ea4b11fd84@gmail.com>
> Thank you too Florian. And also big thank to you Andrew. You helped me
> a lot to debug the RGMII issue. I have been stuck at that for more than
> a month and would not resolve it without your help.
We are here to help. I also got stuck figuring out RGMII issues, so i
know what it feels like...
> As I have done this in a process of upgrading our BSP to a more recent
> kernel, and hopefully mainline, I now need to move on to other parts of
> the board.
Do you think you can mainline the board? It would be nice to have an
in kernel board using this switch. If you post the device tree
patches, please Cc: me.
Andrew
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding
From: Gilad Ben-Yossef @ 2018-05-24 13:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, Herbert Xu, David S. Miller,
Ofir Drang, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, linux-clk
In-Reply-To: <CAMuHMdX-t8WfjRKpZ7UxXBbBNojsn-8z7_BM9grmLJcf=9Ks2Q@mail.gmail.com>
On Tue, May 22, 2018 at 10:48 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Gilad,
>
> On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
>>> does not distinguish between the absence of the clock property, and an
>>> actual error in getting the clock, and never considers any error a failure
>>> (incl. -PROBE_DEFER).
>>>
>>> As of_clk_get() returns -ENOENT for both a missing clock property and a
>>> missing clock, you should use (devm_)clk_get() instead, and distinguish
>>> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>>
>> I was trying to do as you suggested but I didn't quite get what is the
>> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
>
> It's the (optional) name of the clock, helpful in case there is more than one.
> In your case, NULL is fine.
>
I have assumed as much and tried it, it did not work and so I assumed
I am missing something and asked you.
It turns out I was missing the fact I was using the wrong device tree
file... :-(
So thanks, it works now :-)
Having said that, while using devm)clk_get() is a better approach, it
does not seems to distinguish
between no "clocks" and a failure to clock information - it returns
ENOENT in both cases as well.
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
^ permalink raw reply
* Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805
From: Robin Murphy @ 2018-05-24 13:25 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mark Rutland, devicetree, linux-watchdog, Catalin Marinas,
Will Deacon, linux-kernel, Rob Herring, Ray Jui,
bcm-kernel-feedback-list, Wim Van Sebroeck, Frank Rowand,
linux-arm-kernel
In-Reply-To: <20180523181058.GC27570@roeck-us.net>
On 23/05/18 19:10, Guenter Roeck wrote:
> On Wed, May 23, 2018 at 11:57:25AM +0100, Robin Murphy wrote:
>> On 22/05/18 19:47, Ray Jui wrote:
>>> Update the SP805 binding document to add optional 'timeout-sec'
>>> devicetree property
>>>
>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>> ---
>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>> index edc4f0e..f898a86 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>> @@ -19,6 +19,8 @@ Required properties:
>>> Optional properties:
>>> - interrupts : Should specify WDT interrupt number.
>>> +- timeout-sec : Should specify default WDT timeout in seconds. If unset, the
>>> + default timeout is 30 seconds
>>
>> According to the SP805 TRM, the default interval is dependent on the rate of
>> WDOGCLK, but would typically be a lot longer than that :/
>>
> Depends on the definition of "default". In the context of watchdog drivers,
> it is (or should be) a driver default, not a chip default.
DT describes hardware, not driver behaviour.
I appreciate that where a timeout *is* specified, that is effectively a
hardware aspect even if it's something an OS consuming the binding still
has to voluntarily program into the device. The notion of "this is the
longest period of time for which you can reasonably expect to see no
activity under normal operation" is indeed a property of the platform as
a whole - a system with user-accessible PCIe slots may need to reflect
the worst case of one CPU waiting for an ATS invalidation timeout with
interrupts disabled, whereas a much shorter period might be appropriate
for the same SoC in some closed-down embedded device.
The absence of the property, though, doesn't convey anything other than
"I don't know" and/or "it doesn't really matter", and in that situation
the default is always going to be "whatever the OS thinks is
appropriate". The binding itself can't possibly know, whereas an OS
might be configured for some pseudo-real-time application which it knows
warrants a maximum of 100ms regardless of what the DT does or doesn't
say. In the case of SP805, if the OS doesn't reconfigure it at all,
there happens to be an actual hardware default of (2^32 / WDOGCLK), but
since that's already implicit in the compatible it doesn't really need
saying either.
Optional properties don't need to explicitly state what their absence
might infer, especially when it's not directly meaningful (just imagine
trying to do that for bindings/regulator/regulator.txt...), so I would
suggest following the 93% of existing bindings which simply don't try to
claim some default value for this property.
I also think the fact that, within the context of this patch series, the
Linux driver doesn't even do what the binding claims only goes to help
make my point ;)
Robin.
^ permalink raw reply
* Re: OMAP serial runtime PM and autosuspend (was: Re: [PATCH 4/7] dt-bindings: gnss: add u-blox binding))
From: Tony Lindgren @ 2018-05-24 13:32 UTC (permalink / raw)
To: Johan Hovold
Cc: Sebastian Reichel, H. Nikolaus Schaller, Andreas Kemnade,
Mark Rutland, Arnd Bergmann, Pavel Machek,
linux-kernel@vger.kernel.org,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Greg Kroah-Hartman, Rob Herring, linux-serial, linux-omap,
linux-pm, Andy Shevchenko
In-Reply-To: <20180524091742.GZ30172@localhost>
* Johan Hovold <johan@kernel.org> [180524 09:20]:
> On Mon, May 21, 2018 at 08:48:32AM -0700, Tony Lindgren wrote:
> >
> > Yes the bug for closed ports needs to be fixed for sure.
>
> I did some forensic on this and it seems this problem has "always" been
> there. Specifically, closed ports have never been runtime suspended
> unless a non-negative autosuspend delay has been set by user space since
> fcdca75728ac ("ARM: OMAP2+: UART: Add runtime pm support for omap-serial
> driver") which was merged seven years ago.
>
> So while it would certainly be nice to save some more power by default,
> this would really be a new feature rather than a bug or regression fix
> (which reduces the urgency for this issue somewhat too).
Yes it's been there since the start.
> > > > > 2. aggressive serial RPM, where the controller is allowed to
> > > > > suspend while the port is open even though this may result in
> > > > > lost characters when waking up on incoming data
> > > >
> > > > In this case it seems that the only thing needed is to just
> > > > configure the autosuspend delay for the parent port. The use of
> > > > -1 has been around since the start of runtime PM AFAIK, so maybe
> > > > we should just document it. I guess we could also introduce
> > > > pm_runtime_block_autoidle_unless_configured() :)
> > >
> > > The implications of a negative autosuspend delay are already documented
> > > (in Documentation/power/runtime_pm.txt); it's just the omap drivers that
> > > gets it wrong when trying to do things which aren't currently supported
> > > (and never have been).
> > >
> > > So I still think we need a new mechanism for this.
> >
> > Well if you have some better mechanism in mind let's try it out. Short of
> > sprinkling pm_runtime_force_suspend/resume calls all over, I'm out of ideas
> > right now.
>
> Yeah, that would be too much of a hack and likely wouldn't work either
> (and we really should do away with those _force calls altogether).
>
> I've been thinking a bit too much about this already, but it may be
> possible to use the pm QoS framework for this. A resume latency can be
> set through sysfs where "n/a" is defined to mean "no latency accepted"
> (i.e. controller remains always-on while port is open) and "0" means
> "any latency accepted" (i.e. omap aggressive serial RPM is allowed).
Oh yeah, PM QoS might work here!
> Now, implementing this may get a little tricky as we want to be able to
> change this setting on the fly (consider consoles) and we need to figure
> out the interaction with serdev (user space should probably not be
> allowed to request a resume latency for ports used by serdev).
It sounds like Andy Shevchenko has a series of patches that just might
allow us to make this all generic for Linux serial framework. So adding
Andy to Cc, I don't think he has posted all the patches yet.
Andy, see the PM QoS comment above for console idling :)
> I'd be happy to dig into this some more, but not in my spare time I'm
> afraid.
Indeed.
> > > > > For normal ttys, we need a user-space interface for selecting between
> > > > > the two, and for serdev we may want a way to select the RPM scheme from
> > > > > within the kernel.
> > > > >
> > > > > Note that with my serdev controller runtime PM patch, serdev core could
> > > > > always opt for aggressive PM (as by default serdev core holds an RPM
> > > > > reference for the controller while the port is open).
> > > >
> > > > So if your serdev controller was to set the parent autosuspend
> > > > delay on open() and set it back on close() this should work?
> > >
> > > Is it really the job of a serdev driver to set the autosuspend delay of
> > > a parent controller? Isn't this somethings which depends on the
> > > characteristics of the controller (possibly configurable by user space)
> > > such as the cost of runtime suspending and resuming?
> >
> > Only in some cases will the serdev driver know it's safe to configure
> > the parent controller. Configuring the parent controller from userspace
> > works just fine as we've seen for years now.
>
> Yes, user space may override the default settings provided by the serial
> driver, but a serdev driver, in contrast, knows nothing about the
> underlying serial hardware.
>
> > > The patch I posted works with what we have today; if a parent serial
> > > controller driver uses aggressive runtime PM by default or after having
> > > been configured through sysfs to do so.
> >
> > Yeah let's stick with configuring the parent controller from userspace
> > for now at least.
>
> Yep, status quo works for the time being (since this isn't a
> regression).
>
> > > What I'm getting at here is that the delay should be set by the serial
> > > driver implementing aggressive runtime PM. Then all we need is a
> > > mechanism to determine whether an extra RPM reference should be taken at
> > > tty open or not (configurable by user space, defaulting to yes).
> >
> > OK yeah some additional on/off switch seems to be missing here.
>
> As mentioned above, PM QoS resume latency may possibly be used, and
> otherwise me may able to define a new (generic) QoS flag for this.
Good idea.
> > > Specifically, the serial drivers themselves would always use
> > > autosuspend and not have to deal with supporting the two RPM schemes
> > > (normal vs aggressive runtime PM).
> >
> > OK. So if I understand your idea right, we could have autosuspend timeout
> > set to 3000ms in the 8250_omap.c but still default to RPM blocked?
> > Then user can enable aggressive PM via /sys as desired, right?
>
> Not RPM blocked; the ports must always be able to suspend when the port
> is closed. But user space should be able to enable the aggressive
> (active) runtime PM via sysfs independently of the autosuspend delay,
> yes.
Yup OK, I like the PM QoS approach.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip
From: Rob Herring @ 2018-05-24 13:38 UTC (permalink / raw)
To: Heiko Stuebner
Cc: Levin Du, open list:ARM/Rockchip SoC..., Wayne Chou, devicetree,
Linus Walleij, linux-kernel@vger.kernel.org,
open list:GPIO SUBSYSTEM, Mark Rutland,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <3307687.qJF5Pr3uHG@phil>
On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko Stübner <heiko@sntech.de> wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@t-chip.com.cn> wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@t-chip.com.cn wrote:
>> >> >>> From: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du <djw@t-chip.com.cn>
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> Changes in v2:
>> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >> >>>
>> >> >>> Changes in v1:
>> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >> >>> - Add doc rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> .../bindings/gpio/rockchip,gpio-syscon.txt | 41
>> >> >>>
>> >> >>> ++++++++++++++++++++++
>> >> >>>
>> >> >>> drivers/gpio/gpio-syscon.c | 30
>> >> >>>
>> >> >>> ++++++++++++++++
>> >> >>>
>> >> >>> 2 files changed, 71 insertions(+)
>> >> >>> create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 0000000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> + the second cell is used to specify the gpio polarity:
>> >> >>> + 0 = Active high,
>> >> >>> + 1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > + grf: syscon at ff100000 {
>> >> > + compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.
But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?
> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8
Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any provider
properties (e.g. #gpio-cells). The only reason to do that is to make
Linux bind a driver, but DT is not a list of drivers to bind.
This is what I don't want to see:
syscon {
compatible = "foo,soc-sysctrl", "syscon", "simple-mfd";
reg = <...>;
clock-controller {
compatible = "foo,soc-sysctrl-clocks";
#clock-cells = <1>;
};
reset-controller {
compatible = "foo,soc-sysctrl-resets";
#reset-cells = <1>;
};
gpio {
compatible = "foo,soc-sysctrl-gpios";
#gpio-cells = <2>;
gpio-controller;
};
};
But rather:
syscon {
compatible = "foo,soc-sysctrl";
reg = <...>;
#clock-cells = <1>;
#reset-cells = <1>;
#gpio-cells = <2>;
gpio-controller;
};
> which also has a real phy-driver behind it and binding against that
> subnode of the GRF simple-mfd.
>
> These are real IP blocks somewhere on the socs, with regular supplies
> like resets, clocks etc in most cases. Only their controlling registers
> got dumped into the GRF for some reason.
I can tell that from your examples, but I can't tell that with this
binding. For this binding, it looks like you are adding a sub-node for
1 register bit. That wouldn't scale if you have 11 page register list.
> And in retrospect it really looks like we're doing something right,
> because it seems these bindings seem quite stable over time.
>
>
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>>
>> Is there really just one GPIO? If it has a defined function, then is
>> it really GP? Can you control direction? I know Linus W doesn't like
>> that kind of abuse of GPIO.
>
> looks like I convinced Linus that we're not abusing anything with this :-) .
Okay, but still my question remains: is it really only 1 GPIO?
Dropping "-mute" would be more future proof if not.
Rob
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: renesas: r8a7795: add ccree binding
From: Geert Uytterhoeven @ 2018-05-24 13:44 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, Herbert Xu, David S. Miller,
Ofir Drang, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, linux-clk
In-Reply-To: <CAOtvUMdCSoxYjpyb+fD36EE5+4sV4CjrxwK94RQ7bNKE5rajmQ@mail.gmail.com>
Hi Gilad,
On Thu, May 24, 2018 at 3:20 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> On Tue, May 22, 2018 at 10:48 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, May 21, 2018 at 3:43 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>> On Thu, May 17, 2018 at 1:16 PM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>> Indeed. From a quick glance, it looks like drivers/crypto/ccree/cc_driver.c
>>>> does not distinguish between the absence of the clock property, and an
>>>> actual error in getting the clock, and never considers any error a failure
>>>> (incl. -PROBE_DEFER).
>>>>
>>>> As of_clk_get() returns -ENOENT for both a missing clock property and a
>>>> missing clock, you should use (devm_)clk_get() instead, and distinguish
>>>> between NULL (no clock property) and IS_ERR() (actual failure -> abort).
>>>
>>> I was trying to do as you suggested but I didn't quite get what is the
>>> dev_id (2nd) parameter to devm_clk_get parameter is supposed to be.
>>
>> It's the (optional) name of the clock, helpful in case there is more than one.
>> In your case, NULL is fine.
>
> I have assumed as much and tried it, it did not work and so I assumed
> I am missing something and asked you.
> It turns out I was missing the fact I was using the wrong device tree
> file... :-(
>
> So thanks, it works now :-)
Glad to hear that!
> Having said that, while using devm)clk_get() is a better approach, it
> does not seems to distinguish
> between no "clocks" and a failure to clock information - it returns
> ENOENT in both cases as well.
Oh right, I guess I'm too used to not even getting that far due to the PM
Domain code failing to obtain the clock...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [v4 07/11] dt-bindings: hwmon: Add documents for PECI hwmon client drivers
From: Rob Herring @ 2018-05-24 13:47 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Mark Rutland, Haiyue Wang, Vernon Mauery, James Feist, devicetree,
linux-kernel@vger.kernel.org, Andrew Jeffery, Arnd Bergmann,
Jason M Biils, Joel Stanley
In-Reply-To: <8b56c103-fdab-fc98-f4d8-9bf435a9b59b@linux.intel.com>
On Wed, May 23, 2018 at 4:56 PM, Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
> On 5/23/2018 1:03 PM, Jae Hyun Yoo wrote:
>>
>> On 5/23/2018 12:33 PM, Rob Herring wrote:
>>>
>>> On Wed, May 23, 2018 at 11:37 AM, Jae Hyun Yoo
>>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>>
>>>> On 5/23/2018 8:11 AM, Rob Herring wrote:
>>>>>
>>>>>
>>>>> On Tue, May 22, 2018 at 12:18 PM, Jae Hyun Yoo
>>>>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 5/22/2018 9:42 AM, Rob Herring wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, May 21, 2018 at 12:59:05PM -0700, Jae Hyun Yoo wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This commit adds dt-bindings documents for PECI hwmon client
>>>>>>>> drivers.
>>>>>>>>
>>>>>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>>>>>> Reviewed-by: Haiyue Wang <haiyue.wang@linux.intel.com>
>>>>>>>> Reviewed-by: James Feist <james.feist@linux.intel.com>
>>>>>>>> Reviewed-by: Vernon Mauery <vernon.mauery@linux.intel.com>
>>>>>>>> Cc: Andrew Jeffery <andrew@aj.id.au>
>>>>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>>>>> Cc: Jason M Biils <jason.m.bills@linux.intel.com>
>>>>>>>> Cc: Joel Stanley <joel@jms.id.au>
>>>>>>>> ---
>>>>>>>> .../bindings/hwmon/peci-cputemp.txt | 23
>>>>>>>> ++++++++++++++++++
>>>>>>>> .../bindings/hwmon/peci-dimmtemp.txt | 24
>>>>>>>> +++++++++++++++++++
>>>>>>>> 2 files changed, 47 insertions(+)
>>>>>>>> create mode 100644
>>>>>>>> Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
>>>>>>>> create mode 100644
>>>>>>>> Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
>>>>>>>> b/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..2f59aee12d9e
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt
>>>>>>>> @@ -0,0 +1,23 @@
>>>>>>>> +Bindings for Intel PECI (Platform Environment Control Interface)
>>>>>>>> cputemp
>>>>>>>> driver.
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible : Should be "intel,peci-cputemp".
>>>>>>>> +- reg : Should contain address of a client CPU. Address
>>>>>>>> range
>>>>>>>> of
>>>>>>>> CPU
>>>>>>>> + clients is starting from 0x30 based on PECI
>>>>>>>> specification.
>>>>>>>> +
>>>>>>>> +Example:
>>>>>>>> + peci-bus@0 {
>>>>>>>> + #address-cells = <1>;
>>>>>>>> + #size-cells = <0>;
>>>>>>>> + < more properties >
>>>>>>>> +
>>>>>>>> + peci-cputemp@30 {
>>>>>>>> + compatible = "intel,peci-cputemp";
>>>>>>>> + reg = <0x30>;
>>>>>>>> + };
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> + peci-dimmtemp@30 {
>>>>>>>> + compatible = "intel,peci-dimmtemp";
>>>>>>>> + reg = <0x30>;
>>>>>>>> + };
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> As I said in the prior version, 2 nodes at the same address is wrong.
>>>>>>>
>>>>>>> Rob
>>>>>>>
>>>>>>
>>>>>> In PECI bus, there is one and only bus host (adapter) and multiple
>>>>>> clients on a PECI bus, and PECI spec doesn't allow multiple
>>>>>> originators
>>>>>> so only the host device can originate message.
>>>>>
>>>>>
>>>>>
>>>>> Yes, I get that. A single host still has to address slave devices.
>>>>>
>>>>>> In this implementation,
>>>>>> all message transactions on a bus from client driver modules and user
>>>>>> space will be serialized well in the PECI core bus driver so bus
>>>>>> occupation and traffic arbitration will be managed well in the PECI
>>>>>> core
>>>>>> bus driver even in case of a bus has 2 client drivers at the same
>>>>>> address. I'm sure that this implementation doesn't make that kind of
>>>>>> problem to OS.
>>>>>
>>>>>
>>>>>
>>>>> Multiple clients to a single device is common, but that is a software
>>>>> problem and doesn't belong in DT.
>>>>>
>>>>> I don't think there is a single other case in the kernel where
>>>>> multiple drivers can bind to the same device at a given bus address.
>>>>> That is why we have things like MFD. Though in this case, why can't
>>>>> one hwmon driver register multiple hwmon devices (cpu and dimm temps)?
>>>>>
>>>>
>>>> It was implemented as a single driver until v2 but dimm temps need
>>>> delayed creation unlikely the cpu temps on hwmon subsystem because of
>>>> memory training behavior of remote x86 cpus. Since hwmon doesn't allow
>>>> incremental creation, I had to divide it into two, cputemp and dimmtemp,
>>>> so that cputemp can be registered immediately when the remote x86 cpu
>>>> turns on and dimmtemp can be registered by delayed creation. It is the
>>>> reason why I had to make the two hwmon driver modules that sharing a
>>>> single device address.
>>>
>>>
>>> That all sounds like kernel problems to me. Stop designing your DT
>>> binding around what the kernel can or can't *currently* support.
>>>
>>>> Additionally, PECI isn't limited for temperature
>>>> monitoring feature but it can be used for other functions such as
>>>> platform management, cpu interface tuning and diagnostics and failure
>>>> analysis, so in case of adding a new driver for the functions, we should
>>>> add an another DT node which is sharing the same cpu address.
>>>
>>>
>>> No, the driver should add support for those additional functions.
>>> Perhaps you will need to use MFD.
>>>
>>
>> Do you mean that the device address sharing is acceptable if I make
>> these nodes under "simple-mfd"?
>>
>> Thanks,
>>
>> -Jae
>
>
> Hi Rob,
>
> I'm planning to change the whole PECI node like below:
>
> peci: peci@1e78b000 {
> compatible = "simple-bus";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges = <0x0 0x1e78b000 0x60>;
>
> peci0: peci-bus@0 {
> compatible = "aspeed,ast2500-peci";
> reg = <0x0 0x60>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = <15>;
> clocks = <&syscon ASPEED_CLK_GATE_REFCLK>;
> resets = <&syscon ASPEED_RESET_PECI>;
> clock-frequency = <24000000>;
> msg-timing = <1>;
> addr-timing = <1>;
> rd-sampling-point = <8>;
> cmd-timeout-ms = <1000>;
> status = "disabled";
>
> peci-client@30 {
> compatible = "simple-mfd", "syscon";
These compatibles alone is not correct. There should be a specific
compatible for the device.
Also, I don't think "syscon" even makes sense in this case.
> reg = <0x30>;
>
> cputemp: cputemp {
> compatible = "intel,peci-cputemp";
> };
There is no point in this node being in DT. It doesn't define any
resources. All it does is provide you a convenient way to bind your
driver, but that is not the purpose of DT. Put a specific compatible
in the parent and its driver can instantiate whatever child devices it
wants.
Rob
^ permalink raw reply
* Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805
From: Robin Murphy @ 2018-05-24 13:52 UTC (permalink / raw)
To: Ray Jui, Rob Herring
Cc: Wim Van Sebroeck, Guenter Roeck, Mark Rutland, Frank Rowand,
Catalin Marinas, Will Deacon, devicetree, linux-watchdog,
linux-kernel, bcm-kernel-feedback-list, linux-arm-kernel
In-Reply-To: <4b0ec4ab-2d9e-1e4b-e294-c175805f4013@broadcom.com>
On 23/05/18 20:29, Ray Jui wrote:
>
>
> On 5/23/2018 11:59 AM, Rob Herring wrote:
>> On Wed, May 23, 2018 at 09:25:49AM -0700, Ray Jui wrote:
>>>
>>>
>>> On 5/23/2018 3:57 AM, Robin Murphy wrote:
>>>> On 22/05/18 19:47, Ray Jui wrote:
>>>>> Update the SP805 binding document to add optional 'timeout-sec'
>>>>> devicetree property
>>>>>
>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>>>> ---
>>>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> index edc4f0e..f898a86 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
>>>>> @@ -19,6 +19,8 @@ Required properties:
>>>>> Optional properties:
>>>>> - interrupts : Should specify WDT interrupt number.
>>>>> +- timeout-sec : Should specify default WDT timeout in seconds. If
>>>>> unset, the
>>>>> + default timeout is 30 seconds
>>>>
>>>> According to the SP805 TRM, the default interval is dependent on the
>>>> rate of WDOGCLK, but would typically be a lot longer than that :/
>>>>
>>>> On a related note, anyone have any idea why we seem to have two subtly
>>>> different SP805 bindings defined?
>>
>> Sigh.
>>
>>> Interesting, I did not even know that until you pointed this out (and
>>> it's
>>> funny that I found that I actually reviewed arm,sp805.txt internally in
>>> Broadcom code review).
>>>
>>> It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the
>>> other
>>> was done by Anup Patel (arm,sp805.txt). Both were merged at the same
>>> time
>>> around March 20, 2016: 915c56bc01d6. I'd assume both were sent out at
>>> around
>>> the same time.
>>>
>>> It sounds like we should definitely remove one of them. Given that
>>> sp805-wdt.txt appears to have more detailed descriptions on the use
>>> of the
>>> clocks, should we remove arm,sp805.txt?
>>
>> Take whichever text you like, but I prefer filenames using the
>> compatible string and the correct string is 'arm,sp805' because '-wdt'
>> is redundant. You can probably safely just update all the dts files with
>> 'arm,sp805' and just remove 'arm,sp805-wdt' because it is not actually
>> used (as the ID registers are).
>
> Okay. I'll consolidate everything into arm,sp805.txt. Will also fix all
> DTS files to use "arm,sp805". The fix for actual DTS files will be in a
> different patch series.
Looking at the current in-tree DTs, for extra fun try to figure out
which binding each instance was following for the clocks. The most
common answer seems to be "neither"... :(
Robin.
^ permalink raw reply
* Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: Sudeep Holla @ 2018-05-24 14:01 UTC (permalink / raw)
To: ilialin, vireshk, nm, sboyd, robh, mark.rutland, rjw
Cc: Sudeep Holla, linux-pm, devicetree, linux-kernel
In-Reply-To: <000501d3f35f$96794910$c36bdb30$@codeaurora.org>
On 24/05/18 14:03, ilialin@codeaurora.org wrote:
>
>
[...]
>>> +
>>> + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
>>> + "qcom-cpufreq-kryo", -1, NULL, 0));
>>
>>
>> You simply can't do this unconditionally here. This will blow up on platforms
>> where this driver is not supposed to work. The probe will be called on non-
>> QCOM or non-Kryo QCOM platforms and I reckon it will crash trying to
>> execute something in qcom_smem_get.
>
> What do you mean by 'unconditionally'?
Why should you even add/register a device "qcom-cpufreq-kryo" on other
platforms. Drivers can get registered, but only devices that are present
or required by the platform need to be registered.
> The driver depends on the smem and nvmem drivers, which depend on ARCH_QCOM:
> + depends on QCOM_QFPROM
> + depends on QCOM_SMEM
>
Sure, but we have something called single image for all ARM64 platforms.
May be QCOM still used to tweeking config to build binary for your
particular mobile platform but the distro kernel need single binary to
work on all platforms. We have moved far away from platform specific
builds long back now IIRC.
> And if SMEM read in the probe returns something other than Kryo, it will exit.
>
Check what this driver does ?
msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
msm_id++;
switch ((enum _msm_id)*msm_id)
I think it *will and should* crash here ? You need to check the return
value for sure. But since qcom_smem_get return -EPROBE_DEFER, we keep
retrying even on non QCOM platforms which is something I would like to
avoid.
Therefore that's not the main concern. Why do I have to see
"qcom-cpufreq-kryo" device registered on my non QCOM platform ?
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH 4/9] regulator: bd71837: Devicetree bindings for BD71837 regulators
From: Mark Brown @ 2018-05-24 14:01 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
mazziesaccount, linux-clk, devicetree, linux-kernel,
mikko.mutanen, heikki.haikola
In-Reply-To: <20180524055752.GE4249@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 307 bytes --]
On Thu, May 24, 2018 at 08:57:52AM +0300, Matti Vaittinen wrote:
> +Required properties:
> + - compatible: should be "rohm,bd71837-pmic".
> + - regulator-name: should be "buck1", ..., "buck8" and "ldo1", ..., "ldo7"
The MFD is for a single device, there should be no need for compatibles
on subfunctions.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 9/9] regulator: bd71837: Build BD71837 regulator driver
From: Mark Brown @ 2018-05-24 14:01 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
mazziesaccount, linux-clk, devicetree, linux-kernel,
mikko.mutanen, heikki.haikola
In-Reply-To: <20180524060114.GJ4249@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 415 bytes --]
On Thu, May 24, 2018 at 09:01:14AM +0300, Matti Vaittinen wrote:
> Configurations and Makefile for BD71837 regulator driver
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> drivers/regulator/Kconfig | 11 +++++++++++
> drivers/regulator/Makefile | 1 +
> 2 files changed, 12 insertions(+)
Just squash this into the single patch adding the driver, it makes life
easier.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
From: ilialin @ 2018-05-24 14:10 UTC (permalink / raw)
To: 'Sudeep Holla'
Cc: linux-pm, devicetree, linux-kernel, vireshk, nm, sboyd, robh,
mark.rutland, rjw
In-Reply-To: <b4e221af-57f7-fd11-942d-6bc6f6b55a73@arm.com>
Thank you for the explanation. However, could you suggest, which condition should I check then? Device tree?
> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: Thursday, May 24, 2018 17:01
> To: ilialin@codeaurora.org; vireshk@kernel.org; nm@ti.com;
> sboyd@kernel.org; robh@kernel.org; mark.rutland@arm.com;
> rjw@rjwysocki.net
> Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-pm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver
>
>
>
> On 24/05/18 14:03, ilialin@codeaurora.org wrote:
> >
> >
>
> [...]
>
> >>> +
> >>> + ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
> >>> + "qcom-cpufreq-kryo", -1, NULL, 0));
> >>
> >>
> >> You simply can't do this unconditionally here. This will blow up on
> >> platforms where this driver is not supposed to work. The probe will
> >> be called on non- QCOM or non-Kryo QCOM platforms and I reckon it
> >> will crash trying to execute something in qcom_smem_get.
> >
> > What do you mean by 'unconditionally'?
>
> Why should you even add/register a device "qcom-cpufreq-kryo" on other
> platforms. Drivers can get registered, but only devices that are present or
> required by the platform need to be registered.
>
> > The driver depends on the smem and nvmem drivers, which depend on
> ARCH_QCOM:
> > + depends on QCOM_QFPROM
> > + depends on QCOM_SMEM
> >
>
> Sure, but we have something called single image for all ARM64 platforms.
> May be QCOM still used to tweeking config to build binary for your particular
> mobile platform but the distro kernel need single binary to work on all
> platforms. We have moved far away from platform specific builds long back
> now IIRC.
>
> > And if SMEM read in the probe returns something other than Kryo, it will
> exit.
> >
>
> Check what this driver does ?
>
> msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> MSM_ID_SMEM, &len);
> msm_id++;
> switch ((enum _msm_id)*msm_id)
>
> I think it *will and should* crash here ? You need to check the return value
> for sure. But since qcom_smem_get return -EPROBE_DEFER, we keep
> retrying even on non QCOM platforms which is something I would like to
> avoid.
>
> Therefore that's not the main concern. Why do I have to see "qcom-cpufreq-
> kryo" device registered on my non QCOM platform ?
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* Re: [PATCH 8/9] regulator: bd71837: BD71837 PMIC regulator driver
From: Mark Brown @ 2018-05-24 14:14 UTC (permalink / raw)
To: Matti Vaittinen
Cc: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
mazziesaccount, linux-clk, devicetree, linux-kernel,
mikko.mutanen, heikki.haikola
In-Reply-To: <20180524060036.GI4249@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]
On Thu, May 24, 2018 at 09:00:36AM +0300, Matti Vaittinen wrote:
> @@ -0,0 +1,683 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2018 ROHM Semiconductors */
> +/*
> + * bd71837-regulator.c ROHM BD71837MWV regulator driver
> + */
> +#include <linux/kernel.h>
Make the entire comment block a C++ comment so it looks more intentional
and add a blank line before the headers for legibility.
> +static int bd71837_regulator_set_regmap(struct regulator_dev *rdev, int set)
> +{
> + int ret = -EINVAL;
> + struct bd71837_pmic *pmic = rdev->reg_data;
> +
> + if (pmic) {
> + mutex_lock(&pmic->mtx);
> + if (!set)
> + ret = regulator_disable_regmap(rdev);
> + else
> + ret = regulator_enable_regmap(rdev);
> + mutex_unlock(&pmic->mtx);
> + }
This looks very weird - why might we not have a parent PMIC, what is the
lock doing and what is this wrapper function intended to do? Similar
issues apply to the voltage functions, if there's any need for this it
needs to be better documented but it really doesn't look like a good
idea.
> + err =
> + regmap_update_bits(pmic->mfd->regmap, BD71837_REG_REGLOCK,
> + (REGLOCK_PWRSEQ | REGLOCK_VREG), 0);
> + if (err) {
> + dev_err(&pmic->pdev->dev, "Failed to unlock PMIC (%d)\n", err);
> + goto err;
> + } else
> + dev_dbg(&pmic->pdev->dev, "%s: Unlocked lock register 0x%x\n",
> + __func__, BD71837_REG_REGLOCK);
There's loads of coding style problems with this code, please refer to
the coding style - indentation is weird and if there's { } on one side
of an else it should be on both.
> + rdev = regulator_register(desc, &config);
> + if (IS_ERR(rdev)) {
devm_regulator_regster()
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v2 0/5] crypto: ccree: cleanup, fixes and R-Car enabling
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, Herbert Xu, David S. Miller
Cc: Ofir Drang, linux-renesas-soc, devicetree, linux-arm-kernel,
linux-kernel, linux-clk, linux-crypto
The patch set enables the use of CryptoCell found in some Renesas R-Car
Salvator-X boards and fixes some driver issues uncovered that prevented
to work properly.
Changes from v1:
- Properly fix the bug that caused us to read a bad signature register
rather than dropping the check
- Proper DT fields as indicated by Geert Uytterhoeven.
- Better clock enabling as suggested by Geert Uytterhoeven.
Note! the last two patches in the set depend on the
"clk: renesas: r8a7795: Add CR clock" patch from Geert Uytterhoeven.
Gilad Ben-Yossef (5):
crypto: ccree: correct host regs offset
crypto: ccree: better clock handling
crypto: ccree: silence debug prints
clk: renesas: r8a7795: add ccree clock bindings
arm64: dts: renesas: r8a7795: add ccree binding
arch/arm64/boot/dts/renesas/r8a7795.dtsi | 9 +++++++++
drivers/clk/renesas/r8a7795-cpg-mssr.c | 1 +
drivers/crypto/ccree/cc_debugfs.c | 7 +++++--
drivers/crypto/ccree/cc_driver.c | 34 ++++++++++++++++++++++++++------
drivers/crypto/ccree/cc_driver.h | 2 ++
drivers/crypto/ccree/cc_host_regs.h | 6 ++++--
6 files changed, 49 insertions(+), 10 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH v2 1/5] crypto: ccree: correct host regs offset
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, Herbert Xu, David S. Miller
Cc: Ofir Drang, stable, linux-renesas-soc, devicetree,
linux-arm-kernel, linux-kernel, linux-clk, linux-crypto
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
The product signature and HW revision register have different offset on the
older HW revisions.
This fixes the problem of the driver failing sanity check on silicon
despite working on the FPGA emulation systems.
Fixes: 27b3b22dd98c ("crypto: ccree - add support for older HW revs")
Cc: stable@vger.kernel.org
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
drivers/crypto/ccree/cc_debugfs.c | 7 +++++--
drivers/crypto/ccree/cc_driver.c | 8 ++++++--
drivers/crypto/ccree/cc_driver.h | 2 ++
drivers/crypto/ccree/cc_host_regs.h | 6 ++++--
4 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/ccree/cc_debugfs.c b/drivers/crypto/ccree/cc_debugfs.c
index 08f8db4..5ca184e 100644
--- a/drivers/crypto/ccree/cc_debugfs.c
+++ b/drivers/crypto/ccree/cc_debugfs.c
@@ -26,7 +26,8 @@ struct cc_debugfs_ctx {
static struct dentry *cc_debugfs_dir;
static struct debugfs_reg32 debug_regs[] = {
- CC_DEBUG_REG(HOST_SIGNATURE),
+ { .name = "SIGNATURE" }, /* Must be 0th */
+ { .name = "VERSION" }, /* Must be 1st */
CC_DEBUG_REG(HOST_IRR),
CC_DEBUG_REG(HOST_POWER_DOWN_EN),
CC_DEBUG_REG(AXIM_MON_ERR),
@@ -34,7 +35,6 @@ static struct debugfs_reg32 debug_regs[] = {
CC_DEBUG_REG(HOST_IMR),
CC_DEBUG_REG(AXIM_CFG),
CC_DEBUG_REG(AXIM_CACHE_PARAMS),
- CC_DEBUG_REG(HOST_VERSION),
CC_DEBUG_REG(GPR_HOST),
CC_DEBUG_REG(AXIM_MON_COMP),
};
@@ -58,6 +58,9 @@ int cc_debugfs_init(struct cc_drvdata *drvdata)
struct debugfs_regset32 *regset;
struct dentry *file;
+ debug_regs[0].offset = drvdata->sig_offset;
+ debug_regs[1].offset = drvdata->ver_offset;
+
ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 89ce013..6f93ce7 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -207,9 +207,13 @@ static int init_cc_resources(struct platform_device *plat_dev)
if (hw_rev->rev >= CC_HW_REV_712) {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_712;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP);
+ new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_712);
+ new_drvdata->ver_offset = CC_REG(HOST_VERSION_712);
} else {
new_drvdata->hash_len_sz = HASH_LEN_SIZE_630;
new_drvdata->axim_mon_offset = CC_REG(AXIM_MON_COMP8);
+ new_drvdata->sig_offset = CC_REG(HOST_SIGNATURE_630);
+ new_drvdata->ver_offset = CC_REG(HOST_VERSION_630);
}
platform_set_drvdata(plat_dev, new_drvdata);
@@ -276,7 +280,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
}
/* Verify correct mapping */
- signature_val = cc_ioread(new_drvdata, CC_REG(HOST_SIGNATURE));
+ signature_val = cc_ioread(new_drvdata, new_drvdata->sig_offset);
if (signature_val != hw_rev->sig) {
dev_err(dev, "Invalid CC signature: SIGNATURE=0x%08X != expected=0x%08X\n",
signature_val, hw_rev->sig);
@@ -287,7 +291,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
/* Display HW versions */
dev_info(dev, "ARM CryptoCell %s Driver: HW version 0x%08X, Driver version %s\n",
- hw_rev->name, cc_ioread(new_drvdata, CC_REG(HOST_VERSION)),
+ hw_rev->name, cc_ioread(new_drvdata, new_drvdata->ver_offset),
DRV_MODULE_VERSION);
rc = init_cc_regs(new_drvdata, true);
diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
index 2048fde..95f82b2 100644
--- a/drivers/crypto/ccree/cc_driver.h
+++ b/drivers/crypto/ccree/cc_driver.h
@@ -129,6 +129,8 @@ struct cc_drvdata {
enum cc_hw_rev hw_rev;
u32 hash_len_sz;
u32 axim_mon_offset;
+ u32 sig_offset;
+ u32 ver_offset;
};
struct cc_crypto_alg {
diff --git a/drivers/crypto/ccree/cc_host_regs.h b/drivers/crypto/ccree/cc_host_regs.h
index f510018..616b2e1 100644
--- a/drivers/crypto/ccree/cc_host_regs.h
+++ b/drivers/crypto/ccree/cc_host_regs.h
@@ -45,7 +45,8 @@
#define CC_HOST_ICR_DSCRPTR_WATERMARK_QUEUE0_CLEAR_BIT_SIZE 0x1UL
#define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SHIFT 0x17UL
#define CC_HOST_ICR_AXIM_COMP_INT_CLEAR_BIT_SIZE 0x1UL
-#define CC_HOST_SIGNATURE_REG_OFFSET 0xA24UL
+#define CC_HOST_SIGNATURE_712_REG_OFFSET 0xA24UL
+#define CC_HOST_SIGNATURE_630_REG_OFFSET 0xAC8UL
#define CC_HOST_SIGNATURE_VALUE_BIT_SHIFT 0x0UL
#define CC_HOST_SIGNATURE_VALUE_BIT_SIZE 0x20UL
#define CC_HOST_BOOT_REG_OFFSET 0xA28UL
@@ -105,7 +106,8 @@
#define CC_HOST_BOOT_ONLY_ENCRYPT_LOCAL_BIT_SIZE 0x1UL
#define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SHIFT 0x1EUL
#define CC_HOST_BOOT_AES_EXISTS_LOCAL_BIT_SIZE 0x1UL
-#define CC_HOST_VERSION_REG_OFFSET 0xA40UL
+#define CC_HOST_VERSION_712_REG_OFFSET 0xA40UL
+#define CC_HOST_VERSION_630_REG_OFFSET 0xAD8UL
#define CC_HOST_VERSION_VALUE_BIT_SHIFT 0x0UL
#define CC_HOST_VERSION_VALUE_BIT_SIZE 0x20UL
#define CC_HOST_KFDE0_VALID_REG_OFFSET 0xA60UL
--
2.7.4
^ permalink raw reply related
* [PATCH v2 2/5] crypto: ccree: better clock handling
From: Gilad Ben-Yossef @ 2018-05-24 14:19 UTC (permalink / raw)
To: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, Herbert Xu, David S. Miller
Cc: Ofir Drang, linux-renesas-soc, devicetree, linux-arm-kernel,
linux-kernel, linux-clk, linux-crypto
In-Reply-To: <1527171551-21979-1-git-send-email-gilad@benyossef.com>
Use managed clock handling, differentiate between no clock (possibly OK)
and clock init failure (never OK) and correctly handle clock detection
being deferred.
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
drivers/crypto/ccree/cc_driver.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 6f93ce7..b266657 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -190,6 +190,7 @@ static int init_cc_resources(struct platform_device *plat_dev)
u64 dma_mask;
const struct cc_hw_data *hw_rev;
const struct of_device_id *dev_id;
+ struct clk *clk;
int rc = 0;
new_drvdata = devm_kzalloc(dev, sizeof(*new_drvdata), GFP_KERNEL);
@@ -219,7 +220,24 @@ static int init_cc_resources(struct platform_device *plat_dev)
platform_set_drvdata(plat_dev, new_drvdata);
new_drvdata->plat_dev = plat_dev;
- new_drvdata->clk = of_clk_get(np, 0);
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk))
+ switch (PTR_ERR(clk)) {
+ /* Clock is optional so this might be fine */
+ case -ENOENT:
+ break;
+
+ /* Clock not available, let's try again soon */
+ case -EPROBE_DEFER:
+ return -EPROBE_DEFER;
+
+ default:
+ dev_err(dev, "Error getting clock: %ld\n",
+ PTR_ERR(clk));
+ return PTR_ERR(clk);
+ }
+ new_drvdata->clk = clk;
+
new_drvdata->coherent = of_dma_is_coherent(np);
/* Get device resources */
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox