| Message ID | 20220628044222.152794-3-chanho61.park@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] spi: s3c64xx: support loopback mode | expand |
On 28/06/2022 06:42, Chanho Park wrote: > Modern exynos SoCs such as Exynos Auto v9 has different internal clock > divider, for example "4". To support this internal value, this adds > clk_div of the s3c64xx_spi_port_config and assign "2" as the default > value to existing s3c64xx_spi_port_config. > > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > --- > drivers/spi/spi-s3c64xx.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index b3c50c7665fc..51a0e830441b 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -131,6 +131,7 @@ struct s3c64xx_spi_dma_data { > * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS register. > * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. > * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. > + * @clk_div: Internal clock divider, if not specified, use 2 as the default. > * @quirks: Bitmask of known quirks > * @high_speed: True, if the controller supports HIGH_SPEED_EN bit. > * @clk_from_cmu: True, if the controller does not include a clock mux and > @@ -148,6 +149,7 @@ struct s3c64xx_spi_port_config { > int rx_lvl_offset; > int tx_st_done; > int quirks; > + int clk_div; > bool high_speed; > bool clk_from_cmu; > bool clk_ioclk; > @@ -620,6 +622,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) > void __iomem *regs = sdd->regs; > int ret; > u32 val; > + u32 div = sdd->port_conf->clk_div; > > /* Disable Clock */ > if (!sdd->port_conf->clk_from_cmu) { > @@ -668,16 +671,15 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) > writel(val, regs + S3C64XX_SPI_MODE_CFG); > > if (sdd->port_conf->clk_from_cmu) { > - /* The src_clk clock is divided internally by 2 */ > - ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); > + ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * div); > if (ret) > return ret; > - sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2; > + sdd->cur_speed = clk_get_rate(sdd->src_clk) / div; > } else { > /* Configure Clock */ > val = readl(regs + S3C64XX_SPI_CLK_CFG); > val &= ~S3C64XX_SPI_PSR_MASK; > - val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1) > + val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / div - 1) > & S3C64XX_SPI_PSR_MASK); > writel(val, regs + S3C64XX_SPI_CLK_CFG); > > @@ -871,6 +873,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > struct s3c64xx_spi_csinfo *cs = spi->controller_data; > struct s3c64xx_spi_driver_data *sdd; > int err; > + u32 div = 2; This assignment is not effective - shortly later is being overwritten. > > sdd = spi_master_get_devdata(spi->master); > if (spi->dev.of_node) { > @@ -889,22 +892,24 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > > pm_runtime_get_sync(&sdd->pdev->dev); > > + div = sdd->port_conf->clk_div; > + Best regards, Krzysztof
> > Modern exynos SoCs such as Exynos Auto v9 has different internal clock > > divider, for example "4". To support this internal value, this adds > > clk_div of the s3c64xx_spi_port_config and assign "2" as the default > > value to existing s3c64xx_spi_port_config. > > > > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > > --- > > drivers/spi/spi-s3c64xx.c | 28 ++++++++++++++++++++-------- > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > > index b3c50c7665fc..51a0e830441b 100644 > > --- a/drivers/spi/spi-s3c64xx.c > > +++ b/drivers/spi/spi-s3c64xx.c > > @@ -131,6 +131,7 @@ struct s3c64xx_spi_dma_data { > > * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS > register. > > * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. > > * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. > > + * @clk_div: Internal clock divider, if not specified, use 2 as the > default. > > * @quirks: Bitmask of known quirks > > * @high_speed: True, if the controller supports HIGH_SPEED_EN bit. > > * @clk_from_cmu: True, if the controller does not include a clock > > mux and @@ -148,6 +149,7 @@ struct s3c64xx_spi_port_config { > > int rx_lvl_offset; > > int tx_st_done; > > int quirks; > > + int clk_div; > > bool high_speed; > > bool clk_from_cmu; > > bool clk_ioclk; > > @@ -620,6 +622,7 @@ static int s3c64xx_spi_config(struct > s3c64xx_spi_driver_data *sdd) > > void __iomem *regs = sdd->regs; > > int ret; > > u32 val; > > + u32 div = sdd->port_conf->clk_div; > > > > /* Disable Clock */ > > if (!sdd->port_conf->clk_from_cmu) { @@ -668,16 +671,15 @@ static > > int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) > > writel(val, regs + S3C64XX_SPI_MODE_CFG); > > > > if (sdd->port_conf->clk_from_cmu) { > > - /* The src_clk clock is divided internally by 2 */ > > - ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); > > + ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * div); > > if (ret) > > return ret; > > - sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2; > > + sdd->cur_speed = clk_get_rate(sdd->src_clk) / div; > > } else { > > /* Configure Clock */ > > val = readl(regs + S3C64XX_SPI_CLK_CFG); > > val &= ~S3C64XX_SPI_PSR_MASK; > > - val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1) > > + val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / div - > 1) > > & S3C64XX_SPI_PSR_MASK); > > writel(val, regs + S3C64XX_SPI_CLK_CFG); > > > > @@ -871,6 +873,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > > struct s3c64xx_spi_csinfo *cs = spi->controller_data; > > struct s3c64xx_spi_driver_data *sdd; > > int err; > > + u32 div = 2; > > This assignment is not effective - shortly later is being overwritten. I forgot to remove this. I'll drop the assignment. Best Regards, Chanho Park
Hi Chanho, On Tue, Jun 28, 2022 at 01:42:20PM +0900, Chanho Park wrote: > Modern exynos SoCs such as Exynos Auto v9 has different internal clock /has/have/ > divider, for example "4". To support this internal value, this adds > clk_div of the s3c64xx_spi_port_config and assign "2" as the default > value to existing s3c64xx_spi_port_config. > > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > --- > drivers/spi/spi-s3c64xx.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > index b3c50c7665fc..51a0e830441b 100644 > --- a/drivers/spi/spi-s3c64xx.c > +++ b/drivers/spi/spi-s3c64xx.c > @@ -131,6 +131,7 @@ struct s3c64xx_spi_dma_data { > * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS register. > * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. > * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. > + * @clk_div: Internal clock divider, if not specified, use 2 as the default. is it default? Is it not specified anywhere? I think you are assigning '2' to everyone. I would just leave it "Internal clock divider." [...] > @@ -871,6 +873,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > struct s3c64xx_spi_csinfo *cs = spi->controller_data; > struct s3c64xx_spi_driver_data *sdd; > int err; > + u32 div = 2; As per Krzystof review. > sdd = spi_master_get_devdata(spi->master); > if (spi->dev.of_node) { > @@ -889,22 +892,24 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > > pm_runtime_get_sync(&sdd->pdev->dev); > > + div = sdd->port_conf->clk_div; Can you please be consistent with the data type? div is u32, but clk_div is int. [...] Andi
Hi Andy, Thanks for your reviews :) > -----Original Message----- > From: Andi Shyti <andi@etezian.org> > Sent: Wednesday, June 29, 2022 6:43 PM > To: Chanho Park <chanho61.park@samsung.com> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Andi Shyti > <andi@etezian.org>; Mark Brown <broonie@kernel.org>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Alim Akhtar <alim.akhtar@samsung.com>; > devicetree@vger.kernel.org; linux-spi@vger.kernel.org; linux-samsung- > soc@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal > clock divider > > Hi Chanho, > > On Tue, Jun 28, 2022 at 01:42:20PM +0900, Chanho Park wrote: > > Modern exynos SoCs such as Exynos Auto v9 has different internal clock > > /has/have/ I'll correct it. > > > divider, for example "4". To support this internal value, this adds > > clk_div of the s3c64xx_spi_port_config and assign "2" as the default > > value to existing s3c64xx_spi_port_config. > > > > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > > --- > > drivers/spi/spi-s3c64xx.c | 28 ++++++++++++++++++++-------- > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > > index b3c50c7665fc..51a0e830441b 100644 > > --- a/drivers/spi/spi-s3c64xx.c > > +++ b/drivers/spi/spi-s3c64xx.c > > @@ -131,6 +131,7 @@ struct s3c64xx_spi_dma_data { > > * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS > register. > > * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. > > * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. > > + * @clk_div: Internal clock divider, if not specified, use 2 as the > default. > > is it default? Is it not specified anywhere? I think you are assigning '2' > to everyone. I would just leave it "Internal clock divider." It has not been removed since v1. > > [...] > > > @@ -871,6 +873,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi) > > struct s3c64xx_spi_csinfo *cs = spi->controller_data; > > struct s3c64xx_spi_driver_data *sdd; > > int err; > > + u32 div = 2; > > As per Krzystof review. > > > sdd = spi_master_get_devdata(spi->master); > > if (spi->dev.of_node) { > > @@ -889,22 +892,24 @@ static int s3c64xx_spi_setup(struct spi_device > > *spi) > > > > pm_runtime_get_sync(&sdd->pdev->dev); > > > > + div = sdd->port_conf->clk_div; > > Can you please be consistent with the data type? div is u32, but clk_div > is int. It should be int to be matched with any other types of s3c64xx_spi_port_config. Best Regards, Chanho Park
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index b3c50c7665fc..51a0e830441b 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -131,6 +131,7 @@ struct s3c64xx_spi_dma_data { * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS register. * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter. * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter. + * @clk_div: Internal clock divider, if not specified, use 2 as the default. * @quirks: Bitmask of known quirks * @high_speed: True, if the controller supports HIGH_SPEED_EN bit. * @clk_from_cmu: True, if the controller does not include a clock mux and @@ -148,6 +149,7 @@ struct s3c64xx_spi_port_config { int rx_lvl_offset; int tx_st_done; int quirks; + int clk_div; bool high_speed; bool clk_from_cmu; bool clk_ioclk; @@ -620,6 +622,7 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) void __iomem *regs = sdd->regs; int ret; u32 val; + u32 div = sdd->port_conf->clk_div; /* Disable Clock */ if (!sdd->port_conf->clk_from_cmu) { @@ -668,16 +671,15 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) writel(val, regs + S3C64XX_SPI_MODE_CFG); if (sdd->port_conf->clk_from_cmu) { - /* The src_clk clock is divided internally by 2 */ - ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); + ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * div); if (ret) return ret; - sdd->cur_speed = clk_get_rate(sdd->src_clk) / 2; + sdd->cur_speed = clk_get_rate(sdd->src_clk) / div; } else { /* Configure Clock */ val = readl(regs + S3C64XX_SPI_CLK_CFG); val &= ~S3C64XX_SPI_PSR_MASK; - val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / 2 - 1) + val |= ((clk_get_rate(sdd->src_clk) / sdd->cur_speed / div - 1) & S3C64XX_SPI_PSR_MASK); writel(val, regs + S3C64XX_SPI_CLK_CFG); @@ -871,6 +873,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi) struct s3c64xx_spi_csinfo *cs = spi->controller_data; struct s3c64xx_spi_driver_data *sdd; int err; + u32 div = 2; sdd = spi_master_get_devdata(spi->master); if (spi->dev.of_node) { @@ -889,22 +892,24 @@ static int s3c64xx_spi_setup(struct spi_device *spi) pm_runtime_get_sync(&sdd->pdev->dev); + div = sdd->port_conf->clk_div; + /* Check if we can provide the requested rate */ if (!sdd->port_conf->clk_from_cmu) { u32 psr, speed; /* Max possible */ - speed = clk_get_rate(sdd->src_clk) / 2 / (0 + 1); + speed = clk_get_rate(sdd->src_clk) / div / (0 + 1); if (spi->max_speed_hz > speed) spi->max_speed_hz = speed; - psr = clk_get_rate(sdd->src_clk) / 2 / spi->max_speed_hz - 1; + psr = clk_get_rate(sdd->src_clk) / div / spi->max_speed_hz - 1; psr &= S3C64XX_SPI_PSR_MASK; if (psr == S3C64XX_SPI_PSR_MASK) psr--; - speed = clk_get_rate(sdd->src_clk) / 2 / (psr + 1); + speed = clk_get_rate(sdd->src_clk) / div / (psr + 1); if (spi->max_speed_hz < speed) { if (psr+1 < S3C64XX_SPI_PSR_MASK) { psr++; @@ -914,7 +919,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi) } } - speed = clk_get_rate(sdd->src_clk) / 2 / (psr + 1); + speed = clk_get_rate(sdd->src_clk) / div / (psr + 1); if (spi->max_speed_hz >= speed) { spi->max_speed_hz = speed; } else { @@ -1396,6 +1401,7 @@ static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = { .fifo_lvl_mask = { 0x7f }, .rx_lvl_offset = 13, .tx_st_done = 21, + .clk_div = 2, .high_speed = true, }; @@ -1403,12 +1409,14 @@ static const struct s3c64xx_spi_port_config s3c6410_spi_port_config = { .fifo_lvl_mask = { 0x7f, 0x7F }, .rx_lvl_offset = 13, .tx_st_done = 21, + .clk_div = 2, }; static const struct s3c64xx_spi_port_config s5pv210_spi_port_config = { .fifo_lvl_mask = { 0x1ff, 0x7F }, .rx_lvl_offset = 15, .tx_st_done = 25, + .clk_div = 2, .high_speed = true, }; @@ -1416,6 +1424,7 @@ static const struct s3c64xx_spi_port_config exynos4_spi_port_config = { .fifo_lvl_mask = { 0x1ff, 0x7F, 0x7F }, .rx_lvl_offset = 15, .tx_st_done = 25, + .clk_div = 2, .high_speed = true, .clk_from_cmu = true, .quirks = S3C64XX_SPI_QUIRK_CS_AUTO, @@ -1425,6 +1434,7 @@ static const struct s3c64xx_spi_port_config exynos7_spi_port_config = { .fifo_lvl_mask = { 0x1ff, 0x7F, 0x7F, 0x7F, 0x7F, 0x1ff}, .rx_lvl_offset = 15, .tx_st_done = 25, + .clk_div = 2, .high_speed = true, .clk_from_cmu = true, .quirks = S3C64XX_SPI_QUIRK_CS_AUTO, @@ -1434,6 +1444,7 @@ static const struct s3c64xx_spi_port_config exynos5433_spi_port_config = { .fifo_lvl_mask = { 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff}, .rx_lvl_offset = 15, .tx_st_done = 25, + .clk_div = 2, .high_speed = true, .clk_from_cmu = true, .clk_ioclk = true, @@ -1444,6 +1455,7 @@ static struct s3c64xx_spi_port_config fsd_spi_port_config = { .fifo_lvl_mask = { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f}, .rx_lvl_offset = 15, .tx_st_done = 25, + .clk_div = 2, .high_speed = true, .clk_from_cmu = true, .clk_ioclk = false,
Modern exynos SoCs such as Exynos Auto v9 has different internal clock divider, for example "4". To support this internal value, this adds clk_div of the s3c64xx_spi_port_config and assign "2" as the default value to existing s3c64xx_spi_port_config. Signed-off-by: Chanho Park <chanho61.park@samsung.com> --- drivers/spi/spi-s3c64xx.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)