Re: [PATCH 5/6] drivers: firmware: psci: add extended stateid power_state support
On 29 May 2015 at 08:16, Lorenzo Pieralisi lorenzo.pieral...@arm.com wrote:
PSCI v1.0 augmented the power_state parameter format specification
(extended stateid) and introduced a way to probe it through the
PSCI_FEATURES interface.
This patch implements code that detects the power_state format at
run-time through the PSCI_FEATURES interface, so that the power_state
argument can be properly detected and validated in the kernel according
to the information provided through firmware.
Signed-off-by: Lorenzo Pieralisi lorenzo.pieral...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
---
drivers/firmware/psci.c | 34 --
include/uapi/linux/psci.h | 12
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 2330099..4063784 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -74,14 +74,34 @@ static u32 psci_function_id[PSCI_FN_MAX];
PSCI_0_2_POWER_STATE_TYPE_MASK | \
PSCI_0_2_POWER_STATE_AFFL_MASK)
+#define PSCI_EXT_POWER_STATE_MASK \
+ (PSCI_EXT_POWER_STATE_ID_MASK | \
+ PSCI_EXT_POWER_STATE_TYPE_MASK)
+
+static u32 psci_cpu_suspend_feature;
+
+static inline bool psci_has_ext_power_state(void)
+{
+ return psci_cpu_suspend_feature
+ PSCI_FEATURES_CPU_SUSPEND_PF_MASK;
+}
+
bool psci_power_state_loses_context(u32 state)
{
- return state PSCI_0_2_POWER_STATE_TYPE_MASK;
+ const u32 mask = psci_has_ext_power_state() ?
+ PSCI_EXT_POWER_STATE_TYPE_MASK :
+ PSCI_0_2_POWER_STATE_TYPE_MASK;
+
+ return state mask;
}
bool psci_power_state_is_valid(u32 state)
{
- return !(state ~PSCI_0_2_POWER_STATE_MASK);
+ const u32 valid_mask = psci_has_ext_power_state() ?
+ PSCI_EXT_POWER_STATE_MASK :
+ PSCI_0_2_POWER_STATE_MASK;
+
+ return !(state ~valid_mask);
}
static int psci_to_linux_errno(int errno)
@@ -202,6 +222,14 @@ static int __init psci_features(u32 psci_func_id)
psci_func_id, 0, 0);
}
+static void __init psci_init_cpu_suspend(void)
+{
+ int feature = psci_features(psci_function_id[PSCI_FN_CPU_SUSPEND]);
+
+ if (feature != PSCI_RET_NOT_SUPPORTED)
+ psci_cpu_suspend_feature = feature;
+}
+
/*
* Detect the presence of a resident Trusted OS which may cause CPU_OFF to
* return DENIED (which would be fatal).
@@ -280,6 +308,8 @@ static int __init psci_probe(void)
psci_init_migrate();
+ psci_init_cpu_suspend();
+
return 0;
}
diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
index 187b828d..2598d7c 100644
--- a/include/uapi/linux/psci.h
+++ b/include/uapi/linux/psci.h
@@ -58,6 +58,13 @@
#define PSCI_0_2_POWER_STATE_AFFL_MASK \
(0x3 PSCI_0_2_POWER_STATE_AFFL_SHIFT)
+/* PSCI extended power state encoding for CPU_SUSPEND function */
+#define PSCI_EXT_POWER_STATE_ID_MASK 0xfff
+#define PSCI_EXT_POWER_STATE_ID_SHIFT 0
+#define PSCI_EXT_POWER_STATE_TYPE_SHIFT30
+#define PSCI_EXT_POWER_STATE_TYPE_MASK \
+ (0x1 PSCI_EXT_POWER_STATE_TYPE_SHIFT)
+
For consistency, dont you need version numbers here, like we have for v0.2?
/* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
#define PSCI_0_2_AFFINITY_LEVEL_ON 0
#define PSCI_0_2_AFFINITY_LEVEL_OFF1
@@ -78,6 +85,11 @@
#define PSCI_VERSION_MINOR(ver)\
((ver) PSCI_VERSION_MINOR_MASK)
+/* PSCI features decoding (=1.0) */
+#define PSCI_FEATURES_CPU_SUSPEND_PF_SHIFT 1
+#define PSCI_FEATURES_CPU_SUSPEND_PF_MASK \
+ (0x1 PSCI_FEATURES_CPU_SUSPEND_PF_SHIFT)
+
Likewise.
/* PSCI return values (inclusive of all PSCI versions) */
#define PSCI_RET_SUCCESS 0
#define PSCI_RET_NOT_SUPPORTED -1
--
2.2.1
___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/6] ACPI: import watchdog info of GTDT into platform device
On 26 May 2015 at 11:18, Will Deacon will.dea...@arm.com wrote: On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote: On 26 May 2015 at 08:28, Will Deacon will.dea...@arm.com wrote: On Mon, May 25, 2015 at 11:03:13AM +0100, fu@linaro.org wrote: From: Fu Wei fu@linaro.org Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by the ARM SBSA Generic Watchdog driver. Tested-by: Suravee Suthikulpanit suravee.suthikulpa...@amd.com Tested-by: Timur Tabi ti...@codeaurora.org Signed-off-by: Fu Wei fu@linaro.org --- arch/arm64/kernel/acpi.c | 145 +++ 1 file changed, 145 insertions(+) Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly. I'm not really against refactoring the code. But the GTDT looks quite specific to ARM.. ---8 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8 Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64]. Sure. Not arguing about that. :) You said the GTDT isn't really arch specific. That was a bit confusing. The ARM architecture says precisely *nothing* about ACPI, so we should try to keep arch/arm64/kernel/acpi.c to a minimum and not shovel all sorts of table conversion code in there for random peripherals. Will -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/6] ACPI: import watchdog info of GTDT into platform device
Hi Will, On 26 May 2015 at 08:28, Will Deacon will.dea...@arm.com wrote: On Mon, May 25, 2015 at 11:03:13AM +0100, fu@linaro.org wrote: From: Fu Wei fu@linaro.org Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by the ARM SBSA Generic Watchdog driver. Tested-by: Suravee Suthikulpanit suravee.suthikulpa...@amd.com Tested-by: Timur Tabi ti...@codeaurora.org Signed-off-by: Fu Wei fu@linaro.org --- arch/arm64/kernel/acpi.c | 145 +++ 1 file changed, 145 insertions(+) Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly. I'm not really against refactoring the code. But the GTDT looks quite specific to ARM.. ---8 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8 Thanks, Ashwin. -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: perf: Add support for Scorpion PMUs
On 24 February 2015 at 12:23, Ashwin Chaugule
ashwin.chaug...@linaro.org wrote:
On 20 February 2015 at 15:16, Stephen Boyd sb...@codeaurora.org wrote:
On 02/20, Will Deacon wrote:
On Fri, Feb 13, 2015 at 06:24:09PM +, Stephen Boyd wrote:
+static void scorpion_evt_setup(int idx, u32 config_base)
+{
+ u32 val;
+ u32 mask;
+ u32 vval, fval;
+ unsigned int region;
+ unsigned int group;
+ unsigned int code;
+ unsigned int group_shift;
+ bool venum_event;
+
+ krait_decode_event(config_base, region, group, code,
venum_event,
+ NULL);
+
+ group_shift = group * 8;
+ mask = 0xff group_shift;
+
+ /* Configure evtsel for the region and group */
+ if (venum_event)
+ val = SCORPION_VLPM_GROUP0;
+ else
+ val = scorpion_get_pmresrn_event(region);
+ val += group;
+ /* Mix in mode-exclusion bits */
+ val |= config_base (ARMV7_EXCLUDE_USER | ARMV7_EXCLUDE_PL1);
+ armv7_pmnc_write_evtsel(idx, val);
+
+ asm volatile(mcr p15, 0, %0, c9, c15, 0 : : r (0));
What's this guy doing?
This is the same as Krait. It's clearing some implementation
defined register. From what I can tell it's a per-event register
(i.e. PMSELR decides which event this register write actually
affects) and we do this here to reset this register to some
defined value, zero. Otherwise the reset value of this register
is UNPREDICTABLE and that would be bad. I think we might be able
to move it to the pmu reset path, but I don't know. Ashwin?
Its a count control register (PMxEVCNTCR). Theres various conditions
on which you can select when to start/stop counting. e.g. start when
another counter register overflows. Setting it to 0 was the
recommended default value on Scorpions and Kraits. Reset value is
unpredictable. So, just need to make sure this is set every time a
counter is setup. Will that still work if this is moved to the reset
path?
Cheers,
Ashwin.
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: perf: Add support for Scorpion PMUs
On 20 February 2015 at 15:16, Stephen Boyd sb...@codeaurora.org wrote:
On 02/20, Will Deacon wrote:
On Fri, Feb 13, 2015 at 06:24:09PM +, Stephen Boyd wrote:
+static void scorpion_evt_setup(int idx, u32 config_base)
+{
+ u32 val;
+ u32 mask;
+ u32 vval, fval;
+ unsigned int region;
+ unsigned int group;
+ unsigned int code;
+ unsigned int group_shift;
+ bool venum_event;
+
+ krait_decode_event(config_base, region, group, code,
venum_event,
+ NULL);
+
+ group_shift = group * 8;
+ mask = 0xff group_shift;
+
+ /* Configure evtsel for the region and group */
+ if (venum_event)
+ val = SCORPION_VLPM_GROUP0;
+ else
+ val = scorpion_get_pmresrn_event(region);
+ val += group;
+ /* Mix in mode-exclusion bits */
+ val |= config_base (ARMV7_EXCLUDE_USER | ARMV7_EXCLUDE_PL1);
+ armv7_pmnc_write_evtsel(idx, val);
+
+ asm volatile(mcr p15, 0, %0, c9, c15, 0 : : r (0));
What's this guy doing?
This is the same as Krait. It's clearing some implementation
defined register. From what I can tell it's a per-event register
(i.e. PMSELR decides which event this register write actually
affects) and we do this here to reset this register to some
defined value, zero. Otherwise the reset value of this register
is UNPREDICTABLE and that would be bad. I think we might be able
to move it to the pmu reset path, but I don't know. Ashwin?
Will have to check once I get to office tomorrow.
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: perf: Add support for Scorpion PMUs
On 11 February 2015 at 13:27, Stephen Boyd sb...@codeaurora.org wrote: On 02/10, Ashwin Chaugule wrote: Hi Stephen, On 10 February 2015 at 20:05, Stephen Boyd sb...@codeaurora.org wrote: Scorpion supports a set of local performance monitor event selection registers (LPM) sitting behind a cp15 based interface that extend the architected PMU events to include Scorpion CPU and Venum VFP specific events. To use these events the user is expected to program the lpm register with the event code shifted into the group they care about and then point the PMNx event at that region+group combo by writing a LPMn_GROUPx event. Add support for this hardware. Note: the raw event number is a pure software construct that allows us to map the multi-dimensional number space of regions, groups, and event codes into a flat event number space suitable for use by the perf framework. This is based on code originally written by Ashwin Chaugule and Neil Leeder [1] massed to become similar to the Krait PMU support code. Thanks for taking this up! Overall this series looks good to me, but from what I faintly recollect, doesn't this (and the Krait pmu code) get affected by powercollapse issues anymore? e.g. https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/arch/arm/kernel/perf_event_msm.c?h=msm-3.4id=b5ca687960f0fea2f4735e83ca5c9543474c19de Right now there isn't any power collapse support in mainline so there's no immediate problem. Once we add power collapse support (i.e. cpuidle) to the Scorpion and Krait platforms we'll need to do something in the perf event code to properly maintain the counts across idle. I imagine it would be done by registering for cpu_pm notifications and then doing the save/restore on CPU_PM_ENTER and CPU_PM_EXIT. At least, that's what you started doing in this patch[1]. And then it seems the patch you mention came after that and actually did the save/restore of the counts. [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?h=msm-3.4id=464983a7e991a484cac0bc0885cee4fee318d659 Right. Thats essential whenever the power collapse stuff goes in. Thanks, Ashwin. -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: perf: Add support for Scorpion PMUs
Hi Stephen,
On 10 February 2015 at 20:05, Stephen Boyd sb...@codeaurora.org wrote:
Scorpion supports a set of local performance monitor event
selection registers (LPM) sitting behind a cp15 based interface
that extend the architected PMU events to include Scorpion CPU
and Venum VFP specific events. To use these events the user is
expected to program the lpm register with the event code shifted
into the group they care about and then point the PMNx event at
that region+group combo by writing a LPMn_GROUPx event. Add
support for this hardware.
Note: the raw event number is a pure software construct that
allows us to map the multi-dimensional number space of regions,
groups, and event codes into a flat event number space suitable
for use by the perf framework.
This is based on code originally written by Ashwin Chaugule and
Neil Leeder [1] massed to become similar to the Krait PMU support
code.
Thanks for taking this up!
Overall this series looks good to me, but from what I faintly
recollect, doesn't this (and the Krait pmu code) get affected by
powercollapse issues anymore?
e.g.
https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/arch/arm/kernel/perf_event_msm.c?h=msm-3.4id=b5ca687960f0fea2f4735e83ca5c9543474c19de
Thanks,
Ashwin.
[1]
https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/kernel/perf_event_msm.c?h=msm-3.4
Cc: Neil Leeder nlee...@codeaurora.org
Cc: Ashwin Chaugule ashw...@codeaurora.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
Documentation/devicetree/bindings/arm/pmu.txt | 2 +
arch/arm/kernel/perf_event_cpu.c | 2 +
arch/arm/kernel/perf_event_v7.c | 395
++
3 files changed, 399 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/pmu.txt
b/Documentation/devicetree/bindings/arm/pmu.txt
index 75ef91d08f3b..6e54a9d88b7a 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -18,6 +18,8 @@ Required properties:
arm,arm11mpcore-pmu
arm,arm1176-pmu
arm,arm1136-pmu
+ qcom,scorpion-pmu
+ qcom,scorpion-mp-pmu
qcom,krait-pmu
- interrupts : 1 combined interrupt or 1 per core. If the interrupt is a
per-cpu
interrupt (PPI) then 1 interrupt should be specified.
diff --git a/arch/arm/kernel/perf_event_cpu.c
b/arch/arm/kernel/perf_event_cpu.c
index dd9acc95ebc0..010ffd241434 100644
--- a/arch/arm/kernel/perf_event_cpu.c
+++ b/arch/arm/kernel/perf_event_cpu.c
@@ -242,6 +242,8 @@ static struct of_device_id cpu_pmu_of_device_ids[] = {
{.compatible = arm,arm11mpcore-pmu, .data = armv6mpcore_pmu_init},
{.compatible = arm,arm1176-pmu, .data = armv6_1176_pmu_init},
{.compatible = arm,arm1136-pmu, .data = armv6_1136_pmu_init},
+ {.compatible = qcom,scorpion-pmu, .data = scorpion_pmu_init},
+ {.compatible = qcom,scorpion-mp-pmu, .data = scorpion_pmu_init},
{.compatible = qcom,krait-pmu,.data = krait_pmu_init},
{},
};
diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c
index 84a3ec3bc592..14bc8726f554 100644
--- a/arch/arm/kernel/perf_event_v7.c
+++ b/arch/arm/kernel/perf_event_v7.c
@@ -140,6 +140,23 @@ enum krait_perf_types {
KRAIT_PERFCTR_L1_DTLB_ACCESS= 0x12210,
};
+/* ARMv7 Scorpion specific event types */
+enum scorpion_perf_types {
+ SCORPION_LPM0_GROUP0= 0x4c,
+ SCORPION_LPM1_GROUP0= 0x50,
+ SCORPION_LPM2_GROUP0= 0x54,
+ SCORPION_L2LPM_GROUP0 = 0x58,
+ SCORPION_VLPM_GROUP0= 0x5c,
+
+ SCORPION_ICACHE_ACCESS = 0x10053,
+ SCORPION_ICACHE_MISS= 0x10052,
+
+ SCORPION_DTLB_ACCESS= 0x12013,
+ SCORPION_DTLB_MISS = 0x12012,
+
+ SCORPION_ITLB_MISS = 0x12021,
+};
+
/*
* Cortex-A8 HW events mapping
*
@@ -482,6 +499,51 @@ static const unsigned
krait_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
};
/*
+ * Scorpion HW events mapping
+ */
+static const unsigned scorpion_perf_map[PERF_COUNT_HW_MAX] = {
+ PERF_MAP_ALL_UNSUPPORTED,
+ [PERF_COUNT_HW_CPU_CYCLES] = ARMV7_PERFCTR_CPU_CYCLES,
+ [PERF_COUNT_HW_INSTRUCTIONS]= ARMV7_PERFCTR_INSTR_EXECUTED,
+ [PERF_COUNT_HW_BRANCH_INSTRUCTIONS] = ARMV7_PERFCTR_PC_WRITE,
+ [PERF_COUNT_HW_BRANCH_MISSES] =
ARMV7_PERFCTR_PC_BRANCH_MIS_PRED,
+ [PERF_COUNT_HW_BUS_CYCLES] = ARMV7_PERFCTR_CLOCK_CYCLES,
+};
+
+static const unsigned scorpion_perf_cache_map[PERF_COUNT_HW_CACHE_MAX
Re: [PATCHv10 2/4] mailbox: Introduce framework for mailbox
On 25 September 2014 20:57, Jassi Brar jaswinder.si...@linaro.org wrote:
On 24 September 2014 09:14, Ashwin Chaugule ashwin.chaug...@linaro.org
wrote:
On 22 September 2014 14:33, Sudeep Holla sudeep.ho...@arm.com wrote:
+static void poll_txdone(unsigned long data)
+{
+ struct mbox_controller *mbox = (struct mbox_controller *)data;
+ bool txdone, resched = false;
+ int i;
+
+ for (i = 0; i mbox-num_chans; i++) {
+ struct mbox_chan *chan = mbox-chans[i];
+
+ if (chan-active_req chan-cl) {
+ resched = true;
+ txdone = chan-mbox-ops-last_tx_done(chan);
+ if (txdone)
+ tx_tick(chan, 0);
+ }
+ }
+
+ if (resched)
+ mod_timer(mbox-poll, jiffies +
+ msecs_to_jiffies(mbox-period));
While preparing a different patch which uses the Mbox framework, I
noticed that mbox-period might not be initialized anywhere. Also, how
is mbox-txpoll_period to be used? It appears from the description of
txpoll_period in mbox_controller.h that you'd want to use that value
in the mod_timer above, or equate the two somewhere in the controller
registration or eliminate one of the two. FWIW I also looked at your
code in [1].
IIUC the controller needs to set the txpoll_period if it sets
txdone_poll, may be a sanity check for !0 would be good.
Ah, sorry I confused mbox-period to txpoll_period.
You are right mbox-period is not set, the header claims it to be
private, and hence I assume it needs to be handled only in core mailbox
library. Not sure if we need both mbox-period and txpoll_period though.
Right. I dont see the need for having both either. Unless the Mailbox
maintainer wants to fix this in some other way, I can send a patch for
replacing mbox-period with mbox-txpoll_period along with my PCC
work.
Yeah, probably we should just get rid of mbox_controller.period I
have updated the same in for-next.
Looks like linux-next already has the older version[1]. Your for-next
branch seems to have the fix squashed into the previous commit.
Wouldnt you need to send this as a separate patch? Or ask for the
patch to be reverted in linux-next and then pull again from your
branch.
Thanks,
Ashwin
[1] -
http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/mailbox/mailbox.c
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 2/4] mailbox: Introduce framework for mailbox
On 22 September 2014 14:33, Sudeep Holla sudeep.ho...@arm.com wrote:
On 22/09/14 19:15, Sudeep Holla wrote:
On 22/09/14 19:01, Ashwin Chaugule wrote:
Hi Jassi,
On 1 August 2014 08:31, Jassi Brar jaswinder.si...@linaro.org wrote:
Introduce common framework for client/protocol drivers and
controller drivers of Inter-Processor-Communication (IPC).
Client driver developers should have a look at
include/linux/mailbox_client.h to understand the part of
the API exposed to client drivers.
Similarly controller driver developers should have a look
at include/linux/mailbox_controller.h
Signed-off-by: Jassi Brar jaswinder.si...@linaro.org
---
MAINTAINERS| 8 +
drivers/mailbox/Makefile | 4 +
drivers/mailbox/mailbox.c | 466
+
include/linux/mailbox_client.h | 46
include/linux/mailbox_controller.h | 135 +++
5 files changed, 659 insertions(+)
create mode 100644 drivers/mailbox/mailbox.c
create mode 100644 include/linux/mailbox_client.h
create mode 100644 include/linux/mailbox_controller.h
[..]
+
+static void poll_txdone(unsigned long data)
+{
+ struct mbox_controller *mbox = (struct mbox_controller *)data;
+ bool txdone, resched = false;
+ int i;
+
+ for (i = 0; i mbox-num_chans; i++) {
+ struct mbox_chan *chan = mbox-chans[i];
+
+ if (chan-active_req chan-cl) {
+ resched = true;
+ txdone = chan-mbox-ops-last_tx_done(chan);
+ if (txdone)
+ tx_tick(chan, 0);
+ }
+ }
+
+ if (resched)
+ mod_timer(mbox-poll, jiffies +
+ msecs_to_jiffies(mbox-period));
While preparing a different patch which uses the Mbox framework, I
noticed that mbox-period might not be initialized anywhere. Also, how
is mbox-txpoll_period to be used? It appears from the description of
txpoll_period in mbox_controller.h that you'd want to use that value
in the mod_timer above, or equate the two somewhere in the controller
registration or eliminate one of the two. FWIW I also looked at your
code in [1].
IIUC the controller needs to set the txpoll_period if it sets
txdone_poll, may be a sanity check for !0 would be good.
Ah, sorry I confused mbox-period to txpoll_period.
You are right mbox-period is not set, the header claims it to be
private, and hence I assume it needs to be handled only in core mailbox
library. Not sure if we need both mbox-period and txpoll_period though.
Right. I dont see the need for having both either. Unless the Mailbox
maintainer wants to fix this in some other way, I can send a patch for
replacing mbox-period with mbox-txpoll_period along with my PCC
work.
Thanks,
Ashwin
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 2/4] mailbox: Introduce framework for mailbox
Hi Jassi,
On 1 August 2014 08:31, Jassi Brar jaswinder.si...@linaro.org wrote:
Introduce common framework for client/protocol drivers and
controller drivers of Inter-Processor-Communication (IPC).
Client driver developers should have a look at
include/linux/mailbox_client.h to understand the part of
the API exposed to client drivers.
Similarly controller driver developers should have a look
at include/linux/mailbox_controller.h
Signed-off-by: Jassi Brar jaswinder.si...@linaro.org
---
MAINTAINERS| 8 +
drivers/mailbox/Makefile | 4 +
drivers/mailbox/mailbox.c | 466
+
include/linux/mailbox_client.h | 46
include/linux/mailbox_controller.h | 135 +++
5 files changed, 659 insertions(+)
create mode 100644 drivers/mailbox/mailbox.c
create mode 100644 include/linux/mailbox_client.h
create mode 100644 include/linux/mailbox_controller.h
[..]
+
+static void poll_txdone(unsigned long data)
+{
+ struct mbox_controller *mbox = (struct mbox_controller *)data;
+ bool txdone, resched = false;
+ int i;
+
+ for (i = 0; i mbox-num_chans; i++) {
+ struct mbox_chan *chan = mbox-chans[i];
+
+ if (chan-active_req chan-cl) {
+ resched = true;
+ txdone = chan-mbox-ops-last_tx_done(chan);
+ if (txdone)
+ tx_tick(chan, 0);
+ }
+ }
+
+ if (resched)
+ mod_timer(mbox-poll, jiffies +
+ msecs_to_jiffies(mbox-period));
While preparing a different patch which uses the Mbox framework, I
noticed that mbox-period might not be initialized anywhere. Also, how
is mbox-txpoll_period to be used? It appears from the description of
txpoll_period in mbox_controller.h that you'd want to use that value
in the mod_timer above, or equate the two somewhere in the controller
registration or eliminate one of the two. FWIW I also looked at your
code in [1].
Thanks,
Ashwin
[1] -
http://git.linaro.org/landing-teams/working/fujitsu/integration.git/shortlog/refs/heads/mailbox-for-next
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 2/2] mailbox: Introduce framework for mailbox
Hello, On 14 July 2014 05:17, Jassi Brar jaswinder.si...@linaro.org wrote: On 11 July 2014 17:16, Ashwin Chaugule ashwin.chaug...@linaro.org wrote: Hi Jassi, Other than a few nits, this looks good to me. Thanks for the nits. I will club them together with other feedback on the patchset. Hopefully you've run this through checkpatch as well? Also, were you able to sort out the process of getting this stuff hosted on Linaro's git servers? or elsewhere? It would really help maintainers to pick this up and for others to rebase on top of your changes. I didn't think it was a good idea to keep a 'dev-branch' for a new subsystem before having even a single Ack or Reviewed-by. Users should track patch revisions on mailing lists and later maintainers will be sent a tree:branch to pick Acked patches from. Your private dev branch doesn't need to have any Acks/Reviewed-bys. You can add my signature after addressing the feedback. Reviewed-by: Ashwin Chaugule ashwin.chaug...@linaro.org Cheers, Ashwin -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 2/2] mailbox: Introduce framework for mailbox
Hi Jassi,
Other than a few nits, this looks good to me.
On 11 July 2014 10:35, Jassi Brar jaswinder.si...@linaro.org wrote:
Introduce common framework for client/protocol drivers and
controller drivers of Inter-Processor-Communication (IPC).
Client driver developers should have a look at
include/linux/mailbox_client.h to understand the part of
the API exposed to client drivers.
Similarly controller driver developers should have a look
at include/linux/mailbox_controller.h
I think its a better idea to refer to the Documentation/mailbox.txt
instead of these headers. You already have references to the headers
in the Doc.
Signed-off-by: Jassi Brar jaswinder.si...@linaro.org
---
.../devicetree/bindings/mailbox/mailbox.txt| 33 ++
Documentation/mailbox.txt | 107 +
MAINTAINERS| 8 +
drivers/mailbox/Makefile | 4 +
drivers/mailbox/mailbox.c | 490
+
include/linux/mailbox_client.h | 48 ++
include/linux/mailbox_controller.h | 128 ++
7 files changed, 818 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/mailbox.txt
create mode 100644 Documentation/mailbox.txt
create mode 100644 drivers/mailbox/mailbox.c
create mode 100644 include/linux/mailbox_client.h
create mode 100644 include/linux/mailbox_controller.h
diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt
b/Documentation/devicetree/bindings/mailbox/mailbox.txt
new file mode 100644
index 000..3f00955
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -0,0 +1,33 @@
+* Generic Mailbox Controller and client driver bindings
+
+Generic binding to provide a way for Mailbox controller drivers to
+assign appropriate mailbox channel to client drivers.
+
+* Mailbox Controller
+
+Required property:
+- #mbox-cells: Must be at least 1. Number of cells in a mailbox
+ specifier.
+
+Example:
+ mailbox: mailbox {
+ ...
+ #mbox-cells = 1;
+ };
+
+
+* Mailbox Client
+
+Required property:
+ mbox: List of phandle and mailbox channel specifier.
+
+- mbox-names: List of identifier strings for each mailbox channel
+ required by the client.
+
+Example:
+ pwr_cntrl: power {
+ ...
+ mbox-names = pwr-ctrl, rpc;
+ mbox = mailbox 0
+ mailbox 1;
+ };
diff --git a/Documentation/mailbox.txt b/Documentation/mailbox.txt
new file mode 100644
index 000..0a11183
--- /dev/null
+++ b/Documentation/mailbox.txt
@@ -0,0 +1,107 @@
+ The Common Mailbox Framework
+ Jassi Brar jaswinder.si...@linaro.org
+
+ This document aims to help developers write client and controller
+drivers for the API. But before we start, let us note that the
+client (especially) and controller drivers are likely going to be
+very platform specific because the remote firmware is likely to be
+proprietary and implement non-standard protocol. So even if two
+platforms employ, say, PL320 controller, the client drivers can't
+be shared across them. Even the PL320 driver might need to accomodate
s/accomodate/accommodate/
+some platform specific quirks. So the API is meant mainly to avoid
+similar copies of code written for each platform.
+ Some of the choices made during implementation are the result of this
+peculiarity of this common framework.
I'd just skip the last sentence.
+
+
+
+ Part 1 - Controller Driver (See include/linux/mailbox_controller.h)
[..]
+
+ Allocate mbox_controller and the array of mbox_chan.
+Populate mbox_chan_ops, except peek_data() all are mandatory.
+The controller driver might know a message has been consumed
+by the remote by getting an IRQ or polling some hardware flag
+or it can never know (the client knows by way of the protocol).
+The method in order of preference is IRQ - Poll - None, which
+the controller driver should set via 'txdone_irq' or 'txdone_poll'
+or neither.
Including the header definition above the text would help to keep it in context.
Same thing for the Client side.
+
+
+ Part 2 - Client Driver (See include/linux/mailbox_client.h)
+
+ The client might want to operate in blocking mode (synchronously
+send a message through before returning) or non-blocking/async mode (submit
+a message and a callback function to the API and return immediately).
+
+
+static struct mbox_chan *ch_async, *ch_blk;
+static struct mbox_client cl_async, cl_blk;
+static struct completion c_aysnc;
+
+/*
+ * This is the handler for data received from remote. The behaviour is purely
+ * dependent upon the protocol. This is just an example.
+ */
+static void message_from_remote(struct mbox_client *cl, void *mssg)
+{
+ if (cl ==
Re: [PATCHv7 2/5] mailbox: Introduce framework for mailbox
Hello, On 18 June 2014 22:55, Jassi Brar jaswinder.si...@linaro.org wrote: I sure would like to see some more Reviewed-by tags from those folks to confirm that those starting to use it think it's on the right track. The upstreaming attempts have been going on for months now, and via non-public interactions with developers I understand it last worked before the revision mandating DT support and ipc-mailbox symbol renaming. So basic working should still remain the same. Suman(TI), Loic(ST), Girish(Samsung), Ashwin (PCC+ACPI) guys, any word for v7? V7 looks fine overall but it needs some minor changes for ACPI. Something along the lines of what I submitted previously.[1] [1] - http://www.spinics.net/lists/linux-acpi/msg51156.html Cheers, Ashwin -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
