* [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-1-k.konieczny@partner.samsung.com>
Reuse opp core code for setting bus clock and voltage. As a side
effect this allow useage of coupled regulators feature (required
for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
uses regulator_set_voltage_triplet() for setting regulator voltage
while the old code used regulator_set_voltage_tol() with fixed
tolerance. This patch also removes no longer needed parsing of DT
property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
it).
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
1 file changed, 66 insertions(+), 106 deletions(-)
diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 486cc5b422f1..7fc4f76bd848 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -25,7 +25,6 @@
#include <linux/slab.h>
#define DEFAULT_SATURATION_RATIO 40
-#define DEFAULT_VOLTAGE_TOLERANCE 2
struct exynos_bus {
struct device *dev;
@@ -37,9 +36,9 @@ struct exynos_bus {
unsigned long curr_freq;
- struct regulator *regulator;
+ struct opp_table *opp_table;
+
struct clk *clk;
- unsigned int voltage_tolerance;
unsigned int ratio;
};
@@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
{
struct exynos_bus *bus = dev_get_drvdata(dev);
struct dev_pm_opp *new_opp;
- unsigned long old_freq, new_freq, new_volt, tol;
int ret = 0;
-
- /* Get new opp-bus instance according to new bus clock */
+ /*
+ * New frequency for bus may not be exactly matched to opp, adjust
+ * *freq to correct value.
+ */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(new_opp);
}
- new_freq = dev_pm_opp_get_freq(new_opp);
- new_volt = dev_pm_opp_get_voltage(new_opp);
dev_pm_opp_put(new_opp);
- old_freq = bus->curr_freq;
-
- if (old_freq == new_freq)
- return 0;
- tol = new_volt * bus->voltage_tolerance / 100;
-
/* Change voltage and frequency according to new OPP level */
mutex_lock(&bus->lock);
+ ret = dev_pm_opp_set_rate(dev, *freq);
+ if (!ret)
+ bus->curr_freq = *freq;
- if (old_freq < new_freq) {
- ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
- if (ret < 0) {
- dev_err(bus->dev, "failed to set voltage\n");
- goto out;
- }
- }
-
- ret = clk_set_rate(bus->clk, new_freq);
- if (ret < 0) {
- dev_err(dev, "failed to change clock of bus\n");
- clk_set_rate(bus->clk, old_freq);
- goto out;
- }
-
- if (old_freq > new_freq) {
- ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
- if (ret < 0) {
- dev_err(bus->dev, "failed to set voltage\n");
- goto out;
- }
- }
- bus->curr_freq = new_freq;
-
- dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
- old_freq, new_freq, clk_get_rate(bus->clk));
-out:
mutex_unlock(&bus->lock);
return ret;
@@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
if (ret < 0)
dev_warn(dev, "failed to disable the devfreq-event devices\n");
- if (bus->regulator)
- regulator_disable(bus->regulator);
+ if (bus->opp_table)
+ dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
+
clk_disable_unprepare(bus->clk);
}
@@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
{
struct exynos_bus *bus = dev_get_drvdata(dev);
struct dev_pm_opp *new_opp;
- unsigned long old_freq, new_freq;
- int ret = 0;
+ int ret;
- /* Get new opp-bus instance according to new bus clock */
+ /*
+ * New frequency for bus may not be exactly matched to opp, adjust
+ * *freq to correct value.
+ */
new_opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(new_opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(new_opp);
}
- new_freq = dev_pm_opp_get_freq(new_opp);
dev_pm_opp_put(new_opp);
- old_freq = bus->curr_freq;
-
- if (old_freq == new_freq)
- return 0;
-
/* Change the frequency according to new OPP level */
mutex_lock(&bus->lock);
+ ret = dev_pm_opp_set_rate(dev, *freq);
+ if (!ret)
+ bus->curr_freq = *freq;
- ret = clk_set_rate(bus->clk, new_freq);
- if (ret < 0) {
- dev_err(dev, "failed to set the clock of bus\n");
- goto out;
- }
-
- *freq = new_freq;
- bus->curr_freq = new_freq;
-
- dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
- old_freq, new_freq, clk_get_rate(bus->clk));
-out:
mutex_unlock(&bus->lock);
return ret;
@@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
struct exynos_bus *bus)
{
struct device *dev = bus->dev;
- int i, ret, count, size;
-
- /* Get the regulator to provide each bus with the power */
- bus->regulator = devm_regulator_get(dev, "vdd");
- if (IS_ERR(bus->regulator)) {
- dev_err(dev, "failed to get VDD regulator\n");
- return PTR_ERR(bus->regulator);
- }
-
- ret = regulator_enable(bus->regulator);
- if (ret < 0) {
- dev_err(dev, "failed to enable VDD regulator\n");
- return ret;
- }
+ int i, count, size;
/*
* Get the devfreq-event devices to get the current utilization of
@@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
count = devfreq_event_get_edev_count(dev);
if (count < 0) {
dev_err(dev, "failed to get the count of devfreq-event dev\n");
- ret = count;
- goto err_regulator;
+ return count;
}
+
bus->edev_count = count;
size = sizeof(*bus->edev) * count;
bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
- if (!bus->edev) {
- ret = -ENOMEM;
- goto err_regulator;
- }
+ if (!bus->edev)
+ return -ENOMEM;
for (i = 0; i < count; i++) {
bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
- if (IS_ERR(bus->edev[i])) {
- ret = -EPROBE_DEFER;
- goto err_regulator;
- }
+ if (IS_ERR(bus->edev[i]))
+ return -EPROBE_DEFER;
}
/*
@@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
bus->ratio = DEFAULT_SATURATION_RATIO;
- if (of_property_read_u32(np, "exynos,voltage-tolerance",
- &bus->voltage_tolerance))
- bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
-
return 0;
-
-err_regulator:
- regulator_disable(bus->regulator);
-
- return ret;
}
static int exynos_bus_parse_of(struct device_node *np,
- struct exynos_bus *bus)
+ struct exynos_bus *bus, bool passive)
{
struct device *dev = bus->dev;
+ struct opp_table *opp_table;
+ const char *vdd = "vdd";
struct dev_pm_opp *opp;
unsigned long rate;
int ret;
@@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
return ret;
}
+ if (!passive) {
+ opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
+ if (IS_ERR(opp_table)) {
+ ret = PTR_ERR(opp_table);
+ dev_err(dev, "failed to set regulators %d\n", ret);
+ goto err_clk;
+ }
+
+ bus->opp_table = opp_table;
+ }
+
/* Get the freq and voltage from OPP table to scale the bus freq */
ret = dev_pm_opp_of_add_table(dev);
if (ret < 0) {
dev_err(dev, "failed to get OPP table\n");
- goto err_clk;
+ goto err_regulator;
}
rate = clk_get_rate(bus->clk);
@@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
ret = PTR_ERR(opp);
goto err_opp;
}
+
bus->curr_freq = dev_pm_opp_get_freq(opp);
dev_pm_opp_put(opp);
@@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
err_opp:
dev_pm_opp_of_remove_table(dev);
+
+err_regulator:
+ if (bus->opp_table) {
+ dev_pm_opp_put_regulators(bus->opp_table);
+ bus->opp_table = NULL;
+ }
+
err_clk:
clk_disable_unprepare(bus->clk);
@@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
struct exynos_bus *bus;
int ret, max_state;
unsigned long min_freq, max_freq;
+ bool passive = false;
if (!np) {
dev_err(dev, "failed to find devicetree node\n");
@@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
if (!bus)
return -ENOMEM;
+
mutex_init(&bus->lock);
bus->dev = &pdev->dev;
platform_set_drvdata(pdev, bus);
+ node = of_parse_phandle(dev->of_node, "devfreq", 0);
+ if (node) {
+ of_node_put(node);
+ passive = true;
+ }
/* Parse the device-tree to get the resource information */
- ret = exynos_bus_parse_of(np, bus);
+ ret = exynos_bus_parse_of(np, bus, passive);
if (ret < 0)
return ret;
@@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
goto err;
}
- node = of_parse_phandle(dev->of_node, "devfreq", 0);
- if (node) {
- of_node_put(node);
+ if (passive)
goto passive;
- } else {
- ret = exynos_bus_parent_parse_of(np, bus);
- }
+
+ ret = exynos_bus_parent_parse_of(np, bus);
if (ret < 0)
goto err;
@@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
err:
dev_pm_opp_of_remove_table(dev);
+ if (bus->opp_table) {
+ dev_pm_opp_put_regulators(bus->opp_table);
+ bus->opp_table = NULL;
+ }
+
clk_disable_unprepare(bus->clk);
return ret;
--
2.22.0
^ permalink raw reply related
* [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Krzysztof Kozlowski, Kukjin Kim, Kyungmin Park, Mark Rutland,
MyungJoo Ham, Nishanth Menon, Rob Herring, Stephen Boyd,
Viresh Kumar, devicetree, linux-arm-kernel, linux-kernel,
linux-pm, linux-samsung-soc
In-Reply-To: <20190715120416.3561-1-k.konieczny@partner.samsung.com>
Add enable regulators to dev_pm_opp_set_regulators() and disable
regulators to dev_pm_opp_put_regulators(). This prepares for
converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
--
Changes in v2:
- move regulator enable and disable into loop
---
drivers/opp/core.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..069c5cf8827e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
goto free_regulators;
}
+ ret = regulator_enable(reg);
+ if (ret < 0)
+ goto disable;
+
opp_table->regulators[i] = reg;
}
@@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
return opp_table;
+disable:
+ regulator_put(reg);
+ --i;
+
free_regulators:
- while (i != 0)
- regulator_put(opp_table->regulators[--i]);
+ for (; i >= 0; --i) {
+ regulator_disable(opp_table->regulators[i]);
+ regulator_put(opp_table->regulators[i]);
+ }
kfree(opp_table->regulators);
opp_table->regulators = NULL;
@@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));
- for (i = opp_table->regulator_count - 1; i >= 0; i--)
+ for (i = opp_table->regulator_count - 1; i >= 0; i--) {
+ regulator_disable(opp_table->regulators[i]);
regulator_put(opp_table->regulators[i]);
+ }
_free_set_opp_data(opp_table);
--
2.22.0
^ permalink raw reply related
* [PATCH v2 0/4] add coupled regulators for Exynos5422/5800
From: Kamil Konieczny @ 2019-07-15 12:04 UTC (permalink / raw)
To: k.konieczny
Cc: Mark Rutland, Nishanth Menon, linux-samsung-soc, Rob Herring,
linux-arm-kernel, Bartlomiej Zolnierkiewicz, Stephen Boyd,
Viresh Kumar, linux-pm, linux-kernel, Krzysztof Kozlowski,
Chanwoo Choi, Kyungmin Park, Kukjin Kim, MyungJoo Ham, devicetree,
Marek Szyprowski
In-Reply-To: <CGME20190715120430eucas1p1dd216e552403899e614845295373e467@eucas1p1.samsung.com>
Hi,
The main purpose of this patch series is to add coupled regulators for
Exynos5422/5800 to keep constrain on voltage difference between vdd_arm
and vdd_int to be at most 300mV. In exynos-bus instead of using
regulator_set_voltage_tol() with default voltage tolerance it should be
used regulator_set_voltage_triplet() with volatege range, and this is
already present in opp/core.c code, so it can be reused. While at this,
move setting regulators into opp/core.
This patchset was tested on Odroid XU3.
The last patch depends on two previous.
Changes in v2:
- improve regulators enable/disable code in opp/core as suggested by
Viresh Kumar
- add new patch for remove unused dt-bindings as suggested by Krzysztof
Kozlowski
Regards,
Kamil
Kamil Konieczny (3):
opp: core: add regulators enable and disable
devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
dt-bindings: devfreq: exynos-bus: remove unused property
Marek Szyprowski (1):
ARM: dts: exynos: add initial data for coupled regulators for
Exynos5422/5800
.../bindings/devfreq/exynos-bus.txt | 2 -
arch/arm/boot/dts/exynos5420.dtsi | 34 ++--
arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +
arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +
arch/arm/boot/dts/exynos5800.dtsi | 32 ++--
drivers/devfreq/exynos-bus.c | 172 +++++++-----------
drivers/opp/core.c | 18 +-
7 files changed, 122 insertions(+), 144 deletions(-)
--
2.22.0
^ permalink raw reply
* Re: [PATCH v2 1/2] arm64: dts: imx8mq: Add MIPI D-PHY
From: Abel Vesa @ 2019-07-15 11:10 UTC (permalink / raw)
To: Guido Günther
Cc: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, dl-linux-imx,
Pavel Machek, Angus Ainslie (Purism), Lucas Stach, Anson Huang,
Carlo Caione, Andrey Smirnov, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <30c7622bf590670190b93c9b5b6dd1e8f809bbb2.1563187253.git.agx@sigxcpu.org>
On 19-07-15 12:43:05, Guido Günther wrote:
> Add a node for the Mixel MIPI D-PHY, "disabled" by default.
>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Acked-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index d09b808eff87..891ee7578c2d 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -728,6 +728,19 @@
> status = "disabled";
> };
>
> + dphy: dphy@30a00300 {
> + compatible = "fsl,imx8mq-mipi-dphy";
> + reg = <0x30a00300 0x100>;
> + clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> + clock-names = "phy_ref";
> + assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> + assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> + assigned-clock-rates = <24000000>;
We have the following in the clk-imx8mq in the vendor tree:
clk_set_parent(clks[IMX8MQ_VIDEO_PLL1_BYPASS], clks[IMX8MQ_VIDEO_PLL1]);
This unbypasses the video pll 1. And then we also have this:
/* config video_pll1 clock */
clk_set_parent(clks[IMX8MQ_VIDEO_PLL1_REF_SEL], clks[IMX8MQ_CLK_27M]);
clk_set_rate(clks[IMX8MQ_VIDEO_PLL1], 593999999);
But none of that is acceptable upstream since the clock provider should not
use clock consumer API.
So please update the assigned-clock* properties to something like this:
assigned-clocks = <&clk IMX8MQ_VIDEO_PLL1_REF_SEL>,
<&clk IMX8MQ_VIDEO_PLL1_BYPASS>,
<&clk IMX8MQ_CLK_DSI_PHY_REF>,
<&clk IMX8MQ_VIDEO_PLL1>;
assigned-clock-parents = <&clk IMX8MQ_CLK_27M>,
<&clk IMX8MQ_VIDEO_PLL1>,
<&clk IMX8MQ_VIDEO_PLL1_OUT>
<0>;
assigned-clock-rates = <0>,
<0>,
<24000000>,
<593999999>;
I've written this without testing, so please do test it on your setup.
> + #phy-cells = <0>;
> + power-domains = <&pgc_mipi>;
> + status = "disabled";
> + };
> +
> i2c1: i2c@30a20000 {
> compatible = "fsl,imx8mq-i2c", "fsl,imx21-i2c";
> reg = <0x30a20000 0x10000>;
> --
> 2.20.1
>
^ permalink raw reply
* RE: [PATCH v9 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
From: Pawel Laszczak @ 2019-07-15 11:00 UTC (permalink / raw)
To: Felipe Balbi, devicetree@vger.kernel.org
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
hdegoede@redhat.com, heikki.krogerus@linux.intel.com,
robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org,
jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, Suresh Punnoose,
peter.chen@nxp.com, Jayshri Dajiram Pawar, Rahul Kumar
In-Reply-To: <877e8tm25r.fsf@linux.intel.com>
Hi Felipe
>
>Hi,
>
>Pawel Laszczak <pawell@cadence.com> writes:
>> +static void cdns3_gadget_config(struct cdns3_device *priv_dev)
>> +{
>> + struct cdns3_usb_regs __iomem *regs = priv_dev->regs;
>> + u32 reg;
>> +
>> + cdns3_ep0_config(priv_dev);
>> +
>> + /* enable interrupts for endpoint 0 (in and out) */
>> + writel(EP_IEN_EP_OUT0 | EP_IEN_EP_IN0, ®s->ep_ien);
>> +
>> + /*
>> + *Driver need modify LFPS minimal U1 Exit time for 0x00024505 revision
>
>comment style
>
>> + * of controller
>> + */
>> + if (priv_dev->dev_ver == DEV_VER_TI_V1) {
>
>this version is really only for TI? And there's another only for NXP?
Yes, from driver point of view the only difference for this version is LFPS
parameter. It's depend on the kind of used PHY and should be set on
integration level. Default value is incorrect for DEV_VER_TI_V1 version and
it cause some issue for one of the Link Layer test.
>
>+#define DEV_VER_NXP_V1 0x00024502
>+#define DEV_VER_TI_V1 0x00024509
>+#define DEV_VER_V2 0x0002450C
>+#define DEV_VER_V3 0x0002450d
>
>How do you actually decode this?
It's read from register: USB_CAP6
priv_dev->dev_ver = readl(&priv_dev->regs->usb_cap6);
But's only 3 less significant bytes are used as version.
>
>> +static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
>> +{
>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
>> + struct cdns3_endpoint *priv_ep;
>> + u32 bEndpointAddress;
>> + struct usb_ep *ep;
>> + int ret = 0;
>> +
>> + priv_dev->gadget_driver = NULL;
>> +
>> + priv_dev->onchip_used_size = 0;
>> + priv_dev->out_mem_is_allocated = 0;
>> + priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>> +
>> + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) {
>> + priv_ep = ep_to_cdns3_ep(ep);
>> + bEndpointAddress = priv_ep->num | priv_ep->dir;
>> + cdns3_select_ep(priv_dev, bEndpointAddress);
>> + writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd);
>> + ret = cdns3_handshake(&priv_dev->regs->ep_cmd,
>> + EP_CMD_EPRST, 0, 100);
>> + cdns3_free_trb_pool(priv_ep);
>
>are you sure you want to free your trb pool when a gadget driver is
>unloaded? One can easily fragment memory by constantly loading and
>unloading a gadget driver, no?
I think that such constantly loading/unloading will occurs only during
testing.
>
>How about you allocate the trb poll during cdns3 load and free it when
>cdns3 is unloaded?
This allocation is made only in cdns3_gadget_ep_enable, so memory
is allocated only for endpoint in use. We save a lot of memory, especially
for streams and ISOC endpoint.
Streams support is not implemented now but it will be added as separate
patch in the feature. It will require allocation multiple Transfer Rings.
The second issue are ISOC endpoints. For each ITP we need separate TRB.
So, for bInterval > 1 driver must allocate the quite big size of Transfer Ring.
During loading cdns3 we don't know which endpoint and how it will be used.
If someone from customers will complain about current implementation,
Then I will try to implement some improvement.
>
>> +static int cdns3_gadget_start(struct cdns3 *cdns)
>> +{
>> + struct cdns3_device *priv_dev;
>> + u32 max_speed;
>> + int ret;
>> +
>> + priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
>> + if (!priv_dev)
>> + return -ENOMEM;
>> +
>> + cdns->gadget_dev = priv_dev;
>> + priv_dev->sysdev = cdns->dev;
>> + priv_dev->dev = cdns->dev;
>> + priv_dev->regs = cdns->dev_regs;
>> +
>> + device_property_read_u16(priv_dev->dev, "cdns,on-chip-buff-size",
>> + &priv_dev->onchip_buffers);
>> +
>> + if (priv_dev->onchip_buffers <= 0) {
>> + u32 reg = readl(&priv_dev->regs->usb_cap2);
>> +
>> + priv_dev->onchip_buffers = USB_CAP2_ACTUAL_MEM_SIZE(reg);
>> + }
>> +
>> + if (!priv_dev->onchip_buffers)
>> + priv_dev->onchip_buffers = 256;
>> +
>> + max_speed = usb_get_maximum_speed(cdns->dev);
>> +
>> + /* Check the maximum_speed parameter */
>> + switch (max_speed) {
>> + case USB_SPEED_FULL:
>> + case USB_SPEED_HIGH:
>> + case USB_SPEED_SUPER:
>> + break;
>> + default:
>> + dev_err(cdns->dev, "invalid maximum_speed parameter %d\n",
>> + max_speed);
>> + /* fall through */
>> + case USB_SPEED_UNKNOWN:
>> + /* default to superspeed */
>> + max_speed = USB_SPEED_SUPER;
>> + break;
>> + }
>> +
>> + /* fill gadget fields */
>> + priv_dev->gadget.max_speed = max_speed;
>> + priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>> + priv_dev->gadget.ops = &cdns3_gadget_ops;
>> + priv_dev->gadget.name = "usb-ss-gadget";
>> + priv_dev->gadget.sg_supported = 1;
>> +
>> + spin_lock_init(&priv_dev->lock);
>> + INIT_WORK(&priv_dev->pending_status_wq,
>> + cdns3_pending_setup_status_handler);
>> +
>> + /* initialize endpoint container */
>> + INIT_LIST_HEAD(&priv_dev->gadget.ep_list);
>> + INIT_LIST_HEAD(&priv_dev->aligned_buf_list);
>> +
>> + ret = cdns3_init_eps(priv_dev);
>> + if (ret) {
>> + dev_err(priv_dev->dev, "Failed to create endpoints\n");
>> + goto err1;
>> + }
>> +
>> + /* allocate memory for setup packet buffer */
>> + priv_dev->setup_buf = dma_alloc_coherent(priv_dev->sysdev, 8,
>> + &priv_dev->setup_dma, GFP_DMA);
>> + if (!priv_dev->setup_buf) {
>> + ret = -ENOMEM;
>> + goto err2;
>> + }
>> +
>> + priv_dev->dev_ver = readl(&priv_dev->regs->usb_cap6);
>> +
>> + dev_dbg(priv_dev->dev, "Device Controller version: %08x\n",
>> + readl(&priv_dev->regs->usb_cap6));
>> + dev_dbg(priv_dev->dev, "USB Capabilities:: %08x\n",
>> + readl(&priv_dev->regs->usb_cap1));
>> + dev_dbg(priv_dev->dev, "On-Chip memory cnfiguration: %08x\n",
>> + readl(&priv_dev->regs->usb_cap2));
>> +
>> + priv_dev->dev_ver = GET_DEV_BASE_VERSION(priv_dev->dev_ver);
>> +
>> + priv_dev->zlp_buf = kzalloc(CDNS3_EP_ZLP_BUF_SIZE, GFP_KERNEL);
>> + if (!priv_dev->zlp_buf) {
>> + ret = -ENOMEM;
>> + goto err3;
>> + }
>> +
>> + /* add USB gadget device */
>> + ret = usb_add_gadget_udc(priv_dev->dev, &priv_dev->gadget);
>> + if (ret < 0) {
>> + dev_err(priv_dev->dev,
>> + "Failed to register USB device controller\n");
>> + goto err4;
>> + }
>> +
>> + return 0;
>> +err4:
>> + kfree(priv_dev->zlp_buf);
>> +err3:
>> + dma_free_coherent(priv_dev->sysdev, 8, priv_dev->setup_buf,
>> + priv_dev->setup_dma);
>> +err2:
>> + cdns3_free_all_eps(priv_dev);
>> +err1:
>> + cdns->gadget_dev = NULL;
>> + return ret;
>> +}
>> +
>> +static int __cdns3_gadget_init(struct cdns3 *cdns)
>> +{
>> + struct cdns3_device *priv_dev;
>> + int ret = 0;
>> +
>> + cdns3_drd_switch_gadget(cdns, 1);
>> + pm_runtime_get_sync(cdns->dev);
>> +
>> + ret = cdns3_gadget_start(cdns);
>> + if (ret)
>> + return ret;
>> +
>> + priv_dev = cdns->gadget_dev;
>> + ret = devm_request_threaded_irq(cdns->dev, cdns->dev_irq,
>> + cdns3_device_irq_handler,
>> + cdns3_device_thread_irq_handler,
>> + IRQF_SHARED, dev_name(cdns->dev), cdns);
>
>copied handlers here for commenting. Note that you don't have
>IRQF_ONESHOT:
I know, I can't use IRQF_ ONESHOT flag in this case. I have implemented
some code for masking/unmasking interrupts in cdns3_device_irq_handler.
Some priority interrupts should be handled ASAP so I can't blocked interrupt
Line.
>
>> +static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
>> +{
>> + struct cdns3_device *priv_dev;
>> + struct cdns3 *cdns = data;
>> + irqreturn_t ret = IRQ_NONE;
>> + unsigned long flags;
>> + u32 reg;
>> +
>> + priv_dev = cdns->gadget_dev;
>> + spin_lock_irqsave(&priv_dev->lock, flags);
>
>the top half handler runs in hardirq context. You don't need any locks
>here. Also IRQs are *already* disabled, you don't need to disable them again.
I will remove spin_lock_irqsave but I need to disable only some of the interrupts.
I disable interrupts associated with USB endpoints. Handling of them can be
deferred to thread handled.
>> +
>> + /* check USB device interrupt */
>> + reg = readl(&priv_dev->regs->usb_ists);
>> +
>> + if (reg) {
>> + writel(reg, &priv_dev->regs->usb_ists);
>> + cdns3_check_usb_interrupt_proceed(priv_dev, reg);
>> + ret = IRQ_HANDLED;
>
>now, because you _don't_ mask this interrupt, you're gonna have
>issues. Say we actually get both device and endpoint interrupts while
>the thread is already running with previous endpoint interrupts. Now
>we're gonna reenter the top half, because device interrupts are *not*
>masked, which will read usb_ists and handle it here.
Endpoint interrupts are masked in cdns3_device_irq_handler and stay masked
until they are not handled in threaded handler.
Of course, not all endpoint interrupts are masked, but only reported in ep_ists.
USB interrupt will be handled immediately.
Also, I can get next endpoint interrupt from not masked endpoint and driver also again wake
the thread. I saw such situation, but threaded interrupt handler has been working correct
in such situations.
In thread handler driver checks again which endpoint should be handled in ep_ists.
I think that such situation should also occurs during our LPM enter/exit test.
So, driver has been tested for such case. During this test driver during
transferring data generate a huge number of LPM interrupts which
are usb interrupts.
I can't block usb interrupts interrupts because:
/*
* WORKAROUND: CDNS3 controller has issue with hardware resuming
* from L1. To fix it, if any DMA transfer is pending driver
* must starts driving resume signal immediately.
*/
>
>Then it will ahead and read ep_ists and wake the thread that's already
>running.
>
>This hasn't really been tested, has it?
It has been tested. And I've haven't seen any issue with this.
>
>> +static irqreturn_t cdns3_device_thread_irq_handler(int irq, void *data)
>> +{
>> + struct cdns3_device *priv_dev;
>> + struct cdns3 *cdns = data;
>> + irqreturn_t ret = IRQ_NONE;
>> + unsigned long flags;
>> + u32 ep_ien;
>> + int bit;
>> + u32 reg;
>> +
>> + priv_dev = cdns->gadget_dev;
>> + spin_lock_irqsave(&priv_dev->lock, flags);
>
>Ideally, the _irqsave() here wouldn't be necessary (since this device's
>interrupts are already masked), but removing _irqsave() causes problem
>with networking gadgets. Just thought I'd leave a note here, nothing to
>change in this handler.
>
>> + reg = readl(&priv_dev->regs->ep_ists);
>> +
>> + /* handle default endpoint OUT */
>> + if (reg & EP_ISTS_EP_OUT0) {
>> + cdns3_check_ep0_interrupt_proceed(priv_dev, USB_DIR_OUT);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> + /* handle default endpoint IN */
>> + if (reg & EP_ISTS_EP_IN0) {
>> + cdns3_check_ep0_interrupt_proceed(priv_dev, USB_DIR_IN);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> + /* check if interrupt from non default endpoint, if no exit */
>> + reg &= ~(EP_ISTS_EP_OUT0 | EP_ISTS_EP_IN0);
>> + if (!reg)
>> + goto irqend;
>> +
>> + for_each_set_bit(bit, (unsigned long *)®,
>> + sizeof(u32) * BITS_PER_BYTE) {
>> + priv_dev->shadow_ep_en |= BIT(bit);
>> + cdns3_check_ep_interrupt_proceed(priv_dev->eps[bit]);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> + if (priv_dev->run_garbage_colector) {
>
>wait, what?
DMA require data buffer aligned to 8 bytes. So, if buffer data is not aligned
driver allocate aligned buffer for data and copy it from unaligned to
Aligned.
>
>ps: correct spelling is "collector" ;-)
Ok, thanks.
>
>> + struct cdns3_aligned_buf *buf, *tmp;
>> +
>> + list_for_each_entry_safe(buf, tmp, &priv_dev->aligned_buf_list,
>> + list) {
>> + if (!buf->in_use) {
>> + list_del(&buf->list);
>> +
>> + spin_unlock_irqrestore(&priv_dev->lock, flags);
>
>creates the possibility of a race condition
Why? In this place the buf can't be used.
>
>> + dma_free_coherent(priv_dev->sysdev, buf->size,
>> + buf->buf,
>> + buf->dma);
>> + spin_lock_irqsave(&priv_dev->lock, flags);
>> +
>> + kfree(buf);
>
>why do you even need this "garbage collector"?
I need to free not used memory. The once allocated buffer will be associated with
request, but if request.length will be increased in usb_request then driver will
must allocate the bigger buffer. As I remember I couldn't call dma_free_coherent
in interrupt context so I had to move it to thread handled. This flag was used to avoid
going through whole aligned_buf_list every time.
In most cases this part will never called int this place
>
>> +static int cdns3_gadget_suspend(struct cdns3 *cdns, bool do_wakeup)
>> +{
>> + cdns3_gadget_exit(cdns);
>
>so on suspend you completely teardown the entire controller? This means
>that a mounted mass storage gadget will, actually, disconnect from the
>host, no?
Yes It's true. I know that it's not good idea.
I will try to improve it.
>
>> diff --git a/drivers/usb/cdns3/trace.h b/drivers/usb/cdns3/trace.h
>> new file mode 100644
>> index 000000000000..1cc2abca320c
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/trace.h
>> @@ -0,0 +1,447 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * USBSS device controller driver.
>> + * Trace support header file.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Pawel Laszczak <pawell@cadence.com>
>> + */
>> +
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM cdns3
>> +
>> +#if !defined(__LINUX_CDNS3_TRACE) || defined(TRACE_HEADER_MULTI_READ)
>> +#define __LINUX_CDNS3_TRACE
>> +
>> +#include <linux/types.h>
>> +#include <linux/tracepoint.h>
>> +#include <asm/byteorder.h>
>> +#include <linux/usb/ch9.h>
>> +#include "core.h"
>> +#include "gadget.h"
>> +#include "debug.h"
>> +
>> +#define CDNS3_MSG_MAX 500
>> +
>> +TRACE_EVENT(cdns3_log,
>> + TP_PROTO(struct cdns3_device *priv_dev, struct va_format *vaf),
>> + TP_ARGS(priv_dev, vaf),
>> + TP_STRUCT__entry(
>> + __string(name, dev_name(priv_dev->dev))
>> + __dynamic_array(char, msg, CDNS3_MSG_MAX)
>> + ),
>> + TP_fast_assign(
>> + __assign_str(name, dev_name(priv_dev->dev));
>> + vsnprintf(__get_str(msg), CDNS3_MSG_MAX, vaf->fmt, *vaf->va);
>> + ),
>> + TP_printk("%s: %s", __get_str(name), __get_str(msg))
>> +);
>> +
>> +DECLARE_EVENT_CLASS(cdns3_log_doorbell,
>> + TP_PROTO(const char *ep_name, u32 ep_trbaddr),
>> + TP_ARGS(ep_name, ep_trbaddr),
>> + TP_STRUCT__entry(
>> + __string(name, ep_name)
>> + __field(u32, ep_trbaddr)
>> + ),
>> + TP_fast_assign(
>> + __assign_str(name, ep_name);
>> + __entry->ep_trbaddr = ep_trbaddr;
>> + ),
>> + TP_printk("%s, ep_trbaddr %08x", __get_str(name),
>
>nit-picking but... the event name will be printed on trace log. You
>don't need this "Ding Dong" string here :-p
I will remove it:
>
>> + TP_printk("%s: req: %p, req buff %p, length: %u/%u %s%s%s, status: %d,"
>> + cd " trb: [start:%d, end:%d: virt addr %pa], flags:%x ",
>> + __get_str(name), __entry->req, __entry->buf, __entry->actual,
>> + __entry->length,
>> + __entry->zero ? "zero | " : "",
>> + __entry->short_not_ok ? "short | " : "",
>> + __entry->no_interrupt ? "no int" : "",
>
>I guess you didn't really think the formatting through. Think about what
>happens if you get a request with only zero flag or only short flag. How
>will this log look like?
Like this:
cdns3_gadget_giveback: ep0: req: 0000000071a6a5f5, req buff 000000008d40c4db, length: 60/60 zero | , status: 0, trb: [start:0, end:0: virt addr (null)], flags:0
Is it something wrong with this?. Maybe one extra sign |.
Pawell
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: imx8mq-librem5: Enable MIPI D-PHY
From: Lucas Stach @ 2019-07-15 10:55 UTC (permalink / raw)
To: Guido Günther, Rob Herring, Mark Rutland, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Pavel Machek, Angus Ainslie (Purism), Abel Vesa,
Anson Huang, Carlo Caione, Andrey Smirnov, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <f80df8fcae320eb6eb4937fb5a07799fc610adae.1563187253.git.agx@sigxcpu.org>
Am Montag, den 15.07.2019, 12:43 +0200 schrieb Guido Günther:
> This enables the Mixel MIPI D-PHY on the Librem 5 devkit
>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Acked-by: Angus Ainslie (Purism) <angus@akkea.ca>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
> b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
> index 5179e22f5126..683a11035643 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
> @@ -174,6 +174,10 @@
> assigned-clock-rates = <786432000>, <722534400>;
> };
>
> +&dphy {
> + status = "okay";
> +};
> +
> &fec1 {
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_fec1>;
^ permalink raw reply
* Re: [PATCH v2 1/2] arm64: dts: imx8mq: Add MIPI D-PHY
From: Lucas Stach @ 2019-07-15 10:55 UTC (permalink / raw)
To: Guido Günther, Rob Herring, Mark Rutland, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Pavel Machek, Angus Ainslie (Purism), Abel Vesa,
Anson Huang, Carlo Caione, Andrey Smirnov, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <30c7622bf590670190b93c9b5b6dd1e8f809bbb2.1563187253.git.agx@sigxcpu.org>
Am Montag, den 15.07.2019, 12:43 +0200 schrieb Guido Günther:
> Add a node for the Mixel MIPI D-PHY, "disabled" by default.
>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Acked-by: Angus Ainslie (Purism) <angus@akkea.ca>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index d09b808eff87..891ee7578c2d 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -728,6 +728,19 @@
> status = "disabled";
> };
>
> + dphy: dphy@30a00300 {
> + compatible = "fsl,imx8mq-mipi-dphy";
> + reg = <0x30a00300 0x100>;
> + clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> + clock-names = "phy_ref";
> + assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> + assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> + assigned-clock-rates = <24000000>;
> + #phy-cells = <0>;
> + power-domains = <&pgc_mipi>;
> + status = "disabled";
> + };
> +
> i2c1: i2c@30a20000 {
> compatible = "fsl,imx8mq-i2c", "fsl,imx21-i2c";
> reg = <0x30a20000 0x10000>;
^ permalink raw reply
* [PATCH v2 2/2] arm64: dts: imx8mq-librem5: Enable MIPI D-PHY
From: Guido Günther @ 2019-07-15 10:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Pavel Machek, Guido Günther, Angus Ainslie (Purism),
Lucas Stach, Abel Vesa, Anson Huang, Carlo Caione, Andrey Smirnov,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <cover.1563187253.git.agx@sigxcpu.org>
This enables the Mixel MIPI D-PHY on the Librem 5 devkit
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Acked-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
index 5179e22f5126..683a11035643 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
@@ -174,6 +174,10 @@
assigned-clock-rates = <786432000>, <722534400>;
};
+&dphy {
+ status = "okay";
+};
+
&fec1 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_fec1>;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 1/2] arm64: dts: imx8mq: Add MIPI D-PHY
From: Guido Günther @ 2019-07-15 10:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Pavel Machek, Guido Günther, Angus Ainslie (Purism),
Lucas Stach, Abel Vesa, Anson Huang, Carlo Caione, Andrey Smirnov,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <cover.1563187253.git.agx@sigxcpu.org>
Add a node for the Mixel MIPI D-PHY, "disabled" by default.
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Acked-by: Angus Ainslie (Purism) <angus@akkea.ca>
---
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index d09b808eff87..891ee7578c2d 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -728,6 +728,19 @@
status = "disabled";
};
+ dphy: dphy@30a00300 {
+ compatible = "fsl,imx8mq-mipi-dphy";
+ reg = <0x30a00300 0x100>;
+ clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
+ clock-names = "phy_ref";
+ assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
+ assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
+ assigned-clock-rates = <24000000>;
+ #phy-cells = <0>;
+ power-domains = <&pgc_mipi>;
+ status = "disabled";
+ };
+
i2c1: i2c@30a20000 {
compatible = "fsl,imx8mq-i2c", "fsl,imx21-i2c";
reg = <0x30a20000 0x10000>;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 0/2] arm64: dts: imx8mq: Add DT node for the Mixel MIPI D-PHY
From: Guido Günther @ 2019-07-15 10:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Pavel Machek, Guido Günther, Angus Ainslie (Purism),
Lucas Stach, Abel Vesa, Anson Huang, Carlo Caione, Andrey Smirnov,
devicetree, linux-arm-kernel, linux-kernel
Now that the driver is in linux-next as of 20190624 let's have a DT node
for the i.MX8MQ and enable it on the Librem 5 devkit.
Changes from v1:
- Add Acked-by: form Angus, thanks!
Guido Günther (2):
arm64: dts: imx8mq: Add MIPI D-PHY
arm64: dts: imx8mq-librem5: Enable MIPI D-PHY
.../boot/dts/freescale/imx8mq-librem5-devkit.dts | 4 ++++
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 13 +++++++++++++
2 files changed, 17 insertions(+)
--
2.20.1
^ permalink raw reply
* [v4,2/2] Documentation: dt: binding: rtc: add binding for ftm alarm driver
From: Biwen Li @ 2019-07-15 10:15 UTC (permalink / raw)
To: a.zummo, alexandre.belloni, leoyang.li, robh+dt
Cc: linux-rtc, linux-kernel, xiaobo.xie, jiafei.pan, ran.wang_1,
mark.rutland, devicetree, Biwen Li
In-Reply-To: <20190715101520.22562-1-biwen.li@nxp.com>
The patch adds binding for ftm alarm driver
Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v4:
- add note about dts and kernel options
- add aliases in example
Change in v3:
- remove reg-names property
- correct cells number
Change in v2:
- replace ls1043a with ls1088a as example
- add rcpm node and fsl,rcpm-wakeup property
.../bindings/rtc/rtc-fsl-ftm-alarm.txt | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
diff --git a/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
new file mode 100644
index 000000000000..fb018065406c
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
@@ -0,0 +1,49 @@
+Freescale FlexTimer Module (FTM) Alarm
+
+Note:
+- The driver depends on RCPM driver
+ to wake up system in sleep.
+- Need stop using RTC_HCTOSYS or use the DT aliases
+ to ensure the driver is not used as the primary RTC.
+ (Select DT aliases defaultly)
+
+Required properties:
+- compatible : Should be "fsl,<chip>-ftm-alarm", the
+ supported chips include
+ "fsl,ls1012a-ftm-alarm"
+ "fsl,ls1021a-ftm-alarm"
+ "fsl,ls1028a-ftm-alarm"
+ "fsl,ls1043a-ftm-alarm"
+ "fsl,ls1046a-ftm-alarm"
+ "fsl,ls1088a-ftm-alarm"
+ "fsl,ls208xa-ftm-alarm"
+- reg : Specifies base physical address and size of the register sets for the
+ FlexTimer Module and base physical address of IP Powerdown Exception Control
+ Register.
+- interrupts : Should be the FlexTimer Module interrupt.
+- fsl,rcpm-wakeup property and rcpm node : Please refer
+ Documentation/devicetree/bindings/soc/fsl/rcpm.txt
+
+Optional properties:
+- big-endian: If the host controller is big-endian mode, specify this property.
+ The default endian mode is little-endian.
+
+Example:
+aliases {
+ ...
+ rtc1 = ftm_alarm0; /* Use flextimer alarm driver as /dev/rtc1 */
+ ...
+};
+
+rcpm: rcpm@1e34040 {
+ compatible = "fsl,ls1088a-rcpm", "fsl,qoriq-rcpm-2.1+";
+ reg = <0x0 0x1e34040 0x0 0x18>;
+ fsl,#rcpm-wakeup-cells = <6>;
+};
+
+ftm_alarm0: timer@2800000 {
+ compatible = "fsl,ls1088a-ftm-alarm";
+ reg = <0x0 0x2800000 0x0 0x10000>;
+ fsl,rcpm-wakeup = <&rcpm 0x0 0x0 0x0 0x0 0x4000 0x0>;
+ interrupts = <0 44 4>;
+};
--
2.17.1
^ permalink raw reply related
* [v4,1/2] rtc/fsl: add FTM alarm driver as the wakeup source
From: Biwen Li @ 2019-07-15 10:15 UTC (permalink / raw)
To: a.zummo, alexandre.belloni, leoyang.li, robh+dt
Cc: linux-rtc, linux-kernel, xiaobo.xie, jiafei.pan, ran.wang_1,
mark.rutland, devicetree, Biwen Li
For the paltforms including LS1012A, LS1021A, LS1028A, LS1043A,
LS1046A, LS1088A, LS208xA that has the FlexTimer
module, implementing alarm functions within RTC subsystem
to wakeup the system when system going to sleep (work with RCPM driver).
Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v4:
- clean code
- correct requesting irq
- register as a regular RTC driver
- change return value of ftm_rtc_set_alarm from -EINVAL to -ERANGE
- replace pr_err with dev_err
- sort alphabetically
- auto select RCPM driver
- correct UTC time in ftm_rtc_read_time
Change in v3:
- add some comments about clock source and errata
- adjust format
- replace endian with big_endian of struct ftm_rtc
- remove compatible "fsl,ftm-alarm"
Change in v2:
- remove code about setting rcpm
drivers/rtc/Kconfig | 15 ++
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-fsl-ftm-alarm.c | 332 ++++++++++++++++++++++++++++++++
3 files changed, 348 insertions(+)
create mode 100644 drivers/rtc/rtc-fsl-ftm-alarm.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 5b9b2eec1435..ac6728ba7895 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1322,6 +1322,21 @@ config RTC_DRV_IMXDI
This driver can also be built as a module, if so, the module
will be called "rtc-imxdi".
+config RTC_DRV_FSL_FTM_ALARM
+ tristate "Freescale FlexTimer alarm timer"
+ depends on ARCH_LAYERSCAPE
+ select FSL_RCPM
+ default y
+ help
+ For the FlexTimer in LS1012A, LS1021A, LS1028A, LS1043A, LS1046A,
+ LS1088A, LS208xA, we can use FTM as the wakeup source.
+
+ Say y here to enable FTM alarm support. The FTM alarm provides
+ alarm functions for wakeup system from deep sleep.
+
+ This driver can also be built as a module, if so, the module
+ will be called "rtc-fsl-ftm-alarm".
+
config RTC_DRV_MESON
tristate "Amlogic Meson RTC"
depends on (ARM && ARCH_MESON) || COMPILE_TEST
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 84deeddc8f14..5b1fa1d3a7e8 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_RTC_DRV_EFI) += rtc-efi.o
obj-$(CONFIG_RTC_DRV_EM3027) += rtc-em3027.o
obj-$(CONFIG_RTC_DRV_EP93XX) += rtc-ep93xx.o
obj-$(CONFIG_RTC_DRV_FM3130) += rtc-fm3130.o
+obj-$(CONFIG_RTC_DRV_FSL_FTM_ALARM) += rtc-fsl-ftm-alarm.o
obj-$(CONFIG_RTC_DRV_FTRTC010) += rtc-ftrtc010.o
obj-$(CONFIG_RTC_DRV_GENERIC) += rtc-generic.o
obj-$(CONFIG_RTC_DRV_GOLDFISH) += rtc-goldfish.o
diff --git a/drivers/rtc/rtc-fsl-ftm-alarm.c b/drivers/rtc/rtc-fsl-ftm-alarm.c
new file mode 100644
index 000000000000..41891bfa4939
--- /dev/null
+++ b/drivers/rtc/rtc-fsl-ftm-alarm.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Freescale FlexTimer Module (FTM) alarm device driver.
+ *
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ * Copyright 2019 NXP
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/fsl/ftm.h>
+#include <linux/rtc.h>
+#include <linux/time.h>
+
+#define FTM_SC_CLK(c) ((c) << FTM_SC_CLK_MASK_SHIFT)
+
+/*
+ * Select Fixed frequency clock (32KHz) as clock source
+ * of FlexTimer Module
+ */
+#define FTM_SC_CLKS_FIXED_FREQ 0x02
+#define FIXED_FREQ_CLK 32000
+
+/* Select 128 (2^7) as divider factor */
+#define MAX_FREQ_DIV (1 << FTM_SC_PS_MASK)
+
+/* Maximum counter value in FlexTimer's CNT registers */
+#define MAX_COUNT_VAL 0xffff
+
+struct ftm_rtc {
+ struct rtc_device *rtc_dev;
+ void __iomem *base;
+ bool big_endian;
+ u32 alarm_freq;
+};
+
+static inline u32 rtc_readl(struct ftm_rtc *dev, u32 reg)
+{
+ if (dev->big_endian)
+ return ioread32be(dev->base + reg);
+ else
+ return ioread32(dev->base + reg);
+}
+
+static inline void rtc_writel(struct ftm_rtc *dev, u32 reg, u32 val)
+{
+ if (dev->big_endian)
+ iowrite32be(val, dev->base + reg);
+ else
+ iowrite32(val, dev->base + reg);
+}
+
+static inline void ftm_counter_enable(struct ftm_rtc *rtc)
+{
+ u32 val;
+
+ /* select and enable counter clock source */
+ val = rtc_readl(rtc, FTM_SC);
+ val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
+ val |= (FTM_SC_PS_MASK | FTM_SC_CLK(FTM_SC_CLKS_FIXED_FREQ));
+ rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_counter_disable(struct ftm_rtc *rtc)
+{
+ u32 val;
+
+ /* disable counter clock source */
+ val = rtc_readl(rtc, FTM_SC);
+ val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
+ rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_irq_acknowledge(struct ftm_rtc *rtc)
+{
+ unsigned int timeout = 100;
+
+ /*
+ *Fix errata A-007728 for flextimer
+ * If the FTM counter reaches the FTM_MOD value between
+ * the reading of the TOF bit and the writing of 0 to
+ * the TOF bit, the process of clearing the TOF bit
+ * does not work as expected when FTMx_CONF[NUMTOF] != 0
+ * and the current TOF count is less than FTMx_CONF[NUMTOF].
+ * If the above condition is met, the TOF bit remains set.
+ * If the TOF interrupt is enabled (FTMx_SC[TOIE] = 1),the
+ * TOF interrupt also remains asserted.
+ *
+ * Above is the errata discription
+ *
+ * In one word: software clearing TOF bit not works when
+ * FTMx_CONF[NUMTOF] was seted as nonzero and FTM counter
+ * reaches the FTM_MOD value.
+ *
+ * The workaround is clearing TOF bit until it works
+ * (FTM counter doesn't always reache the FTM_MOD anyway),
+ * which may cost some cycles.
+ */
+ while ((FTM_SC_TOF & rtc_readl(rtc, FTM_SC)) && timeout--)
+ rtc_writel(rtc, FTM_SC, rtc_readl(rtc, FTM_SC) & (~FTM_SC_TOF));
+}
+
+static inline void ftm_irq_enable(struct ftm_rtc *rtc)
+{
+ u32 val;
+
+ val = rtc_readl(rtc, FTM_SC);
+ val |= FTM_SC_TOIE;
+ rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_irq_disable(struct ftm_rtc *rtc)
+{
+ u32 val;
+
+ val = rtc_readl(rtc, FTM_SC);
+ val &= ~FTM_SC_TOIE;
+ rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_reset_counter(struct ftm_rtc *rtc)
+{
+ /*
+ * The CNT register contains the FTM counter value.
+ * Reset clears the CNT register. Writing any value to COUNT
+ * updates the counter with its initial value, CNTIN.
+ */
+ rtc_writel(rtc, FTM_CNT, 0x00);
+}
+
+static void ftm_clean_alarm(struct ftm_rtc *rtc)
+{
+ ftm_counter_disable(rtc);
+
+ rtc_writel(rtc, FTM_CNTIN, 0x00);
+ rtc_writel(rtc, FTM_MOD, ~0U);
+
+ ftm_reset_counter(rtc);
+}
+
+static irqreturn_t ftm_rtc_alarm_interrupt(int irq, void *dev)
+{
+ struct ftm_rtc *rtc = dev;
+
+ ftm_irq_acknowledge(rtc);
+ ftm_irq_disable(rtc);
+ ftm_clean_alarm(rtc);
+
+ return IRQ_HANDLED;
+}
+
+static int ftm_rtc_alarm_irq_enable(struct device *dev,
+ unsigned int enabled)
+{
+ struct ftm_rtc *rtc = dev_get_drvdata(dev);
+
+ if (enabled)
+ ftm_irq_enable(rtc);
+ else
+ ftm_irq_disable(rtc);
+
+ return 0;
+}
+
+/*
+ * Note:
+ * The function is not really getting time from the RTC
+ * since FlexTimer is not a RTC device, but we need to
+ * get time to setup alarm, so we are using system time
+ * for now.
+ */
+static int ftm_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+ struct timespec64 ts64;
+
+ ktime_get_real_ts64(&ts64);
+ rtc_time_to_tm(ts64.tv_sec, tm);
+
+ return 0;
+}
+
+static int ftm_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+ return 0;
+}
+
+/*
+ * 1. Select fixed frequency clock (32KHz) as clock source;
+ * 2. Select 128 (2^7) as divider factor;
+ * So clock is 250 Hz (32KHz/128).
+ *
+ * 3. FlexTimer's CNT register is a 32bit register,
+ * but the register's 16 bit as counter value,it's other 16 bit
+ * is reserved.So minimum counter value is 0x0,maximum counter
+ * value is 0xffff.
+ * So max alarm value is 262 (65536 / 250) seconds
+ */
+static int ftm_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+ struct rtc_time tm;
+ unsigned long now, alm_time, cycle;
+ struct ftm_rtc *rtc = dev_get_drvdata(dev);
+
+ ftm_rtc_read_time(dev, &tm);
+ rtc_tm_to_time(&tm, &now);
+ rtc_tm_to_time(&alm->time, &alm_time);
+
+ ftm_clean_alarm(rtc);
+ cycle = (alm_time - now) * rtc->alarm_freq;
+ if (cycle > MAX_COUNT_VAL) {
+ pr_err("Out of alarm range {0~262} seconds.\n");
+ return -ERANGE;
+ }
+
+ ftm_irq_disable(rtc);
+
+ /*
+ * The counter increments until the value of MOD is reached,
+ * at which point the counter is reloaded with the value of CNTIN.
+ * The TOF (the overflow flag) bit is set when the FTM counter
+ * changes from MOD to CNTIN. So we should using the cycle - 1.
+ */
+ rtc_writel(rtc, FTM_MOD, cycle - 1);
+
+ ftm_counter_enable(rtc);
+ ftm_irq_enable(rtc);
+
+ return 0;
+
+}
+
+static const struct rtc_class_ops ftm_rtc_ops = {
+ .read_time = ftm_rtc_read_time,
+ .read_alarm = ftm_rtc_read_alarm,
+ .set_alarm = ftm_rtc_set_alarm,
+ .alarm_irq_enable = ftm_rtc_alarm_irq_enable,
+};
+
+static int ftm_rtc_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct resource *r;
+ int irq;
+ int ret;
+ struct ftm_rtc *rtc;
+
+ rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+ if (unlikely(!rtc)) {
+ dev_err(&pdev->dev, "cannot alloc memery for rtc\n");
+ return -ENOMEM;
+ }
+
+ platform_set_drvdata(pdev, rtc);
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!r) {
+ dev_err(&pdev->dev, "cannot get resource for rtc\n");
+ return -ENODEV;
+ }
+
+ rtc->base = devm_ioremap_resource(&pdev->dev, r);
+ if (IS_ERR(rtc->base)) {
+ dev_err(&pdev->dev, "cannot ioremap resource for rtc\n");
+ return PTR_ERR(rtc->base);
+ }
+
+ irq = irq_of_parse_and_map(np, 0);
+ if (irq <= 0) {
+ dev_err(&pdev->dev, "unable to get IRQ from DT, %d\n", irq);
+ return -EINVAL;
+ }
+
+ rtc->big_endian = of_property_read_bool(np, "big-endian");
+ rtc->alarm_freq = (u32)FIXED_FREQ_CLK / (u32)MAX_FREQ_DIV;
+
+ device_init_wakeup(&pdev->dev, true);
+ rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "ftm-alarm",
+ &ftm_rtc_ops,
+ THIS_MODULE);
+ if (IS_ERR(rtc->rtc_dev)) {
+ dev_err(&pdev->dev, "can't register rtc device\n");
+ return PTR_ERR(rtc->rtc_dev);
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, ftm_rtc_alarm_interrupt,
+ IRQF_NO_SUSPEND, dev_name(&pdev->dev), rtc);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to request irq\n");
+ return ret;
+ }
+
+ return ret;
+}
+
+static const struct of_device_id ftm_rtc_match[] = {
+ { .compatible = "fsl,ls1012a-ftm-alarm", },
+ { .compatible = "fsl,ls1021a-ftm-alarm", },
+ { .compatible = "fsl,ls1043a-ftm-alarm", },
+ { .compatible = "fsl,ls1046a-ftm-alarm", },
+ { .compatible = "fsl,ls1088a-ftm-alarm", },
+ { .compatible = "fsl,ls208xa-ftm-alarm", },
+ { .compatible = "fsl,ls1028a-ftm-alarm", },
+ { },
+};
+
+static struct platform_driver ftm_rtc_driver = {
+ .probe = ftm_rtc_probe,
+ .driver = {
+ .name = "ftm-alarm",
+ .of_match_table = ftm_rtc_match,
+ },
+};
+
+static int __init ftm_alarm_init(void)
+{
+ return platform_driver_register(&ftm_rtc_driver);
+}
+
+device_initcall(ftm_alarm_init);
+
+MODULE_DESCRIPTION("NXP/Freescale FlexTimer alarm driver");
+MODULE_AUTHOR("Biwen Li <biwen.li@nxp.com>");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related
* [PATCH v3 2/2] mailbox: introduce ARM SMC based mailbox
From: Peng Fan @ 2019-07-15 10:10 UTC (permalink / raw)
To: robh+dt@kernel.org, mark.rutland@arm.com,
jassisinghbrar@gmail.com, sudeep.holla@arm.com,
andre.przywara@arm.com, f.fainelli@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx, Peng Fan
In-Reply-To: <1563184103-8493-1-git-send-email-peng.fan@nxp.com>
From: Peng Fan <peng.fan@nxp.com>
This mailbox driver implements a mailbox which signals transmitted data
via an ARM smc (secure monitor call) instruction. The mailbox receiver
is implemented in firmware and can synchronously return data when it
returns execution to the non-secure world again.
An asynchronous receive path is not implemented.
This allows the usage of a mailbox to trigger firmware actions on SoCs
which either don't have a separate management processor or on which such
a core is not available. A user of this mailbox could be the SCP
interface.
Modified from Andre Przywara's v2 patch
https://lore.kernel.org/patchwork/patch/812999/
Cc: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
V3:
Drop interrupt.
Introduce transports for mem/reg usage.
Add chan-id for mem usage.
V2:
Add interrupts notification support.
drivers/mailbox/Kconfig | 7 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/arm-smc-mailbox.c | 215 ++++++++++++++++++++++++++++++++++++++
3 files changed, 224 insertions(+)
create mode 100644 drivers/mailbox/arm-smc-mailbox.c
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 595542bfae85..c3bd0f1ddcd8 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,13 @@ config ARM_MHU
The controller has 3 mailbox channels, the last of which can be
used in Secure mode only.
+config ARM_SMC_MBOX
+ tristate "Generic ARM smc mailbox"
+ depends on OF && HAVE_ARM_SMCCC
+ help
+ Generic mailbox driver which uses ARM smc calls to call into
+ firmware for triggering mailboxes.
+
config IMX_MBOX
tristate "i.MX Mailbox"
depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index c22fad6f696b..93918a84c91b 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
obj-$(CONFIG_ARM_MHU) += arm_mhu.o
+obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
+
obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX) += armada-37xx-rwtm-mailbox.o
diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
new file mode 100644
index 000000000000..76a2ae11ee4d
--- /dev/null
+++ b/drivers/mailbox/arm-smc-mailbox.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016,2017 ARM Ltd.
+ * Copyright 2019 NXP
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/interrupt.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define ARM_SMC_MBOX_MEM_TRANS BIT(0)
+
+struct arm_smc_chan_data {
+ u32 function_id;
+ u32 chan_id;
+ u32 flags;
+};
+
+struct arm_smccc_mbox_cmd {
+ unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
+};
+
+typedef unsigned long (smc_mbox_fn)(unsigned long, unsigned long,
+ unsigned long, unsigned long,
+ unsigned long, unsigned long,
+ unsigned long, unsigned long);
+static smc_mbox_fn *invoke_smc_mbox_fn;
+
+static int arm_smc_send_data(struct mbox_chan *link, void *data)
+{
+ struct arm_smc_chan_data *chan_data = link->con_priv;
+ struct arm_smccc_mbox_cmd *cmd = data;
+ unsigned long ret;
+ u32 function_id;
+ u32 chan_id;
+
+ if (chan_data->flags & ARM_SMC_MBOX_MEM_TRANS) {
+ if (chan_data->function_id != UINT_MAX)
+ function_id = chan_data->function_id;
+ else
+ function_id = cmd->a0;
+ chan_id = chan_data->chan_id;
+ ret = invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0,
+ 0, 0);
+ } else {
+ ret = invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3,
+ cmd->a4, cmd->a5, cmd->a6, cmd->a7);
+ }
+
+ mbox_chan_received_data(link, (void *)ret);
+
+ return 0;
+}
+
+static unsigned long __invoke_fn_hvc(unsigned long function_id,
+ unsigned long arg0, unsigned long arg1,
+ unsigned long arg2, unsigned long arg3,
+ unsigned long arg4, unsigned long arg5,
+ unsigned long arg6)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
+ arg5, arg6, &res);
+ return res.a0;
+}
+
+static unsigned long __invoke_fn_smc(unsigned long function_id,
+ unsigned long arg0, unsigned long arg1,
+ unsigned long arg2, unsigned long arg3,
+ unsigned long arg4, unsigned long arg5,
+ unsigned long arg6)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
+ arg5, arg6, &res);
+ return res.a0;
+}
+
+static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
+ .send_data = arm_smc_send_data,
+};
+
+static int arm_smc_mbox_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mbox_controller *mbox;
+ struct arm_smc_chan_data *chan_data;
+ const char *method;
+ bool mem_trans = false;
+ int ret, i;
+ u32 val;
+
+ if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
+ if (!val) {
+ dev_err(dev, "invalid arm,num-chans value %u\n", val);
+ return -EINVAL;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ if (!of_property_read_string(dev->of_node, "transports", &method)) {
+ if (!strcmp("mem", method)) {
+ mem_trans = true;
+ } else if (!strcmp("reg", method)) {
+ mem_trans = false;
+ } else {
+ dev_warn(dev, "invalid \"transports\" property: %s\n",
+ method);
+
+ return -EINVAL;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ if (!of_property_read_string(dev->of_node, "method", &method)) {
+ if (!strcmp("hvc", method)) {
+ invoke_smc_mbox_fn = __invoke_fn_hvc;
+ } else if (!strcmp("smc", method)) {
+ invoke_smc_mbox_fn = __invoke_fn_smc;
+ } else {
+ dev_warn(dev, "invalid \"method\" property: %s\n",
+ method);
+
+ return -EINVAL;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+ if (!mbox)
+ return -ENOMEM;
+
+ mbox->num_chans = val;
+ mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
+ GFP_KERNEL);
+ if (!mbox->chans)
+ return -ENOMEM;
+
+ chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
+ GFP_KERNEL);
+ if (!chan_data)
+ return -ENOMEM;
+
+ for (i = 0; i < mbox->num_chans; i++) {
+ u32 function_id;
+
+ ret = of_property_read_u32_index(dev->of_node,
+ "arm,func-ids", i,
+ &function_id);
+ if (ret)
+ chan_data[i].function_id = UINT_MAX;
+
+ else
+ chan_data[i].function_id = function_id;
+
+ chan_data[i].chan_id = i;
+
+ if (mem_trans)
+ chan_data[i].flags |= ARM_SMC_MBOX_MEM_TRANS;
+ mbox->chans[i].con_priv = &chan_data[i];
+ }
+
+ mbox->txdone_poll = false;
+ mbox->txdone_irq = false;
+ mbox->ops = &arm_smc_mbox_chan_ops;
+ mbox->dev = dev;
+
+ platform_set_drvdata(pdev, mbox);
+
+ ret = devm_mbox_controller_register(dev, mbox);
+ if (ret)
+ return ret;
+
+ dev_info(dev, "ARM SMC mailbox enabled with %d chan%s.\n",
+ mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
+
+ return ret;
+}
+
+static int arm_smc_mbox_remove(struct platform_device *pdev)
+{
+ struct mbox_controller *mbox = platform_get_drvdata(pdev);
+
+ mbox_controller_unregister(mbox);
+ return 0;
+}
+
+static const struct of_device_id arm_smc_mbox_of_match[] = {
+ { .compatible = "arm,smc-mbox", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, arm_smc_mbox_of_match);
+
+static struct platform_driver arm_smc_mbox_driver = {
+ .driver = {
+ .name = "arm-smc-mbox",
+ .of_match_table = arm_smc_mbox_of_match,
+ },
+ .probe = arm_smc_mbox_probe,
+ .remove = arm_smc_mbox_remove,
+};
+module_platform_driver(arm_smc_mbox_driver);
+
+MODULE_AUTHOR("Andre Przywara <andre.przywara@arm.com>");
+MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
+MODULE_LICENSE("GPL v2");
--
2.16.4
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Peng Fan @ 2019-07-15 10:10 UTC (permalink / raw)
To: robh+dt@kernel.org, mark.rutland@arm.com,
jassisinghbrar@gmail.com, sudeep.holla@arm.com,
andre.przywara@arm.com, f.fainelli@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx, Peng Fan
In-Reply-To: <1563184103-8493-1-git-send-email-peng.fan@nxp.com>
From: Peng Fan <peng.fan@nxp.com>
The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
actions in software layers running in the EL2 or EL3 exception levels.
The term "ARM" here relates to the SMC instruction as part of the ARM
instruction set, not as a standard endorsed by ARM Ltd.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
V3:
Convert to yaml
Drop interrupt
Introudce transports to indicate mem/reg
The func id is still kept as optional, because like SCMI it only
cares about message.
V2:
Introduce interrupts as a property.
.../devicetree/bindings/mailbox/arm-smc.yaml | 124 +++++++++++++++++++++
1 file changed, 124 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
new file mode 100644
index 000000000000..da9b1a03bc4e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM SMC Mailbox Interface
+
+maintainers:
+ - Peng Fan <peng.fan@nxp.com>
+
+description: |
+ This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
+ call) instruction to trigger a mailbox-connected activity in firmware,
+ executing on the very same core as the caller. By nature this operation
+ is synchronous and this mailbox provides no way for asynchronous messages
+ to be delivered the other way round, from firmware to the OS, but
+ asynchronous notification could also be supported. However the value of
+ r0/w0/x0 the firmware returns after the smc call is delivered as a received
+ message to the mailbox framework, so a synchronous communication can be
+ established, for a asynchronous notification, no value will be returned.
+ The exact meaning of both the action the mailbox triggers as well as the
+ return value is defined by their users and is not subject to this binding.
+
+ One use case of this mailbox is the SCMI interface, which uses shared memory
+ to transfer commands and parameters, and a mailbox to trigger a function
+ call. This allows SoCs without a separate management processor (or when
+ such a processor is not available or used) to use this standardized
+ interface anyway.
+
+ This binding describes no hardware, but establishes a firmware interface.
+ Upon receiving an SMC using one of the described SMC function identifiers,
+ the firmware is expected to trigger some mailbox connected functionality.
+ The communication follows the ARM SMC calling convention.
+ Firmware expects an SMC function identifier in r0 or w0. The supported
+ identifiers are passed from consumers, or listed in the the arm,func-ids
+ properties as described below. The firmware can return one value in
+ the first SMC result register, it is expected to be an error value,
+ which shall be propagated to the mailbox client.
+
+ Any core which supports the SMC or HVC instruction can be used, as long as
+ a firmware component running in EL3 or EL2 is handling these calls.
+
+properties:
+ compatible:
+ const: arm,smc-mbox
+
+ "#mbox-cells":
+ const: 1
+
+ arm,num-chans:
+ description: The number of channels supported.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ method:
+ items:
+ - enum:
+ - smc
+ - hvc
+
+ transports:
+ items:
+ - enum:
+ - mem
+ - reg
+
+ arm,func-ids:
+ description: |
+ An array of 32-bit values specifying the function IDs used by each
+ mailbox channel. Those function IDs follow the ARM SMC calling
+ convention standard [1].
+
+ There is one identifier per channel and the number of supported
+ channels is determined by the length of this array.
+ minItems: 0
+ maxItems: 4096 # Should be enough?
+
+required:
+ - compatible
+ - "#mbox-cells"
+ - arm,num-chans
+ - transports
+ - method
+
+examples:
+ - |
+ sram@910000 {
+ compatible = "mmio-sram";
+ reg = <0x0 0x93f000 0x0 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x0 0x93f000 0x1000>;
+
+ cpu_scp_lpri: scp-shmem@0 {
+ compatible = "arm,scmi-shmem";
+ reg = <0x0 0x200>;
+ };
+
+ cpu_scp_hpri: scp-shmem@200 {
+ compatible = "arm,scmi-shmem";
+ reg = <0x200 0x200>;
+ };
+ };
+
+ firmware {
+ smc_mbox: mailbox {
+ #mbox-cells = <1>;
+ compatible = "arm,smc-mbox";
+ method = "smc";
+ arm,num-chans = <0x2>;
+ transports = "mem";
+ /* Optional */
+ arm,func-ids = <0xc20000fe>, <0xc20000ff>;
+ };
+
+ scmi {
+ compatible = "arm,scmi";
+ mboxes = <&mailbox 0 &mailbox 1>;
+ mbox-names = "tx", "rx";
+ shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
+ };
+ };
+
+...
--
2.16.4
^ permalink raw reply related
* [PATCH v3 0/2] mailbox: arm: introduce smc triggered mailbox
From: Peng Fan @ 2019-07-15 10:10 UTC (permalink / raw)
To: robh+dt@kernel.org, mark.rutland@arm.com,
jassisinghbrar@gmail.com, sudeep.holla@arm.com,
andre.przywara@arm.com, f.fainelli@gmail.com
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
V3:
Drop interrupt
Introduce transports for mem/reg usage
Add chan-id for mem usage
Convert to yaml format
V2:
This is a modified version from Andre Przywara's patch series
https://lore.kernel.org/patchwork/cover/812997/.
The modification are mostly:
Introduce arm,num-chans
Introduce arm_smccc_mbox_cmd
txdone_poll and txdone_irq are both set to false
arm,func-ids are kept, but as an optional property.
Rewords SCPI to SCMI, because I am trying SCMI over SMC, not SCPI.
Introduce interrupts notification.
[1] is a draft implementation of i.MX8MM SCMI ATF implementation that
use smc as mailbox, power/clk is included, but only part of clk has been
implemented to work with hardware, power domain only supports get name
for now.
The traditional Linux mailbox mechanism uses some kind of dedicated hardware
IP to signal a condition to some other processing unit, typically a dedicated
management processor.
This mailbox feature is used for instance by the SCMI protocol to signal a
request for some action to be taken by the management processor.
However some SoCs does not have a dedicated management core to provide
those services. In order to service TEE and to avoid linux shutdown
power and clock that used by TEE, need let firmware to handle power
and clock, the firmware here is ARM Trusted Firmware that could also
run SCMI service.
The existing SCMI implementation uses a rather flexible shared memory
region to communicate commands and their parameters, it still requires a
mailbox to actually trigger the action.
This patch series provides a Linux mailbox compatible service which uses
smc calls to invoke firmware code, for instance taking care of SCMI requests.
The actual requests are still communicated using the standard SCMI way of
shared memory regions, but a dedicated mailbox hardware IP can be replaced via
this new driver.
This simple driver uses the architected SMC calling convention to trigger
firmware services, also allows for using "HVC" calls to call into hypervisors
or firmware layers running in the EL2 exception level.
Patch 1 contains the device tree binding documentation, patch 2 introduces
the actual mailbox driver.
Please note that this driver just provides a generic mailbox mechanism,
It could support synchronous TX/RX, or synchronous TX with asynchronous
RX. And while providing SCMI services was the reason for this exercise,
this driver is in no way bound to this use case, but can be used generically
where the OS wants to signal a mailbox condition to firmware or a
hypervisor.
Also the driver is in no way meant to replace any existing firmware
interface, but actually to complement existing interfaces.
[1] https://github.com/MrVan/arm-trusted-firmware/tree/scmi
Peng Fan (2):
dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
mailbox: introduce ARM SMC based mailbox
.../devicetree/bindings/mailbox/arm-smc.yaml | 124 ++++++++++++
drivers/mailbox/Kconfig | 7 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/arm-smc-mailbox.c | 215 +++++++++++++++++++++
4 files changed, 348 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
create mode 100644 drivers/mailbox/arm-smc-mailbox.c
--
2.16.4
^ permalink raw reply
* Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode
From: Will Deacon @ 2019-07-15 9:51 UTC (permalink / raw)
To: Yong Wu
Cc: youlin.pei-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Nicolas Boichat,
cui.zhang-NuS5LvNUpcJWk0Htik3J/w,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Tomasz Figa, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Evan Green,
chao.hao-NuS5LvNUpcJWk0Htik3J/w,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Matthias Brugger,
yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w,
anan.sun-NuS5LvNUpcJWk0Htik3J/w, Robin Murphy, Matthias Kaehlcke,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1563079280.31342.22.camel@mhfsdcap03>
On Sun, Jul 14, 2019 at 12:41:20PM +0800, Yong Wu wrote:
> On Thu, 2019-07-11 at 13:31 +0100, Will Deacon wrote:
> > This looks like the right sort of idea. Basically, I was thinking that you
> > can use the oas in conjunction with the quirk to specify whether or not
> > your two magic bits should be set. You could also then cap the oas using
> > the size of phys_addr_t to deal with my other comment.
> >
> > Finally, I was hoping you could drop the |= BIT_ULL(32) and the &=
> > ~BIT_ULL(32) bits of the mtk driver if the pgtable code now accepts higher
> > addresses. Did that not work out?
>
> After the current patch, the pgtable has accepted the higher address.
> the " |= BIT_ULL(32)" and "& = ~ BIT_ULL(32)" is for a special case(we
> call it 4GB mode).
>
> Now MediaTek IOMMU support 2 kind memory:
> 1) normal case: PA is 0x4000_0000 - 0x3_ffff_ffff. the PA won't be
> remapped. mt8183 and the non-4GB mode of mt8173/mt2712 use this mode.
>
> 2) 4GB Mode: PA is 0x4000_0000 - 0x1_3fff_ffff. But the PA will remapped
> to 0x1_0000_0000 to 0x1_ffff_ffff. This is for the 4GB mode of
> mt8173/mt2712. This case is so special that we should change the PA
> manually(add bit32).
> (mt2712 and mt8173 have both mode: 4GB and non-4GB.)
>
> If we try to use oas and our quirk to cover this two case. Then I can
> use "oas == 33" only for this 4GB mode. and "oas == 34" for the normal
> case even though the PA mayn't reach 34bit. Also I should add some
> "workaround" for the 4GB mode(oas==33).
>
> I copy the new patch in the mail below(have dropped the "|= BIT_ULL(32)"
> and the "&= ~BIT_ULL(32)) in mtk iommu". please help have a look if it
> is ok.
> (another thing: Current the PA can support over 4GB. So the quirk name
> "MTK_4GB" looks not suitable, I used a new patch rename to "MTK_EXT").
Makes sense, thanks. One comment below.
> @@ -205,7 +216,20 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte
> pte, int lvl,
> else
> mask = ARM_V7S_LVL_MASK(lvl);
>
> - return pte & mask;
> + paddr = pte & mask;
> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) &&
> + (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)) {
> + /*
> + * Workaround for MTK 4GB Mode:
> + * Add BIT32 only when PA < 0x4000_0000.
> + */
> + if ((cfg->oas == 33 && paddr < 0x40000000UL) ||
> + (cfg->oas > 33 && (pte & ARM_V7S_ATTR_MTK_PA_BIT32)))
> + paddr |= BIT_ULL(32);
> + if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
> + paddr |= BIT_ULL(33);
> + }
> + return paddr;
> }
>
> static arm_v7s_iopte *iopte_deref(arm_v7s_iopte pte, int lvl,
> @@ -326,9 +350,6 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot,
> int lvl,
> if (lvl == 1 && (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS))
> pte |= ARM_V7S_ATTR_NS_SECTION;
>
> - if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)
> - pte |= ARM_V7S_ATTR_MTK_4GB;
> -
> return pte;
> }
>
> @@ -742,7 +763,9 @@ static struct io_pgtable
> *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> {
> struct arm_v7s_io_pgtable *data;
>
> - if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
> + if (cfg->ias > ARM_V7S_ADDR_BITS ||
> + (cfg->oas > ARM_V7S_ADDR_BITS &&
> + !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))
> return NULL;
I think you can rework this to do something like:
if (cfg->ias > ARM_V7S_ADDR_BITS)
return NULL;
if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) {
if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
cfg->oas = min(cfg->oas, ARM_V7S_ADDR_BITS);
else if (cfg->oas > 34)
return NULL;
} else if (cfg->oas > ARM_V7S_ADDR_BITS) {
return NULL;
}
so that we clamp the oas when phys_addr_t is 32-bit for you. That should
allow you to remove lots of the checking from iopte_to_paddr() too if you
check against oas in the map() function.
Does that make sense?
Will
^ permalink raw reply
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-15 9:47 UTC (permalink / raw)
To: Daniel Thompson
Cc: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland,
linux-leds, linux-kernel, devicetree
In-Reply-To: <20190715092400.sedjumqkecglheyu@holly.lan>
On 15/07/2019 11:24, Daniel Thompson wrote:
> On Mon, Jul 15, 2019 at 11:01:29AM +0200, Jean-Jacques Hiblot wrote:
>> Hi Dan,
>>
>> On 12/07/2019 20:49, Dan Murphy wrote:
>>> JJ
>>>
>>> On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote:
>>>> A LED is usually powered by a voltage/current regulator. Let the LED
>>>> core
>>> Let the LED core know
>>>> about it. This allows the LED core to turn on or off the power supply
>>>> as needed.
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>> ---
>>>> drivers/leds/led-class.c | 10 ++++++++
>>>> drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++---
>>>> include/linux/leds.h | 4 +++
>>>> 3 files changed, 64 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>>> index 4793e77808e2..e01b2d982564 100644
>>>> --- a/drivers/leds/led-class.c
>>>> +++ b/drivers/leds/led-class.c
>>>> @@ -17,6 +17,7 @@
>>>> #include <linux/slab.h>
>>>> #include <linux/spinlock.h>
>>>> #include <linux/timer.h>
>>>> +#include <linux/regulator/consumer.h>
>>> What if you move this to leds.h so core and class can both include it.
>>>
>>>
>>>> #include <uapi/linux/uleds.h>
>>>> #include "leds.h"
>>>> @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device
>>>> *parent, struct device_node *np,
>>>> dev_warn(parent, "Led %s renamed to %s due to name collision",
>>>> led_cdev->name, dev_name(led_cdev->dev));
>>>> + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power");
>>> Is the regulator always going to be called power?
>> Actually in the dts, that will be "power-supply". I lacked the imagination
>> to come up with a better name.
>>
>>
>>
>>>> + if (IS_ERR(led_cdev->regulator)) {
>>>> + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
>>>> + led_cdev->name);
>>>> + device_unregister(led_cdev->dev);
>>>> + mutex_unlock(&led_cdev->led_access);
>>>> + return PTR_ERR(led_cdev->regulator);
>>> This is listed as optional in the DT doc. This appears to be required.
>> The regulator core will provide a dummy regulator if none is given in the
>> device tree. I would rather have an error in that case, but that is not how
>> it works.
> If you actively wanted to get -ENODEV back when there is no regulator
> then you can use devm_regulator_get_optional() for that.
>
> However perhaps be careful what you wish for. If you use get_optional()
> then you will have to sprinkle NULL or IS_ERR() checks everywhere. I'd
> favour using the current approach!
Thanks for the info. I think I'll use the get_optionnal(). That will add
a bit of complexity, but it will avoid deferring some work in
led_set_brightness_nopm() when it is not needed.
JJ
>
>
> Daniel.
>
>>
>>> I prefer to keep it optional. Many LED drivers are connected to fixed
>>> non-managed supplies.
>>>
>>>> + }
>>>> +
>>>> if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>>>> ret = led_add_brightness_hw_changed(led_cdev);
>>>> if (ret) {
>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>>> index 7107cd7e87cf..139de6b08cad 100644
>>>> --- a/drivers/leds/led-core.c
>>>> +++ b/drivers/leds/led-core.c
>>>> @@ -16,6 +16,7 @@
>>>> #include <linux/rwsem.h>
>>>> #include <linux/slab.h>
>>>> #include "leds.h"
>>>> +#include <linux/regulator/consumer.h>
>>>> DECLARE_RWSEM(leds_list_lock);
>>>> EXPORT_SYMBOL_GPL(leds_list_lock);
>>>> @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>>>> LIST_HEAD(leds_list);
>>>> EXPORT_SYMBOL_GPL(leds_list);
>>>> +static bool __led_need_regulator_update(struct led_classdev
>>>> *led_cdev,
>>>> + int brightness)
>>>> +{
>>>> + bool new_regulator_state = (brightness != LED_OFF);
>>>> +
>>>> + return led_cdev->regulator_state != new_regulator_state;
>>>> +}
>>>> +
>>>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>>>> + int brightness)
>>>> +{
>>>> + if (__led_need_regulator_update(led_cdev, brightness)) {
>>>> + int ret;
>>> Prefer to this to be moved up.
>> ok
>>>> +
>>>> + if (brightness != LED_OFF)
>>>> + ret = regulator_enable(led_cdev->regulator);
>>>> + else
>>>> + ret = regulator_disable(led_cdev->regulator);
>>>> + if (ret)
>>>> + return ret;
>>> new line
>>>> + led_cdev->regulator_state = (brightness != LED_OFF);
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int __led_set_brightness(struct led_classdev *led_cdev,
>>>> enum led_brightness value)
>>>> {
>>>> @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t)
>>>> }
>>>> led_set_brightness_nosleep(led_cdev, brightness);
>>>> + __led_handle_regulator(led_cdev, brightness);
>>> Again this seems to indicate that the regulator is a required property
>>> for the LEDs
>>>
>>> This needs to be made optional. And the same comment through out for
>>> every call.
>>>
>>>
>>>> /* Return in next iteration if led is in one-shot mode and
>>>> we are in
>>>> * the final blink state so that the led is toggled each
>>>> delay_on +
>>>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct
>>>> work_struct *ws)
>>>> if (ret == -ENOTSUPP)
>>>> ret = __led_set_brightness_blocking(led_cdev,
>>>> led_cdev->delayed_set_value);
>>>> + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
>>>> +
>>>> if (ret < 0 &&
>>>> /* LED HW might have been unplugged, therefore don't warn */
>>>> !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
>>>> @@ -141,6 +170,7 @@ static void led_set_software_blink(struct
>>>> led_classdev *led_cdev,
>>>> /* never on - just set to off */
>>>> if (!delay_on) {
>>>> led_set_brightness_nosleep(led_cdev, LED_OFF);
>>>> + __led_handle_regulator(led_cdev, LED_OFF);
>>>> return;
>>>> }
>>>> @@ -148,6 +178,7 @@ static void led_set_software_blink(struct
>>>> led_classdev *led_cdev,
>>>> if (!delay_off) {
>>>> led_set_brightness_nosleep(led_cdev,
>>>> led_cdev->blink_brightness);
>>>> + __led_handle_regulator(led_cdev, led_cdev->blink_brightness);
>>>> return;
>>>> }
>>>> @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct
>>>> led_classdev *led_cdev,
>>>> enum led_brightness value)
>>>> {
>>>> /* Use brightness_set op if available, it is guaranteed not to
>>>> sleep */
>>>> - if (!__led_set_brightness(led_cdev, value))
>>>> - return;
>>>> + if (!__led_set_brightness(led_cdev, value)) {
>>>> + /*
>>>> + * if regulator state doesn't need to be changed, that is all/
>>>> + * Otherwise delegate the change to a work queue
>>>> + */
>>>> + if (!__led_need_regulator_update(led_cdev, value))
>>>> + return;
>>>> + }
>>>> /* If brightness setting can sleep, delegate it to a work
>>>> queue task */
>>>> led_cdev->delayed_set_value = value;
>>>> @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>>>> int led_set_brightness_sync(struct led_classdev *led_cdev,
>>>> enum led_brightness value)
>>>> {
>>>> + int ret;
>>>> +
>>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>>>> return -EBUSY;
>>>> @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct
>>>> led_classdev *led_cdev,
>>>> if (led_cdev->flags & LED_SUSPENDED)
>>>> return 0;
>>>> - return __led_set_brightness_blocking(led_cdev,
>>>> led_cdev->brightness);
>>>> + ret = __led_set_brightness_blocking(led_cdev,
>>>> led_cdev->brightness);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = __led_handle_regulator(led_cdev, led_cdev->brightness);
>>> Can't you just return here?
>> ok
>>
>>
>> thanks for the review
>>
>> JJ
>>
>>> Dan
>>>
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return 0;
>>>> }
>>>> EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>> index 9b2bf574a17a..bee8e3f8dddd 100644
>>>> --- a/include/linux/leds.h
>>>> +++ b/include/linux/leds.h
>>>> @@ -123,6 +123,10 @@ struct led_classdev {
>>>> /* Ensures consistent access to the LED Flash Class device */
>>>> struct mutex led_access;
>>>> +
>>>> + /* regulator */
>>>> + struct regulator *regulator;
>>>> + bool regulator_state;
>>>> };
>>>> extern int of_led_classdev_register(struct device *parent,
^ permalink raw reply
* Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp
From: Jarkko Sakkinen @ 2019-07-15 9:45 UTC (permalink / raw)
To: Tomer Maimon
Cc: Oshri Alkobi, Alexander Steffen, Rob Herring, Mark Rutland,
peterhuewe, jgg, Arnd Bergmann, Greg KH, IS20 Oshri Alkoby,
devicetree, AP MS30 Linux Kernel community, linux-integrity,
gcwilson, kgoldman, nayna, IS30 Dan Morav, eyal.cohen
In-Reply-To: <CAP6Zq1hPo9dG71YFyr7z9rjmi-DvoUZJOme4+2uqsfO+7nH+HQ@mail.gmail.com>
On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote:
> Thanks for your feedback and sorry for the late response.
>
> Due to the amount of work required to handle this technical feedback and
> project constraints we need to put this task on hold for the near future.
>
> In the meantime, anyone from the community is welcome to take over this
> code and handle the re-design for the benefit of the entire TPM community.
Ok, so there is already driver for this called tpm_tis_core.
So you go and create a new module, whose name given the framework of
things that we already have deployed, is destined to be tpm_tis_i2c.
Then you roughly implement a new physical layer by using a callback
interface provided to you by tpm_tis_core.
The so called re-design was already addressed by Alexander [1].
How hard can it be seriously?
[1] https://lkml.org/lkml/2019/7/4/331
/Jarkko
^ permalink raw reply
* [PATCH v2 3/3] arm64: dts: renesas: hihope-common: Add HDMI audio support
From: Fabrizio Castro @ 2019-07-15 9:32 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring, Mark Rutland
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-renesas-soc,
devicetree, Chris Paterson, Biju Das, xu_shunji
This patch adds support for HDMI audio to the device tree
common to the HiHope RZ/G2M and the HiHope RZ/G2N.
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2:
* Removed #sound-dai-cells from hdmi0 node
arch/arm64/boot/dts/renesas/hihope-common.dtsi | 47 ++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/hihope-common.dtsi b/arch/arm64/boot/dts/renesas/hihope-common.dtsi
index 52d61c9..e1b95c3 100644
--- a/arch/arm64/boot/dts/renesas/hihope-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/hihope-common.dtsi
@@ -81,6 +81,14 @@
regulator-always-on;
};
+ sound_card: sound {
+ compatible = "audio-graph-card";
+
+ label = "rcar-sound";
+
+ dais = <&rsnd_port0>;
+ };
+
vbus0_usb2: regulator-vbus0-usb2 {
compatible = "regulator-fixed";
@@ -129,6 +137,10 @@
};
};
+&audio_clk_a {
+ clock-frequency = <22579200>;
+};
+
&du {
clocks = <&cpg CPG_MOD 724>,
<&cpg CPG_MOD 723>,
@@ -176,6 +188,12 @@
remote-endpoint = <&hdmi0_con>;
};
};
+ port@2 {
+ reg = <2>;
+ dw_hdmi0_snd_in: endpoint {
+ remote-endpoint = <&rsnd_endpoint0>;
+ };
+ };
};
};
@@ -272,6 +290,11 @@
power-source = <1800>;
};
+ sound_clk_pins: sound_clk {
+ groups = "audio_clk_a_a";
+ function = "audio_clk";
+ };
+
usb0_pins: usb0 {
groups = "usb0";
function = "usb0";
@@ -295,6 +318,30 @@
};
};
+&rcar_sound {
+ pinctrl-0 = <&sound_clk_pins>;
+ pinctrl-names = "default";
+
+ status = "okay";
+
+ /* Single DAI */
+ #sound-dai-cells = <0>;
+
+ ports {
+ rsnd_port0: port@0 {
+ rsnd_endpoint0: endpoint {
+ remote-endpoint = <&dw_hdmi0_snd_in>;
+
+ dai-format = "i2s";
+ bitclock-master = <&rsnd_endpoint0>;
+ frame-master = <&rsnd_endpoint0>;
+
+ playback = <&ssi2>;
+ };
+ };
+ };
+};
+
&rwdt {
timeout-sec = <60>;
status = "okay";
--
2.7.4
^ permalink raw reply related
* RE: [PATCH 3/3] arm64: dts: renesas: hihope-common: Add HDMI audio support
From: Fabrizio Castro @ 2019-07-15 9:29 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Geert Uytterhoeven, Rob Herring, Mark Rutland, Simon Horman,
Magnus Damm, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>, Chris Paterson,
Biju Das, xu_shunji@hoperun.com
In-Reply-To: <CAMuHMdUGfHyPp=5aBig4_Sh91hUku4Xx_Ho54X=EDcqrj_zXqA@mail.gmail.com>
Hi Geert,
Thank you for your feedback!
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 12 July 2019 14:42
> Subject: Re: [PATCH 3/3] arm64: dts: renesas: hihope-common: Add HDMI audio support
>
> Hi Fabrizio,
>
> On Fri, Jul 5, 2019 at 3:40 PM Fabrizio Castro
> <fabrizio.castro@bp.renesas.com> wrote:
> > This patch adds support for HDMI audio to the device tree
> > common to the HiHope RZ/G2M and the HiHope RZ/G2N.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/arch/arm64/boot/dts/renesas/hihope-common.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/hihope-common.dtsi
>
> > @@ -168,6 +180,7 @@
> >
> > &hdmi0 {
> > status = "okay";
> > + #sound-dai-cells = <0>;
>
> Why the above line?
It doesn't belong there, good catch, I'll send a v2 shortly without it.
Thanks,
Fab
>
> With the above question answered:
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core
From: Daniel Thompson @ 2019-07-15 9:24 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland,
linux-leds, linux-kernel, devicetree
In-Reply-To: <ab4818c0-bc7a-13e1-c6ce-e977b0234de0@ti.com>
On Mon, Jul 15, 2019 at 11:01:29AM +0200, Jean-Jacques Hiblot wrote:
> Hi Dan,
>
> On 12/07/2019 20:49, Dan Murphy wrote:
> > JJ
> >
> > On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote:
> > > A LED is usually powered by a voltage/current regulator. Let the LED
> > > core
> > Let the LED core know
> > > about it. This allows the LED core to turn on or off the power supply
> > > as needed.
> >
> > >
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > > ---
> > > drivers/leds/led-class.c | 10 ++++++++
> > > drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++---
> > > include/linux/leds.h | 4 +++
> > > 3 files changed, 64 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > > index 4793e77808e2..e01b2d982564 100644
> > > --- a/drivers/leds/led-class.c
> > > +++ b/drivers/leds/led-class.c
> > > @@ -17,6 +17,7 @@
> > > #include <linux/slab.h>
> > > #include <linux/spinlock.h>
> > > #include <linux/timer.h>
> > > +#include <linux/regulator/consumer.h>
> >
> > What if you move this to leds.h so core and class can both include it.
> >
> >
> > > #include <uapi/linux/uleds.h>
> > > #include "leds.h"
> > > @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device
> > > *parent, struct device_node *np,
> > > dev_warn(parent, "Led %s renamed to %s due to name collision",
> > > led_cdev->name, dev_name(led_cdev->dev));
> > > + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power");
> >
> > Is the regulator always going to be called power?
>
> Actually in the dts, that will be "power-supply". I lacked the imagination
> to come up with a better name.
>
>
>
> >
> > > + if (IS_ERR(led_cdev->regulator)) {
> > > + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> > > + led_cdev->name);
> > > + device_unregister(led_cdev->dev);
> > > + mutex_unlock(&led_cdev->led_access);
> > > + return PTR_ERR(led_cdev->regulator);
> >
> > This is listed as optional in the DT doc. This appears to be required.
>
> The regulator core will provide a dummy regulator if none is given in the
> device tree. I would rather have an error in that case, but that is not how
> it works.
If you actively wanted to get -ENODEV back when there is no regulator
then you can use devm_regulator_get_optional() for that.
However perhaps be careful what you wish for. If you use get_optional()
then you will have to sprinkle NULL or IS_ERR() checks everywhere. I'd
favour using the current approach!
Daniel.
>
>
> >
> > I prefer to keep it optional. Many LED drivers are connected to fixed
> > non-managed supplies.
> >
> > > + }
> > > +
> > > if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
> > > ret = led_add_brightness_hw_changed(led_cdev);
> > > if (ret) {
> > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > > index 7107cd7e87cf..139de6b08cad 100644
> > > --- a/drivers/leds/led-core.c
> > > +++ b/drivers/leds/led-core.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/rwsem.h>
> > > #include <linux/slab.h>
> > > #include "leds.h"
> > > +#include <linux/regulator/consumer.h>
> > > DECLARE_RWSEM(leds_list_lock);
> > > EXPORT_SYMBOL_GPL(leds_list_lock);
> > > @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
> > > LIST_HEAD(leds_list);
> > > EXPORT_SYMBOL_GPL(leds_list);
> > > +static bool __led_need_regulator_update(struct led_classdev
> > > *led_cdev,
> > > + int brightness)
> > > +{
> > > + bool new_regulator_state = (brightness != LED_OFF);
> > > +
> > > + return led_cdev->regulator_state != new_regulator_state;
> > > +}
> > > +
> > > +static int __led_handle_regulator(struct led_classdev *led_cdev,
> > > + int brightness)
> > > +{
> > > + if (__led_need_regulator_update(led_cdev, brightness)) {
> > > + int ret;
> >
> > Prefer to this to be moved up.
> ok
> >
> > > +
> > > + if (brightness != LED_OFF)
> > > + ret = regulator_enable(led_cdev->regulator);
> > > + else
> > > + ret = regulator_disable(led_cdev->regulator);
> > > + if (ret)
> > > + return ret;
> > new line
> > > + led_cdev->regulator_state = (brightness != LED_OFF);
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static int __led_set_brightness(struct led_classdev *led_cdev,
> > > enum led_brightness value)
> > > {
> > > @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t)
> > > }
> > > led_set_brightness_nosleep(led_cdev, brightness);
> > > + __led_handle_regulator(led_cdev, brightness);
> >
> > Again this seems to indicate that the regulator is a required property
> > for the LEDs
> >
> > This needs to be made optional. And the same comment through out for
> > every call.
> >
> >
> > > /* Return in next iteration if led is in one-shot mode and
> > > we are in
> > > * the final blink state so that the led is toggled each
> > > delay_on +
> > > @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct
> > > work_struct *ws)
> > > if (ret == -ENOTSUPP)
> > > ret = __led_set_brightness_blocking(led_cdev,
> > > led_cdev->delayed_set_value);
> > > + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
> > > +
> > > if (ret < 0 &&
> > > /* LED HW might have been unplugged, therefore don't warn */
> > > !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
> > > @@ -141,6 +170,7 @@ static void led_set_software_blink(struct
> > > led_classdev *led_cdev,
> > > /* never on - just set to off */
> > > if (!delay_on) {
> > > led_set_brightness_nosleep(led_cdev, LED_OFF);
> > > + __led_handle_regulator(led_cdev, LED_OFF);
> > > return;
> > > }
> > > @@ -148,6 +178,7 @@ static void led_set_software_blink(struct
> > > led_classdev *led_cdev,
> > > if (!delay_off) {
> > > led_set_brightness_nosleep(led_cdev,
> > > led_cdev->blink_brightness);
> > > + __led_handle_regulator(led_cdev, led_cdev->blink_brightness);
> > > return;
> > > }
> > > @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct
> > > led_classdev *led_cdev,
> > > enum led_brightness value)
> > > {
> > > /* Use brightness_set op if available, it is guaranteed not to
> > > sleep */
> > > - if (!__led_set_brightness(led_cdev, value))
> > > - return;
> > > + if (!__led_set_brightness(led_cdev, value)) {
> > > + /*
> > > + * if regulator state doesn't need to be changed, that is all/
> > > + * Otherwise delegate the change to a work queue
> > > + */
> > > + if (!__led_need_regulator_update(led_cdev, value))
> > > + return;
> > > + }
> > > /* If brightness setting can sleep, delegate it to a work
> > > queue task */
> > > led_cdev->delayed_set_value = value;
> > > @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
> > > int led_set_brightness_sync(struct led_classdev *led_cdev,
> > > enum led_brightness value)
> > > {
> > > + int ret;
> > > +
> > > if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> > > return -EBUSY;
> > > @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct
> > > led_classdev *led_cdev,
> > > if (led_cdev->flags & LED_SUSPENDED)
> > > return 0;
> > > - return __led_set_brightness_blocking(led_cdev,
> > > led_cdev->brightness);
> > > + ret = __led_set_brightness_blocking(led_cdev,
> > > led_cdev->brightness);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = __led_handle_regulator(led_cdev, led_cdev->brightness);
> >
> > Can't you just return here?
>
> ok
>
>
> thanks for the review
>
> JJ
>
> >
> > Dan
> >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(led_set_brightness_sync);
> > > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > > index 9b2bf574a17a..bee8e3f8dddd 100644
> > > --- a/include/linux/leds.h
> > > +++ b/include/linux/leds.h
> > > @@ -123,6 +123,10 @@ struct led_classdev {
> > > /* Ensures consistent access to the LED Flash Class device */
> > > struct mutex led_access;
> > > +
> > > + /* regulator */
> > > + struct regulator *regulator;
> > > + bool regulator_state;
> > > };
> > > extern int of_led_classdev_register(struct device *parent,
^ permalink raw reply
* [PATCH] docs: fix broken doc links due to renames
From: Mauro Carvalho Chehab @ 2019-07-15 9:12 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, rcu, linux-doc, devicetree, linux-arch,
esc.storagedev, linux-scsi
Some files got renamed but the patch was incomplete, as it forgot
to update the documentation reference accordingly.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
This patch is against current linus/master branch.
Documentation/RCU/rculist_nulls.txt | 2 +-
Documentation/devicetree/bindings/arm/idle-states.txt | 2 +-
Documentation/locking/spinlocks.txt | 4 ++--
Documentation/memory-barriers.txt | 2 +-
Documentation/translations/ko_KR/memory-barriers.txt | 2 +-
MAINTAINERS | 6 +++---
drivers/scsi/hpsa.c | 4 ++--
7 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/Documentation/RCU/rculist_nulls.txt b/Documentation/RCU/rculist_nulls.txt
index 8151f0195f76..23f115dc87cf 100644
--- a/Documentation/RCU/rculist_nulls.txt
+++ b/Documentation/RCU/rculist_nulls.txt
@@ -1,7 +1,7 @@
Using hlist_nulls to protect read-mostly linked lists and
objects using SLAB_TYPESAFE_BY_RCU allocations.
-Please read the basics in Documentation/RCU/listRCU.txt
+Please read the basics in Documentation/RCU/listRCU.rst
Using special makers (called 'nulls') is a convenient way
to solve following problem :
diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
index 326f29b270ad..2d325bed37e5 100644
--- a/Documentation/devicetree/bindings/arm/idle-states.txt
+++ b/Documentation/devicetree/bindings/arm/idle-states.txt
@@ -703,4 +703,4 @@ cpus {
https://www.devicetree.org/specifications/
[6] ARM Linux Kernel documentation - Booting AArch64 Linux
- Documentation/arm64/booting.txt
+ Documentation/arm64/booting.rst
diff --git a/Documentation/locking/spinlocks.txt b/Documentation/locking/spinlocks.txt
index ff35e40bdf5b..430b641ae072 100644
--- a/Documentation/locking/spinlocks.txt
+++ b/Documentation/locking/spinlocks.txt
@@ -74,7 +74,7 @@ itself. The read lock allows many concurrent readers. Anything that
_changes_ the list will have to get the write lock.
NOTE! RCU is better for list traversal, but requires careful
- attention to design detail (see Documentation/RCU/listRCU.txt).
+ attention to design detail (see Documentation/RCU/listRCU.rst).
Also, you cannot "upgrade" a read-lock to a write-lock, so if you at _any_
time need to do any changes (even if you don't do it every time), you have
@@ -82,7 +82,7 @@ to get the write-lock at the very beginning.
NOTE! We are working hard to remove reader-writer spinlocks in most
cases, so please don't add a new one without consensus. (Instead, see
- Documentation/RCU/rcu.txt for complete information.)
+ Documentation/RCU/rcu.rst for complete information.)
----
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 045bb8148fe9..1adbb8a371c7 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -548,7 +548,7 @@ There are certain things that the Linux kernel memory barriers do not guarantee:
[*] For information on bus mastering DMA and coherency please read:
- Documentation/PCI/pci.rst
+ Documentation/driver-api/pci/pci.rst
Documentation/DMA-API-HOWTO.txt
Documentation/DMA-API.txt
diff --git a/Documentation/translations/ko_KR/memory-barriers.txt b/Documentation/translations/ko_KR/memory-barriers.txt
index a33c2a536542..2774624ee843 100644
--- a/Documentation/translations/ko_KR/memory-barriers.txt
+++ b/Documentation/translations/ko_KR/memory-barriers.txt
@@ -569,7 +569,7 @@ ACQUIRE 는 해당 오퍼레이션의 로드 부분에만 적용되고 RELEASE
[*] 버스 마스터링 DMA 와 일관성에 대해서는 다음을 참고하시기 바랍니다:
- Documentation/PCI/pci.rst
+ Documentation/driver-api/pci/pci.rst
Documentation/DMA-API-HOWTO.txt
Documentation/DMA-API.txt
diff --git a/MAINTAINERS b/MAINTAINERS
index f5533d1bda2e..51ad84a3f4e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -899,7 +899,7 @@ L: linux-iio@vger.kernel.org
W: http://ez.analog.com/community/linux-device-drivers
S: Supported
F: drivers/iio/adc/ad7124.c
-F: Documentation/devicetree/bindings/iio/adc/adi,ad7124.txt
+F: Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
ANALOG DEVICES INC AD7606 DRIVER
M: Stefan Popa <stefan.popa@analog.com>
@@ -6828,7 +6828,7 @@ R: Sagi Shahar <sagis@google.com>
R: Jon Olson <jonolson@google.com>
L: netdev@vger.kernel.org
S: Supported
-F: Documentation/networking/device_drivers/google/gve.txt
+F: Documentation/networking/device_drivers/google/gve.rst
F: drivers/net/ethernet/google
GPD POCKET FAN DRIVER
@@ -12077,7 +12077,7 @@ M: Juergen Gross <jgross@suse.com>
M: Alok Kataria <akataria@vmware.com>
L: virtualization@lists.linux-foundation.org
S: Supported
-F: Documentation/virtual/paravirt_ops.txt
+F: Documentation/virtual/paravirt_ops.rst
F: arch/*/kernel/paravirt*
F: arch/*/include/asm/paravirt*.h
F: include/linux/hypervisor.h
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 43a6b5350775..eaf6177ac9ee 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -7798,7 +7798,7 @@ static void hpsa_free_pci_init(struct ctlr_info *h)
hpsa_disable_interrupt_mode(h); /* pci_init 2 */
/*
* call pci_disable_device before pci_release_regions per
- * Documentation/PCI/pci.rst
+ * Documentation/driver-api/pci/pci.rst
*/
pci_disable_device(h->pdev); /* pci_init 1 */
pci_release_regions(h->pdev); /* pci_init 2 */
@@ -7881,7 +7881,7 @@ static int hpsa_pci_init(struct ctlr_info *h)
clean1:
/*
* call pci_disable_device before pci_release_regions per
- * Documentation/PCI/pci.rst
+ * Documentation/driver-api/pci/pci.rst
*/
pci_disable_device(h->pdev);
pci_release_regions(h->pdev);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-15 9:01 UTC (permalink / raw)
To: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland,
daniel.thompson
Cc: linux-leds, linux-kernel, devicetree
In-Reply-To: <56d16260-ff82-3439-4c1f-2a3a1552bc7d@ti.com>
Hi Dan,
On 12/07/2019 20:49, Dan Murphy wrote:
> JJ
>
> On 7/8/19 5:35 AM, Jean-Jacques Hiblot wrote:
>> A LED is usually powered by a voltage/current regulator. Let the LED
>> core
> Let the LED core know
>> about it. This allows the LED core to turn on or off the power supply
>> as needed.
>
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>> drivers/leds/led-class.c | 10 ++++++++
>> drivers/leds/led-core.c | 53 +++++++++++++++++++++++++++++++++++++---
>> include/linux/leds.h | 4 +++
>> 3 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 4793e77808e2..e01b2d982564 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -17,6 +17,7 @@
>> #include <linux/slab.h>
>> #include <linux/spinlock.h>
>> #include <linux/timer.h>
>> +#include <linux/regulator/consumer.h>
>
> What if you move this to leds.h so core and class can both include it.
>
>
>> #include <uapi/linux/uleds.h>
>> #include "leds.h"
>> @@ -272,6 +273,15 @@ int of_led_classdev_register(struct device
>> *parent, struct device_node *np,
>> dev_warn(parent, "Led %s renamed to %s due to name collision",
>> led_cdev->name, dev_name(led_cdev->dev));
>> + led_cdev->regulator = devm_regulator_get(led_cdev->dev, "power");
>
> Is the regulator always going to be called power?
Actually in the dts, that will be "power-supply". I lacked the
imagination to come up with a better name.
>
>> + if (IS_ERR(led_cdev->regulator)) {
>> + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
>> + led_cdev->name);
>> + device_unregister(led_cdev->dev);
>> + mutex_unlock(&led_cdev->led_access);
>> + return PTR_ERR(led_cdev->regulator);
>
> This is listed as optional in the DT doc. This appears to be required.
The regulator core will provide a dummy regulator if none is given in
the device tree. I would rather have an error in that case, but that is
not how it works.
>
> I prefer to keep it optional. Many LED drivers are connected to fixed
> non-managed supplies.
>
>> + }
>> +
>> if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
>> ret = led_add_brightness_hw_changed(led_cdev);
>> if (ret) {
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 7107cd7e87cf..139de6b08cad 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -16,6 +16,7 @@
>> #include <linux/rwsem.h>
>> #include <linux/slab.h>
>> #include "leds.h"
>> +#include <linux/regulator/consumer.h>
>> DECLARE_RWSEM(leds_list_lock);
>> EXPORT_SYMBOL_GPL(leds_list_lock);
>> @@ -23,6 +24,31 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>> LIST_HEAD(leds_list);
>> EXPORT_SYMBOL_GPL(leds_list);
>> +static bool __led_need_regulator_update(struct led_classdev
>> *led_cdev,
>> + int brightness)
>> +{
>> + bool new_regulator_state = (brightness != LED_OFF);
>> +
>> + return led_cdev->regulator_state != new_regulator_state;
>> +}
>> +
>> +static int __led_handle_regulator(struct led_classdev *led_cdev,
>> + int brightness)
>> +{
>> + if (__led_need_regulator_update(led_cdev, brightness)) {
>> + int ret;
>
> Prefer to this to be moved up.
ok
>
>> +
>> + if (brightness != LED_OFF)
>> + ret = regulator_enable(led_cdev->regulator);
>> + else
>> + ret = regulator_disable(led_cdev->regulator);
>> + if (ret)
>> + return ret;
> new line
>> + led_cdev->regulator_state = (brightness != LED_OFF);
>> + }
>> + return 0;
>> +}
>> +
>> static int __led_set_brightness(struct led_classdev *led_cdev,
>> enum led_brightness value)
>> {
>> @@ -80,6 +106,7 @@ static void led_timer_function(struct timer_list *t)
>> }
>> led_set_brightness_nosleep(led_cdev, brightness);
>> + __led_handle_regulator(led_cdev, brightness);
>
> Again this seems to indicate that the regulator is a required property
> for the LEDs
>
> This needs to be made optional. And the same comment through out for
> every call.
>
>
>> /* Return in next iteration if led is in one-shot mode and we
>> are in
>> * the final blink state so that the led is toggled each
>> delay_on +
>> @@ -115,6 +142,8 @@ static void set_brightness_delayed(struct
>> work_struct *ws)
>> if (ret == -ENOTSUPP)
>> ret = __led_set_brightness_blocking(led_cdev,
>> led_cdev->delayed_set_value);
>> + __led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
>> +
>> if (ret < 0 &&
>> /* LED HW might have been unplugged, therefore don't warn */
>> !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
>> @@ -141,6 +170,7 @@ static void led_set_software_blink(struct
>> led_classdev *led_cdev,
>> /* never on - just set to off */
>> if (!delay_on) {
>> led_set_brightness_nosleep(led_cdev, LED_OFF);
>> + __led_handle_regulator(led_cdev, LED_OFF);
>> return;
>> }
>> @@ -148,6 +178,7 @@ static void led_set_software_blink(struct
>> led_classdev *led_cdev,
>> if (!delay_off) {
>> led_set_brightness_nosleep(led_cdev,
>> led_cdev->blink_brightness);
>> + __led_handle_regulator(led_cdev, led_cdev->blink_brightness);
>> return;
>> }
>> @@ -256,8 +287,14 @@ void led_set_brightness_nopm(struct
>> led_classdev *led_cdev,
>> enum led_brightness value)
>> {
>> /* Use brightness_set op if available, it is guaranteed not to
>> sleep */
>> - if (!__led_set_brightness(led_cdev, value))
>> - return;
>> + if (!__led_set_brightness(led_cdev, value)) {
>> + /*
>> + * if regulator state doesn't need to be changed, that is all/
>> + * Otherwise delegate the change to a work queue
>> + */
>> + if (!__led_need_regulator_update(led_cdev, value))
>> + return;
>> + }
>> /* If brightness setting can sleep, delegate it to a work
>> queue task */
>> led_cdev->delayed_set_value = value;
>> @@ -280,6 +317,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>> int led_set_brightness_sync(struct led_classdev *led_cdev,
>> enum led_brightness value)
>> {
>> + int ret;
>> +
>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>> return -EBUSY;
>> @@ -288,7 +327,15 @@ int led_set_brightness_sync(struct
>> led_classdev *led_cdev,
>> if (led_cdev->flags & LED_SUSPENDED)
>> return 0;
>> - return __led_set_brightness_blocking(led_cdev,
>> led_cdev->brightness);
>> + ret = __led_set_brightness_blocking(led_cdev,
>> led_cdev->brightness);
>> + if (ret)
>> + return ret;
>> +
>> + ret = __led_handle_regulator(led_cdev, led_cdev->brightness);
>
> Can't you just return here?
ok
thanks for the review
JJ
>
> Dan
>
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> }
>> EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 9b2bf574a17a..bee8e3f8dddd 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -123,6 +123,10 @@ struct led_classdev {
>> /* Ensures consistent access to the LED Flash Class device */
>> struct mutex led_access;
>> +
>> + /* regulator */
>> + struct regulator *regulator;
>> + bool regulator_state;
>> };
>> extern int of_led_classdev_register(struct device *parent,
^ permalink raw reply
* Re: [PATCH v6 7/8] arm64: dts: mediatek: add mt6765 support
From: CK Hu @ 2019-07-15 8:29 UTC (permalink / raw)
To: Macpaul Lin
Cc: Rob Herring, Marc Zyngier, Ryder Lee, Stephen Boyd, Sean Wang,
Mars Cheng, Owen Chen, Matthias Brugger, linux-arm-kernel,
linux-mediatek, linux-kernel, devicetree, CC Hwang, wsd_upstream,
Loda Chou, linux-serial, linux-clk
In-Reply-To: <1562924653-10056-8-git-send-email-macpaul.lin@mediatek.com>
Hi, Macpaul:
On Fri, 2019-07-12 at 17:43 +0800, Macpaul Lin wrote:
> From: Mars Cheng <mars.cheng@mediatek.com>
>
> Add basic chip support for Mediatek 6765, include
> uart node with correct uart clocks, pwrap device
>
> Add clock controller nodes, include topckgen, infracfg,
> apmixedsys and subsystem.
>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm64/boot/dts/mediatek/Makefile | 1 +
> arch/arm64/boot/dts/mediatek/mt6765-evb.dts | 33 +++
> arch/arm64/boot/dts/mediatek/mt6765.dtsi | 253 ++++++++++++++++++++
> 3 files changed, 287 insertions(+)
> create mode 100644 arch/arm64/boot/dts/mediatek/mt6765-evb.dts
> create mode 100644 arch/arm64/boot/dts/mediatek/mt6765.dtsi
>
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 458bbc422a94..22bdf1a99a62 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt2712-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6755-evb.dtb
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt6765-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6795-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-evb.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-x20-dev.dtb
> diff --git a/arch/arm64/boot/dts/mediatek/mt6765-evb.dts b/arch/arm64/boot/dts/mediatek/mt6765-evb.dts
> new file mode 100644
> index 000000000000..36dddff2b7f8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt6765-evb.dts
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * dts file for Mediatek MT6765
> + *
> + * (C) Copyright 2018. Mediatek, Inc.
> + *
> + * Mars Cheng <mars.cheng@mediatek.com>
> + */
> +
> +/dts-v1/;
> +#include "mt6765.dtsi"
> +
> +/ {
> + model = "MediaTek MT6765 EVB";
> + compatible = "mediatek,mt6765-evb", "mediatek,mt6765";
> +
> + aliases {
> + serial0 = &uart0;
> + };
> +
> + memory@40000000 {
> + device_type = "memory";
> + reg = <0 0x40000000 0 0x1e800000>;
> + };
> +
> + chosen {
> + stdout-path = "serial0:921600n8";
> + };
> +};
> +
> +&uart0 {
> + status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/mediatek/mt6765.dtsi b/arch/arm64/boot/dts/mediatek/mt6765.dtsi
> new file mode 100644
> index 000000000000..2662470fe607
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt6765.dtsi
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * dts file for Mediatek MT6765
> + *
> + * (C) Copyright 2018. Mediatek, Inc.
> + *
> + * Mars Cheng <mars.cheng@mediatek.com>
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/mt6765-clk.h>
> +
> +/ {
> + compatible = "mediatek,mt6765";
> + interrupt-parent = <&sysirq>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + psci {
> + compatible = "arm,psci-0.2";
> + method = "smc";
> + };
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x000>;
> + };
> +
> + cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x001>;
> + };
> +
> + cpu@2 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x002>;
> + };
> +
> + cpu@3 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x003>;
> + };
> +
> + cpu@100 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x100>;
> + };
> +
> + cpu@101 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x101>;
> + };
> +
> + cpu@102 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x102>;
> + };
> +
> + cpu@103 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + enable-method = "psci";
> + reg = <0x103>;
> + };
> + };
> +
> + clocks {
> + clk26m: clk26m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <26000000>;
> + };
> +
> + clk32k: clk32k {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32000>;
> + };
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
> + };
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "simple-bus";
> + ranges;
> +
> + gic: interrupt-controller@c000000 {
> + compatible = "arm,gic-v3";
> + #interrupt-cells = <3>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + reg = <0 0x0c000000 0 0x40000>, /* GICD */
> + <0 0x0c100000 0 0x200000>, /* GICR */
> + <0 0x0c400000 0 0x2000>, /* GICC */
> + <0 0x0c410000 0 0x2000>, /* GICH */
> + <0 0x0c420000 0 0x20000>; /* GICV */
> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + topckgen: syscon@10000000 {
> + compatible = "mediatek,mt6765-topckgen", "syscon";
> + reg = <0 0x10000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + infracfg: syscon@10001000 {
> + compatible = "mediatek,mt6765-infracfg", "syscon";
> + reg = <0 0x10001000 0 0x1000>;
> + interrupts = <GIC_SPI 147 IRQ_TYPE_EDGE_RISING>;
> + #clock-cells = <1>;
> + };
> +
> + pericfg: pericfg@10003000 {
> + compatible = "mediatek,mt6765-pericfg", "syscon";
> + reg = <0 0x10003000 0 0x1000>;
> + };
> +
> + scpsys: scpsys@10006000 {
> + compatible = "mediatek,mt6765-scpsys";
> + reg = <0 0x10006000 0 0x1000>; /* spm */
> + #power-domain-cells = <1>;
> + clocks = <&topckgen CLK_TOP_MFG_SEL>,
> + <&topckgen CLK_TOP_MM_SEL>,
> + <&mmsys_config CLK_MM_SMI_COMMON>,
> + <&mmsys_config CLK_MM_SMI_COMM0>,
> + <&mmsys_config CLK_MM_SMI_COMM1>,
> + <&mmsys_config CLK_MM_SMI_LARB0>,
I think you should remove subsys clock in scpsys device node. I've
discussed in [1].
[1] https://patchwork.kernel.org/patch/11005731/
Regards,
CK
> + <&imgsys CLK_IMG_LARB2>,
> + <&mmsys_config CLK_MM_SMI_IMG>,
> + <&camsys CLK_CAM_LARB3>,
> + <&camsys CLK_CAM_DFP_VAD>,
> + <&camsys CLK_CAM>,
> + <&camsys CLK_CAM_CCU>,
> + <&mmsys_config CLK_MM_SMI_CAM>;
> + clock-names = "mfg", "mm",
> + "mm-0", "mm-1", "mm-2", "mm-3",
> + "isp-0", "isp-1", "cam-0", "cam-1",
> + "cam-2", "cam-3", "cam-4";
> + infracfg = <&infracfg>;
> + smi_comm = <&smi_common>;
> + };
> +
> + apmixed: syscon@1000c000 {
> + compatible = "mediatek,mt6765-apmixedsys", "syscon";
> + reg = <0 0x1000c000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + sysirq: interrupt-controller@10200a80 {
> + compatible = "mediatek,mt6765-sysirq",
> + "mediatek,mt6577-sysirq";
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + reg = <0 0x10200a80 0 0x50>;
> + };
> +
> + uart0: serial@11002000 {
> + compatible = "mediatek,mt6765-uart",
> + "mediatek,mt6577-uart";
> + reg = <0 0x11002000 0 0x400>;
> + interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&infracfg CLK_IFR_UART0>,
> + <&infracfg CLK_IFR_AP_DMA>;
> + clock-names = "baud", "bus";
> + status = "disabled";
> + };
> +
> + uart1: serial@11003000 {
> + compatible = "mediatek,mt6765-uart",
> + "mediatek,mt6577-uart";
> + reg = <0 0x11003000 0 0x400>;
> + interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&infracfg CLK_IFR_UART1>,
> + <&infracfg CLK_IFR_AP_DMA>;
> + clock-names = "baud", "bus";
> + status = "disabled";
> + };
> +
> + audio: syscon@11220000 {
> + compatible = "mediatek,mt6765-audsys", "syscon";
> + reg = <0 0x11220000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + mipi_rx_ana_csi0a: syscon@11c10000 {
> + compatible = "mediatek,mt6765-mipi0a",
> + "syscon";
> + reg = <0 0x11c10000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + mmsys_config: syscon@14000000 {
> + compatible = "mediatek,mt6765-mmsys", "syscon";
> + reg = <0 0x14000000 0 0x1000>;
> + interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_LOW>;
> + #clock-cells = <1>;
> + };
> +
> + smi_common: smi_common@14002000 {
> + compatible = "mediatek,mt6765-smi-common", "syscon";
> + reg = <0 0x14002000 0 0x1000>;
> + };
> +
> + imgsys: syscon@15020000 {
> + compatible = "mediatek,mt6765-imgsys", "syscon";
> + reg = <0 0x15020000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + venc_gcon: syscon@17000000 {
> + compatible = "mediatek,mt6765-vcodecsys", "syscon";
> + reg = <0 0x17000000 0 0x10000>;
> + #clock-cells = <1>;
> + };
> +
> + camsys: syscon@1a000000 {
> + compatible = "mediatek,mt6765-camsys", "syscon";
> + reg = <0 0x1a000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> + }; /* end of soc */
> +};
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Vincent Guittot @ 2019-07-15 8:16 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Sweeney, Sean,
daidavid1, Rajendra Nayak, sibis, Bjorn Andersson, Evan Green,
Android Kernel Team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <CAGETcx90WC2o+ZmkyhOPp1xJbfSk1wpAv2RA-4VgnhJfcsmJiA@mail.gmail.com>
On Tue, 9 Jul 2019 at 21:03, Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Jul 9, 2019 at 12:25 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Sun, 7 Jul 2019 at 23:48, Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Jul 4, 2019 at 12:12 AM Vincent Guittot
> > > <vincent.guittot@linaro.org> wrote:
> > > >
> > > > On Wed, 3 Jul 2019 at 23:33, Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Tue, Jul 2, 2019 at 11:45 PM Vincent Guittot
> > > > > <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 3 Jul 2019 at 03:10, Saravana Kannan <saravanak@google.com> wrote:
> > > > > > >
> > > > > > > Interconnect paths can have different performance points. Now that OPP
> > > > > > > framework supports bandwidth OPP tables, add OPP table support for
> > > > > > > interconnects.
> > > > > > >
> > > > > > > Devices can use the interconnect-opp-table DT property to specify OPP
> > > > > > > tables for interconnect paths. And the driver can obtain the OPP table for
> > > > > > > an interconnect path by calling icc_get_opp_table().
> > > > > >
> > > > > > The opp table of a path must come from the aggregation of OPP tables
> > > > > > of the interconnect providers.
> > > > >
> > > > > The aggregation of OPP tables of the providers is certainly the
> > > > > superset of what a path can achieve, but to say that OPPs for
> > > > > interconnect path should match that superset is an oversimplification
> > > > > of the reality in hardware.
> > > > >
> > > > > There are lots of reasons an interconnect path might not want to use
> > > > > all the available bandwidth options across all the interconnects in
> > > > > the route.
> > > > >
> > > > > 1. That particular path might not have been validated or verified
> > > > > during the HW design process for some of the frequencies/bandwidth
> > > > > combinations of the providers.
> > > >
> > > > All these constraint are provider's constraints and not consumer's one
> > > >
> > > > The consumer asks for a bandwidth according to its needs and then the
> > > > providers select the optimal bandwidth of each interconnect after
> > > > aggregating all the request and according to what OPP have been
> > > > validated
> > >
> > > Not really. The screening can be a consumer specific issue. The
> > > consumer IP itself might have some issue with using too low of a
> > > bandwidth or bandwidth that's not within some range. It should not be
> >
> > How can an IP ask for not enough bandwidth ?
> > It asks the needed bandwidth based on its requirements
>
> The "enough bandwidth" is not always obvious. It's only for very
> simple cases that you can calculate the required bandwidth. Even for
> cases that you think might be "obvious/easy" aren't always easy.
>
> For example, you'd think a display IP would have a fixed bandwidth
> requirement for a fixed resolution screen. But that's far from the
> truth. It can also change as the number of layers change per frame.
> For video decoder/encoder, it depends on how well the frames compress
> with a specific compression scheme.
> So the "required" bandwidth is often a heuristic based on the IP
> frequency or traffic measurement.
>
> But that's not even the point I was making in this specific "bullet".
>
> A hardware IP might be screen/verified with only certain bandwidth
> levels. Or it might have hardware bugs that prevent it from using
> lower bandwidths even though it's technically sufficient. We need a
> way to capture that per path. This is not even a fictional case. This
> has been true multiple times over widely used IPs.
here you are mixing HW constraint on the soc and OPP screening with
bandwidth request from consumer
ICC framework is about getting bandwidth request not trying to fix
some HW/voltage dependency of the SoC
>
> > > the provider's job to take into account all the IP that might be
> > > connected to the interconnects. If the interconnect HW itself didn't
> >
> > That's not what I'm saying. The provider knows which bandwidth the
> > interconnect can provide as it is the ones which configures it. So if
> > the interconnect has a finite number of bandwidth point based probably
> > on the possible clock frequency and others config of the interconnect,
> > it selects the best final config after aggregating the request of the
> > consumer.
>
> I completely agree with this. What you are stating above is how it
> should work and that's the whole point of the interconnect framework.
>
> But this is orthogonal to the point I'm making.
It's not orthogonal because you want to add a OPP table pointer in the
ICC path structure to fix your platform HW constraint whereas it's not
the purpose of the framework IMO
>
> > > change, the provider driver shouldn't need to change. By your
> > > definition, a provider driver will have to account for all the
> > > possible bus masters that might be connected to it across all SoCs.
> >
> > you didn't catch my point
>
> Same. I think we are talking over each other. Let me try again.
>
> You are trying to describe how and interconnect provider and framework
> should work. There's no disagreement there.
>
> My point is that consumers might not want to or can not always use all
> the available bandwidth levels offered by the providers. There can be
> many reasons for that (which is what I listed in my earlier emails)
> and we need a good and generic way to capture that so that everyone
> isn't trying to invent their own property.
And my point is that you want to describe some platform or even UCs
specific constraint in the ICC framework which is not the place to do.
If the consumers might not want to use all available bandwidth because
this is not power efficient as an example, this should be describe
somewhere else to express that there is a shared power domain
between some devices and we shoudl ensure that all devices in this
power domain should use the Optimal Operating Point (optimal freq for
a voltage)
ICC framework describes the bandwidth request that are expressed by
the consumers for the current running state of their IP but it doesn't
reflect the fact that on platform A, the consumer should use bandwidth
X because it will select a voltage level of a shared power domain that
is optimized for the other devices B, C ... . It's up to the provider
to know HW details of the bus that it drives and to make such
decision; the consumer should always request the same
>
> > > That's not good design nor is it scalable.
> > >
> > > > >
> > > > > 2. Similarly during parts screening in the factory, some of the
> > > > > combinations might not have been screened and can't be guaranteed
> > > > > to work.
> > > >
> > > > As above, it's the provider's job to select the final bandwidth
> > > > according to its constraint
> > >
> > > Same reply as above.
> > >
> > > > >
> > > > > 3. Only a certain set of bandwidth levels might make sense to use from
> > > > > a power/performance balance given the device using it. For example:
> > > > > - The big CPU might not want to use some of the lower bandwidths
> > > > > but the little CPU might want to.
> > > > > - The big CPU might not want to use some intermediate bandwidth
> > > > > points if they don't save a lot of power compared to a higher
> > > > > bandwidth levels, but the little CPU might want to.
> > > > > - The little CPU might never want to use the higher set of
> > > > > bandwidth levels since they won't be power efficient for the use
> > > > > cases that might run on it.
> > > >
> > > > These example are quite vague about the reasons why little might never
> > > > want to use higher bandwidth.
> > >
> > > How is it vague? I just said because of power/performance balance.
> > >
> > > > But then, if little doesn't ask high bandwidth it will not use them.
> > >
> > > If you are running a heuristics based algorithm to pick bandwidth,
> > > this is how it'll know NOT to use some of the bandwidth levels.
> >
> > so you want to set a bandwidth according to the cpu frequency which is
> > what has been proposed in other thread
>
> Nope, that's just one heuristic. Often times it's based on hardware
> monitors measuring interconnect activity. If you go look at the SDM845
> in a Pixel 3, almost nothing is directly tied to the CPU frequency.
>
> Even if you are scaling bandwidth based on other hardware
> measurements, you might want to avoid some bandwidth level provided by
> the interconnect providers because it's suboptimal.
>
> For example, when making bandwidth votes to accommodate the big CPUs,
> you might never want to use some of the lower bandwidth levels because
> they are not power efficient for any CPU frequency or any bandwidth
> level. Because at those levels the memory/interconnect is so slow that
> it has a non-trivial utilization increase (because the CPU is
> stalling) of the big CPUs.
>
> Again, this is completely different from what the providers/icc
> framework does. Which is, once the request is made, they aggregate and
> set the actual interconnect frequencies correctly.
>
> > >
> > > > >
> > > > > 4. It might not make sense from a system level power perspective.
> > > > > Let's take an example of a path S (source) -> A -> B -> C -> D
> > > > > (destination).
> > > > > - A supports only 2, 5, 7 and 10 GB/s. B supports 1, 2 ... 10 GB/s.
> > > > > C supports 5 and 10 GB/s
> > > > > - If you combine and list the superset of bandwidth levels
> > > > > supported in that path, that'd be 1, 2, 3, ... 10 GB/s.
> > > > > - Which set of bandwidth levels make sense will depend on the
> > > > > hardware characteristics of the interconnects.
> > > > > - If B is the biggest power sink, then you might want to use all 10
> > > > > levels.
> > > > > - If A is the biggest power sink, then you might want to use all 2,
> > > > > 5 and 10 GB/s of the levels.
> > > > > - If C is the biggest power sink then you might only want to use 5
> > > > > and 10 GB/s
> > > > > - The more hops and paths you get the more convoluted this gets.
> > > > >
> > > > > 5. The design of the interconnects themselves might have an impact on
> > > > > which bandwidth levels are used.
> > > > > - For example, the FIFO depth between two specific interconnects
> > > > > might affect the valid bandwidth levels for a specific path.
> > > > > - Say S1 -> A -> B -> D1, S2 -> C -> B -> D1 and S2 -> C -> D2 are
> > > > > three paths.
> > > > > - If C <-> B FIFO depth is small, then there might be a requirement
> > > > > that C and B be closely performance matched to avoid system level
> > > > > congestion due to back pressure.
> > > > > - So S2 -> D1 path can't use all the bandwidth levels supported by
> > > > > C-B combination.
> > > > > - But S2 -> D2 can use all the bandwidth levels supported by C.
> > > > > - And S1 -> D1 can use all the levels supported by A-B combination.
> > > > >
> > > >
> > > > All the examples above makes sense but have to be handle by the
> > > > provider not the consumer. The consumer asks for a bandwidth according
> > > > to its constraints. Then the provider which is the driver that manages
> > > > the interconnect IP, should manage all this hardware and platform
> > > > specific stuff related to the interconnect IP in order to set the
> > > > optimal bandwidth that fit both consumer constraint and platform
> > > > specific configuration.
> > >
> > > Sure, but the provider itself can have interconnect properties to
> > > indicate which other interconnects it's tied to. And the provider will
> > > still need the interconnect-opp-table to denote which bandwidth levels
> > > are sensible to use with each of its connections.
>
> You seem to have missed this comment.
>
> Thanks,
> Saravana
>
> > > So in some instances the interconnect-opp-table covers the needs of
> > > purely consumers and in some instances purely providers. But in either
> > > case, it's still needed to describe the hardware properly.
> > >
> > > -Saravana
> > >
> > > > > These are just some of the reasons I could recollect in a few minutes.
> > > > > These are all real world cases I had to deal with in the past several
> > > > > years of dealing with scaling interconnects. I'm sure vendors and SoCs
> > > > > I'm not familiar with have other good reasons I'm not aware of.
> > > > >
> > > > > Trying to figure this all out by aggregating OPP tables of
> > > > > interconnect providers just isn't feasible nor is it efficient. The
> > > > > OPP tables for an interconnect path is describing the valid BW levels
> > > > > supported by that path and verified in hardware and makes a lot of
> > > > > sense to capture it clearly in DT.
> > > > >
> > > > > > So such kind of OPP table should be at
> > > > > > provider level but not at path level.
> > > > >
> > > > > They can also use it if they want to, but they'll probably want to use
> > > > > a frequency OPP table.
> > > > >
> > > > >
> > > > > -Saravana
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > > > > ---
> > > > > > > drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
> > > > > > > include/linux/interconnect.h | 7 +++++++
> > > > > > > 2 files changed, 33 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> > > > > > > index 871eb4bc4efc..881bac80bc1e 100644
> > > > > > > --- a/drivers/interconnect/core.c
> > > > > > > +++ b/drivers/interconnect/core.c
> > > > > > > @@ -47,6 +47,7 @@ struct icc_req {
> > > > > > > */
> > > > > > > struct icc_path {
> > > > > > > size_t num_nodes;
> > > > > > > + struct opp_table *opp_table;
> > > > > > > struct icc_req reqs[];
> > > > > > > };
> > > > > > >
> > > > > > > @@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > > {
> > > > > > > struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> > > > > > > struct icc_node *src_node, *dst_node;
> > > > > > > - struct device_node *np = NULL;
> > > > > > > + struct device_node *np = NULL, *opp_node;
> > > > > > > struct of_phandle_args src_args, dst_args;
> > > > > > > int idx = 0;
> > > > > > > int ret;
> > > > > > > @@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> > > > > > > dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> > > > > > > mutex_unlock(&icc_lock);
> > > > > > >
> > > > > > > + opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
> > > > > > > + if (opp_node) {
> > > > > > > + path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
> > > > > > > + of_node_put(opp_node);
> > > > > > > + }
> > > > > > > +
> > > > > > > +
> > > > > > > return path;
> > > > > > > }
> > > > > > > EXPORT_SYMBOL_GPL(of_icc_get);
> > > > > > >
> > > > > > > +/**
> > > > > > > + * icc_get_opp_table() - Get the OPP table that corresponds to a path
> > > > > > > + * @path: reference to the path returned by icc_get()
> > > > > > > + *
> > > > > > > + * This function will return the OPP table that corresponds to a path handle.
> > > > > > > + * If the interconnect API is disabled, NULL is returned and the consumer
> > > > > > > + * drivers will still build. Drivers are free to handle this specifically, but
> > > > > > > + * they don't have to.
> > > > > > > + *
> > > > > > > + * Return: opp_table pointer on success. NULL is returned when the API is
> > > > > > > + * disabled or the OPP table is missing.
> > > > > > > + */
> > > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > > +{
> > > > > > > + return path->opp_table;
> > > > > > > +}
> > > > > > > +
> > > > > > > /**
> > > > > > > * icc_set_bw() - set bandwidth constraints on an interconnect path
> > > > > > > * @path: reference to the path returned by icc_get()
> > > > > > > diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> > > > > > > index dc25864755ba..0c0bc55f0e89 100644
> > > > > > > --- a/include/linux/interconnect.h
> > > > > > > +++ b/include/linux/interconnect.h
> > > > > > > @@ -9,6 +9,7 @@
> > > > > > >
> > > > > > > #include <linux/mutex.h>
> > > > > > > #include <linux/types.h>
> > > > > > > +#include <linux/pm_opp.h>
> > > > > > >
> > > > > > > /* macros for converting to icc units */
> > > > > > > #define Bps_to_icc(x) ((x) / 1000)
> > > > > > > @@ -28,6 +29,7 @@ struct device;
> > > > > > > struct icc_path *icc_get(struct device *dev, const int src_id,
> > > > > > > const int dst_id);
> > > > > > > struct icc_path *of_icc_get(struct device *dev, const char *name);
> > > > > > > +struct opp_table *icc_get_opp_table(struct icc_path *path);
> > > > > > > void icc_put(struct icc_path *path);
> > > > > > > int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> > > > > > >
> > > > > > > @@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
> > > > > > > {
> > > > > > > }
> > > > > > >
> > > > > > > +static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
> > > > > > > +{
> > > > > > > + return NULL;
> > > > > > > +}
> > > > > > > +
> > > > > > > static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> > > > > > > {
> > > > > > > return 0;
> > > > > > > --
> > > > > > > 2.22.0.410.gd8fdbe21b5-goog
> > > > > > >
> > > >
> > > > --
> > > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > > >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox