[PATCH] spi: dt-bindings: samsung: make dma properties not required

[PATCH] spi: dt-bindings: samsung: make dma properties not required

[Tudor Ambarus]

Since the addition of the driver in 2009, the driver selects between DMA
and polling mode depending on the transfer length - DMA mode for
transfers bigger than the FIFO depth, polling mode otherwise. All
versions of the IP support polling mode, make the dma properties not
required.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 Documentation/devicetree/bindings/spi/samsung,spi.yaml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
index 2f0a0835ecfb..f681372da81f 100644
--- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
+++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
@@ -76,8 +76,6 @@ required:
   - compatible
   - clocks
   - clock-names
-  - dmas
-  - dma-names
   - interrupts
   - reg
 
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Sam Protsenko]

On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Since the addition of the driver in 2009, the driver selects between DMA
> and polling mode depending on the transfer length - DMA mode for
> transfers bigger than the FIFO depth, polling mode otherwise. All
> versions of the IP support polling mode, make the dma properties not
> required.
>

AFAIU, the device tree has nothing to do with drivers, it's about
hardware description. Does making DMA properties not required here
mean that there are some HW out there which doesn't integrate DMA in
SPI blocks? Even if this change is ok (I'm not sure), the
argumentation doesn't look sound to me.

> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/samsung,spi.yaml | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/spi/samsung,spi.yaml b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> index 2f0a0835ecfb..f681372da81f 100644
> --- a/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> +++ b/Documentation/devicetree/bindings/spi/samsung,spi.yaml
> @@ -76,8 +76,6 @@ required:
>    - compatible
>    - clocks
>    - clock-names
> -  - dmas
> -  - dma-names
>    - interrupts
>    - reg
>
> --
> 2.44.0.278.ge034bb2e1d-goog
>

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Mark Brown]

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

On Fri, Mar 01, 2024 at 01:28:35PM -0600, Sam Protsenko wrote:
> On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> > Since the addition of the driver in 2009, the driver selects between DMA
> > and polling mode depending on the transfer length - DMA mode for
> > transfers bigger than the FIFO depth, polling mode otherwise. All
> > versions of the IP support polling mode, make the dma properties not
> > required.

> AFAIU, the device tree has nothing to do with drivers, it's about
> hardware description. Does making DMA properties not required here
> mean that there are some HW out there which doesn't integrate DMA in
> SPI blocks? Even if this change is ok (I'm not sure), the
> argumentation doesn't look sound to me.

I do remember there being some SoC which shipped a SPI controller in
that configuration for some reason.  Possibly one of the OEM ones rather
than one in a Samsung SoC?

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

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Tudor Ambarus]

On 01.03.2024 22:42, Mark Brown wrote:
> On Fri, Mar 01, 2024 at 01:28:35PM -0600, Sam Protsenko wrote:
>> On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> 
>>> Since the addition of the driver in 2009, the driver selects between DMA
>>> and polling mode depending on the transfer length - DMA mode for
>>> transfers bigger than the FIFO depth, polling mode otherwise. All
>>> versions of the IP support polling mode, make the dma properties not
>>> required.
> 
>> AFAIU, the device tree has nothing to do with drivers, it's about
>> hardware description. Does making DMA properties not required here

correct

>> mean that there are some HW out there which doesn't integrate DMA in

no, to me it means that the IP can work without DMA, only in PIO mode,
regardless if DMA is integrated or not. Not required means that the
property is not mandatory, which is what I'm trying to achieve here.

>> SPI blocks? Even if this change is ok (I'm not sure), the
>> argumentation doesn't look sound to me.

switching to PIO mode in the driver for sizes smaller than FIFO depths
in the driver guarantees that all existing compatibles support PIO mode.

Are you saying that if there is a physical line between an IP and DMA
controller, then the DMA properties must always be specified in dt? I
thought they can be marked as optional in this case, and that's what I
did with this patch.

> 
> I do remember there being some SoC which shipped a SPI controller in
> that configuration for some reason.  Possibly one of the OEM ones rather
> than one in a Samsung SoC?

with DMA you mean?

Thanks,
ta

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Sam Protsenko]

