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 14:07, Kishon Vijay Abraham I wrote:
> 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.

Ok, I've taken a look and I see that it is already used for exactly this purpose
in drivers/usb/phy/phy-tahvo.c and drivers/usb/phy/phy-omap-otg.c .

Since I take it that this is the preferred solution for dealing with this, I'll
go and rework my patchset to move the polling to the phy driver and use extcon
to signal the status of the id / vbus det pins to other interested parties.

Regards,

Hans