Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-12-08 Thread Sinan Kaya 
On 12/5/2015 2:58 AM, Vinod Koul wrote:
> On Tue, Dec 01, 2015 at 11:57:34PM -0500, Sinan Kaya wrote:
>> On 11/30/2015 10:17 PM, Vinod Koul wrote:
>>> On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:
>>>
>> +static int hidma_mgmt_probe(struct platform_device *pdev)
>> +{
>> +struct hidma_mgmt_dev *mgmtdev;
>> +struct resource *res;
>> +void __iomem *virtaddr;
>> +int irq;
>> +int rc;
>> +u32 val;
>> +
>> +pm_runtime_set_autosuspend_delay(>dev, 
>> AUTOSUSPEND_TIMEOUT);
>> +pm_runtime_use_autosuspend(>dev);
>> +pm_runtime_set_active(>dev);
>> +pm_runtime_enable(>dev);
>
> at this time pm core will treat device as fully enabled and pm methods can
> be invoked, but you are not ready yet right. Typically these are done at 
> the
> end of the probe unless you have a reason... 

 I need it here because the clocks are declared as ACPI power resources.
 The kernel is turning off all power resources during initialization. In
 order for this code to touch the hardware, I need to call enable so that
 clocks are enabled once again.
>>>
>>> The question is are you ready in your driver routines to be invoked by pm
>>> core?
>>>
>>
>> I don't have any support for suspend and resume PM APIs. The only PM
>> interface I support is PM runtime. PM can turn on/off the clocks based
>> on the reference counts it maintains after get/set APIs. Since PM is
>> turning off the clocks during power up before my driver load, I do need
>> to grab this lock to re-enable it during HW initialization. Then, let PM
>> turn off the clocks again after the AUTOSUSPEND_TIMEOUT when I'm done.
>>
>> Is there any other interaction with the PM that I'm not aware of?
> 
> No this is fine. The the runtime_resume will be onvoked and it will request
> resources are those set before you enable the device?
> 

Yes, the only resource that this device needs for power management is
the ACPI power resources. The device does not support suspend/resume via
traditional _PS0 and _PS3 calls. ACPI power resources are initialized
during power up while ACPI is being enumerated.

The probing of the HIDMA driver happens much afterwards.


-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-12-04 Thread Vinod Koul 
On Tue, Dec 01, 2015 at 11:57:34PM -0500, Sinan Kaya wrote:
> On 11/30/2015 10:17 PM, Vinod Koul wrote:
> > On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:
> > 
>  +static int hidma_mgmt_probe(struct platform_device *pdev)
>  +{
>  +struct hidma_mgmt_dev *mgmtdev;
>  +struct resource *res;
>  +void __iomem *virtaddr;
>  +int irq;
>  +int rc;
>  +u32 val;
>  +
>  +pm_runtime_set_autosuspend_delay(>dev, 
>  AUTOSUSPEND_TIMEOUT);
>  +pm_runtime_use_autosuspend(>dev);
>  +pm_runtime_set_active(>dev);
>  +pm_runtime_enable(>dev);
> >>>
> >>> at this time pm core will treat device as fully enabled and pm methods can
> >>> be invoked, but you are not ready yet right. Typically these are done at 
> >>> the
> >>> end of the probe unless you have a reason... 
> >>
> >> I need it here because the clocks are declared as ACPI power resources.
> >> The kernel is turning off all power resources during initialization. In
> >> order for this code to touch the hardware, I need to call enable so that
> >> clocks are enabled once again.
> > 
> > The question is are you ready in your driver routines to be invoked by pm
> > core?
> > 
> 
> I don't have any support for suspend and resume PM APIs. The only PM
> interface I support is PM runtime. PM can turn on/off the clocks based
> on the reference counts it maintains after get/set APIs. Since PM is
> turning off the clocks during power up before my driver load, I do need
> to grab this lock to re-enable it during HW initialization. Then, let PM
> turn off the clocks again after the AUTOSUSPEND_TIMEOUT when I'm done.
> 
> Is there any other interaction with the PM that I'm not aware of?

No this is fine. The the runtime_resume will be onvoked and it will request
resources are those set before you enable the device?

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-12-01 Thread Sinan Kaya 
On 11/30/2015 10:17 PM, Vinod Koul wrote:
> On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:
> 
 +static int hidma_mgmt_probe(struct platform_device *pdev)
 +{
 +  struct hidma_mgmt_dev *mgmtdev;
 +  struct resource *res;
 +  void __iomem *virtaddr;
 +  int irq;
 +  int rc;
 +  u32 val;
 +
 +  pm_runtime_set_autosuspend_delay(>dev, AUTOSUSPEND_TIMEOUT);
 +  pm_runtime_use_autosuspend(>dev);
 +  pm_runtime_set_active(>dev);
 +  pm_runtime_enable(>dev);
>>>
>>> at this time pm core will treat device as fully enabled and pm methods can
>>> be invoked, but you are not ready yet right. Typically these are done at the
>>> end of the probe unless you have a reason... 
>>
>> I need it here because the clocks are declared as ACPI power resources.
>> The kernel is turning off all power resources during initialization. In
>> order for this code to touch the hardware, I need to call enable so that
>> clocks are enabled once again.
> 
> The question is are you ready in your driver routines to be invoked by pm
> core?
> 

I don't have any support for suspend and resume PM APIs. The only PM
interface I support is PM runtime. PM can turn on/off the clocks based
on the reference counts it maintains after get/set APIs. Since PM is
turning off the clocks during power up before my driver load, I do need
to grab this lock to re-enable it during HW initialization. Then, let PM
turn off the clocks again after the AUTOSUSPEND_TIMEOUT when I'm done.

Is there any other interaction with the PM that I'm not aware of?

>>
>>>
 +static ssize_t show_values(struct device *dev, struct device_attribute 
 *attr,
 +  char *buf)
>>>
>>> Please fix the coding style here and other places as well. 
>>
>> what's the problem here?
>>
>>> Specifically
>>> please read Chapter 2
>>>
>>
>> Why is checkpatch not complaining about any of the coding style issues?
>> I'm checking my code with checkpatch before submitting. Is there any
>> other tool that would catch this?
> 
> So did you read the Chapter 2.. Quoting here
> 

I did read the chapter 2. Maybe, my lack of native english speaking but
I don't get from this sentence that function parameters need to be
aligned to the opening paranthesis.

> Descendants are always substantially shorter than the parent
> and are placed substantially to the right. The same applies to function 
> headers
> with a long argument list. 
> 

I ran Lindent and manually cleaned up the junk it introduced. The result
is this

static ssize_t show_values(struct device *dev, struct device_attribute
*attr,
   char *buf)


> Your breaking lines is not placed substantially to the right..
> I do not think checkpatch is checking this..
> 


-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-11-30 Thread Vinod Koul 
On Mon, Nov 30, 2015 at 09:42:01AM -0500, Sinan Kaya wrote:

> >> +static int hidma_mgmt_probe(struct platform_device *pdev)
> >> +{
> >> +  struct hidma_mgmt_dev *mgmtdev;
> >> +  struct resource *res;
> >> +  void __iomem *virtaddr;
> >> +  int irq;
> >> +  int rc;
> >> +  u32 val;
> >> +
> >> +  pm_runtime_set_autosuspend_delay(>dev, AUTOSUSPEND_TIMEOUT);
> >> +  pm_runtime_use_autosuspend(>dev);
> >> +  pm_runtime_set_active(>dev);
> >> +  pm_runtime_enable(>dev);
> > 
> > at this time pm core will treat device as fully enabled and pm methods can
> > be invoked, but you are not ready yet right. Typically these are done at the
> > end of the probe unless you have a reason... 
> 
> I need it here because the clocks are declared as ACPI power resources.
> The kernel is turning off all power resources during initialization. In
> order for this code to touch the hardware, I need to call enable so that
> clocks are enabled once again.

The question is are you ready in your driver routines to be invoked by pm
core?

> 
> > 
> >> +static ssize_t show_values(struct device *dev, struct device_attribute 
> >> *attr,
> >> +  char *buf)
> > 
> > Please fix the coding style here and other places as well. 
> 
> what's the problem here?
> 
> > Specifically
> > please read Chapter 2
> > 
> 
> Why is checkpatch not complaining about any of the coding style issues?
> I'm checking my code with checkpatch before submitting. Is there any
> other tool that would catch this?

So did you read the Chapter 2.. Quoting here

"Statements longer than 80 columns will be broken into sensible chunks,
unless
exceeding 80 columns significantly increases readability and does not hide
information. Descendants are always substantially shorter than the parent
and
are placed substantially to the right. The same applies to function headers
with a long argument list. However, never break user-visible strings such as
printk messages, because that breaks the ability to grep for them."

Your breaking lines is not placed substantially to the right..
I do not think checkpatch is checking this..

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-11-30 Thread Vinod Koul 
On Sun, Nov 22, 2015 at 09:28:24PM -0500, Sinan Kaya wrote:

> +++ b/Documentation/ABI/testing/sysfs-platform-hidma-mgmt
> @@ -0,0 +1,97 @@
> +What:/sys/devices/platform/hidma-mgmt*/chan*/priority
> + /sys/devices/platform/QCOM8060:*/chan*/priority
> +Date:Nov 2015
> +KernelVersion:   4.4
> +Contact: "Sinan Kaya "
> +Description:
> + Contains either 0 or 1 and indicates if the DMA channel is a
> + low priority (0) or high priority (1) channel.

What is purpose of adding sysfs entries here ?

> +
> +#define QOS_N_OFFSET 0x300
> +#define CFG_OFFSET   0x400
> +#define MAX_BUS_REQ_LEN_OFFSET   0x41C
> +#define MAX_XACTIONS_OFFSET  0x420
> +#define HW_VERSION_OFFSET0x424
> +#define CHRESET_TIMEOUT_OFFSET   0x418
> +
> +#define MAX_WR_XACTIONS_MASK GENMASK(4, 0)
> +#define MAX_RD_XACTIONS_MASK GENMASK(4, 0)
> +#define WEIGHT_MASK  GENMASK(6, 0)
> +#define MAX_BUS_REQ_LEN_MASK GENMASK(15, 0)
> +#define CHRESET_TIMEOUUT_MASKGENMASK(19, 0)
> +
> +#define MAX_WR_XACTIONS_BIT_POS  16
> +#define MAX_BUS_WR_REQ_BIT_POS   16
> +#define WRR_BIT_POS  8
> +#define PRIORITY_BIT_POS 15
> +
> +#define AUTOSUSPEND_TIMEOUT  2000
> +#define MAX_CHANNEL_WEIGHT   15

These names are quite generic and prone to collide with generic names,
please prefix them with your driver name

> +}
> +EXPORT_SYMBOL_GPL(hidma_mgmt_setup);

Why is this exported or rather who would be users of this?

> +static int hidma_mgmt_probe(struct platform_device *pdev)
> +{
> + struct hidma_mgmt_dev *mgmtdev;
> + struct resource *res;
> + void __iomem *virtaddr;
> + int irq;
> + int rc;
> + u32 val;
> +
> + pm_runtime_set_autosuspend_delay(>dev, AUTOSUSPEND_TIMEOUT);
> + pm_runtime_use_autosuspend(>dev);
> + pm_runtime_set_active(>dev);
> + pm_runtime_enable(>dev);

at this time pm core will treat device as fully enabled and pm methods can
be invoked, but you are not ready yet right. Typically these are done at the
end of the probe unless you have a reason... 

> +static ssize_t show_values(struct device *dev, struct device_attribute *attr,
> + char *buf)

Please fix the coding style here and other places as well. Specifically
please read Chapter 2

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-11-30 Thread Sinan Kaya 
On 11/30/2015 3:21 AM, Vinod Koul wrote:
> On Sun, Nov 22, 2015 at 09:28:24PM -0500, Sinan Kaya wrote:
> 
>> +++ b/Documentation/ABI/testing/sysfs-platform-hidma-mgmt
>> @@ -0,0 +1,97 @@
>> +What:   /sys/devices/platform/hidma-mgmt*/chan*/priority
>> +/sys/devices/platform/QCOM8060:*/chan*/priority
>> +Date:   Nov 2015
>> +KernelVersion:  4.4
>> +Contact:"Sinan Kaya "
>> +Description:
>> +Contains either 0 or 1 and indicates if the DMA channel is a
>> +low priority (0) or high priority (1) channel.
> 
> What is purpose of adding sysfs entries here ?
> 

The goal is to run hidma channels in the guest machines and have
management driver adjust the runtime characteristics of the channels
such as round robing weight, priority, max read request etc. All of
these are runtime configurable.

There will be a userspace application that will configure this on behalf
of the system administrator via sysfs. System administrator will decide
how to allocate hardware resources to the guest machines.


>> +
>> +#define QOS_N_OFFSET0x300
>> +#define CFG_OFFSET  0x400
>> +#define MAX_BUS_REQ_LEN_OFFSET  0x41C
>> +#define MAX_XACTIONS_OFFSET 0x420
>> +#define HW_VERSION_OFFSET   0x424
>> +#define CHRESET_TIMEOUT_OFFSET  0x418
>> +
>> +#define MAX_WR_XACTIONS_MASKGENMASK(4, 0)
>> +#define MAX_RD_XACTIONS_MASKGENMASK(4, 0)
>> +#define WEIGHT_MASK GENMASK(6, 0)
>> +#define MAX_BUS_REQ_LEN_MASKGENMASK(15, 0)
>> +#define CHRESET_TIMEOUUT_MASK   GENMASK(19, 0)
>> +
>> +#define MAX_WR_XACTIONS_BIT_POS 16
>> +#define MAX_BUS_WR_REQ_BIT_POS  16
>> +#define WRR_BIT_POS 8
>> +#define PRIORITY_BIT_POS15
>> +
>> +#define AUTOSUSPEND_TIMEOUT 2000
>> +#define MAX_CHANNEL_WEIGHT  15
> 
> These names are quite generic and prone to collide with generic names,
> please prefix them with your driver name
> 

OK

>> +}
>> +EXPORT_SYMBOL_GPL(hidma_mgmt_setup);
> 
> Why is this exported or rather who would be users of this?

This driver consists of two files as hidma_mgmt.c and hidma_mgmt_sys.c.
I'm calling hidma_mgmt_setup from the hidma_mgmt_sys.c file when a
parameter such as priority and weight is changed to reconfigure the
hardware. I got linker errors without this export as this function is in
hidma_mgmt.c file.

> 
>> +static int hidma_mgmt_probe(struct platform_device *pdev)
>> +{
>> +struct hidma_mgmt_dev *mgmtdev;
>> +struct resource *res;
>> +void __iomem *virtaddr;
>> +int irq;
>> +int rc;
>> +u32 val;
>> +
>> +pm_runtime_set_autosuspend_delay(>dev, AUTOSUSPEND_TIMEOUT);
>> +pm_runtime_use_autosuspend(>dev);
>> +pm_runtime_set_active(>dev);
>> +pm_runtime_enable(>dev);
> 
> at this time pm core will treat device as fully enabled and pm methods can
> be invoked, but you are not ready yet right. Typically these are done at the
> end of the probe unless you have a reason... 

I need it here because the clocks are declared as ACPI power resources.
The kernel is turning off all power resources during initialization. In
order for this code to touch the hardware, I need to call enable so that
clocks are enabled once again.

> 
>> +static ssize_t show_values(struct device *dev, struct device_attribute 
>> *attr,
>> +char *buf)
> 
> Please fix the coding style here and other places as well. 

what's the problem here?

> Specifically
> please read Chapter 2
> 

Why is checkpatch not complaining about any of the coding style issues?
I'm checking my code with checkpatch before submitting. Is there any
other tool that would catch this?

-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-11-23 Thread Andy Shevchenko 
On Mon, Nov 23, 2015 at 4:28 AM, Sinan Kaya  wrote:
> The Qualcomm Technologies HIDMA device has been designed
> to support virtualization technology. The driver has been
> divided into two to follow the hardware design.
>
> 1. HIDMA Management driver
> 2. HIDMA Channel driver
>
> Each HIDMA HW consists of multiple channels. These channels
> share some set of common parameters. These parameters are
> initialized by the management driver during power up.
> Same management driver is used for monitoring the execution
> of the channels. Management driver can change the performance
> behavior dynamically such as bandwidth allocation and
> prioritization.
>
> The management driver is executed in hypervisor context and
> is the main management entity for all channels provided by
> the device.
>

Thanks for an update!

Looks cool! Though still few errors spelling and style nitpicks (you
may ignore them if you wish) below.

Reviewed-by: Andy Shevchenko 

> Signed-off-by: Sinan Kaya 
> ---
>  .../ABI/testing/sysfs-platform-hidma-mgmt  |  97 +++
>  .../devicetree/bindings/dma/qcom_hidma_mgmt.txt|  61 
>  drivers/dma/qcom/Kconfig   |  11 +
>  drivers/dma/qcom/Makefile  |   1 +
>  drivers/dma/qcom/hidma_mgmt.c  | 308 
> +
>  drivers/dma/qcom/hidma_mgmt.h  |  39 +++
>  drivers/dma/qcom/hidma_mgmt_sys.c  | 299 
>  7 files changed, 816 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-hidma-mgmt
>  create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>  create mode 100644 drivers/dma/qcom/hidma_mgmt.c
>  create mode 100644 drivers/dma/qcom/hidma_mgmt.h
>  create mode 100644 drivers/dma/qcom/hidma_mgmt_sys.c
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-hidma-mgmt 
> b/Documentation/ABI/testing/sysfs-platform-hidma-mgmt
> new file mode 100644
> index 000..4fc6876
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-hidma-mgmt
> @@ -0,0 +1,97 @@
> +What:  /sys/devices/platform/hidma-mgmt*/chan*/priority
> +   /sys/devices/platform/QCOM8060:*/chan*/priority
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Contains either 0 or 1 and indicates if the DMA channel is a
> +   low priority (0) or high priority (1) channel.
> +
> +What:  /sys/devices/platform/hidma-mgmt*/chan*/weight
> +   /sys/devices/platform/QCOM8060:*/chan*/weight
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Contains 0..15 and indicates the weight of the channel among
> +   equal priority channels during round robin scheduling.
> +
> +What:  /sys/devices/platform/hidma-mgmt*/chreset_timeout_cycles
> +   /sys/devices/platform/QCOM8060:*/chreset_timeout_cycles
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Contains the platform specific cycle value to wait after a
> +   reset command is issued. If the value is chosen too short,
> +   then the HW will issue a reset failure interrupt. The value
> +   is platform specific and should not be changed without
> +   consultance.
> +
> +What:  /sys/devices/platform/hidma-mgmt*/dma_channels
> +   /sys/devices/platform/QCOM8060:*/dma_channels
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Contains the number of dma channels supported by one instance
> +   of HIDMA hardware. The value may change from chip to chip.
> +
> +What:  /sys/devices/platform/hidma-mgmt*/hw_version_major
> +   /sys/devices/platform/QCOM8060:*/hw_version_major
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Version number major for the hardware.
> +
> +What:  /sys/devices/platform/hidma-mgmt*/hw_version_minor
> +   /sys/devices/platform/QCOM8060:*/hw_version_minor
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Version number minor for the hardware.
> +
> +What:  /sys/devices/platform/hidma-mgmt*/max_rd_xactions
> +   /sys/devices/platform/QCOM8060:*/max_rd_xactions
> +Date:  Nov 2015
> +KernelVersion: 4.4
> +Contact:   "Sinan Kaya "
> +Description:
> +   Contains a value between 0 and 31. Maximum number of
> +   read transactions that can

Re: [PATCH V7 2/3] dma: add Qualcomm Technologies HIDMA management driver

2015-11-23 Thread Sinan Kaya 
On 11/23/2015 1:06 PM, Andy Shevchenko wrote:
> Thanks for an update!
> 
> Looks cool! Though still few errors spelling and style nitpicks (you
> may ignore them if you wish) below.
> 
> Reviewed-by: Andy Shevchenko 

Thanks, I'll post an updated version next week.

-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html