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

[Kishon Vijay Abraham I]

Hi,

On Wednesday 11 March 2015 06:33 PM, Hans de Goede wrote:
> Hi,
>
> On 11-03-15 13:50, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 11 March 2015 05:09 PM, Hans de Goede wrote:
>>> 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
>>
>> It's integrated in the same die but it is not within the controller IP. It's
>> still possible to replace the PHY and have the same controller in the next SoC.
>>> 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 ?
>>
>> We could poll on the GPIO in PHY driver to update the iscr register. I think
>> it's alright to poll on the GPIO in multiple modules for serving it's own
>> purpose IMHO.
>
> No, polling in multiple places is just plain and simple wrong, also where-ever
> possible we use edge triggered gpio-interrupts for this and having multiple
> handlers for the same interrupt is even more wrong.
>
> I'm fine with moving all the polling to the phy driver, but then we need to
> have a way to notify the sunxi musb glue about id pin changes.

As I mentioned before, we can use extcon framework for it.
>
>>> 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,
>>
>> I'm not sure how will that help.
>
> It helps when we move the polling to the phy driver as it has a notification
> mechanism for events which we can then use to tell the musb-sunxi glue about
> id pin changes.

extcon framework might help here.

Cheers
Kishon