| Message ID | 57a4f5352ce6f03bde7aafe8b880f91b52994379.1380550490.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Headers | show |
> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); > + cr |= XIIC_CR_DIR_IS_TX_MASK; > + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); > + Is there no need to clear the bit again when receiving? And did transferring ever work if this bit was never set before?
On 10/04/2013 07:46 AM, Wolfram Sang wrote: > >> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); >> + cr |= XIIC_CR_DIR_IS_TX_MASK; >> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); >> + > > Is there no need to clear the bit again when receiving? This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit() xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); > And did > transferring ever work if this bit was never set before? I really don't know. We have switched from old driver to this new mainline one and based on our eeprom testing we have found that this bit hasn't been setup properly. It is described here. http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf page 28 - step 3. IIC Master Transmitter with a Repeated Start 1. Write the IIC device address to the TX_FIFO. 2. Write data to TX_FIFO. 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. 4. Continue writing data to TX_FIFO. 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. 6. Write to CR to set RSTA = 1. 7. Write IIC device address to TX_FIFO. 8. Write all data except last byte to TX_FIFO. 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. 11. Write last byte of data to TX_FIFO. Thanks, Michal
On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: > On 10/04/2013 07:46 AM, Wolfram Sang wrote: > > > >> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); > >> + cr |= XIIC_CR_DIR_IS_TX_MASK; > >> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); > >> + > > > > Is there no need to clear the bit again when receiving? > > This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit() > > xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); A bit implicit, but OK. > > And did > > transferring ever work if this bit was never set before? > > I really don't know. We have switched from old driver to this new mainline one > and based on our eeprom testing we have found that this bit hasn't been setup properly. > > It is described here. > http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf > page 28 - step 3. > > IIC Master Transmitter with a Repeated Start > 1. Write the IIC device address to the TX_FIFO. > 2. Write data to TX_FIFO. > 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. > 4. Continue writing data to TX_FIFO. > 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. > 6. Write to CR to set RSTA = 1. Repeated start is not happening in the driver as well, or am I overlooking something? > 7. Write IIC device address to TX_FIFO. > 8. Write all data except last byte to TX_FIFO. > 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. > 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. > 11. Write last byte of data to TX_FIFO. CCing more people who worked on the driver in the past and might have experiences Thanks, Wolfram
On 10/04/2013 01:55 PM, Wolfram Sang wrote: > On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: >> On 10/04/2013 07:46 AM, Wolfram Sang wrote: >>> >>>> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); >>>> + cr |= XIIC_CR_DIR_IS_TX_MASK; >>>> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); >>>> + >>> >>> Is there no need to clear the bit again when receiving? >> >> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit() >> >> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); > > A bit implicit, but OK. > >>> And did >>> transferring ever work if this bit was never set before? >> >> I really don't know. We have switched from old driver to this new mainline one >> and based on our eeprom testing we have found that this bit hasn't been setup properly. >> >> It is described here. >> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf >> page 28 - step 3. >> >> IIC Master Transmitter with a Repeated Start >> 1. Write the IIC device address to the TX_FIFO. >> 2. Write data to TX_FIFO. >> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. >> 4. Continue writing data to TX_FIFO. >> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >> 6. Write to CR to set RSTA = 1. > > Repeated start is not happening in the driver as well, or am I > overlooking something? > >> 7. Write IIC device address to TX_FIFO. >> 8. Write all data except last byte to TX_FIFO. >> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. >> 11. Write last byte of data to TX_FIFO. > > CCing more people who worked on the driver in the past and might have > experiences The current version works fine here. The driver uses whats described in the datasheet as "dynamic controller logic flow" and not the "standard controller logic flow". The sequence Michal mentioned above is from the "standard controller logic flow" section. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote: > On 10/04/2013 01:55 PM, Wolfram Sang wrote: >> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: >>> On 10/04/2013 07:46 AM, Wolfram Sang wrote: >>>> >>>>> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); >>>>> + cr |= XIIC_CR_DIR_IS_TX_MASK; >>>>> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); >>>>> + >>>> >>>> Is there no need to clear the bit again when receiving? >>> >>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit() >>> >>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); >> >> A bit implicit, but OK. >> >>>> And did >>>> transferring ever work if this bit was never set before? >>> >>> I really don't know. We have switched from old driver to this new mainline one >>> and based on our eeprom testing we have found that this bit hasn't been setup properly. >>> >>> It is described here. >>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf >>> page 28 - step 3. >>> >>> IIC Master Transmitter with a Repeated Start >>> 1. Write the IIC device address to the TX_FIFO. >>> 2. Write data to TX_FIFO. >>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. >>> 4. Continue writing data to TX_FIFO. >>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >>> 6. Write to CR to set RSTA = 1. >> >> Repeated start is not happening in the driver as well, or am I >> overlooking something? >> >>> 7. Write IIC device address to TX_FIFO. >>> 8. Write all data except last byte to TX_FIFO. >>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. >>> 11. Write last byte of data to TX_FIFO. >> >> CCing more people who worked on the driver in the past and might have >> experiences > > The current version works fine here. The driver uses whats described in the > datasheet as "dynamic controller logic flow" and not the "standard > controller logic flow". The sequence Michal mentioned above is from the > "standard controller logic flow" section. Does this change break "dynamic controller logic flow"? Thanks, Michal
On 10/04/2013 03:38 PM, Lars-Peter Clausen wrote: > On 10/04/2013 03:09 PM, Michal Simek wrote: >> >> >> On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote: >>> On 10/04/2013 01:55 PM, Wolfram Sang wrote: >>>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: >>>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote: >>>>>> >>>>>>> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); >>>>>>> + cr |= XIIC_CR_DIR_IS_TX_MASK; >>>>>>> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); >>>>>>> + >>>>>> >>>>>> Is there no need to clear the bit again when receiving? >>>>> >>>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit() >>>>> >>>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); >>>> >>>> A bit implicit, but OK. >>>> >>>>>> And did >>>>>> transferring ever work if this bit was never set before? >>>>> >>>>> I really don't know. We have switched from old driver to this new mainline one >>>>> and based on our eeprom testing we have found that this bit hasn't been setup properly. >>>>> >>>>> It is described here. >>>>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf >>>>> page 28 - step 3. >>>>> >>>>> IIC Master Transmitter with a Repeated Start >>>>> 1. Write the IIC device address to the TX_FIFO. >>>>> 2. Write data to TX_FIFO. >>>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. >>>>> 4. Continue writing data to TX_FIFO. >>>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >>>>> 6. Write to CR to set RSTA = 1. >>>> >>>> Repeated start is not happening in the driver as well, or am I >>>> overlooking something? >>>> >>>>> 7. Write IIC device address to TX_FIFO. >>>>> 8. Write all data except last byte to TX_FIFO. >>>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >>>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. >>>>> 11. Write last byte of data to TX_FIFO. >>>> >>>> CCing more people who worked on the driver in the past and might have >>>> experiences >>> >>> The current version works fine here. The driver uses whats described in the >>> datasheet as "dynamic controller logic flow" and not the "standard >>> controller logic flow". The sequence Michal mentioned above is from the >>> "standard controller logic flow" section. >> >> Does this change break "dynamic controller logic flow"? > > Not sure, but I would assume that the bit is ignored in this mode. But I > don't think the patch should be applied since this step is not in the > sequence of steps that should be done. Kedar: Can you please look at both these modes and provide feedback? Thanks, Michal
On 10/04/2013 03:09 PM, Michal Simek wrote: > > > On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote: >> On 10/04/2013 01:55 PM, Wolfram Sang wrote: >>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: >>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote: >>>>> >>>>>> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); >>>>>> + cr |= XIIC_CR_DIR_IS_TX_MASK; >>>>>> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); >>>>>> + >>>>> >>>>> Is there no need to clear the bit again when receiving? >>>> >>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() ->xiic_reinit() >>>> >>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, XIIC_CR_TX_FIFO_RESET_MASK); >>> >>> A bit implicit, but OK. >>> >>>>> And did >>>>> transferring ever work if this bit was never set before? >>>> >>>> I really don't know. We have switched from old driver to this new mainline one >>>> and based on our eeprom testing we have found that this bit hasn't been setup properly. >>>> >>>> It is described here. >>>> http://www.xilinx.com/support/documentation/ip_documentation/axi_iic/v1_02_a/axi_iic_ds756.pdf >>>> page 28 - step 3. >>>> >>>> IIC Master Transmitter with a Repeated Start >>>> 1. Write the IIC device address to the TX_FIFO. >>>> 2. Write data to TX_FIFO. >>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. >>>> 4. Continue writing data to TX_FIFO. >>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >>>> 6. Write to CR to set RSTA = 1. >>> >>> Repeated start is not happening in the driver as well, or am I >>> overlooking something? >>> >>>> 7. Write IIC device address to TX_FIFO. >>>> 8. Write all data except last byte to TX_FIFO. >>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has throttled the bus. >>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at the end of the last byte. >>>> 11. Write last byte of data to TX_FIFO. >>> >>> CCing more people who worked on the driver in the past and might have >>> experiences >> >> The current version works fine here. The driver uses whats described in the >> datasheet as "dynamic controller logic flow" and not the "standard >> controller logic flow". The sequence Michal mentioned above is from the >> "standard controller logic flow" section. > > Does this change break "dynamic controller logic flow"? Not sure, but I would assume that the bit is ignored in this mode. But I don't think the patch should be applied since this step is not in the sequence of steps that should be done. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michal, > -----Original Message----- > From: Michal Simek [mailto:monstr@monstr.eu] > Sent: Friday, October 04, 2013 7:08 PM > To: Lars-Peter Clausen > Cc: Wolfram Sang; Michal Simek; linux-kernel@vger.kernel.org; Appana > Durga Kedareswara Rao; Appana Durga Kedareswara Rao; Jean Delvare; > Peter Korsgaard; linux-i2c@vger.kernel.org; Richard Röjfors; Steven A. Falco > Subject: Re: [PATCH v2 2/3] i2c: xilinx: Set tx direction in write operation > > On 10/04/2013 03:38 PM, Lars-Peter Clausen wrote: > > On 10/04/2013 03:09 PM, Michal Simek wrote: > >> > >> > >> On 10/04/2013 02:12 PM, Lars-Peter Clausen wrote: > >>> On 10/04/2013 01:55 PM, Wolfram Sang wrote: > >>>> On Fri, Oct 04, 2013 at 11:53:49AM +0200, Michal Simek wrote: > >>>>> On 10/04/2013 07:46 AM, Wolfram Sang wrote: > >>>>>> > >>>>>>> + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); > >>>>>>> + cr |= XIIC_CR_DIR_IS_TX_MASK; > >>>>>>> + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); > >>>>>>> + > >>>>>> > >>>>>> Is there no need to clear the bit again when receiving? > >>>>> > >>>>> This bit is cleared in xiic_xfer() -> xiic_start_xfer() > >>>>> ->xiic_reinit() > >>>>> > >>>>> xiic_setreg8(i2c, XIIC_CR_REG_OFFSET, > XIIC_CR_TX_FIFO_RESET_MASK); > >>>> > >>>> A bit implicit, but OK. > >>>> > >>>>>> And did > >>>>>> transferring ever work if this bit was never set before? > >>>>> > >>>>> I really don't know. We have switched from old driver to this new > >>>>> mainline one and based on our eeprom testing we have found that > this bit hasn't been setup properly. > >>>>> > >>>>> It is described here. > >>>>> > http://www.xilinx.com/support/documentation/ip_documentation/axi_i > >>>>> ic/v1_02_a/axi_iic_ds756.pdf > >>>>> page 28 - step 3. > >>>>> > >>>>> IIC Master Transmitter with a Repeated Start 1. Write the IIC > >>>>> device address to the TX_FIFO. > >>>>> 2. Write data to TX_FIFO. > >>>>> 3. Write to Control Register (CR) to set MSMS = 1 and TX = 1. > >>>>> 4. Continue writing data to TX_FIFO. > >>>>> 5. Wait for transmit FIFO empty interrupt. This implies the IIC has > throttled the bus. > >>>>> 6. Write to CR to set RSTA = 1. > >>>> > >>>> Repeated start is not happening in the driver as well, or am I > >>>> overlooking something? > >>>> > >>>>> 7. Write IIC device address to TX_FIFO. > >>>>> 8. Write all data except last byte to TX_FIFO. > >>>>> 9. Wait for transmit FIFO empty interrupt. This implies the IIC has > throttled the bus. > >>>>> 10. Write to CR to set MSMS = 0. The IIC generates a stop condition at > the end of the last byte. > >>>>> 11. Write last byte of data to TX_FIFO. > >>>> > >>>> CCing more people who worked on the driver in the past and might > >>>> have experiences > >>> > >>> The current version works fine here. The driver uses whats described > >>> in the datasheet as "dynamic controller logic flow" and not the > >>> "standard controller logic flow". The sequence Michal mentioned > >>> above is from the "standard controller logic flow" section. > >> > >> Does this change break "dynamic controller logic flow"? > > > > Not sure, but I would assume that the bit is ignored in this mode. But > > I don't think the patch should be applied since this step is not in > > the sequence of steps that should be done. > > Kedar: Can you please look at both these modes and provide feedback? The driver is following the Dynamic controller logic flow as per the dynamic Controller logic flow. These bits need not to be set. It was my mistake. Please ignore this patch. There was an incorrect h/w design on which I tested this that's why I thought it is a Solution for this. Now I tested with the proper h/w design it is working fine for me without setting this bit. Thanks lars for the feedback. Regards, Kedar. > > Thanks, > Michal > > -- > Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 > w: www.monstr.eu p: +42-0-721842854 > Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ > Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U- > BOOT custodian and responsible for u-boot arm zynq platform > This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 5eb0a8b..44e6ae7 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -536,6 +536,7 @@ static void xiic_start_recv(struct xiic_i2c *i2c) static void xiic_start_send(struct xiic_i2c *i2c) { struct i2c_msg *msg = i2c->tx_msg; + u32 cr; xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK); @@ -556,6 +557,10 @@ static void xiic_start_send(struct xiic_i2c *i2c) xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data); } + cr = xiic_getreg32(i2c, XIIC_CR_REG_OFFSET); + cr |= XIIC_CR_DIR_IS_TX_MASK; + xiic_setreg32(i2c, XIIC_CR_REG_OFFSET, cr); + xiic_fill_tx_fifo(i2c); /* Clear any pending Tx empty, Tx Error and then enable them. */