Re: [PATCH] dmaengine: qcom_bam_dma: add one more optional clock

[Stanimir Varbanov <svarbanov@mm-sol.com>]

Hi Mark,

Thank you for the comments!

On 09/08/2014 01:36 PM, Mark Rutland wrote:
> On Sun, Sep 07, 2014 at 06:55:47PM +0100, Stanimir Varbanov wrote:
>> The BAM is tightly coupled with the peripheral to which it
>> belongs. In that sprit to access the BAM configuration
>> registers the driver needs to enable some peripheral
>> clocks. Currently the DT node enables bamclk which seems
>> is not enough for some peripherals (for example the crypto
>> engine wants core and iface clocks). This change attempts
>> to solve this issue by adding one more optional clock
>> in bam_dma driver.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>> ---
>>  .../devicetree/bindings/dma/qcom_bam_dma.txt       | 12 ++++--
>>  drivers/dma/qcom_bam_dma.c                         | 44 +++++++++++++++-------
>>  2 files changed, 38 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
>> index d75a9d7..2376897 100644
>> --- a/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
>> +++ b/Documentation/devicetree/bindings/dma/qcom_bam_dma.txt
>> @@ -6,8 +6,11 @@ Required properties:
>>  - interrupts: Should contain the one interrupt shared by all channels
>>  - #dma-cells: must be <1>, the cell in the dmas property of the client device
>>    represents the channel number
>> -- clocks: required clock
>> -- clock-names: must contain "bam_clk" entry
>> +- clocks: list of required clock plus one optional clock. The optional clock
>> +          is needed for some peripherals and can be omitted.
>> +- clock-names: must contain "core" clock name representing the required clock
>> +               plus the optional "iface" clock name depending on
>> +               peripheral needs.
> 
> Please don't change the names of input lines (at least without retaining
> support for the old name). Does this change not break existing DTBs?

It shouldn't break any DTBs. I changed the "bam_clk" because it sounds
like the BAM needs only one "bam_clk". But the practice shows that in
the crypto engine case I need to pass "core" and "iface" clocks to get
bam dma driver initialised.

I hope Andy can share his opinion on that, or even suggest something better.

> 
> Which of these new names did "bam_clk" previously correspond to?

The required one i.e. "core".

> 
> How many clock inputs does the BAM actually have? Are there more which
> we don't yet describe?
> 
> You mention that the iface clock is actually fed into the peripheral the
> BAM is attached to. Can you explain why this clock is necessary to use
> the BAM?

No, I can't explain why, might be hardware reasons.

> 
> If you want do describe multiple clocks and names are required, define
> clocks in terms of clock-names so you don't need to state everything
> twice, Put each clock on a new line, e.g.
> 
> - clocks: A list of phandle + clock-specifier pairs, one for each entry
>   in clock-names
> - clock-names: should contain:
>    * "foo_clk" for the FOO clock
>    * "bar_clk" for the BAR input on baz_xxxx systems

OK. Thanks.

-- 
regards,
Stan