| Message ID | 1381839982-3026-1-git-send-email-ckeepax@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Charles, On Tuesday 15 of October 2013 13:26:22 Charles Keepax wrote: > Correct the SPI bus clock hookups to match their state before the switch > to this driver. With out this patch my system (based on an s3c6410) > can't locate any spibus clock. That's right, a patch like this is needed indeed, however... > Note, I only have a passing familiarity with this driver and subsystem > so this patch could probably use a careful eye. > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> > --- > drivers/clk/samsung/clk-s3c64xx.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/samsung/clk-s3c64xx.c > b/drivers/clk/samsung/clk-s3c64xx.c index 7d2c842..f6ac974 100644 > --- a/drivers/clk/samsung/clk-s3c64xx.c > +++ b/drivers/clk/samsung/clk-s3c64xx.c > @@ -356,8 +356,12 @@ static struct samsung_clock_alias > s3c64xx_clock_aliases[] = { ALIAS(SCLK_MMC2, "s3c-sdhci.2", > "mmc_busclk.2"), > ALIAS(SCLK_MMC1, "s3c-sdhci.1", "mmc_busclk.2"), > ALIAS(SCLK_MMC0, "s3c-sdhci.0", "mmc_busclk.2"), > - ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi-bus"), > - ALIAS(SCLK_SPI0, "s3c6410-spi.0", "spi-bus"), > + ALIAS(PCLK_SPI1, "s3c6410-spi.1", "spi_busclk0"), > + ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi_busclk1"), > + ALIAS(SCLK_SPI1_48, "s3c6410-spi.1", "spi_busclk2"), ...according to the documentation, the order is different. The SPI_CLKSEL field of CLK_CFG register of the SPI block can have following values: 0 - PCLK (aka PCLK_SPIx) 1 - USBCLK (aka SCLK_SPIx_48) 2 - Epll clock (aka SCLK_SPIx) The index after spi_busclk corresponds to the value written to SPI_CLKSEL field, so your patch should be adjusted accordingly. By the way, the USBCLK case is a bit strange, because it requires USB signal mask to be unmasked, which in turn needs USB PHY to be enabled, as otherwise some "unwanted leakage" can occur. Having to enable USB just to use SPI seems rather inconvenient (especially in terms of power consumption), so the usability of this clock is rather limited and it might be better not to let the driver know about it. Best regards, Tomasz P.S. Please keep linux-samsung-soc mailing list on Cc when sending patches for Samsung hardware.
+ samsung soc mailing list On Wed, Oct 16, 2013 at 12:52:42AM +0200, Tomasz Figa wrote: > Hi Charles, > > On Tuesday 15 of October 2013 13:26:22 Charles Keepax wrote: > > ALIAS(SCLK_MMC1, "s3c-sdhci.1", "mmc_busclk.2"), > > ALIAS(SCLK_MMC0, "s3c-sdhci.0", "mmc_busclk.2"), > > - ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi-bus"), > > - ALIAS(SCLK_SPI0, "s3c6410-spi.0", "spi-bus"), > > + ALIAS(PCLK_SPI1, "s3c6410-spi.1", "spi_busclk0"), > > + ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi_busclk1"), > > + ALIAS(SCLK_SPI1_48, "s3c6410-spi.1", "spi_busclk2"), > > ...according to the documentation, the order is different. The SPI_CLKSEL > field of CLK_CFG register of the SPI block can have following values: > 0 - PCLK (aka PCLK_SPIx) > 1 - USBCLK (aka SCLK_SPIx_48) > 2 - Epll clock (aka SCLK_SPIx) > > The index after spi_busclk corresponds to the value written to SPI_CLKSEL > field, so your patch should be adjusted accordingly. Hmm... will probably need to test this to see what happens the old clock setup was this: CLKDEV_INIT(NULL, "spi_busclk0", &clk_p), CLKDEV_INIT("s3c6410-spi.0", "spi_busclk1", &clk_sclk_spi0.clk), CLKDEV_INIT("s3c6410-spi.0", "spi_busclk2", &clk_48m_spi0), CLKDEV_INIT("s3c6410-spi.1", "spi_busclk1", &clk_sclk_spi1.clk), CLKDEV_INIT("s3c6410-spi.1", "spi_busclk2", &clk_48m_spi1), Which appears to differ from the documentation, that said though I would wager that only the first of those has really had much testing. > > By the way, the USBCLK case is a bit strange, because it requires USB > signal mask to be unmasked, which in turn needs USB PHY to be enabled, as > otherwise some "unwanted leakage" can occur. Having to enable USB just to > use SPI seems rather inconvenient (especially in terms of power > consumption), so the usability of this clock is rather limited and it > might be better not to let the driver know about it. Seems reasonable to drop this for now and it could be added back in if someone specifically needed to use it. Thanks, Charles
On Wednesday 16 of October 2013 09:10:35 Charles Keepax wrote: > + samsung soc mailing list > > On Wed, Oct 16, 2013 at 12:52:42AM +0200, Tomasz Figa wrote: > > Hi Charles, > > > > On Tuesday 15 of October 2013 13:26:22 Charles Keepax wrote: > > > ALIAS(SCLK_MMC1, "s3c-sdhci.1", "mmc_busclk.2"), > > > ALIAS(SCLK_MMC0, "s3c-sdhci.0", "mmc_busclk.2"), > > > - ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi-bus"), > > > - ALIAS(SCLK_SPI0, "s3c6410-spi.0", "spi-bus"), > > > + ALIAS(PCLK_SPI1, "s3c6410-spi.1", "spi_busclk0"), > > > + ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi_busclk1"), > > > + ALIAS(SCLK_SPI1_48, "s3c6410-spi.1", "spi_busclk2"), > > > > ...according to the documentation, the order is different. The SPI_CLKSEL > > field of CLK_CFG register of the SPI block can have following values: > > 0 - PCLK (aka PCLK_SPIx) > > 1 - USBCLK (aka SCLK_SPIx_48) > > 2 - Epll clock (aka SCLK_SPIx) > > > > The index after spi_busclk corresponds to the value written to SPI_CLKSEL > > field, so your patch should be adjusted accordingly. > > Hmm... will probably need to test this to see what happens the > old clock setup was this: > > CLKDEV_INIT(NULL, "spi_busclk0", &clk_p), > CLKDEV_INIT("s3c6410-spi.0", "spi_busclk1", &clk_sclk_spi0.clk), > CLKDEV_INIT("s3c6410-spi.0", "spi_busclk2", &clk_48m_spi0), > CLKDEV_INIT("s3c6410-spi.1", "spi_busclk1", &clk_sclk_spi1.clk), > CLKDEV_INIT("s3c6410-spi.1", "spi_busclk2", &clk_48m_spi1), > > Which appears to differ from the documentation, that said though > I would wager that only the first of those has really had much > testing. I believe in 90% cases the driver simply used first available clock, which would be the PCLK, so I wouldn't be really surprised if operation on other clock sources weren't even tested. If you have hardware to test this, especially with possibility of checking the SPI frequency I would really appreciate this, as I unfortunately don't have such. Best regards, Tomasz
On Wed, Oct 16, 2013 at 11:10:59AM +0200, Tomasz Figa wrote: > If you have hardware to test this, especially with possibility of checking > the SPI frequency I would really appreciate this, as I unfortunately don't > have such. I might not be able to get around to it this week, but I certainly should be able to test this on the hardware I have and measure the SPI speed. Thanks, Charles
diff --git a/drivers/clk/samsung/clk-s3c64xx.c b/drivers/clk/samsung/clk-s3c64xx.c index 7d2c842..f6ac974 100644 --- a/drivers/clk/samsung/clk-s3c64xx.c +++ b/drivers/clk/samsung/clk-s3c64xx.c @@ -356,8 +356,12 @@ static struct samsung_clock_alias s3c64xx_clock_aliases[] = { ALIAS(SCLK_MMC2, "s3c-sdhci.2", "mmc_busclk.2"), ALIAS(SCLK_MMC1, "s3c-sdhci.1", "mmc_busclk.2"), ALIAS(SCLK_MMC0, "s3c-sdhci.0", "mmc_busclk.2"), - ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi-bus"), - ALIAS(SCLK_SPI0, "s3c6410-spi.0", "spi-bus"), + ALIAS(PCLK_SPI1, "s3c6410-spi.1", "spi_busclk0"), + ALIAS(SCLK_SPI1, "s3c6410-spi.1", "spi_busclk1"), + ALIAS(SCLK_SPI1_48, "s3c6410-spi.1", "spi_busclk2"), + ALIAS(PCLK_SPI0, "s3c6410-spi.0", "spi_busclk0"), + ALIAS(SCLK_SPI0, "s3c6410-spi.0", "spi_busclk1"), + ALIAS(SCLK_SPI0_48, "s3c6410-spi.0", "spi_busclk2"), ALIAS(SCLK_AUDIO1, "samsung-pcm.1", "audio-bus"), ALIAS(SCLK_AUDIO1, "samsung-i2s.1", "audio-bus"), ALIAS(SCLK_AUDIO0, "samsung-pcm.0", "audio-bus"),
Correct the SPI bus clock hookups to match their state before the switch to this driver. With out this patch my system (based on an s3c6410) can't locate any spibus clock. Note, I only have a passing familiarity with this driver and subsystem so this patch could probably use a careful eye. Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> --- drivers/clk/samsung/clk-s3c64xx.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)