From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kishon Vijay Abraham I Subject: Re: [PATCH 02/15] phy-sun4i-usb: Add a helper function to update the iscr register Date: Wed, 11 Mar 2015 18:37:25 +0530 Message-ID: <55003E0D.9040509@ti.com> References: <1425933628-9672-1-git-send-email-hdegoede@redhat.com> <4772280.xE1WBZVd8Z@wuerfel> <54FEA59B.7020400@redhat.com> <2515825.598CypcWU3@wuerfel> <54FEC3DD.10108@redhat.com> <54FECD30.6080106@ti.com> <54FECF9F.7020606@redhat.com> <55000749.9040606@ti.com> <55002970.8090206@redhat.com> <55003A15.2070900@ti.com> <55003D2B.60502@redhat.com> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Return-path: In-Reply-To: <55003D2B.60502-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Hans de Goede , Arnd Bergmann Cc: Felipe Balbi , Maxime Ripard , Chen-Yu Tsai , Roman Byshko , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Id: devicetree@vger.kernel.org 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