From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dirk Behme Date: Sun, 14 Apr 2013 09:08:23 +0200 Subject: [U-Boot] [PATCH v2] spi: mxc_spi: Set master mode for all channels In-Reply-To: <1365548785-28684-1-git-send-email-festevam@gmail.com> References: <1365548785-28684-1-git-send-email-festevam@gmail.com> Message-ID: <516A55E7.9000304@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Am 10.04.2013 01:06, schrieb Fabio Estevam: > From: Fabio Estevam > > 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 > --- > 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