| Message ID | 577A7D8D.8070004@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Sylwester, > +#if 0 > clocks = <&cmu_peric CLK_PCLK_SPI1>, > <&cmu_top CLK_SCLK_SPI1_PERIC>; > +#else > + clocks = <&cmu_peric CLK_PCLK_SPI1>, > + <&cmu_peric CLK_SCLK_SPI1>; > +#endif Yes, that's how it should be, indeed. > /* Disable Clock */ > if (sdd->port_conf->clk_from_cmu) { > - clk_disable_unprepare(sdd->src_clk); > + /* clk_disable_unprepare(sdd->src_clk); */ > } else { > val = readl(regs + S3C64XX_SPI_CLK_CFG); > val &= ~S3C64XX_SPI_ENCLK_ENABLE; > @@ -626,7 +626,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) > /* There is half-multiplier before the SPI */ > clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); > /* Enable Clock */ > - clk_prepare_enable(sdd->src_clk); > + /* clk_prepare_enable(sdd->src_clk); */ > } else { > /* Configure Clock */ > val = readl(regs + S3C64XX_SPI_CLK_CFG); I don't see anything wrong on the above. We could make it as: @@ -596,9 +597,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) u32 val; /* Disable Clock */ - if (sdd->port_conf->clk_from_cmu) { - clk_disable_unprepare(sdd->src_clk); - } else { + if (!sdd->port_conf->clk_from_cmu) { val = readl(regs + S3C64XX_SPI_CLK_CFG); val &= ~S3C64XX_SPI_ENCLK_ENABLE; writel(val, regs + S3C64XX_SPI_CLK_CFG); @@ -640,13 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) writel(val, regs + S3C64XX_SPI_MODE_CFG); - if (sdd->port_conf->clk_from_cmu) { - /* Configure Clock */ - /* There is half-multiplier before the SPI */ - clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); - /* Enable Clock */ - clk_prepare_enable(sdd->src_clk); - } else { + if (!sdd->port_conf->clk_from_cmu) { /* Configure Clock */ val = readl(regs + S3C64XX_SPI_CLK_CFG); val &= ~S3C64XX_SPI_PSR_MASK; > I meant we could amend which clocks are specified at the SPI bus device > DT nodes and change handling of clocks in the spi-s3c64xx driver to model > everything properly and get it all working. I think that if the clock comes from the cmu it's not necessary to disable it. I would like to avoid adding DTS properties because we have the clock disabling inherited from old code which it might not be required at all (in our tests, indeed it works). Thanks, Andi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/06/2016 06:51 AM, Andi Shyti wrote: > > I don't see anything wrong on the above. We could make it as: > @@ -640,13 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) > > writel(val, regs + S3C64XX_SPI_MODE_CFG); > > - if (sdd->port_conf->clk_from_cmu) { > - /* Configure Clock */ > - /* There is half-multiplier before the SPI */ > - clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); > - /* Enable Clock */ > - clk_prepare_enable(sdd->src_clk); clk_set_rate() call needs to stay, we can only remove clk_prepare_enable(). > - } else { > + if (!sdd->port_conf->clk_from_cmu) { > /* Configure Clock */ > val = readl(regs + S3C64XX_SPI_CLK_CFG); > val &= ~S3C64XX_SPI_PSR_MASK; > >> I meant we could amend which clocks are specified at the SPI bus device >> DT nodes and change handling of clocks in the spi-s3c64xx driver to model >> everything properly and get it all working. > > I think that if the clock comes from the cmu it's not necessary > to disable it. I would like to avoid adding DTS properties > because we have the clock disabling inherited from old code which > it might not be required at all (in our tests, indeed it works). OK, anyway we already need to amend exynos5433.dtsi file to change the "spi_busclk0" clock specifier. I agree we can get rid of the clock gating in s3c64xx_spi_config() function, it should not do any harm and will help in getting rid of the bus access exceptions. In general PCLK should be enough for accessing register of the controller, one hypothesis is that automatic clock gating may be disabling PCLK when it sees SCLK inactive. I tested on exynos4412 trats2 board a large firmware file upload over SPI and didn't notice any bad side effects with above clk_disable_unprepare()/clk_prepare_enable() calls commented out. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi index 8e124fc..f444c66 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi @@ -617,8 +617,13 @@ dma-names = "tx", "rx"; #address-cells = <1>; #size-cells = <0>; +#if 0 clocks = <&cmu_peric CLK_PCLK_SPI1>, <&cmu_top CLK_SCLK_SPI1_PERIC>; +#else + clocks = <&cmu_peric CLK_PCLK_SPI1>, + <&cmu_peric CLK_SCLK_SPI1>; +#endif clock-names = "spi", "spi_busclk0"; samsung,spi-src-clk = <0>; pinctrl-names = "default"; diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c index e3cc935..61d5643 100644 --- a/drivers/clk/samsung/clk-exynos5433.c +++ b/drivers/clk/samsung/clk-exynos5433.c @@ -1675,7 +1675,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = { GATE(CLK_SCLK_SPI2, "sclk_spi2", "sclk_spi2_peric", ENABLE_SCLK_PERIC, 5, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_SPI1, "sclk_spi1", "sclk_spi1_peric", ENABLE_SCLK_PERIC, - 4, CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0), + 4, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_SPI0, "sclk_spi0", "sclk_spi0_peric", ENABLE_SCLK_PERIC, 3, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_UART2, "sclk_uart2", "sclk_uart2_peric", diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 5a76a50..2cb965c 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -578,7 +578,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) /* Disable Clock */ if (sdd->port_conf->clk_from_cmu) { - clk_disable_unprepare(sdd->src_clk); + /* clk_disable_unprepare(sdd->src_clk); */ } else { val = readl(regs + S3C64XX_SPI_CLK_CFG); val &= ~S3C64XX_SPI_ENCLK_ENABLE; @@ -626,7 +626,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) /* There is half-multiplier before the SPI */ clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); /* Enable Clock */ - clk_prepare_enable(sdd->src_clk); + /* clk_prepare_enable(sdd->src_clk); */ } else { /* Configure Clock */ val = readl(regs + S3C64XX_SPI_CLK_CFG);