From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH 02/15] phy-sun4i-usb: Add a helper function to update the iscr register Date: Tue, 10 Mar 2015 11:13:49 +0100 Message-ID: <54FEC3DD.10108@redhat.com> References: <1425933628-9672-1-git-send-email-hdegoede@redhat.com> <4772280.xE1WBZVd8Z@wuerfel> <54FEA59B.7020400@redhat.com> <2515825.598CypcWU3@wuerfel> 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: <2515825.598CypcWU3@wuerfel> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Arnd Bergmann Cc: Felipe Balbi , Kishon Vijay Abraham I , 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 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