Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Dhruva Gole]

Hi all,

On Jan 04, 2024 at 18:15:36 +0200, Roger Quadros wrote:
> 
> 
> On 04/01/2024 17:47, Jai Luthra wrote:
> > Hi Javier,
> > 
> > On Jan 04, 2024 at 19:50:05 +0530, Jai Luthra wrote:
> >> Hi Javier, Greg,
> >>
> >> On Dec 14, 2023 at 17:29:08 +0100, Javier Carrasco wrote:
> >>> This series extends the patch update mechanism to support the tps6598x.
> >>>
> >>> Currently there is only support for the tps25750 part and some
> >>> conditional clauses are used to make a special case out of it. Now that
> >>> different parts support patch updates, a more general approach is
> >>> proposed.
> >>>
> >>> The update mechanism differs from the one required by tps25750 and it is
> >>> explained in the commit message as a summary of the application note in
> >>> that respect.
> >>>
> >>> Note that the series makes use of the TPS_SETUP_MS introduced in
> >>> commit 6a4d4a27f986 ("usb: typec: tps6598x: add reset gpio support"),
> >>> which is currently available in usb-next / usb-testing.
> >>>
> >>> A TPS65987D has been used to test this functionality with positive
> >>> results.
> >>>
> >>> Signed-off-by: Javier Carrasco <javier.carrasco@xxxxxxxxxxxxxx>
> >>> ---
> >>> Changes in v2:
> >>> - Remove probe defeferring mechanism and expect the firmware to be
> >>>   available (Heikki Krogerus).
> >>> - Link to v1: 
> >>> https://lore.kernel.org/r/20231207-tps6598x_update-v1-0-dc21b5301d91@xxxxxxxxxxxxxx
> >>>
> >>
> >> FYI, this series breaks boot for TI SK-AM62A and SK-AM62 which use 
> >> TPS6598x as the USB-C PD chip.

This series also broke boot on TI SK-AM62x [0].

> >>
> >> The platforms stopped booting since next-20240103 [1], and reverting 
> >> this series [4] seems to fix the issue [2]
> >>
> >> Is there any change needed in the board device-tree [3] and bindings?
> >>
> >> We don't specify any firmware in the device-tree node, but seems like 
> >> that is an assumption in this series. I tried reverting it (below 
> >> change) but that did *not* help with the stuck boot.
> >>
> >> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> >> index a956eb976906..fa3bd7349265 100644
> >> --- a/drivers/usb/typec/tipd/core.c
> >> +++ b/drivers/usb/typec/tipd/core.c
> >> @@ -1139,7 +1139,7 @@ static int tps6598x_apply_patch(struct tps6598x *tps)
> >>         ret = device_property_read_string(tps->dev, "firmware-name",
> >>                                           &firmware_name);
> >>         if (ret)
> >> -               return ret;
> >> +               return 0;
> >>
> >>         ret = tps_request_firmware(tps, &fw);
> >>         if (ret)
> >>
> >>
> >> [1] https://linux.kernelci.org/soc/ti/job/next/kernel/next-20240103/plan/baseline-nfs/
> >> [2] https://gist.github.com/jailuthra/0c077176585e4df2f8b78f7784087865
> >> [3] https://gitlab.com/linux-kernel/linux-next/-/blob/master/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts#L305
> >> [4] https://github.com/jailuthra/linux/commits/next-20240103-tps6598-fix/
> > 
> > The following change seems to fix boot on SK-AM62A without reverting 
> > this whole series:
> > 
> > ------------------
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index a956eb976906a5..8ba2aa05db519b 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
> >  	return 0;
> >  }
> >  
> > -static int tps6598x_reset(struct tps6598x *tps)
> > +static int tps25750_reset(struct tps6598x *tps)
> >  {
> >  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> >  }
> >  
> > +static int tps6598x_reset(struct tps6598x *tps)
> > +{
> > +	return 0;
> > +}
> > +
> >  static int
> >  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
> >  {
> > @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
> >  	.trace_status = trace_tps25750_status,
> >  	.apply_patch = tps25750_apply_patch,
> >  	.init = tps25750_init,
> > -	.reset = tps6598x_reset,
> > +	.reset = tps25750_reset,
> >  };
> >  
> >  static const struct of_device_id tps6598x_of_match[] = {
> > 
> > ------------------
> > 
> > I am not an expert on this, will let you/others decide on what should be 
> > the correct way to reset TPS6598x for patching without breaking this SK.
> > 
> > 
> 
> This looks like a correct fix to me.
> Could you please send a proper PATCH with Fixes tag? Thanks!

Thanks for reviewing this Roger, the same patch above worked for me to
fix SK-AM62x as well [1].

[0] https://storage.kernelci.org/next/master/next-20240103/arm64/defconfig/gcc-10/lab-ti/baseline-nfs-am62xx_sk-fs.txt
[1] https://gist.github.com/DhruvaG2000/326b5d7fab4be95f20cd0aac4125f577

-- 
Best regards,
Dhruva Gole <d-gole@xxxxxx>