Message ID | 1365254107-22448-1-git-send-email-festevam@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | Stefano Babic |
Headers | show |
Am 06.04.2013 15:15, schrieb Fabio Estevam: > From: Fabio Estevam <fabio.estevam@freescale.com> > > The glitch in the SPI clock line, which commit 3cea335c34 (spi: mxc_spi: Fix spi > clock glitch durant reset) solved, is back now and itwas re-introduced by > commit d36b39bf0d (spi: mxc_spi: Fix ECSPI reset handling). > > Actually the glitch is happening due to always toggling between slave mode > and master mode by configuring the CHANNEL_MODE bits in this reset function. > > Since the spi driver only supports master mode, set the mode for all channels > always to master mode in order to have a stable, "glitch-free" SPI clock line. Ok, thanks, I wasn't aware of this. > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/spi/mxc_spi.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c > index 4c19e0b..9eb2bce 100644 > --- a/drivers/spi/mxc_spi.c > +++ b/drivers/spi/mxc_spi.c > @@ -137,9 +137,14 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs, > return -1; > } > > - /* Reset spi */ > - reg_write(®s->ctrl, 0); > - reg_write(®s->ctrl, MXC_CSPICTRL_EN); > + /* > + * Reset SPI and set all CSs to master mode, I haven't checked the rest of the driver, but do we touch the CSs bits anywhere else, too? Could that parts dropped with this patch, then. I.e. never touch the bits regs->ctrl[7-4] again? > if toggling > + * between slave and master mode we might see a glitch > + * on the clock line > + */ > + reg_write(®s->ctrl, 0x000000F0); Do we have a macro for the 0xF? > + reg_ctrl = reg_read(®s->ctrl); Do we expect reg_ctrl to be 0x000000F0 then? If yes, do we have to really read it here? If we read it here, should we drop the read below, then? > + reg_write(®s->ctrl, reg_ctrl | MXC_CSPICTRL_EN); > > reg_ctrl = reg_read(®s->ctrl); So, as mentioned by Stefano before, I'd propose to either a) do the reg_read(®s->ctrl); only _once_. E.g. /* * Reset SPI and set all CSs to master mode, if toggling * between slave and master mode we might see a glitch * on the clock line */ reg_write(®s->ctrl, 0x000000F0); /* FIXME: use a macro here*/ reg_ctrl = reg_read(®s->ctrl) | MXC_CSPICTRL_EN; /* Reading back the register content is done because we don't get back the value written above */ reg_write(®s->ctrl, reg_ctrl); => no additional read of reg_ctrl needed here as reg_ctrl contains already the recent register content <= or b) _never_ read back the register. E.g. /* * Reset SPI and set all CSs to master mode, if toggling * between slave and master mode we might see a glitch * on the clock line */ reg_ctrl = 0x000000F0; /* FIXME: use a macro here*/ reg_write(®s->ctrl, reg_ctrl); reg_ctrl |= MXC_CSPICTRL_EN; reg_write(®s->ctrl, reg_ctrl); => no additional read of reg_ctrl needed here as reg_ctrl contains already the recent register content <= Would one of these options work? Best regards Dirk
diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 4c19e0b..9eb2bce 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -137,9 +137,14 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs, return -1; } - /* Reset spi */ - reg_write(®s->ctrl, 0); - reg_write(®s->ctrl, MXC_CSPICTRL_EN); + /* + * Reset SPI and set all CSs to master mode, if toggling + * between slave and master mode we might see a glitch + * on the clock line + */ + reg_write(®s->ctrl, 0x000000F0); + reg_ctrl = reg_read(®s->ctrl); + reg_write(®s->ctrl, reg_ctrl | MXC_CSPICTRL_EN); reg_ctrl = reg_read(®s->ctrl);