Re: [PATCH 02/15] phy-sun4i-usb: Add a helper function to update the iscr register

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 11-03-15 10:13, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 10 March 2015 04:33 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 10-03-15 11:53, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Tuesday 10 March 2015 03:43 PM, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 10-03-15 09:57, Arnd Bergmann wrote:
>>>>> On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 09-03-15 22:47, Arnd Bergmann wrote:
>>>>>>> On Monday 09 March 2015 21:40:15 Hans de Goede wrote:
>>>>>>>> +void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
>>>>>>>> +{
>>>>>>>> +       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
>>>>>>>> +       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
>>>>>>>> +       u32 iscr;
>>>>>>>> +
>>>>>>>> +       iscr = readl(data->base + REG_ISCR);
>>>>>>>> +       iscr &= ~clr;
>>>>>>>> +       iscr |= set;
>>>>>>>> +       writel(iscr, data->base + REG_ISCR);
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
>>>>>>>> +
>>>>>>>>
>>>>>>>
>>>>>>> I would generally consider this a bad design. What is the purpose of
>>>>>>> calling sun4i_usb_phy_update_iscr()
>>>
>>> right. That would bind the PHY driver and the controller driver and would
>>> start creating problems when a different PHY is connected with the
>>> controller.
>>>>>>
>>>>>> There are 2 different use cases for this one is to enable the dataline
>>>>>> pull-ups at driver init and disable them at driver exit, this could /
>>>>>> should probably be moved to the phy_init / phy_exit code for the usb0 phy
>>>>>> removing the need to do this from within the sunxi musb glue.
>>>>>>
>>>>>> The second use-case is more tricky, for some reasons Allwinner has decided
>>>>>> to not use the dedicated id-detect and vusb-sense pins of the phy they are
>>>>>> using (these pins are not routed to the outside).
>>>>>>
>>>>>> Instead id-detect and vusb-sense are done through any $random gpio pins
>>>>>> (including non irq capable pins on some designs requiring polling).
>>>
>>> The polling can still be done in PHY driver and can use the extcon framework
>>> to report the status to controller driver no?
>>
>> Technically the polling can be moved to the phy driver yes, but it is not easy,
>> e.g. we only need to poll when we're in otg mode rather then host-only
>> or peripheral-only mode, and the mode gets set by the musb driver not phy
>> the phy driver. The sunxi musb implementation uses an integrated phy, so it
>> is just much easier and more logical to have all control / polling happening
>> from a single module rather then from 2 different modules.
>>
>>>>>>
>>>>>> But the musb-core still needs to know the status of the id and vbus pins,
>>>
>>> musb-core or the musb-glue (musb/sunxi.c in this case)?
>>>>>> and gets this from the usb0-phy iscr register, which reflects the status of
>>>>>> the not connected dedicated pins of the phy. The reason this can still
>>>>>> work at all is because the iscr register allows the user to override
>>>>>> whatever the not connected phy pins are seeing and forcing a value to
>>>>>> report to the musb core as id and vbus status.
>>>>>>
>>>>>> This is done by these 2 functions in the musb sunxi glue:
>>>>>>
>>>>>> static void sunxi_musb_force_id(struct musb *musb, u32 val)
>>>>>> {
>>>>>>           struct sunxi_glue *glue =
>>>>>> dev_get_drvdata(musb->controller->parent);
>>>>>>
>>>>>>           if (val)
>>>>>>                   val = SUNXI_ISCR_FORCE_ID_HIGH;
>>>>>>           else
>>>>>>                   val = SUNXI_ISCR_FORCE_ID_LOW;
>>>>>>
>>>>>>           sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,
>>>>>> val);
>>>
>>> What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq
>>> interrupt handler?
>>
>> No this changes the vbus and id status as seen by the musb core, and the musb
>> responds to this, by e.g. starting a session, or when vbus does not get high
>> after a session request by reporting a vbus-error interrupt.
>>
>> The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio
>> pins which are used to monitor the id and vbus status.
>>
>>
>>>>>> }
>>>>>>
>>>>>> static void sunxi_musb_force_vbus(struct musb *musb, u32 val)
>>>>>> {
>>>>>>           struct sunxi_glue *glue =
>>>>>> dev_get_drvdata(musb->controller->parent);
>>>>>>
>>>>>>           if (val)
>>>>>>                   val = SUNXI_ISCR_FORCE_VBUS_HIGH;
>>>>>>           else
>>>>>>                   val = SUNXI_ISCR_FORCE_VBUS_LOW;
>>>>>>
>>>>>>           sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,
>>>>>> val);
>>>>>> }
>>>>>>
>>>>>> I will happily admit that these 2 functions are a better API between the
>>>>>> sunxi musb
>>>>>> glue and the sunxi usb phy driver. I started with the minimal
>>>>>> sun4i_usb_phy_update_iscr
>>>>>> approach as I wanted to keep the API as small as possible, but having 2
>>>>>> functions like
>>>>>> the one above, which actually reflect what is happening would indeed be
>>>>>> better.
>>>>>
>>>>> Ok, that would definitely improve things.
>>>>>
>>>>>> Note that the polling of the pins cannot (easily) be moved into the phy
>>>>>> driver for various
>>>>>> reasons:
>>>>>>
>>>>>> 1) It depends on dr_mode, the otg may be used in host only mode in which
>>>>>> case there are no
>>>>>> pins at all.
>>>>>> 2) the musb set_vbus callback needs access to the pins
>>>>>> 3) When id changes some musb core state changes are necessary.
>>>>>>
>>>>>> I'll respin the patch set to do things this way as soon as we've agreement on
>>>>>> your second point.
>>>>>>
>>>>>>   > and why can't there be a high-level
>>>>>>> PHY API for this?
>>>>>>
>>>>>> The current generic phy API seems to not have any bus specific methods, I
>>>>>> know that
>>>>>> in the long run people want to get rid of struct usb_phy, so maybe we should
>>>>>> consider
>>>>>> adding bus specific methods to the generic phy API for things like otg.
>>>>>>
>>>>>> If we decide to add bus specific methods, then the question becomes if having
>>>>>>
>>>>>> int phy_usb_set_id_detect(struct phy *phy, bool val);
>>>>>> int phy_usb_set_vbus_detect(struct phy *phy, bool val);
>>>>>>
>>>>>> Functions in the generic phy API is a good idea, or if this is too sunxi
>>>>>> specific,
>>>>>> I'm fine with doing this either way. If we want to go the generic phy route
>>>>>> I'll split this in 2 patches, one adding these 2 generic functions &
>>>>>> phy-ops, and
>>>>>> 1 doing the sunxi implementation.
>>>
>>> Please don't do that. We don't want to be bloating phy framework with platform
>>> specific hooks.
>>
>> Right, that was my feeling too. I believe that using sunxi specific phy functions
>> here is best given that we're dealing with an otg controller + phy both if which
>
> Using platform specific PHY functions dissolves the very purpose of creating
> the PHY framework. We have to find a better way.

"We have to find a better way" is not really a helpful answer to the problem at
hand. We're talking about a phy integrated on the same die, as, and closely working
together with the otg controller, so we need some sort of coordination between the
two.

The 2 logical options for coordinatinon are:

1) Add generic phy framework functions for such coordination
  -> Which you've nacked

2) Use platform private functions for this
  -> Which you are essentially nacking above.

Which leaves us with what as solution exactly ?

I could make the phy-sun4i-usb.c driver register a struct usb_phy, as
drivers/phy/phy-omap-usb2.c and drivers/phy/phy-twl4030-usb.c does,
and communicate through that, but AFAIK that is deprecated ...

Regards,

Hans