| Message ID | 20240405140906.77831-4-marvin24@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve robustnes during initialization | expand |
On Fri Apr 5, 2024 at 4:09 PM CEST, Marc Dietrich wrote: > The i2c controller needs to read back the data written to its registers. > This way we can avoid the long delay in the interrupt handler. s/i2c/I2C/ perhaps? Also, looking at the preceding patches it looks to me like the reason why we can drop the udelay() call is not just because we read back, but also because we actually wait for completion. If so, maybe that should also be mentioned. > > Signed-off-by: Marc Dietrich <marvin24@gmx.de> > --- > drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++---------------- > 1 file changed, 22 insertions(+), 17 deletions(-) > > diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c > index 282a664c9176..9914c30b6933 100644 > --- a/drivers/staging/nvec/nvec.c > +++ b/drivers/staging/nvec/nvec.c > @@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec) > (uint)nvec->tx->size, nvec->tx->data[1]); > } > > +/** > + * i2c_writel - savely write to an i2c client controller register "safely", also "I2C" > + @ val: value to be written > + @ reg: register to write to The formatting here looks odd. I think this is supposed to be: * @val: * @reg: Thierry > + */ > + > +static void i2c_writel(u32 val, void *reg) > +{ > + writel_relaxed(val, reg); > + > + /* read back register to make sure that register writes completed */ > + readl_relaxed(reg); > +} > + > /** > * nvec_interrupt - Interrupt handler > * @irq: The IRQ > @@ -599,7 +613,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev) > if ((status & RNW) == 0) { > received = readl(nvec->base + I2C_SL_RCVD); > if (status & RCVD) > - writel(0, nvec->base + I2C_SL_RCVD); > + i2c_writel(0, nvec->base + I2C_SL_RCVD); > } > > if (status == (I2C_SL_IRQ | RCVD)) > @@ -691,7 +705,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev) > > /* Send data if requested, but not on end of transmission */ > if ((status & (RNW | END_TRANS)) == RNW) > - writel(to_send, nvec->base + I2C_SL_RCVD); > + i2c_writel(to_send, nvec->base + I2C_SL_RCVD); > > /* If we have send the first byte */ > if (status == (I2C_SL_IRQ | RNW | RCVD)) > @@ -708,15 +722,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev) > status & RCVD ? " RCVD" : "", > status & RNW ? " RNW" : ""); > > - /* > - * TODO: replace the udelay with a read back after each writel above > - * in order to work around a hardware issue, see i2c-tegra.c > - * > - * Unfortunately, this change causes an intialisation issue with the > - * touchpad, which needs to be fixed first. > - */ > - udelay(100); > - > return IRQ_HANDLED; > } > > @@ -732,15 +737,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec) > > val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN | > (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT); > - writel(val, nvec->base + I2C_CNFG); > + i2c_writel(val, nvec->base + I2C_CNFG); > > clk_set_rate(nvec->i2c_clk, 8 * 80000); > > - writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG); > - writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT); > + i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG); > + i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT); > > - writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1); > - writel(0, nvec->base + I2C_SL_ADDR2); > + i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1); > + i2c_writel(0, nvec->base + I2C_SL_ADDR2); > > enable_irq(nvec->irq); > } > @@ -749,7 +754,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec) > static void nvec_disable_i2c_slave(struct nvec_chip *nvec) > { > disable_irq(nvec->irq); > - writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG); > + i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG); > clk_disable_unprepare(nvec->i2c_clk); > } > #endif > -- > 2.43.0
On Fri, Apr 05, 2024 at 04:09:05PM +0200, Marc Dietrich wrote: > The i2c controller needs to read back the data written to its registers. > This way we can avoid the long delay in the interrupt handler. > > Signed-off-by: Marc Dietrich <marvin24@gmx.de> > --- > drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++---------------- > 1 file changed, 22 insertions(+), 17 deletions(-) > > diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c > index 282a664c9176..9914c30b6933 100644 > --- a/drivers/staging/nvec/nvec.c > +++ b/drivers/staging/nvec/nvec.c > @@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec) > (uint)nvec->tx->size, nvec->tx->data[1]); > } > > +/** > + * i2c_writel - savely write to an i2c client controller register > + @ val: value to be written > + @ reg: register to write to > + */ > + > +static void i2c_writel(u32 val, void *reg) I'm not a expert at kernel doc or whatever, but this comment isn't in the right format. And delete the blank line between the comment and the function. /** * i2c_writel - savely write to an i2c client controller register * @ val: value to be written * @ reg: register to write to */ static void i2c_writel(u32 val, void *reg) regards, dan carpenter
diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c index 282a664c9176..9914c30b6933 100644 --- a/drivers/staging/nvec/nvec.c +++ b/drivers/staging/nvec/nvec.c @@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec) (uint)nvec->tx->size, nvec->tx->data[1]); } +/** + * i2c_writel - savely write to an i2c client controller register + @ val: value to be written + @ reg: register to write to + */ + +static void i2c_writel(u32 val, void *reg) +{ + writel_relaxed(val, reg); + + /* read back register to make sure that register writes completed */ + readl_relaxed(reg); +} + /** * nvec_interrupt - Interrupt handler * @irq: The IRQ @@ -599,7 +613,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev) if ((status & RNW) == 0) { received = readl(nvec->base + I2C_SL_RCVD); if (status & RCVD) - writel(0, nvec->base + I2C_SL_RCVD); + i2c_writel(0, nvec->base + I2C_SL_RCVD); } if (status == (I2C_SL_IRQ | RCVD)) @@ -691,7 +705,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev) /* Send data if requested, but not on end of transmission */ if ((status & (RNW | END_TRANS)) == RNW) - writel(to_send, nvec->base + I2C_SL_RCVD); + i2c_writel(to_send, nvec->base + I2C_SL_RCVD); /* If we have send the first byte */ if (status == (I2C_SL_IRQ | RNW | RCVD)) @@ -708,15 +722,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev) status & RCVD ? " RCVD" : "", status & RNW ? " RNW" : ""); - /* - * TODO: replace the udelay with a read back after each writel above - * in order to work around a hardware issue, see i2c-tegra.c - * - * Unfortunately, this change causes an intialisation issue with the - * touchpad, which needs to be fixed first. - */ - udelay(100); - return IRQ_HANDLED; } @@ -732,15 +737,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec) val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN | (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT); - writel(val, nvec->base + I2C_CNFG); + i2c_writel(val, nvec->base + I2C_CNFG); clk_set_rate(nvec->i2c_clk, 8 * 80000); - writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG); - writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT); + i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG); + i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT); - writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1); - writel(0, nvec->base + I2C_SL_ADDR2); + i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1); + i2c_writel(0, nvec->base + I2C_SL_ADDR2); enable_irq(nvec->irq); } @@ -749,7 +754,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec) static void nvec_disable_i2c_slave(struct nvec_chip *nvec) { disable_irq(nvec->irq); - writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG); + i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG); clk_disable_unprepare(nvec->i2c_clk); } #endif
The i2c controller needs to read back the data written to its registers. This way we can avoid the long delay in the interrupt handler. Signed-off-by: Marc Dietrich <marvin24@gmx.de> --- drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 17 deletions(-) -- 2.43.0