| Message ID | 20240223-j7200-usb-suspend-v3-0-b41c9893a130@bootlin.com |
|---|---|
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset-on-resume | expand |
On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote: > Compatible can be A or B, not A or B or A+B. Remove last option. > A=ti,j721e-usb and B=ti,am64-usb. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > index 95ff9791baea..949f45eb45c2 100644 > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > @@ -11,12 +11,9 @@ maintainers: > > properties: > compatible: > - oneOf: > - - const: ti,j721e-usb > - - const: ti,am64-usb > - - items: > - - const: ti,j721e-usb > - - const: ti,am64-usb Correct, this makes no sense. The devices seem to be compatible though, so I would expect this to actually be: oneOf: - const: ti,j721e-usb - items: - const: ti,am64-usb - const: ti,j721e-usb > + enum: > + - ti,j721e-usb > + - ti,am64-usb > > reg: > maxItems: 1 > > -- > 2.43.2 >
On 2/23/24 7:05 PM, Théo Lebrun wrote: > Add match data support, with one boolean to indicate whether the > hardware resets after a system-wide suspend. If hardware resets, we > force execute ->runtime_resume() at system-wide resume to run the > hardware init sequence. > > No compatible exploits this functionality, just yet. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index 4c8a557e6a6f..f76327566798 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c [...] > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > return 0; > } > > +static int cdns_ti_suspend(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_suspend(dev); > + else Pointless *else* after *return*... > + return 0; > +} > + > +static int cdns_ti_resume(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_resume(dev); > + else Here as well... > + return 0; > +} > + > static const struct dev_pm_ops cdns_ti_pm_ops = { > RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) > + SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume) > }; > > static const struct of_device_id cdns_ti_of_match[] = { MBR, Sergey
Hello Sergey, On Sat Feb 24, 2024 at 10:08 AM CET, Sergei Shtylyov wrote: > On 2/23/24 7:05 PM, Théo Lebrun wrote: > > Add match data support, with one boolean to indicate whether the > > hardware resets after a system-wide suspend. If hardware resets, we > > force execute ->runtime_resume() at system-wide resume to run the > > hardware init sequence. > > > > No compatible exploits this functionality, just yet. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index 4c8a557e6a6f..f76327566798 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > [...] > > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int cdns_ti_suspend(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + > > + if (data->match_data && data->match_data->reset_on_resume) > > + return pm_runtime_force_suspend(dev); > > + else > > Pointless *else* after *return*... Indeed! I used this form explicitely as it reads nicely: "if reset on reset, force suspend, else do nothing". It also prevents the error of adding behavior below the if-statement without seeing that it won't apply to both cases. If you do believe it would make the code better I'll happily change it for the next revision, I do not mind. Thanks for the review Sergey! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello Conor, On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote: > On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote: > > Compatible can be A or B, not A or B or A+B. Remove last option. > > A=ti,j721e-usb and B=ti,am64-usb. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > index 95ff9791baea..949f45eb45c2 100644 > > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > @@ -11,12 +11,9 @@ maintainers: > > > > properties: > > compatible: > > - oneOf: > > - - const: ti,j721e-usb > > - - const: ti,am64-usb > > - - items: > > - - const: ti,j721e-usb > > - - const: ti,am64-usb > > Correct, this makes no sense. The devices seem to be compatible though, > so I would expect this to actually be: > oneOf: > - const: ti,j721e-usb > - items: > - const: ti,am64-usb > - const: ti,j721e-usb I need your help to grasp what that change is supposed to express? Would you mind turning it into english sentences? A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is that a device can either be compat with A or B. But B is compatible with A so you express it as a list of items. If B is compat with A then A is compat with B. Does the order of items matter? I've not applied your proposal to check for dtbs_check but I'd guess it would throw warnings for the single existing upstream DTSI (as of v6.8-rc6) that uses "ti,am64-usb"? See: arch/arm64/boot/dts/ti/k3-am64-main.dtsi. Thanks Conor! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote: > Hello Conor, > > On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote: > > On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote: > > > Compatible can be A or B, not A or B or A+B. Remove last option. > > > A=ti,j721e-usb and B=ti,am64-usb. > > > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > > --- > > > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------ > > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > > index 95ff9791baea..949f45eb45c2 100644 > > > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > > @@ -11,12 +11,9 @@ maintainers: > > > > > > properties: > > > compatible: > > > - oneOf: > > > - - const: ti,j721e-usb > > > - - const: ti,am64-usb > > > - - items: > > > - - const: ti,j721e-usb > > > - - const: ti,am64-usb > > > > Correct, this makes no sense. The devices seem to be compatible though, > > so I would expect this to actually be: > > oneOf: > > - const: ti,j721e-usb > > - items: > > - const: ti,am64-usb > > - const: ti,j721e-usb > > I need your help to grasp what that change is supposed to express? Would > you mind turning it into english sentences? > A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is > that a device can either be compat with A or B. But B is compatible > with A so you express it as a list of items. If B is compat with A then > A is compat with B. Does the order of items matter? The two devices are compatible with each other, based on an inspection of the driver and the existing "A+B" setup. If this was a newly submitted binding, "B" would not get approved because "A+B" allows support without software changes and all that jazz. Your patch says that allowing "A", "B" and "A+B" makes no sense and you suggest removing "A+B". I am agreeing that it makes no sense to allow all 3 of these situations. What I also noticed is other problems with the binding. What should have been "A+B" is actually documented as "B+A", but that doesn't make sense when the originally supported device is "A". Therefore my suggestion was to only allow "A" and "A+B", which is what we would (hopefully) tell you to do were you submitting the am64 support as a new patch today. > I've not applied your proposal to check for dtbs_check but I'd guess it > would throw warnings for the single existing upstream DTSI (as of > v6.8-rc6) that uses "ti,am64-usb"? See: > arch/arm64/boot/dts/ti/k3-am64-main.dtsi. Yeah, it would but it's not as if that cannot be changed. There's no concerns here about backwards compatibility here, right?
Hello, On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote: > On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote: > > Hello Conor, > > > > On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote: > > > On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote: > > > > Compatible can be A or B, not A or B or A+B. Remove last option. > > > > A=ti,j721e-usb and B=ti,am64-usb. > > > > > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > > > --- > > > > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------ > > > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > > > index 95ff9791baea..949f45eb45c2 100644 > > > > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > > > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > > > > @@ -11,12 +11,9 @@ maintainers: > > > > > > > > properties: > > > > compatible: > > > > - oneOf: > > > > - - const: ti,j721e-usb > > > > - - const: ti,am64-usb > > > > - - items: > > > > - - const: ti,j721e-usb > > > > - - const: ti,am64-usb > > > > > > Correct, this makes no sense. The devices seem to be compatible though, > > > so I would expect this to actually be: > > > oneOf: > > > - const: ti,j721e-usb > > > - items: > > > - const: ti,am64-usb > > > - const: ti,j721e-usb > > > > I need your help to grasp what that change is supposed to express? Would > > you mind turning it into english sentences? > > A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is > > that a device can either be compat with A or B. But B is compatible > > with A so you express it as a list of items. If B is compat with A then > > A is compat with B. Does the order of items matter? > > The two devices are compatible with each other, based on an inspection of > the driver and the existing "A+B" setup. If this was a newly submitted > binding, "B" would not get approved because "A+B" allows support without > software changes and all that jazz. > > Your patch says that allowing "A", "B" and "A+B" makes no sense and you > suggest removing "A+B". I am agreeing that it makes no sense to allow > all 3 of these situations. > > What I also noticed is other problems with the binding. What should have > been "A+B" is actually documented as "B+A", but that doesn't make sense > when the originally supported device is "A". > > Therefore my suggestion was to only allow "A" and "A+B", which is what > we would (hopefully) tell you to do were you submitting the am64 support > as a new patch today. Thank you for the in-depth explanation! It makes much more sense now, especially the handling of historic stuff that ideally wouldn't have been done this way but that won't be changed from now on. > > I've not applied your proposal to check for dtbs_check but I'd guess it > > would throw warnings for the single existing upstream DTSI (as of > > v6.8-rc6) that uses "ti,am64-usb"? See: > > arch/arm64/boot/dts/ti/k3-am64-main.dtsi. > > Yeah, it would but it's not as if that cannot be changed. There's no > concerns here about backwards compatibility here, right? I'm not involved in the maintenance of this platform so I do not believe I should be answering this question. I asked the question because I taught there always were concerns of backwards-compat when it comes to DT and dt-bindings (in the best of all possible worlds). K3 maintainers are already in cc. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 26/02/24 20:05, Théo Lebrun wrote: > Hello, > > On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote: >> On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote: >>> Hello Conor, >>> >>> On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote: >>>> On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote: >>>>> Compatible can be A or B, not A or B or A+B. Remove last option. >>>>> A=ti,j721e-usb and B=ti,am64-usb. >>>>> >>>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>>>> --- >>>>> Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------ >>>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml >>>>> index 95ff9791baea..949f45eb45c2 100644 >>>>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml >>>>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml >>>>> @@ -11,12 +11,9 @@ maintainers: >>>>> >>>>> properties: >>>>> compatible: >>>>> - oneOf: >>>>> - - const: ti,j721e-usb >>>>> - - const: ti,am64-usb >>>>> - - items: >>>>> - - const: ti,j721e-usb >>>>> - - const: ti,am64-usb >>>> >>>> Correct, this makes no sense. The devices seem to be compatible though, >>>> so I would expect this to actually be: >>>> oneOf: >>>> - const: ti,j721e-usb >>>> - items: >>>> - const: ti,am64-usb >>>> - const: ti,j721e-usb >>> >>> I need your help to grasp what that change is supposed to express? Would >>> you mind turning it into english sentences? >>> A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is >>> that a device can either be compat with A or B. But B is compatible >>> with A so you express it as a list of items. If B is compat with A then >>> A is compat with B. Does the order of items matter? >> >> The two devices are compatible with each other, based on an inspection of >> the driver and the existing "A+B" setup. If this was a newly submitted >> binding, "B" would not get approved because "A+B" allows support without >> software changes and all that jazz. >> >> Your patch says that allowing "A", "B" and "A+B" makes no sense and you >> suggest removing "A+B". I am agreeing that it makes no sense to allow >> all 3 of these situations. >> >> What I also noticed is other problems with the binding. What should have >> been "A+B" is actually documented as "B+A", but that doesn't make sense >> when the originally supported device is "A". >> >> Therefore my suggestion was to only allow "A" and "A+B", which is what >> we would (hopefully) tell you to do were you submitting the am64 support >> as a new patch today. > > Thank you for the in-depth explanation! It makes much more sense now, > especially the handling of historic stuff that ideally wouldn't have > been done this way but that won't be changed from now on. > IIRC, idea behind adding new compatible for AM64 even though register map is very much compatible is just being future proof as AM64 and J721e belong to different product groups and thus have differences wrt SoC level integration etc which may need SoC specific handling later on. I don't see any DT (now or in the past) using compatible = B,A or compatible = A,B So do we really need A+B to be supported by binding? Also, note that AM64 SoC support was added long after J721e. So ideally should be B+A if at all we need a fallback compatible. Regards Vignesh
On Tue, Feb 27, 2024 at 09:54:30AM +0530, Vignesh Raghavendra wrote: > On 26/02/24 20:05, Théo Lebrun wrote: > > On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote: > >> On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote: > >>> Hello Conor, > >>> > >>> On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote: > >>>> On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote: > >>>>> Compatible can be A or B, not A or B or A+B. Remove last option. > >>>>> A=ti,j721e-usb and B=ti,am64-usb. > >>>>> > >>>>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > >>>>> --- > >>>>> Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------ > >>>>> 1 file changed, 3 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > >>>>> index 95ff9791baea..949f45eb45c2 100644 > >>>>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > >>>>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml > >>>>> @@ -11,12 +11,9 @@ maintainers: > >>>>> > >>>>> properties: > >>>>> compatible: > >>>>> - oneOf: > >>>>> - - const: ti,j721e-usb > >>>>> - - const: ti,am64-usb > >>>>> - - items: > >>>>> - - const: ti,j721e-usb > >>>>> - - const: ti,am64-usb > >>>> > >>>> Correct, this makes no sense. The devices seem to be compatible though, > >>>> so I would expect this to actually be: > >>>> oneOf: > >>>> - const: ti,j721e-usb > >>>> - items: > >>>> - const: ti,am64-usb > >>>> - const: ti,j721e-usb > >>> > >>> I need your help to grasp what that change is supposed to express? Would > >>> you mind turning it into english sentences? > >>> A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is > >>> that a device can either be compat with A or B. But B is compatible > >>> with A so you express it as a list of items. If B is compat with A then > >>> A is compat with B. Does the order of items matter? > >> > >> The two devices are compatible with each other, based on an inspection of > >> the driver and the existing "A+B" setup. If this was a newly submitted > >> binding, "B" would not get approved because "A+B" allows support without > >> software changes and all that jazz. > >> > >> Your patch says that allowing "A", "B" and "A+B" makes no sense and you > >> suggest removing "A+B". I am agreeing that it makes no sense to allow > >> all 3 of these situations. > >> > >> What I also noticed is other problems with the binding. What should have > >> been "A+B" is actually documented as "B+A", but that doesn't make sense > >> when the originally supported device is "A". This A and B stuff confused me, I should just have used the actual compatibles. I meant | What should have been "B+A" is actually documented as "A+B", but that | doesn't make sense when the originally supported device is "A" > >> > >> Therefore my suggestion was to only allow "A" and "A+B", which is what > >> we would (hopefully) tell you to do were you submitting the am64 support > >> as a new patch today. > > > > Thank you for the in-depth explanation! It makes much more sense now, > > especially the handling of historic stuff that ideally wouldn't have > > been done this way but that won't be changed from now on. > > > > IIRC, idea behind adding new compatible for AM64 even though register > map is very much compatible is just being future proof as AM64 and J721e > belong to different product groups and thus have differences wrt SoC > level integration etc which may need SoC specific handling later on. That is fine, I don't think anyone here is disputing a soc-specific compatible existing for this device. > Also, note that AM64 SoC support was added long after J721e. So ideally > should be B+A if at all we need a fallback compatible. Correct, I accidentally wrote "A+B", but you can see that that conflicted with the actual example I had given above. > I don't see any DT (now or in the past) using > > compatible = B,A or compatible = A,B > > So do we really need A+B to be supported by binding? Given the mistake, I am going to take this as meaning should the fallback be supported. My take is that if we are going to remove something, it should be "ti,am64-usb" isolation that should go. The devicetrees can be update without concerns about compatibility. Cheers, Conor.
On 2/26/24 1:13 PM, Théo Lebrun wrote: [...] >>> Add match data support, with one boolean to indicate whether the >>> hardware resets after a system-wide suspend. If hardware resets, we >>> force execute ->runtime_resume() at system-wide resume to run the >>> hardware init sequence. >>> >>> No compatible exploits this functionality, just yet. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c >>> index 4c8a557e6a6f..f76327566798 100644 >>> --- a/drivers/usb/cdns3/cdns3-ti.c >>> +++ b/drivers/usb/cdns3/cdns3-ti.c >> [...] >>> @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) >>> return 0; >>> } >>> >>> +static int cdns_ti_suspend(struct device *dev) >>> +{ >>> + struct cdns_ti *data = dev_get_drvdata(dev); >>> + >>> + if (data->match_data && data->match_data->reset_on_resume) >>> + return pm_runtime_force_suspend(dev); >>> + else >> >> Pointless *else* after *return*... > > Indeed! I used this form explicitely as it reads nicely: "if reset on > reset, force suspend, else do nothing". It also prevents the error of s/reset/resume/ here? :-) > adding behavior below the if-statement without seeing that it won't > apply to both cases. You were going to add stuff after the final *return*? :-) > If you do believe it would make the code better I'll happily change it > for the next revision, I do not mind. Up to you! This is a thing people usually complain about when reviewing patches. I even thought checkpatch.pl would complain as well, but it didn't... :-) > Thanks for the review Sergey! > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com MBR, Swrgey
Hi, Here is a new revision of the J7200 USB suspend fix. It is currently broken on the platform, leading to a kernel panic at resume. Patches are tested on a J7200 evaluation board, both s2idle and suspend-to-RAM. Previous discussion showed that runtime PM was not to be deleted. Indeed, we cannot guarantee PM domains will stay active if we don't register to runtime PM. We therefore rely fully on runtime PM and put our init sequence previously done in the probe in our ->runtime_resume(). We then do the necessary to get it called at system-wide resume for the J7200 platform. Have a nice day, Théo -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v3: - dt-bindings: use an enum to list compatibles instead of the previous odd construct. This is done in a separate patch from the one adding J7200 compatible. - dt-bindings: dropped Acked-by Conor as the changes were modified a lot. - Add runtime PM back. Put the init sequence in ->runtime_resume(). It gets called at probe for all compatibles and at resume for J7200. - Introduce a cdns_ti_match_data struct rather than rely on compatible from code. - Reorder code changes. Add infrastructure based on match data THEN add compatible and its match data. - DTSI: use only J7200 compatible rather than both J7200 then J721E. - Link to v2: https://lore.kernel.org/r/20231120-j7200-usb-suspend-v2-0-038c7e4a3df4@bootlin.com Changes in v2: - Remove runtime PM from cdns3-ti; it brings nothing. That means our cdns3-ti suspend/resume patch is simpler; there is no need to handle runtime PM at suspend/resume. - Do not add cdns3 host role suspend/resume callbacks; they are not needed as core detects reset on resume & calls cdns_drd_host_on when needed. - cdns3-ti: Move usb2_refclk_rate_code assignment closer to the value computation. - cdns3/host.c: do not pass XHCI_SUSPEND_RESUME_CLKS quirk to xHCI; it is unneeded on our platform. - Link to v1: https://lore.kernel.org/r/20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com --- Théo Lebrun (8): dt-bindings: usb: ti,j721e-usb: drop useless compatible list dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible usb: cdns3-ti: move reg writes from probe into ->runtime_resume() usb: cdns3-ti: support reset-on-resume behavior usb: cdns3-ti: pass auxdata from match data to of_platform_populate() usb: cdns3: add quirk to platform data for reset-on-resume usb: cdns3-ti: add J7200 support with reset-on-resume behavior arm64: dts: ti: k3-j7200: use J7200-specific USB compatible .../devicetree/bindings/usb/ti,j721e-usb.yaml | 10 +- arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +- drivers/usb/cdns3/cdns3-ti.c | 125 ++++++++++++++++----- drivers/usb/cdns3/core.h | 1 + drivers/usb/cdns3/host.c | 3 + 5 files changed, 104 insertions(+), 37 deletions(-) --- base-commit: 1871c27e3539e5b812d50ec6ccad7567ec5414f2 change-id: 20231113-j7200-usb-suspend-2a47f2281e04 Best regards,