Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Sivaram Nair]

On Thu, Jul 07, 2016 at 07:18:34PM +0900, Alexandre Courbot wrote:
> On Thu, Jul 7, 2016 at 5:17 PM, Joseph Lo  wrote:
> > On 07/06/2016 07:39 PM, Alexandre Courbot wrote:
> >>
> >> Sorry, I will probably need to do several passes on this one to
> >> understand everything, but here is what I can say after a first look:
> >>
> >> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo  wrote:
> >>>
> >>> The Tegra BPMP (Boot and Power Management Processor) is designed for the
> >>> booting process handling, offloading the power management tasks and
> >>> some system control services from the CPU. It can be clock, DVFS,
> >>> thermal/EDP, power gating operation and system suspend/resume handling.
> >>> So the CPU and the drivers of these modules can base on the service that
> >>> the BPMP firmware driver provided to signal the event for the specific PM
> >>> action to BPMP and receive the status update from BPMP.
> >>>
> >>> Comparing to the ARM SCPI, the service provided by BPMP is message-based
> >>> communication but not method-based. The BPMP firmware driver provides the
> >>> send/receive service for the users, when the user concerns the response
> >>> time. If the user needs to get the event or update from the firmware, it
> >>> can request the MRQ service as well. The user needs to take care of the
> >>> message format, which we call BPMP ABI.
> >>>
> >>> The BPMP ABI defines the message format for different modules or usages.
> >>> For example, the clock operation needs an MRQ service code called
> >>> MRQ_CLK with specific message format which includes different sub
> >>> commands for various clock operations. This is the message format that
> >>> BPMP can recognize.
> >>>
> >>> So the user needs two things to initiate IPC between BPMP. Get the
> >>> service from the bpmp_ops structure and maintain the message format as
> >>> the BPMP ABI defined.
> >>>
> >>> Based-on-the-work-by:
> >>> Sivaram Nair 
> >>>
> >>> Signed-off-by: Joseph Lo 
> >>> ---
> >>> Changes in V2:
> >>> - None
> >>> ---
> >>>   drivers/firmware/tegra/Kconfig  |   12 +
> >>>   drivers/firmware/tegra/Makefile |1 +
> >>>   drivers/firmware/tegra/bpmp.c   |  713 +
> >>>   include/soc/tegra/bpmp.h|   29 +
> >>>   include/soc/tegra/bpmp_abi.h| 1601
> >>> +++
> >>>   5 files changed, 2356 insertions(+)
> >>>   create mode 100644 drivers/firmware/tegra/bpmp.c
> >>>   create mode 100644 include/soc/tegra/bpmp.h
> >>>   create mode 100644 include/soc/tegra/bpmp_abi.h
> >>>
> >>> diff --git a/drivers/firmware/tegra/Kconfig
> >>> b/drivers/firmware/tegra/Kconfig
> >>> index 1fa3e4e136a5..ff2730d5c468 100644
> >>> --- a/drivers/firmware/tegra/Kconfig
> >>> +++ b/drivers/firmware/tegra/Kconfig
> >>> @@ -10,4 +10,16 @@ config TEGRA_IVC
> >>>keeps the content is synchronization between host CPU and
> >>> remote
> >>>processors.
> >>>
> >>> +config TEGRA_BPMP
> >>> +   bool "Tegra BPMP driver"
> >>> +   depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC
> >>> +   help
> >>> + BPMP (Boot and Power Management Processor) is designed to
> >>> off-loading
> >>
> >>
> >> s/off-loading/off-load
> >>
> >>> + the PM functions which include clock/DVFS/thermal/power from
> >>> the CPU.
> >>> + It needs HSP as the HW synchronization and notification module
> >>> and
> >>> + IVC module as the message communication protocol.
> >>> +
> >>> + This driver manages the IPC interface between host CPU and the
> >>> + firmware running on BPMP.
> >>> +
> >>>   endmenu
> >>> diff --git a/drivers/firmware/tegra/Makefile
> >>> b/drivers/firmware/tegra/Makefile
> >>> index 92e2153e8173..e34a2f79e1ad 100644
> >>> --- a/drivers/firmware/tegra/Makefile
> >>> +++ b/drivers/firmware/tegra/Makefile
> >>> @@ -1 +1,2 @@
> >>> +obj-$(CONFIG_TEGRA_BPMP)   += bpmp.o
> >>>   obj-$(CONFIG_TEGRA_IVC)+= ivc.o
> >>> diff --git a/drivers/firmware/tegra/bpmp.c
> >>> b/drivers/firmware/tegra/bpmp.c
> >>> new file mode 100644
> >>> index ..24fda626610e
> >>> --- /dev/null
> >>> +++ b/drivers/firmware/tegra/bpmp.c
> >>> @@ -0,0 +1,713 @@
> >>> +/*
> >>> + * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> it
> >>> + * under the terms and conditions of the GNU General Public License,
> >>> + * version 2, as published by the Free Software Foundation.
> >>> + *
> >>> + * This program is distributed in the hope it will be useful, but
> >>> WITHOUT
> >>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> >>> for
> >>> + * more details.
> >>> + */
> >>> +
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Sivaram Nair]

On Tue, Jul 05, 2016 at 05:04:26PM +0800, Joseph Lo wrote:
> The Tegra BPMP (Boot and Power Management Processor) is designed for the
> booting process handling, offloading the power management tasks and
> some system control services from the CPU. It can be clock, DVFS,
> thermal/EDP, power gating operation and system suspend/resume handling.
> So the CPU and the drivers of these modules can base on the service that
> the BPMP firmware driver provided to signal the event for the specific PM
> action to BPMP and receive the status update from BPMP.
> 
> Comparing to the ARM SCPI, the service provided by BPMP is message-based
> communication but not method-based. The BPMP firmware driver provides the
> send/receive service for the users, when the user concerns the response
> time. If the user needs to get the event or update from the firmware, it
> can request the MRQ service as well. The user needs to take care of the
> message format, which we call BPMP ABI.
> 
> The BPMP ABI defines the message format for different modules or usages.
> For example, the clock operation needs an MRQ service code called
> MRQ_CLK with specific message format which includes different sub
> commands for various clock operations. This is the message format that
> BPMP can recognize.
> 
> So the user needs two things to initiate IPC between BPMP. Get the
> service from the bpmp_ops structure and maintain the message format as
> the BPMP ABI defined.
> 
> Based-on-the-work-by:
> Sivaram Nair 
> 
> Signed-off-by: Joseph Lo 
> ---
> Changes in V2:
> - None
> ---
>  drivers/firmware/tegra/Kconfig  |   12 +
>  drivers/firmware/tegra/Makefile |1 +
>  drivers/firmware/tegra/bpmp.c   |  713 +
>  include/soc/tegra/bpmp.h|   29 +
>  include/soc/tegra/bpmp_abi.h| 1601 
> +++
>  5 files changed, 2356 insertions(+)
>  create mode 100644 drivers/firmware/tegra/bpmp.c
>  create mode 100644 include/soc/tegra/bpmp.h
>  create mode 100644 include/soc/tegra/bpmp_abi.h
> 
> diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
> index 1fa3e4e136a5..ff2730d5c468 100644
> --- a/drivers/firmware/tegra/Kconfig
> +++ b/drivers/firmware/tegra/Kconfig
> @@ -10,4 +10,16 @@ config TEGRA_IVC
> keeps the content is synchronization between host CPU and remote
> processors.
>  
> +config TEGRA_BPMP
> + bool "Tegra BPMP driver"
> + depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC
> + help
> +   BPMP (Boot and Power Management Processor) is designed to off-loading
> +   the PM functions which include clock/DVFS/thermal/power from the CPU.
> +   It needs HSP as the HW synchronization and notification module and
> +   IVC module as the message communication protocol.
> +
> +   This driver manages the IPC interface between host CPU and the
> +   firmware running on BPMP.
> +
>  endmenu
> diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
> index 92e2153e8173..e34a2f79e1ad 100644
> --- a/drivers/firmware/tegra/Makefile
> +++ b/drivers/firmware/tegra/Makefile
> @@ -1 +1,2 @@
> +obj-$(CONFIG_TEGRA_BPMP) += bpmp.o
>  obj-$(CONFIG_TEGRA_IVC)  += ivc.o
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> new file mode 100644
> index ..24fda626610e
> --- /dev/null
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -0,0 +1,713 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#define BPMP_MSG_SZ  128
> +#define BPMP_MSG_DATA_SZ 120

These should come from bpmp_abi.h (MSG_MIN_SZ & MSG_MIN_DATA_SZ).

> +
> +#define __MRQ_ATTRS  0xff00
> +#define __MRQ_INDEX(id)  ((id) & ~__MRQ_ATTRS)
> +
> +#define DO_ACK   BIT(0)
> +#define RING_DOORBELLBIT(1)
> +
> +struct tegra_bpmp_soc_data {
> + u32 ch_index;   /* channel index */
> + u32 thread_ch_index;/* thread channel index */
> + u32 cpu_rx_ch_index;/* CPU Rx channel index */
> + u32 nr_ch;  /* number of total channels */
> + u32 nr_thread_ch;   /* number of thread channels */
> + u32 ch_timeout; /* channel timeout */
> + u32 thread_ch_timeout;  /* thread channel timeout */
> +};
> +
> +struct channel_info {
> + u32 tch_free;
> + u32 tch_

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Stephen Warren]

On 07/07/2016 04:18 AM, Alexandre Courbot wrote:

On Thu, Jul 7, 2016 at 5:17 PM, Joseph Lo  wrote:

On 07/06/2016 07:39 PM, Alexandre Courbot wrote:


Sorry, I will probably need to do several passes on this one to
understand everything, but here is what I can say after a first look:

On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo  wrote:


The Tegra BPMP (Boot and Power Management Processor) is designed for the
booting process handling, offloading the power management tasks and
some system control services from the CPU. It can be clock, DVFS,
thermal/EDP, power gating operation and system suspend/resume handling.
So the CPU and the drivers of these modules can base on the service that
the BPMP firmware driver provided to signal the event for the specific PM
action to BPMP and receive the status update from BPMP.

Comparing to the ARM SCPI, the service provided by BPMP is message-based
communication but not method-based. The BPMP firmware driver provides the
send/receive service for the users, when the user concerns the response
time. If the user needs to get the event or update from the firmware, it
can request the MRQ service as well. The user needs to take care of the
message format, which we call BPMP ABI.

The BPMP ABI defines the message format for different modules or usages.
For example, the clock operation needs an MRQ service code called
MRQ_CLK with specific message format which includes different sub
commands for various clock operations. This is the message format that
BPMP can recognize.

So the user needs two things to initiate IPC between BPMP. Get the
service from the bpmp_ops structure and maintain the message format as
the BPMP ABI defined.



+static struct tegra_bpmp *bpmp;


static? Ok, we only need one... for now. How about a private member in
your ivc structure that allows you to retrieve the bpmp and going
dynamic? This will require an extra argument in many functions, but is
cleaner design IMHO.


IVC is designed as a generic IPC protocol among different modules (We have
not introduced some other usages of the IVC right now.). Maybe don't churn
some other stuff into IVC is better.


Anything is fine if you can get rid of that static.


Typically the way this is handled is to store the "struct ivc" inside 
some other struct, and use the container_of macro to "move" from a 
"struct ivc *" to a "struct XXX *" where "struct XXX" contains a "struct 
ivc" field within it. That way, the IVC code's "struct ivc" knows 
nothing about any IVC client's code/structures/..., doesn't have to 
store any "client data", and yet the IVC client (the BPMP driver) can 
acquire a "struct bpmp" pointer from any "struct ivc" pointer that it 
"owns".


struct bpmp_mbox {
struct bpmp *bpmp;
struct ivc ivc;
};

void bpmp_call_ivc(struct bpmp_mbox *mb) {
ivc_something(&mb->ivc);
}

void bpmp_callback_from_ivc(struct ivc *ivc) {
struct bpmp_mbox *mb = container_of(struct bpmp_mbox, ivc, ivc);
struct bpmp *bpmp = mb->bpmp;
}

(I didn't check if I got the parameters to container_of correct above)

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Alexandre Courbot]

On Thu, Jul 7, 2016 at 5:17 PM, Joseph Lo  wrote:
> On 07/06/2016 07:39 PM, Alexandre Courbot wrote:
>>
>> Sorry, I will probably need to do several passes on this one to
>> understand everything, but here is what I can say after a first look:
>>
>> On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo  wrote:
>>>
>>> The Tegra BPMP (Boot and Power Management Processor) is designed for the
>>> booting process handling, offloading the power management tasks and
>>> some system control services from the CPU. It can be clock, DVFS,
>>> thermal/EDP, power gating operation and system suspend/resume handling.
>>> So the CPU and the drivers of these modules can base on the service that
>>> the BPMP firmware driver provided to signal the event for the specific PM
>>> action to BPMP and receive the status update from BPMP.
>>>
>>> Comparing to the ARM SCPI, the service provided by BPMP is message-based
>>> communication but not method-based. The BPMP firmware driver provides the
>>> send/receive service for the users, when the user concerns the response
>>> time. If the user needs to get the event or update from the firmware, it
>>> can request the MRQ service as well. The user needs to take care of the
>>> message format, which we call BPMP ABI.
>>>
>>> The BPMP ABI defines the message format for different modules or usages.
>>> For example, the clock operation needs an MRQ service code called
>>> MRQ_CLK with specific message format which includes different sub
>>> commands for various clock operations. This is the message format that
>>> BPMP can recognize.
>>>
>>> So the user needs two things to initiate IPC between BPMP. Get the
>>> service from the bpmp_ops structure and maintain the message format as
>>> the BPMP ABI defined.
>>>
>>> Based-on-the-work-by:
>>> Sivaram Nair 
>>>
>>> Signed-off-by: Joseph Lo 
>>> ---
>>> Changes in V2:
>>> - None
>>> ---
>>>   drivers/firmware/tegra/Kconfig  |   12 +
>>>   drivers/firmware/tegra/Makefile |1 +
>>>   drivers/firmware/tegra/bpmp.c   |  713 +
>>>   include/soc/tegra/bpmp.h|   29 +
>>>   include/soc/tegra/bpmp_abi.h| 1601
>>> +++
>>>   5 files changed, 2356 insertions(+)
>>>   create mode 100644 drivers/firmware/tegra/bpmp.c
>>>   create mode 100644 include/soc/tegra/bpmp.h
>>>   create mode 100644 include/soc/tegra/bpmp_abi.h
>>>
>>> diff --git a/drivers/firmware/tegra/Kconfig
>>> b/drivers/firmware/tegra/Kconfig
>>> index 1fa3e4e136a5..ff2730d5c468 100644
>>> --- a/drivers/firmware/tegra/Kconfig
>>> +++ b/drivers/firmware/tegra/Kconfig
>>> @@ -10,4 +10,16 @@ config TEGRA_IVC
>>>keeps the content is synchronization between host CPU and
>>> remote
>>>processors.
>>>
>>> +config TEGRA_BPMP
>>> +   bool "Tegra BPMP driver"
>>> +   depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC
>>> +   help
>>> + BPMP (Boot and Power Management Processor) is designed to
>>> off-loading
>>
>>
>> s/off-loading/off-load
>>
>>> + the PM functions which include clock/DVFS/thermal/power from
>>> the CPU.
>>> + It needs HSP as the HW synchronization and notification module
>>> and
>>> + IVC module as the message communication protocol.
>>> +
>>> + This driver manages the IPC interface between host CPU and the
>>> + firmware running on BPMP.
>>> +
>>>   endmenu
>>> diff --git a/drivers/firmware/tegra/Makefile
>>> b/drivers/firmware/tegra/Makefile
>>> index 92e2153e8173..e34a2f79e1ad 100644
>>> --- a/drivers/firmware/tegra/Makefile
>>> +++ b/drivers/firmware/tegra/Makefile
>>> @@ -1 +1,2 @@
>>> +obj-$(CONFIG_TEGRA_BPMP)   += bpmp.o
>>>   obj-$(CONFIG_TEGRA_IVC)+= ivc.o
>>> diff --git a/drivers/firmware/tegra/bpmp.c
>>> b/drivers/firmware/tegra/bpmp.c
>>> new file mode 100644
>>> index ..24fda626610e
>>> --- /dev/null
>>> +++ b/drivers/firmware/tegra/bpmp.c
>>> @@ -0,0 +1,713 @@
>>> +/*
>>> + * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but
>>> WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>>> for
>>> + * more details.
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#define BPMP_MSG_SZ128
>>> +#define BPMP_MSG_DATA_SZ   120
>>> +
>>> +#define __MRQ_ATTRS0xff00
>>> +#define __MRQ_INDEX(id)((id) & ~__MRQ_ATTRS)
>>> +
>>> +#define DO_ACK BIT(0)
>>> +#define RING_DOORBELL  BIT(1)
>>> +
>>> +struct tegra

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Joseph Lo]

On 07/06/2016 07:39 PM, Alexandre Courbot wrote:

Sorry, I will probably need to do several passes on this one to
understand everything, but here is what I can say after a first look:

On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo  wrote:

The Tegra BPMP (Boot and Power Management Processor) is designed for the
booting process handling, offloading the power management tasks and
some system control services from the CPU. It can be clock, DVFS,
thermal/EDP, power gating operation and system suspend/resume handling.
So the CPU and the drivers of these modules can base on the service that
the BPMP firmware driver provided to signal the event for the specific PM
action to BPMP and receive the status update from BPMP.

Comparing to the ARM SCPI, the service provided by BPMP is message-based
communication but not method-based. The BPMP firmware driver provides the
send/receive service for the users, when the user concerns the response
time. If the user needs to get the event or update from the firmware, it
can request the MRQ service as well. The user needs to take care of the
message format, which we call BPMP ABI.

The BPMP ABI defines the message format for different modules or usages.
For example, the clock operation needs an MRQ service code called
MRQ_CLK with specific message format which includes different sub
commands for various clock operations. This is the message format that
BPMP can recognize.

So the user needs two things to initiate IPC between BPMP. Get the
service from the bpmp_ops structure and maintain the message format as
the BPMP ABI defined.

Based-on-the-work-by:
Sivaram Nair 

Signed-off-by: Joseph Lo 
---
Changes in V2:
- None
---
  drivers/firmware/tegra/Kconfig  |   12 +
  drivers/firmware/tegra/Makefile |1 +
  drivers/firmware/tegra/bpmp.c   |  713 +
  include/soc/tegra/bpmp.h|   29 +
  include/soc/tegra/bpmp_abi.h| 1601 +++
  5 files changed, 2356 insertions(+)
  create mode 100644 drivers/firmware/tegra/bpmp.c
  create mode 100644 include/soc/tegra/bpmp.h
  create mode 100644 include/soc/tegra/bpmp_abi.h

diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
index 1fa3e4e136a5..ff2730d5c468 100644
--- a/drivers/firmware/tegra/Kconfig
+++ b/drivers/firmware/tegra/Kconfig
@@ -10,4 +10,16 @@ config TEGRA_IVC
   keeps the content is synchronization between host CPU and remote
   processors.

+config TEGRA_BPMP
+   bool "Tegra BPMP driver"
+   depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC
+   help
+ BPMP (Boot and Power Management Processor) is designed to off-loading


s/off-loading/off-load


+ the PM functions which include clock/DVFS/thermal/power from the CPU.
+ It needs HSP as the HW synchronization and notification module and
+ IVC module as the message communication protocol.
+
+ This driver manages the IPC interface between host CPU and the
+ firmware running on BPMP.
+
  endmenu
diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
index 92e2153e8173..e34a2f79e1ad 100644
--- a/drivers/firmware/tegra/Makefile
+++ b/drivers/firmware/tegra/Makefile
@@ -1 +1,2 @@
+obj-$(CONFIG_TEGRA_BPMP)   += bpmp.o
  obj-$(CONFIG_TEGRA_IVC)+= ivc.o
diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
new file mode 100644
index ..24fda626610e
--- /dev/null
+++ b/drivers/firmware/tegra/bpmp.c
@@ -0,0 +1,713 @@
+/*
+ * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+#define BPMP_MSG_SZ128
+#define BPMP_MSG_DATA_SZ   120
+
+#define __MRQ_ATTRS0xff00
+#define __MRQ_INDEX(id)((id) & ~__MRQ_ATTRS)
+
+#define DO_ACK BIT(0)
+#define RING_DOORBELL  BIT(1)
+
+struct tegra_bpmp_soc_data {
+   u32 ch_index;   /* channel index */
+   u32 thread_ch_index;/* thread channel index */
+   u32 cpu_rx_ch_index;/* CPU Rx channel index */
+   u32 nr_ch;  /* number of total channels */
+   u32 nr_thread_ch;   /* number of thread channels */
+   u32 ch_timeout; /* channel timeout */
+   u32 thread_ch_timeout;  /* thread channel timeout */
+};


With just these comments it is not clear what everything in this
structure does. Maybe a file-level comment explaining ho

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Alexandre Courbot]

On Thu, Jul 7, 2016 at 1:47 AM, Matt Longnecker  wrote:
> Alex,
>
>
> On 07/06/2016 04:39 AM, Alexandre Courbot wrote:
>>>
>>> diff --git a/include/soc/tegra/bpmp_abi.h b/include/soc/tegra/bpmp_abi.h
>>> >new file mode 100644
>>> >index ..0aaef5960e29
>>> >--- /dev/null
>>> >+++ b/include/soc/tegra/bpmp_abi.h
>>> >@@ -0,0 +1,1601 @@
>>> >+/*
>>> >+ * Copyright (c) 2014-2016, NVIDIA CORPORATION.  All rights reserved.
>>> >+ *
>>> >+ * This program is free software; you can redistribute it and/or modify
>>> > it
>>> >+ * under the terms and conditions of the GNU General Public License,
>>> >+ * version 2, as published by the Free Software Foundation.
>>> >+ *
>>> >+ * This program is distributed in the hope it will be useful, but
>>> > WITHOUT
>>> >+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
>>> > or
>>> >+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
>>> > License for
>>> >+ * more details.
>>> >+ *
>>> >+ * You should have received a copy of the GNU General Public License
>>> >+ * along with this program.  If not, see.
>>> >+ */
>>> >+
>>> >+#ifndef_ABI_BPMP_ABI_H_
>>> >+#define_ABI_BPMP_ABI_H_
>>> >+
>>>
>> ...
>>
>> There is a lot of stuff in this file, most of which we are not using
>> now - this is ok, but unless this is a file synced from an outside
>> resource maybe we should trim the structures we don't need and add
>> them as we make use of them? It helps dividing the work in bite-size
>> chunks.
>>
>> Regarding the documentation format of this file, is this valid kernel
>> documentation since the adoption of Sphynx? Or is it whatever the
>> origin is using?
>
> bpmp_abi.h is meant to be delivered as is from an NVIDIA internal repo to a
> variety of OS'es. Each of them has a different documentation standard and
> coding standard.
>
> I'd like to avoid trimming parts from the file (or even worse modifying
> parts of the file) so that future deliveries are trivial.

Makes sense, thanks to you and Stephen for the clarification.

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Matt Longnecker]

Alex,

On 07/06/2016 04:39 AM, Alexandre Courbot wrote:

diff --git a/include/soc/tegra/bpmp_abi.h b/include/soc/tegra/bpmp_abi.h
>new file mode 100644
>index ..0aaef5960e29
>--- /dev/null
>+++ b/include/soc/tegra/bpmp_abi.h
>@@ -0,0 +1,1601 @@
>+/*
>+ * Copyright (c) 2014-2016, NVIDIA CORPORATION.  All rights reserved.
>+ *
>+ * This program is free software; you can redistribute it and/or modify it
>+ * under the terms and conditions of the GNU General Public License,
>+ * version 2, as published by the Free Software Foundation.
>+ *
>+ * This program is distributed in the hope it will be useful, but WITHOUT
>+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>+ * more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program.  If not, see.
>+ */
>+
>+#ifndef_ABI_BPMP_ABI_H_
>+#define_ABI_BPMP_ABI_H_
>+


...

There is a lot of stuff in this file, most of which we are not using
now - this is ok, but unless this is a file synced from an outside
resource maybe we should trim the structures we don't need and add
them as we make use of them? It helps dividing the work in bite-size
chunks.

Regarding the documentation format of this file, is this valid kernel
documentation since the adoption of Sphynx? Or is it whatever the
origin is using?
bpmp_abi.h is meant to be delivered as is from an NVIDIA internal repo 
to a variety of OS'es. Each of them has a different documentation 
standard and coding standard.


I'd like to avoid trimming parts from the file (or even worse modifying 
parts of the file) so that future deliveries are trivial.


Thanks,
Matt Longnecker

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Stephen Warren]

On 07/06/2016 05:39 AM, Alexandre Courbot wrote:

Sorry, I will probably need to do several passes on this one to
understand everything, but here is what I can say after a first look:

On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo  wrote:

The Tegra BPMP (Boot and Power Management Processor) is designed for the
booting process handling, offloading the power management tasks and
some system control services from the CPU. It can be clock, DVFS,
thermal/EDP, power gating operation and system suspend/resume handling.
So the CPU and the drivers of these modules can base on the service that
the BPMP firmware driver provided to signal the event for the specific PM
action to BPMP and receive the status update from BPMP.

Comparing to the ARM SCPI, the service provided by BPMP is message-based
communication but not method-based. The BPMP firmware driver provides the
send/receive service for the users, when the user concerns the response
time. If the user needs to get the event or update from the firmware, it
can request the MRQ service as well. The user needs to take care of the
message format, which we call BPMP ABI.

The BPMP ABI defines the message format for different modules or usages.
For example, the clock operation needs an MRQ service code called
MRQ_CLK with specific message format which includes different sub
commands for various clock operations. This is the message format that
BPMP can recognize.

So the user needs two things to initiate IPC between BPMP. Get the
service from the bpmp_ops structure and maintain the message format as
the BPMP ABI defined.



diff --git a/include/soc/tegra/bpmp_abi.h b/include/soc/tegra/bpmp_abi.h



+#ifndef _ABI_BPMP_ABI_H_
+#define _ABI_BPMP_ABI_H_
+
+#ifdef LK
+#include 
+#endif
+
+#ifndef __ABI_PACKED
+#define __ABI_PACKED __attribute__((packed))
+#endif
+
+#ifdef NO_GCC_EXTENSIONS
+#define EMPTY char empty;
+#define EMPTY_ARRAY 1
+#else
+#define EMPTY
+#define EMPTY_ARRAY 0
+#endif
+
+#ifndef __UNION_ANON
+#define __UNION_ANON
+#endif
+/**
+ * @file
+ */
+
+
+/**
+ * @defgroup MRQ MRQ Messages
+ * @brief Messages sent to/from BPMP via IPC
+ * @{
+ *   @defgroup MRQ_Format Message Format
+ *   @defgroup MRQ_Codes Message Request (MRQ) Codes
+ *   @defgroup MRQ_Payloads Message Payloads
+ *   @defgroup Error_Codes Error Codes
+ * @}
+ */


...

There is a lot of stuff in this file, most of which we are not using
now - this is ok, but unless this is a file synced from an outside
resource maybe we should trim the structures we don't need and add
them as we make use of them? It helps dividing the work in bite-size
chunks.

Regarding the documentation format of this file, is this valid kernel
documentation since the adoption of Sphynx? Or is it whatever the
origin is using?


This file is an ABI document published by the BPMP FW team. I believe it 
would be best to use it unmodified.

Re: [PATCH V2 05/10] firmware: tegra: add BPMP support

[Alexandre Courbot]

Sorry, I will probably need to do several passes on this one to
understand everything, but here is what I can say after a first look:

On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo  wrote:
> The Tegra BPMP (Boot and Power Management Processor) is designed for the
> booting process handling, offloading the power management tasks and
> some system control services from the CPU. It can be clock, DVFS,
> thermal/EDP, power gating operation and system suspend/resume handling.
> So the CPU and the drivers of these modules can base on the service that
> the BPMP firmware driver provided to signal the event for the specific PM
> action to BPMP and receive the status update from BPMP.
>
> Comparing to the ARM SCPI, the service provided by BPMP is message-based
> communication but not method-based. The BPMP firmware driver provides the
> send/receive service for the users, when the user concerns the response
> time. If the user needs to get the event or update from the firmware, it
> can request the MRQ service as well. The user needs to take care of the
> message format, which we call BPMP ABI.
>
> The BPMP ABI defines the message format for different modules or usages.
> For example, the clock operation needs an MRQ service code called
> MRQ_CLK with specific message format which includes different sub
> commands for various clock operations. This is the message format that
> BPMP can recognize.
>
> So the user needs two things to initiate IPC between BPMP. Get the
> service from the bpmp_ops structure and maintain the message format as
> the BPMP ABI defined.
>
> Based-on-the-work-by:
> Sivaram Nair 
>
> Signed-off-by: Joseph Lo 
> ---
> Changes in V2:
> - None
> ---
>  drivers/firmware/tegra/Kconfig  |   12 +
>  drivers/firmware/tegra/Makefile |1 +
>  drivers/firmware/tegra/bpmp.c   |  713 +
>  include/soc/tegra/bpmp.h|   29 +
>  include/soc/tegra/bpmp_abi.h| 1601 
> +++
>  5 files changed, 2356 insertions(+)
>  create mode 100644 drivers/firmware/tegra/bpmp.c
>  create mode 100644 include/soc/tegra/bpmp.h
>  create mode 100644 include/soc/tegra/bpmp_abi.h
>
> diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig
> index 1fa3e4e136a5..ff2730d5c468 100644
> --- a/drivers/firmware/tegra/Kconfig
> +++ b/drivers/firmware/tegra/Kconfig
> @@ -10,4 +10,16 @@ config TEGRA_IVC
>   keeps the content is synchronization between host CPU and remote
>   processors.
>
> +config TEGRA_BPMP
> +   bool "Tegra BPMP driver"
> +   depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC
> +   help
> + BPMP (Boot and Power Management Processor) is designed to 
> off-loading

s/off-loading/off-load

> + the PM functions which include clock/DVFS/thermal/power from the 
> CPU.
> + It needs HSP as the HW synchronization and notification module and
> + IVC module as the message communication protocol.
> +
> + This driver manages the IPC interface between host CPU and the
> + firmware running on BPMP.
> +
>  endmenu
> diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
> index 92e2153e8173..e34a2f79e1ad 100644
> --- a/drivers/firmware/tegra/Makefile
> +++ b/drivers/firmware/tegra/Makefile
> @@ -1 +1,2 @@
> +obj-$(CONFIG_TEGRA_BPMP)   += bpmp.o
>  obj-$(CONFIG_TEGRA_IVC)+= ivc.o
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> new file mode 100644
> index ..24fda626610e
> --- /dev/null
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -0,0 +1,713 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#define BPMP_MSG_SZ128
> +#define BPMP_MSG_DATA_SZ   120
> +
> +#define __MRQ_ATTRS0xff00
> +#define __MRQ_INDEX(id)((id) & ~__MRQ_ATTRS)
> +
> +#define DO_ACK BIT(0)
> +#define RING_DOORBELL  BIT(1)
> +
> +struct tegra_bpmp_soc_data {
> +   u32 ch_index;   /* channel index */
> +   u32 thread_ch_index;/* thread channel index */
> +   u32 cpu_rx_ch_index;/* CPU Rx channel index */
> +   u32 nr_ch;  /* number of total channels */
> +   u32 nr_thread_ch;   /* number of thread channels */
> +   u32 ch_timeout; /* channel timeout */
> +   u32 threa