* Re: [PATCH 08/12] dt-bindings: devfreq: add compatible for mt8183 cci devfreq
From: Chanwoo Choi @ 2020-05-28 7:42 UTC (permalink / raw)
To: Andrew-sh.Cheng, MyungJoo Ham, Kyungmin Park, Rob Herring,
Mark Rutland, Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown
Cc: linux-pm, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, srv_heupstream
In-Reply-To: <20200520034307.20435-9-andrew-sh.cheng@mediatek.com>
Hi,
On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
> This adds dt-binding documentation of cci devfreq
> for Mediatek MT8183 SoC platform.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
> .../devicetree/bindings/devfreq/mt8183-cci.yaml | 51 ++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci.yaml
>
> diff --git a/Documentation/devicetree/bindings/devfreq/mt8183-cci.yaml b/Documentation/devicetree/bindings/devfreq/mt8183-cci.yaml
> new file mode 100644
> index 000000000000..a7341fd94097
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mt8183-cci.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: https://protect2.fireeye.com/url?k=33f1f15d-6e23ea05-33f07a12-0cc47a31c8b4-91b3f8aeecce95dc&q=1&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fdevfreq%2Fmt8183-cci.yaml%23
> +$schema: https://protect2.fireeye.com/url?k=fc7d9089-a1af8bd1-fc7c1bc6-0cc47a31c8b4-b46f5afc59faf86d&q=1&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> +
> +title: CCI_DEVFREQ driver for MT8183.
> +
> +maintainers:
> + - Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> +
> +description: |
> + This module is used to create CCI DEVFREQ.
> + The performance will depend on both CCI frequency and CPU frequency.
> + For MT8183, CCI co-buck with Little core.
> + Contain CCI opp table for voltage and frequency scaling.
> +
> +properties:
> + compatible:
> + const: "mediatek,mt8183-cci"
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + const: "cci"
> +
> + operating-points-v2: true
> + opp-table: true
> +
> + proc-supply:
> + description:
> + Phandle of the regulator that provides the supply voltage.
> +
> +required:
> + - compatible
> + - clocks
> + - clock-names
> + - proc-supply
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/mt8183-clk.h>
> + cci: cci {
> + compatible = "mediatek,mt8183-cci";
> + clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> + clock-names = "cci";
> + operating-points-v2 = <&cci_opp>;
> + proc-supply = <&mt6358_vproc12_reg>;
> + };
> +
>
I recommend that add the more detailed example
with OPP table with CPU node.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 00/91] drm/vc4: Support BCM2711 Display Pipelin
From: Maxime Ripard @ 2020-05-28 7:30 UTC (permalink / raw)
To: Daniel Drake
Cc: Jian-Hong Pan, Nicolas Saenz Julienne, Eric Anholt, dri-devel,
linux-rpi-kernel, bcm-kernel-feedback-list, linux-arm-kernel,
Linux Kernel, devicetree, linux-clk, linux-i2c,
Linux Upstreaming Team
In-Reply-To: <CAD8Lp45ucK-yZ5G_DrUVA7rnxo58UF1LPUy65w2PCOcSxKx_Sg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 767 bytes --]
Hi Daniel,
On Wed, May 27, 2020 at 05:15:12PM +0800, Daniel Drake wrote:
> On Wed, May 27, 2020 at 5:13 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > I'm about to send a v3 today or tomorrow, I can Cc you (and Jian-Hong) if you
> > want.
>
> That would be great, although given the potentially inconsistent
> results we've been seeing so far it would be great if you could
> additionally push a git branch somewhere.
> That way we can have higher confidence that we are applying exactly
> the same patches to the same base etc.
So I sent a new iteration yesterday, and of course forgot to cc you... Sorry for
that.
I've pushed my current branch here:
https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git/log/?h=rpi4-kms
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 09/12] devfreq: add mediatek cci devfreq
From: Chanwoo Choi @ 2020-05-28 7:35 UTC (permalink / raw)
To: Andrew-sh.Cheng, MyungJoo Ham, Kyungmin Park, Rob Herring,
Mark Rutland, Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown
Cc: linux-pm, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, srv_heupstream
In-Reply-To: <20200520034307.20435-10-andrew-sh.cheng@mediatek.com>
Hi Andrew-sh.Cheng,
On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183.
>
> On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> cores. The driver is notified when the regulator voltage changes
> (driven by cpufreq) and adjusts the CCI frequency to the maximum
> possible value.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
> drivers/devfreq/Kconfig | 10 ++
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 206 +++++++++++++++++++++++++++++++++++
The mt8183-cci.c is enough for driver name.
> 3 files changed, 217 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index d9067950af6a..4ed7116271ee 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -103,6 +103,16 @@ config ARM_IMX8M_DDRC_DEVFREQ
> This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
> adjusting DRAM frequency.
>
> +config ARM_MT8183_CCI_DEVFREQ
> + tristate "MT8183 CCI DEVFREQ Driver"
> + depends on ARM_MEDIATEK_CPUFREQ
> + help
> + This adds a devfreq driver for Cache Coherent Interconnect
> + of Mediatek MT8183, which is shared the same regulator
> + with cpu cluster.
> + It can track buck voltage and update a proper cci frequency.
s/cci/CCI
> + Use notification to get regulator status.
> +
> config ARM_TEGRA_DEVFREQ
> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 3eb4d5e6635c..5b1b670c954d 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> # DEVFREQ Drivers
> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ) += mt8183-cci-devfreq.o
> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 000000000000..cd7929a83bf8
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
s/2019/2020
> +
> + * Author: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/time.h>
> +
> +#include "governor.h"
It is not needed. Please remove it.
> +
> +#define MAX_VOLT_LIMIT (1150000)
> +
> +struct cci_devfreq {
> + struct devfreq *devfreq;
> + struct regulator *proc_reg;
'proc' means the 'processor'?
Instead of 'proc_reg', you better to use 'cpu_reg'.
> + struct clk *cci_clk;
> + int old_vproc;
> + unsigned long old_freq;
> +};
> +
> +static int mtk_cci_set_voltage(struct cci_devfreq *cci_df, int vproc)
> +{
> + int ret;
> +
> + ret = regulator_set_voltage(cci_df->proc_reg, vproc,
> + MAX_VOLT_LIMIT);
> + if (!ret)
> + cci_df->old_vproc = vproc;
> + return ret;
> +}
> +
> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> + u32 flags)
> +{
> + int ret;
> + struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> + struct dev_pm_opp *opp;
> + unsigned long opp_rate, opp_voltage, old_voltage;
> +
> + if (!cci_df)
> + return -EINVAL;
> +
> + if (cci_df->old_freq == *freq)
> + return 0;
> +
> + opp_rate = *freq;
> + opp = dev_pm_opp_find_freq_floor(dev, &opp_rate);
> + opp_voltage = dev_pm_opp_get_voltage(opp);
> + dev_pm_opp_put(opp);
You can use the helper function for getting the rate
with devfreq_recommended_opp(). You can refer the following code
in drivers/devfreq/exynos-bus.c
opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(opp);
}
dev_pm_opp_put(opp);
> +
> + old_voltage = cci_df->old_vproc;
> + if (old_voltage == 0)
> + old_voltage = regulator_get_voltage(cci_df->proc_reg);
> +
> + // scale up: set voltage first then freq
> + if (opp_voltage > old_voltage) {
> + ret = mtk_cci_set_voltage(cci_df, opp_voltage);
> + if (ret) {
> + pr_err("cci: failed to scale up voltage\n");
> + return ret;
> + }
> + }
> +
> + ret = clk_set_rate(cci_df->cci_clk, *freq);
> + if (ret) {
> + pr_err("%s: failed cci to set rate: %d\n", __func__,
> + ret);
> + mtk_cci_set_voltage(cci_df, old_voltage);
> + return ret;
> + }
> +
> + // scale down: set freq first then voltage
> + if (opp_voltage < old_voltage) {
> + ret = mtk_cci_set_voltage(cci_df, opp_voltage);
> + if (ret) {
> + pr_err("cci: failed to scale down voltage\n");
> + clk_set_rate(cci_df->cci_clk, cci_df->old_freq);
> + return ret;
> + }
> + }
I recommend that dev_pm_opp_set_rate() and dev_pm_opp_set_regulator()
instead of 'clk_set_rate' and 'regulator_set_voltage'.
In the dev_pm_opp_set_rate(), handle the these sequence.
You can refer the merged patch[1].
[1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
- PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()
> +
> + cci_df->old_freq = *freq;
> +
> + return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> + .target = mtk_cci_devfreq_target,
Need to add '.exit' for calling dev_pm_opp_of_remove_table().
You can refer the merged devfreq patches like exynos_bus.c, imx-bus.c.
> +};
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> + struct device *cci_dev = &pdev->dev;
> + struct cci_devfreq *cci_df;
> + struct devfreq_passive_data *passive_data;
> + int ret;
> +
> + cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> + if (!cci_df)
> + return -ENOMEM;
> +
> + cci_df->cci_clk = devm_clk_get(cci_dev, "cci_clock");
> + ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> + ret);
> + return ret;
> + }
> + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc");
> + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> + ret);
> + return ret;
> + }
I recommend that use dev_pm_opp_set_regulators.
You can refer the merged patch[1].
[1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
- PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()
> + ret = regulator_enable(cci_df->proc_reg);
> + if (ret) {
> + pr_warn("enable buck for cci fail\n");
Use dev_err instead of 'pr_warn'.
> + return ret;
> + }
> +
> + ret = dev_pm_opp_of_add_table(cci_dev);
> + if (ret) {
> + dev_err(cci_dev, "Fail to init CCI OPP table: %d\n", ret);
How about changing the error log as following
because in this driver, use the 'failed to' sentence for error handling?
failed to get OPP table for CCI:L %d
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, cci_df);
> +
> + passive_data = devm_kzalloc(cci_dev, sizeof(*passive_data), GFP_KERNEL);
> + if (!passive_data)
> + return -ENOMEM;
On this error case, you have to call dev_pm_opp_of_remove_table().
You better to make the 'err_opp' jump lable and then add 'goto err_opp'.
> +
> + passive_data->parent_type = CPUFREQ_PARENT_DEV;
> +
> + cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> + &cci_devfreq_profile,
> + DEVFREQ_GOV_PASSIVE,
> + passive_data);
> + if (IS_ERR(cci_df->devfreq)) {
> + ret = PTR_ERR(cci_df->devfreq);
> + dev_err(cci_dev, "cannot create cci devfreq device:%d\n", ret);
> + dev_pm_opp_of_remove_table(cci_dev);
Instead of direct call, use 'goto err_opp'.
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_cci_devfreq_remove(struct platform_device *pdev)
> +{
> + struct device *cci_dev = &pdev->dev;
> + struct cci_devfreq *cci_df;
> + struct notifier_block *opp_nb;
> +
> + cci_df = platform_get_drvdata(pdev);
> + opp_nb = &cci_df->opp_nb;
> +
> + dev_pm_opp_unregister_notifier(cci_dev, opp_nb);
This patch doesn't call the dev_pm_opp_register_notifier.
Please remove it.
> + devm_devfreq_remove_device(cci_dev, cci_df->devfreq);
Don't need to call this function because you used devm_devfreq_add_device().
> + dev_pm_opp_of_remove_table(cci_dev)> + regulator_disable(cci_df->proc_reg);
> +
> + return 0;
> +}
> +
> +static const __maybe_unused struct of_device_id
> + mediatek_cci_devfreq_of_match[] = {
Make it on one line and remove '__maybe_unused' keyword.
- mediatek_cci_devfreq_of_match-> mediatek_cci_of_match
> + { .compatible = "mediatek,mt8183-cci" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
ditto.
> +
> +static struct platform_driver cci_devfreq_driver = {
> + .probe = mtk_cci_devfreq_probe,
> + .remove = mtk_cci_devfreq_remove,
> + .driver = {
> + .name = "mediatek-cci-devfreq",
> + .of_match_table = of_match_ptr(mediatek_cci_devfreq_of_match),
ditto.
> + },
> +};
> +
> +static int __init mtk_cci_devfreq_init(void)
> +{
> + return platform_driver_register(&cci_devfreq_driver);
> +}
> +module_init(mtk_cci_devfreq_init)
> +
> +static void __exit mtk_cci_devfreq_exit(void)
> +{
> + platform_driver_unregister(&cci_devfreq_driver);
> +}
> +module_exit(mtk_cci_devfreq_exit)
Use 'module_platform_driver' instead of module_init and module_exit.
> +
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* RE: [PATCH v14 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem
From: Vishal Sagar @ 2020-05-28 7:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hyun Kwon, mchehab@kernel.org, robh+dt@kernel.org,
mark.rutland@arm.com, Michal Simek, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Dinesh Kumar, Sandip Kothari,
Luca Ceresoli, Jacopo Mondi
In-Reply-To: <20200527161140.GF6171@pendragon.ideasonboard.com>
Hi Laurent,
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, May 27, 2020 9:42 PM
> To: Vishal Sagar <vsagar@xilinx.com>
> Cc: Hyun Kwon <hyunk@xilinx.com>; mchehab@kernel.org;
> robh+dt@kernel.org; mark.rutland@arm.com; Michal Simek
> <michals@xilinx.com>; linux-media@vger.kernel.org;
> devicetree@vger.kernel.org; hans.verkuil@cisco.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Dinesh Kumar
> <dineshk@xilinx.com>; Sandip Kothari <sandipk@xilinx.com>; Luca Ceresoli
> <luca@lucaceresoli.net>; Jacopo Mondi <jacopo@jmondi.org>
> Subject: Re: [PATCH v14 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI
> CSI-2 Rx Subsystem
>
> Hi Vishal,
>
> Thank you for the patch.
>
> On Wed, May 27, 2020 at 07:27:18PM +0530, Vishal Sagar wrote:
> > Add bindings documentation for Xilinx MIPI CSI-2 Rx Subsystem.
> >
> > The Xilinx MIPI CSI-2 Rx Subsystem consists of a CSI-2 Rx controller,
> > a D-PHY in Rx mode and a Video Format Bridge.
> >
> > Signed-off-by: Vishal Sagar <vishal.sagar@xilinx.com>
> > Reviewed-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > v14
> > - Removed xlnx,csi-pxl-format from required properties
> > - Added dependency of xlnx,csi-pxl-format on xlnx,vfb
> > - End the yaml file with ...
> > - Added Reviewed by Laurent
> >
> > v13
> > - Based on Laurent's suggestions
> > - Fixed the datatypes values as minimum and maximum
> > - condition added for en-vcx property
> >
> > v12
> > - Moved to yaml format
> > - Update CSI-2 and D-PHY
> > - Mention that bindings for D-PHY not here
> > - reset -> video-reset
> >
> > v11
> > - Modify compatible string from 4.0 to 5.0
> >
> > v10
> > - No changes
> >
> > v9
> > - Fix xlnx,vfb description.
> > - s/Optional/Required endpoint property.
> > - Move data-lanes description from Ports to endpoint property section.
> >
> > v8
> > - Added reset-gpios optional property to assert video_aresetn
> >
> > v7
> > - Removed the control name from dt bindings
> > - Updated the example dt node name to csi2rx
> >
> > v6
> > - Added "control" after V4L2_CID_XILINX_MIPICSISS_ACT_LANES as
> > suggested by Luca
> > - Added reviewed by Rob Herring
> >
> > v5
> > - Incorporated comments by Luca Cersoli
> > - Removed DPHY clock from description and example
> > - Removed bayer pattern from device tree MIPI CSI IP
> > doesn't deal with bayer pattern.
> >
> > v4
> > - Added reviewed by Hyun Kwon
> >
> > v3
> > - removed interrupt parent as suggested by Rob
> > - removed dphy clock
> > - moved vfb to optional properties
> > - Added required and optional port properties section
> > - Added endpoint property section
> >
> > v2
> > - updated the compatible string to latest version supported
> > - removed DPHY related parameters
> > - added CSI v2.0 related property (including VCX for supporting upto 16
> > virtual channels).
> > - modified csi-pxl-format from string to unsigned int type where the value
> > is as per the CSI specification
> > - Defined port 0 and port 1 as sink and source ports.
> > - Removed max-lanes property as suggested by Rob and Sakari
> >
> > .../bindings/media/xilinx/xlnx,csi2rxss.yaml | 237
> +++++++++++++++++++++
> > 1 file changed, 237 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.yaml
> > b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.yaml
> > new file mode 100644
> > index 0000000..2282231
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/xilinx/xlnx,csi2rxss.yam
> > +++ l
> > @@ -0,0 +1,237 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/xilinx/xlnx,csi2rxss.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx MIPI CSI-2 Receiver Subsystem
> > +
> > +maintainers:
> > + - Vishal Sagar <vishal.sagar@xilinx.com>
> > +
> > +description: |
> > + The Xilinx MIPI CSI-2 Receiver Subsystem is used to capture MIPI
> > +CSI-2
> > + traffic from compliant camera sensors and send the output as AXI4
> > +Stream
> > + video data for image processing.
> > + The subsystem consists of a MIPI D-PHY in slave mode which captures
> > +the
> > + data packets. This is passed along the MIPI CSI-2 Rx IP which
> > +extracts the
> > + packet data. The optional Video Format Bridge (VFB) converts this
> > +data to
> > + AXI4 Stream video data.
> > + For more details, please refer to PG232 Xilinx MIPI CSI-2 Receiver
> Subsystem.
> > + Please note that this bindings includes only the MIPI CSI-2 Rx
> > +controller
> > + and Video Format Bridge and not D-PHY.
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - xlnx,mipi-csi2-rx-subsystem-5.0
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + description: List of clock specifiers
> > + items:
> > + - description: AXI Lite clock
> > + - description: Video clock
> > +
> > + clock-names:
> > + items:
> > + - const: lite_aclk
> > + - const: video_aclk
> > +
> > + xlnx,csi-pxl-format:
> > + description: |
> > + This denotes the CSI Data type selected in hw design.
> > + Packets other than this data type (except for RAW8 and
> > + User defined data types) will be filtered out.
> > + Possible values are as below -
> > + 0x1e - YUV4228B
> > + 0x1f - YUV42210B
> > + 0x20 - RGB444
> > + 0x21 - RGB555
> > + 0x22 - RGB565
> > + 0x23 - RGB666
> > + 0x24 - RGB888
> > + 0x28 - RAW6
> > + 0x29 - RAW7
> > + 0x2a - RAW8
> > + 0x2b - RAW10
> > + 0x2c - RAW12
> > + 0x2d - RAW14
> > + 0x2e - RAW16
> > + 0x2f - RAW20
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/uint32
> > + - anyOf:
> > + - minimum: 0x1e
> > + - maximum: 0x24
> > + - minimum: 0x28
> > + - maximum: 0x2f
> > +
> > + xlnx,vfb:
> > + type: boolean
> > + description: Present when Video Format Bridge is enabled in IP
> > + configuration
> > +
> > + xlnx,en-csi-v2-0:
> > + type: boolean
> > + description: Present if CSI v2 is enabled in IP configuration.
> > +
> > + xlnx,en-vcx:
> > + type: boolean
> > + description: |
> > + When present, there are maximum 16 virtual channels, else only 4.
> > +
> > + xlnx,en-active-lanes:
> > + type: boolean
> > + description: |
> > + Present if the number of active lanes can be re-configured at
> > + runtime in the Protocol Configuration Register. Otherwise all lanes,
> > + as set in IP configuration, are always active.
> > +
> > + video-reset-gpios:
> > + description: Optional specifier for a GPIO that asserts video_aresetn.
> > + maxItems: 1
> > +
> > + ports:
> > + type: object
> > +
> > + properties:
> > + port@0:
> > + type: object
> > + description: |
> > + Input / sink port node, single endpoint describing the
> > + CSI-2 transmitter.
> > +
> > + properties:
> > + reg:
> > + const: 0
> > +
> > + endpoint:
> > + type: object
> > +
> > + properties:
> > +
> > + data-lanes:
> > + description: |
> > + This is required only in the sink port 0 endpoint which
> > + connects to MIPI CSI-2 source like sensor.
> > + The possible values are -
> > + 1 - For 1 lane enabled in IP.
> > + 1 2 - For 2 lanes enabled in IP.
> > + 1 2 3 - For 3 lanes enabled in IP.
> > + 1 2 3 4 - For 4 lanes enabled in IP.
> > + items:
> > + - const: 1
> > + - const: 2
> > + - const: 3
> > + - const: 4
> > +
> > + remote-endpoint: true
> > +
> > + required:
> > + - data-lanes
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + additionalProperties: false
> > +
> > + port@1:
> > + type: object
> > + description: |
> > + Output / source port node, endpoint describing modules
> > + connected the CSI-2 receiver.
> > +
> > + properties:
> > +
> > + reg:
> > + const: 1
> > +
> > + endpoint:
> > + type: object
> > +
> > + properties:
> > +
> > + remote-endpoint: true
> > +
> > + required:
> > + - remote-endpoint
> > +
> > + additionalProperties: false
> > +
> > + additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - ports
> > +
> > +allOf:
> > + - if:
> > + required:
> > + - xlnx,vfb
> > + then:
> > + required:
> > + - xlnx,csi-pxl-format
> > + else:
> > + properties:
> > + xlnx,csi-pxl-format: false
> > +
> > + - if:
> > + not:
> > + required:
> > + - xlnx,en-csi-v2-0
> > + then:
> > + properties:
> > + xlnx,en-vcx: false
>
> There's an indentation problem here, it should be
>
> allOf:
> - if:
> required:
> - xlnx,vfb
> then:
> required:
> - xlnx,csi-pxl-format
> else:
> properties:
> xlnx,csi-pxl-format: false
>
> - if:
> not:
> required:
> - xlnx,en-csi-v2-0
> then:
> properties:
> xlnx,en-vcx: false
>
> Have you run the bindings checks ?
>
> make
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/xilinx/xlnx,csi
> 2rxss.yaml dt_binding_check
>
> It would have caught the issue.
>
By mistake the incorrect patch was sent. Apologies for this.
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + xcsi2rxss_1: csi2rx@a0020000 {
> > + compatible = "xlnx,mipi-csi2-rx-subsystem-5.0";
> > + reg = <0x0 0xa0020000 0x0 0x10000>;
>
> I think I mentioned in a previous review that this should be
>
> reg = <0xa0020000 0x10000>;
>
> even if it doesn't match what the real values, as dt_binding_check compiles
> the examples in the context of a bus that has #address-cells = <1> and #size-
> cells = <1>.
>
> I can fix these when applying the patches to my tree if that's OK with you, and
> send a pull request.
>
Yes that is fine. Thanks!
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 95 4>;
> > + xlnx,csi-pxl-format = <0x2a>;
> > + xlnx,vfb;
> > + xlnx,en-active-lanes;
> > + xlnx,en-csi-v2-0;
> > + xlnx,en-vcx;
> > + clock-names = "lite_aclk", "video_aclk";
> > + clocks = <&misc_clk_0>, <&misc_clk_1>;
> > + video-reset-gpios = <&gpio 86 GPIO_ACTIVE_LOW>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + /* Sink port */
> > + reg = <0>;
> > + csiss_in: endpoint {
> > + data-lanes = <1 2 3 4>;
> > + /* MIPI CSI-2 Camera handle */
> > + remote-endpoint = <&camera_out>;
> > + };
> > + };
> > + port@1 {
> > + /* Source port */
> > + reg = <1>;
> > + csiss_out: endpoint {
> > + remote-endpoint = <&vproc_in>;
> > + };
> > + };
> > + };
> > + };
> > +...
>
> --
> Regards,
>
> Laurent Pinchart
Regards
Vishal Sagar
^ permalink raw reply
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Sakari Ailus @ 2020-05-28 7:23 UTC (permalink / raw)
To: Dongchun Zhu
Cc: Rob Herring, Linus Walleij, Bartosz Golaszewski,
Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland,
Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu,
srv_heupstream, moderated list:ARM/Mediatek SoC support,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang,
Linux Media Mailing List, devicetree, Louis Kuo,
Shengnan Wang (王圣男)
In-Reply-To: <1590636882.8804.474.camel@mhfsdcap03>
Hi Dongchun,
On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> Hi Sakari, Rob,
>
> On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > Hi Rob, Dongchun,
> >
> > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > + properties:
> > > > > > + endpoint:
> > > > > > + type: object
> > > > > > + additionalProperties: false
> > > > > > +
> > > > > > + properties:
> > > >
> > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > >
> > > Yes, if you are using it.
> >
> > Dongchun, can you confirm the chip has a single data and a single clock
> > lane and that it does not support lane reordering?
> >
>
> From the datasheet, 'MIPI inside the OV02A10 provides one single
> uni-directional clock lane and one bi-directional data lane solution for
> communication links between components inside a mobile device.
> The data lane has full support for HS(uni-directional) and
> LP(bi-directional) data transfer mode.'
>
> The sensor doesn't support lane reordering, so 'clock-lanes' property
> would not be added in next release.
>
> > So if there's nothing to convey to the driver, also the data-lanes should
> > be removed IMO.
> >
>
> However, 'data-lanes' property may still be required.
> It is known that either data-lanes or clock-lanes is an array of
> physical data lane indexes. Position of an entry determines the logical
> lane number, while the value of an entry indicates physical lane, e.g.,
> for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> the clock lane is on hardware lane 0.
>
> As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> support lane reordering, so here we shall use 'data-lanes = <1>' as
> there is only a clock lane for OV02A10.
>
> Reminder:
> If 'data-lanes' property is not present, the driver would assume
> four-lane operation. This means for one-lane or two-lane operation, this
> property must be present and set to the right physical lane indexes.
> If the hardware does not support lane reordering, monotonically
> incremented values shall be used from 0 or 1 onwards, depending on
> whether or not there is also a clock lane.
How can the driver use four lanes, considering the device only supports a
single lane??
--
Sakari Ailus
^ permalink raw reply
* Re: linux-next: build warning after merge of the aspeed tree
From: Joel Stanley @ 2020-05-28 7:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Stephen Rothwell, Olof Johansson, ARM, Rob Herring, devicetree,
Devicetree Compiler, Linux Next Mailing List,
Linux Kernel Mailing List, Manikandan Elumalai, Andrew Jeffery,
Vijay Khemka, David Gibson
In-Reply-To: <CACPK8Xdm91DwuKcm_d9xh_+8gPzxWpWWAzJzq8pAFVc79x-q1A@mail.gmail.com>
On Thu, 28 May 2020 at 06:09, Joel Stanley <joel@jms.id.au> wrote:
>
> On Fri, 22 May 2020 at 08:16, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Fri, May 22, 2020 at 2:16 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > On Wed, 20 May 2020 07:56:36 +0000 Joel Stanley <joel@jms.id.au> wrote:
> > > > I've sent the patch so it applies to the dtc tree. It would be good to
> > > > see that change propagate over to -next as others have reported this
> > > > warning.
> > >
> > > These warnings now appear in the arm-soc tree.
> >
> > Right, I also saw them earlier.
> >
> > Joel, have you sent your patch to David Gibson for integration into
> > upstream dtc?
> > I don't know who sent the other patch, but as long as one of them
> > gets merged, I'd hope we can pull that into kernel as well.
>
> David asked for some extra features (and a typo fix) before he would
> merge it. I'll take a look at that now.
Here we go:
https://lore.kernel.org/lkml/20200528072037.1402346-1-joel@jms.id.au/
^ permalink raw reply
* Re: [PATCH V2 2/3] mmc: sdhci-msm: Use internal voltage control
From: Veerabhadrarao Badiganti @ 2020-05-28 7:13 UTC (permalink / raw)
To: Bjorn Andersson
Cc: adrian.hunter, ulf.hansson, robh+dt, linux-mmc, linux-kernel,
linux-arm-msm, devicetree, Asutosh Das, Vijay Viswanath,
Andy Gross
In-Reply-To: <20200522170415.GI11847@yoga>
On 5/22/2020 10:34 PM, Bjorn Andersson wrote:
> On Fri 22 May 06:27 PDT 2020, Veerabhadrarao Badiganti wrote:
>
>> Hi Bjorn,
>>
>> On 5/22/2020 12:37 AM, Bjorn Andersson wrote:
>>> On Thu 21 May 08:23 PDT 2020, Veerabhadrarao Badiganti wrote:
>>>
>>>> On qcom SD host controllers voltage switching be done after the HW
>>>> is ready for it. The HW informs its readiness through power irq.
>>>> The voltage switching should happen only then.
>>>>
>>>> Use the internal voltage switching and then control the voltage
>>>> switching using power irq.
>>>>
>>>> Set the regulator load as well so that regulator can be configured
>>>> in LPM mode when in is not being used.
>>>>
>>>> Co-developed-by: Asutosh Das <asutoshd@codeaurora.org>
>>>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>>>> Co-developed-by: Vijay Viswanath <vviswana@codeaurora.org>
>>>> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
>>>> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>>>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>>> Looks better, thanks.
>>>
>>>> ---
>>>> drivers/mmc/host/sdhci-msm.c | 207 +++++++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 198 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> [..]
>>>> static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>>>> @@ -1298,6 +1302,71 @@ static void sdhci_msm_set_uhs_signaling(struct sdhci_host *host,
>>>> sdhci_msm_hs400(host, &mmc->ios);
>>>> }
>>>> +static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + if (IS_ERR(mmc->supply.vmmc))
>>>> + return 0;
>>>> +
>>>> + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, mmc->ios.vdd);
>>>> + if (ret)
>>>> + dev_err(mmc_dev(mmc), "%s: vmmc set ocr with vdd=%d failed: %d\n",
>>>> + mmc_hostname(mmc), mmc->ios.vdd, ret);
>>> Missed this one on v1, in the event that mmc_regulator_set_ocr() return
>>> a non-zero value it has already printed an error message. So please
>>> replace the tail with just:
>>>
>>> return mmc_regulator_set_ocr(...);
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
>>>> + struct mmc_host *mmc, bool level)
>>>> +{
>>>> + int load, ret;
>>>> + struct mmc_ios ios;
>>>> +
>>>> + if (IS_ERR(mmc->supply.vqmmc) ||
>>>> + (mmc->ios.power_mode == MMC_POWER_UNDEFINED) ||
>>>> + (msm_host->vqmmc_enabled == level))
>>>> + return 0;
>>>> +
>>>> + if (msm_host->vqmmc_load) {
>>>> + load = level ? msm_host->vqmmc_load : 0;
>>>> + ret = regulator_set_load(mmc->supply.vqmmc, load);
>>> Sorry for the late reply on v1, but please see my explanation regarding
>>> load and always-on regulators there.
>> <Merging your comment from V1 here>
>>
>>>> You should still call regulator_enable()/regulator_disable() on your
>>>> consumer regulator in this driver. When you do this the regulator core
>>>> will conclude that the regulator_dev (i.e. the part that represents the
>>>> hardware) is marked always_on and will not enable/disable the regulator.
>>>> But it will still invoke _regulator_handle_consumer_enable() and
>>>> _regulator_handle_consumer_disable(), which will aggregate the "load" of
>>>> all client regulators and update the regulator's load.
>>>> So this will apply the load as you expect regardless of it being
>>>> supplied by a regulator marked as always_on.
>> Since I'm not turning off this regulator for eMMC, I wanted to keep it in
>> LPM mode
>> to save some power.
>> When the regulator configured in auto mode (RPMH_REGULATOR_MODE_AUTO) it
>> switches to LPM/HPM mode based on the active load.
>> So i have to minimize my driver load requirement so that I can let this
>> regulator
>> in LPM mode.
>> So i need to set load every-time I disable/enable the regulator.
>>
> You call regulator_enable(vqmmc) and regulator_disable() below, so you
> are telling the regulator framework that your struct regulator should be
> "on" or "off".
>
> This will cause the sum of all struct regulator's for the underlying
> struct regulator_dev to be recalculated. So after calling
> regulator_disable() below your effective addition to the load
> calculation is 0, regardless of which load you have specified.
>
> Independent of this the property regulator-always-on (always_on in
> struct regulator_dev) will determine if the enable/disable request will
> actually be sent to the RPMh.
>
>
> So, if you where to not call regulator_disable() for eMMC your argument
> is correct, but as far as I can see you are and you're relying on the
> regulator core to keep it always-on - and then the load logic is in
> effect still.
Thanks for the details Bjorn.
My requirement is, for eMMC i shouldn't be turning this regulator off.
But has to configure in LPM mode.
For SD-card, i have to turn-off this regulator.
So I'm planning to update the logic as below, let me know if you have
any other suggestions.
+static int sdhci_msm_set_vqmmc(struct sdhci_msm_host *msm_host,
+ struct mmc_host *mmc, bool level)
+{
+ int ret;
+ bool always_on;
+
+ if (IS_ERR(mmc->supply.vqmmc) ||
+ (mmc->ios.power_mode == MMC_POWER_UNDEFINED))
+ return 0;
+ /*
+ * For eMMC don't turn off Vqmmc, Instead just configure it in LPM
+ * and HPM modes by setting the right amount of load.
+ */
+ always_on = mmc->card && mmc_card_mmc(mmc->card);
+
+ if (always_on)
+ ret = msm_config_vqmmc_mode(msm_host, mmc, level);
+ else
+ ret = msm_toggle_vqmmc(msm_host, mmc, level);
+
+ return ret;
+}
> Regards,
> Bjorn
>
>>>> + if (ret) {
>>>> + dev_err(mmc_dev(mmc), "%s: vqmmc set load failed: %d\n",
>>>> + mmc_hostname(mmc), ret);
>>>> + goto out;
>>>> + }
>>>> + }
>>>> +
>>>> + if (level) {
>>>> + /* Set the IO voltage regulator to default voltage level */
>>>> + if (msm_host->caps_0 & CORE_3_0V_SUPPORT)
>>>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_330;
>>>> + else if (msm_host->caps_0 & CORE_1_8V_SUPPORT)
>>>> + ios.signal_voltage = MMC_SIGNAL_VOLTAGE_180;
>>>> +
>>>> + if (msm_host->caps_0 & CORE_VOLT_SUPPORT) {
>>>> + ret = mmc_regulator_set_vqmmc(mmc, &ios);
>>>> + if (ret < 0) {
>>>> + dev_err(mmc_dev(mmc), "%s: vqmmc set volgate failed: %d\n",
>>>> + mmc_hostname(mmc), ret);
>>>> + goto out;
>>>> + }
>>>> + }
>>>> + ret = regulator_enable(mmc->supply.vqmmc);
>>>> + } else {
>>>> + ret = regulator_disable(mmc->supply.vqmmc);
>>>> + }
>>>> +
>>>> + if (ret)
>>>> + dev_err(mmc_dev(mmc), "%s: vqmm %sable failed: %d\n",
>>>> + mmc_hostname(mmc), level ? "en":"dis", ret);
>>>> + else
>>>> + msm_host->vqmmc_enabled = level;
>>>> +out:
>>>> + return ret;
>>>> +}
^ permalink raw reply
* Re: [GIT PULL] Immutable branch between MFD, IIO and Power due for the v5.8 merge window
From: Sebastian Reichel @ 2020-05-28 7:09 UTC (permalink / raw)
To: Lee Jones
Cc: Saravanan Sekar, andy.shevchenko, robh+dt, jic23, knaack.h, lars,
pmeerw, devicetree, linux-kernel, linux-iio, linux-pm
In-Reply-To: <20200526094702.GN3628@dell>
[-- Attachment #1: Type: text/plain, Size: 2628 bytes --]
Hi,
Thanks for taking care of it Lee, merged!
-- Sebastian
On Tue, May 26, 2020 at 10:47:02AM +0100, Lee Jones wrote:
> Enjoy!
>
> The following changes since commit 8f3d9f354286745c751374f5f1fcafee6b3f3136:
>
> Linux 5.7-rc1 (2020-04-12 12:35:55 -0700)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-iio-power-v5.8
>
> for you to fetch changes up to 904ac71f4b0c1c26ec47ff597cb3d3c7d36e618d:
>
> MAINTAINERS: Add entry for mp2629 Battery Charger driver (2020-05-26 10:42:02 +0100)
>
> ----------------------------------------------------------------
> Immutable branch between MFD, IIO and Power due for the v5.8 merge window
>
> ----------------------------------------------------------------
> Saravanan Sekar (6):
> dt-bindings: mfd: Add document bindings for mp2629
> mfd: mp2629: Add support for mps battery charger
> iio: adc: mp2629: Add support for mp2629 ADC driver
> power: supply: Add support for mps mp2629 battery charger
> power: supply: mp2629: Add impedance compensation config
> MAINTAINERS: Add entry for mp2629 Battery Charger driver
>
> Documentation/ABI/testing/sysfs-class-power-mp2629 | 8 +
> .../devicetree/bindings/mfd/mps,mp2629.yaml | 62 ++
> MAINTAINERS | 5 +
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/mp2629_adc.c | 208 +++++++
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 2 +
> drivers/mfd/mp2629.c | 79 +++
> drivers/power/supply/Kconfig | 10 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/mp2629_charger.c | 669 +++++++++++++++++++++
> include/linux/mfd/mp2629.h | 26 +
> 13 files changed, 1090 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-power-mp2629
> create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> create mode 100644 drivers/iio/adc/mp2629_adc.c
> create mode 100644 drivers/mfd/mp2629.c
> create mode 100644 drivers/power/supply/mp2629_charger.c
> create mode 100644 include/linux/mfd/mp2629.h
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 06/12] PM / devfreq: Add cpu based scaling support to passive_governor
From: Chanwoo Choi @ 2020-05-28 7:17 UTC (permalink / raw)
To: Andrew-sh.Cheng, MyungJoo Ham, Kyungmin Park, Rob Herring,
Mark Rutland, Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown
Cc: linux-pm, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, srv_heupstream, Sibi Sankar
In-Reply-To: <64b81eea-641c-811d-9830-718b28db4188@samsung.com>
Hi Andrew-sh.Cheng,
The exynos-bus.c used the passive governor.
Even if don't make the problem because DEVFREQ_PARENT_DEV is zero,
you need to initialize the parent_type with DEVFREQ_PARENT_DEV as following:
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 8fa8eb541373..1c71c47bc2ac 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -369,6 +369,7 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
return -ENOMEM;
passive_data->parent = parent_devfreq;
+ passive_data->parent_type = DEVFREQ_PARENT_DEV;
/* Add devfreq device for exynos bus with passive governor */
bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
On 5/28/20 3:14 PM, Chanwoo Choi wrote:
> Hi Andrew-sh.Cheng,
>
> Thanks for your posting. I like this approach absolutely.
> I think that it is necessary. When I developed the embedded product,
> I needed this feature always.
>
> I add the comments on below.
>
>
> And the following email is not valid. So, I dropped this email
> from Cc list.
> Saravana Kannan <skannan@codeaurora.org>
>
>
> On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
>> From: Saravana Kannan <skannan@codeaurora.org>
>>
>> Many CPU architectures have caches that can scale independent of the
>> CPUs. Frequency scaling of the caches is necessary to make sure that the
>> cache is not a performance bottleneck that leads to poor performance and
>> power. The same idea applies for RAM/DDR.
>>
>> To achieve this, this patch adds support for cpu based scaling to the
>> passive governor. This is accomplished by taking the current frequency
>> of each CPU frequency domain and then adjust the frequency of the cache
>> (or any devfreq device) based on the frequency of the CPUs. It listens
>> to CPU frequency transition notifiers to keep itself up to date on the
>> current CPU frequency.
>>
>> To decide the frequency of the device, the governor does one of the
>> following:
>> * Derives the optimal devfreq device opp from required-opps property of
>> the parent cpu opp_table.
>>
>> * Scales the device frequency in proportion to the CPU frequency. So, if
>> the CPUs are running at their max frequency, the device runs at its
>> max frequency. If the CPUs are running at their min frequency, the
>> device runs at its min frequency. It is interpolated for frequencies
>> in between.
>>
>> Andrew-sh.Cheng change
>> dev_pm_opp_xlate_opp to dev_pm_opp_xlate_required_opp devfreq->max_freq
>> to devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value
>> for kernel-5.7
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> [Sibi: Integrated cpu-freqmap governor into passive_governor]
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
>> ---
>> drivers/devfreq/Kconfig | 2 +
>> drivers/devfreq/governor_passive.c | 278 ++++++++++++++++++++++++++++++++++---
>> include/linux/devfreq.h | 40 +++++-
>> 3 files changed, 299 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 0b1df12e0f21..d9067950af6a 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
>> device. This governor does not change the frequency by itself
>> through sysfs entries. The passive governor recommends that
>> devfreq device uses the OPP table to get the frequency/voltage.
>> + Alternatively the governor can also be chosen to scale based on
>> + the online CPUs current frequency.
>>
>> comment "DEVFREQ Drivers"
>>
>> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>> index 2d67d6c12dce..7dcda02a5bb7 100644
>> --- a/drivers/devfreq/governor_passive.c
>> +++ b/drivers/devfreq/governor_passive.c
>> @@ -8,11 +8,89 @@
>> */
>>
>> #include <linux/module.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/cpumask.h>
>> #include <linux/device.h>
>> #include <linux/devfreq.h>
>> +#include <linux/slab.h>
>> #include "governor.h"
>>
>> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>> +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
>
> Need to change 'unsigned int' to 'unsigned long'.
>
>> + unsigned int cpu)
>> +{
>> + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
>
> Better to define them separately as following and then need to rename
> the variable. Usually, use the 'min_freq' and 'max_freq' word for
> the minimum/maximum frequency.
>
> unsigned int cpu_min_freq, cpu_max_freq, cpu_curr_freq, cpu_percent;
> unsigned long dev_min_freq, dev_max_freq, dev_max_state,
>
> The devfreq used 'unsigned long'. The cpufreq used 'unsigned long'
> and 'unsigned int'. You need to handle them properly.
>
>
>> + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
>> + struct devfreq *devfreq = (struct devfreq *)data->this;
>> + unsigned long *freq_table = devfreq->profile->freq_table;
>
> In this function, use 'cpu' work for cpufreq and use 'dev' for devfreq.
> So, I think 'dev_freq_table' is proper name instead of 'freq_table'
> for the readability.
>
> freq_table -> dev_freq_table
>
>> + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
>
> In the get_target_freq_with_devfreq(), use 'p_opp' indicating
> the OPP of parent device. For the consistency, I think that
> use 'p_opp' instead of 'cpu_opp'.
>
>> + unsigned long cpu_freq, freq;
>
> Define the 'cpu_freq' on above with cpu_min_freq/cpu_max_freq definition.
> cpu_freq -> cpu_curr_freq.
>
>> +
>> + if (!cpu_state || cpu_state->first_cpu != cpu ||
>> + !cpu_state->opp_table || !devfreq->opp_table)
>> + return 0;
>> +
>> + cpu_freq = cpu_state->freq * 1000;
>> + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
>> + if (IS_ERR(cpu_opp))
>> + return 0;
>> +
>> + opp = dev_pm_opp_xlate_required_opp(cpu_state->opp_table,
>> + devfreq->opp_table, cpu_opp);
>> + dev_pm_opp_put(cpu_opp);
>> +
>> + if (!IS_ERR(opp)) {
>> + freq = dev_pm_opp_get_freq(opp);
>> + dev_pm_opp_put(opp);
>
> Better to add the 'out' goto statement.
> If you use 'goto out', you can reduce the one indentation
> without 'else' statement.
>
>
>> + } else {
>
> As I commented, when dev_pm_opp_xlate_required_opp() return successfully
> , use 'goto out'. We can remove 'else' and then reduce the unneeded indentation.
>
>
>> + /* Use Interpolation if required opps is not available */
>> + cpu_min = cpu_state->min_freq;
>> + cpu_max = cpu_state->max_freq;
>> + cpu_freq = cpu_state->freq;
>> +
>> + if (freq_table) {
>> + /* Get minimum frequency according to sorting order */
>> + max_state = freq_table[devfreq->profile->max_state - 1];
>> + if (freq_table[0] < max_state) {
>> + dev_min = freq_table[0];
>> + dev_max = max_state;
>> + } else {
>> + dev_min = max_state;
>> + dev_max = freq_table[0];
>> + }
>> + } else {
>> + if (devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value
>> + <= devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value)
>> + return 0;
>> + dev_min =
>> + devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value;
>> + dev_max =
>> + devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value;
>
> I think it is not proper to access the variable of pm_qos structure directly.
> Instead of direct access, you have to use the exported PM QoS function such as
> - pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY);
> - pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY);
>
>> + }
>> + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
>> + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
>> + }
>
>
> I think that you better to add 'out' jump label as following:
>
> out:
>
>> +
>> + return freq;
>> +}
>> +
>> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
>> + unsigned long *freq)
>> +{
>> + struct devfreq_passive_data *p_data =
>> + (struct devfreq_passive_data *)devfreq->data;
>> + unsigned int cpu, target_freq = 0;
>
> Need to define 'target_freq' with 'unsigned long' type.
>
>> +
>> + for_each_online_cpu(cpu)
>> + target_freq = max(target_freq,
>> + xlate_cpufreq_to_devfreq(p_data, cpu));
>> +
>> + *freq = target_freq;
>> +
>> + return 0;
>> +}
>> +
>> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
>> unsigned long *freq)
>> {
>> struct devfreq_passive_data *p_data
>> @@ -23,16 +101,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>> int i, count, ret = 0;
>>
>> /*
>> - * If the devfreq device with passive governor has the specific method
>> - * to determine the next frequency, should use the get_target_freq()
>> - * of struct devfreq_passive_data.
>> - */
>> - if (p_data->get_target_freq) {
>> - ret = p_data->get_target_freq(devfreq, freq);
>> - goto out;
>> - }
>> -
>> - /*
>> * If the parent and passive devfreq device uses the OPP table,
>> * get the next frequency by using the OPP table.
>> */
>> @@ -102,6 +170,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>> return ret;
>> }
>>
>> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>> + unsigned long *freq)
>> +{
>> + struct devfreq_passive_data *p_data =
>> + (struct devfreq_passive_data *)devfreq->data;
>> + int ret;
>> +
>> + /*
>> + * If the devfreq device with passive governor has the specific method
>> + * to determine the next frequency, should use the get_target_freq()
>> + * of struct devfreq_passive_data.
>> + */
>> + if (p_data->get_target_freq)
>> + return p_data->get_target_freq(devfreq, freq);
>> +
>> + switch (p_data->parent_type) {
>> + case DEVFREQ_PARENT_DEV:
>> + ret = get_target_freq_with_devfreq(devfreq, freq);
>> + break;
>> + case CPUFREQ_PARENT_DEV:
>> + ret = get_target_freq_with_cpufreq(devfreq, freq);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + dev_err(&devfreq->dev, "Invalid parent type\n");
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>> {
>> int ret;
>> @@ -156,6 +255,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
>> return NOTIFY_DONE;
>> }
>>
>> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
>> + unsigned long event, void *ptr)
>> +{
>> + struct devfreq_passive_data *data =
>> + container_of(nb, struct devfreq_passive_data, nb);
>> + struct devfreq *devfreq = (struct devfreq *)data->this;
>> + struct devfreq_cpu_state *cpu_state;
>> + struct cpufreq_freqs *freq = ptr;
>
> How about changing 'freq' to 'cpu_freqs'?
>
> In the drivers/cpufreq/cpufreq.c, use 'freqs' name indicating
> the instance of 'struct cpufreq_freqs'. And in order to
> identfy, how about adding 'cpu_' prefix for variable name?
>
>> + unsigned int current_freq;
>
> Need to define curr_freq with 'unsigned long' type
> and better to use 'curr_freq' variable name.
>
>> + int ret;
>> +
>> + if (event != CPUFREQ_POSTCHANGE || !freq ||
>> + !data->cpu_state[freq->policy->cpu])
>> + return 0;
>> +
>> + cpu_state = data->cpu_state[freq->policy->cpu];
>> + if (cpu_state->freq == freq->new)
>> + return 0;
>> +
>> + /* Backup current freq and pre-update cpu state freq*/
>> + current_freq = cpu_state->freq;
>> + cpu_state->freq = freq->new;
>> +
>> + mutex_lock(&devfreq->lock);
>> + ret = update_devfreq(devfreq);
>> + mutex_unlock(&devfreq->lock);
>> + if (ret) {
>> + cpu_state->freq = current_freq;
>> + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
>> +{
>> + struct devfreq_passive_data *data = *p_data;
>> + struct devfreq *devfreq = (struct devfreq *)data->this;
>> + struct device *dev = devfreq->dev.parent;
>> + struct opp_table *opp_table = NULL;
>> + struct devfreq_cpu_state *state;
>
> For the readability, I thinkt 'cpu_state' is proper instead of 'state'.
>
>> + struct cpufreq_policy *policy;
>> + struct device *cpu_dev;
>> + unsigned int cpu;
>> + int ret;
>> +
>> + get_online_cpus();
>
> Add blank line.
>
>> + data->nb.notifier_call = cpufreq_passive_notifier_call;
>> + ret = cpufreq_register_notifier(&data->nb,
>> + CPUFREQ_TRANSITION_NOTIFIER);
>> + if (ret) {
>> + dev_err(dev, "Couldn't register cpufreq notifier.\n");
>> + data->nb.notifier_call = NULL;
>> + goto out;
>> + }
>> +
>> + /* Populate devfreq_cpu_state */
>> + for_each_online_cpu(cpu) {
>> + if (data->cpu_state[cpu])
>> + continue;
>> +
>> + policy = cpufreq_cpu_get(cpu);
>
> cpufreq_cpu_get() might return 'NULL'. I think you need to handle
> return value as following:
>
> if (!policy) {
> ret = -EINVAL;
> goto out;
> } else if (PTR_ERR(policy) == -EPROBE_DEFER) {
> goto out;
> } else if (IS_ERR(policy) {
> ret = PTR_ERR(policy);
> dev_err(dev, "Couldn't get the cpufreq_poliy.\n");
> goto out;
> }
>
> If cpufreq_cpu_get() return successfully, to do next.
> It reduces the one indentaion.
>
>
>
>> + if (policy) {
>> + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> + if (!state) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + cpu_dev = get_cpu_device(cpu);
>> + if (!cpu_dev) {
>> + dev_err(dev, "Couldn't get cpu device.\n");
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
>> + if (IS_ERR(devfreq->opp_table)) {
>> + ret = PTR_ERR(opp_table);
>> + goto out;
>> + }
>> +
>> + state->dev = cpu_dev;
>> + state->opp_table = opp_table;
>> + state->first_cpu = cpumask_first(policy->related_cpus);
>> + state->freq = policy->cur;
>> + state->min_freq = policy->cpuinfo.min_freq;
>> + state->max_freq = policy->cpuinfo.max_freq;
>> + data->cpu_state[cpu] = state;
>
> Add blank line.
>
>> + cpufreq_cpu_put(policy);
>> + } else {
>> + ret = -EPROBE_DEFER;
>> + goto out;
>> + }
>> + }
>
> Add blank line.
>
>> +out:
>> + put_online_cpus();
>> + if (ret)
>> + return ret;
>> +
>> + /* Update devfreq */
>> + mutex_lock(&devfreq->lock);
>> + ret = update_devfreq(devfreq);
>> + mutex_unlock(&devfreq->lock);
>> + if (ret)
>> + dev_err(dev, "Couldn't update the frequency.\n");
>> +
>> + return ret;
>> +}
>> +
>> +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
>> +{
>> + struct devfreq_passive_data *data = *p_data;
>> + struct devfreq_cpu_state *cpu_state;
>> + int cpu;
>> +
>> + if (data->nb.notifier_call)
>> + cpufreq_unregister_notifier(&data->nb,
>> + CPUFREQ_TRANSITION_NOTIFIER);
>> +
>> + for_each_possible_cpu(cpu) {
>> + cpu_state = data->cpu_state[cpu];
>> + if (cpu_state) {
>> + if (cpu_state->opp_table)
>> + dev_pm_opp_put_opp_table(cpu_state->opp_table);
>> + kfree(cpu_state);
>> + cpu_state = NULL;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int devfreq_passive_event_handler(struct devfreq *devfreq,
>> unsigned int event, void *data)
>> {
>> @@ -165,7 +398,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>> struct notifier_block *nb = &p_data->nb;
>> int ret = 0;
>>
>> - if (!parent)
>> + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
>> return -EPROBE_DEFER;
>
> If you modify the devfreq_passive_event_handler() as following,
> you can move this condition for DEVFREQ_PARENT_DEV into
> (register|unregister)_parent_dev_notifier.
>
> switch (event) {
> case DEVFREQ_GOV_START:
> ret = register_parent_dev_notifier(p_data);
> break;
> case DEVFREQ_GOV_STOP:
> ret = unregister_parent_dev_notifier(p_data);
> break;
> default:
> ret = -EINVAL;
> break;
> }
>
> return ret;
>
>>
>> switch (event) {
>> @@ -173,13 +406,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
>> if (!p_data->this)
>> p_data->this = devfreq;
>>
>> - nb->notifier_call = devfreq_passive_notifier_call;
>> - ret = devfreq_register_notifier(parent, nb,
>> - DEVFREQ_TRANSITION_NOTIFIER);
>> + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
>> + nb->notifier_call = devfreq_passive_notifier_call;
>> + ret = devfreq_register_notifier(parent, nb,
>> + DEVFREQ_TRANSITION_NOTIFIER);
>> + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
>> + ret = cpufreq_passive_register(&p_data);
>
> I think that we better to collect the code related to notifier registration
> into one function like devfreq_pass_register_notifier() instead of
> cpufreq_passive_register() as following: I think it is more simple and readable.
>
> If you have more proper function name of register_parent_dev_notifier,
> please give your opinion.
>
>
> int register_parent_dev_notifier(struct devfreq_passive_data **p_data)
> switch (p_data->parent_type) {
> case DEVFREQ_PARENT_DEV:
> nb->notifier_call = devfreq_passive_notifier_call;
> ret = devfreq_register_notifier(parent, nb,
> break;
> case CPUFREQ_PARENT_DEV:
> cpufreq_register_notifier(...)
> ...
> break;
> }
>
>
>> + } else {
>> + ret = -EINVAL;
>> + }
>> break;
>> case DEVFREQ_GOV_STOP:
>> - WARN_ON(devfreq_unregister_notifier(parent, nb,
>> - DEVFREQ_TRANSITION_NOTIFIER));
>> + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
>> + WARN_ON(devfreq_unregister_notifier(parent, nb,
>> + DEVFREQ_TRANSITION_NOTIFIER));
>> + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
>> + cpufreq_passive_unregister(&p_data);
>> + else
>> + ret = -EINVAL;
>
> ditto. unregister_parent_dev_notifier(struct devfreq_passive_data **p_data)
>
>> break;
>> default:
>> break;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index a4b19d593151..04ce576fd6f1 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -278,6 +278,32 @@ struct devfreq_simple_ondemand_data {
>>
>> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
>> /**
>> + * struct devfreq_cpu_state - holds the per-cpu state
>> + * @freq: the current frequency of the cpu.
>> + * @min_freq: the min frequency of the cpu.
>> + * @max_freq: the max frequency of the cpu.
>> + * @first_cpu: the cpumask of the first cpu of a policy.
>> + * @dev: reference to cpu device.
>> + * @opp_table: reference to cpu opp table.
>> + *
>> + * This structure stores the required cpu_state of a cpu.
>> + * This is auto-populated by the governor.
>> + */
>> +struct devfreq_cpu_state {> + unsigned int freq;
>
> It is better to change from 'freq' to 'curr_freq'
> for more correct expression.
>
>> + unsigned int min_freq;
>> + unsigned int max_freq;
>> + unsigned int first_cpu;
>> + struct device *dev;
>
> How about changing the name 'dev' to 'cpu_dev'?
>
>
>> + struct opp_table *opp_table;
>> +};
>
> devfreq_cpu_state is only handled by within driver/devfreq/governor_passive.c.
>
> So, you can move it into drivers/devfreq/governor_passive.c
> and just add the definition into include/linux/devfreq.h as following:
> It is able to prevent the access of variable of 'struct devfreq_cpu_state'
> outside.
>
> struct devfreq_cpu_state;
>
>> +
>> +enum devfreq_parent_dev_type {
>> + DEVFREQ_PARENT_DEV,
>> + CPUFREQ_PARENT_DEV,
>> +};
>> +
>> +/**
>> * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
>> * and devfreq_add_device
>> * @parent: the devfreq instance of parent device.
>> @@ -288,13 +314,15 @@ struct devfreq_simple_ondemand_data {
>> * using governors except for passive governor.
>> * If the devfreq device has the specific method to decide
>> * the next frequency, should use this callback.
>> - * @this: the devfreq instance of own device.
>> - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>> + * @parent_type parent type of the device
>
> Need to add ':' at the end of word. -> "parent_type:".
>
>> + * @this: the devfreq instance of own device.
>> + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>
> I knew that you make them with same indentation.
> But, actually, it is not related to this patch like clean-up code.
> Even if it is not pretty, you better to don't touch 'this' and 'nb' indentaion.
>
>> + * @cpu_state: the state min/max/current frequency of all online cpu's
>> *
>> * The devfreq_passive_data have to set the devfreq instance of parent
>> * device with governors except for the passive governor. But, don't need to
>> - * initialize the 'this' and 'nb' field because the devfreq core will handle
>> - * them.
>> + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
>> + * will handle them.
>> */
>> struct devfreq_passive_data {
>> /* Should set the devfreq instance of parent device */
>> @@ -303,9 +331,13 @@ struct devfreq_passive_data {
>> /* Optional callback to decide the next frequency of passvice device */
>> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>>
>> + /* Should set the type of parent device */
>> + enum devfreq_parent_dev_type parent_type;
>> +
>> /* For passive governor's internal use. Don't need to set them */
>> struct devfreq *this;
>> struct notifier_block nb;
>> + struct devfreq_cpu_state *cpu_state[NR_CPUS];
>> };
>> #endif
>>
>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply related
* Re: [PATCH RESEND v3 0/2] syscon: Alter syscon and reboot drivers
From: Sebastian Reichel @ 2020-05-28 7:03 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko,
Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Alexey Kolotnikov,
Thomas Bogendoerfer, Arnd Bergmann, Rob Herring, linux-mips,
devicetree, linux-pm, linux-kernel
In-Reply-To: <20200526135102.21236-1-Sergey.Semin@baikalelectronics.ru>
[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]
Hi,
On Tue, May 26, 2020 at 04:50:59PM +0300, Serge Semin wrote:
> This is a small patchset about tuning the syscon infrastructure a bit.
> As it's going to be general in the framework of the Baikal-T1 SoC support
> integration into the kernel, we suggest to replace the legacy text-based
> syscon-reboot-mode dts-bindings file with yaml-based one. Then seeing a
> syscon reboot block is normally expected to be a part of a system
> controller and based on the discussion
> https://lore.kernel.org/linux-pm/20200306130402.1F4F0803079F@mail.baikalelectronics.ru/
> we decided to alter the syscon reboot driver so one would also try to fetch
> the syscon registers map from a parental DT node. regmap property is left
> supported although it's marked as deprecated from now.
>
> This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> 0e698dfa2822 ("Linux 5.7-rc4")
> tag: v5.7-rc4
>
> Changelog v2:
> - Add Sebastian' Acked-by tag to patch 1.
> - Use a shorter summary describing the bindings modification patches.
> - Our corporate email server doesn't change Message-Id anymore, so the patchset
> is resubmitted being in the cover-letter-threaded format.
> - Discard patch with syscon "-endian" property support. As Rob said It shall be
> in the common dt-schema.
> - Replace patches of adding a regmap property support to the syscon-reboot-mode
> with patches making syscon-reboot a sub-node of a system controller node.
> - Mark regmap property as deprecated from now.
>
> Link: https://lore.kernel.org/linux-pm/20200507233846.11548-1-Sergey.Semin@baikalelectronics.ru/
> Changelog v3:
> - Discard the commit 6acd3ecd88ff ("dt-bindings: power: reset: Convert
> syscon-reboot-mode to DT schema") since it has been merged in by Sebatian.
> - Add Rob's Reviewed-by tag to the patch "dt-bindings: power: reset: Unrequire
> regmap property in syscon-reboot node"
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> Serge Semin (2):
> dt-bindings: power: reset: Unrequire regmap property in syscon-reboot
> node
> power: reset: syscon-reboot: Add parental syscon support
>
> .../bindings/power/reset/syscon-reboot.yaml | 15 ++++++++++-----
> drivers/power/reset/syscon-reboot.c | 7 +++++--
> 2 files changed, 15 insertions(+), 7 deletions(-)
Thanks, I queued both patches to power-supply's for-next branch.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 6/6] dt-bindings: drm: bridge: adi,adv7511.txt: convert to yaml
From: Ricardo Cañuelo @ 2020-05-28 6:36 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Rob Herring, Laurent Pinchart, Collabora Kernel ML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Geert Uytterhoeven, Wei Xu, michal.simek
In-Reply-To: <CAMuHMdXQQXOcVuq7Zhfp4qGH0vmLtxp3fdCJ+7VSAMQYSdjsTg@mail.gmail.com>
Hi all,
On mié 27-05-2020 20:18:07, Geert Uytterhoeven wrote:
> > There's currently no requirement that binding schema don't introduce
> > warnings in dts files. That should change when/if we get to a warning
> > free state (probably per platform/family). I don't think we're close
> > on any platform? (If we are, I'd like to start tracking that). It is
> > good to pay attention to the warnings you get though as the schema may
> > not be doing what you expect or the binding really doesn't match
> > reality.
>
> OK.
>
> > > I do my best to avoid introducing regressions when the binding conversions
> > > go upstream.
> >
> > Meaning you fix the dts files or massage the schema to match? If we
> > just adjust schema to match, what's the point in this effort? We
> > should find things wrong or ill defined.
>
> I fix up DTS files, and fast-track those fixes, so they appear upstream before
> the DT binding conversion, where possible.
Thank you everyone for pitching in and for clarifying the procedure,
I'll prepare a new series with the changes that Laurent proposed
(without the patches that Geert already merged).
Cheers,
Ricardo
^ permalink raw reply
* [PATCH v2 0/4] Spilt PCIe node to comply with hardware design
From: chuanjia.liu @ 2020-05-28 6:16 UTC (permalink / raw)
To: robh+dt, ryder.lee, matthias.bgg
Cc: lorenzo.pieralisi, amurray, linux-pci, linux-mediatek, devicetree,
linux-kernel, linux-arm-kernel, bhelgaas, chuanjia.liu,
jianjun.wang, yong.wu, srv_heupstream
There are two independent PCIe controllers in MT2712/MT7622 platform,
and each of them should contain an independent MSI domain.
In current architecture, MSI domain will be inherited from the root
bridge, and all of the devices will share the same MSI domain.
Hence that, the PCIe devices will not work properly if the irq number
which required is more than 32.
Split the PCIe node for MT2712/MT7622 platform to fix MSI issue and
comply with the hardware design.
change note:
v2: change the allocation of mt2712 PCIe MMIO space due to the allcation
size is not right in v1.
chuanjia.liu (4):
dt-bindings: PCI: Mediatek: Update PCIe binding
PCI: mediatek: Use regmap to get shared pcie-cfg base
arm64: dts: mediatek: Split PCIe node for MT2712/MT7622
ARM: dts: mediatek: Update mt7629 PCIe node
.../bindings/pci/mediatek-pcie-cfg.yaml | 38 +++++
.../devicetree/bindings/pci/mediatek-pcie.txt | 144 +++++++++++-------
arch/arm/boot/dts/mt7629-rfb.dts | 3 +-
arch/arm/boot/dts/mt7629.dtsi | 23 +--
arch/arm64/boot/dts/mediatek/mt2712e.dtsi | 75 +++++----
.../dts/mediatek/mt7622-bananapi-bpi-r64.dts | 16 +-
arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts | 6 +-
arch/arm64/boot/dts/mediatek/mt7622.dtsi | 68 ++++++---
drivers/pci/controller/pcie-mediatek.c | 25 ++-
9 files changed, 258 insertions(+), 140 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/mediatek-pcie-cfg.yaml
--
2.18.0
^ permalink raw reply
* Re: linux-next: build warning after merge of the aspeed tree
From: Joel Stanley @ 2020-05-28 6:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Stephen Rothwell, Olof Johansson, ARM, Rob Herring, devicetree,
Devicetree Compiler, Linux Next Mailing List,
Linux Kernel Mailing List, Manikandan Elumalai, Andrew Jeffery,
Vijay Khemka, David Gibson
In-Reply-To: <CAK8P3a323rPCDDws+us4UYo7ZO6XvkZ13hBChZ40_DwCxBZj_g@mail.gmail.com>
On Fri, 22 May 2020 at 08:16, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, May 22, 2020 at 2:16 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > On Wed, 20 May 2020 07:56:36 +0000 Joel Stanley <joel@jms.id.au> wrote:
> > > I've sent the patch so it applies to the dtc tree. It would be good to
> > > see that change propagate over to -next as others have reported this
> > > warning.
> >
> > These warnings now appear in the arm-soc tree.
>
> Right, I also saw them earlier.
>
> Joel, have you sent your patch to David Gibson for integration into
> upstream dtc?
> I don't know who sent the other patch, but as long as one of them
> gets merged, I'd hope we can pull that into kernel as well.
David asked for some extra features (and a typo fix) before he would
merge it. I'll take a look at that now.
The patch is 20200520075134.1048589-1-joel@jms.id.au on
devicetree-compiler@vger.kernel.org, which doesn't appear to be
archived on lore.
^ permalink raw reply
* Re: [PATCH 06/12] PM / devfreq: Add cpu based scaling support to passive_governor
From: Chanwoo Choi @ 2020-05-28 6:14 UTC (permalink / raw)
To: Andrew-sh.Cheng, MyungJoo Ham, Kyungmin Park, Rob Herring,
Mark Rutland, Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown
Cc: linux-pm, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, srv_heupstream, Sibi Sankar
In-Reply-To: <20200520034307.20435-7-andrew-sh.cheng@mediatek.com>
Hi Andrew-sh.Cheng,
Thanks for your posting. I like this approach absolutely.
I think that it is necessary. When I developed the embedded product,
I needed this feature always.
I add the comments on below.
And the following email is not valid. So, I dropped this email
from Cc list.
Saravana Kannan <skannan@codeaurora.org>
On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
> From: Saravana Kannan <skannan@codeaurora.org>
>
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
>
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
>
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
> the parent cpu opp_table.
>
> * Scales the device frequency in proportion to the CPU frequency. So, if
> the CPUs are running at their max frequency, the device runs at its
> max frequency. If the CPUs are running at their min frequency, the
> device runs at its min frequency. It is interpolated for frequencies
> in between.
>
> Andrew-sh.Cheng change
> dev_pm_opp_xlate_opp to dev_pm_opp_xlate_required_opp devfreq->max_freq
> to devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value
> for kernel-5.7
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
> drivers/devfreq/Kconfig | 2 +
> drivers/devfreq/governor_passive.c | 278 ++++++++++++++++++++++++++++++++++---
> include/linux/devfreq.h | 40 +++++-
> 3 files changed, 299 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0b1df12e0f21..d9067950af6a 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
> device. This governor does not change the frequency by itself
> through sysfs entries. The passive governor recommends that
> devfreq device uses the OPP table to get the frequency/voltage.
> + Alternatively the governor can also be chosen to scale based on
> + the online CPUs current frequency.
>
> comment "DEVFREQ Drivers"
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 2d67d6c12dce..7dcda02a5bb7 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -8,11 +8,89 @@
> */
>
> #include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> #include <linux/device.h>
> #include <linux/devfreq.h>
> +#include <linux/slab.h>
> #include "governor.h"
>
> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
Need to change 'unsigned int' to 'unsigned long'.
> + unsigned int cpu)
> +{
> + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
Better to define them separately as following and then need to rename
the variable. Usually, use the 'min_freq' and 'max_freq' word for
the minimum/maximum frequency.
unsigned int cpu_min_freq, cpu_max_freq, cpu_curr_freq, cpu_percent;
unsigned long dev_min_freq, dev_max_freq, dev_max_state,
The devfreq used 'unsigned long'. The cpufreq used 'unsigned long'
and 'unsigned int'. You need to handle them properly.
> + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + unsigned long *freq_table = devfreq->profile->freq_table;
In this function, use 'cpu' work for cpufreq and use 'dev' for devfreq.
So, I think 'dev_freq_table' is proper name instead of 'freq_table'
for the readability.
freq_table -> dev_freq_table
> + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
In the get_target_freq_with_devfreq(), use 'p_opp' indicating
the OPP of parent device. For the consistency, I think that
use 'p_opp' instead of 'cpu_opp'.
> + unsigned long cpu_freq, freq;
Define the 'cpu_freq' on above with cpu_min_freq/cpu_max_freq definition.
cpu_freq -> cpu_curr_freq.
> +
> + if (!cpu_state || cpu_state->first_cpu != cpu ||
> + !cpu_state->opp_table || !devfreq->opp_table)
> + return 0;
> +
> + cpu_freq = cpu_state->freq * 1000;
> + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
> + if (IS_ERR(cpu_opp))
> + return 0;
> +
> + opp = dev_pm_opp_xlate_required_opp(cpu_state->opp_table,
> + devfreq->opp_table, cpu_opp);
> + dev_pm_opp_put(cpu_opp);
> +
> + if (!IS_ERR(opp)) {
> + freq = dev_pm_opp_get_freq(opp);
> + dev_pm_opp_put(opp);
Better to add the 'out' goto statement.
If you use 'goto out', you can reduce the one indentation
without 'else' statement.
> + } else {
As I commented, when dev_pm_opp_xlate_required_opp() return successfully
, use 'goto out'. We can remove 'else' and then reduce the unneeded indentation.
> + /* Use Interpolation if required opps is not available */
> + cpu_min = cpu_state->min_freq;
> + cpu_max = cpu_state->max_freq;
> + cpu_freq = cpu_state->freq;
> +
> + if (freq_table) {
> + /* Get minimum frequency according to sorting order */
> + max_state = freq_table[devfreq->profile->max_state - 1];
> + if (freq_table[0] < max_state) {
> + dev_min = freq_table[0];
> + dev_max = max_state;
> + } else {
> + dev_min = max_state;
> + dev_max = freq_table[0];
> + }
> + } else {
> + if (devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value
> + <= devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value)
> + return 0;
> + dev_min =
> + devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value;
> + dev_max =
> + devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value;
I think it is not proper to access the variable of pm_qos structure directly.
Instead of direct access, you have to use the exported PM QoS function such as
- pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY);
- pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY);
> + }
> + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
> + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> + }
I think that you better to add 'out' jump label as following:
out:
> +
> + return freq;
> +}
> +
> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + unsigned int cpu, target_freq = 0;
Need to define 'target_freq' with 'unsigned long' type.
> +
> + for_each_online_cpu(cpu)
> + target_freq = max(target_freq,
> + xlate_cpufreq_to_devfreq(p_data, cpu));
> +
> + *freq = target_freq;
> +
> + return 0;
> +}
> +
> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
> unsigned long *freq)
> {
> struct devfreq_passive_data *p_data
> @@ -23,16 +101,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> int i, count, ret = 0;
>
> /*
> - * If the devfreq device with passive governor has the specific method
> - * to determine the next frequency, should use the get_target_freq()
> - * of struct devfreq_passive_data.
> - */
> - if (p_data->get_target_freq) {
> - ret = p_data->get_target_freq(devfreq, freq);
> - goto out;
> - }
> -
> - /*
> * If the parent and passive devfreq device uses the OPP table,
> * get the next frequency by using the OPP table.
> */
> @@ -102,6 +170,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> return ret;
> }
>
> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + int ret;
> +
> + /*
> + * If the devfreq device with passive governor has the specific method
> + * to determine the next frequency, should use the get_target_freq()
> + * of struct devfreq_passive_data.
> + */
> + if (p_data->get_target_freq)
> + return p_data->get_target_freq(devfreq, freq);
> +
> + switch (p_data->parent_type) {
> + case DEVFREQ_PARENT_DEV:
> + ret = get_target_freq_with_devfreq(devfreq, freq);
> + break;
> + case CPUFREQ_PARENT_DEV:
> + ret = get_target_freq_with_cpufreq(devfreq, freq);
> + break;
> + default:
> + ret = -EINVAL;
> + dev_err(&devfreq->dev, "Invalid parent type\n");
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
> {
> int ret;
> @@ -156,6 +255,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
> + unsigned long event, void *ptr)
> +{
> + struct devfreq_passive_data *data =
> + container_of(nb, struct devfreq_passive_data, nb);
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct devfreq_cpu_state *cpu_state;
> + struct cpufreq_freqs *freq = ptr;
How about changing 'freq' to 'cpu_freqs'?
In the drivers/cpufreq/cpufreq.c, use 'freqs' name indicating
the instance of 'struct cpufreq_freqs'. And in order to
identfy, how about adding 'cpu_' prefix for variable name?
> + unsigned int current_freq;
Need to define curr_freq with 'unsigned long' type
and better to use 'curr_freq' variable name.
> + int ret;
> +
> + if (event != CPUFREQ_POSTCHANGE || !freq ||
> + !data->cpu_state[freq->policy->cpu])
> + return 0;
> +
> + cpu_state = data->cpu_state[freq->policy->cpu];
> + if (cpu_state->freq == freq->new)
> + return 0;
> +
> + /* Backup current freq and pre-update cpu state freq*/
> + current_freq = cpu_state->freq;
> + cpu_state->freq = freq->new;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret) {
> + cpu_state->freq = current_freq;
> + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct device *dev = devfreq->dev.parent;
> + struct opp_table *opp_table = NULL;
> + struct devfreq_cpu_state *state;
For the readability, I thinkt 'cpu_state' is proper instead of 'state'.
> + struct cpufreq_policy *policy;
> + struct device *cpu_dev;
> + unsigned int cpu;
> + int ret;
> +
> + get_online_cpus();
Add blank line.
> + data->nb.notifier_call = cpufreq_passive_notifier_call;
> + ret = cpufreq_register_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + dev_err(dev, "Couldn't register cpufreq notifier.\n");
> + data->nb.notifier_call = NULL;
> + goto out;
> + }
> +
> + /* Populate devfreq_cpu_state */
> + for_each_online_cpu(cpu) {
> + if (data->cpu_state[cpu])
> + continue;
> +
> + policy = cpufreq_cpu_get(cpu);
cpufreq_cpu_get() might return 'NULL'. I think you need to handle
return value as following:
if (!policy) {
ret = -EINVAL;
goto out;
} else if (PTR_ERR(policy) == -EPROBE_DEFER) {
goto out;
} else if (IS_ERR(policy) {
ret = PTR_ERR(policy);
dev_err(dev, "Couldn't get the cpufreq_poliy.\n");
goto out;
}
If cpufreq_cpu_get() return successfully, to do next.
It reduces the one indentaion.
> + if (policy) {
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + cpu_dev = get_cpu_device(cpu);
> + if (!cpu_dev) {
> + dev_err(dev, "Couldn't get cpu device.\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> + if (IS_ERR(devfreq->opp_table)) {
> + ret = PTR_ERR(opp_table);
> + goto out;
> + }
> +
> + state->dev = cpu_dev;
> + state->opp_table = opp_table;
> + state->first_cpu = cpumask_first(policy->related_cpus);
> + state->freq = policy->cur;
> + state->min_freq = policy->cpuinfo.min_freq;
> + state->max_freq = policy->cpuinfo.max_freq;
> + data->cpu_state[cpu] = state;
Add blank line.
> + cpufreq_cpu_put(policy);
> + } else {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
> + }
Add blank line.
> +out:
> + put_online_cpus();
> + if (ret)
> + return ret;
> +
> + /* Update devfreq */
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret)
> + dev_err(dev, "Couldn't update the frequency.\n");
> +
> + return ret;
> +}
> +
> +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq_cpu_state *cpu_state;
> + int cpu;
> +
> + if (data->nb.notifier_call)
> + cpufreq_unregister_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> +
> + for_each_possible_cpu(cpu) {
> + cpu_state = data->cpu_state[cpu];
> + if (cpu_state) {
> + if (cpu_state->opp_table)
> + dev_pm_opp_put_opp_table(cpu_state->opp_table);
> + kfree(cpu_state);
> + cpu_state = NULL;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int devfreq_passive_event_handler(struct devfreq *devfreq,
> unsigned int event, void *data)
> {
> @@ -165,7 +398,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> struct notifier_block *nb = &p_data->nb;
> int ret = 0;
>
> - if (!parent)
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
> return -EPROBE_DEFER;
If you modify the devfreq_passive_event_handler() as following,
you can move this condition for DEVFREQ_PARENT_DEV into
(register|unregister)_parent_dev_notifier.
switch (event) {
case DEVFREQ_GOV_START:
ret = register_parent_dev_notifier(p_data);
break;
case DEVFREQ_GOV_STOP:
ret = unregister_parent_dev_notifier(p_data);
break;
default:
ret = -EINVAL;
break;
}
return ret;
>
> switch (event) {
> @@ -173,13 +406,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> if (!p_data->this)
> p_data->this = devfreq;
>
> - nb->notifier_call = devfreq_passive_notifier_call;
> - ret = devfreq_register_notifier(parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> + nb->notifier_call = devfreq_passive_notifier_call;
> + ret = devfreq_register_notifier(parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER);
> + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> + ret = cpufreq_passive_register(&p_data);
I think that we better to collect the code related to notifier registration
into one function like devfreq_pass_register_notifier() instead of
cpufreq_passive_register() as following: I think it is more simple and readable.
If you have more proper function name of register_parent_dev_notifier,
please give your opinion.
int register_parent_dev_notifier(struct devfreq_passive_data **p_data)
switch (p_data->parent_type) {
case DEVFREQ_PARENT_DEV:
nb->notifier_call = devfreq_passive_notifier_call;
ret = devfreq_register_notifier(parent, nb,
break;
case CPUFREQ_PARENT_DEV:
cpufreq_register_notifier(...)
...
break;
}
> + } else {
> + ret = -EINVAL;
> + }
> break;
> case DEVFREQ_GOV_STOP:
> - WARN_ON(devfreq_unregister_notifier(parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER));
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> + WARN_ON(devfreq_unregister_notifier(parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER));
> + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> + cpufreq_passive_unregister(&p_data);
> + else
> + ret = -EINVAL;
ditto. unregister_parent_dev_notifier(struct devfreq_passive_data **p_data)
> break;
> default:
> break;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index a4b19d593151..04ce576fd6f1 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -278,6 +278,32 @@ struct devfreq_simple_ondemand_data {
>
> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> /**
> + * struct devfreq_cpu_state - holds the per-cpu state
> + * @freq: the current frequency of the cpu.
> + * @min_freq: the min frequency of the cpu.
> + * @max_freq: the max frequency of the cpu.
> + * @first_cpu: the cpumask of the first cpu of a policy.
> + * @dev: reference to cpu device.
> + * @opp_table: reference to cpu opp table.
> + *
> + * This structure stores the required cpu_state of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_state {> + unsigned int freq;
It is better to change from 'freq' to 'curr_freq'
for more correct expression.
> + unsigned int min_freq;
> + unsigned int max_freq;
> + unsigned int first_cpu;
> + struct device *dev;
How about changing the name 'dev' to 'cpu_dev'?
> + struct opp_table *opp_table;
> +};
devfreq_cpu_state is only handled by within driver/devfreq/governor_passive.c.
So, you can move it into drivers/devfreq/governor_passive.c
and just add the definition into include/linux/devfreq.h as following:
It is able to prevent the access of variable of 'struct devfreq_cpu_state'
outside.
struct devfreq_cpu_state;
> +
> +enum devfreq_parent_dev_type {
> + DEVFREQ_PARENT_DEV,
> + CPUFREQ_PARENT_DEV,
> +};
> +
> +/**
> * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
> * and devfreq_add_device
> * @parent: the devfreq instance of parent device.
> @@ -288,13 +314,15 @@ struct devfreq_simple_ondemand_data {
> * using governors except for passive governor.
> * If the devfreq device has the specific method to decide
> * the next frequency, should use this callback.
> - * @this: the devfreq instance of own device.
> - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + * @parent_type parent type of the device
Need to add ':' at the end of word. -> "parent_type:".
> + * @this: the devfreq instance of own device.
> + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
I knew that you make them with same indentation.
But, actually, it is not related to this patch like clean-up code.
Even if it is not pretty, you better to don't touch 'this' and 'nb' indentaion.
> + * @cpu_state: the state min/max/current frequency of all online cpu's
> *
> * The devfreq_passive_data have to set the devfreq instance of parent
> * device with governors except for the passive governor. But, don't need to
> - * initialize the 'this' and 'nb' field because the devfreq core will handle
> - * them.
> + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
> + * will handle them.
> */
> struct devfreq_passive_data {
> /* Should set the devfreq instance of parent device */
> @@ -303,9 +331,13 @@ struct devfreq_passive_data {
> /* Optional callback to decide the next frequency of passvice device */
> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>
> + /* Should set the type of parent device */
> + enum devfreq_parent_dev_type parent_type;
> +
> /* For passive governor's internal use. Don't need to set them */
> struct devfreq *this;
> struct notifier_block nb;
> + struct devfreq_cpu_state *cpu_state[NR_CPUS];
> };
> #endif
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Dongchun Zhu @ 2020-05-28 5:53 UTC (permalink / raw)
To: Rob Herring
Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab,
Andy Shevchenko, Mark Rutland, Sakari Ailus, Nicolas Boichat,
Tomasz Figa, Matthias Brugger, Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang,
Linux Media Mailing List, devicetree, Louis Kuo,
Shengnan Wang (王圣男), dongchun.zhu
In-Reply-To: <CAL_Jsq+sN0SVidTrY0ODXEkzkxYFvG1FTnL0oRQBSKf=ynLdyQ@mail.gmail.com>
Hi Rob,
On Wed, 2020-05-27 at 09:27 -0600, Rob Herring wrote:
> On Wed, May 27, 2020 at 2:51 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Rob,
> >
> > Thanks for the review. Please see my replies below.
> >
> > On Tue, 2020-05-26 at 12:28 -0600, Rob Herring wrote:
> > > On Sat, May 23, 2020 at 04:41:02PM +0800, Dongchun Zhu wrote:
> > > > Add DT bindings documentation for Omnivision OV02A10 image sensor.
> > > >
> > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > ---
> > > > .../bindings/media/i2c/ovti,ov02a10.yaml | 172 +++++++++++++++++++++
> > > > MAINTAINERS | 7 +
> > > > 2 files changed, 179 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > new file mode 100644
> > > > index 0000000..56f31b5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> > > > @@ -0,0 +1,172 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +# Copyright (c) 2020 MediaTek Inc.
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov02a10.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Omnivision OV02A10 CMOS Sensor Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > + - Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > +
> > > > +description: |-
> > > > + The Omnivision OV02A10 is a low-cost, high performance, 1/5-inch, 2 megapixel
> > > > + image sensor, which is the latest production derived from Omnivision's CMOS
> > > > + image sensor technology. Ihis chip supports high frame rate speeds up to 30fps
> > > > + @ 1600x1200 (UXGA) resolution transferred over a 1-lane MIPI interface. The
> > > > + sensor output is available via CSI-2 serial data output.
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + const: ovti,ov02a10
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > +
> > > > + clocks:
> > > > + items:
> > > > + - description: top mux camtg clock
> > > > + - description: divider clock
> > > > +
> > > > + clock-names:
> > > > + items:
> > > > + - const: eclk
> > > > + - const: freq_mux
> > > > +
> > > > + clock-frequency:
> > > > + description:
> > > > + Frequency of the eclk clock in Hertz.
> > > > +
> >
> > Rob, shall we use 'maxItems: 1' to constrain property: clock-frequency?
>
> No, because it is a scalar, not an array.
>
Got it.
> > Or could we adopt 'clock-frequency: true' directly here?
>
> As-is is fine.
>
Understood.
> > > > + dovdd-supply:
> > > > + description:
> > > > + Definition of the regulator used as Digital I/O voltage supply.
> > > > +
> >
> > Shall we add 'maxItems: 1' here?
>
> No, supplies are always singular.
>
Fine.
> >
> > > > + avdd-supply:
> > > > + description:
> > > > + Definition of the regulator used as Analog voltage supply.
> > > > +
> >
> > Ditto.
> >
> > > > + dvdd-supply:
> > > > + description:
> > > > + Definition of the regulator used as Digital core voltage supply.
> > > > +
> >
> > Ditto.
> >
> > > > + powerdown-gpios:
> > > > + description:
> > > > + Must be the device tree identifier of the GPIO connected to the
> > > > + PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> > > > + or Shutdown mode. As the line is active low, it should be
> > > > + marked GPIO_ACTIVE_LOW.
> > >
> > > Need to define how many GPIOs ('maxItems: 1')
> > >
> >
> > It would be fixed like this in next release.
> > powerdown-gpios:
> > maxItems: 1
> > description:
> > Must be the device tree identifier of the GPIO connected to the
> > PD_PAD pin. This pin is used to place the OV02A10 into Standby mode
> > or Shutdown mode. As the line is active low, it should be
> > marked GPIO_ACTIVE_LOW.
> >
Tomasz, I don't know whether you noticed this description.
Here I simply defined one necessary GPIO polarity in DT which driver
settings need to follow.
> > > > +
> > > > + reset-gpios:
> > > > + description:
> > > > + Must be the device tree identifier of the GPIO connected to the
> > > > + RST_PD pin. If specified, it will be asserted during driver probe.
> > > > + As the line is active high, it should be marked GPIO_ACTIVE_HIGH.
> > >
> > > Here too.
> > >
> >
> > Similar as 'powerdown-gpios'.
> > Fixed in next release.
> >
> > > > +
> > > > + rotation:
> > > > + description:
> > > > + Definition of the sensor's placement.
> > > > + allOf:
> > > > + - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > + - enum:
> > > > + - 0 # Sensor Mounted Upright
> > > > + - 180 # Sensor Mounted Upside Down
> > > > + default: 0
> > > > +
> > > > + ovti,mipi-tx-speed:
> > > > + description:
> > > > + Indication of MIPI transmission speed select, which is to control D-PHY
> > > > + timing setting by adjusting MIPI clock voltage to improve the clock
> > > > + driver capability.
> > > > + allOf:
> > > > + - $ref: "/schemas/types.yaml#/definitions/uint32"
> > > > + - enum:
> > > > + - 0 # 20MHz - 30MHz
> > > > + - 1 # 30MHz - 50MHz
> > > > + - 2 # 50MHz - 75MHz
> > > > + - 3 # 75MHz - 100MHz
> > > > + - 4 # 100MHz - 130MHz
> > > > + default: 3
> > > > +
> > > > + # See ../video-interfaces.txt for details
> > > > + port:
> > > > + type: object
> > > > + additionalProperties: false
> > >
> > > Should have a description of what data the port has.
> > >
> >
> > It would be updated as below in next release.
> > port:
> > type: object
> > additionalProperties: false
> > description:
> > Input port node, single endpoint describing the CSI-2 transmitter.
>
> Output?
>
Sorry for the typo.
Yes, this should be output port node.
> >
> > > > +
> > > > + properties:
> > > > + endpoint:
> > > > + type: object
> > > > + additionalProperties: false
> > > > +
> > > > + properties:
> >
> > Actually I wonder whether we need to declare 'clock-lanes' here?
>
> Yes, if you are using it.
>
Looking back to the upstreamed patches, it seems that there is a deep
divide in the setting of 'clock-lanes' property.
As the last comment I just posed, for OV02A10, 'clock-lanes' should be
set to <0> (clock lane on hardware lane 0).
But here we may follow IMX219 patch, which removed 'clock-lanes'
property due to the fact that sensor hardware does not support lane
reordering.
Rob, Sakari, what's your opinions?
> Rob
^ permalink raw reply
* Re: [PATCH v1 2/2] Add PWM driver for LGM
From: Tanwar, Rahul @ 2020-05-28 5:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Uwe Kleine-König, thierry.reding, p.zabel, linux-pwm,
robh+dt, linux-kernel, devicetree, songjun.Wu, cheol.yong.kim,
qi-ming.wu
In-Reply-To: <20200527091521.GH1634618@smile.fi.intel.com>
On 27/5/2020 5:15 pm, Andy Shevchenko wrote:
> On Wed, May 27, 2020 at 02:28:53PM +0800, Tanwar, Rahul wrote:
>> On 22/5/2020 4:56 pm, Uwe Kleine-König wrote:
>>> On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote:
> ...
>
>>> I'm a unhappy to have this in the PWM driver. The PWM driver is supposed
>>> to be generic and I think this belongs into a dedicated driver.
>> Well noted about all other review concerns. I will rework the driver in v2.
>> However, i am not very sure about the above point - of having a separate
>> dedicated driver for tach_work because its logic is tightly coupled with
>> this driver.
> Actually I agree with Uwe.
> Here is layering violation, i.e. provider and consumer in the same pot. It's
> not good from design perspective.
>
Just to clarify, the PWM controller in our SoC serves just one purpose which
is to control the fan. Its actually named as PWM Fan Controller. There is no
other generic usage or any other consumer for this PWM driver. So separating
out this part seems redundant to me. Also, if we separate it out as a
dedicated driver, this will endup as a very small daemon which is going to be
very hard to justify while upstreaming..
Regards,
Rahul
^ permalink raw reply
* Re: [PATCH v3 5/6] bus: Add Baikal-T1 APB-bus driver
From: kbuild test robot @ 2020-05-28 4:51 UTC (permalink / raw)
To: Serge Semin, Thomas Bogendoerfer, Greg Kroah-Hartman,
Arnd Bergmann
Cc: kbuild-all, Serge Semin, Alexey Malahov, Paul Burton,
Olof Johansson, Rob Herring, linux-mips, soc, devicetree,
linux-kernel
In-Reply-To: <20200526125928.17096-6-Sergey.Semin@baikalelectronics.ru>
[-- Attachment #1: Type: text/plain, Size: 5653 bytes --]
Hi Serge,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on char-misc/char-misc-testing staging/staging-testing linus/master v5.7-rc7 next-20200526]
[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/Serge-Semin/bus-memory-Add-Baikal-T1-SoC-APB-AXI-L2-drivers/20200526-210837
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
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
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/bus/bt1-apb.c: In function 'inject_error_store':
drivers/bus/bt1-apb.c:329:3: error: implicit declaration of function 'readl' [-Werror=implicit-function-declaration]
329 | readl(apb->res);
| ^~~~~
In file included from include/linux/kobject.h:20,
from include/linux/module.h:20,
from drivers/bus/bt1-apb.c:12:
drivers/bus/bt1-apb.c: At top level:
>> drivers/bus/bt1-apb.c:338:23: error: initialization of 'ssize_t (*)(struct device *, struct device_attribute *, char *)' {aka 'long int (*)(struct device *, struct device_attribute *, char *)'} from incompatible pointer type 'int (*)(struct device *, struct device_attribute *, char *)' [-Werror=incompatible-pointer-types]
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~
include/linux/sysfs.h:104:10: note: in definition of macro '__ATTR'
104 | .show = _show, | ^~~~~
include/linux/device.h:130:45: note: in expansion of macro '__ATTR_RW'
130 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~
>> drivers/bus/bt1-apb.c:338:8: note: in expansion of macro 'DEVICE_ATTR_RW'
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~~~
drivers/bus/bt1-apb.c:338:23: note: (near initialization for 'dev_attr_inject_error.show')
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~
include/linux/sysfs.h:104:10: note: in definition of macro '__ATTR'
104 | .show = _show, | ^~~~~
include/linux/device.h:130:45: note: in expansion of macro '__ATTR_RW'
130 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~
>> drivers/bus/bt1-apb.c:338:8: note: in expansion of macro 'DEVICE_ATTR_RW'
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~~~
>> drivers/bus/bt1-apb.c:338:23: error: initialization of 'ssize_t (*)(struct device *, struct device_attribute *, const char *, size_t)' {aka 'long int (*)(struct device *, struct device_attribute *, const char *, long unsigned int)'} from incompatible pointer type 'int (*)(struct device *, struct device_attribute *, const char *, size_t)' {aka 'int (*)(struct device *, struct device_attribute *, const char *, long unsigned int)'} [-Werror=incompatible-pointer-types]
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~
include/linux/sysfs.h:105:11: note: in definition of macro '__ATTR'
105 | .store = _store, | ^~~~~~
include/linux/device.h:130:45: note: in expansion of macro '__ATTR_RW'
130 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~
>> drivers/bus/bt1-apb.c:338:8: note: in expansion of macro 'DEVICE_ATTR_RW'
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~~~
drivers/bus/bt1-apb.c:338:23: note: (near initialization for 'dev_attr_inject_error.store')
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~
include/linux/sysfs.h:105:11: note: in definition of macro '__ATTR'
105 | .store = _store, | ^~~~~~
include/linux/device.h:130:45: note: in expansion of macro '__ATTR_RW'
130 | struct device_attribute dev_attr_##_name = __ATTR_RW(_name)
| ^~~~~~~~~
>> drivers/bus/bt1-apb.c:338:8: note: in expansion of macro 'DEVICE_ATTR_RW'
338 | static DEVICE_ATTR_RW(inject_error);
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +338 drivers/bus/bt1-apb.c
317
318 static int inject_error_store(struct device *dev,
319 struct device_attribute *attr,
320 const char *data, size_t count)
321 {
322 struct bt1_apb *apb = dev_get_drvdata(dev);
323
324 /*
325 * Either dummy read from the unmapped address in the APB IO area
326 * or manually set the IRQ status.
327 */
328 if (!strncmp(data, "nodev", 5))
329 readl(apb->res);
330 else if (!strncmp(data, "irq", 3))
331 regmap_update_bits(apb->regs, APB_EHB_ISR, APB_EHB_ISR_PENDING,
332 APB_EHB_ISR_PENDING);
333 else
334 return -EINVAL;
335
336 return count;
337 }
> 338 static DEVICE_ATTR_RW(inject_error);
339
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62466 bytes --]
^ permalink raw reply
* [PATCH v9 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
From: Ramuthevar,Vadivel MuruganX @ 2020-05-28 5:12 UTC (permalink / raw)
To: linux-kernel, linux-mtd, devicetree, miquel.raynal
Cc: richard, vigneshr, arnd, brendanhiggins, tglx, boris.brezillon,
anders.roxell, masonccyang, robh+dt, linux-mips, hauke.mehrtens,
andriy.shevchenko, qi-ming.wu, cheol.yong.kim,
Ramuthevar Vadivel Murugan
In-Reply-To: <20200528051211.3063-1-vadivel.muruganx.ramuthevar@linux.intel.com>
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
This patch adds the new IP of Nand Flash Controller(NFC) support
on Intel's Lightning Mountain(LGM) SoC.
DMA is used for burst data transfer operation, also DMA HW supports
aligned 32bit memory address and aligned data access by default.
DMA burst of 8 supported. Data register used to support the read/write
operation from/to device.
NAND controller driver implements ->exec_op() to replace legacy hooks,
these specific call-back method to execute NAND operations.
Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
drivers/mtd/nand/raw/Kconfig | 8 +
drivers/mtd/nand/raw/Makefile | 1 +
drivers/mtd/nand/raw/intel-nand-controller.c | 747 +++++++++++++++++++++++++++
3 files changed, 756 insertions(+)
create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c
diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index a80a46bb5b8b..75ab2afb78cf 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -457,6 +457,14 @@ config MTD_NAND_CADENCE
Enable the driver for NAND flash on platforms using a Cadence NAND
controller.
+config MTD_NAND_INTEL_LGM
+ tristate "Support for NAND controller on Intel LGM SoC"
+ depends on OF || COMPILE_TEST
+ depends on HAS_IOMEM
+ help
+ Enables support for NAND Flash chips on Intel's LGM SoC.
+ NAND flash controller interfaced through the External Bus Unit.
+
comment "Misc"
config MTD_SM_COMMON
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 2d136b158fb7..bfc8fe4d2cb0 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o
obj-$(CONFIG_MTD_NAND_STM32_FMC2) += stm32_fmc2_nand.o
obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o
obj-$(CONFIG_MTD_NAND_CADENCE) += cadence-nand-controller.o
+obj-$(CONFIG_MTD_NAND_INTEL_LGM) += intel-nand-controller.o
nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
nand-objs += nand_onfi.o
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
new file mode 100644
index 000000000000..564d28978943
--- /dev/null
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -0,0 +1,747 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2020 Intel Corporation. */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/mtd/nand.h>
+#include <linux/resource.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/platform_device.h>
+
+#define EBU_CLC 0x000
+#define EBU_CLC_RST 0x00000000u
+
+#define EBU_ADDR_SEL(n) (0x20 + (n) * 4)
+/* 5 bits 26:22 included for comparison in the ADDR_SELx */
+#define EBU_ADDR_MASK(x) ((x) << 4)
+#define EBU_ADDR_SEL_REGEN 0x1
+
+#define EBU_BUSCON(n) (0x60 + (n) * 4)
+#define EBU_BUSCON_CMULT_V4 0x1
+#define EBU_BUSCON_RECOVC(n) ((n) << 2)
+#define EBU_BUSCON_HOLDC(n) ((n) << 4)
+#define EBU_BUSCON_WAITRDC(n) ((n) << 6)
+#define EBU_BUSCON_WAITWRC(n) ((n) << 8)
+#define EBU_BUSCON_BCGEN_CS 0x0
+#define EBU_BUSCON_SETUP_EN BIT(22)
+#define EBU_BUSCON_ALEC 0xC000
+
+#define EBU_CON 0x0B0
+#define EBU_CON_NANDM_EN BIT(0)
+#define EBU_CON_NANDM_DIS 0x0
+#define EBU_CON_CSMUX_E_EN BIT(1)
+#define EBU_CON_ALE_P_LOW BIT(2)
+#define EBU_CON_CLE_P_LOW BIT(3)
+#define EBU_CON_CS_P_LOW BIT(4)
+#define EBU_CON_SE_P_LOW BIT(5)
+#define EBU_CON_WP_P_LOW BIT(6)
+#define EBU_CON_PRE_P_LOW BIT(7)
+#define EBU_CON_IN_CS_S(n) ((n) << 8)
+#define EBU_CON_OUT_CS_S(n) ((n) << 10)
+#define EBU_CON_LAT_EN_CS_P ((0x3D) << 18)
+
+#define EBU_WAIT 0x0B4
+#define EBU_WAIT_RDBY BIT(0)
+#define EBU_WAIT_WR_C BIT(3)
+
+#define HSNAND_CTL1 0x110
+#define HSNAND_CTL1_ADDR_SHIFT 24
+
+#define HSNAND_CTL2 0x114
+#define HSNAND_CTL2_ADDR_SHIFT 8
+#define HSNAND_CTL2_CYC_N_V5 (0x2 << 16)
+
+#define HSNAND_INT_MSK_CTL 0x124
+#define HSNAND_INT_MSK_CTL_WR_C BIT(4)
+
+#define HSNAND_INT_STA 0x128
+#define HSNAND_INT_STA_WR_C BIT(4)
+
+#define HSNAND_CTL 0x130
+#define HSNAND_CTL_ENABLE_ECC BIT(0)
+#define HSNAND_CTL_GO BIT(2)
+#define HSNAND_CTL_CE_SEL_CS(n) BIT(3 + (n))
+#define HSNAND_CTL_RW_READ 0x0
+#define HSNAND_CTL_RW_WRITE BIT(10)
+#define HSNAND_CTL_ECC_OFF_V8TH BIT(11)
+#define HSNAND_CTL_CKFF_EN 0x0
+#define HSNAND_CTL_MSG_EN BIT(17)
+
+#define HSNAND_PARA0 0x13c
+#define HSNAND_PARA0_PAGE_V8192 0x3
+#define HSNAND_PARA0_PIB_V256 (0x3 << 4)
+#define HSNAND_PARA0_BYP_EN_NP 0x0
+#define HSNAND_PARA0_BYP_DEC_NP 0x0
+#define HSNAND_PARA0_TYPE_ONFI BIT(18)
+#define HSNAND_PARA0_ADEP_EN BIT(21)
+
+#define HSNAND_CMSG_0 0x150
+#define HSNAND_CMSG_1 0x154
+
+#define HSNAND_ALE_OFFS BIT(2)
+#define HSNAND_CLE_OFFS BIT(3)
+#define HSNAND_CS_OFFS BIT(4)
+
+#define HSNAND_ECC_OFFSET 0x008
+
+#define NAND_DATA_IFACE_CHECK_ONLY -1
+
+#define MAX_CS 2
+
+struct ebu_nand_cs {
+ void __iomem *chipaddr;
+ dma_addr_t nand_pa;
+ u32 addr_sel;
+};
+
+struct ebu_nand_controller {
+ struct nand_controller controller;
+ struct nand_chip chip;
+ struct device *dev;
+ void __iomem *ebu;
+ void __iomem *hsnand;
+ struct dma_chan *dma_tx;
+ struct dma_chan *dma_rx;
+ struct completion dma_access_complete;
+ unsigned long clk_rate;
+ struct clk *clk;
+ u32 nd_para0;
+ u8 cs_num;
+ struct ebu_nand_cs cs[MAX_CS];
+};
+
+static inline struct ebu_nand_controller *nand_to_ebu(struct nand_chip *chip)
+{
+ return container_of(chip, struct ebu_nand_controller, chip);
+}
+
+static u8 ebu_nand_readb(struct nand_chip *chip)
+{
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+ void __iomem *nand_wait = ebu_host->ebu + EBU_WAIT;
+ u8 cs_num = ebu_host->cs_num;
+ u32 stat;
+ int ret;
+ u8 val;
+
+ val = readb(ebu_host->cs[cs_num].chipaddr + HSNAND_CS_OFFS);
+
+ ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
+ 20, 1000);
+ if (ret)
+ dev_warn(ebu_host->dev,
+ "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
+ nand_wait, readl(nand_wait));
+
+ return val;
+}
+
+static void ebu_nand_writeb(struct nand_chip *chip, u32 offset, u8 value)
+{
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+ void __iomem *nand_wait = ebu_host->ebu + EBU_WAIT;
+ u8 cs_num = ebu_host->cs_num;
+ u32 stat;
+ int ret;
+
+ writeb(value, ebu_host->cs[cs_num].chipaddr + offset);
+
+ ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
+ 20, 1000);
+ if (ret)
+ dev_warn(ebu_host->dev,
+ "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
+ nand_wait, readl(nand_wait));
+}
+
+static void ebu_read_buf(struct nand_chip *chip, u_char *buf, unsigned int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++)
+ buf[i] = ebu_nand_readb(chip);
+}
+
+static void ebu_write_buf(struct nand_chip *chip, const u_char *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++)
+ ebu_nand_writeb(chip, HSNAND_CS_OFFS, buf[i]);
+}
+
+static void ebu_nand_disable(struct nand_chip *chip)
+{
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+
+ writel(0, ebu_host->ebu + EBU_CON);
+}
+
+static void ebu_select_chip(struct nand_chip *chip)
+{
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+ void __iomem *nand_con = ebu_host->ebu + EBU_CON;
+ u32 cs = ebu_host->cs_num;
+
+ writel(EBU_CON_NANDM_EN | EBU_CON_CSMUX_E_EN | EBU_CON_CS_P_LOW |
+ EBU_CON_SE_P_LOW | EBU_CON_WP_P_LOW | EBU_CON_PRE_P_LOW |
+ EBU_CON_IN_CS_S(cs) | EBU_CON_OUT_CS_S(cs) |
+ EBU_CON_LAT_EN_CS_P, nand_con);
+}
+
+static void ebu_nand_setup_timing(struct ebu_nand_controller *ctrl,
+ const struct nand_sdr_timings *timings)
+{
+ unsigned int rate = clk_get_rate(ctrl->clk) / 1000000;
+ unsigned int period = DIV_ROUND_UP(1000000, rate);
+ u32 trecov, thold, twrwait, trdwait;
+ u32 reg = 0;
+
+ trecov = DIV_ROUND_UP(max(timings->tREA_max, timings->tREH_min),
+ period);
+ reg |= EBU_BUSCON_RECOVC(trecov);
+
+ thold = DIV_ROUND_UP(max(timings->tDH_min, timings->tDS_min), period);
+ reg |= EBU_BUSCON_HOLDC(thold);
+
+ trdwait = DIV_ROUND_UP(max(timings->tRC_min, timings->tREH_min),
+ period);
+ reg |= EBU_BUSCON_WAITRDC(trdwait);
+
+ twrwait = DIV_ROUND_UP(max(timings->tWC_min, timings->tWH_min), period);
+ reg |= EBU_BUSCON_WAITWRC(twrwait);
+
+ reg |= EBU_BUSCON_CMULT_V4 | EBU_BUSCON_BCGEN_CS | EBU_BUSCON_ALEC |
+ EBU_BUSCON_SETUP_EN;
+
+ writel(reg, ctrl->ebu + EBU_BUSCON(ctrl->cs_num));
+}
+
+static int ebu_nand_setup_data_interface(struct nand_chip *chip, int csline,
+ const struct nand_data_interface *conf)
+{
+ struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
+ const struct nand_sdr_timings *timings;
+
+ timings = nand_get_sdr_timings(conf);
+ if (IS_ERR(timings))
+ return PTR_ERR(timings);
+
+ if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+ return 0;
+
+ ebu_nand_setup_timing(ctrl, timings);
+
+ return 0;
+}
+
+static int ebu_nand_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+
+ if (section)
+ return -ERANGE;
+
+ oobregion->offset = HSNAND_ECC_OFFSET;
+ oobregion->length = chip->ecc.total;
+
+ return 0;
+}
+
+static int ebu_nand_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *oobregion)
+{
+ struct nand_chip *chip = mtd_to_nand(mtd);
+
+ if (section)
+ return -ERANGE;
+
+ oobregion->offset = chip->ecc.total + HSNAND_ECC_OFFSET;
+ oobregion->length = mtd->oobsize - oobregion->offset;
+
+ return 0;
+}
+
+static const struct mtd_ooblayout_ops ebu_nand_ooblayout_ops = {
+ .ecc = ebu_nand_ooblayout_ecc,
+ .free = ebu_nand_ooblayout_free,
+};
+
+static void ebu_dma_rx_callback(void *cookie)
+{
+ struct ebu_nand_controller *ebu_host = cookie;
+
+ dmaengine_terminate_async(ebu_host->dma_rx);
+
+ complete(&ebu_host->dma_access_complete);
+}
+
+static void ebu_dma_tx_callback(void *cookie)
+{
+ struct ebu_nand_controller *ebu_host = cookie;
+
+ dmaengine_terminate_async(ebu_host->dma_tx);
+
+ complete(&ebu_host->dma_access_complete);
+}
+
+static int ebu_dma_start(struct ebu_nand_controller *ebu_host, u32 dir,
+ const u8 *buf, u32 len)
+{
+ struct dma_async_tx_descriptor *tx;
+ struct completion *dma_completion;
+ dma_async_tx_callback callback;
+ struct dma_chan *chan;
+ dma_cookie_t cookie;
+ unsigned long flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
+ dma_addr_t buf_dma;
+ int ret;
+ u32 timeout;
+
+ if (dir == DMA_DEV_TO_MEM) {
+ chan = ebu_host->dma_rx;
+ dma_completion = &ebu_host->dma_access_complete;
+ callback = ebu_dma_rx_callback;
+ } else {
+ chan = ebu_host->dma_tx;
+ dma_completion = &ebu_host->dma_access_complete;
+ callback = ebu_dma_tx_callback;
+ }
+
+ buf_dma = dma_map_single(chan->device->dev, (void *)buf, len, dir);
+ if (dma_mapping_error(chan->device->dev, buf_dma)) {
+ dev_err(ebu_host->dev, "Failed to map DMA buffer\n");
+ ret = -EIO;
+ goto err_unmap;
+ }
+
+ tx = dmaengine_prep_slave_single(chan, buf_dma, len, dir, flags);
+ if (!tx)
+ return -ENXIO;
+
+ tx->callback = callback;
+ tx->callback_param = ebu_host;
+ cookie = tx->tx_submit(tx);
+
+ ret = dma_submit_error(cookie);
+ if (ret) {
+ dev_err(ebu_host->dev, "dma_submit_error %d\n", cookie);
+ ret = -EIO;
+ goto err_unmap;
+ }
+
+ init_completion(dma_completion);
+ dma_async_issue_pending(chan);
+
+ /* Wait DMA to finish the data transfer.*/
+ timeout =
+ wait_for_completion_timeout(dma_completion, msecs_to_jiffies(1000));
+ if (!timeout) {
+ dev_err(ebu_host->dev, "I/O Error in DMA RX (status %d)\n",
+ dmaengine_tx_status(chan, cookie, NULL));
+ dmaengine_terminate_sync(chan);
+ ret = -ETIMEDOUT;
+ goto err_unmap;
+ }
+
+ return 0;
+
+err_unmap:
+ dma_unmap_single(ebu_host->dev, buf_dma, len, dir);
+
+ return ret;
+}
+
+static void ebu_nand_trigger(struct ebu_nand_controller *ebu_host,
+ int page, u32 cmd)
+{
+ unsigned int val;
+
+ val = cmd | (page & 0xFF) << HSNAND_CTL1_ADDR_SHIFT;
+ writel(val, ebu_host->hsnand + HSNAND_CTL1);
+ val = (page & 0xFFFF00) >> 8 | HSNAND_CTL2_CYC_N_V5;
+ writel(val, ebu_host->hsnand + HSNAND_CTL2);
+
+ writel(ebu_host->nd_para0, ebu_host->hsnand + HSNAND_PARA0);
+
+ /* clear first, will update later */
+ writel(0xFFFFFFFF, ebu_host->hsnand + HSNAND_CMSG_0);
+ writel(0xFFFFFFFF, ebu_host->hsnand + HSNAND_CMSG_1);
+
+ writel(HSNAND_INT_MSK_CTL_WR_C,
+ ebu_host->hsnand + HSNAND_INT_MSK_CTL);
+
+ val = cmd == NAND_CMD_READ0 ? HSNAND_CTL_RW_READ : HSNAND_CTL_RW_WRITE;
+
+ writel(HSNAND_CTL_MSG_EN | HSNAND_CTL_CKFF_EN |
+ HSNAND_CTL_ECC_OFF_V8TH | HSNAND_CTL_CE_SEL_CS(ebu_host->cs_num) |
+ HSNAND_CTL_ENABLE_ECC | HSNAND_CTL_GO | val,
+ ebu_host->hsnand + HSNAND_CTL);
+}
+
+static int ebu_nand_read_page_hwecc(struct nand_chip *chip, u8 *buf,
+ int oob_required, int page)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+ int ret, x;
+
+ ebu_nand_trigger(ebu_host, page, NAND_CMD_READ0);
+
+ ret = ebu_dma_start(ebu_host, DMA_DEV_TO_MEM, buf, mtd->writesize);
+ if (ret)
+ return ret;
+
+ if (oob_required)
+ chip->ecc.read_oob(chip, page);
+
+ x = readl(ebu_host->hsnand + HSNAND_CTL);
+ x &= ~HSNAND_CTL_GO;
+ writel(x, ebu_host->hsnand + HSNAND_CTL);
+
+ return 0;
+}
+
+static int ebu_nand_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
+ int oob_required, int page)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+ void __iomem *int_sta = ebu_host->hsnand + HSNAND_INT_STA;
+ int ret, val, x;
+ u32 reg;
+
+ ebu_nand_trigger(ebu_host, page, NAND_CMD_SEQIN);
+
+ ret = ebu_dma_start(ebu_host, DMA_MEM_TO_DEV, buf, mtd->writesize);
+ if (ret)
+ return ret;
+
+ if (oob_required) {
+ reg = (chip->oob_poi[3] << 24) | (chip->oob_poi[2] << 16) |
+ (chip->oob_poi[1] << 8) | chip->oob_poi[0];
+
+ writel(reg, ebu_host->hsnand + HSNAND_CMSG_0);
+
+ reg = (chip->oob_poi[7] << 24) | (chip->oob_poi[6] << 16) |
+ (chip->oob_poi[5] << 8) | chip->oob_poi[4];
+
+ writel(reg, ebu_host->hsnand + HSNAND_CMSG_1);
+ }
+
+ ret = readl_poll_timeout_atomic(int_sta, val,
+ !(val & HSNAND_INT_STA_WR_C), 10, 1000);
+ if (ret)
+ return -EIO;
+
+ x = readl(ebu_host->hsnand + HSNAND_CTL);
+ x &= ~HSNAND_CTL_GO;
+ writel(x, ebu_host->hsnand + HSNAND_CTL);
+
+ return 0;
+}
+
+static const u8 ecc_strength[] = { 1, 1, 4, 8, 24, 32, 40, 60, };
+
+static int ebu_nand_attach_chip(struct nand_chip *chip)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+ u32 eccsize, eccsteps, eccbytes, ecctotal, pagesize, pg_per_blk;
+ u32 eccstrength = chip->ecc.strength;
+ u32 writesize = mtd->writesize;
+ u32 blocksize = mtd->erasesize;
+ int start, val, i;
+
+ if (chip->ecc.mode != NAND_ECC_HW)
+ return 0;
+
+ /* Check whether eccsize is 0x0 or wrong. assign eccsize = 512 if YES */
+ if (!chip->ecc.size)
+ chip->ecc.size = 512;
+ eccsize = chip->ecc.size;
+
+ switch (eccsize) {
+ case 512:
+ start = 1;
+ if (!eccstrength)
+ eccstrength = 4;
+ break;
+ case 1024:
+ start = 4;
+ if (!eccstrength)
+ eccstrength = 32;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ i = round_up(start + 1, 4);
+ for (val = start; val < i; val++) {
+ if (eccstrength == ecc_strength[val])
+ break;
+ }
+ if (val == i)
+ return -EINVAL;
+
+ if (eccstrength == 8)
+ eccbytes = 14;
+ else
+ eccbytes = DIV_ROUND_UP(eccstrength * fls(8 * eccsize), 8);
+
+ eccsteps = writesize / eccsize;
+ ecctotal = eccsteps * eccbytes;
+ if ((ecctotal + 8) > mtd->oobsize)
+ return -ERANGE;
+
+ chip->ecc.total = ecctotal;
+ pagesize = fls(writesize >> 11);
+ if (pagesize > HSNAND_PARA0_PAGE_V8192)
+ return -ERANGE;
+
+ pg_per_blk = fls((blocksize / writesize) >> 6) << 4;
+ if (pg_per_blk > HSNAND_PARA0_PIB_V256)
+ return -ERANGE;
+
+ ebu_host->nd_para0 = pagesize | pg_per_blk | HSNAND_PARA0_BYP_EN_NP |
+ HSNAND_PARA0_BYP_DEC_NP | HSNAND_PARA0_ADEP_EN |
+ HSNAND_PARA0_TYPE_ONFI | (val << 29);
+
+ mtd_set_ooblayout(mtd, &ebu_nand_ooblayout_ops);
+ chip->ecc.read_page = ebu_nand_read_page_hwecc;
+ chip->ecc.write_page = ebu_nand_write_page_hwecc;
+
+ return 0;
+}
+
+static int ebu_nand_exec_op(struct nand_chip *chip,
+ const struct nand_operation *op, bool check_only)
+{
+ struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
+ const struct nand_op_instr *instr = NULL;
+ unsigned int op_id;
+ int i, time_out, ret = 0;
+ u32 stat;
+
+ ebu_select_chip(chip);
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = &op->instrs[op_id];
+
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ ebu_nand_writeb(chip, HSNAND_CLE_OFFS | HSNAND_CS_OFFS,
+ instr->ctx.cmd.opcode);
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++)
+ ebu_nand_writeb(chip,
+ HSNAND_ALE_OFFS | HSNAND_CS_OFFS,
+ instr->ctx.addr.addrs[i]);
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ ebu_read_buf(chip, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ ebu_write_buf(chip, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ time_out = instr->ctx.waitrdy.timeout_ms * 1000;
+ ret = readl_poll_timeout(ctrl->ebu + EBU_WAIT,
+ stat, stat & EBU_WAIT_RDBY,
+ 20, time_out);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static const struct nand_controller_ops ebu_nand_controller_ops = {
+ .attach_chip = ebu_nand_attach_chip,
+ .exec_op = ebu_nand_exec_op,
+ .setup_data_interface = ebu_nand_setup_data_interface,
+};
+
+static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
+{
+ if (ebu_host->dma_rx)
+ dma_release_channel(ebu_host->dma_rx);
+
+ if (ebu_host->dma_tx)
+ dma_release_channel(ebu_host->dma_tx);
+}
+
+static int ebu_nand_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ebu_nand_controller *ebu_host;
+ struct nand_chip *nand;
+ struct mtd_info *mtd;
+ struct resource *res;
+ char *resname;
+ int ret, i;
+ u32 reg;
+
+ ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
+ if (!ebu_host)
+ return -ENOMEM;
+
+ ebu_host->dev = dev;
+ nand_controller_init(&ebu_host->controller);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
+ ebu_host->ebu = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(ebu_host->ebu))
+ return PTR_ERR(ebu_host->ebu);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
+ ebu_host->hsnand = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(ebu_host->hsnand))
+ return PTR_ERR(ebu_host->hsnand);
+
+ ret = device_property_read_u32(dev, "nand,cs", ®);
+ if (ret) {
+ dev_err(dev, "failed to get chip select: %d\n", ret);
+ return ret;
+ }
+ ebu_host->cs_num = reg;
+
+ for (i = 0; i < MAX_CS; i++) {
+ resname = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", i);
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ resname);
+ if (!res)
+ return -EINVAL;
+ ebu_host->cs[i].chipaddr = devm_ioremap_resource(dev, res);
+ ebu_host->cs[i].nand_pa = res->start;
+ if (IS_ERR(ebu_host->cs[i].chipaddr))
+ return PTR_ERR(ebu_host->cs[i].chipaddr);
+ }
+
+ ebu_host->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(ebu_host->clk)) {
+ ret = PTR_ERR(ebu_host->clk);
+ dev_err(dev, "failed to get clock: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(ebu_host->clk);
+ if (ret) {
+ dev_err(dev, "failed to enable clock: %d\n", ret);
+ return ret;
+ }
+ ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
+
+ ebu_host->dma_tx = dma_request_chan(dev, "tx");
+ if (IS_ERR(ebu_host->dma_tx)) {
+ ret = PTR_ERR(ebu_host->dma_tx);
+ dev_err(dev, "DMA tx channel request fail!.\n");
+ goto err_cleanup_dma;
+ }
+
+ ebu_host->dma_rx = dma_request_chan(dev, "rx");
+ if (IS_ERR(ebu_host->dma_rx)) {
+ ret = PTR_ERR(ebu_host->dma_rx);
+ dev_err(dev, "DMA rx channel request fail!.\n");
+ goto err_cleanup_dma;
+ }
+
+ for (i = 0; i < MAX_CS; i++) {
+ resname = devm_kasprintf(dev, GFP_KERNEL, "addr_sel%d", i);
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ resname);
+ if (!res)
+ return -EINVAL;
+ ebu_host->cs[i].addr_sel = res->start;
+ writel(ebu_host->cs[i].addr_sel | EBU_ADDR_MASK(5) |
+ EBU_ADDR_SEL_REGEN, ebu_host->ebu + EBU_ADDR_SEL(i));
+ }
+
+ nand_set_flash_node(&ebu_host->chip, dev->of_node);
+ mtd = nand_to_mtd(&ebu_host->chip);
+ mtd->dev.parent = dev;
+ ebu_host->dev = dev;
+
+ platform_set_drvdata(pdev, ebu_host);
+ nand_set_controller_data(&ebu_host->chip, ebu_host);
+
+ nand = &ebu_host->chip;
+ nand->controller = &ebu_host->controller;
+ nand->controller->ops = &ebu_nand_controller_ops;
+
+ /* Scan to find existence of the device */
+ ret = nand_scan(&ebu_host->chip, 1);
+ if (ret)
+ goto err_cleanup_dma;
+
+ ret = mtd_device_register(mtd, NULL, 0);
+ if (ret)
+ goto err_clean_nand;
+
+ return 0;
+
+err_clean_nand:
+ nand_cleanup(&ebu_host->chip);
+err_cleanup_dma:
+ ebu_dma_cleanup(ebu_host);
+ clk_disable_unprepare(ebu_host->clk);
+
+ return ret;
+}
+
+static int ebu_nand_remove(struct platform_device *pdev)
+{
+ struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
+
+ mtd_device_unregister(nand_to_mtd(&ebu_host->chip));
+ nand_cleanup(&ebu_host->chip);
+ ebu_nand_disable(&ebu_host->chip);
+ ebu_dma_cleanup(ebu_host);
+ clk_disable_unprepare(ebu_host->clk);
+
+ return 0;
+}
+
+static const struct of_device_id ebu_nand_match[] = {
+ { .compatible = "intel,nand-controller", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ebu_nand_match);
+
+static struct platform_driver ebu_nand_driver = {
+ .probe = ebu_nand_probe,
+ .remove = ebu_nand_remove,
+ .driver = {
+ .name = "intel-nand-controller",
+ .of_match_table = ebu_nand_match,
+ },
+
+};
+module_platform_driver(ebu_nand_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vadivel Murugan R <vadivel.muruganx.ramuthevar@intel.com>");
+MODULE_DESCRIPTION("Intel's LGM External Bus NAND Controller driver");
--
2.11.0
^ permalink raw reply related
* [PATCH v9 0/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
From: Ramuthevar,Vadivel MuruganX @ 2020-05-28 5:12 UTC (permalink / raw)
To: linux-kernel, linux-mtd, devicetree, miquel.raynal
Cc: richard, vigneshr, arnd, brendanhiggins, tglx, boris.brezillon,
anders.roxell, masonccyang, robh+dt, linux-mips, hauke.mehrtens,
andriy.shevchenko, qi-ming.wu, cheol.yong.kim,
Ramuthevar,Vadivel MuruganX
This patch adds the new IP of Nand Flash Controller(NFC) support
on Intel's Lightning Mountain(LGM) SoC.
DMA is used for burst data transfer operation, also DMA HW supports
aligned 32bit memory address and aligned data access by default.
DMA burst of 8 supported. Data register used to support the read/write
operation from/to device.
NAND controller also supports in-built HW ECC engine.
NAND controller driver implements ->exec_op() to replace legacy hooks,
these specific call-back method to execute NAND operations.
Thanks Boris, Andy, Arnd and Rob for the review comments and suggestions.
---
v9:
No change
v8:
- fix the kbuild bot warnings
- correct the typo's
v7:
- indentation issue is fixed
- add error check for retrieve the resource from dt
v6:
- update EBU_ADDR_SELx register base value build it from DT
- Add tabs in in Kconfig
v5:
- replace by 'HSNAND_CLE_OFFS | HSNAND_CS_OFFS' to NAND_WRITE_CMD and NAND_WRITE_ADDR
- remove the unused macros
- update EBU_ADDR_MASK(x) macro
- update the EBU_ADDR_SELx register values to be written
v4:
- add ebu_nand_cs structure for multiple-CS support
- mask/offset encoding for 0x51 value
- update macro HSNAND_CTL_ENABLE_ECC
- drop the op argument and un-used macros.
- updated the datatype and macros
- add function disable nand module
- remove ebu_host->dma_rx = NULL;
- rename MMIO address range variables to ebu and hsnand
- implement ->setup_data_interface()
- update label err_cleanup_nand and err_cleanup_dma
- add return value check in the nand_remove function
- add/remove tabs and spaces as per coding standard
- encoded CS ids by reg property
v3:
- Add depends on MACRO in Kconfig
- file name update in Makefile
- file name update to intel-nand-controller
- modification of MACRO divided like EBU, HSNAND and NAND
- add NAND_ALE_OFFS, NAND_CLE_OFFS and NAND_CS_OFFS
- rename lgm_ to ebu_ and _va suffix is removed in the whole file
- rename structure and varaibles as per review comments.
- remove lgm_read_byte(), lgm_dev_ready() and cmd_ctrl() un-used function
- update in exec_op() as per review comments
- rename function lgm_dma_exit() by lgm_dma_cleanup()
- hardcoded magic value for base and offset replaced by MACRO defined
- mtd_device_unregister() + nand_cleanup() instead of nand_release()
v2:
- implement the ->exec_op() to replaces the legacy hook-up.
- update the commit message
- add MIPS maintainers and xway_nand driver author in CC
v1:
- initial version
dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC
---
v9:
- Rob's review comments address
- dual licensed
- compatible change
- add reg-names
- drop clock-names and clock-cells
- correct typo's
v8:
No change
v7:
- Rob's review comments addressed
- dt-schema build issue fixed with upgraded dt-schema
v6:
- Rob's review comments addressed in YAML file
- add addr_sel0 and addr_sel1 reg-names in YAML example
v5:
- add the example in YAML file
v4:
- No change
v3:
- No change
v2:
YAML compatible string update to intel, lgm-nand-controller
v1:
- initial version
Ramuthevar Vadivel Murugan (2):
dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC
mtd: rawnand: Add NAND controller support on Intel LGM SoC
.../devicetree/bindings/mtd/intel,lgm-nand.yaml | 93 +++
drivers/mtd/nand/raw/Kconfig | 8 +
drivers/mtd/nand/raw/Makefile | 1 +
drivers/mtd/nand/raw/intel-nand-controller.c | 747 +++++++++++++++++++++
4 files changed, 849 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c
--
2.11.0
^ permalink raw reply
* [PATCH v9 1/2] dt-bindings: mtd: Add Nand Flash Controller support for Intel LGM SoC
From: Ramuthevar,Vadivel MuruganX @ 2020-05-28 5:12 UTC (permalink / raw)
To: linux-kernel, linux-mtd, devicetree, miquel.raynal
Cc: richard, vigneshr, arnd, brendanhiggins, tglx, boris.brezillon,
anders.roxell, masonccyang, robh+dt, linux-mips, hauke.mehrtens,
andriy.shevchenko, qi-ming.wu, cheol.yong.kim,
Ramuthevar Vadivel Murugan
In-Reply-To: <20200528051211.3063-1-vadivel.muruganx.ramuthevar@linux.intel.com>
From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
Add YAML file for dt-bindings to support NAND Flash Controller
on Intel's Lightning Mountain SoC.
Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
.../devicetree/bindings/mtd/intel,lgm-nand.yaml | 93 ++++++++++++++++++++++
1 file changed, 93 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
diff --git a/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml b/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
new file mode 100644
index 000000000000..8672d03b4e6a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/intel,lgm-nand.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel LGM SoC NAND Controller Device Tree Bindings
+
+allOf:
+ - $ref: "nand-controller.yaml"
+
+maintainers:
+ - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
+
+properties:
+ compatible:
+ const: intel,lgm-nand
+
+ reg:
+ maxItems: 6
+
+ reg-names:
+ items:
+ - const: ebunand
+ - const: hsnand
+ - const: nand_cs0
+ - const: nand_cs1
+ - const: addr_sel0
+ - const: addr_sel1
+
+ clocks:
+ maxItems: 1
+
+ dmas:
+ maxItems: 2
+
+ dma-names:
+ items:
+ - const: tx
+ - const: rx
+
+patternProperties:
+ "^nand@[a-f0-9]+$":
+ type: object
+ properties:
+ reg:
+ minimum: 0
+ maximum: 7
+
+ nand-ecc-mode: true
+
+ nand-ecc-algo:
+ const: hw
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - clocks
+ - dmas
+ - dma-names
+
+additionalProperties: false
+
+examples:
+ - |
+ nand-controller@e0f00000 {
+ compatible = "intel,lgm-nand";
+ reg = <0xe0f00000 0x100>,
+ <0xe1000000 0x300>,
+ <0xe1400000 0x8000>,
+ <0xe1c00000 0x1000>,
+ <0x17400000 0x4>,
+ <0x17c00000 0x4>;
+ reg-names = "ebunand", "hsnand", "nand_cs0", "nand_cs1",
+ "addr_sel0", "addr_sel1";
+ clocks = <&cgu0 125>;
+ dmas = <&dma0 8>, <&dma0 9>;
+ dma-names = "tx", "rx";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ nand@0 {
+ reg = <0>;
+ nand-on-flash-bbt;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ };
+ };
+
+...
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 06/12] PM / devfreq: Add cpu based scaling support to passive_governor
From: Chanwoo Choi @ 2020-05-28 5:03 UTC (permalink / raw)
To: Andrew-sh.Cheng, MyungJoo Ham, Kyungmin Park, Rob Herring,
Mark Rutland, Matthias Brugger, Rafael J . Wysocki, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Liam Girdwood, Mark Brown
Cc: linux-pm, devicetree, linux-arm-kernel, linux-mediatek,
linux-kernel, srv_heupstream, Saravana Kannan, Sibi Sankar
In-Reply-To: <20200520034307.20435-7-andrew-sh.cheng@mediatek.com>
Hi Andrew-sh.Cheng,
Thanks for your posting. I like this approach absolutely.
I think that it is necessary. When I developed the embedded product,
I needed this feature always.
I add the comments on below.
On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
> From: Saravana Kannan <skannan@codeaurora.org>
>
> Many CPU architectures have caches that can scale independent of the
> CPUs. Frequency scaling of the caches is necessary to make sure that the
> cache is not a performance bottleneck that leads to poor performance and
> power. The same idea applies for RAM/DDR.
>
> To achieve this, this patch adds support for cpu based scaling to the
> passive governor. This is accomplished by taking the current frequency
> of each CPU frequency domain and then adjust the frequency of the cache
> (or any devfreq device) based on the frequency of the CPUs. It listens
> to CPU frequency transition notifiers to keep itself up to date on the
> current CPU frequency.
>
> To decide the frequency of the device, the governor does one of the
> following:
> * Derives the optimal devfreq device opp from required-opps property of
> the parent cpu opp_table.
>
> * Scales the device frequency in proportion to the CPU frequency. So, if
> the CPUs are running at their max frequency, the device runs at its
> max frequency. If the CPUs are running at their min frequency, the
> device runs at its min frequency. It is interpolated for frequencies
> in between.
>
> Andrew-sh.Cheng change
> dev_pm_opp_xlate_opp to dev_pm_opp_xlate_required_opp devfreq->max_freq
> to devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value
> for kernel-5.7
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> [Sibi: Integrated cpu-freqmap governor into passive_governor]
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
> drivers/devfreq/Kconfig | 2 +
> drivers/devfreq/governor_passive.c | 278 ++++++++++++++++++++++++++++++++++---
> include/linux/devfreq.h | 40 +++++-
> 3 files changed, 299 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0b1df12e0f21..d9067950af6a 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
> device. This governor does not change the frequency by itself
> through sysfs entries. The passive governor recommends that
> devfreq device uses the OPP table to get the frequency/voltage.
> + Alternatively the governor can also be chosen to scale based on
> + the online CPUs current frequency.
>
> comment "DEVFREQ Drivers"
>
> diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> index 2d67d6c12dce..7dcda02a5bb7 100644
> --- a/drivers/devfreq/governor_passive.c
> +++ b/drivers/devfreq/governor_passive.c
> @@ -8,11 +8,89 @@
> */
>
> #include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> #include <linux/device.h>
> #include <linux/devfreq.h>
> +#include <linux/slab.h>
> #include "governor.h"
>
> -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
Need to change 'unsigned int' to 'unsigned long'.
> + unsigned int cpu)
> +{
> + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
Better to define them separately as following and then need to rename
the variable. Usually, use the 'min_freq' and 'max_freq' word for
the minimum/maximum frequency.
unsigned int cpu_min_freq, cpu_max_freq, cpu_curr_freq, cpu_percent;
unsigned long dev_min_freq, dev_max_freq, dev_max_state,
The devfreq used 'unsigned long'. The cpufreq used 'unsigned long'
and 'unsigned int'. You need to handle them properly.
> + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + unsigned long *freq_table = devfreq->profile->freq_table;
In this function, use 'cpu' work for cpufreq and use 'dev' for devfreq.
So, I think 'dev_freq_table' is proper name instead of 'freq_table'
for the readability.
freq_table -> dev_freq_table
> + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
In the get_target_freq_with_devfreq(), use 'p_opp' indicating
the OPP of parent device. For the consistency, I think that
use 'p_opp' instead of 'cpu_opp'.
> + unsigned long cpu_freq, freq;
Define the 'cpu_freq' on above with cpu_min_freq/cpu_max_freq definition.
cpu_freq -> cpu_curr_freq.
> +
> + if (!cpu_state || cpu_state->first_cpu != cpu ||
> + !cpu_state->opp_table || !devfreq->opp_table)
> + return 0;
> +
> + cpu_freq = cpu_state->freq * 1000;
> + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
> + if (IS_ERR(cpu_opp))
> + return 0;
> +
> + opp = dev_pm_opp_xlate_required_opp(cpu_state->opp_table,
> + devfreq->opp_table, cpu_opp);
> + dev_pm_opp_put(cpu_opp);
> +
> + if (!IS_ERR(opp)) {
> + freq = dev_pm_opp_get_freq(opp);
> + dev_pm_opp_put(opp);
Better to add the 'out' goto statement.
If you use 'goto out', you can reduce the one indentation
without 'else' statement.
> + } else {
As I commented, when dev_pm_opp_xlate_required_opp() return successfully
, use 'goto out'. We can remove 'else' and then reduce the unneeded indentation.
> + /* Use Interpolation if required opps is not available */
> + cpu_min = cpu_state->min_freq;
> + cpu_max = cpu_state->max_freq;
> + cpu_freq = cpu_state->freq;
> +
> + if (freq_table) {
> + /* Get minimum frequency according to sorting order */
> + max_state = freq_table[devfreq->profile->max_state - 1];
> + if (freq_table[0] < max_state) {
> + dev_min = freq_table[0];
> + dev_max = max_state;
> + } else {
> + dev_min = max_state;
> + dev_max = freq_table[0];
> + }
> + } else {
> + if (devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value
> + <= devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value)
> + return 0;
> + dev_min =
> + devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value;
> + dev_max =
> + devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value;
I think it is not proper to access the variable of pm_qos structure directly.
Instead of direct access, you have to use the exported PM QoS function such as
- pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY);
- pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY);
> + }
> + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
> + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> + }
I think that you better to add 'out' jump label as following:
out:
> +
> + return freq;
> +}
> +
> +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + unsigned int cpu, target_freq = 0;
Need to define 'target_freq' with 'unsigned long' type.
> +
> + for_each_online_cpu(cpu)
> + target_freq = max(target_freq,
> + xlate_cpufreq_to_devfreq(p_data, cpu));
> +
> + *freq = target_freq;
> +
> + return 0;
> +}
> +
> +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
> unsigned long *freq)
> {
> struct devfreq_passive_data *p_data
> @@ -23,16 +101,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> int i, count, ret = 0;
>
> /*
> - * If the devfreq device with passive governor has the specific method
> - * to determine the next frequency, should use the get_target_freq()
> - * of struct devfreq_passive_data.
> - */
> - if (p_data->get_target_freq) {
> - ret = p_data->get_target_freq(devfreq, freq);
> - goto out;
> - }
> -
> - /*
> * If the parent and passive devfreq device uses the OPP table,
> * get the next frequency by using the OPP table.
> */
> @@ -102,6 +170,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> return ret;
> }
>
> +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> + unsigned long *freq)
> +{
> + struct devfreq_passive_data *p_data =
> + (struct devfreq_passive_data *)devfreq->data;
> + int ret;
> +
> + /*
> + * If the devfreq device with passive governor has the specific method
> + * to determine the next frequency, should use the get_target_freq()
> + * of struct devfreq_passive_data.
> + */
> + if (p_data->get_target_freq)
> + return p_data->get_target_freq(devfreq, freq);
> +
> + switch (p_data->parent_type) {
> + case DEVFREQ_PARENT_DEV:
> + ret = get_target_freq_with_devfreq(devfreq, freq);
> + break;
> + case CPUFREQ_PARENT_DEV:
> + ret = get_target_freq_with_cpufreq(devfreq, freq);
> + break;
> + default:
> + ret = -EINVAL;
> + dev_err(&devfreq->dev, "Invalid parent type\n");
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
> {
> int ret;
> @@ -156,6 +255,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
> + unsigned long event, void *ptr)
> +{
> + struct devfreq_passive_data *data =
> + container_of(nb, struct devfreq_passive_data, nb);
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct devfreq_cpu_state *cpu_state;
> + struct cpufreq_freqs *freq = ptr;
How about changing 'freq' to 'cpu_freqs'?
In the drivers/cpufreq/cpufreq.c, use 'freqs' name indicating
the instance of 'struct cpufreq_freqs'. And in order to
identfy, how about adding 'cpu_' prefix for variable name?
> + unsigned int current_freq;
Need to define curr_freq with 'unsigned long' type
and better to use 'curr_freq' variable name.
> + int ret;
> +
> + if (event != CPUFREQ_POSTCHANGE || !freq ||
> + !data->cpu_state[freq->policy->cpu])
> + return 0;
> +
> + cpu_state = data->cpu_state[freq->policy->cpu];
> + if (cpu_state->freq == freq->new)
> + return 0;
> +
> + /* Backup current freq and pre-update cpu state freq*/
> + current_freq = cpu_state->freq;
> + cpu_state->freq = freq->new;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret) {
> + cpu_state->freq = current_freq;
> + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq *devfreq = (struct devfreq *)data->this;
> + struct device *dev = devfreq->dev.parent;
> + struct opp_table *opp_table = NULL;
> + struct devfreq_cpu_state *state;
For the readability, I thinkt 'cpu_state' is proper instead of 'state'.
> + struct cpufreq_policy *policy;
> + struct device *cpu_dev;
> + unsigned int cpu;
> + int ret;
> +
> + get_online_cpus();
Add blank line.
> + data->nb.notifier_call = cpufreq_passive_notifier_call;
> + ret = cpufreq_register_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret) {
> + dev_err(dev, "Couldn't register cpufreq notifier.\n");
> + data->nb.notifier_call = NULL;
> + goto out;
> + }
> +
> + /* Populate devfreq_cpu_state */
> + for_each_online_cpu(cpu) {
> + if (data->cpu_state[cpu])
> + continue;
> +
> + policy = cpufreq_cpu_get(cpu);
cpufreq_cpu_get() might return 'NULL'. I think you need to handle
return value as following:
if (!policy) {
ret = -EINVAL;
goto out;
} else if (PTR_ERR(policy) == -EPROBE_DEFER) {
goto out;
} else if (IS_ERR(policy) {
ret = PTR_ERR(policy);
dev_err(dev, "Couldn't get the cpufreq_poliy.\n");
goto out;
}
If cpufreq_cpu_get() return successfully, to do next.
It reduces the one indentaion.
> + if (policy) {
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + cpu_dev = get_cpu_device(cpu);
> + if (!cpu_dev) {
> + dev_err(dev, "Couldn't get cpu device.\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> + if (IS_ERR(devfreq->opp_table)) {
> + ret = PTR_ERR(opp_table);
> + goto out;
> + }
> +
> + state->dev = cpu_dev;
> + state->opp_table = opp_table;
> + state->first_cpu = cpumask_first(policy->related_cpus);
> + state->freq = policy->cur;
> + state->min_freq = policy->cpuinfo.min_freq;
> + state->max_freq = policy->cpuinfo.max_freq;
> + data->cpu_state[cpu] = state;
Add blank line.
> + cpufreq_cpu_put(policy);
> + } else {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
> + }
Add blank line.
> +out:
> + put_online_cpus();
> + if (ret)
> + return ret;
> +
> + /* Update devfreq */
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret)
> + dev_err(dev, "Couldn't update the frequency.\n");
> +
> + return ret;
> +}
> +
> +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
> +{
> + struct devfreq_passive_data *data = *p_data;
> + struct devfreq_cpu_state *cpu_state;
> + int cpu;
> +
> + if (data->nb.notifier_call)
> + cpufreq_unregister_notifier(&data->nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> +
> + for_each_possible_cpu(cpu) {
> + cpu_state = data->cpu_state[cpu];
> + if (cpu_state) {
> + if (cpu_state->opp_table)
> + dev_pm_opp_put_opp_table(cpu_state->opp_table);
> + kfree(cpu_state);
> + cpu_state = NULL;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int devfreq_passive_event_handler(struct devfreq *devfreq,
> unsigned int event, void *data)
> {
> @@ -165,7 +398,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> struct notifier_block *nb = &p_data->nb;
> int ret = 0;
>
> - if (!parent)
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
> return -EPROBE_DEFER;
If you modify the devfreq_passive_event_handler() as following,
you can move this condition for DEVFREQ_PARENT_DEV into
(register|unregister)_parent_dev_notifier.
switch (event) {
case DEVFREQ_GOV_START:
ret = register_parent_dev_notifier(p_data);
break;
case DEVFREQ_GOV_STOP:
ret = unregister_parent_dev_notifier(p_data);
break;
default:
ret = -EINVAL;
break;
}
return ret;
>
> switch (event) {
> @@ -173,13 +406,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> if (!p_data->this)
> p_data->this = devfreq;
>
> - nb->notifier_call = devfreq_passive_notifier_call;
> - ret = devfreq_register_notifier(parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER);
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> + nb->notifier_call = devfreq_passive_notifier_call;
> + ret = devfreq_register_notifier(parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER);
> + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> + ret = cpufreq_passive_register(&p_data);
I think that we better to collect the code related to notifier registration
into one function like devfreq_pass_register_notifier() instead of
cpufreq_passive_register() as following: I think it is more simple and readable.
If you have more proper function name of register_parent_dev_notifier,
please give your opinion.
int register_parent_dev_notifier(struct devfreq_passive_data **p_data)
switch (p_data->parent_type) {
case DEVFREQ_PARENT_DEV:
nb->notifier_call = devfreq_passive_notifier_call;
ret = devfreq_register_notifier(parent, nb,
break;
case CPUFREQ_PARENT_DEV:
cpufreq_register_notifier(...)
...
break;
}
> + } else {
> + ret = -EINVAL;
> + }
> break;
> case DEVFREQ_GOV_STOP:
> - WARN_ON(devfreq_unregister_notifier(parent, nb,
> - DEVFREQ_TRANSITION_NOTIFIER));
> + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> + WARN_ON(devfreq_unregister_notifier(parent, nb,
> + DEVFREQ_TRANSITION_NOTIFIER));
> + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> + cpufreq_passive_unregister(&p_data);
> + else
> + ret = -EINVAL;
ditto. unregister_parent_dev_notifier(struct devfreq_passive_data **p_data)
> break;
> default:
> break;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index a4b19d593151..04ce576fd6f1 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -278,6 +278,32 @@ struct devfreq_simple_ondemand_data {
>
> #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> /**
> + * struct devfreq_cpu_state - holds the per-cpu state
> + * @freq: the current frequency of the cpu.
> + * @min_freq: the min frequency of the cpu.
> + * @max_freq: the max frequency of the cpu.
> + * @first_cpu: the cpumask of the first cpu of a policy.
> + * @dev: reference to cpu device.
> + * @opp_table: reference to cpu opp table.
> + *
> + * This structure stores the required cpu_state of a cpu.
> + * This is auto-populated by the governor.
> + */
> +struct devfreq_cpu_state {> + unsigned int freq;
It is better to change from 'freq' to 'curr_freq'
for more correct expression.
> + unsigned int min_freq;
> + unsigned int max_freq;
> + unsigned int first_cpu;
> + struct device *dev;
How about changing the name 'dev' to 'cpu_dev'?
> + struct opp_table *opp_table;
> +};
devfreq_cpu_state is only handled by within driver/devfreq/governor_passive.c.
So, you can move it into drivers/devfreq/governor_passive.c
and just add the definition into include/linux/devfreq.h as following:
It is able to prevent the access of variable of 'struct devfreq_cpu_state'
outside.
struct devfreq_cpu_state;
> +
> +enum devfreq_parent_dev_type {
> + DEVFREQ_PARENT_DEV,
> + CPUFREQ_PARENT_DEV,
> +};
> +
> +/**
> * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
> * and devfreq_add_device
> * @parent: the devfreq instance of parent device.
> @@ -288,13 +314,15 @@ struct devfreq_simple_ondemand_data {
> * using governors except for passive governor.
> * If the devfreq device has the specific method to decide
> * the next frequency, should use this callback.
> - * @this: the devfreq instance of own device.
> - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> + * @parent_type parent type of the device
Need to add ':' at the end of word. -> "parent_type:".
> + * @this: the devfreq instance of own device.
> + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
I knew that you make them with same indentation.
But, actually, it is not related to this patch like clean-up code.
Even if it is not pretty, you better to don't touch 'this' and 'nb' indentaion.
> + * @cpu_state: the state min/max/current frequency of all online cpu's
> *
> * The devfreq_passive_data have to set the devfreq instance of parent
> * device with governors except for the passive governor. But, don't need to
> - * initialize the 'this' and 'nb' field because the devfreq core will handle
> - * them.
> + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
> + * will handle them.
> */
> struct devfreq_passive_data {
> /* Should set the devfreq instance of parent device */
> @@ -303,9 +331,13 @@ struct devfreq_passive_data {
> /* Optional callback to decide the next frequency of passvice device */
> int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
>
> + /* Should set the type of parent device */
> + enum devfreq_parent_dev_type parent_type;
> +
> /* For passive governor's internal use. Don't need to set them */
> struct devfreq *this;
> struct notifier_block nb;
> + struct devfreq_cpu_state *cpu_state[NR_CPUS];
> };
> #endif
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 2/3] arm64: dts: sparx5: Add hwmon temperature sensor
From: Alexandre Belloni @ 2020-05-28 3:39 UTC (permalink / raw)
To: Rob Herring
Cc: Lars Povlsen, Guenter Roeck, SoC Team, Jean Delvare,
Microchip Linux Driver Support, linux-hwmon, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20200528022931.GA3238321@bogus>
Hi Rob,
On 27/05/2020 20:29:31-0600, Rob Herring wrote:
> On Wed, May 13, 2020 at 03:41:39PM +0200, Lars Povlsen wrote:
> > This adds a hwmon temperature node sensor to the Sparx5 SoC.
> >
> > Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> > ---
> > arch/arm64/boot/dts/microchip/sparx5.dtsi | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/microchip/sparx5.dtsi b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > index f09a49c41ce19..b5f2d088af30e 100644
> > --- a/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > +++ b/arch/arm64/boot/dts/microchip/sparx5.dtsi
> > @@ -233,5 +233,11 @@ i2c1: i2c@600103000 {
> > clock-frequency = <100000>;
> > clocks = <&ahb_clk>;
> > };
> > +
> > + tmon0: tmon@610508110 {
> > + compatible = "microchip,sparx5-temp";
> > + reg = <0x6 0x10508110 0xc>;
>
> These nodes are all very odd with a couple of registers spread out at
> randomish addresses. DT nodes should roughly correlate to h/w blocks,
> not sets of registers for a driver like this seems to be.
>
The DT nodes correlates to HW block, this and the previous families of
SoCs were designed with packed registers. There is no padding between HW
block registers.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Dongchun Zhu @ 2020-05-28 3:34 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rob Herring, Linus Walleij, Bartosz Golaszewski,
Mauro Carvalho Chehab, Andy Shevchenko, Mark Rutland,
Nicolas Boichat, Tomasz Figa, Matthias Brugger, Cao Bing Bu,
srv_heupstream, moderated list:ARM/Mediatek SoC support,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE, Sj Huang,
Linux Media Mailing List, devicetree, Louis Kuo,
Shengnan Wang (王圣男), dongchun.zhu
In-Reply-To: <20200527211628.GT7618@paasikivi.fi.intel.com>
Hi Sakari, Rob,
On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> Hi Rob, Dongchun,
>
> On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > + properties:
> > > > > + endpoint:
> > > > > + type: object
> > > > > + additionalProperties: false
> > > > > +
> > > > > + properties:
> > >
> > > Actually I wonder whether we need to declare 'clock-lanes' here?
> >
> > Yes, if you are using it.
>
> Dongchun, can you confirm the chip has a single data and a single clock
> lane and that it does not support lane reordering?
>
From the datasheet, 'MIPI inside the OV02A10 provides one single
uni-directional clock lane and one bi-directional data lane solution for
communication links between components inside a mobile device.
The data lane has full support for HS(uni-directional) and
LP(bi-directional) data transfer mode.'
The sensor doesn't support lane reordering, so 'clock-lanes' property
would not be added in next release.
> So if there's nothing to convey to the driver, also the data-lanes should
> be removed IMO.
>
However, 'data-lanes' property may still be required.
It is known that either data-lanes or clock-lanes is an array of
physical data lane indexes. Position of an entry determines the logical
lane number, while the value of an entry indicates physical lane, e.g.,
for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
the clock lane is on hardware lane 0.
As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
support lane reordering, so here we shall use 'data-lanes = <1>' as
there is only a clock lane for OV02A10.
Reminder:
If 'data-lanes' property is not present, the driver would assume
four-lane operation. This means for one-lane or two-lane operation, this
property must be present and set to the right physical lane indexes.
If the hardware does not support lane reordering, monotonically
incremented values shall be used from 0 or 1 onwards, depending on
whether or not there is also a clock lane.
^ permalink raw reply
* Re: [PATCH v3 3/5] drm: panel: Add Xingbangda XBD599 panel (ST7703 controller)
From: Samuel Holland @ 2020-05-28 3:31 UTC (permalink / raw)
To: Ondrej Jirman, linux-sunxi, Thierry Reding, Sam Ravnborg,
David Airlie, Daniel Vetter, Rob Herring, Maxime Ripard,
Chen-Yu Tsai, Linus Walleij, Icenowy Zheng
Cc: dri-devel, devicetree, linux-kernel, linux-arm-kernel,
Martijn Braam, Luca Weiss, Bhushan Shah
In-Reply-To: <20200513212451.1919013-4-megous@megous.com>
On 5/13/20 4:24 PM, Ondrej Jirman wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
>
> Xingbangda XBD599 is a 5.99" 720x1440 MIPI-DSI IPS LCD panel made by
> Xingbangda, which is used on PinePhone final assembled phones.
>
> It is based on Sitronix ST7703 LCD controller.
>
> Add support for it.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
> drivers/gpu/drm/panel/Kconfig | 10 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-sitronix-st7703.c | 386 ++++++++++++++++++
> 3 files changed, 397 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-sitronix-st7703.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 39055c1f0e2f..b7bc157b0612 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -395,6 +395,16 @@ config DRM_PANEL_SITRONIX_ST7701
> ST7701 controller for 480X864 LCD panels with MIPI/RGB/SPI
> system interfaces.
>
> +config DRM_PANEL_SITRONIX_ST7703
> + tristate "Sitronix ST7703 panel driver"
> + depends on OF
> + depends on DRM_MIPI_DSI
> + depends on BACKLIGHT_CLASS_DEVICE
> + help
> + Say Y here if you want to enable support for the Sitronix
> + ST7703 controller for 720X1440 LCD panels with MIPI/RGB/SPI
> + system interfaces.
> +
> config DRM_PANEL_SITRONIX_ST7789V
> tristate "Sitronix ST7789V panel"
> depends on OF && SPI
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index de74f282c433..47f4789a8685 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
> obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
> +obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
> obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> obj-$(CONFIG_DRM_PANEL_SONY_ACX424AKP) += panel-sony-acx424akp.o
> obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> new file mode 100644
> index 000000000000..092dd73c86d0
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
> @@ -0,0 +1,386 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for Sitronix ST7703 MIPI DSI panel
> + *
> + * Copyright (C) 2020 Ondrej Jirman <megous@megous.com>
> + * Copyright (C) 2019-2020 Icenowy Zheng <icenowy@aosc.io>
> + *
> + * Based on panel-rocktech-jh057n00900.c, which is:
> + * Copyright (C) Purism SPC 2019
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +
> +/* Manufacturer specific Commands send via DSI */
> +#define ST7703_CMD_ALL_PIXEL_OFF 0x22
> +#define ST7703_CMD_ALL_PIXEL_ON 0x23
> +#define ST7703_CMD_SETDISP 0xB2
> +#define ST7703_CMD_SETRGBIF 0xB3
> +#define ST7703_CMD_SETCYC 0xB4
> +#define ST7703_CMD_SETBGP 0xB5
> +#define ST7703_CMD_SETVCOM 0xB6
> +#define ST7703_CMD_SETOTP 0xB7
> +#define ST7703_CMD_SETPOWER_EXT 0xB8
> +#define ST7703_CMD_SETEXTC 0xB9
> +#define ST7703_CMD_SETMIPI 0xBA
> +#define ST7703_CMD_SETVDC 0xBC
> +#define ST7703_CMD_SETSCR 0xC0
> +#define ST7703_CMD_SETPOWER 0xC1
> +#define ST7703_CMD_UNK_C6 0xC6
> +#define ST7703_CMD_SETPANEL 0xCC
> +#define ST7703_CMD_SETGAMMA 0xE0
> +#define ST7703_CMD_SETEQ 0xE3
> +#define ST7703_CMD_SETGIP1 0xE9
> +#define ST7703_CMD_SETGIP2 0xEA
> +
> +struct st7703_panel_desc {
> + const struct drm_display_mode *mode;
> + unsigned int lanes;
> + unsigned long flags;
> + enum mipi_dsi_pixel_format format;
> + const char *const *supply_names;
> + unsigned int num_supplies;
> +};
> +
> +struct st7703 {
> + struct device *dev;
> + struct drm_panel panel;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data *supplies;
> + const struct st7703_panel_desc *desc;
> + bool prepared;
> +};
> +
> +static inline struct st7703 *panel_to_st7703(struct drm_panel *panel)
> +{
> + return container_of(panel, struct st7703, panel);
> +}
> +
> +#define dsi_dcs_write_seq(dsi, cmd, seq...) do { \
> + static const u8 d[] = { seq }; \
> + int ret; \
> + ret = mipi_dsi_dcs_write(dsi, cmd, d, ARRAY_SIZE(d)); \
> + if (ret < 0) \
> + return ret; \
> + } while (0)
> +
> +static int st7703_init_sequence(struct st7703 *ctx)
> +{
> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> + struct device *dev = ctx->dev;
> + int ret;
> +
> + /*
> + * Init sequence was supplied by the panel vendor.
> + */
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETEXTC,
> + 0xF1, 0x12, 0x83);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETMIPI,
> + 0x33, 0x81, 0x05, 0xF9, 0x0E, 0x0E, 0x20, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x44, 0x25,
> + 0x00, 0x91, 0x0a, 0x00, 0x00, 0x02, 0x4F, 0x11,
> + 0x00, 0x00, 0x37);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETPOWER_EXT,
> + 0x25, 0x22, 0x20, 0x03);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETRGBIF,
> + 0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
> + 0x00, 0x00);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETSCR,
> + 0x73, 0x73, 0x50, 0x50, 0x00, 0xC0, 0x08, 0x70,
> + 0x00);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0xF0);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETEQ,
> + 0x00, 0x00, 0x0B, 0x0B, 0x10, 0x10, 0x00, 0x00,
> + 0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
> + dsi_dcs_write_seq(dsi, 0xC6, 0x01, 0x00, 0xFF, 0xFF, 0x00);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETPOWER,
> + 0x74, 0x00, 0x32, 0x32, 0x77, 0xF1, 0xFF, 0xFF,
> + 0xCC, 0xCC, 0x77, 0x77);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETBGP, 0x07, 0x07);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETVCOM, 0x2C, 0x2C);
> + dsi_dcs_write_seq(dsi, 0xBF, 0x02, 0x11, 0x00);
> +
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETGIP1,
> + 0x82, 0x10, 0x06, 0x05, 0xA2, 0x0A, 0xA5, 0x12,
> + 0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
> + 0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
> + 0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
> + 0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
> + 0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
> + 0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETGIP2,
> + 0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
> + 0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
> + 0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
> + 0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x0A,
> + 0xA5, 0x00, 0x00, 0x00, 0x00);
> + dsi_dcs_write_seq(dsi, ST7703_CMD_SETGAMMA,
> + 0x00, 0x09, 0x0D, 0x23, 0x27, 0x3C, 0x41, 0x35,
> + 0x07, 0x0D, 0x0E, 0x12, 0x13, 0x10, 0x12, 0x12,
> + 0x18, 0x00, 0x09, 0x0D, 0x23, 0x27, 0x3C, 0x41,
> + 0x35, 0x07, 0x0D, 0x0E, 0x12, 0x13, 0x10, 0x12,
> + 0x12, 0x18);
> + msleep(20);
> +
> + ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to exit sleep mode\n");
> + return ret;
> + }
> + msleep(250);
> +
> + ret = mipi_dsi_dcs_set_display_on(dsi);
Since you have the complementary call to mipi_dsi_dcs_set_display_off in
st7703_disable, I would suggest calling this from st7703_enable, after the call
to st7703_init_sequence. [but see below about moving all of this to prepare.]
> + if (ret)
> + return ret;
> + msleep(50);
This is the last step of the init sequence. According to table 8-1 of the
manual, T10 needs no delay. What is this delay for?
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Panel init sequence done\n");
> + return 0;
> +}
> +
> +static int st7703_prepare(struct drm_panel *panel)
> +{
> + struct st7703 *ctx = panel_to_st7703(panel);
> + int ret;
> +
> + if (ctx->prepared)
> + return 0;
> +
> + ret = regulator_bulk_enable(ctx->desc->num_supplies, ctx->supplies);
> + if (ret)
> + return ret;
> +
> + DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
> + gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> + usleep_range(20, 40);
> + gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> + msleep(20);
> +
> + ctx->prepared = true;
> +
> + return 0;
> +}
> +
> +static int st7703_enable(struct drm_panel *panel)
> +{
> + struct st7703 *ctx = panel_to_st7703(panel);
> + int ret;
> +
> + ret = st7703_init_sequence(ctx);
v1 had this function called from prepare, not enable. From reading the comments
in drm_panel.h, that seems to be the right place for it.
> + if (ret < 0) {
> + DRM_DEV_ERROR(ctx->dev, "Panel init sequence failed: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int st7703_disable(struct drm_panel *panel)
> +{
> + struct st7703 *ctx = panel_to_st7703(panel);
> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> + return mipi_dsi_dcs_set_display_off(dsi);
The datasheet suggests in "5.6 Power on/off Sequence" that there also needs to
be a call to mipi_dsi_dcs_enter_sleep_mode() here.
> +}
> +
> +static int st7703_unprepare(struct drm_panel *panel)
> +{
> + struct st7703 *ctx = panel_to_st7703(panel);
> +
> + if (!ctx->prepared)
> + return 0;
> +
> + gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> + regulator_bulk_disable(ctx->desc->num_supplies, ctx->supplies);
> + ctx->prepared = false;
> +
> + return 0;
> +}
> +
> +static int st7703_get_modes(struct drm_panel *panel,
> + struct drm_connector *connector)
> +{
> + struct st7703 *ctx = panel_to_st7703(panel);
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(connector->dev, ctx->desc->mode);
> + if (!mode) {
> + DRM_DEV_ERROR(ctx->dev, "Failed to add mode\n");
> + return -ENOMEM;
> + }
> +
> + drm_mode_set_name(mode);
> +
> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
This line...
> + connector->display_info.width_mm = mode->width_mm;
> + connector->display_info.height_mm = mode->height_mm;
> + drm_mode_probed_add(connector, mode);
> +
> + return 1;
> +}
> +
> +static const struct drm_panel_funcs st7703_drm_funcs = {
> + .prepare = st7703_prepare,
> + .enable = st7703_enable,
> + .disable = st7703_disable,
> + .unprepare = st7703_unprepare,
> + .get_modes = st7703_get_modes,
> +};
> +
> +static const struct drm_display_mode xbd599_mode = {
> + .hdisplay = 720,
> + .hsync_start = 720 + 40,
> + .hsync_end = 720 + 40 + 40,
> + .htotal = 720 + 40 + 40 + 40,
> + .vdisplay = 1440,
> + .vsync_start = 1440 + 18,
> + .vsync_end = 1440 + 18 + 10,
> + .vtotal = 1440 + 18 + 10 + 17,
> + .vrefresh = 60,
> + .clock = 69000,
> + .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +
> + .width_mm = 68,
> + .height_mm = 136,
> + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
...duplicates this line. I'm not sure which is the better place to put it; panel
drivers seem evenly split. But it doesn't need to be in both places.
Cheers,
Samuel
> +};
> +
> +static const char * const xbd599_supply_names[] = {
> + "iovcc",
> + "vcc",
> +};
> +
> +static const struct st7703_panel_desc xbd599_desc = {
> + .mode = &xbd599_mode,
> + .lanes = 4,
> + .flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE,
> + .format = MIPI_DSI_FMT_RGB888,
> + .supply_names = xbd599_supply_names,
> + .num_supplies = ARRAY_SIZE(xbd599_supply_names),
> +};
> +
> +static int st7703_probe(struct mipi_dsi_device *dsi)
> +{
> + const struct st7703_panel_desc *desc;
> + struct device *dev = &dsi->dev;
> + struct st7703 *ctx;
> + int i, ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->dev = dev;
> + ctx->desc = desc = of_device_get_match_data(dev);
> +
> + dsi->mode_flags = desc->flags;
> + dsi->format = desc->format;
> + dsi->lanes = desc->lanes;
> +
> + ctx->supplies = devm_kcalloc(&dsi->dev, desc->num_supplies,
> + sizeof(*ctx->supplies),
> + GFP_KERNEL);
> + if (!ctx->supplies)
> + return -ENOMEM;
> +
> + for (i = 0; i < desc->num_supplies; i++)
> + ctx->supplies[i].supply = desc->supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&dsi->dev, desc->num_supplies,
> + ctx->supplies);
> + if (ret < 0)
> + return ret;
> +
> + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(ctx->reset_gpio)) {
> + DRM_DEV_ERROR(dev, "Can't get reset gpio\n");
> + return PTR_ERR(ctx->reset_gpio);
> + }
> +
> + mipi_dsi_set_drvdata(dsi, ctx);
> +
> + drm_panel_init(&ctx->panel, &dsi->dev, &st7703_drm_funcs,
> + DRM_MODE_CONNECTOR_DSI);
> +
> + ret = drm_panel_of_backlight(&ctx->panel);
> + if (ret)
> + return ret;
> +
> + drm_panel_add(&ctx->panel);
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "mipi_dsi_attach failed. Is host ready?\n");
> + drm_panel_remove(&ctx->panel);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void st7703_shutdown(struct mipi_dsi_device *dsi)
> +{
> + struct st7703 *ctx = mipi_dsi_get_drvdata(dsi);
> + int ret;
> +
> + ret = drm_panel_unprepare(&ctx->panel);
> + if (ret < 0)
> + DRM_DEV_ERROR(&dsi->dev, "Failed to unprepare panel: %d\n",
> + ret);
> +}
> +
> +static int st7703_remove(struct mipi_dsi_device *dsi)
> +{
> + struct st7703 *ctx = mipi_dsi_get_drvdata(dsi);
> + int ret;
> +
> + st7703_shutdown(dsi);
> +
> + ret = mipi_dsi_detach(dsi);
> + if (ret < 0)
> + DRM_DEV_ERROR(&dsi->dev, "Failed to detach from DSI host: %d\n",
> + ret);
> +
> + drm_panel_remove(&ctx->panel);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id st7703_of_match[] = {
> + { .compatible = "xingbangda,xbd599", .data = &xbd599_desc },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, st7703_of_match);
> +
> +static struct mipi_dsi_driver st7703_driver = {
> + .probe = st7703_probe,
> + .remove = st7703_remove,
> + .shutdown = st7703_shutdown,
> + .driver = {
> + .name = "st7703",
> + .of_match_table = st7703_of_match,
> + },
> +};
> +module_mipi_dsi_driver(st7703_driver);
> +
> +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> +MODULE_DESCRIPTION("DRM driver for Sitronix ST7703 MIPI DSI panel");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* [PATCH 2/4] arm64: dts: imx8m: change ocotp node name on i.MX8M SoCs
From: Anson Huang @ 2020-05-28 3:12 UTC (permalink / raw)
To: robh+dt, shawnguo, s.hauer, kernel, festevam, daniel.baluta,
leonard.crestez, peng.fan, aford173, jun.li, shengjiu.wang,
horia.geanta, aisheng.dong, fugang.duan, agx, l.stach,
andrew.smirnov, devicetree, linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1590635570-8541-1-git-send-email-Anson.Huang@nxp.com>
Change OCOTP node name from ocotp-ctrl to efuse to be compliant with
yaml schema, it requires the nodename to be one of "eeprom|efuse|nvram".
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 +-
arch/arm64/boot/dts/freescale/imx8mn.dtsi | 2 +-
arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 +-
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index aaf6e71..740cc62 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -467,7 +467,7 @@
reg = <0x30340000 0x10000>;
};
- ocotp: ocotp-ctrl@30350000 {
+ ocotp: efuse@30350000 {
compatible = "fsl,imx8mm-ocotp", "syscon";
reg = <0x30350000 0x10000>;
clocks = <&clk IMX8MM_CLK_OCOTP_ROOT>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
index 9a4b65a..0625cc8 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi
@@ -374,7 +374,7 @@
reg = <0x30340000 0x10000>;
};
- ocotp: ocotp-ctrl@30350000 {
+ ocotp: efuse@30350000 {
compatible = "fsl,imx8mn-ocotp", "fsl,imx8mm-ocotp", "syscon";
reg = <0x30350000 0x10000>;
clocks = <&clk IMX8MN_CLK_OCOTP_ROOT>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 23e9a4c..c248e7f 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -340,7 +340,7 @@
reg = <0x30340000 0x10000>;
};
- ocotp: ocotp-ctrl@30350000 {
+ ocotp: efuse@30350000 {
compatible = "fsl,imx8mp-ocotp", "syscon";
reg = <0x30350000 0x10000>;
clocks = <&clk IMX8MP_CLK_OCOTP_ROOT>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 978f812..b156cd5 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -539,7 +539,7 @@
};
};
- ocotp: ocotp-ctrl@30350000 {
+ ocotp: efuse@30350000 {
compatible = "fsl,imx8mq-ocotp", "syscon";
reg = <0x30350000 0x10000>;
clocks = <&clk IMX8MQ_CLK_OCOTP_ROOT>;
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox