On 08/05/2023 13:58, Joy Chakraborty wrote: > On Fri, May 5, 2023 at 5:53 PM Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> On 05/05/2023 11:44, Joy Chakraborty wrote: >>> On Thu, May 4, 2023 at 8:38 PM Krzysztof Kozlowski >>> <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxx> >>>>> --- >>>>> 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
