Re: [PATCH v2 4/4] ARM: OMAP: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack <zonque@gmail.com>]

On 05.11.2012 22:46, Jon Hunter wrote:
> 
> On 11/02/2012 03:23 PM, Daniel Mack wrote:
>> On 02.11.2012 20:57, Jon Hunter wrote:
>>> On 11/02/2012 02:23 PM, Daniel Mack wrote:
>>>> On 02.11.2012 20:18, Jon Hunter wrote:
>>>>> On 11/02/2012 06:14 AM, Daniel Mack wrote:
>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/bus/ti-gpmc.txt b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..6f44487
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/bus/ti-gpmc.txt
>>>>>>>> @@ -0,0 +1,73 @@
>>>>>>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>>>>>>> +
>>>>>>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +
>>>>>>>> + - compatible:		Should be set to "ti,gpmc"
>>>>>>>> + - reg:			A resource specifier for the register space
>>>>>>>> +			(see the example below)
>>>>>>>> + - ti,hwmods:		Should be set to "ti,gpmc" until the DT transition is
>>>>>>>> +			completed.
>>>>>>>> + - #address-cells:	Must be set to 2 to allow memory address translation
>>>>>>>> + - #size-cells:		Must be set to 1 to allow CS address passing
>>>>>>>> + - ranges:		Must be set up to reflect the memory layout
>>>>>>>> +			Note that this property is not currently parsed.
>>>>>>>> +			Calculated values derived from the contents of
>>>>>>>> +			GPMC_CS_CONFIG7 as set up by the bootloader. That will
>>>>>>>> +			change in the future, so be sure to fill the correct
>>>>>>>> +			values here.
>>>>>>>
>>>>>>> I still think it would be good to add number of chip-selects and
>>>>>>> wait-pins here.
>>>>>>
>>>>>> The number of chip-selects can be derived from the ranges property.
>>>>>> Namely, each 4-value entry to this property maps to one chip-select. I
>>>>>> can try and make the more clear in the documentation.
>>>>>
>>>>> Yes but that only tells you how many you are using. The binding should
>>>>> describe the hardware and so should tell us how many chip-selects we
>>>>> have. We should get away from using GPMC_CS_NUM in the code.
>>>>
>>>> Maybe I don't get your point, but we only need to care for as many cs
>>>> lines as we actually use, right?
>>>
>>> But how many does your device have? How many clients can you support?
>>
>> Well, you state that in the ranges property. Even if the chip could in
>> theory support 8 CS lines - if the actual setup only uses the first one
>> of them, the code would only need to allocate and set up the one that is
>> in use. And as the entries in "ranges" are mandatory, there can actually
>> be no mis-allocation.
> 
> Ah, I see your point now. Well typically, we have been putting the
> device-level peripheral info in the device's *.dtsi (ie. am33xx.dtsi)
> and then board specific stuff in the board *.dts file (am335x-bone.dts).
> So I would envision that the device-level info (reg, ti,hwmods,
> interrupt, num-cs) be in am33xx.dtsi and ranges be in am335x.dts. So it
> would still be nice to catch any badly configured ranges property in the
> driver by querying in the number of chip-selects.
> 
>> I can still add the maximum number as a separate property, but I wanted
>> to outline my idea here. Is "num-cs" a good name for the property?
> 
> Sounds good.
> 
>>> If we know how many the device has and then we can get rid of "#define
>>> GPMC_CS_NUM". We currently allocate the CS by calling gpmc_cs_request().
>>> Hmmm ... I now see that your patch is not calling this before
>>> configuring the CS and so that needs to be fixed too.
>>
>> It does implicitly, by calling gpmc_nand_init().
> 
> Yes, you are right!
> 
>>> Without knowing the total CS available, how do we ensure we have the CS
>>> available that someone is asking for?
>>>
>>>>> What about wait-pins?
>>>>
>>>> Afaik, their use depends on the driver acting as GPMC client, right?
>>>> Could you point me to code that acts conditionally and that should be
>>>> reflected in DT?
>>>
>>> Again we need to know how many the device has. Clients may or may not
>>> use these. However, if a client wants one they need to request one which
>>> is just like a chip-select. This is not in the current driver but Afzal
>>> has a patch for this [1].
>>
>> Ah, thanks for the pointer to the patch. Ok, I'll add a "num-waitpins"
>> property. Does that name sound appropriate?
> 
> Yes, that would be great!
> 
>>> Bottom line, for such hardware specific features, device tree is a good
>>> place to describe how many resources we have. Then we can eliminate such
>>> #defines from the driver code.
>>
>> Agreed.
>>
>>>> Quoting Documentation/devicetree/bindings/mtd/gpmc-nand.txt:
>>>>
>>>> 	For NAND specific properties such as ECC modes or bus width,
>>>> 	please refer to Documentation/devicetree/bindings/mtd/nand.txt
>>>
>>> Ok, thanks I see that now. Looking at other bindings, some also include
>>> these details but not all. Could be worth listing ecc-mode under
>>> mandatory and bus-width under optional with a reference to nand.txt
>>> binding. I don't think it is worth duplicating but listing the actual
>>> property names would be nice.
>>
>> Ok, I amended my local version. With the details above sorted out and
>> "num-cs" and "num-waitpins" in place, do you think we're ready for v4?
> 
> Yes, thanks for doing this.

I'll integrate the details mentioned by Philip Avinash on top of yours
and then send a v4, hopefully tomorrow!

Thanks for all the feddback, much appreciated!


Daniel