[PATCH v2 0/4] spi support for Exynos Auto v9 SoC

[PATCH v2 0/4] spi support for Exynos Auto v9 SoC

[Chanho Park]

Add to support Exynos Auto v9 SoC's spi. By supporting USI(Universal
Serial Interface) mode, the SoC can support up to 12 spi ports. Thus, we
need to increase MAX_SPI_PORTS from 6 to 12. The spi of the SoC can
support loopback mode unlike previous exynos SoCs. To separate the
feature, we need to add .has_loopback to the s3c64xx_spi_port_config.
Furthermore, it uses 4 as the default internal clock divider. We also
need to clk_div field of the structure and assign "2" as the default
value to the existing SoC's port config.
Device tree definitions of exynosautov9-spi will be added in separated
patchset to include usi(i2c/uart/spi) nodes all together.

Changes from v1:
- Patch #1 "increase MAX_SPI_PORTS to 12" has been squashed to the patch #4
- Add Krzysztof's RB tags for #1 and #3 patches
- Assign clk_div value to 2 for existing SoC's port configs
- Make const of exynosautov9_spi_port_config

Chanho Park (4):
  spi: s3c64xx: support loopback mode
  spi: s3c64xx: support custom value of internal clock divider
  dt-bindings: samsung,spi: define exynosautov9 compatible
  spi: s3c64xx: add spi port configuration for Exynos Auto v9 SoC

 .../devicetree/bindings/spi/samsung,spi.yaml  |  5 +-
 drivers/spi/spi-s3c64xx.c                     | 54 +++++++++++++++----
 2 files changed, 49 insertions(+), 10 deletions(-)

-- 
2.36.1

[PATCH v2 1/4] spi: s3c64xx: support loopback mode

[Chanho Park]

Modern exynos SoCs can support self loopback mode via setting BIT(3) of
MODE_CFG register. Previous SoCs don't have the bit so we need to add
has_loopback field in the s3c64xx_spi_port_config. Exynos Auto v9 SoC
has the bit and it will define the field to "true".
When it is set, SPI_LOOP mode will be marked.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 7f0faf0d75d9..b3c50c7665fc 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -59,6 +59,7 @@
 #define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD	(1<<17)
 #define S3C64XX_SPI_MODE_BUS_TSZ_WORD		(2<<17)
 #define S3C64XX_SPI_MODE_BUS_TSZ_MASK		(3<<17)
+#define S3C64XX_SPI_MODE_SELF_LOOPBACK		(1<<3)
 #define S3C64XX_SPI_MODE_RXDMA_ON		(1<<2)
 #define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
 #define S3C64XX_SPI_MODE_4BURST			(1<<0)
@@ -135,6 +136,7 @@ struct s3c64xx_spi_dma_data {
  * @clk_from_cmu: True, if the controller does not include a clock mux and
  *	prescaler unit.
  * @clk_ioclk: True if clock is present on this device
+ * @has_loopback: True if loopback mode can be supported
  *
  * The Samsung s3c64xx SPI controller are used on various Samsung SoC's but
  * differ in some aspects such as the size of the fifo and spi bus clock
@@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
 	bool	high_speed;
 	bool	clk_from_cmu;
 	bool	clk_ioclk;
+	bool	has_loopback;
 };
 
 /**
@@ -659,6 +662,9 @@ static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 		break;
 	}
 
+	if ((sdd->cur_mode & SPI_LOOP) && sdd->port_conf->has_loopback)
+		val |= S3C64XX_SPI_MODE_SELF_LOOPBACK;
+
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	if (sdd->port_conf->clk_from_cmu) {
@@ -1148,6 +1154,8 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 					SPI_BPW_MASK(8);
 	/* the spi->mode bits understood by this driver: */
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+	if (sdd->port_conf->has_loopback)
+		master->mode_bits |= SPI_LOOP;
 	master->auto_runtime_pm = true;
 	if (!is_polling(sdd))
 		master->can_dma = s3c64xx_spi_can_dma;
-- 
2.36.1

Re: [PATCH v2 1/4] spi: s3c64xx: support loopback mode

[Andi Shyti]

Hi Chanho,

On Tue, Jun 28, 2022 at 01:42:19PM +0900, Chanho Park wrote:
> Modern exynos SoCs can support self loopback mode via setting BIT(3) of
> MODE_CFG register. Previous SoCs don't have the bit so we need to add
> has_loopback field in the s3c64xx_spi_port_config. Exynos Auto v9 SoC
> has the bit and it will define the field to "true".
> When it is set, SPI_LOOP mode will be marked.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Reviewed-by: Andi Shyti <andi@etezian.org>

Thanks,
Andi

[PATCH v2 2/4] spi: s3c64xx: support custom value of internal clock divider

[Chanho Park]

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;
 
 	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,
-- 
2.36.1

Re: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal clock divider

[Krzysztof Kozlowski]

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

RE: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal clock divider

[Chanho Park]

> > 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

Re: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal clock divider

[Andi Shyti]

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

RE: [PATCH v2 2/4] spi: s3c64xx: support custom value of internal clock divider

[Chanho Park]

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

[PATCH v2 3/4] dt-bindings: samsung,spi: define exynosautov9 compatible

[Chanho Park]

Define "samsung,exynosautov9-spi" for Exynos Auto v9's spi.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 Documentation/devicetree/bindings/spi/samsung,spi.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
index a50f24f9359d..d51ee3e6d604 100644
--- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
+++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
@@ -21,6 +21,7 @@ properties:
           - samsung,s3c6410-spi
           - samsung,s5pv210-spi # for S5PV210 and S5PC110
           - samsung,exynos5433-spi
+          - samsung,exynosautov9-spi
           - tesla,fsd-spi
       - const: samsung,exynos7-spi
         deprecated: true
@@ -85,7 +86,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: samsung,exynos5433-spi
+            enum:
+              - samsung,exynos5433-spi
+              - samsung,exynosautov9-spi
     then:
       properties:
         clocks:
-- 
2.36.1

Re: [PATCH v2 3/4] dt-bindings: samsung,spi: define exynosautov9 compatible

[Andi Shyti]

Hi Chanho,

On Tue, Jun 28, 2022 at 01:42:21PM +0900, Chanho Park wrote:
> Define "samsung,exynosautov9-spi" for Exynos Auto v9's spi.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Reviewed-by: Andi Shyti <andi@etezian.org>

Thanks,
Andi

[PATCH v2 4/4] spi: s3c64xx: add spi port configuration for Exynos Auto v9 SoC

[Chanho Park]

Add exynosautov9 spi port configuration. It supports up to 12 spis so
MAX_SPI_PORTS should be increased from 6 to 12.
It has DIV_4 as the default internal clock divider and an internal
loopback mode to run a loopback test.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 51a0e830441b..0c9e19889809 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -18,7 +18,7 @@
 
 #include <linux/platform_data/spi-s3c64xx.h>
 
-#define MAX_SPI_PORTS		6
+#define MAX_SPI_PORTS		12
 #define S3C64XX_SPI_QUIRK_POLL		(1 << 0)
 #define S3C64XX_SPI_QUIRK_CS_AUTO	(1 << 1)
 #define AUTOSUSPEND_TIMEOUT	2000
@@ -1451,6 +1451,19 @@ static const struct s3c64xx_spi_port_config exynos5433_spi_port_config = {
 	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
+static const struct s3c64xx_spi_port_config exynosautov9_spi_port_config = {
+	.fifo_lvl_mask	= { 0x1ff, 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff, 0x7f,
+			    0x7f, 0x7f, 0x7f, 0x7f},
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.clk_div	= 4,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.clk_ioclk	= true,
+	.has_loopback	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
+};
+
 static struct s3c64xx_spi_port_config fsd_spi_port_config = {
 	.fifo_lvl_mask	= { 0x7f, 0x7f, 0x7f, 0x7f, 0x7f},
 	.rx_lvl_offset	= 15,
@@ -1492,6 +1505,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = {
 	{ .compatible = "samsung,exynos5433-spi",
 			.data = (void *)&exynos5433_spi_port_config,
 	},
+	{ .compatible = "samsung,exynosautov9-spi",
+			.data = (void *)&exynosautov9_spi_port_config,
+	},
 	{ .compatible = "tesla,fsd-spi",
 			.data = (void *)&fsd_spi_port_config,
 	},
-- 
2.36.1

Re: [PATCH v2 4/4] spi: s3c64xx: add spi port configuration for Exynos Auto v9 SoC

[Krzysztof Kozlowski]

On 28/06/2022 06:42, Chanho Park wrote:
> Add exynosautov9 spi port configuration. It supports up to 12 spis so
> MAX_SPI_PORTS should be increased from 6 to 12.
> It has DIV_4 as the default internal clock divider and an internal
> loopback mode to run a loopback test.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 4/4] spi: s3c64xx: add spi port configuration for Exynos Auto v9 SoC

[Andi Shyti]

Hi Chanho,

On Tue, Jun 28, 2022 at 01:42:22PM +0900, Chanho Park wrote:
> Add exynosautov9 spi port configuration. It supports up to 12 spis so
> MAX_SPI_PORTS should be increased from 6 to 12.
> It has DIV_4 as the default internal clock divider and an internal
> loopback mode to run a loopback test.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Reviewed-by: Andi Shyti <andi@etezian.org>

Thanks,
Andi