* Re: [PATCH V8 2/2] mailbox: introduce ARM SMC based mailbox
From: kbuild test robot @ 2019-09-24 3:20 UTC (permalink / raw)
To: Peng Fan
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, Peng Fan,
f.fainelli@gmail.com, andre.przywara@arm.com,
jassisinghbrar@gmail.com, linux-kernel@vger.kernel.org,
robh+dt@kernel.org, kbuild-all, sudeep.holla@arm.com,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <1569287538-10854-3-git-send-email-peng.fan@nxp.com>
[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]
Hi Peng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190920]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Peng-Fan/mailbox-arm-introduce-smc-triggered-mailbox/20190924-091652
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from <command-line>:0:0:
>> include/linux/mailbox/arm-smccc-mbox.h:17:3: error: unknown type name 'u32'
u32 args_smccc32[6];
^~~
>> include/linux/mailbox/arm-smccc-mbox.h:18:3: error: unknown type name 'u64'
u64 args_smccc64[6];
^~~
vim +/u32 +17 include/linux/mailbox/arm-smccc-mbox.h
5
6 /**
7 * struct arm_smccc_mbox_cmd - ARM SMCCC message structure
8 * @function_id: function id passed from client, If mbox
9 * DT has arm,func-id property, the driver will use
10 * that one.
11 * @args_smccc32/64: actual usage of registers is up to the protocol
12 * (within the SMCCC limits)
13 */
14 struct arm_smccc_mbox_cmd {
15 unsigned int function_id;
16 union {
> 17 u32 args_smccc32[6];
> 18 u64 args_smccc64[6];
19 };
20 };
21
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52171 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2] usb: dwc3: enable otg mode for dwc3 usb ip on layerscape
From: Yinbo Zhu @ 2019-09-24 3:29 UTC (permalink / raw)
To: Shawn Guo, Li Yang, Rob Herring, Mark Rutland
Cc: devicetree, xiaobo.xie, linux-kernel, jiafei.pan, Ran Wang,
yinbo.zhu, linux-arm-kernel
layerscape otg function should be supported HNP SRP and ADP protocol
accroing to rm doc, but dwc3 code not realize it and use id pin to
detect who is host or device(0 is host 1 is device) this patch is to
enable OTG mode on ls1028ardb ls1088ardb and ls1046ardb in dts
Signed-off-by: Yinbo Zhu <yinbo.zhu@nxp.com>
---
arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts | 4 ++++
arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts | 4 ++++
arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts | 1 +
3 files changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
index 9fb9113..076cac6 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
@@ -171,3 +171,7 @@
&sata {
status = "okay";
};
+
+&usb1 {
+ dr_mode = "otg";
+};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
index 6a6514d..0c742be 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
@@ -122,6 +122,10 @@
};
};
+&usb1 {
+ dr_mode = "otg";
+};
+
#include "fsl-ls1046-post.dtsi"
&fman0 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts
index 8e925df..90b1989 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts
@@ -95,5 +95,6 @@
};
&usb1 {
+ dr_mode = "otg";
status = "okay";
};
--
2.9.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 1/1] PCI: iproc: Invalidate PAXB address mapping before programming it
From: Abhishek Shah @ 2019-09-24 3:28 UTC (permalink / raw)
To: Andrew Murray
Cc: Lorenzo Pieralisi, Scott Branden, Ray Jui, open list,
BCM Kernel Feedback, linux-pci, Bjorn Helgaas, linux-arm-kernel
In-Reply-To: <CAKUFe6aHGM0qHXcwopVfv_6+ALA=zmtBzSwNUO6qg8OEG-h_Ww@mail.gmail.com>
Hi Bjorn/Lorenzo,
Can you please help review this patch?
Regards,
Abhishek
On Fri, Sep 6, 2019 at 7:41 PM Abhishek Shah <abhishek.shah@broadcom.com> wrote:
>
> Hi Andrew,
>
>
> On Fri, Sep 6, 2019 at 3:31 PM Andrew Murray <andrew.murray@arm.com> wrote:
> >
> > On Fri, Sep 06, 2019 at 02:55:19PM +0530, Abhishek Shah wrote:
> > > Hi Andrew,
> > >
> > > Thanks for the review. Please see my response inline:
> > >
> > > On Fri, Sep 6, 2019 at 2:08 PM Andrew Murray <andrew.murray@arm.com> wrote:
> > > >
> > > > On Fri, Sep 06, 2019 at 09:28:13AM +0530, Abhishek Shah wrote:
> > > > > Invalidate PAXB inbound/outbound address mapping each time before
> > > > > programming it. This is helpful for the cases where we need to
> > > > > reprogram inbound/outbound address mapping without resetting PAXB.
> > > > > kexec kernel is one such example.
> > > >
> > > > Why is this approach better than resetting the PAXB (I assume that's
> > > > the PCI controller IP)? Wouldn't resetting the PAXB address this issue,
> > > > and ensure that no other configuration is left behind?
> > > >
> > > We normally reset PAXB in the firmware(ATF). But for cases like kexec
> > > kernel boot,
> > > we do not execute any firmware code and directly boot into kernel.
> > >
> > > We could have done PAXB reset in the driver itself as you have suggested here.
> > > But note that this detail could vary for each SoC, because these
> > > registers are not part
> > > of PAXB register space itself, rather exists in a register space responsible for
> > > controlling power to various wrappers in PCIe IP. Normally, this kind
> > > of SoC specific
> > > details are handled in firmware itself, we don't bring them to driver level.
> >
> > OK understood.
> >
> > >
> > > > Or is this related to earlier boot stages loading firmware for the emulated
> > > > downstream endpoints (ep_is_internal)?
> > > >
> > > > Finally, in the case where ep_is_internal do you need to disable anything
> > > > prior to invalidating the mappings?
> > > >
> > > No, ep_is_internal is indicator for PAXC IP. It does not have
> > > mappings as in PAXB.
> >
> > I think I meant !ep_is_internal. I.e. is there possibility of inbound traffic
> > prior to invalidating the mappings. I'd assume not, but that's an assumption.
> >
> No, EP devices are not even enumerated yet.
>
> > Either way:
> >
> > Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> >
> > >
> > >
> > > Regards,
> > > Abhishek
> > > > >
> > > > > Signed-off-by: Abhishek Shah <abhishek.shah@broadcom.com>
> > > > > Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> > > > > Reviewed-by: Vikram Mysore Prakash <vikram.prakash@broadcom.com>
> > > > > ---
> > > > > drivers/pci/controller/pcie-iproc.c | 28 ++++++++++++++++++++++++++++
> > > > > 1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c
> > > > > index e3ca46497470..99a9521ba7ab 100644
> > > > > --- a/drivers/pci/controller/pcie-iproc.c
> > > > > +++ b/drivers/pci/controller/pcie-iproc.c
> > > > > @@ -1245,6 +1245,32 @@ static int iproc_pcie_map_dma_ranges(struct iproc_pcie *pcie)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +static void iproc_pcie_invalidate_mapping(struct iproc_pcie *pcie)
> > > > > +{
> > > > > + struct iproc_pcie_ib *ib = &pcie->ib;
> > > > > + struct iproc_pcie_ob *ob = &pcie->ob;
> > > > > + int idx;
> > > > > +
> > > > > + if (pcie->ep_is_internal)
> > > > > + return;
> > > > > +
> > > > > + if (pcie->need_ob_cfg) {
> > > > > + /* iterate through all OARR mapping regions */
> > > > > + for (idx = ob->nr_windows - 1; idx >= 0; idx--) {
> > > > > + iproc_pcie_write_reg(pcie,
> > > > > + MAP_REG(IPROC_PCIE_OARR0, idx), 0);
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + if (pcie->need_ib_cfg) {
> > > > > + /* iterate through all IARR mapping regions */
> > > > > + for (idx = 0; idx < ib->nr_regions; idx++) {
> > > > > + iproc_pcie_write_reg(pcie,
> > > > > + MAP_REG(IPROC_PCIE_IARR0, idx), 0);
> > > > > + }
> > > > > + }
> > > > > +}
> > > > > +
> > > > > static int iproce_pcie_get_msi(struct iproc_pcie *pcie,
> > > > > struct device_node *msi_node,
> > > > > u64 *msi_addr)
> > > > > @@ -1517,6 +1543,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> > > > > iproc_pcie_perst_ctrl(pcie, true);
> > > > > iproc_pcie_perst_ctrl(pcie, false);
> > > > >
> > > > > + iproc_pcie_invalidate_mapping(pcie);
> > > > > +
> > > > > if (pcie->need_ob_cfg) {
> > > > > ret = iproc_pcie_map_ranges(pcie, res);
> > > > > if (ret) {
> > > >
> > > > The code changes look good to me.
> > > >
> > > > Thanks,
> > > >
> > > > Andrew Murray
> > > >
> > > > > --
> > > > > 2.17.1
> > > > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v2] usb: dwc3: enable otg mode for dwc3 usb ip on layerscape
From: Yinbo Zhu @ 2019-09-24 3:30 UTC (permalink / raw)
To: Yinbo Zhu, Shawn Guo, Leo Li, Rob Herring, Mark Rutland
Cc: devicetree@vger.kernel.org, Xiaobo Xie,
linux-kernel@vger.kernel.org, Jiafei Pan, Ran Wang,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190924032903.32775-1-yinbo.zhu@nxp.com>
Hi,
Change in v2:
move the otg property from dtsi to dts.
Regards,
Yinbo Zhu.
-----Original Message-----
From: Yinbo Zhu <yinbo.zhu@nxp.com>
Sent: 2019年9月24日 11:29
To: Shawn Guo <shawnguo@kernel.org>; Leo Li <leoyang.li@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
Cc: Yinbo Zhu <yinbo.zhu@nxp.com>; Xiaobo Xie <xiaobo.xie@nxp.com>; Jiafei Pan <jiafei.pan@nxp.com>; Ran Wang <ran.wang_1@nxp.com>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [PATCH v2] usb: dwc3: enable otg mode for dwc3 usb ip on layerscape
layerscape otg function should be supported HNP SRP and ADP protocol accroing to rm doc, but dwc3 code not realize it and use id pin to detect who is host or device(0 is host 1 is device) this patch is to enable OTG mode on ls1028ardb ls1088ardb and ls1046ardb in dts
Signed-off-by: Yinbo Zhu <yinbo.zhu@nxp.com>
---
arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts | 4 ++++ arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts | 4 ++++ arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts | 1 +
3 files changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
index 9fb9113..076cac6 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
@@ -171,3 +171,7 @@
&sata {
status = "okay";
};
+
+&usb1 {
+ dr_mode = "otg";
+};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
index 6a6514d..0c742be 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
@@ -122,6 +122,10 @@
};
};
+&usb1 {
+ dr_mode = "otg";
+};
+
#include "fsl-ls1046-post.dtsi"
&fman0 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts
index 8e925df..90b1989 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a-rdb.dts
@@ -95,5 +95,6 @@
};
&usb1 {
+ dr_mode = "otg";
status = "okay";
};
--
2.9.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* RE: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent
From: Leo Li @ 2019-09-24 3:41 UTC (permalink / raw)
To: Ulf Hansson, Russell King
Cc: Mark Rutland, DTML, dann frazier, Will Deacon, Nicolin Chen,
Rob Herring, Y.b. Lu, Shawn Guo, Robin Murphy, Christoph Hellwig,
Linux ARM
In-Reply-To: <CAPDyKFpQn+O-M60q-Tc4QOBM+WhFhmnkU6_-t5hnQcndLo==YA@mail.gmail.com>
> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Monday, September 23, 2019 4:27 PM
> To: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Robin Murphy <robin.murphy@arm.com>; dann frazier
> <dann.frazier@canonical.com>; Will Deacon <will.deacon@arm.com>;
> Nicolin Chen <nicoleotsuka@gmail.com>; Y.b. Lu <yangbo.lu@nxp.com>;
> Christoph Hellwig <hch@lst.de>; Mark Rutland <mark.rutland@arm.com>;
> DTML <devicetree@vger.kernel.org>; Leo Li <leoyang.li@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Linux
> ARM <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma
> coherent
>
> On Sun, 22 Sep 2019 at 12:29, Russell King <rmk+kernel@armlinux.org.uk>
> wrote:
> >
> > The LX2160A esdhc controllers are setup by the driver to be DMA
> > coherent, but without marking them as such in DT, Linux thinks they
> > are not. This can lead to random sporadic DMA errors, even to the
> > extent of preventing boot, such as:
> >
> > mmc0: ADMA error
> > mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
> > mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
> > mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
> > mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
> > mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
> > mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
> > mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
> > mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
> > mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
> > mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
> > mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
> > mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
> > mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
> > mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
> > mmc0: sdhci: Host ctl2: 0x00000000
> > mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
> > mmc0: sdhci: ============================================
> > mmc0: error -5 whilst initialising SD card
> >
> > These are caused by the device's descriptor fetch hitting
> > speculatively loaded CPU cache lines that the CPU does not see through
> > the normal, non-cacheable DMA coherent mapping that it uses for
> > non-coherent devices.
> >
> > DT and the device must agree wrt whether the device is DMA coherent or
> > not.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> As I am picking up patch1 and patch2 from this series, I can also help out and
> pick up this one, if that is okay by people?
There is some concern recently from linux-next maintainer about this causing potential conflicts. https://lkml.org/lkml/2019/9/15/225
Regards,
Leo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] soc: ti: ti_sci_pm_domains: Store device id in platform device
From: Lokesh Vutla @ 2019-09-24 4:45 UTC (permalink / raw)
To: Tero Kristo, Nishanth Menon, Santosh Shilimkar
Cc: Sekhar Nori, Linux ARM Mailing List
In-Reply-To: <ff2919a1-1330-31e2-7c96-ecfb0c727f7d@ti.com>
Hi Tero,
On 23/09/19 12:07 PM, Tero Kristo wrote:
> On 23/09/2019 06:34, Lokesh Vutla wrote:
>> Device ID that is passed from power-domains is used by peripheral
>> drivers for communicating with sysfw. Instead of individual drivers
>> traversing power-domains entry in DT node, store the device ID in
>> platform_device so that drivers can directly access it.
>
> Uhm, isn't this kind of wrong place to allocate the ID? The power domain
I do agree that this might not be a right place, but I couldn't find a better
place to populate id. Below is the flow on how platform_device gets created.
of_platform_default_populate_init
->of_platform_default_populate
-> of_platform_populate
->of_platform_bus_create
-> of_platform_device_create_pdata
-> of_device_alloc
-> platform_device_alloc("", PLATFORM_DEVID_NONE);
At this point platform_device gets created with dummy device_id. Also there are
no hooks to add custom device_ids.
> solution itself is a client also. In theory, someone could access the pdev->id
Nope, this is done in dev_pm_domain_attach which is called before driver probe
in platform_drv_probe().
> before this. pdev->id should be assigned by bus driver so that it can be
> properly handled within platform_device_add.
DT doesn't provide any such facility for populating device_add. I am open for
any suggestions :)
Thanks and regards,
Lokesh
>
> -Tero
>
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>> drivers/soc/ti/ti_sci_pm_domains.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/soc/ti/ti_sci_pm_domains.c
>> b/drivers/soc/ti/ti_sci_pm_domains.c
>> index 8c2a2f23982c..a124ac409124 100644
>> --- a/drivers/soc/ti/ti_sci_pm_domains.c
>> +++ b/drivers/soc/ti/ti_sci_pm_domains.c
>> @@ -116,6 +116,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain
>> *domain,
>> struct of_phandle_args pd_args;
>> struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain);
>> const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci;
>> + struct platform_device *pdev = to_platform_device(dev);
>> struct ti_sci_genpd_dev_data *sci_dev_data;
>> struct generic_pm_domain_data *genpd_data;
>> int idx, ret = 0;
>> @@ -129,6 +130,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain
>> *domain,
>> return -EINVAL;
>> idx = pd_args.args[0];
>> + pdev->id = idx;
>> /*
>> * Check the validity of the requested idx, if the index is not valid
>>
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 4/6] PM / devfreq: Introduce devfreq_get_freq_range
From: Chanwoo Choi @ 2019-09-24 5:13 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham, Kyungmin Park, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan, linux-pm,
Viresh Kumar, NXP Linux Team, Krzysztof Kozlowski, Lukasz Luba,
Alexandre Bailon, Martin Kepplinger, Georgi Djakov,
linux-arm-kernel, Jacky Bai
In-Reply-To: <15e599188c5b50edad3efc37787dc2b70d6efead.1569272883.git.leonard.crestez@nxp.com>
Hi,
On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Moving handling of min/max freq to a single function and call it from
> update_devfreq and for printing min/max freq values in sysfs.
>
> This changes the behavior of out-of-range min_freq/max_freq: clamping
> is now done at evaluation time. This means that if an out-of-range
> constraint is imposed by sysfs and it later becomes valid then it will
> be enforced.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/devfreq.c | 111 +++++++++++++++++++++-----------------
> 1 file changed, 63 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1cec816d3d00..7f152a582e78 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -24,10 +24,12 @@
> #include <linux/printk.h>
> #include <linux/hrtimer.h>
> #include <linux/of.h>
> #include "governor.h"
>
> +#define HZ_PER_KHZ 1000
It is not used on this patch. If it is used on later patch,
move it to other patch.
> +
> #define CREATE_TRACE_POINTS
> #include <trace/events/devfreq.h>
>
> static struct class *devfreq_class;
>
> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
> dev_pm_opp_put(opp);
>
> return max_freq;
> }
>
> +/**
> + * devfreq_get_freq_range() - Get the current freq range
> + * @devfreq: the devfreq instance
> + * @min_freq: the min frequency
> + * @max_freq: the max frequency
> + *
> + * This takes into consideration all constraints.
> + */
> +static void devfreq_get_freq_range(struct devfreq *devfreq,
This function is used in the only devfreq core.
I think that the internal function doesn't need to add 'devfreq' prefix.
Also, when I developed the patches, I didn't use 'devfreq' prefix
for internal function.
> + unsigned long *min_freq,
> + unsigned long *max_freq)
> +{
> + unsigned long *freq_table = devfreq->profile->freq_table;
> +
> + lockdep_assert_held(&devfreq->lock);
> +
> + /* Init min/max frequency from freq table */
I think that the comments in the devfreq_get_freq_range(),
it is not necessary. But if you think that it is necessary,
please add more detailed description like as following:
/*
* The devfreq recommend that freq_table must be generated
* in ascending order but, some devfreq driver used the descending order
* for freq_table. In order to support all cases, check the order
* of freq_table and then initialize the min/max frequency from freq_table.
*/
> + if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
> + *min_freq = freq_table[0];
> + *max_freq = freq_table[devfreq->profile->max_state - 1];
> + } else {
> + *min_freq = freq_table[devfreq->profile->max_state - 1];
> + *max_freq = freq_table[0];
> + }
> +
> + /* constraints from sysfs */
ditto. Need to add more detailed comment by keeping the comment style of devfreq.
> + *min_freq = max(*min_freq, devfreq->min_freq);
> + *max_freq = min(*max_freq, devfreq->max_freq);
> +
> + /* constraints from OPP interface */
ditto.
> + *min_freq = max(*min_freq, devfreq->scaling_min_freq);
> + /* scaling_max_freq can be zero on error */
> + if (devfreq->scaling_max_freq)
> + *max_freq = min(*max_freq, devfreq->scaling_max_freq);
> +
> + /* max_freq takes precedence over min_freq */
It is not necessary. We can know that min_freq have to be
under max_freq without any comment.
> + if (*min_freq > *max_freq)
> + *min_freq = *max_freq;
> +}
> +
> /**
> * devfreq_get_freq_level() - Lookup freq_table for the frequency
> * @devfreq: the devfreq instance
> * @freq: the target frequency
> */
> @@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq)
>
> /* Reevaluate the proper frequency */
> err = devfreq->governor->get_target_freq(devfreq, &freq);
> if (err)
> return err;
> + devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
>
> - /*
> - * Adjust the frequency with user freq, QoS and available freq.
> - *
> - * List from the highest priority
> - * max_freq
> - * min_freq
> - */
> - max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
> - min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
> -
> + /* max freq takes priority over min freq */
Please remove it.
> if (freq < min_freq) {
> freq = min_freq;
> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> }
> if (freq > max_freq) {
> @@ -1278,40 +1312,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> ret = sscanf(buf, "%lu", &value);
> if (ret != 1)
> return -EINVAL;
>
> mutex_lock(&df->lock);
> -
> - if (value) {
> - if (value > df->max_freq) {
> - ret = -EINVAL;
> - goto unlock;
> - }
> - } else {
> - unsigned long *freq_table = df->profile->freq_table;
> -
> - /* Get minimum frequency according to sorting order */
> - if (freq_table[0] < freq_table[df->profile->max_state - 1])
> - value = freq_table[0];
> - else
> - value = freq_table[df->profile->max_state - 1];
> - }
> -> df->min_freq = value;> update_devfreq(df);
> - ret = count;
> -unlock:
> mutex_unlock(&df->lock);
> - return ret;
> +
> + return count;
> }
>
> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct devfreq *df = to_devfreq(dev);
> + unsigned long min_freq, max_freq;
>
> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> + mutex_lock(&df->lock);
> + devfreq_get_freq_range(df, &min_freq, &max_freq);
> + mutex_unlock(&df->lock);
> +
> + return sprintf(buf, "%lu\n", min_freq);
> }
>
> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> @@ -1323,40 +1345,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> if (ret != 1)
> return -EINVAL;
>
> mutex_lock(&df->lock);
>
> - if (value) {
> - if (value < df->min_freq) {
> - ret = -EINVAL;
> - goto unlock;
> - }
> - } else {
> - unsigned long *freq_table = df->profile->freq_table;
> -
> - /* Get maximum frequency according to sorting order */
> - if (freq_table[0] < freq_table[df->profile->max_state - 1])
> - value = freq_table[df->profile->max_state - 1];
> - else
> - value = freq_table[0];
> - }
> + /* Interpret zero as "don't care" */
Please remove this comment.
> + if (!value)
> + value = ULONG_MAX;
>
> df->max_freq = value;
> update_devfreq(df);
> - ret = count;
> -unlock:
> mutex_unlock(&df->lock);
> - return ret;
> +
> + return count;
> }
> static DEVICE_ATTR_RW(min_freq);
>
> static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct devfreq *df = to_devfreq(dev);
> + unsigned long min_freq, max_freq;
> +
> + mutex_lock(&df->lock);
> + devfreq_get_freq_range(df, &min_freq, &max_freq);
> + mutex_unlock(&df->lock);
>
> - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
> + return sprintf(buf, "%lu\n", max_freq);
> }
> static DEVICE_ATTR_RW(max_freq);
>
> static ssize_t available_frequencies_show(struct device *d,
> struct device_attribute *attr,
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: don't export unused return_address()
From: Masahiro Yamada @ 2019-09-24 5:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Thomas Gleixner, Enrico Weigelt, Russell King, linux-arm-kernel,
Linux Kernel Mailing List
In-Reply-To: <20190906154706.2449696-1-arnd@arndb.de>
On Sat, Sep 7, 2019 at 12:47 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Without the frame pointer enabled, return_address() is an inline
> function and does not need to be exported, as shown by this warning:
>
> WARNING: "return_address" [vmlinux] is a static EXPORT_SYMBOL_GPL
>
> Move the EXPORT_SYMBOL_GPL() into the #ifdef as well.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>
--
Best Regards
Masahiro Yamada
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH V7 0/2] mailbox: arm: introduce smc triggered mailbox
From: André Przywara @ 2019-09-24 5:15 UTC (permalink / raw)
To: Peng Fan, robh+dt@kernel.org, mark.rutland@arm.com,
jassisinghbrar@gmail.com, sudeep.holla@arm.com,
f.fainelli@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <1569220514-27903-1-git-send-email-peng.fan@nxp.com>
On 23/09/2019 07:36, Peng Fan wrote:
Hi Peng,
thanks for the update!
> From: Peng Fan <peng.fan@nxp.com>
>
> V7:
> Typo fix
> #mbox-cells changed to 0
> Add a new header file arm-smccc-mbox.h
> Use ARM_SMCCC_IS_64
>
> Andre,
> The function_id is still kept in arm_smccc_mbox_cmd, because arm,func-id
> property is optional, so clients could pass function_id to mbox driver.
Well, to be honest, this is the main thing I am opposing:
It should *not* be optional.
The controller driver DT node should *always* contain the function ID.
The reasons for that I explained in the other emails to Jassi:
We can't safely execute smc calls from the Linux kernel, unless we also
comply with the SMCCC standard. So we should not leave the choice of the
function ID to the mailbox client.
Also this much better separates the mailbox controller driver from the
client.
So I think we should reach an agreement here.
Cheers,
Andre
> V6:
> Switch to per-channel a mbox controller
> Drop arm,num-chans, transports, method
> Add arm,hvc-mbox compatible
> Fix smc/hvc args, drop client id and use correct type.
> https://patchwork.kernel.org/cover/11146641/
>
> V5:
> yaml fix
> https://patchwork.kernel.org/cover/11117741/
>
> V4:
> yaml fix for num-chans in patch 1/2.
> https://patchwork.kernel.org/cover/11116521/
>
> V3:
> Drop interrupt
> Introduce transports for mem/reg usage
> Add chan-id for mem usage
> Convert to yaml format
> https://patchwork.kernel.org/cover/11043541/
>
> V2:
> This is a modified version from Andre Przywara's patch series
> https://lore.kernel.org/patchwork/cover/812997/.
> The modification are mostly:
> Introduce arm,num-chans
> Introduce arm_smccc_mbox_cmd
> txdone_poll and txdone_irq are both set to false
> arm,func-ids are kept, but as an optional property.
> Rewords SCPI to SCMI, because I am trying SCMI over SMC, not SCPI.
> Introduce interrupts notification.
>
> [1] is a draft implementation of i.MX8MM SCMI ATF implementation that
> use smc as mailbox, power/clk is included, but only part of clk has been
> implemented to work with hardware, power domain only supports get name
> for now.
>
> The traditional Linux mailbox mechanism uses some kind of dedicated hardware
> IP to signal a condition to some other processing unit, typically a dedicated
> management processor.
> This mailbox feature is used for instance by the SCMI protocol to signal a
> request for some action to be taken by the management processor.
> However some SoCs does not have a dedicated management core to provide
> those services. In order to service TEE and to avoid linux shutdown
> power and clock that used by TEE, need let firmware to handle power
> and clock, the firmware here is ARM Trusted Firmware that could also
> run SCMI service.
>
> The existing SCMI implementation uses a rather flexible shared memory
> region to communicate commands and their parameters, it still requires a
> mailbox to actually trigger the action.
>
> This patch series provides a Linux mailbox compatible service which uses
> smc calls to invoke firmware code, for instance taking care of SCMI requests.
> The actual requests are still communicated using the standard SCMI way of
> shared memory regions, but a dedicated mailbox hardware IP can be replaced via
> this new driver.
>
> This simple driver uses the architected SMC calling convention to trigger
> firmware services, also allows for using "HVC" calls to call into hypervisors
> or firmware layers running in the EL2 exception level.
>
> Patch 1 contains the device tree binding documentation, patch 2 introduces
> the actual mailbox driver.
>
> Please note that this driver just provides a generic mailbox mechanism,
> It could support synchronous TX/RX, or synchronous TX with asynchronous
> RX. And while providing SCMI services was the reason for this exercise,
> this driver is in no way bound to this use case, but can be used generically
> where the OS wants to signal a mailbox condition to firmware or a
> hypervisor.
> Also the driver is in no way meant to replace any existing firmware
> interface, but actually to complement existing interfaces.
>
> [1] https://github.com/MrVan/arm-trusted-firmware/tree/scmi
>
>
>
> Peng Fan (2):
> dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
> mailbox: introduce ARM SMC based mailbox
>
> .../devicetree/bindings/mailbox/arm-smc.yaml | 95 ++++++++++++
> drivers/mailbox/Kconfig | 7 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/arm-smc-mailbox.c | 168 +++++++++++++++++++++
> 4 files changed, 272 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] seccomp: remove unused arg from secure_computing()
From: Christian Brauner @ 2019-09-24 6:19 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-s390, Will Drewry, Kees Cook, Parisc List, X86 ML, linux-um,
Oleg Nesterov, LKML, Andy Lutomirski, Thomas Gleixner,
linux-arm-kernel
In-Reply-To: <20190923193446.GL15355@zn.tnic>
On Mon, Sep 23, 2019 at 09:34:46PM +0200, Borislav Petkov wrote:
> On Mon, Sep 23, 2019 at 11:41:59AM -0700, Andy Lutomirski wrote:
> > On Mon, Sep 23, 2019 at 2:49 AM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Fri, Sep 20, 2019 at 03:19:09PM +0200, Christian Brauner wrote:
> > > > While touching seccomp code I realized that the struct seccomp_data
> > > > argument to secure_computing() seems to be unused by all current
> > > > callers. So let's remove it unless there is some subtlety I missed.
> > > > Note, I only tested this on x86.
> > >
> > > What was amluto thinking in
> > >
> > > 2f275de5d1ed ("seccomp: Add a seccomp_data parameter secure_computing()")
> >
> > IIRC there was a period of time in which x86 used secure_computing()
> > for normal syscalls, and it was a good deal faster to have the arch
> > code supply seccomp_data. x86 no longer works like this, and syscalls
> > aren't fast anymore ayway :(
>
> Uhuh, thanks Andy.
>
> Christian, pls add that piece of history to the commit message.
Yip, will do. Thanks!
Christian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] seccomp: remove unused arg from secure_computing()
From: Christian Brauner @ 2019-09-24 6:30 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-s390, Will Drewry, Kees Cook, Parisc List, X86 ML, linux-um,
LKML, Oleg Nesterov, Borislav Petkov, Thomas Gleixner,
linux-arm-kernel
In-Reply-To: <CALCETrU_fs_At-hTpr231kpaAd0z7xJN4ku-DvzhRU6cvcJA_w@mail.gmail.com>
On Mon, Sep 23, 2019 at 11:41:59AM -0700, Andy Lutomirski wrote:
> On Mon, Sep 23, 2019 at 2:49 AM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Fri, Sep 20, 2019 at 03:19:09PM +0200, Christian Brauner wrote:
> > > While touching seccomp code I realized that the struct seccomp_data
> > > argument to secure_computing() seems to be unused by all current
> > > callers. So let's remove it unless there is some subtlety I missed.
> > > Note, I only tested this on x86.
> >
> > What was amluto thinking in
> >
> > 2f275de5d1ed ("seccomp: Add a seccomp_data parameter secure_computing()")
>
> IIRC there was a period of time in which x86 used secure_computing()
> for normal syscalls, and it was a good deal faster to have the arch
> code supply seccomp_data. x86 no longer works like this, and syscalls
> aren't fast anymore ayway :(
I started looking at this and actually had a slightly bigger cleanup in
mind. It seems odd that we have secure_computing() and
__secure_computing(). Especially in the mips and x86 case. From what I
can tell they could both rely on secure_computing() and don't need
__secure_computing().
If I can make those changes, we can make __secure_computing() static and
have only a single function secure_computing() that is used by all
arches which would make this code simpler.
Apparenly mips once switched from secure_computing() to
__secure_computing() because of bpf and tracepoints. The last change to
this was:
commit 3d729deaf287c43e415c5d791c9ac8414dbeff70
Author: James Hogan <jhogan@kernel.org>
Date: Fri Aug 11 21:56:50 2017 +0100
MIPS: seccomp: Fix indirect syscall args
which references a broken samples/bpf/tracex5 test. But in the thread to
this last change Kees and others were less than sure that this makes
sense. So I'm not sure. Maybe I should just try and send it out...
Christian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v8 3/3] mm: fix double page fault on arm64 if PTE_AF is cleared
From: Justin He (Arm Technology China) @ 2019-09-24 6:43 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, Kaly Xin (Arm Technology China), Ralph Campbell,
Andrew Morton, Suzuki Poulose, Marc Zyngier, Anshuman Khandual,
linux-kernel@vger.kernel.org, Matthew Wilcox, linux-mm@kvack.org,
Jérôme Glisse, James Morse,
linux-arm-kernel@lists.infradead.org, Punit Agrawal,
hejianet@gmail.com, Thomas Gleixner, nd, Will Deacon,
Alex Van Brunt, Kirill A. Shutemov, Robin Murphy
In-Reply-To: <20190923170433.GE10192@arrakis.emea.arm.com>
Hi Catalin
Please see an important comment inline, thanks
> -----Original Message-----
> From: Catalin Marinas <catalin.marinas@arm.com>
> Sent: 2019年9月24日 1:05
> To: Justin He (Arm Technology China) <Justin.He@arm.com>
> Cc: Will Deacon <will@kernel.org>; Mark Rutland
> <Mark.Rutland@arm.com>; James Morse <James.Morse@arm.com>; Marc
> Zyngier <maz@kernel.org>; Matthew Wilcox <willy@infradead.org>; Kirill A.
> Shutemov <kirill.shutemov@linux.intel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> mm@kvack.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Punit
> Agrawal <punitagrawal@gmail.com>; Anshuman Khandual
> <Anshuman.Khandual@arm.com>; Alex Van Brunt
> <avanbrunt@nvidia.com>; Robin Murphy <Robin.Murphy@arm.com>;
> Thomas Gleixner <tglx@linutronix.de>; Andrew Morton <akpm@linux-
> foundation.org>; Jérôme Glisse <jglisse@redhat.com>; Ralph Campbell
> <rcampbell@nvidia.com>; hejianet@gmail.com; Kaly Xin (Arm Technology
> China) <Kaly.Xin@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v8 3/3] mm: fix double page fault on arm64 if PTE_AF
> is cleared
>
> On Sat, Sep 21, 2019 at 09:50:54PM +0800, Jia He wrote:
> > @@ -2151,21 +2163,53 @@ static inline void cow_user_page(struct page
> *dst, struct page *src, unsigned lo
> > * fails, we just zero-fill it. Live with it.
> > */
> > if (unlikely(!src)) {
> > - void *kaddr = kmap_atomic(dst);
> > - void __user *uaddr = (void __user *)(va & PAGE_MASK);
> > + void *kaddr;
> > + pte_t entry;
> > + void __user *uaddr = (void __user *)(addr & PAGE_MASK);
> >
> > + /* On architectures with software "accessed" bits, we would
> > + * take a double page fault, so mark it accessed here.
> > + */
>
> Nitpick: please follow the kernel coding style for multi-line comments
> (above and the for the rest of the patch):
>
> /*
> * Your multi-line comment.
> */
>
> > + if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte))
> {
> > + vmf->pte = pte_offset_map_lock(mm, vmf->pmd,
> addr,
> > + &vmf->ptl);
> > + if (likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> > + entry = pte_mkyoung(vmf->orig_pte);
> > + if (ptep_set_access_flags(vma, addr,
> > + vmf->pte, entry, 0))
> > + update_mmu_cache(vma, addr, vmf-
> >pte);
> > + } else {
> > + /* Other thread has already handled the
> fault
> > + * and we don't need to do anything. If it's
> > + * not the case, the fault will be triggered
> > + * again on the same address.
> > + */
> > + pte_unmap_unlock(vmf->pte, vmf->ptl);
> > + return false;
> > + }
> > + pte_unmap_unlock(vmf->pte, vmf->ptl);
> > + }
>
> Another nit, you could rewrite this block slightly to avoid too much
> indentation. Something like (untested):
>
> if (arch_faults_on_old_pte() && !pte_young(vmf->orig_pte))
> {
> vmf->pte = pte_offset_map_lock(mm, vmf->pmd,
> addr,
> &vmf->ptl);
> if (unlikely(!pte_same(*vmf->pte, vmf->orig_pte))) {
> /*
> * Other thread has already handled the fault
> * and we don't need to do anything. If it's
> * not the case, the fault will be triggered
> * again on the same address.
> */
> pte_unmap_unlock(vmf->pte, vmf->ptl);
> return false;
> }
> entry = pte_mkyoung(vmf->orig_pte);
> if (ptep_set_access_flags(vma, addr,
> vmf->pte, entry, 0))
> update_mmu_cache(vma, addr, vmf->pte);
> pte_unmap_unlock(vmf->pte, vmf->ptl);
> }
>
> > +
> > + kaddr = kmap_atomic(dst);
>
> Since you moved the kmap_atomic() here, could the above
> arch_faults_on_old_pte() run in a preemptible context? I suggested to
> add a WARN_ON in patch 2 to be sure.
Should I move kmap_atomic back to the original line? Thus, we can make sure
that arch_faults_on_old_pte() is in the context of preempt_disabled?
Otherwise, arch_faults_on_old_pte() may cause plenty of warning if I add
a WARN_ON in arch_faults_on_old_pte. I tested it when I enable the PREEMPT=y
on a ThunderX2 qemu guest.
--
Cheers,
Justin (Jia He)
>
> > /*
> > * This really shouldn't fail, because the page is there
> > * in the page tables. But it might just be unreadable,
> > * in which case we just give up and fill the result with
> > * zeroes.
> > */
> > - if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE))
> > + if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
> > + /* Give a warn in case there can be some obscure
> > + * use-case
> > + */
> > + WARN_ON_ONCE(1);
>
> That's more of a question for the mm guys: at this point we do the
> copying with the ptl released; is there anything else that could have
> made the pte old in the meantime? I think unuse_pte() is only called on
> anonymous vmas, so it shouldn't be the case here.
>
> > clear_page(kaddr);
> > + }
> > kunmap_atomic(kaddr);
> > flush_dcache_page(dst);
> > } else
> > - copy_user_highpage(dst, src, va, vma);
> > + copy_user_highpage(dst, src, addr, vma);
> > +
> > + return true;
> > }
>
> --
> Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v1] seccomp: simplify secure_computing()
From: Christian Brauner @ 2019-09-24 6:44 UTC (permalink / raw)
To: christian.brauner
Cc: linux-s390, wad, keescook, linux-parisc, x86, linux-um,
linux-kernel, oleg, Borislav Petkov, luto, tglx, linux-arm-kernel
In-Reply-To: <20190920131907.6886-1-christian.brauner@ubuntu.com>
Afaict, the struct seccomp_data argument to secure_computing() is unused
by all current callers. So let's remove it.
The argument was added in [1]. It was added because having the arch
supply the syscall arguments used to be faster than having it done by
secure_computing() (cf. Andy's comment in [2]). This is not true anymore
though.
/* References */
[1]: 2f275de5d1ed ("seccomp: Add a seccomp_data parameter secure_computing()")
[2]: https://lore.kernel.org/r/CALCETrU_fs_At-hTpr231kpaAd0z7xJN4ku-DvzhRU6cvcJA_w@mail.gmail.com
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Will Drewry <wad@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-parisc@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Cc: linux-um@lists.infradead.org
Cc: x86@kernel.org
---
/* v1 */
- Borislav Petkov <bp@alien8.de>:
- provide context for the arg addition to secure_computing() in the
commit message
/* v0 */
Link: https://lore.kernel.org/r/20190920131907.6886-1-christian.brauner@ubuntu.com
---
arch/arm/kernel/ptrace.c | 2 +-
arch/arm64/kernel/ptrace.c | 2 +-
arch/parisc/kernel/ptrace.c | 2 +-
arch/s390/kernel/ptrace.c | 4 ++--
arch/um/kernel/skas/syscall.c | 2 +-
arch/x86/entry/vsyscall/vsyscall_64.c | 2 +-
include/linux/seccomp.h | 6 +++---
7 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 324352787aea..b606cded90cd 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -923,7 +923,7 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
/* Do seccomp after ptrace; syscall may have changed. */
#ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
- if (secure_computing(NULL) == -1)
+ if (secure_computing() == -1)
return -1;
#else
/* XXX: remove this once OABI gets fixed */
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 3cf3b135027e..010a835302d3 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1816,7 +1816,7 @@ int syscall_trace_enter(struct pt_regs *regs)
}
/* Do the secure computing after ptrace; failures should be fast. */
- if (secure_computing(NULL) == -1)
+ if (secure_computing() == -1)
return -1;
if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index 9f6ff7bc06f9..f8c07dcbfb49 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -342,7 +342,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
}
/* Do the secure computing check after ptrace. */
- if (secure_computing(NULL) == -1)
+ if (secure_computing() == -1)
return -1;
#ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c
index ad71132374f0..ed80bdfbf5fe 100644
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@ -439,7 +439,7 @@ static int poke_user(struct task_struct *child, addr_t addr, addr_t data)
long arch_ptrace(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
{
- ptrace_area parea;
+ ptrace_area parea;
int copied, ret;
switch (request) {
@@ -856,7 +856,7 @@ asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
}
/* Do the secure computing check after ptrace. */
- if (secure_computing(NULL)) {
+ if (secure_computing()) {
/* seccomp failures shouldn't expose any additional code. */
return -1;
}
diff --git a/arch/um/kernel/skas/syscall.c b/arch/um/kernel/skas/syscall.c
index 44bb10785075..fc37259d5971 100644
--- a/arch/um/kernel/skas/syscall.c
+++ b/arch/um/kernel/skas/syscall.c
@@ -35,7 +35,7 @@ void handle_syscall(struct uml_pt_regs *r)
goto out;
/* Do the seccomp check after ptrace; failures should be fast. */
- if (secure_computing(NULL) == -1)
+ if (secure_computing() == -1)
goto out;
syscall = UPT_SYSCALL_NR(r);
diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
index e7c596dea947..b10cbf71a8cc 100644
--- a/arch/x86/entry/vsyscall/vsyscall_64.c
+++ b/arch/x86/entry/vsyscall/vsyscall_64.c
@@ -222,7 +222,7 @@ bool emulate_vsyscall(unsigned long error_code,
*/
regs->orig_ax = syscall_nr;
regs->ax = -ENOSYS;
- tmp = secure_computing(NULL);
+ tmp = secure_computing();
if ((!tmp && regs->orig_ax != syscall_nr) || regs->ip != address) {
warn_bad_vsyscall(KERN_DEBUG, regs,
"seccomp tried to change syscall nr or ip");
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 84868d37b35d..03583b6d1416 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -33,10 +33,10 @@ struct seccomp {
#ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
extern int __secure_computing(const struct seccomp_data *sd);
-static inline int secure_computing(const struct seccomp_data *sd)
+static inline int secure_computing(void)
{
if (unlikely(test_thread_flag(TIF_SECCOMP)))
- return __secure_computing(sd);
+ return __secure_computing(NULL);
return 0;
}
#else
@@ -59,7 +59,7 @@ struct seccomp { };
struct seccomp_filter { };
#ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
-static inline int secure_computing(struct seccomp_data *sd) { return 0; }
+static inline int secure_computing(void) { return 0; }
#else
static inline void secure_computing_strict(int this_syscall) { return; }
#endif
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v7 5/6] PM / devfreq: Add PM QoS support
From: Chanwoo Choi @ 2019-09-24 6:52 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham, Kyungmin Park, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan, linux-pm,
Viresh Kumar, NXP Linux Team, Krzysztof Kozlowski, Lukasz Luba,
Alexandre Bailon, Martin Kepplinger, Georgi Djakov,
linux-arm-kernel, Jacky Bai
In-Reply-To: <e9868310f9543b4f4a6c7bbe5d4d015da9a0e71d.1569272883.git.leonard.crestez@nxp.com>
Hi,
On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Register notifiers with the PM QoS framework in order to respond to
> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>
> No notifiers are added by this patch but PM QoS constraints can be
> imposed externally (for example from other devices).
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> drivers/devfreq/devfreq.c | 76 +++++++++++++++++++++++++++++++++++++++
> include/linux/devfreq.h | 5 +++
> 2 files changed, 81 insertions(+)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7f152a582e78..9887408f23bb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,17 +22,20 @@
> #include <linux/platform_device.h>
> #include <linux/list.h>
> #include <linux/printk.h>
> #include <linux/hrtimer.h>
> #include <linux/of.h>
> +#include <linux/pm_qos.h>
> #include "governor.h"
>
> #define HZ_PER_KHZ 1000
You have to remove it on patch4.
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/devfreq.h>
>
> +#define HZ_PER_KHZ 1000> +
> static struct class *devfreq_class;
>
> /*
> * devfreq core provides delayed work based load monitoring helper
> * functions. Governors can use these or can implement their own
> @@ -123,10 +126,16 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
> } else {
> *min_freq = freq_table[devfreq->profile->max_state - 1];
> *max_freq = freq_table[0];
> }
>
> + /* constraints from PM QoS */
I think that it is not necessary. But, if you think it is required,
Please add the detailed comment with consistent comment style.
> + *min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
> + devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
> + *max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
> + devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
If you use the separate variable for getting the value from dev_pm_qos_read_value(),
you can add this line under 80 char. If there are any special reason,
I prefer to keep the line under 80 char.
> +
> /* constraints from sysfs */
> *min_freq = max(*min_freq, devfreq->min_freq);
> *max_freq = min(*max_freq, devfreq->max_freq);
>
> /* constraints from OPP interface */
> @@ -605,10 +614,53 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
> mutex_unlock(&devfreq->lock);
>
> return ret;
> }
>
> +/**
> + * devfreq_qos_notifier_call() - Common handler for QoS constraints.
> + * @devfreq: the devfreq instance.
> + */
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
Also, as I commented on patch4, we better to remove 'devfreq' prefix
for internal function as following:
devfreq_qos_notifier_call -> qos_notifier_call
> +{
> + int err;
> +
> + mutex_lock(&devfreq->lock);
> + err = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (err)
> + dev_err(&devfreq->dev, "dvfs for QoS constraints"
> + " failed with (%d) error\n", err);
'dvfs' is not full name. Also, the capital letter is more correct.
But, the devfreq used 'failed to ...' comment style on some points.
I suggest the comment as following:
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -628,8 +628,8 @@ static int devfreq_qos_notifier_call(struct devfreq *devfreq)
err = update_devfreq(devfreq);
mutex_unlock(&devfreq->lock);
if (err)
- dev_err(&devfreq->dev, "dvfs for QoS constraints"
- " failed with (%d) error\n", err);
+ dev_err(&devfreq->dev,
+ "failed to update frequency with PMQoS (%d)\n", err);
> +
> + /* QoS is best effort - let all notifiers run on error */
Please remove it.
> + return NOTIFY_OK;
> +}
> +
> +/**
> + * devfreq_qos_min_notifier_call() - Callback for QoS min_freq changes.
> + * @nb: Should be devfreq->nb_min
> + */
> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
ditto.
devfreq_qos_min_notifier_call -> qos_min_notifier_call
> + unsigned long val, void *ptr)
> +{
> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
> +
nitpick. You can remove this line.
> + return devfreq_qos_notifier_call(devfreq);
> +}
> +
> +/**
> + * devfreq_qos_max_notifier_call() - Callback for QoS max_freq changes.
> + * @nb: Should be devfreq->nb_max
> + */
> +static int devfreq_qos_max_notifier_call(struct notifier_block *nb,
ditto.
devfreq_qos_max_notifier_call -> qos_max_notifier_call
> + unsigned long val, void *ptr)
> +{
> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
> +
nitpick. You can remove this line.
> + return devfreq_qos_notifier_call(devfreq);
> +}
> +
> /**
> * devfreq_dev_release() - Callback for struct device to release the device.
> * @dev: the devfreq device
> *
> * Remove devfreq from the list and release its resources.
> @@ -619,10 +671,15 @@ static void devfreq_dev_release(struct device *dev)
>
> mutex_lock(&devfreq_list_lock);
> list_del(&devfreq->node);
> mutex_unlock(&devfreq_list_lock);
>
> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> + DEV_PM_QOS_MAX_FREQUENCY);
> + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> + DEV_PM_QOS_MIN_FREQUENCY);
Even if devfreq_dev_release() is called at the end of device driver,
dev_pm_remove_notifier() have returned the value. Need to check
the return value for checking the error state.
> +
> if (devfreq->profile->exit)
> devfreq->profile->exit(devfreq->dev.parent);
>
> kfree(devfreq->time_in_state);
> kfree(devfreq->trans_table);
> @@ -732,10 +789,28 @@ struct devfreq *devfreq_add_device(struct device *dev,
> if (err) {
> put_device(&devfreq->dev);
> goto err_out;
> }
>
> + /*
> + * Register notifiers for updates to min/max_freq after device is
> + * initialized (and we can handle notifications) but before the
> + * governor is started (which should do an initial enforcement of
> + * constraints).> + */
In the devfreq_add_device(), each step has not contained
the detailed comment. If possible, in order to keep the existing style,
please remove it.
> + devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
> + DEV_PM_QOS_MIN_FREQUENCY);
> + if (err)
> + goto err_devfreq;
> +
> + devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
> + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
> + DEV_PM_QOS_MAX_FREQUENCY);
> + if (err)
> + goto err_devfreq;
> +
> mutex_lock(&devfreq_list_lock);
>
> governor = try_then_request_governor(devfreq->governor_name);
> if (IS_ERR(governor)) {
> dev_err(dev, "%s: Unable to find governor for the device\n",
> @@ -759,10 +834,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>
> return devfreq;
>
> err_init:
> mutex_unlock(&devfreq_list_lock);
> +err_devfreq:
> devfreq_remove_device(devfreq);
> return ERR_PTR(err);
>
> err_dev:
> /*
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2bae9ed3c783..8b92ccbd1962 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -134,10 +134,12 @@ struct devfreq_dev_profile {
> * @total_trans: Number of devfreq transitions
> * @trans_table: Statistics of devfreq transitions
> * @time_in_state: Statistics of devfreq states
> * @last_stat_updated: The last time stat updated
> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
> + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY
> + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY
> *
> * This structure stores the devfreq information for a give device.
> *
> * Note that when a governor accesses entries in struct devfreq in its
> * functions except for the context of callbacks defined in struct
> @@ -176,10 +178,13 @@ struct devfreq {
> unsigned int *trans_table;
> unsigned long *time_in_state;
> unsigned long last_stat_updated;
>
> struct srcu_notifier_head transition_notifier_list;
> +
> + struct notifier_block nb_min;
> + struct notifier_block nb_max;
> };
>
> struct devfreq_freqs {
> unsigned long old;
> unsigned long new;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFT v4 4/8] ARM: dts: exynos: Remove MCT subnode for interrupt map on Exynos4210
From: Marek Szyprowski @ 2019-09-24 6:57 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Mark Rutland, Kukjin Kim, Andrzej Hajda, linux-kernel, devicetree,
linux-arm-kernel, linux-samsung-soc
Cc: Sylwester Nawrocki
In-Reply-To: <20190923161411.9236-4-krzk@kernel.org>
On 23.09.2019 18:14, Krzysztof Kozlowski wrote:
> Multi Core Timer node has interrupts routed to two different parents -
> GIC and combiner. This was modeled with a interrupt-map within a
> subnode but can be expressed in an easier and more common way, directly
> in the node itself.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Works fine on Trats and Origen boards.
> ---
>
> Not tested.
>
> Changes since v3:
> 1. Use interrupts-extended instead of interrupts-map.
> ---
> arch/arm/boot/dts/exynos4210.dtsi | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index 6d3f19562aab..5fa33d43821e 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -109,23 +109,14 @@
> mct: timer@10050000 {
> compatible = "samsung,exynos4210-mct";
> reg = <0x10050000 0x800>;
> - interrupt-parent = <&mct_map>;
> - interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
> clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MCT>;
> clock-names = "fin_pll", "mct";
> -
> - mct_map: mct-map {
> - #interrupt-cells = <1>;
> - #address-cells = <0>;
> - #size-cells = <0>;
> - interrupt-map =
> - <0 &gic 0 57 IRQ_TYPE_LEVEL_HIGH>,
> - <1 &gic 0 69 IRQ_TYPE_LEVEL_HIGH>,
> - <2 &combiner 12 6>,
> - <3 &combiner 12 7>,
> - <4 &gic 0 42 IRQ_TYPE_LEVEL_HIGH>,
> - <5 &gic 0 48 IRQ_TYPE_LEVEL_HIGH>;
> - };
> + interrupts-extended = <&gic 0 57 IRQ_TYPE_LEVEL_HIGH>,
> + <&gic 0 69 IRQ_TYPE_LEVEL_HIGH>,
> + <&combiner 12 6>,
> + <&combiner 12 7>,
> + <&gic 0 42 IRQ_TYPE_LEVEL_HIGH>,
> + <&gic 0 48 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> watchdog: watchdog@10060000 {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFT v4 6/8] ARM: dts: exynos: Remove MCT subnode for interrupt map on Exynos5250
From: Marek Szyprowski @ 2019-09-24 6:58 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Mark Rutland, Kukjin Kim, Andrzej Hajda, linux-kernel, devicetree,
linux-arm-kernel, linux-samsung-soc
Cc: Sylwester Nawrocki
In-Reply-To: <20190923161411.9236-6-krzk@kernel.org>
On 23.09.2019 18:14, Krzysztof Kozlowski wrote:
> Multi Core Timer node has interrupts routed to two different parents -
> GIC and combiner. This was modeled with a interrupt-map within a
> subnode but can be expressed in an easier and more common way, directly
> in the node itself.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Works fine on Arndale and Snow boards.
>
> ---
>
> Not tested.
>
> Changes since v3:
> 1. Use interrupts-extended instead of interrupts-map.
> ---
> arch/arm/boot/dts/exynos5250.dtsi | 23 ++++++-----------------
> 1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 7a01349317a3..a549eafd2c64 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -236,25 +236,14 @@
> timer@101c0000 {
> compatible = "samsung,exynos4210-mct";
> reg = <0x101C0000 0x800>;
> - interrupt-controller;
> - #interrupt-cells = <2>;
> - interrupt-parent = <&mct_map>;
> - interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
> - <4 0>, <5 0>;
> clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MCT>;
> clock-names = "fin_pll", "mct";
> -
> - mct_map: mct-map {
> - #interrupt-cells = <2>;
> - #address-cells = <0>;
> - #size-cells = <0>;
> - interrupt-map = <0x0 0 &combiner 23 3>,
> - <0x1 0 &combiner 23 4>,
> - <0x2 0 &combiner 25 2>,
> - <0x3 0 &combiner 25 3>,
> - <0x4 0 &gic 0 120 IRQ_TYPE_LEVEL_HIGH>,
> - <0x5 0 &gic 0 121 IRQ_TYPE_LEVEL_HIGH>;
> - };
> + interrupts-extended = <&combiner 23 3>,
> + <&combiner 23 4>,
> + <&combiner 25 2>,
> + <&combiner 25 3>,
> + <&gic 0 120 IRQ_TYPE_LEVEL_HIGH>,
> + <&gic 0 121 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> pinctrl_0: pinctrl@11400000 {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFT v4 5/8] ARM: dts: exynos: Remove MCT subnode for interrupt map on Exynos4412
From: Marek Szyprowski @ 2019-09-24 6:58 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Mark Rutland, Kukjin Kim, Andrzej Hajda, linux-kernel, devicetree,
linux-arm-kernel, linux-samsung-soc
Cc: Sylwester Nawrocki
In-Reply-To: <20190923161411.9236-5-krzk@kernel.org>
On 23.09.2019 18:14, Krzysztof Kozlowski wrote:
> Multi Core Timer node has interrupts routed to two different parents -
> GIC and combiner. This was modeled with a interrupt-map within a
> subnode but can be expressed in an easier and more common way, directly
> in the node itself.
>
> Tested on Odroid U3 (Exynos4412).
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Works fine on Trats2 board.
> ---
>
> Changes since v3:
> 1. Use interrupts-extended instead of interrupts-map.
> ---
> arch/arm/boot/dts/exynos4412.dtsi | 19 +++++--------------
> 1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> index 8b6d5875c75d..9b5fb4e54d7c 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -246,22 +246,13 @@
> timer@10050000 {
> compatible = "samsung,exynos4412-mct";
> reg = <0x10050000 0x800>;
> - interrupt-parent = <&mct_map>;
> - interrupts = <0>, <1>, <2>, <3>, <4>;
> clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MCT>;
> clock-names = "fin_pll", "mct";
> -
> - mct_map: mct-map {
> - #interrupt-cells = <1>;
> - #address-cells = <0>;
> - #size-cells = <0>;
> - interrupt-map =
> - <0 &gic 0 57 IRQ_TYPE_LEVEL_HIGH>,
> - <1 &combiner 12 5>,
> - <2 &combiner 12 6>,
> - <3 &combiner 12 7>,
> - <4 &gic 1 12 IRQ_TYPE_LEVEL_HIGH>;
> - };
> + interrupts-extended = <&gic 0 57 IRQ_TYPE_LEVEL_HIGH>,
> + <&combiner 12 5>,
> + <&combiner 12 6>,
> + <&combiner 12 7>,
> + <&gic 1 12 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> watchdog: watchdog@10060000 {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFT v4 6/8] ARM: dts: exynos: Remove MCT subnode for interrupt map on Exynos5250
From: Krzysztof Kozlowski @ 2019-09-24 7:01 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Mark Rutland, devicetree, linux-samsung-soc@vger.kernel.org,
Sylwester Nawrocki, Daniel Lezcano, linux-kernel@vger.kernel.org,
Rob Herring, Andrzej Hajda, Kukjin Kim, Thomas Gleixner,
linux-arm-kernel
In-Reply-To: <7ad1898d-3aa7-db49-78c1-c42b34568c60@samsung.com>
On Tue, 24 Sep 2019 at 08:58, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
>
> On 23.09.2019 18:14, Krzysztof Kozlowski wrote:
> > Multi Core Timer node has interrupts routed to two different parents -
> > GIC and combiner. This was modeled with a interrupt-map within a
> > subnode but can be expressed in an easier and more common way, directly
> > in the node itself.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Works fine on Arndale and Snow boards.
Thanks for testing!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 1/3] watchdog: imx2_wdt: Remove unnecessary blank line
From: Anson Huang @ 2019-09-24 7:07 UTC (permalink / raw)
To: wim, linux, shawnguo, s.hauer, kernel, festevam, linux-watchdog,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
Remove unnecessary blank line.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/watchdog/imx2_wdt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 8d019a9..6711b4e 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -72,7 +72,6 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
-
static unsigned timeout;
module_param(timeout, uint, 0);
MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/3] watchdog: imx2_wdt: Use __maybe_unused instead of #if CONFIG_PM_SLEEP
From: Anson Huang @ 2019-09-24 7:07 UTC (permalink / raw)
To: wim, linux, shawnguo, s.hauer, kernel, festevam, linux-watchdog,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1569308828-8320-1-git-send-email-Anson.Huang@nxp.com>
Use __maybe_unused for power management related functions instead
of #if CONFIG_PM_SLEEP to simply the code.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/watchdog/imx2_wdt.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 6711b4e..034f32c 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -358,9 +358,8 @@ static void imx2_wdt_shutdown(struct platform_device *pdev)
}
}
-#ifdef CONFIG_PM_SLEEP
/* Disable watchdog if it is active or non-active but still running */
-static int imx2_wdt_suspend(struct device *dev)
+static int __maybe_unused imx2_wdt_suspend(struct device *dev)
{
struct watchdog_device *wdog = dev_get_drvdata(dev);
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
@@ -381,7 +380,7 @@ static int imx2_wdt_suspend(struct device *dev)
}
/* Enable watchdog and configure it if necessary */
-static int imx2_wdt_resume(struct device *dev)
+static int __maybe_unused imx2_wdt_resume(struct device *dev)
{
struct watchdog_device *wdog = dev_get_drvdata(dev);
struct imx2_wdt_device *wdev = watchdog_get_drvdata(wdog);
@@ -406,7 +405,6 @@ static int imx2_wdt_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(imx2_wdt_pm_ops, imx2_wdt_suspend,
imx2_wdt_resume);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 3/3] watchdog: imx2_wdt: Use 'dev' instead of dereferencing it repeatedly
From: Anson Huang @ 2019-09-24 7:07 UTC (permalink / raw)
To: wim, linux, shawnguo, s.hauer, kernel, festevam, linux-watchdog,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1569308828-8320-1-git-send-email-Anson.Huang@nxp.com>
Add helper variable dev = &pdev->dev to simply the code.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/watchdog/imx2_wdt.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 034f32c..f8d58bf 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -246,13 +246,14 @@ static const struct regmap_config imx2_wdt_regmap_config = {
static int __init imx2_wdt_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct imx2_wdt_device *wdev;
struct watchdog_device *wdog;
void __iomem *base;
int ret;
u32 val;
- wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
+ wdev = devm_kzalloc(dev, sizeof(*wdev), GFP_KERNEL);
if (!wdev)
return -ENOMEM;
@@ -260,16 +261,16 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
- wdev->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
+ wdev->regmap = devm_regmap_init_mmio_clk(dev, NULL, base,
&imx2_wdt_regmap_config);
if (IS_ERR(wdev->regmap)) {
- dev_err(&pdev->dev, "regmap init failed\n");
+ dev_err(dev, "regmap init failed\n");
return PTR_ERR(wdev->regmap);
}
- wdev->clk = devm_clk_get(&pdev->dev, NULL);
+ wdev->clk = devm_clk_get(dev, NULL);
if (IS_ERR(wdev->clk)) {
- dev_err(&pdev->dev, "can't get Watchdog clock\n");
+ dev_err(dev, "can't get Watchdog clock\n");
return PTR_ERR(wdev->clk);
}
@@ -279,12 +280,12 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
wdog->min_timeout = 1;
wdog->timeout = IMX2_WDT_DEFAULT_TIME;
wdog->max_hw_heartbeat_ms = IMX2_WDT_MAX_TIME * 1000;
- wdog->parent = &pdev->dev;
+ wdog->parent = dev;
ret = platform_get_irq(pdev, 0);
if (ret > 0)
- if (!devm_request_irq(&pdev->dev, ret, imx2_wdt_isr, 0,
- dev_name(&pdev->dev), wdog))
+ if (!devm_request_irq(dev, ret, imx2_wdt_isr, 0,
+ dev_name(dev), wdog))
wdog->info = &imx2_wdt_pretimeout_info;
ret = clk_prepare_enable(wdev->clk);
@@ -294,13 +295,13 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
- wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
+ wdev->ext_reset = of_property_read_bool(dev->of_node,
"fsl,ext-reset-output");
platform_set_drvdata(pdev, wdog);
watchdog_set_drvdata(wdog, wdev);
watchdog_set_nowayout(wdog, nowayout);
watchdog_set_restart_priority(wdog, 128);
- watchdog_init_timeout(wdog, timeout, &pdev->dev);
+ watchdog_init_timeout(wdog, timeout, dev);
if (imx2_wdt_is_running(wdev)) {
imx2_wdt_set_timeout(wdog, wdog->timeout);
@@ -318,7 +319,7 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
if (ret)
goto disable_clk;
- dev_info(&pdev->dev, "timeout %d sec (nowayout=%d)\n",
+ dev_info(dev, "timeout %d sec (nowayout=%d)\n",
wdog->timeout, nowayout);
return 0;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] PM / devfreq: Check NULL governor in available_governors_show
From: Leonard Crestez @ 2019-09-24 7:17 UTC (permalink / raw)
To: Chanwoo Choi, Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park
Cc: Artur Świgoń, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Krzysztof Kozlowski,
Lukasz Luba
In-Reply-To: <d6ea004f-4fbb-9a16-407e-ad8542abe1dc@samsung.com>
On 2019-09-24 4:48 AM, Chanwoo Choi wrote:
> On 19. 9. 24. 오전 1:34, Leonard Crestez wrote:
>> The governor is initialized after sysfs attributes become visible so in
>> theory the governor field can be NULL here.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>> drivers/devfreq/devfreq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Found this by hacking device core to call attribute "show" functions
>> from inside device_add.
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 00fc23fea5b2..896fb2312f2f 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d,
>>
>> /*
>> * The devfreq with immutable governor (e.g., passive) shows
>> * only own governor.
>> */
>> - if (df->governor->immutable) {
>> + if (df->governor && df->governor->immutable) {
>> count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
>> "%s ", df->governor_name);
>> /*
>> * The devfreq device shows the registered governor except for
>> * immutable governors such as passive governor .
>>
>
> As you mentioned, it create sysfs and then initialize the governor instance
> as following:
>
> device_register()
> device_add()
> device_add_attrs()
> creating sysfs entries.
> governor = try_then_request_governor(...)
>
>
> Thanks for fix-up.
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> Additionally, you have to add the following 'fixes' tag
> and then send it to stable mailing list(stable@vger.kernel.org).
> - Fixes: bcf23c79c4e46 ("PM / devfreq: Fix available_governor sysfs")
OK, but I'm not sure this meets the criteria for inclusion into linux
stable:
* It must fix a real bug that bothers people (not a, "This could be a
problem..." type thing).
* No "theoretical race condition" issues, unless an explanation of how
the race can be exploited is also provided.
This patch fixes a theoretical race condition which has been there since
the start.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] PM / devfreq: Check NULL governor in available_governors_show
From: Chanwoo Choi @ 2019-09-24 7:25 UTC (permalink / raw)
To: Leonard Crestez, Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park
Cc: Artur Świgoń, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Krzysztof Kozlowski,
Lukasz Luba
In-Reply-To: <VI1PR04MB70231013FC39AA9A4AD46AC0EE840@VI1PR04MB7023.eurprd04.prod.outlook.com>
On 19. 9. 24. 오후 4:17, Leonard Crestez wrote:
> On 2019-09-24 4:48 AM, Chanwoo Choi wrote:
>> On 19. 9. 24. 오전 1:34, Leonard Crestez wrote:
>>> The governor is initialized after sysfs attributes become visible so in
>>> theory the governor field can be NULL here.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>> drivers/devfreq/devfreq.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Found this by hacking device core to call attribute "show" functions
>>> from inside device_add.
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 00fc23fea5b2..896fb2312f2f 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d,
>>>
>>> /*
>>> * The devfreq with immutable governor (e.g., passive) shows
>>> * only own governor.
>>> */
>>> - if (df->governor->immutable) {
>>> + if (df->governor && df->governor->immutable) {
>>> count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
>>> "%s ", df->governor_name);
>>> /*
>>> * The devfreq device shows the registered governor except for
>>> * immutable governors such as passive governor .
>>>
>>
>> As you mentioned, it create sysfs and then initialize the governor instance
>> as following:
>>
>> device_register()
>> device_add()
>> device_add_attrs()
>> creating sysfs entries.
>> governor = try_then_request_governor(...)
>>
>>
>> Thanks for fix-up.
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> Additionally, you have to add the following 'fixes' tag
>> and then send it to stable mailing list(stable@vger.kernel.org).
>> - Fixes: bcf23c79c4e46 ("PM / devfreq: Fix available_governor sysfs")
>
> OK, but I'm not sure this meets the criteria for inclusion into linux
> stable:
>
> * It must fix a real bug that bothers people (not a, "This could be a
> problem..." type thing).
> * No "theoretical race condition" issues, unless an explanation of how
> the race can be exploited is also provided.
OK. If you think that it is not necessary to send it to stable mailing list,
don't send it. Thanks.
>
> This patch fixes a theoretical race condition which has been there since
> the start.
>
> --
> Regards,
> Leonard
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq
From: Chanwoo Choi @ 2019-09-24 7:26 UTC (permalink / raw)
To: Leonard Crestez, MyungJoo Ham, Kyungmin Park, Matthias Kaehlcke
Cc: Artur Świgoń, Abel Vesa, Saravana Kannan, linux-pm,
Viresh Kumar, NXP Linux Team, Krzysztof Kozlowski, Lukasz Luba,
Alexandre Bailon, Martin Kepplinger, Georgi Djakov,
linux-arm-kernel, Jacky Bai
In-Reply-To: <00747fe09746282ef4d99ffd2a4e58e592ba4f66.1569272883.git.leonard.crestez@nxp.com>
Hi,
On 19. 9. 24. 오전 6:10, Leonard Crestez wrote:
> Switch the handling of min_freq and max_freq from sysfs to use the
> dev_pm_qos_request interface.
>
> Since PM QoS handles frequencies as kHz this change reduces the
> precision of min_freq and max_freq. This shouldn't introduce problems
> because frequencies which are not an integer number of kHz are likely
> not an integer number of Hz either.
>
> Try to ensure compatibility by rounding min values down and rounding
> max values up.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++--------------
> include/linux/devfreq.h | 9 +++----
> 2 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 9887408f23bb..a00737e34d36 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
> *min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
> devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
> *max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
> devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>
> - /* constraints from sysfs */
> - *min_freq = max(*min_freq, devfreq->min_freq);
> - *max_freq = min(*max_freq, devfreq->max_freq);
> -
> /* constraints from OPP interface */
> *min_freq = max(*min_freq, devfreq->scaling_min_freq);
> /* scaling_max_freq can be zero on error */
> if (devfreq->scaling_max_freq)
> *max_freq = min(*max_freq, devfreq->scaling_max_freq);
> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev)
> DEV_PM_QOS_MIN_FREQUENCY);
>
> if (devfreq->profile->exit)
> devfreq->profile->exit(devfreq->dev.parent);
>
> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> kfree(devfreq->time_in_state);
> kfree(devfreq->trans_table);
> mutex_destroy(&devfreq->lock);
> kfree(devfreq);
> }
> @@ -747,18 +745,26 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->scaling_min_freq = find_available_min_freq(devfreq);
> if (!devfreq->scaling_min_freq) {
> err = -EINVAL;
> goto err_dev;
> }
> - devfreq->min_freq = devfreq->scaling_min_freq;
>
> devfreq->scaling_max_freq = find_available_max_freq(devfreq);
> if (!devfreq->scaling_max_freq) {
> err = -EINVAL;
> goto err_dev;
> }
> - devfreq->max_freq = devfreq->scaling_max_freq;
> +
> + /* PM QoS requests for min/max freq from sysfs */
The comment is important. But the devfreq_add_device() has usually
not added the comment for each step. I'm not sure to add the some
comments for only this. How about removing it?
> + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
> + DEV_PM_QOS_MIN_FREQUENCY, 0);
> + if (err < 0)
> + goto err_dev;
> + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
> + DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> + if (err < 0)
> + goto err_dev;
>
> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> atomic_set(&devfreq->suspend_count, 0);
>
> devfreq->trans_table = kzalloc(
> @@ -843,10 +849,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
> err_dev:
> /*
> * Cleanup path for errors that happen before registration.
> * Otherwise we rely on devfreq_dev_release.
> */
> + if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
> + dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> + if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
> + dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> kfree(devfreq->time_in_state);
> kfree(devfreq->trans_table);
> kfree(devfreq);
> err_out:
> return ERR_PTR(err);
> @@ -1387,14 +1397,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>
> ret = sscanf(buf, "%lu", &value);
> if (ret != 1)
> return -EINVAL;
>
> - mutex_lock(&df->lock);
> - df->min_freq = value;
> - update_devfreq(df);
> - mutex_unlock(&df->lock);
> + /* round down to kHz for dev_pm_qos */
Please remove it.
> + if (value)
> + value = value / HZ_PER_KHZ;
It doesn't be necessary.
> +
> + ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
Change it as following:
ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ));
> + if (ret < 0)
> + return ret;
>
> return count;
> }
>
> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
> @@ -1419,19 +1432,19 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>
> ret = sscanf(buf, "%lu", &value);
> if (ret != 1)
> return -EINVAL;
>
> - mutex_lock(&df->lock);
> -
> - /* Interpret zero as "don't care" */
> - if (!value)
> - value = ULONG_MAX;
> + /* round up to kHz for dev_pm_qos and interpret zero as "don't care" */
Please remove it.
> + if (value)
> + value = DIV_ROUND_UP(value, HZ_PER_KHZ);
Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)'
instead of 'value / HZ_PER_KHZ' in min_freq_store()?
> + else
> + value = S32_MAX;> > - df->max_freq = value;
> - update_devfreq(df);
> - mutex_unlock(&df->lock);
> + ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> + if (ret < 0)
> + return ret;
>
> return count;
> }
> static DEVICE_ATTR_RW(min_freq);
>
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 8b92ccbd1962..3162eb9b0954 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -11,10 +11,11 @@
> #define __LINUX_DEVFREQ_H__
>
> #include <linux/device.h>
> #include <linux/notifier.h>
> #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>
> #define DEVFREQ_NAME_LEN 16
>
> /* DEVFREQ governor name */
> #define DEVFREQ_GOV_SIMPLE_ONDEMAND "simple_ondemand"
> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
> * devfreq.nb to the corresponding register notifier call chain.
> * @work: delayed work for load monitoring.
> * @previous_freq: previously configured frequency value.
> * @data: Private data of the governor. The devfreq framework does not
> * touch this.
> - * @min_freq: Limit minimum frequency requested by user (0: none)
> - * @max_freq: Limit maximum frequency requested by user (0: none)
> + * @user_min_freq_req: PM QoS min frequency request from user (via sysfs)
> + * @user_max_freq_req: PM QoS max frequency request from user (via sysfs)
I think that 'user' prefix is not needed. You better to change it as following
@min_freq_req: PM QoS minimum frequency request by user via sysfs
@max_freq_req: PM QoS maximum frequency request by user via sysfs
> * @scaling_min_freq: Limit minimum frequency requested by OPP interface
> * @scaling_max_freq: Limit maximum frequency requested by OPP interface
> * @stop_polling: devfreq polling status of a device.
> * @suspend_freq: frequency of a device set during suspend phase.
> * @resume_freq: frequency of a device set in resume phase.
> @@ -161,12 +162,12 @@ struct devfreq {
> unsigned long previous_freq;
> struct devfreq_dev_status last_status;
>
> void *data; /* private data for governors */
>
> - unsigned long min_freq;
> - unsigned long max_freq;
> + struct dev_pm_qos_request user_min_freq_req;
> + struct dev_pm_qos_request user_max_freq_req;
ditto.
> unsigned long scaling_min_freq;
> unsigned long scaling_max_freq;
> bool stop_polling;
>
> unsigned long suspend_freq;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Re: [PATCH 5/5] arm64: dts: imx8mq: add DCSS node
From: Laurentiu Palcu @ 2019-09-24 7:25 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree@vger.kernel.org, Fabio Estevam, Sascha Hauer,
linux-kernel@vger.kernel.org, dl-linux-imx,
Pengutronix Kernel Team, Shawn Guo, agx@sigxcpu.org,
linux-arm-kernel@lists.infradead.org, l.stach@pengutronix.de
In-Reply-To: <20190923165443.D1B1C20882@mail.kernel.org>
On Mon, Sep 23, 2019 at 09:54:42AM -0700, Stephen Boyd wrote:
> Quoting Laurentiu Palcu (2019-09-23 07:13:19)
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > index 52aae34..d4aa778 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > @@ -871,6 +871,31 @@
> > interrupt-controller;
> > #interrupt-cells = <1>;
> > };
> > +
> > + dcss: dcss@0x32e00000 {
>
> Drop the 0x prefix on node names.
Thanks, will do.
laurentiu
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "nxp,imx8mq-dcss";
> > + reg = <0x32e00000 0x2D000>, <0x32e2f000 0x1000>;
> > + interrupts = <6>, <8>, <9>;
> > + interrupt-names = "ctx_ld", "ctxld_kick", "vblank";
> > + interrupt-parent = <&irqsteer>;
> > + clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>,
> > + <&clk IMX8MQ_CLK_DISP_AXI_ROOT>,
> > + <&clk IMX8MQ_CLK_DISP_RTRM_ROOT>,
> > + <&clk IMX8MQ_VIDEO2_PLL_OUT>,
> > + <&clk IMX8MQ_CLK_DISP_DTRC>;
> > + clock-names = "apb", "axi", "rtrm", "pix", "dtrc";
> > + assigned-clocks = <&clk IMX8MQ_CLK_DISP_AXI>,
> > + <&clk IMX8MQ_CLK_DISP_RTRM>,
> > + <&clk IMX8MQ_VIDEO2_PLL1_REF_SEL>;
> > + assigned-clock-parents = <&clk IMX8MQ_SYS1_PLL_800M>,
> > + <&clk IMX8MQ_SYS1_PLL_800M>,
> > + <&clk IMX8MQ_CLK_27M>;
> > + assigned-clock-rates = <800000000>,
> > + <400000000>;
> > + status = "disabled";
> > + };
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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