* [PATCH 2/2] devicetree: add vendor prefix for Zeitec
From: Jelle van der Waa @ 2016-12-23 16:32 UTC (permalink / raw)
To: Rob Herring, Dmitry Torokhov, linux-input, devicetree; +Cc: Jelle van der Waa
In-Reply-To: <20161223163214.7716-1-jelle@vdwaa.nl>
Signed-off-by: Jelle van der Waa <jelle@vdwaa.nl>
---
Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 16d3b5e7f5d1..a3d7608a5cb3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -329,6 +329,7 @@ xes Extreme Engineering Solutions (X-ES)
xillybus Xillybus Ltd.
xlnx Xilinx
zarlink Zarlink Semiconductor
+zeitec ZEITEC Semiconductor Co., LTD.
zii Zodiac Inflight Innovations
zte ZTE Corp.
zyxel ZyXEL Communications Corp.
--
2.11.0
^ permalink raw reply related
* [PATCH 1/2] input: touchscreen: add driver for Zeitec ZET6223
From: Jelle van der Waa @ 2016-12-23 16:32 UTC (permalink / raw)
To: Rob Herring, Dmitry Torokhov, linux-input, devicetree; +Cc: Jelle van der Waa
This is a basic driver for the Zeitec ZET6223 I2C touchscreen
controllers. The driver does not support firmware loading, which is not
required for all tablets which contain this chip.
Signed-off-by: Jelle van der Waa <jelle@vdwaa.nl>
---
.../bindings/input/touchscreen/zet6223.txt | 29 +++
drivers/input/touchscreen/Kconfig | 11 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/zet6223.c | 198 +++++++++++++++++++++
4 files changed, 239 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/zet6223.txt
create mode 100644 drivers/input/touchscreen/zet6223.c
diff --git a/Documentation/devicetree/bindings/input/touchscreen/zet6223.txt b/Documentation/devicetree/bindings/input/touchscreen/zet6223.txt
new file mode 100644
index 000000000000..cc0f7d06bc8b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/zet6223.txt
@@ -0,0 +1,29 @@
+Zeitec ZET6223 I2C touchscreen controller
+
+Required properties:
+ - compatible : "zeitec,zet6223"
+ - reg : I2C slave address of the chip (0x76)
+ - interrupt-parent : a phandle pointing to the interrupt controller
+ serving the interrupt for this chip
+ - interrupts : interrupt specification for the zet6223 interrupt
+
+Optional properties:
+
+- touchscreen-size-x : See touchscreen.txt
+- touchscreen-size-y : See touchscreen.txt
+- touchscreen-inverted-x : See touchscreen.txt
+- touchscreen-inverted-y : See touchscreen.txt
+- touchscreen-swapped-x-y : See touchscreen.txt
+
+Example:
+
+i2c@00000000 {
+
+ zet6223: touchscreen@76 {
+ compatible = "zeitec,zet6223";
+ reg = <0x76>;
+ interrupt-parent = <&pio>;
+ interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>
+ };
+
+};
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index efca0133e266..3c70adfe676d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1214,4 +1214,15 @@ config TOUCHSCREEN_ROHM_BU21023
To compile this driver as a module, choose M here: the
module will be called bu21023_ts.
+config TOUCHSCREEN_ZET6223
+ tristate "Zeitec ZET6223 touchscreen driver"
+ depends on I2C
+ help
+ Say Y here if you have a touchscreen using Zeitec ZET6223
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called zet6223.
+
endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 81b86451782d..11f8953613cd 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -99,3 +99,4 @@ obj-$(CONFIG_TOUCHSCREEN_TPS6507X) += tps6507x-ts.o
obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50) += colibri-vf50-ts.o
obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023) += rohm_bu21023.o
+obj-$(CONFIG_TOUCHSCREEN_ZET6223) += zet6223.o
diff --git a/drivers/input/touchscreen/zet6223.c b/drivers/input/touchscreen/zet6223.c
new file mode 100644
index 000000000000..aecae06877cf
--- /dev/null
+++ b/drivers/input/touchscreen/zet6223.c
@@ -0,0 +1,198 @@
+/*
+ * Copyright (C) 2016, Jelle van der Waa <jelle@vdwaa.nl>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/module.h>
+
+#define ZET6223_CMD_INFO 0xB2
+#define ZET6223_CMD_INFO_LENGTH 17
+#define ZET6223_VALID_PACKET 0x3c
+
+struct zet6223_data {
+ struct i2c_client *client;
+ struct input_dev *input;
+ struct touchscreen_properties prop;
+ u8 fingernum;
+};
+
+static int zet6223_start(struct input_dev *dev)
+{
+ struct zet6223_data *data = input_get_drvdata(dev);
+
+ enable_irq(data->client->irq);
+
+ return 0;
+}
+
+static void zet6223_stop(struct input_dev *dev)
+{
+ struct zet6223_data *data = input_get_drvdata(dev);
+
+ disable_irq(data->client->irq);
+}
+
+static irqreturn_t irqreturn_t_zet6223(int irq, void *dev_id)
+{
+ struct zet6223_data *data = dev_id;
+ struct device *dev = &data->client->dev;
+ /*
+ * First 3 bytes are an identifier, two bytes of finger data.
+ * X, Y data per finger is 4 bytes.
+ */
+ u8 bufsize = 3 + 4 * data->fingernum;
+ u8 buf[bufsize];
+ u8 i;
+ u16 finger_bits;
+ int ret;
+
+ ret = i2c_master_recv(data->client, buf, bufsize);
+ if (ret != bufsize) {
+ dev_err_ratelimited(dev, "Error reading input data: %d\n", ret);
+ return IRQ_HANDLED;
+ }
+
+ if (buf[0] != ZET6223_VALID_PACKET)
+ return IRQ_HANDLED;
+
+ finger_bits = get_unaligned_be16(buf + 1);
+ for (i = 0; i < data->fingernum; i++) {
+ if (!(finger_bits & BIT(15 - i)))
+ continue;
+
+ input_mt_slot(data->input, i);
+ input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
+ input_event(data->input, EV_ABS, ABS_MT_POSITION_X,
+ ((buf[i + 3] >> 4) << 8) + buf[i + 4]);
+ input_event(data->input, EV_ABS, ABS_MT_POSITION_Y,
+ ((buf[i + 3] & 0xF) << 8) + buf[i + 5]);
+ }
+
+ input_mt_sync_frame(data->input);
+ input_sync(data->input);
+
+ return IRQ_HANDLED;
+}
+
+static int zet6223_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct zet6223_data *data;
+ struct input_dev *input;
+ u8 buf[ZET6223_CMD_INFO_LENGTH];
+ u8 cmd = ZET6223_CMD_INFO;
+ int ret;
+
+ if (!client->irq) {
+ dev_err(dev, "Error no irq specified\n");
+ return -EINVAL;
+ }
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = i2c_master_send(client, &cmd, 1);
+ if (ret < 0) {
+ dev_err(dev, "touchpanel info cmd failed: %d\n", ret);
+ return -ENODEV;
+ }
+
+ ret = i2c_master_recv(client, buf, ZET6223_CMD_INFO_LENGTH);
+ if (ret < 0) {
+ dev_err(dev, "cannot retrieve touchpanel info: %d\n", ret);
+ return -ENODEV;
+ }
+
+ data->fingernum = buf[15] & 0x7F;
+ if (data->fingernum > 16) {
+ data->fingernum = 16;
+ dev_warn(dev, "touchpanel reports more then 16 fingers, limit to 16");
+ }
+
+ input = devm_input_allocate_device(dev);
+ if (!input)
+ return -ENOMEM;
+
+ input_set_abs_params(input, ABS_MT_POSITION_X, 0,
+ get_unaligned_le16(&buf[8]), 0, 0);
+ input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
+ get_unaligned_le16(&buf[10]), 0, 0);
+ touchscreen_parse_properties(input, true, &data->prop);
+
+ input->name = client->name;
+ input->id.bustype = BUS_I2C;
+ input->dev.parent = dev;
+ input->open = zet6223_start;
+ input->close = zet6223_stop;
+
+ ret = input_mt_init_slots(input, data->fingernum,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (ret)
+ return ret;
+
+ data->client = client;
+ data->input = input;
+
+ input_set_drvdata(input, data);
+
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ irqreturn_t_zet6223, IRQF_ONESHOT, client->name, data);
+ if (ret) {
+ dev_err(dev, "Error requesting irq: %d\n", ret);
+ return ret;
+ }
+
+ zet6223_stop(input);
+
+ ret = input_register_device(input);
+ if (ret)
+ return ret;
+
+ i2c_set_clientdata(client, data);
+
+ return 0;
+}
+
+static const struct of_device_id zet6223_of_match[] = {
+ { .compatible = "zeitec", "zet6223" },
+ { }
+};
+
+static const struct i2c_device_id zet6223_id[] = {
+ { "zet6223", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, zet6223_id);
+
+static struct i2c_driver zet6223_driver = {
+ .driver = {
+ .name = "zet6223",
+ .of_match_table = zet6223_of_match,
+ },
+ .probe = zet6223_probe,
+ .id_table = zet6223_id
+};
+
+module_i2c_driver(zet6223_driver);
+
+MODULE_AUTHOR("Jelle van der Waa <jelle@vdwaa.nl>");
+MODULE_DESCRIPTION("ZEITEC zet622x I2C touchscreen driver");
+MODULE_LICENSE("GPL");
--
2.11.0
^ permalink raw reply related
* [PATCH] Documentation: devicetree: "linux,usable-memory" property
From: Heinrich Schuchardt @ 2016-12-23 16:17 UTC (permalink / raw)
To: Rob Herring, Frank Rowand
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Heinrich Schuchardt
Memory nodes may have a "linux,usable-memory" property
overriding the reg property.
This patch adds the missing documentation.
Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---
Documentation/devicetree/booting-without-of.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/booting-without-of.txt b/Documentation/devicetree/booting-without-of.txt
index 280d283304bb..92712c5604b0 100644
--- a/Documentation/devicetree/booting-without-of.txt
+++ b/Documentation/devicetree/booting-without-of.txt
@@ -981,6 +981,10 @@ compatibility.
removed later. The kernel can take this into consideration when
doing nonmovable allocations and when laying out memory zones.
+ - linux,usable-memory : This property overrides the reg property.
+ It can be used if the bootloader changes the reg property to
+ improper values.
+
e) The /chosen node
This node is a bit "special". Normally, that's where Open Firmware
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 2/2] ARM: dts: imx6q-icore-rqs: Update model to support Dual SOM
From: Jagan Teki @ 2016-12-23 16:02 UTC (permalink / raw)
To: Shawn Guo
Cc: linux-arm-kernel, devicetree, linux-kernel, Matteo Lisi,
Michael Trimarchi, Jagan Teki
In-Reply-To: <1482508935-9414-1-git-send-email-jagan@openedev.com>
From: Jagan Teki <jagan@amarulasolutions.com>
Engicam i.CoreM6 Dual and Quad SOM's use same dts file, hence
update model name to add Dual and also added full mode decsription.
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Matteo Lisi <matteo.lisi@engicam.com>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
arch/arm/boot/dts/imx6q-icore-rqs.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6q-icore-rqs.dts b/arch/arm/boot/dts/imx6q-icore-rqs.dts
index 0053188..76757f8 100644
--- a/arch/arm/boot/dts/imx6q-icore-rqs.dts
+++ b/arch/arm/boot/dts/imx6q-icore-rqs.dts
@@ -45,7 +45,7 @@
#include "imx6qdl-icore-rqs.dtsi"
/ {
- model = "Engicam i.CoreM6 Quad SOM";
+ model = "Engicam i.CoreM6 Quad/Dual RQS Starter Kit";
compatible = "engicam,imx6-icore-rqs", "fsl,imx6q";
sound {
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] ARM: dts: imx6dl: Add Engicam i.CoreM6 DualLite/Solo RQS initial support
From: Jagan Teki @ 2016-12-23 16:02 UTC (permalink / raw)
To: Shawn Guo
Cc: linux-arm-kernel, devicetree, linux-kernel, Matteo Lisi,
Michael Trimarchi, Jagan Teki
From: Jagan Teki <jagan@amarulasolutions.com>
i.CoreM6 DualLite/Solo modules are system on module solutions manufactured
by Engicam with following characteristics:
CPU NXP i.MX6 DL, 800MHz
RAM 1GB, 32, 64 bit, DDR3-800/1066
NAND SLC,512MB
Power supply Single 5V
MAX LCD RES FULLHD
and more info at
http://www.engicam.com/en/products/embedded/som/standard/i-core-rqs-m6s-dl-d-q
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Matteo Lisi <matteo.lisi@engicam.com>
Cc: Michael Trimarchi <michael@amarulasolutions.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/imx6dl-icore-rqs.dts | 51 ++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
create mode 100644 arch/arm/boot/dts/imx6dl-icore-rqs.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index cccdbcb..51f8dae 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -349,6 +349,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
imx6dl-gw553x.dtb \
imx6dl-hummingboard.dtb \
imx6dl-icore.dtb \
+ imx6dl-icore-rqs.dtb \
imx6dl-nit6xlite.dtb \
imx6dl-nitrogen6x.dtb \
imx6dl-phytec-pbab01.dtb \
diff --git a/arch/arm/boot/dts/imx6dl-icore-rqs.dts b/arch/arm/boot/dts/imx6dl-icore-rqs.dts
new file mode 100644
index 0000000..5c19927
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-icore-rqs.dts
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2016 Amarula Solutions B.V.
+ * Copyright (C) 2016 Engicam S.r.l.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This file is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Or, alternatively
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED , WITHOUT WARRANTY OF ANY KIND
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6q.dtsi"
+#include "imx6qdl-icore-rqs.dtsi"
+
+/ {
+ model = "Engicam i.CoreM6 DualLite/Solo RQS Starter Kit";
+ compatible = "engicam,imx6-icore-rqs", "fsl,imx6dl";
+};
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v7 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC
From: M'boumba Cedric Madianga @ 2016-12-23 13:09 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
Linus Walleij, Patrice Chotard, linux, Rob Herring, linux-i2c,
Maxime Coquelin, linux-arm-kernel
In-Reply-To: <20161222191103.vzmfhnrtrzw2ivwa@pengutronix.de>
Hi,
2016-12-22 20:11 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Dec 22, 2016 at 02:35:02PM +0100, M'boumba Cedric Madianga wrote:
>> @@ -337,6 +350,16 @@
>> slew-rate = <2>;
>> };
>> };
>> +
>> + i2c1_pins_b: i2c1@0 {
>> + pins1 {
>> + pinmux = <STM32F429_PB9_FUNC_I2C1_SDA>;
>> + drive-open-drain;
>> + };
>> + pins2 {
>> + pinmux = <STM32F429_PB6_FUNC_I2C1_SCL>;
>> + };
>
> the second doesn't need the open-drain property? Why?
I thought that open-drain was only needed for SDA line.
But after double-checking I2C specification, it seems that SDA and SCL
lines need open-drain or open-collector to perform the wired-AND
function.
I will do some trials with this config and will fix it in the V8.
Thanks
>
>> + };
>> };
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] iio: misc: add a generic regulator driver
From: Geert Uytterhoeven @ 2016-12-23 12:56 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Bartosz Golaszewski, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Rob Herring, Mark Rutland,
linux-iio-u79uwXL29TY76Z2rM5mHXA, linux-devicetree, LKML,
Kevin Hilman, Patrick Titiano, Neil Armstrong, Liam Girdwood,
Mark Brown
In-Reply-To: <9609b56b-194c-9899-1142-ff2ee285c6bd-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
Hi Lars,
On Fri, Dec 23, 2016 at 12:35 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
> On 12/23/2016 11:00 AM, Geert Uytterhoeven wrote:
>> On Mon, Dec 12, 2016 at 6:15 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>>> On 12/06/2016 12:12 PM, Bartosz Golaszewski wrote:
>>>> We're already using libiio to read the measured data from the power
>>>> monitor, that's why we'd like to use the iio framework for
>>>> power-cycling the devices as well. My question is: would bridging the
>>>> regulator framework be the right solution? Should we look for
>>>> something else? Bridge the GPIO framework instead?
>>>
>>> I wouldn't necessaries create bridge, but instead just use the GPIO
>>> framework directly.
>>>
>>> We now have the GPIO chardev interface which meant to be used to support
>>> application specific logic that control the GPIOs, but where you don't want
>>> to write a kernel driver.
>>>
>>> My idea was to add GPIOs and GPIO chips as high level object inside libiio
>>> that can be accessed through the same context as the IIO devices. Similar to
>>> the current IIO API you have a API for gpios that allows to enumerate the
>>> GPIO devices and their pins as well as modify the pin state.
>>
>> That would mean libiio has access to all GPIOs, allowing a remote person
>> to not only control through iiod the GPIOs for industrial control, but also the
>> GPIOs not intended for export, right?
>
> Well, it is a policy question. Who gets access to what. Right now it is all
> or nothing, a privileged application gets access to all devices/GPIOs, a
> unprivileged application gets access to nothing. Same for GPIOs as well as
> IIO devices.
>
> iiod at the moment does not have any access control at all, which in itself
> is a problem. We need to add support for that at some point. I don't see an
> issue with implementing a finer grained access scheme when we do so. E.g.
> unprivileged applications only get access to certain pins.
OK, so that's WIP.
>> Having a separate GPIO switch driver avoids that, as DT (or some other means)
>> can be used to specify and label the GPIOs for IIO use.
>
> Sure, functionally this would be equivalent, but we have to ask whether this
> is the right way to use the DT. Is access policy specification part of the
> hardware description? In my opinion the answer is no. At the hardware
> description level there is no operating system, there is no userspace or
> kernelspace, there is are no access levels. Putting the distinction between
> a switch/regulator that can be controlled from userspace or can only be
> controlled from kernel space into the DT would be a layering violation. It
> is analogous to why we don't have spidev DT bindings. This is an issue that
> needs to be solved at a higher level. In my opinion this level is a
> cooperation between kernel- and userspace. Kernelspace offering an interface
> to export a device for userspace access and userspace making use of that
> interface to request access to a device. In a similar way to how vfio is
> structured.
I'm not advocating using DT for policy, only for hardware description.
We have means (bindings) to describe GPIOs connected to LEDs and switches
(incl. their labels), while you can control LEDs through plain GPIO sysfs
export or chardev, too. It's just more error prone to use the latter.
We do not have bindings to describe GPIOs connected to e.g. relays.
Switching external devices (the internals of those devices not described
itself in DT, like in an industrial context), sounds more like something to
be handled by IIO, doesn't it?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/1] ARM64: dts: meson-gxbb-odroidc2: linux,usable-memory
From: Heinrich Schuchardt @ 2016-12-23 12:52 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon,
Carlo Caione, Kevin Hilman, Neil Armstrong
Cc: Heinrich Schuchardt, devicetree, linux-kernel, linux-arm-kernel,
linux-amlogic
After reading the fdt u-boot overwrites the reg property of the
memory node with <0x0 0x0 0x0 0x78000000>.
To override this setting we have to use the property
linux,usable-memory.
If the first 16MB of the 2GB physical memory are used by
the Linux kernel oops occur on the Odroid C2.
For the Odroid C2 this patch replaces
[RFT PATCH] ARM64: dts: meson-gxbb: Add reserved memory zone and
usable memory range
https://lkml.org/lkml/2016/12/12/127
Fixes: 855960342d1e7 ARM64: dts: amlogic: add Hardkernel ODROID-C2
CC: Neil Armstrong <narmstrong@baylibre.com>
CC: Kevin Hilman <khilman@baylibre.com>
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
index 238fbeacd330..82ab94417940 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
@@ -61,7 +61,7 @@
memory@0 {
device_type = "memory";
- reg = <0x0 0x0 0x0 0x80000000>;
+ linux,usable-memory = <0x0 0x01000000 0x0 0x7f000000>;
};
usb_otg_pwr: regulator-usb-pwrs {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v7 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2016-12-23 12:41 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, linux, linux-i2c, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <20161223090019.a3jkhpb3abgjqi55@pengutronix.de>
Hi Uwe,
Thanks for your comments.
Please see below my answers and one question regarding duty cycle:
2016-12-23 10:00 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Thu, Dec 22, 2016 at 02:35:01PM +0100, M'boumba Cedric Madianga wrote:
>> This patch adds support for the STM32F4 I2C controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> ---
>> drivers/i2c/busses/Kconfig | 10 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-stm32f4.c | 896 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 907 insertions(+)
>> create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 0cdc844..2719208 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -886,6 +886,16 @@ config I2C_ST
>> This driver can also be built as module. If so, the module
>> will be called i2c-st.
>>
>> +config I2C_STM32F4
>> + tristate "STMicroelectronics STM32F4 I2C support"
>> + depends on ARCH_STM32 || COMPILE_TEST
>> + help
>> + Enable this option to add support for STM32 I2C controller embedded
>> + in STM32F4 SoCs.
>> +
>> + This driver can also be built as module. If so, the module
>> + will be called i2c-stm32f4.
>> +
>> config I2C_STU300
>> tristate "ST Microelectronics DDC I2C interface"
>> depends on MACH_U300
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 1c1bac8..a2c6ff5 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
>> obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
>> obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
>> obj-$(CONFIG_I2C_ST) += i2c-st.o
>> +obj-$(CONFIG_I2C_STM32F4) += i2c-stm32f4.o
>> obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
>> obj-$(CONFIG_I2C_SUN6I_P2WI) += i2c-sun6i-p2wi.o
>> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o
>> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
>> new file mode 100644
>> index 0000000..ca11dee
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-stm32f4.c
>> @@ -0,0 +1,896 @@
>> +/*
>> + * Driver for STMicroelectronics STM32 I2C controller
>> + *
>> + * This I2C controller is described in the STM32F429/439 Soc reference manual.
>> + * Please see below a link to the documentation:
>> + * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
>> + *
>> + * Copyright (C) M'boumba Cedric Madianga 2016
>> + * Author: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
>> + *
>> + * This driver is based on i2c-st.c
>> + *
>> + * License terms: GNU General Public License (GPL), version 2
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset.h>
>> +
>> +/* STM32F4 I2C offset registers */
>> +#define STM32F4_I2C_CR1 0x00
>> +#define STM32F4_I2C_CR2 0x04
>> +#define STM32F4_I2C_DR 0x10
>> +#define STM32F4_I2C_SR1 0x14
>> +#define STM32F4_I2C_SR2 0x18
>> +#define STM32F4_I2C_CCR 0x1C
>> +#define STM32F4_I2C_TRISE 0x20
>> +#define STM32F4_I2C_FLTR 0x24
>> +
>> +/* STM32F4 I2C control 1*/
>> +#define STM32F4_I2C_CR1_SWRST BIT(15)
>> +#define STM32F4_I2C_CR1_POS BIT(11)
>> +#define STM32F4_I2C_CR1_ACK BIT(10)
>> +#define STM32F4_I2C_CR1_STOP BIT(9)
>> +#define STM32F4_I2C_CR1_START BIT(8)
>> +#define STM32F4_I2C_CR1_PE BIT(0)
>> +
>> +/* STM32F4 I2C control 2 */
>> +#define STM32F4_I2C_CR2_FREQ_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_CR2_FREQ(n) (((n) & STM32F4_I2C_CR2_FREQ_MASK))
>
> ((n) & STM32F4_I2C_CR2_FREQ_MASK)
>
> should be enough.
You are right. I will fix it in the V8.
>
>> +#define STM32F4_I2C_CR2_ITBUFEN BIT(10)
>> +#define STM32F4_I2C_CR2_ITEVTEN BIT(9)
>> +#define STM32F4_I2C_CR2_ITERREN BIT(8)
>> +#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN | \
>> + STM32F4_I2C_CR2_ITEVTEN | \
>> + STM32F4_I2C_CR2_ITERREN)
>> +
>> +/* STM32F4 I2C Status 1 */
>> +#define STM32F4_I2C_SR1_AF BIT(10)
>> +#define STM32F4_I2C_SR1_ARLO BIT(9)
>> +#define STM32F4_I2C_SR1_BERR BIT(8)
>> +#define STM32F4_I2C_SR1_TXE BIT(7)
>> +#define STM32F4_I2C_SR1_RXNE BIT(6)
>> +#define STM32F4_I2C_SR1_BTF BIT(2)
>> +#define STM32F4_I2C_SR1_ADDR BIT(1)
>> +#define STM32F4_I2C_SR1_SB BIT(0)
>> +#define STM32F4_I2C_SR1_ITEVTEN_MASK (STM32F4_I2C_SR1_BTF | \
>> + STM32F4_I2C_SR1_ADDR | \
>> + STM32F4_I2C_SR1_SB)
>> +#define STM32F4_I2C_SR1_ITBUFEN_MASK (STM32F4_I2C_SR1_TXE | \
>> + STM32F4_I2C_SR1_RXNE)
>> +#define STM32F4_I2C_SR1_ITERREN_MASK (STM32F4_I2C_SR1_AF | \
>> + STM32F4_I2C_SR1_ARLO | \
>> + STM32F4_I2C_SR1_BERR)
>> +
>> +/* STM32F4 I2C Status 2 */
>> +#define STM32F4_I2C_SR2_BUSY BIT(1)
>> +
>> +/* STM32F4 I2C Control Clock */
>> +#define STM32F4_I2C_CCR_CCR_MASK GENMASK(11, 0)
>> +#define STM32F4_I2C_CCR_CCR(n) (((n) & STM32F4_I2C_CCR_CCR_MASK))
>
> ditto
ok
>
>> +#define STM32F4_I2C_CCR_FS BIT(15)
>> +#define STM32F4_I2C_CCR_DUTY BIT(14)
>> +
>> +/* STM32F4 I2C Trise */
>> +#define STM32F4_I2C_TRISE_VALUE_MASK GENMASK(5, 0)
>> +#define STM32F4_I2C_TRISE_VALUE(n) (((n) & STM32F4_I2C_TRISE_VALUE_MASK))
>> +
>> +/* STM32F4 I2C Filter */
>> +#define STM32F4_I2C_FLTR_DNF_MASK GENMASK(3, 0)
>> +#define STM32F4_I2C_FLTR_DNF(n) (((n) & STM32F4_I2C_FLTR_DNF_MASK))
>> +#define STM32F4_I2C_FLTR_ANOFF BIT(4)
>> +
>> +#define STM32F4_I2C_MIN_FREQ 2U
>> +#define STM32F4_I2C_MAX_FREQ 42U
>> +#define HZ_TO_MHZ 1000000
>> +
>> +enum stm32f4_i2c_speed {
>> + STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
>> + STM32F4_I2C_SPEED_FAST, /* 400 kHz */
>> + STM32F4_I2C_SPEED_END,
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
>> + * @duty: Fast mode duty cycle
>> + * @scl_period: SCL low/high period in microsecond
>> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
>> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
>
> s/Khz/ kHz/
Good point. Thanks
>
>> + */
>> +struct stm32f4_i2c_timings {
>> + u32 duty;
>> + u32 scl_period;
>> + u32 mul_ccr;
>> + u32 min_ccr;
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_msg - client specific data
>> + * @addr: 8-bit slave addr, including r/w bit
>> + * @count: number of bytes to be transferred
>> + * @buf: data buffer
>> + * @result: result of the transfer
>> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
>> + */
>> +struct stm32f4_i2c_msg {
>> + u8 addr;
>> + u32 count;
>> + u8 *buf;
>> + int result;
>> + bool stop;
>
> You bought the argument about alignment of = in stm32f4_i2c_driver. The
> same logic applies here.
ok
>
>> +};
>> +
>> +/**
>> + * struct stm32f4_i2c_dev - private data of the controller
>> + * @adap: I2C adapter for this controller
>> + * @dev: device for this controller
>> + * @base: virtual memory area
>> + * @complete: completion of I2C message
>> + * @clk: hw i2c clock
>> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
>> + * @msg: I2C transfer information
>> + */
>> +struct stm32f4_i2c_dev {
>> + struct i2c_adapter adap;
>> + struct device *dev;
>> + void __iomem *base;
>> + struct completion complete;
>> + struct clk *clk;
>> + int speed;
>> + struct stm32f4_i2c_msg msg;
>> +};
>
> ditto
ok
>
>> +
>> +/*
>> + * In standard mode:
>> + * SCL high period = SCL low period = CCR * I2C CLK period
>> + * So, CCR = SCL period * I2C CLK frequency
>
> is "SCL period" the same as "SCL low period"? I2C CLK frequency is the
> input clk? If so, it's confusing that it has I2C in its name. The
> reference manual calls it PCLK1 (parent clock?).
SCL high period = SCL low period
I2C CLK frequency = I2C parent clock frequency so I will add this
precision in the V8
>
>> + * In fast mode:
>> + * DUTY = 0: Fast mode tlow/thigh = 2
>> + * DUTY = 1: Fast mode tlow/thigh = 16/9
>> + * If Duty = 0; SCL high period = 1 * CCR * I2C CLK period
>> + * SCL low period = 2 * CCR * I2C CLK period
>> + * If Duty = 1; SCL high period = 9 * CCR * I2C CLK period
>> + * SCL low period = 16 * CCR * I2C CLK period
>
> I'd drop the first two lines about the proportions
>
>> + *
>> + * Note that Duty has to bet set to reach 400khz in Fast mode
>
> s/khz/ kHz/
Ok. Thanks.
>
> I don't understand why DUTY is required to reach 400 kHz. Given a parent
> freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
>
> t_high = 25 * 33.333 ns = 833.333 ns
> t_low = 2 * 25 * 33.333 ns = 1666.667 ns
>
> then t_high and t_low satisfy the i2c bus specification
> (t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
> = 1 / 400 kHz.
>
> Where is the error?
Hum ok you are right. I was a bad interpretation of the datasheet.
So now it is clearer. Thanks for that.
I will correct and improve my comments in the V8.
>
>> + * So, in order to cover both SCL high/low with Duty = 1,
>> + * CCR = 16 * SCL period * I2C CLK frequency
>
> I don't get that. Actually you need to use low + high, so
> CCR = parentrate / (25 * 400 kHz), right?
With your new inputs above, I think I could use a simpler implementation:
CCR = scl_high_period * parent_rate
with scl_high_period = 5 µs in standard mode to reach 100khz
and scl_high_period = 1 µs in fast mode to reach 400khz with 1/2 or
16/9 duty cycle.
So, I am wondering if I have to let the customer setting the duty
cycle in the DT for example with "st,duty=0" or "st,duty=1" property
(0 for 1/2 and 1 for 16/9).
Or perhaps the best option it to use a default value. What is your
feeling regarding this point ?
>
>> + *
>> + * Please note that the minimum allowed value is 0x04, except in FAST DUTY mode
>> + * where the minimum allowed value is 0x01
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> + [STM32F4_I2C_SPEED_STANDARD] = {
>> + .mul_ccr = 1,
>> + .min_ccr = 4,
>> + .duty = 0,
>> + .scl_period = 5,
>> + },
>> + [STM32F4_I2C_SPEED_FAST] = {
>> + .mul_ccr = 16,
>> + .min_ccr = 1,
>> + .duty = 1,
>> + .scl_period = 2,
>> + },
>> +};
>> +
>> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> + writel_relaxed(readl_relaxed(reg) | mask, reg);
>> +}
>> +
>> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> + writel_relaxed(readl_relaxed(reg) & ~mask, reg);
>> +}
>> +
>> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + u32 val;
>> +
>> + val = readl_relaxed(reg);
>> + writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
>> + writel_relaxed(val, reg);
>> +}
>> +
>> +static void stm32f4_i2c_disable_irq(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
>> +}
>> +
>> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 clk_rate, cr2, freq;
>> +
>> + /*
>> + * The minimum allowed frequency is 2 MHz, the maximum frequency is
>> + * limited by the maximum APB frequency 42 MHz
>> + */
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
>> + clk_rate = clk_get_rate(i2c_dev->clk);
>> + freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
>> + freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
>> + cr2 |= STM32F4_I2C_CR2_FREQ(freq);
>> + writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
>
> Last round I suggested error checking here instead of silent clamping.
Ok
>
>> +}
>> +
>> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 trise, freq, cr2;
>> +
>> + /*
>> + * These bits must be programmed with the maximum SCL rise time given in
>> + * the I2C bus specification, incremented by 1.
>> + *
>> + * In standard mode, the maximum allowed SCL rise time is 1000 ns.
>> + * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> + * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> + * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
>> + * So, for I2C standard mode TRISE = FREQ[5:0] + 1
>> + *
>> + * In fast mode, the maximum allowed SCL rise time is 300 ns.
>> + * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> + * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> + * programmed with 03h.(300 ns / 125 ns = 2 + 1)
>> + * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
>> + */
>> +
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> +
>> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
>> + trise = freq + 1;
>> + else
>> + trise = freq * 300 / 1000 + 1;
>
> if freq is big such that freq * 300 overflows does this result in a
> wrong result, or does the compiler optimize correctly?
For sure the compiler will never exceeds u32 max value
>
>> + writel_relaxed(STM32F4_I2C_TRISE_VALUE(trise),
>> + i2c_dev->base + STM32F4_I2C_TRISE);
>> +}
>> +
>> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
>> + u32 cr2, ccr, freq, val;
>> +
>> + ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
>> + ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
>> + STM32F4_I2C_CCR_CCR_MASK);
>> +
>> + /*
>> + * Please see the comments above regarding i2c_timings[] declaration
>> + * to understand the below calculation
>> + */
>> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
>> + val = freq * t->scl_period * t->mul_ccr;
>> + if (val < t->min_ccr)
>> + val = t->min_ccr;
>> + ccr |= STM32F4_I2C_CCR_CCR(val);
>> +
>> + if (t->duty)
>> + ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
>> +
>> + writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
>> +}
>> +
>> +static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 filter;
>> +
>> + /* Enable analog noise filter and disable digital noise filter */
>> + filter = readl_relaxed(i2c_dev->base + STM32F4_I2C_FLTR);
>> + filter &= ~(STM32F4_I2C_FLTR_ANOFF | STM32F4_I2C_FLTR_DNF_MASK);
>> + writel_relaxed(filter, i2c_dev->base + STM32F4_I2C_FLTR);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_hw_config() - Prepare I2C block
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> +
>> + /* Disable I2C */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> +
>> + stm32f4_i2c_set_periph_clk_freq(i2c_dev);
>> +
>> + stm32f4_i2c_set_rise_time(i2c_dev);
>> +
>> + stm32f4_i2c_set_speed_mode(i2c_dev);
>> +
>> + stm32f4_i2c_set_filter(i2c_dev);
>> +
>> + /* Enable I2C */
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
>
> This is the first write to STM32F4_I2C_CR1, right? So the state from the
> bootloader leaks here. This probably works most of the time, but if it
> makes problems later, that's hard to debug. Also, what if the bootloader
> already did some i2c transfers and kept the PE bit 1? I read in the
> manual that PE must be 0 for some things. So this only works most of the
> time.
The first thing I do before configuring I2C device is to clear PE bit
in order to disable I2C.
In that way, all previous config will be erased by the new one.
>
>> +}
>> +
>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + u32 status;
>> + int ret;
>> +
>> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> + status,
>> + !(status & STM32F4_I2C_SR2_BUSY),
>> + 10, 1000);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "bus not free\n");
>
> drop error message please or degrade to dev_debug
ok I will use a dev_dbg message
>
>> + ret = -EBUSY;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
>> + * @i2c_dev: Controller's private data
>> + * @byte: Data to write in the register
>> + */
>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
>> +{
>> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
>> + * @i2c_dev: Controller's private data
>> + *
>> + * This function fills the data register with I2C transfer buffer
>> + */
>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> +
>> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
>> + msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + u32 rbuf;
>> +
>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> + *msg->buf++ = rbuf & 0xff;
>> + msg->count--;
>> +}
>> +
>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> +
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + complete(&i2c_dev->complete);
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + if (msg->count) {
>> + stm32f4_i2c_write_msg(i2c_dev);
>> + if (!msg->count) {
>> + /* Disable buffer interrupts for RXNE/TXE events */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + }
>> + } else {
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + switch (msg->count) {
>> + case 1:
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 2:
>> + case 3:
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
>> + * in case of read
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 mask;
>> + int i;
>> +
>> + switch (msg->count) {
>> + case 2:
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + /* Generate STOP or repeated Start */
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> +
>> + /* Read two last data bytes */
>> + for (i = 2; i > 0; i--)
>> + stm32f4_i2c_read_msg(i2c_dev);
>> +
>> + /* Disable events and error interrupts */
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> + stm32f4_i2c_clr_bits(reg, mask);
>> +
>> + complete(&i2c_dev->complete);
>> + break;
>> + case 3:
>> + /* Enable ACK and read data */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + break;
>> + default:
>> + stm32f4_i2c_read_msg(i2c_dev);
>> + }
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
>> + * master receiver
>> + * @i2c_dev: Controller's private data
>> + */
>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
>> +{
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> +
>> + switch (msg->count) {
>> + case 0:
>> + stm32f4_i2c_terminate_xfer(i2c_dev);
>> + /* Clear ADDR flag */
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + case 1:
>> + /*
>> + * Single byte reception:
>> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> + break;
>> + case 2:
>> + /*
>> + * 2-byte reception:
>> + * Enable NACK and set POS
>> + */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> +
>> + default:
>> + /* N-byte reception: Enable ACK */
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> + break;
>> + }
>> +}
>
> This is still not really understandable.
I have already added some comments from datasheet to explain the
different cases.
I don't see how I could be more understandable as it is clearly the
hardware way of working...
>
>> +
>> +/**
>> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
>> + * @irq: interrupt number
>> + * @data: Controller's private data
>> + */
>> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = data;
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 status, possible_status, ien;
>> + int flag;
>> +
>> + ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + ien &= STM32F4_I2C_CR2_IRQ_MASK;
>> + possible_status = 0;
>
> This can already be done when declaring possible_status.
Ok.
>
>> +
>> + /* Check possible status combinations */
>> + if (ien & STM32F4_I2C_CR2_ITEVTEN) {
>> + possible_status = STM32F4_I2C_SR1_ITEVTEN_MASK;
>> + if (ien & STM32F4_I2C_CR2_ITBUFEN)
>> + possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
>> + }
>> +
>> + status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>> +
>> + if (!(status & possible_status)) {
>> + dev_dbg(i2c_dev->dev,
>> + "spurious evt irq (status=0x%08x, ien=0x%08x)\n",
>> + status, ien);
>> + return IRQ_NONE;
>> + }
>> +
>> + while (status & possible_status) {
>> + /* Use __fls() to check error bits first */
>> + flag = __fls(status & possible_status);
>> +
>> + switch (1 << flag) {
>> + case STM32F4_I2C_SR1_SB:
>> + stm32f4_i2c_write_byte(i2c_dev, msg->addr);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_ADDR:
>> + if (msg->addr & I2C_M_RD)
>> + stm32f4_i2c_handle_rx_addr(i2c_dev);
>> + else
>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
>> +
>> + /* Enable buffer interrupts for RXNE/TXE events */
>> + reg = i2c_dev->base + STM32F4_I2C_CR2;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
>> + possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
>> + break;
>> +
>> + case STM32F4_I2C_SR1_BTF:
>> + if (msg->addr & I2C_M_RD)
>> + stm32f4_i2c_handle_rx_btf(i2c_dev);
>> + else
>> + stm32f4_i2c_handle_write(i2c_dev);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_TXE:
>> + stm32f4_i2c_handle_write(i2c_dev);
>> + break;
>> +
>> + case STM32F4_I2C_SR1_RXNE:
>> + stm32f4_i2c_handle_read(i2c_dev);
>> + break;
>> +
>> + default:
>> + dev_err(i2c_dev->dev,
>> + "evt irq unhandled: status=0x%08x)\n",
>> + status);
>> + return IRQ_NONE;
>> + }
>> + status &= ~(1 << flag);
>> + }
>
> I wouldn't do this in a loop. Just do:
>
> if (status & STM32F4_I2C_SR1_SB) {
> ...
> }
>
> if (status & ...) {
>
> }
ok but I would prefer something like that:
flag = status & possible_status
if (flag & STM32F4_I2C_SR1_SB) {
...
}
if (flag & ...) {
}
>
> Then it's obvious by reading the code in which order they are handled
> without the need to check the definitions. Do you really need to jugle
> with possible_status?
I think I have to use possible_status as some events could occur
whereas the corresponding interrupt is disabled.
For example, for a 2 byte-reception, we don't have to take into accout
RXNE event so the corresponding interrupt is disabled.
If I don't check possible_status, I will call
stm32f4_i2c_handle_read() for nothing and generates unneeded registers
accesses.
>
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_isr_error() - Interrupt routine for I2C bus error
>> + * @irq: interrupt number
>> + * @data: Controller's private data
>> + */
>> +static irqreturn_t stm32f4_i2c_isr_error(int irq, void *data)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = data;
>> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
>> + void __iomem *reg;
>> + u32 status, possible_status, ien;
>> + int flag;
>> +
>> + ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
>> + ien &= STM32F4_I2C_CR2_IRQ_MASK;
>> + possible_status = 0;
>> +
>> + /* Check possible status combinations */
>> + if (ien & STM32F4_I2C_CR2_ITERREN)
>> + possible_status = STM32F4_I2C_SR1_ITERREN_MASK;
>> +
>> + status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
>> +
>> + if (!(status & possible_status)) {
>> + dev_dbg(i2c_dev->dev,
>> + "spurious err it (status=0x%08x, ien=0x%08x)\n",
>> + status, ien);
>> + return IRQ_NONE;
>> + }
>> +
>> + /* Use __fls() to check error bits first */
>> + flag = __fls(status & possible_status);
>> +
>> + switch (1 << flag) {
>> + case STM32F4_I2C_SR1_BERR:
>> + reg = i2c_dev->base + STM32F4_I2C_SR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
>> + msg->result = -EIO;
>> + break;
>> +
>> + case STM32F4_I2C_SR1_ARLO:
>> + reg = i2c_dev->base + STM32F4_I2C_SR1;
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
>> + msg->result = -EAGAIN;
>> + break;
>> +
>> + case STM32F4_I2C_SR1_AF:
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + msg->result = -EIO;
>> + break;
>> +
>> + default:
>> + dev_err(i2c_dev->dev,
>> + "err it unhandled: status=0x%08x)\n", status);
>> + return IRQ_NONE;
>> + }
>
> You only check a single irq flag here.
Yes only the first error could be reported to the i2c clients via
msg->result that's why I don't check all errors.
Moreover, as soon as an error occurs, the I2C device is reset.
>
>> +
>> + stm32f4_i2c_soft_reset(i2c_dev);
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> + complete(&i2c_dev->complete);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_xfer_msg() - Transfer a single I2C message
>> + * @i2c_dev: Controller's private data
>> + * @msg: I2C message to transfer
>> + * @is_first: first message of the sequence
>> + * @is_last: last message of the sequence
>> + */
>> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
>> + struct i2c_msg *msg, bool is_first,
>> + bool is_last)
>> +{
>> + struct stm32f4_i2c_msg *f4_msg = &i2c_dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + unsigned long timeout;
>> + u32 mask;
>> + int ret;
>> +
>> + f4_msg->addr = i2c_8bit_addr_from_msg(msg);
>> + f4_msg->buf = msg->buf;
>> + f4_msg->count = msg->len;
>> + f4_msg->result = 0;
>> + f4_msg->stop = is_last;
>> +
>> + reinit_completion(&i2c_dev->complete);
>> +
>> + /* Enable events and errors interrupts */
>> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
>> + stm32f4_i2c_set_bits(i2c_dev->base + STM32F4_I2C_CR2, mask);
>> +
>> + if (is_first) {
>> + ret = stm32f4_i2c_wait_free_bus(i2c_dev);
>> + if (ret)
>> + return ret;
>> +
>> + /* START generation */
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
>> + }
>> +
>> + timeout = wait_for_completion_timeout(&i2c_dev->complete,
>> + i2c_dev->adap.timeout);
>> + ret = f4_msg->result;
>> +
>> + /* Disable PEC position Ack */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_POS);
>
> This is the only place mentioning PEC. Should this be about POS instead?
Yes you are right. Thanks
>
>> +
>> + if (!timeout)
>> + ret = -ETIMEDOUT;
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * stm32f4_i2c_xfer() - Transfer combined I2C message
>> + * @i2c_adap: Adapter pointer to the controller
>> + * @msgs: Pointer to data to be written.
>> + * @num: Number of messages to be executed
>> + */
>> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
>> + int num)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
>> + int ret, i;
>> +
>> + ret = clk_enable(i2c_dev->clk);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "Failed to enable clock\n");
>> + return ret;
>> + }
>> +
>> + stm32f4_i2c_hw_config(i2c_dev);
>> +
>> + for (i = 0; i < num && !ret; i++)
>> + ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
>> + i == num - 1);
>> +
>> + clk_disable(i2c_dev->clk);
>> +
>> + return (ret < 0) ? ret : num;
>> +}
>> +
>> +static u32 stm32f4_i2c_func(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
>> +
>> +static struct i2c_algorithm stm32f4_i2c_algo = {
>> + .master_xfer = stm32f4_i2c_xfer,
>> + .functionality = stm32f4_i2c_func,
>> +};
>> +
>> +static int stm32f4_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct stm32f4_i2c_dev *i2c_dev;
>> + struct resource *res;
>> + u32 irq_event, irq_error, clk_rate;
>> + struct i2c_adapter *adap;
>> + struct reset_control *rst;
>> + int ret;
>> +
>> + i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
>> + if (!i2c_dev)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(i2c_dev->base))
>> + return PTR_ERR(i2c_dev->base);
>> +
>> + irq_event = irq_of_parse_and_map(np, 0);
>> + if (!irq_event) {
>> + dev_err(&pdev->dev, "IRQ event missing or invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + irq_error = irq_of_parse_and_map(np, 1);
>> + if (!irq_error) {
>> + dev_err(&pdev->dev, "IRQ error missing or invalid\n");
>> + return -EINVAL;
>> + }
>> +
>> + i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(i2c_dev->clk)) {
>> + dev_err(&pdev->dev, "Error: Missing controller clock\n");
>> + return PTR_ERR(i2c_dev->clk);
>> + }
>> + ret = clk_prepare(i2c_dev->clk);
>> + if (ret) {
>> + dev_err(i2c_dev->dev, "Failed to prepare clock\n");
>> + return ret;
>> + }
>> +
>> + rst = devm_reset_control_get(&pdev->dev, NULL);
>> + if (IS_ERR(rst)) {
>> + dev_err(&pdev->dev, "Error: Missing controller reset\n");
>> + ret = PTR_ERR(rst);
>> + goto clk_free;
>> + }
>> + reset_control_assert(rst);
>> + udelay(2);
>> + reset_control_deassert(rst);
>> +
>> + i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
>> + ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
>> + if (!ret && clk_rate >= 40000)
>> + i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
>> +
>> + i2c_dev->dev = &pdev->dev;
>> +
>> + ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
>> + pdev->name, i2c_dev);
>
> Starting here this irq might trigger. Can this happen? If so,
> stm32f4_i2c_isr_event is called without the adapter being registered.
> Probably not an issue as the controller was just reset.
No irq could be triggered as after resett, the I2C is not enable as PE bit = 0.
>
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq event %i\n",
>> + irq_event);
>> + goto clk_free;
>> + }
>> +
>> + ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
>> + pdev->name, i2c_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to request irq error %i\n",
>> + irq_error);
>> + goto clk_free;
>> + }
>> +
>> + adap = &i2c_dev->adap;
>> + i2c_set_adapdata(adap, i2c_dev);
>> + snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
>> + adap->owner = THIS_MODULE;
>> + adap->timeout = 2 * HZ;
>> + adap->retries = 0;
>> + adap->algo = &stm32f4_i2c_algo;
>> + adap->dev.parent = &pdev->dev;
>> + adap->dev.of_node = pdev->dev.of_node;
>> +
>> + init_completion(&i2c_dev->complete);
>> +
>> + ret = i2c_add_adapter(adap);
>> + if (ret)
>> + goto clk_free;
>> +
>> + platform_set_drvdata(pdev, i2c_dev);
>> +
>> + dev_info(i2c_dev->dev, "STM32F4 I2C driver registered\n");
>> +
>> + return 0;
>> +
>> +clk_free:
>> + clk_unprepare(i2c_dev->clk);
>> + return ret;
>> +}
>> +
>> +static int stm32f4_i2c_remove(struct platform_device *pdev)
>> +{
>> + struct stm32f4_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>> +
>> + i2c_del_adapter(&i2c_dev->adap);
>> +
>> + clk_unprepare(i2c_dev->clk);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id stm32f4_i2c_match[] = {
>> + { .compatible = "st,stm32f4-i2c", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
>> +
>> +static struct platform_driver stm32f4_i2c_driver = {
>> + .driver = {
>> + .name = "stm32f4-i2c",
>> + .of_match_table = stm32f4_i2c_match,
>> + },
>> + .probe = stm32f4_i2c_probe,
>> + .remove = stm32f4_i2c_remove,
>> +};
>> +
>> +module_platform_driver(stm32f4_i2c_driver);
>> +
>> +MODULE_AUTHOR("M'boumba Cedric Madianga <cedric.madianga@gmail.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32F4 I2C driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 1/2] devicetree: power: add bindings for GPIO-driven power switches
From: Lars-Peter Clausen @ 2016-12-23 11:40 UTC (permalink / raw)
To: Geert Uytterhoeven, Rob Herring
Cc: Bartosz Golaszewski, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Mark Rutland,
linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-devicetree, LKML, Kevin Hilman, Patrick Titiano,
Neil Armstrong, Linus Walleij, Alexandre Courbot, linux-gpio,
Sebastian Reichel, linux-pm, Mark Brown, Liam Girdwood
In-Reply-To: <CAMuHMdWfAwPE2N-_0WhW5+EEK3bqWoo9HpQZTgLzRRutCg++qg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 12/23/2016 10:07 AM, Geert Uytterhoeven wrote:
> BTW, I'm not an IIO expert, but from my limited knowledge, it looks like "O"
> support in IIO is limited to DACs?
Depends on what you categorize as DACs. There are also
potentiometer/rheostat drivers. They are kind of like DACs but the unit you
control are ohm, rather than current or voltage.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] iio: misc: add a generic regulator driver
From: Lars-Peter Clausen @ 2016-12-23 11:35 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Bartosz Golaszewski, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Rob Herring, Mark Rutland,
linux-iio-u79uwXL29TY76Z2rM5mHXA, linux-devicetree, LKML,
Kevin Hilman, Patrick Titiano, Neil Armstrong, Liam Girdwood,
Mark Brown
In-Reply-To: <CAMuHMdX3u4cb2Yq2+VV_eDUaw2EqiOjN2TorW+F5-39Mm=mXmQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 12/23/2016 11:00 AM, Geert Uytterhoeven wrote:
> Hi Lars,
>
> On Mon, Dec 12, 2016 at 6:15 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>> On 12/06/2016 12:12 PM, Bartosz Golaszewski wrote:
>>> We're already using libiio to read the measured data from the power
>>> monitor, that's why we'd like to use the iio framework for
>>> power-cycling the devices as well. My question is: would bridging the
>>> regulator framework be the right solution? Should we look for
>>> something else? Bridge the GPIO framework instead?
>>
>> I wouldn't necessaries create bridge, but instead just use the GPIO
>> framework directly.
>>
>> We now have the GPIO chardev interface which meant to be used to support
>> application specific logic that control the GPIOs, but where you don't want
>> to write a kernel driver.
>>
>> My idea was to add GPIOs and GPIO chips as high level object inside libiio
>> that can be accessed through the same context as the IIO devices. Similar to
>> the current IIO API you have a API for gpios that allows to enumerate the
>> GPIO devices and their pins as well as modify the pin state.
>
> That would mean libiio has access to all GPIOs, allowing a remote person
> to not only control through iiod the GPIOs for industrial control, but also the
> GPIOs not intended for export, right?
Well, it is a policy question. Who gets access to what. Right now it is all
or nothing, a privileged application gets access to all devices/GPIOs, a
unprivileged application gets access to nothing. Same for GPIOs as well as
IIO devices.
iiod at the moment does not have any access control at all, which in itself
is a problem. We need to add support for that at some point. I don't see an
issue with implementing a finer grained access scheme when we do so. E.g.
unprivileged applications only get access to certain pins.
> Having a separate GPIO switch driver avoids that, as DT (or some other means)
> can be used to specify and label the GPIOs for IIO use.
Sure, functionally this would be equivalent, but we have to ask whether this
is the right way to use the DT. Is access policy specification part of the
hardware description? In my opinion the answer is no. At the hardware
description level there is no operating system, there is no userspace or
kernelspace, there is are no access levels. Putting the distinction between
a switch/regulator that can be controlled from userspace or can only be
controlled from kernel space into the DT would be a layering violation. It
is analogous to why we don't have spidev DT bindings. This is an issue that
needs to be solved at a higher level. In my opinion this level is a
cooperation between kernel- and userspace. Kernelspace offering an interface
to export a device for userspace access and userspace making use of that
interface to request access to a device. In a similar way to how vfio is
structured.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 0/3] Static memory controllers for the Aspeed SoC
From: Cyrille Pitchen @ 2016-12-23 11:22 UTC (permalink / raw)
To: Cédric Le Goater, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Richard Weinberger, devicetree-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Mark Rutland, Joel Stanley
In-Reply-To: <1482339439-26402-1-git-send-email-clg-Bxea+6Xhats@public.gmane.org>
Le 21/12/2016 à 17:57, Cédric Le Goater a écrit :
> Hello,
>
> Here is a series introducing a new driver for the different memory
> controllers of the Aspeed AST2500 and AST2400 SoCs. Each SoC has at
> least a memory controller for the BMC firmware and another one for the
> host firmware. The register set are mostly compatible but there are
> some slight differences on the AST2400.
>
> The driver only supports SPI type flash.
>
> Tested on:
>
> * OpenPOWER Palmetto (AST2400) with
> FMC controller : n25q256a
> SPI controller : mx25l25635e and n25q512ax3
>
> * Evaluation board (AST2500) with
> FMC controller : w25q256
> SPI controller : w25q256
>
> * OpenPOWER Witherspoon (AST2500) with
> FMC controller : mx25l25635e * 2
> SPI controller : mx66l1g45g
>
> Changes since v4:
> - improved IO routines with Cyrille's suggestions
> - added dummy bytes hanling for fast read commands
> - removed the 'label' property from jedec,spi-nor. Needs more
> discussion.
>
> Changes since v3:
> - reworked IO routines to use io{read,write}32_rep
> - changed config option to SPI_ASPEED_SMC
> - fixed aspeed_smc_chip_setup_init() returned value
>
> Changes since v2:
> - splitted patch to distinguish AST2400 and AST2500 controllers
> - fixed controller names
> - introduced prepare/unprepare ops
> - introduced a aspeed_smc_setup_flash() routine
> - various cleanups
>
> Changes since v1:
> - added a set4b ops to handle difference in the controllers
> - simplified the IO routines
> - prepared for fast read using dummy cycles
>
> Work in progress:
> - read optimization using higher SPI clock frequencies
> - command mode to direct reads from AHB
> - DMA support
>
> Thanks,
>
> C.
>
> Cédric Le Goater (3):
> mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC
> mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC
> mtd: spi-nor: bindings for the Aspeed memory controllers
>
> .../devicetree/bindings/mtd/aspeed-smc.txt | 51 ++
> drivers/mtd/spi-nor/Kconfig | 10 +
> drivers/mtd/spi-nor/Makefile | 1 +
> drivers/mtd/spi-nor/aspeed-smc.c | 759 +++++++++++++++++++++
> 4 files changed, 821 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt
> create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c
>
Applied to git://github.com/spi-nor/linux.git
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] xilinx_dma: Add reset support
From: Philipp Zabel @ 2016-12-23 10:26 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Ramiro Oliveira, dmaengine-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
vinod.koul-ral2JQCrhuEAvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, michal.simek-gjFFaj9aHVfQT0dZR+AlfA,
soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA,
appana.durga.rao-gjFFaj9aHVfQT0dZR+AlfA,
anuragku-gjFFaj9aHVfQT0dZR+AlfA,
dan.j.williams-ral2JQCrhuEAvxtiuMwx3w,
CARLOS.PALMINHA-HKixBCOQz3hWk0Htik3J/w
In-Reply-To: <4539024.0pO4eXcfb0@avalon>
Am Donnerstag, den 15.12.2016, 15:56 +0200 schrieb Laurent Pinchart:
> Hi Ramiro,
>
> (CC'ing Philipp Zabel)
>
> On Thursday 15 Dec 2016 11:26:54 Ramiro Oliveira wrote:
> > On 12/14/2016 8:16 PM, Laurent Pinchart wrote:
> > > Hi Ramiro,
> > >
> > > Thank you for the patch.
> > >
> > > On Wednesday 14 Dec 2016 17:18:24 Ramiro Oliveira wrote:
> > >> Add a DT property to control an optional external reset line
> > >>
> > >> Signed-off-by: Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > >> ---
> > >>
> > >> drivers/dma/xilinx/xilinx_dma.c | 23 +++++++++++++++++++++++
> > >> 1 file changed, 23 insertions(+)
> > >>
> > >> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > >> b/drivers/dma/xilinx/xilinx_dma.c index 5c9f11b..b845224 100644
> > >> --- a/drivers/dma/xilinx/xilinx_dma.c
> > >> +++ b/drivers/dma/xilinx/xilinx_dma.c
> > >> @@ -46,6 +46,7 @@
> > >> #include <linux/slab.h>
> > >> #include <linux/clk.h>
> > >> #include <linux/io-64-nonatomic-lo-hi.h>
> > >> +#include <linux/reset.h>
> > >
> > > I had neatly sorted the header alphabetically until someone added clk.h
> > > and io-64-nonatomic-lo-hi.h :-( Could you please move reset.h just before
> > > slab.h ?
> >
> > Sure. Actually I was tempted to reorder it, but I decided not to do it. I'll
> > do it now
>
> Yeah, I'll sleep better tonight :-D
>
> > >> #include "../dmaengine.h"
> > >>
> > >> @@ -409,6 +410,7 @@ struct xilinx_dma_device {
> > >> struct clk *rxs_clk;
> > >> u32 nr_channels;
> > >> u32 chan_id;
> > >> + struct reset_control *rst;
> > >> };
> > >>
> > >> /* Macros */
> > >> @@ -2543,6 +2545,27 @@ static int xilinx_dma_probe(struct platform_device
> > >> *pdev) if (IS_ERR(xdev->regs))
> > >> return PTR_ERR(xdev->regs);
> > >>
> > >> + xdev->rst = devm_reset_control_get_optional(&pdev->dev, "reset");
> > >
> > > devm_reset_control_get_optional() is deprecated as explained in
> > > linux/reset.h, you should use devm_reset_control_get_optional_exclusive()
> > > or devm_reset_control_get_optional_shared() instead, as applicable.
> > >
> > > This being said, I'm wondering why the optional versions of those
> > > functions exist, as they do exactly the same as the non-optional versions.
> > > The API feels wrong, it should have been modelled like the GPIO API. Feel
> > > free to fix it if you want :-) But that's out of scope for this patch.
> >
> > I missed the comment stating that devm_reset_control_get_optional() was
> > deprecated.
> >
> > I could fix it. Your sugestion is modelling these functions like the GPIO
> > API?
>
> I think it would be better for driver if the _get_optional functions would
> return an ERR_PTR() for errors and NULL when reset control is not available,
> and then have the rest of the reset controller API accept NULL as a no-op.
> Your implementation would then be
>
> xdev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> "reset");
> if (IS_ERR(xdev->rst)) {
> err = PTR_ERR(xdev->rst);
> if (err != -EPROBE_DEFER)
> dev_err(xdev->dev, "error getting reset %d\n", err);
> return err;
> }
>
> err = reset_control_deassert(xdev->rst);
> if (err) {
> dev_err(xdev->dev, "failed to deassert reset: %d\n", err);
> return err;
> }
>
> That requires modifying the reset controller API, so it's a bit out-of-scope,
> but if you could give it a go, it would be great.
Seeing that the clk and gpiod APIs behave in the same way, I think it
would be good to align the reset API with the common behaviour.
regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 03/10] dt-bindings: perf: hisi: Add Devicetree bindings for Hisilicon SoC PMU
From: Anurup M @ 2016-12-23 10:23 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland, will.deacon, linux-kernel, devicetree,
linux-arm-kernel, anurup.m, zhangshaokun, tanxiaojun, xuwei5,
sanil.kumar, john.garry, gabriele.paoloni, shiju.jose,
wangkefeng.wang, linuxarm, shyju.pv
In-Reply-To: <20161219163702.c7wvqsaa4jcztthh@rob-hp-laptop>
On Monday 19 December 2016 10:07 PM, Rob Herring wrote:
> On Wed, Dec 07, 2016 at 11:55:59AM -0500, Anurup M wrote:
>> 1) Device tree bindings for Hisilicon SoC PMU.
>> 2) Add example for Hisilicon L3 cache and MN PMU.
>> 3) Add child nodes of L3C and MN in djtag bindings example.
>>
>> Signed-off-by: Anurup M <anurup.m@huawei.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> ---
>> .../devicetree/bindings/arm/hisilicon/djtag.txt | 25 ++++++
>> .../devicetree/bindings/arm/hisilicon/pmu.txt | 98 ++++++++++++++++++++++
>> 2 files changed, 123 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt b/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt
>> index 733498e..c885507 100644
>> --- a/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt
>> +++ b/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt
>> @@ -27,6 +27,31 @@ Example 1: Djtag for CPU die
>> scl-id = <0x02>;
>>
>> /* All connecting components will appear as child nodes */
>> +
>> + pmul3c0 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x02>;
>> + };
>> +
>> + pmul3c1 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x04>;
>> + };
>> +
>> + pmul3c2 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x01>;
>> + };
>> +
>> + pmul3c3 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x08>;
>> + };
>> +
>> + pmumn0 {
>> + compatible = "hisilicon,hisi-pmu-mn-v1";
>> + module-id = <0x0b>;
>> + };
>> };
>>
>> Example 2: Djtag for IO die
>> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt b/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
>> new file mode 100644
>> index 0000000..e2160ad
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
>> @@ -0,0 +1,98 @@
>> +Hisilicon SoC HiP05/06/07 ARMv8 PMU
>> +===================================
>> +
>> +The Hisilicon SoC chips like HiP05/06/07 etc. consist of various independent
>> +system device PMUs such as L3 cache (L3C) and Miscellaneous Nodes(MN). These
>> +PMU devices are independent and have hardware logic to gather statistics and
>> +performance information.
>> +
>> +HiSilicon SoC chip is encapsulated by multiple CPU and IO dies. The CPU die
>> +is called as Super CPU cluster (SCCL) which includes 16 cpu-cores. Every SCCL
>> +in HiP05/06/07 chips are further grouped as CPU clusters (CCL) which includes
>> +4 cpu-cores each.
>> +e.g. In the case of HiP05/06/07, each SCCL has 1 L3 cache and 1 MN PMU device.
>> +The L3 cache is further grouped as 4 L3 cache banks in a SCCL.
>> +
>> +The Hisilicon SoC PMU DT node bindings for uncore PMU devices are as below.
>> +For PMU devices like L3 cache. MN etc. which are accessed using the djtag,
>> +the parent node will be the djtag node of the corresponding CPU die (SCCL).
>> +
>> +L3 cache
>> +---------
>> +The L3 cache is dedicated for each SCCL. Each SCCL in HiP05/06/07 chips have 4
>> +L3 cache banks. Each L3 cache bank have separate DT nodes.
>> +
>> +Required properties:
>> +
>> + - compatible : This value should be as follows
>> + (a) "hisilicon,hisi-pmu-l3c-v1" for v1 hw in HiP05/06 chips
>> + (b) "hisilicon,hisi-pmu-l3c-v2" for v2 hw in HiP07 chip
> Use SoC specific compatible strings.
Ok.
>> +
>> + - module-id : This property is a combination of two values in the below order.
>> + a) Module ID: The module identifier for djtag.
>> + b) Instance or Bank ID: This will identify the L3 cache bank
>> + or instance.
> Needs a vendor prefix.
Ok. Shall change to "hisi-module-id". Also modify for all vendor
specific properties.
>> +
>> +Optional properties:
>> +
>> + - interrupt-parent : A phandle indicating which interrupt controller
>> + this PMU signals interrupts to.
>> +
>> + - interrupts : Interrupt lines used by this L3 cache bank.
> How many interrupts and what are they?
A single interrupt. Shall modify as "Interrupt line used by the L3
cache bank".
Thanks,
Anurup
>> +
>> + *The counter overflow IRQ is not supported in v1 hardware (HiP05/06).
>> +
>> +Miscellaneous Node
>> +------------------
>> +The MN is dedicated for each SCCL and hence there are separate DT nodes for MN
>> +for each SCCL.
> Similar comments here.
>
>> +
>> +Required properties:
>> +
>> + - compatible : This value should be as follows
>> + (a) "hisilicon,hisi-pmu-mn-v1" for v1 hw in HiP05/06 chips
>> + (b) "hisilicon,hisi-pmu-mn-v2" for v2 hw in HiP07 chip
>> +
>> + - module-id : Module ID to input for djtag.
>> +
>> +Optional properties:
>> +
>> + - interrupt-parent : A phandle indicating which interrupt controller
>> + this PMU signals interrupts to.
>> +
>> + - interrupts : Interrupt lines used by this PMU.
>> +
>> + *The counter overflow IRQ is not supported in v1 hardware (HiP05/06).
>> +
>> +Example:
>> +
>> + djtag0: djtag@80010000 {
>> + compatible = "hisilicon,hisi-djtag-v1";
>> + reg = <0x0 0x80010000 0x0 0x10000>;
>> + scl-id = <0x02>;
>> +
>> + pmul3c0 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x02>;
>> + };
>> +
>> + pmul3c1 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x04>;
>> + };
>> +
>> + pmul3c2 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x01>;
>> + };
>> +
>> + pmul3c3 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + module-id = <0x04 0x08>;
>> + };
>> +
>> + pmumn0 {
>> + compatible = "hisilicon,hisi-pmu-mn-v1";
>> + module-id = <0x0b>;
>> + };
>> + };
>> --
>> 2.1.4
>>
^ permalink raw reply
* Re: [PATCH v2 02/10] dt-bindings: hisi: Add Hisilicon HiP05/06/07 Djtag dts bindings
From: Anurup M @ 2016-12-23 10:17 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
anurup.m-hv44wF8Li93QT0dZR+AlfA,
zhangshaokun-C8/M+/jPZTeaMJb+Lgu22Q,
tanxiaojun-hv44wF8Li93QT0dZR+AlfA, xuwei5-C8/M+/jPZTeaMJb+Lgu22Q,
sanil.kumar-C8/M+/jPZTeaMJb+Lgu22Q,
john.garry-hv44wF8Li93QT0dZR+AlfA,
gabriele.paoloni-hv44wF8Li93QT0dZR+AlfA,
shiju.jose-hv44wF8Li93QT0dZR+AlfA,
wangkefeng.wang-hv44wF8Li93QT0dZR+AlfA,
linuxarm-hv44wF8Li93QT0dZR+AlfA, shyju.pv-hv44wF8Li93QT0dZR+AlfA
In-Reply-To: <20161219163126.w6ibkd6ayvblkwqt@rob-hp-laptop>
On Monday 19 December 2016 10:01 PM, Rob Herring wrote:
> On Wed, Dec 07, 2016 at 11:55:19AM -0500, Anurup M wrote:
>> From: Tan Xiaojun <tanxiaojun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>
>> Add Hisilicon HiP05/06/07 Djtag dts bindings for CPU and IO Die
>>
>> Signed-off-by: Tan Xiaojun <tanxiaojun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Anurup M <anurup.m-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>> .../devicetree/bindings/arm/hisilicon/djtag.txt | 41 ++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/djtag.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt b/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt
>> new file mode 100644
>> index 0000000..733498e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/hisilicon/djtag.txt
>> @@ -0,0 +1,41 @@
>> +The Hisilicon Djtag is an independent component which connects with some other
>> +components in the SoC by Debug Bus. The djtag is available in CPU and IO dies
>> +in the chip. The djtag controls access to connecting modules of CPU and IO
>> +dies.
>> +The various connecting components in CPU die (like L3 cache, L3 cache PMU etc.)
>> +are accessed by djtag during real time debugging. In IO die there are connecting
>> +components like RSA. These components appear as devices atatched to djtag bus.
>> +
>> +Hisilicon HiP05/06 djtag for CPU and HiP05 IO die
>> +Required properties:
>> + - compatible : "hisilicon,hisi-djtag-v1"
> These need SoC specific compatible strings. They probably should
> also include cpu or io in the compatible string. I would expect there
> are things like events, triggers, or component connections for debug
> that are SoC specifc.
Ok. Shall include SoC prefix in compatible string. e.g.
"hisilicon,hip06-djtag-v1".
As the djtag driver handling is same for CPU and IO, I think I don't
need to include
them in the compatible string. Please share your comment.
Thanks,
Anurup
>> + - reg : Register address and size
>> + - scl-id : The Super Cluster ID for CPU or IO die
>> +
>> +Hisilicon HiP06 djtag for IO die and HiP07 djtag for CPU and IO die
>> +Required properties:
>> + - compatible : "hisilicon,hisi-djtag-v2"
> Same here.
>
>> + - reg : Register address and size
>> + - scl-id : The Super Cluster ID for CPU or IO die
>> +
>> +Example 1: Djtag for CPU die
>> +
>> + /* for Hisilicon HiP05 djtag for CPU Die */
>> + djtag0: djtag@80010000 {
>> + compatible = "hisilicon,hisi-djtag-v1";
>> + reg = <0x0 0x80010000 0x0 0x10000>;
>> + scl-id = <0x02>;
>> +
>> + /* All connecting components will appear as child nodes */
>> + };
>> +
>> +Example 2: Djtag for IO die
>> +
>> + /* for Hisilicon HiP05 djtag for IO Die */
>> + djtag1: djtag@d0000000 {
>> + compatible = "hisilicon,hisi-djtag-v1";
>> + reg = <0x0 0xd0000000 0x0 0x10000>;
>> + scl-id = <0x01>;
>> +
>> + /* All connecting components will appear as child nodes */
>> + };
>> --
>> 2.1.4
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] iio: misc: add a generic regulator driver
From: Geert Uytterhoeven @ 2016-12-23 10:00 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Bartosz Golaszewski, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Rob Herring, Mark Rutland, linux-iio,
linux-devicetree, LKML, Kevin Hilman, Patrick Titiano,
Neil Armstrong, Liam Girdwood, Mark Brown
In-Reply-To: <c670d597-46b6-235f-545f-7136a3abff7f@metafoo.de>
Hi Lars,
On Mon, Dec 12, 2016 at 6:15 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 12/06/2016 12:12 PM, Bartosz Golaszewski wrote:
>> We're already using libiio to read the measured data from the power
>> monitor, that's why we'd like to use the iio framework for
>> power-cycling the devices as well. My question is: would bridging the
>> regulator framework be the right solution? Should we look for
>> something else? Bridge the GPIO framework instead?
>
> I wouldn't necessaries create bridge, but instead just use the GPIO
> framework directly.
>
> We now have the GPIO chardev interface which meant to be used to support
> application specific logic that control the GPIOs, but where you don't want
> to write a kernel driver.
>
> My idea was to add GPIOs and GPIO chips as high level object inside libiio
> that can be accessed through the same context as the IIO devices. Similar to
> the current IIO API you have a API for gpios that allows to enumerate the
> GPIO devices and their pins as well as modify the pin state.
That would mean libiio has access to all GPIOs, allowing a remote person
to not only control through iiod the GPIOs for industrial control, but also the
GPIOs not intended for export, right?
Having a separate GPIO switch driver avoids that, as DT (or some other means)
can be used to specify and label the GPIOs for IIO use.
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: [RFT PATCH] ARM64: dts: meson-gxbb: Add reserved memory zone and usable memory range
From: Heinrich Schuchardt @ 2016-12-23 9:42 UTC (permalink / raw)
To: Heinrich Schuchardt, Neil Armstrong, khilman
Cc: carlo, linux-amlogic, linux-arm-kernel, linux-kernel, devicetree
In-Reply-To: <2ac01237-a7f8-5b3b-0f97-dd49ce6623fb@gmx.de>
On 12/22/2016 11:02 AM, Heinrich Schuchardt wrote:
> On 12/14/2016 10:52 AM, Neil Armstrong wrote:
>
>> Hi Heinrich,
>>
>> Thanks for testing and for the report,
>> we are still struggling into finding what are these zones and how to label them correctly.
>>
>> We need to identify the zones on all boards, the patch I provided works on a non-odroid-c2 and gxm and gxl boards.
>>
>> Neil
>>
> Hello Neil,
>
> the configuration below works for me on the Hardkernel Odroid C2.
>
> ramoops is needed for CONFIG_PSTORE_RAM.
> Debian Stretch has CONFIG_PSTORE_RAM=m. Same is true for Fedora.
> I have chosen the address arbitrarily. To accommodate 512 MB boards we
> would have to put it below 0x20000000.
> The size parameters are the same as in hisilicon/hi6220-hikey.dts and
> qcom-apq8064-asus-nexus7-flo.dts.
>
> linux,cma is used for contiguous memory assignment. I have taken the
> align parameter from arm-src-kernel-2016-08-18-26e194264c.tar.gz
> provided by Amlogic at
> http://openlinux.amlogic.com:8000/download/ARM/kernel/ .
> See Documentation/DMA-API.txt for the usage of align.
> They use the same value 0x400000 for all GXBB boards.
> So we want to put this zone into meson-gxbb.dtsi.
>
> secmon is used by drivers/firmware/meson/meson_sm.c.
> Amlogic uses the same address range for all 64bit boards.
>
> memory@0 {
> device_type = "memory";
> linux,usable-memory = <0x0 0x1000000 0x0 0x7f000000>;
> };
>
> reserved-memory {
> #address-cells = <0x2>;
> #size-cells = <0x2>;
> ranges;
>
> ramoops@0x23f00000 {
> compatible = "ramoops";
> reg = <0x0 0x23f00000 0x0 0x100000>;
> record-size = <0x20000>;
> console-size = <0x20000>;
> ftrace-size = <0x20000>;
> };
>
> secmon: secmon {
> compatible = "amlogic, aml_secmon_memory";
> reg = <0x0 0x10000000 0x0 0x200000>;
> no-map;
> };
>
> linux,cma {
> compatible = "shared-dma-pool";
> reusable;
> size = <0x0 0xbc00000>;
> alignment = <0x0 0x400000>;
> linux,cma-default;
> };
> };
>
> Best regards
>
> Heinrich Schuchardt
>
Hello Neil,
it really makes a difference if we write
memory@0 {
device_type = "memory";
linux,usable-memory = <0x0 0x1000000 0x0 0x7f000000>;
};
or
memory@0 {
device_type = "memory";
reg = <0x0 0x1000000 0x0 0x7f000000>;
};
The second version leads to failure of the Odroid C2.
When I looked at /sys/firmware/fdt I saw this difference:
--- fails
+++ works
memory@0 {
- device_type = "memory";
reg = <0x0 0x0 0x0 0x78000000>;
+ device_type = "memory";
+ linux,usable-memory = <0x0 0x1000000 0x0 0x7f000000>;
};
I found the following sentence in the NXP forum:
In case you want to overwrite the memory usage passed from u-boot, you
can use "linux,usable-memory".
https://community.nxp.com/thread/382284
Best regards
Heinrich Schuchardt
^ permalink raw reply
* Re: [PATCH 1/2] devicetree: power: add bindings for GPIO-driven power switches
From: Geert Uytterhoeven @ 2016-12-23 9:07 UTC (permalink / raw)
To: Rob Herring
Cc: Bartosz Golaszewski, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Mark Rutland,
linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-devicetree, LKML, Kevin Hilman, Patrick Titiano,
Neil Armstrong, Linus Walleij, Alexandre Courbot, linux-gpio,
Sebastian Reichel, linux-pm, Mark Brown,
Liam Girdwood <lgirdwood@
In-Reply-To: <CAL_JsqLwS51MhOJQb-Lmo=osHiYKcAcCJaxKJFdSvZBpafz7Gw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Dec 15, 2016 at 4:05 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Dec 14, 2016 at 10:58 AM, Bartosz Golaszewski
> <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
>> 2016-12-13 20:27 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>> On Sun, Dec 11, 2016 at 11:21:44PM +0100, Bartosz Golaszewski wrote:
>>>> Some boards are equipped with simple, GPIO-driven power load switches.
>>>> An example of such ICs is the TI tps229* series.
>>>
>>> How is this different than a GPIO regulator? The input and output
>>> voltages just happen to be the same. I could be convinced this is
>>> different enough to have a different compatible, but it somewhat seems
>>> you want to use this for IIO, so you are creating a different binding
>>> for that usecase.
>>
>> It's more of a fixed regulator I suppose. Do you mean adding a new
>> compatible to the fixed-regulator binding (e.g. "gpio-power-switch" or
>> "simple-power-switch") and then providing an iio driver for toggling
>> the switch?
>
> Yes, at least the first part. I view the switch as just a more
> specific subtype of a fixed-regulator. Whether an IIO driver is a
> separate discussion which is happening.
The switch could also be an opto-isolator (for which I could use gpio-leds,
too, although also without libiio control ;-) or a relay.
While I agree a switch is a degenerate regulator, modelling it as a regulator
feels a bit weird to me. Switches could be extended to e.g. double throw
relays, or H-bridges using 4 GPIOs.
BTW, I'm not an IIO expert, but from my limited knowledge, it looks like "O"
support in IIO is limited to DACs?
P.S. My motiviation is using libiio to control my board farm, which has a bank
of opto-isolators in addition to BayLibre's ACME.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v7 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2016-12-23 9:00 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
alexandre.torgue-qxv4g6HH51o,
linus.walleij-QSEj5FYQhm4dnm+yROfE0A, patrice.chotard-qxv4g6HH51o,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1482413704-17531-3-git-send-email-cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hello,
On Thu, Dec 22, 2016 at 02:35:01PM +0100, M'boumba Cedric Madianga wrote:
> This patch adds support for the STM32F4 I2C controller.
>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-stm32f4.c | 896 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 907 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-stm32f4.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 0cdc844..2719208 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -886,6 +886,16 @@ config I2C_ST
> This driver can also be built as module. If so, the module
> will be called i2c-st.
>
> +config I2C_STM32F4
> + tristate "STMicroelectronics STM32F4 I2C support"
> + depends on ARCH_STM32 || COMPILE_TEST
> + help
> + Enable this option to add support for STM32 I2C controller embedded
> + in STM32F4 SoCs.
> +
> + This driver can also be built as module. If so, the module
> + will be called i2c-stm32f4.
> +
> config I2C_STU300
> tristate "ST Microelectronics DDC I2C interface"
> depends on MACH_U300
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1c1bac8..a2c6ff5 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_I2C_SH_MOBILE) += i2c-sh_mobile.o
> obj-$(CONFIG_I2C_SIMTEC) += i2c-simtec.o
> obj-$(CONFIG_I2C_SIRF) += i2c-sirf.o
> obj-$(CONFIG_I2C_ST) += i2c-st.o
> +obj-$(CONFIG_I2C_STM32F4) += i2c-stm32f4.o
> obj-$(CONFIG_I2C_STU300) += i2c-stu300.o
> obj-$(CONFIG_I2C_SUN6I_P2WI) += i2c-sun6i-p2wi.o
> obj-$(CONFIG_I2C_TEGRA) += i2c-tegra.o
> diff --git a/drivers/i2c/busses/i2c-stm32f4.c b/drivers/i2c/busses/i2c-stm32f4.c
> new file mode 100644
> index 0000000..ca11dee
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-stm32f4.c
> @@ -0,0 +1,896 @@
> +/*
> + * Driver for STMicroelectronics STM32 I2C controller
> + *
> + * This I2C controller is described in the STM32F429/439 Soc reference manual.
> + * Please see below a link to the documentation:
> + * http://www.st.com/resource/en/reference_manual/DM00031020.pdf
> + *
> + * Copyright (C) M'boumba Cedric Madianga 2016
> + * Author: M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This driver is based on i2c-st.c
> + *
> + * License terms: GNU General Public License (GPL), version 2
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +/* STM32F4 I2C offset registers */
> +#define STM32F4_I2C_CR1 0x00
> +#define STM32F4_I2C_CR2 0x04
> +#define STM32F4_I2C_DR 0x10
> +#define STM32F4_I2C_SR1 0x14
> +#define STM32F4_I2C_SR2 0x18
> +#define STM32F4_I2C_CCR 0x1C
> +#define STM32F4_I2C_TRISE 0x20
> +#define STM32F4_I2C_FLTR 0x24
> +
> +/* STM32F4 I2C control 1*/
> +#define STM32F4_I2C_CR1_SWRST BIT(15)
> +#define STM32F4_I2C_CR1_POS BIT(11)
> +#define STM32F4_I2C_CR1_ACK BIT(10)
> +#define STM32F4_I2C_CR1_STOP BIT(9)
> +#define STM32F4_I2C_CR1_START BIT(8)
> +#define STM32F4_I2C_CR1_PE BIT(0)
> +
> +/* STM32F4 I2C control 2 */
> +#define STM32F4_I2C_CR2_FREQ_MASK GENMASK(5, 0)
> +#define STM32F4_I2C_CR2_FREQ(n) (((n) & STM32F4_I2C_CR2_FREQ_MASK))
((n) & STM32F4_I2C_CR2_FREQ_MASK)
should be enough.
> +#define STM32F4_I2C_CR2_ITBUFEN BIT(10)
> +#define STM32F4_I2C_CR2_ITEVTEN BIT(9)
> +#define STM32F4_I2C_CR2_ITERREN BIT(8)
> +#define STM32F4_I2C_CR2_IRQ_MASK (STM32F4_I2C_CR2_ITBUFEN | \
> + STM32F4_I2C_CR2_ITEVTEN | \
> + STM32F4_I2C_CR2_ITERREN)
> +
> +/* STM32F4 I2C Status 1 */
> +#define STM32F4_I2C_SR1_AF BIT(10)
> +#define STM32F4_I2C_SR1_ARLO BIT(9)
> +#define STM32F4_I2C_SR1_BERR BIT(8)
> +#define STM32F4_I2C_SR1_TXE BIT(7)
> +#define STM32F4_I2C_SR1_RXNE BIT(6)
> +#define STM32F4_I2C_SR1_BTF BIT(2)
> +#define STM32F4_I2C_SR1_ADDR BIT(1)
> +#define STM32F4_I2C_SR1_SB BIT(0)
> +#define STM32F4_I2C_SR1_ITEVTEN_MASK (STM32F4_I2C_SR1_BTF | \
> + STM32F4_I2C_SR1_ADDR | \
> + STM32F4_I2C_SR1_SB)
> +#define STM32F4_I2C_SR1_ITBUFEN_MASK (STM32F4_I2C_SR1_TXE | \
> + STM32F4_I2C_SR1_RXNE)
> +#define STM32F4_I2C_SR1_ITERREN_MASK (STM32F4_I2C_SR1_AF | \
> + STM32F4_I2C_SR1_ARLO | \
> + STM32F4_I2C_SR1_BERR)
> +
> +/* STM32F4 I2C Status 2 */
> +#define STM32F4_I2C_SR2_BUSY BIT(1)
> +
> +/* STM32F4 I2C Control Clock */
> +#define STM32F4_I2C_CCR_CCR_MASK GENMASK(11, 0)
> +#define STM32F4_I2C_CCR_CCR(n) (((n) & STM32F4_I2C_CCR_CCR_MASK))
ditto
> +#define STM32F4_I2C_CCR_FS BIT(15)
> +#define STM32F4_I2C_CCR_DUTY BIT(14)
> +
> +/* STM32F4 I2C Trise */
> +#define STM32F4_I2C_TRISE_VALUE_MASK GENMASK(5, 0)
> +#define STM32F4_I2C_TRISE_VALUE(n) (((n) & STM32F4_I2C_TRISE_VALUE_MASK))
> +
> +/* STM32F4 I2C Filter */
> +#define STM32F4_I2C_FLTR_DNF_MASK GENMASK(3, 0)
> +#define STM32F4_I2C_FLTR_DNF(n) (((n) & STM32F4_I2C_FLTR_DNF_MASK))
> +#define STM32F4_I2C_FLTR_ANOFF BIT(4)
> +
> +#define STM32F4_I2C_MIN_FREQ 2U
> +#define STM32F4_I2C_MAX_FREQ 42U
> +#define HZ_TO_MHZ 1000000
> +
> +enum stm32f4_i2c_speed {
> + STM32F4_I2C_SPEED_STANDARD, /* 100 kHz */
> + STM32F4_I2C_SPEED_FAST, /* 400 kHz */
> + STM32F4_I2C_SPEED_END,
> +};
> +
> +/**
> + * struct stm32f4_i2c_timings - per-Mode tuning parameters
> + * @duty: Fast mode duty cycle
> + * @scl_period: SCL low/high period in microsecond
> + * @mul_ccr: Value to be multiplied to CCR to reach 100Khz/400Khz SCL frequency
> + * @min_ccr: Minimum clock ctrl reg value to reach 100Khz/400Khz SCL frequency
s/Khz/ kHz/
> + */
> +struct stm32f4_i2c_timings {
> + u32 duty;
> + u32 scl_period;
> + u32 mul_ccr;
> + u32 min_ccr;
> +};
> +
> +/**
> + * struct stm32f4_i2c_msg - client specific data
> + * @addr: 8-bit slave addr, including r/w bit
> + * @count: number of bytes to be transferred
> + * @buf: data buffer
> + * @result: result of the transfer
> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> + */
> +struct stm32f4_i2c_msg {
> + u8 addr;
> + u32 count;
> + u8 *buf;
> + int result;
> + bool stop;
You bought the argument about alignment of = in stm32f4_i2c_driver. The
same logic applies here.
> +};
> +
> +/**
> + * struct stm32f4_i2c_dev - private data of the controller
> + * @adap: I2C adapter for this controller
> + * @dev: device for this controller
> + * @base: virtual memory area
> + * @complete: completion of I2C message
> + * @clk: hw i2c clock
> + * speed: I2C clock frequency of the controller. Standard or Fast only supported
> + * @msg: I2C transfer information
> + */
> +struct stm32f4_i2c_dev {
> + struct i2c_adapter adap;
> + struct device *dev;
> + void __iomem *base;
> + struct completion complete;
> + struct clk *clk;
> + int speed;
> + struct stm32f4_i2c_msg msg;
> +};
ditto
> +
> +/*
> + * In standard mode:
> + * SCL high period = SCL low period = CCR * I2C CLK period
> + * So, CCR = SCL period * I2C CLK frequency
is "SCL period" the same as "SCL low period"? I2C CLK frequency is the
input clk? If so, it's confusing that it has I2C in its name. The
reference manual calls it PCLK1 (parent clock?).
> + * In fast mode:
> + * DUTY = 0: Fast mode tlow/thigh = 2
> + * DUTY = 1: Fast mode tlow/thigh = 16/9
> + * If Duty = 0; SCL high period = 1 * CCR * I2C CLK period
> + * SCL low period = 2 * CCR * I2C CLK period
> + * If Duty = 1; SCL high period = 9 * CCR * I2C CLK period
> + * SCL low period = 16 * CCR * I2C CLK period
I'd drop the first two lines about the proportions.
> + *
> + * Note that Duty has to bet set to reach 400khz in Fast mode
s/khz/ kHz/
I don't understand why DUTY is required to reach 400 kHz. Given a parent
freq of 30 MHz, with CCR = 25 and DUTY = 0 we have:
t_high = 25 * 33.333 ns = 833.333 ns
t_low = 2 * 25 * 33.333 ns = 1666.667 ns
then t_high and t_low satisfy the i2c bus specification
(t_low > 1300 ns, t_high > 600 ns) and we have t_low + t_high = 2500 ns
= 1 / 400 kHz.
Where is the error?
> + * So, in order to cover both SCL high/low with Duty = 1,
> + * CCR = 16 * SCL period * I2C CLK frequency
I don't get that. Actually you need to use low + high, so
CCR = parentrate / (25 * 400 kHz), right?
> + *
> + * Please note that the minimum allowed value is 0x04, except in FAST DUTY mode
> + * where the minimum allowed value is 0x01
> + */
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> + [STM32F4_I2C_SPEED_STANDARD] = {
> + .mul_ccr = 1,
> + .min_ccr = 4,
> + .duty = 0,
> + .scl_period = 5,
> + },
> + [STM32F4_I2C_SPEED_FAST] = {
> + .mul_ccr = 16,
> + .min_ccr = 1,
> + .duty = 1,
> + .scl_period = 2,
> + },
> +};
> +
> +static inline void stm32f4_i2c_set_bits(void __iomem *reg, u32 mask)
> +{
> + writel_relaxed(readl_relaxed(reg) | mask, reg);
> +}
> +
> +static inline void stm32f4_i2c_clr_bits(void __iomem *reg, u32 mask)
> +{
> + writel_relaxed(readl_relaxed(reg) & ~mask, reg);
> +}
> +
> +static void stm32f4_i2c_soft_reset(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> + u32 val;
> +
> + val = readl_relaxed(reg);
> + writel_relaxed(val | STM32F4_I2C_CR1_SWRST, reg);
> + writel_relaxed(val, reg);
> +}
> +
> +static void stm32f4_i2c_disable_irq(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_IRQ_MASK);
> +}
> +
> +static void stm32f4_i2c_set_periph_clk_freq(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 clk_rate, cr2, freq;
> +
> + /*
> + * The minimum allowed frequency is 2 MHz, the maximum frequency is
> + * limited by the maximum APB frequency 42 MHz
> + */
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + cr2 &= ~STM32F4_I2C_CR2_FREQ_MASK;
> + clk_rate = clk_get_rate(i2c_dev->clk);
> + freq = DIV_ROUND_UP(clk_rate, HZ_TO_MHZ);
> + freq = clamp(freq, STM32F4_I2C_MIN_FREQ, STM32F4_I2C_MAX_FREQ);
> + cr2 |= STM32F4_I2C_CR2_FREQ(freq);
> + writel_relaxed(cr2, i2c_dev->base + STM32F4_I2C_CR2);
Last round I suggested error checking here instead of silent clamping.
> +}
> +
> +static void stm32f4_i2c_set_rise_time(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 trise, freq, cr2;
> +
> + /*
> + * These bits must be programmed with the maximum SCL rise time given in
> + * the I2C bus specification, incremented by 1.
> + *
> + * In standard mode, the maximum allowed SCL rise time is 1000 ns.
> + * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
> + * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
> + * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
> + * So, for I2C standard mode TRISE = FREQ[5:0] + 1
> + *
> + * In fast mode, the maximum allowed SCL rise time is 300 ns.
> + * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
> + * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
> + * programmed with 03h.(300 ns / 125 ns = 2 + 1)
> + * So, for I2C fast mode TRISE = FREQ[5:0] * 300 / 1000 + 1
> + */
> +
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> +
> + if (i2c_dev->speed == STM32F4_I2C_SPEED_STANDARD)
> + trise = freq + 1;
> + else
> + trise = freq * 300 / 1000 + 1;
if freq is big such that freq * 300 overflows does this result in a
wrong result, or does the compiler optimize correctly?
> + writel_relaxed(STM32F4_I2C_TRISE_VALUE(trise),
> + i2c_dev->base + STM32F4_I2C_TRISE);
> +}
> +
> +static void stm32f4_i2c_set_speed_mode(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_timings *t = &i2c_timings[i2c_dev->speed];
> + u32 cr2, ccr, freq, val;
> +
> + ccr = readl_relaxed(i2c_dev->base + STM32F4_I2C_CCR);
> + ccr &= ~(STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY |
> + STM32F4_I2C_CCR_CCR_MASK);
> +
> + /*
> + * Please see the comments above regarding i2c_timings[] declaration
> + * to understand the below calculation
> + */
> + cr2 = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + freq = cr2 & STM32F4_I2C_CR2_FREQ_MASK;
> + val = freq * t->scl_period * t->mul_ccr;
> + if (val < t->min_ccr)
> + val = t->min_ccr;
> + ccr |= STM32F4_I2C_CCR_CCR(val);
> +
> + if (t->duty)
> + ccr |= STM32F4_I2C_CCR_FS | STM32F4_I2C_CCR_DUTY;
> +
> + writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
> +}
> +
> +static void stm32f4_i2c_set_filter(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 filter;
> +
> + /* Enable analog noise filter and disable digital noise filter */
> + filter = readl_relaxed(i2c_dev->base + STM32F4_I2C_FLTR);
> + filter &= ~(STM32F4_I2C_FLTR_ANOFF | STM32F4_I2C_FLTR_DNF_MASK);
> + writel_relaxed(filter, i2c_dev->base + STM32F4_I2C_FLTR);
> +}
> +
> +/**
> + * stm32f4_i2c_hw_config() - Prepare I2C block
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> +
> + /* Disable I2C */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> +
> + stm32f4_i2c_set_periph_clk_freq(i2c_dev);
> +
> + stm32f4_i2c_set_rise_time(i2c_dev);
> +
> + stm32f4_i2c_set_speed_mode(i2c_dev);
> +
> + stm32f4_i2c_set_filter(i2c_dev);
> +
> + /* Enable I2C */
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
This is the first write to STM32F4_I2C_CR1, right? So the state from the
bootloader leaks here. This probably works most of the time, but if it
makes problems later, that's hard to debug. Also, what if the bootloader
already did some i2c transfers and kept the PE bit 1? I read in the
manual that PE must be 0 for some things. So this only works most of the
time.
> +}
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 status;
> + int ret;
> +
> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> + status,
> + !(status & STM32F4_I2C_SR2_BUSY),
> + 10, 1000);
> + if (ret) {
> + dev_err(i2c_dev->dev, "bus not free\n");
drop error message please or degrade to dev_debug
> + ret = -EBUSY;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
> + * @i2c_dev: Controller's private data
> + * @byte: Data to write in the register
> + */
> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
> +{
> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
> +}
> +
> +/**
> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
> + * @i2c_dev: Controller's private data
> + *
> + * This function fills the data register with I2C transfer buffer
> + */
> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> +
> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
> + msg->count--;
> +}
> +
> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + u32 rbuf;
> +
> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> + *msg->buf++ = rbuf & 0xff;
> + msg->count--;
> +}
> +
> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + stm32f4_i2c_disable_irq(i2c_dev);
> +
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> + complete(&i2c_dev->complete);
> +}
> +
> +/**
> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + if (msg->count) {
> + stm32f4_i2c_write_msg(i2c_dev);
> + if (!msg->count) {
> + /* Disable buffer interrupts for RXNE/TXE events */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + }
> + } else {
> + stm32f4_i2c_terminate_xfer(i2c_dev);
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + switch (msg->count) {
> + case 1:
> + stm32f4_i2c_disable_irq(i2c_dev);
> + stm32f4_i2c_read_msg(i2c_dev);
> + complete(&i2c_dev->complete);
> + break;
> + case 2:
> + case 3:
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + break;
> + default:
> + stm32f4_i2c_read_msg(i2c_dev);
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> + * in case of read
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> + u32 mask;
> + int i;
> +
> + switch (msg->count) {
> + case 2:
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + /* Generate STOP or repeated Start */
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> +
> + /* Read two last data bytes */
> + for (i = 2; i > 0; i--)
> + stm32f4_i2c_read_msg(i2c_dev);
> +
> + /* Disable events and error interrupts */
> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> + stm32f4_i2c_clr_bits(reg, mask);
> +
> + complete(&i2c_dev->complete);
> + break;
> + case 3:
> + /* Enable ACK and read data */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + stm32f4_i2c_read_msg(i2c_dev);
> + break;
> + default:
> + stm32f4_i2c_read_msg(i2c_dev);
> + }
> +}
> +
> +/**
> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> + * master receiver
> + * @i2c_dev: Controller's private data
> + */
> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> +
> + switch (msg->count) {
> + case 0:
> + stm32f4_i2c_terminate_xfer(i2c_dev);
> + /* Clear ADDR flag */
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> + case 1:
> + /*
> + * Single byte reception:
> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + if (msg->stop)
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + else
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> + break;
> + case 2:
> + /*
> + * 2-byte reception:
> + * Enable NACK and set POS
> + */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> +
> + default:
> + /* N-byte reception: Enable ACK */
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> + break;
> + }
> +}
This is still not really understandable.
> +
> +/**
> + * stm32f4_i2c_isr_event() - Interrupt routine for I2C bus event
> + * @irq: interrupt number
> + * @data: Controller's private data
> + */
> +static irqreturn_t stm32f4_i2c_isr_event(int irq, void *data)
> +{
> + struct stm32f4_i2c_dev *i2c_dev = data;
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> + u32 status, possible_status, ien;
> + int flag;
> +
> + ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + ien &= STM32F4_I2C_CR2_IRQ_MASK;
> + possible_status = 0;
This can already be done when declaring possible_status.
> +
> + /* Check possible status combinations */
> + if (ien & STM32F4_I2C_CR2_ITEVTEN) {
> + possible_status = STM32F4_I2C_SR1_ITEVTEN_MASK;
> + if (ien & STM32F4_I2C_CR2_ITBUFEN)
> + possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
> + }
> +
> + status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
> +
> + if (!(status & possible_status)) {
> + dev_dbg(i2c_dev->dev,
> + "spurious evt irq (status=0x%08x, ien=0x%08x)\n",
> + status, ien);
> + return IRQ_NONE;
> + }
> +
> + while (status & possible_status) {
> + /* Use __fls() to check error bits first */
> + flag = __fls(status & possible_status);
> +
> + switch (1 << flag) {
> + case STM32F4_I2C_SR1_SB:
> + stm32f4_i2c_write_byte(i2c_dev, msg->addr);
> + break;
> +
> + case STM32F4_I2C_SR1_ADDR:
> + if (msg->addr & I2C_M_RD)
> + stm32f4_i2c_handle_rx_addr(i2c_dev);
> + else
> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> +
> + /* Enable buffer interrupts for RXNE/TXE events */
> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> + possible_status |= STM32F4_I2C_SR1_ITBUFEN_MASK;
> + break;
> +
> + case STM32F4_I2C_SR1_BTF:
> + if (msg->addr & I2C_M_RD)
> + stm32f4_i2c_handle_rx_btf(i2c_dev);
> + else
> + stm32f4_i2c_handle_write(i2c_dev);
> + break;
> +
> + case STM32F4_I2C_SR1_TXE:
> + stm32f4_i2c_handle_write(i2c_dev);
> + break;
> +
> + case STM32F4_I2C_SR1_RXNE:
> + stm32f4_i2c_handle_read(i2c_dev);
> + break;
> +
> + default:
> + dev_err(i2c_dev->dev,
> + "evt irq unhandled: status=0x%08x)\n",
> + status);
> + return IRQ_NONE;
> + }
> + status &= ~(1 << flag);
> + }
I wouldn't do this in a loop. Just do:
if (status & STM32F4_I2C_SR1_SB) {
...
}
if (status & ...) {
}
Then it's obvious by reading the code in which order they are handled
without the need to check the definitions. Do you really need to jugle
with possible_status?
> +
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * stm32f4_i2c_isr_error() - Interrupt routine for I2C bus error
> + * @irq: interrupt number
> + * @data: Controller's private data
> + */
> +static irqreturn_t stm32f4_i2c_isr_error(int irq, void *data)
> +{
> + struct stm32f4_i2c_dev *i2c_dev = data;
> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> + void __iomem *reg;
> + u32 status, possible_status, ien;
> + int flag;
> +
> + ien = readl_relaxed(i2c_dev->base + STM32F4_I2C_CR2);
> + ien &= STM32F4_I2C_CR2_IRQ_MASK;
> + possible_status = 0;
> +
> + /* Check possible status combinations */
> + if (ien & STM32F4_I2C_CR2_ITERREN)
> + possible_status = STM32F4_I2C_SR1_ITERREN_MASK;
> +
> + status = readl_relaxed(i2c_dev->base + STM32F4_I2C_SR1);
> +
> + if (!(status & possible_status)) {
> + dev_dbg(i2c_dev->dev,
> + "spurious err it (status=0x%08x, ien=0x%08x)\n",
> + status, ien);
> + return IRQ_NONE;
> + }
> +
> + /* Use __fls() to check error bits first */
> + flag = __fls(status & possible_status);
> +
> + switch (1 << flag) {
> + case STM32F4_I2C_SR1_BERR:
> + reg = i2c_dev->base + STM32F4_I2C_SR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_BERR);
> + msg->result = -EIO;
> + break;
> +
> + case STM32F4_I2C_SR1_ARLO:
> + reg = i2c_dev->base + STM32F4_I2C_SR1;
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_SR1_ARLO);
> + msg->result = -EAGAIN;
> + break;
> +
> + case STM32F4_I2C_SR1_AF:
> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> + msg->result = -EIO;
> + break;
> +
> + default:
> + dev_err(i2c_dev->dev,
> + "err it unhandled: status=0x%08x)\n", status);
> + return IRQ_NONE;
> + }
You only check a single irq flag here.
> +
> + stm32f4_i2c_soft_reset(i2c_dev);
> + stm32f4_i2c_disable_irq(i2c_dev);
> + complete(&i2c_dev->complete);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * stm32f4_i2c_xfer_msg() - Transfer a single I2C message
> + * @i2c_dev: Controller's private data
> + * @msg: I2C message to transfer
> + * @is_first: first message of the sequence
> + * @is_last: last message of the sequence
> + */
> +static int stm32f4_i2c_xfer_msg(struct stm32f4_i2c_dev *i2c_dev,
> + struct i2c_msg *msg, bool is_first,
> + bool is_last)
> +{
> + struct stm32f4_i2c_msg *f4_msg = &i2c_dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> + unsigned long timeout;
> + u32 mask;
> + int ret;
> +
> + f4_msg->addr = i2c_8bit_addr_from_msg(msg);
> + f4_msg->buf = msg->buf;
> + f4_msg->count = msg->len;
> + f4_msg->result = 0;
> + f4_msg->stop = is_last;
> +
> + reinit_completion(&i2c_dev->complete);
> +
> + /* Enable events and errors interrupts */
> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> + stm32f4_i2c_set_bits(i2c_dev->base + STM32F4_I2C_CR2, mask);
> +
> + if (is_first) {
> + ret = stm32f4_i2c_wait_free_bus(i2c_dev);
> + if (ret)
> + return ret;
> +
> + /* START generation */
> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> + }
> +
> + timeout = wait_for_completion_timeout(&i2c_dev->complete,
> + i2c_dev->adap.timeout);
> + ret = f4_msg->result;
> +
> + /* Disable PEC position Ack */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_POS);
This is the only place mentioning PEC. Should this be about POS instead?
> +
> + if (!timeout)
> + ret = -ETIMEDOUT;
> +
> + return ret;
> +}
> +
> +/**
> + * stm32f4_i2c_xfer() - Transfer combined I2C message
> + * @i2c_adap: Adapter pointer to the controller
> + * @msgs: Pointer to data to be written.
> + * @num: Number of messages to be executed
> + */
> +static int stm32f4_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[],
> + int num)
> +{
> + struct stm32f4_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap);
> + int ret, i;
> +
> + ret = clk_enable(i2c_dev->clk);
> + if (ret) {
> + dev_err(i2c_dev->dev, "Failed to enable clock\n");
> + return ret;
> + }
> +
> + stm32f4_i2c_hw_config(i2c_dev);
> +
> + for (i = 0; i < num && !ret; i++)
> + ret = stm32f4_i2c_xfer_msg(i2c_dev, &msgs[i], i == 0,
> + i == num - 1);
> +
> + clk_disable(i2c_dev->clk);
> +
> + return (ret < 0) ? ret : num;
> +}
> +
> +static u32 stm32f4_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm stm32f4_i2c_algo = {
> + .master_xfer = stm32f4_i2c_xfer,
> + .functionality = stm32f4_i2c_func,
> +};
> +
> +static int stm32f4_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct stm32f4_i2c_dev *i2c_dev;
> + struct resource *res;
> + u32 irq_event, irq_error, clk_rate;
> + struct i2c_adapter *adap;
> + struct reset_control *rst;
> + int ret;
> +
> + i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
> + if (!i2c_dev)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + i2c_dev->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(i2c_dev->base))
> + return PTR_ERR(i2c_dev->base);
> +
> + irq_event = irq_of_parse_and_map(np, 0);
> + if (!irq_event) {
> + dev_err(&pdev->dev, "IRQ event missing or invalid\n");
> + return -EINVAL;
> + }
> +
> + irq_error = irq_of_parse_and_map(np, 1);
> + if (!irq_error) {
> + dev_err(&pdev->dev, "IRQ error missing or invalid\n");
> + return -EINVAL;
> + }
> +
> + i2c_dev->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c_dev->clk)) {
> + dev_err(&pdev->dev, "Error: Missing controller clock\n");
> + return PTR_ERR(i2c_dev->clk);
> + }
> + ret = clk_prepare(i2c_dev->clk);
> + if (ret) {
> + dev_err(i2c_dev->dev, "Failed to prepare clock\n");
> + return ret;
> + }
> +
> + rst = devm_reset_control_get(&pdev->dev, NULL);
> + if (IS_ERR(rst)) {
> + dev_err(&pdev->dev, "Error: Missing controller reset\n");
> + ret = PTR_ERR(rst);
> + goto clk_free;
> + }
> + reset_control_assert(rst);
> + udelay(2);
> + reset_control_deassert(rst);
> +
> + i2c_dev->speed = STM32F4_I2C_SPEED_STANDARD;
> + ret = of_property_read_u32(np, "clock-frequency", &clk_rate);
> + if (!ret && clk_rate >= 40000)
> + i2c_dev->speed = STM32F4_I2C_SPEED_FAST;
> +
> + i2c_dev->dev = &pdev->dev;
> +
> + ret = devm_request_irq(&pdev->dev, irq_event, stm32f4_i2c_isr_event, 0,
> + pdev->name, i2c_dev);
Starting here this irq might trigger. Can this happen? If so,
stm32f4_i2c_isr_event is called without the adapter being registered.
Probably not an issue as the controller was just reset.
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq event %i\n",
> + irq_event);
> + goto clk_free;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, irq_error, stm32f4_i2c_isr_error, 0,
> + pdev->name, i2c_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request irq error %i\n",
> + irq_error);
> + goto clk_free;
> + }
> +
> + adap = &i2c_dev->adap;
> + i2c_set_adapdata(adap, i2c_dev);
> + snprintf(adap->name, sizeof(adap->name), "STM32 I2C(%pa)", &res->start);
> + adap->owner = THIS_MODULE;
> + adap->timeout = 2 * HZ;
> + adap->retries = 0;
> + adap->algo = &stm32f4_i2c_algo;
> + adap->dev.parent = &pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;
> +
> + init_completion(&i2c_dev->complete);
> +
> + ret = i2c_add_adapter(adap);
> + if (ret)
> + goto clk_free;
> +
> + platform_set_drvdata(pdev, i2c_dev);
> +
> + dev_info(i2c_dev->dev, "STM32F4 I2C driver registered\n");
> +
> + return 0;
> +
> +clk_free:
> + clk_unprepare(i2c_dev->clk);
> + return ret;
> +}
> +
> +static int stm32f4_i2c_remove(struct platform_device *pdev)
> +{
> + struct stm32f4_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c_dev->adap);
> +
> + clk_unprepare(i2c_dev->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id stm32f4_i2c_match[] = {
> + { .compatible = "st,stm32f4-i2c", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, stm32f4_i2c_match);
> +
> +static struct platform_driver stm32f4_i2c_driver = {
> + .driver = {
> + .name = "stm32f4-i2c",
> + .of_match_table = stm32f4_i2c_match,
> + },
> + .probe = stm32f4_i2c_probe,
> + .remove = stm32f4_i2c_remove,
> +};
> +
> +module_platform_driver(stm32f4_i2c_driver);
> +
> +MODULE_AUTHOR("M'boumba Cedric Madianga <cedric.madianga-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32F4 I2C driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced
From: Ming Lei @ 2016-12-23 7:24 UTC (permalink / raw)
To: zhichang.yuan
Cc: Catalin Marinas, Will Deacon, Rob Herring, Bjorn Helgaas,
Mark Rutland, Olof Johansson, Arnd Bergmann, linux-arm-kernel,
Lorenzo Pieralisi, Linux Kernel Mailing List, linuxarm,
devicetree@vger.kernel.org, linux-pci@vger.kernel.org,
linux-serial, Corey Minyard, Benjamin Herrenschmidt, Liviu Dudau,
Zou Rongrong, john.garry, gabriele.
In-Reply-To: <585C815B.4030000@hisilicon.com>
[-- Attachment #1: Type: text/plain, Size: 13571 bytes --]
On Fri, Dec 23, 2016 at 9:43 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> Hi，Ming,
>
>
> On 2016/12/22 16:15, Ming Lei wrote:
>> Hi Guys,
>>
>> On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
>> <yuanzhichang@hisilicon.com> wrote:
>>> For arm64, there is no I/O space as other architectural platforms, such as
>>> X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
>>> such as Hip06, when accessing some legacy ISA devices connected to LPC, those
>>> known port addresses are used to control the corresponding target devices, for
>>> example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
>>> normal MMIO mode in using.
>>>
>>> To drive these devices, this patch introduces a method named indirect-IO.
>>> In this method the in/out pair in arch/arm64/include/asm/io.h will be
>>> redefined. When upper layer drivers call in/out with those known legacy port
>>> addresses to access the peripherals, the hooking functions corrresponding to
>>> those target peripherals will be called. Through this way, those upper layer
>>> drivers which depend on in/out can run on Hip06 without any changes.
>>>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>>> ---
>>> arch/arm64/Kconfig | 6 +++
>>> arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>>> arch/arm64/include/asm/io.h | 29 +++++++++++++
>>> arch/arm64/kernel/Makefile | 1 +
>>> arch/arm64/kernel/extio.c | 27 ++++++++++++
>>> 5 files changed, 157 insertions(+)
>>
>> When I applied these three patches against current linus tree and
>> enable CONFIG_HISILICON_LPC, the following build failure[1] is
>> triggered when running 'make modules'.
>>
>
> Thanks for your report!
>
> This patch has compilation issue on some architectures, sorry for the inconvenience caused by this!
> The ongoing v6 will solve these issues.
> I will trace this failure and provide a fix if you can not wait for the next version.
>
> Could you send me your .config in private? I don't want to bother all the hacker in the mail-list.
>
Sure, please see the config in attachment.
>
> Thanks,
> Zhichang
>
>>
>> Thanks,
>> Ming
>>
>> [1] 'make modules' failure log
>>
>> Building modules, stage 2.
>> MODPOST 2260 modules
>> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
>> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
>> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
>> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
>> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
>> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
>> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
>> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
>> .....
>> ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
>> ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
>> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
>> make[1]: *** [__modpost] Error 1
>> Makefile:1196: recipe for target 'modules' failed
>> make: *** [modules] Error 2
>>
>>
>>> create mode 100644 arch/arm64/include/asm/extio.h
>>> create mode 100644 arch/arm64/kernel/extio.c
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index 969ef88..b44070b 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>>> config ARCH_MMAP_RND_COMPAT_BITS_MAX
>>> default 16
>>>
>>> +config ARM64_INDIRECT_PIO
>>> + bool "access peripherals with legacy I/O port"
>>> + help
>>> + Support special accessors for ISA I/O devices. This is needed for
>>> + SoCs that do not support standard read/write for the ISA range.
>>> +
>>> config NO_IOPORT_MAP
>>> def_bool y if !PCI
>>>
>>> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
>>> new file mode 100644
>>> index 0000000..6ae0787
>>> --- /dev/null
>>> +++ b/arch/arm64/include/asm/extio.h
>>> @@ -0,0 +1,94 @@
>>> +/*
>>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef __LINUX_EXTIO_H
>>> +#define __LINUX_EXTIO_H
>>> +
>>> +struct extio_ops {
>>> + unsigned long start;/* inclusive, sys io addr */
>>> + unsigned long end;/* inclusive, sys io addr */
>>> +
>>> + u64 (*pfin)(void *devobj, unsigned long ptaddr, size_t dlen);
>>> + void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
>>> + size_t dlen);
>>> + u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
>>> + size_t dlen, unsigned int count);
>>> + void (*pfouts)(void *devobj, unsigned long ptaddr,
>>> + const void *outbuf, size_t dlen,
>>> + unsigned int count);
>>> + void *devpara;
>>> +};
>>> +
>>> +extern struct extio_ops *arm64_extio_ops;
>>> +
>>> +#define DECLARE_EXTIO(bw, type) \
>>> +extern type in##bw(unsigned long addr); \
>>> +extern void out##bw(type value, unsigned long addr); \
>>> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
>>> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
>>> +
>>> +#define BUILD_EXTIO(bw, type) \
>>> +type in##bw(unsigned long addr) \
>>> +{ \
>>> + if (!arm64_extio_ops || arm64_extio_ops->start > addr || \
>>> + arm64_extio_ops->end < addr) \
>>> + return read##bw(PCI_IOBASE + addr); \
>>> + return arm64_extio_ops->pfin ? \
>>> + arm64_extio_ops->pfin(arm64_extio_ops->devpara, \
>>> + addr, sizeof(type)) : -1; \
>>> +} \
>>> + \
>>> +void out##bw(type value, unsigned long addr) \
>>> +{ \
>>> + if (!arm64_extio_ops || arm64_extio_ops->start > addr || \
>>> + arm64_extio_ops->end < addr) \
>>> + write##bw(value, PCI_IOBASE + addr); \
>>> + else \
>>> + if (arm64_extio_ops->pfout) \
>>> + arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>>> + addr, value, sizeof(type)); \
>>> +} \
>>> + \
>>> +void ins##bw(unsigned long addr, void *buffer, unsigned int count) \
>>> +{ \
>>> + if (!arm64_extio_ops || arm64_extio_ops->start > addr || \
>>> + arm64_extio_ops->end < addr) \
>>> + reads##bw(PCI_IOBASE + addr, buffer, count); \
>>> + else \
>>> + if (arm64_extio_ops->pfins) \
>>> + arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
>>> + addr, buffer, sizeof(type), count); \
>>> +} \
>>> + \
>>> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count) \
>>> +{ \
>>> + if (!arm64_extio_ops || arm64_extio_ops->start > addr || \
>>> + arm64_extio_ops->end < addr) \
>>> + writes##bw(PCI_IOBASE + addr, buffer, count); \
>>> + else \
>>> + if (arm64_extio_ops->pfouts) \
>>> + arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
>>> + addr, buffer, sizeof(type), count); \
>>> +}
>>> +
>>> +static inline void arm64_set_extops(struct extio_ops *ops)
>>> +{
>>> + if (ops)
>>> + WRITE_ONCE(arm64_extio_ops, ops);
>>> +}
>>> +
>>> +#endif /* __LINUX_EXTIO_H*/
>>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>>> index 0bba427..136735d 100644
>>> --- a/arch/arm64/include/asm/io.h
>>> +++ b/arch/arm64/include/asm/io.h
>>> @@ -31,6 +31,7 @@
>>> #include <asm/early_ioremap.h>
>>> #include <asm/alternative.h>
>>> #include <asm/cpufeature.h>
>>> +#include <asm/extio.h>
>>>
>>> #include <xen/xen.h>
>>>
>>> @@ -149,6 +150,34 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>>> #define IO_SPACE_LIMIT (PCI_IO_SIZE - 1)
>>> #define PCI_IOBASE ((void __iomem *)PCI_IO_START)
>>>
>>> +
>>> +/*
>>> + * redefine the in(s)b/out(s)b for indirect-IO.
>>> + */
>>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>>> +#define inb inb
>>> +#define outb outb
>>> +#define insb insb
>>> +#define outsb outsb
>>> +/* external declaration */
>>> +DECLARE_EXTIO(b, u8)
>>> +
>>> +#define inw inw
>>> +#define outw outw
>>> +#define insw insw
>>> +#define outsw outsw
>>> +
>>> +DECLARE_EXTIO(w, u16)
>>> +
>>> +#define inl inl
>>> +#define outl outl
>>> +#define insl insl
>>> +#define outsl outsl
>>> +
>>> +DECLARE_EXTIO(l, u32)
>>> +#endif
>>> +
>>> +
>>> /*
>>> * String version of I/O memory access operations.
>>> */
>>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>>> index 7d66bba..60e0482 100644
>>> --- a/arch/arm64/kernel/Makefile
>>> +++ b/arch/arm64/kernel/Makefile
>>> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT) += sys32.o kuser32.o signal32.o \
>>> sys_compat.o entry32.o
>>> arm64-obj-$(CONFIG_FUNCTION_TRACER) += ftrace.o entry-ftrace.o
>>> arm64-obj-$(CONFIG_MODULES) += arm64ksyms.o module.o
>>> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO) += extio.o
>>> arm64-obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o
>>> arm64-obj-$(CONFIG_PERF_EVENTS) += perf_regs.o perf_callchain.o
>>> arm64-obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o
>>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>>> new file mode 100644
>>> index 0000000..647b3fa
>>> --- /dev/null
>>> +++ b/arch/arm64/kernel/extio.c
>>> @@ -0,0 +1,27 @@
>>> +/*
>>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/io.h>
>>> +
>>> +struct extio_ops *arm64_extio_ops;
>>> +
>>> +
>>> +BUILD_EXTIO(b, u8)
>>> +
>>> +BUILD_EXTIO(w, u16)
>>> +
>>> +BUILD_EXTIO(l, u32)
>>> --
>>> 1.9.1
>>>
>>
>>
>>
>
Thanks,
Ming Lei
[-- Attachment #2: config.tar.gz --]
[-- Type: application/x-gzip, Size: 37193 bytes --]
^ permalink raw reply
* [PATCH v5 6/6] arm64: arch_timer: acpi: add hisi timer errata data
From: Ding Tianhong @ 2016-12-23 7:04 UTC (permalink / raw)
To: catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
marc.zyngier-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
oss-fOR+EgIDQEHk1uMJSBkQmQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
shawnguo-DgEjT+Ai2ygdnm+yROfE0A, stuart.yoder-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linuxarm-hv44wF8Li93QT0dZR+AlfA
Cc: Hanjun Guo, Ding Tianhong
In-Reply-To: <1482476669-15596-1-git-send-email-dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
From: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Add hisi timer specific erratum fixes.
v3: add hisilicon erratum 161601 for ACPI mode.
v4: update some data structures.
Signed-off-by: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
drivers/clocksource/arm_arch_timer.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 212bfa5..7b15d2a 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1089,10 +1089,28 @@ struct gtdt_arch_timer_fixup {
void *context;
};
+#ifdef CONFIG_HISILICON_ERRATUM_161601
+static void __init erratum_workaround_enable(void *context)
+{
+ u64 erratum = (u64) context;
+
+ if (erratum & HISILICON_161601) {
+ timer_unstable_counter_workaround = &arch_timer_hisi_161601;
+ static_branch_enable(&arch_timer_read_ool_enabled);
+ pr_info("Enabling workaround for HISILICON ERRATUM 161601\n");
+ }
+}
+#endif
+
/* note: this needs to be updated according to the doc of OEM ID
* and TABLE ID for different board.
*/
struct gtdt_arch_timer_fixup arch_timer_quirks[] __initdata = {
+#ifdef CONFIG_HISILICON_ERRATUM_161601
+ {"HISI", "hip05", 0, &erratum_workaround_enable, (void *) HISILICON_161601},
+ {"HISI", "hip06", 0, &erratum_workaround_enable, (void *) HISILICON_161601},
+ {"HISI", "hip07", 0, &erratum_workaround_enable, (void *) HISILICON_161601},
+#endif
};
void __init arch_timer_acpi_quirks_handler(char *oem_id,
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v5 5/6] arm64: arch_timer: apci: Introduce a generic aquirk framework for erratum
From: Ding Tianhong @ 2016-12-23 7:04 UTC (permalink / raw)
To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland, oss,
devicetree, shawnguo, stuart.yoder, linux-arm-kernel, linuxarm
Cc: Hanjun Guo, Ding Tianhong
In-Reply-To: <1482476669-15596-1-git-send-email-dingtianhong@huawei.com>
From: Hanjun Guo <hanjun.guo@linaro.org>
Introduce a general quirk framework for each timer erratum in ACPI,
which use the oem information in GTDT table for platform specific erratums.
The struct gtdt_arch_timer_fixup is introduced to record the oem
information to match the quirk and handle the erratum.
v3: Introduce a generic aquick framework for erratum in ACPI mode.
v4: rename the quirk handler parameter to make it more generic, and
avoid break loop when handling the quirk becasue it need to
support multi quirks handler.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
drivers/clocksource/arm_arch_timer.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 9a82496..212bfa5 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1079,6 +1079,39 @@ static int __init arch_timer_mem_init(struct device_node *np)
arch_timer_mem_init);
#ifdef CONFIG_ACPI
+struct gtdt_arch_timer_fixup {
+ char oem_id[ACPI_OEM_ID_SIZE];
+ char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
+ u32 oem_revision;
+
+ /* quirk handler for arch timer erratum */
+ void (*handler)(void *context);
+ void *context;
+};
+
+/* note: this needs to be updated according to the doc of OEM ID
+ * and TABLE ID for different board.
+ */
+struct gtdt_arch_timer_fixup arch_timer_quirks[] __initdata = {
+};
+
+void __init arch_timer_acpi_quirks_handler(char *oem_id,
+ char *oem_table_id,
+ u32 oem_revision)
+{
+ struct gtdt_arch_timer_fixup *quirks = arch_timer_quirks;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(arch_timer_quirks); i++, quirks++) {
+ if (!memcmp(quirks->oem_id, oem_id, ACPI_OEM_ID_SIZE) &&
+ !memcmp(quirks->oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+ quirks->oem_revision == oem_revision) {
+ if (quirks->handler && quirks->context)
+ quirks->handler(quirks->context);
+ }
+ }
+}
+
static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
{
int trigger, polarity;
@@ -1105,6 +1138,9 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
return -EINVAL;
}
+ arch_timer_acpi_quirks_handler(table->oem_id, table->oem_table_id,
+ table->oem_revision);
+
gtdt = container_of(table, struct acpi_table_gtdt, header);
arch_timers_present |= ARCH_CP15_TIMER;
--
1.9.0
^ permalink raw reply related
* [PATCH v5 4/6] arm64: arch timer: Add timer erratum property for Hip05-d02 and Hip06-d03
From: Ding Tianhong @ 2016-12-23 7:04 UTC (permalink / raw)
To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland, oss,
devicetree, shawnguo, stuart.yoder, linux-arm-kernel, linuxarm
Cc: Ding Tianhong
In-Reply-To: <1482476669-15596-1-git-send-email-dingtianhong@huawei.com>
Enable workaround for hisilicon erratum 161601 on Hip05-d02 and Hip06-d03 board.
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
arch/arm64/boot/dts/hisilicon/hip05.dtsi | 1 +
arch/arm64/boot/dts/hisilicon/hip06.dtsi | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/hisilicon/hip05.dtsi b/arch/arm64/boot/dts/hisilicon/hip05.dtsi
index 4b472a3..a8e9969 100644
--- a/arch/arm64/boot/dts/hisilicon/hip05.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hip05.dtsi
@@ -281,6 +281,7 @@
<GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
<GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
<GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
+ hisilicon,erratum-161601;
};
pmu {
diff --git a/arch/arm64/boot/dts/hisilicon/hip06.dtsi b/arch/arm64/boot/dts/hisilicon/hip06.dtsi
index a049b64..344e0f0 100644
--- a/arch/arm64/boot/dts/hisilicon/hip06.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hip06.dtsi
@@ -260,6 +260,7 @@
<GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>,
<GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>,
<GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
+ hisilicon,erratum-161601;
};
pmu {
--
1.9.0
^ permalink raw reply related
* [PATCH v5 3/6] arm64: arch_timer: Work around Erratum Hisilicon-161601
From: Ding Tianhong @ 2016-12-23 7:04 UTC (permalink / raw)
To: catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
marc.zyngier-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
oss-fOR+EgIDQEHk1uMJSBkQmQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
shawnguo-DgEjT+Ai2ygdnm+yROfE0A, stuart.yoder-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linuxarm-hv44wF8Li93QT0dZR+AlfA
Cc: Ding Tianhong
In-Reply-To: <1482476669-15596-1-git-send-email-dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Erratum Hisilicon-161601 says that the ARM generic timer counter "has the
potential to contain an erroneous value when the timer value changes".
Accesses to TVAL (both read and write) are also affected due to the implicit counter
read. Accesses to CVAL are not affected.
The workaround is to reread the system count registers until the value of the second
read is larger than the first one by less than 32, the system counter can be guaranteed
not to return wrong value twice by back-to-back read and the error value is always larger
than the correct one by 32. Writes to TVAL are replaced with an equivalent write to CVAL.
The workaround is enabled if the hisilicon,erratum-161601 property is found in
the timer node in the device tree. This can be overridden with the
clocksource.arm_arch_timer.hisilicon-161601 boot parameter, which allows KVM
users to enable the workaround until a mechanism is implemented to
automatically communicate this information.
Fix some description for fsl erratum a008585.
v2: Significant rework based on feedback, including seperate the fsl erratum a008585
to another patch, update the erratum name and remove unwanted code.
v3: Significant rework based on feedback, including fix some alignment problem, make the
#define __hisi_161601_read_reg to be private to the .c file instead of being globally
visible, add more accurate annotation and modify a bit of logical format to enable
arch_timer_read_ool_enabled, remove the kernel commandline parameter
clocksource.arm_arch_timer.hisilicon-161601.
v5: Theoretically the erratum should not occur more than twice in succession when reading
the system counter, but it is possible that some interrupts may lead to more than twice
read errors, triggering the warning, so setting the number of retries to 50 which is far
beyond the number of iterations the loop has been observed to take.
Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
Documentation/arm64/silicon-errata.txt | 1 +
arch/arm64/include/asm/arch_timer.h | 2 +-
drivers/clocksource/Kconfig | 9 +++++
drivers/clocksource/arm_arch_timer.c | 70 +++++++++++++++++++++++++++++++---
4 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 405da11..1c1a95f 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -63,3 +63,4 @@ stable kernels.
| Cavium | ThunderX SMMUv2 | #27704 | N/A |
| | | | |
| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
+| Hisilicon | Hip0{5,6,7} | #161601 | HISILICON_ERRATUM_161601|
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index f882c7c..ebf4cde 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -29,7 +29,7 @@
#include <clocksource/arm_arch_timer.h>
-#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585)
+#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
extern struct static_key_false arch_timer_read_ool_enabled;
#define needs_unstable_timer_counter_workaround() \
static_branch_unlikely(&arch_timer_read_ool_enabled)
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4866f7a..162d820 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -335,6 +335,15 @@ config FSL_ERRATUM_A008585
value"). The workaround will only be active if the
fsl,erratum-a008585 property is found in the timer node.
+config HISILICON_ERRATUM_161601
+ bool "Workaround for Hisilicon Erratum 161601"
+ default y
+ depends on ARM_ARCH_TIMER && ARM64
+ help
+ This option enables a workaround for Hisilicon Erratum
+ 161601. The workaround will be active if the hisilicon,erratum-161601
+ property is found in the timer node.
+
config ARM_GLOBAL_TIMER
bool "Support for the ARM global timer" if COMPILE_TEST
select CLKSRC_OF if OF
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index e7406ad..9a82496 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -95,15 +95,18 @@ static int __init early_evtstrm_cfg(char *buf)
* Architected system timer support.
*/
-#ifdef CONFIG_FSL_ERRATUM_A008585
+#if CONFIG_FSL_ERRATUM_A008585 || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
struct arch_timer_erratum_workaround *timer_unstable_counter_workaround = NULL;
EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
#define FSL_A008585 0x0001
+#define HISILICON_161601 0x0002
DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
+#endif
+#ifdef CONFIG_FSL_ERRATUM_A008585
/*
* The number of retries is an arbitrary value well beyond the highest number
* of iterations the loop has been observed to take.
@@ -145,6 +148,54 @@ static u64 fsl_a008585_read_cntvct_el0(void)
};
#endif /* CONFIG_FSL_ERRATUM_A008585 */
+#ifdef CONFIG_HISILICON_ERRATUM_161601
+/*
+ * Verify whether the value of the second read is larger than the first by
+ * less than 32 is the only way to confirm the value is correct, so clear the
+ * lower 5 bits to check whether the difference is greater than 32 or not.
+ * Theoretically the erratum should not occur more than twice in succession
+ * when reading the system counter, but it is possible that some interrupts
+ * may lead to more than twice read errors, triggering the warning, so setting
+ * the number of retries far beyond the number of iterations the loop has been
+ * observed to take.
+ */
+#define __hisi_161601_read_reg(reg) ({ \
+ u64 _old, _new; \
+ int _retries = 50; \
+ \
+ do { \
+ _old = read_sysreg(reg); \
+ _new = read_sysreg(reg); \
+ _retries--; \
+ } while (unlikely((_new - _old) >> 5) && _retries); \
+ \
+ WARN_ON_ONCE(!_retries); \
+ _new; \
+})
+
+static u32 hisi_161601_read_cntp_tval_el0(void)
+{
+ return __hisi_161601_read_reg(cntp_tval_el0);
+}
+
+static u32 hisi_161601_read_cntv_tval_el0(void)
+{
+ return __hisi_161601_read_reg(cntv_tval_el0);
+}
+
+static u64 hisi_161601_read_cntvct_el0(void)
+{
+ return __hisi_161601_read_reg(cntvct_el0);
+}
+
+static struct arch_timer_erratum_workaround arch_timer_hisi_161601 = {
+ .erratum = HISILICON_161601,
+ .read_cntp_tval_el0 = hisi_161601_read_cntp_tval_el0,
+ .read_cntv_tval_el0 = hisi_161601_read_cntv_tval_el0,
+ .read_cntvct_el0 = hisi_161601_read_cntvct_el0,
+};
+#endif /* CONFIG_HISILICON_ERRATUM_161601 */
+
static __always_inline
void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
struct clock_event_device *clk)
@@ -294,7 +345,7 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
}
-#ifdef CONFIG_FSL_ERRATUM_A008585
+#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
static __always_inline void erratum_set_next_event_generic(const int access,
unsigned long evt, struct clock_event_device *clk)
{
@@ -358,7 +409,7 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
static void erratum_workaround_set_sne(struct clock_event_device *clk)
{
-#ifdef CONFIG_FSL_ERRATUM_A008585
+#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
return;
@@ -618,7 +669,7 @@ static void __init arch_counter_register(unsigned type)
clocksource_counter.archdata.vdso_direct = true;
-#ifdef CONFIG_FSL_ERRATUM_A008585
+#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
/*
* Don't use the vdso fastpath if errata require using
* the out-of-line counter accessor.
@@ -909,10 +960,19 @@ static int __init arch_timer_of_init(struct device_node *np)
#ifdef CONFIG_FSL_ERRATUM_A008585
if (!timer_unstable_counter_workaround && of_property_read_bool(np, "fsl,erratum-a008585"))
timer_unstable_counter_workaround = &arch_timer_fsl_a008585;
+#endif
+
+#ifdef CONFIG_HISILICON_ERRATUM_161601
+ if (!timer_unstable_counter_workaround && of_property_read_bool(np, "hisilicon,erratum-161601"))
+ timer_unstable_counter_workaround = &arch_timer_hisi_161601;
+#endif
+#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
if (timer_unstable_counter_workaround) {
static_branch_enable(&arch_timer_read_ool_enabled);
- pr_info("Enabling workaround for FSL erratum A-008585\n");
+ pr_info("Enabling workaround for %s\n",
+ timer_unstable_counter_workaround->erratum == FSL_A008585 ?
+ "FSL ERRATUM A-008585" : "HISILICON ERRATUM 161601");
}
#endif
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v5 2/6] arm64: arch_timer: Introduce a generic erratum handing mechanism for fsl-a008585
From: Ding Tianhong @ 2016-12-23 7:04 UTC (permalink / raw)
To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland, oss,
devicetree, shawnguo, stuart.yoder, linux-arm-kernel, linuxarm
Cc: Ding Tianhong
In-Reply-To: <1482476669-15596-1-git-send-email-dingtianhong@huawei.com>
The workaround for hisilicon,161601 will check the return value of the system counter
by different way, in order to distinguish with the fsl-a008585 workaround, introduce
a new generic erratum handing mechanism for fsl-a008585 and rename some functions.
v2: Introducing a new generic erratum handling mechanism for fsl erratum a008585.
v3: Introducing the erratum_workaround_set_sne generic function for fsl erratum a008585
and make the #define __fsl_a008585_read_reg to be private to the .c file instead of
being globally visible. After discussion with Marc and Will, a consensus decision was
made to remove the commandline parameter for enabling fsl,erratum-a008585 erratum,
and make some generic name more specific, export timer_unstable_counter_workaround
for module access.
v5: Adapt the new documentation for kernel-parameters.txt.
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
Documentation/admin-guide/kernel-parameters.txt | 9 ---
arch/arm64/include/asm/arch_timer.h | 36 ++++--------
drivers/clocksource/arm_arch_timer.c | 78 +++++++++++++++----------
3 files changed, 58 insertions(+), 65 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 21e2d88..76437ad 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -539,15 +539,6 @@
loops can be debugged more effectively on production
systems.
- clocksource.arm_arch_timer.fsl-a008585=
- [ARM64]
- Format: <bool>
- Enable/disable the workaround of Freescale/NXP
- erratum A-008585. This can be useful for KVM
- guests, if the guest device tree doesn't show the
- erratum. If unspecified, the workaround is
- enabled based on the device tree.
-
clearcpuid=BITNUM [X86]
Disable CPUID feature X for the kernel. See
arch/x86/include/asm/cpufeatures.h for the valid bit
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index eaa5bbe..f882c7c 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -31,39 +31,27 @@
#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585)
extern struct static_key_false arch_timer_read_ool_enabled;
-#define needs_fsl_a008585_workaround() \
+#define needs_unstable_timer_counter_workaround() \
static_branch_unlikely(&arch_timer_read_ool_enabled)
#else
-#define needs_fsl_a008585_workaround() false
+#define needs_unstable_timer_counter_workaround() false
#endif
-u32 __fsl_a008585_read_cntp_tval_el0(void);
-u32 __fsl_a008585_read_cntv_tval_el0(void);
-u64 __fsl_a008585_read_cntvct_el0(void);
-/*
- * The number of retries is an arbitrary value well beyond the highest number
- * of iterations the loop has been observed to take.
- */
-#define __fsl_a008585_read_reg(reg) ({ \
- u64 _old, _new; \
- int _retries = 200; \
- \
- do { \
- _old = read_sysreg(reg); \
- _new = read_sysreg(reg); \
- _retries--; \
- } while (unlikely(_old != _new) && _retries); \
- \
- WARN_ON_ONCE(!_retries); \
- _new; \
-})
+struct arch_timer_erratum_workaround {
+ int erratum; /* Indicate the Erratum ID */
+ u32 (*read_cntp_tval_el0)(void);
+ u32 (*read_cntv_tval_el0)(void);
+ u64 (*read_cntvct_el0)(void);
+};
+
+extern struct arch_timer_erratum_workaround *timer_unstable_counter_workaround;
#define arch_timer_reg_read_stable(reg) \
({ \
u64 _val; \
- if (needs_fsl_a008585_workaround()) \
- _val = __fsl_a008585_read_##reg(); \
+ if (needs_unstable_timer_counter_workaround()) \
+ _val = timer_unstable_counter_workaround->read_##reg();\
else \
_val = read_sysreg(reg); \
_val; \
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 02fef68..e7406ad 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -96,40 +96,53 @@ static int __init early_evtstrm_cfg(char *buf)
*/
#ifdef CONFIG_FSL_ERRATUM_A008585
-DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
-EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
-
-static int fsl_a008585_enable = -1;
+struct arch_timer_erratum_workaround *timer_unstable_counter_workaround = NULL;
+EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
-static int __init early_fsl_a008585_cfg(char *buf)
-{
- int ret;
- bool val;
+#define FSL_A008585 0x0001
- ret = strtobool(buf, &val);
- if (ret)
- return ret;
-
- fsl_a008585_enable = val;
- return 0;
-}
-early_param("clocksource.arm_arch_timer.fsl-a008585", early_fsl_a008585_cfg);
+DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
+EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
-u32 __fsl_a008585_read_cntp_tval_el0(void)
+/*
+ * The number of retries is an arbitrary value well beyond the highest number
+ * of iterations the loop has been observed to take.
+ */
+#define __fsl_a008585_read_reg(reg) ({ \
+ u64 _old, _new; \
+ int _retries = 200; \
+ \
+ do { \
+ _old = read_sysreg(reg); \
+ _new = read_sysreg(reg); \
+ _retries--; \
+ } while (unlikely(_old != _new) && _retries); \
+ \
+ WARN_ON_ONCE(!_retries); \
+ _new; \
+})
+
+static u32 fsl_a008585_read_cntp_tval_el0(void)
{
return __fsl_a008585_read_reg(cntp_tval_el0);
}
-u32 __fsl_a008585_read_cntv_tval_el0(void)
+static u32 fsl_a008585_read_cntv_tval_el0(void)
{
return __fsl_a008585_read_reg(cntv_tval_el0);
}
-u64 __fsl_a008585_read_cntvct_el0(void)
+static u64 fsl_a008585_read_cntvct_el0(void)
{
return __fsl_a008585_read_reg(cntvct_el0);
}
-EXPORT_SYMBOL(__fsl_a008585_read_cntvct_el0);
+
+static struct arch_timer_erratum_workaround arch_timer_fsl_a008585 = {
+ .erratum = FSL_A008585,
+ .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0,
+ .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0,
+ .read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
+};
#endif /* CONFIG_FSL_ERRATUM_A008585 */
static __always_inline
@@ -282,7 +295,7 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
}
#ifdef CONFIG_FSL_ERRATUM_A008585
-static __always_inline void fsl_a008585_set_next_event(const int access,
+static __always_inline void erratum_set_next_event_generic(const int access,
unsigned long evt, struct clock_event_device *clk)
{
unsigned long ctrl;
@@ -300,17 +313,17 @@ static __always_inline void fsl_a008585_set_next_event(const int access,
arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
}
-static int fsl_a008585_set_next_event_virt(unsigned long evt,
+static int erratum_set_next_event_virt(unsigned long evt,
struct clock_event_device *clk)
{
- fsl_a008585_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
+ erratum_set_next_event_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
return 0;
}
-static int fsl_a008585_set_next_event_phys(unsigned long evt,
+static int erratum_set_next_event_phys(unsigned long evt,
struct clock_event_device *clk)
{
- fsl_a008585_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
+ erratum_set_next_event_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
return 0;
}
#endif /* CONFIG_FSL_ERRATUM_A008585 */
@@ -343,16 +356,16 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
return 0;
}
-static void fsl_a008585_set_sne(struct clock_event_device *clk)
+static void erratum_workaround_set_sne(struct clock_event_device *clk)
{
#ifdef CONFIG_FSL_ERRATUM_A008585
if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
return;
if (arch_timer_uses_ppi == VIRT_PPI)
- clk->set_next_event = fsl_a008585_set_next_event_virt;
+ clk->set_next_event = erratum_set_next_event_virt;
else
- clk->set_next_event = fsl_a008585_set_next_event_phys;
+ clk->set_next_event = erratum_set_next_event_phys;
#endif
}
@@ -385,7 +398,7 @@ static void __arch_timer_setup(unsigned type,
BUG();
}
- fsl_a008585_set_sne(clk);
+ erratum_workaround_set_sne(clk);
} else {
clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
clk->name = "arch_mem_timer";
@@ -894,9 +907,10 @@ static int __init arch_timer_of_init(struct device_node *np)
arch_timer_c3stop = !of_property_read_bool(np, "always-on");
#ifdef CONFIG_FSL_ERRATUM_A008585
- if (fsl_a008585_enable < 0)
- fsl_a008585_enable = of_property_read_bool(np, "fsl,erratum-a008585");
- if (fsl_a008585_enable) {
+ if (!timer_unstable_counter_workaround && of_property_read_bool(np, "fsl,erratum-a008585"))
+ timer_unstable_counter_workaround = &arch_timer_fsl_a008585;
+
+ if (timer_unstable_counter_workaround) {
static_branch_enable(&arch_timer_read_ool_enabled);
pr_info("Enabling workaround for FSL erratum A-008585\n");
}
--
1.9.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox