* Re: [PATCH v4 5/5] coresight: sysfs: Allow select default sink on source enable.
From: Suzuki K Poulose @ 2020-06-02 11:51 UTC (permalink / raw)
To: mike.leach, linux-arm-kernel, coresight, mathieu.poirier; +Cc: acme
In-Reply-To: <20200526104642.9526-6-mike.leach@linaro.org>
Hi Mike,
On 05/26/2020 11:46 AM, Mike Leach wrote:
> When enabling a trace source using sysfs, allow the CoreSight system to
> auto-select a default sink if none has been enabled by the user.
>
> Uses the sink select algorithm that uses the default select priorities
> set when sinks are registered with the system. At present this will
> prefer ETR over ETB / ETF.
>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
> drivers/hwtracing/coresight/coresight.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 7632d060e25d..bd1a52a65d00 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -965,8 +965,15 @@ int coresight_enable(struct coresight_device *csdev)
> */
> sink = coresight_get_enabled_sink(false);
> if (!sink) {
> - ret = -EINVAL;
> - goto out;
> + /* look for a default sink if nothing enabled */
> + sink = coresight_find_default_sink(csdev);
> + if (!sink) {
> + ret = -EINVAL;
> + goto out;
> + }
> + /* mark the default as enabled */
> + sink->activated = true;
> + dev_info(&sink->dev, "Enabled default sink.");
> }
To be honest, I would drop this change and mandate that the
user enables the sink(s). There is no way to reliably match
which ETM is tracing to the sink above in case multiple ETMs
are being enabled, even with the above message. It is important
for sysfs mode, as the user must collect the trace data manually,
unlike the perf mode where the race data is collected and presented to
the user via memory buffers.
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for platform devices
From: Jean-Philippe Brucker @ 2020-06-02 11:46 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: devicetree@vger.kernel.org, kevin.tian@intel.com,
fenghua.yu@intel.com, linux-pci@vger.kernel.org,
felix.kuehling@amd.com, robin.murphy@arm.com,
christian.koenig@amd.com, hch@infradead.org, jgg@ziepe.ca,
iommu@lists.linux-foundation.org, catalin.marinas@arm.com,
zhangfei.gao@linaro.org, will@kernel.org, linux-mm@kvack.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1517c4d97b5849e6b6d32e7d7ed35289@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 5830 bytes --]
On Tue, Jun 02, 2020 at 10:31:29AM +0000, Shameerali Kolothum Thodi wrote:
> Hi Jean,
>
> > -----Original Message-----
> > From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org]
> > On Behalf Of Jean-Philippe Brucker
> > Sent: 02 June 2020 10:39
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: devicetree@vger.kernel.org; kevin.tian@intel.com; will@kernel.org;
> > fenghua.yu@intel.com; jgg@ziepe.ca; linux-pci@vger.kernel.org;
> > felix.kuehling@amd.com; hch@infradead.org; linux-mm@kvack.org;
> > iommu@lists.linux-foundation.org; catalin.marinas@arm.com;
> > zhangfei.gao@linaro.org; robin.murphy@arm.com;
> > christian.koenig@amd.com; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for
> > platform devices
> >
> > Hi Shameer,
> >
> > On Mon, Jun 01, 2020 at 12:42:15PM +0000, Shameerali Kolothum Thodi
> > wrote:
> > > > /* IRQ and event handlers */
> > > > +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64
> > > > +*evt) {
> > > > + int ret;
> > > > + u32 perm = 0;
> > > > + struct arm_smmu_master *master;
> > > > + bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > > > + u8 type = FIELD_GET(EVTQ_0_ID, evt[0]);
> > > > + u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> > > > + struct iommu_fault_event fault_evt = { };
> > > > + struct iommu_fault *flt = &fault_evt.fault;
> > > > +
> > > > + /* Stage-2 is always pinned at the moment */
> > > > + if (evt[1] & EVTQ_1_S2)
> > > > + return -EFAULT;
> > > > +
> > > > + master = arm_smmu_find_master(smmu, sid);
> > > > + if (!master)
> > > > + return -EINVAL;
> > > > +
> > > > + if (evt[1] & EVTQ_1_READ)
> > > > + perm |= IOMMU_FAULT_PERM_READ;
> > > > + else
> > > > + perm |= IOMMU_FAULT_PERM_WRITE;
> > > > +
> > > > + if (evt[1] & EVTQ_1_EXEC)
> > > > + perm |= IOMMU_FAULT_PERM_EXEC;
> > > > +
> > > > + if (evt[1] & EVTQ_1_PRIV)
> > > > + perm |= IOMMU_FAULT_PERM_PRIV;
> > > > +
> > > > + if (evt[1] & EVTQ_1_STALL) {
> > > > + flt->type = IOMMU_FAULT_PAGE_REQ;
> > > > + flt->prm = (struct iommu_fault_page_request) {
> > > > + .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > > > + .pasid = FIELD_GET(EVTQ_0_SSID, evt[0]),
> > > > + .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > > > + .perm = perm,
> > > > + .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > > > + };
> > > > +
> > >
> > > > + if (ssid_valid)
> > > > + flt->prm.flags |=
> > IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > >
> > > Do we need to set this for STALL mode only support? I had an issue
> > > with this being set on a vSVA POC based on our D06 zip device(which is
> > > a "fake " pci dev that supports STALL mode but no PRI). The issue is,
> > > CMDQ_OP_RESUME doesn't have any ssid or SSV params and works on sid
> > and stag only.
> >
> > I don't understand the problem, arm_smmu_page_response() doesn't set SSID
> > or SSV when sending a CMDQ_OP_RESUME. Could you detail the flow of a stall
> > event and RESUME command in your prototype? Are you getting issues with
> > the host driver or the guest driver?
>
> The issue is on the host side iommu_page_response(). The flow is something like
> below.
>
> Stall: Host:-
>
> arm_smmu_handle_evt()
> iommu_report_device_fault()
> vfio_pci_iommu_dev_fault_handler()
>
> Stall: Qemu:-
>
> vfio_dma_fault_notifier_handler()
> inject_faults()
> smmuv3_inject_faults()
>
> Stall: Guest:-
>
> arm_smmu_handle_evt()
> iommu_report_device_fault()
> iommu_queue_iopf
> ...
> iopf_handle_group()
> iopf_handle_single()
> handle_mm_fault()
> iopf_complete()
> iommu_page_response()
> arm_smmu_page_response()
> arm_smmu_cmdq_issue_cmd(CMDQ_OP_RESUME)
>
> Resume: Qemu:-
>
> smmuv3_cmdq_consume(SMMU_CMD_RESUME)
> smmuv3_notify_page_resp()
> vfio:ioctl(page_response) --> struct iommu_page_response is filled
> with only version, grpid and code.
>
> Resume: Host:-
> ioctl(page_response)
> iommu_page_response() --> fails as the pending req has PASID_VALID flag
> set and it checks for a match.
I believe the fix needs to be here. It's also wrong for PRI since not all
PCIe endpoint require a PASID in the page response. Could you try the
attached patch?
Thanks,
Jean
> arm_smmu_page_response()
>
> Hope the above is clear.
>
> > We do need to forward the SSV flag all the way to the guest driver, so the guest
> > can find the faulting address space using the SSID. Once the guest handled the
> > fault, then we don't send the SSID back to the host as part of the RESUME
> > command.
>
> True, the guest requires SSV flag to handle the page fault. But, as shown in the
> flow above, the issue is on the host side iommu_page_response() where it
> searches for a matching pending req based on pasid. Not sure we can bypass
> that and call arm_smmu_page_response() directly but then have to delete the
> pending req from the list as well.
>
> Please let me know if there is a better way to handle the host side page
> response.
>
> Thanks,
> Shameer
>
> > Thanks,
> > Jean
> >
> > > Hence, it is difficult for
> > > Qemu SMMUv3 to populate this fields while preparing a page response. I
> > > can see that this flag is being checked in iopf_handle_single() (patch
> > > 04/24) as well. For POC, I used a temp fix[1] to work around this. Please let
> > me know your thoughts.
> > >
> > > Thanks,
> > > Shameer
> > >
> > > 1.
> > > https://github.com/hisilicon/kernel-dev/commit/99ff96146e924055f38d97a
> > > 5897e4becfa378d15
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #2: 0001-iommu-Allow-page-responses-without-PASID.patch --]
[-- Type: text/plain, Size: 1682 bytes --]
From 9baf5b9894d4e4be05e665d80fd0ebac8b621aa4 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
Date: Tue, 2 Jun 2020 13:13:27 +0200
Subject: [PATCH] iommu: Allow page responses without PASID
Some PCIe devices do not expect a PASID value in PRI Page Responses. If
the "PRG Response PASID Required" bit in the PRI capability is zero,
then the OS should not set the PASID field. Similarly on Arm SMMU,
responses to stall events do not have a PASID.
Currently iommu_page_response() checks that the PASID in the page
response corresponds to the one in the page request without first
checking the "PASID valid" bit. A page response coming from a guest OS
does not necessarily have a PASID, if the passed-through device does not
require one.
Allow page responses without PASID. The page request corresponding to a
page response is identified by device and by Page Response Group Index
(or stall tag).
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
drivers/iommu/iommu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e61a9fc65b7e4..e481fdfafb77c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1296,7 +1296,8 @@ int iommu_page_response(struct device *dev,
*/
list_for_each_entry(evt, ¶m->fault_param->faults, list) {
prm = &evt->fault.prm;
- pasid_valid = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+ pasid_valid = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID
+ && msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
if ((pasid_valid && prm->pasid != msg->pasid) ||
prm->grpid != msg->grpid)
--
2.26.2
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 06/12] PM / devfreq: Add cpu based scaling support to passive_governor
From: andrew-sh.cheng @ 2020-06-02 11:43 UTC (permalink / raw)
To: Chanwoo Choi
Cc: Mark Rutland, Nishanth Menon, srv_heupstream, devicetree,
Stephen Boyd, Viresh Kumar, Mark Brown, linux-pm,
Rafael J . Wysocki, Liam Girdwood, Rob Herring, linux-kernel,
Kyungmin Park, MyungJoo Ham, linux-mediatek, Sibi Sankar,
Matthias Brugger, linux-arm-kernel
In-Reply-To: <64b81eea-641c-811d-9830-718b28db4188@samsung.com>
On Thu, 2020-05-28 at 15:14 +0900, Chanwoo Choi wrote:
> Hi Andrew-sh.Cheng,
>
> Thanks for your posting. I like this approach absolutely.
> I think that it is necessary. When I developed the embedded product,
> I needed this feature always.
>
> I add the comments on below.
>
>
> And the following email is not valid. So, I dropped this email
> from Cc list.
> Saravana Kannan <skannan@codeaurora.org>
>
>
> On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
> > From: Saravana Kannan <skannan@codeaurora.org>
> >
> > Many CPU architectures have caches that can scale independent of the
> > CPUs. Frequency scaling of the caches is necessary to make sure that the
> > cache is not a performance bottleneck that leads to poor performance and
> > power. The same idea applies for RAM/DDR.
> >
> > To achieve this, this patch adds support for cpu based scaling to the
> > passive governor. This is accomplished by taking the current frequency
> > of each CPU frequency domain and then adjust the frequency of the cache
> > (or any devfreq device) based on the frequency of the CPUs. It listens
> > to CPU frequency transition notifiers to keep itself up to date on the
> > current CPU frequency.
> >
> > To decide the frequency of the device, the governor does one of the
> > following:
> > * Derives the optimal devfreq device opp from required-opps property of
> > the parent cpu opp_table.
> >
> > * Scales the device frequency in proportion to the CPU frequency. So, if
> > the CPUs are running at their max frequency, the device runs at its
> > max frequency. If the CPUs are running at their min frequency, the
> > device runs at its min frequency. It is interpolated for frequencies
> > in between.
> >
> > Andrew-sh.Cheng change
> > dev_pm_opp_xlate_opp to dev_pm_opp_xlate_required_opp devfreq->max_freq
> > to devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value
> > for kernel-5.7
> >
> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> > [Sibi: Integrated cpu-freqmap governor into passive_governor]
> > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> > drivers/devfreq/Kconfig | 2 +
> > drivers/devfreq/governor_passive.c | 278 ++++++++++++++++++++++++++++++++++---
> > include/linux/devfreq.h | 40 +++++-
> > 3 files changed, 299 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 0b1df12e0f21..d9067950af6a 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -73,6 +73,8 @@ config DEVFREQ_GOV_PASSIVE
> > device. This governor does not change the frequency by itself
> > through sysfs entries. The passive governor recommends that
> > devfreq device uses the OPP table to get the frequency/voltage.
> > + Alternatively the governor can also be chosen to scale based on
> > + the online CPUs current frequency.
> >
> > comment "DEVFREQ Drivers"
> >
> > diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
> > index 2d67d6c12dce..7dcda02a5bb7 100644
> > --- a/drivers/devfreq/governor_passive.c
> > +++ b/drivers/devfreq/governor_passive.c
> > @@ -8,11 +8,89 @@
> > */
> >
> > #include <linux/module.h>
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/cpumask.h>
> > #include <linux/device.h>
> > #include <linux/devfreq.h>
> > +#include <linux/slab.h>
> > #include "governor.h"
> >
> > -static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> > +static unsigned int xlate_cpufreq_to_devfreq(struct devfreq_passive_data *data,
>
> Need to change 'unsigned int' to 'unsigned long'
Get it.
> .
>
> > + unsigned int cpu)
> > +{
> > + unsigned int cpu_min, cpu_max, dev_min, dev_max, cpu_percent, max_state;
>
> Better to define them separately as following and then need to rename
> the variable. Usually, use the 'min_freq' and 'max_freq' word for
> the minimum/maximum frequency.
>
> unsigned int cpu_min_freq, cpu_max_freq, cpu_curr_freq, cpu_percent;
> unsigned long dev_min_freq, dev_max_freq, dev_max_state,
>
> The devfreq used 'unsigned long'. The cpufreq used 'unsigned long'
> and 'unsigned int'. You need to handle them properly.
Get it.
For cpu_freq, I separate it into "unsigned long cpu_curr_freq" and
"unsigned int cpu_curr_freq_khz"
>
>
> > + struct devfreq_cpu_state *cpu_state = data->cpu_state[cpu];
> > + struct devfreq *devfreq = (struct devfreq *)data->this;
> > + unsigned long *freq_table = devfreq->profile->freq_table;
>
> In this function, use 'cpu' work for cpufreq and use 'dev' for devfreq.
> So, I think 'dev_freq_table' is proper name instead of 'freq_table'
> for the readability.
>
> freq_table -> dev_freq_table
>
> > + struct dev_pm_opp *opp = NULL, *cpu_opp = NULL;
>
> In the get_target_freq_with_devfreq(), use 'p_opp' indicating
> the OPP of parent device. For the consistency, I think that
> use 'p_opp' instead of 'cpu_opp'.
>
> > + unsigned long cpu_freq, freq;
>
> Define the 'cpu_freq' on above with cpu_min_freq/cpu_max_freq definition.
> cpu_freq -> cpu_curr_freq.
Get it.
Will modify them for readability.
>
> > +
> > + if (!cpu_state || cpu_state->first_cpu != cpu ||
> > + !cpu_state->opp_table || !devfreq->opp_table)
> > + return 0;
> > +
> > + cpu_freq = cpu_state->freq * 1000;
> > + cpu_opp = devfreq_recommended_opp(cpu_state->dev, &cpu_freq, 0);
> > + if (IS_ERR(cpu_opp))
> > + return 0;
> > +
> > + opp = dev_pm_opp_xlate_required_opp(cpu_state->opp_table,
> > + devfreq->opp_table, cpu_opp);
> > + dev_pm_opp_put(cpu_opp);
> > +
> > + if (!IS_ERR(opp)) {
> > + freq = dev_pm_opp_get_freq(opp);
> > + dev_pm_opp_put(opp);
>
> Better to add the 'out' goto statement.
> If you use 'goto out', you can reduce the one indentation
> without 'else' statement.
Get it.
>
>
> > + } else {
>
> As I commented, when dev_pm_opp_xlate_required_opp() return successfully
> , use 'goto out'. We can remove 'else' and then reduce the unneeded indentation.
>
>
> > + /* Use Interpolation if required opps is not available */
> > + cpu_min = cpu_state->min_freq;
> > + cpu_max = cpu_state->max_freq;
> > + cpu_freq = cpu_state->freq;
> > +
> > + if (freq_table) {
> > + /* Get minimum frequency according to sorting order */
> > + max_state = freq_table[devfreq->profile->max_state - 1];
> > + if (freq_table[0] < max_state) {
> > + dev_min = freq_table[0];
> > + dev_max = max_state;
> > + } else {
> > + dev_min = max_state;
> > + dev_max = freq_table[0];
> > + }
> > + } else {
> > + if (devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value
> > + <= devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value)
> > + return 0;
> > + dev_min =
> > + devfreq->user_min_freq_req.data.freq.qos->min_freq.target_value;
> > + dev_max =
> > + devfreq->user_max_freq_req.data.freq.qos->max_freq.target_value;
>
> I think it is not proper to access the variable of pm_qos structure directly.
> Instead of direct access, you have to use the exported PM QoS function such as
> - pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY);
> - pm_qos_read_value(devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY);
Get it.
>
> > + }
> > + cpu_percent = ((cpu_freq - cpu_min) * 100) / cpu_max - cpu_min;
> > + freq = dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> > + }
>
>
> I think that you better to add 'out' jump label as following:
>
> out:
>
> > +
> > + return freq;
> > +}
> > +
> > +static int get_target_freq_with_cpufreq(struct devfreq *devfreq,
> > + unsigned long *freq)
> > +{
> > + struct devfreq_passive_data *p_data =
> > + (struct devfreq_passive_data *)devfreq->data;
> > + unsigned int cpu, target_freq = 0;
>
> Need to define 'target_freq' with 'unsigned long' type.
Get it.
>
> > +
> > + for_each_online_cpu(cpu)
> > + target_freq = max(target_freq,
> > + xlate_cpufreq_to_devfreq(p_data, cpu));
> > +
> > + *freq = target_freq;
> > +
> > + return 0;
> > +}
> > +
> > +static int get_target_freq_with_devfreq(struct devfreq *devfreq,
> > unsigned long *freq)
> > {
> > struct devfreq_passive_data *p_data
> > @@ -23,16 +101,6 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> > int i, count, ret = 0;
> >
> > /*
> > - * If the devfreq device with passive governor has the specific method
> > - * to determine the next frequency, should use the get_target_freq()
> > - * of struct devfreq_passive_data.
> > - */
> > - if (p_data->get_target_freq) {
> > - ret = p_data->get_target_freq(devfreq, freq);
> > - goto out;
> > - }
> > -
> > - /*
> > * If the parent and passive devfreq device uses the OPP table,
> > * get the next frequency by using the OPP table.
> > */
> > @@ -102,6 +170,37 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> > return ret;
> > }
> >
> > +static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> > + unsigned long *freq)
> > +{
> > + struct devfreq_passive_data *p_data =
> > + (struct devfreq_passive_data *)devfreq->data;
> > + int ret;
> > +
> > + /*
> > + * If the devfreq device with passive governor has the specific method
> > + * to determine the next frequency, should use the get_target_freq()
> > + * of struct devfreq_passive_data.
> > + */
> > + if (p_data->get_target_freq)
> > + return p_data->get_target_freq(devfreq, freq);
> > +
> > + switch (p_data->parent_type) {
> > + case DEVFREQ_PARENT_DEV:
> > + ret = get_target_freq_with_devfreq(devfreq, freq);
> > + break;
> > + case CPUFREQ_PARENT_DEV:
> > + ret = get_target_freq_with_cpufreq(devfreq, freq);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + dev_err(&devfreq->dev, "Invalid parent type\n");
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
> > {
> > int ret;
> > @@ -156,6 +255,140 @@ static int devfreq_passive_notifier_call(struct notifier_block *nb,
> > return NOTIFY_DONE;
> > }
> >
> > +static int cpufreq_passive_notifier_call(struct notifier_block *nb,
> > + unsigned long event, void *ptr)
> > +{
> > + struct devfreq_passive_data *data =
> > + container_of(nb, struct devfreq_passive_data, nb);
> > + struct devfreq *devfreq = (struct devfreq *)data->this;
> > + struct devfreq_cpu_state *cpu_state;
> > + struct cpufreq_freqs *freq = ptr;
>
> How about changing 'freq' to 'cpu_freqs'?
>
> In the drivers/cpufreq/cpufreq.c, use 'freqs' name indicating
> the instance of 'struct cpufreq_freqs'. And in order to
> identfy, how about adding 'cpu_' prefix for variable name?
>
> > + unsigned int current_freq;
>
> Need to define curr_freq with 'unsigned long' type
> and better to use 'curr_freq' variable name.
It is good to change current_freq to curr_freq, but why should it us
'unsigned long'?
I think it is 'unsigned int'.
>
> > + int ret;
> > +
> > + if (event != CPUFREQ_POSTCHANGE || !freq ||
> > + !data->cpu_state[freq->policy->cpu])
> > + return 0;
> > +
> > + cpu_state = data->cpu_state[freq->policy->cpu];
> > + if (cpu_state->freq == freq->new)
> > + return 0;
> > +
> > + /* Backup current freq and pre-update cpu state freq*/
> > + current_freq = cpu_state->freq;
> > + cpu_state->freq = freq->new;
> > +
> > + mutex_lock(&devfreq->lock);
> > + ret = update_devfreq(devfreq);
> > + mutex_unlock(&devfreq->lock);
> > + if (ret) {
> > + cpu_state->freq = current_freq;
> > + dev_err(&devfreq->dev, "Couldn't update the frequency.\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int cpufreq_passive_register(struct devfreq_passive_data **p_data)
> > +{
> > + struct devfreq_passive_data *data = *p_data;
> > + struct devfreq *devfreq = (struct devfreq *)data->this;
> > + struct device *dev = devfreq->dev.parent;
> > + struct opp_table *opp_table = NULL;
> > + struct devfreq_cpu_state *state;
>
> For the readability, I thinkt 'cpu_state' is proper instead of 'state'.
Get it.
>
> > + struct cpufreq_policy *policy;
> > + struct device *cpu_dev;
> > + unsigned int cpu;
> > + int ret;
> > +
> > + get_online_cpus();
>
> Add blank line.
Get it.
>
> > + data->nb.notifier_call = cpufreq_passive_notifier_call;
> > + ret = cpufreq_register_notifier(&data->nb,
> > + CPUFREQ_TRANSITION_NOTIFIER);
> > + if (ret) {
> > + dev_err(dev, "Couldn't register cpufreq notifier.\n");
> > + data->nb.notifier_call = NULL;
> > + goto out;
> > + }
> > +
> > + /* Populate devfreq_cpu_state */
> > + for_each_online_cpu(cpu) {
> > + if (data->cpu_state[cpu])
> > + continue;
> > +
> > + policy = cpufreq_cpu_get(cpu);
>
> cpufreq_cpu_get() might return 'NULL'. I think you need to handle
> return value as following:
>
> if (!policy) {
> ret = -EINVAL;
> goto out;
> } else if (PTR_ERR(policy) == -EPROBE_DEFER) {
> goto out;
> } else if (IS_ERR(policy) {
> ret = PTR_ERR(policy);
> dev_err(dev, "Couldn't get the cpufreq_poliy.\n");
> goto out;
> }
>
> If cpufreq_cpu_get() return successfully, to do next.
> It reduces the one indentaion.
>
>
Get it.
>
> > + if (policy) {
> > + state = kzalloc(sizeof(*state), GFP_KERNEL);
> > + if (!state) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + cpu_dev = get_cpu_device(cpu);
> > + if (!cpu_dev) {
> > + dev_err(dev, "Couldn't get cpu device.\n");
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + opp_table = dev_pm_opp_get_opp_table(cpu_dev);
> > + if (IS_ERR(devfreq->opp_table)) {
> > + ret = PTR_ERR(opp_table);
> > + goto out;
> > + }
> > +
> > + state->dev = cpu_dev;
> > + state->opp_table = opp_table;
> > + state->first_cpu = cpumask_first(policy->related_cpus);
> > + state->freq = policy->cur;
> > + state->min_freq = policy->cpuinfo.min_freq;
> > + state->max_freq = policy->cpuinfo.max_freq;
> > + data->cpu_state[cpu] = state;
>
> Add blank line.
>
> > + cpufreq_cpu_put(policy);
> > + } else {
> > + ret = -EPROBE_DEFER;
> > + goto out;
> > + }
> > + }
>
> Add blank line.
Get it.
> > +out:
> > + put_online_cpus();
> > + if (ret)
> > + return ret;
> > +
> > + /* Update devfreq */
> > + mutex_lock(&devfreq->lock);
> > + ret = update_devfreq(devfreq);
> > + mutex_unlock(&devfreq->lock);
> > + if (ret)
> > + dev_err(dev, "Couldn't update the frequency.\n");
> > +
> > + return ret;
> > +}
> > +
> > +static int cpufreq_passive_unregister(struct devfreq_passive_data **p_data)
> > +{
> > + struct devfreq_passive_data *data = *p_data;
> > + struct devfreq_cpu_state *cpu_state;
> > + int cpu;
> > +
> > + if (data->nb.notifier_call)
> > + cpufreq_unregister_notifier(&data->nb,
> > + CPUFREQ_TRANSITION_NOTIFIER);
> > +
> > + for_each_possible_cpu(cpu) {
> > + cpu_state = data->cpu_state[cpu];
> > + if (cpu_state) {
> > + if (cpu_state->opp_table)
> > + dev_pm_opp_put_opp_table(cpu_state->opp_table);
> > + kfree(cpu_state);
> > + cpu_state = NULL;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int devfreq_passive_event_handler(struct devfreq *devfreq,
> > unsigned int event, void *data)
> > {
> > @@ -165,7 +398,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> > struct notifier_block *nb = &p_data->nb;
> > int ret = 0;
> >
> > - if (!parent)
> > + if (p_data->parent_type == DEVFREQ_PARENT_DEV && !parent)
> > return -EPROBE_DEFER;
>
> If you modify the devfreq_passive_event_handler() as following,
> you can move this condition for DEVFREQ_PARENT_DEV into
> (register|unregister)_parent_dev_notifier.
>
> switch (event) {
> case DEVFREQ_GOV_START:
> ret = register_parent_dev_notifier(p_data);
> break;
> case DEVFREQ_GOV_STOP:
> ret = unregister_parent_dev_notifier(p_data);
> break;
> default:
> ret = -EINVAL;
> break;
> }
>
> return ret;
>
Get it.
> >
> > switch (event) {
> > @@ -173,13 +406,24 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
> > if (!p_data->this)
> > p_data->this = devfreq;
> >
> > - nb->notifier_call = devfreq_passive_notifier_call;
> > - ret = devfreq_register_notifier(parent, nb,
> > - DEVFREQ_TRANSITION_NOTIFIER);
> > + if (p_data->parent_type == DEVFREQ_PARENT_DEV) {
> > + nb->notifier_call = devfreq_passive_notifier_call;
> > + ret = devfreq_register_notifier(parent, nb,
> > + DEVFREQ_TRANSITION_NOTIFIER);
> > + } else if (p_data->parent_type == CPUFREQ_PARENT_DEV) {
> > + ret = cpufreq_passive_register(&p_data);
>
> I think that we better to collect the code related to notifier registration
> into one function like devfreq_pass_register_notifier() instead of
> cpufreq_passive_register() as following: I think it is more simple and readable.
>
> If you have more proper function name of register_parent_dev_notifier,
> please give your opinion.
>
> int register_parent_dev_notifier(struct devfreq_passive_data **p_data)
> switch (p_data->parent_type) {
> case DEVFREQ_PARENT_DEV:
> nb->notifier_call = devfreq_passive_notifier_call;
> ret = devfreq_register_notifier(parent, nb,
> break;
> case CPUFREQ_PARENT_DEV:
> cpufreq_register_notifier(...)
> ...
> break;
> }
Not fully understanding.
Do you mean expanding cpufreq_passive_register()?
I think leave it in function will be with clean for this code segment.
>
>
> > + } else {
> > + ret = -EINVAL;
> > + }
> > break;
> > case DEVFREQ_GOV_STOP:
> > - WARN_ON(devfreq_unregister_notifier(parent, nb,
> > - DEVFREQ_TRANSITION_NOTIFIER));
> > + if (p_data->parent_type == DEVFREQ_PARENT_DEV)
> > + WARN_ON(devfreq_unregister_notifier(parent, nb,
> > + DEVFREQ_TRANSITION_NOTIFIER));
> > + else if (p_data->parent_type == CPUFREQ_PARENT_DEV)
> > + cpufreq_passive_unregister(&p_data);
> > + else
> > + ret = -EINVAL;
>
> ditto. unregister_parent_dev_notifier(struct devfreq_passive_data **p_data)
Get it.
>
> > break;
> > default:
> > break;
> > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> > index a4b19d593151..04ce576fd6f1 100644
> > --- a/include/linux/devfreq.h
> > +++ b/include/linux/devfreq.h
> > @@ -278,6 +278,32 @@ struct devfreq_simple_ondemand_data {
> >
> > #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
> > /**
> > + * struct devfreq_cpu_state - holds the per-cpu state
> > + * @freq: the current frequency of the cpu.
> > + * @min_freq: the min frequency of the cpu.
> > + * @max_freq: the max frequency of the cpu.
> > + * @first_cpu: the cpumask of the first cpu of a policy.
> > + * @dev: reference to cpu device.
> > + * @opp_table: reference to cpu opp table.
> > + *
> > + * This structure stores the required cpu_state of a cpu.
> > + * This is auto-populated by the governor.
> > + */
> > +struct devfreq_cpu_state {> + unsigned int freq;
>
> It is better to change from 'freq' to 'curr_freq'
> for more correct expression.
Get it.
>
> > + unsigned int min_freq;
> > + unsigned int max_freq;
> > + unsigned int first_cpu;
> > + struct device *dev;
>
> How about changing the name 'dev' to 'cpu_dev'?
Okay.
>
>
> > + struct opp_table *opp_table;
> > +};
>
> devfreq_cpu_state is only handled by within driver/devfreq/governor_passive.c.
>
> So, you can move it into drivers/devfreq/governor_passive.c
> and just add the definition into include/linux/devfreq.h as following:
> It is able to prevent the access of variable of 'struct devfreq_cpu_state'
> outside.
>
> struct devfreq_cpu_state;
Get it.
>
> > +
> > +enum devfreq_parent_dev_type {
> > + DEVFREQ_PARENT_DEV,
> > + CPUFREQ_PARENT_DEV,
> > +};
> > +
> > +/**
> > * struct devfreq_passive_data - ``void *data`` fed to struct devfreq
> > * and devfreq_add_device
> > * @parent: the devfreq instance of parent device.
> > @@ -288,13 +314,15 @@ struct devfreq_simple_ondemand_data {
> > * using governors except for passive governor.
> > * If the devfreq device has the specific method to decide
> > * the next frequency, should use this callback.
> > - * @this: the devfreq instance of own device.
> > - * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
> > + * @parent_type parent type of the device
>
> Need to add ':' at the end of word. -> "parent_type:".
>
> > + * @this: the devfreq instance of own device.
> > + * @nb: the notifier block for DEVFREQ_TRANSITION_NOTIFIER list
>
> I knew that you make them with same indentation.
> But, actually, it is not related to this patch like clean-up code.
> Even if it is not pretty, you better to don't touch 'this' and 'nb' indentaion.
Get it.
>
> > + * @cpu_state: the state min/max/current frequency of all online cpu's
> > *
> > * The devfreq_passive_data have to set the devfreq instance of parent
> > * device with governors except for the passive governor. But, don't need to
> > - * initialize the 'this' and 'nb' field because the devfreq core will handle
> > - * them.
> > + * initialize the 'this', 'nb' and 'cpu_state' field because the devfreq core
> > + * will handle them.
> > */
> > struct devfreq_passive_data {
> > /* Should set the devfreq instance of parent device */
> > @@ -303,9 +331,13 @@ struct devfreq_passive_data {
> > /* Optional callback to decide the next frequency of passvice device */
> > int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
> >
> > + /* Should set the type of parent device */
> > + enum devfreq_parent_dev_type parent_type;
> > +
> > /* For passive governor's internal use. Don't need to set them */
> > struct devfreq *this;
> > struct notifier_block nb;
> > + struct devfreq_cpu_state *cpu_state[NR_CPUS];
> > };
> > #endif
> >
> >
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf
From: Suzuki K Poulose @ 2020-06-02 11:45 UTC (permalink / raw)
To: mike.leach, linux-arm-kernel, coresight, mathieu.poirier; +Cc: acme
In-Reply-To: <20200526104642.9526-5-mike.leach@linaro.org>
On 05/26/2020 11:46 AM, Mike Leach wrote:
> Add default sink selection to the perf trace handling in the etm driver.
> Uses the select default sink infrastructure to select a sink for the perf
> session, if no other sink is specified.
>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
This patch looks fine to me as such. But please see below for some
discussion on the future support for other configurations.
> ---
> .../hwtracing/coresight/coresight-etm-perf.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 84f1dcb69827..1a3169e69bb1 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> sink = coresight_get_enabled_sink(true);
> }
>
> - if (!sink)
> - goto err;
> -
> mask = &event_data->mask;
>
> /*
> @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> continue;
> }
>
> + /*
> + * No sink provided - look for a default sink for one of the
> + * devices. At present we only support topology where all CPUs
> + * use the same sink [N:1], so only need to find one sink. The
> + * coresight_build_path later will remove any CPU that does not
> + * attach to the sink, or if we have not found a sink.
> + */
> + if (!sink)
> + sink = coresight_find_default_sink(csdev);
> +
While we are here, should we remove the "find enabled sink" if the csink
is not specified via config. ? That step is problematic, as the user may
not remember which sinks were enabled. Also, we can't hit that with
perf tool as it prevents any invocation without sink (until this change).
So may be this is a good time to get rid of that ?
Also, we may need to do special handling for cases where there multiple
sinks (ETRS) and the cpus in the event mask has different preferred
sink. We can defer it for now as we don't claim to support such
configurations yet.
When we do, we could either :
1) Make sure the event is bound to a single CPU, in which case
the sink remains the same for the event.
OR
2) All the different "preferred" sinks (ETRs selected by the ETM) have
the same capabilitiy. i.e, we can move around the "sink" specific
buffers and use them where we end up using.
We can defer all of this, until we get platforms which ned the support.
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] media: stm32-dcmi: Set minimum cpufreq requirement
From: Benjamin GAIGNARD @ 2020-06-02 11:37 UTC (permalink / raw)
To: Valentin Schneider
Cc: Alexandre TORGUE, rjw@rjwysocki.net, linux-kernel@vger.kernel.org,
mcoquelin.stm32@gmail.com, Hugues FRUCHET, mchehab@kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <jhjpnahizkm.mognet@arm.com>
On 6/2/20 11:31 AM, Valentin Schneider wrote:
> Hi Benjamin,
>
> On 27/05/20 16:16, Benjamin Gaignard wrote:
>> Before start streaming set cpufreq minimum frequency requirement.
>> The cpufreq governor will adapt the frequencies and we will have
>> no latency for handling interrupts.
>>
> Few comments below from someone oblivious to your platform, they may not
> be all that relevant but I figured I'd pitch in anyway.
>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> drivers/media/platform/stm32/stm32-dcmi.c | 29 ++++++++++++++++++++++++++++-
>> 1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
>> index b8931490b83b..97c342351569 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -13,6 +13,7 @@
>>
>> #include <linux/clk.h>
>> #include <linux/completion.h>
>> +#include <linux/cpufreq.h>
>> #include <linux/delay.h>
>> #include <linux/dmaengine.h>
>> #include <linux/init.h>
>> @@ -99,6 +100,8 @@ enum state {
>>
>> #define OVERRUN_ERROR_THRESHOLD 3
>>
>> +#define DCMI_MIN_FREQ 650000 /* in KHz */
>> +
> This assumes the handling part is guaranteed to always run on the same CPU
> with the same performance profile (regardless of the platform). If that's
> not guaranteed, it feels like you'd want this to be configurable in some
> way.
Yes I could add a st,stm32-dcmi-min-frequency (in KHz) parameter the
device tree node.
>
>> struct dcmi_graph_entity {
>> struct v4l2_async_subdev asd;
>>
> [...]
>> @@ -2020,6 +2042,8 @@ static int dcmi_probe(struct platform_device *pdev)
>> goto err_cleanup;
>> }
>>
>> + dcmi->policy = cpufreq_cpu_get(0);
>> +
> Ideally you'd want to fetch the policy of the CPU your IRQ (and handling
> thread) is affined to; The only compatible DTS I found describes a single
> A7, which is somewhat limited in the affinity area...
If I move this code just before start streaming and use get_cpu(), would
it works ?
Benjamin
>
>> dev_info(&pdev->dev, "Probe done\n");
>>
>> platform_set_drvdata(pdev, dcmi);
>> @@ -2049,6 +2073,9 @@ static int dcmi_remove(struct platform_device *pdev)
>>
>> pm_runtime_disable(&pdev->dev);
>>
>> + if (dcmi->policy)
>> + cpufreq_cpu_put(dcmi->policy);
>> +
>> v4l2_async_notifier_unregister(&dcmi->notifier);
>> v4l2_async_notifier_cleanup(&dcmi->notifier);
>> media_entity_cleanup(&dcmi->vdev->entity);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model
From: Lukasz Luba @ 2020-06-02 11:31 UTC (permalink / raw)
To: Daniel Lezcano, linux-kernel, linux-pm, linux-arm-kernel,
dri-devel, linux-omap, linux-mediatek, linux-arm-msm, linux-imx
Cc: nm, juri.lelli, peterz, viresh.kumar, liviu.dudau,
bjorn.andersson, bsegall, festevam, mka, robh, amit.kucheria,
lorenzo.pieralisi, khilman, steven.price, cw00.choi, mingo,
mgorman, rui.zhang, alyssa.rosenzweig, orjan.eide, daniel,
b.zolnierkie, s.hauer, rostedt, matthias.bgg, Dietmar.Eggemann,
airlied, tomeu.vizoso, qperret, sboyd, rdunlap, rjw, agross,
kernel, sudeep.holla, patrick.bellasi, shawnguo
In-Reply-To: <d45e5592-8e11-858b-d3a3-2ec9ce1d1f54@linaro.org>
Hi Daniel,
On 6/1/20 10:44 PM, Daniel Lezcano wrote:
> On 27/05/2020 11:58, Lukasz Luba wrote:
>> Add support for other devices than CPUs. The registration function
>> does not require a valid cpumask pointer and is ready to handle new
>> devices. Some of the internal structures has been reorganized in order to
>> keep consistent view (like removing per_cpu pd pointers).
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>
> [ ... ]
>
>> }
>> EXPORT_SYMBOL_GPL(em_register_perf_domain);
>> +
>> +/**
>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device
>> + * @dev : Device for which the EM is registered
>> + *
>> + * Try to unregister the EM for the specified device (but not a CPU).
>> + */
>> +void em_dev_unregister_perf_domain(struct device *dev)
>> +{
>> + if (IS_ERR_OR_NULL(dev) || !dev->em_pd)
>> + return;
>> +
>> + if (_is_cpu_device(dev))
>> + return;
>> +
>> + mutex_lock(&em_pd_mutex);
>
> Is the mutex really needed?
I just wanted to align this unregister code with register. Since there
is debugfs dir lookup and the device's EM existence checks I thought it
wouldn't harm just to lock for a while and make sure the registration
path is not used. These two paths shouldn't affect each other, but with
modules loading/unloading I wanted to play safe.
I can change it maybe to just dmb() and the end of the function if it's
a big performance problem in this unloading path. What do you think?
>
> If this function is called that means there is no more user of the
> em_pd, no?
True, that EM users should already be unregistered i.e. thermal cooling.
>
>> + em_debug_remove_pd(dev);
>> +
>> + kfree(dev->em_pd->table);
>> + kfree(dev->em_pd);
>> + dev->em_pd = NULL;
>> + mutex_unlock(&em_pd_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(em_dev_unregister_perf_domain);
>>
>
>
Thank you for reviewing this.
Regards,
Lukasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 00/91] drm/vc4: Support BCM2711 Display Pipelin
From: Maxime Ripard @ 2020-06-02 11:04 UTC (permalink / raw)
To: Jian-Hong Pan
Cc: linux-arm-kernel, devicetree, Linux Kernel, dri-devel,
Daniel Drake, Eric Anholt, bcm-kernel-feedback-list,
linux-rpi-kernel, Linux Upstreaming Team, linux-clk,
Nicolas Saenz Julienne, linux-i2c
In-Reply-To: <CAPpJ_ec1KRwUrHGVVZrReaDPz4iga-Nvj5H652-tTKmkXL=Xmg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1807 bytes --]
Hi,
On Mon, Jun 01, 2020 at 03:58:26PM +0800, Jian-Hong Pan wrote:
> Maxime Ripard <maxime@cerno.tech> 於 2020年5月28日 週四 下午3:30寫道：
> >
> > Hi Daniel,
> >
> > On Wed, May 27, 2020 at 05:15:12PM +0800, Daniel Drake wrote:
> > > On Wed, May 27, 2020 at 5:13 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > I'm about to send a v3 today or tomorrow, I can Cc you (and Jian-Hong) if you
> > > > want.
> > >
> > > That would be great, although given the potentially inconsistent
> > > results we've been seeing so far it would be great if you could
> > > additionally push a git branch somewhere.
> > > That way we can have higher confidence that we are applying exactly
> > > the same patches to the same base etc.
> >
> > So I sent a new iteration yesterday, and of course forgot to cc you... Sorry for
> > that.
> >
> > I've pushed my current branch here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/mripard/linux.git/log/?h=rpi4-kms
>
> Thanks to Maxime!
>
> I have tried your repository on branch rpi4-kms. The DRM VC4 is used!
> But got some issues:
> 1. Some weird error message in dmesg. Not sure it is related, or not
> [ 5.219321] [drm:vc5_hdmi_init_resources] *ERROR* Failed to get
> HDMI state machine clock
> https://gist.github.com/starnight/3f317dca121065a361cf08e91225e389
That's a deferred probing. The first time the HDMI driver is being
probed, the firmware clock driver has not been probed yet. It's making
another attempt later on, which succeeds.
> 2. The screen flashes suddenly sometimes.
>
> 3. The higher resolutions, like 1920x1080 ... are lost after hot
> re-plug HDMI cable (HDMI0)
I'm not sure on how to exactly reproduce those issues (or what they are)
though, can you expand on this?
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC 0/2] iommu: arm-smmu: Add support for early direct mappings
From: Thierry Reding @ 2020-06-02 11:02 UTC (permalink / raw)
To: Will Deacon
Cc: linux-arm-msm, Joerg Roedel, Bjorn Andersson, iommu, John Stultz,
linux-tegra, Robin Murphy, linux-arm-kernel, Laurentiu Tudor
In-Reply-To: <20200527110343.GD11111@willie-the-truck>
[-- Attachment #1.1: Type: text/plain, Size: 5292 bytes --]
On Wed, May 27, 2020 at 12:03:44PM +0100, Will Deacon wrote:
> Hi John, Bjorn,
>
> On Tue, May 26, 2020 at 01:34:45PM -0700, John Stultz wrote:
> > On Thu, May 14, 2020 at 12:34 PM <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Thu 27 Feb 18:57 PST 2020, Bjorn Andersson wrote:
> > >
> > > Rob, Will, we're reaching the point where upstream has enough
> > > functionality that this is becoming a critical issue for us.
> > >
> > > E.g. Lenovo Yoga C630 is lacking this and a single dts patch to boot
> > > mainline with display, GPU, WiFi and audio working and the story is
> > > similar on several devboards.
> > >
> > > As previously described, the only thing I want is the stream mapping
> > > related to the display controller in place, either with the CB with
> > > translation disabled or possibly with a way to specify the framebuffer
> > > region (although this turns out to mess things up in the display
> > > driver...)
> > >
> > > I did pick this up again recently and concluded that by omitting the
> > > streams for the USB controllers causes an instability issue seen on one
> > > of the controller to disappear. So I would prefer if we somehow could
> > > have a mechanism to only pick the display streams and the context
> > > allocation for this.
> > >
> > >
> > > Can you please share some pointers/insights/wishes for how we can
> > > conclude on this subject?
> >
> > Ping? I just wanted to follow up on this discussion as this small
> > series is crucial for booting mainline on the Dragonboard 845c
> > devboard. It would be really valuable to be able to get some solution
> > upstream so we can test mainline w/o adding additional patches.
>
> Sorry, it's been insanely busy recently and I haven't had a chance to think
> about this on top of everything else. We're also carrying a hack in Android
> for you :)
>
> > The rest of the db845c series has been moving forward smoothly, but
> > this set seems to be very stuck with no visible progress since Dec.
> >
> > Are there any pointers for what folks would prefer to see?
>
> I've had a chat with Robin about this. Originally, I was hoping that
> people would all work together towards an idyllic future where firmware
> would be able to describe arbitrary pre-existing mappings for devices,
> irrespective of the IOMMU through which they master and Linux could
> inherit this configuration. However, that hasn't materialised (there was
> supposed to be an IORT update, but I don't know what happened to that)
> and, in actual fact, the problem that you have on db845 is /far/ more
> restricted than the general problem.
It doesn't sound to me like implementing platform-specific workarounds
is a good long-term solution (especially since, according to Bjorn, they
aren't as trivial to implement as it sounds). And we already have all
the infrastructure in place to implement what you describe, so I don't
see why we shouldn't do that. This patchset uses standard device tree
bindings that were designed for exactly this kind of use-case.
So at least for device-tree based boot firmware can already describe
these pre-existing mappings. If something standard materializes for ACPI
eventually I'm sure we can find ways to integrate that into whatever we
come up with now for DT.
I think between Bjorn, John, Laurentiu and myself there's pretty broad
consensus (correct me if I'm wrong, guys) that solving this via reserved
memory regions is a good solution that works. So I think what's really
missing is feedback on whether the changes proposed here or Laurentiu's
updated proposal[0] are acceptable, and if not, what the preference is
for getting something equivalent upstream.
Just to highlight: the IOMMU framework already provides infrastructure
to create direct mappings (via iommu_get_resv_regions(), called from
iommu_create_device_direct_mappings()). I have patches that make use of
this on Tegra210 and earlier where a non-ARM SMMU is used and where the
IOMMU driver enables translations (and doesn't fault by default) only at
device attachment time. That works perfectly using reserved-memory
regions. Perhaps that infrastructure could be extended to cover the
kinds of early mappings that we're discussing here. On the other hand it
might be a bit premature at this point because the ARM SMMU is the only
device that currently needs this, as far as I can tell.
Thierry
[0]: https://patchwork.ozlabs.org/project/linux-tegra/list/?series=164853
> Could you please try hacking something along the following lines and see
> how you get on? You may need my for-joerg/arm-smmu/updates branch for
> all the pieces:
>
> 1. Use the ->cfg_probe() callback to reserve the SMR/S2CRs you need
> "pinning" and configure for bypass.
>
> 2. Use the ->def_domain_type() callback to return IOMMU_DOMAIN_IDENTITY
> for the display controller
>
> I /think/ that's sufficient, but note that it differs from the current
> approach because we don't end up reserving a CB -- bypass is configured
> in the S2CR instead. Some invalidation might therefore be needed in
> ->cfg_probe() after unhooking the CB.
>
> Thanks, and please yell if you run into problems with this approach.
>
> Will
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] ARM: dts: imx6sll: Make ssi node name same as other platforms
From: Shengjiu Wang @ 2020-06-02 10:44 UTC (permalink / raw)
To: robh+dt, shawnguo, s.hauer, kernel, festevam, linux-imx,
devicetree, linux-arm-kernel, linux-kernel
In imx6sll.dtsi, the ssi node name is different with other
platforms (imx6qdl, imx6sl, imx6sx), but the
sound/soc/fsl/fsl-asoc-card.c machine driver needs to check
ssi node name for audmux configuration, then different ssi
node name causes issue on imx6sll platform.
So we change ssi node name to make all platforms have same
name.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
arch/arm/boot/dts/imx6sll.dtsi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/imx6sll.dtsi b/arch/arm/boot/dts/imx6sll.dtsi
index edd3abb9a9f1..51e59e16f6c9 100644
--- a/arch/arm/boot/dts/imx6sll.dtsi
+++ b/arch/arm/boot/dts/imx6sll.dtsi
@@ -271,7 +271,7 @@
status = "disabled";
};
- ssi1: ssi-controller@2028000 {
+ ssi1: ssi@2028000 {
compatible = "fsl,imx6sl-ssi", "fsl,imx51-ssi";
reg = <0x02028000 0x4000>;
interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
@@ -284,7 +284,7 @@
status = "disabled";
};
- ssi2: ssi-controller@202c000 {
+ ssi2: ssi@202c000 {
compatible = "fsl,imx6sl-ssi", "fsl,imx51-ssi";
reg = <0x0202c000 0x4000>;
interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
@@ -297,7 +297,7 @@
status = "disabled";
};
- ssi3: ssi-controller@2030000 {
+ ssi3: ssi@2030000 {
compatible = "fsl,imx6sl-ssi", "fsl,imx51-ssi";
reg = <0x02030000 0x4000>;
interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
--
2.21.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem driver
From: Hans Verkuil @ 2020-06-02 10:44 UTC (permalink / raw)
To: Vishal Sagar, Hyun Kwon, laurent.pinchart@ideasonboard.com,
mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
Michal Simek, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Dinesh Kumar, Sandip Kothari,
Joe Perches
In-Reply-To: <DM6PR02MB6876F989682935D38AA9BF19A78A0@DM6PR02MB6876.namprd02.prod.outlook.com>
On 01/06/2020 16:59, Vishal Sagar wrote:
> Hi Hans,
>
> Thanks for reviewing!
>
>>> + case V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED:
>>> + if (!xsdirxss->vidlocked) {
>>> + dev_err(dev, "Can't get values when video not
>> locked!\n");
>>> + return -EINVAL;
>>> + }
>>> + ctrl->val = xsdirxss->ts_is_interlaced;
>>
>> This control makes no sense: the v4l2_dv_timings struct will already tell you
>> if it is an interlaced format or not. Same for v4l2_mbus_framefmt.
>>
>
> The SDI has a concept of supporting progressive, interlaced (both as we know normally) and a progressive segmented frames(psf).
> The progressive segmented frames have their video content in progressive format but the transport stream is interlaced.
> This is distinguished using the bit 6 and 7 of Byte 2 in the 4 byte ST352 payload.
> Refer to sec 5.3 in SMPTE ST 352:2010.
>
> This control can be used by the application to distinguish normal interlaced and progressive segmented frames.
Ah, interesting. So this relies on the receiver to reconstruct the progressive
frame by combining the top and bottom field, right?
I think this deserves a new v4l2_field value:
V4L2_FIELD_ALTERNATE_PROG
Basically this is identical to V4L2_FIELD_ALTERNATE, except that the two fields
combine to a single progressive frame.
Regards,
Hans
PS: I'll look at your other comments separately
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] net: stmmac: Drop condition with no effect
From: Aishwarya Ramakrishnan @ 2020-06-02 10:44 UTC (permalink / raw)
To: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, netdev, linux-stm32,
linux-arm-kernel, linux-kernel
Cc: aishwaryarj100
As the "else if" and "else" branch body are identical the
condition has no effect. So removing "else if" condition.
Signed-off-by: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index bcda49dcf619..f59813a0405c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -229,8 +229,6 @@ static int stmmac_mtl_setup(struct platform_device *pdev,
plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WFQ;
else if (of_property_read_bool(tx_node, "snps,tx-sched-dwrr"))
plat->tx_sched_algorithm = MTL_TX_ALGORITHM_DWRR;
- else if (of_property_read_bool(tx_node, "snps,tx-sched-sp"))
- plat->tx_sched_algorithm = MTL_TX_ALGORITHM_SP;
else
plat->tx_sched_algorithm = MTL_TX_ALGORITHM_SP;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v6 2/2] hwrng: add sec-rng driver
From: Greg Kroah-Hartman @ 2020-06-02 10:38 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, Herbert Xu, Arnd Bergmann, Sean Wang, lkml,
wsd_upstream, Rob Herring, linux-mediatek, linux-crypto,
Matt Mackall, Matthias Brugger, Crystal Guo, linux-arm-kernel
In-Reply-To: <1591085678-22764-3-git-send-email-neal.liu@mediatek.com>
On Tue, Jun 02, 2020 at 04:14:38PM +0800, Neal Liu wrote:
> For security awareness SoCs on ARMv8 with TrustZone enabled,
> peripherals like entropy sources is not accessible from normal world
> (linux) and rather accessible from secure world (HYP/ATF/TEE) only.
> This driver aims to provide a generic interface to Arm Trusted
> Firmware or Hypervisor rng service.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---
> drivers/char/hw_random/Kconfig | 13 ++++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/sec-rng.c | 155 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 169 insertions(+)
> create mode 100644 drivers/char/hw_random/sec-rng.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 9bc46da..cb9c8a9 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -474,6 +474,19 @@ config HW_RANDOM_KEYSTONE
> help
> This option enables Keystone's hardware random generator.
>
> +config HW_RANDOM_SECURE
> + tristate "Arm Security Random Number Generator support"
> + depends on HAVE_ARM_SMCCC || COMPILE_TEST
> + default HW_RANDOM
> + help
> + This driver provides kernel-side support for the Arm Security
> + Random Number Generator.
> +
> + To compile this driver as a module, choose M here. the
> + module will be called sec-rng.
> +
> + If unsure, say Y.
Why Y?
> +
> endif # HW_RANDOM
>
> config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index a7801b4..04533d1 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o
> obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o
> obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o
> obj-$(CONFIG_HW_RANDOM_NPCM) += npcm-rng.o
> +obj-$(CONFIG_HW_RANDOM_SECURE) += sec-rng.o
> diff --git a/drivers/char/hw_random/sec-rng.c b/drivers/char/hw_random/sec-rng.c
> new file mode 100644
> index 0000000..c6d3872
> --- /dev/null
> +++ b/drivers/char/hw_random/sec-rng.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 MediaTek Inc.
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/hw_random.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define SMC_RET_NUM 4
> +#define SEC_RND_SIZE (sizeof(u32) * SMC_RET_NUM)
> +
> +#define HWRNG_SMC_FAST_CALL_VAL(func_num) \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_SIP, (func_num))
> +
> +#define to_sec_rng(p) container_of(p, struct sec_rng_priv, rng)
> +
> +typedef void (sec_rng_fn)(unsigned long, unsigned long, unsigned long,
> + unsigned long, unsigned long, unsigned long,
> + unsigned long, unsigned long,
> + struct arm_smccc_res *);
Why not throw some more unsigned longs in there? :)
Seriously, no variable names for these? Why not?
And given that you only use the first parameter, why have 7 of them that
are not used at all? That feels pointless and needlessly complex.
> +
> +struct sec_rng_priv {
> + u16 func_num;
> + sec_rng_fn *rng_fn;
> + struct hwrng rng;
> +};
Nit, if you put 'struct hwrng' at the top of the structure, your
"to_sec_rng()" macro resolves to a simple cast, no math at all.
> +
> +/* Simple wrapper functions to be able to use a function pointer */
> +static void sec_rng_smc(unsigned long a0, unsigned long a1,
> + unsigned long a2, unsigned long a3,
> + unsigned long a4, unsigned long a5,
> + unsigned long a6, unsigned long a7,
> + struct arm_smccc_res *res)
> +{
> + arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> +}
> +
> +static void sec_rng_hvc(unsigned long a0, unsigned long a1,
> + unsigned long a2, unsigned long a3,
> + unsigned long a4, unsigned long a5,
> + unsigned long a6, unsigned long a7,
> + struct arm_smccc_res *res)
> +{
> + arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
> +}
> +
> +static bool __sec_get_rnd(struct sec_rng_priv *priv, uint32_t *val)
> +{
> + struct arm_smccc_res res;
> +
> + priv->rng_fn(HWRNG_SMC_FAST_CALL_VAL(priv->func_num),
> + 0, 0, 0, 0, 0, 0, 0, &res);
See, all 0's :(
You could hard-code them in the functions above instead.
But, all of this pointer indirection is really odd, why is it needed at
all? Why not just call one or the other depending on the "type" at
runtime? Wouldn't that actually be faster (hint, it is...), if you
cared about speed here (hint, I doubt it matters).
> +
> + if (!res.a0 && !res.a1 && !res.a2 && !res.a3)
> + return false;
> +
> + val[0] = res.a0;
> + val[1] = res.a1;
> + val[2] = res.a2;
> + val[3] = res.a3;
So no values out of the random number generator can be 0? Feels like an
odd thing for a random number not to be allowed to do, why this
restriction?
> +
> + return true;
> +}
> +
> +static int sec_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct sec_rng_priv *priv = to_sec_rng(rng);
> + u32 val[4] = {0};
> + int retval = 0;
> + int i;
> +
> + while (max >= SEC_RND_SIZE) {
> + if (!__sec_get_rnd(priv, val))
> + return retval;
> +
> + for (i = 0; i < SMC_RET_NUM; i++) {
> + *(u32 *)buf = val[i];
> + buf += sizeof(u32);
Wait, what happens if buf is not a multiple of 4? Didn't you just
overwrite some memory above with the previous line?
> + }
> +
> + retval += SEC_RND_SIZE;
> + max -= SEC_RND_SIZE;
> + }
> +
> + return retval;
> +}
> +
> +static int sec_rng_probe(struct platform_device *pdev)
> +{
> + struct sec_rng_priv *priv;
> + const char *method;
> + int ret;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + if (of_property_read_string(pdev->dev.of_node, "method", &method))
> + return -ENXIO;
> +
> + if (!strncmp("smc", method, strlen("smc")))
> + priv->rng_fn = sec_rng_smc;
> + else if (!strncmp("hvc", method, strlen("hvc")))
> + priv->rng_fn = sec_rng_hvc;
> +
> + if (IS_ERR(priv->rng_fn)) {
How can this ever be true?
Just put another else on the above list and you should be fine.
> + dev_err(&pdev->dev, "method %s is not supported\n", method);
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u16(pdev->dev.of_node, "method-fid",
> + &priv->func_num))
> + return -ENXIO;
> +
> + if (of_property_read_u16(pdev->dev.of_node, "quality",
> + &priv->rng.quality))
> + return -ENXIO;
> +
> + priv->rng.name = pdev->name;
> + priv->rng.read = sec_rng_read;
> + priv->rng.priv = (unsigned long)&pdev->dev;
> +
> + ret = devm_hwrng_register(&pdev->dev, &priv->rng);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register rng device: %d\n", ret);
Doesn't the caller print out something if this fails?
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v10] mfd: mt6360: add pmic mt6360 driver
From: Lee Jones @ 2020-06-02 10:32 UTC (permalink / raw)
To: Matthias Brugger
Cc: gene_chen, linux-kernel, cy_huang, linux-mediatek, Gene Chen,
Wilma.Wu, linux-arm-kernel, shufan_lee
In-Reply-To: <2231bffe-27d1-6aee-4699-77d2f754beef@gmail.com>
On Tue, 02 Jun 2020, Matthias Brugger wrote:
>
>
> On 02/06/2020 10:28, Lee Jones wrote:
> > On Tue, 02 Jun 2020, Gene Chen wrote:
> >
> >> From: Gene Chen <gene_chen@richtek.com>
> >>
> >> Add MFD driver for mt6360 pmic chip include Battery Charger/
> >> USB_PD/Flash, LED/RGB and LED/LDO/Buck
> >>
> >> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> >> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >
> > I did not sign this off.
> >
>
> You are right, you provided your Acked-for-MFD-by and took an earlier version of
> the patch [1]. But as this didn't show up in linux-next I suppose you dropped it
> afterwards because of kbuild test errors (deducing from the changes log).
If the builders can see it, -next can pull from it.
It was never dropped.
> I suppose if this errors are fixed now, we should be fine :)
Indeed. No more build errors. :)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for platform devices
From: Shameerali Kolothum Thodi @ 2020-06-02 10:31 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: devicetree@vger.kernel.org, kevin.tian@intel.com,
fenghua.yu@intel.com, linux-pci@vger.kernel.org,
felix.kuehling@amd.com, robin.murphy@arm.com,
christian.koenig@amd.com, hch@infradead.org, jgg@ziepe.ca,
iommu@lists.linux-foundation.org, catalin.marinas@arm.com,
zhangfei.gao@linaro.org, will@kernel.org, linux-mm@kvack.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20200602093836.GA1029680@myrica>
Hi Jean,
> -----Original Message-----
> From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@lists.infradead.org]
> On Behalf Of Jean-Philippe Brucker
> Sent: 02 June 2020 10:39
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: devicetree@vger.kernel.org; kevin.tian@intel.com; will@kernel.org;
> fenghua.yu@intel.com; jgg@ziepe.ca; linux-pci@vger.kernel.org;
> felix.kuehling@amd.com; hch@infradead.org; linux-mm@kvack.org;
> iommu@lists.linux-foundation.org; catalin.marinas@arm.com;
> zhangfei.gao@linaro.org; robin.murphy@arm.com;
> christian.koenig@amd.com; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for
> platform devices
>
> Hi Shameer,
>
> On Mon, Jun 01, 2020 at 12:42:15PM +0000, Shameerali Kolothum Thodi
> wrote:
> > > /* IRQ and event handlers */
> > > +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64
> > > +*evt) {
> > > + int ret;
> > > + u32 perm = 0;
> > > + struct arm_smmu_master *master;
> > > + bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > > + u8 type = FIELD_GET(EVTQ_0_ID, evt[0]);
> > > + u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> > > + struct iommu_fault_event fault_evt = { };
> > > + struct iommu_fault *flt = &fault_evt.fault;
> > > +
> > > + /* Stage-2 is always pinned at the moment */
> > > + if (evt[1] & EVTQ_1_S2)
> > > + return -EFAULT;
> > > +
> > > + master = arm_smmu_find_master(smmu, sid);
> > > + if (!master)
> > > + return -EINVAL;
> > > +
> > > + if (evt[1] & EVTQ_1_READ)
> > > + perm |= IOMMU_FAULT_PERM_READ;
> > > + else
> > > + perm |= IOMMU_FAULT_PERM_WRITE;
> > > +
> > > + if (evt[1] & EVTQ_1_EXEC)
> > > + perm |= IOMMU_FAULT_PERM_EXEC;
> > > +
> > > + if (evt[1] & EVTQ_1_PRIV)
> > > + perm |= IOMMU_FAULT_PERM_PRIV;
> > > +
> > > + if (evt[1] & EVTQ_1_STALL) {
> > > + flt->type = IOMMU_FAULT_PAGE_REQ;
> > > + flt->prm = (struct iommu_fault_page_request) {
> > > + .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > > + .pasid = FIELD_GET(EVTQ_0_SSID, evt[0]),
> > > + .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > > + .perm = perm,
> > > + .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > > + };
> > > +
> >
> > > + if (ssid_valid)
> > > + flt->prm.flags |=
> IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> >
> > Do we need to set this for STALL mode only support? I had an issue
> > with this being set on a vSVA POC based on our D06 zip device(which is
> > a "fake " pci dev that supports STALL mode but no PRI). The issue is,
> > CMDQ_OP_RESUME doesn't have any ssid or SSV params and works on sid
> and stag only.
>
> I don't understand the problem, arm_smmu_page_response() doesn't set SSID
> or SSV when sending a CMDQ_OP_RESUME. Could you detail the flow of a stall
> event and RESUME command in your prototype? Are you getting issues with
> the host driver or the guest driver?
The issue is on the host side iommu_page_response(). The flow is something like
below.
Stall: Host:-
arm_smmu_handle_evt()
iommu_report_device_fault()
vfio_pci_iommu_dev_fault_handler()
Stall: Qemu:-
vfio_dma_fault_notifier_handler()
inject_faults()
smmuv3_inject_faults()
Stall: Guest:-
arm_smmu_handle_evt()
iommu_report_device_fault()
iommu_queue_iopf
...
iopf_handle_group()
iopf_handle_single()
handle_mm_fault()
iopf_complete()
iommu_page_response()
arm_smmu_page_response()
arm_smmu_cmdq_issue_cmd(CMDQ_OP_RESUME)
Resume: Qemu:-
smmuv3_cmdq_consume(SMMU_CMD_RESUME)
smmuv3_notify_page_resp()
vfio:ioctl(page_response) --> struct iommu_page_response is filled
with only version, grpid and code.
Resume: Host:-
ioctl(page_response)
iommu_page_response() --> fails as the pending req has PASID_VALID flag
set and it checks for a match.
arm_smmu_page_response()
Hope the above is clear.
> We do need to forward the SSV flag all the way to the guest driver, so the guest
> can find the faulting address space using the SSID. Once the guest handled the
> fault, then we don't send the SSID back to the host as part of the RESUME
> command.
True, the guest requires SSV flag to handle the page fault. But, as shown in the
flow above, the issue is on the host side iommu_page_response() where it
searches for a matching pending req based on pasid. Not sure we can bypass
that and call arm_smmu_page_response() directly but then have to delete the
pending req from the list as well.
Please let me know if there is a better way to handle the host side page
response.
Thanks,
Shameer
> Thanks,
> Jean
>
> > Hence, it is difficult for
> > Qemu SMMUv3 to populate this fields while preparing a page response. I
> > can see that this flag is being checked in iopf_handle_single() (patch
> > 04/24) as well. For POC, I used a temp fix[1] to work around this. Please let
> me know your thoughts.
> >
> > Thanks,
> > Shameer
> >
> > 1.
> > https://github.com/hisilicon/kernel-dev/commit/99ff96146e924055f38d97a
> > 5897e4becfa378d15
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 3/5] coresight: tmc: Update sink types for default selection.
From: Suzuki K Poulose @ 2020-06-02 10:31 UTC (permalink / raw)
To: mike.leach, linux-arm-kernel, coresight, mathieu.poirier; +Cc: acme
In-Reply-To: <20200526104642.9526-4-mike.leach@linaro.org>
On 05/26/2020 11:46 AM, Mike Leach wrote:
> An additional sink subtype is added to differentiate ETB/ETF buffer
> sinks and ETR type system memory sinks.
>
> This allows the prioritised selection of default sinks.
>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 39fba1d16e6e..0d2eb7e0e1bb 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -484,7 +484,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
> break;
> case TMC_CONFIG_TYPE_ETR:
> desc.type = CORESIGHT_DEV_TYPE_SINK;
> - desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> + desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
> desc.ops = &tmc_etr_cs_ops;
> ret = tmc_etr_setup_caps(dev, devid,
> coresight_get_uci_data(id));
> @@ -496,6 +496,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
> break;
> case TMC_CONFIG_TYPE_ETF:
> desc.type = CORESIGHT_DEV_TYPE_LINKSINK;
> + desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
> desc.ops = &tmc_etf_cs_ops;
> dev_list = &etf_devs;
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 2/5] coresight: Add default sink selection to CoreSight base
From: Suzuki K Poulose @ 2020-06-02 10:25 UTC (permalink / raw)
To: mike.leach, linux-arm-kernel, coresight, mathieu.poirier; +Cc: acme
In-Reply-To: <20200526104642.9526-3-mike.leach@linaro.org>
On 05/26/2020 11:46 AM, Mike Leach wrote:
> Adds a method to select a suitable sink connected to a given source.
>
> In cases where no sink is defined, the coresight_find_default_sink
> routine can search from a given source, through the child connections
> until a suitable sink is found.
>
> The suitability is defined in by the sink coresight_dev_subtype on the
> CoreSight device, and the distance from the source by counting
> connections.
>
> Higher value subtype is preferred - where these are equal, shorter
> distance from source is used as a tie-break.
>
> This allows for default sink to be discovered were none is specified
> (e.g. perf command line)
>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
Feel free to add:
Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> +/**
> + * coresight_find_default_sink: Find a sink suitable for use as a
> + * default sink.
> + *
> + * @csdev: starting source to find a connected sink.
> + *
> + * Walks connections graph looking for a suitable sink to enable for the
> + * supplied source. Uses CoreSight device subtypes and distance from source
> + * to select the best sink.
> + *
> + * If a sink is found, then the default sink for this device is set and
> + * will be automatically used in future.
> + *
> + * Used in cases where the CoreSight user (perf / sysfs) has not selected a
> + * sink.
> + */
> +struct coresight_device *
> +coresight_find_default_sink(struct coresight_device *csdev)
> +{
> + int depth = 0;
> +
> + /* look for a default sink if we have not found for this device */
> + if (!csdev->def_sink)
> + csdev->def_sink = coresight_find_sink(csdev, &depth);
Could we add a helper to clear the cached "def-sink" from all the
"sources", when the sink is going away ? You may be able to do this via
coresight_remove_match()
Rest looks good to me.
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] pinctrl: freescale: imx: Fix an error handling path in 'imx_pinctrl_probe()'
From: Dan Carpenter @ 2020-06-02 10:13 UTC (permalink / raw)
To: Christophe JAILLET
Cc: aisheng.dong, festevam, linus.walleij, kernel-janitors,
linux-kernel, stefan, gary.bisson, linux-gpio, linux-imx, kernel,
shawnguo, s.hauer, linux-arm-kernel
In-Reply-To: <20200530204955.588962-1-christophe.jaillet@wanadoo.fr>
On Sat, May 30, 2020 at 10:49:55PM +0200, Christophe JAILLET wrote:
> 'pinctrl_unregister()' should not be called to undo
> 'devm_pinctrl_register_and_init()', it is already handled by the framework.
>
> This simplifies the error handling paths of the probe function.
> The 'imx_free_resources()' can be removed as well.
>
> Fixes: a51c158bf0f7 ("pinctrl: imx: use radix trees for groups and functions")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
You didn't introduce this but the:
ipctl->input_sel_base = of_iomap(np, 0);
should be changed to devm_of_iomap().
regards,
dan carpenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 4/4] USB: pci-quirks: Add Raspberry Pi 4 quirk
From: Nicolas Saenz Julienne @ 2020-06-02 10:05 UTC (permalink / raw)
To: Mathias Nyman, Rob Herring
Cc: tim.gover, linux-pci, linux-usb, linux-kernel@vger.kernel.org,
Florian Fainelli, bcm-kernel-feedback-list, linux-rpi-kernel,
gregkh, linux-arm-kernel
In-Reply-To: <20200505161318.26200-5-nsaenzjulienne@suse.de>
[-- Attachment #1.1: Type: text/plain, Size: 3923 bytes --]
On Tue, 2020-05-05 at 18:13 +0200, Nicolas Saenz Julienne wrote:
> On the Raspberry Pi 4, after a PCI reset, VL805's firmware may either be
> loaded directly from an EEPROM or, if not present, by the SoC's
> VideoCore. Inform VideoCore that VL805 was just reset.
>
> Also, as this creates a dependency between USB_PCI and VideoCore's
> firmware interface, and since USB_PCI can't be set as a module neither
> this can. Reflect that on the firmware interface Kconfg.
>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
It was pointed out to me on the u-boot mailing lists that all this could be
implemented trough a reset controller. In other words have xhci get the reset
controller trough device-tree, assert it, ultamately causing the firmware
routine to be run.
As much as it pains me to go over stuff that's already 'fixed', it seems to me
it's a better solution. On one hand we get over the device-tree dependency mess
(see patch #3), and on the other we transform a pci-quirk into something less
hacky.
That said, before getting my hands dirty, I was wondering if there is any
obvious reasons why I shouldn't do this, note that:
- We're talking here of a PCIe XCHI device, maybe there's an issue integrating
it with DT, given the fact that, as of today, it's not really represented
there.
- There is no reset controller support in xhci-pci, maybe there are good
reasons why. For instance, it's not something that's reflected in any way in
the spec.
Regards,
Nicolas
>
> Changes since v5:
> - Fix Kconfig issue with allmodconfig
>
> Changes since v4:
> - Do not split up error message
>
> Changes since v3:
> - Add more complete error message
>
> Changes since v1:
> - Make RASPBERRYPI_FIRMWARE dependent on this quirk to make sure it
> gets compiled when needed.
>
> drivers/firmware/Kconfig | 3 ++-
> drivers/usb/host/pci-quirks.c | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 8007d4aa76dc..b42140cff8ac 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -178,8 +178,9 @@ config ISCSI_IBFT
> Otherwise, say N.
>
> config RASPBERRYPI_FIRMWARE
> - tristate "Raspberry Pi Firmware Driver"
> + bool "Raspberry Pi Firmware Driver"
> depends on BCM2835_MBOX
> + default USB_PCI
> help
> This option enables support for communicating with the firmware on the
> Raspberry Pi.
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 92150ecdb036..0b949acfa258 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -16,6 +16,9 @@
> #include <linux/export.h>
> #include <linux/acpi.h>
> #include <linux/dmi.h>
> +
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> #include "pci-quirks.h"
> #include "xhci-ext-caps.h"
>
> @@ -1243,11 +1246,24 @@ static void quirk_usb_handoff_xhci(struct pci_dev
> *pdev)
>
> static void quirk_usb_early_handoff(struct pci_dev *pdev)
> {
> + int ret;
> +
> /* Skip Netlogic mips SoC's internal PCI USB controller.
> * This device does not need/support EHCI/OHCI handoff
> */
> if (pdev->vendor == 0x184e) /* vendor Netlogic */
> return;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_VIA && pdev->device == 0x3483) {
> + ret = rpi_firmware_init_vl805(pdev);
> + if (ret) {
> + /* Firmware might be outdated, or something failed */
> + dev_warn(&pdev->dev,
> + "Failed to load VL805's firmware: %d. Will
> continue to attempt to work, but bad things might happen. You should fix
> this...\n",
> + ret);
> + }
> + }
> +
> if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
> pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
> pdev->class != PCI_CLASS_SERIAL_USB_EHCI &&
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Sakari Ailus @ 2020-06-02 9:56 UTC (permalink / raw)
To: Dongchun Zhu
Cc: Mark Rutland, Rob Herring, Andy Shevchenko, srv_heupstream,
linux-devicetree, Linus Walleij,
Shengnan Wang (王圣男), Tomasz Figa,
Bartosz Golaszewski, Sj Huang, Nicolas Boichat,
moderated list:ARM/Mediatek SoC support, Louis Kuo,
Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Linux Media Mailing List
In-Reply-To: <1591078501.8804.539.camel@mhfsdcap03>
Hi Dongchun,
On Tue, Jun 02, 2020 at 02:15:01PM +0800, Dongchun Zhu wrote:
> Hi Tomasz, Sakari,
>
> On Mon, 2020-06-01 at 20:18 +0200, Tomasz Figa wrote:
> > On Mon, Jun 1, 2020 at 4:35 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Fri, 2020-05-29 at 15:43 +0200, Tomasz Figa wrote:
> > > > On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> > > > >
> > > > > Hi Sakari,
> > > > >
> > > > > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > > > > Hi Dongchun,
> > > > > >
> > > > > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > > > > Hi Sakari, Rob,
> > > > > > >
> > > > > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > > > > Hi Rob, Dongchun,
> > > > > > > >
> > > > > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > > > > + properties:
> > > > > > > > > > > > + endpoint:
> > > > > > > > > > > > + type: object
> > > > > > > > > > > > + additionalProperties: false
> > > > > > > > > > > > +
> > > > > > > > > > > > + properties:
> > > > > > > > > >
> > > > > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > > > > >
> > > > > > > > > Yes, if you are using it.
> > > > > > > >
> > > > > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > > > > lane and that it does not support lane reordering?
> > > > > > > >
> > > > > > >
> > > > > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > > > > communication links between components inside a mobile device.
> > > > > > > The data lane has full support for HS(uni-directional) and
> > > > > > > LP(bi-directional) data transfer mode.'
> > > > > > >
> > > > > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > > > > would not be added in next release.
> > > > > > >
> > > > > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > > > > be removed IMO.
> > > > > > > >
> > > > > > >
> > > > > > > However, 'data-lanes' property may still be required.
> > > > > > > It is known that either data-lanes or clock-lanes is an array of
> > > > > > > physical data lane indexes. Position of an entry determines the logical
> > > > > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > > > > the clock lane is on hardware lane 0.
> > > > > > >
> > > > > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > > > > there is only a clock lane for OV02A10.
> > > > > > >
> > > > > > > Reminder:
> > > > > > > If 'data-lanes' property is not present, the driver would assume
> > > > > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > > > > property must be present and set to the right physical lane indexes.
> > > > > > > If the hardware does not support lane reordering, monotonically
> > > > > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > > > > whether or not there is also a clock lane.
> > > > > >
> > > > > > How can the driver use four lanes, considering the device only supports a
> > > > > > single lane??
> > > > > >
> > > > >
> > > > > I understood your meaning.
> > > > > If we omit the property 'data-lanes', the sensor should work still.
> > > > > But then what's the meaning of the existence of 'data-lanes'?
> > > > > If this property 'data-lanes' is always optional, then why dt-bindings
> > > > > provide the interface?
> > > > >
> > > > > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > > > > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > > > > shall enable four-lane configuration, which may increase consumption of
> > > > > both power and resource in the process of IIC communication.
> > > >
> > > > Wouldn't the receiver still have the data-lanes property under its
> > > > endpoint node, telling it how many lanes and in which order should be
> > > > used?
> > > >
> > >
> > > The MIPI receiver(RX) shall use
> > > v4l2_async_notifier_add_fwnode_remote_subdev() API to parse the property
> > > "data-lanes" under sensor output port.
> >
> > That's not true. The MIPI receiver driver parses its own port node
> > corresponding to the sensor. Also quoting the documentation [1]:
> >
> > "An endpoint subnode of a device contains all properties needed for
> > _configuration of this device_ for data exchange with other device. In most
> > cases properties at the peer 'endpoint' nodes will be identical, however they
> > might need to be different when there is any signal modifications on the bus
> > between two devices, e.g. there are logic signal inverters on the lines."
> >
> > In this case, there is such a signal modification if the sensor has a
> > 1-lane bus and the receiver more lines, so the data-lanes properties
> > would be different on both sides.
> >
> > [1] https://elixir.bootlin.com/linux/v5.7/source/Documentation/devicetree/bindings/media/video-interfaces.txt
> >
>
> Sorry for the misunderstanding.
> After doing some experiments about the data-lanes property under sensor
> i2c node, we found the API
> v4l2_async_notifier_add_fwnode_remote_subdev() that MIPI receiver driver
> used indeed parses the data-lanes under its own port node.
>
> Sorry make a mistake for the use case of sensor data-lanes previously.
> Now We may encounter one new question for this patch.
> In practice we haven't used the data-lanes under sensor i2c node
> anywhere, if sensor driver itself doesn't parse that.
>
> But there is still one reason to keep the exactly right data-lanes in
> DT. That is, the data-lanes under sensor i2c node could be used as a
> reference for MIPI receiver driver.
> Just as Tomasz said, 'The MIPI receiver driver parses its own port node
> corresponding to the sensor'.
>
> Sakari, Tomasz, what's your opinions about the present of data-lanes
> under sensor node or not?
The receiver driver doesn't parse the properties in the sensor
(transmitter) device's endpoint. If that property provides no information
to the receiver, as is the case here, it should be omitted.
--
Regards,
Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 00/10] spi: Adding support for Microchip Sparx5 SoC
From: Mark Brown @ 2020-06-02 9:56 UTC (permalink / raw)
To: Lars Povlsen
Cc: devicetree, linux-kernel, Serge Semin, linux-spi, Serge Semin,
SoC Team, Microchip Linux Driver Support, linux-arm-kernel
In-Reply-To: <87img9q3sb.fsf@soft-dev15.microsemi.net>
[-- Attachment #1.1: Type: text/plain, Size: 365 bytes --]
On Tue, Jun 02, 2020 at 10:18:28AM +0200, Lars Povlsen wrote:
> I think I would be able to work on the SPI patches this week. Should I
> base it on the current spi-next or 5.7? Then address the comments and
> send out a new revision?
At this point any new patches should be sent against -next, the merge
window is open and -next has already been merged by Linus.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V9, 1/2] media: dt-bindings: media: i2c: Document OV02A10 bindings
From: Sakari Ailus @ 2020-06-02 9:53 UTC (permalink / raw)
To: Tomasz Figa
Cc: Mark Rutland, Rob Herring, Andy Shevchenko, srv_heupstream,
linux-devicetree, Linus Walleij,
Shengnan Wang (王圣男), Bartosz Golaszewski,
Sj Huang, Nicolas Boichat,
moderated list:ARM/Mediatek SoC support, Dongchun Zhu, Louis Kuo,
Matthias Brugger, Cao Bing Bu, Mauro Carvalho Chehab,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Linux Media Mailing List
In-Reply-To: <CAAFQd5AuHDpQN8xZsWgnAt6m2reAYJbs9nBp0+mBo7_FS81LbQ@mail.gmail.com>
On Fri, May 29, 2020 at 03:43:30PM +0200, Tomasz Figa wrote:
> On Thu, May 28, 2020 at 10:06 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
> >
> > Hi Sakari,
> >
> > On Thu, 2020-05-28 at 10:23 +0300, Sakari Ailus wrote:
> > > Hi Dongchun,
> > >
> > > On Thu, May 28, 2020 at 11:34:42AM +0800, Dongchun Zhu wrote:
> > > > Hi Sakari, Rob,
> > > >
> > > > On Thu, 2020-05-28 at 00:16 +0300, Sakari Ailus wrote:
> > > > > Hi Rob, Dongchun,
> > > > >
> > > > > On Wed, May 27, 2020 at 09:27:22AM -0600, Rob Herring wrote:
> > > > > > > > > + properties:
> > > > > > > > > + endpoint:
> > > > > > > > > + type: object
> > > > > > > > > + additionalProperties: false
> > > > > > > > > +
> > > > > > > > > + properties:
> > > > > > >
> > > > > > > Actually I wonder whether we need to declare 'clock-lanes' here?
> > > > > >
> > > > > > Yes, if you are using it.
> > > > >
> > > > > Dongchun, can you confirm the chip has a single data and a single clock
> > > > > lane and that it does not support lane reordering?
> > > > >
> > > >
> > > > From the datasheet, 'MIPI inside the OV02A10 provides one single
> > > > uni-directional clock lane and one bi-directional data lane solution for
> > > > communication links between components inside a mobile device.
> > > > The data lane has full support for HS(uni-directional) and
> > > > LP(bi-directional) data transfer mode.'
> > > >
> > > > The sensor doesn't support lane reordering, so 'clock-lanes' property
> > > > would not be added in next release.
> > > >
> > > > > So if there's nothing to convey to the driver, also the data-lanes should
> > > > > be removed IMO.
> > > > >
> > > >
> > > > However, 'data-lanes' property may still be required.
> > > > It is known that either data-lanes or clock-lanes is an array of
> > > > physical data lane indexes. Position of an entry determines the logical
> > > > lane number, while the value of an entry indicates physical lane, e.g.,
> > > > for 1-lane MIPI CSI-2 bus we could have "data-lanes = <1>;", assuming
> > > > the clock lane is on hardware lane 0.
> > > >
> > > > As mentioned earlier, the OV02A10 sensor supports only 1C1D and does not
> > > > support lane reordering, so here we shall use 'data-lanes = <1>' as
> > > > there is only a clock lane for OV02A10.
> > > >
> > > > Reminder:
> > > > If 'data-lanes' property is not present, the driver would assume
> > > > four-lane operation. This means for one-lane or two-lane operation, this
> > > > property must be present and set to the right physical lane indexes.
> > > > If the hardware does not support lane reordering, monotonically
> > > > incremented values shall be used from 0 or 1 onwards, depending on
> > > > whether or not there is also a clock lane.
> > >
> > > How can the driver use four lanes, considering the device only supports a
> > > single lane??
> > >
> >
> > I understood your meaning.
> > If we omit the property 'data-lanes', the sensor should work still.
> > But then what's the meaning of the existence of 'data-lanes'?
> > If this property 'data-lanes' is always optional, then why dt-bindings
> > provide the interface?
> >
> > In the meantime, if omitting 'data-lanes' for one sensor(transmitter)
> > that has only one physical data lane, MIPI receiver(e.g., MIPI CSI-2)
> > shall enable four-lane configuration, which may increase consumption of
> > both power and resource in the process of IIC communication.
>
> Wouldn't the receiver still have the data-lanes property under its
> endpoint node, telling it how many lanes and in which order should be
> used?
Yes.
--
Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/5] dt-bindings: reset: ocelot: Add documentation for 'microchip, reset-switch-core' property
From: Lars Povlsen @ 2020-06-02 9:49 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Alexandre Belloni, linux-pm, linux-kernel,
Sebastian Reichel, Microchip Linux Driver Support, SoC Team,
linux-arm-kernel, Lars Povlsen
In-Reply-To: <20200528022502.GA3234572@bogus>
Rob Herring writes:
> On Wed, May 13, 2020 at 03:08:40PM +0200, Lars Povlsen wrote:
>> This documents the 'microchip,reset-switch-core' property in the
>> ocelot-reset driver.
>>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>> ---
>> .../devicetree/bindings/power/reset/ocelot-reset.txt | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt b/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
>> index 4d530d8154848..20fff03753ad2 100644
>> --- a/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
>> +++ b/Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
>> @@ -9,9 +9,15 @@ microchip Sparx5 armv8 SoC's.
>> Required Properties:
>> - compatible: "mscc,ocelot-chip-reset" or "microchip,sparx5-chip-reset"
>>
>> +Optional properties:
>> +- microchip,reset-switch-core : Perform a switch core reset at the
>> + time of driver load. This is may be used to initialize the switch
>> + core to a known state (before other drivers are loaded).
>
> How do you know when other drivers are loaded? This could be a module
> perhaps. Doesn't seem like something that belongs in DT.
>
The reset driver is loaded at postcore_initcall() time, which ensures it
is loaded before other drivers using the switch core. I noticed other
drivers do the same to do low-level system reset and initialization at
early boot time.
> Can this behavior be implied with "microchip,sparx5-chip-reset"?
Since we need to cater for both modus operandi, I would need two driver
compatible strings per platform, which scales worse than a single
property.
The "microchip,reset-switch-core" is a device configuration property
which tells the system (driver) how the hw should be handled. Since you
do not *always* want to reset the switch core (f.ex. when implementing
systems with warm reboot), I think it makes perfect sense - but I may be
biased off course :-)
Thank you for (all) of your comments, by the way!
---Lars
>
> Rob
--
Lars Povlsen,
Microchip
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v10] mfd: mt6360: add pmic mt6360 driver
From: Matthias Brugger @ 2020-06-02 9:43 UTC (permalink / raw)
To: Lee Jones, Gene Chen
Cc: gene_chen, linux-kernel, cy_huang, linux-mediatek, Wilma.Wu,
linux-arm-kernel, shufan_lee
In-Reply-To: <20200602082816.GC3714@dell>
On 02/06/2020 10:28, Lee Jones wrote:
> On Tue, 02 Jun 2020, Gene Chen wrote:
>
>> From: Gene Chen <gene_chen@richtek.com>
>>
>> Add MFD driver for mt6360 pmic chip include Battery Charger/
>> USB_PD/Flash, LED/RGB and LED/LDO/Buck
>>
>> Signed-off-by: Gene Chen <gene_chen@richtek.com>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> I did not sign this off.
>
You are right, you provided your Acked-for-MFD-by and took an earlier version of
the patch [1]. But as this didn't show up in linux-next I suppose you dropped it
afterwards because of kbuild test errors (deducing from the changes log).
I suppose if this errors are fixed now, we should be fine :)
Regards,
Matthias
[1]
https://lkml.kernel.org/lkml/1587641093-25441-1-git-send-email-gene.chen.richtek@gmail.com/T/#m75e8ee81950ee34e155eccd2dc5ad9b1d2cef40a
>> ---
>> drivers/mfd/Kconfig | 12 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/mt6360-core.c | 424 +++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/mt6360.h | 240 +++++++++++++++++++++++++
>> 4 files changed, 677 insertions(+)
>> create mode 100644 drivers/mfd/mt6360-core.c
>> create mode 100644 include/linux/mfd/mt6360.h
>>
>> changelogs between v1 & v2
>> - include missing header file
>>
>> changelogs between v2 & v3
>> - add changelogs
>>
>> changelogs between v3 & v4
>> - fix Kconfig description
>> - replace mt6360_pmu_info with mt6360_pmu_data
>> - replace probe with probe_new
>> - remove unnecessary irq_chip variable
>> - remove annotation
>> - replace MT6360_MFD_CELL with OF_MFD_CELL
>>
>> changelogs between v4 & v5
>> - remove unnecessary parse dt function
>> - use devm_i2c_new_dummy_device
>> - add base-commit message
>>
>> changelogs between v5 & v6
>> - review return value
>> - remove i2c id_table
>> - use GPL license v2
>>
>> changelogs between v6 & v7
>> - add author description
>> - replace MT6360_REGMAP_IRQ_REG by REGMAP_IRQ_REG_LINE
>> - remove mt6360-private.h
>>
>> changelogs between v7 & v8
>> - fix kbuild auto reboot by include interrupt header
>>
>> changelogs between v8 & v9
>> - fix GPL license out of date
>> - add commit message about Acked-for-MFD-by
>>
>> changelogs between v9 & v10
>
> v9 of this patch is already applied.
>
> You cannot send a v10.
>
>> - fix duplicate define of kbuild test reboot initializer-overrides
>
> I fixed this already.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next&id=098c4adf249c198519a4abebe482b1e6b8c50e47
>
>> - sync commit message format
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v7 21/24] iommu/arm-smmu-v3: Add stall support for platform devices
From: Jean-Philippe Brucker @ 2020-06-02 9:38 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: devicetree@vger.kernel.org, kevin.tian@intel.com, will@kernel.org,
fenghua.yu@intel.com, jgg@ziepe.ca, linux-pci@vger.kernel.org,
felix.kuehling@amd.com, hch@infradead.org, linux-mm@kvack.org,
iommu@lists.linux-foundation.org, catalin.marinas@arm.com,
zhangfei.gao@linaro.org, robin.murphy@arm.com,
christian.koenig@amd.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <4741b6c45d1a43b69041ecb5ce0be0d5@huawei.com>
Hi Shameer,
On Mon, Jun 01, 2020 at 12:42:15PM +0000, Shameerali Kolothum Thodi wrote:
> > /* IRQ and event handlers */
> > +static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> > +{
> > + int ret;
> > + u32 perm = 0;
> > + struct arm_smmu_master *master;
> > + bool ssid_valid = evt[0] & EVTQ_0_SSV;
> > + u8 type = FIELD_GET(EVTQ_0_ID, evt[0]);
> > + u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> > + struct iommu_fault_event fault_evt = { };
> > + struct iommu_fault *flt = &fault_evt.fault;
> > +
> > + /* Stage-2 is always pinned at the moment */
> > + if (evt[1] & EVTQ_1_S2)
> > + return -EFAULT;
> > +
> > + master = arm_smmu_find_master(smmu, sid);
> > + if (!master)
> > + return -EINVAL;
> > +
> > + if (evt[1] & EVTQ_1_READ)
> > + perm |= IOMMU_FAULT_PERM_READ;
> > + else
> > + perm |= IOMMU_FAULT_PERM_WRITE;
> > +
> > + if (evt[1] & EVTQ_1_EXEC)
> > + perm |= IOMMU_FAULT_PERM_EXEC;
> > +
> > + if (evt[1] & EVTQ_1_PRIV)
> > + perm |= IOMMU_FAULT_PERM_PRIV;
> > +
> > + if (evt[1] & EVTQ_1_STALL) {
> > + flt->type = IOMMU_FAULT_PAGE_REQ;
> > + flt->prm = (struct iommu_fault_page_request) {
> > + .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > + .pasid = FIELD_GET(EVTQ_0_SSID, evt[0]),
> > + .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > + .perm = perm,
> > + .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > + };
> > +
>
> > + if (ssid_valid)
> > + flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
>
> Do we need to set this for STALL mode only support? I had an issue with this
> being set on a vSVA POC based on our D06 zip device(which is a "fake " pci dev
> that supports STALL mode but no PRI). The issue is, CMDQ_OP_RESUME doesn't
> have any ssid or SSV params and works on sid and stag only.
I don't understand the problem, arm_smmu_page_response() doesn't set SSID
or SSV when sending a CMDQ_OP_RESUME. Could you detail the flow of a stall
event and RESUME command in your prototype? Are you getting issues with
the host driver or the guest driver?
We do need to forward the SSV flag all the way to the guest driver, so the
guest can find the faulting address space using the SSID. Once the guest
handled the fault, then we don't send the SSID back to the host as part of
the RESUME command.
Thanks,
Jean
> Hence, it is difficult for
> Qemu SMMUv3 to populate this fields while preparing a page response. I can see
> that this flag is being checked in iopf_handle_single() (patch 04/24) as well. For POC,
> I used a temp fix[1] to work around this. Please let me know your thoughts.
>
> Thanks,
> Shameer
>
> 1. https://github.com/hisilicon/kernel-dev/commit/99ff96146e924055f38d97a5897e4becfa378d15
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] media: stm32-dcmi: Set minimum cpufreq requirement
From: Valentin Schneider @ 2020-06-02 9:31 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: alexandre.torgue, rjw, linux-kernel, mcoquelin.stm32,
hugues.fruchet, mchehab, linux-stm32, linux-arm-kernel,
linux-media
In-Reply-To: <20200527151613.16083-1-benjamin.gaignard@st.com>
Hi Benjamin,
On 27/05/20 16:16, Benjamin Gaignard wrote:
> Before start streaming set cpufreq minimum frequency requirement.
> The cpufreq governor will adapt the frequencies and we will have
> no latency for handling interrupts.
>
Few comments below from someone oblivious to your platform, they may not
be all that relevant but I figured I'd pitch in anyway.
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
> drivers/media/platform/stm32/stm32-dcmi.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index b8931490b83b..97c342351569 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -13,6 +13,7 @@
>
> #include <linux/clk.h>
> #include <linux/completion.h>
> +#include <linux/cpufreq.h>
> #include <linux/delay.h>
> #include <linux/dmaengine.h>
> #include <linux/init.h>
> @@ -99,6 +100,8 @@ enum state {
>
> #define OVERRUN_ERROR_THRESHOLD 3
>
> +#define DCMI_MIN_FREQ 650000 /* in KHz */
> +
This assumes the handling part is guaranteed to always run on the same CPU
with the same performance profile (regardless of the platform). If that's
not guaranteed, it feels like you'd want this to be configurable in some
way.
> struct dcmi_graph_entity {
> struct v4l2_async_subdev asd;
>
[...]
> @@ -2020,6 +2042,8 @@ static int dcmi_probe(struct platform_device *pdev)
> goto err_cleanup;
> }
>
> + dcmi->policy = cpufreq_cpu_get(0);
> +
Ideally you'd want to fetch the policy of the CPU your IRQ (and handling
thread) is affined to; The only compatible DTS I found describes a single
A7, which is somewhat limited in the affinity area...
> dev_info(&pdev->dev, "Probe done\n");
>
> platform_set_drvdata(pdev, dcmi);
> @@ -2049,6 +2073,9 @@ static int dcmi_remove(struct platform_device *pdev)
>
> pm_runtime_disable(&pdev->dev);
>
> + if (dcmi->policy)
> + cpufreq_cpu_put(dcmi->policy);
> +
> v4l2_async_notifier_unregister(&dcmi->notifier);
> v4l2_async_notifier_cleanup(&dcmi->notifier);
> media_entity_cleanup(&dcmi->vdev->entity);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox