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 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()
>>
>> 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).
>>
>> But the musb-core still needs to know the status of the id and vbus pins,
>> 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);
>> }
>>
>> 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.
>>
>> If people believe this is too sunxi specific (I believe it is, but as said I'm
>> fine with doing this either way). I'll respin this patch to remove the too
>> generic sun4i_usb_phy_update_iscr function, and instead add these 2:
>>
>> void sun4i_usb_phy_set_id_detect(struct phy *phy, bool val);
>> void sun4i_usb_phy_set_vbus_detect(struct phy *phy, bool val);
>
> Thanks for your detailed explanations. I think this is something for
> Kishon to decide, based on where he wants to take the phy API in the
> long run. I'm fine with it either way, and it seems easy enough to
> change between those two interfaces if we make up our minds about it
> later.

Ok, so I've fixed things in my personal tree to use 2 sun4i private functions for
now:

void sun4i_usb_phy_set_id_detect(struct phy *phy, bool val);
void sun4i_usb_phy_set_vbus_detect(struct phy *phy, bool val);

You can find this change here:
https://github.com/jwrdegoede/linux-sunxi/commits/sunxi-wip

I'll wait a bit to see if there are more review comments and then
I'll send a v2 of the set.

Regards,

Hans