[PATCH] spi: s3c64xx: Get fifosize via device tree

[PATCH] spi: s3c64xx: Get fifosize via device tree

[Youngmin Nam]

SPI driver on some SoCs only differ in the fifosize of each
SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure
or create a compatible name for such a change.

We can get fifosize via the device tree nodes (not mandatory).
Also the device tree binding document was updated.

Signed-off-by: Youngmin Nam <ym0914-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt |  6 ++++++
 drivers/spi/spi-s3c64xx.c                             | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 6dbdeb3..5c4a08d 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -23,6 +23,11 @@ Required SoC Specific Properties:
 - dma-names: Names for the dma channels. There must be at least one channel
   named "tx" for transmit and named "rx" for receive.
 
+Optional SoC Specific properties:
+
+- samsung,spi-fifosize: The fifo size supported by the SPI channel
+
+
 Required Board Specific Properties:
 
 - #address-cells: should be 1.
@@ -73,6 +78,7 @@ Example:
 		dma-names = "tx", "rx";
 		#address-cells = <1>;
 		#size-cells = <0>;
+		samsung,spi-fifosize = <64>;
 	};
 
 - Board Specific Portion:
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 5a76a50..44f1aeb 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1032,6 +1032,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	struct spi_master *master;
 	int ret, irq;
 	char clk_name[16];
+	int fifosize;
 
 	if (!sci && pdev->dev.of_node) {
 		sci = s3c64xx_spi_parse_dt(&pdev->dev);
@@ -1083,6 +1084,18 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		sdd->port_id = pdev->id;
 	}
 
+	if (pdev->dev.of_node) {
+		if (of_property_read_u32(pdev->dev.of_node,
+			"samsung,spi-fifosize", &fifosize)) {
+		} else {
+			sdd->port_conf->fifo_lvl_mask[sdd->port_id] =
+							(fifosize << 1) - 1;
+			dev_info(&pdev->dev, "PORT %d fifo_lvl_mask = 0x%x\n",
+				sdd->port_id,
+				sdd->port_conf->fifo_lvl_mask[sdd->port_id]);
+		}
+	}
+
 	sdd->cur_bpw = 8;
 
 	if (!sdd->pdev->dev.of_node && (!sci->dma_tx || !sci->dma_rx)) {
-- 
2.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On Wed, Feb 10, 2016 at 12:26:25AM +0900, Youngmin Nam wrote:

> SPI driver on some SoCs only differ in the fifosize of each
> SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure
> or create a compatible name for such a change.

I disagree that it is useless to do this, it means that if we realize
later that there is some other difference between the implementations
then we have the information in the DT to handle this without needing 
to update the ABI.

> +Optional SoC Specific properties:
> +
> +- samsung,spi-fifosize: The fifo size supported by the SPI channel
> +
> +

In units of...?  FIFO sizes do get quoted in both words and bytes so it
seems good to be clear.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Youngmin Nam]

Hello Mark.
Thank you for your review.
Please ignore previous mail that I sent to you.
I sent this mail without CC-ing. I resend including CC list.

On 2016년 02월 10일 01:10, Mark Brown wrote:
> On Wed, Feb 10, 2016 at 12:26:25AM +0900, Youngmin Nam wrote:
> 
>> SPI driver on some SoCs only differ in the fifosize of each
>> SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure
>> or create a compatible name for such a change.
> 
> I disagree that it is useless to do this, it means that if we realize
> later that there is some other difference between the implementations
> then we have the information in the DT to handle this without needing 
> to update the ABI.
> 

To clarify, let me check that I understood.
Do you mean we can fix fifosize of each SPI channel with DT handling
if there is difference on this fifosize with driver code?

If I understand your words correctly, let me modify commit messages. 

>> +Optional SoC Specific properties:
>> +
>> +- samsung,spi-fifosize: The fifo size supported by the SPI channel
>> +
>> +
> 
> In units of...?  FIFO sizes do get quoted in both words and bytes so it
> seems good to be clear.
> 

"samsung,spi-fifosize = <64>" means 64 bytes of fifosize.
Let me add the units as bytes.

Thank you. 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On Sun, Feb 14, 2016 at 03:28:30PM +0900, Youngmin Nam wrote:
> On 2016년 02월 10일 01:10, Mark Brown wrote:
> > On Wed, Feb 10, 2016 at 12:26:25AM +0900, Youngmin Nam wrote:

> > I disagree that it is useless to do this, it means that if we realize
> > later that there is some other difference between the implementations
> > then we have the information in the DT to handle this without needing 
> > to update the ABI.

> To clarify, let me check that I understood.
> Do you mean we can fix fifosize of each SPI channel with DT handling
> if there is difference on this fifosize with driver code?
> 
> If I understand your words correctly, let me modify commit messages. 

I'm having a hard time understanding what you're saying here but it
sounds like you've understood the oppositve of my intention here.  I'm
saying that it's not clear to me that this is better than adding more
compatibles.  At the very least we need a new compatible which requires
this property as Krzysztof was suggesting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Krzysztof Kozlowski]

W dniu 10.02.2016 o 00:26, Youngmin Nam pisze:
> SPI driver on some SoCs only differ in the fifosize of each
> SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure
> or create a compatible name for such a change.
> 
> We can get fifosize via the device tree nodes (not mandatory).
> Also the device tree binding document was updated.
> 
> Signed-off-by: Youngmin Nam <ym0914-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-samsung.txt |  6 ++++++
>  drivers/spi/spi-s3c64xx.c                             | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
> index 6dbdeb3..5c4a08d 100644
> --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
> @@ -23,6 +23,11 @@ Required SoC Specific Properties:
>  - dma-names: Names for the dma channels. There must be at least one channel
>    named "tx" for transmit and named "rx" for receive.
>  
> +Optional SoC Specific properties:
> +
> +- samsung,spi-fifosize: The fifo size supported by the SPI channel
> +
> +
>  Required Board Specific Properties:
>  
>  - #address-cells: should be 1.
> @@ -73,6 +78,7 @@ Example:
>  		dma-names = "tx", "rx";
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> +		samsung,spi-fifosize = <64>;
>  	};
>  

That does not look good. Is it a configurable part of SoC? I think
not...  rather it must contain exact value supported by given device...
So this should be part of compatible because AFAIU you just made
compatible devices uncompatible...

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Youngmin Nam]

Hello Krzysztof,

As you mentioned, spi fifosize is not configurable in the given SoC.
The point is we can set fifosize without changing driver code.
For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current
our compatible, we can't cover this situation without adding new compatible into spi driver code.
Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change.
I think this is not efficient. I think we can reduces this works through DT handling.

Thanks.

On 2016년 02월 14일 16:01, Krzysztof Kozlowski wrote:
> W dniu 10.02.2016 o 00:26, Youngmin Nam pisze:
>> SPI driver on some SoCs only differ in the fifosize of each
>> SPI channel. It is useless to duplicate the s3c64xx_spi_port_config structure
>> or create a compatible name for such a change.
>>
>> We can get fifosize via the device tree nodes (not mandatory).
>> Also the device tree binding document was updated.
>>
>> Signed-off-by: Youngmin Nam <ym0914-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/spi/spi-samsung.txt |  6 ++++++
>>  drivers/spi/spi-s3c64xx.c                             | 13 +++++++++++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
>> index 6dbdeb3..5c4a08d 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
>> @@ -23,6 +23,11 @@ Required SoC Specific Properties:
>>  - dma-names: Names for the dma channels. There must be at least one channel
>>    named "tx" for transmit and named "rx" for receive.
>>  
>> +Optional SoC Specific properties:
>> +
>> +- samsung,spi-fifosize: The fifo size supported by the SPI channel
>> +
>> +
>>  Required Board Specific Properties:
>>  
>>  - #address-cells: should be 1.
>> @@ -73,6 +78,7 @@ Example:
>>  		dma-names = "tx", "rx";
>>  		#address-cells = <1>;
>>  		#size-cells = <0>;
>> +		samsung,spi-fifosize = <64>;
>>  	};
>>  
> 
> That does not look good. Is it a configurable part of SoC? I think
> not...  rather it must contain exact value supported by given device...
> So this should be part of compatible because AFAIU you just made
> compatible devices uncompatible...
> 
> Best regards,
> Krzysztof
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Krzysztof Kozlowski]

W dniu 14.02.2016 o 17:13, Youngmin Nam pisze:
> Hello Krzysztof,
> 
> As you mentioned, spi fifosize is not configurable in the given SoC.
> The point is we can set fifosize without changing driver code.
> For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current
> our compatible, we can't cover this situation without adding new compatible into spi driver code.

Yes, because new SoC is not compatible with old ones... so a new
compatible is required!

> Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change.
> I think this is not efficient. I think we can reduces this works through DT handling.

I agree that this is not the most efficient possible way of setting some
specific properties of devices but this is the way how DT works.

What if the property is not present on board with Exynos7? What value
should be used?

You did not want to add a new compatible but you are adding a
compatible-like property which apparently is required for some devices.

That looks bad. It's easy to make a mistake, messes with compatibles.

Best regards,
Krzysztof

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Youngmin Nam]

When I think about this patch, I was motivated by below patch.

commit 135f07c3252dc77d0245714d0b413ecc711cd823
Author: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Date:   Mon Jul 14 17:07:16 2014 +0530

    serial: samsung: get fifosize via device tree
    
    UART modules on some SoCs only differ in the fifosize of each
    UART channel. Its useless to duplicate the drv_data structure
    or create a compatible name for such a change.
    
    We can get fifosize via the device tree nodes (not mandating it).
    
    Also updates the documentation.
    
    Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
    Reviewed-by: Tomasz Figa <t.figa@samsung.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 

I thought we can apply above patch to SPI also.
The fifosize should be required for fifo_lvl_mask value in SPI of Exynos series.
I think this patch is useful when we port for the new exynos7xxx SoC come out.
Because SPI of new exynos7xxx SoC has only difference on fifosize. 

Thanks.


On 2016년 02월 14일 17:31, Krzysztof Kozlowski wrote:
> W dniu 14.02.2016 o 17:13, Youngmin Nam pisze:
>> Hello Krzysztof,
>>
>> As you mentioned, spi fifosize is not configurable in the given SoC.
>> The point is we can set fifosize without changing driver code.
>> For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current
>> our compatible, we can't cover this situation without adding new compatible into spi driver code.
> 
> Yes, because new SoC is not compatible with old ones... so a new
> compatible is required!
> 
>> Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change.
>> I think this is not efficient. I think we can reduces this works through DT handling.
> 
> I agree that this is not the most efficient possible way of setting some
> specific properties of devices but this is the way how DT works.
> 
> What if the property is not present on board with Exynos7? What value
> should be used?
> 
> You did not want to add a new compatible but you are adding a
> compatible-like property which apparently is required for some devices.
> 
> That looks bad. It's easy to make a mistake, messes with compatibles.
> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH] spi: s3c64xx: Get fifosize via device tree

[Krzysztof Kozlowski]

2016-02-14 22:22 GMT+09:00 Youngmin Nam <ym0914-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> When I think about this patch, I was motivated by below patch.
>
> commit 135f07c3252dc77d0245714d0b413ecc711cd823
> Author: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Date:   Mon Jul 14 17:07:16 2014 +0530
>
>     serial: samsung: get fifosize via device tree
>
>     UART modules on some SoCs only differ in the fifosize of each
>     UART channel. Its useless to duplicate the drv_data structure
>     or create a compatible name for such a change.
>
>     We can get fifosize via the device tree nodes (not mandating it).
>
>     Also updates the documentation.
>
>     Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>     Reviewed-by: Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>     Signed-off-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>
> I thought we can apply above patch to SPI also.
> The fifosize should be required for fifo_lvl_mask value in SPI of Exynos series.
> I think this patch is useful when we port for the new exynos7xxx SoC come out.
> Because SPI of new exynos7xxx SoC has only difference on fifosize.
>

Please, do not top post.

You skipped some of arguments from my reply instead saying "patch is
useful". That is not a sufficient argument for my doubts... but let me
state again the most important question:

What happens if this *optional* property is not present on board with
Exynos7? What value should be used?

Best regards,
Krzysztof

> Thanks.
>
>
> On 2016년 02월 14일 17:31, Krzysztof Kozlowski wrote:
>> W dniu 14.02.2016 o 17:13, Youngmin Nam pisze:
>>> Hello Krzysztof,
>>>
>>> As you mentioned, spi fifosize is not configurable in the given SoC.
>>> The point is we can set fifosize without changing driver code.
>>> For example, if some SoC in exynos7 series has different spi fifosize of on each channel with current
>>> our compatible, we can't cover this situation without adding new compatible into spi driver code.
>>
>> Yes, because new SoC is not compatible with old ones... so a new
>> compatible is required!
>>
>>> Whenever new SoC kind of exynos7 come out, we should add new compatible into driver code only for fifosize change.
>>> I think this is not efficient. I think we can reduces this works through DT handling.
>>
>> I agree that this is not the most efficient possible way of setting some
>> specific properties of devices but this is the way how DT works.
>>
>> What if the property is not present on board with Exynos7? What value
>> should be used?
>>
>> You did not want to add a new compatible but you are adding a
>> compatible-like property which apparently is required for some devices.
>>
>> That looks bad. It's easy to make a mistake, messes with compatibles.
>>
>> Best regards,
>> Krzysztof
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html