mbox series

[v6,0/5] net: macb: cover letter

Message ID 1562769391-31803-1-git-send-email-pthombar@cadence.com
Headers show
Series net: macb: cover letter | expand

Message

Parshuram Raju Thombare July 10, 2019, 2:36 p.m. UTC
Hello !

This is 6th version of patch set containing following patches
for Cadence ethernet controller driver.

1. 0001-net-macb-add-phylink-support.patch
   Replace phylib API's with phylink API's.
2. 0002-net-macb-add-support-for-sgmii-MAC-PHY-interface.patch
   This patch add support for SGMII mode.
3. 0004-net-macb-add-support-for-c45-PHY.patch
   This patch is to support C45 PHY.
4. 0005-net-macb-add-support-for-high-speed-interface
   This patch add support for 10G USXGMII PCS in fixed mode.

Changes in v2:
1. Dropped patch configuring TI PHY DP83867 from
   Cadence PCI wrapper driver.
2. Removed code registering emulated PHY for fixed mode. 
3. Code reformatting as per Andrew's and Florian's suggestions.

Changes in v3:
Based on Russell's suggestions
1. Configure MAC in mac_config only for non in-band modes
2. Handle dynamic phy_mode changes in mac_config
3. Move MAC configurations to mac_config
4. Removed seemingly redundant check for phylink handle
5. Removed code from mac_an_restart and mac_link_state
   now just return -EOPNOTSUPP

Changes in v4:
1. Removed PHY_INTERFACE_MODE_2500BASEX, PHY_INTERFACE_MODE_1000BASEX and
   2.5G PHY_INTERFACE_MODE_SGMII phy modes from supported modes

Changes in v5:
1. Code refactoring

Changes in v6:
1. Allow phylink to validate particular phy_mode support by hardware.
2. Remove device tree parameter and 5G serdes rate for USXGMII

Regards,
Parshuram Thombare

Parshuram Thombare (4):
  net: macb: add phylink support
  net: macb: add support for sgmii MAC-PHY interface
  net: macb: add support for c45 PHY
  net: macb: add support for high speed interface

 drivers/net/ethernet/cadence/Kconfig     |   2 +-
 drivers/net/ethernet/cadence/macb.h      | 115 ++++-
 drivers/net/ethernet/cadence/macb_main.c | 543 ++++++++++++++++-------
 3 files changed, 483 insertions(+), 177 deletions(-)

Comments

David Miller July 10, 2019, 6:47 p.m. UTC | #1
As announced clearly yesterday on this list, the net-next tree is closed.

Please resubmit these changes when the tree opens back up again.

Thank you.
Parshuram Raju Thombare July 11, 2019, 5:20 a.m. UTC | #2
Hi David,

Ok, I will resubmit it.

Regards,
Parshuram Thombare
Andrew Lunn July 18, 2019, 3:13 p.m. UTC | #3
On Wed, Jul 10, 2019 at 03:36:31PM +0100, Parshuram Thombare wrote:
> Hello !
> 
> This is 6th version of patch set containing following patches
> for Cadence ethernet controller driver.

Hi Parshuram

One thing which was never clear is how you are testing the features
you are adding. Please could you describe your test setup and how each
new feature is tested using that hardware. I'm particularly interested
in what C45 device are you using? But i expect Russell would like to
know more about SFP modules you are using. Do you have any which
require 1000BaseX, 2500BaseX, or provide copper 1G?

Thanks
	Andrew
Parshuram Raju Thombare July 25, 2019, 1:27 p.m. UTC | #4
Hi Andrew,

>One thing which was never clear is how you are testing the features you are
>adding. Please could you describe your test setup and how each new feature
>is tested using that hardware. I'm particularly interested in what C45 device
>are you using? But i expect Russell would like to know more about SFP
>modules you are using. Do you have any which require 1000BaseX,
>2500BaseX, or provide copper 1G?

