From: kishon@ti•com (Kishon Vijay Abraham I)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 02/15] phy-sun4i-usb: Add a helper function to update the iscr register
Date: Wed, 11 Mar 2015 14:43:45 +0530 [thread overview]
Message-ID: <55000749.9040606@ti.com> (raw)
In-Reply-To: <54FECF9F.7020606@redhat.com>
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.
Thanks
Kishon
next prev parent reply other threads:[~2015-03-11 9:13 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-09 20:40 [PATCH 00/15] musb: Add support for the Allwinner sunxi musb controller Hans de Goede
2015-03-09 20:40 ` [PATCH 01/15] ARM: sunxi: Add register bit definitions for SRAM mapping syscon Hans de Goede
2015-03-09 20:40 ` [PATCH 02/15] phy-sun4i-usb: Add a helper function to update the iscr register Hans de Goede
2015-03-09 21:47 ` Arnd Bergmann
2015-03-10 8:04 ` Hans de Goede
2015-03-10 8:57 ` Arnd Bergmann
2015-03-10 10:13 ` Hans de Goede
2015-03-10 10:53 ` Kishon Vijay Abraham I
2015-03-10 11:03 ` Hans de Goede
2015-03-11 9:13 ` Kishon Vijay Abraham I [this message]
2015-03-11 11:39 ` Hans de Goede
2015-03-11 12:50 ` Kishon Vijay Abraham I
2015-03-11 13:03 ` Hans de Goede
2015-03-11 13:07 ` Kishon Vijay Abraham I
2015-03-11 14:44 ` Hans de Goede
2015-03-09 20:40 ` [PATCH 03/15] musb: Make musb_write_rxfun* and musb_write_rxhub* work like their tx versions Hans de Goede
2015-03-09 20:40 ` [PATCH 04/15] musb: Make busctl_offset an io-op rather then a define Hans de Goede
2015-03-09 20:40 ` [PATCH 05/15] musb: Do not use musb_read[b|w] / _write[b|w] wrappers in generic fifo functions Hans de Goede
2015-03-09 21:50 ` Arnd Bergmann
2015-03-10 7:43 ` Hans de Goede
2015-03-10 8:50 ` Arnd Bergmann
2015-03-10 8:56 ` Hans de Goede
2015-03-10 13:43 ` Arnd Bergmann
2015-03-09 20:40 ` [PATCH 06/15] musb: Fix platform code being unable to override ep access ops Hans de Goede
2015-03-09 20:40 ` [PATCH 07/15] musb: Add support for the Allwinner sunxi musb controller Hans de Goede
2015-03-09 20:40 ` [PATCH 08/15] ARM: dts: sunxi: Add syscon node for controlling SRAM mapping Hans de Goede
2015-03-09 20:40 ` [PATCH 09/15] ARM: dts: sun4i: Add USB Dual Role Controller Hans de Goede
2015-03-09 23:31 ` [linux-sunxi] " Julian Calaby
2015-03-10 9:10 ` Hans de Goede
2015-03-09 20:40 ` [PATCH 10/15] ARM: dts: sun5i: " Hans de Goede
2015-03-09 20:40 ` [PATCH 11/15] ARM: dts: sun7i: " Hans de Goede
2015-03-09 20:40 ` [PATCH 12/15] ARM: dts: sun4i: Enable USB DRC on Chuwi V7 CW0825 Hans de Goede
2015-03-10 15:07 ` Maxime Ripard
2015-03-10 15:23 ` Hans de Goede
2015-03-10 18:17 ` Maxime Ripard
2015-03-09 20:40 ` [PATCH 13/15] ARM: dts: sun5i: Enable USB DRC on UTOO P66 Hans de Goede
2015-03-09 20:40 ` [PATCH 14/15] ARM: dts: sun7i: Enable USB DRC on Cubietruck Hans de Goede
2015-03-09 20:40 ` [PATCH 15/15] ARM: dts: sun7i: Enable USB DRC on A20-OLinuxIno-Lime Hans de Goede
2015-03-09 21:44 ` [PATCH 00/15] musb: Add support for the Allwinner sunxi musb controller Arnd Bergmann
2015-03-10 1:46 ` Chen-Yu Tsai
2015-03-10 7:38 ` Hans de Goede
2015-03-10 8:31 ` Arnd Bergmann
2015-03-10 17:41 ` Maxime Ripard
2015-03-10 22:35 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55000749.9040606@ti.com \
--to=kishon@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox