linux-kernel - Re: [PATCH V4 3/4] ARM: shmobile: koelsch: Add USBHS and internal PCI USB support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANqRtoQjFMezrw0oEYCE97+sWu2mNZnHz0nher8oKxzx7FH5_w@mail.gmail.com>
Date:	Mon, 24 Feb 2014 17:05:26 +0900
From:	Magnus Damm <magnus.damm@...il.com>
To:	Vladimir Barinov <vladimir.barinov@...entembedded.com>
Cc:	SH-Linux <linux-sh@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-gpio <linux-gpio@...r.kernel.org>,
	"Simon Horman [Horms]" <horms@...ge.net.au>,
	Alexandre Courbot <gnurou@...il.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
	Valentine Barshak <valentine.barshak@...entembedded.com>
Subject: Re: [PATCH V4 3/4] ARM: shmobile: koelsch: Add USBHS and internal PCI
 USB support

Hi Vladimir,

On Mon, Feb 24, 2014 at 4:34 PM, Vladimir Barinov
<vladimir.barinov@...entembedded.com> wrote:
> Hello Magnus,
>
> Thank you for the quick response.
>
>
> On 02/24/2014 07:52 AM, Magnus Damm wrote:
>>
>> +static int usbhs_hardware_init(struct platform_device *pdev)
>> +{
>> +       struct usbhs_private *priv = usbhs_get_priv(pdev);
>> +       struct usb_phy *phy;
>> +
>> +       phy = usb_get_phy_dev(&pdev->dev, 0);
>> +       if (IS_ERR(phy))
>> +               return PTR_ERR(phy);
>> +
>> +       priv->phy = phy;
>> +       return 0;
>> +}
>> +
>> +static int usbhs_hardware_exit(struct platform_device *pdev)
>> +{
>> +       struct usbhs_private *priv = usbhs_get_priv(pdev);
>> +
>> +       if (!priv->phy)
>> +               return 0;
>> +
>> +       usb_put_phy(priv->phy);
>> +       priv->phy = NULL;
>> +       return 0;
>> +}
>> +
>> +static int usbhs_get_id(struct platform_device *pdev)
>> +{
>> +       return USBHS_GADGET;
>> +}
>> Uhm, I sort of expected this place to be where you read out the ID pin
>> state from the MAX device.
>
> Yes, I've seen your work for lager board.
> I did differently then you beacause of problem in USBHS Host driver, hence
> the need of setup PHY at initial stage to PCI USB and not to USBHS.

Yeah, I understand. But with the current patches I wonder, isn't there
risk for short circuit ? Say that a USB host cable is connected during
boot and the PCI USB host controller is hooked up, what is stopping us
from switching the cable and driving VBUS from two sides using a USB
function cable? So the current patches seem a bit unsafe to me.

>>> +static u32 koelsch_usbhs_pipe_type[] = {
>>> +
>>> +/* Add all available USB devices */
>>> +static void __init koelsch_add_usb_devices(void)
>>> +{
>>> +       /* MAX3355E ID pin */
>>> +       gpio_request_one(RCAR_GP_PIN(5, 31), GPIOF_IN, NULL);
>>> +       if (!gpio_get_value(RCAR_GP_PIN(5, 31))) {
>>> +               usbhs_phy_pdata.chan0_pci = 1; /* Channel 0 is PCI USB
>>> host */
>>> +               koelsch_add_usb0_host();
>>> +       } else {
>>> +               usbhs_phy_pdata.chan0_pci = 0; /* Channel 0 is USBHS */
>>> +               koelsch_add_usb0_gadget();
>>> +       }
>>
>> I don't think we should perform this kind of check at boot-up. This
>> goes without saying, but normal USB supports hot-plug so we should
>> check the cable type when the cable insertion event happens. Please
>> see my comment above for USBHS-specific location.
>>
>> I do however understand that according to your investigation you
>> cannot use USBHS in host mode. I believe further investigation is
>> needed in that area to determine what is the best way forward.
>> Regarding VBUS control, I believe it should be possible to drive the
>> USB0_VBUS as GPIO and manually control the power.
>
> I see.

To control USB0_VBUS as GPIO you may need to adjust the PFC tables for
pinctrl. At some point, would it be possible for you to cook up some
prototype code to try to control the USB0_VBUS signal via GPIO? This
may be pointless if the USBHS hardware cannot operate in Host mode
though.

>> Would it be possible for you to break out USB PCI support for USB1 and
>> resend that so we can begin by merging that?
>
> Wouldn't you like me also add USBHS in gadget mode for USB0 with the similar
> check like you did on lager (with ID pin),
> since it does not need the gpio-rcar changes too.

Thanks for your offer. Yes, that would be nice. May I suggest doing it
on two levels:
1) Gadget-only support (is it possible to perform runtime check of ID
pin value at insert event and give error in case of Host?)
2) Incremental USBHS host patch

Using two incremental patches like above we can begin by merging 1)
and keep on working on 2).

> Also if you'd like I can add the USBHS in Host mode with the ID pin check
> like you suggested, but the usb0 host will not be stable.
> Probably this could speed up the USBHS Host development/fixing.

Please add it as a separate incremental patch if possible. I'd like to
have some kind of stable level of support without any funky
dependencies as a first goal, then keep on trying to get USBHS
working.

I think PCI USB for the micro USB port can simply be dropped now and
only use USBHS.

Thank you!

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/