[PATCH v9 0/8] Generic PHY Framework

[Kishon Vijay Abraham I]

Hi,

On Monday 08 July 2013 04:54 PM, Patel, Satish wrote:
> Hi,
>
>
>> -----Original Message-----
>> From: Balbi, Felipe
>> Sent: Thursday, July 04, 2013 3:42 PM
>> To: ABRAHAM, KISHON VIJAY
>> Cc: Patel, Satish; Balbi, Felipe; grant.likely at linaro.org;
>> tony at atomide.com; arnd at arndb.de; swarren at nvidia.com;
>> sylvester.nawrocki at gmail.com; linux-kernel at vger.kernel.org; linux-
>> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
>> usb at vger.kernel.org; gregkh at linuxfoundation.org; akpm at linux-
>> foundation.org; rob.herring at calxeda.com; rob at landley.net;
>> linux at arm.linux.org.uk; benoit.cousson at linaro.org; mchehab at redhat.com;
>> cesarb at cesarb.net; davem at davemloft.net; Nayak, Rajendra;
>> shawn.guo at linaro.org; Shilimkar, Santosh; devicetree-
>> discuss at lists.ozlabs.org; linux-doc at vger.kernel.org; Nori, Sekhar;
>> Krishnamoorthy, Balaji T; Cherian, George; Mankad, Maulik Ojas
>> Subject: Re: [PATCH v9 0/8] Generic PHY Framework
>>
>> On Thu, Jul 04, 2013 at 03:25:32PM +0530, Kishon Vijay Abraham I
>> wrote:
>>> On Thursday 04 July 2013 02:51 PM, Patel, Satish wrote:
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Balbi, Felipe
>>>>> Sent: Wednesday, July 03, 2013 6:51 PM
>>>>> To: ABRAHAM, KISHON VIJAY
>>>>> Cc: Patel, Satish; grant.likely at linaro.org; tony at atomide.com;
>> Balbi,
>>>>> Felipe; arnd at arndb.de; swarren at nvidia.com;
>>>>> sylvester.nawrocki at gmail.com; linux-kernel at vger.kernel.org; linux-
>>>>> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
>>>>> usb at vger.kernel.org; gregkh at linuxfoundation.org; akpm at linux-
>>>>> foundation.org; rob.herring at calxeda.com; rob at landley.net;
>>>>> linux at arm.linux.org.uk; benoit.cousson at linaro.org;
>> mchehab at redhat.com;
>>>>> cesarb at cesarb.net; davem at davemloft.net; Nayak, Rajendra;
>>>>> shawn.guo at linaro.org; Shilimkar, Santosh; devicetree-
>>>>> discuss at lists.ozlabs.org; linux-doc at vger.kernel.org; Nori, Sekhar;
>>>>> Krishnamoorthy, Balaji T; Cherian, George
>>>>> Subject: Re: [PATCH v9 0/8] Generic PHY Framework
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Wed, Jul 03, 2013 at 03:35:39PM +0530, Kishon Vijay Abraham I
>>>>> wrote:
>>>>>> On Wednesday 03 July 2013 03:02 PM, Patel, Satish wrote:
>>>>>>> Hi Kishon,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: ABRAHAM, KISHON VIJAY
>>>>>>>> Sent: Wednesday, June 26, 2013 5:17 PM
>>>>>>>> To: grant.likely at linaro.org; tony at atomide.com; Balbi, Felipe;
>>>>> ABRAHAM,
>>>>>>>> KISHON VIJAY; arnd at arndb.de; swarren at nvidia.com;
>>>>>>>> sylvester.nawrocki at gmail.com; linux-kernel at vger.kernel.org;
>> linux-
>>>>>>>> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> linux-
>>>>>>>> usb at vger.kernel.org; gregkh at linuxfoundation.org; akpm at linux-
>>>>>>>> foundation.org
>>>>>>>> Cc: rob.herring at calxeda.com; rob at landley.net;
>>>>> linux at arm.linux.org.uk;
>>>>>>>> benoit.cousson at linaro.org; mchehab at redhat.com;
>> cesarb at cesarb.net;
>>>>>>>> davem at davemloft.net; Nayak, Rajendra; shawn.guo at linaro.org;
>>>>> Shilimkar,
>>>>>>>> Santosh; devicetree-discuss at lists.ozlabs.org; linux-
>>>>>>>> doc at vger.kernel.org; Nori, Sekhar; Krishnamoorthy, Balaji T;
>>>>> Cherian,
>>>>>>>> George
>>>>>>>> Subject: [PATCH v9 0/8] Generic PHY Framework
>>>>>>>>
>>>>>>>> Added a generic PHY framework that provides a set of APIs for
>> the
>>>>> PHY
>>>>>>>> drivers
>>>>>>>> to create/destroy a PHY and APIs for the PHY users to obtain a
>>>>>>>> reference to
>>>>>>>> the PHY with or without using phandle.
>>>>>>>>
>>>>>>>> This framework will be of use only to devices that uses
>> external
>>>>> PHY
>>>>>>>> (PHY
>>>>>>>> functionality is not embedded within the controller).
>>>>>>>>
>>>>>>>> The intention of creating this framework is to bring the phy
>>>>> drivers
>>>>>>>> spread
>>>>>>>> all over the Linux kernel to drivers/phy to increase code re-
>> use
>>>>> and
>>>>>>>> to
>>>>>>>> increase code maintainability.
>>>>>>>
>>>>>>> I would like to use this framework for a smart-card controller
>>>>> connected to a
>>>>>>> smart-card phy. I have some questions and would like to get
>>>>> feedback on the same.
>>>>>>
>>>>>> glad to know that :-)
>>>>>>>
>>>>>>> I am using “TDA8026" Smartcard PHY from NXP. Here is the link
>> for
>>>>> datasheet
>>>>>>> and app note for the same. The smart card controller is inside
>> the
>>>>> TI SoC
>>>>>>> I am working with.
>>>>>>>
>>>>>>> Datasheet :
>>>>>>> www.nxp.com/documents/data_sheet/TDA8026.pdf?
>>>>>>>
>>>>>>> Appnote :
>>>>>>> http://www.nxp.com/documents/application_note/AN10724.pdf
>>>>>>>
>>>>>>> The TI SoC details are not public (yet). I can provide details
>> to
>>>>> you offline.
>>>>>>>
>>>>>>> Brief about operation:
>>>>>>> -	The controller can work with and without a PHY
>>>>>>> -	When not using PHY, it is limited to talking to a single
>>>>>>> 	smart card. There is also a need to put external de-activation
>>>>> logic
>>>>>>> 	on card removal for this case.
>>>>>>> -	With a PHY you can use more than one smart card.
>>>>>>> -	Phy has 5 slots :  1 for smart card (credit/debit/other
>> card
>>>>> with chip)
>>>>>>>        and others for SAM – SIM like modules
>>>>>>> - 	Once the PHY is initialized, there are some operations
>> that the
>>>>> controller
>>>>>>> 	can request of the PHY like:
>>>>>>> 	- Card configurations  - set voltage
>>>>>>> 	- Activation of card
>>>>>>> 	- ATR – Answer to reset
>>>>>>> 	- Warm reset
>>>>>>> 	- ADPU exchange
>>>>>>> 	- Deactivation ( Normal/Emergency)
>>>>>>
>>>>>> hmm.. We should think about extending the phy_ops to include
>> these
>>>>>> operations (something like phy_smart_card_ops so that other
>>>>>> smart_card PHYs will also be able to use it).
>>>>>
>>>>> let's try to avoid use-case specific additions. set_voltage sounds
>>>>> like
>>>>> a regulator thing, but the regulator is controlled through the
>> PHY. I
>>>>> guess it makes sense to have a generic phy_set_voltage() call
>> since
>>>>> even
>>>>> USB can make use of that.
>>>>>
>>>>> For card activation, it sounds like phy_init()/phy_shutdown()
>> would
>>>>> cover it.
>>>>>
>>>>> For warm reset perhaps a phy_reset() callback ? Although that
>> could,
>>>>> easily, get abused.
>>>>>
>>>>> For deactivation, that's phy_shutdown().
>>>>>
>>>>> ATR and ADPU needs more thought, I guess.
>>>>>
>>>>>>> - 	In the mode when smartcard controller talks directly to
>> the
>>>>> card without the need
>>>>>>> 	for a PHY, all the above operations will be carried out by the
>>>>> controller itself
>>>>>>>
>>>>>>> My current thought process is to make the controller driver
>> provide
>>>>> the user interface
>>>>>>> and talk to the PHY using the generic PHY framework you
>> proposed.
>>>>> In the case where there
>>>>>>> is no PHY, my idea is to create a "dummy" PHY which uses the
>>>>> controller functionality itself.
>>>>>>
>>>>>> right. And in the case where you actually have a PHY, create a
>> PHY
>>>>>> driver and implement the phy_smart_card_ops and register with the
>>>>> PHY
>>>>>> framework.
>>>>>
>>>>> I would try to avoid that. Otherwise we will have phy_usb_ops,
>>>>> phy_sata_ops, phy_network_ops, phy_pci_ops, etc etc etc. It would
>>>>> easily
>>>>> blow up.
>>>>>
>>>>
>>>> - I do agree with you. Creating Phy specific ops will blow up whole
>>>>    concept of generic phy f/w.
>>>> - Can we have interface like phy_setconfig - with parameter like
>>>>    phy_setconfig(int param, void *value)
>>>> 	- Here param can be enum of available config parameters for
>>>> 	  specific phy.
>>>> Phy can perform different operation/set internal state based on
>>>> param selection and value passed by.
>>>>
>>>> e.x in case of smartcard
>>>> 	enum set_config {
>>>> 		SET_VOLATAGE,
>>>> 		SET_ACTIVATE,
>>>> 		SET_WARMRESET,
>>>> 		SET_ATR,
>>>> 		SET_DEACTIVE,
>>>> 		....
>>>> 	};
>>>
>>> hmm.. this looks similar to ioctl and can be abused easily IMO :s
>>
>> +1
>>
>> What we need is to come up with generic ways to model those, if we
>> need
>> set voltage, then add a phy_set_voltage() kinda thing, perhaps add
>> capability flags to enable/disable support fort those (just like mmc
>> does).
>>
>> But adding something which can handle "anything" like
>> phy_set_config(),
>> it's the same as adding use-case specific ops.
>>
>
> We have two options over here
>
> Option 1:
>
> Defining generic api to which can be mapped over multiple phys
> For smartcard case, I have can thought of following mapping with new
> generic apis as suggested.
>
> Smartcard_poweron -> power_on
> Smartcard_powerdown -> power_off
> Smartcard_set_voltage -> phy_set_voltage
> Smartcard_activate_card -> phy_activate_slot
> Smartcard_deactivate_card -> phy_deactivate_slot

how is activate/deactivate different from poweron/poweroff in this
use case?
> Smartcard_set_c4/c8/rst/io -> phy_set_pin

Whats should be exactly done here? Looks to me like it should be
part of init. Does these pin settings need to be changed dynamically?
> Smartcard_warm_reset -> phy_warmreset

Again looks to me like it should be part of init.
> Smartcard_set_clkdiv -> phy_set_clock
> Smartcard_get_clkdiv -> phy_get_clock

Why would the smartcard give clock to the PHY? Shouldn't the driver
of the PHY driver be doing that by itself?

> Smartcard_set_atr_mute_timeout -> ??
> Smartcard_set_atr_early_timeout -> ??	
> Smartcard_get_isr_status -> phy_get_status

Why? Is it like the detection of an external event? Then the PHY
driver should use *extcon* for event detection and passing.

> Smartcard_get_version -> phy_get_version

Again why? Why would the smartcard need the version of the PHY?

Thanks
Kishon