diff mbox series

[13/13] usb: ehci-mx6: Improve the bind function

Message ID 20200916125705.4341-14-peng.fan@nxp.com
State Deferred
Delegated to: Tom Rini
Headers show
Series ehci-mx6: update and fix | expand

Commit Message

Peng Fan Sept. 16, 2020, 12:57 p.m. UTC 
From: Ye Li <ye.li@nxp.com>

To avoid calling devfdt_get_addr_index in bind, which introduces
much overhead, checks the req_seq and only call the devfdt_get_addr_index
when the req_seq (usb alias) is not set in DTS

Signed-off-by: Ye Li <ye.li@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/usb/host/ehci-mx6.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Marek Vasut Sept. 16, 2020, 1:46 p.m. UTC | #1 
On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
[...]
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice *dev)
>  	 * the driver is fully converted to DT probing.
>  	 */
>  	u32 controller_spacing;
> -	if (IS_ENABLED(CONFIG_MX6))
> -		controller_spacing = 0x200;
> -	else
> -		controller_spacing = 0x10000;
> -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>  
> -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
> +	if (dev->req_seq == -1) {
> +		if (IS_ENABLED(CONFIG_MX6))
> +			controller_spacing = 0x200;
> +		else
> +			controller_spacing = 0x10000;
> +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);

Can we get rid of the whole req_seq stuff ?
It is a workaround, see
501547cec1 ("usb: ehci-mx6: Fix bus enumeration for DM case")
Peng Fan Sept. 16, 2020, 1:56 p.m. UTC | #2 
Hi Marek,

> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
> 
> On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
> [...]
> > +++ b/drivers/usb/host/ehci-mx6.c
> > @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice *dev)
> >  	 * the driver is fully converted to DT probing.
> >  	 */
> >  	u32 controller_spacing;
> > -	if (IS_ENABLED(CONFIG_MX6))
> > -		controller_spacing = 0x200;
> > -	else
> > -		controller_spacing = 0x10000;
> > -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
> >
> > -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
> > +	if (dev->req_seq == -1) {
> > +		if (IS_ENABLED(CONFIG_MX6))
> > +			controller_spacing = 0x200;
> > +		else
> > +			controller_spacing = 0x10000;
> > +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
> 
> Can we get rid of the whole req_seq stuff ?

Could the restructure be done after the patchset? Or you need NXP
to restructure the driver, then upstream NXP production ready
patches?

If restructure first, there will be lots conflicts when I pick downstream
patches, and error prone.

Thanks,
Peng.

> It is a workaround, see
> 501547cec1 ("usb: ehci-mx6: Fix bus enumeration for DM case")
Marek Vasut Sept. 16, 2020, 2:16 p.m. UTC | #3 
On 9/16/20 3:56 PM, Peng Fan wrote:
> Hi Marek,

Hi,

>> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
>>
>> On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
>> [...]
>>> +++ b/drivers/usb/host/ehci-mx6.c
>>> @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice *dev)
>>>  	 * the driver is fully converted to DT probing.
>>>  	 */
>>>  	u32 controller_spacing;
>>> -	if (IS_ENABLED(CONFIG_MX6))
>>> -		controller_spacing = 0x200;
>>> -	else
>>> -		controller_spacing = 0x10000;
>>> -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>>>
>>> -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
>>> +	if (dev->req_seq == -1) {
>>> +		if (IS_ENABLED(CONFIG_MX6))
>>> +			controller_spacing = 0x200;
>>> +		else
>>> +			controller_spacing = 0x10000;
>>> +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>>
>> Can we get rid of the whole req_seq stuff ?
> 
> Could the restructure be done after the patchset? Or you need NXP
> to restructure the driver, then upstream NXP production ready
> patches?
> 
> If restructure first, there will be lots conflicts when I pick downstream
> patches, and error prone.

Can you prepare an RFC patchset for the restructuring on top of this
one, so you can bisect breakage caused by the restructuring, and post
those patches ? Then you will get your production-ready patches in
without much changes and also there will be the long-overdue cleanup on
the ML.

I really want NXP to do the cleanup, because the driver is becoming real
bad and it is piling up a lot of unrelated code in it. I don't care
about the order in which the patches go in though.

Does that work ?
Peng Fan Sept. 27, 2020, 2:38 a.m. UTC | #4 
Hi Marek,

> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
> 
> On 9/16/20 3:56 PM, Peng Fan wrote:
> > Hi Marek,
> 
> Hi,
> 
> >> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
> >>
> >> On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
> >> [...]
> >>> +++ b/drivers/usb/host/ehci-mx6.c
> >>> @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice *dev)
> >>>  	 * the driver is fully converted to DT probing.
> >>>  	 */
> >>>  	u32 controller_spacing;
> >>> -	if (IS_ENABLED(CONFIG_MX6))
> >>> -		controller_spacing = 0x200;
> >>> -	else
> >>> -		controller_spacing = 0x10000;
> >>> -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
> >>>
> >>> -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
> >>> +	if (dev->req_seq == -1) {
> >>> +		if (IS_ENABLED(CONFIG_MX6))
> >>> +			controller_spacing = 0x200;
> >>> +		else
> >>> +			controller_spacing = 0x10000;
> >>> +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
> >>
> >> Can we get rid of the whole req_seq stuff ?
> >
> > Could the restructure be done after the patchset? Or you need NXP to
> > restructure the driver, then upstream NXP production ready patches?
> >
> > If restructure first, there will be lots conflicts when I pick
> > downstream patches, and error prone.
> 
> Can you prepare an RFC patchset for the restructuring on top of this one, so
> you can bisect breakage caused by the restructuring, and post those patches ?
> Then you will get your production-ready patches in without much changes
> and also there will be the long-overdue cleanup on the ML.
> 
> I really want NXP to do the cleanup, because the driver is becoming real bad
> and it is piling up a lot of unrelated code in it. I don't care about the order in
> which the patches go in though.
> 
> Does that work ?

Ok, so you agree that we do a cleanup patch based on the pachset, but you
wanna to see NXP has start the real work before considering take the
patchset, if I understand correct.

Then let me reserve time or ask other NXP guys doing this.

Thanks,
Peng.
Marek Vasut Sept. 27, 2020, 2:44 p.m. UTC | #5 
On 9/27/20 4:38 AM, Peng Fan wrote:
> Hi Marek,
> 
>> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
>>
>> On 9/16/20 3:56 PM, Peng Fan wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>>> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
>>>>
>>>> On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
>>>> [...]
>>>>> +++ b/drivers/usb/host/ehci-mx6.c
>>>>> @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice *dev)
>>>>>  	 * the driver is fully converted to DT probing.
>>>>>  	 */
>>>>>  	u32 controller_spacing;
>>>>> -	if (IS_ENABLED(CONFIG_MX6))
>>>>> -		controller_spacing = 0x200;
>>>>> -	else
>>>>> -		controller_spacing = 0x10000;
>>>>> -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>>>>>
>>>>> -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
>>>>> +	if (dev->req_seq == -1) {
>>>>> +		if (IS_ENABLED(CONFIG_MX6))
>>>>> +			controller_spacing = 0x200;
>>>>> +		else
>>>>> +			controller_spacing = 0x10000;
>>>>> +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>>>>
>>>> Can we get rid of the whole req_seq stuff ?
>>>
>>> Could the restructure be done after the patchset? Or you need NXP to
>>> restructure the driver, then upstream NXP production ready patches?
>>>
>>> If restructure first, there will be lots conflicts when I pick
>>> downstream patches, and error prone.
>>
>> Can you prepare an RFC patchset for the restructuring on top of this one, so
>> you can bisect breakage caused by the restructuring, and post those patches ?
>> Then you will get your production-ready patches in without much changes
>> and also there will be the long-overdue cleanup on the ML.
>>
>> I really want NXP to do the cleanup, because the driver is becoming real bad
>> and it is piling up a lot of unrelated code in it. I don't care about the order in
>> which the patches go in though.
>>
>> Does that work ?
> 
> Ok, so you agree that we do a cleanup patch based on the pachset, but you
> wanna to see NXP has start the real work before considering take the
> patchset, if I understand correct.
> 
> Then let me reserve time or ask other NXP guys doing this.

I just want to see that cleanup happen, yes, it is already way overdue.
Peng Fan Oct. 15, 2020, 9:45 a.m. UTC | #6 
Marek,

> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
> 
> On 9/27/20 4:38 AM, Peng Fan wrote:
> > Hi Marek,
> >
> >> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
> >>
> >> On 9/16/20 3:56 PM, Peng Fan wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>>> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
> >>>>
> >>>> On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
> >>>> [...]
> >>>>> +++ b/drivers/usb/host/ehci-mx6.c
> >>>>> @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice
> *dev)
> >>>>>  	 * the driver is fully converted to DT probing.
> >>>>>  	 */
> >>>>>  	u32 controller_spacing;
> >>>>> -	if (IS_ENABLED(CONFIG_MX6))
> >>>>> -		controller_spacing = 0x200;
> >>>>> -	else
> >>>>> -		controller_spacing = 0x10000;
> >>>>> -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
> >>>>>
> >>>>> -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
> >>>>> +	if (dev->req_seq == -1) {
> >>>>> +		if (IS_ENABLED(CONFIG_MX6))
> >>>>> +			controller_spacing = 0x200;
> >>>>> +		else
> >>>>> +			controller_spacing = 0x10000;
> >>>>> +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
> >>>>
> >>>> Can we get rid of the whole req_seq stuff ?
> >>>
> >>> Could the restructure be done after the patchset? Or you need NXP to
> >>> restructure the driver, then upstream NXP production ready patches?
> >>>
> >>> If restructure first, there will be lots conflicts when I pick
> >>> downstream patches, and error prone.
> >>
> >> Can you prepare an RFC patchset for the restructuring on top of this
> >> one, so you can bisect breakage caused by the restructuring, and post
> those patches ?
> >> Then you will get your production-ready patches in without much
> >> changes and also there will be the long-overdue cleanup on the ML.
> >>
> >> I really want NXP to do the cleanup, because the driver is becoming
> >> real bad and it is piling up a lot of unrelated code in it. I don't
> >> care about the order in which the patches go in though.
> >>
> >> Does that work ?
> >
> > Ok, so you agree that we do a cleanup patch based on the pachset, but
> > you wanna to see NXP has start the real work before considering take
> > the patchset, if I understand correct.
> >
> > Then let me reserve time or ask other NXP guys doing this.
> 
> I just want to see that cleanup happen, yes, it is already way overdue.

I have posted the dts patch to Linux Community,
https://patchwork.kernel.org/project/linux-arm-kernel/patch/
1602638861-20278-1-git-send-email-peng.fan@nxp.com/

After that gets into Shawn's tree, I will sync with U-Boot.

So before that, could you please drop this patch 13 and only
pick up the other patches? Or you need to wait the dts sync and
then remove the bind function in ehci-mx6.c?

Thanks,
Peng.
Marek Vasut Oct. 15, 2020, 12:32 p.m. UTC | #7 
On 10/15/20 11:45 AM, Peng Fan wrote:
Hi,

[...]

>>>>>> Subject: Re: [PATCH 13/13] usb: ehci-mx6: Improve the bind function
>>>>>>
>>>>>> On 9/16/20 2:57 PM, peng.fan@nxp.com wrote:
>>>>>> [...]
>>>>>>> +++ b/drivers/usb/host/ehci-mx6.c
>>>>>>> @@ -735,13 +735,16 @@ static int ehci_usb_bind(struct udevice
>> *dev)
>>>>>>>  	 * the driver is fully converted to DT probing.
>>>>>>>  	 */
>>>>>>>  	u32 controller_spacing;
>>>>>>> -	if (IS_ENABLED(CONFIG_MX6))
>>>>>>> -		controller_spacing = 0x200;
>>>>>>> -	else
>>>>>>> -		controller_spacing = 0x10000;
>>>>>>> -	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>>>>>>>
>>>>>>> -	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
>>>>>>> +	if (dev->req_seq == -1) {
>>>>>>> +		if (IS_ENABLED(CONFIG_MX6))
>>>>>>> +			controller_spacing = 0x200;
>>>>>>> +		else
>>>>>>> +			controller_spacing = 0x10000;
>>>>>>> +		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
>>>>>>
>>>>>> Can we get rid of the whole req_seq stuff ?
>>>>>
>>>>> Could the restructure be done after the patchset? Or you need NXP to
>>>>> restructure the driver, then upstream NXP production ready patches?
>>>>>
>>>>> If restructure first, there will be lots conflicts when I pick
>>>>> downstream patches, and error prone.
>>>>
>>>> Can you prepare an RFC patchset for the restructuring on top of this
>>>> one, so you can bisect breakage caused by the restructuring, and post
>> those patches ?
>>>> Then you will get your production-ready patches in without much
>>>> changes and also there will be the long-overdue cleanup on the ML.
>>>>
>>>> I really want NXP to do the cleanup, because the driver is becoming
>>>> real bad and it is piling up a lot of unrelated code in it. I don't
>>>> care about the order in which the patches go in though.
>>>>
>>>> Does that work ?
>>>
>>> Ok, so you agree that we do a cleanup patch based on the pachset, but
>>> you wanna to see NXP has start the real work before considering take
>>> the patchset, if I understand correct.
>>>
>>> Then let me reserve time or ask other NXP guys doing this.
>>
>> I just want to see that cleanup happen, yes, it is already way overdue.
> 
> I have posted the dts patch to Linux Community,
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/
> 1602638861-20278-1-git-send-email-peng.fan@nxp.com/
> 
> After that gets into Shawn's tree, I will sync with U-Boot.
>
> So before that, could you please drop this patch 13 and only
> pick up the other patches? Or you need to wait the dts sync and
> then remove the bind function in ehci-mx6.c?

I am waiting for a cleanup of ehci-mx6.c patch series.

Also, setting any aliases in DT does not fix the problem described in
501547cec1 ("usb: ehci-mx6: Fix bus enumeration for DM case")
does it ?
diff mbox series

Patch

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index 080bde71d3..20617850f3 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -735,13 +735,16 @@  static int ehci_usb_bind(struct udevice *dev)
 	 * the driver is fully converted to DT probing.
 	 */
 	u32 controller_spacing;
-	if (IS_ENABLED(CONFIG_MX6))
-		controller_spacing = 0x200;
-	else
-		controller_spacing = 0x10000;
-	fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
 
-	dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
+	if (dev->req_seq == -1) {
+		if (IS_ENABLED(CONFIG_MX6))
+			controller_spacing = 0x200;
+		else
+			controller_spacing = 0x10000;
+		fdt_addr_t addr = devfdt_get_addr_index(dev, 0);
+
+		dev->req_seq = (addr - USB_BASE_ADDR) / controller_spacing;
+	}
 
 	return 0;
 }