* [PATCH] mmc: sunxi: mark PM functions as __maybe_unused
From: Arnd Bergmann @ 2018-05-28 15:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAHp75VffA5Q2VxgkQqT6M_iLwZtv46Gy5Fz4Fx7PA9CvRnX0Ag@mail.gmail.com>
On Mon, May 28, 2018 at 2:12 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> +Cc: Jean
>
> On Mon, May 28, 2018 at 2:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, May 28, 2018 at 1:04 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 25 May 2018 at 23:07, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> The newly added runtime-pm functions cause a harmless warning
>>>> when CONFIG_PM is disabled:
>>>>
>>>> drivers/mmc/host/sunxi-mmc.c:1452:12: error: 'sunxi_mmc_runtime_suspend' defined but not used [-Werror=unused-function]
>>>> static int sunxi_mmc_runtime_suspend(struct device *dev)
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~
>>>> drivers/mmc/host/sunxi-mmc.c:1435:12: error: 'sunxi_mmc_runtime_resume' defined but not used [-Werror=unused-function]
>>>> static int sunxi_mmc_runtime_resume(struct device *dev)
>>>>
>>>> This marks them as __maybe_unused to shut up the warning.
>>>
>>> Most mmc drivers uses #ifdef CONFIG_PM instead of the __maybe_unused() option.
>>>
>>> It's not a big deal, but consistency is always good. Would you mind changing?
>>
>> I'd prefer not to. Most uses of #ifdef CONFIG_PM that get introduced are wrong,
>> and cause additional randconfig warnings that I end up having to fix,
>> so I always
>> do it with __maybe_unused.
>
> Some of the maintainers have strong objection against such changes.
> http://lkml.iu.edu/hypermail/linux/kernel/1805.1/06100.html
>
> It seems we might have a split in the opinions, which is not good in
> this case (consistency for PM callbacks overall will be broken).
We actually talked about this at the kernel summit last year, and the conclusion
was that we should replace the SET_RUNTIME_PM_OPS(),
SIMPLE_DEV_PM_OPS(), SET_SYSTEM_SLEEP_PM_OPS()
etc macros with new ones that don't require the ugly #ifdef and just silently
drop the unused functions.
Unfortunately we can't extend the existing macros to work that way (it
breaks for drivers that have an #ifdef today), and nobody could come up
with a new name that is at least as readable as the current one.
Note that incorrect #ifdef usage here is the most common source of
newly introduced build warnings and errors in the kernel, everyone gets
those wrong since they are just very confusing.
Arnd
^ permalink raw reply
* [PATCH] drivers/bus: arm-cci: fix build warnings
From: Arnd Bergmann @ 2018-05-28 15:41 UTC (permalink / raw)
To: linux-arm-kernel
When the arm-cci driver is enabled, but both CONFIG_ARM_CCI5xx_PMU and
CONFIG_ARM_CCI400_PMU are not, we get a warning about how parts of
the driver are never used:
drivers/perf/arm-cci.c:1454:29: error: 'cci_pmu_models' defined but not used [-Werror=unused-variable]
drivers/perf/arm-cci.c:693:16: error: 'cci_pmu_event_show' defined but not used [-Werror=unused-function]
drivers/perf/arm-cci.c:685:16: error: 'cci_pmu_format_show' defined but not used [-Werror=unused-function]
Marking all three functions as __maybe_unused avoids the warnings in
randconfig builds. I'm doing this lacking any ideas for a better fix.
Fixes: 3de6be7a3dd8 ("drivers/bus: Split Arm CCI driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/perf/arm-cci.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/perf/arm-cci.c b/drivers/perf/arm-cci.c
index e6fadc8e1178..0d09d8e669cd 100644
--- a/drivers/perf/arm-cci.c
+++ b/drivers/perf/arm-cci.c
@@ -120,9 +120,9 @@ enum cci_models {
static void pmu_write_counters(struct cci_pmu *cci_pmu,
unsigned long *mask);
-static ssize_t cci_pmu_format_show(struct device *dev,
+static ssize_t __maybe_unused cci_pmu_format_show(struct device *dev,
struct device_attribute *attr, char *buf);
-static ssize_t cci_pmu_event_show(struct device *dev,
+static ssize_t __maybe_unused cci_pmu_event_show(struct device *dev,
struct device_attribute *attr, char *buf);
#define CCI_EXT_ATTR_ENTRY(_name, _func, _config) \
@@ -1451,7 +1451,7 @@ static int cci_pmu_offline_cpu(unsigned int cpu)
return 0;
}
-static struct cci_pmu_model cci_pmu_models[] = {
+static __maybe_unused struct cci_pmu_model cci_pmu_models[] = {
#ifdef CONFIG_ARM_CCI400_PMU
[CCI400_R0] = {
.name = "CCI_400",
--
2.9.0
^ permalink raw reply related
* [PATCH] ARM: mcpm, perf/arm-cci: export mcpm_is_available
From: Arnd Bergmann @ 2018-05-28 15:44 UTC (permalink / raw)
To: linux-arm-kernel
Now that the ARM CCI PMU driver can be built as a loadable module,
we get a link failure when MCPM is enabled:
ERROR: "mcpm_is_available" [drivers/perf/arm-cci.ko] undefined!
The simplest fix is to export that helper function.
Fixes: 8b0c93c20ef7 ("perf/arm-cci: Allow building as a module")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
The patch that caused this is currently part of the arm-perf/for-next/perf
branch, it would be good to have the fix there as well.
---
arch/arm/common/mcpm_entry.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/common/mcpm_entry.c b/arch/arm/common/mcpm_entry.c
index ed9e87ddbb06..037a4479b8c3 100644
--- a/arch/arm/common/mcpm_entry.c
+++ b/arch/arm/common/mcpm_entry.c
@@ -9,6 +9,7 @@
* published by the Free Software Foundation.
*/
+#include <linux/export.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/irqflags.h>
@@ -174,6 +175,7 @@ bool mcpm_is_available(void)
{
return (platform_ops) ? true : false;
}
+EXPORT_SYMBOL_GPL(mcpm_is_available);
/*
* We can't use regular spinlocks. In the switcher case, it is possible
--
2.9.0
^ permalink raw reply related
* [PATCH] PCI: versatile: include pci.h
From: Arnd Bergmann @ 2018-05-28 15:48 UTC (permalink / raw)
To: linux-arm-kernel
Two patches for PCI host drivers clashed recently, leading to a broken
build for versatile-pci:
drivers/pci/host/pci-versatile.c: In function 'versatile_pci_parse_request_of_pci_ranges':
drivers/pci/host/pci-versatile.c:70:8: error: implicit declaration of function 'devm_of_pci_get_host_bridge_resources'; did you mean 'pci_get_host_bridge_device'? [-Werror=implicit-function-declaration]
This adds the missing header inclusion that became necessary now.
Fixes: 9e2aee80c78d ("PCI: Move private DT related functions into private header")
Fixes: 64720fd1f630 ("PCI: Rework of_pci_get_host_bridge_resources() to devm_of_pci_get_host_bridge_resources()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/pci/host/pci-versatile.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c
index ff2cd12b3978..994f32061b32 100644
--- a/drivers/pci/host/pci-versatile.c
+++ b/drivers/pci/host/pci-versatile.c
@@ -15,6 +15,8 @@
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include "../pci.h"
+
static void __iomem *versatile_pci_base;
static void __iomem *versatile_cfg_base[2];
--
2.9.0
^ permalink raw reply related
* [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU
From: Sebastian Reichel @ 2018-05-28 15:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180528072034.GB3143@dragon>
Hi,
On Mon, May 28, 2018 at 03:20:35PM +0800, Shawn Guo wrote:
> On Mon, May 28, 2018 at 08:41:31AM +0200, Sebastian Reichel wrote:
> > > Are you saying this is a very specific setup required by i.MX53 only?
> >
> > Yes, all other SoCs supported by Linux ARM PMU counters driver can
> > just use the registers without having to enable platform specific
> > bits first.
> >
> > > In that case, I can live with it.
> >
> > What about the DT node? I did not add it, since this is a i.MX53
> > specific workaround anyways.
>
> What you are adding here is secure-reg-access property, which has an
> defined meaning in PMU binding doc. I'm not really sure if it's
> appropriate to use the property as a condition for DBGEN bit setup.
> Or can we set up the bit regardless of the property?
The description for DBGEN bit is:
> Debug enable. This allows the user to manually activate clocks
> within the debug system. This register bit directly controls the
> platform's dbgen_out output signal which connects to the DAP_SYS to
> enable all debug clocks. Once enabled, the clocks cannot be disabled
> except by asserting the disable_trace input of the DAP_SYS.
I only enable this bit when the kernel configuration allows
using the PMU, since otherwise we do not need the clocks
in the debug system. This limits any potential side-effects
of this patchset.
-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180528/a922700f/attachment.sig>
^ permalink raw reply
* [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
From: Peter Rosin @ 2018-05-28 15:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180528162718.6be7191f@bbrezillon>
On 2018-05-28 16:27, Boris Brezillon wrote:
> Hi Peter,
>
> On Mon, 28 May 2018 12:10:02 +0200
> Peter Rosin <peda@axentia.se> wrote:
>
>> On 2018-05-28 00:11, Peter Rosin wrote:
>>> On 2018-05-27 11:18, Peter Rosin wrote:
>>>> On 2018-05-25 16:51, Tudor Ambarus wrote:
>>>>> We think the best way is to keep LCD on DDR Ports 2 and 3 (8th and 9th
>>>>> slaves), to have maximum bandwidth and to use DMA on DDR port 1 for NAND
>>>>> (7th slave).
>>>>
>>>> Exactly how do I accomplish that?
>>>>
>>>> I can see how I can move the LCD between slave DDR port 2 and 3 by
>>>> selecting LCDC DMA master 8 or 9 (but according to the above it should
>>>> not matter). The big question is how I control what slave the NAND flash
>>>> is going to use? I find nothing in the datasheet, and the code is also
>>>> non-transparent enough for me to figure it out by myself without
>>>> throwing out this question first...
>>>
>>> I added this:
>>>
>>> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
>>> index e686fe73159e..3b33c63d2ed4 100644
>>> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
>>> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
>>> @@ -1991,6 +1991,9 @@ static int atmel_nand_controller_init(struct atmel_nand_controller *nc,
>>> nc->dmac = dma_request_channel(mask, NULL, NULL);
>>> if (!nc->dmac)
>>> dev_err(nc->dev, "Failed to request DMA channel\n");
>>> +
>>> + dev_info(nc->dev, "using %s for DMA transfers\n",
>>> + dma_chan_name(nc->dmac));
>>> }
>>>
>>> /* We do not retrieve the SMC syscon when parsing old DTs. */
>>>
>>>
>>>
>>> and the output is
>>>
>>> atmel-nand-controller 10000000.ebi:nand-controller: using dma0chan5 for DMA transfers
>>>
>>> So, DMA controller 0 is in use. I still don't know if IF0, IF1 or IF2 is used
>>> or how to find out. I guess IF2 is not in use since that does not allow any
>>> DDR2 port as slave...
>>>
>>> From the datasheet, DMAC0/IF0 uses DDR2 Port 2, and DMAC0/IF1 uses DDR2 Port 1.
>>> But, by the looks of the register content in my other mail, it seems as if
>>> DMA0/IF1 can also use DDR2 Port 3.
>>>
>>> So, I think I want either
>>>
>>> A) the NAND controller to use DMAC0/IF0 (i.e. DDR2 port 1, and possibly 3) and
>>> the LCDC to use master 9 (i.e. DDR2 Port 2)
>>>
>>> or
>>>
>>> B) the NAND controller to use DMAC1/IF1 (i.e. DDR2 port 2) and the LCDC to use
>>> master 8 (i.e. DDR2 Port 3)
>>
>> Crap, that was not what I meant to express. Sorry for the confusion. This is
>> better.
>>
>> So, I think I want either
>>
>> A) the NAND controller to use master 1 DMAC0/IF0 (i.e. slave 8 DDR2 port 2) and
>> the LCDC to use master 9 (i.e. slave 9 DDR2 Port 3)
>>
>> or
>>
>> B) the NAND controller to use master 2 DMAC0/IF1 (i.e. slave 7 DDR2 port 1, and
>> possibly slave 9 DDR2 port 3 (if my previous findings are relevant) and the
>> LCDC to use master 8 (i.e. slave 8 DDR2 Port 2)
>>
>>> But, again, how do I limit DMAC0 to either of IF0 or IF1 for NAND accesses?
>>
>> So, I added a horrid patch (attached), which mainly adds printk lines, but
>> additionally does one more thing in atc_prep_dma_memcpy. It changes the DSCR_IF
>> field (from 0) to 1 for DMA-memcpy for dma0chan5 (i.e. the NAND). At least I
>> think it does that?
>>
>> Running with that patch gets me this:
>>
>> # dmesg | grep -i dma
>> at_hdmac ffffe600.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), 8 channels
>> at_hdmac ffffe800.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), 8 channels
>> dma dma0chan0: xlate 0 2
>> dma dma0chan1: xlate 0 2
>> at91_i2c f0014000.i2c: using dma0chan0 (tx) and dma0chan1 (rx) for DMA transfers
>> dma dma1chan0: xlate 0 2
>> dma dma1chan1: xlate 0 2
>> at91_i2c f801c000.i2c: using dma1chan0 (tx) and dma1chan1 (rx) for DMA transfers
>> dma dma0chan2: xlate 0 2
>> dma dma0chan3: xlate 0 2
>> dma dma0chan4: xlate 0 2
>> atmel_mci f0000000.mmc: using dma0chan4 for DMA transfers
>> dma dma1chan2: xlate 0 2
>> dma dma1chan3: xlate 0 2
>> atmel_aes f8038000.aes: Atmel AES - Using dma1chan2, dma1chan3 for DMA transfers
>> dma dma1chan4: xlate 0 2
>> atmel_sha f8034000.sha: using dma1chan4 for DMA transfers
>> dma dma1chan5: xlate 0 2
>> dma dma1chan6: xlate 0 2
>> atmel_tdes f803c000.tdes: using dma1chan5, dma1chan6 for DMA transfers
>> atmel-nand-controller 10000000.ebi:nand-controller: using dma0chan5 for DMA transfers
>> dma dma0chan5: memcpy: 0
>> dma dma0chan5: DSCR_IF: 1
>> dma dma0chan5: memcpy: 1
>>
>> So, output is as expected and I believe that the patch makes the NAND DMA
>> accesses use master 2 DMAC0/IF1 and are thus forced to use slave 7 DDR2 Port 1
>> (and possibly 9). The LCDC is using slave 8 DDR2 Port 2. So there should be no
>> slave conflict?
>>
>> But the on-screen crap remains during NAND accesses.
>>
>> But pressing on.
>>
>> I then changed the priorities for all accesses to 0 in the PRxSy registers, except
>> the ones for masters 8/9 LCDC (slaves 8/9) which I left at priority 3.
>>
>> But the on-screen crap remains during NAND accesses.
>>
>> My guess is that the NAND DMA is doing too long bursts and that the LCDC therefore
>> has to wait too long and simply fails to keep the pipeline from running short?
>>
>> So I tried to reduce the maximum SLOT_CYCLE for slaves 7 and 9 in the SCFGx
>> registers. No noticeable effect either.
>>
>> I then tried to split bursts from master 2 (DMAC0/IF1) with small values in the
>> MCFG2 register. No effect.
>>
>> I'm getting nowhere.
>
> Could it just be that you're reaching the DDR bus limit. As I said
> previously, when you go through the CPU, and assuming you're consuming
> the data directly, you have:
>
> 1/ NFC SRAM -> CPU
> 2/ CPU -> L1 data cache --write-back--> DRAM
> 3/ L1-cache -> CPU
>
> While, if you use DMA you get:
>
> 1/ NFC SRAM -> DRAM
> 2/ SDRAM -> L1 data cache -> CPU
>
> So, if you're approaching the limit of (LP)DDR bandwidth, using the CPU
> might make things a bit better. Still, if the limitation really comes
> from the DDR bus, my opinion is that you should maybe use a smaller
> resolution or use a more compact pixel format (RGB565?).
The issue is still there if I use a CLUT mode instead of rgb565, which is
what I normally use (and what I would like to use, CLUT is just alien and
a pain these days).
The panels we are using only supports one resolution (each), but the issue
is there with both 1920x1080 at 16bpp and 1024x768 at 8bpp (~60Hz).
> Did you calculate how much of the bandwidth is taken by the HLCDC
> block and compared it to the max (LP)DDR bandwidth?
I did, but don't remember the exact details. There is some room even for
1920x1080 at 16bpp, but not oceans of it. We were a bit uncertain if 16bpp
would be possible, and in fact that was the reason I worked on CLUT
support for atmel-hlcdc last year. But since the problem persists with
much less memory pressure as well, I don't think that's it either.
Admittedly I have not tested these AHB matrix tricks with a smaller
panel (it would take a bit of work to arrange for those tests), but the
issue was there when I last tried (using defaults).
Cheers,
Peter
^ permalink raw reply
* [patch v22 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2018-05-28 15:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180528123527.GA17613@kroah.com>
Hi Greg.
Thanks for your review.
Please see my comments inline.
Best Regards,
Oleksandr Shamray
> -----Original Message-----
> From: Greg KH [mailto:gregkh at linuxfoundation.org]
> Sent: 28 ??? 2018 ?. 15:35
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: arnd at arndb.de; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; devicetree at vger.kernel.org;
> openbmc at lists.ozlabs.org; joel at jms.id.au; jiri at resnulli.us;
> tklauser at distanz.ch; linux-serial at vger.kernel.org; Vadim Pasternak
> <vadimp@mellanox.com>; system-sw-low-level <system-sw-low-
> level at mellanox.com>; robh+dt at kernel.org; openocd-devel-
> owner at lists.sourceforge.net; linux-api at vger.kernel.org;
> davem at davemloft.net; mchehab at kernel.org
> Subject: Re: [patch v22 1/4] drivers: jtag: Add JTAG core driver
>
> On Mon, May 28, 2018 at 01:34:09PM +0300, Oleksandr Shamray wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support hardware/software
> > JTAG platform drivers. It provide user layer API interface for
> > flashing and debugging external devices which equipped with JTAG
> > interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next entry
> > in dev folder:
> > /dev/jtagX
> >
> > Signed-off-by: Oleksandr Shamray <oleksandrs@mellanox.com>
> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> > Acked-by: Philippe Ombredanne <pombredanne@nexb.com>
> > ---
> > v21->v22
>
> 22 versions, way to stick with this...
>
> Anyway, time to review it again. I feel it's really close, but I don't want to
> apply it yet as the api feels odd in places. If others have asked you to make
> these changes to look like this, I'm sorry, then please push back on me:
>
> > +/*
> > + * JTAG core driver supports up to 256 devices
> > + * JTAG device name will be in range jtag0-jtag255
> > + * Set maximum len of jtag id to 3
> > + */
> > +
> > +#define MAX_JTAG_DEVS 255
>
> Why have a max at all? You should be able to just dynamically allocate them.
> Anyway, if you do want a max, you need to be able to properly keep the max
> number, and right now you have a race when registering (you check the
> number, and then much later do you increment it, a pointless use of an
> atomic value if I've ever seen one...)
>
You are right. It's not good idea to have restriction of max dev number.
I will remove all regarding It.
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 3)
> > +
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> If you are a miscdev, why even have a max number? Just let the max
> number of miscdev devices rule things.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
>
> You shouldn't care about this, but ok...
Jtag HW not support to using with multiple requests from different users. So we prohibit this.
>
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
> > +
> > +static DEFINE_IDA(jtag_ida);
> > +
> > +static atomic_t jtag_refcount = ATOMIC_INIT(0);
>
> Either use it as an atomic properly (test_and_set), or just leave it as an int, or
> better yet, don't use it at all.
It will be removed.
>
> > +void *jtag_priv(struct jtag *jtag)
> > +{
> > + return jtag->priv;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_priv);
>
> Api naming issue here. Traditionally this is a "get/set_drvdata" type function,
> as in dev_get_drvdata(), or dev_set_drvdata. But, you are right in that the
> network stack has a netdev_priv() call, where you probably copied this idea
> from.
>
> I don't know, I guess it's ok as-is, as it's the way networking does it, it's your
> call though.
>
Yes, I did as in networking.
> > +static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned
> > +long arg) {
> > + struct jtag *jtag = file->private_data;
> > + struct jtag_run_test_idle idle;
> > + struct jtag_xfer xfer;
> > + u8 *xfer_data;
> > + u32 data_size;
> > + u32 value;
> > + int err;
> > +
> > + if (!arg)
> > + return -EINVAL;
> > +
> > + switch (cmd) {
> > + case JTAG_GIOCFREQ:
> > + if (!jtag->ops->freq_get)
> > + return -EOPNOTSUPP;
> > +
> > + err = jtag->ops->freq_get(jtag, &value);
> > + if (err)
> > + break;
> > +
> > + if (put_user(value, (__u32 __user *)arg))
> > + err = -EFAULT;
> > + break;
> > +
> > + case JTAG_SIOCFREQ:
> > + if (!jtag->ops->freq_set)
> > + return -EOPNOTSUPP;
> > +
> > + if (get_user(value, (__u32 __user *)arg))
> > + return -EFAULT;
> > + if (value == 0)
> > + return -EINVAL;
> > +
> > + err = jtag->ops->freq_set(jtag, value);
> > + break;
> > +
> > + case JTAG_IOCRUNTEST:
> > + if (copy_from_user(&idle, (const void __user *)arg,
> > + sizeof(struct jtag_run_test_idle)))
> > + return -EFAULT;
> > +
> > + if (idle.endstate > JTAG_STATE_PAUSEDR)
> > + return -EINVAL;
>
> No validation that idle contains sane values? Don't make every jtag driver
> have to do this type of validation please.
>
I have partially validation of idle structure (if (idle.endstate > JTAG_STATE_PAUSEDR)).
But I will add more validation.
>
> > +
> > + err = jtag->ops->idle(jtag, &idle);
> > + break;
> > +
> > + case JTAG_IOCXFER:
> > + if (copy_from_user(&xfer, (const void __user *)arg,
> > + sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > +
> > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > + return -EINVAL;
> > +
> > + if (xfer.type > JTAG_SDR_XFER)
> > + return -EINVAL;
> > +
> > + if (xfer.direction > JTAG_WRITE_XFER)
> > + return -EINVAL;
> > +
> > + if (xfer.endstate > JTAG_STATE_PAUSEDR)
> > + return -EINVAL;
> > +
>
> See, you do good error checking here, why not for the previous ioctl?
>
>
> > + data_size = DIV_ROUND_UP(xfer.length, BITS_PER_BYTE);
> > + xfer_data = memdup_user(u64_to_user_ptr(xfer.tdio),
> data_size);
> > +
>
> No blank line.
>
Will remove
> > + if (IS_ERR(xfer_data))
> > + return -EFAULT;
> > +
> > + err = jtag->ops->xfer(jtag, &xfer, xfer_data);
> > + if (err) {
> > + kfree(xfer_data);
> > + return -EFAULT;
>
> Why not return the error given to you? -EFAULT is only if there is a memory
> error in a copy_to/from_user() call.
>
Will change return code to err
> > + }
> > +
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)xfer_data, data_size);
> > + kfree(xfer_data);
> > + if (err)
> > + return -EFAULT;
>
> Like here, that's correct.
>
> > +
> > + if (copy_to_user((void __user *)arg, (void *)&xfer,
> > + sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > + break;
> > +
> > + case JTAG_GIOCSTATUS:
> > + err = jtag->ops->status_get(jtag, &value);
> > + if (err)
> > + break;
> > +
> > + err = put_user(value, (__u32 __user *)arg);
> > + break;
> > + case JTAG_SIOCMODE:
> > + if (!jtag->ops->mode_set)
> > + return -EOPNOTSUPP;
> > +
> > + if (get_user(value, (__u32 __user *)arg))
> > + return -EFAULT;
> > + if (value == 0)
> > + return -EINVAL;
> > +
> > + err = jtag->ops->mode_set(jtag, value);
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > + return err;
> > +}
> > +
> > +static int jtag_open(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > +miscdev);
> > +
> > + if (mutex_lock_interruptible(&jtag->open_lock))
> > + return -ERESTARTSYS;
>
> Why restart? Just grab the lock, you don't want to have to do restartable
> logic in userspace, right?
Will change like:
ret = mutex_lock_interruptible(&jtag->open_lock);
if (ret)
return ret;
>
> > +
> > + if (jtag->opened) {
> > + mutex_unlock(&jtag->open_lock);
> > + return -EBUSY;
>
> Why do you care about only 1 userspace open call? What does that buy you?
> Userspace can always pass around that file descriptor and use it in multiple
> places, so you are not really "protecting" yourself from anything.
>
Accessing to JTAG HW from multiple processes will make confusion in the work of the JTAG protocol.
And I want to prohibit this.
> And better yet, if you get rid of this, your open/release functions get very
> tiny and simple.
>
> > + }
> > + jtag->opened = true;
> > + mutex_unlock(&jtag->open_lock);
> > +
> > + nonseekable_open(inode, file);
>
> No error checking of the return value? Not good :(
Will add error handling
>
> > + file->private_data = jtag;
> > + return 0;
> > +}
> > +
> > +static int jtag_release(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = file->private_data;
> > +
> > + mutex_lock(&jtag->open_lock);
> > + jtag->opened = false;
> > + mutex_unlock(&jtag->open_lock);
> > + return 0;
> > +}
> > +
> > +static const struct file_operations jtag_fops = {
> > + .owner = THIS_MODULE,
> > + .open = jtag_open,
> > + .release = jtag_release,
> > + .llseek = noop_llseek,
> > + .unlocked_ioctl = jtag_ioctl,
> > +};
> > +
> > +struct jtag *jtag_alloc(struct device *host, size_t priv_size,
> > + const struct jtag_ops *ops)
> > +{
> > + struct jtag *jtag;
> > +
> > + if (!host)
> > + return NULL;
> > +
> > + if (!ops)
> > + return NULL;
> > +
> > + if (!ops->idle || !ops->status_get || !ops->xfer)
> > + return NULL;
> > +
> > + jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL);
> > + if (!jtag)
> > + return NULL;
> > +
> > + jtag->ops = ops;
> > + jtag->miscdev.parent = host;
> > +
> > + return jtag;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_alloc);
> > +
> > +void jtag_free(struct jtag *jtag)
> > +{
> > + kfree(jtag);
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_free);
> > +
> > +static int jtag_register(struct jtag *jtag) {
> > + struct device *dev = jtag->miscdev.parent;
> > + char *name;
> > + int err;
> > + int id;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + if (atomic_read(&jtag_refcount) >= MAX_JTAG_DEVS)
> > + return -ENOSPC;
>
> Here, you are reading the value, and then setting it a long ways away.
> What happens if two people call this at the same time. Not good.
> Either do it correctly, or better yet, don't do it at all.
>
Removed.
> > +
> > + id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
> > +
> > + jtag->id = id;
> > + jtag->opened = false;
> > +
> > + name = kzalloc(MAX_JTAG_NAME_LEN, GFP_KERNEL);
> > + if (!name) {
> > + err = -ENOMEM;
> > + goto err_jtag_alloc;
> > + }
> > +
> > + err = snprintf(name, MAX_JTAG_NAME_LEN, "jtag%d", id);
> > + if (err < 0)
> > + goto err_jtag_name;
> > +
> > + mutex_init(&jtag->open_lock);
> > + jtag->miscdev.fops = &jtag_fops;
> > + jtag->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + jtag->miscdev.name = name;
> > +
> > + err = misc_register(&jtag->miscdev);
> > + if (err) {
> > + dev_err(jtag->miscdev.parent, "Unable to register
> device\n");
> > + goto err_jtag_name;
> > + }
> > + pr_info("%s %d\n", __func__, __LINE__);
> > + atomic_inc(&jtag_refcount);
> > + return 0;
> > +
> > +err_jtag_name:
> > + kfree(name);
> > +err_jtag_alloc:
> > + ida_simple_remove(&jtag_ida, id);
> > + return err;
> > +}
> > +
> > +static void jtag_unregister(struct jtag *jtag) {
> > + misc_deregister(&jtag->miscdev);
> > + kfree(jtag->miscdev.name);
> > + ida_simple_remove(&jtag_ida, jtag->id);
> > + atomic_dec(&jtag_refcount);
> > +}
> > +
> > +static void devm_jtag_unregister(struct device *dev, void *res) {
> > + jtag_unregister(*(struct jtag **)res); }
> > +
> > +int devm_jtag_register(struct device *dev, struct jtag *jtag) {
> > + struct jtag **ptr;
> > + int ret;
> > +
> > + ptr = devres_alloc(devm_jtag_unregister, sizeof(*ptr),
> GFP_KERNEL);
> > + if (!ptr)
> > + return -ENOMEM;
> > +
> > + ret = jtag_register(jtag);
> > + if (!ret) {
> > + *ptr = jtag;
> > + devres_add(dev, ptr);
> > + } else {
> > + devres_free(ptr);
> > + }
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_jtag_register);
> > +
> > +static void __exit jtag_exit(void)
> > +{
> > + ida_destroy(&jtag_ida);
> > +}
> > +
> > +module_exit(jtag_exit);
> > +
> > +MODULE_AUTHOR("Oleksandr Shamray <oleksandrs@mellanox.com>");
> > +MODULE_DESCRIPTION("Generic jtag support"); MODULE_LICENSE("GPL
> v2");
> > diff --git a/include/linux/jtag.h b/include/linux/jtag.h new file mode
> > 100644 index 0000000..56d784d
> > --- /dev/null
> > +++ b/include/linux/jtag.h
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// include/linux/jtag.h - JTAG class driver // // Copyright (c) 2018
> > +Mellanox Technologies. All rights reserved.
> > +// Copyright (c) 2018 Oleksandr Shamray <oleksandrs@mellanox.com>
> > +
> > +#ifndef __JTAG_H
> > +#define __JTAG_H
> > +
> > +#include <uapi/linux/jtag.h>
> > +
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
> Why do you need such a horrid cast? What is this for? And why use uintptr_t
> at all? It's not a "normal" kernel type.
>
Not needed - will be removed.
> > +
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
> > +
> > +struct jtag;
> > +/**
> > + * struct jtag_ops - callbacks for JTAG control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by dev driver
> > + * @freq_set: set frequency function. Filled by dev driver
> > + * @status_get: set status function. Mandatory func. Filled by dev
> > +driver
> > + * @idle: set JTAG to idle state function. Mandatory func. Filled by
> > +dev driver
> > + * @xfer: send JTAG xfer function. Mandatory func. Filled by dev
> > +driver
> > + * @mode_set: set specific work mode for JTAG. Filled by dev driver
> > +*/ struct jtag_ops {
> > + int (*freq_get)(struct jtag *jtag, u32 *freq);
> > + int (*freq_set)(struct jtag *jtag, u32 freq);
> > + int (*status_get)(struct jtag *jtag, u32 *state);
> > + int (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);
> > + int (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer, u8 *xfer_data);
> > + int (*mode_set)(struct jtag *jtag, u32 mode_mask); };
> > +
> > +void *jtag_priv(struct jtag *jtag);
> > +int devm_jtag_register(struct device *dev, struct jtag *jtag); void
> > +devm_jtag_unregister(struct device *dev, void *res); struct jtag
> > +*jtag_alloc(struct device *host, size_t priv_size,
> > + const struct jtag_ops *ops);
> > +void jtag_free(struct jtag *jtag);
> > +
> > +#endif /* __JTAG_H */
> > diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h new
> > file mode 100644 index 0000000..8bbf1a7
> > --- /dev/null
> > +++ b/include/uapi/linux/jtag.h
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// include/uapi/linux/jtag.h - JTAG class driver uapi // // Copyright
> > +(c) 2018 Mellanox Technologies. All rights reserved.
> > +// Copyright (c) 2018 Oleksandr Shamray <oleksandrs@mellanox.com>
> > +
> > +#ifndef __UAPI_LINUX_JTAG_H
> > +#define __UAPI_LINUX_JTAG_H
> > +
> > +/*
> > + * JTAG_XFER_HW_MODE: JTAG hardware mode. Used to set HW drived
> or
> > +bitbang
> > + * mode. This is bitmask param of ioctl JTAG_SIOCMODE command */
> > +#define JTAG_XFER_HW_MODE 1
> > +
> > +/**
> > + * enum jtag_endstate:
> > + *
> > + * @JTAG_STATE_IDLE: JTAG state machine IDLE state
> > + * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state
> > + * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state */
> enum
> > +jtag_endstate {
> > + JTAG_STATE_IDLE,
> > + JTAG_STATE_PAUSEIR,
> > + JTAG_STATE_PAUSEDR,
> > +};
> > +
> > +/**
> > + * enum jtag_xfer_type:
> > + *
> > + * @JTAG_SIR_XFER: SIR transfer
> > + * @JTAG_SDR_XFER: SDR transfer
> > + */
> > +enum jtag_xfer_type {
> > + JTAG_SIR_XFER,
> > + JTAG_SDR_XFER,
> > +};
> > +
> > +/**
> > + * enum jtag_xfer_direction:
> > + *
> > + * @JTAG_READ_XFER: read transfer
> > + * @JTAG_WRITE_XFER: write transfer
> > + */
> > +enum jtag_xfer_direction {
> > + JTAG_READ_XFER,
> > + JTAG_WRITE_XFER,
> > +};
> > +
> > +/**
> > + * struct jtag_run_test_idle - forces JTAG state machine to
> > + * RUN_TEST/IDLE state
> > + *
> > + * @reset: 0 - run IDLE/PAUSE from current state
> > + * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE
> > + * @end: completion flag
> > + * @tck: clock counter
> > + *
> > + * Structure provide interface to JTAG device for JTAG IDLE execution.
> > + */
> > +struct jtag_run_test_idle {
> > + __u8 reset;
> > + __u8 endstate;
> > + __u8 tck;
> > +};
> > +
> > +/**
> > + * struct jtag_xfer - jtag xfer:
> > + *
> > + * @type: transfer type
> > + * @direction: xfer direction
> > + * @length: xfer bits len
> > + * @tdio : xfer data array
> > + * @endir: xfer end state
> > + *
> > + * Structure provide interface to JTAG device for JTAG SDR/SIR xfer
> execution.
> > + */
> > +struct jtag_xfer {
> > + __u8 type;
> > + __u8 direction;
> > + __u8 endstate;
> > + __u8 padding;
> > + __u32 length;
> > + __u64 tdio;
> > +};
> > +
> > +/* ioctl interface */
> > +#define __JTAG_IOCTL_MAGIC 0xb2
> > +
> > +#define JTAG_IOCRUNTEST _IOW(__JTAG_IOCTL_MAGIC, 0,\
> > + struct jtag_run_test_idle)
>
> No need for 2 lines here, fits just fine on one.
Ok
>
> > +#define JTAG_SIOCFREQ _IOW(__JTAG_IOCTL_MAGIC, 1, unsigned
> int)
> > +#define JTAG_GIOCFREQ _IOR(__JTAG_IOCTL_MAGIC, 2, unsigned int)
> > +#define JTAG_IOCXFER _IOWR(__JTAG_IOCTL_MAGIC, 3, struct
> jtag_xfer)
> > +#define JTAG_GIOCSTATUS _IOWR(__JTAG_IOCTL_MAGIC, 4, enum
> jtag_endstate)
> > +#define JTAG_SIOCMODE _IOW(__JTAG_IOCTL_MAGIC, 5, unsigned
> int)
> > +
> > +#define JTAG_FIRST_MINOR 0
>
> Why is this in a uapi file?
Not needed - will be removed.
>
> > +#define JTAG_MAX_DEVICES 32
>
> This is never used, why is it in a uapi file?
>
Not needed - will be removed.
> So, almost there, with the above mentioned things, I think you can make the
> code even simpler, which is always better, right?
>
> thanks,
>
> greg k-h
Thanks.
BR,
Oleksandr Shamray
^ permalink raw reply
* [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
From: Boris Brezillon @ 2018-05-28 16:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <db31aac0-6b59-8835-2181-635670e7f176@axentia.se>
On Mon, 28 May 2018 17:52:53 +0200
Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-28 16:27, Boris Brezillon wrote:
> > Hi Peter,
> >
> > On Mon, 28 May 2018 12:10:02 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >
> >> On 2018-05-28 00:11, Peter Rosin wrote:
> >>> On 2018-05-27 11:18, Peter Rosin wrote:
> >>>> On 2018-05-25 16:51, Tudor Ambarus wrote:
> >>>>> We think the best way is to keep LCD on DDR Ports 2 and 3 (8th and 9th
> >>>>> slaves), to have maximum bandwidth and to use DMA on DDR port 1 for NAND
> >>>>> (7th slave).
> >>>>
> >>>> Exactly how do I accomplish that?
> >>>>
> >>>> I can see how I can move the LCD between slave DDR port 2 and 3 by
> >>>> selecting LCDC DMA master 8 or 9 (but according to the above it should
> >>>> not matter). The big question is how I control what slave the NAND flash
> >>>> is going to use? I find nothing in the datasheet, and the code is also
> >>>> non-transparent enough for me to figure it out by myself without
> >>>> throwing out this question first...
> >>>
> >>> I added this:
> >>>
> >>> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
> >>> index e686fe73159e..3b33c63d2ed4 100644
> >>> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> >>> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> >>> @@ -1991,6 +1991,9 @@ static int atmel_nand_controller_init(struct atmel_nand_controller *nc,
> >>> nc->dmac = dma_request_channel(mask, NULL, NULL);
> >>> if (!nc->dmac)
> >>> dev_err(nc->dev, "Failed to request DMA channel\n");
> >>> +
> >>> + dev_info(nc->dev, "using %s for DMA transfers\n",
> >>> + dma_chan_name(nc->dmac));
> >>> }
> >>>
> >>> /* We do not retrieve the SMC syscon when parsing old DTs. */
> >>>
> >>>
> >>>
> >>> and the output is
> >>>
> >>> atmel-nand-controller 10000000.ebi:nand-controller: using dma0chan5 for DMA transfers
> >>>
> >>> So, DMA controller 0 is in use. I still don't know if IF0, IF1 or IF2 is used
> >>> or how to find out. I guess IF2 is not in use since that does not allow any
> >>> DDR2 port as slave...
> >>>
> >>> From the datasheet, DMAC0/IF0 uses DDR2 Port 2, and DMAC0/IF1 uses DDR2 Port 1.
> >>> But, by the looks of the register content in my other mail, it seems as if
> >>> DMA0/IF1 can also use DDR2 Port 3.
> >>>
> >>> So, I think I want either
> >>>
> >>> A) the NAND controller to use DMAC0/IF0 (i.e. DDR2 port 1, and possibly 3) and
> >>> the LCDC to use master 9 (i.e. DDR2 Port 2)
> >>>
> >>> or
> >>>
> >>> B) the NAND controller to use DMAC1/IF1 (i.e. DDR2 port 2) and the LCDC to use
> >>> master 8 (i.e. DDR2 Port 3)
> >>
> >> Crap, that was not what I meant to express. Sorry for the confusion. This is
> >> better.
> >>
> >> So, I think I want either
> >>
> >> A) the NAND controller to use master 1 DMAC0/IF0 (i.e. slave 8 DDR2 port 2) and
> >> the LCDC to use master 9 (i.e. slave 9 DDR2 Port 3)
> >>
> >> or
> >>
> >> B) the NAND controller to use master 2 DMAC0/IF1 (i.e. slave 7 DDR2 port 1, and
> >> possibly slave 9 DDR2 port 3 (if my previous findings are relevant) and the
> >> LCDC to use master 8 (i.e. slave 8 DDR2 Port 2)
> >>
> >>> But, again, how do I limit DMAC0 to either of IF0 or IF1 for NAND accesses?
> >>
> >> So, I added a horrid patch (attached), which mainly adds printk lines, but
> >> additionally does one more thing in atc_prep_dma_memcpy. It changes the DSCR_IF
> >> field (from 0) to 1 for DMA-memcpy for dma0chan5 (i.e. the NAND). At least I
> >> think it does that?
> >>
> >> Running with that patch gets me this:
> >>
> >> # dmesg | grep -i dma
> >> at_hdmac ffffe600.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), 8 channels
> >> at_hdmac ffffe800.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), 8 channels
> >> dma dma0chan0: xlate 0 2
> >> dma dma0chan1: xlate 0 2
> >> at91_i2c f0014000.i2c: using dma0chan0 (tx) and dma0chan1 (rx) for DMA transfers
> >> dma dma1chan0: xlate 0 2
> >> dma dma1chan1: xlate 0 2
> >> at91_i2c f801c000.i2c: using dma1chan0 (tx) and dma1chan1 (rx) for DMA transfers
> >> dma dma0chan2: xlate 0 2
> >> dma dma0chan3: xlate 0 2
> >> dma dma0chan4: xlate 0 2
> >> atmel_mci f0000000.mmc: using dma0chan4 for DMA transfers
> >> dma dma1chan2: xlate 0 2
> >> dma dma1chan3: xlate 0 2
> >> atmel_aes f8038000.aes: Atmel AES - Using dma1chan2, dma1chan3 for DMA transfers
> >> dma dma1chan4: xlate 0 2
> >> atmel_sha f8034000.sha: using dma1chan4 for DMA transfers
> >> dma dma1chan5: xlate 0 2
> >> dma dma1chan6: xlate 0 2
> >> atmel_tdes f803c000.tdes: using dma1chan5, dma1chan6 for DMA transfers
> >> atmel-nand-controller 10000000.ebi:nand-controller: using dma0chan5 for DMA transfers
> >> dma dma0chan5: memcpy: 0
> >> dma dma0chan5: DSCR_IF: 1
> >> dma dma0chan5: memcpy: 1
> >>
> >> So, output is as expected and I believe that the patch makes the NAND DMA
> >> accesses use master 2 DMAC0/IF1 and are thus forced to use slave 7 DDR2 Port 1
> >> (and possibly 9). The LCDC is using slave 8 DDR2 Port 2. So there should be no
> >> slave conflict?
> >>
> >> But the on-screen crap remains during NAND accesses.
> >>
> >> But pressing on.
> >>
> >> I then changed the priorities for all accesses to 0 in the PRxSy registers, except
> >> the ones for masters 8/9 LCDC (slaves 8/9) which I left at priority 3.
> >>
> >> But the on-screen crap remains during NAND accesses.
> >>
> >> My guess is that the NAND DMA is doing too long bursts and that the LCDC therefore
> >> has to wait too long and simply fails to keep the pipeline from running short?
> >>
> >> So I tried to reduce the maximum SLOT_CYCLE for slaves 7 and 9 in the SCFGx
> >> registers. No noticeable effect either.
> >>
> >> I then tried to split bursts from master 2 (DMAC0/IF1) with small values in the
> >> MCFG2 register. No effect.
> >>
> >> I'm getting nowhere.
> >
> > Could it just be that you're reaching the DDR bus limit. As I said
> > previously, when you go through the CPU, and assuming you're consuming
> > the data directly, you have:
> >
> > 1/ NFC SRAM -> CPU
> > 2/ CPU -> L1 data cache --write-back--> DRAM
> > 3/ L1-cache -> CPU
> >
> > While, if you use DMA you get:
> >
> > 1/ NFC SRAM -> DRAM
> > 2/ SDRAM -> L1 data cache -> CPU
> >
> > So, if you're approaching the limit of (LP)DDR bandwidth, using the CPU
> > might make things a bit better. Still, if the limitation really comes
> > from the DDR bus, my opinion is that you should maybe use a smaller
> > resolution or use a more compact pixel format (RGB565?).
>
> The issue is still there if I use a CLUT mode instead of rgb565, which is
> what I normally use (and what I would like to use, CLUT is just alien and
> a pain these days).
>
> The panels we are using only supports one resolution (each), but the issue
> is there with both 1920x1080 at 16bpp and 1024x768 at 8bpp (~60Hz).
Duh! This adds to the weirdness of this issue. I'd thought that by
dividing the required bandwidth by 2 you would get a reliable setup.
>
> > Did you calculate how much of the bandwidth is taken by the HLCDC
> > block and compared it to the max (LP)DDR bandwidth?
>
> I did, but don't remember the exact details. There is some room even for
> 1920x1080 at 16bpp, but not oceans of it. We were a bit uncertain if 16bpp
> would be possible, and in fact that was the reason I worked on CLUT
> support for atmel-hlcdc last year. But since the problem persists with
> much less memory pressure as well, I don't think that's it either.
>
> Admittedly I have not tested these AHB matrix tricks with a smaller
> panel (it would take a bit of work to arrange for those tests), but the
> issue was there when I last tried (using defaults).
Okay. I think I'll take your initial patch, but I'd really like to
understand the root cause of this problem. Tudor, any idea why the
various stuff Peter tried did not work?
^ permalink raw reply
* [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
From: Nicolas Ferre @ 2018-05-28 16:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <db31aac0-6b59-8835-2181-635670e7f176@axentia.se>
On 28/05/2018 at 17:52, Peter Rosin wrote:
> On 2018-05-28 16:27, Boris Brezillon wrote:
[..]
>> Could it just be that you're reaching the DDR bus limit. As I said
>> previously, when you go through the CPU, and assuming you're consuming
>> the data directly, you have:
>>
>> 1/ NFC SRAM -> CPU
>> 2/ CPU -> L1 data cache --write-back--> DRAM
>> 3/ L1-cache -> CPU
>>
>> While, if you use DMA you get:
>>
>> 1/ NFC SRAM -> DRAM
>> 2/ SDRAM -> L1 data cache -> CPU
>>
>> So, if you're approaching the limit of (LP)DDR bandwidth, using the CPU
>> might make things a bit better. Still, if the limitation really comes
>> from the DDR bus, my opinion is that you should maybe use a smaller
>> resolution or use a more compact pixel format (RGB565?).
>
> The issue is still there if I use a CLUT mode instead of rgb565, which is
> what I normally use (and what I would like to use, CLUT is just alien and
> a pain these days).
>
> The panels we are using only supports one resolution (each), but the issue
> is there with both 1920x1080 at 16bpp and 1024x768 at 8bpp (~60Hz).
>
>> Did you calculate how much of the bandwidth is taken by the HLCDC
>> block and compared it to the max (LP)DDR bandwidth?
>
> I did, but don't remember the exact details. There is some room even for
> 1920x1080 at 16bpp, but not oceans of it. We were a bit uncertain if 16bpp
> would be possible, and in fact that was the reason I worked on CLUT
> support for atmel-hlcdc last year. But since the problem persists with
> much less memory pressure as well, I don't think that's it either.
Just jumping in the conversation with another perspective (maybe already
tried actually).
Can you try to make all that you can to maximize the blanking period of
your screen (some are more tolerant than others according to that). By
doing so, you would allow the LCD FIFO to recover better after each
line. You might loose some columns on the side of your display but it
would give us a good idea of how far we are from getting rid of those
annoying LCD reset glitches (that are due to underruns on LCD FIFO).
> Admittedly I have not tested these AHB matrix tricks with a smaller
> panel (it would take a bit of work to arrange for those tests), but the
> issue was there when I last tried (using defaults).
If what I said earlier has an impact, reducing the panel size will also
make a difference. But the question is how small is "smaller" ;-)
Best regards,
--
Nicolas Ferre
^ permalink raw reply
* [PATCH 09/12] ARM: dts: exynos5250: add mipi-phy node
From: Krzysztof Kozlowski @ 2018-05-28 16:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527500833-16005-10-git-send-email-m.purski@samsung.com>
On Mon, May 28, 2018 at 11:47 AM, Maciej Purski <m.purski@samsung.com> wrote:
> The patch adds phy node, required by MIPI devices.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
This have pretty small changes since original Andrzej's patch so
probably Andrzej's authorship should be preserved.
> ---
> arch/arm/boot/dts/exynos5250.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 2daf505..a63b655 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -733,6 +733,12 @@
> #phy-cells = <0>;
> };
>
> + mipi_phy: video-phy at 10040710 {
> + compatible = "samsung,s5pv210-mipi-video-phy";
No DTC warnings (make dtbs W=1)? reg is missing so DTC should complain.
Best regards,
Krzysztof
> + #phy-cells = <1>;
> + syscon = <&pmu_system_controller>;
> + };
> +
> adc: adc at 12d10000 {
> compatible = "samsung,exynos-adc-v1";
> reg = <0x12D10000 0x100>;
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 08/12] drm/bridge: tc358764: Add DSI to LVDS bridge driver
From: Randy Dunlap @ 2018-05-28 16:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527500833-16005-9-git-send-email-m.purski@samsung.com>
On 05/28/2018 02:47 AM, Maciej Purski wrote:
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index fa2c799..58e19af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -110,6 +110,13 @@ config DRM_THINE_THC63LVD1024
> ---help---
> Thine THC63LVD1024 LVDS/parallel converter driver.
>
> +config DRM_TOSHIBA_TC358764
> + tristate "TC358764 DSI/LVDS bridge"
> + depends on DRM && DRM_PANEL
> + depends on OF
> + select DRM_MIPI_DSI
> + select VIDEOMODE_HELPERS
> +
Kconfig symbols with "prompt strings" should also have help text.
I expected checkpatch to catch that, but I tested it and there was no warning
from checkpatch about help text. OTOH, there was a warning that there are
2 lines in this patch that end with whitespace, so that should be fixed.
Did you use checkpatch (scripts/checkpatch.pl)?
> config DRM_TOSHIBA_TC358767
> tristate "Toshiba TC358767 eDP bridge"
> depends on OF
thanks,
--
~Randy
^ permalink raw reply
* [PATCH 10/12] ARM: dts: exynos5250: add DSI node
From: Krzysztof Kozlowski @ 2018-05-28 16:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527501272-16365-1-git-send-email-m.purski@samsung.com>
On Mon, May 28, 2018 at 11:54 AM, Maciej Purski <m.purski@samsung.com> wrote:
> The patch adds common part of DSI node for Exynos5250 platforms.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> arch/arm/boot/dts/exynos5250.dtsi | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
This should be squashed with previous patch because it is logically
the same feature. The purpose of PHY is this DSI node so splitting it
is slightly too much.
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH 12/12] ARM: dts: exynos5250-arndale: add dsi and panel nodes
From: Krzysztof Kozlowski @ 2018-05-28 16:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527501347-16521-1-git-send-email-m.purski@samsung.com>
On Mon, May 28, 2018 at 11:55 AM, Maciej Purski <m.purski@samsung.com> wrote:
> The patch adds bridge and panel nodes.
> It adds also DSI properties specific for arndale board.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
> arch/arm/boot/dts/exynos5250-arndale.dts | 39 ++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
> index 80221fa..6f0b1c4 100644
> --- a/arch/arm/boot/dts/exynos5250-arndale.dts
> +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
> @@ -71,6 +71,17 @@
> };
> };
>
> + panel: panel {
> + compatible = "boe,hv070wsa-100";
> + power-supply = <&vcc_3v3_reg>;
> + enable-gpios = <&gpd1 3 GPIO_ACTIVE_HIGH>;
> + port {
> + panel_ep: endpoint {
> + remote-endpoint = <&bridge_out_ep>;
> + };
> + };
> + };
> +
> regulators {
> compatible = "simple-bus";
> #address-cells = <1>;
> @@ -476,6 +487,34 @@
> };
> };
>
> +&dsi_0 {
Please put the node alphabetically, so before &dp.
Also this patch should be squashed with previous one (regulators),
because adding non-controllable fixed regulators on its own does not
bring benefits. Linux will not disable them. You added these
regulators for the bridge below. Without the bridge, their existence
does not have much sense.
Best regards,
Krzysztof
> + vddcore-supply = <&ldo8_reg>;
> + vddio-supply = <&ldo10_reg>;
> + samsung,pll-clock-frequency = <24000000>;
> + samsung,burst-clock-frequency = <320000000>;
> + samsung,esc-clock-frequency = <10000000>;
> + status = "okay";
> +
> + bridge at 0 {
> + reg = <0>;
> + compatible = "toshiba,tc358764";
> + vddc-supply = <&vcc_1v2_reg>;
> + vddio-supply = <&vcc_1v8_reg>;
> + vddmipi-supply = <&vcc_1v2_reg>;
> + vddlvds133-supply = <&vcc_3v3_reg>;
> + vddlvds112-supply = <&vcc_1v2_reg>;
> + reset-gpios = <&gpd1 6 GPIO_ACTIVE_LOW>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port at 1 {
> + reg = <1>;
> + bridge_out_ep: endpoint {
> + remote-endpoint = <&panel_ep>;
> + };
> + };
> + };
> +};
> +
> &i2c_2 {
> status = "okay";
> /* used by HDMI DDC */
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH] perf tools: Fix indexing for decoder packet queue
From: Mathieu Poirier @ 2018-05-28 16:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CALZQ+UMbndvogrSb0Ey3ac1vDGqtsiRTR=ee8VGzU2Ni9_s0og@mail.gmail.com>
On 27 May 2018 at 21:13, Leo Yan <leo.yan@linaro.org> wrote:
> On Fri, May 25, 2018 at 05:10:54PM -0600, Mathieu Poirier wrote:
>> The tail of a queue is supposed to be pointing to the next available slot
>> in a queue. In this implementation the tail is incremented before it is
>> used and as such points to the last used element, something that has the
>> immense advantage of centralizing tail management at a single location
>> and eliminating a lot of redundant code.
>>
>> But this needs to be taken into consideration on the dequeueing side where
>> the head also needs to be incremented before it is used, or the first
>> available element of the queue will be skipped.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>> tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> index c8b98fa22997..4d5fc374e730 100644
>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> @@ -96,11 +96,19 @@ int cs_etm_decoder__get_packet(struct cs_etm_decoder *decoder,
>> /* Nothing to do, might as well just return */
>> if (decoder->packet_count == 0)
>> return 0;
>> + /*
>> + * The queueing process in function cs_etm_decoder__buffer_packet()
>> + * increments the tail *before* using it. This is somewhat counter
>> + * intuitive but it has the advantage of centralizing tail management
>> + * at a single location. Because of that we need to follow the same
>> + * heuristic with the head, i.e we increment it before using its
>> + * value. Otherwise the first element of the packet queue is not
>> + * used.
>> + */
>> + decoder->head = (decoder->head + 1) & (MAX_BUFFER - 1);
>>
>> *packet = decoder->packet_buffer[decoder->head];
>>
>> - decoder->head = (decoder->head + 1) & (MAX_BUFFER - 1);
>> -
>
> I tested this patch and confirmed it can work well with python
> decoding script:
>
> Tested-by: Leo Yan <leo.yan@linaro.org>
>
> Actually, I have another idea for this fixing, seems to me
> the unchanged code has right logic for decoder->head, and I think this
> issue is more related with incorrect initialization index. So we can
> change the initialization index for decoder->head as below. How about
> you think for this?
>
> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> index c8b98fa..b133260 100644
> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
> @@ -249,7 +249,7 @@ static void cs_etm_decoder__clear_buffer(struct
> cs_etm_decoder *decoder)
> {
> int i;
>
> - decoder->head = 0;
> + decoder->head = 1;
I flirted with that idea but thought the problem is really with the
tail and as such would have done:
decoder->tail = -1;
But since both head and tail are declared as u32 it would have
required changing the types to int, necessitating modifications
everywhere other parts of the code deals with them. As such I just
decided to swap the order of events in cs_etm_decoder__get_packet().
I'm not strongly opinionated on this and can send another patch if you're keen.
Thanks for the feedback,
Mathieu
> decoder->tail = 0;
> decoder->packet_count = 0;
> for (i = 0; i < MAX_BUFFER; i++) {
>
> Thanks,
> Leo Yan
>
>> decoder->packet_count--;
>>
>> return 1;
>> --
>> 2.7.4
>>
^ permalink raw reply
* [PATCH 9/9] clk: davinci: Fix link errors when not all SoCs are enabled
From: David Lechner @ 2018-05-28 16:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a1b62396-534c-bc68-63a4-ac493690a3e7@ti.com>
On 05/28/2018 08:43 AM, Sekhar Nori wrote:
> On Friday 25 May 2018 11:41 PM, David Lechner wrote:
>> This fixes linker errors due to undefined symbols when one or more of
>> the TI DaVinci SoCs is not enabled in the kernel config.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>> drivers/clk/davinci/pll.c | 16 ++++++++++++++++
>> drivers/clk/davinci/pll.h | 11 ++++++++---
>> drivers/clk/davinci/psc.c | 14 ++++++++++++++
>> drivers/clk/davinci/psc.h | 12 ++++++++++++
>> include/linux/clk/davinci.h | 19 +++++++++++++++----
>> 5 files changed, 65 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/davinci/pll.c b/drivers/clk/davinci/pll.c
>> index 84a343060bc8..65abd371692d 100644
>> --- a/drivers/clk/davinci/pll.c
>> +++ b/drivers/clk/davinci/pll.c
>> @@ -860,25 +860,41 @@ static struct davinci_pll_platform_data *davinci_pll_get_pdata(struct device *de
>> }
>>
>> /* needed in early boot for clocksource/clockevent */
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> CLK_OF_DECLARE(da850_pll0, "ti,da850-pll0", of_da850_pll0_init);
>> +#endif
>>
>> static const struct of_device_id davinci_pll_of_match[] = {
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> { .compatible = "ti,da850-pll1", .data = of_da850_pll1_init },
>> +#endif
>> { }
>> };
>>
>> static const struct platform_device_id davinci_pll_id_table[] = {
>> +#ifdef CONFIG_ARCH_DAVINCI_DA830
>> { .name = "da830-pll", .driver_data = (kernel_ulong_t)da830_pll_init },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> { .name = "da850-pll0", .driver_data = (kernel_ulong_t)da850_pll0_init },
>> { .name = "da850-pll1", .driver_data = (kernel_ulong_t)da850_pll1_init },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM355
>> { .name = "dm355-pll1", .driver_data = (kernel_ulong_t)dm355_pll1_init },
>> { .name = "dm355-pll2", .driver_data = (kernel_ulong_t)dm355_pll2_init },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM365
>> { .name = "dm365-pll1", .driver_data = (kernel_ulong_t)dm365_pll1_init },
>> { .name = "dm365-pll2", .driver_data = (kernel_ulong_t)dm365_pll2_init },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM644x
>> { .name = "dm644x-pll1", .driver_data = (kernel_ulong_t)dm644x_pll1_init },
>> { .name = "dm644x-pll2", .driver_data = (kernel_ulong_t)dm644x_pll2_init },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM646x
>> { .name = "dm646x-pll1", .driver_data = (kernel_ulong_t)dm646x_pll1_init },
>> { .name = "dm646x-pll2", .driver_data = (kernel_ulong_t)dm646x_pll2_init },
>> +#endif
>> { }
>> };
>>
>> diff --git a/drivers/clk/davinci/pll.h b/drivers/clk/davinci/pll.h
>> index b2e5c4496645..7cc354dd29e2 100644
>> --- a/drivers/clk/davinci/pll.h
>> +++ b/drivers/clk/davinci/pll.h
>> @@ -122,14 +122,19 @@ int of_davinci_pll_init(struct device *dev, struct device_node *node,
>>
>> /* Platform-specific callbacks */
>>
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> int da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
>> void of_da850_pll0_init(struct device_node *node);
>> int of_da850_pll1_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
>> -
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM355
>> int dm355_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
>> -
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM644x
>> int dm644x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
>> -
>> +#endif
>
> The traditional way of dealing with this for functions is to provide a
> static inline stub when CONFIG_ARCH_DAVINCI_DM644x is not defined. Can
> we use that? That helps in avoiding ifdefs elsewhere.
I suppose we could. But then we would be taking function pointers to
static inline functions, which seems kind of weird to me.
>
>> +#ifdef CONFIG_ARCH_DAVINCI_DM646x
>> int dm646x_pll2_init(struct device *dev, void __iomem *base, struct regmap *cfgchip);
>> +#endif
>>
>> #endif /* __CLK_DAVINCI_PLL_H___ */
>> diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c
>> index 6326ba1fe3cc..fffbed5e263b 100644
>> --- a/drivers/clk/davinci/psc.c
>> +++ b/drivers/clk/davinci/psc.c
>> @@ -513,20 +513,34 @@ int of_davinci_psc_clk_init(struct device *dev,
>> }
>>
>> static const struct of_device_id davinci_psc_of_match[] = {
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> { .compatible = "ti,da850-psc0", .data = &of_da850_psc0_init_data },
>> { .compatible = "ti,da850-psc1", .data = &of_da850_psc1_init_data },
>> +#endif
>> { }
>> };
>>
>> static const struct platform_device_id davinci_psc_id_table[] = {
>> +#ifdef CONFIG_ARCH_DAVINCI_DA830
>> { .name = "da830-psc0", .driver_data = (kernel_ulong_t)&da830_psc0_init_data },
>> { .name = "da830-psc1", .driver_data = (kernel_ulong_t)&da830_psc1_init_data },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> { .name = "da850-psc0", .driver_data = (kernel_ulong_t)&da850_psc0_init_data },
>> { .name = "da850-psc1", .driver_data = (kernel_ulong_t)&da850_psc1_init_data },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM355
>> { .name = "dm355-psc", .driver_data = (kernel_ulong_t)&dm355_psc_init_data },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM365
>> { .name = "dm365-psc", .driver_data = (kernel_ulong_t)&dm365_psc_init_data },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM644x
>> { .name = "dm644x-psc", .driver_data = (kernel_ulong_t)&dm644x_psc_init_data },
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM646x
>> { .name = "dm646x-psc", .driver_data = (kernel_ulong_t)&dm646x_psc_init_data },
>> +#endif
>> { }
>> };
>>
>> diff --git a/drivers/clk/davinci/psc.h b/drivers/clk/davinci/psc.h
>> index c2a7df6413fe..6a42529d31a9 100644
>> --- a/drivers/clk/davinci/psc.h
>> +++ b/drivers/clk/davinci/psc.h
>> @@ -94,15 +94,27 @@ struct davinci_psc_init_data {
>> int (*psc_init)(struct device *dev, void __iomem *base);
>> };
>>
>> +#ifdef CONFIG_ARCH_DAVINCI_DA830
>> extern const struct davinci_psc_init_data da830_psc0_init_data;
>> extern const struct davinci_psc_init_data da830_psc1_init_data;
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DA850
>> extern const struct davinci_psc_init_data da850_psc0_init_data;
>> extern const struct davinci_psc_init_data da850_psc1_init_data;
>> extern const struct davinci_psc_init_data of_da850_psc0_init_data;
>> extern const struct davinci_psc_init_data of_da850_psc1_init_data;
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM355
>> extern const struct davinci_psc_init_data dm355_psc_init_data;
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM356
>> extern const struct davinci_psc_init_data dm365_psc_init_data;
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM644x
>> extern const struct davinci_psc_init_data dm644x_psc_init_data;
>> +#endif
>> +#ifdef CONFIG_ARCH_DAVINCI_DM646x
>> extern const struct davinci_psc_init_data dm646x_psc_init_data;
>> +#endif
>
> ifdefs around these extern declarations are not needed.
True. I guess I just prefer a compiler error to a linker error.
^ permalink raw reply
* [PATCH v4 0/6] Enhance support for the SP805 WDT
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
This patch series enhances the support for the SP805 watchdog timer.
First of all, 'timeout-sec' devicetree property is added. In addition,
support is also added to allow the driver to reset the watchdog if it
has been detected that watchdot has been started in the bootloader. In
this case, the driver will initiate the ping service from the kernel
watchdog subsystem, before a user mode daemon takes over. This series
also enables SP805 in the default ARM64 defconfig
This patch series is based off v4.17-rc5 and is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: sp805-wdt-v4
Changes since v3:
- Improve description of 'timeout-sec' in the binding document, per
recommendation from Guenter Roeck
Changes since v2:
- Fix indent and format to make them consistent within arm,sp805.txt
Changes since v1:
- Consolidate two duplicated SP805 binding documents into one
- Slight change of the wdt_is_running implementation per discussion
Ray Jui (6):
Documentation: DT: Consolidate SP805 binding docs
Documentation: DT: Add optional 'timeout-sec' property for sp805
watchdog: sp805: add 'timeout-sec' DT property support
watchdog: sp805: set WDOG_HW_RUNNING when appropriate
arm64: dt: set initial SR watchdog timeout to 60 seconds
arm64: defconfig: add CONFIG_ARM_SP805_WATCHDOG
.../devicetree/bindings/watchdog/arm,sp805.txt | 29 +++++++++++++++-----
.../devicetree/bindings/watchdog/sp805-wdt.txt | 31 ----------------------
.../arm64/boot/dts/broadcom/stingray/stingray.dtsi | 1 +
arch/arm64/configs/defconfig | 1 +
drivers/watchdog/sp805_wdt.c | 28 ++++++++++++++++++-
5 files changed, 51 insertions(+), 39 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
--
2.1.4
^ permalink raw reply
* [PATCH v4 1/6] Documentation: DT: Consolidate SP805 binding docs
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527530497-10392-1-git-send-email-ray.jui@broadcom.com>
Consolidate two SP805 binding documents "arm,sp805.txt" and
"sp805-wdt.txt" into "arm,sp805.txt" that matches the naming of the
desired compatible string to be used
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
.../devicetree/bindings/watchdog/arm,sp805.txt | 27 ++++++++++++++-----
.../devicetree/bindings/watchdog/sp805-wdt.txt | 31 ----------------------
2 files changed, 20 insertions(+), 38 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt
index ca99d64..0fa3629 100644
--- a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt
+++ b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt
@@ -1,17 +1,30 @@
ARM AMBA Primecell SP805 Watchdog
+SP805 WDT is a ARM Primecell Peripheral and has a standard-id register that
+can be used to identify the peripheral type, vendor, and revision.
+This value can be used for driver matching.
+
+As SP805 WDT is a primecell IP, it follows the base bindings specified in
+'arm/primecell.txt'
+
Required properties:
-- compatible: Should be "arm,sp805" & "arm,primecell"
-- reg: Should contain location and length for watchdog timer register.
-- interrupts: Should contain the list of watchdog timer interrupts.
-- clocks: clocks driving the watchdog timer hardware. This list should be 2
- clocks. With 2 clocks, the order is wdogclk clock, apb_pclk.
+- compatible: Should be "arm,sp805" & "arm,primecell"
+- reg: Should contain location and length for watchdog timer register
+- clocks: Clocks driving the watchdog timer hardware. This list should be
+ 2 clocks. With 2 clocks, the order is wdog_clk, apb_pclk
+ wdog_clk can be equal to or be a sub-multiple of the apb_pclk
+ frequency
+- clock-names: Shall be "wdog_clk" for first clock and "apb_pclk" for the
+ second one
+
+Optional properties:
+- interrupts: Should specify WDT interrupt number
Example:
watchdog at 66090000 {
compatible = "arm,sp805", "arm,primecell";
reg = <0x66090000 0x1000>;
interrupts = <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&apb_pclk>,<&apb_pclk>;
- clock-names = "wdogclk", "apb_pclk";
+ clocks = <&wdt_clk>, <&apb_pclk>;
+ clock-names = "wdog_clk", "apb_pclk";
};
diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
deleted file mode 100644
index edc4f0e..0000000
--- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
+++ /dev/null
@@ -1,31 +0,0 @@
-* ARM SP805 Watchdog Timer (WDT) Controller
-
-SP805 WDT is a ARM Primecell Peripheral and has a standard-id register that
-can be used to identify the peripheral type, vendor, and revision.
-This value can be used for driver matching.
-
-As SP805 WDT is a primecell IP, it follows the base bindings specified in
-'arm/primecell.txt'
-
-Required properties:
-- compatible : Should be "arm,sp805-wdt", "arm,primecell"
-- reg : Base address and size of the watchdog timer registers.
-- clocks : From common clock binding.
- First clock is PCLK and the second is WDOGCLK.
- WDOGCLK can be equal to or be a sub-multiple of the PCLK frequency.
-- clock-names : From common clock binding.
- Shall be "apb_pclk" for first clock and "wdog_clk" for the
- second one.
-
-Optional properties:
-- interrupts : Should specify WDT interrupt number.
-
-Examples:
-
- cluster1_core0_watchdog: wdt at c000000 {
- compatible = "arm,sp805-wdt", "arm,primecell";
- reg = <0x0 0xc000000 0x0 0x1000>;
- clocks = <&clockgen 4 3>, <&clockgen 4 3>;
- clock-names = "apb_pclk", "wdog_clk";
- };
-
--
2.1.4
^ permalink raw reply related
* [PATCH v4 2/6] Documentation: DT: Add optional 'timeout-sec' property for sp805
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527530497-10392-1-git-send-email-ray.jui@broadcom.com>
Update the SP805 binding document to add optional 'timeout-sec'
devicetree property
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
Documentation/devicetree/bindings/watchdog/arm,sp805.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt
index 0fa3629..bee6f1f 100644
--- a/Documentation/devicetree/bindings/watchdog/arm,sp805.txt
+++ b/Documentation/devicetree/bindings/watchdog/arm,sp805.txt
@@ -19,6 +19,8 @@ Required properties:
Optional properties:
- interrupts: Should specify WDT interrupt number
+- timeout-sec: Should specify default WDT timeout in seconds. If unset, the
+ default timeout is determined by the driver
Example:
watchdog at 66090000 {
--
2.1.4
^ permalink raw reply related
* [PATCH v4 3/6] watchdog: sp805: add 'timeout-sec' DT property support
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527530497-10392-1-git-send-email-ray.jui@broadcom.com>
Add support for optional devicetree property 'timeout-sec'.
'timeout-sec' is used in the driver if specified in devicetree.
Otherwise, fall back to driver default, i.e., 60 seconds
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/watchdog/sp805_wdt.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 03805bc..1484609 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -230,7 +230,14 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
spin_lock_init(&wdt->lock);
watchdog_set_nowayout(&wdt->wdd, nowayout);
watchdog_set_drvdata(&wdt->wdd, wdt);
- wdt_setload(&wdt->wdd, DEFAULT_TIMEOUT);
+
+ /*
+ * If 'timeout-sec' devicetree property is specified, use that.
+ * Otherwise, use DEFAULT_TIMEOUT
+ */
+ wdt->wdd.timeout = DEFAULT_TIMEOUT;
+ watchdog_init_timeout(&wdt->wdd, 0, &adev->dev);
+ wdt_setload(&wdt->wdd, wdt->wdd.timeout);
ret = watchdog_register_device(&wdt->wdd);
if (ret) {
--
2.1.4
^ permalink raw reply related
* [PATCH v4 4/6] watchdog: sp805: set WDOG_HW_RUNNING when appropriate
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527530497-10392-1-git-send-email-ray.jui@broadcom.com>
If the watchdog hardware is already enabled during the boot process,
when the Linux watchdog driver loads, it should reset the watchdog and
tell the watchdog framework. As a result, ping can be generated from
the watchdog framework, until the userspace watchdog daemon takes over
control
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/watchdog/sp805_wdt.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 1484609..d662a6f 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -42,6 +42,7 @@
/* control register masks */
#define INT_ENABLE (1 << 0)
#define RESET_ENABLE (1 << 1)
+ #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE)
#define WDTINTCLR 0x00C
#define WDTRIS 0x010
#define WDTMIS 0x014
@@ -74,6 +75,15 @@ module_param(nowayout, bool, 0);
MODULE_PARM_DESC(nowayout,
"Set to 1 to keep watchdog running after device release");
+/* returns true if wdt is running; otherwise returns false */
+static bool wdt_is_running(struct watchdog_device *wdd)
+{
+ struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
+ u32 wdtcontrol = readl_relaxed(wdt->base + WDTCONTROL);
+
+ return (wdtcontrol & ENABLE_MASK) == ENABLE_MASK;
+}
+
/* This routine finds load value that will reset system in required timout */
static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout)
{
@@ -239,6 +249,15 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
watchdog_init_timeout(&wdt->wdd, 0, &adev->dev);
wdt_setload(&wdt->wdd, wdt->wdd.timeout);
+ /*
+ * If HW is already running, enable/reset the wdt and set the running
+ * bit to tell the wdt subsystem
+ */
+ if (wdt_is_running(&wdt->wdd)) {
+ wdt_enable(&wdt->wdd);
+ set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+ }
+
ret = watchdog_register_device(&wdt->wdd);
if (ret) {
dev_err(&adev->dev, "watchdog_register_device() failed: %d\n",
--
2.1.4
^ permalink raw reply related
* [PATCH v4 5/6] arm64: dt: set initial SR watchdog timeout to 60 seconds
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527530497-10392-1-git-send-email-ray.jui@broadcom.com>
Set initial Stingray watchdog timeout to 60 seconds
By the time when the userspace watchdog daemon is ready and taking
control over, the watchdog timeout will then be reset to what's
configured in the daemon
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index 99aaff0..1e1cf49 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -420,6 +420,7 @@
interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&hsls_25m_div2_clk>, <&hsls_div4_clk>;
clock-names = "wdogclk", "apb_pclk";
+ timeout-sec = <60>;
};
gpio_hsls: gpio at d0000 {
--
2.1.4
^ permalink raw reply related
* [PATCH v4 6/6] arm64: defconfig: add CONFIG_ARM_SP805_WATCHDOG
From: Ray Jui @ 2018-05-28 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527530497-10392-1-git-send-email-ray.jui@broadcom.com>
Enable the SP805 watchdog timer
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index ecf6137..3fe5eb5 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -351,6 +351,7 @@ CONFIG_ROCKCHIP_THERMAL=m
CONFIG_TEGRA_BPMP_THERMAL=m
CONFIG_UNIPHIER_THERMAL=y
CONFIG_WATCHDOG=y
+CONFIG_ARM_SP805_WATCHDOG=y
CONFIG_S3C2410_WATCHDOG=y
CONFIG_MESON_GXBB_WATCHDOG=m
CONFIG_MESON_WATCHDOG=m
--
2.1.4
^ permalink raw reply related
* [PATCH] dt-bindings: arm: Remove obsolete insignal-boards.txt
From: Krzysztof Kozlowski @ 2018-05-28 18:53 UTC (permalink / raw)
To: linux-arm-kernel
The compatibles mentioned in insignal-boards.txt are already documented
under devicetree/bindings/arm/samsung/samsung-boards.txt. Also the
contents of insignal-boards.txt is not accurate, e.g. does not mention
Arndale boards.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
Documentation/devicetree/bindings/arm/insignal-boards.txt | 8 --------
1 file changed, 8 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/arm/insignal-boards.txt
diff --git a/Documentation/devicetree/bindings/arm/insignal-boards.txt b/Documentation/devicetree/bindings/arm/insignal-boards.txt
deleted file mode 100644
index 524c3dc5d808..000000000000
--- a/Documentation/devicetree/bindings/arm/insignal-boards.txt
+++ /dev/null
@@ -1,8 +0,0 @@
-* Insignal's Exynos4210 based Origen evaluation board
-
-Origen low-cost evaluation board is based on Samsung's Exynos4210 SoC.
-
-Required root node properties:
- - compatible = should be one or more of the following.
- (a) "samsung,smdkv310" - for Samsung's SMDKV310 eval board.
- (b) "samsung,exynos4210" - for boards based on Exynos4210 SoC.
--
2.14.1
^ permalink raw reply related
* [PATCH v2] Documentation: dt-bindings: Explicitly mark Samsung Exynos SoC bindings as unstable
From: Krzysztof Kozlowski @ 2018-05-28 19:13 UTC (permalink / raw)
To: linux-arm-kernel
From: Marek Szyprowski <m.szyprowski@samsung.com>
Samsung Exynos SoCs and boards related bindings evolved since the initial
introduction, but initially the bindings were minimal and a bit incomplete
(they never described all the hardware modules available in the SoCs).
Since then some significant (not fully compatible) changes have been
already committed a few times (like gpio replaced by pinctrl, display ddc,
mfc reserved memory, some core clocks added to various hardware modules,
added more required nodes).
On the other side there are no boards which have device tree embedded in
the bootloader. Device tree blob is always compiled from the kernel tree
and updated together with the kernel image.
Thus to avoid further adding a bunch of workarounds for old/missing
bindings, make development of new platforms easier and allow to make
cleanup of the existing code and device tree files, lets mark some
Samsung Exynos SoC platform bindings as unstable. This means that
bindings can may change at any time and users should use the dtb file
compiled from the same kernel source tree as the kernel image.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
Changes since v1:
1. Rebase
2. Add specific compatibles to mark unstable.
v1 is here:
https://patchwork.kernel.org/patch/9477963/
Previous tags (not applying due to change in contents):
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
.../devicetree/bindings/arm/samsung/exynos.txt | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/samsung/exynos.txt
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos.txt b/Documentation/devicetree/bindings/arm/samsung/exynos.txt
new file mode 100644
index 000000000000..7410cb79e4b2
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos.txt
@@ -0,0 +1,26 @@
+Samsung Exynos SoC Family Device Tree Bindings
+---------------------------------------------------------------
+
+Work in progress statement:
+
+Following Device Tree files and bindings applying to Samsung Exynos SoCs and
+boards are considered "unstable":
+
+ - samsung,exynos5433* (all compatibles related to Exynos5433)
+ - samsung,exynos7* (all compatibles related to Exynos7)
+ - samsung,tm2-audio
+ - samsung,mfc-v10
+ - samsung,exynos*-mipi-dsi
+ - samsung,exynos5-dp
+ - samsung,exynos*-hdmi
+ - samsung,exynos*-hdmiddc
+ - samsung,exynos*-hdmiphy
+ - samsung,exynos*-mixer
+ - samsung,exynos*-fimd
+
+Any Samsung Exynos device tree binding mentioned may change at any time. Be
+sure to use a device tree binary and a kernel image generated from the same
+source tree.
+
+Please refer to Documentation/devicetree/bindings/ABI.txt for a definition of a
+stable binding/ABI.
--
2.14.1
^ permalink raw reply related
* [PATCH] ARM: dts: cygnus: add ethernet0 alias
From: Scott Branden @ 2018-05-28 19:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJiuCcd6Y+H0XJn+7dObiTuhSPXhefrYNh9qDi+=cs8S6-TGgw@mail.gmail.com>
On 18-05-28 01:22 AM, Cl?ment P?ron wrote:
> Could you review it, please.
> Thanks
>
> On Thu, 3 May 2018 at 17:13, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>> +Ray, Scott, Eric, list
>> On 05/03/2018 02:56 AM, Cl?ment P?ron wrote:
>>> In order to avoid Linux generating a random mac address on every boot,
>>> add an ethernet0 alias that will allow u-boot to patch the dtb with
>>> the MAC address.
>>>
>>> Signed-off-by: Cl?ment P?ron <peron.clem@gmail.com>
Acked-by: Scott Branden <scott.branden@broadcom.com>
>>> ---
>>> arch/arm/boot/dts/bcm-cygnus.dtsi | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> index 9fe4f5a6379e..1a05b8f48b54 100644
>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> @@ -41,6 +41,10 @@
>>> model = "Broadcom Cygnus SoC";
>>> interrupt-parent = <&gic>;
>>>
>>> + aliases {
>>> + ethernet0 = ð0;
>>> + };
>>> +
>>> cpus {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>>
>> --
>> Florian
> Clement
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox