Re: [PATCH AUTOSEL 5.8 08/29] net: dsa: microchip: look for phy-mode in port nodes

2020-09-29 Thread Helmut Grohne
Hi Sascha,

On Tue, Sep 29, 2020 at 03:30:05AM +0200, Sasha Levin wrote:
> From: Helmut Grohne 
> 
> [ Upstream commit edecfa98f602a597666e3c5cab2677ada38d93c5 ]
> 
> Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
> property should be specified on port nodes. However, the microchip
> drivers read it from the switch node.
> 
> Let the driver use the per-port property and fall back to the old
> location with a warning.
> 
> Fix in-tree users.

I don't think this patch is useful for stable users. It corrects a
device tree layout issue. Any existing users of the functionality will
have an odd, but working device tree and that will continue working
(with a warning) after applying this patch. It just has a property on
the "wrong" node. I don't think I'd like to update my device tree in a
stable update.

If you apply it anyway, please also apply a fixup:
https://lore.kernel.org/netdev/20200924083746.GA9410@laureti-dev/

Helmut


Re: [LINUX PATCH v19] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-08-20 Thread Helmut Grohne
Hi,

On Tue, Jul 30, 2019 at 05:43:37AM -0600, Naga Sureshkumar Relli wrote:
> Add driver for arm pl353 static memory controller nand interface.
> This controller is used in Xilinx Zynq SoC for interfacing the
> NAND flash memory.

Is there a reason that you dropped me from the Cc list? If you Cc me,
the feedback loop is faster. Please continue to Cc me on this driver.

> -> Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion S34ML01G1.

I tested this version of the driver with this exact Micron flash in an
on-die configuration against v5.3-rc4. The patch applied cleanly and
built without problems. The driver detects the chip and works
"somewhat". One can interact with portions of the flash, but the amount
of ECC errors returned makes it unusable. I was able to reproduce the
same issue on multiple devices.

...
[   14.909894] jffs2: mtd->read(0x178 bytes from 0x21e0688) returned ECC error
[   14.917250] jffs2: mtd->read(0x800 bytes from 0x21e) returned ECC error
[   14.952765] jffs2: mtd->read(0x364 bytes from 0x21c049c) returned ECC error
[   14.960070] jffs2: mtd->read(0x6f8 bytes from 0x21c0108) returned ECC error
[   14.967435] jffs2: mtd->read(0x800 bytes from 0x21c) returned ECC error
[   15.001194] [ cut here ]
[   15.006092] WARNING: CPU: 0 PID: 94 at 
drivers/mtd/nand/raw/nand_micron.c:245 
micron_nand_read_page_on_die_ecc+0x394/0x3a0
[   15.018148] ---[ end trace 2d1d02f05cac8fbb ]---
[   15.022909] jffs2: error: (94) jffs2_get_inode_nodes: can not read 344 bytes 
from 0x021a16a8, error code: -22.
[   15.035205] jffs2: error: (94) jffs2_do_read_inode_internal: cannot read 
nodes for ino 8375, returned error is -22
[   15.045744] jffs2: Returned error for crccheck of ino #8375. Expect 
badness...
[   15.231220] jffs2: Checked all inodes but still 0x15361c bytes of unchecked 
space?
[   15.238851] jffs2: No space for garbage collection. Aborting GC thread
...

I cannot confirm that the driver works.

For completeness sake, here is the decompiled DT that I used:

memory-controller@e000e000 {
#address-cells = <0x2>;
#size-cells = <0x1>;
status = "okay";
clock-names = "memclk", "apb_pclk";
clocks = <0x1 0xb 0x1 0x2c>;
compatible = "arm,pl353-smc-r2p1", "arm,primecell";
interrupt-parent = <0x4>;
interrupts = <0x0 0x12 0x4>;
ranges = <0x0 0x0 0xe100 0x100>;
reg = <0xe000e000 0x1000>;

flash@e100 {
status = "okay";
compatible = "arm,pl353-nand-r2p1";
reg = <0x0 0x0 0x100>;
#address-cells = <0x1>;
#size-cells = <0x1>;
nand-ecc-mode = "on-die";
nand-bus-width = <0x8>;

};
};

I am posting a decompiled DT, because parts are generated using
https://github.com/Xilinx/device-tree-xlnx.

The driver from the xlnx 4.14 tree continues to work with the hardware I
used for testing.

Helmut


[PATCH] clocksource/drivers/sp804: make CONFIG_ARM_TIMER_SP804 selectable again

2019-08-16 Thread Helmut Grohne
Adding a dependency on CONFIG_COMPILE_TEST makes the relevant item
unselectable for practical purposes. The correct solution is to add a
dependency alternative on the relevant architecture.

Fixes: dfc82faad72520 ("clocksource/drivers/sp804: Add COMPILE_TEST to 
CONFIG_ARM_TIMER_SP804")
Link: https://lore.kernel.org/lkml/20190618120719.a4kgyiuljm5uivfq@laureti-dev/
Link: 
https://lore.kernel.org/lkml/alpine.deb.2.21.1908152227590.1...@nanos.tec.linutronix.de/
Signed-off-by: Helmut Grohne 
---
 drivers/clocksource/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Hi Thomas,

On Thu, Aug 15, 2019 at 10:30:39PM +0200, Thomas Gleixner wrote:
> The obvious fix is to add
> 
>   depends on ARM || ARM64 || COMPILE_TEST
> 
> instead of reverting the whole thing. Care to do that?

Incidentally, that's what I proposed earlier as RFC. Resending that
variant now.

I also note that there are likely more instances for this pattern.
Should they be fixed in a similar way? You can find a lot using the
following incantation:

$ git describe --tags
v5.3-rc4
$ git ls-files -- "*/Kconfig" | xargs git grep --cached 'bool .* if 
COMPILE_TEST$' -- | wc -l
185
$

Seems like an anti-pattern to me. It is particularly common in the
clocksource subtree.

Helmut

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5e9317dc3d39..7081a250573b 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -393,7 +393,8 @@ config ARM_GLOBAL_TIMER
  This options enables support for the ARM global timer unit
 
 config ARM_TIMER_SP804
-   bool "Support for Dual Timer SP804 module" if COMPILE_TEST
+   bool "Support for Dual Timer SP804 module"
+   depends on ARM || ARM64 || COMPILE_TEST
depends on GENERIC_SCHED_CLOCK && CLKDEV_LOOKUP
select CLKSRC_MMIO
select TIMER_OF if OF
-- 
2.11.0



[PATCH] Revert "clocksource/drivers/sp804: Add COMPILE_TEST to CONFIG_ARM_TIMER_SP804"

2019-08-15 Thread Helmut Grohne
This reverts commit dfc82faad72520769ca146f857e65c23632eed5a.

The commit effectively makes ARM_TIMER_SP804 depend on COMPILE_TEST,
which makes it unselectable for practical uses.

Link: https://lore.kernel.org/lkml/20190618120719.a4kgyiuljm5uivfq@laureti-dev/
Signed-off-by: Helmut Grohne 
---
 drivers/clocksource/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5e9317dc3d39..72e924374591 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -393,7 +393,7 @@ config ARM_GLOBAL_TIMER
  This options enables support for the ARM global timer unit
 
 config ARM_TIMER_SP804
-   bool "Support for Dual Timer SP804 module" if COMPILE_TEST
+   bool "Support for Dual Timer SP804 module"
depends on GENERIC_SCHED_CLOCK && CLKDEV_LOOKUP
select CLKSRC_MMIO
select TIMER_OF if OF
-- 
2.11.0



Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-06-25 Thread Helmut Grohne
Thank you for the quick update.

On Tue, Jun 25, 2019 at 06:46:30AM +0200, Naga Sureshkumar Relli wrote:
> -> Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion S34ML01G1.

I tested the v17 series with the MT29F2G08ABAEAWP. I can now mount
existing jffs2 volumes without issues.

When running nandtest on a 64MB area, I no longer see lots of
consecutive errors. However I see few (4-8) single byte errors for
random locations on about half the runs. In comparison, I couldn't
reproduce these on the older driver on v4.14.

When writing random 1MB files on a fresh jffs2 filesystem and reading
them back after umounting and mounting the filesystem, I got one faulty
file in 50 attempts.

So this driver mostly works for me, but I suspect that something
(possibly the tested hardware) doesn't fully work yet. To say more, I'll
need long term testing results. In the mean time, I'm in favour of
merging the driver.

Helmut


Re: [LINUX PATCH v17 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()

2019-06-25 Thread Helmut Grohne
On Tue, Jun 25, 2019 at 06:46:29AM +0200, Naga Sureshkumar Relli wrote:
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
>   chip->ecc.size = 512;
>   chip->ecc.strength = chip->base.eccreq.strength;
>   chip->ecc.algo = NAND_ECC_BCH;
> - chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> - chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> + if (!chip->ecc.read_page)
> + chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> +
> + if (!chip->ecc.write_page)
> + chip->ecc.write_page = 
> micron_nand_write_page_on_die_ecc;

When used with pl353_nand.c, this change prioritizes the
pl353_nand_read_page_raw/pl353_nand_write_page_raw implementations over
micron_nand_read_page_on_die_ecc/micron_nand_write_page_on_die_ecc. The
pl353 implementations don't check the status register of the flash for
NAND_ECC_STATUS_WRITE_RECOMMENDED nor do they update ecc_stats.failed in
any way. Unless I am mistaken, this implies that bitflips cannot be
detected at all anymore.

However, this is the change that makes a MT29F2G08ABAEAWP practically
work with jffs2 on the Zynq platform.

In this context, I countered a document from Micron[1] indicating that
their on-die chips are incompatible with jffs2 as is, because the on-die
oob layout is incompatible with jffs2. I suppose that using the raw
variants puts jffs2 in full control of the oob area, but is this really
the correct solution?

Helmut

[1] 
https://www.micron.com/~/media/Documents/Products/Technical%20Note/NAND%20Flash/tn2975_enable_on-die-ECC_NAND_JFFS2.pdf


Re: [LINUX PATCH v16] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-06-21 Thread Helmut Grohne
Hi,

On Mon, Jun 17, 2019 at 02:50:02AM -0600, Naga Sureshkumar Relli wrote:
> Add driver for arm pl353 static memory controller nand interface with
> HW ECC support. This controller is used in Xilinx Zynq SoC for
> interfacing the NAND flash memory.

Thank you for the update.

> -> Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion S34ML01G1.

I've tested this driver with the same Micron MT29F2G08ABAEAWP using
v5.2-rc5 and I am still seeing lots of ecc errors aka mtd_read returning
-EBADMSG. I traced the source of these errors to
micron_nand_on_die_ecc_status_4 where the NAND_STATUS_FAIL bit is often
found. I reproduced this symptom on multiple boards. An older version of
the driver (against v4.14) does not show this behaviour on the same
devices. I was able to reliably reproduce this behaviour using the
following sequence:
 * flash_erase -j /dev/mtdN 0 0
 * mount -t jffs2 /dev/mtdblockN /mnt
 * touch /mnt/foo
 * umount /mnt
 * mount -t jffs2 /dev/mtdblockN /mnt
The relevant kernel message is:

jffs2: mtd->read(0xXXX bytes from 0xXXX) returned ECC error

I also occasionally saw errors from nandtest ("Byte 0xX is XX should
be XX"). They only reproduce when running nandtest multiple times (less
than 10).  When such errors happen, they are not simple bit flips. Lots
of consecutive bytes differ entirely. Again, I am unable to reproduce
these errors with the older driver.

Possibly I'm wrongly configuring the flash. Can you share a correct
device tree for it? Given my reading of the driver, the nand-ecc-algo is
irrelevant, because nand_micron.c forces bch for on-die ecc-mode anyway.
The ecc-strength thus becomes 4. So I'm left wondering what needs to be
configured beyond nand-ecc-mode = "on-die" and nand-bus-width = <8>?

In addition to testing the driver, I looked at the source again.

> Changes in v15:

It seems that this version lost the Kconfig and Makefile integration.

> --- /dev/null
> +++ b/drivers/mtd/nand/raw/pl353_nand.c
> @@ -0,0 +1,1306 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM PL353 NAND flash controller driver
> + *
> + * Copyright (C) 2017 Xilinx, Inc
> + * Author: Punnaiah chowdary kalluri 
> + * Author: Naga Sureshkumar Relli 
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PL353_NAND_DRIVER_NAME "pl353-nand"
> +
> +/* NAND flash driver defines */
> +#define PL353_NAND_ECC_SIZE  512 /* Size of data for ECC operation */
> +
> +/* AXI Address definitions */
> +#define START_CMD_SHIFT  3
> +#define END_CMD_SHIFT11
> +#define END_CMD_VALID_SHIFT  20
> +#define ADDR_CYCLES_SHIFT21
> +#define CLEAR_CS_SHIFT   21
> +#define ECC_LAST_SHIFT   10
> +#define COMMAND_PHASE(0 << 19)
> +#define DATA_PHASE   BIT(19)
> +
> +#define PL353_NAND_ECC_LAST  BIT(ECC_LAST_SHIFT) /* Set ECC_Last */
> +#define PL353_NAND_CLEAR_CS  BIT(CLEAR_CS_SHIFT) /* Clear chip select */
> +
> +#define PL353_NAND_ECC_BUSY_TIMEOUT  (1 * HZ)
> +#define PL353_NAND_DEV_BUSY_TIMEOUT  (1 * HZ)
> +#define PL353_NAND_LAST_TRANSFER_LENGTH  4
> +#define PL353_NAND_ECC_VALID_SHIFT   24
> +#define PL353_NAND_ECC_VALID_MASK0x40
> +#define PL353_ECC_BITS_BYTEOFF_MASK  0x1FF
> +#define PL353_ECC_BITS_BITOFF_MASK   0x7
> +#define PL353_ECC_BIT_MASK   0xFFF
> +#define PL353_TREA_MAX_VALUE 1
> +#define PL353_MAX_ECC_CHUNKS 4
> +#define PL353_MAX_ECC_BYTES  3
> +
> +struct pl353_nfc_op {
> + u32 cmnds[2];
> + u32 addrs;
> + u32 naddrs;
> + u16 addrs_56;   /* Address cycles 5 and 6 */
> + unsigned int data_instr_idx;
> + unsigned int rdy_timeout_ms;
> + unsigned int rdy_delay_ns;
> + const struct nand_op_instr *data_instr;
> +};
> +
> +/**
> + * struct pl353_nand_controller - Defines the NAND flash controller driver
> + * instance
> + * @controller:  NAND controller structure
> + * @chip:NAND chip information structure
> + * @dev: Parent device (used to print error messages)
> + * @regs:Virtual address of the NAND flash device
> + * @dataphase_addrflags:Flags required for data phase transfers
> + * @addr_cycles: Address cycles
> + * @mclk:Memory controller clock
> + * @mclk_rate:   Clock rate of the Memory controller
> + * @buswidth:Bus width 8 or 16
> + */
> +struct pl353_nand_controller {
> + struct nand_controller controller;
> + struct nand_chip chip;
> + struct device *dev;
> + void __iomem *regs;
> + u32 dataphase_addrflags;
> + u8 addr_cycles;
> + struct clk *mclk;

The mclk attribute is only referenced in pl353_nand_probe. There is no
need to store it in this struct.


Re: [PATCH 02/15] clocksource/drivers/sp804: Add COMPILE_TEST to CONFIG_ARM_TIMER_SP804

2019-06-18 Thread Helmut Grohne
On Thu, May 09, 2019 at 01:10:35PM +0200, Daniel Lezcano wrote:
> From: David Abdurachmanov 
> 
> This is only used on arm and arm64 platforms. Add COMPILE_TEST option.

This patch breaks selecting CONFIG_ARM_TIMER_SP804 here. I don't quite
understand why, but commit dfc82faad72520769ca146f857e65c23632eed5a is
where bisection stops.

When I try make allnoconfig with a KCONFIG_ALLCONFIG that explicitly
enables this option, it remains disabled.

When I try make menuconfig, the clocksource menu is empty.

If I apply the patch below, the option is selectable in menuconfig and
with KCONFIG_ALLCONFIG again. It could be used as an alternative
implementation, but I don't have a good rationale for why the previous
approach breaks.

My reading of the kconfig documentation indicates that the "if
condition" should only influence the default value, but it seems like it
entirely disables the option here. I'm left wondering why.

Can we revert the patch until this is sorted out?

Helmut

--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -388,7 +388,8 @@ config ARM_GLOBAL_TIMER
  This options enables support for the ARM global timer unit
 
 config ARM_TIMER_SP804
-   bool "Support for Dual Timer SP804 module" if COMPILE_TEST
+   bool "Support for Dual Timer SP804 module"
+   depends on ARM || ARM64 || COMPILE_TEST
depends on GENERIC_SCHED_CLOCK && CLKDEV_LOOKUP
select CLKSRC_MMIO
select TIMER_OF if OF


Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-06-13 Thread Helmut Grohne
Hi Naga,

On Thu, Jun 13, 2019 at 10:18:00AM +, Naga Sureshkumar Relli wrote:
> I spent much of time to address all your comments.
> All are addressed and tested. except the above one(address offset calculation)
> I didn't see any issue with the address calculation.

Let me first point out that this comment was not trying to imply a bug.
I was trying to understand the code by comparing it to similar code and
that turned up an inconsistency, which can be intentional or a bug in
either of the sides being compared.

> for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
>   nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
>(8 * i);
> }
> If you go through the nand_base.c, there nand_fill_column_cycles() API, fills 
> the first two or one address cycle
> Based on bus width and page size.
> That means, addrs[0]/[1] will be updated here.

The problem at hand is that `addrs` is imprecise. In this code, there
are `instr->ctx.addr.addrs`, `addrs`, and `nfc_op->addrs`. All of them
are different. My original remark was targeting the possible confusion
of these different `addrs`.

> And the page is updated to the next offsets.
> In the similar way we have to extract the offsets in driver.
> So the first four address bytes are stored using the above for() loop and if 
> the
> Address cycles are more than 4, then store the remaining offsets as well.
> 
> I just compared the offsets that are updated in driver with the offsets(page 
> and column) that the frame work(nand_base.c) is sending, and the offsets are 
> same.
> I have also checked these offsets with older driver(not exec_op() 
> implemented) and both are matching.
> 
> So I didn't see any issue with this addrs calculation.
> As per the statement mentioned by you, this driver consumes addr[0], addr[1], 
> addr[2], addr[3] and
> If more address cycles needed, then addr[4] and addr[5]. This is correct.

Again, the lack of precision makes it difficult to discuss the matter.
You refer to `addr`, but there is no `addr`. I assume that you meant
`addrs` here. Based on that assumption, your second last statement is
wrong. The driver consumes `addrs[0]|addrs[-offset]` rather than
`addrs[0]` as the first byte.  Then it proceeds consuming
`addrs[1-offset]` instead of `addrs[1]`, `addrs[2-offset]` instead of
`addrs[2]`, and `addrs[3-offset]` instead of `addrs[3]`. Finally it
consumes `addrs[4]` and `addrs[5]` if more cycles are needed.

I would not have commented the code if it were actually using `addrs[0]`
through `addrs[5]`. Your description looks reasonable to me, but it
doesn't match the code.

I'm looking forward to the next version of the patch.

Helmut


Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-04-29 Thread Helmut Grohne
On Mon, Apr 29, 2019 at 11:31:14AM +, Naga Sureshkumar Relli wrote:
> But just wanted to know, do you see issues with these __force and __iomem 
> castings?

I only see a minor issue: They're (deliberately) lengthy. Using many of
them diverts attention of the reader. Therefore, my proposal attempted
to reduce their frequency. The only issue I see here is readability.

> > 
> > > + u8 addr_cycles;
> > > + struct clk *mclk;
> > 
> > All you need here is the memory clock frequency. Wouldn't it be easier to 
> > extract that
> > frequency once during probe and store it here? That assumes a constant 
> > frequency, but if the
> > frequency isn't constant, you have a race condition.
> That is what we are doing in the probe.
> In the probe, we are getting mclk using of_clk_get() and then we are getting 
> the actual frequency
> Using clk_get_rate().
> And this is constant frequency only(getting from dts)

Not quite. You're getting a clock reference in probe and then repeatedly
access the frequency elswhere. I am suggesting that you get the clock
frequency during probe and never save the clock reference to a struct.

> > > + case NAND_OP_ADDR_INSTR:
> > > + offset = nand_subop_get_addr_start_off(subop, op_id);
> > > + naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
> > > + addrs = >ctx.addr.addrs[offset];
> > > + nfc_op->addrs = instr->ctx.addr.addrs[offset];
> > > + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> > > + nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> > 
> > I don't quite understand what this code does, but it looks strange to me. I 
> > compared it to other
> > drivers. The code here is quite similar to marvell_nand.c. It seems like we 
> > are copying a
> > varying number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs. 
> > However their
> > indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is 
> > non-consecutive and different from
> > marvell_nand.c in this regard. Could it be that you really meant index 
> > offset+i here?
> I didn't get, what you are saying here.
> It is about updating page and column addresses.
> Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; 
> before for loop?

I compared this code to marvell_nand.c and noticed a subtle difference.
Both snippets read 6 address bytes and consume them in a driver-specific
way. Now which address bytes are consumed differs.

marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
(In my previous mail, I didn't notice that it was also consuming the
offset index.)

I would have expected this behaviour to be consistent between different
drivers. If I assume marvell_nand.c to do the right thing and
pl353_nand.c to be wrong (which is not necessarily a correct
assumption), then the code woule likely becom:

addrs = >ctx.addr.addrs[offset];
for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
nfc_op->addrs |= addrs[i] << (8 * i);
  // ^
}

Hope this helps.

Helmut


Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-04-25 Thread Helmut Grohne
On Wed, Apr 24, 2019 at 05:04:38AM +, Naga Sureshkumar Relli wrote:
> > You previously used cond_resched (via nand_wait_ready) here. Why did you 
> > change it to
> > cpu_relax()?
> I just replicated the pl353_wait_for_ecc_done() API definition.
> But did you see any issue with this?
> Anyway I will replace it with cond_resched(), instead of cpu_releax()

This was an observation and it made me ask for reasons. I did not have
any practical issues here.

> Did you follow the same thing that you tried earlier?
> i.e. updated "nand-bus-width" property and "nand-ecc-mode" ?

Yes, I used the same device tree that made v13 partially work here.

> > After trying the driver, the flash chip was bricked. Neither the old driver 
> > nor the uboot-xlnx
> > driver nor the Xilinx fsbl are able to talk to the chip afterwards. This 
> > behaviour persists even
> > after a full power cycle. I'll try reinitializing the flash chip next. I've 
> > only seen this behaviour
> > once, so there is a slight chance that the cause is something else.
> Sometimes I also faced the same problem during driver development.
> What I did is, in standalone nandps driver example,  I forcibly created BBT 
> in the init and once
>  it is done. I just reloaded the actual example. Then after wards u-boot and 
> Linux are able to scan
> the BBT.

I confirm. It was just the BBT being bad. It can also be recreated using
u-boot with "nand scrib.chip".

I also spent some time reviewing the code and will send another mail
about that.

Helmut


Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-04-25 Thread Helmut Grohne
Without much knowledge of the nand framework, I attempted reviewing the
code. Hope this helps.

Helmut

On Mon, Apr 15, 2019 at 04:40:13PM +0530, Naga Sureshkumar Relli wrote:
> diff --git a/drivers/mtd/nand/raw/pl353_nand.c 
> b/drivers/mtd/nand/raw/pl353_nand.c
> new file mode 100644
> index 000..eb63778
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/pl353_nand.c
> @@ -0,0 +1,1399 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM PL353 NAND flash controller driver
> + *
> + * Copyright (C) 2017 Xilinx, Inc
> + * Author: Punnaiah chowdary kalluri 
> + * Author: Naga Sureshkumar Relli 
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PL353_NAND_DRIVER_NAME "pl353-nand"
> +
> +/* NAND flash driver defines */
> +#define PL353_NAND_CMD_PHASE 1   /* End command valid in command phase */
> +#define PL353_NAND_DATA_PHASE2   /* End command valid in data 
> phase */

The two macros above are entirely unused. They're a relict from an
earlier driver version of the driver and were used in struct
pl35x_nand_command_format member end_cmd_valid. I think they can safely
be removed now.

> +#define PL353_NAND_ECC_SIZE  512 /* Size of data for ECC operation */
> +
> +/* Flash memory controller operating parameters */
> +
> +#define PL353_NAND_ECC_CONFIG(BIT(4)  |  /* ECC read at end of 
> page */ \
> +  (0 << 5))  /* No Jumping */

This macro is also unused even in older versions of the driver.

> +/* AXI Address definitions */
> +#define START_CMD_SHIFT  3
> +#define END_CMD_SHIFT11
> +#define END_CMD_VALID_SHIFT  20
> +#define ADDR_CYCLES_SHIFT21
> +#define CLEAR_CS_SHIFT   21
> +#define ECC_LAST_SHIFT   10
> +#define COMMAND_PHASE(0 << 19)
> +#define DATA_PHASE   BIT(19)
> +
> +#define PL353_NAND_ECC_LAST  BIT(ECC_LAST_SHIFT) /* Set ECC_Last */
> +#define PL353_NAND_CLEAR_CS  BIT(CLEAR_CS_SHIFT) /* Clear chip select */
> +
> +#define PL353_NAND_ECC_BUSY_TIMEOUT  (1 * HZ)
> +#define PL353_NAND_DEV_BUSY_TIMEOUT  (1 * HZ)

These timeouts are a second each. I've remarked earlier that you are
waiting with cpu_relax() on these. Having the CPU spin for a full second
is bad. Please try using less intensive waiting methods for such long
delays or reduce the timeouts.

> +#define PL353_NAND_LAST_TRANSFER_LENGTH  4
> +#define PL353_NAND_ECC_VALID_SHIFT   24
> +#define PL353_NAND_ECC_VALID_MASK0x40
> +#define PL353_ECC_BITS_BYTEOFF_MASK  0x1FF
> +#define PL353_ECC_BITS_BITOFF_MASK   0x7
> +#define PL353_ECC_BIT_MASK   0xFFF
> +#define PL353_TREA_MAX_VALUE 1
> +#define PL353_MAX_ECC_CHUNKS 4
> +#define PL353_MAX_ECC_BYTES  3
> +
> +struct pl353_nfc_op {
> + u32 cmnds[4];

Why does this hold 4 elements? In the code, this array is only indexed
with 0 and 1.

> + u32 end_cmd;

What is the purpose of this field. It is never accessed.

> + u32 addrs;
> + u32 naddrs;
> + u32 addr5;
> + u32 addr6;

Why are addr5 and addr6 u32? You only ever store u8 values in here. How
about merging them into an u16 addr56? Doing so would make the access in
pl353_nand_exec_op_cmd simpler and move a little complexity into
pl353_nfc_parse_instructions.

> + unsigned int data_instr_idx;
> + unsigned int rdy_timeout_ms;
> + unsigned int rdy_delay_ns;
> + unsigned int cle_ale_delay_ns;

What is the purpose of this field. It is set in two places, but never
read. No driver logic depends on its value.

> + const struct nand_op_instr *data_instr;
> +};
> +
> +/**
> + * struct pl353_nand_controller - Defines the NAND flash controller driver
> + * instance
> + * @chip:NAND chip information structure
> + * @dev: Parent device (used to print error messages)
> + * @regs:Virtual address of the NAND flash device
> + * @buf_addr:Virtual address of the NAND flash device for
> + *   data read/writes
> + * @addr_cycles: Address cycles
> + * @mclk:Memory controller clock
> + * @buswidth:Bus width 8 or 16
> + */
> +struct pl353_nand_controller {
> + struct nand_controller controller;
> + struct nand_chip chip;
> + struct device *dev;
> + void __iomem *regs;
> + void __iomem *buf_addr;

I find the use of buf_addr unfortunate. It is consumed by two functions
pl353_nand_read_data_op and pl353_nand_write_data_op. All other
functions update its value. Semantically, its value is regs + some
flags. For the updaters that means a complex logic where they first have
to subtract reg, then change flags and add reg again. To make matters
worse, this computation involves __force 

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-04-23 Thread Helmut Grohne
WARNING: This driver might brick the hardware. See below.

Hi Naga,

On Mon, Apr 15, 2019 at 04:40:13PM +0530, Naga Sureshkumar Relli wrote:
> Changes in v14:
>  - Removed legacy hooks as per Miquel comments

Thank you for the update.

> +static inline int pl353_wait_for_dev_ready(struct nand_chip *chip)
> +{
> + unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> +
> + do {
> + if (pl353_smc_get_nand_int_status_raw()) {
> + pl353_smc_clr_nand_int();
> + break;

A closing brace is missing here. This causes a compilation failure.

> +
> + cpu_relax();

You previously used cond_resched (via nand_wait_ready) here. Why did you
change it to cpu_relax()?

> + } while (!time_after_eq(jiffies, timeout));
> +
> + if (time_after_eq(jiffies, timeout)) {
> + pr_err("%s timed out\n", __func__);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}


> +static int pl353_nand_read_page_hwecc(struct nand_chip *chip,
> +   u8 *buf, int oob_required, int page)
> +{
> + int i, stat, eccsize = chip->ecc.size;
> + int eccbytes = chip->ecc.bytes;
> + int eccsteps = chip->ecc.steps;
> + u8 *p = buf;
> + u8 *ecc_calc = chip->ecc.calc_buf;
> + u8 *ecc = chip->ecc.code_buf;
> + unsigned int max_bitflips = 0;
> + u8 *oob_ptr;
> + u32 ret;
> + unsigned long data_phase_addr;
> + unsigned long nand_offset = (unsigned long __force)xnfc->regs;

The variable xnfc is undeclared here. Consider swapping the line with
the next one.

> + struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> + struct mtd_info *mtd = nand_to_mtd(chip);

After loading the driver, the device does not work. The dmesg output is:

nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
nand: Micron MT29F2G08ABAEAWP
nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
Bad block table not found for chip 0
Bad block table not found for chip 0
Scanning device for bad blocks
nand_bbt: error while writing BBT block -524
nand_bbt: error while writing BBT block -524
nand_bbt: error while writing BBT block -524
nand_bbt: error while writing BBT block -524
No space left to write bad block table
nand_bbt: error while writing bad block table -28
pl353-nand e100.flash: could not scan the nand chip
pl353-nand: probe of e100.flash failed with error -28

After trying the driver, the flash chip was bricked. Neither the old
driver nor the uboot-xlnx driver nor the Xilinx fsbl are able to talk to
the chip afterwards. This behaviour persists even after a full power
cycle. I'll try reinitializing the flash chip next. I've only seen this
behaviour once, so there is a slight chance that the cause is something
else.

Helmut


Re: [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-03-28 Thread Helmut Grohne
Hi Naga,

On Wed, Mar 27, 2019 at 09:13:59AM +, Naga Sureshkumar Relli wrote:
> It's a on-die ECC capable device. Did u mentioned nand-ecc-mode = "on-die" in 
> dts.
> The same part I tested by mentioning "on-die" property in dts and it worked 
> for me.
> Please share the dts entries for NAND.
> Also if it is x8 bus then please mention nand-bus-width = <8>;
> If it is x16 mention nand-bus-width = <16>;

Thank you for pointing at the relevant properties. Indeed, these were
missing in my previous tests. I am now using the following dt (generated
from multiple fragments, giving the decompiled dt here):

| memory-controller@e000e000 {
|   #address-cells = <0x2>;
|   #size-cells = <0x1>;
|   status = "okay";
|   clock-names = "memclk", "apb_pclk";
|   clocks = <0x1 0xb 0x1 0x2c>;
|   compatible = "arm,pl353-smc-r2p1", "arm,primecell";
|   interrupt-parent = <0x4>;
|   interrupts = <0x0 0x12 0x4>;
|   ranges = <0x0 0x0 0xe100 0x100>;
|   reg = <0xe000e000 0x1000>;
| 
|   flash@e100 {
|   status = "okay";
|   compatible = "arm,pl353-nand-r2p1";
|   reg = <0x0 0x0 0x100>;
|   #address-cells = <0x1>;
|   #size-cells = <0x1>;
|   nand-ecc-mode = "on-die";
|   nand-ecc-algo = "hamming";
|   nand-bus-width = <0x8>;
|   };
| };

With this dt, the device is successfully initialized and the data read
is mostly intact. When using it with jffs2, I get loads of ECC errors
though (offsets and lengths vary):

| jffs2: mtd->read(0x800 bytes from 0xb6) returned ECC error

Reverting back to the out-of-tree driver (4.14), it works normally, so a
hardware defect seems unlikely. I compared a register dump of the smc between
those drivers and the only difference I could find was NAND timings (at
0xE000E180), which are much lower with the new drivers as it does not consume
the arm,nand-cycle-* properties that the old driver consumed. I tried hard
coding the previous timings, but the ECC errors persist. This leads me to
conclude that timings are not the cause for what I am seeing.

Is there anything else I can try to diagnose it?

Helmut


Re: [LINUX PATCH v13] rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2019-03-26 Thread Helmut Grohne
On Sat, Feb 09, 2019 at 12:07:27PM +0530, Naga Sureshkumar Relli wrote:
> +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> + bool force_8bit)
> +{
> + struct pl353_nand_controller *xnfc =
> + container_of(chip, struct pl353_nand_controller, chip);

This `xnfc' variable is never used anywhere inside this function.

> +/**
> + * pl353_nand_exec_op_cmd - Send command to NAND device
> + * @chip:Pointer to the NAND chip info structure
> + * @subop:   Pointer to array of instructions
> + * Return:   Always return zero
> + */
> +static int pl353_nand_exec_op_cmd(struct nand_chip *chip,
> +   const struct nand_subop *subop)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + const struct nand_op_instr *instr;
> + struct pl353_nfc_op nfc_op = {};
> + struct pl353_nand_controller *xnfc =
> + container_of(chip, struct pl353_nand_controller, chip);
> + unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> + unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> + unsigned int op_id, len, offset;
> + bool reading;
> +
> + pl353_nfc_parse_instructions(chip, subop, _op);
> + instr = nfc_op.data_instr;
> + op_id = nfc_op.data_instr_idx;
> +
> + offset = nand_subop_get_data_start_off(subop, op_id);

This `offset' variable is never used anywhere inside this function. The
call is unnecessary and should be removed.

Beyond being useless, it also is harmful. When applying this patch on
top of a v5.1-rc2, this can be found in dmesg:

| [ cut here ]
| WARNING: CPU: 0 PID: 1 at .../linux/drivers/mtd/nand/raw/nand_base.c:2299 
nand_subop_get_data_start_off+0x30/0x6c
| CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc2-dirty #3
| Hardware name: Xilinx Zynq Platform
| [] (unwind_backtrace) from [] (show_stack+0x18/0x1c)
| [] (show_stack) from [] (dump_stack+0xa0/0xcc)
| [] (dump_stack) from [] (__warn+0x10c/0x128)
| [] (__warn) from [] (warn_slowpath_null+0x48/0x50)
| [] (warn_slowpath_null) from [] 
(nand_subop_get_data_start_off+0x30/0x6c)
| [] (nand_subop_get_data_start_off) from [] 
(pl353_nand_exec_op_cmd+0x94/0x2f0)
| [] (pl353_nand_exec_op_cmd) from [] 
(nand_op_parser_exec_op+0x460/0x4cc)
| [] (nand_op_parser_exec_op) from [] 
(nand_reset_op+0x134/0x1a0)
| [] (nand_reset_op) from [] (nand_reset+0x60/0xbc)
| [] (nand_reset) from [] (nand_scan_with_ids+0x288/0x1600)
| [] (nand_scan_with_ids) from [] 
(pl353_nand_probe+0xf8/0x1a0)
| [] (pl353_nand_probe) from [] 
(platform_drv_probe+0x3c/0x74)
| [] (platform_drv_probe) from [] (really_probe+0x278/0x400)
| [] (really_probe) from [] (bus_for_each_drv+0x68/0x9c)
| [] (bus_for_each_drv) from [] (__device_attach+0xa8/0x11c)
| [] (__device_attach) from [] (bus_probe_device+0x90/0x98)
| [] (bus_probe_device) from [] (device_add+0x3b4/0x5f0)
| [] (device_add) from [] 
(of_platform_device_create_pdata+0x98/0xc8)
| [] (of_platform_device_create_pdata) from [] 
(pl353_smc_probe+0x194/0x234)
| [] (pl353_smc_probe) from [] (amba_probe+0x60/0x74)
| [] (amba_probe) from [] (really_probe+0x278/0x400)
| [] (really_probe) from [] (device_driver_attach+0x60/0x68)
| [] (device_driver_attach) from [] 
(__driver_attach+0x88/0x180)
| [] (__driver_attach) from [] (bus_for_each_dev+0x60/0x9c)
| [] (bus_for_each_dev) from [] (bus_add_driver+0x10c/0x208)
| [] (bus_add_driver) from [] (driver_register+0x80/0x114)
| [] (driver_register) from [] (do_one_initcall+0x164/0x374)
| [] (do_one_initcall) from [] 
(kernel_init_freeable+0x394/0x474)
| [] (kernel_init_freeable) from [] (kernel_init+0x14/0x100)
| [] (kernel_init) from [] (ret_from_fork+0x14/0x28)
| Exception stack(0xdd8c9fb0 to 0xdd8c9ff8)
| 9fa0:    
| 9fc0:        
| 9fe0:     0013 
| irq event stamp: 414355
| hardirqs last  enabled at (414361): [] console_unlock+0x4c4/0x690
| hardirqs last disabled at (414366): [] console_unlock+0xdc/0x690
| softirqs last  enabled at (414350): [] __do_softirq+0x454/0x544
| softirqs last disabled at (414345): [] irq_exit+0x124/0x128
| ---[ end trace 3be9247df2f8dfb5 ]---

After removing the call (and the variable), this particular problem goes away.

> +/**
> + * pl353_nand_probe - Probe method for the NAND driver
> + * @pdev:Pointer to the platform_device structure
> + *
> + * This function initializes the driver data structures and the hardware.
> + * The NAND driver has dependency with the pl353_smc memory controller
> + * driver for initializing the NAND timing parameters, bus width, ECC modes,
> + * control and status information.
> + *
> + * Return:   0 on success or error value on failure
> + */
> +static int pl353_nand_probe(struct platform_device *pdev)
> +{
> + struct pl353_nand_controller *xnfc;
> + struct mtd_info *mtd;
> + 

Re: [LINUX PATCH v12] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2018-11-05 Thread Helmut Grohne
On Tue, Aug 07, 2018 at 11:10:14AM +0530, Naga Sureshkumar Relli wrote:
> Add driver for arm pl353 static memory controller nand interface with
> HW ECC support. This controller is used in Xilinx Zynq SoC for
> interfacing the NAND flash memory.
> 
> Signed-off-by: Naga Sureshkumar Relli 
> ---
> Changes in v12:

I attempted to apply this v12 together with the v11 of pl353-smc (which seems
to be the latest version) onto a vanilla v4.19 kernel tree. Application was
smooth and a build was without errors. During boot, my dmesg was spammed with
lots of repetitions of this:

[5.816275] [ cut here ]
[5.820981] WARNING: CPU: 0 PID: 1 at 
.../linux/drivers/mtd/nand/raw/nand_base.c:2773 
nand_subop_get_data_len+0x60/0xa4
[5.836868] Modules linked in:
[5.836912] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 
4.19.0-dirty #3
[5.836929] Hardware name: Xilinx Zynq Platform
[5.836943] Backtrace: 
[5.836981] [] (dump_backtrace) from [] 
(show_stack+0x20/0x24)
[5.837004]  r7:c062e618 r6: r5:600f0013 r4:c062e618
[5.837027] [] (show_stack) from [] 
(dump_stack+0xac/0xd8)
[5.837055] [] (dump_stack) from [] (__warn+0x118/0x130)
[5.837079]  r10: r9:c0270be8 r8:0ad5 r7:c0550358 r6:0009 
r5:
[5.837097]  r4: r3:de4c6000
[5.837122] [] (__warn) from [] 
(warn_slowpath_null+0x50/0x58)
[5.837146]  r9:de4c78c5 r8:da4d1010 r7:de4c78f8 r6:c0270be8 r5:0ad5 
r4:c0550358
[5.837173] [] (warn_slowpath_null) from [] 
(nand_subop_get_data_len+0x60/0xa4)
[5.837192]  r6:003c r5:0003 r4:de4c7870
[5.837217] [] (nand_subop_get_data_len) from [] 
(pl353_nand_exec_op_cmd+0x60/0x314)
[5.837238]  r7:de4c78f8 r6:003c r5:de4c7870 r4:0003
[5.837262] [] (pl353_nand_exec_op_cmd) from [] 
(nand_op_parser_exec_op+0x484/0x4e8)
[5.837285]  r10:0002 r9:0014 r8:de4c790c r7:0004 r6:c0634260 
r5:c052cc44
[5.837301]  r4:c0620bcc
[5.837325] [] (nand_op_parser_exec_op) from [] 
(pl353_nfc_exec_op+0x24/0x2c)
[5.837348]  r10:de4c7a18 r9:0080 r8:03463678 r7:89705f41 r6:36b4a597 
r5:de4c78d0
[5.837364]  r4:da4d1010
[5.837387] [] (pl353_nfc_exec_op) from [] 
(nand_erase_op+0x150/0x1f0)
[5.837411] [] (nand_erase_op) from [] 
(single_erase+0x28/0x2c)
[5.837434]  r9:e08c1000 r8:0006 r7:0040 r6:0001 r5:0001ff80 
r4:da4d1010
[5.837459] [] (single_erase) from [] 
(nand_erase_nand+0x1f8/0x3f8)
[5.837484] [] (nand_erase_nand) from [] 
(write_bbt+0x30c/0x748)
[5.837508]  r10:0006 r9:e08c1000 r8:0006 r7:0002 r6:da4d1010 
r5:
[5.837524]  r4:0ffc
[5.837549] [] (write_bbt) from [] 
(nand_create_bbt+0x30c/0x6d0)
[5.837572]  r10:c061fd2c r9:c061fce8 r8: r7:c061fd2c r6: 
r5:0003
[5.837587]  r4:
[5.837614] [] (nand_create_bbt) from [] 
(nand_scan_with_ids+0x1248/0x1e74)
[5.837636]  r10:c0620524 r9:0004 r8:0001 r7:0004 r6:da4d1378 
r5:0001
[5.837652]  r4:da4d1010
[5.837676] [] (nand_scan_with_ids) from [] 
(pl353_nand_probe+0x144/0x208)
[5.837699]  r10: r9:c0620ae0 r8:c0633ba0 r7: r6:da4d8e10 
r5:da4d8e00
[5.837715]  r4:da4d1010
[5.837738] [] (pl353_nand_probe) from [] 
(platform_drv_probe+0x44/0x7c)
[5.837757]  r6:c0e0205c r5:c0620ae0 r4:da4d8e10
[5.837783] [] (platform_drv_probe) from [] 
(really_probe+0x27c/0x408)
[5.837801]  r5:da4d8e10 r4:c0e02058
[5.837827] [] (really_probe) from [] 
(driver_probe_device+0x138/0x198)
[5.837850]  r10: r9: r8:c0e02014 r7:0001 r6:de4c7c70 
r5:c0620ae0
[5.837865]  r4:da4d8e10
[5.837891] [] (driver_probe_device) from [] 
(__device_attach_driver+0xc8/0x144)
[5.837913]  r9: r8:c0e02014 r6:de4c7c70 r5:da4d8e10 r4:c0620ae0
[5.837938] [] (__device_attach_driver) from [] 
(bus_for_each_drv+0x70/0xa4)
[5.837958]  r7:0001 r6:c0251f0c r5:de4c7c70 r4:
[5.837982] [] (bus_for_each_drv) from [] 
(__device_attach+0xb0/0x11c)
[5.838001]  r6:c061e8d0 r5:da4d8e44 r4:da4d8e10
[5.838027] [] (__device_attach) from [] 
(device_initial_probe+0x1c/0x20)
[5.838047]  r7:da4d8e10 r6:c061e8d0 r5:da4d8e10 r4:da4d8e18
[5.838073] [] (device_initial_probe) from [] 
(bus_probe_device+0x98/0xa0)
[5.838097] [] (bus_probe_device) from [] 
(device_add+0x380/0x5dc)
[5.838117]  r7:da4d8e10 r6:de595e00 r5: r4:da4d8e18
[5.838142] [] (device_add) from [] 
(of_device_add+0x44/0x4c)
[5.838164]  r10:c0563da4 r9:dfbf0a70 r8:dfbf0ac0 r7: r6:de595e00 
r5:
[5.838180]  r4:da4d8e00
[5.838205] [] (of_device_add) from [] 
(of_platform_device_create_pdata+0xa0/0xd0)
[5.838229] [] (of_platform_device_create_pdata) from [] 
(of_platform_device_create+0x20/0x24)
[5.838252]  r9:dfbf0a70 r8:dfbf0780 r7:c0470470 r6:ddfe9f50 r5: 
r4:de595e00
[5.838280] [] (of_platform_device_create) 

Re: [LINUX PATCH v12] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

2018-11-05 Thread Helmut Grohne
On Tue, Aug 07, 2018 at 11:10:14AM +0530, Naga Sureshkumar Relli wrote:
> Add driver for arm pl353 static memory controller nand interface with
> HW ECC support. This controller is used in Xilinx Zynq SoC for
> interfacing the NAND flash memory.
> 
> Signed-off-by: Naga Sureshkumar Relli 
> ---
> Changes in v12:

I attempted to apply this v12 together with the v11 of pl353-smc (which seems
to be the latest version) onto a vanilla v4.19 kernel tree. Application was
smooth and a build was without errors. During boot, my dmesg was spammed with
lots of repetitions of this:

[5.816275] [ cut here ]
[5.820981] WARNING: CPU: 0 PID: 1 at 
.../linux/drivers/mtd/nand/raw/nand_base.c:2773 
nand_subop_get_data_len+0x60/0xa4
[5.836868] Modules linked in:
[5.836912] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW 
4.19.0-dirty #3
[5.836929] Hardware name: Xilinx Zynq Platform
[5.836943] Backtrace: 
[5.836981] [] (dump_backtrace) from [] 
(show_stack+0x20/0x24)
[5.837004]  r7:c062e618 r6: r5:600f0013 r4:c062e618
[5.837027] [] (show_stack) from [] 
(dump_stack+0xac/0xd8)
[5.837055] [] (dump_stack) from [] (__warn+0x118/0x130)
[5.837079]  r10: r9:c0270be8 r8:0ad5 r7:c0550358 r6:0009 
r5:
[5.837097]  r4: r3:de4c6000
[5.837122] [] (__warn) from [] 
(warn_slowpath_null+0x50/0x58)
[5.837146]  r9:de4c78c5 r8:da4d1010 r7:de4c78f8 r6:c0270be8 r5:0ad5 
r4:c0550358
[5.837173] [] (warn_slowpath_null) from [] 
(nand_subop_get_data_len+0x60/0xa4)
[5.837192]  r6:003c r5:0003 r4:de4c7870
[5.837217] [] (nand_subop_get_data_len) from [] 
(pl353_nand_exec_op_cmd+0x60/0x314)
[5.837238]  r7:de4c78f8 r6:003c r5:de4c7870 r4:0003
[5.837262] [] (pl353_nand_exec_op_cmd) from [] 
(nand_op_parser_exec_op+0x484/0x4e8)
[5.837285]  r10:0002 r9:0014 r8:de4c790c r7:0004 r6:c0634260 
r5:c052cc44
[5.837301]  r4:c0620bcc
[5.837325] [] (nand_op_parser_exec_op) from [] 
(pl353_nfc_exec_op+0x24/0x2c)
[5.837348]  r10:de4c7a18 r9:0080 r8:03463678 r7:89705f41 r6:36b4a597 
r5:de4c78d0
[5.837364]  r4:da4d1010
[5.837387] [] (pl353_nfc_exec_op) from [] 
(nand_erase_op+0x150/0x1f0)
[5.837411] [] (nand_erase_op) from [] 
(single_erase+0x28/0x2c)
[5.837434]  r9:e08c1000 r8:0006 r7:0040 r6:0001 r5:0001ff80 
r4:da4d1010
[5.837459] [] (single_erase) from [] 
(nand_erase_nand+0x1f8/0x3f8)
[5.837484] [] (nand_erase_nand) from [] 
(write_bbt+0x30c/0x748)
[5.837508]  r10:0006 r9:e08c1000 r8:0006 r7:0002 r6:da4d1010 
r5:
[5.837524]  r4:0ffc
[5.837549] [] (write_bbt) from [] 
(nand_create_bbt+0x30c/0x6d0)
[5.837572]  r10:c061fd2c r9:c061fce8 r8: r7:c061fd2c r6: 
r5:0003
[5.837587]  r4:
[5.837614] [] (nand_create_bbt) from [] 
(nand_scan_with_ids+0x1248/0x1e74)
[5.837636]  r10:c0620524 r9:0004 r8:0001 r7:0004 r6:da4d1378 
r5:0001
[5.837652]  r4:da4d1010
[5.837676] [] (nand_scan_with_ids) from [] 
(pl353_nand_probe+0x144/0x208)
[5.837699]  r10: r9:c0620ae0 r8:c0633ba0 r7: r6:da4d8e10 
r5:da4d8e00
[5.837715]  r4:da4d1010
[5.837738] [] (pl353_nand_probe) from [] 
(platform_drv_probe+0x44/0x7c)
[5.837757]  r6:c0e0205c r5:c0620ae0 r4:da4d8e10
[5.837783] [] (platform_drv_probe) from [] 
(really_probe+0x27c/0x408)
[5.837801]  r5:da4d8e10 r4:c0e02058
[5.837827] [] (really_probe) from [] 
(driver_probe_device+0x138/0x198)
[5.837850]  r10: r9: r8:c0e02014 r7:0001 r6:de4c7c70 
r5:c0620ae0
[5.837865]  r4:da4d8e10
[5.837891] [] (driver_probe_device) from [] 
(__device_attach_driver+0xc8/0x144)
[5.837913]  r9: r8:c0e02014 r6:de4c7c70 r5:da4d8e10 r4:c0620ae0
[5.837938] [] (__device_attach_driver) from [] 
(bus_for_each_drv+0x70/0xa4)
[5.837958]  r7:0001 r6:c0251f0c r5:de4c7c70 r4:
[5.837982] [] (bus_for_each_drv) from [] 
(__device_attach+0xb0/0x11c)
[5.838001]  r6:c061e8d0 r5:da4d8e44 r4:da4d8e10
[5.838027] [] (__device_attach) from [] 
(device_initial_probe+0x1c/0x20)
[5.838047]  r7:da4d8e10 r6:c061e8d0 r5:da4d8e10 r4:da4d8e18
[5.838073] [] (device_initial_probe) from [] 
(bus_probe_device+0x98/0xa0)
[5.838097] [] (bus_probe_device) from [] 
(device_add+0x380/0x5dc)
[5.838117]  r7:da4d8e10 r6:de595e00 r5: r4:da4d8e18
[5.838142] [] (device_add) from [] 
(of_device_add+0x44/0x4c)
[5.838164]  r10:c0563da4 r9:dfbf0a70 r8:dfbf0ac0 r7: r6:de595e00 
r5:
[5.838180]  r4:da4d8e00
[5.838205] [] (of_device_add) from [] 
(of_platform_device_create_pdata+0xa0/0xd0)
[5.838229] [] (of_platform_device_create_pdata) from [] 
(of_platform_device_create+0x20/0x24)
[5.838252]  r9:dfbf0a70 r8:dfbf0780 r7:c0470470 r6:ddfe9f50 r5: 
r4:de595e00
[5.838280] [] (of_platform_device_create) 

Re: RFC: remove the "tile" architecture from glibc

2018-03-07 Thread Helmut Grohne
On Wed, Mar 07, 2018 at 04:39:47PM +0100, Arnd Bergmann wrote:
> - Helmut Grohne has done the work to add tile-gx to debian
>rebootstrap, and send several patches, as seen in
>https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=823167
>However, I could find no information on this actually
>being turned into a full port, or anyone still actively involved
>in it. The Debian rebootstrap page at
>https://wiki.debian.org/HelmutGrohne/rebootstrap
>mentions lots of other architectures, but not this one.

I happened help tilegx, because Adrian found someone with hw and tilegx
appeared to be very well maintained upstream. Very little needed to be
done to make it work in Debian. The patches I sent were pretty generic
and addressed issues that most ports face. What is there allows
constructing most of an essential package set and (unlike a number of
other architectures) bootstrapping tilegx works fairly well.  Debian has
a number of source-only ports and tilegx is now one of them.

That said, nobody has taken up the work to proceed a native bootstrap.
Like with nios2, progress is stalled, because the tooling for fully
automating the native phase is missing.

In any case, I won't be able to fix binutils/gcc/glibc/linux issues with
tilegx. So unless someone steps up to do that work, I won't object to
dropping it. It would be sad to see tilegx go, but it certainly isn't my
core interest either.

I'd appreciate a note if it gets dropped from any of these, because I'd
clean up outstanding bug reports then.

The reverse is also true: If you want to see an new architecture in
Debian, I also appreciate an early conversation to avoid duplicating
work.

Hope this helps

Helmut


Re: RFC: remove the "tile" architecture from glibc

2018-03-07 Thread Helmut Grohne
On Wed, Mar 07, 2018 at 04:39:47PM +0100, Arnd Bergmann wrote:
> - Helmut Grohne has done the work to add tile-gx to debian
>rebootstrap, and send several patches, as seen in
>https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=823167
>However, I could find no information on this actually
>being turned into a full port, or anyone still actively involved
>in it. The Debian rebootstrap page at
>https://wiki.debian.org/HelmutGrohne/rebootstrap
>mentions lots of other architectures, but not this one.

I happened help tilegx, because Adrian found someone with hw and tilegx
appeared to be very well maintained upstream. Very little needed to be
done to make it work in Debian. The patches I sent were pretty generic
and addressed issues that most ports face. What is there allows
constructing most of an essential package set and (unlike a number of
other architectures) bootstrapping tilegx works fairly well.  Debian has
a number of source-only ports and tilegx is now one of them.

That said, nobody has taken up the work to proceed a native bootstrap.
Like with nios2, progress is stalled, because the tooling for fully
automating the native phase is missing.

In any case, I won't be able to fix binutils/gcc/glibc/linux issues with
tilegx. So unless someone steps up to do that work, I won't object to
dropping it. It would be sad to see tilegx go, but it certainly isn't my
core interest either.

I'd appreciate a note if it gets dropped from any of these, because I'd
clean up outstanding bug reports then.

The reverse is also true: If you want to see an new architecture in
Debian, I also appreciate an early conversation to avoid duplicating
work.

Hope this helps

Helmut


PROBLEM: SECCOMP documentation outdated in some arch/*/Kconfig

2008-01-22 Thread Helmut Grohne
Hi,

I didn't find out whom to report this bug to and thus report to
linux-kernel@vger.kernel.org as described in
http://kernel.org/pub/linux/docs/lkml/reporting-bugs.html.

I'm posting from outside, so please CC me.

[1] The description about seccomp is outdated in some arch/*/Kconfig
files.

[2] According to the source (2.6.23.14) seccomp is to be activated using
pcrtl. It was previously activated using a file /proc//seccomp.
The Kconfig documentation (also displayed in menuconfig) does not
reflect this change and is thus wrong.

[3] seccomp documentation Kconfig

[4] 2.6.23.14, seems to also apply to git head:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/x86/Kconfig;h=80b7ba4056dbbb566841c1e1cbef9475730fe199;hb=HEAD

[5] no oops

[6] less arch/x86_64/Kconfig
/SECCOMP

[7] Ask me again if you really think you need information about the
environment for a documentation bug.

Helmut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


PROBLEM: SECCOMP documentation outdated in some arch/*/Kconfig

2008-01-22 Thread Helmut Grohne
Hi,

I didn't find out whom to report this bug to and thus report to
linux-kernel@vger.kernel.org as described in
http://kernel.org/pub/linux/docs/lkml/reporting-bugs.html.

I'm posting from outside, so please CC me.

[1] The description about seccomp is outdated in some arch/*/Kconfig
files.

[2] According to the source (2.6.23.14) seccomp is to be activated using
pcrtl. It was previously activated using a file /proc/pid/seccomp.
The Kconfig documentation (also displayed in menuconfig) does not
reflect this change and is thus wrong.

[3] seccomp documentation Kconfig

[4] 2.6.23.14, seems to also apply to git head:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/x86/Kconfig;h=80b7ba4056dbbb566841c1e1cbef9475730fe199;hb=HEAD

[5] no oops

[6] less arch/x86_64/Kconfig
/SECCOMP

[7] Ask me again if you really think you need information about the
environment for a documentation bug.

Helmut
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/