[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Vasily Khoruzhick]

ohci-hcd casts priv_data pointer to (ohci_t *), thus it must be
the first member in private data struct.

Fixes 831cc98b1 ("usb: sunxi: Simplify ccm reg base code")

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/usb/host/ohci-sunxi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
index e13f6ec9a4..db6f438275 100644
--- a/drivers/usb/host/ohci-sunxi.c
+++ b/drivers/usb/host/ohci-sunxi.c
@@ -33,9 +33,9 @@ struct ohci_sunxi_cfg {
 };
 
 struct ohci_sunxi_priv {
+	ohci_t ohci;
 	struct sunxi_ccm_reg *ccm;
 	u32 *reset0_cfg;
-	ohci_t ohci;
 	int ahb_gate_mask; /* Mask of ahb_gate0 clk gate bits for this hcd */
 	int usb_gate_mask; /* Mask of usb_clk_cfg clk gate bits for this hcd */
 	struct phy phy;
-- 
2.17.1

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Marek Vasut]

On 06/17/2018 06:13 PM, Vasily Khoruzhick wrote:
> ohci-hcd casts priv_data pointer to (ohci_t *), thus it must be
> the first member in private data struct.
> 
> Fixes 831cc98b1 ("usb: sunxi: Simplify ccm reg base code")
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Sigh, I really wonder how (or if at all!) the original series was
tested. And then I get flak for scrutinizing patches, right ...

Thanks for the fix, applied.

> ---
>  drivers/usb/host/ohci-sunxi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ohci-sunxi.c b/drivers/usb/host/ohci-sunxi.c
> index e13f6ec9a4..db6f438275 100644
> --- a/drivers/usb/host/ohci-sunxi.c
> +++ b/drivers/usb/host/ohci-sunxi.c
> @@ -33,9 +33,9 @@ struct ohci_sunxi_cfg {
>  };
>  
>  struct ohci_sunxi_priv {
> +	ohci_t ohci;
>  	struct sunxi_ccm_reg *ccm;
>  	u32 *reset0_cfg;
> -	ohci_t ohci;
>  	int ahb_gate_mask; /* Mask of ahb_gate0 clk gate bits for this hcd */
>  	int usb_gate_mask; /* Mask of usb_clk_cfg clk gate bits for this hcd */
>  	struct phy phy;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Jagan Teki]

On Mon, Jun 18, 2018 at 6:44 AM, Marek Vasut <marex@denx.de> wrote:
> On 06/17/2018 06:13 PM, Vasily Khoruzhick wrote:
>> ohci-hcd casts priv_data pointer to (ohci_t *), thus it must be
>> the first member in private data struct.
>>
>> Fixes 831cc98b1 ("usb: sunxi: Simplify ccm reg base code")
>>
>> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>
> Sigh, I really wonder how (or if at all!) the original series was
> tested. And then I get flak for scrutinizing patches, right ...

APAIK, I did basic sanity with possible tests.

But one thing for sure is, you should have to wait for sometime to
apply this patch. Applying fast (that to during weekend) making
reviewers or maintainers not giving enough room to work.

Jagan.

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Marek Vasut]

On 06/18/2018 08:15 AM, Jagan Teki wrote:
> On Mon, Jun 18, 2018 at 6:44 AM, Marek Vasut <marex@denx.de> wrote:
>> On 06/17/2018 06:13 PM, Vasily Khoruzhick wrote:
>>> ohci-hcd casts priv_data pointer to (ohci_t *), thus it must be
>>> the first member in private data struct.
>>>
>>> Fixes 831cc98b1 ("usb: sunxi: Simplify ccm reg base code")
>>>
>>> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>>
>> Sigh, I really wonder how (or if at all!) the original series was
>> tested. And then I get flak for scrutinizing patches, right ...
> 
> APAIK, I did basic sanity with possible tests.

From what I see in the other thread, the USB never worked with the
series. If the controller returns 0s as EHCI version, something is
obviously broken and I don't even understand how that could be an
acceptable positive test result. The USB HCD version register can NOT
EVER contain zeroes as per the specification. I am really unhappy here.

> But one thing for sure is, you should have to wait for sometime to
> apply this patch. Applying fast (that to during weekend) making
> reviewers or maintainers not giving enough room to work.

Putting my USB maintainer hat on, I am quite sure I can evaluate such a
simple yet critical bugfix, if only by spending those two minutes to
look at the code in ohci_register() .

And since you complain about the rate patches get in again, let me
remind you how much flak I got for taking my time reviewing your series
and not applying it right away. Now I am getting flak for applying stuff
too quickly instead. You know, maybe if you spend more time testing the
patches you send thoroughly instead of lecturing people on the MLs, we
wouldn't be having this conversation.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Jagan Teki]

