dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Adrian Bunk]

Hi,

looking at the ti,fifo-depth property to set the TX FIFO Depth in the 
dp83867 driver I was wondering:

1. Why does it set only TX?
Is there a reason why TX needs setting but RX does not?
(RX FIFO Depth is SGMII-only, but that's what I am using)

2. Why is it a mandatory property?
Perhaps I am missing something obvious, but why can't the driver either
leave the value untouched or set the maximum when nothing is configured?

Thanks in advance
Adrian

Re: dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Dan Murphy]

Adrian

On 11/14/19 10:24 AM, Adrian Bunk wrote:
> Hi,
>
> looking at the ti,fifo-depth property to set the TX FIFO Depth in the
> dp83867 driver I was wondering:
>
> 1. Why does it set only TX?
> Is there a reason why TX needs setting but RX does not?
> (RX FIFO Depth is SGMII-only, but that's what I am using)

There was no RX fifo depth setting for this device only TX fifo depth 
setting at the original submission.

See 8.6.14 PHY Control Register (PHYCR) only defines tx

> 2. Why is it a mandatory property?
> Perhaps I am missing something obvious, but why can't the driver either
> leave the value untouched or set the maximum when nothing is configured?

When the driver was originally written it was written only for RGMII 
interfaces as that is the MII that the data sheet references and does 
not reference SGMII.  We did not have SGMII samples available at that 
time. According to the HW guys setting the FIFO depth is required for 
RGMII interfaces.  When SGMII support was added in commit 
507ddd5c0d47ad869f361c71d700ffe7f12d1dd6 the rx fifo-depth DT property 
should have been added and both tx and rx should have been made 
optional.  We should probably deprecate the ti,fifo-depth in favor of 
the standard rx-fifo-depth and tx-fifo-depth common properties.

Dan

Re: dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Adrian Bunk]

On Thu, Nov 14, 2019 at 11:53:36AM -0600, Dan Murphy wrote:
> Adrian

Hi Dan,

>...
> > 2. Why is it a mandatory property?
> > Perhaps I am missing something obvious, but why can't the driver either
> > leave the value untouched or set the maximum when nothing is configured?
> 
> When the driver was originally written it was written only for RGMII
> interfaces as that is the MII that the data sheet references and does not
> reference SGMII.  We did not have SGMII samples available at that time.
> According to the HW guys setting the FIFO depth is required for RGMII
> interfaces.

My reading of the datasheets is that it isn't needed at all for RGMII,
only for SGMII and gigabit GMII.

Which makes it weird that it is only written in the RGMII case where it
is documented to be disabled.

And there is a documented default value so writing shouldn't be mandatory
in any case.

Perhaps I am looking at the wrong datasheets or there's a hardware errata?

> When SGMII support was added in commit
> 507ddd5c0d47ad869f361c71d700ffe7f12d1dd6

That's adding 6-wire mode support, the version of the driver I use with 
SGMII in 4.14 is much older and not far from the original submission.

Is there anything that might be missing for SGMII you are aware of?

> the rx fifo-depth DT property
> should have been added and both tx and rx should have been made optional. 
> We should probably deprecate the ti,fifo-depth in favor of the standard
> rx-fifo-depth and tx-fifo-depth common properties.
> 
> Dan

Thanks
Adrian

Re: dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Dan Murphy]

Adrian

On 11/14/19 1:47 PM, Adrian Bunk wrote:
> On Thu, Nov 14, 2019 at 11:53:36AM -0600, Dan Murphy wrote:
>> Adrian
> Hi Dan,
>
>> ...
>>> 2. Why is it a mandatory property?
>>> Perhaps I am missing something obvious, but why can't the driver either
>>> leave the value untouched or set the maximum when nothing is configured?
>> When the driver was originally written it was written only for RGMII
>> interfaces as that is the MII that the data sheet references and does not
>> reference SGMII.  We did not have SGMII samples available at that time.
>> According to the HW guys setting the FIFO depth is required for RGMII
>> interfaces.
> My reading of the datasheets is that it isn't needed at all for RGMII,
> only for SGMII and gigabit GMII.
>
> Which makes it weird that it is only written in the RGMII case where it
> is documented to be disabled.
>
> And there is a documented default value so writing shouldn't be mandatory
> in any case.
>
> Perhaps I am looking at the wrong datasheets or there's a hardware errata?
>
>> When SGMII support was added in commit
>> 507ddd5c0d47ad869f361c71d700ffe7f12d1dd6
> That's adding 6-wire mode support, the version of the driver I use with
> SGMII in 4.14 is much older and not far from the original submission.
>
> Is there anything that might be missing for SGMII you are aware of?
>
I forwarded this to our PHY support guy as I did not work on the SGMII 
device they shipped.

