[AMD Official Use Only - General] > -----Original Message----- > From: Robert Hancock <robert.hancock@xxxxxxxxxx> > Sent: Friday, July 8, 2022 3:45 AM > To: shubhrajyoti.datta@xxxxxxxxxx; wsa@xxxxxxxxxx > Cc: michal.simek@xxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Datta, Shubhrajyoti > <shubhrajyoti.datta@xxxxxxx> > Subject: Re: [PATCH v2] i2c: cadence: Add standard bus recovery support > > [CAUTION: External Email] > > On Thu, 2022-07-07 at 23:03 +0200, Wolfram Sang wrote: > > Hi, > > > > On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote: > > > From: Robert Hancock <robert.hancock@xxxxxxxxxx> > > > > > > Hook up the standard GPIO/pinctrl-based recovery support.. > > > In the discurssion > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa > > > tchwork.ozlabs.org%2Fproject%2Flinux- > i2c%2Fpatch%2F20211129090116.16 > > > 628-1- > shubhrajyoti.datta%40xilinx.com%2F&data=05%7C01%7Cshubhraj > > > > yoti.datta%40amd.com%7C4e1a91e059a8495756a208da60661551%7C3dd89 > 61fe4 > > > > 884e608e11a82d994e183d%7C0%7C0%7C637928288864536085%7CUnknow > n%7CTWFp > > > > bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI > 6 > > > > Mn0%3D%7C3000%7C%7C%7C&sdata=xhjvSNSf1%2FO5ihd%2B92O%2B > LCOci265Q > > > R8HiNh%2B8TBWXw8%3D&reserved=0 > > > > > > recovery should be done at the beginning of the transaction. > > > Here we are doing the recovery at the beginning on a timeout. > > > > Which is still wrong. Will fix this. > > > > > > > > Signed-off-by: Robert Hancock <robert.hancock@xxxxxxxxxx> > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx> > > > > This is an AMD address, but the one you sent from is from Xilinx? > > > > > if (time_left == 0) { > > > + i2c_recover_bus(adap); > > > > This is the wrong part. Will fix > > > > > cdns_i2c_master_reset(adap); > > > dev_err(id->adap.dev.parent, > > > "timeout waiting on completion\n"); > > > @@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct > > > i2c_adapter *adap, struct i2c_msg *msgs, #endif > > > > > > /* Check if the bus is free */ > > > - if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) > > > { > > > + ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET, > > > reg, > > > + !(reg & CDNS_I2C_SR_BA), > > > + CDNS_I2C_POLL_US, > > > CDNS_I2C_TIMEOUT_US); > > > + if (ret) { > > > ret = -EAGAIN; > > > + i2c_recover_bus(adap); > > > goto out; > > > > This is proper. > > > > > } > > > > > > @@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct > > > platform_device *pdev) > > > id->adap.retries = 3; /* Default retry value. */ > > > id->adap.algo_data = id; > > > id->adap.dev.parent = &pdev->dev; > > > + id->adap.bus_recovery_info = &id->rinfo; > > > > Since 'rinfo' is never populated with actual data, I am quite sure > > this patch has never been tested. > > I think this (setting bus_recovery_info to point to a zeroed structure) is > sufficient to allow the generic recovery initialization in i2c-core- base.c to > work. i2c_gpio_init_recovery should fill in the required info based on the > available pinctrl and gpio configuration in this case. I checked the handler calls however I will try to check with a setup where I can probe and Verify the clock cycles. > > > > > Regards, > > > > Wolfram > >