Sorry for late reply.
Here is a little more information on our setup used for testing C45 patch with a view to
try clarify a few points. 
Regarding the MDIO communication channel that our controller supports - We have tested
MDIO transfers through Clause 22, but none of our local PHY's support Clause 45 so our hardware
team have created an example Clause 45 slave device for us to add support to the driver.
Note our hardware has been in silicon for 20 years, with customers using their own software to support
MDIO (both clause 22 and clause 45 functionality) and so this has been in Cadence's hardware controller
many times. 
The programming interface is not hugely different between the two clauses and therefore we feel the risk is low.

For other features like SGMII, USXGMII we are using kc705 and vcu118 FPGA boards.
10G SFP+ module from Tyco electronics is used for testing 10G USXGMII in fixed AN mode.

Regards,
Parshuram Thombare
Russell King (Oracle) July 25, 2019, 1:36 p.m. UTC | #5
On Thu, Jul 25, 2019 at 01:27:58PM +0000, Parshuram Raju Thombare wrote:
> Hi Andrew,
> 
> >One thing which was never clear is how you are testing the features you are
> >adding. Please could you describe your test setup and how each new feature
> >is tested using that hardware. I'm particularly interested in what C45 device
> >are you using? But i expect Russell would like to know more about SFP
> >modules you are using. Do you have any which require 1000BaseX,
> >2500BaseX, or provide copper 1G?
> 
> Sorry for late reply.
> Here is a little more information on our setup used for testing C45 patch with a view to
> try clarify a few points. 
> Regarding the MDIO communication channel that our controller supports - We have tested
> MDIO transfers through Clause 22, but none of our local PHY's support Clause 45 so our hardware
> team have created an example Clause 45 slave device for us to add support to the driver.
> Note our hardware has been in silicon for 20 years, with customers using their own software to support
> MDIO (both clause 22 and clause 45 functionality) and so this has been in Cadence's hardware controller
> many times. 
> The programming interface is not hugely different between the two clauses and therefore we feel the risk is low.
> 
> For other features like SGMII, USXGMII we are using kc705 and vcu118 FPGA boards.
> 10G SFP+ module from Tyco electronics is used for testing 10G USXGMII in fixed AN mode.

SFP and SFP+ modules take SGMII, 1000BASE-X, possibly 2500BASE-X and
10GBASE-R all over a single serdes lane.

USXGMII might be used from the MAC to some sort of PHY which then
converts to 10GBASE-R.

If you have a PHY present, then using phylink and trying to link the
MAC directly with the SFP cage in software is the _wrong approach_.
I've stated this several times.

I'm getting to the point of asking you not to persist with your use
of phylink with your driver - I do not believe that your hardware has
any justification for its use, and I also believe that your use of
phylink is positively hurtful to the long term maintenance of phylink
itself.

In other words, you persisting to (ab)use phylink _hurts_ our ability
to maintain it into the future.

I'm also at the point where I'm giving up reviewing your patches - you
don't seem to take the issues I raise on board at all, so I feel like
I'm completely wasting my time trying to get you to make improvements.

Thanks.
Andrew Lunn July 25, 2019, 1:48 p.m. UTC | #6
On Thu, Jul 25, 2019 at 01:27:58PM +0000, Parshuram Raju Thombare wrote:
> Hi Andrew,
> 
> >One thing which was never clear is how you are testing the features you are
> >adding. Please could you describe your test setup and how each new feature
> >is tested using that hardware. I'm particularly interested in what C45 device
> >are you using? But i expect Russell would like to know more about SFP
> >modules you are using. Do you have any which require 1000BaseX,
> >2500BaseX, or provide copper 1G?
> 
> Sorry for late reply.
> Here is a little more information on our setup used for testing C45 patch with a view to
> try clarify a few points. 
> Regarding the MDIO communication channel that our controller supports - We have tested
> MDIO transfers through Clause 22, but none of our local PHY's support Clause 45 so our hardware
> team have created an example Clause 45 slave device for us to add support to the driver.

O.K.

Given Russells reply, i suggest you submit the MDIO Clause 45 patch,
and throw all the other patches away.

    Andrew