I only had a RGMII certified device.

Dan

Re: dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Dan Murphy]

Adrian

On 11/14/19 2:01 PM, Dan Murphy wrote:
> Adrian
>
> On 11/14/19 1:47 PM, Adrian Bunk wrote:
>> On Thu, Nov 14, 2019 at 11:53:36AM -0600, Dan Murphy wrote:
>>> Adrian
>> Hi Dan,
>>
>>> ...
>>>> 2. Why is it a mandatory property?
>>>> Perhaps I am missing something obvious, but why can't the driver 
>>>> either
>>>> leave the value untouched or set the maximum when nothing is 
>>>> configured?
>>> When the driver was originally written it was written only for RGMII
>>> interfaces as that is the MII that the data sheet references and 
>>> does not
>>> reference SGMII.  We did not have SGMII samples available at that time.
>>> According to the HW guys setting the FIFO depth is required for RGMII
>>> interfaces.
>> My reading of the datasheets is that it isn't needed at all for RGMII,
>> only for SGMII and gigabit GMII.
>>
>> Which makes it weird that it is only written in the RGMII case where it
>> is documented to be disabled.
>>
>> And there is a documented default value so writing shouldn't be 
>> mandatory
>> in any case.
>>
>> Perhaps I am looking at the wrong datasheets or there's a hardware 
>> errata?
>>
>>> When SGMII support was added in commit
>>> 507ddd5c0d47ad869f361c71d700ffe7f12d1dd6
>> That's adding 6-wire mode support, the version of the driver I use with
>> SGMII in 4.14 is much older and not far from the original submission.
>>
>> Is there anything that might be missing for SGMII you are aware of?
>>
> I forwarded this to our PHY support guy as I did not work on the SGMII 
> device they shipped.
>
> I only had a RGMII certified device.

I only got half the info but the FIFO depth is not required for RGMII.  
They are still trying to determine if it is for SGMII

Waiting on a response from someone OoO

Dan


>
> Dan
>

Re: dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Dan Murphy]

Adrian

On 11/14/19 2:01 PM, Dan Murphy wrote:
> Adrian
>
> On 11/14/19 1:47 PM, Adrian Bunk wrote:
>> On Thu, Nov 14, 2019 at 11:53:36AM -0600, Dan Murphy wrote:
>>> Adrian
>> Hi Dan,
>>
>>> ...
>>>> 2. Why is it a mandatory property?
>>>> Perhaps I am missing something obvious, but why can't the driver 
>>>> either
>>>> leave the value untouched or set the maximum when nothing is 
>>>> configured?
>>> When the driver was originally written it was written only for RGMII
>>> interfaces as that is the MII that the data sheet references and 
>>> does not
>>> reference SGMII.  We did not have SGMII samples available at that time.
>>> According to the HW guys setting the FIFO depth is required for RGMII
>>> interfaces.
>> My reading of the datasheets is that it isn't needed at all for RGMII,
>> only for SGMII and gigabit GMII.
>>
>> Which makes it weird that it is only written in the RGMII case where it
>> is documented to be disabled.
>>
>> And there is a documented default value so writing shouldn't be 
>> mandatory
>> in any case.
>>
>> Perhaps I am looking at the wrong datasheets or there's a hardware 
>> errata?
>>
OK the HW team said that FIFO depth is no longer a mandatory field to be 
written for either RGMII or SGMII.

So my suggestion here is that we deprecate, but support in the driver, 
the ti-fifo-depth, and add the already documented

rx-fifo-depth and tx-fifo-depth as optional DT entries.

So I can change the driver and DT docs and test the RGMII device as 
above as long as we are in agreement

Dan

Re: dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?

[Adrian Bunk]

On Thu, Nov 21, 2019 at 02:24:01PM -0600, Dan Murphy wrote:
> Adrian

Hi Dan,

>...
> OK the HW team said that FIFO depth is no longer a mandatory field to be
> written for either RGMII or SGMII.
> 
> So my suggestion here is that we deprecate, but support in the driver, the
> ti-fifo-depth, and add the already documented
> 
> rx-fifo-depth and tx-fifo-depth as optional DT entries.
> 
> So I can change the driver and DT docs and test the RGMII device as above as
> long as we are in agreement

sounds good to me.

> Dan

Thanks
Adrian