Re: [net-next PATCH 1/3] net: phy: dp83867: Add documentation for optional impedance control

[Mugunthan V N]

Rob,

On Friday 22 July 2016 02:44 AM, Rob Herring wrote:
> On Thu, Jul 21, 2016 at 11:52:36AM +0530, Sekhar Nori wrote:
>> Nishanth,
>>
>> On Wednesday 20 July 2016 09:03 PM, Nishanth Menon wrote:
>>> On 07/20/2016 09:56 AM, Mugunthan V N wrote:
>>>> Add documention of ti,impedance-control which can be used to
>>>> correct MAC impedance mismatch using phy extended registers.
>>>>
>>>> Signed-off-by: Mugunthan V N <mugunthanvnm@xxxxxx>
>>>> ---
>>>>   Documentation/devicetree/bindings/net/ti,dp83867.txt | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83867.txt
>>>> b/Documentation/devicetree/bindings/net/ti,dp83867.txt
>>>> index 5d21141..531ea3c5 100644
>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83867.txt
>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83867.txt
>>>> @@ -9,6 +9,13 @@ Required properties:
>>>>       - ti,fifo-depth - Transmitt FIFO depth- see
>>>> dt-bindings/net/ti-dp83867.h
>>>>           for applicable values
>>>>
>>>> +Optional property:
>>>> +    - ti,impedance-control - MAC Interface Impedance control to vary the
>>>> +                 output impedance with an approximate range
>>>> +                 from 35-70 ohms in 32 steps. Value range can
>>>> +                 be 0x0 to 0x1f. Lowest impedance value is
>>>> +                 0x1f and highest impedance being 0x0.
>>>> +
>>>
>>> Should'nt you be using the impedance values of 35 to 70 as the valid
>>> values here? that would be the hardware description, and the values to
>>> program corresponding to those are 0x00 to 0x1f. Right?
>>>
>>> Rob: is'nt that the right way to do it?
> 
> Normally, yes that is preferred. I don't have an issue with it here if 
> that is what makes sense.
> 
>> Agree that that is usually the right way to do it. I believe this case
>> is bit peculiar though. Here is the extract from the datasheet[1] about
>> how the register in question works.
>>
>> "
>> Output impedance approximate range from 35-70 ohms in 32 steps.
>> Lowest being 11110 and highest being 00000. Range and Step size
>> will vary with process.
>>
>> Default is set to 50 ohms by trim. But the default register value can
>> vary by process. Non default values of MAC I/O impedance can be
>> used based on trace impedance. Mismatch between device and
>> trace impedance can cause voltage overshoot and undershoot.
>> "
>>
>> So clearly, there is no easy correspondence that the hardware guarantees
>> between output impedance ohmage and the register value programmed. Only
>> couple of things are guaranteed.
>>
>> 1) Programming a value of 0 gives approximately 35 ohms
>> 2) Programming a value of 0x1F gives approximately 70 ohms
>> 3) Default value of the register gives 50 ohms (the default value could
>> vary by device).
>> 4) Programming a value in between will give some ohmage in the
>> approximate range 35-70 (curve is unknown).

Sekhar got it wrong, programming 0x1F provides the minimum impedance of
35 ohms and 0x0 provides the maximum impedance of 70 ohms.

>>
>> Given this, I am not sure how one can convert a given user supplied ohms
>> values to a reasonable register value. Clearly, the register is supposed
>> to be programmed by experimentation, not calculation.
> 
> So your are going to change from a known value tuned for the individual 
> Si (50ohm) to the same value for all chips (at least for a given board). 
> Or does this get set by firmware at boot? If not, seems like you would 
> want to specify a delta from the default register value in this case.

The delta will not be constant for each steps of the register content.
So this approach cannot be taken.

> 
>> That said, we could take another approach. At least for the current
>> issue we are trying to address, we only need to configure the register
>> to max value. And given the nature of the register, I am pretty sure
>> that is what most people will end up doing.
> 
> How does the max value get determined? Possibly it should just be set in 
> the kernel.

Yes, the min/max value will be determined in the kernel and the DT will
have either of the properties below.

>  
>> So, may be we have two properties:
>>
>> ti,max-output-impedance
>> ti,min-output-imepdance
> 
> Just define max if that's all you currently need.

Currently we care about only the min-output-impedance (register content
0x1f). Will respin the patch using ti,min-output-impedance.

Regards
Mugunthan V N