Re: [PATCH v9 0/8] Generic PHY Framework

[Rahul Sharma <r.sh.open@gmail.com>]

Hi Balbi, Kishon,

I have similar requirements for exynos hdmi phy. hdmi driver (controller)
need to set the operating clock to hdmi phy which depends on the pixel
clock.

Phy has an internal pll to be configured as per changing resolution. It can
be done by writing into phy registers. CCF callbacks (set/get_rate) can
not be used unless I register phy as a clock controller. But that way
hdmi driver loses finer power control for the phy. Phy should be ON, only
when hdmi hotplugged.

Callback: set_phy_operating_freq in phy framework, seems generic and
addressing similar requirements for other phys. What you say on this?

regards,
Rahul Sharma.

On Tue, Jul 9, 2013 at 11:04 PM, Felipe Balbi <balbi@ti.com> wrote:
>
> Hi,
>
> On Tue, Jul 09, 2013 at 02:33:58PM +0200, Patel, Satish wrote:
> > > > Operation:
> > > >
> > > > Once the card gets inserted into the slot, user application can
> > > > initiate activate sequence anytime, it is not mandatory that
> > > > activation has to be called Immediately. It all depends whether
> > > other
> > > > connections ( network, printer) are proper.  User application can do
> > > > some checks and then dynamically activate the card
> > >
> > > The card or the phy ? Who actually activates ? Is the activation
> > > handling part of the PHY or part of the smartcard controller ?
> > >
> >
> > It’s a activation of smart card which is inserted into the phy slot.
> > Smart card activation will by done phy but initiated by user application
> > Handling of activation should be done at both the side - controller as well
> > As phy. Phy will raise certain signals and check timeout while activation
> > , while controller will store data coming from smart card to FIFO. Its
> > Controller which will give command to phy to initiate activation sequence when it
> > is ready.
> >
> > Which means, both controller and phy has role to play while activation.
> > Also it is applicable while deactivation.
>
> right, so when the smartcard device is opened, you want to powerup the
> PHY, powerup the smartcard controller and get things going, which means
> that all your activation logic can, and should, be hidden by
> phy_power_on() and phy_power_off()
>
> Perhaps you also want to delay phy_init() and phy_shutdown() to that
> very time, which would mean that we wouldn't even need
> phy_power_{on,off}() calls whatsoever.
>
> > > > There are 2 sequence of activation - normal activation and warm
> > > reset.
> > > > If first fails then warm reset should be executed, if card does not
> > > > response to both then deactivation will be carried out
> > >
> > > ret = phy_resume(phy);
> > > if (ret == -EINVAL) { /* perhaps there's a better error code */
> > >     ret = phy_reset(phy);
> > >     if (ret)
> > >             phy_suspend(phy);
> > > }
> > >
> >
> > Its not like this. There are the cases, when during normal activation
> > card respond With some characters, but they might not be valid or
> > error prone - This check will be done by user application as only
> > application knows how to decode characters as per protocol.
>
> sure, and if the characters are invalid, the application needs to tell
> the kernel, perhaps it would be enough to send and IOCTL or, even
> easier, close the device and reopen it.
>
> > It there is any error in characters/timeout paratmeters, user
> > application will ask to initiate Warm reset.
>
> and for that we can use an IOCTL which is implemented by smart card
> layer. I mean, I'm assuming the smartcard would be seen as a char dev by
> userland so it would be something like this:
>
> ret = read(fd, buf, SIZE);
> if (ret < 0)
>         boom();
>
> ret = process_buffer(buf, ret);
> if (ret == -EINVAL) { /* we have received invalid characters */
>         ret = ioctl(fd, IOCTL_SMARTCARD_WARM_RESET);
>         if (ret < 0)
>                 handle_error();
> }
>
> in kernel space we would have an ioctl handler like so:
>
> static int smart_card_ioctl(struct file *fd, unsigned code, usigned long v)
> {
>         struct smart_card *sc = fd->private_data;
>         unsigned long flags;
>         int ret;
>
>         spin_lock_irqsave(&sc->lock, flags);
>         switch (v)
>         case SMART_CARD_WARM_RESET:
>                 ret = phy_shutdown(sc->phy);
>                 if (ret < 0)
>                         boom();
>
>                 ret = smart_card_reset(sc);
>                 if (ret < 0)
>                         boom();
>
>                 ret = phy_init(sc->phy);
>                 if (ret < 0)
>                         boom();
>                 break;
>         [ ... ]
>         }
>
>         spin_unlock_irqrestore(&sc->lock, flags);
>
>         return ret;
> }
>
> see, we don't need to add a bunch of stuff in the PHY framework, because
> you shouldn't expose the PHY directly to userland. You *must* have an
> abstraction to userland with an in-kernel usage of the PHY layer.
>
> > > > - on activation card will respond with some initial characters
> > > (ATR),
> > > > which will be verified by application, in case of error user
> > > > application will de-activate the card not the phy module
> > > >
> > > > - After activation, use application initiate normal tx/rx
> > > >
> > > > - Once tx/rx is done, it will initiate deactivation of card not the
> > > > phy module.
> > >
> > > then that has nothing to do with the PHY framework, right ?
> >
> > But all such sequence will be initiated by PHY by raising certain
> > signals and only controller can give the push when to start.
>
> sure, that's fair. But you don't need to expose every single detail of
> the PHY. You need to use abstractions to make accesses to the PHY easy
> to maintain. See the example above.
>
> > > > > > 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?
> > > >
> > > > These are all phy pins which will be physically come in contact with
> > > > smart card When inserted. In case of synchronous card, these pins
> > > will
> > > > be toggled manually by s/w bit bang algorithm
> > >
> > > is that a requirement or just something that everybody does ?
> > >
> >
> > Its requirement for synchronous card. At present there is no open driver
> > Which does this thing.
>
> alright, so you need to tell the PHY in which mode it should work and
> the PHY driver needs to be smart enough to choose proper callbacks
> depending on the mode.
>
> > > In any case, you can hide that under some gpio calls, or something
> > > similar.
> >
> > Can't be done using GPIO as pin can be toggled only which i2c read/write
> > to the phy.
>
> so ? look at the numerous I2C GPIO expanders we have in the kernel ;-)
>
> > > > > > Smartcard_get_version -> phy_get_version
> > > > >
> > > > > Again why? Why would the smartcard need the version of the PHY?
> > > > >
> > > >
> > > > Can be omitted. This is for the test suite reference.
> > >
> > > if it's for testsuite, it sounds like something that should be done
> > > through userland and, as such, should be implemented by a set of
> > > debugfs
> > > files which expose the necessary information. That has nothing to do
> > > with the PHY framework.
> > >
> > Agree.. can be omitted.
> >
> >
> > I am ok to give internal webex presentation (TI team) to brief
> > on phy and smart card controller interface.
>
> let's try to cut down on internal agreements and let the community help
> defining a proper way to handle the PHY :-)
>
> Still, if you want to point me and Kishon (privately) to some internal
> documentation, we can certainly read through it :-)
>
> cheers
>
> --
> balbi
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>