Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Mon, Mar 16, 2015 at 11:55:16AM -0700, Florian Fainelli wrote:
> On 06/03/15 17:18, Brian Norris wrote:
> > +static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
> > +{
> > + static const unsigned int block_sizes_v6[] = { 8, 16, 128, 256, 512,
> > 1024, 2048, 0 };
> > + static const unsigned int block_sizes_v4[] = { 16, 128, 8, 512, 256,
> > 1024, 2048, 0 };
> > + static const unsigned int page_sizes[] = { 512, 2048, 4096, 8192, 0 };
> > +
> > + ctrl->nand_version = nand_readreg(ctrl, 0) & 0x;
> > +
> > + /* Only support v4.0+? */
> > + if (ctrl->nand_version < 0x0400)
> > + return -ENODEV;
>
> It could be nice to have an informative error message here that this is
> either:
>
> - an unknown controller revision (> 7.1)
> - an older controller revision
> - a check against the compatible property, just in case?
I'll add a message that the revision is not supported. The DT binding
should catch this too, so this check is just an extra safeguard.
> [snip]
>
> > + ctrl->cs_offsets = brcmnand_cs_offsets_v71;
> > + } else {
> > + ctrl->cs_offsets = brcmnand_cs_offsets;
> > +
> > + /* pre-v5.0 has a different CS0 offset layout */
> > + if (ctrl->nand_version <= 0x0500)
> > + ctrl->cs0_offsets = brcmnand_cs_offsets_cs0;
>
> Based on this check, should the comment should be "pre-v5.0 and v5.0
> have a different CS0 offset layout"?
Yes. Fixed.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Mon, Mar 16, 2015 at 11:55:16AM -0700, Florian Fainelli wrote:
On 06/03/15 17:18, Brian Norris wrote:
+static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
+{
+ static const unsigned int block_sizes_v6[] = { 8, 16, 128, 256, 512,
1024, 2048, 0 };
+ static const unsigned int block_sizes_v4[] = { 16, 128, 8, 512, 256,
1024, 2048, 0 };
+ static const unsigned int page_sizes[] = { 512, 2048, 4096, 8192, 0 };
+
+ ctrl-nand_version = nand_readreg(ctrl, 0) 0x;
+
+ /* Only support v4.0+? */
+ if (ctrl-nand_version 0x0400)
+ return -ENODEV;
It could be nice to have an informative error message here that this is
either:
- an unknown controller revision ( 7.1)
- an older controller revision
- a check against the compatible property, just in case?
I'll add a message that the revision is not supported. The DT binding
should catch this too, so this check is just an extra safeguard.
[snip]
+ ctrl-cs_offsets = brcmnand_cs_offsets_v71;
+ } else {
+ ctrl-cs_offsets = brcmnand_cs_offsets;
+
+ /* pre-v5.0 has a different CS0 offset layout */
+ if (ctrl-nand_version = 0x0500)
+ ctrl-cs0_offsets = brcmnand_cs_offsets_cs0;
Based on this check, should the comment should be pre-v5.0 and v5.0
have a different CS0 offset layout?
Yes. Fixed.
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
[snip]
> +static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
> + u32 len, u8 dma_cmd)
> +{
> + struct brcmnand_controller *ctrl = host->ctrl;
> + dma_addr_t buf_pa;
> + int dir = dma_cmd == CMD_PAGE_READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +
> + buf_pa = dma_map_single(ctrl->dev, buf, len, dir);
We are missing a dma_mapping_error() check here.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 06/03/15 17:18, Brian Norris wrote:
> This core originated in Set-Top Box chips (BCM7xxx) but is used in a
> variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and
> iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it
> to those architectures.
>
> There are multiple revisions of this core throughout the years, and
> almost every version broke register compatibility in some small way, but
> with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0,
> and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so
> there hopefully are no more lurking inconsistencies.
>
> Signed-off-by: Brian Norris
> ---
Looks good to me, just one nitpick below:
[snip]
> +static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
> +{
> + static const unsigned int block_sizes_v6[] = { 8, 16, 128, 256, 512,
> 1024, 2048, 0 };
> + static const unsigned int block_sizes_v4[] = { 16, 128, 8, 512, 256,
> 1024, 2048, 0 };
> + static const unsigned int page_sizes[] = { 512, 2048, 4096, 8192, 0 };
> +
> + ctrl->nand_version = nand_readreg(ctrl, 0) & 0x;
> +
> + /* Only support v4.0+? */
> + if (ctrl->nand_version < 0x0400)
> + return -ENODEV;
It could be nice to have an informative error message here that this is
either:
- an unknown controller revision (> 7.1)
- an older controller revision
- a check against the compatible property, just in case?
[snip]
> + ctrl->cs_offsets = brcmnand_cs_offsets_v71;
> + } else {
> + ctrl->cs_offsets = brcmnand_cs_offsets;
> +
> + /* pre-v5.0 has a different CS0 offset layout */
> + if (ctrl->nand_version <= 0x0500)
> + ctrl->cs0_offsets = brcmnand_cs_offsets_cs0;
Based on this check, should the comment should be "pre-v5.0 and v5.0
have a different CS0 offset layout"?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 06/03/15 17:18, Brian Norris wrote:
This core originated in Set-Top Box chips (BCM7xxx) but is used in a
variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and
iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it
to those architectures.
There are multiple revisions of this core throughout the years, and
almost every version broke register compatibility in some small way, but
with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0,
and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so
there hopefully are no more lurking inconsistencies.
Signed-off-by: Brian Norris computersforpe...@gmail.com
---
Looks good to me, just one nitpick below:
[snip]
+static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
+{
+ static const unsigned int block_sizes_v6[] = { 8, 16, 128, 256, 512,
1024, 2048, 0 };
+ static const unsigned int block_sizes_v4[] = { 16, 128, 8, 512, 256,
1024, 2048, 0 };
+ static const unsigned int page_sizes[] = { 512, 2048, 4096, 8192, 0 };
+
+ ctrl-nand_version = nand_readreg(ctrl, 0) 0x;
+
+ /* Only support v4.0+? */
+ if (ctrl-nand_version 0x0400)
+ return -ENODEV;
It could be nice to have an informative error message here that this is
either:
- an unknown controller revision ( 7.1)
- an older controller revision
- a check against the compatible property, just in case?
[snip]
+ ctrl-cs_offsets = brcmnand_cs_offsets_v71;
+ } else {
+ ctrl-cs_offsets = brcmnand_cs_offsets;
+
+ /* pre-v5.0 has a different CS0 offset layout */
+ if (ctrl-nand_version = 0x0500)
+ ctrl-cs0_offsets = brcmnand_cs_offsets_cs0;
Based on this check, should the comment should be pre-v5.0 and v5.0
have a different CS0 offset layout?
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
[snip]
+static int brcmnand_dma_trans(struct brcmnand_host *host, u64 addr, u32 *buf,
+ u32 len, u8 dma_cmd)
+{
+ struct brcmnand_controller *ctrl = host-ctrl;
+ dma_addr_t buf_pa;
+ int dir = dma_cmd == CMD_PAGE_READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
+
+ buf_pa = dma_map_single(ctrl-dev, buf, len, dir);
We are missing a dma_mapping_error() check here.
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 3/9/2015 10:49 AM, Brian Norris wrote: > On Sun, Mar 08, 2015 at 01:44:02AM +0100, Rafał Miłecki wrote: >> On 7 March 2015 at 18:39, Rafał Miłecki wrote: >>> It seems that brcmnand_ctlrdy_irq never fires on my device. Just like >>> controller was never generating any IRQ. >>> >>> >>> I started comparing your driver with OpenWrt's bcm_nand.c (which >>> should be very similar to Broadcom's SDK NAND driver for ARM). Below >>> are few things I've noticed. >>> >>> 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). >> >> Oh, wait, I was wrong there. So in bcm_nand.c IRQ handler fires very >> often, just not during the early phase (RESET, READID), when we don't >> use IRQs. During standard read/program/etc IRQ is commonly used. >> >> So maybe all we're missing in case of brcmstb_nand.c is >> enabling/acking/disabling IRQ? > > Yes, that's likely the main problem. > >> It seem that my controller 6.01 has: >> >> 1) Following IRQs: >> DIREC_READ_MISS >> ERASE_COMPLETE >> COPYBACK_COMPLETE >> PROGRAM_COMPLETE >> CONTROLLER_RDY >> RDBSY_RDY >> ECC_UNCORRECTABLE >> ECC_CORRECTABLE > > Right, that's the standard set of NAND interrupts. This driver only uses > CONTROLLER_RDY, BTW. > >> 2) Registers for reading/acking above IRQs: >> 0xf00 DIREC_READ_MISS >> 0xf04 ERASE_COMPLETE >> 0xf08 COPYBACK_COMPLETE >> 0xf0c PROGRAM_COMPLETE >> 0xf10 CONTROLLER_RDY >> 0xf14 RDBSY_RDY >> 0xf18 ECC_UNCORRECTABLE >> 0xf1c ECC_CORRECTABLE >> (if 0x1 is set, it means IRQ was raised, writing 0x1 ack-es it) >> >> 3) Register 0x408 for enabling/disabling IRQs: >> 0x0004 DIREC_READ_MISS >> 0x0008 ERASE_COMPLETE >> 0x0010 COPYBACK_COMPLETE >> 0x0020 PROGRAM_COMPLETE >> 0x0040 CONTROLLER_RDY >> 0x0080 RDBSY_RDY >> 0x0100 ECC_UNCORRECTABLE >> 0x0200 ECC_CORRECTABLE > > (2) and (3) look pretty similar to the Cygnus chips. We should probably > end up using the same solution for both. Several related to the Cygnus > project are on CC. > > One problem for Cygnus is that some of the other bits in their register > 0x408 deal with non-interrupt-related functionality (endianness and > clocks), so it might not make the cleanest design to handle this with an > irqchip driver. It's not just similar, but it's identical to Cygnus. The register offset to read/ack the IRQ are the same. and Cygnus also uses a standard alone register to enable/disable IRQ (and their bit field is the same). Based on the link from Rafal, this chip also have the same issue with APB endianess control, clock control, interrupt control all stuffed in the same register: #define NANDC_IDM_AXI_BIG_ENDIANIDMREG_BIT_FIELD(0x408, 28, 1) #define NANDC_IDM_APB_LITTLE_ENDIAN IDMREG_BIT_FIELD(0x408, 24, 1) #define NANDC_IDM_TMIDMREG_BIT_FIELD(0x408, 16, 5) #define NANDC_IDM_IRQ_CORRECABLE_EN IDMREG_BIT_FIELD(0x408, 9, 1) #define NANDC_IDM_IRQ_UNCORRECABLE_EN IDMREG_BIT_FIELD(0x408, 8, 1) #define NANDC_IDM_IRQ_RDYBSY_RDY_EN IDMREG_BIT_FIELD(0x408, 7, 1) #define NANDC_IDM_IRQ_CONTROLLER_RDY_EN IDMREG_BIT_FIELD(0x408, 6, 1) #define NANDC_IDM_IRQ_PRPOGRAM_COMP_EN IDMREG_BIT_FIELD(0x408, 5, 1) #define NANDC_IDM_IRQ_COPYBK_COMP_ENIDMREG_BIT_FIELD(0x408, 4, 1) #define NANDC_IDM_IRQ_ERASE_COMP_EN IDMREG_BIT_FIELD(0x408, 3, 1) #define NANDC_IDM_IRQ_READ_MISS_EN IDMREG_BIT_FIELD(0x408, 2, 1) #define NANDC_IDM_IRQ_N_EN(n) IDMREG_BIT_FIELD(0x408, 2+(n), 1) #define NANDC_IDM_CLOCK_EN IDMREG_BIT_FIELD(0x408, 0, 1) > > Brian > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Sun, Mar 08, 2015 at 01:44:02AM +0100, Rafał Miłecki wrote: > On 7 March 2015 at 18:39, Rafał Miłecki wrote: > > It seems that brcmnand_ctlrdy_irq never fires on my device. Just like > > controller was never generating any IRQ. > > > > > > I started comparing your driver with OpenWrt's bcm_nand.c (which > > should be very similar to Broadcom's SDK NAND driver for ARM). Below > > are few things I've noticed. > > > > 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). > > Oh, wait, I was wrong there. So in bcm_nand.c IRQ handler fires very > often, just not during the early phase (RESET, READID), when we don't > use IRQs. During standard read/program/etc IRQ is commonly used. > > So maybe all we're missing in case of brcmstb_nand.c is > enabling/acking/disabling IRQ? Yes, that's likely the main problem. > It seem that my controller 6.01 has: > > 1) Following IRQs: > DIREC_READ_MISS > ERASE_COMPLETE > COPYBACK_COMPLETE > PROGRAM_COMPLETE > CONTROLLER_RDY > RDBSY_RDY > ECC_UNCORRECTABLE > ECC_CORRECTABLE Right, that's the standard set of NAND interrupts. This driver only uses CONTROLLER_RDY, BTW. > 2) Registers for reading/acking above IRQs: > 0xf00 DIREC_READ_MISS > 0xf04 ERASE_COMPLETE > 0xf08 COPYBACK_COMPLETE > 0xf0c PROGRAM_COMPLETE > 0xf10 CONTROLLER_RDY > 0xf14 RDBSY_RDY > 0xf18 ECC_UNCORRECTABLE > 0xf1c ECC_CORRECTABLE > (if 0x1 is set, it means IRQ was raised, writing 0x1 ack-es it) > > 3) Register 0x408 for enabling/disabling IRQs: > 0x0004 DIREC_READ_MISS > 0x0008 ERASE_COMPLETE > 0x0010 COPYBACK_COMPLETE > 0x0020 PROGRAM_COMPLETE > 0x0040 CONTROLLER_RDY > 0x0080 RDBSY_RDY > 0x0100 ECC_UNCORRECTABLE > 0x0200 ECC_CORRECTABLE (2) and (3) look pretty similar to the Cygnus chips. We should probably end up using the same solution for both. Several related to the Cygnus project are on CC. One problem for Cygnus is that some of the other bits in their register 0x408 deal with non-interrupt-related functionality (endianness and clocks), so it might not make the cleanest design to handle this with an irqchip driver. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Sat, Mar 07, 2015 at 02:21:26PM +0100, Rafał Miłecki wrote: > On 7 March 2015 at 02:18, Brian Norris wrote: > > + /* NAND register range */ > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ctrl->nand_base = devm_ioremap_resource(dev, res); > > + if (!ctrl->nand_base) > > + return -ENODEV; > > This is what I got during my first test: > brcmstb_nand 11315e0.nand: can't request region for resource [mem > 0x011315e0-0x011325df] > Unable to handle kernel paging request at virtual address fff0 > (not sure why yet). > > Hint: devm_ioremap_resource does never return NULL Thanks, fixed in 3 places. I expect you've figured out your problem here by now. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Sat, Mar 07, 2015 at 01:39:43PM +0100, Paul Bolle wrote:
> Only a license nit.
>
> Brian Norris schreef op vr 06-03-2015 om 17:18 [-0800]:
>
> > --- /dev/null
> > +++ b/drivers/mtd/nand/brcmstb_nand.c
> > @@ -0,0 +1,2182 @@
> > +/*
> > + * Copyright © 2010-2015 Broadcom Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
>
> This states the license is GPL v2.
>
> > +MODULE_LICENSE("GPL");
>
> So you probably want
>MODULE_LICENSE("GPL v2");
>
> here.
Thanks. Fixed.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Sat, Mar 07, 2015 at 01:39:43PM +0100, Paul Bolle wrote: Only a license nit. Brian Norris schreef op vr 06-03-2015 om 17:18 [-0800]: --- /dev/null +++ b/drivers/mtd/nand/brcmstb_nand.c @@ -0,0 +1,2182 @@ +/* + * Copyright © 2010-2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ This states the license is GPL v2. +MODULE_LICENSE(GPL); So you probably want MODULE_LICENSE(GPL v2); here. Thanks. Fixed. Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Sun, Mar 08, 2015 at 01:44:02AM +0100, Rafał Miłecki wrote: On 7 March 2015 at 18:39, Rafał Miłecki zaj...@gmail.com wrote: It seems that brcmnand_ctlrdy_irq never fires on my device. Just like controller was never generating any IRQ. I started comparing your driver with OpenWrt's bcm_nand.c (which should be very similar to Broadcom's SDK NAND driver for ARM). Below are few things I've noticed. 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). Oh, wait, I was wrong there. So in bcm_nand.c IRQ handler fires very often, just not during the early phase (RESET, READID), when we don't use IRQs. During standard read/program/etc IRQ is commonly used. So maybe all we're missing in case of brcmstb_nand.c is enabling/acking/disabling IRQ? Yes, that's likely the main problem. It seem that my controller 6.01 has: 1) Following IRQs: DIREC_READ_MISS ERASE_COMPLETE COPYBACK_COMPLETE PROGRAM_COMPLETE CONTROLLER_RDY RDBSY_RDY ECC_UNCORRECTABLE ECC_CORRECTABLE Right, that's the standard set of NAND interrupts. This driver only uses CONTROLLER_RDY, BTW. 2) Registers for reading/acking above IRQs: 0xf00 DIREC_READ_MISS 0xf04 ERASE_COMPLETE 0xf08 COPYBACK_COMPLETE 0xf0c PROGRAM_COMPLETE 0xf10 CONTROLLER_RDY 0xf14 RDBSY_RDY 0xf18 ECC_UNCORRECTABLE 0xf1c ECC_CORRECTABLE (if 0x1 is set, it means IRQ was raised, writing 0x1 ack-es it) 3) Register 0x408 for enabling/disabling IRQs: 0x0004 DIREC_READ_MISS 0x0008 ERASE_COMPLETE 0x0010 COPYBACK_COMPLETE 0x0020 PROGRAM_COMPLETE 0x0040 CONTROLLER_RDY 0x0080 RDBSY_RDY 0x0100 ECC_UNCORRECTABLE 0x0200 ECC_CORRECTABLE (2) and (3) look pretty similar to the Cygnus chips. We should probably end up using the same solution for both. Several related to the Cygnus project are on CC. One problem for Cygnus is that some of the other bits in their register 0x408 deal with non-interrupt-related functionality (endianness and clocks), so it might not make the cleanest design to handle this with an irqchip driver. Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 3/9/2015 10:49 AM, Brian Norris wrote: On Sun, Mar 08, 2015 at 01:44:02AM +0100, Rafał Miłecki wrote: On 7 March 2015 at 18:39, Rafał Miłecki zaj...@gmail.com wrote: It seems that brcmnand_ctlrdy_irq never fires on my device. Just like controller was never generating any IRQ. I started comparing your driver with OpenWrt's bcm_nand.c (which should be very similar to Broadcom's SDK NAND driver for ARM). Below are few things I've noticed. 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). Oh, wait, I was wrong there. So in bcm_nand.c IRQ handler fires very often, just not during the early phase (RESET, READID), when we don't use IRQs. During standard read/program/etc IRQ is commonly used. So maybe all we're missing in case of brcmstb_nand.c is enabling/acking/disabling IRQ? Yes, that's likely the main problem. It seem that my controller 6.01 has: 1) Following IRQs: DIREC_READ_MISS ERASE_COMPLETE COPYBACK_COMPLETE PROGRAM_COMPLETE CONTROLLER_RDY RDBSY_RDY ECC_UNCORRECTABLE ECC_CORRECTABLE Right, that's the standard set of NAND interrupts. This driver only uses CONTROLLER_RDY, BTW. 2) Registers for reading/acking above IRQs: 0xf00 DIREC_READ_MISS 0xf04 ERASE_COMPLETE 0xf08 COPYBACK_COMPLETE 0xf0c PROGRAM_COMPLETE 0xf10 CONTROLLER_RDY 0xf14 RDBSY_RDY 0xf18 ECC_UNCORRECTABLE 0xf1c ECC_CORRECTABLE (if 0x1 is set, it means IRQ was raised, writing 0x1 ack-es it) 3) Register 0x408 for enabling/disabling IRQs: 0x0004 DIREC_READ_MISS 0x0008 ERASE_COMPLETE 0x0010 COPYBACK_COMPLETE 0x0020 PROGRAM_COMPLETE 0x0040 CONTROLLER_RDY 0x0080 RDBSY_RDY 0x0100 ECC_UNCORRECTABLE 0x0200 ECC_CORRECTABLE (2) and (3) look pretty similar to the Cygnus chips. We should probably end up using the same solution for both. Several related to the Cygnus project are on CC. One problem for Cygnus is that some of the other bits in their register 0x408 deal with non-interrupt-related functionality (endianness and clocks), so it might not make the cleanest design to handle this with an irqchip driver. It's not just similar, but it's identical to Cygnus. The register offset to read/ack the IRQ are the same. and Cygnus also uses a standard alone register to enable/disable IRQ (and their bit field is the same). Based on the link from Rafal, this chip also have the same issue with APB endianess control, clock control, interrupt control all stuffed in the same register: #define NANDC_IDM_AXI_BIG_ENDIANIDMREG_BIT_FIELD(0x408, 28, 1) #define NANDC_IDM_APB_LITTLE_ENDIAN IDMREG_BIT_FIELD(0x408, 24, 1) #define NANDC_IDM_TMIDMREG_BIT_FIELD(0x408, 16, 5) #define NANDC_IDM_IRQ_CORRECABLE_EN IDMREG_BIT_FIELD(0x408, 9, 1) #define NANDC_IDM_IRQ_UNCORRECABLE_EN IDMREG_BIT_FIELD(0x408, 8, 1) #define NANDC_IDM_IRQ_RDYBSY_RDY_EN IDMREG_BIT_FIELD(0x408, 7, 1) #define NANDC_IDM_IRQ_CONTROLLER_RDY_EN IDMREG_BIT_FIELD(0x408, 6, 1) #define NANDC_IDM_IRQ_PRPOGRAM_COMP_EN IDMREG_BIT_FIELD(0x408, 5, 1) #define NANDC_IDM_IRQ_COPYBK_COMP_ENIDMREG_BIT_FIELD(0x408, 4, 1) #define NANDC_IDM_IRQ_ERASE_COMP_EN IDMREG_BIT_FIELD(0x408, 3, 1) #define NANDC_IDM_IRQ_READ_MISS_EN IDMREG_BIT_FIELD(0x408, 2, 1) #define NANDC_IDM_IRQ_N_EN(n) IDMREG_BIT_FIELD(0x408, 2+(n), 1) #define NANDC_IDM_CLOCK_EN IDMREG_BIT_FIELD(0x408, 0, 1) Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On Sat, Mar 07, 2015 at 02:21:26PM +0100, Rafał Miłecki wrote: On 7 March 2015 at 02:18, Brian Norris computersforpe...@gmail.com wrote: + /* NAND register range */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ctrl-nand_base = devm_ioremap_resource(dev, res); + if (!ctrl-nand_base) + return -ENODEV; This is what I got during my first test: brcmstb_nand 11315e0.nand: can't request region for resource [mem 0x011315e0-0x011325df] Unable to handle kernel paging request at virtual address fff0 (not sure why yet). Hint: devm_ioremap_resource does never return NULL Thanks, fixed in 3 places. I expect you've figured out your problem here by now. Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 18:39, Rafał Miłecki wrote: > It seems that brcmnand_ctlrdy_irq never fires on my device. Just like > controller was never generating any IRQ. > > > I started comparing your driver with OpenWrt's bcm_nand.c (which > should be very similar to Broadcom's SDK NAND driver for ARM). Below > are few things I've noticed. > > 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). Oh, wait, I was wrong there. So in bcm_nand.c IRQ handler fires very often, just not during the early phase (RESET, READID), when we don't use IRQs. During standard read/program/etc IRQ is commonly used. So maybe all we're missing in case of brcmstb_nand.c is enabling/acking/disabling IRQ? It seem that my controller 6.01 has: 1) Following IRQs: DIREC_READ_MISS ERASE_COMPLETE COPYBACK_COMPLETE PROGRAM_COMPLETE CONTROLLER_RDY RDBSY_RDY ECC_UNCORRECTABLE ECC_CORRECTABLE 2) Registers for reading/acking above IRQs: 0xf00 DIREC_READ_MISS 0xf04 ERASE_COMPLETE 0xf08 COPYBACK_COMPLETE 0xf0c PROGRAM_COMPLETE 0xf10 CONTROLLER_RDY 0xf14 RDBSY_RDY 0xf18 ECC_UNCORRECTABLE 0xf1c ECC_CORRECTABLE (if 0x1 is set, it means IRQ was raised, writing 0x1 ack-es it) 3) Register 0x408 for enabling/disabling IRQs: 0x0004 DIREC_READ_MISS 0x0008 ERASE_COMPLETE 0x0010 COPYBACK_COMPLETE 0x0020 PROGRAM_COMPLETE 0x0040 CONTROLLER_RDY 0x0080 RDBSY_RDY 0x0100 ECC_UNCORRECTABLE 0x0200 ECC_CORRECTABLE -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 02:18, Brian Norris wrote:
> +static int brcmnand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
> +{
> + struct nand_chip *chip = mtd->priv;
> + struct brcmnand_host *host = chip->priv;
> + struct brcmnand_controller *ctrl = host->ctrl;
> + unsigned long timeo = msecs_to_jiffies(100);
> +
> + dev_dbg(ctrl->dev, "wait on native cmd %d\n", ctrl->cmd_pending);
> + if (ctrl->cmd_pending &&
> + wait_for_completion_timeout(>done, timeo) <= 0)
> {
> + unsigned long cmd = brcmnand_read_reg(ctrl,
> BRCMNAND_CMD_START)
> + >> brcmnand_cmd_shift(ctrl);
> +
> + dev_err_ratelimited(ctrl->dev,
> + "timeout waiting for command %u (%ld)\n",
> + host->last_cmd, cmd);
I think that using host->last_cmd in the above message is really
misleading. Please consider following:
[2.061139] [brcmnand_cmdfunc:1151] command:0x90 native_cmd:0x07
[2.067123] brcmstb_nand 18028000.nand: send native cmd 7 addr_lo 0x40
[2.168395] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7)
[2.175321] brcmstb_nand 18028000.nand: intfc status f000
[2.181051] nand: device found, Manufacturer ID: 0x92, Chip ID: 0xf1
[2.187370] nand: Eon NAND 128MiB 3,3V 8-bit
[2.191632] nand: 128 MiB, SLC, erase size: 128 KiB, page size:
2048, OOB size: 64
[2.199192] brcmstb_nand 18028000.nand: detected 128MiB total,
128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8
[2.199192]
[2.210724] Scanning device for bad blocks
[2.214812] brcmstb_nand 18028000.nand: send native cmd 1 addr_lo 0x0
[2.318394] brcmstb_nand 18028000.nand: timeout waiting for command 144 (1)
As you can see, first there was a brcmnand_cmdfunc call with 0x90
(NAND_CMD_READID). It was translated into internal command 0x07
(CMD_DEVICE_ID_READ).
Then there was a call to brcmnand_read_by_pio which resulted in
sending internal command 0x01 (CMD_PAGE_READ). This one overwrote
previous command. So the timeout wasn't about 144 (0x90) anymore.
I suggest replacing that with something like
"timeout waiting for internal command 0x%02x\n", cmd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 18:39, Rafał Miłecki wrote: > On 7 March 2015 at 02:18, Brian Norris wrote: >> This core originated in Set-Top Box chips (BCM7xxx) but is used in a >> variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and >> iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it >> to those architectures. >> >> There are multiple revisions of this core throughout the years, and >> almost every version broke register compatibility in some small way, but >> with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0, >> and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so >> there hopefully are no more lurking inconsistencies. > > Unfortunately, it seems I can't make this driver work with my BCM4708 > based router. This is a SoC with 6.01 controller. > > Sometimes I get this: > [0.478443] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) > [0.485385] brcmstb_nand 18028000.nand: intfc status f000 > [0.588393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) > [0.595326] brcmstb_nand 18028000.nand: intfc status f000 > [0.698392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) > [0.705322] brcmstb_nand 18028000.nand: intfc status f000 > [0.711051] nand: second ID read did not match 20,20 against 20,00 > [0.717197] nand: No NAND device found > > Other time: > [0.478441] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) > [0.485385] brcmstb_nand 18028000.nand: intfc status f000 > [0.588396] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) > [0.595327] brcmstb_nand 18028000.nand: intfc status f000 > [0.698393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) > [0.705321] brcmstb_nand 18028000.nand: intfc status f000 > [0.808394] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) > [0.815326] brcmstb_nand 18028000.nand: intfc status f000 > [0.918392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) > [0.925322] brcmstb_nand 18028000.nand: intfc status f000 > [0.931047] nand: No NAND device found > > (Btw. I guess you could use 0x%x instead of %d for printing commands). > > It seems that brcmnand_ctlrdy_irq never fires on my device. Just like > controller was never generating any IRQ. Oh, damn, I was doing sth stupid. I copied example from Documentation which has "brcm,nandcs" with reg = <1>;. That was wrong of course. after changing chip from 1 to 0 I got: [1.688413] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [1.695355] brcmstb_nand 18028000.nand: intfc status f000 [1.808384] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [1.815320] brcmstb_nand 18028000.nand: intfc status f000 [1.928385] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [1.935312] brcmstb_nand 18028000.nand: intfc status f000 [2.048460] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [2.055392] brcmstb_nand 18028000.nand: intfc status f000 [2.168386] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [2.175315] brcmstb_nand 18028000.nand: intfc status f000 [2.185640] nand: device found, Manufacturer ID: 0x92, Chip ID: 0xf1 [2.191967] nand: Eon NAND 128MiB 3,3V 8-bit [2.196221] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [2.203784] brcmstb_nand 18028000.nand: detected 128MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 So there is still problem with lacking IRQ (and timeouts), but at least communication with chip looks OK in general. Of course the lost of log is also spammed in these "timeout waiting" and driver is unusable. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 02:18, Brian Norris wrote: > This core originated in Set-Top Box chips (BCM7xxx) but is used in a > variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and > iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it > to those architectures. > > There are multiple revisions of this core throughout the years, and > almost every version broke register compatibility in some small way, but > with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0, > and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so > there hopefully are no more lurking inconsistencies. Unfortunately, it seems I can't make this driver work with my BCM4708 based router. This is a SoC with 6.01 controller. Sometimes I get this: [0.478443] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [0.485385] brcmstb_nand 18028000.nand: intfc status f000 [0.588393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.595326] brcmstb_nand 18028000.nand: intfc status f000 [0.698392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.705322] brcmstb_nand 18028000.nand: intfc status f000 [0.711051] nand: second ID read did not match 20,20 against 20,00 [0.717197] nand: No NAND device found Other time: [0.478441] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [0.485385] brcmstb_nand 18028000.nand: intfc status f000 [0.588396] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.595327] brcmstb_nand 18028000.nand: intfc status f000 [0.698393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.705321] brcmstb_nand 18028000.nand: intfc status f000 [0.808394] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.815326] brcmstb_nand 18028000.nand: intfc status f000 [0.918392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.925322] brcmstb_nand 18028000.nand: intfc status f000 [0.931047] nand: No NAND device found (Btw. I guess you could use 0x%x instead of %d for printing commands). It seems that brcmnand_ctlrdy_irq never fires on my device. Just like controller was never generating any IRQ. I started comparing your driver with OpenWrt's bcm_nand.c (which should be very similar to Broadcom's SDK NAND driver for ARM). Below are few things I've noticed. 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). 2) In brcmnand_waitfunc you seem to be always waiting for an interrupt. This is different from OpenWrt's driver and Broadcom's reference code. In bcm_nand.c in bcmnand_wait_cmd we first check BIT(31) of IRQ STATUS (0x6c / 0x14 / BRCMNAND_INTFC_STATUS). If it's set, we assume command was already completed. 3) In bcm_nand.c in IRQ waiting function bcmnand_wait_interrupt we ack/enable/disable NANDC_IRQ_CONTROLLER_RDY (nand_ctlrdy) IRQ. I don't see that in your code, not sure if it's important. 4) It seems bcm_nand.c also uses some extra register 0x500 and its bit 0x1 (we call it NANDC_IDM_IO_CTRL_RDY). It this bit it set, we don't ever care about NANDC_IRQ_CONTROLLER_RDY (nand_ctlrdy) IRQ. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 02:18, Brian Norris wrote: > + /* NAND register range */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ctrl->nand_base = devm_ioremap_resource(dev, res); > + if (!ctrl->nand_base) > + return -ENODEV; This is what I got during my first test: brcmstb_nand 11315e0.nand: can't request region for resource [mem 0x011315e0-0x011325df] Unable to handle kernel paging request at virtual address fff0 (not sure why yet). Hint: devm_ioremap_resource does never return NULL -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
Only a license nit.
Brian Norris schreef op vr 06-03-2015 om 17:18 [-0800]:
> --- /dev/null
> +++ b/drivers/mtd/nand/brcmstb_nand.c
> @@ -0,0 +1,2182 @@
> +/*
> + * Copyright © 2010-2015 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably want
MODULE_LICENSE("GPL v2");
here.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 02:18, Brian Norris computersforpe...@gmail.com wrote: This core originated in Set-Top Box chips (BCM7xxx) but is used in a variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it to those architectures. There are multiple revisions of this core throughout the years, and almost every version broke register compatibility in some small way, but with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0, and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so there hopefully are no more lurking inconsistencies. Unfortunately, it seems I can't make this driver work with my BCM4708 based router. This is a SoC with 6.01 controller. Sometimes I get this: [0.478443] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [0.485385] brcmstb_nand 18028000.nand: intfc status f000 [0.588393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.595326] brcmstb_nand 18028000.nand: intfc status f000 [0.698392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.705322] brcmstb_nand 18028000.nand: intfc status f000 [0.711051] nand: second ID read did not match 20,20 against 20,00 [0.717197] nand: No NAND device found Other time: [0.478441] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [0.485385] brcmstb_nand 18028000.nand: intfc status f000 [0.588396] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.595327] brcmstb_nand 18028000.nand: intfc status f000 [0.698393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.705321] brcmstb_nand 18028000.nand: intfc status f000 [0.808394] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.815326] brcmstb_nand 18028000.nand: intfc status f000 [0.918392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.925322] brcmstb_nand 18028000.nand: intfc status f000 [0.931047] nand: No NAND device found (Btw. I guess you could use 0x%x instead of %d for printing commands). It seems that brcmnand_ctlrdy_irq never fires on my device. Just like controller was never generating any IRQ. I started comparing your driver with OpenWrt's bcm_nand.c (which should be very similar to Broadcom's SDK NAND driver for ARM). Below are few things I've noticed. 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). 2) In brcmnand_waitfunc you seem to be always waiting for an interrupt. This is different from OpenWrt's driver and Broadcom's reference code. In bcm_nand.c in bcmnand_wait_cmd we first check BIT(31) of IRQ STATUS (0x6c / 0x14 / BRCMNAND_INTFC_STATUS). If it's set, we assume command was already completed. 3) In bcm_nand.c in IRQ waiting function bcmnand_wait_interrupt we ack/enable/disable NANDC_IRQ_CONTROLLER_RDY (nand_ctlrdy) IRQ. I don't see that in your code, not sure if it's important. 4) It seems bcm_nand.c also uses some extra register 0x500 and its bit 0x1 (we call it NANDC_IDM_IO_CTRL_RDY). It this bit it set, we don't ever care about NANDC_IRQ_CONTROLLER_RDY (nand_ctlrdy) IRQ. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 02:18, Brian Norris computersforpe...@gmail.com wrote: + /* NAND register range */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ctrl-nand_base = devm_ioremap_resource(dev, res); + if (!ctrl-nand_base) + return -ENODEV; This is what I got during my first test: brcmstb_nand 11315e0.nand: can't request region for resource [mem 0x011315e0-0x011325df] Unable to handle kernel paging request at virtual address fff0 (not sure why yet). Hint: devm_ioremap_resource does never return NULL -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
Only a license nit. Brian Norris schreef op vr 06-03-2015 om 17:18 [-0800]: --- /dev/null +++ b/drivers/mtd/nand/brcmstb_nand.c @@ -0,0 +1,2182 @@ +/* + * Copyright © 2010-2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ This states the license is GPL v2. +MODULE_LICENSE(GPL); So you probably want MODULE_LICENSE(GPL v2); here. Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 18:39, Rafał Miłecki zaj...@gmail.com wrote: On 7 March 2015 at 02:18, Brian Norris computersforpe...@gmail.com wrote: This core originated in Set-Top Box chips (BCM7xxx) but is used in a variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it to those architectures. There are multiple revisions of this core throughout the years, and almost every version broke register compatibility in some small way, but with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0, and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so there hopefully are no more lurking inconsistencies. Unfortunately, it seems I can't make this driver work with my BCM4708 based router. This is a SoC with 6.01 controller. Sometimes I get this: [0.478443] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [0.485385] brcmstb_nand 18028000.nand: intfc status f000 [0.588393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.595326] brcmstb_nand 18028000.nand: intfc status f000 [0.698392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.705322] brcmstb_nand 18028000.nand: intfc status f000 [0.711051] nand: second ID read did not match 20,20 against 20,00 [0.717197] nand: No NAND device found Other time: [0.478441] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [0.485385] brcmstb_nand 18028000.nand: intfc status f000 [0.588396] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.595327] brcmstb_nand 18028000.nand: intfc status f000 [0.698393] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.705321] brcmstb_nand 18028000.nand: intfc status f000 [0.808394] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.815326] brcmstb_nand 18028000.nand: intfc status f000 [0.918392] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [0.925322] brcmstb_nand 18028000.nand: intfc status f000 [0.931047] nand: No NAND device found (Btw. I guess you could use 0x%x instead of %d for printing commands). It seems that brcmnand_ctlrdy_irq never fires on my device. Just like controller was never generating any IRQ. Oh, damn, I was doing sth stupid. I copied example from Documentation which has brcm,nandcs with reg = 1;. That was wrong of course. after changing chip from 1 to 0 I got: [1.688413] brcmstb_nand 18028000.nand: timeout waiting for command 255 (9) [1.695355] brcmstb_nand 18028000.nand: intfc status f000 [1.808384] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [1.815320] brcmstb_nand 18028000.nand: intfc status f000 [1.928385] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [1.935312] brcmstb_nand 18028000.nand: intfc status f000 [2.048460] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [2.055392] brcmstb_nand 18028000.nand: intfc status f000 [2.168386] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7) [2.175315] brcmstb_nand 18028000.nand: intfc status f000 [2.185640] nand: device found, Manufacturer ID: 0x92, Chip ID: 0xf1 [2.191967] nand: Eon NAND 128MiB 3,3V 8-bit [2.196221] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 [2.203784] brcmstb_nand 18028000.nand: detected 128MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8 So there is still problem with lacking IRQ (and timeouts), but at least communication with chip looks OK in general. Of course the lost of log is also spammed in these timeout waiting and driver is unusable. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 02:18, Brian Norris computersforpe...@gmail.com wrote:
+static int brcmnand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
+{
+ struct nand_chip *chip = mtd-priv;
+ struct brcmnand_host *host = chip-priv;
+ struct brcmnand_controller *ctrl = host-ctrl;
+ unsigned long timeo = msecs_to_jiffies(100);
+
+ dev_dbg(ctrl-dev, wait on native cmd %d\n, ctrl-cmd_pending);
+ if (ctrl-cmd_pending
+ wait_for_completion_timeout(ctrl-done, timeo) = 0)
{
+ unsigned long cmd = brcmnand_read_reg(ctrl,
BRCMNAND_CMD_START)
+brcmnand_cmd_shift(ctrl);
+
+ dev_err_ratelimited(ctrl-dev,
+ timeout waiting for command %u (%ld)\n,
+ host-last_cmd, cmd);
I think that using host-last_cmd in the above message is really
misleading. Please consider following:
[2.061139] [brcmnand_cmdfunc:1151] command:0x90 native_cmd:0x07
[2.067123] brcmstb_nand 18028000.nand: send native cmd 7 addr_lo 0x40
[2.168395] brcmstb_nand 18028000.nand: timeout waiting for command 144 (7)
[2.175321] brcmstb_nand 18028000.nand: intfc status f000
[2.181051] nand: device found, Manufacturer ID: 0x92, Chip ID: 0xf1
[2.187370] nand: Eon NAND 128MiB 3,3V 8-bit
[2.191632] nand: 128 MiB, SLC, erase size: 128 KiB, page size:
2048, OOB size: 64
[2.199192] brcmstb_nand 18028000.nand: detected 128MiB total,
128KiB blocks, 2KiB pages, 16B OOB, 8-bit, BCH-8
[2.199192]
[2.210724] Scanning device for bad blocks
[2.214812] brcmstb_nand 18028000.nand: send native cmd 1 addr_lo 0x0
[2.318394] brcmstb_nand 18028000.nand: timeout waiting for command 144 (1)
As you can see, first there was a brcmnand_cmdfunc call with 0x90
(NAND_CMD_READID). It was translated into internal command 0x07
(CMD_DEVICE_ID_READ).
Then there was a call to brcmnand_read_by_pio which resulted in
sending internal command 0x01 (CMD_PAGE_READ). This one overwrote
previous command. So the timeout wasn't about 144 (0x90) anymore.
I suggest replacing that with something like
timeout waiting for internal command 0x%02x\n, cmd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
On 7 March 2015 at 18:39, Rafał Miłecki zaj...@gmail.com wrote: It seems that brcmnand_ctlrdy_irq never fires on my device. Just like controller was never generating any IRQ. I started comparing your driver with OpenWrt's bcm_nand.c (which should be very similar to Broadcom's SDK NAND driver for ARM). Below are few things I've noticed. 1) In bcm_nand.c IRQ handler also doesn't seem to be fired (or very rarely). Oh, wait, I was wrong there. So in bcm_nand.c IRQ handler fires very often, just not during the early phase (RESET, READID), when we don't use IRQs. During standard read/program/etc IRQ is commonly used. So maybe all we're missing in case of brcmstb_nand.c is enabling/acking/disabling IRQ? It seem that my controller 6.01 has: 1) Following IRQs: DIREC_READ_MISS ERASE_COMPLETE COPYBACK_COMPLETE PROGRAM_COMPLETE CONTROLLER_RDY RDBSY_RDY ECC_UNCORRECTABLE ECC_CORRECTABLE 2) Registers for reading/acking above IRQs: 0xf00 DIREC_READ_MISS 0xf04 ERASE_COMPLETE 0xf08 COPYBACK_COMPLETE 0xf0c PROGRAM_COMPLETE 0xf10 CONTROLLER_RDY 0xf14 RDBSY_RDY 0xf18 ECC_UNCORRECTABLE 0xf1c ECC_CORRECTABLE (if 0x1 is set, it means IRQ was raised, writing 0x1 ack-es it) 3) Register 0x408 for enabling/disabling IRQs: 0x0004 DIREC_READ_MISS 0x0008 ERASE_COMPLETE 0x0010 COPYBACK_COMPLETE 0x0020 PROGRAM_COMPLETE 0x0040 CONTROLLER_RDY 0x0080 RDBSY_RDY 0x0100 ECC_UNCORRECTABLE 0x0200 ECC_CORRECTABLE -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
This core originated in Set-Top Box chips (BCM7xxx) but is used in a
variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and
iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it
to those architectures.
There are multiple revisions of this core throughout the years, and
almost every version broke register compatibility in some small way, but
with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0,
and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so
there hopefully are no more lurking inconsistencies.
Signed-off-by: Brian Norris
---
drivers/mtd/nand/Kconfig|8 +
drivers/mtd/nand/Makefile |1 +
drivers/mtd/nand/brcmstb_nand.c | 2182 +++
3 files changed, 2191 insertions(+)
create mode 100644 drivers/mtd/nand/brcmstb_nand.c
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 5b76a173cd95..6445323a8cff 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -394,6 +394,14 @@ config MTD_NAND_GPMI_NAND
block, such as SD card. So pay attention to it when you enable
the GPMI.
+config MTD_NAND_BRCMSTB
+ tristate "Broadcom STB NAND controller"
+ depends on ARM || MIPS
+ help
+ Enables the Broadcom NAND controller driver. The controller was
+ originally designed for Set-Top Box but is used on various BCM7xxx,
+ BCM3xxx, BCM63xxx, iProc/Cygnus and more.
+
config MTD_NAND_BCM47XXNFLASH
tristate "Support for NAND flash on BCM4706 BCMA bus"
depends on BCMA_NFLASH
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 582bbd05aff7..3b1adddc83dd 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -52,5 +52,6 @@ obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o
obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/
obj-$(CONFIG_MTD_NAND_SUNXI) += sunxi_nand.o
obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
+obj-$(CONFIG_MTD_NAND_BRCMSTB) += brcmstb_nand.o
nand-objs := nand_base.o nand_bbt.o nand_timings.o
diff --git a/drivers/mtd/nand/brcmstb_nand.c b/drivers/mtd/nand/brcmstb_nand.c
new file mode 100644
index ..4d74f7a17dc3
--- /dev/null
+++ b/drivers/mtd/nand/brcmstb_nand.c
@@ -0,0 +1,2182 @@
+/*
+ * Copyright © 2010-2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * This flag controls if WP stays on between erase/write commands to mitigate
+ * flash corruption due to power glitches. Values:
+ * 0: NAND_WP is not used or not available
+ * 1: NAND_WP is set by default, cleared for erase/write operations
+ * 2: NAND_WP is always cleared
+ */
+static int wp_on = 1;
+module_param(wp_on, int, 0444);
+
+/***
+ * Definitions
+ ***/
+
+#define DRV_NAME "brcmstb_nand"
+
+#define CMD_NULL 0x00
+#define CMD_PAGE_READ 0x01
+#define CMD_SPARE_AREA_READ0x02
+#define CMD_STATUS_READ0x03
+#define CMD_PROGRAM_PAGE 0x04
+#define CMD_PROGRAM_SPARE_AREA 0x05
+#define CMD_COPY_BACK 0x06
+#define CMD_DEVICE_ID_READ 0x07
+#define CMD_BLOCK_ERASE0x08
+#define CMD_FLASH_RESET0x09
+#define CMD_BLOCKS_LOCK0x0a
+#define CMD_BLOCKS_LOCK_DOWN 0x0b
+#define CMD_BLOCKS_UNLOCK 0x0c
+#define CMD_READ_BLOCKS_LOCK_STATUS0x0d
+#define CMD_PARAMETER_READ 0x0e
+#define CMD_PARAMETER_CHANGE_COL 0x0f
+#define CMD_LOW_LEVEL_OP 0x10
+
+struct brcm_nand_dma_desc {
+ u32 next_desc;
+ u32 next_desc_ext;
+ u32 cmd_irq;
+ u32 dram_addr;
+ u32 dram_addr_ext;
+ u32 tfr_len;
+ u32 total_len;
+ u32 flash_addr;
+ u32 flash_addr_ext;
+ u32 cs;
+ u32 pad2[5];
+ u32 status_valid;
+} __packed;
+
+/* Bitfields for brcm_nand_dma_desc::status_valid */
+#define FLASH_DMA_ECC_ERROR(1 << 8)
+#define FLASH_DMA_CORR_ERROR (1 << 9)
+
+/* 512B flash cache in the NAND
[PATCH 3/3] mtd: nand: add NAND driver for Broadcom STB NAND controller
This core originated in Set-Top Box chips (BCM7xxx) but is used in a
variety of other Broadcom chips, including some BCM63xxx, BCM33xx, and
iProc/Cygnus. It's been used only on ARM and MIPS SoCs, so restrict it
to those architectures.
There are multiple revisions of this core throughout the years, and
almost every version broke register compatibility in some small way, but
with some effort, this driver is able to support v4.0, v5.0, v6.x, v7.0,
and v7.1. It's been tested on v5.0, v6.0, v7.0, and v7.1 recently, so
there hopefully are no more lurking inconsistencies.
Signed-off-by: Brian Norris computersforpe...@gmail.com
---
drivers/mtd/nand/Kconfig|8 +
drivers/mtd/nand/Makefile |1 +
drivers/mtd/nand/brcmstb_nand.c | 2182 +++
3 files changed, 2191 insertions(+)
create mode 100644 drivers/mtd/nand/brcmstb_nand.c
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 5b76a173cd95..6445323a8cff 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -394,6 +394,14 @@ config MTD_NAND_GPMI_NAND
block, such as SD card. So pay attention to it when you enable
the GPMI.
+config MTD_NAND_BRCMSTB
+ tristate Broadcom STB NAND controller
+ depends on ARM || MIPS
+ help
+ Enables the Broadcom NAND controller driver. The controller was
+ originally designed for Set-Top Box but is used on various BCM7xxx,
+ BCM3xxx, BCM63xxx, iProc/Cygnus and more.
+
config MTD_NAND_BCM47XXNFLASH
tristate Support for NAND flash on BCM4706 BCMA bus
depends on BCMA_NFLASH
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 582bbd05aff7..3b1adddc83dd 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -52,5 +52,6 @@ obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o
obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/
obj-$(CONFIG_MTD_NAND_SUNXI) += sunxi_nand.o
obj-$(CONFIG_MTD_NAND_HISI504) += hisi504_nand.o
+obj-$(CONFIG_MTD_NAND_BRCMSTB) += brcmstb_nand.o
nand-objs := nand_base.o nand_bbt.o nand_timings.o
diff --git a/drivers/mtd/nand/brcmstb_nand.c b/drivers/mtd/nand/brcmstb_nand.c
new file mode 100644
index ..4d74f7a17dc3
--- /dev/null
+++ b/drivers/mtd/nand/brcmstb_nand.c
@@ -0,0 +1,2182 @@
+/*
+ * Copyright © 2010-2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include linux/version.h
+#include linux/module.h
+#include linux/init.h
+#include linux/delay.h
+#include linux/device.h
+#include linux/platform_device.h
+#include linux/err.h
+#include linux/completion.h
+#include linux/interrupt.h
+#include linux/spinlock.h
+#include linux/dma-mapping.h
+#include linux/ioport.h
+#include linux/bug.h
+#include linux/kernel.h
+#include linux/bitops.h
+#include linux/mm.h
+#include linux/mtd/mtd.h
+#include linux/mtd/nand.h
+#include linux/mtd/partitions.h
+#include linux/of.h
+#include linux/of_mtd.h
+#include linux/of_platform.h
+#include linux/slab.h
+#include linux/list.h
+#include linux/log2.h
+
+/*
+ * This flag controls if WP stays on between erase/write commands to mitigate
+ * flash corruption due to power glitches. Values:
+ * 0: NAND_WP is not used or not available
+ * 1: NAND_WP is set by default, cleared for erase/write operations
+ * 2: NAND_WP is always cleared
+ */
+static int wp_on = 1;
+module_param(wp_on, int, 0444);
+
+/***
+ * Definitions
+ ***/
+
+#define DRV_NAME brcmstb_nand
+
+#define CMD_NULL 0x00
+#define CMD_PAGE_READ 0x01
+#define CMD_SPARE_AREA_READ0x02
+#define CMD_STATUS_READ0x03
+#define CMD_PROGRAM_PAGE 0x04
+#define CMD_PROGRAM_SPARE_AREA 0x05
+#define CMD_COPY_BACK 0x06
+#define CMD_DEVICE_ID_READ 0x07
+#define CMD_BLOCK_ERASE0x08
+#define CMD_FLASH_RESET0x09
+#define CMD_BLOCKS_LOCK0x0a
+#define CMD_BLOCKS_LOCK_DOWN 0x0b
+#define CMD_BLOCKS_UNLOCK 0x0c
+#define CMD_READ_BLOCKS_LOCK_STATUS0x0d
+#define CMD_PARAMETER_READ 0x0e
+#define CMD_PARAMETER_CHANGE_COL 0x0f
+#define CMD_LOW_LEVEL_OP 0x10
+
+struct brcm_nand_dma_desc {
+ u32 next_desc;
+ u32 next_desc_ext;
+ u32 cmd_irq;
+ u32
