dp83867: Why does ti,fifo-depth set only TX, and why is it mandatory?
Subsystems (2)
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
Adrian
Quoted 8 lines · 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)
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
Quoted 3 lines
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
Quoted 1 line · Dan Murphy
Adrian
Hi Dan,
Quoted 10 lines
...
Quoted 3 lines
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?
Quoted 2 lines
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?
Quoted 6 lines
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
Adrian
Quoted 22 lines · Adrian Bunk
Quoted 1 line · Dan Murphy
Adrian
Hi Dan,
Quoted 9 lines
...
Quoted 3 lines
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.
Quoted 9 lines
Perhaps I am looking at the wrong datasheets or there's a hardware errata?
Quoted 2 lines
When SGMII support was added in commit
507ddd5c0d47ad869f361c71d700ffe7f12d1dd6That'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
Adrian
Quoted 29 lines · Dan Murphy
Adrian
Quoted 26 lines · Adrian Bunk
Quoted 1 line · Dan Murphy
Adrian
Hi Dan,
Quoted 9 lines
...
Quoted 3 lines
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?
Quoted 2 lines
When SGMII support was added in commit
507ddd5c0d47ad869f361c71d700ffe7f12d1dd6
Quoted 8 lines
Quoted 5 lines
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
Quoted 3 lines
Dan
Adrian
Quoted 27 lines · Dan Murphy
Adrian
Quoted 24 lines · Adrian Bunk
Quoted 1 line · Dan Murphy
Adrian
Hi Dan,
Quoted 9 lines
...
Quoted 3 lines
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
Quoted 1 line · Dan Murphy
Adrian
Hi Dan,
Quoted 11 lines
...
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.
Quoted 1 line
Dan
Thanks
Adrian