On Sat, Mar 2, 2024 at 3:36 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 01.03.2024 22:42, Mark Brown wrote:
> > On Fri, Mar 01, 2024 at 01:28:35PM -0600, Sam Protsenko wrote:
> >> On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >
> >>> Since the addition of the driver in 2009, the driver selects between DMA
> >>> and polling mode depending on the transfer length - DMA mode for
> >>> transfers bigger than the FIFO depth, polling mode otherwise. All
> >>> versions of the IP support polling mode, make the dma properties not
> >>> required.
> >
> >> AFAIU, the device tree has nothing to do with drivers, it's about
> >> hardware description. Does making DMA properties not required here
>
> correct
>
> >> mean that there are some HW out there which doesn't integrate DMA in
>
> no, to me it means that the IP can work without DMA, only in PIO mode,
> regardless if DMA is integrated or not. Not required means that the
> property is not mandatory, which is what I'm trying to achieve here.
>
> >> SPI blocks? Even if this change is ok (I'm not sure), the
> >> argumentation doesn't look sound to me.
>
> switching to PIO mode in the driver for sizes smaller than FIFO depths
> in the driver guarantees that all existing compatibles support PIO mode.
>
> Are you saying that if there is a physical line between an IP and DMA
> controller, then the DMA properties must always be specified in dt? I
> thought they can be marked as optional in this case, and that's what I
> did with this patch.
>

No, I would wait for maintainers to clarify on that bit. Change itself
can be ok. But the commit message shouldn't mention the driver,
because the driver uses (depends on) device tree, not vice versa. The
device tree can be used in other projects as well (like U-Boot and
OP-TEE), so it should be designed to be universal and not depend on
kernel drivers. The commit message should be based on particular HW
layout features and how the patch makes the bindings describe that HW
better. It shouldn't rely on driver implementations.

Also, it may be beneficial for reviewers/maintainers if you mention
briefly (either in the commit message, patch #0, or under the "---"
stanza) what exactly problem are you trying to solve in your case with
this patch.

> >
> > I do remember there being some SoC which shipped a SPI controller in
> > that configuration for some reason.  Possibly one of the OEM ones rather
> > than one in a Samsung SoC?
>
> with DMA you mean?
>
> Thanks,
> ta

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Rob Herring]

On Sat, Mar 02, 2024 at 10:23:16AM -0600, Sam Protsenko wrote:
> On Sat, Mar 2, 2024 at 3:36 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >
> >
> >
> > On 01.03.2024 22:42, Mark Brown wrote:
> > > On Fri, Mar 01, 2024 at 01:28:35PM -0600, Sam Protsenko wrote:
> > >> On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> > >
> > >>> Since the addition of the driver in 2009, the driver selects between DMA
> > >>> and polling mode depending on the transfer length - DMA mode for
> > >>> transfers bigger than the FIFO depth, polling mode otherwise. All
> > >>> versions of the IP support polling mode, make the dma properties not
> > >>> required.
> > >
> > >> AFAIU, the device tree has nothing to do with drivers, it's about
> > >> hardware description. Does making DMA properties not required here
> >
> > correct
> >
> > >> mean that there are some HW out there which doesn't integrate DMA in
> >
> > no, to me it means that the IP can work without DMA, only in PIO mode,
> > regardless if DMA is integrated or not. Not required means that the
> > property is not mandatory, which is what I'm trying to achieve here.
> >
> > >> SPI blocks? Even if this change is ok (I'm not sure), the
> > >> argumentation doesn't look sound to me.
> >
> > switching to PIO mode in the driver for sizes smaller than FIFO depths
> > in the driver guarantees that all existing compatibles support PIO mode.
> >
> > Are you saying that if there is a physical line between an IP and DMA
> > controller, then the DMA properties must always be specified in dt? I
> > thought they can be marked as optional in this case, and that's what I
> > did with this patch.
> >
> 
> No, I would wait for maintainers to clarify on that bit. Change itself
> can be ok. But the commit message shouldn't mention the driver,
> because the driver uses (depends on) device tree, not vice versa. The
> device tree can be used in other projects as well (like U-Boot and
> OP-TEE), so it should be designed to be universal and not depend on
> kernel drivers. The commit message should be based on particular HW
> layout features and how the patch makes the bindings describe that HW
> better. It shouldn't rely on driver implementations.

If the controller is DMA capable then it should have dma properties. The 
compatible should be enough to tell if it is a case of 'can only work 
with DMA'. Otherwise, it is going to be up to a specific user. Even 
within Linux, you may have a serial port that doesn't use DMA for the 
console, but uses it for the tty or serdev.

Of course, if a new device is added without DMA properties and they 
are added later on, then they are going to be optional even though the 
DMA support is always there. I can't fully understand everyone's h/w. 

Rob

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Tudor Ambarus]

