* Re: [PATCH v3 1/3] dma: Support multiple interleaved frames with non-contiguous memory
From: Srikanth Thokala @ 2014-02-18 17:46 UTC (permalink / raw)
To: Jassi Brar
Cc: Srikanth Thokala, Williams, Dan J, Koul, Vinod, michal.simek,
Grant Likely, robh+dt, devicetree, Levente Kurusa,
Lars-Peter Clausen, lkml, dmaengine, Andy Shevchenko,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAJe_ZhdOmu3k9gbPdQzPteyzwsMA8OzWHdhHnXudbF=X7JMhVQ@mail.gmail.com>
On Tue, Feb 18, 2014 at 10:20 PM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 18 February 2014 16:58, Srikanth Thokala <sthokal@xilinx.com> wrote:
>> On Mon, Feb 17, 2014 at 3:27 PM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>>> On 15 February 2014 17:30, Srikanth Thokala <sthokal@xilinx.com> wrote:
>>>> The current implementation of interleaved DMA API support multiple
>>>> frames only when the memory is contiguous by incrementing src_start/
>>>> dst_start members of interleaved template.
>>>>
>>>> But, when the memory is non-contiguous it will restrict slave device
>>>> to not submit multiple frames in a batch. This patch handles this
>>>> issue by allowing the slave device to send array of interleaved dma
>>>> templates each having a different memory location.
>>>>
>>> How fragmented could be memory in your case? Is it inefficient to
>>> submit separate transfers for each segment/frame?
>>> It will help if you could give a typical example (chunk size and gap
>>> in bytes) of what you worry about.
>>
>> With scatter-gather engine feature in the hardware, submitting separate
>> transfers for each frame look inefficient. As an example, our DMA engine
>> supports up to 16 video frames, with each frame (a typical video frame
>> size) being contiguous in memory but frames are scattered into different
>> locations. We could not definitely submit frame by frame as it would be
>> software overhead (HW interrupting for each frame) resulting in video lags.
>>
> IIUIC, it is 30fps and one dma interrupt per frame ... it doesn't seem
> inefficient at all. Even poor-latency audio would generate a higher
> interrupt-rate. So the "inefficiency concern" doesn't seem valid to
> me.
>
> Not to mean we shouldn't strive to reduce the interrupt-rate further.
> Another option is to emulate the ring-buffer scheme of ALSA.... which
> should be possible since for a session of video playback the frame
> buffers' locations wouldn't change.
>
> Yet another option is to use the full potential of the
> interleaved-xfer api as such. It seems you confuse a 'video frame'
> with the interleaved-xfer api's 'frame'. They are different.
>
> Assuming your one video frame is F bytes long and Gk is the gap in
> bytes between end of frame [k] and start of frame [k+1] and Gi != Gj
> for i!=j
> In the context of interleaved-xfer api, you have just 1 Frame of 16
> chunks. Each chunk is Fbytes and the inter-chunk-gap(ICG) is Gk where
> 0<=k<15
> So for your use-case .....
> dma_interleaved_template.numf = 1 /* just 1 frame */
> dma_interleaved_template.frame_size = 16 /* containing 16 chunks */
> ...... //other parameters
>
> You have 3 options to choose from and all should work just as fine.
> Otherwise please state your problem in real numbers (video-frames'
> size, count & gap in bytes).
Initially I interpreted interleaved template the same. But, Lars corrected me
in the subsequent discussion and let me put it here briefly,
In the interleaved template, each frame represents a line of size denoted by
chunk.size and the stride by icg. 'numf' represent number of frames i.e.
number of lines.
In video frame context,
chunk.size -> hsize
chunk.icg -> stride
numf -> vsize
and frame_size is always 1 as it will have only one chunk in a line.
So, the API would not allow to pass multiple frames and we came up with a
resolution to pass an array of interleaved template structs to handle this.
Srikanth
>
> -Jassi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 1/2] mfd: palmas: support IRQ inversion at the board level
From: Stephen Warren @ 2014-02-18 17:42 UTC (permalink / raw)
To: Lee Jones
Cc: Samuel Ortiz, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, J Keerthy, Ian Lartey,
Stefan Agner, josephl-DDmLM1+adcrQT0dZR+AlfA,
ldewangan-DDmLM1+adcrQT0dZR+AlfA, Stephen Warren
In-Reply-To: <20140217092658.GC17875@lee--X1>
On 02/17/2014 02:26 AM, Lee Jones wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> Some boards or SoCs have an inverter between the PMIC IRQ output pin and
>> the IRQ controller input signal.
>>
>> The IRQ specifier in DT is meant to represent the IRQ flags at the input
>> to the IRQ controller.
>>
>> The Palmas HW's IRQ output has configurable polarity. The driver
>> currently selects the output polarity by querying the input polarity at
>> the IRQ controller. This works fine if the IRQ signal is routed directly
>> from the PMIC to the IRQ controller with no intervening logic. However,
>> if the signal is inverted between the two, this automatic polarity
>> selection gets the wrong answer.
>>
>> Add an additional optional DT and platform data parameter which indicates
>> that such an inversion occurs. If this option is enabled, the Palmas
>> driver will configure its IRQ output to the opposite polarity of the IRQ
>> controller's input.
>>
>> An alternative would have been to add a new non-optional DT parameter to
>> indicate the exact desired output polarity. However, this would have been
>> an incompatible change to the DT binding.
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> If this patch could be applied to its own branch (w/ signed tag) in the
>> MFD tree, that would great; then I can pull patch 1/2 into the Tegra tree
>> so that I can apply patch 2/2 to the Tegra tree. Thanks.
>> ---
>> Documentation/devicetree/bindings/mfd/palmas.txt | 6 ++++++
>> drivers/mfd/palmas.c | 4 ++++
>> include/linux/mfd/palmas.h | 1 +
>> 3 files changed, 11 insertions(+)
>
> For the core changes:
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Thanks. Since you ack'd this and are an MFD maintainer, was that an
indication that I should take this patch through the Tegra tree?
^ permalink raw reply
* Re: [PATCH 1/2] I2C: EMMA Mobile I2C master driver
From: Wolfram Sang @ 2014-02-18 17:38 UTC (permalink / raw)
To: Ian Molton
Cc: magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
rob.herring-bsGFqQB8/DxBDgjK7y7TUQ,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-sh-u79uwXL29TY76Z2rM5mHXA,
ben.dooks-4yDnlxn2s6sWdaTGBSpHTA
In-Reply-To: <1378226969-18722-2-git-send-email-ian.molton-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 8231 bytes --]
On Tue, Sep 03, 2013 at 05:49:29PM +0100, Ian Molton wrote:
> Add a driver for the EMMA mobile I2C block.
>
> The driver supports low and high-speed interrupt driven PIO transfers.
>
> Signed-off-by: Ian Molton <ian.molton-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -777,6 +777,16 @@ config I2C_RCAR
> This driver can also be built as a module. If so, the module
> will be called i2c-rcar.
>
> +config I2C_EM
> + tristate "EMMA Mobile series I2C adapter"
> + depends on I2C && HAVE_CLK
> + help
> + If you say yes to this option, support will be included for the
> + I2C interface on the Renesas Electronics EM/EV family of processors.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-em
> +
Please keep it sorted.
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-em.c
> @@ -0,0 +1,501 @@
> +/*
> + * Copyright 2013 Codethink Ltd.
> + * Parts Copyright 2010 Renesas Electronics Corporation
> + *
> + * 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, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA.
> + */
Skip the address please.
> +static int em_i2c_xfer(struct i2c_adapter *, struct i2c_msg[], int);
You can skip this forward declaration by reordering.
> +
> +struct em_i2c_device {
> + struct i2c_adapter adap;
> + wait_queue_head_t i2c_wait;
> + void __iomem *membase;
> + struct clk *clk;
> + struct clk *sclk;
> + int irq;
> + int flags;
> + int pending;
> + spinlock_t irq_lock;
> +};
> +
> +#define to_em_i2c(adap) (struct em_i2c_device *)i2c_get_adapdata(adap)
> +
> +static u32 em_i2c_func(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
Have you tried SMBUS_QUICK (via 'i2cdetect -q')?
> +
> +static struct i2c_algorithm em_i2c_algo = {
> + .master_xfer = em_i2c_xfer,
> + .smbus_xfer = NULL,
No need to specify the NULL.
> + .functionality = em_i2c_func,
> +};
> +
> +static int em_i2c_wait_for_event(struct em_i2c_device *i2c_dev, u16 *status)
> +{
> + int interrupted;
> +
> + do {
> + interrupted = wait_event_interruptible_timeout(
> + i2c_dev->i2c_wait, i2c_dev->pending,
> + i2c_dev->adap.timeout);
Have you tested signals extensively? It can be done right, yet it is
complex. Most drivers decide to skip the interruptible.
> +
> + if (i2c_dev->pending) {
> + spin_lock_irq(&i2c_dev->irq_lock);
> + i2c_dev->pending = 0;
> + spin_unlock_irq(&i2c_dev->irq_lock);
> + *status = readl(i2c_dev->membase + I2C_OFS_IICSE0);
> + return 0;
> + }
> +
> + } while (interrupted);
> +
> + *status = 0;
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int em_i2c_stop(struct em_i2c_device *i2c_dev)
> +{
> + u16 status;
> +
> + /* Send Stop condition */
> + writel((readl(i2c_dev->membase + I2C_OFS_IICC0) | I2C_BIT_SPT0 |
> + I2C_BIT_SPIE0), i2c_dev->membase + I2C_OFS_IICC0);
I'd think a em_set_bit() function would make the code more readable.
> +
> + /* Wait for stop condition */
> + em_i2c_wait_for_event(i2c_dev, &status);
> + /* FIXME - check status? */
What about the FIXME?
> +
> + if ((readl(i2c_dev->membase + I2C_OFS_IICSE0) & I2C_BIT_SPD0) != 0)
!= 0 is superfluous, same for == 1 later.
> + return 0;
> +
> + return -EBUSY;
> +}
> +
> + /* Send / receive data */
> + do {
> + /* Arbitration, Extension mode or Slave mode are errors*/
> + if (status & (I2C_BIT_EXC0 | I2C_BIT_COI0 | I2C_BIT_ALD0)
> + || !(status & I2C_BIT_MSTS0))
> + goto out_reset;
> +
> + if (!(status & I2C_BIT_TRC0)) { /* Read transaction */
> +
> + /* msg->flags is Write type */
> + if (!(msg->flags & I2C_M_RD))
> + goto out_reset;
> +
> + if (count == msg->len)
> + break;
> +
> + msg->buf[count++] =
> + readl(i2c_dev->membase + I2C_OFS_IIC0);
> +
> +
> + writel((readl(i2c_dev->membase + I2C_OFS_IICC0)
> + | I2C_BIT_WREL0),
> + i2c_dev->membase + I2C_OFS_IICC0);
> +
> + } else { /* Write transaction */
> +
> + /* msg->flags is Read type */
> + if ((msg->flags & I2C_M_RD))
> + goto out_reset;
> +
> + /* Recieved No ACK */
> + if (!(status & I2C_BIT_ACKD0)) {
> + em_i2c_stop(i2c_dev);
> + goto out;
> + }
> +
> + if (count == msg->len)
> + break;
> +
> + /* Write data */
> + writel(msg->buf[count++], i2c_dev->membase
> + + I2C_OFS_IIC0);
> + }
> +
> + /* Wait for R/W transaction */
> + if (em_i2c_wait_for_event(i2c_dev, &status))
> + goto out_reset;
> +
> + } while (1);
Have you tried using another loop than this endless do/while?
> +
> + if (stop)
> + em_i2c_stop(i2c_dev);
> +
> + return count;
> +
> +out_reset:
> + em_i2c_reset(adap);
> +out:
> + return -EREMOTEIO;
> +}
> +
> +static int em_i2c_wait_free(struct i2c_adapter *adap)
> +{
> + struct em_i2c_device *i2c_dev = to_em_i2c(adap);
> + int timeout = adap->timeout;
> + int status;
> +
> + /* wait until I2C bus free */
> + while ((readl(i2c_dev->membase + I2C_OFS_IICF0) & I2C_BIT_IICBSY)
> + && timeout--) {
> + schedule_timeout_uninterruptible(1);
> + }
Are you sure you want to loop with a 1 jiffy granularity?
> +
> + status = (timeout <= 0);
> +
> + if (status)
> + dev_info(&adap->dev, "I2C bus is busy\n");
> +
> + return status;
> +}
> +
> +static int em_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct em_i2c_device *i2c_dev = to_em_i2c(adap);
> + int ret = 0;
> + int i;
> +
> + em_i2c_enable_clock(i2c_dev);
> + em_i2c_reset(adap);
You reset the adapter before every transfer?
> +
> + /* Attempt to gain control of the adapter */
> + i = 0;
> + while (em_i2c_wait_free(adap)) {
> + switch (i) {
> + case 0:
> + if (!(readl(i2c_dev->membase + I2C_OFS_IICSE0)
> + & I2C_BIT_MSTS0)) {
> + /* Slave mode -> Error */
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + em_i2c_stop(i2c_dev);
> + break;
> + case 1:
> + em_i2c_reset(adap);
...and here again?
> + break;
> + case 2:
> + ret = -EREMOTEIO;
> + goto out;
> + }
> + i++;
> + }
> +
> + /* Send messages */
> + for (i = 0; i < num; i++) {
> + ret = __em_i2c_xfer(adap, &msgs[i], (i == (num - 1)));
> + if (ret < 0)
> + goto out;
> + }
> +
> + /* I2C transfer completed */
> + ret = i;
> +
> +out:
> + em_i2c_disable_clock(i2c_dev);
> +
> + return ret;
> +}
> +
> + spin_lock_init(&i2c_dev->irq_lock);
> +
> + if (of_find_property(pdev->dev.of_node, "high-speed", NULL))
> + i2c_dev->flags |= I2C_BIT_SMC0;
Please derive this from the standard property "clock-frequency" which
configures the bus speed for I2C.
> + of_i2c_register_devices(&i2c_dev->adap);
The core does this for you.
> +static int em_i2c_remove(struct platform_device *dev)
> +{
> + struct em_i2c_device *i2c_dev = platform_get_drvdata(dev);
> +
> + i2c_del_adapter(&i2c_dev->adap);
> +
> + clk_disable(i2c_dev->clk);
Aren't the clocks off already?
> + clk_unprepare(i2c_dev->clk);
> +
> + return 0;
> +}
> +
> +
> +MODULE_LICENSE("GPLv2");
GPL v2
> +MODULE_DESCRIPTION("EMEV2 I2C bus driver");
> +MODULE_AUTHOR("Ian Molton");
Email address?
> +
> --
> 1.7.10.4
>
> --
> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] clk: fixed-rate: use full DT node name
From: Stephen Warren @ 2014-02-18 17:34 UTC (permalink / raw)
To: Mark Rutland
Cc: Mike Turquette,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <20140218112323.GB6051-NuALmloUBlrZROr8t4l/smS4ubULX0JqMm0uRHvK7Nw@public.gmane.org>
On 02/18/2014 04:23 AM, Mark Rutland wrote:
> On Fri, Feb 14, 2014 at 04:43:04PM +0000, Stephen Warren wrote:
>> On 02/14/2014 03:35 AM, Mark Rutland wrote:
>>> On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote:
>>>> clk-fixed-rate currently names clocks according to a node's name without
>>>> the unit address. When faced with the legal and technically correct DT
>>>> structure below, this causes rgistration attempts for 3 clocks with the
>>>> same name, 2 of which fail.
>>>>
>>>> clocks {
>>>> compatible = "simple-bus";
>>>> #address-cells = <1>;
>>>> #size-cells = <0>;
>>>>
>>>> clk_mmc: clock@0 {
>>>> compatible = "fixed-clock";
>>>> reg = <0>;
>>>> ...
>>>> clk_i2c: clock@1 {
>>>> compatible = "fixed-clock";
>>>> reg = <1>;
>>>> ...
>>>> clk_spi: clock@2 {
>>>> compatible = "fixed-clock";
>>>> reg = <2>;
>>>> ...
>>>
>>> I'd argue that this case isn't valid.
>>
>> Well, it's very widely used, and was the result of numerous discussions
>> of how this kind of thing should be represented:-/
>
> Maybe we have to live with it then. :/
Great:-)
...
>>> It's just nonsensical; rename them to clock_{0,1,..} instead and get rid
>>> of the reg properties. Then they're named uniquely.
>>
>> That's not legal either. DT node names are supposed to represent the
>> type of device/object (i.e. just "clock"), not the identity of the
>> device/object (i.e. not include IDs etc.). Hence, the node name needs to
>> be "clock" for all of them, and the unit address must be used to
>> differentiate them.
>
> As far as I can see from ePAPR, the only requriement is:
>
> The node-name shall start with a lower or uppercase character and
> should describe the general class of device.
>
> IMO clock_1 describes the general class of device as well as clock@1,
> while also not filling a unexpected property with a meaningless value.
I believe section 2.2.2 "Generic Names Recommendation" is the source of
the rule that nodes should be named after the type of object rather than
the identity.
"""
The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming model. If appropriate, the
name should be one of the following choices:
* atm
* cache-controller
* compact-flash
* can
* cpu
...
"""
(I note e.g. "cpu" not "cpu-1", "cpu_2", etc.)
--
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/3] usb: chipidea: msm: Add device tree binding information
From: Ivan T. Ivanov @ 2014-02-18 17:27 UTC (permalink / raw)
To: Josh Cartwright
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Rob Landley, Greg Kroah-Hartman, David Brown, devicetree,
linux-doc, linux-kernel, linux-arm-msm
In-Reply-To: <20140218161303.GD31116@joshc.qualcomm.com>
Hi,
On Tue, 2014-02-18 at 10:13 -0600, Josh Cartwright wrote:
> On Tue, Feb 18, 2014 at 03:21:19PM +0200, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> >
> > Document device tree binding information as required by
> > the Qualcomm USB controller.
> >
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> > .../devicetree/bindings/usb/msm-hsusb.txt | 17 +++++++++++++++++
>
> Is this really the appropriate place to document this? It seems like
> this binding doc should be merged with the i.MX ci13xxx binding in a
> common ci13xxx doc.
>
This driver is a "glue" layer driver which control Qualcomm
specific logic around Chipidea IP core. It is supposed to
hold "non standard" Chipidea properties, but I suppose that
ci-hdrc-qcom.txt will be better name and will be similar to
i.MX chosen name.
What do you think?
Regards,
Ivan
^ permalink raw reply
* Re: [PATCH v2 2/3] usb: chipidea: msm: Add device tree support
From: Ivan T. Ivanov @ 2014-02-18 17:14 UTC (permalink / raw)
To: Josh Cartwright
Cc: Peter Chen, Grant Likely, Rob Herring, Greg Kroah-Hartman,
linux-usb, linux-kernel, devicetree, linux-arm-msm
In-Reply-To: <20140218140854.GC31116@joshc.qualcomm.com>
On Tue, 2014-02-18 at 08:08 -0600, Josh Cartwright wrote:
> Hey Ivan-
>
> Nit below.
>
> On Tue, Feb 18, 2014 at 03:21:20PM +0200, Ivan T. Ivanov wrote:
> >
> > +static struct of_device_id msm_ci_dt_match[] = {
>
> const?
>
Thanks, will do.
Regards,
Ivan
> > + { .compatible = "qcom,ci-hdrc", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, msm_ci_dt_match);
>
^ permalink raw reply
* Re: [PATCH v2 2/3] usb: chipidea: msm: Add device tree support
From: Ivan T. Ivanov @ 2014-02-18 17:14 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Peter Chen, Grant Likely, Rob Herring, Greg Kroah-Hartman,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <53039E2E.2030503-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Hi,
On Tue, 2014-02-18 at 20:53 +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 02/18/2014 04:21 PM, Ivan T. Ivanov wrote:
>
> > From: "Ivan T. Ivanov" <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
>
> > Allows controller to be specified via device tree.
> > Pass PHY phandle specified in DT to core driver.
>
> > Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> > ---
> > drivers/usb/chipidea/ci_hdrc_msm.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
>
> You also need to describe the binding you're creating in
> Documentation/devicetree/bindings/usb/<file>.txt.
Did you check "[PATCH v2 1/3]"?
Regards,
Ivan
>
> WBR, Sergei
>
--
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] net: add init-regs for of_phy support
From: Mark Rutland @ 2014-02-18 17:13 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Ben Dooks, Florian Fainelli, netdev, devicetree@vger.kernel.org,
Linux-sh list, David Miller
In-Reply-To: <53039193.4010500@cogentembedded.com>
On Tue, Feb 18, 2014 at 05:00:03PM +0000, Sergei Shtylyov wrote:
> Hello.
>
> On 02/18/2014 02:54 PM, Mark Rutland wrote:
>
> >> [snip]
>
> >>>>> - fixing up some design mistake?
> >>>>> - accounting for a specific board design?
>
> >>>> Kind of both. This was invented to defy the necessity of having platform
> >>>> fixup in the DT case (where there should be no board file to place it into).
> >>>> I have already described that platform fixup necessary on the Renesas
> >>>> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal
> >>>> on the SoC and the PHY reset sets the LED control bits to default 0 which
> >>>> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and
> >>>> cause ETH_LINK to bounce off/on after each packet.
>
> >>>>> In any case a PHY fixup would do the job for you.
>
> >>>> Not in any case. In case of DT we have no place for it, so should invent
> >>>> something involving DT.
>
> >>> How is DT different than any machine probing mechanism here? The way
> >>> to involve DT is to do the following:
>
> >>> if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy"))
> >>> phy_register_fixup(&foo_board_with_broken_micrel_phy);
>
> >> Oh yes, but now I have to do that for Linux, for $BSD, and for
> >> anything else I want to run on the device. I thought dt was meant
> >> to allow us to describe the hardware.
>
> > It does allow you to describe the hardware. Arbitrary register writes
> > aren't a description of the hardware, they're a sequence of instructions
> > that tells the OS nothing about the hardware and limit the ability of an
> > OS to do something different that might be better.
>
> > It's already the case that the OS has to have some knowledge of the
> > hardware that's implicit in a binding. We don't expect to have to
> > include bytecode to tell the OS how to poke a particular UART when it
> > can figure that out from a compatible string.
>
> >> If this is the case, let's just call this linuxtree and let everyone
> >> else get on with their own thing again.
>
> > This doesn't follow at all. Any OS needs to have some understanding of
> > the hardware it will try to poke. Describing a specific sequence of
> > writes in a DT is no more operating system independent than identifying
> > the hardware and expecting the OS to have a driver for it. The
> > requirements aren't any more suited to an individual OS in either case.
>
> >> See also comment below.
>
> >>> If your machine compatible string does not allow you to uniquely
> >>> identify your machine, this is a DT problem, as this should really be
> >>> the case. If you do not want to add this code to wherever this is
> >>> relevant in arch/arm/mach-shmobile/board-*.c, neither is
> >>> drivers/net/phy/phy_device.c this the place to add it.
>
> >> So where should it be added? If we keep piling stuff into board files
> >> in arch/arm.... then we're just back to the pre-dt case and going to
> >> keep getting shouted at.
>
> > The general trend has been to allocate new compatible strings for
> > components and let individual drivers handle this.
>
> > As far as I can see your case doesn't involve any components external to
> > the PHY, so should probably live in a PHY driver. The PHY can have a
>
> It does involve LEDs which should function in the way described by their
> labels, and it does involve SoC for which ETH_LINK signal should remain stable
> and not bouncing after each packet.
Ah, I see I misunderstood.
>
> > specific compatible string with the generic string as a fallback (if it
> > works to some degree without special poking).
>
> It can but that doesn't solve this issue in any way. The issue is board
> specific, not only PHY specific.
Sure. So additional properties are required.
>
> > I don't see that we need anything board-specific.
>
> Did you read the text substantially above this in this mail for more
> complete description of the issue? We're trying to emulate the PHY *platform*
> fixup here which didn't belong with the PHY driver.
Apologies, I was indeed mistaken.
Cheers,
Mark.
^ permalink raw reply
* Re: [PATCH 1/2] Revert "of: search the best compatible match first in __of_match_node()"
From: Grant Likely @ 2014-02-18 17:02 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Kevin Hao, Sebastian Hesselbarth, Stephen N Chivers, Rob Herring
In-Reply-To: <1392710250-29913-2-git-send-email-haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Tue, 18 Feb 2014 15:57:29 +0800, Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This reverts commit 06b29e76a74b2373e6f8b5a7938b3630b9ae98b2.
> As pointed out by Grant Likely, we should also take the type and name
> into account when searching the best compatible match. That means the
> match with compatible, type and name should be better than the match
> just with the same compatible string. So revert this and we will
> implement another method to find the best match entry.
>
> Signed-off-by: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/of/base.c | 43 +------------------------------------------
> 1 file changed, 1 insertion(+), 42 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 10b51106c854..ba195fbce4c6 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -730,49 +730,13 @@ out:
> }
> EXPORT_SYMBOL(of_find_node_with_property);
>
> -static const struct of_device_id *
> -of_match_compatible(const struct of_device_id *matches,
> - const struct device_node *node)
> -{
> - const char *cp;
> - int cplen, l;
> - const struct of_device_id *m;
> -
> - cp = __of_get_property(node, "compatible", &cplen);
> - while (cp && (cplen > 0)) {
> - m = matches;
> - while (m->name[0] || m->type[0] || m->compatible[0]) {
> - /* Only match for the entries without type and name */
> - if (m->name[0] || m->type[0] ||
> - of_compat_cmp(m->compatible, cp,
> - strlen(m->compatible)))
> - m++;
> - else
> - return m;
> - }
> -
> - /* Get node's next compatible string */
> - l = strlen(cp) + 1;
> - cp += l;
> - cplen -= l;
> - }
> -
> - return NULL;
> -}
> -
> static
> const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> const struct device_node *node)
> {
> - const struct of_device_id *m;
> -
> if (!matches)
> return NULL;
>
> - m = of_match_compatible(matches, node);
> - if (m)
> - return m;
> -
> while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
> int match = 1;
> if (matches->name[0])
> @@ -796,12 +760,7 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> * @matches: array of of device match structures to search in
> * @node: the of device structure to match against
> *
> - * Low level utility function used by device matching. We have two ways
> - * of matching:
> - * - Try to find the best compatible match by comparing each compatible
> - * string of device node with all the given matches respectively.
> - * - If the above method failed, then try to match the compatible by using
> - * __of_device_is_compatible() besides the match in type and name.
> + * Low level utility function used by device matching.
> */
> const struct of_device_id *of_match_node(const struct of_device_id *matches,
> const struct device_node *node)
> --
> 1.8.5.3
>
--
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_mdio: fix phy interrupt passing
From: Grant Likely @ 2014-02-18 17:02 UTC (permalink / raw)
To: Ben Dooks
Cc: linux-kernel, devicetree, linux-kernel, netdev, linux-sh,
sergei.shtylyov
In-Reply-To: <53032A97.8050201@codethink.co.uk>
On Tue, 18 Feb 2014 09:40:39 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 18/02/14 09:30, Grant Likely wrote:
> > On Mon, 17 Feb 2014 16:29:40 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> >> The of_mdiobus_register_phy() is not setting phy->irq this causing
> >> some drivers to incorrectly assume that the PHY does not have an
> >> IRQ associated with it or install an interrupt handler for the
> >> PHY.
> >>
> >> Simplify the code setting irq and set the phy->irq at the same
> >> time so that the case if mdio->irq is not NULL is easier to read.
> >>
> >> This fixes the issue:
> >> net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
> >>
> >> to the correct:
> >> net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI
> >>
> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >> ---
> >> drivers/of/of_mdio.c | 12 ++++++------
> >> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> >> index 875b7b6..7b3e7b0 100644
> >> --- a/drivers/of/of_mdio.c
> >> +++ b/drivers/of/of_mdio.c
> >> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
> >> {
> >> struct phy_device *phy;
> >> bool is_c45;
> >> - int rc, prev_irq;
> >> + int rc;
> >> u32 max_speed = 0;
> >>
> >> is_c45 = of_device_is_compatible(child,
> >> @@ -55,11 +55,11 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
> >> return 1;
> >>
> >> if (mdio->irq) {
> >> - prev_irq = mdio->irq[addr];
> >> - mdio->irq[addr] =
> >> - irq_of_parse_and_map(child, 0);
> >> - if (!mdio->irq[addr])
> >> - mdio->irq[addr] = prev_irq;
> >
> > I cannot for the life for me remeber why the code was structured that
> > way. Your change is better.
> >
> >> + rc = irq_of_parse_and_map(child, 0);
> >> + if (rc > 0) {
> >> + mdio->irq[addr] = rc;
> >> + phy->irq = rc;
> >> + }
> >> }
> >
> > The outer if is merely protecting against no irq array being allocated
> > for the bus. Would not the following be better:
> >
> > rc = irq_of_parse_and_map(child, 0);
> > if (rc > 0) {
> > phy->irq = rc;
> > if (mdio->irq)
> > mdio->irq[addr] = rc;
> > }
>
> Thanks, that makes sense, although we've both failed to work
> out if mdio->irq is set, and rc <= 0 case, so:
>
> rc = irq_of_parse_and_map(child, 0);
> if (rc > 0) {
> phy->irq = rc;
> if (mdio->irq)
> mdio->irq[addr] = rc;
> } else {
> if (mdio->irq)
> phy->irq = mdio->irq[addr];
> }
Is this actually a valid thing to do? I think the only time mdio->irq[]
is non-zero is when it is set to PHY_POLL. Is it valid to set phy->irq
to PHY_POLL? I didn't think it was.
g.
>
> I think that covers all cases. This does rely on mdio->irq
> being initialised to PHY_POLL if allocated.
>
> Once this is in, it may be easier then to not allocate
> mdio->irq for the OF case by default unless the driver
> registering the PHY knows it has the interrupt number(s)
> for the PHY already.
>
> --
> Ben Dooks http://www.codethink.co.uk/
> Senior Engineer Codethink - Providing Genius
^ permalink raw reply
* Re: [PATCH] net: add init-regs for of_phy support
From: Sergei Shtylyov @ 2014-02-18 17:00 UTC (permalink / raw)
To: Mark Rutland, Ben Dooks
Cc: Florian Fainelli, netdev, devicetree@vger.kernel.org,
Linux-sh list, David Miller
In-Reply-To: <20140218115443.GC6051@e106331-lin.cambridge.arm.com>
Hello.
On 02/18/2014 02:54 PM, Mark Rutland wrote:
>> [snip]
>>>>> - fixing up some design mistake?
>>>>> - accounting for a specific board design?
>>>> Kind of both. This was invented to defy the necessity of having platform
>>>> fixup in the DT case (where there should be no board file to place it into).
>>>> I have already described that platform fixup necessary on the Renesas
>>>> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK signal
>>>> on the SoC and the PHY reset sets the LED control bits to default 0 which
>>>> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity and
>>>> cause ETH_LINK to bounce off/on after each packet.
>>>>> In any case a PHY fixup would do the job for you.
>>>> Not in any case. In case of DT we have no place for it, so should invent
>>>> something involving DT.
>>> How is DT different than any machine probing mechanism here? The way
>>> to involve DT is to do the following:
>>> if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy"))
>>> phy_register_fixup(&foo_board_with_broken_micrel_phy);
>> Oh yes, but now I have to do that for Linux, for $BSD, and for
>> anything else I want to run on the device. I thought dt was meant
>> to allow us to describe the hardware.
> It does allow you to describe the hardware. Arbitrary register writes
> aren't a description of the hardware, they're a sequence of instructions
> that tells the OS nothing about the hardware and limit the ability of an
> OS to do something different that might be better.
> It's already the case that the OS has to have some knowledge of the
> hardware that's implicit in a binding. We don't expect to have to
> include bytecode to tell the OS how to poke a particular UART when it
> can figure that out from a compatible string.
>> If this is the case, let's just call this linuxtree and let everyone
>> else get on with their own thing again.
> This doesn't follow at all. Any OS needs to have some understanding of
> the hardware it will try to poke. Describing a specific sequence of
> writes in a DT is no more operating system independent than identifying
> the hardware and expecting the OS to have a driver for it. The
> requirements aren't any more suited to an individual OS in either case.
>> See also comment below.
>>> If your machine compatible string does not allow you to uniquely
>>> identify your machine, this is a DT problem, as this should really be
>>> the case. If you do not want to add this code to wherever this is
>>> relevant in arch/arm/mach-shmobile/board-*.c, neither is
>>> drivers/net/phy/phy_device.c this the place to add it.
>> So where should it be added? If we keep piling stuff into board files
>> in arch/arm.... then we're just back to the pre-dt case and going to
>> keep getting shouted at.
> The general trend has been to allocate new compatible strings for
> components and let individual drivers handle this.
> As far as I can see your case doesn't involve any components external to
> the PHY, so should probably live in a PHY driver. The PHY can have a
It does involve LEDs which should function in the way described by their
labels, and it does involve SoC for which ETH_LINK signal should remain stable
and not bouncing after each packet.
> specific compatible string with the generic string as a fallback (if it
> works to some degree without special poking).
It can but that doesn't solve this issue in any way. The issue is board
specific, not only PHY specific.
> I don't see that we need anything board-specific.
Did you read the text substantially above this in this mail for more
complete description of the issue? We're trying to emulate the PHY *platform*
fixup here which didn't belong with the PHY driver.
[...]
> Cheers,
> Mark.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v3 3/6] drivers: of: implement reserved-memory handling for dma
From: Grant Likely @ 2014-02-18 16:58 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, linaro-mm-sig, devicetree,
linux-doc
Cc: Marek Szyprowski, Kyungmin Park, Benjamin Herrenschmidt,
Arnd Bergmann, Michal Nazarewicz, Tomasz Figa, Sascha Hauer,
Laura Abbott, Rob Herring, Olof Johansson, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Tomasz Figa,
Kumar Gala, Nishanth Peethambaran, Marc, Josh Cartwright
In-Reply-To: <1392730681-14695-4-git-send-email-m.szyprowski@samsung.com>
On Tue, 18 Feb 2014 14:37:58 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> From: Josh Cartwright <joshc@codeaurora.org>
>
> Add support for handling 'shared-dma-pool' reserved-memory nodes using
> dma exclusive driver (dma_alloc_coherent()).
>
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/of/Kconfig | 7 ++++
> drivers/of/Makefile | 1 +
> drivers/of/of_reserved_mem_dma.c | 65 ++++++++++++++++++++++++++++++++++++++
I don't see any reason to have this separate from of_reserved_mem.c
> 3 files changed, 73 insertions(+)
> create mode 100644 drivers/of/of_reserved_mem_dma.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index f25931dfc6db..7f00b801bcd2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -80,4 +80,11 @@ config OF_RESERVED_MEM
> help
> Helpers to allow for reservation of memory regions
>
> +config OF_RESERVED_MEM_DMA
> + depends on OF_RESERVED_MEM
> + depends on HAVE_GENERIC_DMA_COHERENT
> + def_bool y
> + help
> + Helpers for reserving memory regions for DMA use
> +
> endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index ed9660adad77..6142227ca854 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI) += of_pci.o
> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> obj-$(CONFIG_OF_MTD) += of_mtd.o
> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> +obj-$(CONFIG_OF_RESERVED_MEM_DMA) += of_reserved_mem_dma.o
> diff --git a/drivers/of/of_reserved_mem_dma.c b/drivers/of/of_reserved_mem_dma.c
> new file mode 100644
> index 000000000000..a3e596d1091d
> --- /dev/null
> +++ b/drivers/of/of_reserved_mem_dma.c
> @@ -0,0 +1,65 @@
> +/*
> + * Device tree based initialization code for DMA reserved regions.
> + *
> + * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
> + * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + * Author: Marek Szyprowski <m.szyprowski@samsung.com>
> + * Author: Josh Cartwright <joshc@codeaurora.org>
> + *
> + * 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 optional) any later version of the license.
> + */
> +#include <linux/memblock.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_platform.h>
> +#include <linux/mm.h>
> +#include <linux/sizes.h>
> +#include <linux/mm_types.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/of_reserved_mem.h>
> +
> +static void rmem_dma_device_init(struct reserved_mem *rmem,
> + struct device *dev,
> + struct of_phandle_args *args)
> +{
> + dma_declare_coherent_memory(dev, rmem->base, rmem->base,
> + rmem->size, DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE);
> +}
> +
> +static void rmem_dma_device_release(struct reserved_mem *rmem,
> + struct device *dev)
> +{
> + dma_release_declared_memory(dev);
> +}
> +
> +static const struct reserved_mem_ops rmem_dma_ops = {
> + .device_init = rmem_dma_device_init,
> + .device_release = rmem_dma_device_release,
> +};
> +
> +static int __init rmem_dma_setup(struct reserved_mem *rmem,
> + unsigned long node,
> + const char *uname)
> +{
> + int err;
> +
> + if (of_get_flat_dt_prop(node, "reusable", NULL))
> + return -EINVAL;
> +
> + err = memblock_remove(rmem->base, rmem->size);
Isn't the memblock_remove() now handled by the core code? Or am I
mis-reading it?
> + if (err == 0) {
> + rmem->ops = &rmem_dma_ops;
> + pr_info("Reserved memory: created DMA memory pool at %pa, size %ld MiB\n",
> + &rmem->base, (unsigned long)rmem->size / SZ_1M);
> + } else {
> + pr_err("Reserved memory: unable to setup '%s' memory region for DMA.\n",
> + uname);
> + }
> + return err;
> +}
> +RESERVEDMEM_OF_DECLARE(dma, "shared-dma-pool", rmem_dma_setup);
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH v3 2/6] drivers: of: add initialization code for reserved memory
From: Grant Likely @ 2014-02-18 16:56 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, linaro-mm-sig, devicetree,
linux-doc
Cc: Marek Szyprowski, Kyungmin Park, Benjamin Herrenschmidt,
Arnd Bergmann, Michal Nazarewicz, Tomasz Figa, Sascha Hauer,
Laura Abbott, Rob Herring, Olof Johansson, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Tomasz Figa,
Kumar Gala, Nishanth Peethambaran, Marc, Josh Cartwright
In-Reply-To: <1392730681-14695-3-git-send-email-m.szyprowski@samsung.com>
On Tue, 18 Feb 2014 14:37:57 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> This patch adds device tree support for contiguous and reserved memory
> regions defined in device tree.
>
> Large memory blocks can be reliably reserved only during early boot.
> This must happen before the whole memory management subsystem is
> initialized, because we need to ensure that the given contiguous blocks
> are not yet allocated by kernel. Also it must happen before kernel
> mappings for the whole low memory are created, to ensure that there will
> be no mappings (for reserved blocks) or mapping with special properties
> can be created (for CMA blocks). This all happens before device tree
> structures are unflattened, so we need to get reserved memory layout
> directly from fdt.
>
> Later, those reserved memory regions are assigned to devices on each
> device structure initialization.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> [joshc: rework to implement new DT binding, provide mechanism for
> plugging in new reserved-memory node handlers via
> RESERVEDMEM_OF_DECLARE]
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> [mszyprow: added generic memory reservation code]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/of/Kconfig | 5 +
> drivers/of/Makefile | 1 +
> drivers/of/fdt.c | 2 +
> drivers/of/of_reserved_mem.c | 390 +++++++++++++++++++++++++++++++++++++
> drivers/of/platform.c | 7 +
> include/asm-generic/vmlinux.lds.h | 11 ++
> include/linux/of_reserved_mem.h | 65 +++++++
> 7 files changed, 481 insertions(+)
> create mode 100644 drivers/of/of_reserved_mem.c
> create mode 100644 include/linux/of_reserved_mem.h
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index c6973f101a3e..f25931dfc6db 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -75,4 +75,9 @@ config OF_MTD
> depends on MTD
> def_bool y
>
> +config OF_RESERVED_MEM
> + bool
> + help
> + Helpers to allow for reservation of memory regions
> +
> endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index efd05102c405..ed9660adad77 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
> obj-$(CONFIG_OF_PCI) += of_pci.o
> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> obj-$(CONFIG_OF_MTD) += of_mtd.o
> +obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
As mentioned previously, parts of this are absolutely non-optional and
cannot be compiled out. If a region is marked as reserved with this
binding, then the kernel must respect it. That part of the code must be
always configured in.
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 758b4f8b30b7..c205c84e51a1 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -15,6 +15,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_fdt.h>
> +#include <linux/of_reserved_mem.h>
> #include <linux/string.h>
> #include <linux/errno.h>
> #include <linux/slab.h>
> @@ -907,6 +908,7 @@ void __init unflatten_device_tree(void)
>
> /* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
> of_alias_scan(early_init_dt_alloc_memory_arch);
> + of_reserved_mem_scan();
> }
>
> /**
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> new file mode 100644
> index 000000000000..074d66e41da8
> --- /dev/null
> +++ b/drivers/of/of_reserved_mem.c
> @@ -0,0 +1,390 @@
> +/*
> + * Device tree based initialization code for reserved memory.
> + *
> + * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
> + * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + * Author: Marek Szyprowski <m.szyprowski@samsung.com>
> + * Author: Josh Cartwright <joshc@codeaurora.org>
> + *
> + * 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 optional) any later version of the license.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_platform.h>
> +#include <linux/mm.h>
> +#include <linux/sizes.h>
> +#include <linux/of_reserved_mem.h>
> +
> +#define MAX_RESERVED_REGIONS 16
> +static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> +static int reserved_mem_count;
> +
> +#if defined(CONFIG_HAVE_MEMBLOCK)
> +#include <linux/memblock.h>
> +int __init __weak
> +early_init_dt_reserve_memory_arch(phys_addr_t base, phys_addr_t size,
> + bool nomap)
> +{
> + if (memblock_is_region_reserved(base, size))
> + return -EBUSY;
> + if (nomap)
> + return memblock_remove(base, size);
> + return memblock_reserve(base, size);
> +}
> +
> +int __init __weak
> +early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, phys_addr_t align,
> + phys_addr_t start, phys_addr_t end,
> + bool nomap, phys_addr_t *res_base)
> +{
> + /*
> + * We use __memblock_alloc_base() since memblock_alloc_base() panic()s.
> + */
> + phys_addr_t base = __memblock_alloc_base(size, align, end);
> + if (!base)
> + return -ENOMEM;
Just realized this; this is actually a problem because an allocated
range may end up conflicting with a static range. Reservations must be
done in two passes. First pass should handle static ranges. Second pass
for doing dynamic allocations.
> +
> + if (base < start) {
> + memblock_free(base, size);
> + return -ENOMEM;
> + }
> +
> + *res_base = base;
> + if (nomap)
> + return memblock_remove(base, size);
> + return 0;
> +}
> +#else
> +int __init __weak
> +early_init_dt_reserve_memory_arch(phys_addr_t base, phys_addr_t size,
> + bool nomap)
> +{
> + pr_error("Reserved memory not supported, ignoring range 0x%llx - 0x%llx%s\n",
> + base, size, nomap ? " (nomap)" : "");
> + return -ENOSYS;
> +}
> +
> +int __init __weak
> +early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, phys_addr_t align,
> + phys_addr_t start, phys_addr_t end,
> + bool nomap, phys_addr_t *res_base)
> +{
> + pr_error("Reserved memory not supported, ignoring region 0x%llx%s\n",
> + size, nomap ? " (nomap)" : "");
> + return -ENOSYS;
> +}
> +#endif
> +
> +/**
> + * res_mem_reserve_reg() - reserve all memory described in 'reg' property
> + */
> +static int __init
> +res_mem_reserve_reg(unsigned long node, const char *uname, int nomap,
> + phys_addr_t *res_base, phys_addr_t *res_size)
> +{
> + int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> + phys_addr_t base, size;
> + unsigned long len;
> + __be32 *prop;
> +
> + prop = of_get_flat_dt_prop(node, "reg", &len);
> + if (!prop)
> + return -ENOENT;
> +
> + if (len && len % t_len != 0) {
> + pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
> + uname);
> + return -EINVAL;
> + }
> +
> + /* store base and size values from the first reg tuple */
> + *res_base = 0;
> + while (len > 0) {
> + base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> + size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +
> + if (base && size &&
> + early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
> + pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %ld MiB\n",
> + uname, &base, (unsigned long)size / SZ_1M);
> + else
> + pr_info("Reserved memory: failed to reserve memory for node '%s': base %pa, size %ld MiB\n",
> + uname, &base, (unsigned long)size / SZ_1M);
> +
> + len -= t_len;
> +
> + if (!(*res_base)) {
> + *res_base = base;
> + *res_size = size;
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + * res_mem_alloc_size() - allocate reserved memory described by 'size', 'align'
> + * and 'alloc-ranges' properties
> + */
> +static int __init
> +res_mem_alloc_size(unsigned long node, const char *uname, int nomap,
> + phys_addr_t *res_base, phys_addr_t *res_size)
> +{
> + int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> + phys_addr_t start = 0, end = 0;
> + phys_addr_t base = 0, align = 0, size;
> + unsigned long len;
> + __be32 *prop;
> + int ret;
> +
> + prop = of_get_flat_dt_prop(node, "size", &len);
> + if (!prop)
> + return -EINVAL;
> +
> + if (len != dt_root_size_cells * sizeof(__be32)) {
> + pr_err("Reserved memory: invalid size property in '%s' node.\n",
> + uname);
> + return -EINVAL;
> + }
> + size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +
> + prop = of_get_flat_dt_prop(node, "align", &len);
> + if (prop) {
> + if (len != dt_root_addr_cells * sizeof(__be32)) {
> + pr_err("Reserved memory: invalid align property in '%s' node.\n",
> + uname);
> + return -EINVAL;
> + }
> + align = dt_mem_next_cell(dt_root_addr_cells, &prop);
> + }
> +
> + prop = of_get_flat_dt_prop(node, "alloc-ranges", &len);
> + if (prop) {
> +
> + if (len % t_len != 0) {
> + pr_err("Reserved memory: invalid alloc-ranges property in '%s', skipping node.\n",
> + uname);
> + return -EINVAL;
> + }
> +
> + base = 0;
> +
> + while (len > 0) {
> + start = dt_mem_next_cell(dt_root_addr_cells, &prop);
> + end = start + dt_mem_next_cell(dt_root_size_cells,
> + &prop);
> +
> + ret = early_init_dt_alloc_reserved_memory_arch(size,
> + align, start, end, nomap, &base);
> + if (ret == 0) {
> + pr_debug("Reserved memory: allocated memory for '%s' node: base %pa, size %ld MiB\n",
> + uname, &base,
> + (unsigned long)size / SZ_1M);
> + break;
> + }
> + len -= t_len;
> + }
> +
> + } else {
> + ret = early_init_dt_alloc_reserved_memory_arch(size, align,
> + 0, 0, nomap, &base);
> + if (ret == 0)
> + pr_debug("Reserved memory: allocated memory for '%s' node: base %pa, size %ld MiB\n",
> + uname, &base, (unsigned long)size / SZ_1M);
> + }
> +
> + if (base == 0) {
> + pr_info("Reserved memory: failed to allocate memory for node '%s'\n",
> + uname);
> + return -ENOMEM;
> + }
> +
> + *res_base = base;
> + *res_size = size;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id __rmem_of_table_sentinel
> + __used __section(__reservedmem_of_table_end);
> +
> +/**
> + * res_mem_init_node() - call region specific reserved memory init code
> + */
> +static int __init
> +res_mem_init_node(unsigned long node, const char *uname, phys_addr_t base,
> + phys_addr_t size)
> +{
> + struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
> + extern const struct of_device_id __reservedmem_of_table[];
> + const struct of_device_id *i;
> +
> + if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) {
> + pr_err("Reserved memory: not enough space all defined regions.\n");
> + return -ENOSPC;
> + }
> +
> + rmem->base = base;
> + rmem->size = size;
> +
> + for (i = __reservedmem_of_table; i < &__rmem_of_table_sentinel; i++) {
> + reservedmem_of_init_fn initfn = i->data;
> + const char *compat = i->compatible;
> +
> + if (!of_flat_dt_is_compatible(node, compat))
> + continue;
> +
> + if (initfn(rmem, node, uname) == 0) {
> + pr_info("Reserved memory: initialized node %s, compatible id %s\n",
> + uname, compat);
> + rmem->name = uname;
> + reserved_mem_count++;
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
> +
> +/**
> + * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
> + */
> +static int __init
> +fdt_scan_reserved_mem(unsigned long node, const char *uname, int depth,
> + void *data)
> +{
> + phys_addr_t base, size;
> + const char *status;
> + int nomap;
> + int err;
> +
> + status = of_get_flat_dt_prop(node, "status", NULL);
> + if (status && strcmp(status, "okay") != 0 && strcmp(status, "ok") != 0)
> + return 0;
> +
> + nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> +
> + err = res_mem_reserve_reg(node, uname, nomap, &base, &size);
> + if (err == -ENOENT)
> + err = res_mem_alloc_size(node, uname, nomap, &base, &size);
> + if (err)
> + goto end;
> +
> + res_mem_init_node(node, uname, base, size);
> +end:
> + /* scan next node */
> + return 0;
> +}
> +
> +/**
> + * early_init_dt_scan_reserved_mem() - create reserved memory regions
> + *
> + * This function grabs memory from early allocator for device exclusive use
> + * defined in device tree structures. It should be called by arch specific code
> + * once the early allocator (i.e. memblock) has been fully activated.
> + */
> +void __init early_init_dt_scan_reserved_mem(void)
> +{
> + of_scan_flat_dt_by_path("/reserved-memory", fdt_scan_reserved_mem,
> + NULL);
> +}
Just about everything above this point must be moved to fdt.c. Processing the
static reserved regions must be non-optional code. You can factor out
the table of reserved regions if you like, but I want the core
functionality directly in fdt.c.
The dynamic allocation code can be optional, but that is because it
describes dynamic regions that have no possibility of hardware already
using them.
> +
> +/**
> + * of_reserved_mem_scan() - scan and create structures required by reserved
> + * memory regions
> + *
> + * This function creates all structures required by reserved memory regions
> + * management code. It should be called by common code once the device tree
> + * has been unflattened.
> + */
> +void __init of_reserved_mem_scan(void)
> +{
> + struct device_node *root, *np;
> +
> + root = of_find_node_by_path("/reserved-memory");
> +
> + if (of_n_addr_cells(root) != dt_root_addr_cells ||
> + of_n_size_cells(root) != dt_root_size_cells)
> + panic("Unsupported address or size cells for /reserved-memory node\n");
> +
> + for (np = NULL;;) {
> + const char *name;
> + int i;
> +
> + np = of_get_next_available_child(root, np);
> + if (!np)
> + break;
> +
> + name = kbasename(np->full_name);
> + for (i = 0; i < reserved_mem_count; i++)
> + if (strcmp(name, reserved_mem[i].name) == 0)
> + reserved_mem[i].node = np;
I've already commented on the above. kbasename is not a safe match.
> + }
> +}
> +
> +static inline struct reserved_mem *find_rmem(struct device_node *phandle)
> +{
> + unsigned int i;
> + for (i = 0; i < reserved_mem_count; i++)
> + if (reserved_mem[i].node == phandle)
> + return &reserved_mem[i];
> + return NULL;
> +}
> +
> +/**
> + * of_reserved_mem_device_init() - assign reserved memory region to given device
> + *
> + * This function assign memory region pointed by "memory-region" device tree
> + * property to the given device.
> + */
> +void of_reserved_mem_device_init(struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct reserved_mem *rmem;
> + struct of_phandle_args s;
> + unsigned int i;
> +
> + for (i = 0; of_parse_phandle_with_args(np, "memory-region",
> + "#memory-region-cells", i, &s) == 0; i++) {
> +
> + rmem = find_rmem(s.np);
> + if (!rmem || !rmem->ops || !rmem->ops->device_init) {
> + of_node_put(s.np);
> + continue;
> + }
> +
> + rmem->ops->device_init(rmem, dev, &s);
> + dev_info(dev, "assigned reserved memory node %s\n",
> + rmem->name);
> + of_node_put(s.np);
> + break;
> + }
> +}
> +
> +/**
> + * of_reserved_mem_device_release() - release reserved memory device structures
> + *
> + * This function releases structures allocated for memory region handling for
> + * the given device.
> + */
> +void of_reserved_mem_device_release(struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct reserved_mem *rmem;
> + struct of_phandle_args s;
> + unsigned int i;
> +
> + for (i = 0; of_parse_phandle_with_args(np, "memory-region",
> + "#memory-region-cells", i, &s) == 0; i++) {
> +
> + rmem = find_rmem(s.np);
> + if (rmem && rmem->ops && rmem->ops->device_release)
> + rmem->ops->device_release(rmem, dev);
> +
> + of_node_put(s.np);
> + }
> +}
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1daebefa..3df0b1826e8b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -21,6 +21,7 @@
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> #include <linux/of_platform.h>
> +#include <linux/of_reserved_mem.h>
> #include <linux/platform_device.h>
>
> const struct of_device_id of_default_bus_match_table[] = {
> @@ -220,6 +221,8 @@ static struct platform_device *of_platform_device_create_pdata(
> dev->dev.bus = &platform_bus_type;
> dev->dev.platform_data = platform_data;
>
> + of_reserved_mem_device_init(&dev->dev);
> +
> /* We do not fill the DMA ops for platform devices by default.
> * This is currently the responsibility of the platform code
> * to do such, possibly using a device notifier
> @@ -227,6 +230,7 @@ static struct platform_device *of_platform_device_create_pdata(
>
> if (of_device_add(dev) != 0) {
> platform_device_put(dev);
> + of_reserved_mem_device_release(&dev->dev);
> return NULL;
> }
>
> @@ -282,6 +286,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> else
> of_device_make_bus_id(&dev->dev);
>
> + of_reserved_mem_device_init(&dev->dev);
> +
> /* Allow the HW Peripheral ID to be overridden */
> prop = of_get_property(node, "arm,primecell-periphid", NULL);
> if (prop)
> @@ -308,6 +314,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
> return dev;
>
> err_free:
> + of_reserved_mem_device_release(&dev->dev);
> amba_device_put(dev);
> return NULL;
> }
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index bc2121fa9132..f10f64fcc815 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -167,6 +167,16 @@
> #define CLK_OF_TABLES()
> #endif
>
> +#ifdef CONFIG_OF_RESERVED_MEM
> +#define RESERVEDMEM_OF_TABLES() \
> + . = ALIGN(8); \
> + VMLINUX_SYMBOL(__reservedmem_of_table) = .; \
> + *(__reservedmem_of_table) \
> + *(__reservedmem_of_table_end)
> +#else
> +#define RESERVEDMEM_OF_TABLES()
> +#endif
> +
> #define KERNEL_DTB() \
> STRUCT_ALIGN(); \
> VMLINUX_SYMBOL(__dtb_start) = .; \
> @@ -490,6 +500,7 @@
> TRACE_SYSCALLS() \
> MEM_DISCARD(init.rodata) \
> CLK_OF_TABLES() \
> + RESERVEDMEM_OF_TABLES() \
> CLKSRC_OF_TABLES() \
> KERNEL_DTB() \
> IRQCHIP_OF_MATCH_TABLE()
> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
> new file mode 100644
> index 000000000000..39a4fb17a5ea
> --- /dev/null
> +++ b/include/linux/of_reserved_mem.h
> @@ -0,0 +1,65 @@
> +#ifndef __OF_RESERVED_MEM_H
> +#define __OF_RESERVED_MEM_H
> +
> +struct cma;
> +struct platform_device;
> +struct of_phandle_args;
> +struct reserved_mem_ops;
> +
> +struct reserved_mem {
> + const char *name;
> + struct device_node *node;
> + const struct reserved_mem_ops *ops;
> + phys_addr_t base;
> + phys_addr_t size;
> + void *priv;
> +};
> +
> +struct reserved_mem_ops {
> + void (*device_init)(struct reserved_mem *rmem,
> + struct device *dev,
> + struct of_phandle_args *args);
> + void (*device_release)(struct reserved_mem *rmem,
> + struct device *dev);
> +};
> +
> +typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem,
> + unsigned long node, const char *uname);
> +
> +#ifdef CONFIG_OF_RESERVED_MEM
> +void of_reserved_mem_device_init(struct device *dev);
> +void of_reserved_mem_device_release(struct device *dev);
> +void early_init_dt_scan_reserved_mem(void);
> +void of_reserved_mem_scan(void);
> +
> +int of_parse_flat_dt_reg(unsigned long node, const char *uname,
> + phys_addr_t *base, phys_addr_t *size);
> +int of_parse_flat_dt_size(unsigned long node, const char *uname,
> + phys_addr_t *size);
> +
> +#define RESERVEDMEM_OF_DECLARE(name, compat, init) \
> + static const struct of_device_id __reservedmem_of_table_##name \
> + __used __section(__reservedmem_of_table) \
> + = { .compatible = compat, \
> + .data = (init == (reservedmem_of_init_fn)NULL) ? \
> + init : init }
> +
> +#else
> +static inline void of_reserved_mem_device_init(struct device *dev) { }
> +
> +static inline
> +void of_reserved_mem_device_release(struct device *pdev) { }
> +
> +static inline void early_init_dt_scan_reserved_mem(void) { }
> +static inline void of_reserved_mem_scan(void) { }
> +
> +#define RESERVEDMEM_OF_DECLARE(name, compat, init) \
> + static const struct of_device_id __reservedmem_of_table_##name \
> + __attribute__((unused)) \
> + = { .compatible = compat, \
> + .data = (init == (reservedmem_of_init_fn)NULL) ? \
> + init : init }
> +
> +#endif
> +
> +#endif /* __OF_RESERVED_MEM_H */
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH v3 1/3] dma: Support multiple interleaved frames with non-contiguous memory
From: Jassi Brar @ 2014-02-18 16:50 UTC (permalink / raw)
To: Srikanth Thokala
Cc: Williams, Dan J, Koul, Vinod, michal.simek, Grant Likely, robh+dt,
devicetree, Levente Kurusa, Lars-Peter Clausen, lkml, dmaengine,
Andy Shevchenko, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CA+mB=1KdZZpTmpA0Xp_c4mhgFbjRZ6urYrCwdvoTCmcugQMDUA@mail.gmail.com>
On 18 February 2014 16:58, Srikanth Thokala <sthokal@xilinx.com> wrote:
> On Mon, Feb 17, 2014 at 3:27 PM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>> On 15 February 2014 17:30, Srikanth Thokala <sthokal@xilinx.com> wrote:
>>> The current implementation of interleaved DMA API support multiple
>>> frames only when the memory is contiguous by incrementing src_start/
>>> dst_start members of interleaved template.
>>>
>>> But, when the memory is non-contiguous it will restrict slave device
>>> to not submit multiple frames in a batch. This patch handles this
>>> issue by allowing the slave device to send array of interleaved dma
>>> templates each having a different memory location.
>>>
>> How fragmented could be memory in your case? Is it inefficient to
>> submit separate transfers for each segment/frame?
>> It will help if you could give a typical example (chunk size and gap
>> in bytes) of what you worry about.
>
> With scatter-gather engine feature in the hardware, submitting separate
> transfers for each frame look inefficient. As an example, our DMA engine
> supports up to 16 video frames, with each frame (a typical video frame
> size) being contiguous in memory but frames are scattered into different
> locations. We could not definitely submit frame by frame as it would be
> software overhead (HW interrupting for each frame) resulting in video lags.
>
IIUIC, it is 30fps and one dma interrupt per frame ... it doesn't seem
inefficient at all. Even poor-latency audio would generate a higher
interrupt-rate. So the "inefficiency concern" doesn't seem valid to
me.
Not to mean we shouldn't strive to reduce the interrupt-rate further.
Another option is to emulate the ring-buffer scheme of ALSA.... which
should be possible since for a session of video playback the frame
buffers' locations wouldn't change.
Yet another option is to use the full potential of the
interleaved-xfer api as such. It seems you confuse a 'video frame'
with the interleaved-xfer api's 'frame'. They are different.
Assuming your one video frame is F bytes long and Gk is the gap in
bytes between end of frame [k] and start of frame [k+1] and Gi != Gj
for i!=j
In the context of interleaved-xfer api, you have just 1 Frame of 16
chunks. Each chunk is Fbytes and the inter-chunk-gap(ICG) is Gk where
0<=k<15
So for your use-case .....
dma_interleaved_template.numf = 1 /* just 1 frame */
dma_interleaved_template.frame_size = 16 /* containing 16 chunks */
...... //other parameters
You have 3 options to choose from and all should work just as fine.
Otherwise please state your problem in real numbers (video-frames'
size, count & gap in bytes).
-Jassi
^ permalink raw reply
* Re: [RFC PATCH] [media]: of: move graph helpers from drivers/media/v4l2-core to drivers/of
From: Grant Likely @ 2014-02-18 16:26 UTC (permalink / raw)
To: Sascha Hauer
Cc: Rob Herring, Philipp Zabel, Russell King - ARM Linux,
Mauro Carvalho Chehab, Rob Herring, Sylwester Nawrocki,
Laurent Pinchart, Tomi Valkeinen, Kyungmin Park,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, Philipp Zabel
In-Reply-To: <20140218070624.GP17250@pengutronix.de>
On Tue, 18 Feb 2014 08:06:24 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> Hi Grant,
>
> On Mon, Feb 17, 2014 at 06:14:51PM +0000, Grant Likely wrote:
> > On Tue, 11 Feb 2014 07:56:33 -0600, Rob Herring <robherring2@gmail.com> wrote:
> > > On Tue, Feb 11, 2014 at 5:45 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > From: Philipp Zabel <philipp.zabel@gmail.com>
> > > >
> > > > This patch moves the parsing helpers used to parse connected graphs
> > > > in the device tree, like the video interface bindings documented in
> > > > Documentation/devicetree/bindings/media/video-interfaces.txt, from
> > > > drivers/media/v4l2-core to drivers/of.
> > >
> > > This is the opposite direction things have been moving...
> > >
> > > > This allows to reuse the same parser code from outside the V4L2 framework,
> > > > most importantly from display drivers. There have been patches that duplicate
> > > > the code (and I am going to send one of my own), such as
> > > > http://lists.freedesktop.org/archives/dri-devel/2013-August/043308.html
> > > > and others that parse the same binding in a different way:
> > > > https://www.mail-archive.com/linux-omap@vger.kernel.org/msg100761.html
> > > >
> > > > I think that all common video interface parsing helpers should be moved to a
> > > > single place, outside of the specific subsystems, so that it can be reused
> > > > by all drivers.
> > >
> > > Perhaps that should be done rather than moving to drivers/of now and
> > > then again to somewhere else.
> >
> > This is just parsing helpers though, isn't it? I have no problem pulling
> > helper functions into drivers/of if they are usable by multiple
> > subsystems. I don't really understand the model being used though. I
> > would appreciate a description of the usage model for these functions
> > for poor folks like me who can't keep track of what is going on in
> > subsystems.
>
> You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
Okay, I think I'm okay with moving the helpers, but I will make one
requirement. I would like to have a short binding document describing
the pattern being used. The document above talks a lot about video
specific issues, but the helpers appear to be specifically about the
tree walking properties of the API.
g.
^ permalink raw reply
* Re: [PATCH v2] of_mdio: fix phy interrupt passing
From: Grant Likely @ 2014-02-18 16:15 UTC (permalink / raw)
Cc: linux-kernel, devicetree, linux-kernel, netdev, linux-sh,
sergei.shtylyov, Ben Dooks
In-Reply-To: <1392725818-558-1-git-send-email-ben.dooks@codethink.co.uk>
On Tue, 18 Feb 2014 12:16:58 +0000, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> The of_mdiobus_register_phy() is not setting phy->irq thus causing
> some drivers to incorrectly assume that the PHY does not have an
> IRQ associated with it. Not only do some drivers report no IRQ
> they do not install an interrupt handler for the PHY.
>
> Simplify the code setting irq and set the phy->irq at the same
> time so that we cover the following issues, which should cover
> all the cases the code will find:
>
> - Set phy->irq if node has irq property and mdio->irq is NULL
> - Set phy->irq if node has no irq and mdio->irq is not NULL
> - Leave phy->irq as PHY_POLL default if none of the above
>
> This fixes the issue:
> net eth0: attached PHY 1 (IRQ -1) to driver Micrel KSZ8041RNLI
>
> to the correct:
> net eth0: attached PHY 1 (IRQ 416) to driver Micrel KSZ8041RNLI
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Looks okay to me
Reviewed-by: Grant Likely <grant.likely@linaro.org>
>
> ---
> Since v1:
> - Updated phy->irq setting code
> - Deal with issue if mdio->irq array NULL
>
> Notes:
>
> It was discussed if this should be two patches, but we end up
> making enough changes in #2 the same area as #1 means that we
> basically rewrite #1 in #2. So keep it as 1 patch.
> ---
> drivers/of/of_mdio.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 875b7b6..46d95fc 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -44,7 +44,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
> {
> struct phy_device *phy;
> bool is_c45;
> - int rc, prev_irq;
> + int rc;
> u32 max_speed = 0;
>
> is_c45 = of_device_is_compatible(child,
> @@ -54,12 +54,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
> if (!phy || IS_ERR(phy))
> return 1;
>
> - if (mdio->irq) {
> - prev_irq = mdio->irq[addr];
> - mdio->irq[addr] =
> - irq_of_parse_and_map(child, 0);
> - if (!mdio->irq[addr])
> - mdio->irq[addr] = prev_irq;
> + rc = irq_of_parse_and_map(child, 0);
> + if (rc > 0) {
> + phy->irq = rc;
> + if (mdio->irq)
> + mdio->irq[addr] = rc;
> + } else {
> + if (mdio->irq)
> + phy->irq = mdio->irq[addr];
> }
>
> /* Associate the OF node with the device structure so it
> --
> 1.8.5.3
>
^ permalink raw reply
* Re: [PATCH v2 1/3] usb: chipidea: msm: Add device tree binding information
From: Josh Cartwright @ 2014-02-18 16:13 UTC (permalink / raw)
To: Ivan T. Ivanov
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Rob Landley, Greg Kroah-Hartman, David Brown, devicetree,
linux-doc, linux-kernel, linux-arm-msm
In-Reply-To: <1392729681-21022-2-git-send-email-iivanov@mm-sol.com>
On Tue, Feb 18, 2014 at 03:21:19PM +0200, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>
> Document device tree binding information as required by
> the Qualcomm USB controller.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
> .../devicetree/bindings/usb/msm-hsusb.txt | 17 +++++++++++++++++
Is this really the appropriate place to document this? It seems like
this binding doc should be merged with the i.MX ci13xxx binding in a
common ci13xxx doc.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v7 03/12] mfd: omap-usb-host: Use clock names as per function for reference clocks
From: Lee Jones @ 2014-02-18 16:07 UTC (permalink / raw)
To: Roger Quadros
Cc: tony, bcousson, balbi, nm, khilman, linux-omap, linux-arm-kernel,
linux-kernel, devicetree, linux-usb, Samuel Ortiz
In-Reply-To: <53037C2B.6070701@ti.com>
> >>>> Use a meaningful name for the reference clocks so that it indicates the function.
> >>>>
> >>>> CC: Lee Jones <lee.jones@linaro.org>
> >>>> CC: Samuel Ortiz <sameo@linux.intel.com>
> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>>> ---
> >>>> drivers/mfd/omap-usb-host.c | 6 +++---
> >>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
> >>>> index 60a3bed..ce620a8 100644
> >>>> --- a/drivers/mfd/omap-usb-host.c
> >>>> +++ b/drivers/mfd/omap-usb-host.c
> >>>> @@ -714,21 +714,21 @@ static int usbhs_omap_probe(struct platform_device *pdev)
> >>>> goto err_mem;
> >>>> }
> >>>>
> >>>> - omap->xclk60mhsp1_ck = devm_clk_get(dev, "xclk60mhsp1_ck");
> >>>> + omap->xclk60mhsp1_ck = devm_clk_get(dev, "refclk_60m_ext_p1");
> >>>
> >>> You can't do that. These changes will have to be in the same patch as
> >>> the core change i.e. where they are initialised.
> >>
> >> I'm not touching them anywhere in this series. When core changes are you
> >> referring to?
> >
> > The ones in:
> > arch/arm/mach-omap2/cclock3xxx_data.c
>
> OK, right. They are now no longer needed so I'll get rid of them.
> In fact that change should either be in a separate patch or combined with PATCH 2
> in this series. What do you suggest?
A separate patch will do fine.
> >>>> if (IS_ERR(omap->xclk60mhsp1_ck)) {
> >>>> ret = PTR_ERR(omap->xclk60mhsp1_ck);
> >>>> dev_err(dev, "xclk60mhsp1_ck failed error:%d\n", ret);
> >>>> goto err_mem;
> >>>> }
> >>>>
> >>>> - omap->xclk60mhsp2_ck = devm_clk_get(dev, "xclk60mhsp2_ck");
> >>>> + omap->xclk60mhsp2_ck = devm_clk_get(dev, "refclk_60m_ext_p2");
> >>>> if (IS_ERR(omap->xclk60mhsp2_ck)) {
> >>>> ret = PTR_ERR(omap->xclk60mhsp2_ck);
> >>>> dev_err(dev, "xclk60mhsp2_ck failed error:%d\n", ret);
> >>>> goto err_mem;
> >>>> }
> >>>>
> >>>> - omap->init_60m_fclk = devm_clk_get(dev, "init_60m_fclk");
> >>>> + omap->init_60m_fclk = devm_clk_get(dev, "refclk_60m_int");
> >>>> if (IS_ERR(omap->init_60m_fclk)) {
> >>>> ret = PTR_ERR(omap->init_60m_fclk);
> >>>> dev_err(dev, "init_60m_fclk failed error:%d\n", ret);
> >>>
> >>
> >
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: devicetree repository separation/migration
From: Sascha Hauer @ 2014-02-18 15:57 UTC (permalink / raw)
To: Jason Cooper
Cc: Grant Likely, Rob Herring, Ian Campbell, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, galak-sgV2jX0FEOL9JmXXK+q4OQ,
rob-VoJi6FS/r0vR7s880joybQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
devicetree-spec-u79uwXL29TY76Z2rM5mHXA,
devicetree-compiler-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20140217180544.GU7862-u4khhh1J0LxI1Ri9qeTfzeTW4wlIGRCZ@public.gmane.org>
On Mon, Feb 17, 2014 at 01:05:44PM -0500, Jason Cooper wrote:
> All,
>
> At last weeks devicetree irc meeting, I took on the task of writing this
> email. I'm a bit behind.
>
> One of the outcomes of the ARM/devicetree discussion at the 2013 Kernel
> Summit was that we were going to hold off on separating the devicetree
> from the Linux kernel tree. The primary reason for this was to get
> through the backlog of patches.
>
> It's been several months, and we're seeing evidence of other projects
> having difficulty keeping in sync with the kernel tree. Specifically,
> barebox is having trouble syncing:
>
> http://list-archives.org/2014/02/07/barebox-lists-infradead-org/devicetree-maintenance-in-barebox/f/5820726136
>
> U-boot has some questionable (admittedly from early on) nodes. I'll
> refrain from singling out a board file or commit. Interested
> individuals can look through the irc archives for #devicetree. At any
> rate, reportedly, Wolfgang confirmed at the U-Boot minisummit that the
> dts files in U-Boot would match the ones from the kernel. So U-Boot is
> in the same situation as barebox, wrt syncing.
>
> BSD is also adopting devicetree, but I'm not personally familiar with
> the status of that. Nor whether it's diverged from what we have.
>
> On the Linux side, it appears to me that we have successfully removed
> the bottleneck, and patches seem to be flowing fairly well now.
>
> For the past few months, Ian has been maintaining a devicetree repo at
>
> http://xenbits.xen.org/gitweb/?p=people/ianc/device-tree-rebasing.git;a=summary
>
>
> Now for the questions:
>
> - Is the Linux development workflow ready for devicetree to move out
> of the Linux Kernel?
I hope so since keeping the devicetrees in sync with the kernel is a
pain for all external users.
>
> - Would having the devicetree in it's own repo, with it's own workflow
> and release schedule help the other users of it?
>
> - Where should it be hosted?
>
> - How do we envision projects will use it? git submodule? reference
> a version tag? (this is primarily targeted at bootloaders that need
> to compile in a dtb or subset of a dtb into the bootloader)
I would prefer to use it as a submodule. I'll likely need some barebox
specific additions to the devicetrees. Right now our idea is to leave
the provided devicetrees untouched and instead of compiling the board
dts files directly we create <boardname>-barebox.dts files which include
the original board files. That would allow us to provide additional
information to barebox without having to carry patches for the
devicetrees.
>
> Other thoughts I may have missed?
It will be interesting to see which rules should apply for merging new
bindings. I know that devicetrees should be OS agnostic, but sometimes
they are modelled after how Linux currently works. What happens when the
*BSD guys have different ideas how a good binding looks like? How will
such conflicts be resolved?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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 1/6] usb: gadget: gr_udc: Make struct platform_device variable name clearer
From: Felipe Balbi @ 2014-02-18 15:52 UTC (permalink / raw)
To: Andreas Larsson
Cc: Felipe Balbi, Greg Kroah-Hartman, Mark Rutland, linux-usb,
linux-kernel, devicetree, software
In-Reply-To: <1389264858-32664-2-git-send-email-andreas@gaisler.com>
[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]
On Thu, Jan 09, 2014 at 11:54:13AM +0100, Andreas Larsson wrote:
> Rename struct platform_device pointers from ofdev to pdev for clarity.
> Suggested by Mark Rutland.
>
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
> drivers/usb/gadget/gr_udc.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/gadget/gr_udc.c b/drivers/usb/gadget/gr_udc.c
> index 914cbd8..e66dcf0 100644
> --- a/drivers/usb/gadget/gr_udc.c
> +++ b/drivers/usb/gadget/gr_udc.c
> @@ -2071,9 +2071,9 @@ static int gr_udc_init(struct gr_udc *dev)
> return 0;
> }
>
> -static int gr_remove(struct platform_device *ofdev)
> +static int gr_remove(struct platform_device *pdev)
> {
> - struct gr_udc *dev = dev_get_drvdata(&ofdev->dev);
> + struct gr_udc *dev = dev_get_drvdata(&pdev->dev);
you can use platform_get_drvdata()
> @@ -2083,7 +2083,7 @@ static int gr_remove(struct platform_device *ofdev)
> gr_dfs_delete(dev);
> if (dev->desc_pool)
> dma_pool_destroy(dev->desc_pool);
> - dev_set_drvdata(&ofdev->dev, NULL);
> + dev_set_drvdata(&pdev->dev, NULL);
and platform_set_drvdata()
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] V4L: s5k6a3: Add DT binding documentation
From: Sylwester Nawrocki @ 2014-02-18 15:43 UTC (permalink / raw)
To: Sylwester Nawrocki, devicetree
Cc: Rob Herring, Mauro Carvalho Chehab, linux-media,
linux-samsung-soc, Kyungmin Park, Mark Rutland, Pawel Moll,
Kumar Gala
In-Reply-To: <1387747620-24676-1-git-send-email-s.nawrocki@samsung.com>
On 22/12/13 22:27, Sylwester Nawrocki wrote:
> This patch adds DT binding documentation for the Samsung S5K6A3(YX)
> raw image sensor.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> This patch adds missing documentation [1] for the "samsung,s5k6a3"
> compatible. Rob, can you please merge it through your tree if it
> looks OK ?
Anyone cares to Ack this patch so it can be merged through the media
tree ?
> Thanks,
> Sylwester
>
> [1] http://www.spinics.net/lists/devicetree/msg10693.html
>
> Changes since v3 (https://linuxtv.org/patch/20429):
> - rephrased 'clocks' and 'clock-names' properties' description.
> ---
> .../devicetree/bindings/media/samsung-s5k6a3.txt | 33 ++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/samsung-s5k6a3.txt
>
> diff --git a/Documentation/devicetree/bindings/media/samsung-s5k6a3.txt b/Documentation/devicetree/bindings/media/samsung-s5k6a3.txt
> new file mode 100644
> index 0000000..cce01e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/samsung-s5k6a3.txt
> @@ -0,0 +1,33 @@
> +Samsung S5K6A3(YX) raw image sensor
> +---------------------------------
> +
> +S5K6A3(YX) is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
> +and CCI (I2C compatible) control bus.
> +
> +Required properties:
> +
> +- compatible : "samsung,s5k6a3";
> +- reg : I2C slave address of the sensor;
> +- svdda-supply : core voltage supply;
> +- svddio-supply : I/O voltage supply;
> +- afvdd-supply : AF (actuator) voltage supply;
> +- gpios : specifier of a GPIO connected to the RESET pin;
> +- clocks : should contain list of phandle and clock specifier pairs
> + according to common clock bindings for the clocks described
> + in the clock-names property;
> +- clock-names : should contain "extclk" entry for the sensor's EXTCLK clock;
> +
> +Optional properties:
> +
> +- clock-frequency : the frequency at which the "extclk" clock should be
> + configured to operate, in Hz; if this property is not
> + specified default 24 MHz value will be used.
> +
> +The common video interfaces bindings (see video-interfaces.txt) should be
> +used to specify link to the image data receiver. The S5K6A3(YX) device
> +node should contain one 'port' child node with an 'endpoint' subnode.
> +
> +Following properties are valid for the endpoint node:
> +
> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> + video-interfaces.txt. The sensor supports only one data lane.
^ permalink raw reply
* Re: [PATCH v7 8/8] ARM: sunxi: Add documentation for driver for SD/MMC hosts found on Allwinner sunxi SoCs
From: Maxime Ripard @ 2014-02-18 15:42 UTC (permalink / raw)
To: David Lanzendörfer
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson, Laurent Pinchart,
Mike Turquette, Simon Baatz, Hans de Goede, Emilio López,
linux-mmc-u79uwXL29TY76Z2rM5mHXA, Chris Ball,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, H Hartley Sweeten,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Tejun Heo,
Guennadi Liakhovetski,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20140217100302.15040.56273.stgit-pgFh0Jf6HD9Xzn/AsuzBOg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]
On Mon, Feb 17, 2014 at 11:03:02AM +0100, David Lanzendörfer wrote:
> Signed-off-by: David Lanzendörfer <david.lanzendoerfer-Z7Kmv9EsliU@public.gmane.org>
> ---
> .../devicetree/bindings/mmc/sunxi-mmc.txt | 32 ++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> new file mode 100644
> index 0000000..5ce8c5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> @@ -0,0 +1,32 @@
> +* Allwinner sunxi MMC controller
> +
> +The highspeed MMC host controller on Allwinner SoCs provides an interface
> +for MMC, SD and SDIO types of memory cards.
> +
> +Supported maximum speeds are the ones of the eMMC standard 4.5 as well
> +as the speed of SD standard 3.0.
> +Absolute maximum transfer rate is 200MB/s
> +
> +Required properties:
> +- compatible: Should be "allwinner,<revision>-<chip>-mmc".
> + The supported chips include a10, a10s, 13, a20 and a31.
Please use the real compatibles here. It's much easier to search
for. Plus, your driver doesn't support all the SoCs you're mentionning here.
> +- base registers are 0x1000 appart, so the base of mmc1
> + would be 0x01c0f000+0x1000=0x01c10000(see example)
> + and so on.
Please provide the real property name to use. No need for an example
here, you have a full-fledged one in a few lines.
> +- clocks requires the reference at the ahb clock gate
> + with the correct index (mmc0 -> 8, mmc1 -> 9, and so on)
> + as well as the reference to the correct mod0 clock.
Ditto. Plus, this is not a mod0 clock.
> +- interrupts requires the correct IRQ line
> + (mmc0 -> 32, mmc1 -> 33, and so on)
Ditto.
> +
> +Examples:
> +
> +mmc0: mmc@01c0f000 {
> + compatible = "allwinner,sun5i-a13-mmc";
> + reg = <0x01c0f000 0x1000>;
> + clocks = <&ahb_gates 8>, <&mmc0_clk>;
> + clock-names = "ahb", "mod";
You never talked about the clock-names property, and which clocks were
supposed to be provided.
> + interrupts = <0 32 4>;
> + bus-width = <4>;
And you never talked about bus-width either.
> + status = "disabled";
> +};
>
Isn't the cd-gpios property requested too?
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v7 4/8] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs
From: Maxime Ripard @ 2014-02-18 15:37 UTC (permalink / raw)
To: David Lanzendörfer
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson, Laurent Pinchart,
Mike Turquette, Simon Baatz, Hans de Goede, Emilio López,
linux-mmc-u79uwXL29TY76Z2rM5mHXA, Chris Ball,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, H Hartley Sweeten,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Tejun Heo,
Guennadi Liakhovetski,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20140217100234.15040.84232.stgit-pgFh0Jf6HD9Xzn/AsuzBOg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 42135 bytes --]
Hi,
Sorry for taking a bit of time to review this.
On Mon, Feb 17, 2014 at 11:02:34AM +0100, David Lanzendörfer wrote:
> This is based on the driver Allwinner ships in their Android kernel sources.
>
> Initial porting to upstream kernels done by David Lanzendörfer, additional
> fixes and cleanups by Hans de Goede.
This belongs to the driver copyright or MODULE_AUTHOR, not really in
the commit log.
What you should describe here is what the driver does, what the device
it drives is capable of, would it be possible to share some common
code with other drivers, why didn't you do it, etc.
>
> It uses dma in bus-master mode using a built-in designware idmac controller,
> which is identical to the one found in the mmc-dw hosts.
> The rest of the host is not identical to mmc-dw.
>
> Signed-off-by: David Lanzendörfer <david.lanzendoerfer-Z7Kmv9EsliU@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/mmc/host/Kconfig | 7
> drivers/mmc/host/Makefile | 2
> drivers/mmc/host/sunxi-mmc.c | 876 ++++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/host/sunxi-mmc.h | 239 +++++++++++
> 4 files changed, 1124 insertions(+)
> create mode 100644 drivers/mmc/host/sunxi-mmc.c
> create mode 100644 drivers/mmc/host/sunxi-mmc.h
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1384f67..7caf266 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -689,3 +689,10 @@ config MMC_REALTEK_PCI
> help
> Say Y here to include driver code to support SD/MMC card interface
> of Realtek PCI-E card reader
> +
> +config MMC_SUNXI
> + tristate "Allwinner sunxi SD/MMC Host Controller support"
> + depends on ARCH_SUNXI
> + help
> + This selects support for the SD/MMC Host Controller on
> + Allwinner sunxi SoCs.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 3483b6b..f3c7c243 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -54,6 +54,8 @@ obj-$(CONFIG_MMC_WMT) += wmt-sdmmc.o
>
> obj-$(CONFIG_MMC_REALTEK_PCI) += rtsx_pci_sdmmc.o
>
> +obj-$(CONFIG_MMC_SUNXI) += sunxi-mmc.o
> +
> obj-$(CONFIG_MMC_SDHCI_PLTFM) += sdhci-pltfm.o
> obj-$(CONFIG_MMC_SDHCI_CNS3XXX) += sdhci-cns3xxx.o
> obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> new file mode 100644
> index 0000000..752e913
> --- /dev/null
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -0,0 +1,876 @@
> +/*
> + * Driver for sunxi SD/MMC host controllers
> + * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
> + * (C) Copyright 2007-2011 Aaron Maoye <leafy.myeh-jFKXxz0WcGyYHARAtoI1EgC/G2K4zDHf@public.gmane.org>
> + * (C) Copyright 2013-2014 O2S GmbH <www.o2s.ch>
> + * (C) Copyright 2013-2014 David Lanzend?rfer <david.lanzendoerfer@o2s.ch>
> + * (C) Copyright 2013-2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +
> +#include <linux/clk.h>
> +#include <linux/clk-private.h>
> +#include <linux/clk/sunxi.h>
> +
> +#include <linux/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/scatterlist.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/sdio.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/slot-gpio.h>
> +
> +#include "sunxi-mmc.h"
Usually, drivers don't have a header of their own.
> +static int sunxi_mmc_init_host(struct mmc_host *mmc)
> +{
> + u32 rval;
> + struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
> + int ret;
> +
> + ret = clk_prepare_enable(smc_host->clk_ahb);
> + if (ret) {
> + dev_err(mmc_dev(smc_host->mmc), "AHB clk err %d\n", ret);
> + return ret;
> + }
Newline.
> + ret = clk_prepare_enable(smc_host->clk_mod);
> + if (ret) {
> + dev_err(mmc_dev(smc_host->mmc), "MOD clk err %d\n", ret);
> + clk_disable_unprepare(smc_host->clk_ahb);
> + return ret;
> + }
> +
> + /* reset controller */
> + rval = mci_readl(smc_host, REG_GCTRL) | SDXC_HARDWARE_RESET;
You seem to be sometimes using either a oneline construct like this
one, or on several lines. It would be great if you were to be using
the same everywhere.
> + mci_writel(smc_host, REG_GCTRL, rval);
> +
> + mci_writel(smc_host, REG_FTRGL, 0x20070008);
> + mci_writel(smc_host, REG_TMOUT, 0xffffffff);
> + mci_writel(smc_host, REG_IMASK, smc_host->sdio_imask);
> + mci_writel(smc_host, REG_RINTR, 0xffffffff);
> + mci_writel(smc_host, REG_DBGC, 0xdeb);
> + mci_writel(smc_host, REG_FUNS, 0xceaa0000);
> + mci_writel(smc_host, REG_DLBA, smc_host->sg_dma);
Newline.
> + rval = mci_readl(smc_host, REG_GCTRL)|SDXC_INTERRUPT_ENABLE_BIT;
> + rval &= ~SDXC_ACCESS_DONE_DIRECT;
> + mci_writel(smc_host, REG_GCTRL, rval);
> +
> + return 0;
> +}
> +
> +static void sunxi_mmc_exit_host(struct sunxi_mmc_host *smc_host)
> +{
> + mci_writel(smc_host, REG_GCTRL, SDXC_HARDWARE_RESET);
> + clk_disable_unprepare(smc_host->clk_ahb);
> + clk_disable_unprepare(smc_host->clk_mod);
> +}
> +
> +/* /\* UHS-I Operation Modes */
> +/* * DS 25MHz 12.5MB/s 3.3V */
> +/* * HS 50MHz 25MB/s 3.3V */
> +/* * SDR12 25MHz 12.5MB/s 1.8V */
> +/* * SDR25 50MHz 25MB/s 1.8V */
> +/* * SDR50 100MHz 50MB/s 1.8V */
> +/* * SDR104 208MHz 104MB/s 1.8V */
> +/* * DDR50 50MHz 50MB/s 1.8V */
> +/* * MMC Operation Modes */
> +/* * DS 26MHz 26MB/s 3/1.8/1.2V */
> +/* * HS 52MHz 52MB/s 3/1.8/1.2V */
> +/* * HSDDR 52MHz 104MB/s 3/1.8/1.2V */
> +/* * HS200 200MHz 200MB/s 1.8/1.2V */
> +/* * */
> +/* * Spec. Timing */
> +/* * SD3.0 */
> +/* * Fcclk Tcclk Fsclk Tsclk Tis Tih odly RTis RTih */
> +/* * 400K 2.5us 24M 41ns 5ns 5ns 1 2209ns 41ns */
> +/* * 25M 40ns 600M 1.67ns 5ns 5ns 3 14.99ns 5.01ns */
> +/* * 50M 20ns 600M 1.67ns 6ns 2ns 3 14.99ns 5.01ns */
> +/* * 50MDDR 20ns 600M 1.67ns 6ns 0.8ns 2 6.67ns 3.33ns */
> +/* * 104M 9.6ns 600M 1.67ns 3ns 0.8ns 1 7.93ns 1.67ns */
> +/* * 208M 4.8ns 600M 1.67ns 1.4ns 0.8ns 1 3.33ns 1.67ns */
> +
> +/* * 25M 40ns 300M 3.33ns 5ns 5ns 2 13.34ns 6.66ns */
> +/* * 50M 20ns 300M 3.33ns 6ns 2ns 2 13.34ns 6.66ns */
> +/* * 50MDDR 20ns 300M 3.33ns 6ns 0.8ns 1 6.67ns 3.33ns */
> +/* * 104M 9.6ns 300M 3.33ns 3ns 0.8ns 0 7.93ns 1.67ns */
> +/* * 208M 4.8ns 300M 3.33ns 1.4ns 0.8ns 0 3.13ns 1.67ns */
> +
> +/* * eMMC4.5 */
> +/* * 400K 2.5us 24M 41ns 3ns 3ns 1 2209ns 41ns */
> +/* * 25M 40ns 600M 1.67ns 3ns 3ns 3 14.99ns 5.01ns */
> +/* * 50M 20ns 600M 1.67ns 3ns 3ns 3 14.99ns 5.01ns */
> +/* * 50MDDR 20ns 600M 1.67ns 2.5ns 2.5ns 2 6.67ns 3.33ns */
> +/* * 200M 5ns 600M 1.67ns 1.4ns 0.8ns 1 3.33ns 1.67ns */
> +/* *\/ */
> +
> +static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host,
> + struct mmc_data *data)
> +{
> + struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu;
> + struct sunxi_idma_des *pdes_pa = (struct sunxi_idma_des *)host->sg_dma;
> + int i, max_len = (1 << host->idma_des_size_bits);
Please use the BIT() macro here.
> +
> + for (i = 0; i < data->sg_len; i++) {
> + pdes[i].config = SDXC_IDMAC_DES0_CH | SDXC_IDMAC_DES0_OWN |
> + SDXC_IDMAC_DES0_DIC;
> +
> + if (data->sg[i].length == max_len)
> + pdes[i].buf_size = 0; /* 0 == max_len */
> + else
> + pdes[i].buf_size = data->sg[i].length;
> +
> + pdes[i].buf_addr_ptr1 = sg_dma_address(&data->sg[i]);
> + pdes[i].buf_addr_ptr2 = (u32)&pdes_pa[i + 1];
> + }
> +
> + pdes[0].config |= SDXC_IDMAC_DES0_FD;
> + pdes[i - 1].config = SDXC_IDMAC_DES0_OWN | SDXC_IDMAC_DES0_LD;
> +
> + wmb(); /* Ensure idma_des hit main mem before we start the idmac */
wmb ensure the proper ordering of the instructions, not flushing the
caches like what your comment implies.
> +}
> +
> +static enum dma_data_direction sunxi_mmc_get_dma_dir(struct mmc_data *data)
> +{
> + if (data->flags & MMC_DATA_WRITE)
> + return DMA_TO_DEVICE;
> + else
> + return DMA_FROM_DEVICE;
> +}
> +
> +static int sunxi_mmc_prepare_dma(struct sunxi_mmc_host *smc_host,
> + struct mmc_data *data)
> +{
> + u32 dma_len;
> + u32 i;
> + u32 temp;
> + struct scatterlist *sg;
> +
> + dma_len = dma_map_sg(mmc_dev(smc_host->mmc), data->sg, data->sg_len,
> + sunxi_mmc_get_dma_dir(data));
> + if (dma_len == 0) {
> + dev_err(mmc_dev(smc_host->mmc), "dma_map_sg failed\n");
> + return -ENOMEM;
> + }
> +
> + for_each_sg(data->sg, sg, data->sg_len, i) {
> + if (sg->offset & 3 || sg->length & 3) {
> + dev_err(mmc_dev(smc_host->mmc),
> + "unaligned scatterlist: os %x length %d\n",
> + sg->offset, sg->length);
> + return -EINVAL;
> + }
> + }
> +
> + sunxi_mmc_init_idma_des(smc_host, data);
> +
> + temp = mci_readl(smc_host, REG_GCTRL);
> + temp |= SDXC_DMA_ENABLE_BIT;
> + mci_writel(smc_host, REG_GCTRL, temp);
> + temp |= SDXC_DMA_RESET;
> + mci_writel(smc_host, REG_GCTRL, temp);
Does it really need to be done in two steps?
(Newline)
> + mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_SOFT_RESET);
> +
> + if (!(data->flags & MMC_DATA_WRITE))
> + mci_writel(smc_host, REG_IDIE, SDXC_IDMAC_RECEIVE_INTERRUPT);
> +
> + mci_writel(smc_host, REG_DMAC, SDXC_IDMAC_FIX_BURST | SDXC_IDMAC_IDMA_ON);
> +
> + return 0;
> +}
> +
> +static void sunxi_mmc_send_manual_stop(struct sunxi_mmc_host *host,
> + struct mmc_request *req)
> +{
> + u32 cmd_val = SDXC_START | SDXC_RESP_EXPIRE | SDXC_STOP_ABORT_CMD
> + | SDXC_CHECK_RESPONSE_CRC | MMC_STOP_TRANSMISSION;
> + u32 ri = 0;
> + unsigned long expire = jiffies + msecs_to_jiffies(1000);
> +
> + mci_writel(host, REG_CARG, 0);
> + mci_writel(host, REG_CMDR, cmd_val);
> +
> + do {
> + ri = mci_readl(host, REG_RINTR);
> + } while (!(ri & (SDXC_COMMAND_DONE | SDXC_INTERRUPT_ERROR_BIT)) &&
> + time_before(jiffies, expire));
> +
> + if (ri & SDXC_INTERRUPT_ERROR_BIT) {
> + dev_err(mmc_dev(host->mmc), "send stop command failed\n");
> + if (req->stop)
> + req->stop->resp[0] = -ETIMEDOUT;
> + } else {
> + if (req->stop)
> + req->stop->resp[0] = mci_readl(host, REG_RESP0);
> + }
> +
> + mci_writel(host, REG_RINTR, 0xffff);
> +}
> +
> +static void sunxi_mmc_dump_errinfo(struct sunxi_mmc_host *smc_host)
> +{
> + struct mmc_command *cmd = smc_host->mrq->cmd;
> + struct mmc_data *data = smc_host->mrq->data;
> +
> + /* For some cmds timeout is normal with sd/mmc cards */
> + if ((smc_host->int_sum & SDXC_INTERRUPT_ERROR_BIT) == SDXC_RESP_TIMEOUT &&
> + (cmd->opcode == SD_IO_SEND_OP_COND || cmd->opcode == SD_IO_RW_DIRECT))
> + return;
> +
> + dev_err(mmc_dev(smc_host->mmc),
I'd rather put it at a debug loglevel.
> + "smc %d err, cmd %d,%s%s%s%s%s%s%s%s%s%s !!\n",
> + smc_host->mmc->index, cmd->opcode,
> + data ? (data->flags & MMC_DATA_WRITE ? " WR" : " RD") : "",
> + smc_host->int_sum & SDXC_RESP_ERROR ? " RE" : "",
> + smc_host->int_sum & SDXC_RESP_CRC_ERROR ? " RCE" : "",
> + smc_host->int_sum & SDXC_DATA_CRC_ERROR ? " DCE" : "",
> + smc_host->int_sum & SDXC_RESP_TIMEOUT ? " RTO" : "",
> + smc_host->int_sum & SDXC_DATA_TIMEOUT ? " DTO" : "",
> + smc_host->int_sum & SDXC_FIFO_RUN_ERROR ? " FE" : "",
> + smc_host->int_sum & SDXC_HARD_WARE_LOCKED ? " HL" : "",
> + smc_host->int_sum & SDXC_START_BIT_ERROR ? " SBE" : "",
> + smc_host->int_sum & SDXC_END_BIT_ERROR ? " EBE" : ""
> + );
> +}
> +
> +static void sunxi_mmc_finalize_request(struct sunxi_mmc_host *host)
> +{
> + struct mmc_request *mrq;
> + unsigned long iflags;
> +
> + spin_lock_irqsave(&host->lock, iflags);
> +
> + mrq = host->mrq;
> + if (!mrq) {
> + spin_unlock_irqrestore(&host->lock, iflags);
> + dev_err(mmc_dev(host->mmc), "no request to finalize\n");
> + return;
> + }
> +
> + if (host->int_sum & SDXC_INTERRUPT_ERROR_BIT) {
> + sunxi_mmc_dump_errinfo(host);
> + mrq->cmd->error = -ETIMEDOUT;
Newline
> + if (mrq->data)
> + mrq->data->error = -ETIMEDOUT;
Newline
> + if (mrq->stop)
> + mrq->stop->error = -ETIMEDOUT;
> + } else {
> + if (mrq->cmd->flags & MMC_RSP_136) {
> + mrq->cmd->resp[0] = mci_readl(host, REG_RESP3);
> + mrq->cmd->resp[1] = mci_readl(host, REG_RESP2);
> + mrq->cmd->resp[2] = mci_readl(host, REG_RESP1);
> + mrq->cmd->resp[3] = mci_readl(host, REG_RESP0);
> + } else {
> + mrq->cmd->resp[0] = mci_readl(host, REG_RESP0);
> + }
> +
Newline
> + if (mrq->data)
> + mrq->data->bytes_xfered =
> + mrq->data->blocks * mrq->data->blksz;
> + }
> +
> + if (mrq->data) {
> + struct mmc_data *data = mrq->data;
> + u32 temp;
> +
> + mci_writel(host, REG_IDST, 0x337);
> + mci_writel(host, REG_DMAC, 0);
> + temp = mci_readl(host, REG_GCTRL);
> + mci_writel(host, REG_GCTRL, temp|SDXC_DMA_RESET);
> + temp &= ~SDXC_DMA_ENABLE_BIT;
> + mci_writel(host, REG_GCTRL, temp);
> + temp |= SDXC_FIFO_RESET;
> + mci_writel(host, REG_GCTRL, temp);
> + dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> + sunxi_mmc_get_dma_dir(data));
> + }
> +
> + mci_writel(host, REG_RINTR, 0xffff);
> +
> + dev_dbg(mmc_dev(host->mmc), "req done, resp %08x %08x %08x %08x\n",
> + mrq->cmd->resp[0], mrq->cmd->resp[1],
> + mrq->cmd->resp[2], mrq->cmd->resp[3]);
> +
> + host->mrq = NULL;
> + host->int_sum = 0;
> + host->wait_dma = 0;
> +
> + spin_unlock_irqrestore(&host->lock, iflags);
> +
> + if (mrq->data && mrq->data->error) {
> + dev_err(mmc_dev(host->mmc),
> + "data error, sending stop command\n");
> + sunxi_mmc_send_manual_stop(host, mrq);
> + }
> +
> + mmc_request_done(host->mmc, mrq);
> +}
> +
> +static irqreturn_t sunxi_mmc_irq(int irq, void *dev_id)
> +{
> + struct sunxi_mmc_host *host = dev_id;
> + u32 finalize = 0;
> + u32 sdio_int = 0;
> + u32 msk_int;
> + u32 idma_int;
> +
> + spin_lock(&host->lock);
> +
> + idma_int = mci_readl(host, REG_IDST);
> + msk_int = mci_readl(host, REG_MISTA);
> +
> + dev_dbg(mmc_dev(host->mmc), "irq: rq %p mi %08x idi %08x\n",
> + host->mrq, msk_int, idma_int);
> +
> + if (host->mrq) {
> + if (idma_int & SDXC_IDMAC_RECEIVE_INTERRUPT)
> + host->wait_dma = 0;
> +
> + host->int_sum |= msk_int;
> +
> + /* Wait for COMMAND_DONE on RESPONSE_TIMEOUT before finishing the req */
> + if ((host->int_sum & SDXC_RESP_TIMEOUT) &&
> + !(host->int_sum & SDXC_COMMAND_DONE))
> + mci_writel(host, REG_IMASK,
> + host->sdio_imask | SDXC_COMMAND_DONE);
> + else if (host->int_sum & SDXC_INTERRUPT_ERROR_BIT)
> + finalize = 1; /* Don't wait for dma on error */
> + else if (host->int_sum & SDXC_INTERRUPT_DONE_BIT && !host->wait_dma)
> + finalize = 1; /* Done */
Usually, the comments are not placed on the same line, but on a
separate line just above (like you did a few lines earlier).
> +
> + if (finalize) {
> + mci_writel(host, REG_IMASK, host->sdio_imask);
> + mci_writel(host, REG_IDIE, 0);
> + }
> + }
> +
> + if (msk_int & SDXC_SDIO_INTERRUPT)
> + sdio_int = 1;
> +
> + mci_writel(host, REG_RINTR, msk_int);
> + mci_writel(host, REG_IDST, idma_int);
> +
> + spin_unlock(&host->lock);
> +
> + if (finalize)
> + tasklet_schedule(&host->tasklet);
> +
> + if (sdio_int)
> + mmc_signal_sdio_irq(host->mmc);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void sunxi_mmc_tasklet(unsigned long data)
> +{
> + struct sunxi_mmc_host *smc_host = (struct sunxi_mmc_host *) data;
> + sunxi_mmc_finalize_request(smc_host);
> +}
> +
> +static void sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
> +{
> + unsigned long expire = jiffies + msecs_to_jiffies(2000);
> + u32 rval;
> +
> + rval = mci_readl(host, REG_CLKCR);
> + rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
> +
> + if (oclk_en)
> + rval |= SDXC_CARD_CLOCK_ON;
> +
> + if (!host->io_flag)
> + rval |= SDXC_LOW_POWER_ON;
> +
> + mci_writel(host, REG_CLKCR, rval);
> +
> + rval = SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER;
> + if (host->voltage_switching)
> + rval |= SDXC_VOLTAGE_SWITCH;
> + mci_writel(host, REG_CMDR, rval);
> +
> + do {
> + rval = mci_readl(host, REG_CMDR);
> + } while (time_before(jiffies, expire) && (rval & SDXC_START));
> +
> + if (rval & SDXC_START) {
> + dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
> + host->ferror = 1;
> + }
> +}
> +
> +static void sunxi_mmc_set_clk_dly(struct sunxi_mmc_host *smc_host,
> + u32 oclk_dly, u32 sclk_dly)
> +{
> + unsigned long iflags;
> + struct clk_hw *hw = __clk_get_hw(smc_host->clk_mod);
> +
> + spin_lock_irqsave(&smc_host->lock, iflags);
> + clk_sunxi_mmc_phase_control(hw, sclk_dly, oclk_dly);
> + spin_unlock_irqrestore(&smc_host->lock, iflags);
> +}
> +
> +struct sunxi_mmc_clk_dly mmc_clk_dly[MMC_CLK_MOD_NUM] = {
> + { MMC_CLK_400K, 0, 7 },
> + { MMC_CLK_25M, 0, 5 },
> + { MMC_CLK_50M, 3, 5 },
> + { MMC_CLK_50MDDR, 2, 4 },
> + { MMC_CLK_50MDDR_8BIT, 2, 4 },
> + { MMC_CLK_100M, 1, 4 },
> + { MMC_CLK_200M, 1, 4 },
> +};
> +
> +static void sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *smc_host,
> + unsigned int rate)
> +{
> + u32 newrate;
> + u32 src_clk;
> + u32 oclk_dly;
> + u32 sclk_dly;
> + u32 temp;
> + struct sunxi_mmc_clk_dly *dly = NULL;
> +
> + newrate = clk_round_rate(smc_host->clk_mod, rate);
> + if (smc_host->clk_mod_rate == newrate) {
> + dev_dbg(mmc_dev(smc_host->mmc), "clk already %d, rounded %d\n",
> + rate, newrate);
> + return;
> + }
> +
> + dev_dbg(mmc_dev(smc_host->mmc), "setting clk to %d, rounded %d\n",
> + rate, newrate);
> +
> + /* setting clock rate */
> + clk_disable(smc_host->clk_mod);
> + clk_set_rate(smc_host->clk_mod, newrate);
> + clk_enable(smc_host->clk_mod);
You don't need to disable/enable the clocks here.
> + smc_host->clk_mod_rate = newrate = clk_get_rate(smc_host->clk_mod);
> + dev_dbg(mmc_dev(smc_host->mmc), "clk is now %d\n", newrate);
You don't seem to use this newrate variable somewhere else in the
function, can't you just use the clk_mod_rate one?
And it doesn't seem like you use clk_mod_rate much either.
> +
> + sunxi_mmc_oclk_onoff(smc_host, 0);
> + /* clear internal divider */
> + temp = mci_readl(smc_host, REG_CLKCR);
> + temp &= ~0xff;
> + mci_writel(smc_host, REG_CLKCR, temp);
> +
> + /* determine delays */
> + if (rate <= 400000) {
> + dly = &mmc_clk_dly[MMC_CLK_400K];
> + } else if (rate <= 25000000) {
> + dly = &mmc_clk_dly[MMC_CLK_25M];
> + } else if (rate <= 50000000) {
> + if (smc_host->ddr) {
> + if (smc_host->bus_width == 8)
> + dly = &mmc_clk_dly[MMC_CLK_50MDDR_8BIT];
> + else
> + dly = &mmc_clk_dly[MMC_CLK_50MDDR];
> + } else {
> + dly = &mmc_clk_dly[MMC_CLK_50M];
> + }
> + } else if (rate <= 104000000) {
> + dly = &mmc_clk_dly[MMC_CLK_100M];
> + } else if (rate <= 208000000) {
> + dly = &mmc_clk_dly[MMC_CLK_200M];
> + } else {
> + dly = &mmc_clk_dly[MMC_CLK_50M];
> + }
> +
> + oclk_dly = dly->oclk_dly;
> + sclk_dly = dly->sclk_dly;
> +
> + src_clk = clk_get_rate(clk_get_parent(smc_host->clk_mod));
> +
> + if (src_clk >= 300000000 && src_clk <= 400000000) {
> + if (oclk_dly)
> + oclk_dly--;
> + if (sclk_dly)
> + sclk_dly--;
> + }
> +
> + sunxi_mmc_set_clk_dly(smc_host, oclk_dly, sclk_dly);
> + sunxi_mmc_oclk_onoff(smc_host, 1);
> +
> + /* oclk_onoff sets various irq status bits, clear these */
> + mci_writel(smc_host, REG_RINTR,
> + mci_readl(smc_host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT);
> +}
> +
> +static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct sunxi_mmc_host *host = mmc_priv(mmc);
> + u32 temp;
> + s32 err;
> +
> + /* Set the power state */
> + switch (ios->power_mode) {
> + case MMC_POWER_ON:
> + break;
> +
> + case MMC_POWER_UP:
> + if (!IS_ERR(host->vmmc)) {
> + mmc_regulator_set_ocr(host->mmc, host->vmmc, ios->vdd);
> + udelay(200);
> + }
> +
> + err = sunxi_mmc_init_host(mmc);
> + if (err) {
> + host->ferror = 1;
> + return;
> + }
Newline
> + enable_irq(host->irq);
> +
> + dev_dbg(mmc_dev(host->mmc), "power on!\n");
> + host->ferror = 0;
> + break;
> +
> + case MMC_POWER_OFF:
> + dev_dbg(mmc_dev(host->mmc), "power off!\n");
> + disable_irq(host->irq);
> + sunxi_mmc_exit_host(host);
> + if (!IS_ERR(host->vmmc))
> + mmc_regulator_set_ocr(host->mmc, host->vmmc, 0);
Newline
> + host->ferror = 0;
> + break;
> + }
> +
> + /* set bus width */
> + switch (ios->bus_width) {
> + case MMC_BUS_WIDTH_1:
> + mci_writel(host, REG_WIDTH, SDXC_WIDTH1);
> + host->bus_width = 1;
> + break;
> + case MMC_BUS_WIDTH_4:
> + mci_writel(host, REG_WIDTH, SDXC_WIDTH4);
> + host->bus_width = 4;
> + break;
> + case MMC_BUS_WIDTH_8:
> + mci_writel(host, REG_WIDTH, SDXC_WIDTH8);
> + host->bus_width = 8;
> + break;
> + }
> +
> + /* set ddr mode */
> + temp = mci_readl(host, REG_GCTRL);
> + if (ios->timing == MMC_TIMING_UHS_DDR50) {
> + temp |= SDXC_DDR_MODE;
> + host->ddr = 1;
> + } else {
> + temp &= ~SDXC_DDR_MODE;
> + host->ddr = 0;
> + }
> + mci_writel(host, REG_GCTRL, temp);
> +
> + /* set up clock */
> + if (ios->clock && ios->power_mode) {
> + dev_dbg(mmc_dev(host->mmc), "ios->clock: %d\n", ios->clock);
> + sunxi_mmc_clk_set_rate(host, ios->clock);
> + usleep_range(50000, 55000);
> + }
> +}
> +
> +static void sunxi_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
> +{
> + struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
> + unsigned long flags;
> + int ret;
> + u32 imask;
> +
> + spin_lock_irqsave(&smc_host->lock, flags);
> +
> + /* Make sure the controller is in a sane state before enabling irqs */
> + ret = sunxi_mmc_init_host(mmc);
> + if (ret) {
> + spin_unlock_irqrestore(&smc_host->lock, flags);
> + return;
> + }
> +
> + imask = mci_readl(smc_host, REG_IMASK);
> + if (enable) {
> + smc_host->sdio_imask = SDXC_SDIO_INTERRUPT;
> + imask |= SDXC_SDIO_INTERRUPT;
> + } else {
> + smc_host->sdio_imask = 0;
> + imask &= ~SDXC_SDIO_INTERRUPT;
> + }
> + mci_writel(smc_host, REG_IMASK, imask);
> + spin_unlock_irqrestore(&smc_host->lock, flags);
> +}
> +
> +static void sunxi_mmc_hw_reset(struct mmc_host *mmc)
> +{
> + struct sunxi_mmc_host *smc_host = mmc_priv(mmc);
> + mci_writel(smc_host, REG_HWRST, 0);
> + udelay(10);
> + mci_writel(smc_host, REG_HWRST, 1);
> + udelay(300);
> +}
> +
> +static void sunxi_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> +{
> + struct sunxi_mmc_host *host = mmc_priv(mmc);
> + struct mmc_command *cmd = mrq->cmd;
> + struct mmc_data *data = mrq->data;
> + unsigned long iflags;
> + u32 imask = SDXC_INTERRUPT_ERROR_BIT;
> + u32 cmd_val = SDXC_START | (cmd->opcode & 0x3f);
> + u32 byte_cnt = 0;
> + int ret;
> +
> + if (!mmc_gpio_get_cd(mmc) || host->ferror) {
> + dev_dbg(mmc_dev(host->mmc), "no medium present\n");
> + mrq->cmd->error = -ENOMEDIUM;
> + mmc_request_done(mmc, mrq);
> + return;
> + }
> +
> + if (data) {
> + byte_cnt = data->blksz * data->blocks;
> + mci_writel(host, REG_BLKSZ, data->blksz);
> + mci_writel(host, REG_BCNTR, byte_cnt);
> + ret = sunxi_mmc_prepare_dma(host, data);
> + if (ret < 0) {
> + dev_err(mmc_dev(host->mmc), "prepare DMA failed\n");
> + cmd->error = ret;
> + cmd->data->error = ret;
> + mmc_request_done(host->mmc, mrq);
> + return;
> + }
> + }
> +
> + if (cmd->opcode == MMC_GO_IDLE_STATE) {
> + cmd_val |= SDXC_SEND_INIT_SEQUENCE;
> + imask |= SDXC_COMMAND_DONE;
> + }
> +
> + if (cmd->opcode == SD_SWITCH_VOLTAGE) {
> + cmd_val |= SDXC_VOLTAGE_SWITCH;
> + imask |= SDXC_VOLTAGE_CHANGE_DONE;
> + host->voltage_switching = 1;
> + sunxi_mmc_oclk_onoff(host, 1);
> + }
> +
> + if (cmd->flags & MMC_RSP_PRESENT) {
> + cmd_val |= SDXC_RESP_EXPIRE;
> + if (cmd->flags & MMC_RSP_136)
> + cmd_val |= SDXC_LONG_RESPONSE;
> + if (cmd->flags & MMC_RSP_CRC)
> + cmd_val |= SDXC_CHECK_RESPONSE_CRC;
> +
> + if ((cmd->flags & MMC_CMD_MASK) == MMC_CMD_ADTC) {
> + cmd_val |= SDXC_DATA_EXPIRE | SDXC_WAIT_PRE_OVER;
> + if (cmd->data->flags & MMC_DATA_STREAM) {
> + imask |= SDXC_AUTO_COMMAND_DONE;
> + cmd_val |= SDXC_SEQUENCE_MODE | SDXC_SEND_AUTO_STOP;
> + }
Newline
> + if (cmd->data->stop) {
> + imask |= SDXC_AUTO_COMMAND_DONE;
> + cmd_val |= SDXC_SEND_AUTO_STOP;
> + } else
> + imask |= SDXC_DATA_OVER;
You should have braces here
> + if (cmd->data->flags & MMC_DATA_WRITE)
> + cmd_val |= SDXC_WRITE;
> + else
> + host->wait_dma = 1;
> + } else
> + imask |= SDXC_COMMAND_DONE;
And here
> + } else
> + imask |= SDXC_COMMAND_DONE;
And here
> + dev_dbg(mmc_dev(host->mmc), "cmd %d(%08x) arg %x ie 0x%08x len %d\n",
> + cmd_val & 0x3f, cmd_val, cmd->arg, imask,
> + mrq->data ? mrq->data->blksz * mrq->data->blocks : 0);
> +
> + spin_lock_irqsave(&host->lock, iflags);
> + host->mrq = mrq;
> + mci_writel(host, REG_IMASK, host->sdio_imask | imask);
> + spin_unlock_irqrestore(&host->lock, iflags);
> +
> + mci_writel(host, REG_CARG, cmd->arg);
> + mci_writel(host, REG_CMDR, cmd_val);
> +}
> +
> +static const struct of_device_id sunxi_mmc_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-mmc", },
> + { .compatible = "allwinner,sun5i-a13-mmc", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
> +
> +static struct mmc_host_ops sunxi_mmc_ops = {
> + .request = sunxi_mmc_request,
> + .set_ios = sunxi_mmc_set_ios,
> + .get_ro = mmc_gpio_get_ro,
> + .get_cd = mmc_gpio_get_cd,
> + .enable_sdio_irq = sunxi_mmc_enable_sdio_irq,
> + .hw_reset = sunxi_mmc_hw_reset,
> +};
> +
> +static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
> + struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + int ret;
> +
> + if (of_device_is_compatible(np, "allwinner,sun4i-a10-mmc"))
> + host->idma_des_size_bits = 13;
> + else
> + host->idma_des_size_bits = 16;
> +
> + host->vmmc = devm_regulator_get_optional(&pdev->dev, "vmmc");
> + if (IS_ERR(host->vmmc) && PTR_ERR(host->vmmc) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + host->reg_base = devm_ioremap_resource(&pdev->dev,
> + platform_get_resource(pdev, IORESOURCE_MEM, 0));
> + if (IS_ERR(host->reg_base))
> + return PTR_ERR(host->reg_base);
> +
> + host->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(host->clk_ahb)) {
> + dev_err(&pdev->dev, "Could not get ahb clock\n");
> + return PTR_ERR(host->clk_ahb);
> + }
> +
> + host->clk_mod = devm_clk_get(&pdev->dev, "mod");
> + if (IS_ERR(host->clk_mod)) {
> + dev_err(&pdev->dev, "Could not get mod clock\n");
> + return PTR_ERR(host->clk_mod);
> + }
> +
> + /* Make sure the controller is in a sane state before enabling irqs */
> + ret = sunxi_mmc_init_host(host->mmc);
> + if (ret)
> + return ret;
> +
> + host->irq = platform_get_irq(pdev, 0);
> + ret = devm_request_irq(&pdev->dev, host->irq, sunxi_mmc_irq, 0,
> + "sunxi-mmc", host);
> + if (ret == 0)
> + disable_irq(host->irq);
The disable_irq is useless here. Just exit.
> +
> + /* And put it back in reset */
> + sunxi_mmc_exit_host(host);
Hu? If it's in reset, how can it generate some IRQs?
> + return ret;
> +}
> +
> +static int sunxi_mmc_probe(struct platform_device *pdev)
> +{
> + struct sunxi_mmc_host *host;
> + struct mmc_host *mmc;
> + int ret;
> +
> + mmc = mmc_alloc_host(sizeof(struct sunxi_mmc_host), &pdev->dev);
> + if (!mmc) {
> + dev_err(&pdev->dev, "mmc alloc host failed\n");
> + return -ENOMEM;
> + }
> +
> + ret = mmc_of_parse(mmc);
> + if (ret)
> + goto error_free_host;
> +
> + host = mmc_priv(mmc);
> + host->mmc = mmc;
> + spin_lock_init(&host->lock);
> + tasklet_init(&host->tasklet, sunxi_mmc_tasklet, (unsigned long)host);
> +
> + ret = sunxi_mmc_resource_request(host, pdev);
> + if (ret)
> + goto error_free_host;
> +
> + host->sg_cpu = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
> + &host->sg_dma, GFP_KERNEL);
> + if (!host->sg_cpu) {
> + dev_err(&pdev->dev, "Failed to allocate DMA descriptor mem\n");
> + ret = -ENOMEM;
> + goto error_free_host;
> + }
> +
> + mmc->ops = &sunxi_mmc_ops;
> + mmc->max_blk_count = 8192;
> + mmc->max_blk_size = 4096;
> + mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des);
> + mmc->max_seg_size = (1 << host->idma_des_size_bits);
> + mmc->max_req_size = mmc->max_seg_size * mmc->max_segs;
> + /* 400kHz ~ 50MHz */
> + mmc->f_min = 400000;
> + mmc->f_max = 50000000;
> + /* available voltages */
> + if (!IS_ERR(host->vmmc))
> + mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vmmc);
> + else
> + mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> +
> + mmc->caps = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED |
> + MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
> + MMC_CAP_UHS_DDR50 | MMC_CAP_SDIO_IRQ | MMC_CAP_DRIVER_TYPE_A;
> + mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP;
> +
> + ret = mmc_add_host(mmc);
> +
> + if (ret)
> + goto error_free_dma;
> +
> + dev_info(&pdev->dev, "base:0x%p irq:%u\n", host->reg_base, host->irq);
> + platform_set_drvdata(pdev, mmc);
This should be before the registration. Otherwise, you're racy.
> + return 0;
> +
> +error_free_dma:
> + dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> +error_free_host:
> + mmc_free_host(mmc);
> + return ret;
> +}
> +
> +static int sunxi_mmc_remove(struct platform_device *pdev)
> +{
> + struct mmc_host *mmc = platform_get_drvdata(pdev);
> + struct sunxi_mmc_host *host = mmc_priv(mmc);
> +
> + mmc_remove_host(mmc);
> + sunxi_mmc_exit_host(host);
> + tasklet_disable(&host->tasklet);
> + dma_free_coherent(&pdev->dev, PAGE_SIZE, host->sg_cpu, host->sg_dma);
> + mmc_free_host(mmc);
> +
> + return 0;
> +}
> +
> +static struct platform_driver sunxi_mmc_driver = {
> + .driver = {
> + .name = "sunxi-mmc",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(sunxi_mmc_of_match),
> + },
> + .probe = sunxi_mmc_probe,
> + .remove = sunxi_mmc_remove,
> +};
> +module_platform_driver(sunxi_mmc_driver);
> +
> +MODULE_DESCRIPTION("Allwinner's SD/MMC Card Controller Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("David Lanzend?rfer <david.lanzendoerfer-Z7Kmv9EsliU@public.gmane.org>");
> +MODULE_ALIAS("platform:sunxi-mmc");
> diff --git a/drivers/mmc/host/sunxi-mmc.h b/drivers/mmc/host/sunxi-mmc.h
> new file mode 100644
> index 0000000..75eaa02
> --- /dev/null
> +++ b/drivers/mmc/host/sunxi-mmc.h
> @@ -0,0 +1,239 @@
> +/*
> + * Driver for sunxi SD/MMC host controllers
> + * (C) Copyright 2007-2011 Reuuimlla Technology Co., Ltd.
> + * (C) Copyright 2007-2011 Aaron Maoye <leafy.myeh-jFKXxz0WcGyYHARAtoI1EgC/G2K4zDHf@public.gmane.org>
> + * (C) Copyright 2013-2014 O2S GmbH <www.o2s.ch>
> + * (C) Copyright 2013-2014 David Lanzend?rfer <david.lanzendoerfer@o2s.ch>
> + * (C) Copyright 2013-2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#ifndef __SUNXI_MMC_H__
> +#define __SUNXI_MMC_H__
> +
> +/* register offset definitions */
> +#define SDXC_REG_GCTRL (0x00) /* SMC Global Control Register */
> +#define SDXC_REG_CLKCR (0x04) /* SMC Clock Control Register */
> +#define SDXC_REG_TMOUT (0x08) /* SMC Time Out Register */
> +#define SDXC_REG_WIDTH (0x0C) /* SMC Bus Width Register */
> +#define SDXC_REG_BLKSZ (0x10) /* SMC Block Size Register */
> +#define SDXC_REG_BCNTR (0x14) /* SMC Byte Count Register */
> +#define SDXC_REG_CMDR (0x18) /* SMC Command Register */
> +#define SDXC_REG_CARG (0x1C) /* SMC Argument Register */
> +#define SDXC_REG_RESP0 (0x20) /* SMC Response Register 0 */
> +#define SDXC_REG_RESP1 (0x24) /* SMC Response Register 1 */
> +#define SDXC_REG_RESP2 (0x28) /* SMC Response Register 2 */
> +#define SDXC_REG_RESP3 (0x2C) /* SMC Response Register 3 */
> +#define SDXC_REG_IMASK (0x30) /* SMC Interrupt Mask Register */
> +#define SDXC_REG_MISTA (0x34) /* SMC Masked Interrupt Status Register */
> +#define SDXC_REG_RINTR (0x38) /* SMC Raw Interrupt Status Register */
> +#define SDXC_REG_STAS (0x3C) /* SMC Status Register */
> +#define SDXC_REG_FTRGL (0x40) /* SMC FIFO Threshold Watermark Registe */
> +#define SDXC_REG_FUNS (0x44) /* SMC Function Select Register */
> +#define SDXC_REG_CBCR (0x48) /* SMC CIU Byte Count Register */
> +#define SDXC_REG_BBCR (0x4C) /* SMC BIU Byte Count Register */
> +#define SDXC_REG_DBGC (0x50) /* SMC Debug Enable Register */
> +#define SDXC_REG_HWRST (0x78) /* SMC Card Hardware Reset for Register */
> +#define SDXC_REG_DMAC (0x80) /* SMC IDMAC Control Register */
> +#define SDXC_REG_DLBA (0x84) /* SMC IDMAC Descriptor List Base Addre */
> +#define SDXC_REG_IDST (0x88) /* SMC IDMAC Status Register */
> +#define SDXC_REG_IDIE (0x8C) /* SMC IDMAC Interrupt Enable Register */
> +#define SDXC_REG_CHDA (0x90)
> +#define SDXC_REG_CBDA (0x94)
> +
> +#define mci_readl(host, reg) \
> + readl((host)->reg_base + SDXC_##reg)
> +#define mci_writel(host, reg, value) \
> + writel((value), (host)->reg_base + SDXC_##reg)
Please use some inline functions here.
> +/* global control register bits */
> +#define SDXC_SOFT_RESET BIT(0)
> +#define SDXC_FIFO_RESET BIT(1)
> +#define SDXC_DMA_RESET BIT(2)
> +#define SDXC_HARDWARE_RESET (SDXC_SOFT_RESET|SDXC_FIFO_RESET|SDXC_DMA_RESET)
> +#define SDXC_INTERRUPT_ENABLE_BIT BIT(4)
> +#define SDXC_DMA_ENABLE_BIT BIT(5)
> +#define SDXC_DEBOUNCE_ENABLE_BIT BIT(8)
> +#define SDXC_POSEDGE_LATCH_DATA BIT(9)
> +#define SDXC_DDR_MODE BIT(10)
> +#define SDXC_MEMORY_ACCESS_DONE BIT(29)
> +#define SDXC_ACCESS_DONE_DIRECT BIT(30)
> +#define SDXC_ACCESS_BY_AHB BIT(31)
> +#define SDXC_ACCESS_BY_DMA (0U << 31)
Isn't it 0?
> +/* clock control bits */
> +#define SDXC_CARD_CLOCK_ON BIT(16)
> +#define SDXC_LOW_POWER_ON BIT(17)
> +/* bus width */
> +#define SDXC_WIDTH1 (0)
> +#define SDXC_WIDTH4 (1)
> +#define SDXC_WIDTH8 (2)
> +/* smc command bits */
> +#define SDXC_RESP_EXPIRE BIT(6)
> +#define SDXC_LONG_RESPONSE BIT(7)
> +#define SDXC_CHECK_RESPONSE_CRC BIT(8)
> +#define SDXC_DATA_EXPIRE BIT(9)
> +#define SDXC_WRITE BIT(10)
> +#define SDXC_SEQUENCE_MODE BIT(11)
> +#define SDXC_SEND_AUTO_STOP BIT(12)
> +#define SDXC_WAIT_PRE_OVER BIT(13)
> +#define SDXC_STOP_ABORT_CMD BIT(14)
> +#define SDXC_SEND_INIT_SEQUENCE BIT(15)
> +#define SDXC_UPCLK_ONLY BIT(21)
> +#define SDXC_READ_CEATA_DEV BIT(22)
> +#define SDXC_CCS_EXPIRE BIT(23)
> +#define SDXC_ENABLE_BIT_BOOT BIT(24)
> +#define SDXC_ALT_BOOT_OPTIONS BIT(25)
> +#define SDXC_BOOT_ACK_EXPIRE BIT(26)
> +#define SDXC_BOOT_ABORT BIT(27)
> +#define SDXC_VOLTAGE_SWITCH BIT(28)
> +#define SDXC_USE_HOLD_REGISTER BIT(29)
> +#define SDXC_START BIT(31)
> +/* interrupt bits */
> +#define SDXC_RESP_ERROR BIT(1)
> +#define SDXC_COMMAND_DONE BIT(2)
> +#define SDXC_DATA_OVER BIT(3)
> +#define SDXC_TX_DATA_REQUEST BIT(4)
> +#define SDXC_RX_DATA_REQUEST BIT(5)
> +#define SDXC_RESP_CRC_ERROR BIT(6)
> +#define SDXC_DATA_CRC_ERROR BIT(7)
> +#define SDXC_RESP_TIMEOUT BIT(8)
> +#define SDXC_DATA_TIMEOUT BIT(9)
> +#define SDXC_VOLTAGE_CHANGE_DONE BIT(10)
> +#define SDXC_FIFO_RUN_ERROR BIT(11)
> +#define SDXC_HARD_WARE_LOCKED BIT(12)
> +#define SDXC_START_BIT_ERROR BIT(13)
> +#define SDXC_AUTO_COMMAND_DONE BIT(14)
> +#define SDXC_END_BIT_ERROR BIT(15)
> +#define SDXC_SDIO_INTERRUPT BIT(16)
> +#define SDXC_CARD_INSERT BIT(30)
> +#define SDXC_CARD_REMOVE BIT(31)
> +#define SDXC_INTERRUPT_ERROR_BIT (SDXC_RESP_ERROR | SDXC_RESP_CRC_ERROR | \
> + SDXC_DATA_CRC_ERROR | SDXC_RESP_TIMEOUT | \
> + SDXC_DATA_TIMEOUT | SDXC_FIFO_RUN_ERROR | \
> + SDXC_HARD_WARE_LOCKED | SDXC_START_BIT_ERROR | \
> + SDXC_END_BIT_ERROR) /* 0xbbc2 */
> +#define SDXC_INTERRUPT_DONE_BIT (SDXC_AUTO_COMMAND_DONE | SDXC_DATA_OVER | \
> + SDXC_COMMAND_DONE | SDXC_VOLTAGE_CHANGE_DONE)
> +/* status */
> +#define SDXC_RXWL_FLAG BIT(0)
> +#define SDXC_TXWL_FLAG BIT(1)
> +#define SDXC_FIFO_EMPTY BIT(2)
> +#define SDXC_FIFO_FULL BIT(3)
> +#define SDXC_CARD_PRESENT BIT(8)
> +#define SDXC_CARD_DATA_BUSY BIT(9)
> +#define SDXC_DATA_FSM_BUSY BIT(10)
> +#define SDXC_DMA_REQUEST BIT(31)
> +#define SDXC_FIFO_SIZE (16)
> +/* Function select */
> +#define SDXC_CEATA_ON (0xceaaU << 16)
> +#define SDXC_SEND_IRQ_RESPONSE BIT(0)
> +#define SDXC_SDIO_READ_WAIT BIT(1)
> +#define SDXC_ABORT_READ_DATA BIT(2)
> +#define SDXC_SEND_CCSD BIT(8)
> +#define SDXC_SEND_AUTO_STOPCCSD BIT(9)
> +#define SDXC_CEATA_DEV_INTERRUPT_ENABLE_BIT BIT(10)
> +/* IDMA controller bus mod bit field */
> +#define SDXC_IDMAC_SOFT_RESET BIT(0)
> +#define SDXC_IDMAC_FIX_BURST BIT(1)
> +#define SDXC_IDMAC_IDMA_ON BIT(7)
> +#define SDXC_IDMAC_REFETCH_DES BIT(31)
> +/* IDMA status bit field */
> +#define SDXC_IDMAC_TRANSMIT_INTERRUPT BIT(0)
> +#define SDXC_IDMAC_RECEIVE_INTERRUPT BIT(1)
> +#define SDXC_IDMAC_FATAL_BUS_ERROR BIT(2)
> +#define SDXC_IDMAC_DESTINATION_INVALID BIT(4)
> +#define SDXC_IDMAC_CARD_ERROR_SUM BIT(5)
> +#define SDXC_IDMAC_NORMAL_INTERRUPT_SUM BIT(8)
> +#define SDXC_IDMAC_ABNORMAL_INTERRUPT_SUM BIT(9)
> +#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_TX BIT(10)
> +#define SDXC_IDMAC_HOST_ABORT_INTERRUPT_RX BIT(10)
> +#define SDXC_IDMAC_IDLE (0U << 13)
Ditto
> +#define SDXC_IDMAC_SUSPEND (1U << 13)
> +#define SDXC_IDMAC_DESC_READ (2U << 13)
> +#define SDXC_IDMAC_DESC_CHECK (3U << 13)
> +#define SDXC_IDMAC_READ_REQUEST_WAIT (4U << 13)
> +#define SDXC_IDMAC_WRITE_REQUEST_WAIT (5U << 13)
> +#define SDXC_IDMAC_READ (6U << 13)
> +#define SDXC_IDMAC_WRITE (7U << 13)
> +#define SDXC_IDMAC_DESC_CLOSE (8U << 13)
Please use BIT as much as possible here.
> +
> +/*
> +* If the idma-des-size-bits of property is ie 13, bufsize bits are:
> +* Bits 0-12: buf1 size
> +* Bits 13-25: buf2 size
> +* Bits 26-31: not used
> +* Since we only ever set buf1 size, we can simply store it directly.
> +*/
> +#define SDXC_IDMAC_DES0_DIC BIT(1) /* disable interrupt on completion */
> +#define SDXC_IDMAC_DES0_LD BIT(2) /* last descriptor */
> +#define SDXC_IDMAC_DES0_FD BIT(3) /* first descriptor */
> +#define SDXC_IDMAC_DES0_CH BIT(4) /* chain mode */
> +#define SDXC_IDMAC_DES0_ER BIT(5) /* end of ring */
> +#define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */
> +#define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */
Overall, I prefer to have the registers right beneath the register
declaration. It's easier to spot what bits belong to what registers
that way.
> +struct sunxi_idma_des {
> + u32 config;
> + u32 buf_size;
> + u32 buf_addr_ptr1;
> + u32 buf_addr_ptr2;
> +};
Some comments on top of this structure would be great.
> +struct sunxi_mmc_host {
> + struct mmc_host *mmc;
> + struct regulator *vmmc;
> +
> + /* IO mapping base */
> + void __iomem *reg_base;
> +
> + spinlock_t lock;
> + struct tasklet_struct tasklet;
> +
> + /* clock management */
> + struct clk *clk_ahb;
> + struct clk *clk_mod;
> +
> + /* ios information */
> + u32 clk_mod_rate;
> + u32 bus_width;
> + u32 idma_des_size_bits;
> + u32 ddr;
> + u32 voltage_switching;
> +
> + /* irq */
> + int irq;
> + u32 int_sum;
> + u32 sdio_imask;
> +
> + /* flags */
> + u32 power_on:1;
> + u32 io_flag:1;
> + u32 wait_dma:1;
bool?
> + dma_addr_t sg_dma;
> + void *sg_cpu;
> +
> + struct mmc_request *mrq;
> + u32 ferror;
> +};
Please be consistent in your spacing, either align the variable names,
or don't, but make a decision :)
> +#define MMC_CLK_400K 0
> +#define MMC_CLK_25M 1
> +#define MMC_CLK_50M 2
> +#define MMC_CLK_50MDDR 3
> +#define MMC_CLK_50MDDR_8BIT 4
> +#define MMC_CLK_100M 5
> +#define MMC_CLK_200M 6
> +#define MMC_CLK_MOD_NUM 7
Wouldn't an enum be better here?
> +
> +struct sunxi_mmc_clk_dly {
> + u32 mode;
> + u32 oclk_dly;
> + u32 sclk_dly;
> +};
Comments here would be great too.
Thanks for working on this!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v4 7/7] ARM: dts: imx6sl: Add power-domain information to gpc node
From: Philipp Zabel @ 2014-02-18 15:34 UTC (permalink / raw)
To: Shawn Guo
Cc: Rob Herring, Mark Rutland, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Philipp Zabel
In-Reply-To: <1392737687-25003-1-git-send-email-p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
The PGC that is part of GPC controls isolation and power sequencing of the
power domains. The PU power domain will be handled by the generic pm domain
framework and needs a phandle to the PU regulator to turn off power when
the domain is disabled.
Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
arch/arm/boot/dts/imx6sl.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index 28558f1..774e1fb 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -529,9 +529,27 @@
};
gpc: gpc@020dc000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
compatible = "fsl,imx6sl-gpc", "fsl,imx6q-gpc";
reg = <0x020dc000 0x4000>;
interrupts = <0 89 0x04>;
+ pu-supply = <®_pu>;
+
+ pd_display: display-power-domain@020dc240 {
+ compatible = "fsl,imx6q-power-domain";
+ reg = <0x020dc240 0x10>;
+ };
+
+ pd_pu: pu-power-domain@020dc260 {
+ compatible = "fsl,imx6q-power-domain";
+ reg = <0x020dc260 0x10>;
+ };
+
+ pd_arm: cpu-power-domain@020dc2a0 {
+ compatible = "fsl,imx6q-power-domain";
+ reg = <0x020dc2a0 0x10>;
+ };
};
gpr: iomuxc-gpr@020e0000 {
--
1.8.5.3
--
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 6/7] ARM: dts: imx6qdl: Add power-domain information to gpc node
From: Philipp Zabel @ 2014-02-18 15:34 UTC (permalink / raw)
To: Shawn Guo
Cc: Rob Herring, Mark Rutland, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Philipp Zabel
In-Reply-To: <1392737687-25003-1-git-send-email-p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
The PGC that is part of GPC controls isolation and power sequencing of the
power domains. The PU power domain will be handled by the generic pm domain
framework and needs a phandle to the PU regulator to turn off power when
the domain is disabled.
Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
arch/arm/boot/dts/imx6qdl.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 253d82c..fd1be55 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -598,9 +598,22 @@
};
gpc: gpc@020dc000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
compatible = "fsl,imx6q-gpc";
reg = <0x020dc000 0x4000>;
interrupts = <0 89 0x04 0 90 0x04>;
+ pu-supply = <®_pu>;
+
+ pd_pu: pu-power-domain@020dc260 {
+ compatible = "fsl,imx6q-power-domain";
+ reg = <0x020dc260 0x10>;
+ };
+
+ pd_arm: cpu-power-domain@020dc2a0 {
+ compatible = "fsl,imx6q-power-domain";
+ reg = <0x020dc2a0 0x10>;
+ };
};
gpr: iomuxc-gpr@020e0000 {
--
1.8.5.3
--
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
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