On Mon, Jun 18, 2018 at 12:49 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/18/2018 08:15 AM, Jagan Teki wrote:
>> On Mon, Jun 18, 2018 at 6:44 AM, Marek Vasut <marex@denx.de> wrote:
>>> On 06/17/2018 06:13 PM, Vasily Khoruzhick wrote:
>>>> ohci-hcd casts priv_data pointer to (ohci_t *), thus it must be
>>>> the first member in private data struct.
>>>>
>>>> Fixes 831cc98b1 ("usb: sunxi: Simplify ccm reg base code")
>>>>
>>>> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>>>
>>> Sigh, I really wonder how (or if at all!) the original series was
>>> tested. And then I get flak for scrutinizing patches, right ...
>>
>> APAIK, I did basic sanity with possible tests.
>
> >From what I see in the other thread, the USB never worked with the
> series. If the controller returns 0s as EHCI version, something is
> obviously broken and I don't even understand how that could be an
> acceptable positive test result. The USB HCD version register can NOT
> EVER contain zeroes as per the specification. I am really unhappy here.

This is untrue, the controller returns 0's only when single node
enabled not with both ehci0 and echi1 atleast on BPI-M64. and we do
enable both controller on dts even with other boards too. having
single node enablement is not a proper test or not with my dts
atleast.

>
>> But one thing for sure is, you should have to wait for sometime to
>> apply this patch. Applying fast (that to during weekend) making
>> reviewers or maintainers not giving enough room to work.
>
> Putting my USB maintainer hat on, I am quite sure I can evaluate such a
> simple yet critical bugfix, if only by spending those two minutes to
> look at the code in ohci_register() .
>
> And since you complain about the rate patches get in again, let me
> remind you how much flak I got for taking my time reviewing your series
> and not applying it right away. Now I am getting flak for applying stuff
> too quickly instead. You know, maybe if you spend more time testing the
> patches you send thoroughly instead of lecturing people on the MLs, we
> wouldn't be having this conversation.

Again we do testing based on our usage scenario's and we're done with
that and good to go. And giving room for 3 releases time and sending
till v10 expecting other boards or usage scenario's can be verified by
other people in the ML. for your words on 'lecturing people on the
MLs' we are here to work like other developers in ML not to give
lecture like in training organizations, better be clear before framing
your words.

Jagan.

-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Vasily Khoruzhick]

On Mon, Jun 18, 2018 at 12:39 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> >From what I see in the other thread, the USB never worked with the
>> series. If the controller returns 0s as EHCI version, something is
>> obviously broken and I don't even understand how that could be an
>> acceptable positive test result. The USB HCD version register can NOT
>> EVER contain zeroes as per the specification. I am really unhappy here.
>
> This is untrue, the controller returns 0's only when single node
> enabled not with both ehci0 and echi1 atleast on BPI-M64. and we do
> enable both controller on dts even with other boards too. having
> single node enablement is not a proper test or not with my dts
> atleast.

Clock for EHCI0 or OHCI0 was never enabled on your series, so reading
USB HCD version for EHCI0/OHCI0 always returned 0 and you should have
seen 0.0 as version for EHCI0 and OHCI0.

With both nodes enabled you had only EHCI1/OHCI1 functional.

[U-Boot] [PATCH] usb: sunxi: ohci: make ohci_t the first member in private data

[Tom Rini]

On Mon, Jun 18, 2018 at 11:45:23AM +0530, Jagan Teki wrote:
> On Mon, Jun 18, 2018 at 6:44 AM, Marek Vasut <marex@denx.de> wrote:
> > On 06/17/2018 06:13 PM, Vasily Khoruzhick wrote:
> >> ohci-hcd casts priv_data pointer to (ohci_t *), thus it must be
> >> the first member in private data struct.
> >>
> >> Fixes 831cc98b1 ("usb: sunxi: Simplify ccm reg base code")
> >>
> >> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> >
> > Sigh, I really wonder how (or if at all!) the original series was
> > tested. And then I get flak for scrutinizing patches, right ...
> 
> APAIK, I did basic sanity with possible tests.
> 
> But one thing for sure is, you should have to wait for sometime to
> apply this patch. Applying fast (that to during weekend) making
> reviewers or maintainers not giving enough room to work.

It is a custodian's job to pick up what they see as obviously correct
and/or important fixes ASAP.  While things are working in your test
cases, there are clearly other hardware combinations that are not
working and that this corrects.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180618/4164b6c5/attachment.sig>