* [PATCH v4 0/6] media: rcar-vin: Brush endpoint properties
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
4th round for the VIN endpoint brushing series.
Slightly enlarged the linux-media receiver list, as this new version
introduces a common property in 'video-interfaces.txt'.
Compared to v3 I have dropped the last controversial parts:
- The custom 'renesas,hsync-as-de' property has been dropped: do not handle
CHS bit for the moment.
- Do not remove properties not parsed by the driver and not listed in the
interface bindings from Gen2 boards. As this lead to a long discussion, I
have now proposed a patch to clearly state that properties not listed in the
interface bindings can be optionally specified, but they don't affect the
interface behaviour. To avoid more discussions this patch may be dropped
and things will stay the way they are now.
For the common 'data-enable-active' property, I guess with Rob's ack we should
be fine there and I hope the rest of the series won't slow down its acceptance.
Thanks
j
Jacopo Mondi (6):
dt-bindings: media: rcar-vin: Describe optional ep properties
dt-bindings: media: Document data-enable-active property
media: v4l2-fwnode: parse 'data-enable-active' prop
dt-bindings: media: rcar-vin: Add 'data-enable-active'
media: rcar-vin: Handle data-enable polarity
dt-bindings: media: rcar-vin: Clarify optional props
Documentation/devicetree/bindings/media/rcar_vin.txt | 18 ++++++++++++++++++
.../devicetree/bindings/media/video-interfaces.txt | 2 ++
drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++
drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++++
include/media/v4l2-mediabus.h | 2 ++
5 files changed, 31 insertions(+)
--
2.7.4
^ permalink raw reply
* [PATCH v4 1/6] dt-bindings: media: rcar-vin: Describe optional ep properties
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-1-git-send-email-jacopo+renesas@jmondi.org>
Describe the optional endpoint properties for endpoint nodes of port at 0
and port at 1 of the R-Car VIN driver device tree bindings documentation.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/media/rcar_vin.txt | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index a19517e1..87f93ec 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -53,6 +53,14 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
from external SoC pins described in video-interfaces.txt[1].
Describing more then one endpoint in port 0 is invalid. Only VIN
instances that are connected to external pins should have port 0.
+
+ - Optional properties for endpoint nodes of port at 0:
+ - hsync-active: see [1] for description. Default is active high.
+ - vsync-active: see [1] for description. Default is active high.
+
+ If both HSYNC and VSYNC polarities are not specified, embedded
+ synchronization is selected.
+
- port 1 - sub-nodes describing one or more endpoints connected to
the VIN from local SoC CSI-2 receivers. The endpoint numbers must
use the following schema.
@@ -62,6 +70,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
- Endpoint 2 - sub-node describing the endpoint connected to CSI40
- Endpoint 3 - sub-node describing the endpoint connected to CSI41
+ Endpoint nodes of port at 1 do not support any optional endpoint property.
+
Device node example for Gen2 platforms
--------------------------------------
--
2.7.4
^ permalink raw reply related
* [PATCH v4 2/6] dt-bindings: media: Document data-enable-active property
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-1-git-send-email-jacopo+renesas@jmondi.org>
Add 'data-enable-active' property to endpoint node properties list.
The property allows to specify the polarity of the data-enable signal, which
when in active state determinates when data lines have to sampled for valid
pixel data.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 258b8df..9839d26 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -109,6 +109,8 @@ Optional endpoint properties
Note, that if HSYNC and VSYNC polarities are not specified, embedded
synchronization may be required, where supported.
- data-active: similar to HSYNC and VSYNC, specifies data line polarity.
+- data-enable-active: similar to HSYNC and VSYNC, specifies the data enable
+ signal polarity.
- field-even-active: field signal level during the even field data transmission.
- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
signal.
--
2.7.4
^ permalink raw reply related
* [PATCH v4 3/6] media: v4l2-fwnode: parse 'data-enable-active' prop
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-1-git-send-email-jacopo+renesas@jmondi.org>
Parse the newly defined 'data-enable-active' property in parallel endpoint
parsing function.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
---
drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++++
include/media/v4l2-mediabus.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3f77aa3..6105191 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -154,6 +154,10 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
flags |= v ? V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH :
V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW;
+ if (!fwnode_property_read_u32(fwnode, "data-enable-active", &v))
+ flags |= v ? V4L2_MBUS_DATA_ENABLE_HIGH :
+ V4L2_MBUS_DATA_ENABLE_LOW;
+
bus->flags = flags;
}
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 4d8626c..4bbb5f3 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -45,6 +45,8 @@
/* Active state of Sync-on-green (SoG) signal, 0/1 for LOW/HIGH respectively. */
#define V4L2_MBUS_VIDEO_SOG_ACTIVE_HIGH BIT(12)
#define V4L2_MBUS_VIDEO_SOG_ACTIVE_LOW BIT(13)
+#define V4L2_MBUS_DATA_ENABLE_HIGH BIT(14)
+#define V4L2_MBUS_DATA_ENABLE_LOW BIT(15)
/* Serial flags */
/* How many lanes the client can use */
--
2.7.4
^ permalink raw reply related
* [PATCH v4 4/6] dt-bindings: media: rcar-vin: Add 'data-enable-active'
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-1-git-send-email-jacopo+renesas@jmondi.org>
Describe optional endpoint property 'data-enable-active' for R-Car VIN.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Niklas S?derlund <niklas.soderlund+renesas at ragnatech.se
---
Documentation/devicetree/bindings/media/rcar_vin.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 87f93ec..8130849 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -57,6 +57,8 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
- Optional properties for endpoint nodes of port@0:
- hsync-active: see [1] for description. Default is active high.
- vsync-active: see [1] for description. Default is active high.
+ - data-enable-active: polarity of CLKENB signal, see [1] for
+ description. Default is active high.
If both HSYNC and VSYNC polarities are not specified, embedded
synchronization is selected.
--
2.7.4
^ permalink raw reply related
* [PATCH v4 5/6] media: rcar-vin: Handle data-enable polarity
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-1-git-send-email-jacopo+renesas@jmondi.org>
Handle data-enable signal polarity. If the polarity is not specifically
requested to be active low, use the active high default.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
---
drivers/media/platform/rcar-vin/rcar-dma.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index d2b7002..9145b56 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -123,6 +123,7 @@
/* Video n Data Mode Register 2 bits */
#define VNDMR2_VPS (1 << 30)
#define VNDMR2_HPS (1 << 29)
+#define VNDMR2_CES (1 << 28)
#define VNDMR2_FTEV (1 << 17)
#define VNDMR2_VLV(n) ((n & 0xf) << 12)
@@ -698,6 +699,10 @@ static int rvin_setup(struct rvin_dev *vin)
/* Vsync Signal Polarity Select */
if (!(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
dmr2 |= VNDMR2_VPS;
+
+ /* Data Enable Polarity Select */
+ if (vin->parallel->mbus_flags & V4L2_MBUS_DATA_ENABLE_LOW)
+ dmr2 |= VNDMR2_CES;
}
/*
--
2.7.4
^ permalink raw reply related
* [PATCH v4 6/6] dt-bindings: media: rcar-vin: Clarify optional props
From: Jacopo Mondi @ 2018-06-12 14:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-1-git-send-email-jacopo+renesas@jmondi.org>
Add a note to the R-Car VIN interface bindings to clarify that all
properties listed as generic properties in video-interfaces.txt can
be included in port at 0 endpoint, but if not explicitly listed in the
interface bindings documentation, they do not modify it behaviour.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 8130849..03544c7 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
instances that are connected to external pins should have port 0.
- Optional properties for endpoint nodes of port at 0:
+
+ All properties described in [1] and which apply to the selected
+ media bus type could be optionally listed here to better describe
+ the current hardware configuration, but only the following ones do
+ actually modify the VIN interface behaviour:
+
- hsync-active: see [1] for description. Default is active high.
- vsync-active: see [1] for description. Default is active high.
- data-enable-active: polarity of CLKENB signal, see [1] for
--
2.7.4
^ permalink raw reply related
* [v3, 00/10] Support DPAA PTP clock and timestamping
From: Madalin-cristian Bucur @ 2018-06-12 14:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180607092050.46128-1-yangbo.lu@nxp.com>
> -----Original Message-----
> From: Yangbo Lu [mailto:yangbo.lu at nxp.com]
> Sent: Thursday, June 7, 2018 12:21 PM
> To: netdev at vger.kernel.org; Madalin-cristian Bucur
> <madalin.bucur@nxp.com>; Richard Cochran <richardcochran@gmail.com>;
> Rob Herring <robh+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>;
> David S . Miller <davem@davemloft.net>
> Cc: devicetree at vger.kernel.org; linuxppc-dev at lists.ozlabs.org; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Y.b. Lu
> <yangbo.lu@nxp.com>
> Subject: [v3, 00/10] Support DPAA PTP clock and timestamping
>
> This patchset is to support DPAA FMAN PTP clock and HW timestamping.
> It had been verified on both ARM platform and PPC platform.
> - The patch #1 to patch #5 are to support DPAA FMAN 1588 timer in
> ptp_qoriq driver.
> - The patch #6 to patch #10 are to add HW timestamping support in
> DPAA ethernet driver.
>
> Yangbo Lu (10):
> fsl/fman: share the event interrupt
> ptp: support DPAA FMan 1588 timer in ptp_qoriq
> dt-binding: ptp_qoriq: add DPAA FMan support
> powerpc/mpc85xx: move ptp timer out of fman in dts
> arm64: dts: fsl: move ptp timer out of fman
> fsl/fman: add set_tstamp interface
> fsl/fman_port: support getting timestamp
> fsl/fman: define frame description command UPD
> dpaa_eth: add support for hardware timestamping
> dpaa_eth: add the get_ts_info interface for ethtool
>
> Documentation/devicetree/bindings/net/fsl-fman.txt | 25 +-----
> .../devicetree/bindings/ptp/ptp-qoriq.txt | 15 +++-
> arch/arm64/boot/dts/freescale/qoriq-fman3-0.dtsi | 14 ++-
> arch/powerpc/boot/dts/fsl/qoriq-fman-0.dtsi | 14 ++-
> arch/powerpc/boot/dts/fsl/qoriq-fman-1.dtsi | 14 ++-
> arch/powerpc/boot/dts/fsl/qoriq-fman3-0.dtsi | 14 ++-
> arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi | 14 ++-
> arch/powerpc/boot/dts/fsl/qoriq-fman3l-0.dtsi | 14 ++-
> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 88
> ++++++++++++++++-
> drivers/net/ethernet/freescale/dpaa/dpaa_eth.h | 3 +
> drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c | 39 ++++++++
> drivers/net/ethernet/freescale/fman/fman.c | 3 +-
> drivers/net/ethernet/freescale/fman/fman.h | 1 +
> drivers/net/ethernet/freescale/fman/fman_dtsec.c | 27 +++++
> drivers/net/ethernet/freescale/fman/fman_dtsec.h | 1 +
> drivers/net/ethernet/freescale/fman/fman_memac.c | 5 +
> drivers/net/ethernet/freescale/fman/fman_memac.h | 1 +
> drivers/net/ethernet/freescale/fman/fman_port.c | 12 +++
> drivers/net/ethernet/freescale/fman/fman_port.h | 2 +
> drivers/net/ethernet/freescale/fman/fman_tgec.c | 21 ++++
> drivers/net/ethernet/freescale/fman/fman_tgec.h | 1 +
> drivers/net/ethernet/freescale/fman/mac.c | 3 +
> drivers/net/ethernet/freescale/fman/mac.h | 1 +
> drivers/ptp/Kconfig | 2 +-
> drivers/ptp/ptp_qoriq.c | 104 ++++++++++++-------
> include/linux/fsl/ptp_qoriq.h | 38 ++++++--
> 26 files changed, 361 insertions(+), 115 deletions(-)
Acked-by: Madalin Bucur <madalin.bucur@nxp.com>
^ permalink raw reply
* [PATCH v7 3/6] kernel/reboot.c: export pm_power_off_prepare
From: Rafael J. Wysocki @ 2018-06-12 14:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <daba73df-037c-2583-3a08-f3f27c4129d1@pengutronix.de>
On Tuesday, June 12, 2018 2:42:12 PM CEST Oleksij Rempel wrote:
> This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
> --Sj2PRcQlY7eZybdA0sq9wWzJEO8fKS924
> Content-Type: multipart/mixed; boundary="d6BZYFRi4L3iCmOh3nm6wjii3dWC9QFDg";
> protected-headers="v1"
> From: Oleksij Rempel <o.rempel@pengutronix.de>
> To: Shawn Guo <shawnguo@kernel.org>, Mark Brown <broonie@kernel.org>,
> "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: kernel at pengutronix.de, devicetree at vger.kernel.org,
> linux-arm-kernel at lists.infradead.org, linux-clk at vger.kernel.org,
> linux-kernel at vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,
> Liam Girdwood <lgirdwood@gmail.com>,
> Leonard Crestez <leonard.crestez@nxp.com>, Rob Herring <robh+dt@kernel.org>,
> Mark Rutland <mark.rutland@arm.com>,
> Michael Turquette <mturquette@baylibre.com>,
> Stephen Boyd <sboyd@codeaurora.org>, Fabio Estevam <fabio.estevam@nxp.com>,
> Russell King <linux@armlinux.org.uk>
> Message-ID: <daba73df-037c-2583-3a08-f3f27c4129d1@pengutronix.de>
> Subject: Re: [PATCH v7 3/6] kernel/reboot.c: export pm_power_off_prepare
> References: <20180517055014.6607-1-o.rempel@pengutronix.de>
> <20180517055014.6607-4-o.rempel@pengutronix.de>
> In-Reply-To: <20180517055014.6607-4-o.rempel@pengutronix.de>
>
> --d6BZYFRi4L3iCmOh3nm6wjii3dWC9QFDg
> Content-Type: text/plain; charset=utf-8
> Content-Language: en-US
> Content-Transfer-Encoding: quoted-printable
>
> Hi Rafael,
>
> Last version of this patch was send at 17.05.2018. No other comment was
> provided and this patch is a blocker for other patches in this serie.
> Can you please give some feedback on it.
I would have done that had I not missed the patch.
Which probably wouldn't have happened had you CCed it to linux-pm.
Anyway, I have no particular problems with exporting pm_power_off_prepare via
EXPORT_SYMBOL_GPL().
>
> On 17.05.2018 07:50, Oleksij Rempel wrote:
> > Export pm_power_off_prepare. It is needed to implement power off on
> > Freescale/NXP iMX6 based boards with external power management
> > integrated circuit (PMIC).
> >=20
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> > kernel/reboot.c | 1 +
> > 1 file changed, 1 insertion(+)
> >=20
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index e4ced883d8de..83810d726f3e 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -49,6 +49,7 @@ int reboot_force;
> > */
> > =20
> > void (*pm_power_off_prepare)(void);
> > +EXPORT_SYMBOL_GPL(pm_power_off_prepare);
> > =20
> > /**
> > * emergency_restart - reboot the system
> >=20
>
>
> --d6BZYFRi4L3iCmOh3nm6wjii3dWC9QFDg--
>
> --Sj2PRcQlY7eZybdA0sq9wWzJEO8fKS924
> Content-Type: application/pgp-signature; name="signature.asc"
> Content-Description: OpenPGP digital signature
> Content-Disposition: attachment; filename="signature.asc"
>
>
> --Sj2PRcQlY7eZybdA0sq9wWzJEO8fKS924--
>
^ permalink raw reply
* [PATCH] drm/meson: Fix an un-handled error path in 'meson_drv_bind_master()'
From: Neil Armstrong @ 2018-06-12 14:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180611165335.24542-1-christophe.jaillet@wanadoo.fr>
On 11/06/2018 18:53, Christophe JAILLET wrote:
> If 'platform_get_resource_byname()' fails, we should release some resources
> before leaving, as already done in the other error handling path of the
> function.
>
> Fixes: acaa3f13b8dd ("drm/meson: Fix potential NULL dereference in meson_drv_bind_master()")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/gpu/drm/meson/meson_drv.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
> index 32b1a6cdecfc..d3443125e661 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -197,8 +197,10 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> priv->io_base = regs;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hhi");
> - if (!res)
> - return -EINVAL;
> + if (!res) {
> + ret = -EINVAL;
> + goto free_drm;
> + }
> /* Simply ioremap since it may be a shared register zone */
> regs = devm_ioremap(dev, res->start, resource_size(res));
> if (!regs) {
> @@ -215,8 +217,10 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
> }
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dmc");
> - if (!res)
> - return -EINVAL;
> + if (!res) {
> + ret = -EINVAL;
> + goto free_drm;
> + }
> /* Simply ioremap since it may be a shared register zone */
> regs = devm_ioremap(dev, res->start, resource_size(res));
> if (!regs) {
>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
I'll push it when rc1 is tagged.
Neil
^ permalink raw reply
* [RFC V2 3/3] perf: qcom: Add Falkor CPU PMU IMPLEMENTATION DEFINED event support
From: Mark Rutland @ 2018-06-12 14:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528379808-27970-4-git-send-email-agustinv@codeaurora.org>
Hi,
On Thu, Jun 07, 2018 at 09:56:48AM -0400, Agustin Vega-Frias wrote:
> Selection of these events can be envisioned as indexing them from
> a 3D matrix:
> - the first index selects a Region Event Selection Register (PMRESRx_EL0)
> - the second index selects a group from which only one event at a time
> can be selected
> - the third index selects the event
>
> The event is encoded into perf_event_attr.config as 0xPRCCG, where:
> P [config:16 ] = prefix (flag that indicates a matrix-based event)
> R [config:12-15] = register (specifies the PMRESRx_EL0 instance)
> G [config:0-3 ] = group (specifies the event group)
> CC [config:4-11 ] = code (specifies the event)
>
> Events with the P flag set to zero are treated as common PMUv3 events
> and are directly programmed into PMXEVTYPERx_EL0.
>
> The first two indexes are set combining the RESR and group number with
> a base number and writing it into the architected PMXEVTYPER_EL0 register.
> The third index is set by writing the code into the bits corresponding
> with the group into the appropriate IMPLEMENTATION DEFINED PMRESRx_EL0
> register.
When are the IMP DEF registers accessible at EL0? Are those goverend by
the same controls as the architected registers?
[...]
> +/*
> + * Qualcomm Technologies CPU PMU IMPLEMENTATION DEFINED extensions support
> + *
> + * Current extensions supported:
> + *
> + * - Matrix-based microarchitectural events support
> + *
> + * Selection of these events can be envisioned as indexing them from
> + * a 3D matrix:
> + * - the first index selects a Region Event Selection Register (PMRESRx_EL0)
> + * - the second index selects a group from which only one event at a time
> + * can be selected
> + * - the third index selects the event
> + *
> + * The event is encoded into perf_event_attr.config as 0xPRCCG, where:
> + * P [config:16 ] = prefix (flag that indicates a matrix-based event)
> + * R [config:12-15] = register (specifies the PMRESRx_EL0 instance)
> + * G [config:0-3 ] = group (specifies the event group)
> + * CC [config:4-11 ] = code (specifies the event)
> + *
> + * Events with the P flag set to zero are treated as common PMUv3 events
> + * and are directly programmed into PMXEVTYPERx_EL0.
When PMUv3 is given a raw event code, the config fields should be the
PMU event number, and this conflicts with RESERVED encodings.
I'd rather we used a separate field for the QC extension events. e.g.
turn config1 into a flags field, and move the P flag there.
We *should* add code to sanity check those fields are zero in the PMUv3
driver, even though it's a potential ABI break to start now.
> + *
> + * The first two indexes are set combining the RESR and group number with
> + * a base number and writing it into the architected PMXEVTYPER_EL0 register.
> + * The third index is set by writing the code into the bits corresponding
> + * with the group into the appropriate IMPLEMENTATION DEFINED PMRESRx_EL0
> + * register.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/perf/arm_pmu.h>
You'll also need:
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/perf_event.h>
#include <linux/printk.h>
#include <linux/types.h>
#include <asm/barrier.h>
#include <asm/sysreg.h>
> +
> +#define pmresr0_el0 sys_reg(3, 5, 11, 3, 0)
> +#define pmresr1_el0 sys_reg(3, 5, 11, 3, 2)
> +#define pmresr2_el0 sys_reg(3, 5, 11, 3, 4)
> +#define pmxevcntcr_el0 sys_reg(3, 5, 11, 0, 3)
> +
> +#define QC_EVT_PFX_SHIFT 16
> +#define QC_EVT_REG_SHIFT 12
> +#define QC_EVT_CODE_SHIFT 4
> +#define QC_EVT_GRP_SHIFT 0
> +#define QC_EVT_PFX_MASK GENMASK(QC_EVT_PFX_SHIFT, QC_EVT_PFX_SHIFT)
> +#define QC_EVT_REG_MASK GENMASK(QC_EVT_REG_SHIFT + 3, QC_EVT_REG_SHIFT)
> +#define QC_EVT_CODE_MASK GENMASK(QC_EVT_CODE_SHIFT + 7, QC_EVT_CODE_SHIFT)
> +#define QC_EVT_GRP_MASK GENMASK(QC_EVT_GRP_SHIFT + 3, QC_EVT_GRP_SHIFT)
> +#define QC_EVT_PRG_MASK (QC_EVT_PFX_MASK | QC_EVT_REG_MASK | QC_EVT_GRP_MASK)
> +#define QC_EVT_PRG(event) ((event) & QC_EVT_PRG_MASK)
> +#define QC_EVT_REG(event) (((event) & QC_EVT_REG_MASK) >> QC_EVT_REG_SHIFT)
> +#define QC_EVT_CODE(event) (((event) & QC_EVT_CODE_MASK) >> QC_EVT_CODE_SHIFT)
> +#define QC_EVT_GROUP(event) (((event) & QC_EVT_GRP_MASK) >> QC_EVT_GRP_SHIFT)
> +
> +#define QC_MAX_GROUP 7
> +#define QC_MAX_RESR 2
> +#define QC_BITS_PER_GROUP 8
> +#define QC_RESR_ENABLE BIT_ULL(63)
> +#define QC_RESR_EVT_BASE 0xd8
> +
> +static struct arm_pmu *def_ops;
> +
> +static inline void falkor_write_pmresr(u64 reg, u64 val)
> +{
> + if (reg == 0)
> + write_sysreg_s(val, pmresr0_el0);
> + else if (reg == 1)
> + write_sysreg_s(val, pmresr1_el0);
> + else
> + write_sysreg_s(val, pmresr2_el0);
> +}
> +
> +static inline u64 falkor_read_pmresr(u64 reg)
> +{
> + return (reg == 0 ? read_sysreg_s(pmresr0_el0) :
> + reg == 1 ? read_sysreg_s(pmresr1_el0) :
> + read_sysreg_s(pmresr2_el0));
> +}
Please use switch statements for both of these.
> +
> +static void falkor_set_resr(u64 reg, u64 group, u64 code)
> +{
> + u64 shift = group * QC_BITS_PER_GROUP;
> + u64 mask = GENMASK(shift + QC_BITS_PER_GROUP - 1, shift);
> + u64 val;
> +
> + val = falkor_read_pmresr(reg) & ~mask;
> + val |= (code << shift);
> + val |= QC_RESR_ENABLE;
> + falkor_write_pmresr(reg, val);
> +}
> +
> +static void falkor_clear_resr(u64 reg, u64 group)
> +{
> + u32 shift = group * QC_BITS_PER_GROUP;
> + u64 mask = GENMASK(shift + QC_BITS_PER_GROUP - 1, shift);
> + u64 val = falkor_read_pmresr(reg) & ~mask;
> +
> + falkor_write_pmresr(reg, val == QC_RESR_ENABLE ? 0 : val);
> +}
> +
> +/*
> + * Check if e1 and e2 conflict with each other
> + *
> + * e1 is a matrix-based microarchitectural event we are checking against e2.
> + * A conflict exists if the events use the same reg, group, and a different
> + * code. Events with the same code are allowed because they could be using
> + * different filters (e.g. one to count user space and the other to count
> + * kernel space events).
> + */
What problem occurs when there's a conflict?
Does the filter matter at all? When happens if I open two identical
events, both counting the same reg, group, and code, with the same
filter?
> +static inline int events_conflict(struct perf_event *e1, struct perf_event *e2)
> +{
> + if ((e1 != e2) &&
> + (e1->pmu == e2->pmu) &&
> + (QC_EVT_PRG(e1->attr.config) == QC_EVT_PRG(e2->attr.config)) &&
> + (QC_EVT_CODE(e1->attr.config) != QC_EVT_CODE(e2->attr.config))) {
> + pr_debug_ratelimited(
> + "Group exclusion: conflicting events %llx %llx\n",
> + e1->attr.config,
> + e2->attr.config);
> + return 1;
> + }
> + return 0;
> +}
This would be easier to read as a series of tests:
static inline bool events_conflict(struct perf_event *new,
struct perf_event *other)
{
/* own group leader */
if (new == other)
return false;
/* software events can't conflict */
if (is_sw_event(other))
return false;
/* No conflict if using different reg or group */
if (QC_EVT_PRG(new->attr.config) != QC_EVT_PRG(other->attr.config))
return false;
/* Same reg and group is fine so long as code matches */
if (QC_EVT_CODE(new->attr.config) == QC_EVT_PRG(other->attr.config)
return false;
pr_debug_ratelimited("Group exclusion: conflicting events %llx %llx\n",
new->attr.config, other->attr.config);
return true;
}
> +
> +/*
> + * Check if the given event is valid for the PMU and if so return the value
> + * that can be used in PMXEVTYPER_EL0 to select the event
> + */
> +static int falkor_map_event(struct perf_event *event)
> +{
> + u64 reg = QC_EVT_REG(event->attr.config);
> + u64 group = QC_EVT_GROUP(event->attr.config);
> + struct perf_event *leader;
> + struct perf_event *sibling;
> +
> + if (!(event->attr.config & QC_EVT_PFX_MASK))
> + /* Common PMUv3 event, forward to the original op */
> + return def_ops->map_event(event);
The QC event codes should only be used when either:
* event->attr.type is PERF_TYPE_RAW, or:
* event->pmu.type is this PMU's dynamic type
... otherwise this will accept events meant for other PMUs, and/or
override conflicting events in other type namespaces (e.g.
PERF_EVENT_TYPE_HW, PERF_EVENT_TYPE_CACHE).
> +
> + /* Is it a valid matrix event? */
> + if ((group > QC_MAX_GROUP) || (reg > QC_MAX_RESR))
> + return -ENOENT;
> +
> + /* If part of an event group, check if the event can be put in it */
> +
> + leader = event->group_leader;
> + if (events_conflict(event, leader))
> + return -ENOENT;
> +
> + for_each_sibling_event(sibling, leader)
> + if (events_conflict(event, sibling))
> + return -ENOENT;
> +
> + return QC_RESR_EVT_BASE + reg*8 + group;
Nit: spacing around binary operators please.
> +}
> +
> +/*
> + * Find a slot for the event on the current CPU
> + */
> +static int falkor_get_event_idx(struct pmu_hw_events *cpuc, struct perf_event *event)
> +{
> + int idx;
> +
> + if (!!(event->attr.config & QC_EVT_PFX_MASK))
The '!!' isn't required.
> + /* Matrix event, check for conflicts with existing events */
> + for_each_set_bit(idx, cpuc->used_mask, ARMPMU_MAX_HWEVENTS)
> + if (cpuc->events[idx] &&
> + events_conflict(event, cpuc->events[idx]))
> + return -ENOENT;
> +
> + /* Let the original op handle the rest */
> + idx = def_ops->get_event_idx(cpuc, event);
Same comments as for falkor_map_event().
> +
> + /*
> + * This is called for actually allocating the events, but also with
> + * a dummy pmu_hw_events when validating groups, for that case we
> + * need to ensure that cpuc->events[idx] is NULL so we don't use
> + * an uninitialized pointer. Conflicts for matrix events in groups
> + * are checked during event mapping anyway (see falkor_event_map).
> + */
> + cpuc->events[idx] = NULL;
> +
> + return idx;
> +}
> +
> +/*
> + * Reset the PMU
> + */
> +static void falkor_reset(void *info)
> +{
> + struct arm_pmu *pmu = (struct arm_pmu *)info;
> + u32 i, ctrs = pmu->num_events;
> +
> + /* PMRESRx_EL0 regs are unknown at reset, except for the EN field */
> + for (i = 0; i <= QC_MAX_RESR; i++)
> + falkor_write_pmresr(i, 0);
> +
> + /* PMXEVCNTCRx_EL0 regs are unknown at reset */
> + for (i = 0; i <= ctrs; i++) {
> + write_sysreg(i, pmselr_el0);
> + isb();
> + write_sysreg_s(0, pmxevcntcr_el0);
> + }
> +
> + /* Let the original op handle the rest */
> + def_ops->reset(info);
> +}
> +
> +/*
> + * Enable the given event
> + */
> +static void falkor_enable(struct perf_event *event)
> +{
> + if (!!(event->attr.config & QC_EVT_PFX_MASK)) {
The '!!' isn't required.
> + /* Matrix event, program the appropriate PMRESRx_EL0 */
> + struct arm_pmu *pmu = to_arm_pmu(event->pmu);
> + struct pmu_hw_events *events = this_cpu_ptr(pmu->hw_events);
> + u64 reg = QC_EVT_REG(event->attr.config);
> + u64 code = QC_EVT_CODE(event->attr.config);
> + u64 group = QC_EVT_GROUP(event->attr.config);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&events->pmu_lock, flags);
> + falkor_set_resr(reg, group, code);
> + raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
Why is the spinlock required?
AFACIT this should only ever be called in contexts where IRQs are
disabled already.
> + }
> +
> + /* Let the original op handle the rest */
> + def_ops->enable(event);
> +}
> +
> +/*
> + * Disable the given event
> + */
> +static void falkor_disable(struct perf_event *event)
> +{
> + /* Use the original op to disable the counter and interrupt */
> + def_ops->enable(event);
> +
> + if (!!(event->attr.config & QC_EVT_PFX_MASK)) {
> + /* Matrix event, de-program the appropriate PMRESRx_EL0 */
> + struct arm_pmu *pmu = to_arm_pmu(event->pmu);
> + struct pmu_hw_events *events = this_cpu_ptr(pmu->hw_events);
> + u64 reg = QC_EVT_REG(event->attr.config);
> + u64 group = QC_EVT_GROUP(event->attr.config);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&events->pmu_lock, flags);
> + falkor_clear_resr(reg, group);
> + raw_spin_unlock_irqrestore(&events->pmu_lock, flags);
> + }
> +}
Same comments as with falkor_enable().
> +
> +PMU_FORMAT_ATTR(event, "config:0-15");
> +PMU_FORMAT_ATTR(prefix, "config:16");
> +PMU_FORMAT_ATTR(reg, "config:12-15");
> +PMU_FORMAT_ATTR(code, "config:4-11");
> +PMU_FORMAT_ATTR(group, "config:0-3");
What sort of events are available? Do you plan to add anything to the
userspace event database in tools/perf/pmu-events/ ?
> +
> +static struct attribute *falkor_pmu_formats[] = {
> + &format_attr_event.attr,
> + &format_attr_prefix.attr,
> + &format_attr_reg.attr,
> + &format_attr_code.attr,
> + &format_attr_group.attr,
> + NULL,
> +};
> +
> +static struct attribute_group falkor_pmu_format_attr_group = {
> + .name = "format",
> + .attrs = falkor_pmu_formats,
> +};
> +
> +static int qcom_falkor_pmu_init(struct arm_pmu *pmu, struct device *dev)
> +{
> + /* Save base arm_pmu so we can invoke its ops when appropriate */
> + def_ops = devm_kmemdup(dev, pmu, sizeof(*def_ops), GFP_KERNEL);
> + if (!def_ops) {
> + pr_warn("Failed to allocate arm_pmu for QCOM extensions");
> + return -ENODEV;
> + }
> +
> + pmu->name = "qcom_pmuv3";
All the other CPU PMUs on an ARM ACPI system will have an index suffix,
e.g. "armv8_pmuv3_0". I can see why we might want to change the name to
indicate the QC extensions, but I think we should keep the existing
pattern, with a '_0' suffix here.
> +
> + /* Override the necessary ops */
> + pmu->map_event = falkor_map_event;
> + pmu->get_event_idx = falkor_get_event_idx;
> + pmu->reset = falkor_reset;
> + pmu->enable = falkor_enable;
> + pmu->disable = falkor_disable;
I'm somewhat concerned by hooking into the existing PMU code at this
level, but I don't currently have a better suggestion.
Thanks,
Mark.
> +
> + /* Override the necessary attributes */
> + pmu->pmu.attr_groups[ARMPMU_ATTR_GROUP_FORMATS] =
> + &falkor_pmu_format_attr_group;
> +
> + return 1;
> +}
> +
> +ACPI_DECLARE_PMU_VARIANT(qcom_falkor, "QCOM8150", qcom_falkor_pmu_init);
> --
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
>
^ permalink raw reply
* [PATCH RESEND v4 2/2] arm/arm64: KVM: Add KVM_GET/SET_VCPU_EVENTS
From: gengdongjiu @ 2018-06-12 14:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <45e94aae-ed9f-1fb7-f10e-d95c2f969ddd@arm.com>
Hi James,
thanks for the review.
On 2018/6/11 21:36, James Morse wrote:
> Hi Dongjiu Geng,
>
> Please only put 'RESEND' in the subject if the patch content is identical.
> This patch is not the same as v4.
Yes, it should
>
> On 08/06/18 20:48, Dongjiu Geng wrote:
>> For the migrating VMs, user space may need to know the exception
>> state. For example, in the machine A, KVM make an SError pending,
>> when migrate to B, KVM also needs to pend an SError.
>>
>> This new IOCTL exports user-invisible states related to SError.
>> Together with appropriate user space changes, user space can get/set
>> the SError exception state to do migrate/snapshot/suspend.
>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index fdac969..8896737 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -835,11 +835,13 @@ struct kvm_clock_data {
>>
>> Capability: KVM_CAP_VCPU_EVENTS
>> Extended by: KVM_CAP_INTR_SHADOW
>> -Architectures: x86
>> +Architectures: x86, arm, arm64
>> Type: vm ioctl
>
> Isn't this actually a per-vcpu ioctl? Can we fix the documentation?
I will modify the original documentation
>
>
>> Parameters: struct kvm_vcpu_event (out)
>> Returns: 0 on success, -1 on error
>>
>> +X86:
>> +
>> Gets currently pending exceptions, interrupts, and NMIs as well as related
>> states of the vcpu.
>>
>> @@ -881,15 +883,32 @@ Only two fields are defined in the flags field:
>> - KVM_VCPUEVENT_VALID_SMM may be set in the flags field to signal that
>> smi contains a valid state.
>>
>> +ARM, ARM64:
>> +
>> +Gets currently pending SError exceptions as well as related states of the vcpu.
>> +
>> +struct kvm_vcpu_events {
>> + struct {
>> + __u8 serror_pending;
>> + __u8 serror_has_esr;
>> + /* Align it to 8 bytes */
>> + __u8 pad[6];
>> + __u64 serror_esr;
>> + } exception;
>> + __u32 reserved[12];
>> +};
>> +
>> 4.32 KVM_SET_VCPU_EVENTS
>>
>> -Capability: KVM_CAP_VCPU_EVENTS
>> +Capebility: KVM_CAP_VCPU_EVENTS
>
> (please fix this)
Ok, will fix this
>
>
>> Extended by: KVM_CAP_INTR_SHADOW
>> -Architectures: x86
>> +Architectures: x86, arm, arm64
>> Type: vm ioctl
>
> (this is also a vcpu ioctl)
will fix
>
>
>> Parameters: struct kvm_vcpu_event (in)
>> Returns: 0 on success, -1 on error
>>
>> +X86:
>> +
>> Set pending exceptions, interrupts, and NMIs as well as related states of the
>> vcpu.
>>
>> @@ -910,6 +929,12 @@ shall be written into the VCPU.
>>
>> KVM_VCPUEVENT_VALID_SMM can only be set if KVM_CAP_X86_SMM is available.
>>
>> +ARM, ARM64:
>> +
>> +Set pending SError exceptions as well as related states of the vcpu.
>
> There are some deliberate choices here I think we should document:
> | This API can't be used to clear a pending SError.
>
> If there already was an SError pending, this API just overwrites it with the new
> one. The architecture has some rules about merging multiple SError. (details in
> 2.5.3 Multiple SError interrupts of [0])
>
> I don't think KVM needs to enforce these, as they are implementation-defined if
> one of the ESR is implementation-defined... the part that matters is reporting
> the 'most severe' RAS ESR if there are multiple pending. As only user-space ever
> sets these, let's make it user-spaces problem to do.
>
> I think we should recommend user-space always reads the pending values and
> applies its merging-multiple-SError logic. (I assume your Qemu patches do this).
I will check whether QEMU can be possible to do such things, anyway this patch
not need to do such merging.
> Something like:
> | User-space should first use KVM_GET_VCPU_EVENTS in case KVM has made an SError
> | pending as part of its device emulation. When both values are architected RAS
> | SError ESR values, the new ESR should reflect the combined effect of both
> | errors.>
>
>> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
>> index caae484..c3e6975 100644
>> --- a/arch/arm/include/uapi/asm/kvm.h
>> +++ b/arch/arm/include/uapi/asm/kvm.h
>> @@ -124,6 +124,18 @@ struct kvm_sync_regs {
>> struct kvm_arch_memory_slot {
>> };
>>
>> +/* for KVM_GET/SET_VCPU_EVENTS */
>> +struct kvm_vcpu_events {
>> + struct {
>> + __u8 serror_pending;
>> + __u8 serror_has_esr;
>> + /* Align it to 8 bytes */
>> + __u8 pad[6];
>> + __u64 serror_esr;
>> + } exception;
>> + __u32 reserved[12];
>> +};
>> +
>
> You haven't defined __KVM_HAVE_VCPU_EVENTS for 32bit, so presumably this struct
> will never be used. Why is it here?
if not add it for 32 bits. the 32 arm platform will build Fail, whether you have good
idea to avoid this Failure if not add this struct for the 32 bit?
> (I agree if we ever provide it on 32bit, the struct layout should be the same.
> Is this only here to force that to happen?)
>
> [...]
>
>
>> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>> + struct kvm_vcpu_events *events)
>> +{
>> + bool serror_pending = events->exception.serror_pending;
>> + bool has_esr = events->exception.serror_has_esr;
>> +
>> + if (serror_pending && has_esr) {
>> + if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
>> + return -EINVAL;
>> +
>> + kvm_set_sei_esr(vcpu, events->exception.serror_esr);
>
> kvm_set_sei_esr() will silently discard the top 40 bits of serror_esr, (which is
> correct, we shouldn't copy them into hardware without know what they do).
>
> Could we please force user-space to zero these bits, we can advertise extra CAPs
> if new features turn up in that space, instead of user-space passing <something>
> and relying on the kernel to remove it.
yes, I can zero these bits in the user-space and not depend on kernel to remove it.
>
> (Background: VSESR is a 64bit register that holds the value to go in a 32bit
> register. I suspect the top-half could get re-used for control values or
> something we don't want to give user-space)
do you mean when user-space get the VSESR value through KVM_GET_VCPU_EVENTS it only return the low-half 32 bits?
>
>
>> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
>> index d8e7165..a55e91d 100644
>> --- a/arch/arm64/kvm/inject_fault.c
>> +++ b/arch/arm64/kvm/inject_fault.c
>> @@ -164,9 +164,9 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu)
>> inject_undef64(vcpu);
>> }
>>
>> -static void pend_guest_serror(struct kvm_vcpu *vcpu, u64 esr)
>> +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 esr)
>> {
>> - vcpu_set_vsesr(vcpu, esr);
>> + vcpu_set_vsesr(vcpu, esr & ESR_ELx_ISS_MASK);
>> *vcpu_hcr(vcpu) |= HCR_VSE;
>> }
>>
>
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index a4c1b76..79ecba9 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -1107,6 +1107,25 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>
>> + case KVM_SET_VCPU_EVENTS: {
>> + struct kvm_vcpu_events events;
>> +
>> + if (copy_from_user(&events, argp, sizeof(events)))
>> + return -EFAULT;
>> +
>> + return kvm_arm_vcpu_set_events(vcpu, &events);
>> + }
>
> Please check the padding[] and reserved[] are zero, otherwise we can't re-use these.
Ok, thanks
>
>
> Thanks,
>
> James
>
> [0]
> https://static.docs.arm.com/ddi0587/a/RAS%20Extension-release%20candidate_march_29.pdf
>
> .
>
^ permalink raw reply
* [PATCH RESEND v4 2/2] arm/arm64: KVM: Add KVM_GET/SET_VCPU_EVENTS
From: gengdongjiu @ 2018-06-12 14:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <af93e4b1-80d5-33be-d5ed-312c3ea1d715@arm.com>
On 2018/6/11 21:36, James Morse wrote:
> Hi Dongjiu Geng,
>
> On 09/06/18 13:40, Marc Zyngier wrote:
>> On Fri, 08 Jun 2018 20:48:40 +0100, Dongjiu Geng wrote:
>>> For the migrating VMs, user space may need to know the exception
>>> state. For example, in the machine A, KVM make an SError pending,
>>> when migrate to B, KVM also needs to pend an SError.
>>>
>>> This new IOCTL exports user-invisible states related to SError.
>>> Together with appropriate user space changes, user space can get/set
>>> the SError exception state to do migrate/snapshot/suspend.
>
>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>>> index 04b3256..df4faee 100644
>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>> @@ -153,6 +154,18 @@ struct kvm_sync_regs {
>>> struct kvm_arch_memory_slot {
>>> };
>>>
>>> +/* for KVM_GET/SET_VCPU_EVENTS */
>>> +struct kvm_vcpu_events {
>>> + struct {
>>> + __u8 serror_pending;
>>> + __u8 serror_has_esr;
>>> + /* Align it to 8 bytes */
>>> + __u8 pad[6];
>>> + __u64 serror_esr;
>>> + } exception;
>>> + __u32 reserved[12];
>>> +};
>
>>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
>>> index 56a0260..4426915 100644
>>> --- a/arch/arm64/kvm/guest.c
>>> +++ b/arch/arm64/kvm/guest.c
>
>>> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>>> + struct kvm_vcpu_events *events)
>>> +{
>>> + bool serror_pending = events->exception.serror_pending;
>>> + bool has_esr = events->exception.serror_has_esr;
>>> +
>>> + if (serror_pending && has_esr) {
>>> + if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
>>> + return -EINVAL;
>>> +
>>> + kvm_set_sei_esr(vcpu, events->exception.serror_esr);
>>> + } else if (serror_pending) {
>>> + kvm_inject_vabt(vcpu);
>>> + }
>>> +
>>> + return 0;
>>
>> There was an earlier request to check that all the padding is set to
>> zero. I still think this makes sense.
>
> I agree, not just the exception.padding[], but reserved[] too.
Ok, thanks for the reminder again.
>
>
> Thanks,
>
> James
>
> .
>
^ permalink raw reply
* [PATCH v3 1/3] cpufreq: imx6q: check speed grades for i.MX6ULL
From: Rafael J. Wysocki @ 2018-06-12 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180611053826.GC16091@dragon>
On Monday, June 11, 2018 7:38:27 AM CEST Shawn Guo wrote:
> On Tue, May 22, 2018 at 08:28:51AM +0200, S?bastien Szymanski wrote:
> > Check the max speed supported from the fuses for i.MX6ULL and update the
> > operating points table accordingly.
> >
> > Signed-off-by: S?bastien Szymanski <sebastien.szymanski@armadeus.com>
>
> Acked-by: Shawn Guo <shawnguo@kernel.org>
Patch applied, thanks!
^ permalink raw reply
* [PATCH v3 0/5] PM / Domains: Add support for multi PM domains per device
From: Rafael J. Wysocki @ 2018-06-12 14:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180531105959.14843-1-ulf.hansson@linaro.org>
On Thursday, May 31, 2018 12:59:54 PM CEST Ulf Hansson wrote:
> Changes in v3:
> - Drop patch 1->4 as they have already been applied.
> - Collected tags, for tests and reviews.
> - Minor update to function descriptions in patch 4 (earlier 8) and 5
> (earlier9).
> - Note, because of the minor changes, no history is provided per patch.
>
> Changes in v2:
> - Addressed comments from Geert around DT doc.
> - Addressed comments from Jon around clarification of how to use this
> and changes to returned error codes.
> - Fixed build error in case CONFIG_PM was unset.
>
> There are devices that are partitioned across multiple PM domains. Currently
> these can't be supported well by the available PM infrastructures we have in
> the kernel. This series is an attempt to address this.
>
> One existing case where devices are partitioned across multiple PM domains, is
> the Nvida Tegra 124/210 X-USB subsystem. A while ago Jon Hunter (Nvidia) sent a
> series, trying to address these issues, however this is a new approach, while
> it re-uses the same concepts from DT point of view.
>
> The Tegra 124/210 X-USB subsystem contains of a host controller and a device
> controller. Each controller have its own independent PM domain, but are being
> partitioned across another shared PM domain for the USB super-speed logic.
>
> Currently to make the drivers work, either the related PM domains needs to stay
> powered on always or the PM domain topology needs to be in-correctly modelled
> through sub-domains. In both cases PM domains may be powered on while they
> don't need to be, so in the end this means - wasting power -.
>
> As stated above, this series intends to address these problem from a PM
> infrastructure point of view. More details are available in each changelog.
>
> Kind regards
> Ulf Hansson
>
> Ulf Hansson (5):
> PM / Domains: dt: Allow power-domain property to be a list of
> specifiers
> PM / Domains: Don't attach devices in genpd with multi PM domains
> PM / Domains: Split genpd_dev_pm_attach()
> PM / Domains: Add support for multi PM domains per device to genpd
> PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM
> domains
>
> .../bindings/power/power_domain.txt | 19 ++-
> drivers/base/power/common.c | 43 +++++-
> drivers/base/power/domain.c | 134 +++++++++++++++---
> include/linux/pm_domain.h | 15 ++
> 4 files changed, 183 insertions(+), 28 deletions(-)
>
>
Applied, thanks!
^ permalink raw reply
* [PATCH 1/2] arm64: avoid alloc memory on offline node
From: Punit Agrawal @ 2018-06-12 15:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180611145330.GO13364@dhcp22.suse.cz>
Michal Hocko <mhocko@kernel.org> writes:
> On Mon 11-06-18 08:43:03, Bjorn Helgaas wrote:
>> On Mon, Jun 11, 2018 at 08:32:10PM +0800, Xie XiuQi wrote:
>> > Hi Michal,
>> >
>> > On 2018/6/11 16:52, Michal Hocko wrote:
>> > > On Mon 11-06-18 11:23:18, Xie XiuQi wrote:
>> > >> Hi Michal,
>> > >>
>> > >> On 2018/6/7 20:21, Michal Hocko wrote:
>> > >>> On Thu 07-06-18 19:55:53, Hanjun Guo wrote:
>> > >>>> On 2018/6/7 18:55, Michal Hocko wrote:
>> > >>> [...]
>> > >>>>> I am not sure I have the full context but pci_acpi_scan_root calls
>> > >>>>> kzalloc_node(sizeof(*info), GFP_KERNEL, node)
>> > >>>>> and that should fall back to whatever node that is online. Offline node
>> > >>>>> shouldn't keep any pages behind. So there must be something else going
>> > >>>>> on here and the patch is not the right way to handle it. What does
>> > >>>>> faddr2line __alloc_pages_nodemask+0xf0 tells on this kernel?
>> > >>>>
>> > >>>> The whole context is:
>> > >>>>
>> > >>>> The system is booted with a NUMA node has no memory attaching to it
>> > >>>> (memory-less NUMA node), also with NR_CPUS less than CPUs presented
>> > >>>> in MADT, so CPUs on this memory-less node are not brought up, and
>> > >>>> this NUMA node will not be online (but SRAT presents this NUMA node);
>> > >>>>
>> > >>>> Devices attaching to this NUMA node such as PCI host bridge still
>> > >>>> return the valid NUMA node via _PXM, but actually that valid NUMA node
>> > >>>> is not online which lead to this issue.
>> > >>>
>> > >>> But we should have other numa nodes on the zonelists so the allocator
>> > >>> should fall back to other node. If the zonelist is not intiailized
>> > >>> properly, though, then this can indeed show up as a problem. Knowing
>> > >>> which exact place has blown up would help get a better picture...
>> > >>>
>> > >>
>> > >> I specific a non-exist node to allocate memory using kzalloc_node,
>> > >> and got this following error message.
>> > >>
>> > >> And I found out there is just a VM_WARN, but it does not prevent the memory
>> > >> allocation continue.
>> > >>
>> > >> This nid would be use to access NODE_DADA(nid), so if nid is invalid,
>> > >> it would cause oops here.
>> > >>
>> > >> 459 /*
>> > >> 460 * Allocate pages, preferring the node given as nid. The node must be valid and
>> > >> 461 * online. For more general interface, see alloc_pages_node().
>> > >> 462 */
>> > >> 463 static inline struct page *
>> > >> 464 __alloc_pages_node(int nid, gfp_t gfp_mask, unsigned int order)
>> > >> 465 {
>> > >> 466 VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
>> > >> 467 VM_WARN_ON(!node_online(nid));
>> > >> 468
>> > >> 469 return __alloc_pages(gfp_mask, order, nid);
>> > >> 470 }
>> > >> 471
>> > >>
>> > >> (I wrote a ko, to allocate memory on a non-exist node using kzalloc_node().)
>> > >
>> > > OK, so this is an artificialy broken code, right. You shouldn't get a
>> > > non-existent node via standard APIs AFAICS. The original report was
>> > > about an existing node which is offline AFAIU. That would be a different
>> > > case. If I am missing something and there are legitimate users that try
>> > > to allocate from non-existing nodes then we should handle that in
>> > > node_zonelist.
>> >
>> > I think hanjun's comments may help to understood this question:
>> > - NUMA node will be built if CPUs and (or) memory are valid on this NUMA
>> > node;
>> >
>> > - But if we boot the system with memory-less node and also with
>> > CONFIG_NR_CPUS less than CPUs in SRAT, for example, 64 CPUs total with 4
>> > NUMA nodes, 16 CPUs on each NUMA node, if we boot with
>> > CONFIG_NR_CPUS=48, then we will not built numa node for node 3, but with
>> > devices on that numa node, alloc memory will be panic because NUMA node
>> > 3 is not a valid node.
>
> Hmm, but this is not a memory-less node. It sounds like a misconfigured
> kernel to me or the broken initialization. Each CPU should have a
> fallback numa node to be used.
>
>> > I triggered this BUG on arm64 platform, and I found a similar bug has
>> > been fixed on x86 platform. So I sent a similar patch for this bug.
>> >
>> > Or, could we consider to fix it in the mm subsystem?
>>
>> The patch below (b755de8dfdfe) seems like totally the wrong direction.
>> I don't think we want every caller of kzalloc_node() to have check for
>> node_online().
>
> absolutely.
>
>> Why would memory on an off-line node even be in the allocation pool?
>> I wouldn't expect that memory to be put in the pool until the node
>> comes online and the memory is accessible, so this sounds like some
>> kind of setup issue.
>>
>> But I'm definitely not an mm person.
>
> Well, the standard way to handle memory less NUMA nodes is to simply
> fallback to the closest NUMA node. We even have an API for that
> (numa_mem_id).
CONFIG_HAVE_MEMORYLESS node is not enabled on arm64 which means we end
up returning the original node in the fallback path.
Xie, does the below patch help? I can submit a proper patch if this
fixes the issue for you.
-- >8 --
Subject: [PATCH] arm64/numa: Enable memoryless numa nodes
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
arch/arm64/Kconfig | 4 ++++
arch/arm64/mm/numa.c | 2 ++
2 files changed, 6 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf4938f6d..5317e9aa93ab 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -756,6 +756,10 @@ config USE_PERCPU_NUMA_NODE_ID
def_bool y
depends on NUMA
+config HAVE_MEMORYLESS_NODES
+ def_bool y
+ depends on NUMA
+
config HAVE_SETUP_PER_CPU_AREA
def_bool y
depends on NUMA
diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index dad128ba98bf..c699dcfe93de 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -73,6 +73,8 @@ EXPORT_SYMBOL(cpumask_of_node);
static void map_cpu_to_node(unsigned int cpu, int nid)
{
set_cpu_numa_node(cpu, nid);
+ set_numa_mem(local_memory_node(nid));
+
if (nid >= 0)
cpumask_set_cpu(cpu, node_to_cpumask_map[nid]);
}
--
2.17.0
^ permalink raw reply related
* [RFC] Configure i.MX6 RGMII pad group control registers from device tree
From: Michal Vokáč @ 2018-06-12 15:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f5a01de5-fc81-52c1-c2ea-edb77ecff076@ysoft.com>
On 11.6.2018 14:36, Michal Vok?? wrote:
> Ahoj,
>
> To configure individual pad's characteristics on i.MX6 SoC a
> fsl,pins = <PIN_FUNC_ID CONFIG> property can be used. Is there any convenient
> way to configure the pad group control registers?
>
> The issue is that some bits (DDR_SEL and ODT) in the individual RGMII pad
> control registers are read-only. To tweak those parameters (signal voltage and
> termination resistors) one need to write to the pad group control registers for
> the whole RGMII pad group. Namely IOMUXC_SW_PAD_CTL_GRP_DDR_TYPE_RGMII and
> IOMUXC_SW_PAD_CTL_GRP_RGMII_TERM. The group registers in general are not
> accessible from the list in arch/arm/boot/dts/imx6dl-pinfunc.h.
>
> I could not find any other way to change the group registers than hacking-in
> some lines into the imx6q_init_machine(void) function in
> arch/arm/mach-imx/mach-imx6q.c source. As I work towards upstreaming my board
> this should be done from my device tree or solved in some universal way.
>
> Any hints will be much appreciated.
> Michal
I figured out this is more "pinctrl-imx.c" than "device-tree" related
so I am kindly adding maintainers of that file in hope somebody will
shed some light to it.
I am diving deeper into the code and it seems there really is no generic
option to set the i.MX6 pad group control registers from device tree.
Or am I looking at the problem from a wrong angle?
How should we deal with boards that need to configure some pad
characteristics available only through the pad group control registers?
I also raised this question at the NXP community forum [1] and get quite
unsatisfying answer so far. I would love to find/implement a proper
and universal solution.
Thanks in advance for your time,
Michal
[1] https://community.nxp.com/thread/477464
^ permalink raw reply
* [PATCH 1/2] arm64: avoid alloc memory on offline node
From: Michal Hocko @ 2018-06-12 15:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87lgbk59gs.fsf@e105922-lin.cambridge.arm.com>
On Tue 12-06-18 16:08:03, Punit Agrawal wrote:
> Michal Hocko <mhocko@kernel.org> writes:
[...]
> > Well, the standard way to handle memory less NUMA nodes is to simply
> > fallback to the closest NUMA node. We even have an API for that
> > (numa_mem_id).
>
> CONFIG_HAVE_MEMORYLESS node is not enabled on arm64 which means we end
> up returning the original node in the fallback path.
Yes this makes more sense.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH] ARM64: dts: rockchip: add some pins to rk3399
From: Randy Li @ 2018-06-12 15:25 UTC (permalink / raw)
To: linux-arm-kernel
Those pins would be used by many boards.
Signed-off-by: Randy Li <ayaka@soulik.info>
---
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 97 +++++++++++++++++++++++++++-----
1 file changed, 83 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index e0040b648f43..9fa629857929 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -160,7 +160,7 @@
};
};
- display-subsystem {
+ display_subsystem: display-subsystem {
compatible = "rockchip,display-subsystem";
ports = <&vopl_out>, <&vopb_out>;
};
@@ -1936,19 +1936,49 @@
drive-strength = <12>;
};
+ pcfg_pull_none_13ma: pcfg-pull-none-13ma {
+ bias-disable;
+ drive-strength = <13>;
+ };
+
+ pcfg_pull_none_18ma: pcfg-pull-none-18ma {
+ bias-disable;
+ drive-strength = <18>;
+ };
+
+ pcfg_pull_none_20ma: pcfg-pull-none-20ma {
+ bias-disable;
+ drive-strength = <20>;
+ };
+
+ pcfg_pull_up_2ma: pcfg-pull-up-2ma {
+ bias-pull-up;
+ drive-strength = <2>;
+ };
+
pcfg_pull_up_8ma: pcfg-pull-up-8ma {
bias-pull-up;
drive-strength = <8>;
};
+ pcfg_pull_up_18ma: pcfg-pull-up-18ma {
+ bias-pull-up;
+ drive-strength = <18>;
+ };
+
+ pcfg_pull_up_20ma: pcfg-pull-up-20ma {
+ bias-pull-up;
+ drive-strength = <20>;
+ };
+
pcfg_pull_down_4ma: pcfg-pull-down-4ma {
bias-pull-down;
drive-strength = <4>;
};
- pcfg_pull_up_2ma: pcfg-pull-up-2ma {
- bias-pull-up;
- drive-strength = <2>;
+ pcfg_pull_down_8ma: pcfg-pull-down-8ma {
+ bias-pull-down;
+ drive-strength = <8>;
};
pcfg_pull_down_12ma: pcfg-pull-down-12ma {
@@ -1956,9 +1986,22 @@
drive-strength = <12>;
};
- pcfg_pull_none_13ma: pcfg-pull-none-13ma {
- bias-disable;
- drive-strength = <13>;
+ pcfg_pull_down_18ma: pcfg-pull-down-18ma {
+ bias-pull-down;
+ drive-strength = <18>;
+ };
+
+ pcfg_pull_down_20ma: pcfg-pull-down-20ma {
+ bias-pull-down;
+ drive-strength = <18>;
+ };
+
+ pcfg_output_high: pcfg-output-high {
+ output-high;
+ };
+
+ pcfg_output_low: pcfg-output-low {
+ output-low;
};
clock {
@@ -2484,10 +2527,21 @@
<4 18 RK_FUNC_1 &pcfg_pull_none>;
};
+ pwm0_pin_pull_down: pwm0-pin-pull-down {
+ rockchip,pins =
+ <4 18 RK_FUNC_1 &pcfg_pull_down>;
+ };
+
vop0_pwm_pin: vop0-pwm-pin {
rockchip,pins =
<4 18 RK_FUNC_2 &pcfg_pull_none>;
};
+
+ vop1_pwm_pin: vop1-pwm-pin {
+ rockchip,pins =
+ <4 18 RK_FUNC_3 &pcfg_pull_none>;
+ };
+
};
pwm1 {
@@ -2496,9 +2550,9 @@
<4 22 RK_FUNC_1 &pcfg_pull_none>;
};
- vop1_pwm_pin: vop1-pwm-pin {
+ pwm1_pin_pull_down: pwm1-pin-pull-down {
rockchip,pins =
- <4 18 RK_FUNC_3 &pcfg_pull_none>;
+ <4 22 RK_FUNC_1 &pcfg_pull_down>;
};
};
@@ -2507,6 +2561,11 @@
rockchip,pins =
<1 19 RK_FUNC_1 &pcfg_pull_none>;
};
+
+ pwm2_pin_pull_down: pwm2-pin-pull-down {
+ rockchip,pins =
+ <1 19 RK_FUNC_1 &pcfg_pull_none>;
+ };
};
pwm3a {
@@ -2526,25 +2585,35 @@
hdmi {
hdmi_i2c_xfer: hdmi-i2c-xfer {
rockchip,pins =
- <4 RK_PC1 RK_FUNC_3 &pcfg_pull_none>,
- <4 RK_PC0 RK_FUNC_3 &pcfg_pull_none>;
+ <4 17 RK_FUNC_3 &pcfg_pull_none>,
+ <4 16 RK_FUNC_3 &pcfg_pull_none>;
};
hdmi_cec: hdmi-cec {
rockchip,pins =
- <4 RK_PC7 RK_FUNC_1 &pcfg_pull_none>;
+ <4 23 RK_FUNC_1 &pcfg_pull_none>;
};
};
pcie {
+ pcie_clkreqn: pci-clkreqn {
+ rockchip,pins =
+ <2 26 RK_FUNC_2 &pcfg_pull_none>;
+ };
+
+ pcie_clkreqnb: pci-clkreqnb {
+ rockchip,pins =
+ <4 24 RK_FUNC_1 &pcfg_pull_none>;
+ };
+
pcie_clkreqn_cpm: pci-clkreqn-cpm {
rockchip,pins =
- <2 RK_PD2 RK_FUNC_GPIO &pcfg_pull_none>;
+ <2 26 RK_FUNC_GPIO &pcfg_pull_none>;
};
pcie_clkreqnb_cpm: pci-clkreqnb-cpm {
rockchip,pins =
- <4 RK_PD0 RK_FUNC_GPIO &pcfg_pull_none>;
+ <4 24 RK_FUNC_GPIO &pcfg_pull_none>;
};
};
--
2.14.4
^ permalink raw reply related
* [PATCH RESEND v4 2/2] arm/arm64: KVM: Add KVM_GET/SET_VCPU_EVENTS
From: James Morse @ 2018-06-12 15:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f7f59176-16a1-3b24-5acc-87bf1b4c1490@huawei.com>
Hi gengdongjiu,
On 12/06/18 15:50, gengdongjiu wrote:
> On 2018/6/11 21:36, James Morse wrote:
>> On 08/06/18 20:48, Dongjiu Geng wrote:
>>> For the migrating VMs, user space may need to know the exception
>>> state. For example, in the machine A, KVM make an SError pending,
>>> when migrate to B, KVM also needs to pend an SError.
>>>
>>> This new IOCTL exports user-invisible states related to SError.
>>> Together with appropriate user space changes, user space can get/set
>>> the SError exception state to do migrate/snapshot/suspend.
>>> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
>>> index caae484..c3e6975 100644
>>> --- a/arch/arm/include/uapi/asm/kvm.h
>>> +++ b/arch/arm/include/uapi/asm/kvm.h
>>> @@ -124,6 +124,18 @@ struct kvm_sync_regs {
>>> struct kvm_arch_memory_slot {
>>> };
>>>
>>> +/* for KVM_GET/SET_VCPU_EVENTS */
>>> +struct kvm_vcpu_events {
>>> + struct {
>>> + __u8 serror_pending;
>>> + __u8 serror_has_esr;
>>> + /* Align it to 8 bytes */
>>> + __u8 pad[6];
>>> + __u64 serror_esr;
>>> + } exception;
>>> + __u32 reserved[12];
>>> +};
>>> +
>>
>> You haven't defined __KVM_HAVE_VCPU_EVENTS for 32bit, so presumably this struct
>> will never be used. Why is it here?
> if not add it for 32 bits. the 32 arm platform will build Fail, whether you have good
> idea to avoid this Failure if not add this struct for the 32 bit?
How does this 32bit code build without this patch?
If do you provide the struct, how will that code build with older headers?
As far as I can see, this is what the __KVM_HAVE_VCPU_EVENTS define is for.
This should be both, or neither. Having just the struct is useless.
>>> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>>> + struct kvm_vcpu_events *events)
>>> +{
>>> + bool serror_pending = events->exception.serror_pending;
>>> + bool has_esr = events->exception.serror_has_esr;
>>> +
>>> + if (serror_pending && has_esr) {
>>> + if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
>>> + return -EINVAL;
>>> +
>>> + kvm_set_sei_esr(vcpu, events->exception.serror_esr);
>>
>> kvm_set_sei_esr() will silently discard the top 40 bits of serror_esr, (which is
>> correct, we shouldn't copy them into hardware without know what they do).
>>
>> Could we please force user-space to zero these bits, we can advertise extra CAPs
>> if new features turn up in that space, instead of user-space passing <something>
>> and relying on the kernel to remove it.
>
> yes, I can zero these bits in the user-space and not depend on kernel to remove it.
But the kernel must check that user-space did zero those bits. Otherwise
user-space may start using them when a future version of the architecture gives
them a meaning, but an older kernel version doesn't know it has to do extra
work, but still lets the bits from user-space through into the hardware.
If new bits do turn up, we can advertise a CAP that says that KVM supports
whatever that feature is.
>> (Background: VSESR is a 64bit register that holds the value to go in a 32bit
>> register. I suspect the top-half could get re-used for control values or
>> something we don't want to give user-space)
> do you mean when user-space get the VSESR value through KVM_GET_VCPU_EVENTS
> it only return the low-half 32 bits?
No, the kernel will only ever set a 24bit value here. If we force user-space to
only provide a 24bit value then we don't need to check it on read. We never read
the value back from hardware.
These high bits are RES0 at the moment, they may get used for something in the
future. As we are exposing this via a user-space ABI we need to make sure we
only expose the bits we understand today.
Thanks,
James
^ permalink raw reply
* [PATCH v4 6/6] dt-bindings: media: rcar-vin: Clarify optional props
From: Sakari Ailus @ 2018-06-12 15:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1528813566-17927-7-git-send-email-jacopo+renesas@jmondi.org>
Hi Jacopo,
On Tue, Jun 12, 2018 at 04:26:06PM +0200, Jacopo Mondi wrote:
> Add a note to the R-Car VIN interface bindings to clarify that all
> properties listed as generic properties in video-interfaces.txt can
> be included in port at 0 endpoint, but if not explicitly listed in the
> interface bindings documentation, they do not modify it behaviour.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
> index 8130849..03544c7 100644
> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> @@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on SoC.
> instances that are connected to external pins should have port 0.
>
> - Optional properties for endpoint nodes of port at 0:
> +
> + All properties described in [1] and which apply to the selected
> + media bus type could be optionally listed here to better describe
> + the current hardware configuration, but only the following ones do
> + actually modify the VIN interface behaviour:
> +
I don't think this should be needed. You should only have properties that
describe the hardware configuration in a given system.
> - hsync-active: see [1] for description. Default is active high.
> - vsync-active: see [1] for description. Default is active high.
> - data-enable-active: polarity of CLKENB signal, see [1] for
> --
> 2.7.4
>
--
Sakari Ailus
sakari.ailus at linux.intel.com
^ permalink raw reply
* [PATCH RESEND v4 2/2] arm/arm64: KVM: Add KVM_GET/SET_VCPU_EVENTS
From: gengdongjiu @ 2018-06-12 15:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6887237f-d252-5b9e-02cb-5a44fef27080@arm.com>
On 2018/6/12 23:29, James Morse wrote:
> Hi gengdongjiu,
>
> On 12/06/18 15:50, gengdongjiu wrote:
>> On 2018/6/11 21:36, James Morse wrote:
>>> On 08/06/18 20:48, Dongjiu Geng wrote:
>>>> For the migrating VMs, user space may need to know the exception
>>>> state. For example, in the machine A, KVM make an SError pending,
>>>> when migrate to B, KVM also needs to pend an SError.
>>>>
>>>> This new IOCTL exports user-invisible states related to SError.
>>>> Together with appropriate user space changes, user space can get/set
>>>> the SError exception state to do migrate/snapshot/suspend.
>
>
>>>> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
>>>> index caae484..c3e6975 100644
>>>> --- a/arch/arm/include/uapi/asm/kvm.h
>>>> +++ b/arch/arm/include/uapi/asm/kvm.h
>>>> @@ -124,6 +124,18 @@ struct kvm_sync_regs {
>>>> struct kvm_arch_memory_slot {
>>>> };
>>>>
>>>> +/* for KVM_GET/SET_VCPU_EVENTS */
>>>> +struct kvm_vcpu_events {
>>>> + struct {
>>>> + __u8 serror_pending;
>>>> + __u8 serror_has_esr;
>>>> + /* Align it to 8 bytes */
>>>> + __u8 pad[6];
>>>> + __u64 serror_esr;
>>>> + } exception;
>>>> + __u32 reserved[12];
>>>> +};
>>>> +
>>>
>>> You haven't defined __KVM_HAVE_VCPU_EVENTS for 32bit, so presumably this struct
>>> will never be used. Why is it here?
>
>> if not add it for 32 bits. the 32 arm platform will build Fail, whether you have good
>> idea to avoid this Failure if not add this struct for the 32 bit?
>
> How does this 32bit code build without this patch?
> If do you provide the struct, how will that code build with older headers?
>
> As far as I can see, this is what the __KVM_HAVE_VCPU_EVENTS define is for.
>
> This should be both, or neither. Having just the struct is useless.
It because the caller of kvm_arm_vcpu_get/set_events() is in "virt/kvm/arm/arm.c".
the virt/kvm/arm/arm.c will used by both arm64 and arm.
so It needs to add kvm_arm_vcpu_get/set_events() for the 32 bits, however, kvm_arm_vcpu_get/set_events() will directly return,
I attached the build erros below:
https://lkml.org/lkml/2018/6/4/918
https://lkml.org/lkml/2018/6/4/873
[..]
I will continue check below comments, thanks
^ permalink raw reply
* [PATCH v4 6/6] dt-bindings: media: rcar-vin: Clarify optional props
From: Laurent Pinchart @ 2018-06-12 16:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180612154553.kgqnqkwv3y6srivg@kekkonen.localdomain>
Hello,
On Tuesday, 12 June 2018 18:45:53 EEST Sakari Ailus wrote:
> On Tue, Jun 12, 2018 at 04:26:06PM +0200, Jacopo Mondi wrote:
> > Add a note to the R-Car VIN interface bindings to clarify that all
> > properties listed as generic properties in video-interfaces.txt can
> > be included in port at 0 endpoint, but if not explicitly listed in the
> > interface bindings documentation, they do not modify it behaviour.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >
> > Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > b/Documentation/devicetree/bindings/media/rcar_vin.txt index
> > 8130849..03544c7 100644
> > --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> > @@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on
> > SoC.
> >
> > instances that are connected to external pins should have port 0.
> >
> > - Optional properties for endpoint nodes of port at 0:
> > +
> > + All properties described in [1] and which apply to the selected
> > + media bus type could be optionally listed here to better describe
> > + the current hardware configuration, but only the following ones
> > do
> > + actually modify the VIN interface behaviour:
> > +
>
> I don't think this should be needed. You should only have properties that
> describe the hardware configuration in a given system.
I agree. Please list explicitly all properties that are applicable for this
device, and don't tell which one(s) are used by the driver.
> > - hsync-active: see [1] for description. Default is active high.
> > - vsync-active: see [1] for description. Default is active high.
> > - data-enable-active: polarity of CLKENB signal, see [1] for
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH v4 2/7] clk: at91: add I2S clock mux driver
From: Stephen Boyd @ 2018-06-12 16:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d2558674-9aee-8b08-293f-34754fd2b185@microchip.com>
Quoting Codrin Ciubotariu (2018-06-04 01:20:29)
> On 31.05.2018 18:26, Stephen Boyd wrote:
> > Quoting Codrin Ciubotariu (2018-05-25 05:34:23)
> >
> >> + .get_parent = clk_i2s_mux_get_parent,
> >> + .set_parent = clk_i2s_mux_set_parent,
> >> + .determine_rate = __clk_mux_determine_rate,
> >> +};
> >> +
> >> +static struct clk_hw * __init
> >> +at91_clk_i2s_mux_register(struct regmap *regmap, const char *name,
> >> + const char * const *parent_names,
> >> + unsigned int num_parents, u32 bus_id)
> >> +{
> >> + struct clk_init_data init = {};
> >> + struct clk_i2s_mux *i2s_ck;
> >> + int ret;
> >> +
> >> + i2s_ck = kzalloc(sizeof(*i2s_ck), GFP_KERNEL);
> >> + if (!i2s_ck)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + init.name = name;
> >> + init.ops = &clk_i2s_mux_ops;
> >> + init.parent_names = parent_names;
> >> + init.num_parents = num_parents;
> >> + init.flags = CLK_IGNORE_UNUSED;
> >
> > Really? Why?
>
> I am thinking that there is no need to gate this clock, since there is
> no way to gate this clock in HW.
This flag is not necessary if the clk can't be gated via hardware
control registers. Please remove the flag.
^ permalink raw reply
* [PATCH 2/6] PCI: iproc: Add INTx support with better modeling
From: Ray Jui @ 2018-06-12 17:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3ecc33ebe3db1f010f5c8938dfb87677@codeaurora.org>
On 6/12/2018 1:52 AM, poza at codeaurora.org wrote:
> On 2018-05-30 03:28, Ray Jui wrote:
>> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
>> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
>> INTD share the same interrupt line connected to the GIC in the system,
>> while the status of each INTx can be obtained through the INTX CSR
>> register
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>> ?drivers/pci/host/pcie-iproc-platform.c |? 2 +
>> ?drivers/pci/host/pcie-iproc.c????????? | 95
>> +++++++++++++++++++++++++++++++++-
>> ?drivers/pci/host/pcie-iproc.h????????? |? 6 +++
>> ?3 files changed, 101 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c
>> b/drivers/pci/host/pcie-iproc-platform.c
>> index e764a2a..7a51e6c 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -70,6 +70,8 @@ static int iproc_pcie_pltfm_probe(struct
>> platform_device *pdev)
>> ???? }
>> ???? pcie->base_addr = reg.start;
>>
>> +??? pcie->irq = platform_get_irq(pdev, 0);
>> +
>> ???? if (of_property_read_bool(np, "brcm,pcie-ob")) {
>> ???????? u32 val;
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c
>> b/drivers/pci/host/pcie-iproc.c
>> index 14f374d..0bd2c14 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -14,6 +14,7 @@
>> ?#include <linux/delay.h>
>> ?#include <linux/interrupt.h>
>> ?#include <linux/irqchip/arm-gic-v3.h>
>> +#include <linux/irqchip/chained_irq.h>
>> ?#include <linux/platform_device.h>
>> ?#include <linux/of_address.h>
>> ?#include <linux/of_pci.h>
>> @@ -264,6 +265,7 @@ enum iproc_pcie_reg {
>>
>> ???? /* enable INTx */
>> ???? IPROC_PCIE_INTX_EN,
>> +??? IPROC_PCIE_INTX_CSR,
>>
>> ???? /* outbound address mapping */
>> ???? IPROC_PCIE_OARR0,
>> @@ -305,6 +307,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = {
>> ???? [IPROC_PCIE_CFG_ADDR]??????? = 0x1f8,
>> ???? [IPROC_PCIE_CFG_DATA]??????? = 0x1fc,
>> ???? [IPROC_PCIE_INTX_EN]??????? = 0x330,
>> +??? [IPROC_PCIE_INTX_CSR]??????? = 0x334,
>> ???? [IPROC_PCIE_LINK_STATUS]??? = 0xf0c,
>> ?};
>>
>> @@ -316,6 +319,7 @@ static const u16 iproc_pcie_reg_paxb[] = {
>> ???? [IPROC_PCIE_CFG_ADDR]??????? = 0x1f8,
>> ???? [IPROC_PCIE_CFG_DATA]??????? = 0x1fc,
>> ???? [IPROC_PCIE_INTX_EN]??????? = 0x330,
>> +??? [IPROC_PCIE_INTX_CSR]??????? = 0x334,
>> ???? [IPROC_PCIE_OARR0]??????? = 0xd20,
>> ???? [IPROC_PCIE_OMAP0]??????? = 0xd40,
>> ???? [IPROC_PCIE_OARR1]??????? = 0xd28,
>> @@ -332,6 +336,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = {
>> ???? [IPROC_PCIE_CFG_ADDR]??????? = 0x1f8,
>> ???? [IPROC_PCIE_CFG_DATA]??????? = 0x1fc,
>> ???? [IPROC_PCIE_INTX_EN]??????? = 0x330,
>> +??? [IPROC_PCIE_INTX_CSR]??????? = 0x334,
>> ???? [IPROC_PCIE_OARR0]??????? = 0xd20,
>> ???? [IPROC_PCIE_OMAP0]??????? = 0xd40,
>> ???? [IPROC_PCIE_OARR1]??????? = 0xd28,
>> @@ -782,9 +787,90 @@ static int iproc_pcie_check_link(struct
>> iproc_pcie *pcie)
>> ???? return link_is_active ? 0 : -ENODEV;
>> ?}
>>
>> -static void iproc_pcie_enable(struct iproc_pcie *pcie)
>> +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned
>> int irq,
>> +?????????????????? irq_hw_number_t hwirq)
>> ?{
>> +??? irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
>> +??? irq_set_chip_data(irq, domain->host_data);
>> +
>> +??? return 0;
>> +}
>> +
>> +static const struct irq_domain_ops intx_domain_ops = {
>> +??? .map = iproc_pcie_intx_map,
>> +};
>> +
>> +static void iproc_pcie_isr(struct irq_desc *desc)
>> +{
>> +??? struct irq_chip *chip = irq_desc_get_chip(desc);
>> +??? struct iproc_pcie *pcie;
>> +??? struct device *dev;
>> +??? unsigned long status;
>> +??? u32 bit, virq;
>> +
>> +??? chained_irq_enter(chip, desc);
>> +??? pcie = irq_desc_get_handler_data(desc);
>> +??? dev = pcie->dev;
>> +
>> +??? /* go through INTx A, B, C, D until all interrupts are handled */
>> +??? while ((status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR) &
>> +??????? SYS_RC_INTX_MASK) != 0) {
>> +??????? for_each_set_bit(bit, &status, PCI_NUM_INTX) {
>> +??????????? virq = irq_find_mapping(pcie->irq_domain, bit + 1);
>> +??????????? if (virq)
>> +??????????????? generic_handle_irq(virq);
>> +??????????? else
>> +??????????????? dev_err(dev, "unexpected INTx%u\n", bit);
>> +??????? }
>> +??? }
>> +
>
> Are these level or edge interrupts ? although I see DT says: IRQ_TYPE_NONE
DT entries should be fixed to trigger on level HIGH like Florian and I
discussed on the mailing list yesterday. It looks like you are missing a
lot of discussions on the mailing list. Could you please help to make
sure you read all the existing discussions when commenting on a patch?
> do you not need to clear interrupt status bits in IPROC_PCIE_INTX_CSR ?
>
RO
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox