Re: [PATCH v3 0/2] Common SerDes driver for TI's Keystone Platforms

[Murali Karicheri]

+ Alexandre Torgue <alexandre.torgue@xxxxxx> (Owner of phy-miphy28lp.c)
+ Loc Ho <lho@xxxxxxx> (Owner of phy-miphy28lp.c)

On 10/22/2015 05:56 PM, Murali Karicheri wrote:
> On 10/22/2015 01:48 PM, Russell King - ARM Linux wrote:
> > On Thu, Oct 22, 2015 at 11:05:26AM -0400, Murali Karicheri wrote:
> > > On 10/21/2015 08:56 AM, WingMan Kwok wrote:
> > > > On TI's Keystone platforms, several peripherals such as the
> > > > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > > > require the use of a SerDes for converting SoC parallel data into
> > > > serialized data that can be output over a high-speed electrical
> > > > interface, and also converting high-speed serial input data
> > > > into parallel data that can be processed by the SoC.  The
> > > > SerDeses used by those peripherals, though they may be different,
> > > > are largely similar in functionality and setup.
> ------------Cut-------------------------------------------------
> > > >   Documentation/devicetree/bindings/phy/ti-phy.txt |  239 +++
> > > >   drivers/pci/host/pci-keystone.c                  |   24 +-
> > > >   drivers/pci/host/pci-keystone.h                  |    1 +
> > > >   drivers/phy/Kconfig                              |    8 +
> > > >   drivers/phy/Makefile                             |    1 +
> > > >   drivers/phy/phy-keystone-serdes.c                | 2366
> > > > ++++++++++++++++++++++
> > > >   6 files changed, 2629 insertions(+), 10 deletions(-)
> > > >   create mode 100644 drivers/phy/phy-keystone-serdes.c
> > > Kishon, Bjorn
> > >
> > > Who will pick this up? Do we have time to get this in 4.4?
> >
> > I've been avoiding this since my initial comments, but if you're wanting
> > to get it into v4.4, then I have to say something.
> Russell,
>
> I saw you have raised this point earlier against v1 of the patch series.
> I have responded as below  (cut-n-pasted from that email)
>
> "The serdes on K2 are re-used on multiple hardware blocks as already
> indicated in this thread. It has got multiple lanes, each lane can be
> enabled/disabled, shutdown etc. Isn't generic phy framework added to
> support this type of hardware block? I see some enhancements needed for
> K2 serdes to support monitoring the serdes link and providing a status
> to the higher layer device. So I am not clear what different way you
> would like to handle serdes drivers? Why do you need a new framework?
> "
>
> KISHON VIJAY had responded saying
>
> "The PHY framework (in drivers/phy/) already provides a standard
> interface to be used by the controller drivers no?"
>
> But I have not seen your response to these questions from us. v2 and v3
> has gone by and since all of the outstanding comments have been
> addressed and you have not responded to our questions, I thought this
> can be merged for 4.4. Good to see you have responded now :)
> > Again, there's other SoCs out there which have serdes.  Adding 2.5k of
> > lines for vendor serdes implementations does not scale - this needs to
> > be re-thought in a way which reduces the code maintanence burden.
> >
> > Other SoCs like Marvell Armada have serdes links which can be configured
> > between SATA, PCIe and Gbe.  Should Armada end up adding another 2.5k
> > lines to support their device too?  What happens when we have 10 of
> > these, and we have 25k lines of code here?
> >
> > Again, this does not scale.  Please look at what can be done to reduce
> > the code size when other implementations come along.
>
> Well, per our understanding, this driver is a Generic phy driver and we
> have implemented a device driver based on Generic Phy API. This driver
> is expected to support all of the 3 peripherals :- PCIe, 1G and 10G
> Ethernet.  You have mentioned about Marvell & Armada . Did Marvell post
> any patch already? Without seeing their code, how will we be able to
> investigate what can be factored out to a generic serdes core driver? By
> making this statement, I assume you are still considering using the
> Generic Phy driver framework for SerDes drivers. Don't you?
>
> I did a search in the phy folder and these are the top ones that came
> out in terms of number of lines of code after Phy-core.c.
>
> ls *.[ch] | xargs wc -l | sort -n
>
>     943 phy-core.c
>    1279 phy-miphy28lp.c
>    1735 phy-xgene.c
>    2367 phy-keystone-serdes.c
>
> So focusing on the top 3 drivers (including keystone serdes) under phy.
>
> phy-xgene.c
> -----------
>
> Looking at other drivers under drivers/phy, I could find phy-xgene.c
> which is close Keystone SerDes driver (. This is called APM X-Gene
> Multi-Purpose PHY driver. It defines following mode per the driver code
>
>          MODE_SATA       = 0,    /* List them for simple reference */
>          MODE_SGMII      = 1,
>          MODE_PCIE       = 2,
>          MODE_USB        = 3,
>          MODE_XFI        = 4,
>
> But seems to support only MODE_SATA. From the code, it appears, this
> driver is expected to be enhanced in the future to support additional
> modes. I have copied the author to this email to participate in this
> discussion.
>
> Keystone SerDes supports following modes
> ----------------------------------------
>      KSERDES_PHY_SGMII,
>      KSERDES_PHY_XGE,
>      KSERDES_PHY_PCIE,
>      KSERDES_PHY_HYPERLINK,
>      KSERDES_PHY_SRIO
>
> And phy-miphy28lp.c
> ---------------------
>
> +#define PHY_TYPE_SATA          1
> +#define PHY_TYPE_PCIE          2
> +#define PHY_TYPE_USB2          3
> +#define PHY_TYPE_USB3          4
>
> Keystone SerDes hardware is highly parameterized. The init has following
> steps:-
>    - Configure the Phy to one of the mode (SATA,SGMII,PCIE,USB,XFI)
>    - Configure the Phy to the specific mode
>    - Configure N lanes for the selected mode
>    - Enable N Lanes
>
> So at a high level, I can imagine these kind of Phys require additionally
>
>    - Enable/Disable Lane
>    - check lane status periodically
>
> So there is a scope for enhancing the Phy core API to handle these kinds
> of phy ops. This might help to re-use some code. But at the lower level
> driver, we still need to write to vendor specific registers and
> configure the SerDes which is the major part of the driver and that
> still will be a major part of these drivers.
>
> I would also like to hear from Kishon (Maintainer) on his ideas for
> Generic Phy driver to support these kind of SerDes hardwares.
>
> I think it is fair to ask to merge the Keystone SerDes driver right now
> as we have spend considerable time reviewing the current series and
> taken care of all other outstanding comments. We are most happy to
> enhance the Phy core framework to help re-use code across the above and
> future SerDes driver that supports multiple modes.
>
> Or do you have some other ideas that you would like to share?
>
> Murali
>
> > (I am aware that guys working on Marvell Armada are looking into this
> > problem - but I know they're ready to post anything yet.)


--
Murali Karicheri
Linux Kernel, Keystone
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/