Message-ID: <56295B9E.9030201@ti.com>
Date:	Thu, 22 Oct 2015 17:56:46 -0400
From:	Murali Karicheri <m-karicheri2@...com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>, <lho@....com>,
	KISHON VIJAY <kishon@...com>
CC:	WingMan Kwok <w-kwok2@...com>, <robh+dt@...nel.org>,
	<pawel.moll@....com>, <mark.rutland@....com>,
	<ijc+devicetree@...lion.org.uk>, <galak@...eaurora.org>,
	<kishon@...com>, <rogerq@...com>, <bhelgaas@...gle.com>,
	<ssantosh@...nel.org>, <devicetree@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 0/2] Common SerDes driver for TI's Keystone Platforms

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists