Re: [RFC] ARM: BCM5301X: add NAND flash chip description

[Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>]

On 05/28/2015 10:21 AM, Arnd Bergmann wrote:
> On Wednesday 27 May 2015 23:46:48 Hauke Mehrtens wrote:
>> On 05/27/2015 09:37 AM, Arnd Bergmann wrote:
>>> On Sunday 24 May 2015 20:32:29 Hauke Mehrtens wrote:
>>>> @@ -124,17 +124,7 @@
>>>>                         <0x00026000 0 &gic GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>,
>>>>  
>>>>                         /* Ethernet Controller 3 */
>>>> -                       <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>,
>>>> -
>>>> -                       /* NAND Controller */
>>>> -                       <0x00028000 0 &gic GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 1 &gic GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 2 &gic GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 3 &gic GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 4 &gic GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 5 &gic GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 6 &gic GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>,
>>>> -                       <0x00028000 7 &gic GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
>>>> +                       <0x00027000 0 &gic GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>;
>>>>  
>>>>                 chipcommon: chipcommon@0 {
>>>>                         reg = <0x00000000 0x1000>;
>>>> @@ -143,4 +133,30 @@
>>>>                         #gpio-cells = <2>;
>>>>                 };
>>>>         };
>>>> +
>>>> +       nand: nand@18028000 {
>>>> +               compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1", "brcm,brcmnand";
>>>> +               reg = <0x18028000 0x600>, <0x1811a408 0x600>, <0x18028f00 0x20>;
>>>> +               reg-names = "nand", "iproc-idm", "iproc-ext";
>>>> +               interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
>>>
>>> I think I'd rather leave the interrupt-map in the brcm node, and use
>>>
>>> 	interrupts = <0>;
>>>
>>> here.
>>
>> There are two conflicting interests ;-)
>>
>> In the first approach we probed the NAND controller through bcma and
>> just added the additional information we can not probe through device
>> tree. That was this version:
>> https://patchwork.ozlabs.org/patch/473181/
>>
>> This version needs some changes to the brcmnand driver and this
>> additional part:
>> https://patchwork.ozlabs.org/patch/473182/
>>
>> This code is simular to the iproc_nand.c
>>
>> Brian wanted us to use the iproc_nand.c which is possible, but then we
>> completely ignore what we have probed in bcma and only use device tree.
>> And this nand driver is not a subnode of the bcma bus any more so we can
>> not use the irqs defined there.
> 
> Ok, I see. Maybe we should however leave the interrupt map entries
> in the bcma node, to leave the option of doing it either way?

Ok, I will do that as there are already there.

> 
>>>
>>>> +               status = "disabled";
>>>> +
>>>> +               #address-cells = <1>;
>>>> +               #size-cells = <0>;
>>>> +
>>>> +               brcm,nand-has-wp;
>>>> +
>>>> +               nandcs@0 {
>>>> +                       compatible = "brcm,nandcs";
>>>> +                       reg = <0>;
>>>> +                       #address-cells = <1>;
>>>> +                       #size-cells = <1>;
>>>> +
>>>> +                       nand-ecc-strength = <8>;
>>>> +                       nand-ecc-step-size = <512>;
>>>> +
>>>> +                       linux,part-probe = "ofpart", "bcm47xxpart";
>>>> +               };
>>>> +       };
>>>
>>> This seems fine in principle, once the exact binding has been nailed down.
>>>
>>> "bcm47xxpart" does not seem like an appropriate string here.
>>
>> We are already thinking and talking about how to solve this problem.
>> There are some parsers that are able to parse some partition layout
>> based on information various sources or some vendor headers and some are
>> also guessing in addition, like they are assuming that the bootloader is
>> in the first block and n bytes long. The current module is that the
>> flash driver defines which partition parsers to take, it is hard coded
>> into the code. I do not like that. ;-)
>>
>> I am trying to get a simple patch in which makes it possible to
>> overwrite the defaults from the flash driver with device tree.
>> As it uses the same format it should be easy to convert devices from the
>> old style to the new one. This deice tree attribute is already supported
>> by one driver I moved the code to make it available for all drivers.
>>
>> The patch I am talking about: https://patchwork.ozlabs.org/patch/475988/
> 
> I was mainly referring to the use of 'xx' wildcards in the bcm47xxpart
> string. We try to avoid wildcards in compatible strings, and I think it
> makes sense to follow the same model here.

Ah, ok currently this is referring to the old names of the partition
parsers and this code was added before we know that there will be arm
SoCs using this. As it is not clear how this will be represented in
device tree I would remove it for now form our device tree.

Hauke
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html