Re: [PATCH 3/6] dt/bindings: Add bindings for Tegra GMI controller

[Jon Hunter <jonathanh@nvidia.com>]

On 09/08/16 21:48, Mirza Krak wrote:
> 2016-08-09 15:34 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>
>> On 09/08/16 09:40, Mirza Krak wrote:
>>> 2016-08-08 16:44 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>:
>>>>
>>>> On 06/08/16 20:40, Mirza Krak wrote:
>>>>> From: Mirza Krak <mirza.krak@gmail.com>
>>>>>
> 
> <--snip-->
> 
>>>>> + - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
>>>>> +   Valid values are 0-15, default is 1
>>>>> + - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
>>>>> +   Valid values are 0-255, default is 1
>>>>> + - nvidia,snor-wait-width: Number of cycles before READY is asserted.
>>>>> +   Valid values are 0-255, default is 3
>>>>> +
>>>>> +Example with two SJA1000 CAN controllers connected to the GMI bus:
>>>>> +
>>>>> +  gmi@70090000 {
>>>>> +    #address-cells = <1>;
>>>>> +    #size-cells = <1>;
>>>>
>>>> I think 0 for size makes sense. I know that caused you problems before,
>>>> but I am wondering if ...
>>>>
>>>>> +    ranges;
>>>>
>>>> ... ranges is needed here? If we do have it, I am wondering if it should
>>>> be a single entry for the chip-select that is being used. For now we
>>>> could only support a ranges with one entry.
>>>>
>>>>         #address-cells = <1>;
>>>>         #size-cells = <1>;
>>>>         ranges = <4 0x48000000 0x00040000>;
>>>
>>> I prefer if we have "ranges" with one single entry, and warn if user enters
>>> multiple for now, like we discussed earlier. Should have really done it in
>>> this series.
>>
>> I think I do as well.
>>
>>>>
>>>>> +    nvidia,snor-mux-mode;
>>>>> +    nvidia,snor-adv-inv;
>>>>> +    nvidia,snor-cs-select = <4>;
>>>>
>>>> I would have expected these under bus@X node as they are specific to the
>>>> GMI CS.
>>>
>>> Yes, that is true.
>>>
>>>>
>>>> I would also expect that the actual chip-select number is encoded in the
>>>> reg property.
>>>>
>>>>> +
>>>>> +    bus@0,0 {
>>>>
>>>> bus@4
>>>
>>> ACK.
>>>
>>>>
>>>> No mention of this bus node in the above documentation.
>>>
>>> I was hesitant documenting it since I am not sure if we really need it
>>> in a generic case? It does make sense in my
>>> specific case. But what would it look like if we could maintain CS in software.
>>>
>>> Do you then have a bus node per CS? I am guessing not. Is it enough to
>>> document it in my "example brief"?
>>
>> I see what you mean. May be it is fine to document with the examples
>> below how child devices should be populated. You should also state that
>> currently the GMI only supports one child device currently for the
>> chip-select that is being used.
> 
> Should I really need to state that? Is it not always the case, that
> you have one child device per chip-select?

I think so. Remember it is one child device for the GMI, however, that
child device could still have one than one child device (per you example
with two CANs). The GMI child represents the active chip-select and we
only currently support one with this driver.

Jon

-- 
nvpublic