* Re: [PATCH] dt-bindings: reset: Convert i.MX reset to json-schema
From: Philipp Zabel @ 2020-05-18 9:41 UTC (permalink / raw)
To: Anson Huang, robh+dt, shawnguo, s.hauer, kernel, festevam,
devicetree, linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1589250194-29441-1-git-send-email-Anson.Huang@nxp.com>
Hi Anson,
On Tue, 2020-05-12 at 10:23 +0800, Anson Huang wrote:
> Convert the i.MX reset binding to DT schema format using json-schema.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
thank you for the conversion.
> ---
> .../devicetree/bindings/reset/fsl,imx-src.txt | 49 ------------------
> .../devicetree/bindings/reset/fsl,imx-src.yaml | 58 ++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 49 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
>
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> deleted file mode 100644
> index 6ed79e6..0000000
> --- a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -Freescale i.MX System Reset Controller
> -======================================
> -
> -Please also refer to reset.txt in this directory for common reset
> -controller binding usage.
> -
> -Required properties:
> -- compatible: Should be "fsl,<chip>-src"
> -- reg: should be register base and length as documented in the
> - datasheet
> -- interrupts: Should contain SRC interrupt and CPU WDOG interrupt,
> - in this order.
> -- #reset-cells: 1, see below
> -
> -example:
> -
> -src: src@20d8000 {
> - compatible = "fsl,imx6q-src";
> - reg = <0x020d8000 0x4000>;
> - interrupts = <0 91 0x04 0 96 0x04>;
> - #reset-cells = <1>;
> -};
> -
> -Specifying reset lines connected to IP modules
> -==============================================
> -
> -The system reset controller can be used to reset the GPU, VPU,
> -IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
> -nodes should specify the reset line on the SRC in their resets
> -property, containing a phandle to the SRC device node and a
> -RESET_INDEX specifying which module to reset, as described in
> -reset.txt
> -
> -example:
> -
> - ipu1: ipu@2400000 {
> - resets = <&src 2>;
> - };
> - ipu2: ipu@2800000 {
> - resets = <&src 4>;
> - };
> -
> -The following RESET_INDEX values are valid for i.MX5:
> -GPU_RESET 0
> -VPU_RESET 1
> -IPU1_RESET 2
> -OPEN_VG_RESET 3
> -The following additional RESET_INDEX value is valid for i.MX6:
> -IPU2_RESET 4
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> new file mode 100644
> index 0000000..276a533
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/fsl,imx-src.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX System Reset Controller
> +
> +maintainers:
> + - Philipp Zabel <p.zabel@pengutronix.de>
> +
> +description: |
> + The system reset controller can be used to reset the GPU, VPU,
> + IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
> + nodes should specify the reset line on the SRC in their resets
> + property, containing a phandle to the SRC device node and a
> + RESET_INDEX specifying which module to reset, as described in
> + reset.txt
> +
> + The following RESET_INDEX values are valid for i.MX5:
> + GPU_RESET 0
> + VPU_RESET 1
> + IPU1_RESET 2
> + OPEN_VG_RESET 3
> + The following additional RESET_INDEX value is valid for i.MX6:
> + IPU2_RESET 4
> +
> +properties:
> + compatible:
> + items:
> + - const: "fsl,imx51-src"
"fsl,imx51-src" is the only compatible the driver matches on, but we
have these combinations on compatible SRCs in the device trees:
"fsl,imx50-src", "fsl,imx51-src"
"fsl,imx51-src"
"fsl,imx53-src", "fsl,imx51-src"
"fsl,imx6q-src", "fsl,imx51-src"
"fsl,imx6sl-src", "fsl,imx51-src"
"fsl,imx6sll-src", "fsl,imx51-src"
"fsl,imx6sx-src", "fsl,imx51-src"
"fsl,imx6ul-src", "fsl,imx51-src"
That could be described using oneOf and and an items list of const
values per SoC like in the qcom bindings.
regards
Philipp
^ permalink raw reply
* Re: [PATCH 03/17] ARM: dts: r8a7742: Add I2C and IIC support
From: Lad, Prabhakar @ 2020-05-18 9:43 UTC (permalink / raw)
To: Wolfram Sang, Geert Uytterhoeven
Cc: Lad Prabhakar, Jens Axboe, Rob Herring, Ulf Hansson,
Sergei Shtylyov, David S. Miller, Wim Van Sebroeck, Guenter Roeck,
linux-ide,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
linux-i2c, Linux MMC List, netdev, Linux-Renesas, linux-watchdog
In-Reply-To: <20200518092601.GA3268@ninjato>
Hi Wolfram,
On Mon, May 18, 2020 at 10:26 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > > However, both versions (with and without automatic transmission) are
> > > described with the same "renesas,iic-r8a7742" compatible. Is it possible
> > > to detect the reduced variant at runtime somehow?
> > >
> > I couldn't find anything the manual that would be useful to detect at runtime.
> >
> > > My concern is that the peculiarity of this SoC might be forgotten if we
> > > describe it like this and ever add "automatic transmissions" somewhen.
> > >
> > Agreed.
>
> Well, I guess reading from a register which is supposed to not be there
> on the modified IP core is too hackish.
>
> Leaves us with a seperate compatible entry for it?
>
Sounds okay to me, how about "renesas,iic-no-dvfs" ? So that this
could be used on all the SoC's which don't support DVFS.
Cheers,
--Prabhakar
^ permalink raw reply
* Re: [PATCH v1 8/9] arm64: dts: actions: Add MMC controller support for S700
From: Andreas Färber @ 2020-05-18 9:48 UTC (permalink / raw)
To: André Przywara, Manivannan Sadhasivam, Amit Tomer
Cc: Rob Herring, cristian.ciocaltea, linux-arm-kernel, linux-actions,
devicetree
In-Reply-To: <29500523-0fe4-955d-57de-d4b9f11cedb8@arm.com>
Hi,
Am 18.05.20 um 10:29 schrieb André Przywara:
> On 18/05/2020 07:17, Manivannan Sadhasivam wrote:
>> I don't know if the DT fallback is discouraged or not.
>
> I don't know if there is an "official" statement on this, but last thing
> I heard, adding SoC specific compatibles to generic fallback strings was
> encouraged. Hence my proposal, to add one.
I believe the official guidance would be to never be too generic in the
first place. I.e., prefer s500 (oldest model tested) over generic owl.
But now that we have it, prepending a more specific one (rather than
replacing it) is the only way to go. In that case the binding needs to
be changed to allow both the old and the recommended new variant, as
André points out. Please add a comment to help bindings readers choose
which of the two to adopt then.
Amit, next time please quote errors you see right away, that could've
spared a handful of messages discussing about the driver when it was in
fact just a bindings issue (which you had been asked to fix by André).
Regards,
Andreas
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v2 4/7] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers
From: Srinivas Kandagatla @ 2020-05-18 9:49 UTC (permalink / raw)
To: Ajit Pandey, broonie, plai, bgoswami; +Cc: alsa-devel, devicetree, linux-kernel
On 14/05/2020 17:38, Ajit Pandey wrote:
> I2SCTL and DMACTL registers has different bits alignment for newer
> LPASS variants of SC7180 soc. Instead of adding extra overhead for
> calculating masks and shifts for newer variants registers layout we
> changed the approach to use regmap_field_write() API to update bit.
> Such API's will internally do the required bit shift and mask based
> on reg_field struct defined for bit fields. We'll define REG_FIELD()
> macros with bit layout for both lpass variants and use such macros
> to initialize register fields in variant specific driver callbacks.
> Also added new bitfieds values for I2SCTL and DMACTL registers and
> removed shifts and mask macros for such registers from header file.
>
> Signed-off-by: Ajit Pandey <ajitp@codeaurora.org>
> ---
> sound/soc/qcom/lpass-apq8016.c | 61 ++++++++++++
> sound/soc/qcom/lpass-cpu.c | 114 +++++++++++++---------
> sound/soc/qcom/lpass-lpaif-reg.h | 203 ++++++++++++++++++++++++---------------
> sound/soc/qcom/lpass-platform.c | 86 +++++++++++------
> sound/soc/qcom/lpass.h | 30 ++++++
> 5 files changed, 340 insertions(+), 154 deletions(-)
>
Thanks for moving this to regmap fields, looks clean!
However this patch just removed support to lpass-ipq806x.c variant,
which should to be taken care of while doing patches that apply to all
variants.
> diff --git a/sound/soc/qcom/lpass-apq8016.c b/sound/soc/qcom/lpass-apq8016.c
> index 8210e37..3149645 100644
> --- a/sound/soc/qcom/lpass-apq8016.c
> +++ b/sound/soc/qcom/lpass-apq8016.c
> @@ -124,6 +124,32 @@
> },
> };
>
> +static int apq8016_init_dmactl_bitfields(struct lpaif_dmactl *dmactl,
> + struct regmap *map,
> + unsigned int reg)
> +{
> + struct reg_field bursten = DMACTL_BURSTEN_FLD(reg);
> + struct reg_field wpscnt = DMACTL_WPSCNT_FLD(reg);
> + struct reg_field fifowm = DMACTL_FIFOWM_FLD(reg);
> + struct reg_field intf = DMACTL_AUDINTF_FLD(reg);
> + struct reg_field enable = DMACTL_ENABLE_FLD(reg);
> + struct reg_field dyncclk = DMACTL_DYNCLK_FLD(reg);
> +
> + dmactl->bursten = regmap_field_alloc(map, bursten);
> + dmactl->wpscnt = regmap_field_alloc(map, wpscnt);
> + dmactl->fifowm = regmap_field_alloc(map, fifowm);
> + dmactl->intf = regmap_field_alloc(map, intf);
> + dmactl->enable = regmap_field_alloc(map, enable);
> + dmactl->dyncclk = regmap_field_alloc(map, dyncclk);
My idea was to move this all regmap fields to variant structure and
common code will do the regmap_filed_alloc rather than each variant
duplicating the same code for each variant, also am guessing some of the
members in the lpass_variant structure tp become redundant due to regmap
field which can be removed as well.
ex :
struct lpass_variant {
...
struct reg_field bursten
...
};
in lpass-apq8016.c
we do
static struct lpass_variant apq8016_data = {
.bursten = REG_FIELD(reg, 11, 11),
...
}
in lpass-cpu.c we can do the real regmap_field_alloc
asoc_qcom_lpass_cpu_platform_probe
> +
> + if (IS_ERR(dmactl->bursten) || IS_ERR(dmactl->wpscnt) ||
> + IS_ERR(dmactl->fifowm) || IS_ERR(dmactl->intf) ||
> + IS_ERR(dmactl->enable) || IS_ERR(dmactl->dyncclk))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int apq8016_lpass_alloc_dma_channel(struct lpass_data *drvdata,
> int direction)
> {
> @@ -158,6 +184,39 @@ static int apq8016_lpass_free_dma_channel(struct lpass_data *drvdata, int chan)
> return 0;
> }
>
> +static int sc7180_init_i2sctl_bitfields(struct lpaif_i2sctl *i2sctl,
> + struct regmap *map, unsigned int reg)
> +{
Should this be apq8016_init_i2sctl_bitfields
Please make sure that you compile the code before sending it out!
--srini
>
^ permalink raw reply
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Rafael J. Wysocki @ 2020-05-18 9:53 UTC (permalink / raw)
To: Viresh Kumar, Serge Semin
Cc: Serge Semin, Thomas Bogendoerfer, Rafael J. Wysocki, Ulf Hansson,
Matthias Kaehlcke, Alexey Malahov, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree, stable,
Frederic Weisbecker, Ingo Molnar, Yue Hu, linux-pm, linux-kernel
In-Reply-To: <20200518074142.c6kbofpdlxro2pjz@vireshk-i7>
On 5/18/2020 9:41 AM, Viresh Kumar wrote:
> On 16-05-20, 15:52, Serge Semin wrote:
>> On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote:
>>>> @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state)
>>>> break;
>>>> }
>>>> - return ret;
>>>> + return ret < 0 ? ret : 0;
>>>> }
>>>> int cpufreq_boost_trigger_state(int state)
>>> IMO it is better to update the caller of this function to handle the
>>> positive value possibly returned by it correctly.
>> Could you elaborate why? Viresh seems to be ok with this solution.
> And it is absolutely fine for Rafael to not agree with it :)
>
>> As I see it the caller doesn't expect the positive value returned by the
>> original freq_qos_update_request(). It just doesn't need to know whether the
>> effective policy has been updated or not, it only needs to make sure the
>> operations has been successful. Moreover the positive value is related only
>> to the !last! active policy, which doesn't give the caller a full picture
>> of the policy change anyway. So taking all of these into account I'd leave the
>> fix as is.
> Rafael: This function is called via a function pointer, which can call
> this or a platform dependent routine (like in acpi-cpufreq.c), and it
> would be reasonable IMO for the return of that callback to only look
> for 0 or negative values, as is generally done in the kernel.
But it only has one caller that can easily check ret < 0 instead of just
ret, so the extra branch can be saved.
That said if you really only want it to return 0 on success, you may as
well add a ret = 0; statement (with a comment explaining why it is
needed) after the last break in the loop.
Cheers!
^ permalink raw reply
* Re: [PATCH 03/17] ARM: dts: r8a7742: Add I2C and IIC support
From: Wolfram Sang @ 2020-05-18 9:53 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Geert Uytterhoeven, Lad Prabhakar, Jens Axboe, Rob Herring,
Ulf Hansson, Sergei Shtylyov, David S. Miller, Wim Van Sebroeck,
Guenter Roeck, linux-ide,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
linux-i2c, Linux MMC List, netdev, Linux-Renesas, linux-watchdog
In-Reply-To: <CA+V-a8sTm8YEP2Upu1t6tb6YMpaANFRnnLVW=1TXP2LpVMvrNw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 513 bytes --]
Hi Prabhakar,
> > Leaves us with a seperate compatible entry for it?
> >
> Sounds okay to me, how about "renesas,iic-no-dvfs" ? So that this
> could be used on all the SoC's which don't support DVFS.
Well, the feature missing is used for DVFS, but its name is "automatic
transmission". So, I'd rather suggest "-no-auto" as suffix. Also, there
are already quite some IIC variants out there, so plain "iic" won't
catch them all. My suggestion would be "renesas,rcar-gen2-iic-no-auto".
All the best,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 01/17] dt-bindings: i2c: renesas,i2c: Document r8a7742 support
From: Geert Uytterhoeven @ 2020-05-18 9:56 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Jens Axboe, Rob Herring, Wolfram Sang, Ulf Hansson,
Sergei Shtylyov, David S. Miller, Wim Van Sebroeck, Guenter Roeck,
linux-ide,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Linux I2C, Linux MMC List, netdev,
Linux-Renesas, Linux Watchdog Mailing List, Prabhakar
In-Reply-To: <1589555337-5498-2-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Fri, May 15, 2020 at 5:09 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Document i2c controller for RZ/G1H (R8A7742) SoC, which is compatible
> with R-Car Gen2 SoC family.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
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: [PATCH] dt-bindings: reset: Convert i.MX reset to json-schema
From: Anson Huang @ 2020-05-18 9:57 UTC (permalink / raw)
To: Aisheng Dong, p.zabel@pengutronix.de, robh+dt@kernel.org,
shawnguo@kernel.org, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <AM6PR04MB496643088C27DB303166ED0D80B80@AM6PR04MB4966.eurprd04.prod.outlook.com>
> Subject: RE: [PATCH] dt-bindings: reset: Convert i.MX reset to json-schema
>
> > From: Anson Huang <Anson.Huang@nxp.com>
> > Sent: Tuesday, May 12, 2020 10:23 AM
> >
> > Convert the i.MX reset binding to DT schema format using json-schema.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > .../devicetree/bindings/reset/fsl,imx-src.txt | 49 ------------------
> > .../devicetree/bindings/reset/fsl,imx-src.yaml | 58
> > ++++++++++++++++++++++
> > 2 files changed, 58 insertions(+), 49 deletions(-) delete mode
> > 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > create mode 100644
> > Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > deleted file mode 100644
> > index 6ed79e6..0000000
> > --- a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > +++ /dev/null
> > @@ -1,49 +0,0 @@
> > -Freescale i.MX System Reset Controller
> > -======================================
> > -
> > -Please also refer to reset.txt in this directory for common reset
> > -controller binding usage.
> > -
> > -Required properties:
> > -- compatible: Should be "fsl,<chip>-src"
> > -- reg: should be register base and length as documented in the
> > - datasheet
> > -- interrupts: Should contain SRC interrupt and CPU WDOG interrupt,
> > - in this order.
> > -- #reset-cells: 1, see below
> > -
> > -example:
> > -
> > -src: src@20d8000 {
> > - compatible = "fsl,imx6q-src";
> > - reg = <0x020d8000 0x4000>;
> > - interrupts = <0 91 0x04 0 96 0x04>;
> > - #reset-cells = <1>;
> > -};
> > -
> > -Specifying reset lines connected to IP modules
> > -==============================================
> > -
> > -The system reset controller can be used to reset the GPU, VPU, -IPU,
> > and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device -nodes
> > should specify the reset line on the SRC in their resets -property,
> > containing a phandle to the SRC device node and a -RESET_INDEX
> > specifying which module to reset, as described in -reset.txt
> > -
> > -example:
> > -
> > - ipu1: ipu@2400000 {
> > - resets = <&src 2>;
> > - };
> > - ipu2: ipu@2800000 {
> > - resets = <&src 4>;
> > - };
> > -
> > -The following RESET_INDEX values are valid for i.MX5:
> > -GPU_RESET 0
> > -VPU_RESET 1
> > -IPU1_RESET 2
> > -OPEN_VG_RESET 3
> > -The following additional RESET_INDEX value is valid for i.MX6:
> > -IPU2_RESET 4
> > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> > b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> > new file mode 100644
> > index 0000000..276a533
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +title: Freescale i.MX System Reset Controller
> > +
> > +maintainers:
> > + - Philipp Zabel <p.zabel@pengutronix.de>
> > +
> > +description: |
> > + The system reset controller can be used to reset the GPU, VPU,
> > + IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
> > + nodes should specify the reset line on the SRC in their resets
> > + property, containing a phandle to the SRC device node and a
> > + RESET_INDEX specifying which module to reset, as described in
> > + reset.txt
> > +
> > + The following RESET_INDEX values are valid for i.MX5:
> > + GPU_RESET 0
> > + VPU_RESET 1
> > + IPU1_RESET 2
> > + OPEN_VG_RESET 3
> > + The following additional RESET_INDEX value is valid for i.MX6:
> > + IPU2_RESET 4
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - const: "fsl,imx51-src"
>
> What about mx6?
Will add all i.MX6 compatible according to DT.
>
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
>
> Need description here
OK.
>
> > + minItems: 1
> > + maxItems: 2
> > +
> > + '#reset-cells':
> > + const: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - '#reset-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + src@73fd0000 {
>
> s/src/reset-controller
OK.
Anson
^ permalink raw reply
* Re: [PATCH 3/3] hwmon: sparx5: Add Sparx5 SoC temperature driver
From: Lars Povlsen @ 2020-05-18 9:57 UTC (permalink / raw)
To: Guenter Roeck
Cc: Lars Povlsen, SoC Team, Jean Delvare,
Microchip Linux Driver Support, linux-hwmon, devicetree,
linux-arm-kernel, linux-kernel, Alexandre Belloni
In-Reply-To: <20200513152018.GA60367@roeck-us.net>
Guenter Roeck writes:
> On Wed, May 13, 2020 at 03:41:40PM +0200, Lars Povlsen wrote:
>> This patch adds a temperature sensor driver to the Sparx5 SoC.
>>
>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>> ---
>> drivers/hwmon/Kconfig | 10 +++
>> drivers/hwmon/Makefile | 2 +-
>> drivers/hwmon/sparx5-temp.c | 154 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 165 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/hwmon/sparx5-temp.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 4c62f900bf7e8..130cb1f1748ff 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -480,6 +480,16 @@ config SENSORS_I5K_AMB
>> This driver can also be built as a module. If so, the module
>> will be called i5k_amb.
>>
>> +config SENSORS_SPARX5
>> + tristate "Sparx5 SoC temperature sensor"
>> + depends on ARCH_SPARX5
>> + help
>> + If you say yes here you get support for temperature monitoring
>> + with the Microchip Sparx5 SoC.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called sparx5-temp.
>> +
>> config SENSORS_F71805F
>> tristate "Fintek F71805F/FG, F71806F/FG and F71872F/FG"
>> depends on !PPC
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index b0b9c8e571762..28a09986b7a62 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_DS1621) += ds1621.o
>> obj-$(CONFIG_SENSORS_EMC1403) += emc1403.o
>> obj-$(CONFIG_SENSORS_EMC2103) += emc2103.o
>> obj-$(CONFIG_SENSORS_EMC6W201) += emc6w201.o
>> +obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
>> obj-$(CONFIG_SENSORS_F71805F) += f71805f.o
>> obj-$(CONFIG_SENSORS_F71882FG) += f71882fg.o
>> obj-$(CONFIG_SENSORS_F75375S) += f75375s.o
>> @@ -190,4 +191,3 @@ obj-$(CONFIG_SENSORS_OCC) += occ/
>> obj-$(CONFIG_PMBUS) += pmbus/
>>
>> ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>> -
>> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
>> new file mode 100644
>> index 0000000000000..bf9dd102a9825
>> --- /dev/null
>> +++ b/drivers/hwmon/sparx5-temp.c
>> @@ -0,0 +1,154 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/* Sparx5 SoC temperature sensor driver
>> + *
>> + * Copyright (C) 2020 Lars Povlsen <lars.povlsen@microchip.com>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include
Ack.
>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +#define TEMP_CTRL 0
>> +#define TEMP_CFG 4
>> +#define TEMP_CFG_CYCLES GENMASK(24, 15)
>> +#define TEMP_CFG_CYCLES_OFF 15
>> +#define TEMP_CFG_ENA BIT(0)
>> +#define TEMP_STAT 8
>> +#define TEMP_STAT_VALID BIT(12)
>> +#define TEMP_STAT_TEMP GENMASK(11, 0)
>> +
>> +struct s5_hwmon {
>> + void __iomem *base;
>> +};
>> +
>> +static void s5_temp_enable(struct s5_hwmon *hwmon)
>> +{
>> + u32 val = readl(hwmon->base + TEMP_CFG);
>> + u32 clk = 250;
>> +
>> + val &= ~TEMP_CFG_CYCLES;
>> + val |= (clk << TEMP_CFG_CYCLES_OFF);
>> + val |= TEMP_CFG_ENA;
>> +
>> + writel(val, hwmon->base + TEMP_CFG);
>> +}
>> +
>> +static void s5_temp_disable(void *data)
>> +{
>> + struct s5_hwmon *hwmon = data;
>> + u32 val = readl(hwmon->base + TEMP_CFG);
>> +
>> + val &= ~TEMP_CFG_ENA;
>> +
>> + writel(val, hwmon->base + TEMP_CFG);
>> +}
>> +
>> +static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *temp)
>> +{
>> + struct s5_hwmon *hwmon = dev_get_drvdata(dev);
>> + int rc = 0, value;
>> + u32 stat;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + stat = readl_relaxed(hwmon->base + TEMP_STAT);
>> + if (stat & TEMP_STAT_VALID) {
>> + value = (stat & TEMP_STAT_TEMP);
>
> Unnecessary ( )
Removed.
>
>> + value = DIV_ROUND_CLOSEST(value * 3522, 4096) - 1094;
>> + value *= 100;
>> + *temp = value;
>> + } else
>> + rc = -EINVAL;
>
> -EINVAL is for bad used input. -EIO, maybe, unless there is a better error.
> Also,
>
> if (!(stat & TEMP_STAT_VALID))
> return -EIO;
Ok, changed.
> ...
>
> would be easier to read (and not result in a checkpatch warning).
>
>> + break;
>> + default:
>> + rc = -EOPNOTSUPP;
>
> break;
>
Added.
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +static umode_t s5_is_visible(const void *_data, enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + if (type != hwmon_temp)
>> + return 0;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + return 0444;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static const struct hwmon_channel_info *s5_info[] = {
>> + HWMON_CHANNEL_INFO(chip,
>> + HWMON_C_REGISTER_TZ),
>> + HWMON_CHANNEL_INFO(temp,
>> + HWMON_T_INPUT),
>> + NULL
>> +};
>> +
>> +static const struct hwmon_ops s5_hwmon_ops = {
>> + .is_visible = s5_is_visible,
>> + .read = s5_read,
>> +};
>> +
>> +static const struct hwmon_chip_info s5_chip_info = {
>> + .ops = &s5_hwmon_ops,
>> + .info = s5_info,
>> +};
>> +
>> +static int s5_temp_probe(struct platform_device *pdev)
>> +{
>> + struct device *hwmon_dev;
>> + struct s5_hwmon *hwmon;
>> + int err = 0;
>
> Unnecessary initialization
>
Removed
>> +
>> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
>> + if (!hwmon)
>> + return -ENOMEM;
>> +
>> + hwmon->base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(hwmon->base))
>> + return PTR_ERR(hwmon->base);
>> +
>> + err = devm_add_action(&pdev->dev, s5_temp_disable, hwmon);
>> + if (err)
>> + return err;
>> +
>> + s5_temp_enable(hwmon);
>> +
>> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
>> + "s5_temp",
>> + hwmon,
>> + &s5_chip_info,
>> + NULL);
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +const struct of_device_id s5_temp_match[] = {
>> + { .compatible = "microchip,sparx5-temp" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, s5_temp_match);
>> +
>> +static struct platform_driver s5_temp_driver = {
>> + .probe = s5_temp_probe,
>> + .driver = {
>> + .name = "sparx5-temp",
>> + .of_match_table = s5_temp_match,
>> + },
>> +};
>> +
>> +module_platform_driver(s5_temp_driver);
>> +
>> +MODULE_AUTHOR("Lars Povlsen <lars.povlsen@microchip.com>");
>> +MODULE_DESCRIPTION("Sparx5 SoC temperature sensor driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.26.2
Thank you for your input, I will submit anew asap.
---Lars
--
Lars Povlsen,
Microchip
^ permalink raw reply
* RE: [PATCH] dt-bindings: reset: Convert i.MX reset to json-schema
From: Anson Huang @ 2020-05-18 9:58 UTC (permalink / raw)
To: Philipp Zabel, robh+dt@kernel.org, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <719ed778a36e7edfea080d42e9b837432e595d15.camel@pengutronix.de>
Hi, Philipp
> Subject: Re: [PATCH] dt-bindings: reset: Convert i.MX reset to json-schema
>
> Hi Anson,
>
> On Tue, 2020-05-12 at 10:23 +0800, Anson Huang wrote:
> > Convert the i.MX reset binding to DT schema format using json-schema.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>
> thank you for the conversion.
>
> > ---
> > .../devicetree/bindings/reset/fsl,imx-src.txt | 49 ------------------
> > .../devicetree/bindings/reset/fsl,imx-src.yaml | 58
> ++++++++++++++++++++++
> > 2 files changed, 58 insertions(+), 49 deletions(-) delete mode
> > 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > create mode 100644
> > Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > deleted file mode 100644
> > index 6ed79e6..0000000
> > --- a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
> > +++ /dev/null
> > @@ -1,49 +0,0 @@
> > -Freescale i.MX System Reset Controller
> > -======================================
> > -
> > -Please also refer to reset.txt in this directory for common reset
> > -controller binding usage.
> > -
> > -Required properties:
> > -- compatible: Should be "fsl,<chip>-src"
> > -- reg: should be register base and length as documented in the
> > - datasheet
> > -- interrupts: Should contain SRC interrupt and CPU WDOG interrupt,
> > - in this order.
> > -- #reset-cells: 1, see below
> > -
> > -example:
> > -
> > -src: src@20d8000 {
> > - compatible = "fsl,imx6q-src";
> > - reg = <0x020d8000 0x4000>;
> > - interrupts = <0 91 0x04 0 96 0x04>;
> > - #reset-cells = <1>;
> > -};
> > -
> > -Specifying reset lines connected to IP modules
> > -==============================================
> > -
> > -The system reset controller can be used to reset the GPU, VPU, -IPU,
> > and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device -nodes
> > should specify the reset line on the SRC in their resets -property,
> > containing a phandle to the SRC device node and a -RESET_INDEX
> > specifying which module to reset, as described in -reset.txt
> > -
> > -example:
> > -
> > - ipu1: ipu@2400000 {
> > - resets = <&src 2>;
> > - };
> > - ipu2: ipu@2800000 {
> > - resets = <&src 4>;
> > - };
> > -
> > -The following RESET_INDEX values are valid for i.MX5:
> > -GPU_RESET 0
> > -VPU_RESET 1
> > -IPU1_RESET 2
> > -OPEN_VG_RESET 3
> > -The following additional RESET_INDEX value is valid for i.MX6:
> > -IPU2_RESET 4
> > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> > b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> > new file mode 100644
> > index 0000000..276a533
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Freset%2Ffsl%2Cimx-src.yaml%23&data=02%
> 7C01
> >
> +%7Canson.huang%40nxp.com%7Cd622f3b31cca4b2e7b4808d7fb0fae99%7C
> 686ea1d
> >
> +3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637253917082767416&s
> data=7e%2
> >
> +FAy3dz4goXW6rNH2tAl3z5oNQ%2FIZ6Nykn4uBZ7S20%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cans
> on.hua
> >
> +ng%40nxp.com%7Cd622f3b31cca4b2e7b4808d7fb0fae99%7C686ea1d3bc2b
> 4c6fa92
> >
> +cd99c5c301635%7C0%7C0%7C637253917082767416&sdata=U%2Fs60
> QKHaPc5Ar
> > +GxJP97FLL%2BsClgZkuSmeQWKofm4oQ%3D&reserved=0
> > +
> > +title: Freescale i.MX System Reset Controller
> > +
> > +maintainers:
> > + - Philipp Zabel <p.zabel@pengutronix.de>
> > +
> > +description: |
> > + The system reset controller can be used to reset the GPU, VPU,
> > + IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
> > + nodes should specify the reset line on the SRC in their resets
> > + property, containing a phandle to the SRC device node and a
> > + RESET_INDEX specifying which module to reset, as described in
> > + reset.txt
> > +
> > + The following RESET_INDEX values are valid for i.MX5:
> > + GPU_RESET 0
> > + VPU_RESET 1
> > + IPU1_RESET 2
> > + OPEN_VG_RESET 3
> > + The following additional RESET_INDEX value is valid for i.MX6:
> > + IPU2_RESET 4
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - const: "fsl,imx51-src"
>
> "fsl,imx51-src" is the only compatible the driver matches on, but we have
> these combinations on compatible SRCs in the device trees:
>
> "fsl,imx50-src", "fsl,imx51-src"
> "fsl,imx51-src"
> "fsl,imx53-src", "fsl,imx51-src"
> "fsl,imx6q-src", "fsl,imx51-src"
> "fsl,imx6sl-src", "fsl,imx51-src"
> "fsl,imx6sll-src", "fsl,imx51-src"
> "fsl,imx6sx-src", "fsl,imx51-src"
> "fsl,imx6ul-src", "fsl,imx51-src"
>
> That could be described using oneOf and and an items list of const values per
> SoC like in the qcom bindings.
Yes, I noticed this and will add all of them in V2 per SoC, thanks.
Anson
^ permalink raw reply
* Re: [PATCH 02/17] dt-bindings: i2c: renesas,iic: Document r8a7742 support
From: Geert Uytterhoeven @ 2020-05-18 9:59 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Jens Axboe, Rob Herring, Wolfram Sang, Ulf Hansson,
Sergei Shtylyov, David S. Miller, Wim Van Sebroeck, Guenter Roeck,
linux-ide,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Linux I2C, Linux MMC List, netdev,
Linux-Renesas, Linux Watchdog Mailing List, Prabhakar
In-Reply-To: <1589555337-5498-3-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Fri, May 15, 2020 at 5:09 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Document IIC controller for RZ/G1H (R8A7742) SoC, which is compatible
> with R-Car Gen2 SoC family.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
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: [PATCH v2 4/5] PCI: uniphier: Add iATU register support
From: Kunihiko Hayashi @ 2020-05-18 10:05 UTC (permalink / raw)
To: Gustavo Pimentel
Cc: Bjorn Helgaas, Lorenzo Pieralisi, Jingoo Han, Rob Herring,
Masahiro Yamada, linux-pci@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Masami Hiramatsu, Jassi Brar
In-Reply-To: <DM5PR12MB1276D6181D86C8DF0F98427ADABD0@DM5PR12MB1276.namprd12.prod.outlook.com>
Hi Gustavo,
On 2020/05/15 22:16, Gustavo Pimentel wrote:
> Hi Kunihiko,
>
> On Fri, May 15, 2020 at 10:59:2, Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
>
>> This gets iATU register area from reg property. In Synopsis DWC version
>
> s/Synopsis/Synopsys
> in all patches
Thank you for pointing out.
I'll fix and be careful about this.
Thank you,
---
Best Regards
Kunihiko Hayashi
^ permalink raw reply
* Re: [PATCH 03/17] ARM: dts: r8a7742: Add I2C and IIC support
From: Geert Uytterhoeven @ 2020-05-18 10:10 UTC (permalink / raw)
To: Wolfram Sang
Cc: Lad, Prabhakar, Lad Prabhakar, Jens Axboe, Rob Herring,
Ulf Hansson, Sergei Shtylyov, David S. Miller, Wim Van Sebroeck,
Guenter Roeck, linux-ide,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Linux I2C, Linux MMC List, netdev, Linux-Renesas,
Linux Watchdog Mailing List
In-Reply-To: <20200518092601.GA3268@ninjato>
Hi Wolfram,
On Mon, May 18, 2020 at 11:26 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > However, both versions (with and without automatic transmission) are
> > > described with the same "renesas,iic-r8a7742" compatible. Is it possible
> > > to detect the reduced variant at runtime somehow?
> > >
> > I couldn't find anything the manual that would be useful to detect at runtime.
Hence if we really need that (see below), we need a quirk based on compatible
value + base address.
> > > My concern is that the peculiarity of this SoC might be forgotten if we
> > > describe it like this and ever add "automatic transmissions" somewhen.
> > >
> > Agreed.
>
> Well, I guess reading from a register which is supposed to not be there
> on the modified IP core is too hackish.
According to the Hardware User's Manual Rev. 1.00, the registers do exist
on all RZ/G1, except for RZ/G1E (see below).
"(automatic transmission can be used as a hardware function, but this is
not meaningful for actual use cases)."
(whatever that comment may mean?)
> Leaves us with a seperate compatible entry for it?
On R-Car E3 and RZ/G2E, which have a single IIC instance, we
handled that by:
The r8a77990 (R-Car E3) and r8a774c0 (RZ/G2E)
controllers are not considered compatible with
"renesas,rcar-gen3-iic" or "renesas,rmobile-iic"
due to the absence of automatic transmission registers.
On R-Car E2 and RZ/G1E, we forgot, and used both SoC-specific and
family-specific compatible values.
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: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Viresh Kumar @ 2020-05-18 10:11 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Serge Semin, Serge Semin, Thomas Bogendoerfer, Rafael J. Wysocki,
Ulf Hansson, Matthias Kaehlcke, Alexey Malahov, Paul Burton,
Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips, devicetree,
stable, Frederic Weisbecker, Ingo Molnar, Yue Hu, linux-pm,
linux-kernel
In-Reply-To: <a8dfa493-f858-e35d-7e57-78478be555c4@intel.com>
On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> That said if you really only want it to return 0 on success, you may as well
> add a ret = 0; statement (with a comment explaining why it is needed) after
> the last break in the loop.
That can be done as well, but will be a bit less efficient as the loop
will execute once for each policy, and so the statement will run
multiple times. Though it isn't going to add any significant latency
in the code.
--
viresh
^ permalink raw reply
* [PATCH V2] dt-bindings: reset: Convert i.MX reset to json-schema
From: Anson Huang @ 2020-05-18 10:03 UTC (permalink / raw)
To: p.zabel, robh+dt, shawnguo, s.hauer, kernel, festevam, devicetree,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
Convert the i.MX reset binding to DT schema format using json-schema.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V1:
- add the compatible strings per SoC;
- use "reset-controller" as node name instead of src;
- add interrupt items description.
---
.../devicetree/bindings/reset/fsl,imx-src.txt | 49 -----------
.../devicetree/bindings/reset/fsl,imx-src.yaml | 98 ++++++++++++++++++++++
2 files changed, 98 insertions(+), 49 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.txt
create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
deleted file mode 100644
index 6ed79e6..0000000
--- a/Documentation/devicetree/bindings/reset/fsl,imx-src.txt
+++ /dev/null
@@ -1,49 +0,0 @@
-Freescale i.MX System Reset Controller
-======================================
-
-Please also refer to reset.txt in this directory for common reset
-controller binding usage.
-
-Required properties:
-- compatible: Should be "fsl,<chip>-src"
-- reg: should be register base and length as documented in the
- datasheet
-- interrupts: Should contain SRC interrupt and CPU WDOG interrupt,
- in this order.
-- #reset-cells: 1, see below
-
-example:
-
-src: src@20d8000 {
- compatible = "fsl,imx6q-src";
- reg = <0x020d8000 0x4000>;
- interrupts = <0 91 0x04 0 96 0x04>;
- #reset-cells = <1>;
-};
-
-Specifying reset lines connected to IP modules
-==============================================
-
-The system reset controller can be used to reset the GPU, VPU,
-IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
-nodes should specify the reset line on the SRC in their resets
-property, containing a phandle to the SRC device node and a
-RESET_INDEX specifying which module to reset, as described in
-reset.txt
-
-example:
-
- ipu1: ipu@2400000 {
- resets = <&src 2>;
- };
- ipu2: ipu@2800000 {
- resets = <&src 4>;
- };
-
-The following RESET_INDEX values are valid for i.MX5:
-GPU_RESET 0
-VPU_RESET 1
-IPU1_RESET 2
-OPEN_VG_RESET 3
-The following additional RESET_INDEX value is valid for i.MX6:
-IPU2_RESET 4
diff --git a/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
new file mode 100644
index 0000000..7cd6095
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/fsl,imx-src.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/fsl,imx-src.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX System Reset Controller
+
+maintainers:
+ - Philipp Zabel <p.zabel@pengutronix.de>
+
+description: |
+ The system reset controller can be used to reset the GPU, VPU,
+ IPU, and OpenVG IP modules on i.MX5 and i.MX6 ICs. Those device
+ nodes should specify the reset line on the SRC in their resets
+ property, containing a phandle to the SRC device node and a
+ RESET_INDEX specifying which module to reset, as described in
+ reset.txt
+
+ The following RESET_INDEX values are valid for i.MX5:
+ GPU_RESET 0
+ VPU_RESET 1
+ IPU1_RESET 2
+ OPEN_VG_RESET 3
+ The following additional RESET_INDEX value is valid for i.MX6:
+ IPU2_RESET 4
+
+properties:
+ compatible:
+ oneOf:
+ - description: on i.MX51 the following compatible must be specified
+ items:
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX50 the following compatibles must be specified
+ items:
+ - const: "fsl,imx50-src"
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX53 the following compatibles must be specified
+ items:
+ - const: "fsl,imx53-src"
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX6Q the following compatibles must be specified
+ items:
+ - const: "fsl,imx6q-src"
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX6SX the following compatibles must be specified
+ items:
+ - const: "fsl,imx6sx-src"
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX6SL the following compatibles must be specified
+ items:
+ - const: "fsl,imx6sl-src"
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX6UL the following compatibles must be specified
+ items:
+ - const: "fsl,imx6ul-src"
+ - const: "fsl,imx51-src"
+
+ - description: on i.MX6SLL the following compatibles must be specified
+ items:
+ - const: "fsl,imx6sll-src"
+ - const: "fsl,imx51-src"
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ items:
+ - description: SRC interrupt
+ - description: CPU WDOG interrupts out of SRC
+ minItems: 1
+ maxItems: 2
+
+ '#reset-cells':
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - '#reset-cells'
+
+additionalProperties: false
+
+examples:
+ - |
+ reset-controller@73fd0000 {
+ compatible = "fsl,imx51-src";
+ reg = <0x73fd0000 0x4000>;
+ interrupts = <75>;
+ #reset-cells = <1>;
+ };
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Rafael J. Wysocki @ 2020-05-18 10:22 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Serge Semin, Serge Semin, Thomas Bogendoerfer,
Ulf Hansson, Matthias Kaehlcke, Alexey Malahov, Paul Burton,
Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips, devicetree,
stable, Frederic Weisbecker, Ingo Molnar, Yue Hu, linux-pm,
linux-kernel
In-Reply-To: <20200518101109.4uggngudy4gfmlvo@vireshk-i7>
On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > That said if you really only want it to return 0 on success, you may as well
> > add a ret = 0; statement (with a comment explaining why it is needed) after
> > the last break in the loop.
>
> That can be done as well, but will be a bit less efficient as the loop
> will execute once for each policy, and so the statement will run
> multiple times. Though it isn't going to add any significant latency
> in the code.
Right.
However, the logic in this entire function looks somewhat less than
straightforward to me, because it looks like it should return an
error on the first policy without a frequency table (having a frequency
table depends on the driver and that is the same for all policies, so it
is pointless to iterate any further in that case).
Also, the error should not be -EINVAL, because that means "invalid
argument" which would be the state value.
So I would do something like this:
---
drivers/cpufreq/cpufreq.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
static int cpufreq_boost_set_sw(int state)
{
struct cpufreq_policy *policy;
- int ret = -EINVAL;
for_each_active_policy(policy) {
+ int ret;
+
if (!policy->freq_table)
- continue;
+ return -ENXIO;
ret = cpufreq_frequency_table_cpuinfo(policy,
policy->freq_table);
if (ret) {
pr_err("%s: Policy frequency update failed\n",
__func__);
- break;
+ return ret;
}
ret = freq_qos_update_request(policy->max_freq_req, policy->max);
if (ret < 0)
- break;
+ return ret;
}
- return ret;
+ return 0;
}
int cpufreq_boost_trigger_state(int state)
^ permalink raw reply
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Viresh Kumar @ 2020-05-18 10:24 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Serge Semin, Serge Semin, Thomas Bogendoerfer,
Ulf Hansson, Matthias Kaehlcke, Alexey Malahov, Paul Burton,
Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips, devicetree,
stable, Frederic Weisbecker, Ingo Molnar, Yue Hu, linux-pm,
linux-kernel
In-Reply-To: <10461949.HoJUxHt8jL@kreacher>
On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > That said if you really only want it to return 0 on success, you may as well
> > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > the last break in the loop.
> >
> > That can be done as well, but will be a bit less efficient as the loop
> > will execute once for each policy, and so the statement will run
> > multiple times. Though it isn't going to add any significant latency
> > in the code.
>
> Right.
>
> However, the logic in this entire function looks somewhat less than
> straightforward to me, because it looks like it should return an
> error on the first policy without a frequency table (having a frequency
> table depends on the driver and that is the same for all policies, so it
> is pointless to iterate any further in that case).
>
> Also, the error should not be -EINVAL, because that means "invalid
> argument" which would be the state value.
>
> So I would do something like this:
>
> ---
> drivers/cpufreq/cpufreq.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> static int cpufreq_boost_set_sw(int state)
> {
> struct cpufreq_policy *policy;
> - int ret = -EINVAL;
>
> for_each_active_policy(policy) {
> + int ret;
> +
> if (!policy->freq_table)
> - continue;
> + return -ENXIO;
>
> ret = cpufreq_frequency_table_cpuinfo(policy,
> policy->freq_table);
> if (ret) {
> pr_err("%s: Policy frequency update failed\n",
> __func__);
> - break;
> + return ret;
> }
>
> ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> if (ret < 0)
> - break;
> + return ret;
> }
>
> - return ret;
> + return 0;
> }
>
> int cpufreq_boost_trigger_state(int state)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* Re: [PATCH v4 00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards
From: Pali Rohár @ 2020-05-18 10:30 UTC (permalink / raw)
To: Lorenzo Pieralisi, Gregory CLEMENT
Cc: Andrew Murray, Bjorn Helgaas, Thomas Petazzoni, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Rob Herring, Remi Pommarel,
Marek Behún, Tomasz Maciej Nowak, Xogium, devicetree,
linux-kernel, linux-pci
In-Reply-To: <87pnb2h7w1.fsf@FE-laptop>
On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> Hello,
>
> > Hello,
> >
> > On Thu, 30 Apr 2020 10:06:13 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> >
> >> Marek Behún (5):
> >> PCI: aardvark: Improve link training
> >> PCI: aardvark: Add PHY support
> >> dt-bindings: PCI: aardvark: Describe new properties
> >> arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> >> arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> >>
> >> Pali Rohár (7):
> >> PCI: aardvark: Train link immediately after enabling training
> >> PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> >> read-only register
> >> PCI: of: Zero max-link-speed value is invalid
> >> PCI: aardvark: Issue PERST via GPIO
> >> PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> >> PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> >> macros
> >> arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> >
> > Thanks a lot for this work. For a number of reasons, I'm less involved
> > in Marvell platform support in Linux, but I reviewed your series and
> > followed the discussions around it, and I'm happy to give my:
> >
> > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>
> With this acked-by for the series, the reviewed-by from Rob on the
> binding and the tested-by, I am pretty confident so I applied the
> patches 10, 11 and 12 on mvebu/dt64.
>
> Thanks,
>
> Gregory
Thank you!
Lorenzo, would you now take remaining patches?
>
> >
> > for the whole series. The changes all seem sensible, and have been
> > tested by several folks.
> >
> > Thanks!
> >
> > Thomas
> > --
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
>
> --
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Serge Semin @ 2020-05-18 10:31 UTC (permalink / raw)
To: Viresh Kumar
Cc: Serge Semin, Rafael J. Wysocki, Rafael J. Wysocki,
Thomas Bogendoerfer, Ulf Hansson, Matthias Kaehlcke,
Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann,
Rob Herring, linux-mips, devicetree, stable, Frederic Weisbecker,
Ingo Molnar, Yue Hu, linux-pm, linux-kernel
In-Reply-To: <20200518102415.k4c5qglodij5ac6h@vireshk-i7>
On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > That said if you really only want it to return 0 on success, you may as well
> > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > the last break in the loop.
> > >
> > > That can be done as well, but will be a bit less efficient as the loop
> > > will execute once for each policy, and so the statement will run
> > > multiple times. Though it isn't going to add any significant latency
> > > in the code.
> >
> > Right.
> >
> > However, the logic in this entire function looks somewhat less than
> > straightforward to me, because it looks like it should return an
> > error on the first policy without a frequency table (having a frequency
> > table depends on the driver and that is the same for all policies, so it
> > is pointless to iterate any further in that case).
> >
> > Also, the error should not be -EINVAL, because that means "invalid
> > argument" which would be the state value.
> >
> > So I would do something like this:
> >
> > ---
> > drivers/cpufreq/cpufreq.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > static int cpufreq_boost_set_sw(int state)
> > {
> > struct cpufreq_policy *policy;
> > - int ret = -EINVAL;
> >
> > for_each_active_policy(policy) {
> > + int ret;
> > +
> > if (!policy->freq_table)
> > - continue;
> > + return -ENXIO;
> >
> > ret = cpufreq_frequency_table_cpuinfo(policy,
> > policy->freq_table);
> > if (ret) {
> > pr_err("%s: Policy frequency update failed\n",
> > __func__);
> > - break;
> > + return ret;
> > }
> >
> > ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > if (ret < 0)
> > - break;
> > + return ret;
> > }
> >
> > - return ret;
> > + return 0;
> > }
> >
> > int cpufreq_boost_trigger_state(int state)
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Ok. Thanks for the comments. Shall I resend the patch with update Rafael
suggests or you'll merge the Rafael's fix in yourself?
-Sergey
>
> --
> viresh
^ permalink raw reply
* Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support
From: Ravi Kumar Bokka (Temp) @ 2020-05-18 10:33 UTC (permalink / raw)
To: Doug Anderson
Cc: Srinivas Kandagatla, Rob Herring, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rajendra Nayak, Sai Prakash Ranjan, dhavalp, mturney, sparate,
c_rbokka, mkurumel
In-Reply-To: <CAD=FV=Xw-ZYMrvgUTvw2uZPigrRYdVYw85Y5_vvJ+JeqV5U5iw@mail.gmail.com>
Hi Doug,
Please find my inline comments.
On 5/14/2020 11:51 PM, Doug Anderson wrote:
> Hi,
>
> I notice that you didn't respond to any of my feedback [1], only
> Srinivas's. Any reason why? It turns out that Srinivas had quite a
> lot of the same feedback as I did, but please make sure you didn't
> miss anything I suggested. More inline below.
>
> On Thu, May 14, 2020 at 5:26 AM Ravi Kumar Bokka (Temp)
> <rbokka@codeaurora.org> wrote:
>>
>> Hi Srinivas,
>> Thanks for your feedback by giving review comments. Please find my
>> inline comments.
>>
>>
>> Regards,
>> Ravi Kumar.B
>>
>> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>>>> This patch adds new driver for QTI qfprom-efuse controller. This
>>>> driver can
>>>> access the raw qfprom regions for fuse blowing.
>>>
>>> QTI?
>>
>> guidance I have received from internal Legal/LOST team is that the QCOM
>> prefix needs to be changed to QTI everywhere it is used
>
> I'll let Srinivas comment if he cares. I'm really not sure why a
> legal team cares about the Kconfig name in a GPL-licensed Linux
> kernel.
>
I will maintain Qualcomm Technologies Inc(QTI) in the patch description
and change Kconfig as you suggested.
>
>>>> The current existed qfprom driver is only supports for cpufreq,
>>>> thermal sensors
>>>> drivers by read out calibration data, speed bins..etc which is stored
>>>> by qfprom efuses.
>>>
>>> Can you explain bit more about this QFPROM instance, Is this QFPROM part
>>> of secure controller address space?
>>> Is this closely tied to SoC or Secure controller version?
>>>
>>> Any reason why this can not be integrated into qfprom driver with
>>> specific compatible.
>>>
>>
>> QFPROM driver communicates with sec_controller address space however
>> scope and functionalities of this driver is different and not limited as
>> existing qfprom fuse Read-Only driver for specific “fuse buckets’ like
>> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch
>> requires specific sequence to write/blow fuses unlike other driver.
>> Scope/functionalities are different and this is separate driver.
>
> If the underlying IP blocks are the same it should be one driver and
> it should just work in read-only mode for the other range of stuff.
>
Based on the compatible, do i need to separate probe function for
qfprom-efuse and maintain separate nvmem object to register nvmem
framework. Is this what you are suggesting to implementing this in to
one existing driver?
Do I need to maintain separate efuse dt node?
Could you please suggest me to proceed further.
>
>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>> ---
>>>> drivers/nvmem/Kconfig | 10 +
>>>> drivers/nvmem/Makefile | 2 +
>>>> drivers/nvmem/qfprom-efuse.c | 476
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 488 insertions(+)
>>>> create mode 100644 drivers/nvmem/qfprom-efuse.c
>>>>
>>> ...
>>>
>>>> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
>>>> new file mode 100644
>>>> index 0000000..2e3c275
>>>> --- /dev/null
>>>> +++ b/drivers/nvmem/qfprom-efuse.c
>>>> @@ -0,0 +1,476 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/nvmem-provider.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>>>> +#define QFPROM_BLOW_STATUS_READY 0x0
>>>> +
>>>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>>>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>>>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>>>> +
>>>> +/* Amount of time required to hold charge to blow fuse in
>>>> micro-seconds */
>>>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>>>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>>>> +
>>>> +#define QFPROM_ACCEL_OFFSET 0x044
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>>>> + * platform data
>>>> + *
>>>> + * @name: qfprom-efuse compatible name
>>>
>>> ??
>>
>> Thanks for your feedback. I will address this change
>>
>>>> + * @fuse_blow_time_in_us: Should contain the wait time when doing the
>>>> fuse blow
>>>> + * @accel_value: Should contain qfprom accel value
>>>> + * @accel_reset_value: The reset value of qfprom accel value
>>>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
>>>> efuse blow
>>>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
>>>> blowing
>>>> + * is done
>>>> + * @qfprom_blow_set_freq: The frequency required to set when we start
>>>> the
>>>> + * fuse blowing
>>>> + * @qfprom_max_vol: max voltage required to set fuse blow
>>>> + * @qfprom_min_vol: min voltage required to set fuse blow
>>>
>>> How specific are these values per SoC?
>>>
>>
>> This voltage level may change based on SoC and/or fuse-hardware
>> technology, it would change for SoC with different technology, hence we
>> have kept it in SOC specific settings.
>
> Generally I'd expect the SoC specific settings to be in the device
> tree. Drivers don't need to specify this. Please respond to the
> comments I posed in my review.
>
Thanks for your feedback. I will address this change.
>
>>>> + */
>>>> +struct qfprom_efuse_platform_data {
>>>> + const char *name;
>>>> + u8 fuse_blow_time_in_us;
>>>> + u32 accel_value;
>>>> + u32 accel_reset_value;
>>>> + u32 qfprom_blow_timer_value;
>>>> + u32 qfprom_blow_reset_freq;
>>>> + u32 qfprom_blow_set_freq;
>>>> + u32 qfprom_max_vol;
>>>> + u32 qfprom_min_vol;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
>>>> + *
>>>> + * @qfpbase: iomapped memory space for qfprom base
>>>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>>>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>>>> +
>>>> + * @dev: qfprom device structure
>>>> + * @secclk: clock supply
>>>> + * @vcc: regulator supply
>>>> +
>>>> + * @qfpraw_start: qfprom raw fuse start region
>>>> + * @qfpraw_end: qfprom raw fuse end region
>>>> + * @qfprom_efuse_platform_data: qfprom platform data
>>>> + */
>>>> +struct qfprom_efuse_priv {
>>>> + void __iomem *qfpbase;
>>>> + void __iomem *qfpraw;
>>>> + void __iomem *qfpmap;
>>>
>>> Why are these memory regions split? Can't you just have complete qfprom
>>> area and add fixed offset for qfpraw within the driver?
>>>
>>
>> Thanks for your feedback. I will address this change.
>> I have separated this memory regions because to identify raw fuse
>> regions separately and compare these raw fuse regions from the user
>> given input.
>
> How are you addressing? Can you go back to just having one range? If
> you need to know where the raw fuse region is inside your range just
> put the offset in the of_match data.
>
I will maintain one range as suggested reg = <0 0x00780000 0 0x2100>.
>
>>>> + struct device *dev;
>>>> + struct clk *secclk;
>>>> + struct regulator *vcc;
>>>> + resource_size_t qfpraw_start;
>>>> + resource_size_t qfpraw_end;
>>> Why do we need to check this range? as long as we set the nvmem_config
>>> with correct range then you should not need this check.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and based
>> on internal review with our security team, this check is important to
>> avoid dependency on other upper layer.
>
> There is harm: it adds extra complexity. Please remove.
>
> You are talking as if it was somehow important for this code to be the
> same as the code on other OSes / in other contexts. It isn't. This
> is a Linux driver and it should not be written to duplicate stuff that
> Linux is already doing.
>
This for your feedback. I will address this change.
>
>>>> + struct qfprom_efuse_platform_data efuse;
>>> A pointer here should be good enough?
>>>> +};
>>>> +
>>>
>>
>> Thanks for your feedback. I will address this change
>>
>>> ...
>>>
>>>> +/*
>>>> + * sets the value of the blow timer, accel register and the clock
>>>> + * and voltage settings
>>>> + */
>>>> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv
>>>> *priv)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = qfprom_disable_fuse_blowing(priv);
>>>> + if (ret) {
>>>> + dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>>>> + return ret;
>>>> + }
>>>
>>> Why do we need to qfprom_disable_fuse_blowing() for every call to enable
>>> it?
>>>
>>> Or are we missing some error handling in the caller?
>>>
>>
>> We must disable/vote-off this QFPROM fuse power rail after blowing fuse,
>> it is the safe and right approach as per hardware programming guide for
>> fuse blowing process. Caller here is user space, can’t control
>> fuse-power-rail or can’t be relied to follow the required process. There
>> could also be unnecessary risk of leaving the vote/power-rail configured
>> at specific level after blowing the fuse. As per hardware requirement,
>> right after fuse blowing, we need to disable power rail.
>
> Please remove your disable here. Though the user initiates the call
> you can still rely on Linux to make sure two users aren't trying to
> blow fuses at the same time. The Linux driver should always leave
> things in a "disabled" state and you can rely on that.
>
> ...besides the only way you aren't hitting an underflow on the
> regulator enable count is that you are constantly enabling over and
> over again.
>
I will address this change.
>
>>>> +
>>>> + writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>>>> + QFPROM_BLOW_TIMER_OFFSET);
>>>> + writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
>>>> +
>>>> + ret = qfprom_set_clock_settings(priv);
>>>> + if (ret) {
>>>> + dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
>>>> + priv->efuse.qfprom_max_vol);
>>>> + if (ret) {
>>>> + dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> <<
>>>> +/*
>>>> + * verifying to make sure address being written or read is from qfprom
>>>> + * raw address range
>>>> + */
>>>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
>>>> + size_t bytes)
>>>> +{
>>>> + if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>>>> + ((reg + bytes) <= priv->qfpraw_end)) {
>>>> + return 1;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>> >>
>>> Above function is totally redundant, nvmem core already has checks for
>>> this.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and based
>> on internal review with our security team, this check is important to
>> avoid dependency on other upper layer.
>
> Please remove. You are a Linux driver.
>
I will address this change.
>
>>>> +
>>>> +/*
>>>> + * API for reading from raw qfprom region
>>>> + */
>>>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
>>>> void *_val,
>>>> + size_t bytes)
>>>> +{
>>>> + struct qfprom_efuse_priv *priv = context;
>>>> + u32 *value = _val;
>>>> + u32 align_check;
>>>> + int i = 0, words = bytes / 4;
>>>> +
>>>> + dev_info(priv->dev,
>>>> + "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>>>> + reg, bytes);
>>>
>>> In general there is lot of debug info across the code, do you really
>>> need all this? Consider removing these!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> + if (bytes % 4 != 0x00) {
>>>> + dev_err(priv->dev,
>>>> + "Bytes: %zd to read should be word align\n",
>>>> + bytes);
>>>> + return -EINVAL;
>>>> + }
>>>
>>> This word align check is also redundant once you set nvmem_config with
>>> correct word_size.
>>>
>>
>> I understand that there may be different approach to handle this. We
>> have used this approach and tested this driver thoroughly. Unless there
>> is technical limitation, changing this word_size would end up requiring
>> re-writing write/read APIs and going through testing again, there is not
>> much difference in either approach, we would like to keep this approach
>> unless there is technical concern.
>
> The driver isn't done until it lands in Linux. While it's important
> to test the driver before posting upstream it is completely expected
> and normal that then posting upstream you will be asked to change
> things. After you change things you will need to re-test. The fact
> that you already tested this the old way is not an excuse. Please
> fix.
>
Thanks for your feedback, i will address this change.
>
>>>> +
>>>> + if (!addr_in_qfprom_range(priv, reg, bytes)) {
>>>> + dev_err(priv->dev,
>>>> + "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>>>> + reg, bytes);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + align_check = (reg & 0xF);
>>>> +
>>>> + if (((align_check & ~3) == align_check) && value != NULL)
>>>> + while (words--)
>>>> + *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>>>> +
>>>> + else
>>>> + dev_err(priv->dev,
>>>> + "Invalid input parameter 0x%08x fuse blow address\n",
>>>> + reg);
>>>> +
>>>> + return 0;
>>>> +}
>>> ...
>>>
>>>> +
>>>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct resource *qfpbase, *qfpraw, *qfpmap;
>>>> + struct nvmem_device *nvmem;
>>>> + struct nvmem_config *econfig;
>>>> + struct qfprom_efuse_priv *priv;
>>>> + const struct qfprom_efuse_platform_data *drvdata;
>>>> + int ret;
>>>> +
>>>> + dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>>>> +
>>>
>>> too much debug!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> + drvdata = of_device_get_match_data(&pdev->dev);
>>>> + if (!drvdata)
>>>> + return -EINVAL;
>>> Unnecessary check as this driver will not be probed unless there is a
>>> compatible match.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>
>>>> +
>>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> + if (!priv)
>>>> + return -ENOMEM;
>>>> +
>>>> + priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>>>> + priv->efuse.accel_value = drvdata->accel_value;
>>>> + priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>>>> + priv->efuse.qfprom_blow_timer_value =
>>>> drvdata->qfprom_blow_timer_value;
>>>> + priv->efuse.qfprom_blow_reset_freq =
>>>> drvdata->qfprom_blow_reset_freq;
>>>> + priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>>>> + priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>>>> + priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>>>> + priv->dev = dev;
>>>> +
>>>> + qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +
>>>> + priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>>>> + if (IS_ERR(priv->qfpbase)) {
>>>> + ret = PTR_ERR(priv->qfpbase);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +
>>>> + priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>>>> + if (IS_ERR(priv->qfpraw)) {
>>>> + ret = PTR_ERR(priv->qfpraw);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + priv->qfpraw_start = qfpraw->start - qfpbase->start;
>>>> + priv->qfpraw_end = qfpraw->end - qfpbase->start;
>>>> +
>>>> + qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>>> +
>>>> + priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>>>> + if (IS_ERR(priv->qfpmap)) {
>>>> + ret = PTR_ERR(priv->qfpmap);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>>
>>> I see no reference to this regulator in dt bindings.
>>
>> This perameter kept in board specific file i.e., sc7180-idp.dts file
>
> Yes, but it still needs to be in the bindings.
>
Thanks for your feedback. i will address this change.
>
>>>> + if (IS_ERR(priv->vcc)) {
>>>> + ret = PTR_ERR(priv->vcc);
>>>> + if (ret == -ENODEV)
>>>> + ret = -EPROBE_DEFER;
>>> Can you explain what is going on here?
>>>
>>
>> As i took other drivers reference, i have kept this check.
>
> Then other drivers are wrong. Please remove.
>
Thanks for your feedback. I will address this change.
>
>>>> +
>>>> + goto err;
>>>> + }
>>>> +
>>>> + priv->secclk = devm_clk_get(dev, "secclk");
>>>> + if (IS_ERR(priv->secclk)) {
>>>> + ret = PTR_ERR(priv->secclk);
>>>> + if (ret != -EPROBE_DEFER)
>>>> + dev_err(dev, "secclk error getting : %d\n", ret);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + ret = clk_prepare_enable(priv->secclk);
>>>> + if (ret) {
>>>> + dev_err(dev, "clk_prepare_enable() failed\n");
>>>> + goto err;
>>>> + }
>>>> +
>>>> + econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>>>> + if (!econfig)
>>> Why not disabling the clk here?
>>>> + return -ENOMEM;
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> + econfig->dev = dev;
>>>> + econfig->name = "qfprom-efuse";
>>>> + econfig->stride = 1;
>>>> + econfig->word_size = 1;
>>>> + econfig->reg_read = qfprom_efuse_reg_read;
>>>> + econfig->reg_write = qfprom_efuse_reg_write;
>>>> + econfig->size = resource_size(qfpraw);
>>>> + econfig->priv = priv;
>>>> +
>>>> + nvmem = devm_nvmem_register(dev, econfig);
>>>> +
>>>> + return PTR_ERR_OR_ZERO(nvmem);
>>> probably you should check the nvmem here before returning to disable the
>>> clk properly.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +err:
>>>> + clk_disable_unprepare(priv->secclk);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
>>>> + .name = "sc7180-qfprom-efuse",
>>> Redundant.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> + .fuse_blow_time_in_us = 10,
>>>> + .accel_value = 0xD10,
>>>> + .accel_reset_value = 0x800,
>>>> + .qfprom_blow_timer_value = 25,
>>>> + .qfprom_blow_reset_freq = 19200000,
>>>> + .qfprom_blow_set_freq = 4800000,
>>>> + .qfprom_max_vol = 1904000,
>>>> + .qfprom_min_vol = 1800000,
>>>> +};
>>>> +
>>>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>>>> + {
>>>> + .compatible = "qcom,sc7180-qfprom-efuse",
>>>> + .data = &sc7180_qfp_efuse_data
>>>> + },
>>>> + {/* sentinel */},
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>>>> +
>>>> +static struct platform_driver qfprom_efuse_driver = {
>>>> + .probe = qfprom_efuse_probe,
>>>> + .driver = {
>>>> + .name = "sc7180-qfprom-efuse",
>>>> + .of_match_table = qfprom_efuse_of_match,
>>>> + },
>>>> +};
>>>> +
>>>> +module_platform_driver(qfprom_efuse_driver);
>>>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>
>> --
>> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of the Code Aurora Forum, hosted by the Linux Foundation.
>
> [1] https://lore.kernel.org/r/CAD=FV=UZQRfBTbh2CLnwsRSpbXFf=8iF2MG20hdj47s42aP8HQ@mail.gmail.com
>
Regards,
Ravi Kumar.B
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by the Linux Foundation.
^ permalink raw reply
* Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support
From: Ravi Kumar Bokka (Temp) @ 2020-05-18 10:39 UTC (permalink / raw)
To: Srinivas Kandagatla, Rob Herring
Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp,
mturney, sparate, c_rbokka, mkurumel, dianders
In-Reply-To: <d5902226-21b3-7941-6405-688d7a115142@linaro.org>
Hi Srinivas,
Thanks for your feedback. Please find my inline-comments.
On 5/15/2020 4:39 PM, Srinivas Kandagatla wrote:
>
>
> On 14/05/2020 13:26, Ravi Kumar Bokka (Temp) wrote:
>> Hi Srinivas,
>> Thanks for your feedback by giving review comments. Please find my
>> inline comments.
>>
>>
>> Regards,
>> Ravi Kumar.B
>>
>> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>>>> This patch adds new driver for QTI qfprom-efuse controller. This
>>>> driver can
>>>> access the raw qfprom regions for fuse blowing.
>>>
>>> QTI?
>>
>> guidance I have received from internal Legal/LOST team is that the
>> QCOM prefix needs to be changed to QTI everywhere it is used
>>
>
> I dont mind this in comments or patch subject line but the valid vendor
> prefix for Qualcomm is "qcom".
>
I will maintain Qualcomm Technologies Inc(QTI) in the patch description
and change Kconfig as suggested.
>
>>>
>>>>
>>>> The current existed qfprom driver is only supports for cpufreq,
>>>> thermal sensors
>>>> drivers by read out calibration data, speed bins..etc which is stored
>>>> by qfprom efuses.
>>>
>>> Can you explain bit more about this QFPROM instance, Is this QFPROM
>>> part of secure controller address space?
>>> Is this closely tied to SoC or Secure controller version?
>>>
>>> Any reason why this can not be integrated into qfprom driver with
>>> specific compatible.
>>>
>>
>> QFPROM driver communicates with sec_controller address space however
>> scope and functionalities of this driver is different and not limited
>> as existing qfprom fuse Read-Only driver for specific “fuse buckets’
>> like cpufreq, thermal sensors etc. QFPROM fuse write driver in this
>> patch requires specific sequence to write/blow fuses unlike other
>> driver. Scope/functionalities are different and this is separate driver.
>>
>
> This is another variant of qfprom, so please add this support in the
> existing qfprom driver, you could deal with the differences using
> specific compatible within the driver.
>
> Doug already covered most of the comments, consider them as mandatory
> requirements for upstreaming!
>
> --srini
>
Based on the compatible, do i need to separate probe function for
qfprom-efuse and maintain separate nvmem object to register nvmem
framework. Is this what you are suggesting to implementing this in to
one existing driver?
Do I need to maintain separate efuse dt node?
Could you please suggest me to proceed further.
>
>>>>
>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>> ---
>>>> drivers/nvmem/Kconfig | 10 +
>>>> drivers/nvmem/Makefile | 2 +
>>>> drivers/nvmem/qfprom-efuse.c | 476
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 488 insertions(+)
>>>> create mode 100644 drivers/nvmem/qfprom-efuse.c
>>>>
>>> ...
>>>
>>>> diff --git a/drivers/nvmem/qfprom-efuse.c
>>>> b/drivers/nvmem/qfprom-efuse.c
>>>> new file mode 100644
>>>> index 0000000..2e3c275
>>>> --- /dev/null
>>>> +++ b/drivers/nvmem/qfprom-efuse.c
>>>> @@ -0,0 +1,476 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/nvmem-provider.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>>>> +#define QFPROM_BLOW_STATUS_READY 0x0
>>>> +
>>>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>>>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>>>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>>>> +
>>>> +/* Amount of time required to hold charge to blow fuse in
>>>> micro-seconds */
>>>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>>>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>>>> +
>>>> +#define QFPROM_ACCEL_OFFSET 0x044
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>>>> + * platform data
>>>> + *
>>>> + * @name: qfprom-efuse compatible name
>>>
>>> ??
>>
>> Thanks for your feedback. I will address this change
>>
>>>> + * @fuse_blow_time_in_us: Should contain the wait time when doing
>>>> the fuse blow
>>>> + * @accel_value: Should contain qfprom accel value
>>>> + * @accel_reset_value: The reset value of qfprom accel value
>>>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
>>>> efuse blow
>>>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
>>>> blowing
>>>> + * is done
>>>> + * @qfprom_blow_set_freq: The frequency required to set when we
>>>> start the
>>>> + * fuse blowing
>>>> + * @qfprom_max_vol: max voltage required to set fuse blow
>>>> + * @qfprom_min_vol: min voltage required to set fuse blow
>>>
>>> How specific are these values per SoC?
>>>
>>
>> This voltage level may change based on SoC and/or fuse-hardware
>> technology, it would change for SoC with different technology, hence
>> we have kept it in SOC specific settings.
>>
>>>
>>>> + */
>>>> +struct qfprom_efuse_platform_data {
>>>> + const char *name;
>>>> + u8 fuse_blow_time_in_us;
>>>> + u32 accel_value;
>>>> + u32 accel_reset_value;
>>>> + u32 qfprom_blow_timer_value;
>>>> + u32 qfprom_blow_reset_freq;
>>>> + u32 qfprom_blow_set_freq;
>>>> + u32 qfprom_max_vol;
>>>> + u32 qfprom_min_vol;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse
>>>> attributes
>>>> + *
>>>> + * @qfpbase: iomapped memory space for qfprom base
>>>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>>>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>>>> +
>>>> + * @dev: qfprom device structure
>>>> + * @secclk: clock supply
>>>> + * @vcc: regulator supply
>>>> +
>>>> + * @qfpraw_start: qfprom raw fuse start region
>>>> + * @qfpraw_end: qfprom raw fuse end region
>>>> + * @qfprom_efuse_platform_data: qfprom platform data
>>>> + */
>>>> +struct qfprom_efuse_priv {
>>>> + void __iomem *qfpbase;
>>>> + void __iomem *qfpraw;
>>>> + void __iomem *qfpmap;
>>>
>>> Why are these memory regions split? Can't you just have complete
>>> qfprom area and add fixed offset for qfpraw within the driver?
>>>
>>
>> Thanks for your feedback. I will address this change.
>> I have separated this memory regions because to identify raw fuse
>> regions separately and compare these raw fuse regions from the user
>> given input.
>>
>>>> + struct device *dev;
>>>> + struct clk *secclk;
>>>> + struct regulator *vcc;
>>>> + resource_size_t qfpraw_start;
>>>> + resource_size_t qfpraw_end;
>>> Why do we need to check this range? as long as we set the
>>> nvmem_config with correct range then you should not need this check.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and
>> based on internal review with our security team, this check is
>> important to avoid dependency on other upper layer.
>>
>>
>>>
>>>> + struct qfprom_efuse_platform_data efuse;
>>> A pointer here should be good enough?
>>>> +};
>>>> +
>>>
>>
>> Thanks for your feedback. I will address this change
>>
>>> ...
>>>
>>>> +/*
>>>> + * sets the value of the blow timer, accel register and the clock
>>>> + * and voltage settings
>>>> + */
>>>> +static int qfprom_enable_fuse_blowing(const struct
>>>> qfprom_efuse_priv *priv)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = qfprom_disable_fuse_blowing(priv);
>>>> + if (ret) {
>>>> + dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>>>> + return ret;
>>>> + }
>>>
>>> Why do we need to qfprom_disable_fuse_blowing() for every call to
>>> enable it?
>>>
>>> Or are we missing some error handling in the caller?
>>>
>>
>> We must disable/vote-off this QFPROM fuse power rail after blowing
>> fuse, it is the safe and right approach as per hardware programming
>> guide for fuse blowing process. Caller here is user space, can’t
>> control fuse-power-rail or can’t be relied to follow the required
>> process. There could also be unnecessary risk of leaving the
>> vote/power-rail configured at specific level after blowing the fuse.
>> As per hardware requirement, right after fuse blowing, we need to
>> disable power rail.
>>
>>>> +
>>>> + writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>>>> + QFPROM_BLOW_TIMER_OFFSET);
>>>> + writel(priv->efuse.accel_value, priv->qfpmap +
>>>> QFPROM_ACCEL_OFFSET);
>>>> +
>>>> + ret = qfprom_set_clock_settings(priv);
>>>> + if (ret) {
>>>> + dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = qfprom_set_voltage_settings(priv,
>>>> priv->efuse.qfprom_min_vol,
>>>> + priv->efuse.qfprom_max_vol);
>>>> + if (ret) {
>>>> + dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>
>>> <<
>>>> +/*
>>>> + * verifying to make sure address being written or read is from qfprom
>>>> + * raw address range
>>>> + */
>>>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32
>>>> reg,
>>>> + size_t bytes)
>>>> +{
>>>> + if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>>>> + ((reg + bytes) <= priv->qfpraw_end)) {
>>>> + return 1;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>> >>
>>> Above function is totally redundant, nvmem core already has checks
>>> for this.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and
>> based on internal review with our security team, this check is
>> important to avoid dependency on other upper layer.
>>
>>>
>>>
>>>> +
>>>> +/*
>>>> + * API for reading from raw qfprom region
>>>> + */
>>>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
>>>> void *_val,
>>>> + size_t bytes)
>>>> +{
>>>> + struct qfprom_efuse_priv *priv = context;
>>>> + u32 *value = _val;
>>>> + u32 align_check;
>>>> + int i = 0, words = bytes / 4;
>>>> +
>>>> + dev_info(priv->dev,
>>>> + "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>>>> + reg, bytes);
>>>
>>> In general there is lot of debug info across the code, do you really
>>> need all this? Consider removing these!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> + if (bytes % 4 != 0x00) {
>>>> + dev_err(priv->dev,
>>>> + "Bytes: %zd to read should be word align\n",
>>>> + bytes);
>>>> + return -EINVAL;
>>>> + }
>>>
>>> This word align check is also redundant once you set nvmem_config
>>> with correct word_size.
>>>
>>
>> I understand that there may be different approach to handle this. We
>> have used this approach and tested this driver thoroughly. Unless
>> there is technical limitation, changing this word_size would end up
>> requiring re-writing write/read APIs and going through testing again,
>> there is not much difference in either approach, we would like to keep
>> this approach unless there is technical concern.
>>
>>>
>>>> +
>>>> + if (!addr_in_qfprom_range(priv, reg, bytes)) {
>>>> + dev_err(priv->dev,
>>>> + "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>>>> + reg, bytes);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + align_check = (reg & 0xF);
>>>> +
>>>> + if (((align_check & ~3) == align_check) && value != NULL)
>>>> + while (words--)
>>>> + *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>>>> +
>>>> + else
>>>> + dev_err(priv->dev,
>>>> + "Invalid input parameter 0x%08x fuse blow address\n",
>>>> + reg);
>>>> +
>>>> + return 0;
>>>> +}
>>> ...
>>>
>>>> +
>>>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct resource *qfpbase, *qfpraw, *qfpmap;
>>>> + struct nvmem_device *nvmem;
>>>> + struct nvmem_config *econfig;
>>>> + struct qfprom_efuse_priv *priv;
>>>> + const struct qfprom_efuse_platform_data *drvdata;
>>>> + int ret;
>>>> +
>>>> + dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>>>> +
>>>
>>> too much debug!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> + drvdata = of_device_get_match_data(&pdev->dev);
>>>> + if (!drvdata)
>>>> + return -EINVAL;
>>> Unnecessary check as this driver will not be probed unless there is a
>>> compatible match.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>
>>>> +
>>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> + if (!priv)
>>>> + return -ENOMEM;
>>>> +
>>>> + priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>>>> + priv->efuse.accel_value = drvdata->accel_value;
>>>> + priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>>>> + priv->efuse.qfprom_blow_timer_value =
>>>> drvdata->qfprom_blow_timer_value;
>>>> + priv->efuse.qfprom_blow_reset_freq =
>>>> drvdata->qfprom_blow_reset_freq;
>>>> + priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>>>> + priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>>>> + priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>>>> + priv->dev = dev;
>>>> +
>>>> + qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +
>>>> + priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>>>> + if (IS_ERR(priv->qfpbase)) {
>>>> + ret = PTR_ERR(priv->qfpbase);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +
>>>> + priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>>>> + if (IS_ERR(priv->qfpraw)) {
>>>> + ret = PTR_ERR(priv->qfpraw);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + priv->qfpraw_start = qfpraw->start - qfpbase->start;
>>>> + priv->qfpraw_end = qfpraw->end - qfpbase->start;
>>>> +
>>>> + qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>>> +
>>>> + priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>>>> + if (IS_ERR(priv->qfpmap)) {
>>>> + ret = PTR_ERR(priv->qfpmap);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>>
>>> I see no reference to this regulator in dt bindings.
>>
>> This perameter kept in board specific file i.e., sc7180-idp.dts file
>>
>>>> + if (IS_ERR(priv->vcc)) {
>>>> + ret = PTR_ERR(priv->vcc);
>>>> + if (ret == -ENODEV)
>>>> + ret = -EPROBE_DEFER;
>>> Can you explain what is going on here?
>>>
>>
>> As i took other drivers reference, i have kept this check.
>>
>>>> +
>>>> + goto err;
>>>> + }
>>>> +
>>>> + priv->secclk = devm_clk_get(dev, "secclk");
>>>> + if (IS_ERR(priv->secclk)) {
>>>> + ret = PTR_ERR(priv->secclk);
>>>> + if (ret != -EPROBE_DEFER)
>>>> + dev_err(dev, "secclk error getting : %d\n", ret);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + ret = clk_prepare_enable(priv->secclk);
>>>> + if (ret) {
>>>> + dev_err(dev, "clk_prepare_enable() failed\n");
>>>> + goto err;
>>>> + }
>>>> +
>>>> + econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>>>> + if (!econfig)
>>> Why not disabling the clk here?
>>>> + return -ENOMEM;
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> + econfig->dev = dev;
>>>> + econfig->name = "qfprom-efuse";
>>>> + econfig->stride = 1;
>>>> + econfig->word_size = 1;
>>>> + econfig->reg_read = qfprom_efuse_reg_read;
>>>> + econfig->reg_write = qfprom_efuse_reg_write;
>>>> + econfig->size = resource_size(qfpraw);
>>>> + econfig->priv = priv;
>>>> +
>>>> + nvmem = devm_nvmem_register(dev, econfig);
>>>> +
>>>> + return PTR_ERR_OR_ZERO(nvmem);
>>> probably you should check the nvmem here before returning to disable
>>> the clk properly.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +err:
>>>> + clk_disable_unprepare(priv->secclk);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct qfprom_efuse_platform_data
>>>> sc7180_qfp_efuse_data = {
>>>> + .name = "sc7180-qfprom-efuse",
>>> Redundant.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> + .fuse_blow_time_in_us = 10,
>>>> + .accel_value = 0xD10,
>>>> + .accel_reset_value = 0x800,
>>>> + .qfprom_blow_timer_value = 25,
>>>> + .qfprom_blow_reset_freq = 19200000,
>>>> + .qfprom_blow_set_freq = 4800000,
>>>> + .qfprom_max_vol = 1904000,
>>>> + .qfprom_min_vol = 1800000,
>>>> +};
>>>> +
>>>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>>>> + {
>>>> + .compatible = "qcom,sc7180-qfprom-efuse",
>>>> + .data = &sc7180_qfp_efuse_data
>>>> + },
>>>> + {/* sentinel */},
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>>>> +
>>>> +static struct platform_driver qfprom_efuse_driver = {
>>>> + .probe = qfprom_efuse_probe,
>>>> + .driver = {
>>>> + .name = "sc7180-qfprom-efuse",
>>>> + .of_match_table = qfprom_efuse_of_match,
>>>> + },
>>>> +};
>>>> +
>>>> +module_platform_driver(qfprom_efuse_driver);
>>>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>
Regards,
Ravi Kumar.B
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by the Linux Foundation.
^ permalink raw reply
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Rafael J. Wysocki @ 2020-05-18 10:41 UTC (permalink / raw)
To: Serge Semin
Cc: Viresh Kumar, Serge Semin, Rafael J. Wysocki, Thomas Bogendoerfer,
Ulf Hansson, Matthias Kaehlcke, Alexey Malahov, Paul Burton,
Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips, devicetree,
stable, Frederic Weisbecker, Ingo Molnar, Yue Hu, linux-pm,
linux-kernel
In-Reply-To: <20200518103102.t3a3g4uxeeuwsnix@mobilestation>
On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > the last break in the loop.
> > > >
> > > > That can be done as well, but will be a bit less efficient as the loop
> > > > will execute once for each policy, and so the statement will run
> > > > multiple times. Though it isn't going to add any significant latency
> > > > in the code.
> > >
> > > Right.
> > >
> > > However, the logic in this entire function looks somewhat less than
> > > straightforward to me, because it looks like it should return an
> > > error on the first policy without a frequency table (having a frequency
> > > table depends on the driver and that is the same for all policies, so it
> > > is pointless to iterate any further in that case).
> > >
> > > Also, the error should not be -EINVAL, because that means "invalid
> > > argument" which would be the state value.
> > >
> > > So I would do something like this:
> > >
> > > ---
> > > drivers/cpufreq/cpufreq.c | 11 ++++++-----
> > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > static int cpufreq_boost_set_sw(int state)
> > > {
> > > struct cpufreq_policy *policy;
> > > - int ret = -EINVAL;
> > >
> > > for_each_active_policy(policy) {
> > > + int ret;
> > > +
> > > if (!policy->freq_table)
> > > - continue;
> > > + return -ENXIO;
> > >
> > > ret = cpufreq_frequency_table_cpuinfo(policy,
> > > policy->freq_table);
> > > if (ret) {
> > > pr_err("%s: Policy frequency update failed\n",
> > > __func__);
> > > - break;
> > > + return ret;
> > > }
> > >
> > > ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > if (ret < 0)
> > > - break;
> > > + return ret;
> > > }
> > >
> > > - return ret;
> > > + return 0;
> > > }
> > >
> > > int cpufreq_boost_trigger_state(int state)
> >
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> suggests or you'll merge the Rafael's fix in yourself?
I'll apply the fix directly, thanks!
^ permalink raw reply
* Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support
From: Srinivas Kandagatla @ 2020-05-18 10:44 UTC (permalink / raw)
To: Ravi Kumar Bokka (Temp), Rob Herring
Cc: linux-kernel, devicetree, rnayak, saiprakash.ranjan, dhavalp,
mturney, sparate, c_rbokka, mkurumel, dianders
In-Reply-To: <b80aaca0-0594-e04b-5320-b5b3c4478161@codeaurora.org>
On 18/05/2020 11:39, Ravi Kumar Bokka (Temp) wrote:
>>
>
> Based on the compatible, do i need to separate probe function for
> qfprom-efuse and maintain separate nvmem object to register nvmem
> framework. Is this what you are suggesting to implementing this in to
> one existing driver?
Yes for same driver we should add new compatible string and add support
to this in existing qfprom driver.
Ideally we should allocate nvmem_config object at probe with different
parameters based on compatible string.
> Do I need to maintain separate efuse dt node?
Not sure what you mean this w.r.t driver, but based on compatible string
the device tree bindings might vary like clocks, regulators and so on.
--srini
>
> Could you please suggest me to proceed further.
^ permalink raw reply
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Serge Semin @ 2020-05-18 10:46 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Serge Semin, Viresh Kumar, Rafael J. Wysocki, Thomas Bogendoerfer,
Ulf Hansson, Matthias Kaehlcke, Alexey Malahov, Paul Burton,
Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips, devicetree,
stable, Frederic Weisbecker, Ingo Molnar, Yue Hu, linux-pm,
linux-kernel
In-Reply-To: <5284478.EF2IWm2iUs@kreacher>
On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
> On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > > the last break in the loop.
> > > > >
> > > > > That can be done as well, but will be a bit less efficient as the loop
> > > > > will execute once for each policy, and so the statement will run
> > > > > multiple times. Though it isn't going to add any significant latency
> > > > > in the code.
> > > >
> > > > Right.
> > > >
> > > > However, the logic in this entire function looks somewhat less than
> > > > straightforward to me, because it looks like it should return an
> > > > error on the first policy without a frequency table (having a frequency
> > > > table depends on the driver and that is the same for all policies, so it
> > > > is pointless to iterate any further in that case).
> > > >
> > > > Also, the error should not be -EINVAL, because that means "invalid
> > > > argument" which would be the state value.
> > > >
> > > > So I would do something like this:
> > > >
> > > > ---
> > > > drivers/cpufreq/cpufreq.c | 11 ++++++-----
> > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > > static int cpufreq_boost_set_sw(int state)
> > > > {
> > > > struct cpufreq_policy *policy;
> > > > - int ret = -EINVAL;
> > > >
> > > > for_each_active_policy(policy) {
> > > > + int ret;
> > > > +
> > > > if (!policy->freq_table)
> > > > - continue;
> > > > + return -ENXIO;
> > > >
> > > > ret = cpufreq_frequency_table_cpuinfo(policy,
> > > > policy->freq_table);
> > > > if (ret) {
> > > > pr_err("%s: Policy frequency update failed\n",
> > > > __func__);
> > > > - break;
> > > > + return ret;
> > > > }
> > > >
> > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > > if (ret < 0)
> > > > - break;
> > > > + return ret;
> > > > }
> > > >
> > > > - return ret;
> > > > + return 0;
> > > > }
> > > >
> > > > int cpufreq_boost_trigger_state(int state)
> > >
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >
> > Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> > suggests or you'll merge the Rafael's fix in yourself?
>
> I'll apply the fix directly, thanks!
Great. Is it going to be available in the repo:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
?
I'll need it to back port into my local kernel tree. Thanks.
-Sergey
>
>
>
^ permalink raw reply
* Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting
From: Rafael J. Wysocki @ 2020-05-18 10:51 UTC (permalink / raw)
To: Serge Semin
Cc: Rafael J. Wysocki, Serge Semin, Viresh Kumar, Rafael J. Wysocki,
Thomas Bogendoerfer, Ulf Hansson, Matthias Kaehlcke,
Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann,
Rob Herring, linux-mips, devicetree@vger.kernel.org, Stable,
Frederic Weisbecker, Ingo Molnar, Yue Hu, Linux PM,
Linux Kernel Mailing List
In-Reply-To: <20200518104602.mjh2p5iltf2x4wmq@mobilestation>
On Mon, May 18, 2020 at 12:46 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
> > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > > > the last break in the loop.
> > > > > >
> > > > > > That can be done as well, but will be a bit less efficient as the loop
> > > > > > will execute once for each policy, and so the statement will run
> > > > > > multiple times. Though it isn't going to add any significant latency
> > > > > > in the code.
> > > > >
> > > > > Right.
> > > > >
> > > > > However, the logic in this entire function looks somewhat less than
> > > > > straightforward to me, because it looks like it should return an
> > > > > error on the first policy without a frequency table (having a frequency
> > > > > table depends on the driver and that is the same for all policies, so it
> > > > > is pointless to iterate any further in that case).
> > > > >
> > > > > Also, the error should not be -EINVAL, because that means "invalid
> > > > > argument" which would be the state value.
> > > > >
> > > > > So I would do something like this:
> > > > >
> > > > > ---
> > > > > drivers/cpufreq/cpufreq.c | 11 ++++++-----
> > > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > > >
> > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > > > ===================================================================
> > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > > > static int cpufreq_boost_set_sw(int state)
> > > > > {
> > > > > struct cpufreq_policy *policy;
> > > > > - int ret = -EINVAL;
> > > > >
> > > > > for_each_active_policy(policy) {
> > > > > + int ret;
> > > > > +
> > > > > if (!policy->freq_table)
> > > > > - continue;
> > > > > + return -ENXIO;
> > > > >
> > > > > ret = cpufreq_frequency_table_cpuinfo(policy,
> > > > > policy->freq_table);
> > > > > if (ret) {
> > > > > pr_err("%s: Policy frequency update failed\n",
> > > > > __func__);
> > > > > - break;
> > > > > + return ret;
> > > > > }
> > > > >
> > > > > ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > > > if (ret < 0)
> > > > > - break;
> > > > > + return ret;
> > > > > }
> > > > >
> > > > > - return ret;
> > > > > + return 0;
> > > > > }
> > > > >
> > > > > int cpufreq_boost_trigger_state(int state)
> > > >
> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >
> > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> > > suggests or you'll merge the Rafael's fix in yourself?
> >
> > I'll apply the fix directly, thanks!
>
> Great. Is it going to be available in the repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> ?
Yes, it is. Please see the bleeding-edge branch in there, thanks!
^ permalink raw reply
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