Subject: Re: [PATCH_V3 0/2] mtd: nand: jz4780: Add nand and bch driver

Hi,

On 31/03/15 18:20, Brian Norris wrote:
> On Tue, Mar 31, 2015 at 05:59:39PM +0100, Zubair Lutfullah Kakakhel wrote:
>> Hi,
>
> Hi!
>
> Nit: can you drop the underscore in your 'PATCH_Vx' subjects? It'd make
> my filtering a bit easier. I usually expect 'PATCH v3'. Thanks!

Sure. Will do that for v4 if we get there.

Responses below.

>
>> Two patches based on 4.0-rc6 that add NAND and BCH controller
>> drivers for the Ingenic JZ4780 SoC.
>>
>> Hope these can make it in time for 4.1.
>>
>> Tested on the MIPS Creator CI20.
>>
>> Core JZ4780 support is still in-flight.
>
> Sorry, I can't even compile test your patches, since I don't have the
> dependencies. So I can't accept your patches yet, and they most likely
> won't make it to 4.1. Or if you can point me to the right place, perhaps
> we can work something out between the tree(s) that will contain the
> dependencies.
>
> Note that I'm not worried about the missing MACH_JZ4780 sybmol and your
> core platform support, as much as the missing JZ4780_NEMC support.

I understand. Just trying to spread the load and sending everywhere
via separate subsystems.

NEMC is going via greg-kh. They went through greg's char-misc-testing and
just got applied here.

https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/log/?h=char-misc-next

Do you think that can work for compile testing? It is the upstream tree with nemc.

I could point our github ones for the ci20 board.
But it has quite a bit of other code too for jz4780 support.

>
>> Review and feedback welcome.
>>
>> V2 - > V3
>> Rebase to 4.0-rc6
>> Binding changes and fixes based on feedback by Brian Norris (Thank-you)
>
> I had a few questions on the driver that you didn't answer, I believe.
> It looks like maybe you silently answered them in this v3 code?
>

I apologize for the confusion. Rushing cause of 4.0-rc6

Yes. I followed all changes accordingly except one.

>> +static int jz4780_nand_init_chips(struct jz4780_nand *nand, struct device *dev)
>> +{
>> + struct jz4780_nand_chip *chip;
>> + const __be32 *prop;
>> + u64 addr, size;
>> + int i = 0;
>> +
>> + /*
>> + * Iterate over each bank assigned to this device and request resources.
>> + * The bank numbers may not be consecutive, but nand_scan_ident()
>> + * expects chip numbers to be, so fill out a consecutive array of chips
>> + * which map chip number to actual bank number.
>> + */
>> + while ((prop = of_get_address(dev->of_node, i, &size, NULL))) {
>> + chip = &nand->chips[i];
>> + chip->bank = of_read_number(prop, 1);
>> +
>> + jz47xx_nemc_set_type(nand->dev, chip->bank,
>> + JZ47XX_NEMC_BANK_NAND);
>> +
>> + addr = of_translate_address(dev->of_node, prop);
>> + if (addr == OF_BAD_ADDR)
>> + return -EINVAL;
>
> Is it not possible to use platform_get_resource() here? I'm not real
> sure about the platform and resource APIs vs. of_* APIs here, so I'm not
> sure.

I don't think so.

of_translate_address traverses the DT nodes tree to get the addresses.
platform_get_resource would just give the sub-address.

the DT node in the SoC file for nemc is

nemc: nemc@13410000 {
compatible = "ingenic,jz4780-nemc";
reg = <0x13410000 0x10000>;
#address-cells = <2>;
#size-cells = <1>;
ranges = <1 0 0x1b000000 0x1000000
2 0 0x1a000000 0x1000000
3 0 0x19000000 0x1000000
4 0 0x18000000 0x1000000
5 0 0x17000000 0x1000000
6 0 0x16000000 0x1000000>;

clocks = <&cgu JZ4780_CLK_NEMC>;
};

Then the DT node in the board file is

&nemc {
/*
* Only CLE/ALE are needed for the devices that are connected, rather
* than the full address line set.
*/
pinctrl-names = "default";
pinctrl-0 = <&pins_nemc_data &pins_nemc_cle_ale &pins_nemc_frd_fwe>;

nand: nand@1 {
compatible = "ingenic,jz4780-nand";
reg = <1 0 0x1000000>;

ingenic,nemc-tAS = <10>;
ingenic,nemc-tAH = <5>;
ingenic,nemc-tBP = <10>;
ingenic,nemc-tAW = <15>;
ingenic,nemc-tSTRV = <100>;

ingenic,bch-device = <&bch>;
nand-ecc-step-size = <1024>;
nand-ecc-strength = <24>;

rb-gpios = <&gpa 20 GPIO_ACTIVE_LOW>;
wp-gpios = <&gpf 22 GPIO_ACTIVE_LOW>;

pinctrl-names = "default";
pinctrl-0 = <&pins_nemc_cs1>;

#address-cells = <2>;
#size-cells = <2>;

partition@0 {
label = "u-boot-spl";
reg = <0x0 0x0 0x0 0x800000>;
};

partition@0x800000 {
label = "u-boot";
reg = <0x0 0x800000 0x0 0x200000>;
};

partition@0xa00000 {
label = "u-boot-env";
reg = <0x0 0xa00000 0x0 0x200000>;
};

partition@0xc00000 {
label = "system";
reg = <0x0 0xc00000 0x1 0xff400000>;
};
};

dm9000@6 {
compatible = "davicom,dm9000";
davicom,no-eeprom;

reg = <6 0x0 1 /* addr */
6 0x2 1>; /* data */

ingenic,nemc-tAS = <15>;
ingenic,nemc-tAH = <10>;
ingenic,nemc-tBP = <20>;
ingenic,nemc-tAW = <50>;
ingenic,nemc-tSTRV = <100>;

reset-gpios = <&gpf 12 GPIO_ACTIVE_HIGH>;
vcc-supply = <&eth0_power>;

interrupt-parent = <&gpe>;
interrupts = <19 0x4>;
};
};

Hope this clears the use of of_translate_address

Regards,
ZubairLK

>> V1 - > V2
>> Fixed module license macros
>> Rebase to 4.0-rc3
>>
>> Thanks,
>> ZubairLK
>>
>> Alex Smith (2):
>> dt-bindings: binding for jz4780-{nand,bch}
>> mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs
>>
>> .../bindings/mtd/ingenic,jz4780-nand.txt | 57 ++++
>> drivers/mtd/nand/Kconfig | 7 +
>> drivers/mtd/nand/Makefile | 1 +
>> drivers/mtd/nand/jz4780_bch.c | 353 +++++++++++++++++++
>> drivers/mtd/nand/jz4780_bch.h | 42 +++
>> drivers/mtd/nand/jz4780_nand.c | 376 +++++++++++++++++++++
>> 6 files changed, 836 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
>> create mode 100644 drivers/mtd/nand/jz4780_bch.c
>> create mode 100644 drivers/mtd/nand/jz4780_bch.h
>> create mode 100644 drivers/mtd/nand/jz4780_nand.c
>>
>
> Brian
>


2015-06-03 09:02:13

by Richard Weinberger

[permalink] [raw]
Subject: Re: [PATCH_V3 0/2] mtd: nand: jz4780: Add nand and bch driver

Zubair,

On Wed, Apr 1, 2015 at 11:30 AM, Zubair Lutfullah Kakakhel
<[email protected]> wrote:
> NEMC is going via greg-kh. They went through greg's char-misc-testing and
> just got applied here.
>
> https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/log/?h=char-misc-next
>
> Do you think that can work for compile testing? It is the upstream tree with nemc.
>
> I could point our github ones for the ci20 board.
> But it has quite a bit of other code too for jz4780 support.

what's the state of this driver? It would be nice to see it upstream.

--
Thanks,
//richard

2015-07-24 19:34:29

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH_V3 0/2] mtd: nand: jz4780: Add nand and bch driver

On Wed, Jun 03, 2015 at 11:02:05AM +0200, Richard Weinberger wrote:
> On Wed, Apr 1, 2015 at 11:30 AM, Zubair Lutfullah Kakakhel
> <[email protected]> wrote:
> > NEMC is going via greg-kh. They went through greg's char-misc-testing and
> > just got applied here.
> >
> > https://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/log/?h=char-misc-next
> >
> > Do you think that can work for compile testing? It is the upstream tree with nemc.
> >
> > I could point our github ones for the ci20 board.
> > But it has quite a bit of other code too for jz4780 support.
>
> what's the state of this driver? It would be nice to see it upstream.

Status: doesn't build against mainline. Looks like a lot of 'jz47xx...'
names changed to 'jz4780...'. Please resubmit.

Thanks,
Brian