Hi, Rob,

On 3/4/24 16:56, Rob Herring wrote:
> On Sat, Mar 02, 2024 at 10:23:16AM -0600, Sam Protsenko wrote:
>> On Sat, Mar 2, 2024 at 3:36 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>>
>>>
>>> On 01.03.2024 22:42, Mark Brown wrote:
>>>> On Fri, Mar 01, 2024 at 01:28:35PM -0600, Sam Protsenko wrote:
>>>>> On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>
>>>>>> Since the addition of the driver in 2009, the driver selects between DMA
>>>>>> and polling mode depending on the transfer length - DMA mode for
>>>>>> transfers bigger than the FIFO depth, polling mode otherwise. All
>>>>>> versions of the IP support polling mode, make the dma properties not
>>>>>> required.
>>>>
>>>>> AFAIU, the device tree has nothing to do with drivers, it's about
>>>>> hardware description. Does making DMA properties not required here
>>>
>>> correct
>>>
>>>>> mean that there are some HW out there which doesn't integrate DMA in
>>>
>>> no, to me it means that the IP can work without DMA, only in PIO mode,
>>> regardless if DMA is integrated or not. Not required means that the
>>> property is not mandatory, which is what I'm trying to achieve here.
>>>
>>>>> SPI blocks? Even if this change is ok (I'm not sure), the
>>>>> argumentation doesn't look sound to me.
>>>
>>> switching to PIO mode in the driver for sizes smaller than FIFO depths
>>> in the driver guarantees that all existing compatibles support PIO mode.
>>>
>>> Are you saying that if there is a physical line between an IP and DMA
>>> controller, then the DMA properties must always be specified in dt? I
>>> thought they can be marked as optional in this case, and that's what I
>>> did with this patch.
>>>
>>
>> No, I would wait for maintainers to clarify on that bit. Change itself
>> can be ok. But the commit message shouldn't mention the driver,
>> because the driver uses (depends on) device tree, not vice versa. The
>> device tree can be used in other projects as well (like U-Boot and
>> OP-TEE), so it should be designed to be universal and not depend on
>> kernel drivers. The commit message should be based on particular HW
>> layout features and how the patch makes the bindings describe that HW
>> better. It shouldn't rely on driver implementations.
> 
> If the controller is DMA capable then it should have dma properties. The 

should have as in required/mandatory?

> compatible should be enough to tell if it is a case of 'can only work 

yes, I agree

> with DMA'. Otherwise, it is going to be up to a specific user. Even 
> within Linux, you may have a serial port that doesn't use DMA for the 
> console, but uses it for the tty or serdev.
> 
> Of course, if a new device is added without DMA properties and they 
> are added later on, then they are going to be optional even though the 
> DMA support is always there. I can't fully understand everyone's h/w. 
> 

The SPI controller that I'm working with has a dedicated channel to the
DMA controller. It can work without DMA too, just by polling registers
or by interrupts.

I can't get the DMA controller to work correctly yet, and since the SPI
controller can work without DMA, I thought that I can mark the DMA
properties as optional, add the SPI node in dt without DMA, and add the
DMA properties later on, after I have the DMA controller working
correctly. Is this approach wrong?

Thanks,
ta

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Jaewon Kim]

Hello all,


On 24. 3. 5. 03:15, Tudor Ambarus wrote:
> Hi, Rob,
>
> On 3/4/24 16:56, Rob Herring wrote:
>> On Sat, Mar 02, 2024 at 10:23:16AM -0600, Sam Protsenko wrote:
>>> On Sat, Mar 2, 2024 at 3:36 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>
>>>>
>>>> On 01.03.2024 22:42, Mark Brown wrote:
>>>>> On Fri, Mar 01, 2024 at 01:28:35PM -0600, Sam Protsenko wrote:
>>>>>> On Fri, Mar 1, 2024 at 5:55 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>>>> Since the addition of the driver in 2009, the driver selects between DMA
>>>>>>> and polling mode depending on the transfer length - DMA mode for
>>>>>>> transfers bigger than the FIFO depth, polling mode otherwise. All
>>>>>>> versions of the IP support polling mode, make the dma properties not
>>>>>>> required.
>>>>>> AFAIU, the device tree has nothing to do with drivers, it's about
>>>>>> hardware description. Does making DMA properties not required here
>>>> correct
>>>>
>>>>>> mean that there are some HW out there which doesn't integrate DMA in
>>>> no, to me it means that the IP can work without DMA, only in PIO mode,
>>>> regardless if DMA is integrated or not. Not required means that the
>>>> property is not mandatory, which is what I'm trying to achieve here.
>>>>
>>>>>> SPI blocks? Even if this change is ok (I'm not sure), the
>>>>>> argumentation doesn't look sound to me.
>>>> switching to PIO mode in the driver for sizes smaller than FIFO depths
>>>> in the driver guarantees that all existing compatibles support PIO mode.
>>>>
>>>> Are you saying that if there is a physical line between an IP and DMA
>>>> controller, then the DMA properties must always be specified in dt? I
>>>> thought they can be marked as optional in this case, and that's what I
>>>> did with this patch.
>>>>
>>> No, I would wait for maintainers to clarify on that bit. Change itself
>>> can be ok. But the commit message shouldn't mention the driver,
>>> because the driver uses (depends on) device tree, not vice versa. The
>>> device tree can be used in other projects as well (like U-Boot and
>>> OP-TEE), so it should be designed to be universal and not depend on
>>> kernel drivers. The commit message should be based on particular HW
>>> layout features and how the patch makes the bindings describe that HW
>>> better. It shouldn't rely on driver implementations.
>> If the controller is DMA capable then it should have dma properties. The
> should have as in required/mandatory?
>
>> compatible should be enough to tell if it is a case of 'can only work
> yes, I agree
>
>> with DMA'. Otherwise, it is going to be up to a specific user. Even
>> within Linux, you may have a serial port that doesn't use DMA for the
>> console, but uses it for the tty or serdev.
>>
>> Of course, if a new device is added without DMA properties and they
>> are added later on, then they are going to be optional even though the
>> DMA support is always there. I can't fully understand everyone's h/w.
>>
> The SPI controller that I'm working with has a dedicated channel to the
> DMA controller. It can work without DMA too, just by polling registers
> or by interrupts.
>
> I can't get the DMA controller to work correctly yet, and since the SPI
> controller can work without DMA, I thought that I can mark the DMA
> properties as optional, add the SPI node in dt without DMA, and add the
> DMA properties later on, after I have the DMA controller working
> correctly. Is this approach wrong?
>
> Thanks,
> ta
>
>

I agree with this patch.

I don`t think DMA property needs to be "required" because it can operate 
well without DMA property.


Last year, I put a patch that makes dma property optional.

  - d1a7718ee8db (spi: s3c64xx: change polling mode to optional)

- https://lore.kernel.org/r/20230502062813.112434-2-jaewon02.kim@samsung.com


In the past, there was SoC without DMA, so it was a quirk that used 
polling mode.

Now, I want to change this to be optional to support cases where DMA is 
not available according to OS environment(Virtual Machine).


Thanks

Jaewon Kim

Re: [PATCH] spi: dt-bindings: samsung: make dma properties not required

[Mark Brown]

On Fri, 01 Mar 2024 11:55:46 +0000, Tudor Ambarus wrote:
> Since the addition of the driver in 2009, the driver selects between DMA
> and polling mode depending on the transfer length - DMA mode for
> transfers bigger than the FIFO depth, polling mode otherwise. All
> versions of the IP support polling mode, make the dma properties not
> required.
> 
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: dt-bindings: samsung: make dma properties not required
      commit: ee09bb727bff1f14f3f2d81592741b8a081af2ee

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark