Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1387114 > unrolled thread

Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks Vinod Koul <vinod.koul@intel.com> - 2016-04-26 05:30 +0200
    Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks okaya@codeaurora.org - 2016-04-26 14:10 +0200
      Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks Vinod Koul <vinod.koul@intel.com> - 2016-04-26 18:20 +0200
        Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks Sinan Kaya <okaya@codeaurora.org> - 2016-04-26 19:00 +0200
          Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks Vinod Koul <vinod.koul@intel.com> - 2016-04-27 10:20 +0200
            Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks Marc Zyngier <marc.zyngier@arm.com> - 2016-04-27 10:50 +0200
              Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks okaya@codeaurora.org - 2016-04-27 15:30 +0200
            Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks okaya@codeaurora.org - 2016-04-27 15:00 +0200

#1387114 — Re: [PATCH V17 2/3] dmaengine: qcom_hidma: add debugfs hooks

On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:

> +static int hidma_chan_stats(struct seq_file *s, void *unused)
> +{
> +	struct hidma_chan *mchan = s->private;
> +	struct hidma_desc *mdesc;
> +	struct hidma_dev *dmadev = mchan->dmadev;
> +
> +	pm_runtime_get_sync(dmadev->ddev.dev);

debug shouldn't power up device, why do you want to do that

-- 
~Vinod

[toc] | [next] | [standalone]

#1387411

On 2016-04-25 23:30, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
> 
>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>> +{
>> +	struct hidma_chan *mchan = s->private;
>> +	struct hidma_desc *mdesc;
>> +	struct hidma_dev *dmadev = mchan->dmadev;
>> +
>> +	pm_runtime_get_sync(dmadev->ddev.dev);
> 
> debug shouldn't power up device, why do you want to do that


Clocks are turned off while the hw is idle. I can’t reach hw registers 
without restoring power.

[toc] | [prev] | [next] | [standalone]

#1387669

On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
> On 2016-04-25 23:30, Vinod Koul wrote:
> >On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
> >
> >>+static int hidma_chan_stats(struct seq_file *s, void *unused)
> >>+{
> >>+	struct hidma_chan *mchan = s->private;
> >>+	struct hidma_desc *mdesc;
> >>+	struct hidma_dev *dmadev = mchan->dmadev;
> >>+
> >>+	pm_runtime_get_sync(dmadev->ddev.dev);
> >
> >debug shouldn't power up device, why do you want to do that
> 
> 
> Clocks are turned off while the hw is idle. I can’t reach hw
> registers without restoring power.

Hmm, have you thought about using regmap?

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1387721

On 4/26/2016 12:25 PM, Vinod Koul wrote:
> On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>> On 2016-04-25 23:30, Vinod Koul wrote:
>>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>>
>>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>>> +{
>>>> +	struct hidma_chan *mchan = s->private;
>>>> +	struct hidma_desc *mdesc;
>>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>>>> +
>>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>>>
>>> debug shouldn't power up device, why do you want to do that
>>
>>
>> Clocks are turned off while the hw is idle. I can’t reach hw
>> registers without restoring power.
> 
> Hmm, have you thought about using regmap?
> 

To be honest, I didn't know what regmap is but I just read some code
and looked at how it is used. Feel free to correct me if I got it 
wrong. 

Regmap seems to be designed for *slow* speed peripherals to improve frequent
accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.

It seems to cache the register contents and flush/invalidate them only when
needed.

The MMIO version seems to be assuming the presence of device-tree like CLK
API which doesn't exist on ACPI systems and is not portable.

My reaction is that it is a lot of code with no added functionality to what
HIDMA driver is trying to achieve. 

Given that the use case here is only for debug purposes; I think it is OK 
to keep this runtime call here. I don't want to add any overhead into the
existing code just to support the debug use case.  

None of my register read/writes are slow. This file will only be used to 
troubleshoot customer issues.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1388775

On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
> On 4/26/2016 12:25 PM, Vinod Koul wrote:
> > On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
> >> On 2016-04-25 23:30, Vinod Koul wrote:
> >>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
> >>>
> >>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
> >>>> +{
> >>>> +	struct hidma_chan *mchan = s->private;
> >>>> +	struct hidma_desc *mdesc;
> >>>> +	struct hidma_dev *dmadev = mchan->dmadev;
> >>>> +
> >>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
> >>>
> >>> debug shouldn't power up device, why do you want to do that
> >>
> >>
> >> Clocks are turned off while the hw is idle. I can’t reach hw
> >> registers without restoring power.
> > 
> > Hmm, have you thought about using regmap?
> > 
> 
> To be honest, I didn't know what regmap is but I just read some code
> and looked at how it is used. Feel free to correct me if I got it 
> wrong. 
> 
> Regmap seems to be designed for *slow* speed peripherals to improve frequent
> accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.
> 
> It seems to cache the register contents and flush/invalidate them only when
> needed.
> 
> The MMIO version seems to be assuming the presence of device-tree like CLK
> API which doesn't exist on ACPI systems and is not portable.
> 
> My reaction is that it is a lot of code with no added functionality to what
> HIDMA driver is trying to achieve. 
> 
> Given that the use case here is only for debug purposes; I think it is OK 
> to keep this runtime call here. I don't want to add any overhead into the
> existing code just to support the debug use case.  
> 
> None of my register read/writes are slow. This file will only be used to 
> troubleshoot customer issues.

$ is always faster than MMIO. This way you can give reg contents to users
without waking up hw.

Also we at Intel use regmap on ACPI systems without CLK API

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1388793

On 27/04/16 09:15, Vinod Koul wrote:
> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>>> On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>>>> On 2016-04-25 23:30, Vinod Koul wrote:
>>>>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>>>>
>>>>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>>>>> +{
>>>>>> +	struct hidma_chan *mchan = s->private;
>>>>>> +	struct hidma_desc *mdesc;
>>>>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>>>>>> +
>>>>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>>>>>
>>>>> debug shouldn't power up device, why do you want to do that
>>>>
>>>>
>>>> Clocks are turned off while the hw is idle. I can’t reach hw
>>>> registers without restoring power.
>>>
>>> Hmm, have you thought about using regmap?
>>>
>>
>> To be honest, I didn't know what regmap is but I just read some code
>> and looked at how it is used. Feel free to correct me if I got it 
>> wrong. 
>>
>> Regmap seems to be designed for *slow* speed peripherals to improve frequent
>> accesses by the SW. It looks like it is used by MFD, SPI and I2C drivers.
>>
>> It seems to cache the register contents and flush/invalidate them only when
>> needed.
>>
>> The MMIO version seems to be assuming the presence of device-tree like CLK
>> API which doesn't exist on ACPI systems and is not portable.
>>
>> My reaction is that it is a lot of code with no added functionality to what
>> HIDMA driver is trying to achieve. 
>>
>> Given that the use case here is only for debug purposes; I think it is OK 
>> to keep this runtime call here. I don't want to add any overhead into the
>> existing code just to support the debug use case.  
>>
>> None of my register read/writes are slow. This file will only be used to 
>> troubleshoot customer issues.

I'd recommend you actually run perf on a a few of your MMIO accesses. I
believe the result will be eye opening. On the KVM side, we've trimmed
our MMIO access as much as possible, using a memory-based cache (similar
to regmap in concept). This has made some code paths about 40% faster.

> $ is always faster than MMIO. This way you can give reg contents to users
> without waking up hw.

Indeed. MMIO access sucks rocks, even on a very fast box. Actually, the
faster the box is, the slower MMIO feels (compared to memory).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1389038

On 2016-04-27 04:47, Marc Zyngier wrote:
> On 27/04/16 09:15, Vinod Koul wrote:
>> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>>>> On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org 
>>>> wrote:
>>>>> On 2016-04-25 23:30, Vinod Koul wrote:
>>>>>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>>>>>> 
>>>>>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>>>>>>> +{
>>>>>>> +	struct hidma_chan *mchan = s->private;
>>>>>>> +	struct hidma_desc *mdesc;
>>>>>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>>>>>>> +
>>>>>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>>>>>> 
>>>>>> debug shouldn't power up device, why do you want to do that
>>>>> 
>>>>> 
>>>>> Clocks are turned off while the hw is idle. I can’t reach hw
>>>>> registers without restoring power.
>>>> 
>>>> Hmm, have you thought about using regmap?
>>>> 
>>> 
>>> To be honest, I didn't know what regmap is but I just read some code
>>> and looked at how it is used. Feel free to correct me if I got it
>>> wrong.
>>> 
>>> Regmap seems to be designed for *slow* speed peripherals to improve 
>>> frequent
>>> accesses by the SW. It looks like it is used by MFD, SPI and I2C 
>>> drivers.
>>> 
>>> It seems to cache the register contents and flush/invalidate them 
>>> only when
>>> needed.
>>> 
>>> The MMIO version seems to be assuming the presence of device-tree 
>>> like CLK
>>> API which doesn't exist on ACPI systems and is not portable.
>>> 
>>> My reaction is that it is a lot of code with no added functionality 
>>> to what
>>> HIDMA driver is trying to achieve.
>>> 
>>> Given that the use case here is only for debug purposes; I think it 
>>> is OK
>>> to keep this runtime call here. I don't want to add any overhead into 
>>> the
>>> existing code just to support the debug use case.
>>> 
>>> None of my register read/writes are slow. This file will only be used 
>>> to
>>> troubleshoot customer issues.
> 
> I'd recommend you actually run perf on a a few of your MMIO accesses. I
> believe the result will be eye opening. On the KVM side, we've trimmed
> our MMIO access as much as possible, using a memory-based cache 
> (similar
> to regmap in concept). This has made some code paths about 40% faster.
> 
>> $ is always faster than MMIO. This way you can give reg contents to 
>> users
>> without waking up hw.
> 
> Indeed. MMIO access sucks rocks, even on a very fast box. Actually, the
> faster the box is, the slower MMIO feels (compared to memory).
> 
> Thanks,
> 
> 	M.


Agreed. However, I need to understand how regmap really works under the 
covers and whether it is compatible with the hardware.

[toc] | [prev] | [next] | [standalone]

#1389015

On 2016-04-27 04:15, Vinod Koul wrote:
> On Tue, Apr 26, 2016 at 12:55:18PM -0400, Sinan Kaya wrote:
>> On 4/26/2016 12:25 PM, Vinod Koul wrote:
>> > On Tue, Apr 26, 2016 at 08:08:16AM -0400, okaya@codeaurora.org wrote:
>> >> On 2016-04-25 23:30, Vinod Koul wrote:
>> >>> On Mon, Apr 11, 2016 at 10:21:12AM -0400, Sinan Kaya wrote:
>> >>>
>> >>>> +static int hidma_chan_stats(struct seq_file *s, void *unused)
>> >>>> +{
>> >>>> +	struct hidma_chan *mchan = s->private;
>> >>>> +	struct hidma_desc *mdesc;
>> >>>> +	struct hidma_dev *dmadev = mchan->dmadev;
>> >>>> +
>> >>>> +	pm_runtime_get_sync(dmadev->ddev.dev);
>> >>>
>> >>> debug shouldn't power up device, why do you want to do that
>> >>
>> >>
>> >> Clocks are turned off while the hw is idle. I can’t reach hw
>> >> registers without restoring power.
>> >
>> > Hmm, have you thought about using regmap?
>> >
>> 
>> To be honest, I didn't know what regmap is but I just read some code
>> and looked at how it is used. Feel free to correct me if I got it
>> wrong.
>> 
>> Regmap seems to be designed for *slow* speed peripherals to improve 
>> frequent
>> accesses by the SW. It looks like it is used by MFD, SPI and I2C 
>> drivers.
>> 
>> It seems to cache the register contents and flush/invalidate them only 
>> when
>> needed.
>> 
>> The MMIO version seems to be assuming the presence of device-tree like 
>> CLK
>> API which doesn't exist on ACPI systems and is not portable.
>> 
>> My reaction is that it is a lot of code with no added functionality to 
>> what
>> HIDMA driver is trying to achieve.
>> 
>> Given that the use case here is only for debug purposes; I think it is 
>> OK
>> to keep this runtime call here. I don't want to add any overhead into 
>> the
>> existing code just to support the debug use case.
>> 
>> None of my register read/writes are slow. This file will only be used 
>> to
>> troubleshoot customer issues.
> 
> $ is always faster than MMIO. This way you can give reg contents to 
> users
> without waking up hw.
> 
> Also we at Intel use regmap on ACPI systems without CLK API

I can try and see the performance impact is. What happens to registers 
that hw updates like status registers. Those will be most interesting 
during debug. How does remap get updated for those? Is there a way to 
tell it not to cache certain registers

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web