* Re: [PATCH-tip v2 02/12] locking/rwsem: Implement lock handoff to prevent lock starvation
From: Waiman Long @ 2019-04-10 15:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Thomas Gleixner, linux-kernel, x86,
Davidlohr Bueso, Linus Torvalds, Tim Chen
In-Reply-To: <20190410150714.GK7905@worktop.programming.kicks-ass.net>
On 04/10/2019 11:07 AM, Peter Zijlstra wrote:
> On Fri, Apr 05, 2019 at 03:21:05PM -0400, Waiman Long wrote:
>> Because of writer lock stealing, it is possible that a constant
>> stream of incoming writers will cause a waiting writer or reader to
>> wait indefinitely leading to lock starvation.
>>
>> The mutex code has a lock handoff mechanism to prevent lock starvation.
>> This patch implements a similar lock handoff mechanism to disable
>> lock stealing and force lock handoff to the first waiter in the queue
>> after at least a 5ms waiting period. The waiting period is used to
>> avoid discouraging lock stealing too much to affect performance.
> So the mutex code doesn't have that timeout, it foces the handoff if the
> top waiter fails to acquire.
>
> I don't find the above sufficiently justifies the additional complexity.
> What were the numbers with the simple scheme vs this etc..
When the handoff bit is set, it stops the lock from being acquired by
anyone else until the front waiter is woken up, scheduled and take the
lock. Doing that too frequently will impede the throughput when the
rwsem is highly contended. I can ran some performance test to show the
difference it can make.
I have also been thinking about having the front waiter set the handoff
bit and then spin on owner so that it can acquire the lock immediately
after it is freed. It will be a follow up patch.
Cheers,
Longman
^ permalink raw reply
* [PATCH] t3000 (ls-files -o): widen description to reflect current tests
From: Kyle Meyer @ 2019-04-10 15:28 UTC (permalink / raw)
To: git; +Cc: Kyle Meyer
Remove the mention of symlinks from the test description because
several tests that are not related to symlinks have been added since
this file was introduced long ago.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
t/t3000-ls-files-others.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index afd4756134..0aefadacb0 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -3,7 +3,7 @@
# Copyright (c) 2005 Junio C Hamano
#
-test_description='git ls-files test (--others should pick up symlinks).
+test_description='basic tests for ls-files --others
This test runs git ls-files --others with the following on the
filesystem.
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] PM / core: Propagate dev->power.wakeup_path when no callbacks
From: Alexandre Torgue @ 2019-04-10 15:28 UTC (permalink / raw)
To: Ulf Hansson, Rafael J . Wysocki, linux-pm
Cc: Geert Uytterhoeven, Loic Pallardy, Linus Walleij, Rob Herring,
Greg Kroah-Hartman, Johan Hovold, linux-gpio, linux-serial,
linux-kernel
In-Reply-To: <20190410095516.6170-1-ulf.hansson@linaro.org>
Hi Ulf
On 4/10/19 11:55 AM, Ulf Hansson wrote:
> The dev->power.direct_complete flag may become set in device_prepare() in
> case the device don't have any PM callbacks (dev->power.no_pm_callbacks is
> set). This leads to a broken behaviour, when there is child having wakeup
> enabled and relies on its parent to be used in the wakeup path.
>
> More precisely, when the direct complete path becomes selected for the
> child in __device_suspend(), the propagation of the dev->power.wakeup_path
> becomes skipped as well.
>
> Let's address this problem, by checking if the device is a part the wakeup
> path or has wakeup enabled, then prevent the direct complete path from
> being used.
>
> Reported-by: Loic Pallardy <loic.pallardy@st.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
Thanks Ulf for this patch. It'll avoid to have dirty hack in serial
suspend callback (at least for stm32). I just tested it on stm32 kernel
v4.19 (which embeds all our genpd based power features). Replacing
device_may_wakeup(dev) by
device_may_wakeup(dev) || dev->power.wakeup_path) then dirty hack to
test ttydev wakeup flag in stm32 usart driver is no more needed.
So you can add my Tested-by
Regards
Alex
> More background:
>
> This problem was reported by Loic Pallardy, offlist, while he was working
> on enabling wakeup for a tty serial console driver.
>
> When I looked more closely, I noticed that uart_suspend_port() calls
> device_may_wakeup() for the tty child devices, and then also the used serial
> driver check its device (parent) for device_may_wakeup(). To me this looks like
> workarounds to fix a behaviour that really should be dealt with from the PM
> core, no matter of whether the child have PM callbacks assigned or not.
>
> In other words, it seems like the serial driver(s) should be checking the
> wakeup_path flag for the parent, solely, instead.
>
> I haven't digested further behaviours for other subsystem, but recently
> reviewed a patch for a gpio driver [1], that seems to be suffering from the
> similar problems.
>
> Kind regards
> Ulf Hansson
>
> [1]
> https://lkml.org/lkml/2019/4/4/1283
>
> ---
> drivers/base/power/main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 41eba82ee7b9..f9cfdeee8288 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1747,6 +1747,10 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
> if (dev->power.syscore)
> goto Complete;
>
> + /* Avoid direct_complete, to let wakeup_path being propagated. */
> + if (device_may_wakeup(dev) || dev->power.wakeup_path)
> + dev->power.direct_complete = false;
> +
> if (dev->power.direct_complete) {
> if (pm_runtime_status_suspended(dev)) {
> pm_runtime_disable(dev);
>
^ permalink raw reply
* Re: [PATCH-tip v2 02/12] locking/rwsem: Implement lock handoff to prevent lock starvation
From: Waiman Long @ 2019-04-10 15:29 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Will Deacon, Thomas Gleixner, linux-kernel, x86,
Davidlohr Bueso, Linus Torvalds, Tim Chen
In-Reply-To: <20190410151018.GL7905@worktop.programming.kicks-ass.net>
On 04/10/2019 11:10 AM, Peter Zijlstra wrote:
> On Fri, Apr 05, 2019 at 03:21:05PM -0400, Waiman Long wrote:
>> +#define RWSEM_WAIT_TIMEOUT ((HZ - 1)/200 + 1)
> Given the choices in HZ, the above seems fairly 'optimistic'.
I can tighten it up and make it less "optimistic" :-)
Cheers,
Longman
^ permalink raw reply
* [PATCH V4 0/2] blk-mq/nvme: cancel request synchronously
From: Keith Busch @ 2019-04-10 15:30 UTC (permalink / raw)
In-Reply-To: <20190410150436.GA23621@ming.t460p>
On Wed, Apr 10, 2019@11:04:38PM +0800, Ming Lei wrote:
> On Tue, Apr 09, 2019@06:31:20AM +0800, Ming Lei wrote:
> > Hi,
> >
> > This patchset introduces blk_mq_complete_request_sync() for canceling
> > request synchronously in error handler context, then one race between
> > completing request remotely and destroying contoller/queues can be fixed.
> >
> >
> > V4:
> > - drop return value
> > - don't handle fake timeout
> >
> > V3:
> > - avoid extra cost to blk_mq_complete_request
> >
> > V2:
> > - export via EXPORT_SYMBOL_GPL
> > - minor commit log change
> >
> > Ming Lei (2):
> > blk-mq: introduce blk_mq_complete_request_sync()
> > nvme: cancel request synchronously
> >
> > block/blk-mq.c | 7 +++++++
> > drivers/nvme/host/core.c | 2 +-
> > include/linux/blk-mq.h | 1 +
> > 3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > Cc: Keith Busch <kbusch at kernel.org>
> > Cc: Sagi Grimberg <sagi at grimberg.me>
> > Cc: Bart Van Assche <bvanassche at acm.org>
> > Cc: James Smart <james.smart at broadcom.com>
> > Cc: Christoph Hellwig <hch at lst.de>
> > Cc: linux-nvme at lists.infradead.org
> >
>
> Hi Jens,
>
> These two patches have been posted for a while, any chance to make them
> in v5.1?
FWIW, series looks good to me too.
Reviewed-by: Keith Busch <keith.busch at intel.com>
^ permalink raw reply
* Applied "spi: spi-mem: Make spi_mem_default_supports_op() static inline" to the spi tree
From: Mark Brown @ 2019-04-10 15:30 UTC (permalink / raw)
To: YueHaibing
Cc: broonie, linux-kernel, linux-spi, Mark Brown,
naga.sureshkumar.relli, vigneshr
In-Reply-To: <20190410131828.35316-1-yuehaibing@huawei.com>
The patch
spi: spi-mem: Make spi_mem_default_supports_op() static inline
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From d4a91044e241d8f87fb990b673549a7d2f9cacc4 Mon Sep 17 00:00:00 2001
From: YueHaibing <yuehaibing@huawei.com>
Date: Wed, 10 Apr 2019 21:18:28 +0800
Subject: [PATCH] spi: spi-mem: Make spi_mem_default_supports_op() static
inline
Stub helper spi_mem_default_supports_op() should
be set to static inline
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
include/linux/spi/spi-mem.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 1941b845aa15..af9ff2f0f1b2 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -315,6 +315,7 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
{
}
+static inline
bool spi_mem_default_supports_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
--
2.20.1
^ permalink raw reply related
* Applied "spi: spi-mem: Make spi_mem_default_supports_op() static inline" to the spi tree
From: Mark Brown @ 2019-04-10 15:30 UTC (permalink / raw)
To: YueHaibing
Cc: broonie, linux-kernel, linux-spi, Mark Brown,
naga.sureshkumar.relli, vigneshr
In-Reply-To: <20190410131828.35316-1-yuehaibing@huawei.com>
The patch
spi: spi-mem: Make spi_mem_default_supports_op() static inline
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From d4a91044e241d8f87fb990b673549a7d2f9cacc4 Mon Sep 17 00:00:00 2001
From: YueHaibing <yuehaibing@huawei.com>
Date: Wed, 10 Apr 2019 21:18:28 +0800
Subject: [PATCH] spi: spi-mem: Make spi_mem_default_supports_op() static
inline
Stub helper spi_mem_default_supports_op() should
be set to static inline
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
include/linux/spi/spi-mem.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 1941b845aa15..af9ff2f0f1b2 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -315,6 +315,7 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
{
}
+static inline
bool spi_mem_default_supports_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
--
2.20.1
^ permalink raw reply related
* Re: [Qemu-devel] [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
From: Paul Durrant @ 2019-04-10 15:20 UTC (permalink / raw)
To: Anthony Perard
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org,
xen-devel@lists.xenproject.org, Stefano Stabellini,
Stefan Hajnoczi, Kevin Wolf, Max Reitz
In-Reply-To: <20190410125708.GE1435@perard.uk.xensource.com>
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 10 April 2019 13:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
>
> On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > and then uses aio_set_fd_handler() to set the callback rather than
> > qemu_set_fd_handler().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > }
> >
> > XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > + AioContext *ctx,
> > unsigned int port,
> > XenEventHandler handler,
> > void *opaque, Error **errp)
> > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > channel->handler = handler;
> > channel->opaque = opaque;
> >
> > - qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > - channel);
> > + channel->ctx = ctx;
> > + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > + xen_device_event, NULL, NULL, channel);
>
> I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> `true' here, instead. That flag seems to be used when making a snapshot
> of a blockdev, for example.
>
> That was introduced by:
> dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
>
> What do you think?
Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?
Cheers,
Paul
>
>
> --
> Anthony PERARD
^ permalink raw reply
* [PATCH] [NVMe-CLI] Fix Failure to read 0xCA Log Page on SN200 Device [NVMe-CLI] Fix Incorrect Data Formats with the 0xCA and 0xD0 Log Pages
From: Keith Busch @ 2019-04-10 15:31 UTC (permalink / raw)
In-Reply-To: <1554909267-30735-1-git-send-email-jeff.lien@wdc.com>
Applied, thanks.
^ permalink raw reply
* Re: [RFC V4 2/2] ath10k: add tx hw 802.11 encapusaltion offloading support
From: Kalle Valo @ 2019-04-10 15:31 UTC (permalink / raw)
To: John Crispin
Cc: Johannes Berg, linux-wireless, Srini Kode, Rajkumar Manoharan,
Shashidhar Lakkavalli, Vasanthakumar Thiagarajan
In-Reply-To: <20190410073514.12794-3-john@phrozen.org>
John Crispin <john@phrozen.org> writes:
> From: Vasanthakumar Thiagarajan <vthiagar@qti.qualcomm.com>
>
> This patch adds support for ethernet rxtx mode to the driver. The feature
> is enabled via a new module parameter. If enabled to driver will enable
> the feature on a per vif basis if all other requirements were met.
>
> Testing on a IPQ4019 based hardware shows a increase in TCP throughput
> of ~20% when the feature is enabled.
>
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qti.qualcomm.com>
> Signed-off-by: John Crispin <john@phrozen.org>
Looks good to me.
BTW, for ath10k patches (patchsets including ath10k patches) please try
to CC also the ath10k list. Easier to find ath10k patches that way.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
From: Paul Durrant @ 2019-04-10 15:20 UTC (permalink / raw)
To: Anthony Perard
Cc: Kevin Wolf, Stefano Stabellini, qemu-block@nongnu.org,
qemu-devel@nongnu.org, Max Reitz, Stefan Hajnoczi,
xen-devel@lists.xenproject.org
In-Reply-To: <20190410125708.GE1435@perard.uk.xensource.com>
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 10 April 2019 13:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
>
> On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > and then uses aio_set_fd_handler() to set the callback rather than
> > qemu_set_fd_handler().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > }
> >
> > XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > + AioContext *ctx,
> > unsigned int port,
> > XenEventHandler handler,
> > void *opaque, Error **errp)
> > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > channel->handler = handler;
> > channel->opaque = opaque;
> >
> > - qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > - channel);
> > + channel->ctx = ctx;
> > + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > + xen_device_event, NULL, NULL, channel);
>
> I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> `true' here, instead. That flag seems to be used when making a snapshot
> of a blockdev, for example.
>
> That was introduced by:
> dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
>
> What do you think?
Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?
Cheers,
Paul
>
>
> --
> Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [RFC PATCH 04/12] s390/cio: introduce cio DMA pool
From: Halil Pasic @ 2019-04-10 15:31 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, linux-s390, Martin Schwidefsky, Sebastian Ott,
virtualization, Christian Borntraeger, Viktor Mihajlovski,
Vasily Gorbik, Janosch Frank, Claudio Imbrenda, Farhan Ali,
Eric Farman
In-Reply-To: <20190409191453.01444317.cohuck@redhat.com>
On Tue, 9 Apr 2019 19:14:53 +0200
Cornelia Huck <cohuck@redhat.com> wrote:
[..]
> > > At this point, you're always going via the css0 device. I'm
> > > wondering whether you should pass in the cssid here and use
> > > css_by_id(cssid) to make this future proof. But then, I'm not
> > > quite clear from which context this will be called.
> >
> > As said before I don't see MCSS-E coming. Regarding the client code,
> > please check out the rest of the series. Especially patch 6.
> >
> > From my perspective it would be at this stage saner to make it more
> > obvious that only one css is supported (at the moment), than to
> > implement MCSS-E, or to make this (kind of) MCSS-E ready.
>
> I disagree. I think there's value in keeping the interfaces clean
> (within reasonable bounds, of course.) Even if there is no
> implementation of MCSS-E other than QEMU... it is probably a good idea
> to spend some brain cycles to make this conceptually clean.
AFAIU Linux currently does not support MCSS-E. I don't have the
bandwidth to implement MCSS-E support in the kernel.
I fully agree for external interfaces should be MCSS-E conform, so
should we ever decide to implement we don't have to overcome self-made
obstacles.
Kernel internal stuff however, IMHO, should avoid carrying a ballast of
an 20%-30% implemented MCSS-E support. I see no benefit.
But I don't insist. If you have a good idea how to make this more MCSS-E
conform, you are welcome. In think something like this is best done on
top of a series that provides a basic working solution. Especially if the
conceptually clean thing is conceptually or code-wise more complex than
the basic solution. If you have something in mind that is simpler and
more lightweight than what I did here, I would be more than happy to make
that happen.
Regards,
Halil
^ permalink raw reply
* Re: [PATCH] drm/amdgpu: support dpm level modification under virtualization v3
From: Alex Deucher @ 2019-04-10 15:31 UTC (permalink / raw)
To: Yintian Tao; +Cc: amd-gfx list
In-Reply-To: <1554906332-10229-1-git-send-email-yttao-5C7GfCeVMHo@public.gmane.org>
On Wed, Apr 10, 2019 at 10:25 AM Yintian Tao <yttao@amd.com> wrote:
>
> Under vega10 virtualuzation, smu ip block will not be added.
> Therefore, we need add pp clk query and force dpm level function
> at amdgpu_virt_ops to support the feature.
>
> v2: add get_pp_clk existence check and use kzalloc to allocate buf
>
> v3: return -ENOMEM for allocation failure and correct the coding style
>
> Change-Id: I713419c57b854082f6f739f1d32a055c7115e620
> Signed-off-by: Yintian Tao <yttao@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 15 ++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 49 +++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h | 11 +++++
> drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 78 ++++++++++++++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/mxgpu_ai.h | 6 +++
> 7 files changed, 164 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 3ff8899..bb0fd5a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2486,6 +2486,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> mutex_init(&adev->virt.vf_errors.lock);
> hash_init(adev->mn_hash);
> mutex_init(&adev->lock_reset);
> + mutex_init(&adev->virt.dpm_mutex);
>
> amdgpu_device_check_arguments(adev);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 6190495..29ec28f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -727,6 +727,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
> if (adev->pm.dpm_enabled) {
> dev_info.max_engine_clock = amdgpu_dpm_get_sclk(adev, false) * 10;
> dev_info.max_memory_clock = amdgpu_dpm_get_mclk(adev, false) * 10;
> + } else if (amdgpu_sriov_vf(adev) && amdgim_is_hwperf(adev) &&
> + adev->virt.ops->get_pp_clk) {
> + dev_info.max_engine_clock = amdgpu_virt_get_sclk(adev, false) * 10;
> + dev_info.max_memory_clock = amdgpu_virt_get_mclk(adev, false) * 10;
> } else {
> dev_info.max_engine_clock = adev->clock.default_sclk * 10;
> dev_info.max_memory_clock = adev->clock.default_mclk * 10;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 5540259..0162d1e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -380,6 +380,17 @@ static ssize_t amdgpu_set_dpm_forced_performance_level(struct device *dev,
> goto fail;
> }
>
> + if (amdgpu_sriov_vf(adev)) {
> + if (amdgim_is_hwperf(adev) &&
> + adev->virt.ops->force_dpm_level) {
> + mutex_lock(&adev->pm.mutex);
> + adev->virt.ops->force_dpm_level(adev, level);
> + mutex_unlock(&adev->pm.mutex);
> + return count;
> + } else
> + return -EINVAL;
Coding style. If any clause as parens, all should. E.g., this should be :
} else {
return -EINVAL;
}
With that fixed, this patch is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> + }
> +
> if (current_level == level)
> return count;
>
> @@ -843,6 +854,10 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = ddev->dev_private;
>
> + if (amdgpu_sriov_vf(adev) && amdgim_is_hwperf(adev) &&
> + adev->virt.ops->get_pp_clk)
> + return adev->virt.ops->get_pp_clk(adev, PP_SCLK, buf);
> +
> if (is_support_sw_smu(adev))
> return smu_print_clk_levels(&adev->smu, PP_SCLK, buf);
> else if (adev->powerplay.pp_funcs->print_clock_levels)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 462a04e..7e7f9ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -375,4 +375,53 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
> }
> }
>
> +static uint32_t parse_clk(char *buf, bool min)
> +{
> + char *ptr = buf;
> + uint32_t clk = 0;
> +
> + do {
> + ptr = strchr(ptr, ':');
> + if (!ptr)
> + break;
> + ptr+=2;
> + clk = simple_strtoul(ptr, NULL, 10);
> + } while (!min);
> +
> + return clk * 100;
> +}
> +
> +uint32_t amdgpu_virt_get_sclk(struct amdgpu_device *adev, bool lowest)
> +{
> + char *buf = NULL;
> + uint32_t clk = 0;
> +
> + buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + adev->virt.ops->get_pp_clk(adev, PP_SCLK, buf);
> + clk = parse_clk(buf, lowest);
> +
> + kfree(buf);
> +
> + return clk;
> +}
> +
> +uint32_t amdgpu_virt_get_mclk(struct amdgpu_device *adev, bool lowest)
> +{
> + char *buf = NULL;
> + uint32_t clk = 0;
> +
> + buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + adev->virt.ops->get_pp_clk(adev, PP_MCLK, buf);
> + clk = parse_clk(buf, lowest);
> +
> + kfree(buf);
> +
> + return clk;
> +}
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> index 722deef..584947b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> @@ -57,6 +57,8 @@ struct amdgpu_virt_ops {
> int (*reset_gpu)(struct amdgpu_device *adev);
> int (*wait_reset)(struct amdgpu_device *adev);
> void (*trans_msg)(struct amdgpu_device *adev, u32 req, u32 data1, u32 data2, u32 data3);
> + int (*get_pp_clk)(struct amdgpu_device *adev, u32 type, char *buf);
> + int (*force_dpm_level)(struct amdgpu_device *adev, u32 level);
> };
>
> /*
> @@ -83,6 +85,8 @@ enum AMDGIM_FEATURE_FLAG {
> AMDGIM_FEATURE_GIM_LOAD_UCODES = 0x2,
> /* VRAM LOST by GIM */
> AMDGIM_FEATURE_GIM_FLR_VRAMLOST = 0x4,
> + /* HW PERF SIM in GIM */
> + AMDGIM_FEATURE_HW_PERF_SIMULATION = (1 << 3),
> };
>
> struct amd_sriov_msg_pf2vf_info_header {
> @@ -252,6 +256,8 @@ struct amdgpu_virt {
> struct amdgpu_vf_error_buffer vf_errors;
> struct amdgpu_virt_fw_reserve fw_reserve;
> uint32_t gim_feature;
> + /* protect DPM events to GIM */
> + struct mutex dpm_mutex;
> };
>
> #define amdgpu_sriov_enabled(adev) \
> @@ -278,6 +284,9 @@ static inline bool is_virtual_machine(void)
> #endif
> }
>
> +#define amdgim_is_hwperf(adev) \
> + ((adev)->virt.gim_feature & AMDGIM_FEATURE_HW_PERF_SIMULATION)
> +
> bool amdgpu_virt_mmio_blocked(struct amdgpu_device *adev);
> void amdgpu_virt_init_setting(struct amdgpu_device *adev);
> uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg);
> @@ -295,5 +304,7 @@ int amdgpu_virt_fw_reserve_get_checksum(void *obj, unsigned long obj_size,
> unsigned int key,
> unsigned int chksum);
> void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev);
> +uint32_t amdgpu_virt_get_sclk(struct amdgpu_device *adev, bool lowest);
> +uint32_t amdgpu_virt_get_mclk(struct amdgpu_device *adev, bool lowest);
>
> #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> index 73851eb..8dbad49 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c
> @@ -157,6 +157,82 @@ static void xgpu_ai_mailbox_trans_msg (struct amdgpu_device *adev,
> xgpu_ai_mailbox_set_valid(adev, false);
> }
>
> +static int xgpu_ai_get_pp_clk(struct amdgpu_device *adev, u32 type, char *buf)
> +{
> + int r = 0;
> + u32 req, val, size;
> +
> + if (!amdgim_is_hwperf(adev) || buf == NULL)
> + return -EBADRQC;
> +
> + switch(type) {
> + case PP_SCLK:
> + req = IDH_IRQ_GET_PP_SCLK;
> + break;
> + case PP_MCLK:
> + req = IDH_IRQ_GET_PP_MCLK;
> + break;
> + default:
> + return -EBADRQC;
> + }
> +
> + mutex_lock(&adev->virt.dpm_mutex);
> +
> + xgpu_ai_mailbox_trans_msg(adev, req, 0, 0, 0);
> +
> + r = xgpu_ai_poll_msg(adev, IDH_SUCCESS);
> + if (!r && adev->fw_vram_usage.va != NULL) {
> + val = RREG32_NO_KIQ(
> + SOC15_REG_OFFSET(NBIO, 0,
> + mmBIF_BX_PF0_MAILBOX_MSGBUF_RCV_DW1));
> + size = strnlen((((char *)adev->virt.fw_reserve.p_pf2vf) +
> + val), PAGE_SIZE);
> +
> + if (size < PAGE_SIZE)
> + strcpy(buf,((char *)adev->virt.fw_reserve.p_pf2vf + val));
> + else
> + size = 0;
> +
> + r = size;
> + goto out;
> + }
> +
> + r = xgpu_ai_poll_msg(adev, IDH_FAIL);
> + if(r)
> + pr_info("%s DPM request failed",
> + (type == PP_SCLK)? "SCLK" : "MCLK");
> +
> +out:
> + mutex_unlock(&adev->virt.dpm_mutex);
> + return r;
> +}
> +
> +static int xgpu_ai_force_dpm_level(struct amdgpu_device *adev, u32 level)
> +{
> + int r = 0;
> + u32 req = IDH_IRQ_FORCE_DPM_LEVEL;
> +
> + if (!amdgim_is_hwperf(adev))
> + return -EBADRQC;
> +
> + mutex_lock(&adev->virt.dpm_mutex);
> + xgpu_ai_mailbox_trans_msg(adev, req, level, 0, 0);
> +
> + r = xgpu_ai_poll_msg(adev, IDH_SUCCESS);
> + if (!r)
> + goto out;
> +
> + r = xgpu_ai_poll_msg(adev, IDH_FAIL);
> + if (!r)
> + pr_info("DPM request failed");
> + else
> + pr_info("Mailbox is broken");
> +
> +out:
> + mutex_unlock(&adev->virt.dpm_mutex);
> + return r;
> +}
> +
> static int xgpu_ai_send_access_requests(struct amdgpu_device *adev,
> enum idh_request req)
> {
> @@ -375,4 +451,6 @@ const struct amdgpu_virt_ops xgpu_ai_virt_ops = {
> .reset_gpu = xgpu_ai_request_reset,
> .wait_reset = NULL,
> .trans_msg = xgpu_ai_mailbox_trans_msg,
> + .get_pp_clk = xgpu_ai_get_pp_clk,
> + .force_dpm_level = xgpu_ai_force_dpm_level,
> };
> diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.h b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.h
> index b4a9cee..39d151b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.h
> +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.h
> @@ -35,6 +35,10 @@ enum idh_request {
> IDH_REL_GPU_FINI_ACCESS,
> IDH_REQ_GPU_RESET_ACCESS,
>
> + IDH_IRQ_FORCE_DPM_LEVEL = 10,
> + IDH_IRQ_GET_PP_SCLK,
> + IDH_IRQ_GET_PP_MCLK,
> +
> IDH_LOG_VF_ERROR = 200,
> };
>
> @@ -43,6 +47,8 @@ enum idh_event {
> IDH_READY_TO_ACCESS_GPU,
> IDH_FLR_NOTIFICATION,
> IDH_FLR_NOTIFICATION_CMPL,
> + IDH_SUCCESS,
> + IDH_FAIL,
> IDH_EVENT_MAX
> };
>
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH 2/2] firmware: Move Trusted Foundations support
From: Thierry Reding @ 2019-04-10 15:31 UTC (permalink / raw)
To: Dmitry Osipenko; +Cc: linux-tegra, Russell King, linux-arm-kernel, Jon Hunter
In-Reply-To: <b0b88e3b-405d-6199-71ac-07e15a29eb9a@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 6052 bytes --]
On Wed, Apr 10, 2019 at 01:36:35PM +0300, Dmitry Osipenko wrote:
> 10.04.2019 11:57, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Move the Trusted Foundations support out of arch/arm/firmware and into
> > drivers/firmware where most other firmware support implementations are
> > located.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > arch/arm/Kconfig | 2 --
> > arch/arm/Makefile | 1 -
> > arch/arm/firmware/Kconfig | 29 -------------------
> > arch/arm/firmware/Makefile | 4 ---
> > arch/arm/mach-tegra/Kconfig | 2 +-
> > arch/arm/mach-tegra/cpuidle-tegra114.c | 3 +-
> > arch/arm/mach-tegra/pm.c | 3 +-
> > arch/arm/mach-tegra/reset.c | 3 +-
> > arch/arm/mach-tegra/tegra.c | 3 +-
> > drivers/firmware/Kconfig | 15 ++++++++++
> > drivers/firmware/Makefile | 1 +
> > .../firmware/trusted_foundations.c | 4 ++-
> > .../linux/firmware}/trusted_foundations.h | 4 +--
> > 13 files changed, 30 insertions(+), 44 deletions(-)
> > delete mode 100644 arch/arm/firmware/Kconfig
> > delete mode 100644 arch/arm/firmware/Makefile
> > rename {arch/arm => drivers}/firmware/trusted_foundations.c (98%)
> > rename {arch/arm/include/asm => include/linux/firmware}/trusted_foundations.h (97%)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 054ead960f98..f006b3c69247 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -899,8 +899,6 @@ config PLAT_PXA
> > config PLAT_VERSATILE
> > bool
> >
> > -source "arch/arm/firmware/Kconfig"
> > -
> > source "arch/arm/mm/Kconfig"
> >
> > config IWMMXT
> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> > index 807a7d06c2a0..05ecc004de86 100644
> > --- a/arch/arm/Makefile
> > +++ b/arch/arm/Makefile
> > @@ -290,7 +290,6 @@ core-y += arch/arm/kernel/ arch/arm/mm/ arch/arm/common/
> > core-y += arch/arm/probes/
> > core-y += arch/arm/net/
> > core-y += arch/arm/crypto/
> > -core-y += arch/arm/firmware/
> > core-y += $(machdirs) $(platdirs)
> >
> > drivers-$(CONFIG_OPROFILE) += arch/arm/oprofile/
> > diff --git a/arch/arm/firmware/Kconfig b/arch/arm/firmware/Kconfig
> > deleted file mode 100644
> > index ad396af68e47..000000000000
> > --- a/arch/arm/firmware/Kconfig
> > +++ /dev/null
> > @@ -1,29 +0,0 @@
> > -config ARCH_SUPPORTS_FIRMWARE
> > - bool
> > -
> > -config ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
> > - bool
> > - select ARCH_SUPPORTS_FIRMWARE
> > -
> > -menu "Firmware options"
> > - depends on ARCH_SUPPORTS_FIRMWARE
> > -
> > -config TRUSTED_FOUNDATIONS
> > - bool "Trusted Foundations secure monitor support"
> > - depends on ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
> > - default y
> > - help
> > - Some devices (including most Tegra-based consumer devices on the
> > - market) are booted with the Trusted Foundations secure monitor
> > - active, requiring some core operations to be performed by the secure
> > - monitor instead of the kernel.
> > -
> > - This option allows the kernel to invoke the secure monitor whenever
> > - required on devices using Trusted Foundations. See
> > - arch/arm/include/asm/trusted_foundations.h or the
> > - tlm,trusted-foundations device tree binding documentation for details
> > - on how to use it.
> > -
> > - Say n if you don't know what this is about.
> > -
> > -endmenu
> > diff --git a/arch/arm/firmware/Makefile b/arch/arm/firmware/Makefile
> > deleted file mode 100644
> > index 6e41336b0bc4..000000000000
> > --- a/arch/arm/firmware/Makefile
> > +++ /dev/null
> > @@ -1,4 +0,0 @@
> > -obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
> > -
> > -# tf_generic_smc() fails to build with -fsanitize-coverage=trace-pc
> > -KCOV_INSTRUMENT := n
> > diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> > index 7b3fd0995a16..cbad7823f602 100644
> > --- a/arch/arm/mach-tegra/Kconfig
> > +++ b/arch/arm/mach-tegra/Kconfig
> > @@ -3,7 +3,6 @@ menuconfig ARCH_TEGRA
> > bool "NVIDIA Tegra"
> > depends on ARCH_MULTI_V7
> > select ARCH_HAS_RESET_CONTROLLER
> > - select ARCH_SUPPORTS_TRUSTED_FOUNDATIONS
> > select ARM_AMBA
> > select ARM_GIC
> > select CLKSRC_MMIO
> > @@ -14,6 +13,7 @@ menuconfig ARCH_TEGRA
> > select PM_OPP
> > select RESET_CONTROLLER
> > select SOC_BUS
> > + select TRUSTED_FOUNDATIONS
>
> Do we really want to force-enable the driver? I think it's better to
> have a choice and then instead enable the driver in the
> tegra_defconfig.
The driver is getting enabled by default prior to this patch, and given
that it's fairly small, I thought it'd be okay to include it by default.
But I guess there's no harm in enabling it in tegra_defconfig and
multi_v7_defconfig, that way people do have the possibility to disable
it if they really don't want it.
> > select ZONE_DMA if ARM_LPAE
> > help
> > This enables support for NVIDIA Tegra based systems.
>
>
> [snip]
>
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -267,6 +267,21 @@ config TI_SCI_PROTOCOL
> > This protocol library is used by client drivers to use the features
> > provided by the system controller.
> >
> > +config TRUSTED_FOUNDATIONS
> > + bool "Trusted Foundations secure monitor support"
>
> Th driver shall depend at least on ARM because of the assembly. And
> maybe even on ARCH_TEGRA.
Indeed, I've added a "depends on ARM" in v2.
> Do you know if there are any platforms other than Tegra that use that
> firmware? Won't it make sense to move the driver to tegra/ ?
I don't think there's any reason to make this Tegra specific, even if
this was only ever deployed on Tegra.
Thierry
[-- 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
* Re: [Qemu-devel] [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
From: Paul Durrant @ 2019-04-10 15:20 UTC (permalink / raw)
To: Anthony Perard
Cc: Kevin Wolf, Stefano Stabellini, qemu-block@nongnu.org,
qemu-devel@nongnu.org, Max Reitz, Stefan Hajnoczi,
xen-devel@lists.xenproject.org
In-Reply-To: <20190410125708.GE1435@perard.uk.xensource.com>
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 10 April 2019 13:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
>
> On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > and then uses aio_set_fd_handler() to set the callback rather than
> > qemu_set_fd_handler().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > }
> >
> > XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > + AioContext *ctx,
> > unsigned int port,
> > XenEventHandler handler,
> > void *opaque, Error **errp)
> > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > channel->handler = handler;
> > channel->opaque = opaque;
> >
> > - qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > - channel);
> > + channel->ctx = ctx;
> > + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > + xen_device_event, NULL, NULL, channel);
>
> I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> `true' here, instead. That flag seems to be used when making a snapshot
> of a blockdev, for example.
>
> That was introduced by:
> dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
>
> What do you think?
Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?
Cheers,
Paul
>
>
> --
> Anthony PERARD
^ permalink raw reply
* Re: [PATCH v3] platform: chrome: Add ChromeOS EC ISHTP driver
From: Jett Rink @ 2019-04-10 15:31 UTC (permalink / raw)
To: Rushikesh S Kadam
Cc: Srinivas Pandruvada, Benson Leung, Enric Balletbo i Serra,
Guenter Roeck, Nick Crews, Gwendal Grignou, linux-kernel
In-Reply-To: <1554639014-28363-1-git-send-email-rushikesh.s.kadam@intel.com>
Reviewed-by: Jett Rink <jettrink@chromium.org>
Tested-by: Jett Rink <jettrink@chromium.org>
On Sun, Apr 7, 2019 at 6:10 AM Rushikesh S Kadam
<rushikesh.s.kadam@intel.com> wrote:
>
> This driver implements a slim layer to enable the ChromeOS
> EC kernel stack (cros_ec) to communicate with ChromeOS EC
> firmware running on the Intel Integrated Sensor Hub (ISH).
>
> The driver registers a ChromeOS EC MFD device to connect
> with cros_ec kernel stack (upper layer), and it registers a
> client with the ISH Transport Protocol bus (lower layer) to
> talk with the ISH firwmare. See description of the ISHTP
> protocol at Documentation/hid/intel-ish-hid.txt
>
> Signed-off-by: Rushikesh S Kadam <rushikesh.s.kadam@intel.com>
> ---
>
> v3
> - Made several changes to improve code readability. Replaced
> multiple cl_data_to_dev(client_data) with dev variable. Use
> reverse Xmas tree for variable defintion where it made sense.
> Dropped few debug prints. Add docstring for function
> prepare_cros_ec_rx().
> - Fix code in function prepare_cros_ec_rx() under label
> end_cros_ec_dev_init_error.
> - Recycle buffer in process_recv() on failing to obtain the
> semaphore.
> - Increase ISHTP TX/RX ring buffer size to 8.
> - Alphabetically ordered CROS_EC_ISHTP entries in Makefile and
> Kconfig.
> - Updated commit message.
>
> v2
> - Dropped unused "reset" parameter in function cros_ec_init()
> - Change driver name to cros_ec_ishtp to be consistent with other
> references in the code.
> - Fixed a few typos.
>
> v1
> - Initial version
>
> drivers/platform/chrome/Kconfig | 13 +
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/cros_ec_ishtp.c | 765 ++++++++++++++++++++++++++++++++
> 3 files changed, 779 insertions(+)
> create mode 100644 drivers/platform/chrome/cros_ec_ishtp.c
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 16b1615..5848179 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -62,6 +62,19 @@ config CROS_EC_I2C
> a checksum. Failing accesses will be retried three times to
> improve reliability.
>
> +config CROS_EC_ISHTP
> + tristate "ChromeOS Embedded Controller (ISHTP)"
> + depends on MFD_CROS_EC
> + depends on INTEL_ISH_HID
> + help
> + If you say Y here, you get support for talking to the ChromeOS EC
> + firmware running on Intel Integrated Sensor Hub (ISH), using the
> + ISH Transport protocol (ISH-TP). This uses a simple byte-level
> + protocol with a checksum.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called cros_ec_ishtp.
> +
> config CROS_EC_SPI
> tristate "ChromeOS Embedded Controller (SPI)"
> depends on MFD_CROS_EC && SPI
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index cd591bf..4efe102 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -7,6 +7,7 @@ cros_ec_ctl-objs := cros_ec_sysfs.o cros_ec_lightbar.o \
> cros_ec_vbc.o cros_ec_debugfs.o
> obj-$(CONFIG_CROS_EC_CTL) += cros_ec_ctl.o
> obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o
> +obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_reg.o
> cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o
> diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
> new file mode 100644
> index 0000000..b1d19c4
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_ishtp.c
> @@ -0,0 +1,765 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ISHTP client driver for talking to the Chrome OS EC firmware running
> + * on Intel Integrated Sensor Hub (ISH) using the ISH Transport protocol
> + * (ISH-TP).
> + *
> + * Copyright (c) 2019, Intel Corporation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/intel-ish-client-if.h>
> +
> +/*
> + * ISH TX/RX ring buffer pool size
> + *
> + * The AP->ISH messages and corresponding ISH->AP responses are
> + * serialized. We need 1 TX and 1 RX buffer for these.
> + *
> + * The MKBP ISH->AP events are serialized. We need one additional RX
> + * buffer for them.
> + */
> +#define CROS_ISH_CL_TX_RING_SIZE 8
> +#define CROS_ISH_CL_RX_RING_SIZE 8
> +
> +/* ISH CrOS EC Host Commands */
> +enum cros_ec_ish_channel {
> + CROS_EC_COMMAND = 1, /* AP->ISH message */
> + CROS_MKBP_EVENT = 2, /* ISH->AP events */
> +};
> +
> +/*
> + * ISH firmware timeout for 1 message send failure is 1Hz, and the
> + * firmware will retry 2 times, so 3Hz is used for timeout.
> + */
> +#define ISHTP_SEND_TIMEOUT (3 * HZ)
> +
> +/* ISH Transport CrOS EC ISH client unique GUID */
> +static const guid_t cros_ish_guid =
> + GUID_INIT(0x7b7154d0, 0x56f4, 0x4bdc,
> + 0xb0, 0xd8, 0x9e, 0x7c, 0xda, 0xe0, 0xd6, 0xa0);
> +
> +struct header {
> + u8 channel;
> + u8 status;
> + u8 reserved[2];
> +} __packed;
> +
> +struct cros_ish_out_msg {
> + struct header hdr;
> + struct ec_host_request ec_request;
> +} __packed;
> +
> +struct cros_ish_in_msg {
> + struct header hdr;
> + struct ec_host_response ec_response;
> +} __packed;
> +
> +#define IN_MSG_EC_RESPONSE_PREAMBLE \
> + offsetof(struct cros_ish_in_msg, ec_response)
> +
> +#define OUT_MSG_EC_REQUEST_PREAMBLE \
> + offsetof(struct cros_ish_out_msg, ec_request)
> +
> +#define cl_data_to_dev(client_data) ishtp_device((client_data)->cl_device)
> +
> +/*
> + * The Read-Write Semaphore is used to prevent message TX or RX while
> + * the ishtp client is being initialized or undergoing reset.
> + *
> + * The readers are the kernel function calls responsible for IA->ISH
> + * and ISH->AP messaging.
> + *
> + * The writers are .reset() and .probe() function.
> + */
> +DECLARE_RWSEM(init_lock);
> +
> +/**
> + * struct response_info - Encapsulate firmware response related
> + * information for passing between function
> + * ish_send() and process_recv() callback.
> + * @data: Copy the data received from firmware here.
> + * @max_size: Max size allocated for the @data buffer. If the
> + * received data exceeds this value, we log an
> + * error.
> + * @size: Actual size of data received from firmware.
> + * @error: 0 for success, negative error code for a
> + * failure in function process_recv().
> + * @received: Set to true on receiving a valid firmware
> + * response to host command
> + * @wait_queue: Wait queue for Host firmware loading where the
> + * client sends message to ISH firmware and waits
> + * for response
> + */
> +struct response_info {
> + void *data;
> + size_t max_size;
> + size_t size;
> + int error;
> + bool received;
> + wait_queue_head_t wait_queue;
> +};
> +
> +/**
> + * struct ishtp_cl_data - Encapsulate per ISH TP Client.
> + * @cros_ish_cl: ISHTP firmware client instance.
> + * @cl_device: ISHTP client device instance.
> + * @response: Firmware response information for passing
> + * between function ish_send() and process_recv()
> + * callback.
> + * @work_ishtp_reset: Work queue reset handling.
> + * @work_ec_evt: Work queue for EC events.
> + * @ec_dev: CrOS EC MFD device.
> + *
> + * This structure is used to store per client data.
> + */
> +struct ishtp_cl_data {
> + struct ishtp_cl *cros_ish_cl;
> + struct ishtp_cl_device *cl_device;
> +
> + /*
> + * Used for passing firmware response information between
> + * ish_send() and process_recv() callback.
> + */
> + struct response_info response;
> +
> + struct work_struct work_ishtp_reset;
> + struct work_struct work_ec_evt;
> + struct cros_ec_device *ec_dev;
> +};
> +
> +/**
> + * ish_evt_handler - ISH to AP event handler
> + * @work: Work struct
> + */
> +static void ish_evt_handler(struct work_struct *work)
> +{
> + struct ishtp_cl_data *client_data =
> + container_of(work, struct ishtp_cl_data, work_ec_evt);
> + struct cros_ec_device *ec_dev = client_data->ec_dev;
> +
> + if (cros_ec_get_next_event(ec_dev, NULL) > 0) {
> + blocking_notifier_call_chain(&ec_dev->event_notifier,
> + 0, ec_dev);
> + }
> +}
> +
> +/**
> + * ish_send() - Send message from host to firmware
> + * @client_data: Client data instance
> + * @out_msg: Message buffer to be sent to firmware
> + * @out_size: Size of out going message
> + * @in_msg: Message buffer where the incoming data copied.
> + * This buffer is allocated by calling
> + * @in_size: Max size of incoming message
> + *
> + * Return: Number of bytes copied in the in_msg on success, negative
> + * error code on failure.
> + */
> +static int ish_send(struct ishtp_cl_data *client_data,
> + u8 *out_msg, size_t out_size,
> + u8 *in_msg, size_t in_size)
> +{
> + int rv;
> + struct header *out_hdr = (struct header *)out_msg;
> + struct ishtp_cl *cros_ish_cl = client_data->cros_ish_cl;
> +
> + dev_dbg(cl_data_to_dev(client_data),
> + "%s: channel=%02u status=%02u\n",
> + __func__, out_hdr->channel, out_hdr->status);
> +
> + /* Setup for incoming response */
> + client_data->response.data = in_msg;
> + client_data->response.max_size = in_size;
> + client_data->response.error = 0;
> + client_data->response.received = false;
> +
> + rv = ishtp_cl_send(cros_ish_cl, out_msg, out_size);
> + if (rv) {
> + dev_err(cl_data_to_dev(client_data),
> + "ishtp_cl_send error %d\n", rv);
> + return rv;
> + }
> +
> + wait_event_interruptible_timeout(client_data->response.wait_queue,
> + client_data->response.received,
> + ISHTP_SEND_TIMEOUT);
> + if (!client_data->response.received) {
> + dev_err(cl_data_to_dev(client_data),
> + "Timed out for response to host message\n");
> + return -ETIMEDOUT;
> + }
> +
> + if (client_data->response.error < 0)
> + return client_data->response.error;
> +
> + return client_data->response.size;
> +}
> +
> +/**
> + * process_recv() - Received and parse incoming packet
> + * @cros_ish_cl: Client instance to get stats
> + * @rb_in_proc: Host interface message buffer
> + *
> + * Parse the incoming packet. If it is a response packet then it will
> + * update per instance flags and wake up the caller waiting to for the
> + * response. If it is an event packet then it will schedule event work.
> + */
> +static void process_recv(struct ishtp_cl *cros_ish_cl,
> + struct ishtp_cl_rb *rb_in_proc)
> +{
> + size_t data_len = rb_in_proc->buf_idx;
> + struct ishtp_cl_data *client_data =
> + ishtp_get_client_data(cros_ish_cl);
> + struct device *dev = cl_data_to_dev(client_data);
> + struct cros_ish_in_msg *in_msg =
> + (struct cros_ish_in_msg *)rb_in_proc->buffer.data;
> +
> + /* Proceed only if reset or init is not in progress */
> + if (!down_read_trylock(&init_lock)) {
> + /* Free the buffer */
> + ishtp_cl_io_rb_recycle(rb_in_proc);
> + dev_warn(dev,
> + "Host is not ready to receive incoming messages\n");
> + return;
> + }
> +
> + /*
> + * All firmware messages contain a header. Check the buffer size
> + * before accessing elements inside.
> + */
> + if (!rb_in_proc->buffer.data) {
> + dev_warn(dev, "rb_in_proc->buffer.data returned null");
> + client_data->response.error = -EBADMSG;
> + goto end_error;
> + }
> +
> + if (data_len < sizeof(struct header)) {
> + dev_err(dev, "data size %zu is less than header %zu\n",
> + data_len, sizeof(struct header));
> + client_data->response.error = -EMSGSIZE;
> + goto end_error;
> + }
> +
> + dev_dbg(dev, "channel=%02u status=%02u\n",
> + in_msg->hdr.channel, in_msg->hdr.status);
> +
> + switch (in_msg->hdr.channel) {
> + case CROS_EC_COMMAND:
> + /* Sanity check */
> + if (!client_data->response.data) {
> + dev_err(dev,
> + "Receiving buffer is null. Should be allocated by calling function\n");
> + client_data->response.error = -EINVAL;
> + goto error_wake_up;
> + }
> +
> + if (client_data->response.received) {
> + dev_err(dev,
> + "Previous firmware message not yet processed\n");
> + client_data->response.error = -EINVAL;
> + goto error_wake_up;
> + }
> +
> + if (data_len > client_data->response.max_size) {
> + dev_err(dev,
> + "Received buffer size %zu is larger than allocated buffer %zu\n",
> + data_len, client_data->response.max_size);
> + client_data->response.error = -EMSGSIZE;
> + goto error_wake_up;
> + }
> +
> + if (in_msg->hdr.status) {
> + dev_err(dev, "firmware returned status %d\n",
> + in_msg->hdr.status);
> + client_data->response.error = -EIO;
> + goto error_wake_up;
> + }
> +
> + /* Update the actual received buffer size */
> + client_data->response.size = data_len;
> +
> + /*
> + * Copy the buffer received in firmware response for the
> + * calling thread.
> + */
> + memcpy(client_data->response.data,
> + rb_in_proc->buffer.data, data_len);
> +
> + /* Set flag before waking up the caller */
> + client_data->response.received = true;
> +error_wake_up:
> + /* Wake the calling thread */
> + wake_up_interruptible(&client_data->response.wait_queue);
> +
> + break;
> +
> + case CROS_MKBP_EVENT:
> + /* The event system doesn't send any data in buffer */
> + schedule_work(&client_data->work_ec_evt);
> +
> + break;
> +
> + default:
> + dev_err(dev, "Invalid channel=%02d\n", in_msg->hdr.channel);
> + }
> +
> +end_error:
> + /* Free the buffer */
> + ishtp_cl_io_rb_recycle(rb_in_proc);
> +
> + up_read(&init_lock);
> +}
> +
> +/**
> + * ish_event_cb() - bus driver callback for incoming message
> + * @cl_device: ISHTP client device for which this message is
> + * targeted.
> + *
> + * Remove the packet from the list and process the message by calling
> + * process_recv.
> + */
> +static void ish_event_cb(struct ishtp_cl_device *cl_device)
> +{
> + struct ishtp_cl_rb *rb_in_proc;
> + struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device);
> +
> + while ((rb_in_proc = ishtp_cl_rx_get_rb(cros_ish_cl)) != NULL) {
> + /* Decide what to do with received data */
> + process_recv(cros_ish_cl, rb_in_proc);
> + }
> +}
> +
> +/**
> + * cros_ish_init() - Init function for ISHTP client
> + * @cros_ish_cl: ISHTP client instance
> + *
> + * This function complete the initializtion of the client.
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int cros_ish_init(struct ishtp_cl *cros_ish_cl)
> +{
> + int rv;
> + struct ishtp_device *dev;
> + struct ishtp_fw_client *fw_client;
> + struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
> +
> + rv = ishtp_cl_link(cros_ish_cl);
> + if (rv) {
> + dev_err(cl_data_to_dev(client_data),
> + "ishtp_cl_link failed\n");
> + return rv;
> + }
> +
> + dev = ishtp_get_ishtp_device(cros_ish_cl);
> +
> + /* Connect to firmware client */
> + ishtp_set_tx_ring_size(cros_ish_cl, CROS_ISH_CL_TX_RING_SIZE);
> + ishtp_set_rx_ring_size(cros_ish_cl, CROS_ISH_CL_RX_RING_SIZE);
> +
> + fw_client = ishtp_fw_cl_get_client(dev, &cros_ish_guid);
> + if (!fw_client) {
> + dev_err(cl_data_to_dev(client_data),
> + "ish client uuid not found\n");
> + rv = -ENOENT;
> + goto err_cl_unlink;
> + }
> +
> + ishtp_cl_set_fw_client_id(cros_ish_cl,
> + ishtp_get_fw_client_id(fw_client));
> + ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_CONNECTING);
> +
> + rv = ishtp_cl_connect(cros_ish_cl);
> + if (rv) {
> + dev_err(cl_data_to_dev(client_data),
> + "client connect fail\n");
> + goto err_cl_unlink;
> + }
> +
> + ishtp_register_event_cb(client_data->cl_device, ish_event_cb);
> + return 0;
> +
> +err_cl_unlink:
> + ishtp_cl_unlink(cros_ish_cl);
> + return rv;
> +}
> +
> +/**
> + * cros_ish_deinit() - Deinit function for ISHTP client
> + * @cros_ish_cl: ISHTP client instance
> + *
> + * Unlink and free cros_ec client
> + */
> +static void cros_ish_deinit(struct ishtp_cl *cros_ish_cl)
> +{
> + ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_DISCONNECTING);
> + ishtp_cl_disconnect(cros_ish_cl);
> + ishtp_cl_unlink(cros_ish_cl);
> + ishtp_cl_flush_queues(cros_ish_cl);
> +
> + /* Disband and free all Tx and Rx client-level rings */
> + ishtp_cl_free(cros_ish_cl);
> +}
> +
> +/**
> + * prepare_cros_ec_rx() - Check & prepare receive buffer
> + * @in_msg: Incoming message buffer
> + * @cros_ec_command cros_ec command used to send & receive data
> + *
> + * Check the received buffer. Convert to cros_ec_command format.
> + */
> +static int prepare_cros_ec_rx(struct cros_ec_device *ec_dev,
> + const struct cros_ish_in_msg *in_msg,
> + struct cros_ec_command *msg)
> +{
> + u8 sum = 0;
> + int i, rv, offset;
> +
> + /* Check response error code */
> + msg->result = in_msg->ec_response.result;
> + rv = cros_ec_check_result(ec_dev, msg);
> + if (rv < 0)
> + return rv;
> +
> + if (in_msg->ec_response.data_len > msg->insize) {
> + dev_err(ec_dev->dev, "Packet too long (%d bytes, expected %d)",
> + in_msg->ec_response.data_len, msg->insize);
> + return -ENOSPC;
> + }
> +
> + /* Copy response packet payload and compute checksum */
> + for (i = 0; i < sizeof(struct ec_host_response); i++)
> + sum += ((u8 *)in_msg)[IN_MSG_EC_RESPONSE_PREAMBLE + i];
> +
> + offset = sizeof(struct cros_ish_in_msg);
> + for (i = 0; i < in_msg->ec_response.data_len; i++)
> + sum += msg->data[i] = ((u8 *)in_msg)[offset + i];
> +
> + if (sum) {
> + dev_dbg(ec_dev->dev, "Bad received packet checksum %d\n", sum);
> + return -EBADMSG;
> + }
> +
> + return 0;
> +}
> +
> +static int cros_ec_pkt_xfer_ish(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg)
> +{
> + int rv;
> + struct ishtp_cl *cros_ish_cl = ec_dev->priv;
> + struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
> + struct device *dev = cl_data_to_dev(client_data);
> + struct cros_ish_in_msg *in_msg = (struct cros_ish_in_msg *)ec_dev->din;
> + struct cros_ish_out_msg *out_msg =
> + (struct cros_ish_out_msg *)ec_dev->dout;
> + size_t in_size = sizeof(struct cros_ish_in_msg) + msg->insize;
> + size_t out_size = sizeof(struct cros_ish_out_msg) + msg->outsize;
> +
> + /* Proceed only if reset-init is not in progress */
> + if (!down_read_trylock(&init_lock)) {
> + dev_warn(dev,
> + "Host is not ready to send messages to ISH. Try again\n");
> + return -EAGAIN;
> + }
> +
> + /* Sanity checks */
> + if (in_size > ec_dev->din_size) {
> + dev_err(dev,
> + "Incoming payload size %zu is too large for ec_dev->din_size %d\n",
> + in_size, ec_dev->din_size);
> + return -EMSGSIZE;
> + }
> +
> + if (out_size > ec_dev->dout_size) {
> + dev_err(dev,
> + "Outgoing payload size %zu is too large for ec_dev->dout_size %d\n",
> + out_size, ec_dev->dout_size);
> + return -EMSGSIZE;
> + }
> +
> + /* Prepare the package to be sent over ISH TP */
> + out_msg->hdr.channel = CROS_EC_COMMAND;
> + out_msg->hdr.status = 0;
> +
> + ec_dev->dout += OUT_MSG_EC_REQUEST_PREAMBLE;
> + cros_ec_prepare_tx(ec_dev, msg);
> + ec_dev->dout -= OUT_MSG_EC_REQUEST_PREAMBLE;
> +
> + dev_dbg(dev,
> + "out_msg: struct_ver=0x%x checksum=0x%x command=0x%x command_ver=0x%x data_len=0x%x\n",
> + out_msg->ec_request.struct_version,
> + out_msg->ec_request.checksum,
> + out_msg->ec_request.command,
> + out_msg->ec_request.command_version,
> + out_msg->ec_request.data_len);
> +
> + /* Send command to ISH EC firmware and read response */
> + rv = ish_send(client_data,
> + (u8 *)out_msg, out_size,
> + (u8 *)in_msg, in_size);
> + if (rv < 0)
> + goto end_error;
> +
> + rv = prepare_cros_ec_rx(ec_dev, in_msg, msg);
> + if (rv)
> + goto end_error;
> +
> + rv = in_msg->ec_response.data_len;
> +
> + dev_dbg(dev,
> + "in_msg: struct_ver=0x%x checksum=0x%x result=0x%x data_len=0x%x\n",
> + in_msg->ec_response.struct_version,
> + in_msg->ec_response.checksum,
> + in_msg->ec_response.result,
> + in_msg->ec_response.data_len);
> +
> +end_error:
> + if (msg->command == EC_CMD_REBOOT_EC)
> + msleep(EC_REBOOT_DELAY_MS);
> +
> + up_read(&init_lock);
> +
> + return rv;
> +}
> +
> +static int cros_ec_dev_init(struct ishtp_cl_data *client_data)
> +{
> + struct cros_ec_device *ec_dev;
> + struct device *dev = cl_data_to_dev(client_data);
> +
> + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> + if (!ec_dev)
> + return -ENOMEM;
> +
> + client_data->ec_dev = ec_dev;
> + dev->driver_data = ec_dev;
> +
> + ec_dev->dev = dev;
> + ec_dev->priv = client_data->cros_ish_cl;
> + ec_dev->cmd_xfer = NULL;
> + ec_dev->pkt_xfer = cros_ec_pkt_xfer_ish;
> + ec_dev->phys_name = dev_name(dev);
> + ec_dev->din_size = sizeof(struct cros_ish_in_msg) +
> + sizeof(struct ec_response_get_protocol_info);
> + ec_dev->dout_size = sizeof(struct cros_ish_out_msg);
> +
> + return cros_ec_register(ec_dev);
> +}
> +
> +static void reset_handler(struct work_struct *work)
> +{
> + int rv;
> + struct device *dev;
> + struct ishtp_cl *cros_ish_cl;
> + struct ishtp_cl_device *cl_device;
> + struct ishtp_cl_data *client_data =
> + container_of(work, struct ishtp_cl_data, work_ishtp_reset);
> +
> + /* Lock for reset to complete */
> + down_write(&init_lock);
> +
> + cros_ish_cl = client_data->cros_ish_cl;
> + cl_device = client_data->cl_device;
> +
> + /* Unlink, flush queues & start again */
> + ishtp_cl_unlink(cros_ish_cl);
> + ishtp_cl_flush_queues(cros_ish_cl);
> + ishtp_cl_free(cros_ish_cl);
> +
> + cros_ish_cl = ishtp_cl_allocate(cl_device);
> + if (!cros_ish_cl) {
> + up_write(&init_lock);
> + return;
> + }
> +
> + ishtp_set_drvdata(cl_device, cros_ish_cl);
> + ishtp_set_client_data(cros_ish_cl, client_data);
> + client_data->cros_ish_cl = cros_ish_cl;
> +
> + rv = cros_ish_init(cros_ish_cl);
> + if (rv) {
> + ishtp_cl_free(cros_ish_cl);
> + dev_err(cl_data_to_dev(client_data), "Reset Failed\n");
> + up_write(&init_lock);
> + return;
> + }
> +
> + /* Refresh ec_dev device pointers */
> + client_data->ec_dev->priv = client_data->cros_ish_cl;
> + dev = cl_data_to_dev(client_data);
> + dev->driver_data = client_data->ec_dev;
> +
> + dev_info(cl_data_to_dev(client_data), "Chrome EC ISH reset done\n");
> +
> + up_write(&init_lock);
> +}
> +
> +/**
> + * cros_ec_ishtp_probe() - ISHTP client driver probe callback
> + * @cl_device: ISHTP client device instance
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int cros_ec_ishtp_probe(struct ishtp_cl_device *cl_device)
> +{
> + int rv;
> + struct ishtp_cl *cros_ish_cl;
> + struct ishtp_cl_data *client_data =
> + devm_kzalloc(ishtp_device(cl_device),
> + sizeof(*client_data), GFP_KERNEL);
> + if (!client_data)
> + return -ENOMEM;
> +
> + /* Lock for initialization to complete */
> + down_write(&init_lock);
> +
> + cros_ish_cl = ishtp_cl_allocate(cl_device);
> + if (!cros_ish_cl) {
> + rv = -ENOMEM;
> + goto end_ishtp_cl_alloc_error;
> + }
> +
> + ishtp_set_drvdata(cl_device, cros_ish_cl);
> + ishtp_set_client_data(cros_ish_cl, client_data);
> + client_data->cros_ish_cl = cros_ish_cl;
> + client_data->cl_device = cl_device;
> +
> + init_waitqueue_head(&client_data->response.wait_queue);
> +
> + INIT_WORK(&client_data->work_ishtp_reset,
> + reset_handler);
> + INIT_WORK(&client_data->work_ec_evt,
> + ish_evt_handler);
> +
> + rv = cros_ish_init(cros_ish_cl);
> + if (rv)
> + goto end_ishtp_cl_init_error;
> +
> + ishtp_get_device(cl_device);
> +
> + up_write(&init_lock);
> +
> + /* Register croc_ec_dev mfd */
> + rv = cros_ec_dev_init(client_data);
> + if (rv)
> + goto end_cros_ec_dev_init_error;
> +
> + return 0;
> +
> +end_cros_ec_dev_init_error:
> + ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_DISCONNECTING);
> + ishtp_cl_disconnect(cros_ish_cl);
> + ishtp_cl_unlink(cros_ish_cl);
> + ishtp_cl_flush_queues(cros_ish_cl);
> + ishtp_put_device(cl_device);
> +end_ishtp_cl_init_error:
> + ishtp_cl_free(cros_ish_cl);
> +end_ishtp_cl_alloc_error:
> + up_write(&init_lock);
> + return rv;
> +}
> +
> +/**
> + * cros_ec_ishtp_remove() - ISHTP client driver remove callback
> + * @cl_device: ISHTP client device instance
> + *
> + * Return: 0
> + */
> +static int cros_ec_ishtp_remove(struct ishtp_cl_device *cl_device)
> +{
> + struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device);
> + struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
> +
> + cancel_work_sync(&client_data->work_ishtp_reset);
> + cancel_work_sync(&client_data->work_ec_evt);
> + cros_ish_deinit(cros_ish_cl);
> + ishtp_put_device(cl_device);
> +
> + return 0;
> +}
> +
> +/**
> + * cros_ec_ishtp_reset() - ISHTP client driver reset callback
> + * @cl_device: ISHTP client device instance
> + *
> + * Return: 0
> + */
> +static int cros_ec_ishtp_reset(struct ishtp_cl_device *cl_device)
> +{
> + struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device);
> + struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
> +
> + schedule_work(&client_data->work_ishtp_reset);
> +
> + return 0;
> +}
> +
> +/**
> + * cros_ec_ishtp_suspend() - ISHTP client driver suspend callback
> + * @device: device instance
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int __maybe_unused cros_ec_ishtp_suspend(struct device *device)
> +{
> + struct ishtp_cl_device *cl_device = dev_get_drvdata(device);
> + struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device);
> + struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
> +
> + return cros_ec_suspend(client_data->ec_dev);
> +}
> +
> +/**
> + * cros_ec_ishtp_resume() - ISHTP client driver resume callback
> + * @device: device instance
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int __maybe_unused cros_ec_ishtp_resume(struct device *device)
> +{
> + struct ishtp_cl_device *cl_device = dev_get_drvdata(device);
> + struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device);
> + struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl);
> +
> + return cros_ec_resume(client_data->ec_dev);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(cros_ec_ishtp_pm_ops, cros_ec_ishtp_suspend,
> + cros_ec_ishtp_resume);
> +
> +static struct ishtp_cl_driver cros_ec_ishtp_driver = {
> + .name = "cros_ec_ishtp",
> + .guid = &cros_ish_guid,
> + .probe = cros_ec_ishtp_probe,
> + .remove = cros_ec_ishtp_remove,
> + .reset = cros_ec_ishtp_reset,
> + .driver = {
> + .pm = &cros_ec_ishtp_pm_ops,
> + },
> +};
> +
> +static int __init cros_ec_ishtp_mod_init(void)
> +{
> + return ishtp_cl_driver_register(&cros_ec_ishtp_driver, THIS_MODULE);
> +}
> +
> +static void __exit cros_ec_ishtp_mod_exit(void)
> +{
> + ishtp_cl_driver_unregister(&cros_ec_ishtp_driver);
> +}
> +
> +module_init(cros_ec_ishtp_mod_init);
> +module_exit(cros_ec_ishtp_mod_exit);
> +
> +MODULE_DESCRIPTION("ChromeOS EC ISHTP Client Driver");
> +MODULE_AUTHOR("Rushikesh S Kadam <rushikesh.s.kadam@intel.com>");
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("ishtp:*");
> --
> 1.9.1
>
^ permalink raw reply
* Re: [Xen-devel] [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
From: Paul Durrant @ 2019-04-10 15:20 UTC (permalink / raw)
To: Anthony Perard
Cc: Kevin Wolf, Stefano Stabellini, qemu-block@nongnu.org,
qemu-devel@nongnu.org, Max Reitz, Stefan Hajnoczi,
xen-devel@lists.xenproject.org
In-Reply-To: <20190410125708.GE1435@perard.uk.xensource.com>
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 10 April 2019 13:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
>
> On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > and then uses aio_set_fd_handler() to set the callback rather than
> > qemu_set_fd_handler().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > }
> >
> > XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > + AioContext *ctx,
> > unsigned int port,
> > XenEventHandler handler,
> > void *opaque, Error **errp)
> > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > channel->handler = handler;
> > channel->opaque = opaque;
> >
> > - qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > - channel);
> > + channel->ctx = ctx;
> > + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > + xen_device_event, NULL, NULL, channel);
>
> I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> `true' here, instead. That flag seems to be used when making a snapshot
> of a blockdev, for example.
>
> That was introduced by:
> dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
>
> What do you think?
Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?
Cheers,
Paul
>
>
> --
> Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree
From: Rob Herring @ 2019-04-10 15:32 UTC (permalink / raw)
To: Aisheng Dong
Cc: devicetree@vger.kernel.org, sboyd@kernel.org,
mturquette@baylibre.com, dl-linux-imx, kernel@pengutronix.de,
Fabio Estevam, shawnguo@kernel.org, linux-clk@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM0PR04MB42110F1F251243A165DB9EC680580@AM0PR04MB4211.eurprd04.prod.outlook.com>
On Wed, Mar 27, 2019 at 9:35 AM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, March 26, 2019 9:47 PM
> > On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> > > There's a few limitations on one cell clock binding (#clock-cells =
> > > <1>) that we have to define all clock IDs for device tree to reference.
> > > This may cause troubles if we want to use common clock IDs for multi
> > > platforms support when the clock of those platforms are mostly the same.
> > > e.g. Current clock IDs name are defined with SS prefix. However the
> > > device may reside in different SS across CPUs, that means the SS
> > > prefix may not valid anymore for a new SoC. Furthermore, the device
> > > availability of those clocks may also vary a bit.
> > >
> > > For such situation, We formerly planned to add all new IDs for each SS
> > > and dynamically check availability for different SoC in driver. That
> > > can be done but that may involve a lot effort and may result in more
> > > changes and duplicated code in driver, also make device tree
> > > upstreaming hard which depends on Clock IDs.
> > >
> > > To relief this situation, we want to move the clock definition into
> > > device tree which can fully decouple the dependency of Clock ID
> > > definition from device tree. And no frequent changes required in clock driver
> > any more.
> > >
> > > Then we can use the existence of clock nodes in device tree to address
> > > the device and clock availability differences across different SoCs.
> > >
> > > For SCU clocks, only two params required, thus two new property created:
> > > rsrc-id = <IMX_SC_R_UART_0>;
> > > clk-type = <IMX_SC_PM_CLK_PER>;
> > >
> > > And as we want to support clock set parent function, 'clocks' property
> > > is also used to pass all the possible input parents.
> > >
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > > .../devicetree/bindings/arm/freescale/fsl,scu.txt | 29
> > ++++++++++++++++------
> > > include/dt-bindings/firmware/imx/rsrc.h | 17
> > +++++++++++++
> > > 2 files changed, 38 insertions(+), 8 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 72d481c..2816789 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -78,6 +78,19 @@ Required properties:
> > > "fsl,imx8qm-clock"
> > > "fsl,imx8qxp-clock"
> > > followed by "fsl,scu-clk"
> > > +- #clock-cells: Should be 0.
> > > +- rsrc-id: Resource ID associated with this clock
> > > +- clk-type: Type of this clock.
> > > + Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > > + available clock types supported by SCU.
> >
> > Can't you just make these 2 values clock cells? I'm all for getting rid of made
> > up clock numbers.
> >
>
> Thanks for the agreement to remove clock IDs.
>
> The 2 values clock cell seems not the best approach for i.MX because it still needs
> to define all clocks in the driver which is exactly we want to avoid now due to some
> special HW characteristic:
Why's that? You can walk the DT and extract the 2 cells for each clock
present. That's not any different than walking child nodes here and
getting the resource ids and type. That's not really fast, but if
speed is really an issue we can consider addressing that in ways that
extend rather than change the binding.
> 1. clock resources may be allocated to different SW execution partition by firmware
> and A core may not have access rights for those clocks not belong to its partition.
> So we want to describe them in DT according to the partition configuration.
Do you have an example? I'd assume you assign peripherals to different
partitions and resource assignment simply follows that. Can clocks not
be available when a peripheral still is?
> 2. Each clock is associated with a different power domain which is better to be
> described in device tree. And clock state will be lost and need restore after power cycle
> of the domain.
>
> Based on above requirements, do you think we can do as below?
Can you provide an example that shows the whole hierarchy for a
peripheral. Here you have FSPI, PWM, and MMC. Reviewing SCU clocks and
LPCG clocks separately is not helpful.
>
> //LSIO SS
> lsio_scu_clk: lsio-scu-clock-controller {
> compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
>
> fspi0_clk: clock-fspi0{
> #clock-cells = <0>;
> rsrc-id = <IMX_SC_R_FSPI_0>;
> clk-type = <IMX_SC_PM_CLK_PER>;
> power-domains = <&pd IMX_SC_R_FSPI_0>;
Are the power domain ID and rsrc-id always the same for a clock?
> };
>
> fspi1_clk: clock-fspi1{
> ...
> };
> ...
> };
>
> /* LPCG clocks */
> lsio_lpcg_clk: lsio-lpcg-clock-controller {
> compatible = "fsl,imx8qxp-lpcg";
I think this wrapper node should be removed and the compatible moved
into the child nodes.
> pwm0_lpcg: clock-controller@5d400000 {
> reg = <0x5d400000 0x10000>;
> #clock-cells = <1>;
> clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
> <&lsio_bus_clk>, <&pwm0_clk>;
> bit-offset = <0 4 16 20 24>;
Are all LPCG instances the same, but some clocks are missing if the
child peripheral doesn't use certain clocks? IOW, bit 0 is always
ipg_clk, bit 24 is always ipg_mstr_clk?
Assuming so, 'bit-offset' should be removed and you should either have
a fixed number of clock entries with 0 entries for non-connected
clocks or use clock-names to define which clocks are present (with the
same set of defined names for all LPCG instances).
> clock-output-names = "pwm0_lpcg_ipg_clk",
> "pwm0_lpcg_ipg_hf_clk",
> "pwm0_lpcg_ipg_s_clk",
> "pwm0_lpcg_ipg_slv_clk",
> "pwm0_lpcg_ipg_mstr_clk";
IMO, this is wrong as the names should be relative to the module. So
'ipg_clk', 'ipg_hf_clk', etc.
> power-domains = <&pd IMX_SC_R_PWM_0>;
> status = "disabled";
> };
>
> pwm1_lpcg: clock-controller@5d410000 {
> ...
> }
> ...
> };
>
> And for users, it could simply be:
> usdhc1: mmc@5b010000 {
> clocks = <&sdhc0_lpcg 1>,
> <&sdhc0_lpcg 0>,
> <&sdhc0_lpcg 2>;
> clock-names = "ipg", "per", "ahb";
> assigned-clocks = <&sdhc0_clk>;
> assigned-clock-rates = <200000000>;
> ....
> };
^ permalink raw reply
* [PATCH dev-5.0] hwmon: (pmbus/isl68137): Add driver for Intersil ISL68137 PWM Controller
From: Patrick Venture @ 2019-04-10 15:31 UTC (permalink / raw)
To: venture, joel; +Cc: Maxim Sloyko, andrew, openbmc, Robert Lippert
From: Maxim Sloyko <maxims@google.com>
Adds the pmbus driver for the Intersil ISL68137 PWM Controller.
Signed-off-by: Maxim Sloyko <maxims@google.com>
Signed-off-by: Robert Lippert <rlippert@google.com>
Signed-off-by: Patrick Venture <venture@google.com>
---
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/isl68137.c | 203 +++++++++++++++++++++++++++++++++
3 files changed, 214 insertions(+)
create mode 100644 drivers/hwmon/pmbus/isl68137.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 629cb45f8557..15c197f1c4c4 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -54,6 +54,16 @@ config SENSORS_IR35221
This driver can also be built as a module. If so, the module will
be called ir35521.
+config SENSORS_ISL68137
+ tristate "Intersil ISL68137"
+ default y
+ help
+ If you say yes here you get hardware monitoring support for Intersil
+ ISL68137.
+
+ This driver can also be built as a module. If so, the module will
+ be called isl68137.
+
config SENSORS_LM25066
tristate "National Semiconductor LM25066 and compatibles"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index ea0e39518c21..0684b35216da 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
+obj-$(CONFIG_SENSORS_ISL68137) += isl68137.o
obj-$(CONFIG_SENSORS_LM25066) += lm25066.o
obj-$(CONFIG_SENSORS_LTC2978) += ltc2978.o
obj-$(CONFIG_SENSORS_LTC3815) += ltc3815.o
diff --git a/drivers/hwmon/pmbus/isl68137.c b/drivers/hwmon/pmbus/isl68137.c
new file mode 100644
index 000000000000..ccac14bdb985
--- /dev/null
+++ b/drivers/hwmon/pmbus/isl68137.c
@@ -0,0 +1,203 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Hardware monitoring driver for Intersil ISL68137
+ *
+ * Copyright (c) 2017 Google Inc
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/hwmon-sysfs.h>
+#include "pmbus.h"
+
+#define ISL68137_VOUT_AVS 0x30
+
+static struct pmbus_driver_info isl68137_info = {
+ .pages = 2,
+ .format[PSC_VOLTAGE_IN] = direct,
+ .format[PSC_VOLTAGE_OUT] = direct,
+ .format[PSC_CURRENT_IN] = direct,
+ .format[PSC_CURRENT_OUT] = direct,
+ .format[PSC_POWER] = direct,
+ .format[PSC_TEMPERATURE] = direct,
+ .m[PSC_VOLTAGE_IN] = 1,
+ .b[PSC_VOLTAGE_IN] = 0,
+ .R[PSC_VOLTAGE_IN] = 3,
+ .m[PSC_VOLTAGE_OUT] = 1,
+ .b[PSC_VOLTAGE_OUT] = 0,
+ .R[PSC_VOLTAGE_OUT] = 3,
+ .m[PSC_CURRENT_IN] = 1,
+ .b[PSC_CURRENT_IN] = 0,
+ .R[PSC_CURRENT_IN] = 2,
+ .m[PSC_CURRENT_OUT] = 1,
+ .b[PSC_CURRENT_OUT] = 0,
+ .R[PSC_CURRENT_OUT] = 1,
+ .m[PSC_POWER] = 1,
+ .b[PSC_POWER] = 0,
+ .R[PSC_POWER] = 0,
+ .m[PSC_TEMPERATURE] = 1,
+ .b[PSC_TEMPERATURE] = 0,
+ .R[PSC_TEMPERATURE] = 0,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_PIN
+ | PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
+ | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_TEMP
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
+ .func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_POUT,
+};
+
+static ssize_t isl68137_avs_enable_show_page(struct i2c_client *client,
+ int page,
+ char *buf)
+{
+ int val = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+
+ return sprintf(buf, "%d\n",
+ (val & ISL68137_VOUT_AVS) == ISL68137_VOUT_AVS ? 1 : 0);
+}
+
+static ssize_t isl68137_avs_enable_store_page(struct i2c_client *client,
+ int page,
+ const char *buf, size_t count)
+{
+ int rc, op_val;
+
+ if (count < 1) {
+ rc = -EINVAL;
+ goto out;
+ }
+
+ switch (buf[0]) {
+ case '0':
+ op_val = 0;
+ break;
+ case '1':
+ op_val = ISL68137_VOUT_AVS;
+ break;
+ default:
+ rc = -EINVAL;
+ goto out;
+ }
+
+ /*
+ * Writes to VOUT setpoint over AVSBus will persist after the VRM is
+ * switched to PMBus control. Switching back to AVSBus control
+ * restores this persisted setpoint rather than re-initializing to
+ * PMBus VOUT_COMMAND. Writing VOUT_COMMAND first over PMBus before
+ * enabling AVS control is the workaround.
+ */
+ if (op_val == ISL68137_VOUT_AVS) {
+ int vout_command = pmbus_read_word_data(client, page,
+ PMBUS_VOUT_COMMAND);
+ rc = pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
+ vout_command);
+ if (rc)
+ goto out;
+ }
+
+ rc = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+ ISL68137_VOUT_AVS, op_val);
+
+out:
+ return rc < 0 ? rc : count;
+}
+
+static ssize_t isl68137_avs_enable_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ struct i2c_client *client = kobj_to_i2c_client(&dev->kobj);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+
+ return isl68137_avs_enable_show_page(client, attr->index, buf);
+}
+
+static ssize_t isl68137_avs_enable_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct i2c_client *client = kobj_to_i2c_client(&dev->kobj);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+
+ return isl68137_avs_enable_store_page(client, attr->index, buf, count);
+}
+
+static SENSOR_DEVICE_ATTR_2(avs0_enabled, 0644,
+ isl68137_avs_enable_show, isl68137_avs_enable_store,
+ 0, 0);
+static SENSOR_DEVICE_ATTR_2(avs1_enabled, 0644,
+ isl68137_avs_enable_show, isl68137_avs_enable_store,
+ 0, 1);
+
+static int isl68137_remove(struct i2c_client *client);
+
+static int isl68137_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int rc;
+
+ rc = pmbus_do_probe(client, id, &isl68137_info);
+ if (rc)
+ return rc;
+
+ rc = device_create_file(&client->dev,
+ &sensor_dev_attr_avs0_enabled.dev_attr);
+ if (rc)
+ goto out_fail;
+ rc = device_create_file(&client->dev,
+ &sensor_dev_attr_avs1_enabled.dev_attr);
+ if (rc)
+ goto out_fail;
+
+ return rc;
+
+out_fail:
+ isl68137_remove(client);
+ return rc;
+}
+
+static int isl68137_remove(struct i2c_client *client)
+{
+ device_remove_file(&client->dev,
+ &sensor_dev_attr_avs1_enabled.dev_attr);
+ device_remove_file(&client->dev,
+ &sensor_dev_attr_avs0_enabled.dev_attr);
+ return pmbus_do_remove(client);
+}
+
+static const struct i2c_device_id isl68137_id[] = {
+ {"isl68137", 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, isl68137_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver isl68137_driver = {
+ .driver = {
+ .name = "isl68137",
+ },
+ .probe = isl68137_probe,
+ .remove = isl68137_remove,
+ .id_table = isl68137_id,
+};
+
+module_i2c_driver(isl68137_driver);
+
+MODULE_AUTHOR("Maxim Sloyko <maxims@google.com>");
+MODULE_DESCRIPTION("PMBus driver for Intersil ISL68137");
+MODULE_LICENSE("GPL");
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* Re: Tun congestion/BQL
From: David Woodhouse @ 2019-04-10 15:32 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Jason Wang, netdev
In-Reply-To: <875zrlvr9a.fsf@toke.dk>
[-- Attachment #1: Type: text/plain, Size: 3265 bytes --]
On Wed, 2019-04-10 at 17:01 +0200, Toke Høiland-Jørgensen wrote:
> David Woodhouse <dwmw2@infradead.org> writes:
>
> > On Wed, 2019-04-10 at 15:42 +0200, Toke Høiland-Jørgensen wrote:
> > > > > That doesn't seem to make much difference at all; it's still dropping a
> > > > > lot of packets because ptr_ring_produce() is returning non-zero.
> > > >
> > > >
> > > > I think you need try to stop the queue just in this case? Ideally we may
> > > > want to stop the queue when the queue is about to full, but we don't
> > > > have such helper currently.
> >
> > I don't quite understand. If the ring isn't full after I've put a
> > packet into it... how can it be full subsequently? We can't end up in
> > tun_net_xmit() concurrently, right? I'm not (knowingly) using XDP.
> >
> > > Ideally we want to react when the queue starts building rather than when
> > > it starts getting full; by pushing back on upper layers (or, if
> > > forwarding, dropping packets to signal congestion).
> >
> > This is precisely what my first accidental if (!ptr_ring_empty())
> > variant was doing, right? :)
>
> Yeah, I guess. But maybe a too aggressively? How are you processing
> packets on the dequeue side (for crypto)? One at a time, or is there
> some kind of batching in play?
Slight batching. The main loop in OpenConnect will suck packets out of
the tun device until its queue is "full", which by default is 10
packets but tweaking that makes little difference at all to my testing
until I take it below 3.
(Until fairly recently, I was *ignoring* the result of sendto() on the
UDP side, which meant that I was wasting time encrypting packets that
got dropped. Now I react appropriately to -EAGAIN (-ENOBUFS?) on the
sending side, and I don't pull any more packets from the tun device
until my packet queue is no longer "full". The latest 8.02 release of
OpenConnect still has that behaviour.)
> > > In practice, this means tuning the TX ring to the *minimum* size it can
> > > be without starving (this is basically what BQL does for Ethernet), and
> > > keeping packets queued in the qdisc layer instead, where it can be
> > > managed...
> >
> > I was going to add BQL (as $SUBJECT may have caused you to infer) but
> > trivially adding the netdev_sent_queue() in tun_net_xmit() and
> > netdev_completed_queue() for xdp vs. skb in tun_do_read() was tripping
> > the BUG in dql_completed(). I just ripped that part out and focused on
> > the queue stop/start and haven't gone back to it yet.
>
> Right, makes sense. What qdisc are you running on the tun device? Also,
> I assume that netperf is running on the same host that has the tun
> device on it, right?
I haven't looked at the qdisc, so it's fq_codel which it got as
standard. And yes, netperf is running on the same host.
My test setup is here (inline in the first, refined and attached to the
second):
https://lists.infradead.org/pipermail/openconnect-devel/2019-April/005261.html
https://lists.infradead.org/pipermail/openconnect-devel/2019-April/005265.html
Basically, I started by setting up in-kernel ESP over UDP between the
two hosts, then tricked OpenConnect into thinking there was a real VPN
server on one of them.
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply
* Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes
From: Stanimir Varbanov via iommu @ 2019-04-10 15:32 UTC (permalink / raw)
To: Marc Gonzalez, Bjorn Andersson
Cc: Jeffrey Hugo, MSM, Douglas Anderson, Evan Green,
Stanimir Varbanov, Manu Gautam, iommu, Srinivas Kandagatla, PCI,
Bjorn Helgaas, Robin Murphy
In-Reply-To: <186f7ca2-84e2-a37e-79e6-f1fec8e25374-GANU6spQydw@public.gmane.org>
Hi Marc,
Few comments inline.
On 3/28/19 7:06 PM, Marc Gonzalez wrote:
> Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes.
>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org>
> ---
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 5a1c0961b281..9f979a51f679 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -621,6 +621,84 @@
> <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
> };
>
> + pcie0: pci@1c00000 {
> + compatible = "qcom,pcie-msm8996";
> + reg-names = "parf", "dbi", "elbi", "config";
> + reg = <0x01c00000 0x2000>,
> + <0x1b000000 0xf1d>,
> + <0x1b000f20 0xa8>,
> + <0x1b100000 0x100000>;
could you please populate reg property and after that reg-names property.
> + device_type = "pci";
> + linux,pci-domain = <0>;
> + bus-range = <0x00 0xff>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + power-domains = <&gcc PCIE_0_GDSC>;
> +
> + num-lanes = <1>;
> + phy-names = "pciephy";
> + phys = <&pciephy>;
> +
> + ranges =
> + /*** downstream I/O ***/
could you make this a standard kernel comment or completely drop it
> + <0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>,
> + /*** non-prefetchable memory ***/
ditto
> + <0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>;
> +
> + #interrupt-cells = <1>;
> + interrupt-names = "msi";
> + interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-map-mask = <0 0 0 0x7>;
> + interrupt-map =
> + <0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
move this to a line upper
> + <0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> + <0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> + <0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> + clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux";
> + clocks =
> + <&gcc GCC_PCIE_0_PIPE_CLK>,
move this to a line upper
> + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
> + <&gcc GCC_PCIE_0_SLV_AXI_CLK>,
> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> + <&gcc GCC_PCIE_0_AUX_CLK>;
please swap the order clocks and clock-names
> +
> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>;
iommu-map-mask? It is optional but I had to ask :)
> +
> + /* PCIe Fundamental Reset */
this comment is useless :) please drop it
> + perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
> + };
> +
> + phy@1c06000 {
> + compatible = "qcom,msm8998-qmp-pcie-phy";
> + reg = <0x01c06000 0x18c>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + clock-names = "aux", "cfg_ahb", "ref";
> + clocks =
> + <&gcc GCC_PCIE_PHY_AUX_CLK>,
> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> + <&gcc GCC_PCIE_CLKREF_CLK>;\
please, swap the order of clocks and clock-names, and move first clock a
line upper. Also delete '\' symbol at the end of last line.
> +
> + reset-names = "phy", "common";
> + resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>;
resets prop and after that reset-names, please.
> +
> + vdda-phy-supply = <&vreg_l1a_0p875>;
> + vdda-pll-supply = <&vreg_l2a_1p2>;
> +
> + pciephy: lane@1c06800 {
> + reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>;
> + #phy-cells = <0>;
> +
> + clock-names = "pipe0";
> + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
please, swap clocks and clock-names
> + clock-output-names = "pcie_0_pipe_clk_src";
> + #clock-cells = <0>;
> + };
> + };
> +
> tcsr_mutex_regs: syscon@1f40000 {
> compatible = "syscon";
> reg = <0x1f40000 0x20000>;
>
--
regards,
Stan
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree
From: Rob Herring @ 2019-04-10 15:32 UTC (permalink / raw)
To: Aisheng Dong
Cc: linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
sboyd@kernel.org, mturquette@baylibre.com, shawnguo@kernel.org,
Fabio Estevam, dl-linux-imx, kernel@pengutronix.de,
devicetree@vger.kernel.org
In-Reply-To: <AM0PR04MB42110F1F251243A165DB9EC680580@AM0PR04MB4211.eurprd04.prod.outlook.com>
On Wed, Mar 27, 2019 at 9:35 AM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, March 26, 2019 9:47 PM
> > On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> > > There's a few limitations on one cell clock binding (#clock-cells =
> > > <1>) that we have to define all clock IDs for device tree to reference.
> > > This may cause troubles if we want to use common clock IDs for multi
> > > platforms support when the clock of those platforms are mostly the same.
> > > e.g. Current clock IDs name are defined with SS prefix. However the
> > > device may reside in different SS across CPUs, that means the SS
> > > prefix may not valid anymore for a new SoC. Furthermore, the device
> > > availability of those clocks may also vary a bit.
> > >
> > > For such situation, We formerly planned to add all new IDs for each SS
> > > and dynamically check availability for different SoC in driver. That
> > > can be done but that may involve a lot effort and may result in more
> > > changes and duplicated code in driver, also make device tree
> > > upstreaming hard which depends on Clock IDs.
> > >
> > > To relief this situation, we want to move the clock definition into
> > > device tree which can fully decouple the dependency of Clock ID
> > > definition from device tree. And no frequent changes required in clock driver
> > any more.
> > >
> > > Then we can use the existence of clock nodes in device tree to address
> > > the device and clock availability differences across different SoCs.
> > >
> > > For SCU clocks, only two params required, thus two new property created:
> > > rsrc-id = <IMX_SC_R_UART_0>;
> > > clk-type = <IMX_SC_PM_CLK_PER>;
> > >
> > > And as we want to support clock set parent function, 'clocks' property
> > > is also used to pass all the possible input parents.
> > >
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > > .../devicetree/bindings/arm/freescale/fsl,scu.txt | 29
> > ++++++++++++++++------
> > > include/dt-bindings/firmware/imx/rsrc.h | 17
> > +++++++++++++
> > > 2 files changed, 38 insertions(+), 8 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 72d481c..2816789 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -78,6 +78,19 @@ Required properties:
> > > "fsl,imx8qm-clock"
> > > "fsl,imx8qxp-clock"
> > > followed by "fsl,scu-clk"
> > > +- #clock-cells: Should be 0.
> > > +- rsrc-id: Resource ID associated with this clock
> > > +- clk-type: Type of this clock.
> > > + Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > > + available clock types supported by SCU.
> >
> > Can't you just make these 2 values clock cells? I'm all for getting rid of made
> > up clock numbers.
> >
>
> Thanks for the agreement to remove clock IDs.
>
> The 2 values clock cell seems not the best approach for i.MX because it still needs
> to define all clocks in the driver which is exactly we want to avoid now due to some
> special HW characteristic:
Why's that? You can walk the DT and extract the 2 cells for each clock
present. That's not any different than walking child nodes here and
getting the resource ids and type. That's not really fast, but if
speed is really an issue we can consider addressing that in ways that
extend rather than change the binding.
> 1. clock resources may be allocated to different SW execution partition by firmware
> and A core may not have access rights for those clocks not belong to its partition.
> So we want to describe them in DT according to the partition configuration.
Do you have an example? I'd assume you assign peripherals to different
partitions and resource assignment simply follows that. Can clocks not
be available when a peripheral still is?
> 2. Each clock is associated with a different power domain which is better to be
> described in device tree. And clock state will be lost and need restore after power cycle
> of the domain.
>
> Based on above requirements, do you think we can do as below?
Can you provide an example that shows the whole hierarchy for a
peripheral. Here you have FSPI, PWM, and MMC. Reviewing SCU clocks and
LPCG clocks separately is not helpful.
>
> //LSIO SS
> lsio_scu_clk: lsio-scu-clock-controller {
> compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
>
> fspi0_clk: clock-fspi0{
> #clock-cells = <0>;
> rsrc-id = <IMX_SC_R_FSPI_0>;
> clk-type = <IMX_SC_PM_CLK_PER>;
> power-domains = <&pd IMX_SC_R_FSPI_0>;
Are the power domain ID and rsrc-id always the same for a clock?
> };
>
> fspi1_clk: clock-fspi1{
> ...
> };
> ...
> };
>
> /* LPCG clocks */
> lsio_lpcg_clk: lsio-lpcg-clock-controller {
> compatible = "fsl,imx8qxp-lpcg";
I think this wrapper node should be removed and the compatible moved
into the child nodes.
> pwm0_lpcg: clock-controller@5d400000 {
> reg = <0x5d400000 0x10000>;
> #clock-cells = <1>;
> clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
> <&lsio_bus_clk>, <&pwm0_clk>;
> bit-offset = <0 4 16 20 24>;
Are all LPCG instances the same, but some clocks are missing if the
child peripheral doesn't use certain clocks? IOW, bit 0 is always
ipg_clk, bit 24 is always ipg_mstr_clk?
Assuming so, 'bit-offset' should be removed and you should either have
a fixed number of clock entries with 0 entries for non-connected
clocks or use clock-names to define which clocks are present (with the
same set of defined names for all LPCG instances).
> clock-output-names = "pwm0_lpcg_ipg_clk",
> "pwm0_lpcg_ipg_hf_clk",
> "pwm0_lpcg_ipg_s_clk",
> "pwm0_lpcg_ipg_slv_clk",
> "pwm0_lpcg_ipg_mstr_clk";
IMO, this is wrong as the names should be relative to the module. So
'ipg_clk', 'ipg_hf_clk', etc.
> power-domains = <&pd IMX_SC_R_PWM_0>;
> status = "disabled";
> };
>
> pwm1_lpcg: clock-controller@5d410000 {
> ...
> }
> ...
> };
>
> And for users, it could simply be:
> usdhc1: mmc@5b010000 {
> clocks = <&sdhc0_lpcg 1>,
> <&sdhc0_lpcg 0>,
> <&sdhc0_lpcg 2>;
> clock-names = "ipg", "per", "ahb";
> assigned-clocks = <&sdhc0_clk>;
> assigned-clock-rates = <200000000>;
> ....
> };
^ permalink raw reply
* Re: [PATCH 21/29] drm/i915: Switch back to an array of logical per-engine HW contexts
From: Tvrtko Ursulin @ 2019-04-10 15:32 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
In-Reply-To: <20190408091728.20207-21-chris@chris-wilson.co.uk>
On 08/04/2019 10:17, Chris Wilson wrote:
> We switched to a tree of per-engine HW context to accommodate the
> introduction of virtual engines. However, we plan to also support
> multiple instances of the same engine within the GEM context, defeating
> our use of the engine as a key to looking up the HW context. Just
> allocate a logical per-engine instance and always use an index into the
> ctx->engines[]. Later on, this ctx->engines[] may be replaced by a user
> specified map.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gt/intel_context.c | 97 ++-----------------
> drivers/gpu/drm/i915/gt/intel_context.h | 25 +----
> drivers/gpu/drm/i915/gt/intel_context_types.h | 2 -
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +-
> drivers/gpu/drm/i915/gt/mock_engine.c | 3 +-
> drivers/gpu/drm/i915/gvt/scheduler.c | 2 +-
> drivers/gpu/drm/i915/i915_gem.c | 29 +++---
> drivers/gpu/drm/i915/i915_gem_context.c | 92 +++++++++++++++---
> drivers/gpu/drm/i915/i915_gem_context.h | 42 ++++++++
> drivers/gpu/drm/i915/i915_gem_context_types.h | 26 ++++-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 70 ++++++-------
> drivers/gpu/drm/i915/i915_perf.c | 81 +++++++++-------
> drivers/gpu/drm/i915/i915_request.c | 2 +-
> drivers/gpu/drm/i915/intel_guc_submission.c | 24 +++--
> .../gpu/drm/i915/selftests/i915_gem_context.c | 2 +-
> drivers/gpu/drm/i915/selftests/mock_context.c | 14 ++-
> 16 files changed, 283 insertions(+), 230 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 46bf8d8fb7e4..803ac9a1dd15 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -17,7 +17,7 @@ static struct i915_global_context {
> struct kmem_cache *slab_ce;
> } global;
>
> -struct intel_context *intel_context_alloc(void)
> +static struct intel_context *intel_context_alloc(void)
> {
> return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
> }
> @@ -28,104 +28,17 @@ void intel_context_free(struct intel_context *ce)
> }
>
> struct intel_context *
> -intel_context_lookup(struct i915_gem_context *ctx,
> +intel_context_create(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> - struct intel_context *ce = NULL;
> - struct rb_node *p;
> -
> - spin_lock(&ctx->hw_contexts_lock);
> - p = ctx->hw_contexts.rb_node;
> - while (p) {
> - struct intel_context *this =
> - rb_entry(p, struct intel_context, node);
> -
> - if (this->engine == engine) {
> - GEM_BUG_ON(this->gem_context != ctx);
> - ce = this;
> - break;
> - }
> -
> - if (this->engine < engine)
> - p = p->rb_right;
> - else
> - p = p->rb_left;
> - }
> - spin_unlock(&ctx->hw_contexts_lock);
> -
> - return ce;
> -}
> -
> -struct intel_context *
> -__intel_context_insert(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine,
> - struct intel_context *ce)
> -{
> - struct rb_node **p, *parent;
> - int err = 0;
> -
> - spin_lock(&ctx->hw_contexts_lock);
> -
> - parent = NULL;
> - p = &ctx->hw_contexts.rb_node;
> - while (*p) {
> - struct intel_context *this;
> -
> - parent = *p;
> - this = rb_entry(parent, struct intel_context, node);
> -
> - if (this->engine == engine) {
> - err = -EEXIST;
> - ce = this;
> - break;
> - }
> -
> - if (this->engine < engine)
> - p = &parent->rb_right;
> - else
> - p = &parent->rb_left;
> - }
> - if (!err) {
> - rb_link_node(&ce->node, parent, p);
> - rb_insert_color(&ce->node, &ctx->hw_contexts);
> - }
> -
> - spin_unlock(&ctx->hw_contexts_lock);
> -
> - return ce;
> -}
> -
> -void __intel_context_remove(struct intel_context *ce)
> -{
> - struct i915_gem_context *ctx = ce->gem_context;
> -
> - spin_lock(&ctx->hw_contexts_lock);
> - rb_erase(&ce->node, &ctx->hw_contexts);
> - spin_unlock(&ctx->hw_contexts_lock);
> -}
> -
> -struct intel_context *
> -intel_context_instance(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine)
> -{
> - struct intel_context *ce, *pos;
> -
> - ce = intel_context_lookup(ctx, engine);
> - if (likely(ce))
> - return intel_context_get(ce);
> + struct intel_context *ce;
>
> ce = intel_context_alloc();
> if (!ce)
> return ERR_PTR(-ENOMEM);
>
> intel_context_init(ce, ctx, engine);
> -
> - pos = __intel_context_insert(ctx, engine, ce);
> - if (unlikely(pos != ce)) /* Beaten! Use their HW context instead */
> - intel_context_free(ce);
> -
> - GEM_BUG_ON(intel_context_lookup(ctx, engine) != pos);
> - return intel_context_get(pos);
> + return ce;
> }
>
> int __intel_context_do_pin(struct intel_context *ce)
> @@ -204,6 +117,8 @@ intel_context_init(struct intel_context *ce,
> struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> + GEM_BUG_ON(!engine->cops);
> +
> kref_init(&ce->ref);
>
> ce->gem_context = ctx;
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
> index 4f8ef45e6344..567a415be513 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -12,24 +12,16 @@
> #include "intel_context_types.h"
> #include "intel_engine_types.h"
>
> -struct intel_context *intel_context_alloc(void);
> -void intel_context_free(struct intel_context *ce);
> -
> void intel_context_init(struct intel_context *ce,
> struct i915_gem_context *ctx,
> struct intel_engine_cs *engine);
>
> -/**
> - * intel_context_lookup - Find the matching HW context for this (ctx, engine)
> - * @ctx - the parent GEM context
> - * @engine - the target HW engine
> - *
> - * May return NULL if the HW context hasn't been instantiated (i.e. unused).
> - */
> struct intel_context *
> -intel_context_lookup(struct i915_gem_context *ctx,
> +intel_context_create(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine);
>
> +void intel_context_free(struct intel_context *ce);
> +
> /**
> * intel_context_pin_lock - Stablises the 'pinned' status of the HW context
> * @ctx - the parent GEM context
> @@ -59,17 +51,6 @@ intel_context_is_pinned(struct intel_context *ce)
>
> void intel_context_pin_unlock(struct intel_context *ce);
>
> -struct intel_context *
> -__intel_context_insert(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine,
> - struct intel_context *ce);
> -void
> -__intel_context_remove(struct intel_context *ce);
> -
> -struct intel_context *
> -intel_context_instance(struct i915_gem_context *ctx,
> - struct intel_engine_cs *engine);
> -
> int __intel_context_do_pin(struct intel_context *ce);
>
> static inline int intel_context_pin(struct intel_context *ce)
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index f02d27734e3b..3579c2708321 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -10,7 +10,6 @@
> #include <linux/kref.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> -#include <linux/rbtree.h>
> #include <linux/types.h>
>
> #include "i915_active_types.h"
> @@ -61,7 +60,6 @@ struct intel_context {
> struct i915_active_request active_tracker;
>
> const struct intel_context_ops *ops;
> - struct rb_node node;
>
> /** sseu: Control eu/slice partitioning */
> struct intel_sseu sseu;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 3f794bc71958..0df3c5238c04 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -716,7 +716,7 @@ static int pin_context(struct i915_gem_context *ctx,
> struct intel_context *ce;
> int err;
>
> - ce = intel_context_instance(ctx, engine);
> + ce = i915_gem_context_get_engine(ctx, engine->id);
Staying with intel_context_instance wouldn't help to reduce the churn?
> if (IS_ERR(ce))
> return PTR_ERR(ce);
>
> diff --git a/drivers/gpu/drm/i915/gt/mock_engine.c b/drivers/gpu/drm/i915/gt/mock_engine.c
> index 3b672e011cf0..87e538825fee 100644
> --- a/drivers/gpu/drm/i915/gt/mock_engine.c
> +++ b/drivers/gpu/drm/i915/gt/mock_engine.c
> @@ -23,6 +23,7 @@
> */
>
> #include "i915_drv.h"
> +#include "i915_gem_context.h"
> #include "intel_context.h"
> #include "intel_engine_pm.h"
>
> @@ -286,7 +287,7 @@ int mock_engine_init(struct intel_engine_cs *engine)
> i915_timeline_set_subclass(&engine->timeline, TIMELINE_ENGINE);
>
> engine->kernel_context =
> - intel_context_instance(i915->kernel_context, engine);
> + i915_gem_context_get_engine(i915->kernel_context, engine->id);
> if (IS_ERR(engine->kernel_context))
> goto err_timeline;
>
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
> index 606fc2713240..8b6574e1b495 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -1188,7 +1188,7 @@ int intel_vgpu_setup_submission(struct intel_vgpu *vgpu)
> INIT_LIST_HEAD(&s->workload_q_head[i]);
> s->shadow[i] = ERR_PTR(-EINVAL);
>
> - ce = intel_context_instance(ctx, engine);
> + ce = i915_gem_context_get_engine(ctx, i);
> if (IS_ERR(ce)) {
> ret = PTR_ERR(ce);
> goto out_shadow_ctx;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 50266e87c225..21b4a04c424b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4312,8 +4312,9 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>
> static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> {
> - struct i915_gem_context *ctx;
> struct intel_engine_cs *engine;
> + struct i915_gem_context *ctx;
> + struct i915_gem_engines *e;
> enum intel_engine_id id;
> int err = 0;
>
> @@ -4330,18 +4331,26 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> if (IS_ERR(ctx))
> return PTR_ERR(ctx);
>
> + e = i915_gem_context_engine_list_lock(ctx);
> +
> for_each_engine(engine, i915, id) {
Do we need i915 version of for_each_context_engine? If all call sites
will doing this "lock ctx" -> "walk physical engines" -> "lookup in
context" then it seems a bit disconnected.
> + struct intel_context *ce = e->engines[id];
How will index by engine->id work for engine map?
> struct i915_request *rq;
>
> - rq = i915_request_alloc(engine, ctx);
> + err = intel_context_pin(ce);
> + if (err)
> + goto err_active;
> +
> + rq = i915_request_create(ce);
> + intel_context_unpin(ce);
Kind of verbose, no? Do you want to add some
middle-ground-between-request-alloc-and-create helper?
> if (IS_ERR(rq)) {
> err = PTR_ERR(rq);
> - goto out_ctx;
> + goto err_active;
> }
>
> err = 0;
> - if (engine->init_context)
> - err = engine->init_context(rq);
> + if (rq->engine->init_context)
> + err = rq->engine->init_context(rq);
>
> i915_request_add(rq);
> if (err)
> @@ -4355,15 +4364,10 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> }
>
> for_each_engine(engine, i915, id) {
> - struct intel_context *ce;
> - struct i915_vma *state;
> + struct intel_context *ce = e->engines[id];
> + struct i915_vma *state = ce->state;
> void *vaddr;
>
> - ce = intel_context_lookup(ctx, engine);
> - if (!ce)
> - continue;
> -
> - state = ce->state;
> if (!state)
> continue;
>
> @@ -4419,6 +4423,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> }
>
> out_ctx:
> + i915_gem_context_engine_list_unlock(ctx);
> i915_gem_context_set_closed(ctx);
> i915_gem_context_put(ctx);
> return err;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 1e84fe0a4c55..517816558c85 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -150,7 +150,7 @@ lookup_user_engine(struct i915_gem_context *ctx, u16 class, u16 instance)
> if (!engine)
> return ERR_PTR(-EINVAL);
>
> - return intel_context_instance(ctx, engine);
> + return i915_gem_context_get_engine(ctx, engine->id);
> }
>
> static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
> @@ -242,10 +242,54 @@ static void release_hw_id(struct i915_gem_context *ctx)
> mutex_unlock(&i915->contexts.mutex);
> }
>
> -static void i915_gem_context_free(struct i915_gem_context *ctx)
> +static void free_engines(struct i915_gem_engines *e)
> +{
> + unsigned int n;
> +
> + for (n = 0; n < e->num_engines; n++) {
> + if (!e->engines[n])
> + continue;
> +
> + intel_context_put(e->engines[n]);
> + }
> + kfree(e);
> +}
> +
> +static void free_engines_n(struct i915_gem_engines *e, unsigned long n)
> {
> - struct intel_context *it, *n;
> + e->num_engines = n;
> + free_engines(e);
> +}
> +
> +static struct i915_gem_engines *default_engines(struct i915_gem_context *ctx)
> +{
> + struct intel_engine_cs *engine;
> + struct i915_gem_engines *e;
> + enum intel_engine_id id;
> +
> + e = kzalloc(struct_size(e, engines, I915_NUM_ENGINES), GFP_KERNEL);
> + if (!e)
> + return ERR_PTR(-ENOMEM);
> +
> + e->i915 = ctx->i915;
> + for_each_engine(engine, ctx->i915, id) {
> + struct intel_context *ce;
>
> + ce = intel_context_create(ctx, engine);
> + if (IS_ERR(ce)) {
> + free_engines_n(e, id);
I dislike piggy-back of n into e. How about:
__free_engines(e, n)
{
...
}
free_engines(e)
{
__fre_engines(e, e->num_engines):
}
?
Or even you could e->num_engines++ in the above loop and just have one
free_engines.
> + return ERR_CAST(ce);
> + }
> +
> + e->engines[id] = ce;
Each context would have a sparse array of engines, on most platforms.
Would it be workable to instead create a compact array per context, and
just have a per device translation table of idx to engine->id? Or
vice-versa, I can't figure out straight from the bat which one would you
need.
I guess in this scheme you would need some translation as well to
support customised engine maps.. will see if I will spot that later in
the patch.
> + }
> + e->num_engines = id;
> +
> + return e;
> +}
> +
> +static void i915_gem_context_free(struct i915_gem_context *ctx)
> +{
> lockdep_assert_held(&ctx->i915->drm.struct_mutex);
> GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
> GEM_BUG_ON(!list_empty(&ctx->active_engines));
> @@ -253,8 +297,8 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
> release_hw_id(ctx);
> i915_ppgtt_put(ctx->ppgtt);
>
> - rbtree_postorder_for_each_entry_safe(it, n, &ctx->hw_contexts, node)
> - intel_context_put(it);
> + free_engines(rcu_access_pointer(ctx->engines));
> + mutex_destroy(&ctx->engines_mutex);
>
> if (ctx->timeline)
> i915_timeline_put(ctx->timeline);
> @@ -363,6 +407,8 @@ static struct i915_gem_context *
> __create_context(struct drm_i915_private *dev_priv)
> {
> struct i915_gem_context *ctx;
> + struct i915_gem_engines *e;
> + int err;
> int i;
>
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> @@ -376,8 +422,13 @@ __create_context(struct drm_i915_private *dev_priv)
> INIT_LIST_HEAD(&ctx->active_engines);
> mutex_init(&ctx->mutex);
>
> - ctx->hw_contexts = RB_ROOT;
> - spin_lock_init(&ctx->hw_contexts_lock);
> + mutex_init(&ctx->engines_mutex);
> + e = default_engines(ctx);
> + if (IS_ERR(e)) {
> + err = PTR_ERR(e);
> + goto err_free;
> + }
> + RCU_INIT_POINTER(ctx->engines, e);
>
> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> INIT_LIST_HEAD(&ctx->handles_list);
> @@ -399,6 +450,10 @@ __create_context(struct drm_i915_private *dev_priv)
> ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
>
> return ctx;
> +
> +err_free:
> + kfree(ctx);
> + return ERR_PTR(err);
> }
>
> static struct i915_hw_ppgtt *
> @@ -862,7 +917,8 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> {
> struct drm_i915_private *i915 = ctx->i915;
> struct context_barrier_task *cb;
> - struct intel_context *ce, *next;
> + struct i915_gem_engines *e;
> + unsigned int i;
> int err = 0;
>
> lockdep_assert_held(&i915->drm.struct_mutex);
> @@ -875,20 +931,29 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> i915_active_init(i915, &cb->base, cb_retire);
> i915_active_acquire(&cb->base);
>
> - rbtree_postorder_for_each_entry_safe(ce, next, &ctx->hw_contexts, node) {
> - struct intel_engine_cs *engine = ce->engine;
> + e = i915_gem_context_engine_list_lock(ctx);
> + for (i = 0; i < e->num_engines; i++) {
> + struct intel_context *ce = e->engines[i];
> struct i915_request *rq;
>
> - if (!(engine->mask & engines))
> + if (!ce || !(ce->engine->mask & engines))
Definitely looks like a case for for_each_context_engine.
> + continue;
> +
> + if (!intel_context_is_pinned(ce))
> continue;
>
> if (I915_SELFTEST_ONLY(context_barrier_inject_fault &
> - engine->mask)) {
> + ce->engine->mask)) {
> err = -ENXIO;
> break;
> }
>
> - rq = i915_request_alloc(engine, ctx);
> + err = intel_context_pin(ce);
> + if (err)
> + break;
> +
> + rq = i915_request_create(ce);
> + intel_context_unpin(ce);
Yep as mentioned somewhere above. I definitely think another helper
would help code base redability. Even if called unimaginatively as
__i915_request_add.
> if (IS_ERR(rq)) {
> err = PTR_ERR(rq);
> break;
> @@ -904,6 +969,7 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> if (err)
> break;
> }
> + i915_gem_context_engine_list_unlock(ctx);
>
> cb->task = err ? NULL : task; /* caller needs to unwind instead */
> cb->data = data;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 5a8e080499fb..d94fc4c1907c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -176,6 +176,48 @@ static inline void i915_gem_context_put(struct i915_gem_context *ctx)
> kref_put(&ctx->ref, i915_gem_context_release);
> }
>
> +static inline struct i915_gem_engines *
> +i915_gem_context_engine_list(struct i915_gem_context *ctx)
> +{
> + return rcu_dereference_protected(ctx->engines,
> + lockdep_is_held(&ctx->engines_mutex));
> +}
> +
> +static inline struct i915_gem_engines *
> +i915_gem_context_engine_list_lock(struct i915_gem_context *ctx)
> + __acquires(&ctx->engines_mutex)
> +{
> + mutex_lock(&ctx->engines_mutex);
> + return i915_gem_context_engine_list(ctx);
> +}
> +
> +static inline void
> +i915_gem_context_engine_list_unlock(struct i915_gem_context *ctx)
> + __releases(&ctx->engines_mutex)
> +{
> + mutex_unlock(&ctx->engines_mutex);
> +}
> +
> +static inline struct intel_context *
> +i915_gem_context_lookup_engine(struct i915_gem_context *ctx, unsigned long idx)
> +{
> + return i915_gem_context_engine_list(ctx)->engines[idx];
> +}
> +
> +static inline struct intel_context *
> +i915_gem_context_get_engine(struct i915_gem_context *ctx, unsigned long idx)
> +{
> + struct intel_context *ce = ERR_PTR(-EINVAL);
> +
> + rcu_read_lock(); {
> + struct i915_gem_engines *e = rcu_dereference(ctx->engines);
> + if (likely(idx < e->num_engines && e->engines[idx]))
> + ce = intel_context_get(e->engines[idx]);
> + } rcu_read_unlock();
> +
> + return ce;
> +}
> +
> struct i915_lut_handle *i915_lut_handle_alloc(void);
> void i915_lut_handle_free(struct i915_lut_handle *lut);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context_types.h b/drivers/gpu/drm/i915/i915_gem_context_types.h
> index d282a6ab3b9f..1610a74b0283 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context_types.h
> @@ -29,6 +29,13 @@ struct i915_hw_ppgtt;
> struct i915_timeline;
> struct intel_ring;
>
> +struct i915_gem_engines {
> + struct rcu_work rcu;
> + struct drm_i915_private *i915;
> + unsigned long num_engines;
unsigned int?
> + struct intel_context *engines[];
> +};
> +
> /**
> * struct i915_gem_context - client state
> *
> @@ -42,6 +49,21 @@ struct i915_gem_context {
> /** file_priv: owning file descriptor */
> struct drm_i915_file_private *file_priv;
>
> + /**
> + * @engines: User defined engines for this context
> + *
> + * NULL means to use legacy definitions (including random meaning of
> + * I915_EXEC_BSD with I915_EXEC_BSD_SELECTOR overrides).
> + *
> + * If defined, execbuf uses the I915_EXEC_MASK as an index into
> + * array, and various uAPI other the ability to lookup up an
> + * index from this array to select an engine operate on.
> + *
> + * User defined by I915_CONTEXT_PARAM_ENGINE.
> + */
> + struct i915_gem_engines __rcu *engines;
> + struct mutex engines_mutex; /* guards writes to engines */
> +
> struct i915_timeline *timeline;
>
> /**
> @@ -134,10 +156,6 @@ struct i915_gem_context {
>
> struct i915_sched_attr sched;
>
> - /** hw_contexts: per-engine logical HW state */
> - struct rb_root hw_contexts;
> - spinlock_t hw_contexts_lock;
> -
> /** ring_size: size for allocating the per-engine ring buffer */
> u32 ring_size;
> /** desc_template: invariant fields for the HW context descriptor */
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index c700cbc2f594..941192c60832 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2077,9 +2077,7 @@ gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
> return file_priv->bsd_engine;
> }
>
> -#define I915_USER_RINGS (4)
> -
> -static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> +static const enum intel_engine_id user_ring_map[] = {
> [I915_EXEC_DEFAULT] = RCS0,
> [I915_EXEC_RENDER] = RCS0,
> [I915_EXEC_BLT] = BCS0,
> @@ -2087,10 +2085,8 @@ static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
> [I915_EXEC_VEBOX] = VECS0
> };
>
> -static int eb_pin_context(struct i915_execbuffer *eb,
> - struct intel_engine_cs *engine)
> +static int eb_pin_context(struct i915_execbuffer *eb, struct intel_context *ce)
> {
> - struct intel_context *ce;
> int err;
>
> /*
> @@ -2101,21 +2097,16 @@ static int eb_pin_context(struct i915_execbuffer *eb,
> if (err)
> return err;
>
> - ce = intel_context_instance(eb->gem_context, engine);
> - if (IS_ERR(ce))
> - return PTR_ERR(ce);
> -
> /*
> * Pinning the contexts may generate requests in order to acquire
> * GGTT space, so do this first before we reserve a seqno for
> * ourselves.
> */
> err = intel_context_pin(ce);
> - intel_context_put(ce);
> if (err)
> return err;
>
> - eb->engine = engine;
> + eb->engine = ce->engine;
> eb->context = ce;
> return 0;
> }
> @@ -2125,24 +2116,18 @@ static void eb_unpin_context(struct i915_execbuffer *eb)
> intel_context_unpin(eb->context);
> }
>
> -static int
> -eb_select_engine(struct i915_execbuffer *eb,
> - struct drm_file *file,
> - struct drm_i915_gem_execbuffer2 *args)
> +static unsigned int
> +eb_select_legacy_ring(struct i915_execbuffer *eb,
> + struct drm_file *file,
> + struct drm_i915_gem_execbuffer2 *args)
> {
> unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
> - struct intel_engine_cs *engine;
> -
> - if (user_ring_id > I915_USER_RINGS) {
> - DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
> - return -EINVAL;
> - }
>
> - if ((user_ring_id != I915_EXEC_BSD) &&
> - ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
> + if (user_ring_id != I915_EXEC_BSD &&
> + (args->flags & I915_EXEC_BSD_MASK)) {
> DRM_DEBUG("execbuf with non bsd ring but with invalid "
> "bsd dispatch flags: %d\n", (int)(args->flags));
> - return -EINVAL;
> + return -1;
> }
>
> if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(eb->i915, VCS1)) {
> @@ -2157,20 +2142,39 @@ eb_select_engine(struct i915_execbuffer *eb,
> } else {
> DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
> bsd_idx);
> - return -EINVAL;
> + return -1;
> }
>
> - engine = eb->i915->engine[_VCS(bsd_idx)];
> - } else {
> - engine = eb->i915->engine[user_ring_map[user_ring_id]];
> + return _VCS(bsd_idx);
> }
>
> - if (!engine) {
> - DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
> - return -EINVAL;
> + if (user_ring_id >= ARRAY_SIZE(user_ring_map)) {
> + DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
> + return -1;
> }
>
> - return eb_pin_context(eb, engine);
> + return user_ring_map[user_ring_id];
> +}
> +
> +static int
> +eb_select_engine(struct i915_execbuffer *eb,
> + struct drm_file *file,
> + struct drm_i915_gem_execbuffer2 *args)
> +{
> + struct intel_context *ce;
> + unsigned int idx;
> + int err;
> +
> + idx = eb_select_legacy_ring(eb, file, args);
> +
> + ce = i915_gem_context_get_engine(eb->gem_context, idx);
> + if (IS_ERR(ce))
> + return PTR_ERR(ce);
> +
> + err = eb_pin_context(eb, ce);
> + intel_context_put(ce);
> +
> + return err;
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index afaeabe5e531..304e597e19e6 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
I leave the rest for a second pass.
Regards,
Tvrtko
> @@ -1203,35 +1203,37 @@ static int i915_oa_read(struct i915_perf_stream *stream,
> static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
> struct i915_gem_context *ctx)
> {
> - struct intel_engine_cs *engine = i915->engine[RCS0];
> - struct intel_context *ce;
> + struct i915_gem_engines *e;
> + unsigned int i;
> int err;
>
> - ce = intel_context_instance(ctx, engine);
> - if (IS_ERR(ce))
> - return ce;
> -
> err = i915_mutex_lock_interruptible(&i915->drm);
> - if (err) {
> - intel_context_put(ce);
> + if (err)
> return ERR_PTR(err);
> - }
>
> - /*
> - * As the ID is the gtt offset of the context's vma we
> - * pin the vma to ensure the ID remains fixed.
> - *
> - * NB: implied RCS engine...
> - */
> - err = intel_context_pin(ce);
> + e = i915_gem_context_engine_list_lock(ctx);
> + for (i = 0; i < e->num_engines; i++) {
> + struct intel_context *ce = e->engines[i];
> +
> + if (ce->engine->class != RENDER_CLASS)
> + continue;
> +
> + /*
> + * As the ID is the gtt offset of the context's vma we
> + * pin the vma to ensure the ID remains fixed.
> + */
> + err = intel_context_pin(ce);
> + if (err == 0) {
> + i915->perf.oa.pinned_ctx = ce;
> + break;
> + }
> + }
> + i915_gem_context_engine_list_unlock(ctx);
> mutex_unlock(&i915->drm.struct_mutex);
> - intel_context_put(ce);
> if (err)
> return ERR_PTR(err);
>
> - i915->perf.oa.pinned_ctx = ce;
> -
> - return ce;
> + return i915->perf.oa.pinned_ctx;
> }
>
> /**
> @@ -1717,10 +1719,10 @@ gen8_update_reg_state_unlocked(struct intel_context *ce,
> static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
> const struct i915_oa_config *oa_config)
> {
> - struct intel_engine_cs *engine = dev_priv->engine[RCS0];
> unsigned int map_type = i915_coherent_map_type(dev_priv);
> struct i915_gem_context *ctx;
> struct i915_request *rq;
> + unsigned int i;
> int ret;
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
> @@ -1746,30 +1748,41 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>
> /* Update all contexts now that we've stalled the submission. */
> list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
> - struct intel_context *ce = intel_context_lookup(ctx, engine);
> - u32 *regs;
> + struct i915_gem_engines *e =
> + i915_gem_context_engine_list_lock(ctx);
>
> - /* OA settings will be set upon first use */
> - if (!ce || !ce->state)
> - continue;
> + for (i = 0; i < e->num_engines; i++) {
> + struct intel_context *ce = e->engines[i];
> + u32 *regs;
> +
> + if (!ce || ce->engine->class != RENDER_CLASS)
> + continue;
> +
> + /* OA settings will be set upon first use */
> + if (!ce->state)
> + continue;
>
> - regs = i915_gem_object_pin_map(ce->state->obj, map_type);
> - if (IS_ERR(regs))
> - return PTR_ERR(regs);
> + regs = i915_gem_object_pin_map(ce->state->obj,
> + map_type);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
>
> - ce->state->obj->mm.dirty = true;
> - regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
> + ce->state->obj->mm.dirty = true;
> + regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
>
> - gen8_update_reg_state_unlocked(ce, regs, oa_config);
> + gen8_update_reg_state_unlocked(ce, regs, oa_config);
> +
> + i915_gem_object_unpin_map(ce->state->obj);
> + }
>
> - i915_gem_object_unpin_map(ce->state->obj);
> + i915_gem_context_engine_list_unlock(ctx);
> }
>
> /*
> * Apply the configuration by doing one context restore of the edited
> * context image.
> */
> - rq = i915_request_create(engine->kernel_context);
> + rq = i915_request_create(dev_priv->engine[RCS0]->kernel_context);
> if (IS_ERR(rq))
> return PTR_ERR(rq);
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 8abd891d9287..5e52c1623c17 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -767,7 +767,7 @@ i915_request_alloc(struct intel_engine_cs *engine, struct i915_gem_context *ctx)
> * GGTT space, so do this first before we reserve a seqno for
> * ourselves.
> */
> - ce = intel_context_instance(ctx, engine);
> + ce = i915_gem_context_get_engine(ctx, engine->id);
> if (IS_ERR(ce))
> return ERR_CAST(ce);
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 7fbfcb3d63e0..ae230b38138a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -364,11 +364,10 @@ static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
> static void guc_stage_desc_init(struct intel_guc_client *client)
> {
> struct intel_guc *guc = client->guc;
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> - struct intel_engine_cs *engine;
> struct i915_gem_context *ctx = client->owner;
> + struct i915_gem_engines *e;
> struct guc_stage_desc *desc;
> - unsigned int tmp;
> + unsigned int i;
> u32 gfx_addr;
>
> desc = __get_stage_desc(client);
> @@ -382,10 +381,13 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> desc->priority = client->priority;
> desc->db_id = client->doorbell_id;
>
> - for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
> - struct intel_context *ce = intel_context_lookup(ctx, engine);
> - u32 guc_engine_id = engine->guc_id;
> - struct guc_execlist_context *lrc = &desc->lrc[guc_engine_id];
> + e = i915_gem_context_engine_list_lock(ctx);
> + for (i = 0; i < e->num_engines; i++) {
> + struct intel_context *ce = e->engines[i];
> + struct guc_execlist_context *lrc;
> +
> + if (!ce || !(ce->engine->mask & client->engines))
> + continue;
>
> /* TODO: We have a design issue to be solved here. Only when we
> * receive the first batch, we know which engine is used by the
> @@ -394,7 +396,7 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> * for now who owns a GuC client. But for future owner of GuC
> * client, need to make sure lrc is pinned prior to enter here.
> */
> - if (!ce || !ce->state)
> + if (!ce->state)
> break; /* XXX: continue? */
>
> /*
> @@ -404,6 +406,7 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> * Instead, the GuC uses the LRCA of the user mode context (see
> * guc_add_request below).
> */
> + lrc = &desc->lrc[ce->engine->guc_id];
> lrc->context_desc = lower_32_bits(ce->lrc_desc);
>
> /* The state page is after PPHWSP */
> @@ -414,15 +417,16 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> * here. In proxy submission, it wants the stage id
> */
> lrc->context_id = (client->stage_id << GUC_ELC_CTXID_OFFSET) |
> - (guc_engine_id << GUC_ELC_ENGINE_OFFSET);
> + (ce->engine->guc_id << GUC_ELC_ENGINE_OFFSET);
>
> lrc->ring_begin = intel_guc_ggtt_offset(guc, ce->ring->vma);
> lrc->ring_end = lrc->ring_begin + ce->ring->size - 1;
> lrc->ring_next_free_location = lrc->ring_begin;
> lrc->ring_current_tail_pointer_value = 0;
>
> - desc->engines_used |= (1 << guc_engine_id);
> + desc->engines_used |= BIT(ce->engine->guc_id);
> }
> + i915_gem_context_engine_list_unlock(ctx);
>
> DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
> client->engines, desc->engines_used);
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 214d1fd2f4dc..7fd224a4ca4c 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -1094,7 +1094,7 @@ __igt_ctx_sseu(struct drm_i915_private *i915,
>
> wakeref = intel_runtime_pm_get(i915);
>
> - ce = intel_context_instance(ctx, i915->engine[RCS0]);
> + ce = i915_gem_context_get_engine(ctx, RCS0);
> if (IS_ERR(ce)) {
> ret = PTR_ERR(ce);
> goto out_rpm;
> diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
> index 0426093bf1d9..71c750693585 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_context.c
> @@ -30,6 +30,7 @@ mock_context(struct drm_i915_private *i915,
> const char *name)
> {
> struct i915_gem_context *ctx;
> + struct i915_gem_engines *e;
> int ret;
>
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> @@ -40,8 +41,11 @@ mock_context(struct drm_i915_private *i915,
> INIT_LIST_HEAD(&ctx->link);
> ctx->i915 = i915;
>
> - ctx->hw_contexts = RB_ROOT;
> - spin_lock_init(&ctx->hw_contexts_lock);
> + mutex_init(&ctx->engines_mutex);
> + e = default_engines(ctx);
> + if (IS_ERR(e))
> + goto err_free;
> + RCU_INIT_POINTER(ctx->engines, e);
>
> INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> INIT_LIST_HEAD(&ctx->handles_list);
> @@ -51,7 +55,7 @@ mock_context(struct drm_i915_private *i915,
>
> ret = i915_gem_context_pin_hw_id(ctx);
> if (ret < 0)
> - goto err_handles;
> + goto err_engines;
>
> if (name) {
> struct i915_hw_ppgtt *ppgtt;
> @@ -69,7 +73,9 @@ mock_context(struct drm_i915_private *i915,
>
> return ctx;
>
> -err_handles:
> +err_engines:
> + free_engines(rcu_access_pointer(ctx->engines));
> +err_free:
> kfree(ctx);
> return NULL;
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v1 3/3] arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes
From: Stanimir Varbanov @ 2019-04-10 15:32 UTC (permalink / raw)
To: Marc Gonzalez, Bjorn Andersson
Cc: Jeffrey Hugo, Vivek Gautam, Manu Gautam, Evan Green,
Douglas Anderson, Robin Murphy, Lorenzo Pieralisi, Joerg Roedel,
Stanimir Varbanov, Srinivas Kandagatla, Bjorn Helgaas, MSM, PCI,
iommu
In-Reply-To: <186f7ca2-84e2-a37e-79e6-f1fec8e25374@free.fr>
Hi Marc,
Few comments inline.
On 3/28/19 7:06 PM, Marc Gonzalez wrote:
> Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes.
>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++
> 1 file changed, 78 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 5a1c0961b281..9f979a51f679 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -621,6 +621,84 @@
> <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>;
> };
>
> + pcie0: pci@1c00000 {
> + compatible = "qcom,pcie-msm8996";
> + reg-names = "parf", "dbi", "elbi", "config";
> + reg = <0x01c00000 0x2000>,
> + <0x1b000000 0xf1d>,
> + <0x1b000f20 0xa8>,
> + <0x1b100000 0x100000>;
could you please populate reg property and after that reg-names property.
> + device_type = "pci";
> + linux,pci-domain = <0>;
> + bus-range = <0x00 0xff>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + power-domains = <&gcc PCIE_0_GDSC>;
> +
> + num-lanes = <1>;
> + phy-names = "pciephy";
> + phys = <&pciephy>;
> +
> + ranges =
> + /*** downstream I/O ***/
could you make this a standard kernel comment or completely drop it
> + <0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>,
> + /*** non-prefetchable memory ***/
ditto
> + <0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>;
> +
> + #interrupt-cells = <1>;
> + interrupt-names = "msi";
> + interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-map-mask = <0 0 0 0x7>;
> + interrupt-map =
> + <0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
move this to a line upper
> + <0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> + <0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> + <0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> + clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux";
> + clocks =
> + <&gcc GCC_PCIE_0_PIPE_CLK>,
move this to a line upper
> + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
> + <&gcc GCC_PCIE_0_SLV_AXI_CLK>,
> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> + <&gcc GCC_PCIE_0_AUX_CLK>;
please swap the order clocks and clock-names
> +
> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>;
iommu-map-mask? It is optional but I had to ask :)
> +
> + /* PCIe Fundamental Reset */
this comment is useless :) please drop it
> + perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
> + };
> +
> + phy@1c06000 {
> + compatible = "qcom,msm8998-qmp-pcie-phy";
> + reg = <0x01c06000 0x18c>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + clock-names = "aux", "cfg_ahb", "ref";
> + clocks =
> + <&gcc GCC_PCIE_PHY_AUX_CLK>,
> + <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> + <&gcc GCC_PCIE_CLKREF_CLK>;\
please, swap the order of clocks and clock-names, and move first clock a
line upper. Also delete '\' symbol at the end of last line.
> +
> + reset-names = "phy", "common";
> + resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>;
resets prop and after that reset-names, please.
> +
> + vdda-phy-supply = <&vreg_l1a_0p875>;
> + vdda-pll-supply = <&vreg_l2a_1p2>;
> +
> + pciephy: lane@1c06800 {
> + reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>;
> + #phy-cells = <0>;
> +
> + clock-names = "pipe0";
> + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>;
please, swap clocks and clock-names
> + clock-output-names = "pcie_0_pipe_clk_src";
> + #clock-cells = <0>;
> + };
> + };
> +
> tcsr_mutex_regs: syscon@1f40000 {
> compatible = "syscon";
> reg = <0x1f40000 0x20000>;
>
--
regards,
Stan
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: firmware: imx-scu: new binding to parse clocks from device tree
From: Rob Herring @ 2019-04-10 15:32 UTC (permalink / raw)
To: Aisheng Dong
Cc: devicetree@vger.kernel.org, sboyd@kernel.org,
mturquette@baylibre.com, dl-linux-imx, kernel@pengutronix.de,
Fabio Estevam, shawnguo@kernel.org, linux-clk@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM0PR04MB42110F1F251243A165DB9EC680580@AM0PR04MB4211.eurprd04.prod.outlook.com>
On Wed, Mar 27, 2019 at 9:35 AM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Rob Herring [mailto:robh@kernel.org]
> > Sent: Tuesday, March 26, 2019 9:47 PM
> > On Thu, Feb 21, 2019 at 06:03:43PM +0000, Aisheng Dong wrote:
> > > There's a few limitations on one cell clock binding (#clock-cells =
> > > <1>) that we have to define all clock IDs for device tree to reference.
> > > This may cause troubles if we want to use common clock IDs for multi
> > > platforms support when the clock of those platforms are mostly the same.
> > > e.g. Current clock IDs name are defined with SS prefix. However the
> > > device may reside in different SS across CPUs, that means the SS
> > > prefix may not valid anymore for a new SoC. Furthermore, the device
> > > availability of those clocks may also vary a bit.
> > >
> > > For such situation, We formerly planned to add all new IDs for each SS
> > > and dynamically check availability for different SoC in driver. That
> > > can be done but that may involve a lot effort and may result in more
> > > changes and duplicated code in driver, also make device tree
> > > upstreaming hard which depends on Clock IDs.
> > >
> > > To relief this situation, we want to move the clock definition into
> > > device tree which can fully decouple the dependency of Clock ID
> > > definition from device tree. And no frequent changes required in clock driver
> > any more.
> > >
> > > Then we can use the existence of clock nodes in device tree to address
> > > the device and clock availability differences across different SoCs.
> > >
> > > For SCU clocks, only two params required, thus two new property created:
> > > rsrc-id = <IMX_SC_R_UART_0>;
> > > clk-type = <IMX_SC_PM_CLK_PER>;
> > >
> > > And as we want to support clock set parent function, 'clocks' property
> > > is also used to pass all the possible input parents.
> > >
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > > .../devicetree/bindings/arm/freescale/fsl,scu.txt | 29
> > ++++++++++++++++------
> > > include/dt-bindings/firmware/imx/rsrc.h | 17
> > +++++++++++++
> > > 2 files changed, 38 insertions(+), 8 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > index 72d481c..2816789 100644
> > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > @@ -78,6 +78,19 @@ Required properties:
> > > "fsl,imx8qm-clock"
> > > "fsl,imx8qxp-clock"
> > > followed by "fsl,scu-clk"
> > > +- #clock-cells: Should be 0.
> > > +- rsrc-id: Resource ID associated with this clock
> > > +- clk-type: Type of this clock.
> > > + Refer to <include/dt-bindings/firmware/imx/rsrc.h> for
> > > + available clock types supported by SCU.
> >
> > Can't you just make these 2 values clock cells? I'm all for getting rid of made
> > up clock numbers.
> >
>
> Thanks for the agreement to remove clock IDs.
>
> The 2 values clock cell seems not the best approach for i.MX because it still needs
> to define all clocks in the driver which is exactly we want to avoid now due to some
> special HW characteristic:
Why's that? You can walk the DT and extract the 2 cells for each clock
present. That's not any different than walking child nodes here and
getting the resource ids and type. That's not really fast, but if
speed is really an issue we can consider addressing that in ways that
extend rather than change the binding.
> 1. clock resources may be allocated to different SW execution partition by firmware
> and A core may not have access rights for those clocks not belong to its partition.
> So we want to describe them in DT according to the partition configuration.
Do you have an example? I'd assume you assign peripherals to different
partitions and resource assignment simply follows that. Can clocks not
be available when a peripheral still is?
> 2. Each clock is associated with a different power domain which is better to be
> described in device tree. And clock state will be lost and need restore after power cycle
> of the domain.
>
> Based on above requirements, do you think we can do as below?
Can you provide an example that shows the whole hierarchy for a
peripheral. Here you have FSPI, PWM, and MMC. Reviewing SCU clocks and
LPCG clocks separately is not helpful.
>
> //LSIO SS
> lsio_scu_clk: lsio-scu-clock-controller {
> compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";
>
> fspi0_clk: clock-fspi0{
> #clock-cells = <0>;
> rsrc-id = <IMX_SC_R_FSPI_0>;
> clk-type = <IMX_SC_PM_CLK_PER>;
> power-domains = <&pd IMX_SC_R_FSPI_0>;
Are the power domain ID and rsrc-id always the same for a clock?
> };
>
> fspi1_clk: clock-fspi1{
> ...
> };
> ...
> };
>
> /* LPCG clocks */
> lsio_lpcg_clk: lsio-lpcg-clock-controller {
> compatible = "fsl,imx8qxp-lpcg";
I think this wrapper node should be removed and the compatible moved
into the child nodes.
> pwm0_lpcg: clock-controller@5d400000 {
> reg = <0x5d400000 0x10000>;
> #clock-cells = <1>;
> clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
> <&lsio_bus_clk>, <&pwm0_clk>;
> bit-offset = <0 4 16 20 24>;
Are all LPCG instances the same, but some clocks are missing if the
child peripheral doesn't use certain clocks? IOW, bit 0 is always
ipg_clk, bit 24 is always ipg_mstr_clk?
Assuming so, 'bit-offset' should be removed and you should either have
a fixed number of clock entries with 0 entries for non-connected
clocks or use clock-names to define which clocks are present (with the
same set of defined names for all LPCG instances).
> clock-output-names = "pwm0_lpcg_ipg_clk",
> "pwm0_lpcg_ipg_hf_clk",
> "pwm0_lpcg_ipg_s_clk",
> "pwm0_lpcg_ipg_slv_clk",
> "pwm0_lpcg_ipg_mstr_clk";
IMO, this is wrong as the names should be relative to the module. So
'ipg_clk', 'ipg_hf_clk', etc.
> power-domains = <&pd IMX_SC_R_PWM_0>;
> status = "disabled";
> };
>
> pwm1_lpcg: clock-controller@5d410000 {
> ...
> }
> ...
> };
>
> And for users, it could simply be:
> usdhc1: mmc@5b010000 {
> clocks = <&sdhc0_lpcg 1>,
> <&sdhc0_lpcg 0>,
> <&sdhc0_lpcg 2>;
> clock-names = "ipg", "per", "ahb";
> assigned-clocks = <&sdhc0_clk>;
> assigned-clock-rates = <200000000>;
> ....
> };
_______________________________________________
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.