| Message ID | 1365548785-28684-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefano Babic |
| Headers | show |
On 10/04/2013 01:06, Fabio Estevam wrote: > 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. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- Hi Fabio, > Changes since v1: > - Introduce MXC_CSPICTRL_MODE_MASK definition > - Remove additional read of reg_ctrl > > arch/arm/include/asm/arch-mx5/imx-regs.h | 1 + > arch/arm/include/asm/arch-mx6/imx-regs.h | 1 + > drivers/spi/mxc_spi.c | 17 +++++++++-------- > 3 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h > index 249d15a..a71cc13 100644 > --- a/arch/arm/include/asm/arch-mx5/imx-regs.h > +++ b/arch/arm/include/asm/arch-mx5/imx-regs.h > @@ -230,6 +230,7 @@ > #define MXC_CSPICTRL_EN (1 << 0) > #define MXC_CSPICTRL_MODE (1 << 1) > #define MXC_CSPICTRL_XCH (1 << 2) > +#define MXC_CSPICTRL_MODE_MASK (0xf << 4) > #define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) > #define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) > #define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) > diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h > index eaa7439..d79ab2f 100644 > --- a/arch/arm/include/asm/arch-mx6/imx-regs.h > +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h > @@ -346,6 +346,7 @@ struct cspi_regs { > #define MXC_CSPICTRL_EN (1 << 0) > #define MXC_CSPICTRL_MODE (1 << 1) > #define MXC_CSPICTRL_XCH (1 << 2) > +#define MXC_CSPICTRL_MODE_MASK (0xf << 4) > #define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) > #define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) > #define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c > index 4c19e0b..20419e6 100644 > --- a/drivers/spi/mxc_spi.c > +++ b/drivers/spi/mxc_spi.c > @@ -137,11 +137,15 @@ 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); > - > - reg_ctrl = reg_read(®s->ctrl); > + /* > + * 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 = MXC_CSPICTRL_MODE_MASK; > + reg_write(®s->ctrl, reg_ctrl); > + reg_ctrl |= MXC_CSPICTRL_EN; > + reg_write(®s->ctrl, reg_ctrl); I am afraid you are breaking MX3x / MX25 because you add MXC_CSPICTRL_MODE_MASK only to MX5 / MX6. Best regards, Stefano
Hi Stefano, On Wed, Apr 10, 2013 at 3:10 AM, Stefano Babic <sbabic@denx.de> wrote: > I am afraid you are breaking MX3x / MX25 because you add > MXC_CSPICTRL_MODE_MASK only to MX5 / MX6. It will not break other platforms because this code is protected with a "#ifdef MXC_ECSPI". And MXC_ECSPI is only defined on mx5/mx6: arch/arm/include/asm/arch-mx5/imx-regs.h:#define MXC_ECSPI arch/arm/include/asm/arch-mx6/imx-regs.h:#define MXC_ECSPI ,so we are safe here. Regards, Fabio Estevam
On 10/04/2013 14:01, Fabio Estevam wrote: > Hi Stefano, > > On Wed, Apr 10, 2013 at 3:10 AM, Stefano Babic <sbabic@denx.de> wrote: > >> I am afraid you are breaking MX3x / MX25 because you add >> MXC_CSPICTRL_MODE_MASK only to MX5 / MX6. > > It will not break other platforms because this code is protected with > a "#ifdef MXC_ECSPI". > > And MXC_ECSPI is only defined on mx5/mx6: > > arch/arm/include/asm/arch-mx5/imx-regs.h:#define MXC_ECSPI > arch/arm/include/asm/arch-mx6/imx-regs.h:#define MXC_ECSPI > > ,so we are safe here. Thanks, you're right. Applied to u-boot-imx, thanks. Best regards, Stefano Babic
Am 10.04.2013 01:06, 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. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Changes since v1: > - Introduce MXC_CSPICTRL_MODE_MASK definition > - Remove additional read of reg_ctrl > > arch/arm/include/asm/arch-mx5/imx-regs.h | 1 + > arch/arm/include/asm/arch-mx6/imx-regs.h | 1 + > drivers/spi/mxc_spi.c | 17 +++++++++-------- > 3 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h > index 249d15a..a71cc13 100644 > --- a/arch/arm/include/asm/arch-mx5/imx-regs.h > +++ b/arch/arm/include/asm/arch-mx5/imx-regs.h > @@ -230,6 +230,7 @@ > #define MXC_CSPICTRL_EN (1 << 0) > #define MXC_CSPICTRL_MODE (1 << 1) > #define MXC_CSPICTRL_XCH (1 << 2) > +#define MXC_CSPICTRL_MODE_MASK (0xf << 4) > #define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) > #define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) > #define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) > diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h > index eaa7439..d79ab2f 100644 > --- a/arch/arm/include/asm/arch-mx6/imx-regs.h > +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h > @@ -346,6 +346,7 @@ struct cspi_regs { > #define MXC_CSPICTRL_EN (1 << 0) > #define MXC_CSPICTRL_MODE (1 << 1) > #define MXC_CSPICTRL_XCH (1 << 2) > +#define MXC_CSPICTRL_MODE_MASK (0xf << 4) > #define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) > #define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) > #define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c > index 4c19e0b..20419e6 100644 > --- a/drivers/spi/mxc_spi.c > +++ b/drivers/spi/mxc_spi.c > @@ -137,11 +137,15 @@ 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); > - > - reg_ctrl = reg_read(®s->ctrl); > + /* > + * 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 = MXC_CSPICTRL_MODE_MASK; I was offline some days, giving me some time to think about this ;) Most probably it does no harm, but I somehow feel uncomfortable with setting *all* CSs to master mode. Just because there might be already (one, several?) CS at master mode and switching it back to the (reset default) slave will give a glitch on the clock line. Wouldn't it be cleaner to keep the master mode only for the CS which are already in master mode before? E.g. instead of reg_ctrl = MXC_CSPICTRL_MODE_MASK; from above something like reg_ctrl = reg_read(®s->ctrl) & MXC_CSPICTRL_MODE_MASK; ? And then ... > + reg_write(®s->ctrl, reg_ctrl); > + reg_ctrl |= MXC_CSPICTRL_EN; > + reg_write(®s->ctrl, reg_ctrl); > > /* > * The following computation is taken directly from Freescale's code. > @@ -174,9 +178,6 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs, > reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) | > MXC_CSPICTRL_POSTDIV(post_div); > > - /* always set to master mode */ > - reg_ctrl |= 1 << (cs + 4); ... keeping thins line? Best regards Dirk
diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h index 249d15a..a71cc13 100644 --- a/arch/arm/include/asm/arch-mx5/imx-regs.h +++ b/arch/arm/include/asm/arch-mx5/imx-regs.h @@ -230,6 +230,7 @@ #define MXC_CSPICTRL_EN (1 << 0) #define MXC_CSPICTRL_MODE (1 << 1) #define MXC_CSPICTRL_XCH (1 << 2) +#define MXC_CSPICTRL_MODE_MASK (0xf << 4) #define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) #define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) #define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h b/arch/arm/include/asm/arch-mx6/imx-regs.h index eaa7439..d79ab2f 100644 --- a/arch/arm/include/asm/arch-mx6/imx-regs.h +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h @@ -346,6 +346,7 @@ struct cspi_regs { #define MXC_CSPICTRL_EN (1 << 0) #define MXC_CSPICTRL_MODE (1 << 1) #define MXC_CSPICTRL_XCH (1 << 2) +#define MXC_CSPICTRL_MODE_MASK (0xf << 4) #define MXC_CSPICTRL_CHIPSELECT(x) (((x) & 0x3) << 12) #define MXC_CSPICTRL_BITCOUNT(x) (((x) & 0xfff) << 20) #define MXC_CSPICTRL_PREDIV(x) (((x) & 0xF) << 12) diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 4c19e0b..20419e6 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -137,11 +137,15 @@ 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); - - reg_ctrl = reg_read(®s->ctrl); + /* + * 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 = MXC_CSPICTRL_MODE_MASK; + reg_write(®s->ctrl, reg_ctrl); + reg_ctrl |= MXC_CSPICTRL_EN; + reg_write(®s->ctrl, reg_ctrl); /* * The following computation is taken directly from Freescale's code. @@ -174,9 +178,6 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs, reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) | MXC_CSPICTRL_POSTDIV(post_div); - /* always set to master mode */ - reg_ctrl |= 1 << (cs + 4); - /* We need to disable SPI before changing registers */ reg_ctrl &= ~MXC_CSPICTRL_EN;