[7/7] dt-bindings: dmaengine: pl330: Add new quirks

  Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
and "arm,pl330-periph-single-dregs"

Signed-off-by: Joy Chakraborty <joychakr@google.com>
---
 Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)
  

On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/05/2023 16:57, Joy Chakraborty wrote:
> > Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> > and "arm,pl330-periph-single-dregs"
>
> This we can see from the diff. You need to answer why?
>

Sure will change it to:
"
Addition of following quirks :
- "arm,pl330-periph-use-diff-axsize"
   AxSize of transactions to peripherals are limited by the peripheral
address width which inturn limits the AxSize used for transactions
towards memory.
   This quirk will make transactions to memory use the maximum
possible bus width(AxSize), store data in MFIFO and use narrow
multi-beat transactions to move data to peripherals.
   This only applies to transfers between memory and peripherals where
bus widths available are different for memory and the peripheral.
- "arm,pl330-periph-complete-with-singles" :
   When transfer sizes are not a multiple of a block of burst
transfers (AxLen * AxSize configured at the peripheral), certain
peripherals might choose not to set the burst request at the
peripheral request interface of the DMA.
   This quirk moves the remaining bytes to the peripheral using single
transactions.
"

> >
> > Signed-off-by: Joy Chakraborty <joychakr@google.com>
> > ---
> >  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> > index 4a3dd6f5309b..0499a7fba88d 100644
> > --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> > +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> > @@ -53,6 +53,14 @@ properties:
> >      type: boolean
> >      description: quirk for performing burst transfer only
> >
> > +  arm,pl330-optimize-dev2mem-axsize:
> > +    type: boolean
> > +    description: quirk for optimizing AxSize used between dev<->mem
>
> This tells me nothing... Neither what it is about nor why this is
> property of a board or PL330 hardware implementation. Please describe
> hardware, not drivers.
>

Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
"
Quirk to use different AxSize for bursts while accessing source and
destination when moving data between memory and peripheral.
Maximum possible bus width is used as AxSize for transactions towards
memory and transactions towards peripherals use AxSize as per
peripheral address width.
"

> > +
> > +  arm,pl330-periph-single-dregs:
> > +    type: boolean
> > +    description: quirk for using dma-singles for peripherals in _dregs()
>
> Same concerns.
>

Will change the name to  "arm,pl330-periph-complete-with-singles" and
add description:
"
Quirk to use dma singles n times instead of an n beat burst to
complete a transfer when the transfer size is not a multiple of the
burst size and burst length configured at the peripheral.
n being bytes left after the major chunk is transferred with
peripheral configured burst transactions.
"

>
> Best regards,
> Krzysztof
>

Will update the patch series if this is satisfactory.

Thanks
Joy
  

On 05/05/2023 11:44, Joy Chakraborty wrote:
> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 04/05/2023 16:57, Joy Chakraborty wrote:
>>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
>>> and "arm,pl330-periph-single-dregs"
>>
>> This we can see from the diff. You need to answer why?
>>
> 
> Sure will change it to:
> "
> Addition of following quirks :
> - "arm,pl330-periph-use-diff-axsize"
>    AxSize of transactions to peripherals are limited by the peripheral
> address width which inturn limits the AxSize used for transactions
> towards memory.
>    This quirk will make transactions to memory use the maximum
> possible bus width(AxSize), store data in MFIFO and use narrow
> multi-beat transactions to move data to peripherals.
>    This only applies to transfers between memory and peripherals where
> bus widths available are different for memory and the peripheral.
> - "arm,pl330-periph-complete-with-singles" :
>    When transfer sizes are not a multiple of a block of burst
> transfers (AxLen * AxSize configured at the peripheral), certain
> peripherals might choose not to set the burst request at the
> peripheral request interface of the DMA.
>    This quirk moves the remaining bytes to the peripheral using single
> transactions.
> "

This does not answer why. You just copied again the patch contents.

> 
>>>
>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
>>> ---
>>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>> index 4a3dd6f5309b..0499a7fba88d 100644
>>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>> @@ -53,6 +53,14 @@ properties:
>>>      type: boolean
>>>      description: quirk for performing burst transfer only
>>>
>>> +  arm,pl330-optimize-dev2mem-axsize:
>>> +    type: boolean
>>> +    description: quirk for optimizing AxSize used between dev<->mem
>>
>> This tells me nothing... Neither what it is about nor why this is
>> property of a board or PL330 hardware implementation. Please describe
>> hardware, not drivers.
>>
> 
> Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
> "
> Quirk to use different AxSize for bursts while accessing source and
> destination when moving data between memory and peripheral.
> Maximum possible bus width is used as AxSize for transactions towards
> memory and transactions towards peripherals use AxSize as per
> peripheral address width.
> "

Still no answer. Why this is property of a board or PL330 hardware
implementation?

I also asked to describe hardware but I still see "quirk to ...". We use
"quirk" as concept in Linux driver. Describe the hardware, not Linux driver.


> 
>>> +
>>> +  arm,pl330-periph-single-dregs:
>>> +    type: boolean
>>> +    description: quirk for using dma-singles for peripherals in _dregs()
>>
>> Same concerns.
>>
> 
> Will change the name to  "arm,pl330-periph-complete-with-singles" and
> add description:
> "
> Quirk to use dma singles n times instead of an n beat burst to
> complete a transfer when the transfer size is not a multiple of the

No, how you wrote it sounds like driver. Don't add driver quirks to DT.

Best regards,
Krzysztof
  

On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/05/2023 11:44, Joy Chakraborty wrote:
> > On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 04/05/2023 16:57, Joy Chakraborty wrote:
> >>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> >>> and "arm,pl330-periph-single-dregs"
> >>
> >> This we can see from the diff. You need to answer why?
> >>
> >
> > Sure will change it to:
> > "
> > Addition of following quirks :
> > - "arm,pl330-periph-use-diff-axsize"
> >    AxSize of transactions to peripherals are limited by the peripheral
> > address width which inturn limits the AxSize used for transactions
> > towards memory.
> >    This quirk will make transactions to memory use the maximum
> > possible bus width(AxSize), store data in MFIFO and use narrow
> > multi-beat transactions to move data to peripherals.
> >    This only applies to transfers between memory and peripherals where
> > bus widths available are different for memory and the peripheral.
> > - "arm,pl330-periph-complete-with-singles" :
> >    When transfer sizes are not a multiple of a block of burst
> > transfers (AxLen * AxSize configured at the peripheral), certain
> > peripherals might choose not to set the burst request at the
> > peripheral request interface of the DMA.
> >    This quirk moves the remaining bytes to the peripheral using single
> > transactions.
> > "
>
> This does not answer why. You just copied again the patch contents.
>
Hi Krzysztof,
Both the changes could be useful for SOC's which have PL330 integrated
with a peripheral but I am not sure if all SOC's need/want this change
hence wanted to keep it as a DT knob to avoid any regressions.
But like you say it might not be the right thing to do.

> >
> >>>
> >>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>> index 4a3dd6f5309b..0499a7fba88d 100644
> >>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>> @@ -53,6 +53,14 @@ properties:
> >>>      type: boolean
> >>>      description: quirk for performing burst transfer only
> >>>
> >>> +  arm,pl330-optimize-dev2mem-axsize:
> >>> +    type: boolean
> >>> +    description: quirk for optimizing AxSize used between dev<->mem
> >>
> >> This tells me nothing... Neither what it is about nor why this is
> >> property of a board or PL330 hardware implementation. Please describe
> >> hardware, not drivers.
> >>
> >
> > Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
> > "
> > Quirk to use different AxSize for bursts while accessing source and
> > destination when moving data between memory and peripheral.
> > Maximum possible bus width is used as AxSize for transactions towards
> > memory and transactions towards peripherals use AxSize as per
> > peripheral address width.
> > "
>
> Still no answer. Why this is property of a board or PL330 hardware
> implementation?
> I also asked to describe hardware but I still see "quirk to ...". We use
> "quirk" as concept in Linux driver. Describe the hardware, not Linux driver.
>

This comes to use when the bus width requirement between peripheral
and memory is different, but buswidth is something we read from HW
registers so this can be enabled by default.

>
> >
> >>> +
> >>> +  arm,pl330-periph-single-dregs:
> >>> +    type: boolean
> >>> +    description: quirk for using dma-singles for peripherals in _dregs()
> >>
> >> Same concerns.
> >>

An example of such a case is given in the ARM TRM for PL330, so maybe
we can have this by default as well.
Link : https://developer.arm.com/documentation/ddi0424/d/functional-overview/peripheral-request-interface/dmac-length-management#:~:text=DMAC%20length%20management-,Example%202.3,-shows%20a%20DMAC

> >
> > Will change the name to  "arm,pl330-periph-complete-with-singles" and
> > add description:
> > "
> > Quirk to use dma singles n times instead of an n beat burst to
> > complete a transfer when the transfer size is not a multiple of the
>
> No, how you wrote it sounds like driver. Don't add driver quirks to DT.
>
> Best regards,
> Krzysztof
>

Hi Vinod Kaul,

Do you think it is feasible to enable these changes by default instead
of a DT property ?

Thanks
Joy
  

On 08/05/2023 13:58, Joy Chakraborty wrote:
> On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 05/05/2023 11:44, Joy Chakraborty wrote:
>>> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 04/05/2023 16:57, Joy Chakraborty wrote:
>>>>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
>>>>> and "arm,pl330-periph-single-dregs"
>>>>
>>>> This we can see from the diff. You need to answer why?
>>>>
>>>
>>> Sure will change it to:
>>> "
>>> Addition of following quirks :
>>> - "arm,pl330-periph-use-diff-axsize"
>>>    AxSize of transactions to peripherals are limited by the peripheral
>>> address width which inturn limits the AxSize used for transactions
>>> towards memory.
>>>    This quirk will make transactions to memory use the maximum
>>> possible bus width(AxSize), store data in MFIFO and use narrow
>>> multi-beat transactions to move data to peripherals.
>>>    This only applies to transfers between memory and peripherals where
>>> bus widths available are different for memory and the peripheral.
>>> - "arm,pl330-periph-complete-with-singles" :
>>>    When transfer sizes are not a multiple of a block of burst
>>> transfers (AxLen * AxSize configured at the peripheral), certain
>>> peripherals might choose not to set the burst request at the
>>> peripheral request interface of the DMA.
>>>    This quirk moves the remaining bytes to the peripheral using single
>>> transactions.
>>> "
>>
>> This does not answer why. You just copied again the patch contents.
>>
> Hi Krzysztof,
> Both the changes could be useful for SOC's which have PL330 integrated
> with a peripheral 

What do you mean here by "PL330 integrated with a peripheral"?

> but I am not sure if all SOC's need/want this change
> hence wanted to keep it as a DT knob to avoid any regressions.
> But like you say it might not be the right thing to do.

Devicetree is for describing hardware, not the contents of registers of
a device. Your changes might fit or might not, I don't know this good
enough, so I wait for your justification. Without justification this
looks like controlling driver from DT...

> 
>>>
>>>>>
>>>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>>>> index 4a3dd6f5309b..0499a7fba88d 100644
>>>>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>>>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
>>>>> @@ -53,6 +53,14 @@ properties:
>>>>>      type: boolean
>>>>>      description: quirk for performing burst transfer only
>>>>>
>>>>> +  arm,pl330-optimize-dev2mem-axsize:
>>>>> +    type: boolean
>>>>> +    description: quirk for optimizing AxSize used between dev<->mem
>>>>
>>>> This tells me nothing... Neither what it is about nor why this is
>>>> property of a board or PL330 hardware implementation. Please describe
>>>> hardware, not drivers.
>>>>
>>>
>>> Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
>>> "
>>> Quirk to use different AxSize for bursts while accessing source and
>>> destination when moving data between memory and peripheral.
>>> Maximum possible bus width is used as AxSize for transactions towards
>>> memory and transactions towards peripherals use AxSize as per
>>> peripheral address width.
>>> "
>>
>> Still no answer. Why this is property of a board or PL330 hardware
>> implementation?
>> I also asked to describe hardware but I still see "quirk to ...". We use
>> "quirk" as concept in Linux driver. Describe the hardware, not Linux driver.
>>
> 
> This comes to use when the bus width requirement between peripheral
> and memory is different, but buswidth is something we read from HW
> registers so this can be enabled by default.

Don't add discoverable stuff to DT.

> 
>>
>>>
>>>>> +
>>>>> +  arm,pl330-periph-single-dregs:
>>>>> +    type: boolean
>>>>> +    description: quirk for using dma-singles for peripherals in _dregs()
>>>>
>>>> Same concerns.
>>>>
> 
> An example of such a case is given in the ARM TRM for PL330, so maybe
> we can have this by default as well.
> Link : https://developer.arm.com/documentation/ddi0424/d/functional-overview/peripheral-request-interface/dmac-length-management#:~:text=DMAC%20length%20management-,Example%202.3,-shows%20a%20DMAC

I could not find here a case describing hardware. You pointed out some
code. What does the code have anything to do with DT?


Best regards,
Krzysztof
  

On Mon, May 8, 2023 at 10:13 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/05/2023 13:58, Joy Chakraborty wrote:
> > On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 05/05/2023 11:44, Joy Chakraborty wrote:
> >>> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 04/05/2023 16:57, Joy Chakraborty wrote:
> >>>>> Add 2 new quirks added to the driver "arm,pl330-optimize-dev2mem-axsize"
> >>>>> and "arm,pl330-periph-single-dregs"
> >>>>
> >>>> This we can see from the diff. You need to answer why?
> >>>>
> >>>
> >>> Sure will change it to:
> >>> "
> >>> Addition of following quirks :
> >>> - "arm,pl330-periph-use-diff-axsize"
> >>>    AxSize of transactions to peripherals are limited by the peripheral
> >>> address width which inturn limits the AxSize used for transactions
> >>> towards memory.
> >>>    This quirk will make transactions to memory use the maximum
> >>> possible bus width(AxSize), store data in MFIFO and use narrow
> >>> multi-beat transactions to move data to peripherals.
> >>>    This only applies to transfers between memory and peripherals where
> >>> bus widths available are different for memory and the peripheral.
> >>> - "arm,pl330-periph-complete-with-singles" :
> >>>    When transfer sizes are not a multiple of a block of burst
> >>> transfers (AxLen * AxSize configured at the peripheral), certain
> >>> peripherals might choose not to set the burst request at the
> >>> peripheral request interface of the DMA.
> >>>    This quirk moves the remaining bytes to the peripheral using single
> >>> transactions.
> >>> "
> >>
> >> This does not answer why. You just copied again the patch contents.
> >>
> > Hi Krzysztof,
> > Both the changes could be useful for SOC's which have PL330 integrated
> > with a peripheral
>
> What do you mean here by "PL330 integrated with a peripheral"?

Hi Krzysztof,

By integration with peripheral I mean when the PL330 DMA is used to
copy data to/from memory to a peripheral hardware (e.g. FIFO of a SPI
master) where flow control of data is managed by the peripheral
request interface exposed by PL330 :
https://developer.arm.com/documentation/ddi0424/a/functional-overview/peripheral-request-interface

>
> > but I am not sure if all SOC's need/want this change
> > hence wanted to keep it as a DT knob to avoid any regressions.
> > But like you say it might not be the right thing to do.
>
> Devicetree is for describing hardware, not the contents of registers of
> a device. Your changes might fit or might not, I don't know this good
> enough, so I wait for your justification. Without justification this
> looks like controlling driver from DT...
>

Yes this does control the driver behaviour on how the PL330 DMA
hardware is programmed but it also is a function of
   - The bus width available in the soc towards memory and peripheral
to be different.
   - The requirement of peripherals interfaced with PL330 on how odd
transfer sizes are to be copied from memory to peripheral.

But, both changes IMO can be enabled by default as well in the driver
without devicetree knobs but it carries the risk of regression on
SOC's which do not have such a requirement.

Hence I was looking for some insight from Vinod Koul to see if it is
okay to go ahead with the changes without device tree knobs.

> >
> >>>
> >>>>>
> >>>>> Signed-off-by: Joy Chakraborty <joychakr@google.com>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/dma/arm,pl330.yaml | 8 ++++++++
> >>>>>  1 file changed, 8 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/dma/arm,pl330.yaml b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>>>> index 4a3dd6f5309b..0499a7fba88d 100644
> >>>>> --- a/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/dma/arm,pl330.yaml
> >>>>> @@ -53,6 +53,14 @@ properties:
> >>>>>      type: boolean
> >>>>>      description: quirk for performing burst transfer only
> >>>>>
> >>>>> +  arm,pl330-optimize-dev2mem-axsize:
> >>>>> +    type: boolean
> >>>>> +    description: quirk for optimizing AxSize used between dev<->mem
> >>>>
> >>>> This tells me nothing... Neither what it is about nor why this is
> >>>> property of a board or PL330 hardware implementation. Please describe
> >>>> hardware, not drivers.
> >>>>
> >>>
> >>> Will change the name to "arm,pl330-periph-use-diff-axsize" and add description:
> >>> "
> >>> Quirk to use different AxSize for bursts while accessing source and
> >>> destination when moving data between memory and peripheral.
> >>> Maximum possible bus width is used as AxSize for transactions towards
> >>> memory and transactions towards peripherals use AxSize as per
> >>> peripheral address width.
> >>> "
> >>
> >> Still no answer. Why this is property of a board or PL330 hardware
> >> implementation?
> >> I also asked to describe hardware but I still see "quirk to ...". We use
> >> "quirk" as concept in Linux driver. Describe the hardware, not Linux driver.
> >>
> >
> > This comes to use when the bus width requirement between peripheral
> > and memory is different, but buswidth is something we read from HW
> > registers so this can be enabled by default.
>
> Don't add discoverable stuff to DT.

Sure, will not add this to DT.

>
> >
> >>
> >>>
> >>>>> +
> >>>>> +  arm,pl330-periph-single-dregs:
> >>>>> +    type: boolean
> >>>>> +    description: quirk for using dma-singles for peripherals in _dregs()
> >>>>
> >>>> Same concerns.
> >>>>
> >
> > An example of such a case is given in the ARM TRM for PL330, so maybe
> > we can have this by default as well.
> > Link : https://developer.arm.com/documentation/ddi0424/d/functional-overview/peripheral-request-interface/dmac-length-management#:~:text=DMAC%20length%20management-,Example%202.3,-shows%20a%20DMAC
>
> I could not find here a case describing hardware. You pointed out some
> code. What does the code have anything to do with DT?
>

The instructions mentioned here are consumed by the PL330 Hardware to
generate AXI transactions on the system bus. The example mentioned in
the link is similar to how the driver would behave when this is
enabled.

I shall remove this as well and create a new patch without any DT
depency for the changes.

Thanks
Joy
>
> Best regards,
> Krzysztof
>