* Re: [PATCH 2/2] mmc: dw_mmc-rockchip: parse rockchip,default-num-phases from DT
From: Shawn Lin @ 2017-05-02 6:58 UTC (permalink / raw)
To: Doug Anderson
Cc: Jaehoon Chung, Ulf Hansson, Rob Herring,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Ziyuan Xu,
open list:ARM/Rockchip SoC...,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAD=FV=Uyqvzg5H+Mg5RtQAWiCxVARx7uB4jxPe=2fcSmJQoOjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Doug,
在 2017/4/25 0:18, Doug Anderson 写道:
> Hi,
>
> On Wed, Apr 19, 2017 at 6:21 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> Hi Doug,
>>
>> 在 2017/4/20 4:19, Doug Anderson 写道:
>>>
>>> Hi,
>>>
>>> On Wed, Apr 19, 2017 at 2:00 AM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>> wrote:
>>>>
>>>> Currently we unconditionally do tuning for each degree, which
>>>> costs 900ms for each boot and resume.
>>>>
>>>> May someone argue that this is a question of accuracy VS time. But I
>>>> would say it's a trick of how we need to do decision for our boards.
>>>> If we don't care the time we spend at all, we could definitely do tuning
>>>> for each degree. But when we need to improve the user experience, for
>>>> instance, speed up resuming from S3, we should also have the right to
>>>> do that. This patch add parsing "rockchip,default-num-phases", for folks
>>>> to specify the number of doing tuning. If not specified, 360 will be used
>>>> as before.
>>>>
>>>> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>>>
>>>> ---
>>>>
>>>> drivers/mmc/host/dw_mmc-rockchip.c | 48
>>>> ++++++++++++++++++++++++--------------
>>>> 1 file changed, 30 insertions(+), 18 deletions(-)
>>>
>>>
>>> No huge objection here, but I do remember we ended up at the 360
>>> phases due to some of the craziness with dw_mmc delay elements on
>>> rk3288. IIRC one of the big problems was that the delay elements
>>> changed _a lot_ with the "LOGIC" voltage and we tweaked the voltage at
>>> runtime for DDR DVFS. That imposed an extra need to be very accurate
>>> on that SoC, at least on any board that was designed to support DDR
>>> DVFS.
>>>
>>
>> Not just with the vdd_logic but also with the process of Soc.
>> To better guaratee the accuracy, firstly we use delay element to do
>> tuning and then convert it to be combination of degree + delay element.
>> But as the dalay elements aren't accuracy themself, so all the math we
>> do here is trick.
>
> Yup. I brought up the vdd logic specifically because it's something
> that can make the phases change quite dramatically on the same machine
> between the time you tuned and the time you used it.
>
>
>>> I also remember there being some weirdness on the Rockchip
>>> implementation where there was a certain set of phases that the MMC
>>> controller was essentially "blind". This blind spot was in the middle
>>> of an otherwise good range of points. Unfortunately this blind spot
>>> was somewhat hard to detect properly because it was not very big.
>>> ...the variability of the delay elements meant that there could be big
>>> ranges where we weren't getting any good test coverage, but also the
>>> fact that they changed with the LOGIC voltage might mean that we
>>> weren't in the "blind" spot and then suddenly we were.
>>
>>
>> I undertand all of these as I was suffering from it when bringing up
>> RK3288.
>>
>>>
>>> One other note is that i remember that the vast majority of time spent
>>> tuning was dealing with "bad" phases, not dealing with "good" phases.
>>> If you're looking to speed things up, maybe finding a way to make
>>> "bad" phases fail faster would be wise? I think one of the reasons
>>> bad phases failed so slowly is because the dw_mmc timeouts are all so
>>> long.
>>
>>
>> Good point. I haven't thought of speeding up the handle of bad phases,
>> but I will take a look at this.
>>
>>>
>>> Oh, and I guess one last note is that I have no idea if folks will
>>> like the device bindings here. Part of me thinks it should be
>>> something more "symbolic" like "rockchip,need-accurate-tuning" or
>>> something like that. I guess I'd let the DT experts chime in.
>>>
>>>
>>> So I guess to summarize:
>>> * On rk3288 boards w/ DDR DVFS (or any other similar boards), 360
>>> seems to provide real benefit.
>>> * On other boards, probably you can get by with fewer phases.
>>>
>>
>> I would try to say it's a question of "900ms for a single time" VS.
>> "some of discrete tuning cost for more chance to do retune".
>>
>> (1)We could try to do a more accurate tuning process and spends 900ms.
>> Then we have less chance to do retune later.
>>
>> (2)We do a rough tuning and have more chance to do retune later.
>
> Ah, interesting point. I haven't used newer versions of Linux much,
> but I seem to remember that they will automatically retune sometimes
> if they see errors. That makes your strategy a bit more valid.
>
>
>> I also would say that this is a game , and we can't say which
>> one is better. Obvious now the "900ms" alwyas happens in the spot
>> routine, for instance, booting and resuming from S3.
>
> Is it really 900 ms? I don't quite remember it being that long, but I
> could be remembering incorrectly.
I saw the worst case was nearly 900ms. But mostly we need 600ms there.
>
> -Doug
>
>
>
--
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 2/2] regulator: Allow for asymmetric settling times
From: Laxman Dewangan @ 2017-05-02 6:53 UTC (permalink / raw)
To: Matthias Kaehlcke, Liam Girdwood, Mark Brown, Rob Herring,
Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris
In-Reply-To: <20170501183715.35375-2-mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
On Tuesday 02 May 2017 12:07 AM, Matthias Kaehlcke wrote:
> Some regulators have different settling times for voltage increases and
> decreases. To avoid a time penalty on the faster transition allow for
> different settings for up- and downward transitions.
>
> Signed-off-by: Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
Acked-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
--
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 1/2] regulator: DT: Add properties for asymmetric settling times
From: Laxman Dewangan @ 2017-05-02 6:51 UTC (permalink / raw)
To: Matthias Kaehlcke, Liam Girdwood, Mark Brown, Rob Herring,
Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris
In-Reply-To: <20170501183715.35375-1-mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
On Tuesday 02 May 2017 12:07 AM, Matthias Kaehlcke wrote:
> Some regulators have different settling times for voltage increases and
> decreases. Add DT properties to define separate settling times for up-
> and downward voltage changes.
>
> Signed-off-by: Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
Acked-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
--
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 RFC] Documentation/devicetree: Add specification for FSI busses
From: Jeremy Kerr @ 2017-05-02 6:25 UTC (permalink / raw)
To: Joel Stanley; +Cc: devicetree, OpenBMC Maillist
In-Reply-To: <CACPK8Xf2regdT4Y-TW4nOc1qYwDcH=zYPRqdjTqL9yvZH49bRA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Joel,
> Looks good to me Jeremy. One small question on the description, but
> please add Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org> to future
> revisions.
Thanks for the review!
>> +The FSI bus is probe-able, so Linux is able to enumerate FSI slaves, and
>> +engines within those slaves. However, we have a facility to match devicetree
>> +nodes to probed engines. This allows for fsi engines to expose non-probeable
>> +busses, which are then exposed by the device tree. For example, a FSI engine
>
> A nit: Can we can expose any device as part of the engine, not just a bus?
Absolutely. I'll clarify in a future version. Something like:
This allows for fsi engines to expose non-probeable busses, or for
drivers to access extra device properties, by data described in the
device tree.
Cheers,
Jeremy
--
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 v6 07/23] drivers/fsi: Implement slave initialisation
From: Joel Stanley @ 2017-05-02 6:24 UTC (permalink / raw)
To: Christopher Bostic
Cc: Rob Herring, Mark Rutland, Russell King,
rostedt-nx8X9YLhiw1AfugRpC6u6w, mingo-H+wXaHxf7aLQT0dZR+AlfA,
Greg KH, devicetree,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Jeremy Kerr,
Linux Kernel Mailing List, Andrew Jeffery, Alistair Popple,
Benjamin Herrenschmidt
In-Reply-To: <20170410194706.64280-8-cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
<cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> From: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
>
> Implement fsi_slave_init: if we can read a chip ID, create fsi_slave
> devices and register with the driver core.
>
> Includes changes from Chris Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>.
>
> Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Chris Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> ---
> drivers/fsi/fsi-core.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 6e1cfdf..c705ca2 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -17,9 +17,12 @@
> #include <linux/fsi.h>
> #include <linux/idr.h>
> #include <linux/module.h>
> +#include <linux/slab.h>
>
> #include "fsi-master.h"
>
> +#define FSI_SLAVE_SIZE_23b 0x800000
> +
> static DEFINE_IDA(master_ida);
>
> struct fsi_slave {
> @@ -114,11 +117,70 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
> addr, val, size);
> }
>
> +static void fsi_slave_release(struct device *dev)
> +{
> + struct fsi_slave *slave = to_fsi_slave(dev);
> +
> + kfree(slave);
> +}
> +
> static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
> {
> - /* todo: initialise slave device, perform engine scan */
> + struct fsi_slave *slave;
> + uint32_t chip_id;
> + uint8_t crc;
> + int rc;
> +
> + /* Currently, we only support single slaves on a link, and use the
> + * full 23-bit address range
> + */
> + if (id != 0)
> + return -EINVAL;
> +
> + rc = fsi_master_read(master, link, id, 0, &chip_id, sizeof(chip_id));
> + if (rc) {
> + dev_warn(&master->dev, "can't read slave %02x:%02x %d\n",
> + link, id, rc);
When I boot a system with this driver loaded, I get his warning:
[ 9.740000] usbhid: USB HID core driver
[ 9.840000] fsi0: can't read slave 00:00 -5
[ 9.840000] NET: Registered protocol family 10
Two things:
There's a space in front of "fsi0".
This warning isn't useful at that point. The slave is not readable as
the FSI master is not present (the P9 hasn't been turned on). Can we
avoid printing the warning at boot?
Cheers,
Joel
> + return -ENODEV;
> + }
> + chip_id = be32_to_cpu(chip_id);
> +
> + crc = fsi_crc4(0, chip_id, 32);
> + if (crc) {
> + dev_warn(&master->dev, "slave %02x:%02x invalid chip id CRC!\n",
> + link, id);
> + return -EIO;
> + }
> +
> + dev_info(&master->dev, "fsi: found chip %08x at %02x:%02x:%02x\n",
> + chip_id, master->idx, link, id);
> +
> + /* We can communicate with a slave; create the slave device and
> + * register.
> + */
> + slave = kzalloc(sizeof(*slave), GFP_KERNEL);
> + if (!slave)
> + return -ENOMEM;
> +
> + slave->master = master;
> + slave->dev.parent = &master->dev;
> + slave->dev.release = fsi_slave_release;
> + slave->link = link;
> + slave->id = id;
> + slave->size = FSI_SLAVE_SIZE_23b;
> +
> + dev_set_name(&slave->dev, "slave@%02x:%02x", link, id);
> + rc = device_register(&slave->dev);
> + if (rc < 0) {
> + dev_warn(&master->dev, "failed to create slave device: %d\n",
> + rc);
> + put_device(&slave->dev);
> + return rc;
> + }
> +
> + /* todo: perform engine scan */
>
> - return -ENODEV;
> + return rc;
> }
>
> /* FSI master support */
> --
> 1.8.2.2
>
--
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 RFC] Documentation/devicetree: Add specification for FSI busses
From: Joel Stanley @ 2017-05-02 6:19 UTC (permalink / raw)
To: Jeremy Kerr; +Cc: devicetree, OpenBMC Maillist
In-Reply-To: <1493704508-27119-1-git-send-email-jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
On Tue, May 2, 2017 at 3:25 PM, Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org> wrote:
> This change introduces a proposed layout for describing FSI busses in
> the device tree. While the bus is probe-able, we'd still like a method
> of describing subordinate (eg i2c) busses that are behind FSI devices.
>
> The FSI core will be responsible for matching probed slaves & engines to
> their device tree nodes, so the FSI device drivers' probe() functions
> will be passed a struct device with the appropriate of_node populated
> where a matching DT node is found.
>
> RFC at this stage, so I'd welcome any feedback.
>
> Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
Looks good to me Jeremy. One small question on the description, but
please add Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org> to future
revisions.
> ---
> Documentation/devicetree/bindings/fsi/fsi.txt | 144 ++++++++++++++++++++++++++
> 1 file changed, 144 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/fsi/fsi.txt
>
> diff --git a/Documentation/devicetree/bindings/fsi/fsi.txt b/Documentation/devicetree/bindings/fsi/fsi.txt
> new file mode 100644
> index 0000000..b5e85c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fsi/fsi.txt
> @@ -0,0 +1,144 @@
> +FSI bus & engine generic device tree bindings
> +=============================================
> +
> +The FSI bus is probe-able, so Linux is able to enumerate FSI slaves, and
> +engines within those slaves. However, we have a facility to match devicetree
> +nodes to probed engines. This allows for fsi engines to expose non-probeable
> +busses, which are then exposed by the device tree. For example, a FSI engine
A nit: Can we can expose any device as part of the engine, not just a bus?
> +that is an I2C master - the I2C bus can be described by the device tree under
> +the engine's device tree node.
Cheers,
Joel
--
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 v4 3/3] USB3/DWC3: Enable undefined length INCR burst type
From: Jerry Huang @ 2017-05-02 6:13 UTC (permalink / raw)
To: Felipe Balbi, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
catalin.marinas-5wv7dgnIgG8@public.gmane.org
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Rajesh Bhagat
In-Reply-To: <87tw71xthj.fsf-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> -----Original Message-----
> From: Felipe Balbi [mailto:balbi-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Friday, March 10, 2017 7:27 PM
> To: Jerry Huang <jerry.huang-3arQi8VN3Tc@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> mark.rutland-5wv7dgnIgG8@public.gmane.org; catalin.marinas-5wv7dgnIgG8@public.gmane.org
> Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; Rajesh
> Bhagat <rajesh.bhagat-3arQi8VN3Tc@public.gmane.org>
> Subject: RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst
> type
>
>
> Hi,
>
> Jerry Huang <jerry.huang-3arQi8VN3Tc@public.gmane.org> writes:
> >> >> --
> >> >> 1.7.9.5
> >> > Hi, Balbi and all guys,
> >> > Any comment for these patches? Can they be accepted?
> >>
> >> Rob had comments which you didn't reply yet. I cannot take this
> >> patchset yet ;-)
> >>
> > Balbi,
> >
> > I look into his mail again, which was based v3, and I replied it.
> >
> > He had different understanding for undefined length burst mode.
> >
> > It seems he think for this mode, just setting bit[0] (INCRBrstEna) and
> > don't need to set other field.
> >
> > However, according to the DWC USB3.0 controller databook, when it is
> > undefined length INCR burst mode, we still need to set one max burst
> > type, such as INCR8, which means controller will use any length less
> > than or equal to this INCR8.
>
> Rob, do you agree with the patch now?
>
> --
> balbi
Hi, Balbi,
Any comment for these patches? Or any chance to merge them?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 RFC] Documentation/devicetree: Add specification for FSI busses
From: Jeremy Kerr @ 2017-05-02 5:55 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ
This change introduces a proposed layout for describing FSI busses in
the device tree. While the bus is probe-able, we'd still like a method
of describing subordinate (eg i2c) busses that are behind FSI devices.
The FSI core will be responsible for matching probed slaves & engines to
their device tree nodes, so the FSI device drivers' probe() functions
will be passed a struct device with the appropriate of_node populated
where a matching DT node is found.
RFC at this stage, so I'd welcome any feedback.
Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
---
Documentation/devicetree/bindings/fsi/fsi.txt | 144 ++++++++++++++++++++++++++
1 file changed, 144 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fsi/fsi.txt
diff --git a/Documentation/devicetree/bindings/fsi/fsi.txt b/Documentation/devicetree/bindings/fsi/fsi.txt
new file mode 100644
index 0000000..b5e85c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/fsi/fsi.txt
@@ -0,0 +1,144 @@
+FSI bus & engine generic device tree bindings
+=============================================
+
+The FSI bus is probe-able, so Linux is able to enumerate FSI slaves, and
+engines within those slaves. However, we have a facility to match devicetree
+nodes to probed engines. This allows for fsi engines to expose non-probeable
+busses, which are then exposed by the device tree. For example, a FSI engine
+that is an I2C master - the I2C bus can be described by the device tree under
+the engine's device tree node.
+
+FSI masters may require their own DT nodes (to describe the master HW itself);
+that requirement is defined by the master's implementation, and is described by
+the fsi-master-* binding specifications.
+
+Under the masters' nodes, we can describe the bus topology using nodes to
+represent the FSI slaves and their slave engines. As a basic outline:
+
+ fsi-master {
+ /* top-level of FSI bus topology, bound to a FSI master driver and
+ * exposes a FSI bus */
+
+ fsi-slave@<link,id> {
+ /* this node defines the FSI slave device, and is handled
+ * entirely with FSI core code */
+
+ fsi-slave-engine@<addr> {
+ /* this node defines the engine endpoint & address range, which
+ * is bound to the relevant fsi device driver */
+ ...
+ };
+
+ fsi-slave-engine@<addr> {
+ ...
+ };
+
+ };
+ };
+
+Note that since the bus is probe-able, some (or all) of the topology may
+not be described; this binding only provides an optional facility for
+adding subordinate device tree nodes as children of FSI engines.
+
+FSI masters
+-----------
+
+FSI master nodes declare themselves as such with the "fsi-master" compatible
+value. It's likely that an implementation-specific compatible value will
+be needed as well, for example:
+
+ compatible = "fsi-master-gpio", "fsi-master";
+
+Since the master nodes describe the top-level of the FSI topology, they also
+need to declare the FSI-standard addressing scheme. This requires two cells for
+addresses (link index and slave ID), and no size:
+
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+FSI slaves
+----------
+
+Slaves are identified by a (link-index, slave-id) pair, so require two cells
+for an address identifier. Since these are not a range, no size cells are
+required. For an example, a slave on link 1, with ID 2, could be represented
+as:
+
+ cfam@1,2 {
+ reg = <1 2>;
+ [...];
+ }
+
+Each slave provides an address-space, under which the engines are accessible.
+That address space has a maximum of 23 bits, so we use one cell to represent
+addresses and sizes in the slave address space:
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+
+FSI engines (devices)
+---------------------
+
+Engines are identified by their address under the slaves' address spaces. We
+use a single cell for address and size. Engine nodes represent the endpoint
+FSI device, and are passed to those FSI device drivers' ->probe() functions.
+
+For example, for a slave using a single 0x400-byte page starting at address
+0xc00:
+
+ engine@c00 {
+ reg = <0xc00 0x400>;
+ };
+
+
+Full example
+------------
+
+Here's an example that illustrates:
+ - a FSI master
+ - connected to a FSI slave
+ - that contains an engine that is an I2C master
+ - connected to an I2C EEPROM
+
+The FSI master may be connected to additional slaves, and slaves may have
+additional engines, but they don't necessarily need to be describe in the
+device tree if no extra platform information is required.
+
+ /* The GPIO-based FSI master node, describing the top level of the
+ * FSI bus
+ */
+ gpio-fsi {
+ compatible = "fsi-master-gpio", "fsi-master";
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ /* A FSI slave (aka. CFAM) at link 0, ID 0. */
+ cfam@0,0 {
+ reg = <0 0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ /* FSI engine at 0xc00, using a single page. In this example,
+ * it's an I2C master controller, so subnodes describe the
+ * I2C bus.
+ */
+ i2c-controller@c00 {
+ reg = <0xc00 0x400>;
+
+ /* Engine-specific data. In this case, we're describing an
+ * I2C bus, so we're conforming to the generic I2C binding
+ */
+ compatible = "ibm,fsi-i2c";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ /* I2C endpoint device: an Atmel EEPROM */
+ eeprom@50 {
+ compatible = "atmel,24c256";
+ reg = <0x50>;
+ pagesize = <64>;
+ };
+ };
+ };
+ };
--
2.7.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 related
* Re: [v6,10/23] drivers/fsi: Add device read/write/peek API
From: Brad Bishop @ 2017-05-02 5:25 UTC (permalink / raw)
To: Christopher Bostic
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mingo-H+wXaHxf7aLQT0dZR+AlfA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
andrew-zrmu5oMJ5Fs, alistair-Y4h6yKqj69EXC2x5gXVKYQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jeremy Kerr,
Edward A . James, benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
joel-U3u1mxZcP9KHXe+LvDLADg
In-Reply-To: <20170410194706.64280-11-cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> On Apr 10, 2017, at 3:46 PM, Christopher Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
>
> From: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
>
> This change introduces the fsi device API: simple read, write and peek
> accessors for the devices' address spaces.
>
> Includes contributions from Chris Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> and Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>.
>
> Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Chris Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> ---
> drivers/fsi/fsi-core.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/fsi.h | 7 ++++++-
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index a8faa89..4da0b030 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -32,6 +32,8 @@
> #define FSI_SLAVE_CONF_CRC_MASK 0x0000000f
> #define FSI_SLAVE_CONF_DATA_BITS 28
>
> +#define FSI_PEEK_BASE 0x410
> +
> static const int engine_page_size = 0x400;
>
> #define FSI_SLAVE_BASE 0x800
> @@ -73,9 +75,40 @@ static int fsi_master_read(struct fsi_master *master, int link,
> uint8_t slave_id, uint32_t addr, void *val, size_t size);
> static int fsi_master_write(struct fsi_master *master, int link,
> uint8_t slave_id, uint32_t addr, const void *val, size_t size);
> +static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
> + void *val, size_t size);
> +static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
> + const void *val, size_t size);
>
> /* FSI endpoint-device support */
>
> +int fsi_device_read(struct fsi_device *dev, uint32_t addr, void *val,
> + size_t size)
> +{
> + if (addr > dev->size || size > dev->size || addr > dev->size - size)
> + return -EINVAL;
> +
> + return fsi_slave_read(dev->slave, dev->addr + addr, val, size);
> +}
> +EXPORT_SYMBOL_GPL(fsi_device_read);
> +
> +int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
> + size_t size)
> +{
> + if (addr > dev->size || size > dev->size || addr > dev->size - size)
> + return -EINVAL;
> +
> + return fsi_slave_write(dev->slave, dev->addr + addr, val, size);
> +}
> +EXPORT_SYMBOL_GPL(fsi_device_write);
> +
> +int fsi_device_peek(struct fsi_device *dev, void *val)
> +{
> + uint32_t addr = FSI_PEEK_BASE + ((dev->unit - 2) * sizeof(uint32_t));
> +
> + return fsi_slave_read(dev->slave, addr, val, sizeof(uint32_t));
> +}
> +
> static void fsi_device_release(struct device *_device)
> {
> struct fsi_device *device = to_fsi_dev(_device);
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index efa55ba..66bce48 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -27,6 +27,12 @@ struct fsi_device {
> uint32_t size;
> };
>
> +extern int fsi_device_read(struct fsi_device *dev, uint32_t addr,
> + void *val, size_t size);
> +extern int fsi_device_write(struct fsi_device *dev, uint32_t addr,
> + const void *val, size_t size);
> +extern int fsi_device_peek(struct fsi_device *dev, void *val);
> +
> struct fsi_device_id {
> u8 engine_type;
> u8 version;
> @@ -40,7 +46,6 @@ struct fsi_device_id {
> #define FSI_DEVICE_VERSIONED(t, v) \
> .engine_type = (t), .version = (v),
>
> -
> struct fsi_driver {
> struct device_driver drv;
> const struct fsi_device_id *id_table;
I wrote a driver using this API. It seems to be suitable enough.
Acked-by: Brad Bishop <bradleyb-r5pk2Da7Bxt8sGd51Jp2sdBPR1lH4CV8@public.gmane.org>
--
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 v4 4/4] of: detect invalid phandle in overlay
From: frowand.list @ 2017-05-02 2:46 UTC (permalink / raw)
To: Rob Herring, stephen.boyd; +Cc: devicetree, linux-kernel
In-Reply-To: <1493693164-22826-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
Overlays are not allowed to modify phandle values of previously existing
nodes because there is no information available to allow fixup up
properties that use the previously existing phandle.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index ca0b85f5deb1..20ab49d2f7a4 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -130,6 +130,10 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,
/* NOTE: Multiple mods of created nodes not supported */
tchild = of_get_child_by_name(target, cname);
if (tchild != NULL) {
+ /* new overlay phandle value conflicts with existing value */
+ if (child->phandle)
+ return -EINVAL;
+
/* apply overlay recursively */
ret = of_overlay_apply_one(ov, tchild, child);
of_node_put(tchild);
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 3/4] of: be consistent in form of file mode
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-05-02 2:46 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493693164-22826-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
checkpatch whined about using S_IRUGO instead of octal equivalent
when adding phandle sysfs code, so used octal in that patch.
Change other instances of the S_* constants in the same file to
the octal form.
Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
drivers/of/base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8a0cf9003cf8..8a7ca2a49ba8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -168,7 +168,7 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
sysfs_bin_attr_init(&pp->attr);
pp->attr.attr.name = safe_name(&np->kobj, pp->name);
- pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
+ pp->attr.attr.mode = secure ? 0400 : 0444;
pp->attr.size = secure ? 0 : pp->length;
pp->attr.read = of_node_property_read;
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
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 v4 2/4] of: make __of_attach_node() static
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-05-02 2:46 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493693164-22826-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
__of_attach_node() is not used outside of drivers/of/dynamic.c. Make
it static and remove it from drivers/of/of_private.h.
Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
drivers/of/dynamic.c | 2 +-
drivers/of/of_private.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 59545b8fbf46..0b9cf6d5914c 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -216,7 +216,7 @@ int of_property_notify(int action, struct device_node *np,
return of_reconfig_notify(action, &pr);
}
-void __of_attach_node(struct device_node *np)
+static void __of_attach_node(struct device_node *np)
{
np->child = NULL;
np->sibling = np->parent->child;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 33f11a5384f3..b1ff70e1fdda 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -79,7 +79,6 @@ extern int __of_update_property(struct device_node *np,
extern void __of_update_property_sysfs(struct device_node *np,
struct property *newprop, struct property *oldprop);
-extern void __of_attach_node(struct device_node *np);
extern int __of_attach_node_sysfs(struct device_node *np);
extern void __of_detach_node(struct device_node *np);
extern void __of_detach_node_sysfs(struct device_node *np);
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
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 v4 1/4] of: remove *phandle properties from expanded device tree
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-05-02 2:46 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493693164-22826-1-git-send-email-frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
Remove "phandle", "linux,phandle", and "ibm,phandle" properties from
the internal device tree. The phandle will still be in the struct
device_node phandle field.
This is to resolve the issue found by Stephen Boyd [1] when he changed
the type of struct property.value from void * to const void *. As
a result of the type change, the overlay code had compile errors
where the resolver updates phandle values.
[1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
- Add sysfs infrastructure to report np->phandle, as if it was a property.
- Do not create "phandle" "ibm,phandle", and "linux,phandle" properties
in the expanded device tree.
- Remove phandle properties in of_attach_node(), for nodes dynamically
attached to the live tree. Add the phandle sysfs entry for these nodes.
- When creating an overlay changeset, duplicate the node phandle in
__of_node_dup().
- Remove no longer needed checks to exclude "phandle" and "linux,phandle"
properties in several locations.
- A side effect of these changes is that the obsolete "linux,phandle" and
"ibm,phandle" properties will no longer appear in /proc/device-tree (they
will appear as "phandle").
Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
drivers/of/base.c | 48 ++++++++++++++++++++++++++++++++++++++++---
drivers/of/dynamic.c | 54 +++++++++++++++++++++++++++++++++++++------------
drivers/of/fdt.c | 40 +++++++++++++++++++++---------------
drivers/of/of_private.h | 1 +
drivers/of/overlay.c | 4 +---
drivers/of/resolver.c | 23 +--------------------
include/linux/of.h | 1 +
7 files changed, 114 insertions(+), 57 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index d7c4629a3a2d..8a0cf9003cf8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -116,6 +116,19 @@ static ssize_t of_node_property_read(struct file *filp, struct kobject *kobj,
return memory_read_from_buffer(buf, count, &offset, pp->value, pp->length);
}
+static ssize_t of_node_phandle_read(struct file *filp, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t offset, size_t count)
+{
+ phandle phandle;
+ struct device_node *np;
+
+ np = container_of(bin_attr, struct device_node, attr_phandle);
+ phandle = cpu_to_be32(np->phandle);
+ return memory_read_from_buffer(buf, count, &offset, &phandle,
+ sizeof(phandle));
+}
+
/* always return newly allocated name, caller must free after use */
static const char *safe_name(struct kobject *kobj, const char *orig_name)
{
@@ -164,6 +177,35 @@ int __of_add_property_sysfs(struct device_node *np, struct property *pp)
return rc;
}
+/*
+ * In the imported device tree (fdt), phandle is a property. In the
+ * internal data structure it is instead stored in the struct device_node.
+ * Make phandle visible in sysfs as if it was a property.
+ */
+int __of_add_phandle_sysfs(struct device_node *np)
+{
+ int rc;
+
+ if (!IS_ENABLED(CONFIG_SYSFS))
+ return 0;
+
+ if (!of_kset || !of_node_is_attached(np))
+ return 0;
+
+ if (!np->phandle || np->phandle == 0xffffffff)
+ return 0;
+
+ sysfs_bin_attr_init(&np->attr_phandle);
+ np->attr_phandle.attr.name = "phandle";
+ np->attr_phandle.attr.mode = 0444;
+ np->attr_phandle.size = sizeof(np->phandle);
+ np->attr_phandle.read = of_node_phandle_read;
+
+ rc = sysfs_create_bin_file(&np->kobj, &np->attr_phandle);
+ WARN(rc, "error adding attribute phandle to node %s\n", np->full_name);
+ return rc;
+}
+
int __of_attach_node_sysfs(struct device_node *np)
{
const char *name;
@@ -193,6 +235,8 @@ int __of_attach_node_sysfs(struct device_node *np)
if (rc)
return rc;
+ __of_add_phandle_sysfs(np);
+
for_each_property_of_node(np, pp)
__of_add_property_sysfs(np, pp);
@@ -2097,9 +2141,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
int id, len;
/* Skip those we do not want to proceed */
- if (!strcmp(pp->name, "name") ||
- !strcmp(pp->name, "phandle") ||
- !strcmp(pp->name, "linux,phandle"))
+ if (!strcmp(pp->name, "name"))
continue;
np = of_find_node_by_path(pp->value);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 888fdbc09992..59545b8fbf46 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -218,19 +218,6 @@ int of_property_notify(int action, struct device_node *np,
void __of_attach_node(struct device_node *np)
{
- const __be32 *phandle;
- int sz;
-
- np->name = __of_get_property(np, "name", NULL) ? : "<NULL>";
- np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>";
-
- phandle = __of_get_property(np, "phandle", &sz);
- if (!phandle)
- phandle = __of_get_property(np, "linux,phandle", &sz);
- if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle)
- phandle = __of_get_property(np, "ibm,phandle", &sz);
- np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0;
-
np->child = NULL;
np->sibling = np->parent->child;
np->parent->child = np;
@@ -243,11 +230,47 @@ void __of_attach_node(struct device_node *np)
int of_attach_node(struct device_node *np)
{
struct of_reconfig_data rd;
+ struct property *prev;
+ struct property *prop;
+ struct property *save_next;
unsigned long flags;
+ const __be32 *phandle;
+ int sz;
memset(&rd, 0, sizeof(rd));
rd.dn = np;
+ np->name = __of_get_property(np, "name", NULL) ? : "<NULL>";
+ np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>";
+
+ phandle = __of_get_property(np, "phandle", &sz);
+ if (!phandle)
+ phandle = __of_get_property(np, "linux,phandle", &sz);
+ if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle)
+ phandle = __of_get_property(np, "ibm,phandle", &sz);
+ np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0;
+
+ /* remove phandle properties from node */
+ prev = NULL;
+ for (prop = np->properties; prop != NULL; ) {
+ save_next = prop->next;
+ if (!strcmp(prop->name, "phandle") ||
+ !strcmp(prop->name, "ibm,phandle") ||
+ !strcmp(prop->name, "linux,phandle")) {
+ if (prev)
+ prev->next = prop->next;
+ else
+ np->properties = prop->next;
+ prop->next = np->deadprops;
+ np->deadprops = prop;
+ } else {
+ prev = prop;
+ }
+ prop = save_next;
+ }
+
+ __of_add_phandle_sysfs(np);
+
mutex_lock(&of_mutex);
raw_spin_lock_irqsave(&devtree_lock, flags);
__of_attach_node(np);
@@ -429,6 +452,7 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
/* Iterate over and duplicate all properties */
if (np) {
struct property *pp, *new_pp;
+ node->phandle = np->phandle;
for_each_property_of_node(np, pp) {
new_pp = __of_prop_dup(pp, GFP_KERNEL);
if (!new_pp)
@@ -441,6 +465,10 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
}
}
}
+
+ node->name = __of_get_property(node, "name", NULL) ? : "<NULL>";
+ node->type = __of_get_property(node, "device_type", NULL) ? : "<NULL>";
+
return node;
err_prop:
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..270f31b0c259 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -181,6 +181,8 @@ static void populate_properties(const void *blob,
const __be32 *val;
const char *pname;
u32 sz;
+ int prop_is_phandle = 0;
+ int prop_is_ibm_phandle = 0;
val = fdt_getprop_by_offset(blob, cur, &pname, &sz);
if (!val) {
@@ -196,11 +198,6 @@ static void populate_properties(const void *blob,
if (!strcmp(pname, "name"))
has_name = true;
- pp = unflatten_dt_alloc(mem, sizeof(struct property),
- __alignof__(struct property));
- if (dryrun)
- continue;
-
/* We accept flattened tree phandles either in
* ePAPR-style "phandle" properties, or the
* legacy "linux,phandle" properties. If both
@@ -208,23 +205,34 @@ static void populate_properties(const void *blob,
* will get weird. Don't do that.
*/
if (!strcmp(pname, "phandle") ||
- !strcmp(pname, "linux,phandle")) {
- if (!np->phandle)
- np->phandle = be32_to_cpup(val);
- }
+ !strcmp(pname, "linux,phandle"))
+ prop_is_phandle = 1;
/* And we process the "ibm,phandle" property
* used in pSeries dynamic device tree
* stuff
*/
- if (!strcmp(pname, "ibm,phandle"))
- np->phandle = be32_to_cpup(val);
+ if (!strcmp(pname, "ibm,phandle")) {
+ prop_is_phandle = 1;
+ prop_is_ibm_phandle = 1;
+ }
+
+ if (!prop_is_phandle)
+ pp = unflatten_dt_alloc(mem, sizeof(struct property),
+ __alignof__(struct property));
- pp->name = (char *)pname;
- pp->length = sz;
- pp->value = (__be32 *)val;
- *pprev = pp;
- pprev = &pp->next;
+ if (dryrun)
+ continue;
+
+ if (!prop_is_phandle) {
+ pp->name = (char *)pname;
+ pp->length = sz;
+ pp->value = (__be32 *)val;
+ *pprev = pp;
+ pprev = &pp->next;
+ } else if (prop_is_ibm_phandle || !np->phandle) {
+ np->phandle = be32_to_cpup(val);
+ }
}
/* With version 0x10 we may not have the name property,
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 18bbb4517e25..33f11a5384f3 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -47,6 +47,7 @@ extern int of_property_notify(int action, struct device_node *np,
extern void of_node_release(struct kobject *kobj);
extern int __of_changeset_apply(struct of_changeset *ocs);
extern int __of_changeset_revert(struct of_changeset *ocs);
+extern int __of_add_phandle_sysfs(struct device_node *np);
#else /* CONFIG_OF_DYNAMIC */
static inline int of_property_notify(int action, struct device_node *np,
struct property *prop, struct property *old_prop)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7827786718d8..ca0b85f5deb1 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -101,9 +101,7 @@ static int of_overlay_apply_single_property(struct of_overlay *ov,
tprop = of_find_property(target, prop->name, NULL);
/* special properties are not meant to be updated (silent NOP) */
- if (of_prop_cmp(prop->name, "name") == 0 ||
- of_prop_cmp(prop->name, "phandle") == 0 ||
- of_prop_cmp(prop->name, "linux,phandle") == 0)
+ if (!of_prop_cmp(prop->name, "name"))
return 0;
propn = __of_prop_dup(prop, GFP_KERNEL);
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 7ae9863cb0a4..624cbaeae0a4 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -71,30 +71,11 @@ static void adjust_overlay_phandles(struct device_node *overlay,
int phandle_delta)
{
struct device_node *child;
- struct property *prop;
- phandle phandle;
/* adjust node's phandle in node */
if (overlay->phandle != 0 && overlay->phandle != OF_PHANDLE_ILLEGAL)
overlay->phandle += phandle_delta;
- /* copy adjusted phandle into *phandle properties */
- for_each_property_of_node(overlay, prop) {
-
- if (of_prop_cmp(prop->name, "phandle") &&
- of_prop_cmp(prop->name, "linux,phandle"))
- continue;
-
- if (prop->length < 4)
- continue;
-
- phandle = be32_to_cpup(prop->value);
- if (phandle == OF_PHANDLE_ILLEGAL)
- continue;
-
- *(uint32_t *)prop->value = cpu_to_be32(overlay->phandle);
- }
-
for_each_child_of_node(overlay, child)
adjust_overlay_phandles(child, phandle_delta);
}
@@ -197,9 +178,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
for_each_property_of_node(local_fixups, prop_fix) {
/* skip properties added automatically */
- if (!of_prop_cmp(prop_fix->name, "name") ||
- !of_prop_cmp(prop_fix->name, "phandle") ||
- !of_prop_cmp(prop_fix->name, "linux,phandle"))
+ if (!of_prop_cmp(prop_fix->name, "name"))
continue;
if ((prop_fix->length % 4) != 0 || prop_fix->length == 0)
diff --git a/include/linux/of.h b/include/linux/of.h
index 21e6323de0f3..4e86e05853f3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -61,6 +61,7 @@ struct device_node {
struct kobject kobj;
unsigned long _flags;
void *data;
+ struct bin_attribute attr_phandle;
#if defined(CONFIG_SPARC)
const char *path_component_name;
unsigned int unique_id;
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
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 v4 0/4] of: remove *phandle properties from expanded device tree
From: frowand.list-Re5JQEeQqe8AvxtiuMwx3w @ 2017-05-02 2:46 UTC (permalink / raw)
To: Rob Herring, stephen.boyd-QSEj5FYQhm4dnm+yROfE0A
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
Remove "phandle" and "linux,phandle" properties from the internal
device tree. The phandle will still be in the struct device_node
phandle field.
This is to resolve the issue found by Stephen Boyd [1] when he changed
the type of struct property.value from void * to const void *. As
a result of the type change, the overlay code had compile errors
where the resolver updates phandle values.
[1] http://lkml.iu.edu/hypermail/linux/kernel/1702.1/04160.html
Patch 1 is the phandle related changes.
Patches 2 - 4 are minor fixups for issues that became visible
while implementing patch 1.
Changes from v3:
- patch 1: fix incorrect variable name in __of_add_phandle_sysfs().
Problem was reported by the kbuild test robot
Changes from v2:
- patch 1: Remove check in __of_add_phandle_sysfs() that would not
add a sysfs entry if IS_ENABLED(CONFIG_PPC_PSERIES)
Changes from v1:
- Remove phandle properties in of_attach_node(), before attaching
the node to the live tree.
- Add the phandle sysfs entry for the node in of_attach_node().
- When creating an overlay changeset, duplicate the node phandle in
__of_node_dup().
Frank Rowand (4):
of: remove *phandle properties from expanded device tree
of: make __of_attach_node() static
of: be consistent in form of file mode
of: detect invalid phandle in overlay
drivers/of/base.c | 50 +++++++++++++++++++++++++++++++++++++++----
drivers/of/dynamic.c | 56 ++++++++++++++++++++++++++++++++++++-------------
drivers/of/fdt.c | 40 +++++++++++++++++++++--------------
drivers/of/of_private.h | 2 +-
drivers/of/overlay.c | 8 ++++---
drivers/of/resolver.c | 23 +-------------------
include/linux/of.h | 1 +
7 files changed, 120 insertions(+), 60 deletions(-)
--
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
--
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 7/9] power: Adds support for Smart Battery System Manager
From: Phil Reid @ 2017-05-02 2:20 UTC (permalink / raw)
To: Sebastian Reichel
Cc: wsa, robh+dt, mark.rutland, peda, linux-i2c, devicetree, linux-pm,
Karl-Heinz Schneider
In-Reply-To: <20170501134136.u2tebw77xcirvaqx@earth>
G'day Sebastian,
Thanks for the feedback. Next version coming soon.
In regards the le to cpu conversion.
grepping the supply folder finds a couple of other suspicious cases.
sbs-battery & the bq24735-cahrger do similar things.
thoughts on if they need to be corrected?
On 1/05/2017 21:41, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 01, 2017 at 04:49:57PM +0800, Phil Reid wrote:
>> From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>>
>> This patch adds support for Smart Battery System Manager.
>> A SBSM is a device listening at I2C/SMBus address 0x0a and is capable of
>> communicating up to four I2C smart battery devices. All smart battery
>> devices are listening at address 0x0b, so the SBSM muliplexes between
>> them. The driver makes use of the I2C-Mux framework to allow smart
>> batteries to be bound via device tree, i.e. the sbs-battery driver.
>>
>> Via sysfs interface the online state and charge type are presented. If
>> the driver is bound as ltc1760 (an implementation of a Dual Smart Battery
>> System Manager) the charge type can also be changed from trickle to fast.
>>
>> Tested-by: Phil Reid <preid@electromag.com.au>
>> Reviewed-by: Phil Reid <preid@electromag.com.au>
>> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>> drivers/power/supply/Kconfig | 13 ++
>> drivers/power/supply/Makefile | 1 +
>> drivers/power/supply/sbs-manager.c | 325 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 339 insertions(+)
>> create mode 100644 drivers/power/supply/sbs-manager.c
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index da54ac8..8aa5324 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -533,4 +533,17 @@ config AXP20X_POWER
>> This driver provides support for the power supply features of
>> AXP20x PMIC.
>>
>> +config MANAGER_SBS
>> + tristate "Smart Battery System Manager"
>> + depends on I2C && I2C_MUX
>> + help
>> + Say Y here to include support for Smart Battery System Manager
>> + ICs. The driver reports online and charging status via sysfs.
>> + It presents itself also as I2C mux which allows to bind
>> + smart battery driver to its ports.
>> + Supported is for example LTC1760.
>> +
>> + This driver can also be built as a module. If so, the module will be
>> + called sbs-manager.
>> +
>
> Let's move add this directly next to CHARGER_SBS.
>
>> endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index 3789a2c..4f53c98 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o
>> obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o
>> obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>> obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o
>> +obj-$(CONFIG_MANAGER_SBS) += sbs-manager.o
>
> same here.
>
>> diff --git a/drivers/power/supply/sbs-manager.c b/drivers/power/supply/sbs-manager.c
>> new file mode 100644
>> index 0000000..adf9e41
>> --- /dev/null
>> +++ b/drivers/power/supply/sbs-manager.c
>> @@ -0,0 +1,325 @@
>> +/*
>> + * Driver for SBS compliant Smart Battery System Managers
>> + *
>> + * The device communicates via i2c at address 0x0a and multiplexes access to up
>> + * to four smart batteries at address 0x0b.
>> + *
>> + * Via sysfs interface the online state and charge type are presented.
>> + *
>> + * Datasheet SBSM: http://sbs-forum.org/specs/sbsm100b.pdf
>> + * Datasheet LTC1760: http://cds.linear.com/docs/en/datasheet/1760fb.pdf
>> + *
>> + * Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-mux.h>
>> +#include <linux/power_supply.h>
>> +
>> +#define SBSM_MAX_BATS 4
>> +#define SBSM_RETRY_CNT 3
>> +
>> +/* registers addresses */
>> +#define SBSM_CMD_BATSYSSTATE 0x01
>> +#define SBSM_CMD_BATSYSSTATECONT 0x02
>> +#define SBSM_CMD_BATSYSINFO 0x04
>> +#define SBSM_CMD_LTC 0x3c
>> +
>> +struct sbsm_data {
>> + struct i2c_client *client;
>> + struct i2c_mux_core *muxc;
>> +
>> + struct power_supply *psy;
>> +
>> + int cur_chan; /* currently selected channel */
>
> u8?
>
>> + bool is_ltc1760; /* special capabilities */
>> +};
>> +
>> +static enum power_supply_property sbsm_props[] = {
>> + POWER_SUPPLY_PROP_ONLINE,
>> + POWER_SUPPLY_PROP_CHARGE_TYPE,
>> +};
>> +
>> +static int sbsm_read_word(struct i2c_client *client, u8 address)
>> +{
>> + int reg, retries = SBSM_RETRY_CNT;
>> +
>> + while (retries > 0) {
>> + reg = i2c_smbus_read_word_data(client, address);
>> + if (reg >= 0)
>> + break;
>> + --retries;
>> + }
>
> for (retries = SBSM_RETRY_CNT; retries > 0; retries--)
>
>> +
>> + if (reg < 0) {
>> + dev_err(&client->dev, "failed to read register %i\n",
>> + (int)address);
>
> dev_err(&client->dev, "failed to read register 0x%02x\n", address);
>
>> + return reg;
>> + }
>> +
>> + return le16_to_cpu(reg);
>
> This is already done by i2c_smbus_read_word_data() and doing it
> again will result in incorrect values on big endian architectures.
>
>> +}
>> +
>> +static int sbsm_write_word(struct i2c_client *client, u8 address, u16 word)
>> +{
>> + int ret, retries = SBSM_RETRY_CNT;
>> +
>> + word = cpu_to_le16(word);
>
> Same as for read_word: This is already done by the i2c-core.
>
>> + while (retries > 0) {
>> + ret = i2c_smbus_write_word_data(client, address, word);
>> + if (ret >= 0)
>> + break;
>> + --retries;
>> + }
>
> for (retries = SBSM_RETRY_CNT; retries > 0; retries--)
>
>> + if (ret < 0)
>> + dev_err(&client->dev, "failed to write to register %i\n",
>> + (int)address);
>
> dev_err(&client->dev, "failed to read register 0x%02x\n", address);
>
>> +
>> + return ret;
>> +}
>> +
>> +static int sbsm_get_property(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + union power_supply_propval *val)
>> +{
>> + struct sbsm_data *data = power_supply_get_drvdata(psy);
>> + int regval = 0;
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + regval = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATECONT);
>> + if (regval < 0)
>> + return regval;
>> + val->intval = !!(regval & 0x1);
>> + break;
>> +
>> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> + regval = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE);
>> + if (regval < 0)
>> + return regval;
>> +
>> + if ((regval & 0x00f0) == 0) {
>> + val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
>> + return 0;
>> + }
>> + val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
>> +
>> + if (data->is_ltc1760) {
>> + /* charge mode fast if turbo is active */
>> + regval = sbsm_read_word(data->client, SBSM_CMD_LTC);
>> + if (regval < 0)
>> + return regval;
>> + else if (regval & 0x80)
>> + val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
>> + }
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int sbsm_prop_is_writeable(struct power_supply *psy,
>> + enum power_supply_property psp)
>> +{
>> + struct sbsm_data *data = power_supply_get_drvdata(psy);
>> +
>> + return (psp == POWER_SUPPLY_PROP_CHARGE_TYPE) && data->is_ltc1760;
>> +}
>> +
>> +static int sbsm_set_property(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + const union power_supply_propval *val)
>> +{
>> + struct sbsm_data *data = power_supply_get_drvdata(psy);
>> + int ret = -EINVAL;
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> + /* write 1 to TURBO if type fast is given */
>> + if (data->is_ltc1760) {
>> + u16 regval = val->intval ==
>> + POWER_SUPPLY_CHARGE_TYPE_FAST ? (0x1 << 7) : 0;
>> + ret = sbsm_write_word(data->client, SBSM_CMD_LTC,
>> + regval);
>> + }
>
> That's not nicely indented. Try doing it this way to
> reduce indention:
>
> if (data->is_ltc1760)
> break;
> u16 regval = ...
>
>> + break;
>> +
>> + default:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Switch to battery
>> + * Parameter chan is directly the content of SMB_BAT* nibble
>> + */
>> +static int sbsm_select(struct i2c_mux_core *muxc, u32 chan)
>> +{
>> + struct sbsm_data *data = i2c_mux_priv(muxc);
>> + int ret = 0;
>> + u16 reg;
>> +
>> + if (data->cur_chan == chan)
>> + return ret;
>> +
>> + /* chan goes from 1 ... 4 */
>> + reg = 1 << (11 + chan);
>> + ret = sbsm_write_word(data->client, SBSM_CMD_BATSYSSTATE, reg);
>> + if (ret)
>> + dev_err(&data->client->dev, "Failed to select channel %i\n",
>> + chan);
>
> Add
>
> struct device *dev = &data->client->dev;
>
> at the beginning of the function and use it here to avoid line
> break.
>
>> + else
>> + data->cur_chan = chan;
>> +
>> + return ret;
>> +}
>> +
>> +#if defined(CONFIG_OF)
>> +
>> +#include <linux/of_device.h>
>
> Move include to top of file. You can include it unconditionally.
>
>> +static const struct of_device_id sbsm_dt_ids[] = {
>> + { .compatible = "sbs,sbs-manager" },
>> + { .compatible = "lltc,ltc1760" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sbsm_dt_ids);
>
> This can go next to the other MODULE_DEVICE_TABLE.
>
>> +#endif
>> +
>> +static const struct power_supply_desc sbsm_default_psy_desc = {
>> + .type = POWER_SUPPLY_TYPE_MAINS,
>> + .properties = sbsm_props,
>> + .num_properties = ARRAY_SIZE(sbsm_props),
>> + .get_property = &sbsm_get_property,
>> + .set_property = &sbsm_set_property,
>> + .property_is_writeable = &sbsm_prop_is_writeable,
>> +};
>> +
>> +static int sbsm_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>> + struct sbsm_data *data;
>> + struct device *dev = &client->dev;
>> + struct power_supply_desc *psy_desc;
>> + struct power_supply_config psy_cfg = {};
>> + int ret = 0, i, supported_bats;
>> +
>> + /* Device listens only at address 0x0a */
>> + if (client->addr != 0x0a)
>> + return -ENODEV;
>
> I guess EINVAL makes more sense?
>
>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>> + return -EPFNOSUPPORT;
>> +
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, data);
>> +
>> + data->client = client;
>> + data->is_ltc1760 = !!strstr(id->name, "ltc1760");
>> +
>> + ret = sbsm_read_word(client, SBSM_CMD_BATSYSINFO);
>> + if (ret < 0)
>> + return ret;
>> + supported_bats = le16_to_cpu(ret) & 0xf;
>
> How often do you want to convert endianess? :)
> Drop this.
>
>> + data->muxc = i2c_mux_alloc(adapter, dev, SBSM_MAX_BATS, 0,
>> + I2C_MUX_LOCKED, &sbsm_select, NULL);
>> + if (!data->muxc) {
>> + dev_err(dev, "failed to alloc i2c mux\n");
>> + ret = -ENOMEM;
>> + goto err_mux_alloc;
>> + }
>> + data->muxc->priv = data;
>> +
>> + /* register muxed i2c channels. One for each supported battery */
>> + for (i = 0; i < SBSM_MAX_BATS; ++i) {
>> + if ((1 << i) & supported_bats) {
>
> This can be written more readable as
>
> if (supported_bats & BIT(i))
>
>> + ret = i2c_mux_add_adapter(data->muxc, 0, i + 1, 0);
>> + if (ret) {
>> + dev_err(dev,
>> + "failed to register i2c mux channel %d\n",
>> + i + 1);
>> + goto err_mux_register;
>> + }
>> + }
>> + }
>> +
>> + psy_desc = devm_kmemdup(dev, &sbsm_default_psy_desc,
>> + sizeof(struct power_supply_desc),
>> + GFP_KERNEL);
>> + if (!psy_desc) {
>> + ret = -ENOMEM;
>> + goto err_psy;
>> + }
>> +
>> + psy_desc->name = devm_kasprintf(dev, GFP_KERNEL, "sbsm-%s",
>> + dev_name(&client->dev));
>> + if (!psy_desc->name) {
>> + ret = -ENOMEM;
>> + goto err_psy;
>> + }
>> +
>> + psy_cfg.drv_data = data;
>
> Please add:
>
> psy_cfg->of_node = dev->of_node;
>
>> + data->psy = devm_power_supply_register(dev, psy_desc, &psy_cfg);
>> + if (IS_ERR(data->psy)) {
>> + ret = PTR_ERR(data->psy);
>> + dev_err(dev, "failed to register power supply %s\n",
>> + psy_desc->name);
>> + goto err_psy;
>> + }
>> +
>> + dev_info(dev, "sbsm registered\n");
>> + return 0;
>> +
>> +err_psy:
>> +err_mux_register:
>> + i2c_mux_del_adapters(data->muxc);
>> +
>> +err_mux_alloc:
>> + return ret;
>> +}
>> +
>> +static int sbsm_remove(struct i2c_client *client)
>> +{
>> + struct sbsm_data *data = i2c_get_clientdata(client);
>> +
>> + i2c_mux_del_adapters(data->muxc);
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sbsm_ids[] = {
>> + { "sbs-manager", 0 },
>> + { "ltc1760", 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, sbsm_ids);
>> +
>> +static struct i2c_driver sbsm_driver = {
>> + .driver = {
>> + .name = "sbsm",
>> + .owner = THIS_MODULE,
>
> Please drop .owner assignment. It will be done by i2c-core.
> But you should add the following:
>
> .of_match_table = of_match_ptr(sbsm_dt_ids),
>
>> + },
>> + .probe = sbsm_probe,
>> + .remove = sbsm_remove,
>> + .id_table = sbsm_ids
>> +};
>> +module_i2c_driver(sbsm_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Karl-Heinz Schneider <karl-heinz@schneider-inet.de>");
>> +MODULE_DESCRIPTION("SBSM Smart Battery System Manager");
>
> -- Sebastian
>
--
Regards
Phil Reid
^ permalink raw reply
* Re: [PATCHv2 1/3] dt-bindings: display: Intel FPGA VIP drm driver Devicetree bindings
From: Ong, Hean Loong @ 2017-05-02 2:10 UTC (permalink / raw)
To: robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
dinguyen-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Loh, Tien Hock,
Vetter, Daniel, Ong@rob-hp-laptop,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170428183226.7u4sses4gzan6g5e@rob-hp-laptop>
On Fri, 2017-04-28 at 13:32 -0500, Rob Herring wrote:
> On Tue, Apr 25, 2017 at 10:06:44AM +0800, hean.loong.ong@intel.com
> wrote:
> >
> > From: "Ong, Hean Loong" <hean.loong.ong@intel.com>
> >
> > Device tree binding for Intel FPGA Video and Image
> > Processing Suite. The binding involved would be generated
> > from the Altera (Intel) Qsys system. The bindings would
> > set the max width, max height, buts per pixel and memory
> > port width. The device tree binding only supports the Intel
> > Arria10 devkit and its variants. Vendor name retained as
> > altr.
> >
> > Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> > ---
> > v2:
> > * Moved Device Tree bindings to
> > Documentation/devicetree/bindings/display/
> > * Added vendor name altr, to description
> > ---
> > .../devicetree/bindings/display/altr,vip-fb2.txt | 30
> > ++++++++++++++++++++++
> > 1 file changed, 30 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/altr,vip-
> > fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-
> > fb2.txt
> > new file mode 100644
> > index 0000000..bdffefb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> > @@ -0,0 +1,30 @@
> > +Intel Video and Image Processing(VIP) Frame Buffer II bindings
> > +
> > +Supported hardware: Arria 10 and above with display port IP
> > +
> > +The drm driver for the Arria 10 devkit would require the display
> > resolution
> Bindings describe h/w. DRM driver is a Linux term.
>
Noted.
> >
> > +and pixel information to be included as these values are generated
> > based
> > +on the FPGA design that drives the video connector attached to the
> > drm driver
> > +Information the FPGA video IP component can be acquired from
> > +https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/li
> > terature/ug/ug_vip.pdf
> > +
> > +Required properties:
> > +
> > +- compatible: "altr,vip-frame-buffer-2.0"
> > +- reg: Physical base address and length of the framebuffer
> > controller's
> > + registers.
> > +- altr,max-width: The width of the framebuffer in pixels.
> > +- altr,max-height: The height of the framebuffer in pixels.
> > +- altr,bits-per-symbol: only "8" is currently supported
> Supported in the driver or IP? The former isn't relevant to the
> binding.
> In the latter case, you don't need it if that's the only thing
> supported.
>
Since the device is an FPGA the values here are based on how the FPGA
HW design is created or programmed. The values here are the optimal
reference design proposed for the Intel Arria10 devkit.
However anyone that uses the Intel Arria10 devkit could create a device
that runs with a different resolution that has varying values but with
the condition that they need to fill these values accordingly with
Intel Quartus Programmer tools.
Once programmed the parameters could not be changed at runtime and the
HW rerence designs currently only support 1 type of resolution per
design.
Therefore the driver needs to support a hardware with varying
parameters programmed specifically into the FPGA.
> >
> > +- altr,mem-port-width = the bus width of the avalon master port on
> > the frame reader
> In bits or bytes?
>
> >
> > +
> > +Example:
> > +
> > + dp_0_frame_buf: vip@100000280 {
> > + compatible = "altr,vip-frame-buffer-2.0";
> > + reg = <0x00000001 0x00000280 0x00000040>;
> > + altr,max-width = <1280>;
> > + altr,max-height = <720>;
> > + altr,bits-per-symbol = <8>;
> > + altr,mem-port-width = <128>;
> > + };
^ permalink raw reply
* Re: [PATCH v4 1/2] dt-bindings: Document the STM32 QSPI bindings
From: Brian Norris @ 2017-05-02 1:09 UTC (permalink / raw)
To: Cyrille Pitchen
Cc: Ludovic Barre, Cyrille Pitchen, Marek Vasut, Boris Brezillon,
Alexandre Torgue, devicetree-u79uwXL29TY76Z2rM5mHXA,
Richard Weinberger, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
David Woodhouse
In-Reply-To: <b62272c9-027c-9c9b-42f0-1056d88aac7c-yU5RGvR974pGWvitb5QawA@public.gmane.org>
On Sun, Apr 16, 2017 at 06:53:06PM +0200, Cyrille Pitchen wrote:
> Hi all,
>
> Rob, is this version ok for you? If so, I can take it into the
> github/spi-nor tree.
I see this was missing from your pull request. Rob has since acked,
so...
Applied to l2-mtd.git
--
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 v4 2/2] mtd: spi-nor: add driver for STM32 quad spi flash controller
From: Brian Norris @ 2017-05-02 1:05 UTC (permalink / raw)
To: Ludovic Barre
Cc: Cyrille Pitchen, Marek Vasut, David Woodhouse, Boris Brezillon,
Richard Weinberger, Alexandre Torgue, Rob Herring,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Joachim Eastwood
In-Reply-To: <1492103757-22375-3-git-send-email-ludovic.Barre-qxv4g6HH51o@public.gmane.org>
Hi,
On Thu, Apr 13, 2017 at 07:15:57PM +0200, Ludovic Barre wrote:
> From: Ludovic Barre <ludovic.barre-qxv4g6HH51o@public.gmane.org>
>
> The quadspi is a specialized communication interface targeting single,
> dual or quad SPI Flash memories.
>
> It can operate in any of the following modes:
> -indirect mode: all the operations are performed using the quadspi
> registers
> -read memory-mapped mode: the external Flash memory is mapped to the
> microcontroller address space and is seen by the system as if it was
> an internal memory
>
> Signed-off-by: Ludovic Barre <ludovic.barre-qxv4g6HH51o@public.gmane.org>
> ---
> drivers/mtd/spi-nor/Kconfig | 7 +
> drivers/mtd/spi-nor/Makefile | 1 +
> drivers/mtd/spi-nor/stm32-quadspi.c | 694 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 702 insertions(+)
> create mode 100644 drivers/mtd/spi-nor/stm32-quadspi.c
>
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 7252087..bfdfb1e 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -106,4 +106,11 @@ config SPI_INTEL_SPI_PLATFORM
> To compile this driver as a module, choose M here: the module
> will be called intel-spi-platform.
>
> +config SPI_STM32_QUADSPI
> + tristate "STM32 Quad SPI controller"
> + depends on ARCH_STM32
> + help
> + This enables support for the STM32 Quad SPI controller.
> + We only connect the NOR to this controller.
> +
> endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 72238a7..285aab8 100644
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c
> @@ -0,0 +1,694 @@
...
> +static int stm32_qspi_flash_setup(struct stm32_qspi *qspi,
> + struct device_node *np)
> +{
> + u32 width, flash_read, presc, cs_num, max_rate = 0;
> + struct stm32_qspi_flash *flash;
> + struct mtd_info *mtd;
> + int ret;
> +
> + of_property_read_u32(np, "reg", &cs_num);
> + if (cs_num >= STM32_MAX_NORCHIP)
> + return -EINVAL;
> +
> + of_property_read_u32(np, "spi-max-frequency", &max_rate);
> + if (!max_rate)
> + return -EINVAL;
> +
> + presc = DIV_ROUND_UP(qspi->clk_rate, max_rate) - 1;
> +
> + if (of_property_read_u32(np, "spi-rx-bus-width", &width))
Can we move handling of this into spi-nor.c sometime? This is the 2nd
driver that wants this. And in this case, there's absolutely no
driver-specific handling for it. (For nxp-spifi.c, the hanling looks a
little wrong anyway -- the DT could have a larger bus width, but the
flash might only support a smaller. So the driver should gracefull
downgrade *after* we detect this, I think.)
Not a blocker for now, but just room for future work.
Brian
> + width = 1;
> +
> + if (width == 4)
> + flash_read = SPI_NOR_QUAD;
> + else if (width == 2)
> + flash_read = SPI_NOR_DUAL;
> + else if (width == 1)
> + flash_read = SPI_NOR_NORMAL;
> + else
> + return -EINVAL;
[...]
Brian
--
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 v4 1/3] drm/vc4: Turn the V3D clock on at runtime.
From: Rob Herring @ 2017-05-01 22:59 UTC (permalink / raw)
To: Eric Anholt
Cc: dri-devel, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20170428224223.21904-1-eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
On Fri, Apr 28, 2017 at 5:42 PM, Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org> wrote:
> For the Raspberry Pi's bindings, the power domain also implicitly
> turns on the clock and deasserts reset, but for the new Cygnus port we
> start representing the clock in the devicetree.
>
> v2: Document the clock-names property, check for -ENOENT for no clock
> in DT.
> v3: Drop NULL checks around clk calls which embed NULL checks.
> v4: Drop clk-names (feedback by Rob Herring)
>
> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> ---
> .../devicetree/bindings/display/brcm,bcm-vc4.txt | 3 +++
> drivers/gpu/drm/vc4/vc4_drv.h | 1 +
> drivers/gpu/drm/vc4/vc4_v3d.c | 31 +++++++++++++++++++++-
> 3 files changed, 34 insertions(+), 1 deletion(-)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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] of: undeclared variable when CONFIG_DEBUG_LOCK_ALLOC
From: Frank Rowand @ 2017-05-01 22:23 UTC (permalink / raw)
To: Rowand, Frank, Rob Herring
Cc: Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <ED290C750E1B324E972B0017C9D4BAB01FDD5370-t8YLG9SB9XQEb75RT/aEbJZPSYnAH24X@public.gmane.org>
On 05/01/17 14:20, Rowand, Frank wrote:
> On Monday, May 01, 2017 1:13 PM, Rob Herring [mailto:robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org] wrote:
>>
>> On Sun, Apr 30, 2017 at 3:00 PM, Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
>> wrote:
>>> An undeclared variable was used in a macro that evaluates to nothing
>>> when CONFIG_DEBUG_LOCK_ALLOC is not defined. Change to use the correct
>>> variable that does exist.
>>>
>>> ---
>>>
>>> reported by kbuild test robot on on robh/for-next
>>> https://lkml.org/lkml/2017/4/29/134
>>
>> That's a bit misleading because I've not applied the offending patch.
>> Please squash this.
>>
>> Rob
>
> Does "squash this" mean to redo the original path to include this fix?
^^^^ patch
>
> Thanks,
>
> Frank
>
--
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] of: undeclared variable when CONFIG_DEBUG_LOCK_ALLOC
From: Rowand, Frank @ 2017-05-01 21:20 UTC (permalink / raw)
To: Rob Herring
Cc: Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Frank Rowand
In-Reply-To: <CAL_JsqKMB4exBFTDNFAio87JeOF0sXjMBkueizgfoiG38Mb_8A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 797 bytes --]
On Monday, May 01, 2017 1:13 PM, Rob Herring [mailto:robh+dt@kernel.org] wrote:
>
> On Sun, Apr 30, 2017 at 3:00 PM, Frank Rowand <frank.rowand@sony.com>
> wrote:
>> An undeclared variable was used in a macro that evaluates to nothing
>> when CONFIG_DEBUG_LOCK_ALLOC is not defined. Change to use the correct
>> variable that does exist.
>>
>> ---
>>
>> reported by kbuild test robot on on robh/for-next
>> https://lkml.org/lkml/2017/4/29/134
>
> That's a bit misleading because I've not applied the offending patch.
> Please squash this.
>
> Rob
Does "squash this" mean to redo the original path to include this fix?
Thanks,
Frank
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i
^ permalink raw reply
* Re: [PATCH] of: undeclared variable when CONFIG_DEBUG_LOCK_ALLOC
From: Rob Herring @ 2017-05-01 20:13 UTC (permalink / raw)
To: Frank Rowand
Cc: Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1493582403-12291-1-git-send-email-frank.rowand-7U/KSKJipcs@public.gmane.org>
On Sun, Apr 30, 2017 at 3:00 PM, Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org> wrote:
> An undeclared variable was used in a macro that evaluates to nothing
> when CONFIG_DEBUG_LOCK_ALLOC is not defined. Change to use the correct
> variable that does exist.
>
> ---
>
> reported by kbuild test robot on on robh/for-next
> https://lkml.org/lkml/2017/4/29/134
That's a bit misleading because I've not applied the offending patch.
Please squash this.
Rob
--
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 v2 2/2] regulator: Allow for asymmetric settling times
From: Matthias Kaehlcke @ 2017-05-01 18:37 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Laxman Dewangan, Rob Herring,
Mark Rutland
Cc: linux-kernel, devicetree, Douglas Anderson, Brian Norris,
Matthias Kaehlcke
In-Reply-To: <20170501183715.35375-1-mka@chromium.org>
Some regulators have different settling times for voltage increases and
decreases. To avoid a time penalty on the faster transition allow for
different settings for up- and downward transitions.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- Don't remove settling_time property
- Split DT bindings doc into separate patch
drivers/regulator/core.c | 6 ++++++
drivers/regulator/of_regulator.c | 9 +++++++++
include/linux/regulator/machine.h | 6 ++++++
3 files changed, 21 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 811096b23143..7836f07431d8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2775,6 +2775,12 @@ static int _regulator_set_voltage_time(struct regulator_dev *rdev,
ramp_delay = rdev->desc->ramp_delay;
else if (rdev->constraints->settling_time)
return rdev->constraints->settling_time;
+ else if (rdev->constraints->settling_time_up &&
+ (new_uV > old_uV))
+ return rdev->constraints->settling_time_up;
+ else if (rdev->constraints->settling_time_down &&
+ (new_uV < old_uV))
+ return rdev->constraints->settling_time_down;
if (ramp_delay == 0) {
rdev_dbg(rdev, "ramp_delay not set\n");
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 09d677d5d3f0..b0a007c349ef 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -90,6 +90,15 @@ static void of_get_regulation_constraints(struct device_node *np,
if (!ret)
constraints->settling_time = pval;
+ ret = of_property_read_u32(np, "regulator-settling-time-up-us", &pval);
+ if (!ret)
+ constraints->settling_time_up = pval;
+
+ ret = of_property_read_u32(np, "regulator-settling-time-down-us",
+ &pval);
+ if (!ret)
+ constraints->settling_time_down = pval;
+
ret = of_property_read_u32(np, "regulator-enable-ramp-delay", &pval);
if (!ret)
constraints->enable_time = pval;
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 117699d1f7df..9cd4fef37203 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -110,6 +110,10 @@ struct regulator_state {
* @ramp_delay: Time to settle down after voltage change (unit: uV/us)
* @settling_time: Time to settle down after voltage change when voltage
* change is non-linear (unit: microseconds).
+ * @settling_time_up: Time to settle down after voltage increase when voltage
+ * change is non-linear (unit: microseconds).
+ * @settling_time_down : Time to settle down after voltage decrease when
+ * voltage change is non-linear (unit: microseconds).
* @active_discharge: Enable/disable active discharge. The enum
* regulator_active_discharge values are used for
* initialisation.
@@ -152,6 +156,8 @@ struct regulation_constraints {
unsigned int ramp_delay;
unsigned int settling_time;
+ unsigned int settling_time_up;
+ unsigned int settling_time_down;
unsigned int enable_time;
unsigned int active_discharge;
--
2.13.0.rc0.306.g87b477812d-goog
^ permalink raw reply related
* [PATCH v2 1/2] regulator: DT: Add properties for asymmetric settling times
From: Matthias Kaehlcke @ 2017-05-01 18:37 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Laxman Dewangan, Rob Herring,
Mark Rutland
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, Brian Norris,
Matthias Kaehlcke
Some regulators have different settling times for voltage increases and
decreases. Add DT properties to define separate settling times for up-
and downward voltage changes.
Signed-off-by: Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Moved DT binding doc to separate patch
- Don't remove settling_time property
Documentation/devicetree/bindings/regulator/regulator.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index d18edb075e1c..378f6dc8b8bd 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -24,6 +24,14 @@ Optional properties:
- regulator-settling-time-us: Settling time, in microseconds, for voltage
change if regulator have the constant time for any level voltage change.
This is useful when regulator have exponential voltage change.
+- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
+ increase if the regulator needs a constant time to settle after voltage
+ increases of any level. This is useful for regulators with exponential
+ voltage changes.
+- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
+ decrease if the regulator needs a constant time to settle after voltage
+ decreases of any level. This is useful for regulators with exponential
+ voltage changes.
- regulator-soft-start: Enable soft start so that voltage ramps slowly
- regulator-state-mem sub-root node for Suspend-to-RAM mode
: suspend to memory, the device goes to sleep, but all data stored in memory,
--
2.13.0.rc0.306.g87b477812d-goog
--
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
* Re: [PATCH v2 2/2] fpga: lattice machxo2: Add Lattice MachXO2 support
From: Moritz Fischer @ 2017-05-01 17:59 UTC (permalink / raw)
To: Paolo Pisati
Cc: Alan Tull, Rob Herring, Mark Rutland, Moritz Fischer,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fpga-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1493644512-14776-3-git-send-email-p.pisati-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 8011 bytes --]
Hi Paolo,
couple of nits inline, looks pretty good already.
On Mon, May 01, 2017 at 03:15:12PM +0200, Paolo Pisati wrote:
> This patch adds support to the FPGA manager for programming
> MachXO2 device’s internal flash memory, via slave SPI.
>
> Signed-off-by: Paolo Pisati <p.pisati-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/fpga/Kconfig | 7 ++
> drivers/fpga/Makefile | 1 +
> drivers/fpga/machxo2-spi.c | 214 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 222 insertions(+)
> create mode 100644 drivers/fpga/machxo2-spi.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index c81cb7d..cce135b 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -26,6 +26,13 @@ config FPGA_MGR_ICE40_SPI
> help
> FPGA manager driver support for Lattice iCE40 FPGAs over SPI.
>
> +config FPGA_MGR_MACHXO2_SPI
> + tristate "Lattice MachXO2 SPI"
> + depends on SPI
> + help
> + FPGA manager driver support for Lattice MachXO2 configuration
> + over slave SPI interface.
> +
> config FPGA_MGR_SOCFPGA
> tristate "Altera SOCFPGA FPGA Manager"
> depends on ARCH_SOCFPGA || COMPILE_TEST
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index c6f5d74..cdab1fe 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_FPGA) += fpga-mgr.o
>
> # FPGA Manager Drivers
> obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o
> +obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI) += machxo2-spi.o
> obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o
> obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o
> obj-$(CONFIG_FPGA_MGR_TS73XX) += ts73xx-fpga.o
> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
> new file mode 100644
> index 0000000..264ef63
> --- /dev/null
> +++ b/drivers/fpga/machxo2-spi.c
> @@ -0,0 +1,214 @@
> +/**
> + * Lattice MachXO2 Slave SPI Driver
> + *
> + * Copyright (C) 2017 Paolo Pisati <p.pisati-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * Manage Lattice FPGA firmware that is loaded over SPI using
> + * the slave serial configuration interface.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +/* MachXO2 Programming Guide - sysCONFIG Programming Commands */
> +
> +#define ISC_ENABLE 0x000008c6
> +#define ISC_ERASE 0x0000040e
> +#define ISC_PROGRAMDONE 0x0000005e
> +#define LSC_CHECKBUSY 0x000000f0
> +#define LSC_INITADDRESS 0x00000046
> +#define LSC_PROGINCRNV 0x01000070
> +#define LSC_REFRESH 0x00000079
> +
> +#define BUSYFLAG 0x80
Maybe BIT() macro?
> +
> +/*
> + * Max CCLK in Slave SPI mode according to 'MachXO2 Family Data
> + * Sheet' sysCONFIG Port Timing Specifications (3-36)
> + */
> +#define MACHXO2_MAX_SPEED 66000000
> +
> +#define MACHXO2_LOW_DELAY 5 /* us */
> +#define MACHXO2_HIGH_DELAY 200 /* us */
> +
> +#define MACHXO2_OP_SIZE sizeof(u32)
> +#define MACHXO2_PAGE_SIZE 16
> +#define MACHXO2_BUF_SIZE (MACHXO2_OP_SIZE + MACHXO2_PAGE_SIZE)
> +
> +static int wait_until_not_busy(struct spi_device *spi)
> +{
> + u8 rx;
> + u32 checkbusy = LSC_CHECKBUSY;
> + int ret;
> +
> + do {
> + ret = spi_write_then_read(spi, &checkbusy, MACHXO2_OP_SIZE,
> + &rx, sizeof(rx));
> + if (ret)
> + return ret;
> + } while (rx & BUSYFLAG);
> +
> + return 0;
> +}
> +
> +static enum fpga_mgr_states machxo2_spi_state(struct fpga_manager *mgr)
> +{
> + return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int machxo2_write_init(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + struct spi_device *spi = mgr->priv;
> + u32 enable = ISC_ENABLE;
> + u32 erase = ISC_ERASE;
> + u32 initaddr = LSC_INITADDRESS;
> + int ret;
> +
> + if ((info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + dev_err(&mgr->dev,
> + "Partial reconfiguration is not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + ret = spi_write(spi, &enable, MACHXO2_OP_SIZE);
> + if (ret)
> + goto fail;
> +
> + udelay(MACHXO2_LOW_DELAY);
Does this have to be a delay, or can this be a sleep?
Can you maybe make the whole thing a chain of transactions
making use of spi_message_init() and trans.delay_usecs?
> + ret = spi_write(spi, &erase, MACHXO2_OP_SIZE);
> + if (ret)
> + goto fail;
> +
> + ret = wait_until_not_busy(spi);
> + if (ret)
> + goto fail;
> +
> + ret = spi_write(spi, &initaddr, MACHXO2_OP_SIZE);
> + if (ret)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + dev_err(&mgr->dev, "Error during FPGA init.\n");
> + return ret;
> +}
> +
> +static int machxo2_write(struct fpga_manager *mgr, const char *buf,
> + size_t count)
> +{
> + struct spi_device *spi = mgr->priv;
> + u32 progincr = LSC_PROGINCRNV;
> + u8 payload[MACHXO2_BUF_SIZE];
> + int i, ret;
> +
> + if (count % MACHXO2_PAGE_SIZE != 0) {
> + dev_err(&mgr->dev, "Malformed payload.\n");
> + return -EINVAL;
> + }
> +
> + memcpy(payload, &progincr, MACHXO2_OP_SIZE);
> + for (i = 0; i < count; i += MACHXO2_PAGE_SIZE) {
> + memcpy(&payload[MACHXO2_OP_SIZE], &buf[i], MACHXO2_PAGE_SIZE);
> + ret = spi_write(spi, payload, MACHXO2_BUF_SIZE);
> + if (ret) {
> + dev_err(&mgr->dev, "Error loading the bitstream.\n");
> + return ret;
> + }
> + udelay(MACHXO2_HIGH_DELAY);
> + }
Same here.
> +
> + return 0;
> +}
> +
> +static int machxo2_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + struct spi_device *spi = mgr->priv;
> + u32 progdone = ISC_PROGRAMDONE;
> + u32 refresh = LSC_REFRESH;
> + int ret;
> +
> + ret = spi_write(spi, &progdone, MACHXO2_OP_SIZE);
> + if (ret)
> + goto fail;
> +
> + udelay(MACHXO2_HIGH_DELAY);
See above.
> + /* yep, LSC_REFRESH is 3 bytes long actually */
> + ret = spi_write(spi, &refresh, MACHXO2_OP_SIZE - 1);
> + if (ret)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + dev_err(&mgr->dev, "Refresh failed.\n");
> + return ret;
> +}
> +
> +static const struct fpga_manager_ops machxo2_ops = {
> + .state = machxo2_spi_state,
> + .write_init = machxo2_write_init,
> + .write = machxo2_write,
> + .write_complete = machxo2_write_complete,
> +};
> +
> +static int machxo2_spi_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> +
> + if (spi->max_speed_hz > MACHXO2_MAX_SPEED) {
> + dev_err(dev, "Speed is too high\n");
> + return -EINVAL;
> + }
> +
> + return fpga_mgr_register(dev, "Lattice MachXO2 SPI FPGA Manager",
> + &machxo2_ops, spi);
> +}
> +
> +static int machxo2_spi_remove(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> +
> + fpga_mgr_unregister(dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_match[] = {
> + { .compatible = "lattice,machxo2-slave-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_match);
> +
> +static const struct spi_device_id lattice_ids[] = {
> + { "machxo2-slave-spi", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(spi, lattice_ids);
> +
> +static struct spi_driver machxo2_spi_driver = {
> + .driver = {
> + .name = "machxo2-slave-spi",
> + .of_match_table = of_match_ptr(of_match),
> + },
> + .probe = machxo2_spi_probe,
> + .remove = machxo2_spi_remove,
> + .id_table = lattice_ids,
> +};
> +
> +module_spi_driver(machxo2_spi_driver)
> +
> +MODULE_AUTHOR("Paolo Pisati <p.pisati-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("Load Lattice FPGA firmware over SPI");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
Thanks,
Moritz
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox