[PATCH 0/2] arm: shimobile: add HS-USB device node on r8a7790

[PATCH 0/2] arm: shimobile: add HS-USB device node on r8a7790

[Yoshihiro Shimoda]

 This series is based on Simon's renesas.git branch and
renesas-devel-20141002-v3.17-rc7 tag. If we use the generic phy
driver for R-Car Gen2 (drivers/phy/phy-rcar-gen2.c), we can use
the HS-USB on lager.

Yoshihiro Shimoda (2):
  arm: shimobile: r8a7790: add HS-USB device node
  arm: shmobile: lager: enable HS-USB

 arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
 arch/arm/boot/dts/r8a7790.dtsi      |   10 ++++++++++
 2 files changed, 15 insertions(+)

-- 
1.7.9.5

[PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Yoshihiro Shimoda]

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 2380fd5..fd9fc37 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -1314,4 +1314,14 @@
 			ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
 		};
 	};
+
+	hsusb: usb@e6590000 {
+		compatible = "renesas,usbhs-r8a7790";
+		reg = <0 0xe6590000 0 0x100>;
+		interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
+		renesas,buswait_bwait = <4>;
+		phys = <&usb0 1>;
+		phy-names = "usb";
+	};
 };
-- 
1.7.9.5

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Sergei Shtylyov]

On 10/2/2014 12:04 PM, Yoshihiro Shimoda wrote:

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
>   1 file changed, 10 insertions(+)

> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index 2380fd5..fd9fc37 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -1314,4 +1314,14 @@
>   			ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
>   		};
>   	};
> +
> +	hsusb: usb@e6590000 {
> +		compatible = "renesas,usbhs-r8a7790";
> +		reg = <0 0xe6590000 0 0x100>;
> +		interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;

    I don't see where are you handling this prop in the driver...

> +		renesas,buswait_bwait = <4>;

    This prop is named "renesas,buswait" now.

>" +		phys = <&usb0 1>;
> +		phy-names = "usb";
> +	};
>   };

WBR, Sergei

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Yoshihiro Shimoda]

Hello.

(2014/10/02 18:58), Sergei Shtylyov wrote:
> On 10/2/2014 12:04 PM, Yoshihiro Shimoda wrote:
> 
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>   arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
>>   1 file changed, 10 insertions(+)
> 
>> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
>> index 2380fd5..fd9fc37 100644
>> --- a/arch/arm/boot/dts/r8a7790.dtsi
>> +++ b/arch/arm/boot/dts/r8a7790.dtsi
>> @@ -1314,4 +1314,14 @@
>>               ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
>>           };
>>       };
>> +
>> +    hsusb: usb@e6590000 {
>> +        compatible = "renesas,usbhs-r8a7790";
>> +        reg = <0 0xe6590000 0 0x100>;
>> +        interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> 
>    I don't see where are you handling this prop in the driver...

Thank you for the point. I misunderstood this prop.
Since the driver is using pm_runtime functions, I thought that
this device node needs this prop.

So, I will remove this. And, I will also modify the Documentation.

>> +        renesas,buswait_bwait = <4>;
> 
>    This prop is named "renesas,buswait" now.

Thank you for the point. I will fix it.

Best regards,
Yoshihiro Shimoda

>> " +        phys = <&usb0 1>;
>> +        phy-names = "usb";
>> +    };
>>   };
> 
> WBR, Sergei
> 

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Sergei Shtylyov]

Hello.

On 10/02/2014 03:40 PM, Yoshihiro Shimoda wrote:

>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>>    arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
>>>    1 file changed, 10 insertions(+)

>>> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
>>> index 2380fd5..fd9fc37 100644
>>> --- a/arch/arm/boot/dts/r8a7790.dtsi
>>> +++ b/arch/arm/boot/dts/r8a7790.dtsi
>>> @@ -1314,4 +1314,14 @@
>>>                ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
>>>            };
>>>        };
>>> +
>>> +    hsusb: usb@e6590000 {
>>> +        compatible = "renesas,usbhs-r8a7790";
>>> +        reg = <0 0xe6590000 0 0x100>;
>>> +        interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
>>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;

>>     I don't see where are you handling this prop in the driver...

> Thank you for the point. I misunderstood this prop.
> Since the driver is using pm_runtime functions, I thought that
> this device node needs this prop.

    Ah, I've overlooked the RPM related code. No need to remove the prop then.

WBR, Sergei

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Sergei Shtylyov]

On 10/2/2014 12:04 PM, Yoshihiro Shimoda wrote:

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
>   1 file changed, 10 insertions(+)

> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index 2380fd5..fd9fc37 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -1314,4 +1314,14 @@
>   			ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
>   		};
>   	};
> +
> +	hsusb: usb@e6590000 {
> +		compatible = "renesas,usbhs-r8a7790";
> +		reg = <0 0xe6590000 0 0x100>;
> +		interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		renesas,buswait_bwait = <4>;
> +		phys = <&usb0 1>;
> +		phy-names = "usb";

    You need.

    status = "disabled";

since you're enabling the device node in the board .dts.

WBR, Sergei

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Yoshihiro Shimoda]

Hello.

(2014/10/02 18:59), Sergei Shtylyov wrote:
> On 10/2/2014 12:04 PM, Yoshihiro Shimoda wrote:
> 
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>   arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
>>   1 file changed, 10 insertions(+)
> 
>> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
>> index 2380fd5..fd9fc37 100644
>> --- a/arch/arm/boot/dts/r8a7790.dtsi
>> +++ b/arch/arm/boot/dts/r8a7790.dtsi
>> @@ -1314,4 +1314,14 @@
>>               ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
>>           };
>>       };
>> +
>> +    hsusb: usb@e6590000 {
>> +        compatible = "renesas,usbhs-r8a7790";
>> +        reg = <0 0xe6590000 0 0x100>;
>> +        interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
>> +        renesas,buswait_bwait = <4>;
>> +        phys = <&usb0 1>;
>> +        phy-names = "usb";
> 
>    You need.
> 
>    status = "disabled";
> 
> since you're enabling the device node in the board .dts.

Thank you for the point again.
I will add this.

Best regards,
Yoshihiro Shimoda

> WBR, Sergei
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Sergei Shtylyov]

On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   arch/arm/boot/dts/r8a7790.dtsi |   10 ++++++++++
>   1 file changed, 10 insertions(+)

> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index 2380fd5..fd9fc37 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -1314,4 +1314,14 @@
>   			ssi9: ssi@9 { interrupts = <0 379 IRQ_TYPE_LEVEL_HIGH>; };
>   		};
>   	};
> +
> +	hsusb: usb@e6590000 {
> +		compatible = "renesas,usbhs-r8a7790";
> +		reg = <0 0xe6590000 0 0x100>;
> +		interrupts = <0 107 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&mstp7_clks R8A7790_CLK_HSUSB>;
> +		renesas,buswait_bwait = <4>;
> +		phys = <&usb0 1>;
> +		phy-names = "usb";
> +	};

    I think the better placement would be prior to USB PHY node, not at the 
end of the file.

WBR, Sergei

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Sergei Shtylyov]

Hello.

On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

    The subject has "shimobile" instead of "shmobile" on both this and R8A7791 
patch. And capitalize "arm:" please on all the patches.

WBR, Sergei

Re: [PATCH 1/2] arm: shimobile: r8a7790: add HS-USB device node

[Yoshihiro Shimoda]

Hello.

(2014/10/03 23:34), Sergei Shtylyov wrote:
> Hello.
> 
> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:
> 
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
>     The subject has "shimobile" instead of "shmobile" on both this and R8A7791 
> patch. And capitalize "arm:" please on all the patches.

Thank you for the point. I will fix it.

Best regards,
Yoshihiro Shimoda

> WBR, Sergei
> 

[PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Yoshihiro Shimoda]

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index 1698591..4badd0a 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -445,3 +445,8 @@
 		};
 	};
 };
+
+&hsusb {
+	status = "okay";
+	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
+};
-- 
1.7.9.5

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Sergei Shtylyov]

On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>   1 file changed, 5 insertions(+)

> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index 1698591..4badd0a 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -445,3 +445,8 @@
>   		};
>   	};
>   };
> +
> +&hsusb {
> +	status = "okay";
> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;

    It's certainly active-high.

WBR, Sergei

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Yoshihiro Shimoda]

Hello.

(2014/10/04 4:50), Sergei Shtylyov wrote:
> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:
> 
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>   arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>   1 file changed, 5 insertions(+)
> 
>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>> index 1698591..4badd0a 100644
>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>> @@ -445,3 +445,8 @@
>>   		};
>>   	};
>>   };
>> +
>> +&hsusb {
>> +	status = "okay";
>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
> 
>     It's certainly active-high.

Since the current code has the following, we have to set the active_low...
However, the code is unreadable, I think. So, I will modify the code.

	/* check GPIO determining if USB function should be enabled */
	if (priv->dparam.enable_gpio) {
		gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
		ret = !gpio_get_value(priv->dparam.enable_gpio);
		gpio_free(priv->dparam.enable_gpio);
		if (ret) {
			dev_warn(&pdev->dev,
				 "USB function not selected (GPIO %d)\n",
				 priv->dparam.enable_gpio);
			ret = -ENOTSUPP;
			goto probe_end_mod_exit;
		}
	}

Best regards,
Yoshihiro Shimoda

> WBR, Sergei
> 

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Simon Horman]

On Mon, Oct 06, 2014 at 09:59:48AM +0900, Yoshihiro Shimoda wrote:
> Hello.
> 
> (2014/10/04 4:50), Sergei Shtylyov wrote:
> > On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:
> > 
> >> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >> ---
> >>   arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
> >>   1 file changed, 5 insertions(+)
> > 
> >> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> >> index 1698591..4badd0a 100644
> >> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> >> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> >> @@ -445,3 +445,8 @@
> >>   		};
> >>   	};
> >>   };
> >> +
> >> +&hsusb {
> >> +	status = "okay";
> >> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
> > 
> >     It's certainly active-high.
> 
> Since the current code has the following, we have to set the active_low...
> However, the code is unreadable, I think. So, I will modify the code.

It seems to me that would be best.

As far as possible the bindings and their use should
describe the hardware rather than the software.

> 	/* check GPIO determining if USB function should be enabled */
> 	if (priv->dparam.enable_gpio) {
> 		gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
> 		ret = !gpio_get_value(priv->dparam.enable_gpio);
> 		gpio_free(priv->dparam.enable_gpio);
> 		if (ret) {
> 			dev_warn(&pdev->dev,
> 				 "USB function not selected (GPIO %d)\n",
> 				 priv->dparam.enable_gpio);
> 			ret = -ENOTSUPP;
> 			goto probe_end_mod_exit;
> 		}
> 	}
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > WBR, Sergei
> > 
> 

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Sergei Shtylyov]

Hello.

On 10/6/2014 4:59 AM, Yoshihiro Shimoda wrote:

>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>>    arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>>    1 file changed, 5 insertions(+)

>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>>> index 1698591..4badd0a 100644
>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>>> @@ -445,3 +445,8 @@
>>>    		};
>>>    	};
>>>    };
>>> +
>>> +&hsusb {
>>> +	status = "okay";
>>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;

>>      It's certainly active-high.

> Since the current code has the following, we have to set the active_low...

    Exactly opposite conclusion follows from this code.

> However, the code is unreadable, I think. So, I will modify the code.

    There's no dire need, I think.

> 	/* check GPIO determining if USB function should be enabled */
> 	if (priv->dparam.enable_gpio) {
> 		gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
> 		ret = !gpio_get_value(priv->dparam.enable_gpio);
> 		gpio_free(priv->dparam.enable_gpio);
> 		if (ret) {

    'ret' is non-zero here if gpio_get_value() returned 0, so 0 means "host" 
and 1 means "gadget", i.e. GPIO is active-high.

> 			dev_warn(&pdev->dev,
> 				 "USB function not selected (GPIO %d)\n",
> 				 priv->dparam.enable_gpio);
> 			ret = -ENOTSUPP;
> 			goto probe_end_mod_exit;
> 		}
> 	}

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Yoshihiro Shimoda]

Hello.

(2014/10/06 9:59), Yoshihiro Shimoda wrote:
> Hello.
> 
> (2014/10/04 4:50), Sergei Shtylyov wrote:
>> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:
>>
>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>>   arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>>   1 file changed, 5 insertions(+)
>>
>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>>> index 1698591..4badd0a 100644
>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>>> @@ -445,3 +445,8 @@
>>>   		};
>>>   	};
>>>   };
>>> +
>>> +&hsusb {
>>> +	status = "okay";
>>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
>>
>>     It's certainly active-high.
> 
> Since the current code has the following, we have to set the active_low...
> However, the code is unreadable, I think. So, I will modify the code.
> 
> 	/* check GPIO determining if USB function should be enabled */
> 	if (priv->dparam.enable_gpio) {
> 		gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
> 		ret = !gpio_get_value(priv->dparam.enable_gpio);
> 		gpio_free(priv->dparam.enable_gpio);
> 		if (ret) {
> 			dev_warn(&pdev->dev,
> 				 "USB function not selected (GPIO %d)\n",
> 				 priv->dparam.enable_gpio);
> 			ret = -ENOTSUPP;
> 			goto probe_end_mod_exit;
> 		}
> 	}

I am confusing about the gpio_get_value()...
In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally.

gpio_get_value() in arch/arm/include/asm/gpio.h
 --> __gpio_get_value() in include/asm-generic/gpio.h
  --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c

The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}.
So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver?
Or, Do I misunderstand something?

Best regards,
Yoshihiro Shimoda

> Best regards,
> Yoshihiro Shimoda
> 
>> WBR, Sergei
>>

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Yoshihiro Shimoda]

Hello.

(2014/10/07 12:25), Yoshihiro Shimoda wrote:
> Hello.
> 
> (2014/10/06 9:59), Yoshihiro Shimoda wrote:
>> Hello.
>>
>> (2014/10/04 4:50), Sergei Shtylyov wrote:
>>> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:
>>>
< snip >
>>>> +&hsusb {
>>>> +	status = "okay";
>>>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
>>>
>>>     It's certainly active-high.
>>
>> Since the current code has the following, we have to set the active_low...
>> However, the code is unreadable, I think. So, I will modify the code.
>>
>> 	/* check GPIO determining if USB function should be enabled */
>> 	if (priv->dparam.enable_gpio) {
>> 		gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
>> 		ret = !gpio_get_value(priv->dparam.enable_gpio);
>> 		gpio_free(priv->dparam.enable_gpio);
>> 		if (ret) {
>> 			dev_warn(&pdev->dev,
>> 				 "USB function not selected (GPIO %d)\n",
>> 				 priv->dparam.enable_gpio);
>> 			ret = -ENOTSUPP;
>> 			goto probe_end_mod_exit;
>> 		}
>> 	}
> 
> I am confusing about the gpio_get_value()...
> In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally.
> 
> gpio_get_value() in arch/arm/include/asm/gpio.h
>  --> __gpio_get_value() in include/asm-generic/gpio.h
>   --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c
> 
> The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}.
> So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver?
> Or, Do I misunderstand something?

I looked at the gpio-keys driver, and then I understand the renesas_usbhs driver should
have the gpio flags from of_get_named_gpio_flags().

So, I will try to modify the renesas_usbhs driver.

Best regards,
Yoshihiro Shimoda

> Best regards,
> Yoshihiro Shimoda
> 
>> Best regards,
>> Yoshihiro Shimoda
>>
>>> WBR, Sergei
>>>

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Sergei Shtylyov]

On 10/7/2014 7:25 AM, Yoshihiro Shimoda wrote:

>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>> ---
>>>>    arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>>>    1 file changed, 5 insertions(+)

>>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>>>> index 1698591..4badd0a 100644
>>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>>>> @@ -445,3 +445,8 @@
>>>>    		};
>>>>    	};
>>>>    };
>>>> +
>>>> +&hsusb {
>>>> +	status = "okay";
>>>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;

>>>      It's certainly active-high.

>> Since the current code has the following, we have to set the active_low...
>> However, the code is unreadable, I think. So, I will modify the code.
>>
>> 	/* check GPIO determining if USB function should be enabled */
>> 	if (priv->dparam.enable_gpio) {
>> 		gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
>> 		ret = !gpio_get_value(priv->dparam.enable_gpio);
>> 		gpio_free(priv->dparam.enable_gpio);
>> 		if (ret) {
>> 			dev_warn(&pdev->dev,
>> 				 "USB function not selected (GPIO %d)\n",
>> 				 priv->dparam.enable_gpio);
>> 			ret = -ENOTSUPP;
>> 			goto probe_end_mod_exit;
>> 		}
>> 	}

> I am confusing about the gpio_get_value()...
> In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally.

> gpio_get_value() in arch/arm/include/asm/gpio.h
>   --> __gpio_get_value() in include/asm-generic/gpio.h
>    --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c

> The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}.
> So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver?

    Why do you still think it's active-low?!

> Or, Do I misunderstand something?

    Look at the above code again.

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Yoshihiro Shimoda]

Hello.

(2014/10/07 18:46), Sergei Shtylyov wrote:
> On 10/7/2014 7:25 AM, Yoshihiro Shimoda wrote:
> 
>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>> ---
>>>>>    arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>>>>    1 file changed, 5 insertions(+)
> 
>>>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>>>>> index 1698591..4badd0a 100644
>>>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>>>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>>>>> @@ -445,3 +445,8 @@
>>>>>            };
>>>>>        };
>>>>>    };
>>>>> +
>>>>> +&hsusb {
>>>>> +    status = "okay";
>>>>> +    renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
> 
>>>>      It's certainly active-high.
> 
>>> Since the current code has the following, we have to set the active_low...
>>> However, the code is unreadable, I think. So, I will modify the code.
>>>
>>>     /* check GPIO determining if USB function should be enabled */
>>>     if (priv->dparam.enable_gpio) {
>>>         gpio_request_one(priv->dparam.enable_gpio, GPIOF_IN, NULL);
>>>         ret = !gpio_get_value(priv->dparam.enable_gpio);
>>>         gpio_free(priv->dparam.enable_gpio);
>>>         if (ret) {
>>>             dev_warn(&pdev->dev,
>>>                  "USB function not selected (GPIO %d)\n",
>>>                  priv->dparam.enable_gpio);
>>>             ret = -ENOTSUPP;
>>>             goto probe_end_mod_exit;
>>>         }
>>>     }
> 
>> I am confusing about the gpio_get_value()...
>> In case of ARM, gpio_get_value() will call gpiod_get_raw_value() finally.
> 
>> gpio_get_value() in arch/arm/include/asm/gpio.h
>>   --> __gpio_get_value() in include/asm-generic/gpio.h
>>    --> gpiod_get_raw_value() in drivers/gpio/gpiolib.c
> 
>> The gpiod_get_raw_value() doesn't care of the GPIO_ACTIVE_{HIGH,LOW}.
>> So, should I add gpiod_is_active_low() or someting in the renesas_usbhs driver?
> 
>    Why do you still think it's active-low?!
> 
>> Or, Do I misunderstand something?
> 
>    Look at the above code again.

Thank you for the comment.
Finally, I understood this. I will not modify the driver and
I just change the "GPIO_ACTIVE_LOW" in the dts file to "GPIO_ACTIVE_HIGH"
because of your previous comment below:

>>     'ret' is non-zero here if gpio_get_value() returned 0, so 0 means "host"
>> and 1 means "gadget", i.e. GPIO is active-high.

Best regards,
Yoshihiro Shimoda

>> Best regards,
>> Yoshihiro Shimoda
> 
> WBR, Sergei
> 

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Sergei Shtylyov]

Hello.

On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>   arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>   1 file changed, 5 insertions(+)

> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index 1698591..4badd0a 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -445,3 +445,8 @@
>   		};
>   	};
>   };
> +
> +&hsusb {
> +	status = "okay";
> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
> +};

    Darn, this also misses the PinMux props... I was going to post v2 of these 
patches but now would need to add that and retest... :-/

WBR, Sergei

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Yoshihiro Shimoda]

Hello.

(2014/10/07 6:38), Sergei Shtylyov wrote:
> Hello.
> 
> On 10/02/2014 12:04 PM, Yoshihiro Shimoda wrote:
> 
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>   arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>   1 file changed, 5 insertions(+)
> 
>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>> index 1698591..4badd0a 100644
>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>> @@ -445,3 +445,8 @@
>>   		};
>>   	};
>>   };
>> +
>> +&hsusb {
>> +	status = "okay";
>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
>> +};
> 
>     Darn, this also misses the PinMux props... I was going to post v2 of these 
> patches but now would need to add that and retest... :-/

I'm sorry for the my delayed work... I will submit v2 patches today.

By the way, I cannot understand that "this also misses the PinMux props".
According to the original "legacy" code in board-lager.c, it uses gpio5 18:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id6be7686caa05334ca8d52df157b373a41d8d9f1

Best regards,
Yoshihiro Shimoda

> WBR, Sergei
> 

Re: [PATCH 2/2] arm: shmobile: lager: enable HS-USB

[Sergei Shtylyov]

Hello.

On 10/7/2014 4:20 AM, Yoshihiro Shimoda wrote:

>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>>    arch/arm/boot/dts/r8a7790-lager.dts |    5 +++++
>>>    1 file changed, 5 insertions(+)

>>> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
>>> index 1698591..4badd0a 100644
>>> --- a/arch/arm/boot/dts/r8a7790-lager.dts
>>> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
>>> @@ -445,3 +445,8 @@
>>>    		};
>>>    	};
>>>    };
>>> +
>>> +&hsusb {
>>> +	status = "okay";
>>> +	renesas,enable-gpio = <&gpio5 18 GPIO_ACTIVE_LOW>;
>>> +};

>>      Darn, this also misses the PinMux props... I was going to post v2 of these
>> patches but now would need to add that and retest... :-/

> I'm sorry for the my delayed work... I will submit v2 patches today.

> By the way, I cannot understand that "this also misses the PinMux props".
> According to the original "legacy" code in board-lager.c, it uses gpio5 18:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id6be7686caa05334ca8d52df157b373a41d8d9f1

    Right. But there are also USB0 pins.

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei