* [PATCH v21 4/9] fpga-mgr: add fpga image information struct
From: Alan Tull @ 2016-10-25 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025145540.3722-1-atull@opensource.altera.com>
This patch adds a minor change in the FPGA Manager API
to hold information that is specific to an FPGA image
file. This change is expected to bring little, if any,
pain. The socfpga and zynq drivers are fixed up in
this patch.
An FPGA image file will have particulars that affect how the
image is programmed to the FPGA. One example is that
current 'flags' currently has one bit which shows whether the
FPGA image was built for full reconfiguration or partial
reconfiguration. Another example is timeout values for
enabling or disabling the bridges in the FPGA. As the
complexity of the FPGA design increases, the bridges in the
FPGA may take longer times to enable or disable.
This patch adds a new 'struct fpga_image_info', moves the
current 'u32 flags' to it. Two other image-specific u32's
are added for the bridge enable/disable timeouts. The FPGA
Manager API functions are changed, replacing the 'u32 flag'
parameter with a pointer to struct fpga_image_info.
Subsequent patches fix the existing low level FPGA manager
drivers.
Signed-off-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
---
v19: Added in v19 of this patchset
v20: Squashed patches that change API for socfpga and zynq
v21: Add Moritz' ack
s/Mangager/Manager/
---
drivers/fpga/fpga-mgr.c | 17 +++++++++--------
drivers/fpga/socfpga.c | 7 ++++---
drivers/fpga/zynq-fpga.c | 10 ++++++----
include/linux/fpga/fpga-mgr.h | 23 +++++++++++++++++++----
4 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 953dc91..c58b4c4 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -32,7 +32,7 @@ static struct class *fpga_mgr_class;
/**
* fpga_mgr_buf_load - load fpga from image in buffer
* @mgr: fpga manager
- * @flags: flags setting fpga confuration modes
+ * @info: fpga image specific information
* @buf: buffer contain fpga image
* @count: byte count of buf
*
@@ -43,8 +43,8 @@ static struct class *fpga_mgr_class;
*
* Return: 0 on success, negative error code otherwise.
*/
-int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
- size_t count)
+int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
+ const char *buf, size_t count)
{
struct device *dev = &mgr->dev;
int ret;
@@ -55,7 +55,7 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
* ready to receive an FPGA image.
*/
mgr->state = FPGA_MGR_STATE_WRITE_INIT;
- ret = mgr->mops->write_init(mgr, flags, buf, count);
+ ret = mgr->mops->write_init(mgr, info, buf, count);
if (ret) {
dev_err(dev, "Error preparing FPGA for writing\n");
mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
@@ -78,7 +78,7 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
* steps to finish and set the FPGA into operating mode.
*/
mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
- ret = mgr->mops->write_complete(mgr, flags);
+ ret = mgr->mops->write_complete(mgr, info);
if (ret) {
dev_err(dev, "Error after writing image data to FPGA\n");
mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
@@ -93,7 +93,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
/**
* fpga_mgr_firmware_load - request firmware and load to fpga
* @mgr: fpga manager
- * @flags: flags setting fpga confuration modes
+ * @info: fpga image specific information
* @image_name: name of image file on the firmware search path
*
* Request an FPGA image using the firmware class, then write out to the FPGA.
@@ -103,7 +103,8 @@ EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
*
* Return: 0 on success, negative error code otherwise.
*/
-int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
+int fpga_mgr_firmware_load(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *image_name)
{
struct device *dev = &mgr->dev;
@@ -121,7 +122,7 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
return ret;
}
- ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
+ ret = fpga_mgr_buf_load(mgr, info, fw->data, fw->size);
release_firmware(fw);
diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index 27d2ff2..b6672e6 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -407,13 +407,14 @@ static int socfpga_fpga_reset(struct fpga_manager *mgr)
/*
* Prepare the FPGA to receive the configuration data.
*/
-static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr, u32 flags,
+static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *buf, size_t count)
{
struct socfpga_fpga_priv *priv = mgr->priv;
int ret;
- if (flags & FPGA_MGR_PARTIAL_RECONFIG) {
+ if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
return -EINVAL;
}
@@ -478,7 +479,7 @@ static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
}
static int socfpga_fpga_ops_configure_complete(struct fpga_manager *mgr,
- u32 flags)
+ struct fpga_image_info *info)
{
struct socfpga_fpga_priv *priv = mgr->priv;
u32 status;
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index c2fb412..249682e 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -175,7 +175,8 @@ static irqreturn_t zynq_fpga_isr(int irq, void *data)
return IRQ_HANDLED;
}
-static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
+static int zynq_fpga_ops_write_init(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *buf, size_t count)
{
struct zynq_fpga_priv *priv;
@@ -189,7 +190,7 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
return err;
/* don't globally reset PL if we're doing partial reconfig */
- if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+ if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
/* assert AXI interface resets */
regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
FPGA_RST_ALL_MASK);
@@ -343,7 +344,8 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,
return err;
}
-static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
+static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr,
+ struct fpga_image_info *info)
{
struct zynq_fpga_priv *priv = mgr->priv;
int err;
@@ -364,7 +366,7 @@ static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr, u32 flags)
return err;
/* for the partial reconfig case we didn't touch the level shifters */
- if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
+ if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
/* enable level shifters from PL to PS */
regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
LVL_SHFTR_ENABLE_PL_TO_PS);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0940bf4..040b86d 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -69,6 +69,18 @@ enum fpga_mgr_states {
#define FPGA_MGR_PARTIAL_RECONFIG BIT(0)
/**
+ * struct fpga_image_info - information specific to a FPGA image
+ * @flags: boolean flags as defined above
+ * @enable_timeout_us: maximum time to enable traffic through bridge (uSec)
+ * @disable_timeout_us: maximum time to disable traffic through bridge (uSec)
+ */
+struct fpga_image_info {
+ u32 flags;
+ u32 enable_timeout_us;
+ u32 disable_timeout_us;
+};
+
+/**
* struct fpga_manager_ops - ops for low level fpga manager drivers
* @state: returns an enum value of the FPGA's state
* @write_init: prepare the FPGA to receive confuration data
@@ -82,10 +94,12 @@ enum fpga_mgr_states {
*/
struct fpga_manager_ops {
enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
- int (*write_init)(struct fpga_manager *mgr, u32 flags,
+ int (*write_init)(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *buf, size_t count);
int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
- int (*write_complete)(struct fpga_manager *mgr, u32 flags);
+ int (*write_complete)(struct fpga_manager *mgr,
+ struct fpga_image_info *info);
void (*fpga_remove)(struct fpga_manager *mgr);
};
@@ -109,10 +123,11 @@ struct fpga_manager {
#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
+int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
const char *buf, size_t count);
-int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
+int fpga_mgr_firmware_load(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *image_name);
struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
--
2.10.1
^ permalink raw reply related
* [PATCH v21 3/9] add sysfs document for fpga bridge class
From: Alan Tull @ 2016-10-25 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025145540.3722-1-atull@opensource.altera.com>
Add documentation for new FPGA bridge class's sysfs interface.
Signed-off-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
--
v15: Document added in v15 of patch set
v16: No change to this patch in v16 of patch set
v17: No change to this patch in v17 of patch set
v18: No change to this patch in v18 of patch set
v19: No change to this patch in this version of patch set
v20: Added Moritz' ack
v21: No change for this patch in v21 of patchset
---
Documentation/ABI/testing/sysfs-class-fpga-bridge | 11 +++++++++++
1 file changed, 11 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
diff --git a/Documentation/ABI/testing/sysfs-class-fpga-bridge b/Documentation/ABI/testing/sysfs-class-fpga-bridge
new file mode 100644
index 0000000..312ae2c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-bridge
@@ -0,0 +1,11 @@
+What: /sys/class/fpga_bridge/<bridge>/name
+Date: January 2016
+KernelVersion: 4.5
+Contact: Alan Tull <atull@opensource.altera.com>
+Description: Name of low level FPGA bridge driver.
+
+What: /sys/class/fpga_bridge/<bridge>/state
+Date: January 2016
+KernelVersion: 4.5
+Contact: Alan Tull <atull@opensource.altera.com>
+Description: Show bridge state as "enabled" or "disabled"
--
2.10.1
^ permalink raw reply related
* [PATCH v21 2/9] doc: fpga-mgr: add fpga image info to api
From: Alan Tull @ 2016-10-25 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025145540.3722-1-atull@opensource.altera.com>
This patch adds a minor change in the FPGA Manager API
to hold information that is specific to an FPGA image
file. This change is expected to bring little, if any,
pain.
An FPGA image file will have particulars that affect how the
image is programmed to the FPGA. One example is that
current 'flags' currently has one bit which shows whether the
FPGA image was built for full reconfiguration or partial
reconfiguration. Another example is timeout values for
enabling or disabling the bridges in the FPGA. As the
complexity of the FPGA design increases, the bridges in the
FPGA may take longer times to enable or disable.
This patch documents the change in the FPGA Manager API
functions, replacing the 'u32 flag' parameter with a pointer
to struct fpga_image_info.
Signed-off-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
---
v19: Added in v19 of this patchset
v20: No change for this patch in v20 of patchset
v21: s/Mangager/Manager/
Add Moritz' ack
---
Documentation/fpga/fpga-mgr.txt | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index ce3e84f..9227e3f 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -18,21 +18,25 @@ API Functions:
To program the FPGA from a file or from a buffer:
-------------------------------------------------
- int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags,
+ int fpga_mgr_buf_load(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *buf, size_t count);
Load the FPGA from an image which exists as a buffer in memory.
- int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
+ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
const char *image_name);
Load the FPGA from an image which exists as a file. The image file must be on
-the firmware search path (see the firmware class documentation).
-
-For both these functions, flags == 0 for normal full reconfiguration or
-FPGA_MGR_PARTIAL_RECONFIG for partial reconfiguration. If successful, the FPGA
-ends up in operating mode. Return 0 on success or a negative error code.
+the firmware search path (see the firmware class documentation). If successful,
+the FPGA ends up in operating mode. Return 0 on success or a negative error
+code.
+A FPGA design contained in a FPGA image file will likely have particulars that
+affect how the image is programmed to the FPGA. These are contained in struct
+fpga_image_info. Currently the only such particular is a single flag bit
+indicating whether the image is for full or partial reconfiguration.
To get/put a reference to a FPGA manager:
-----------------------------------------
@@ -70,8 +74,11 @@ struct device_node *mgr_node = ...
char *buf = ...
int count = ...
+/* struct with information about the FPGA image to program. */
+struct fpga_image_info info;
+
/* flags indicates whether to do full or partial reconfiguration */
-int flags = 0;
+info.flags = 0;
int ret;
@@ -79,7 +86,7 @@ int ret;
struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
/* Load the buffer to the FPGA */
-ret = fpga_mgr_buf_load(mgr, flags, buf, count);
+ret = fpga_mgr_buf_load(mgr, &info, buf, count);
/* Release the FPGA manager */
fpga_mgr_put(mgr);
@@ -96,8 +103,11 @@ struct device_node *mgr_node = ...
/* FPGA image is in this file which is in the firmware search path */
const char *path = "fpga-image-9.rbf"
+/* struct with information about the FPGA image to program. */
+struct fpga_image_info info;
+
/* flags indicates whether to do full or partial reconfiguration */
-int flags = 0;
+info.flags = 0;
int ret;
@@ -105,7 +115,7 @@ int ret;
struct fpga_manager *mgr = of_fpga_mgr_get(mgr_node);
/* Get the firmware image (path) and load it to the FPGA */
-ret = fpga_mgr_firmware_load(mgr, flags, path);
+ret = fpga_mgr_firmware_load(mgr, &info, path);
/* Release the FPGA manager */
fpga_mgr_put(mgr);
--
2.10.1
^ permalink raw reply related
* [PATCH v21 1/9] fpga: add bindings document for fpga region
From: Alan Tull @ 2016-10-25 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025145540.3722-1-atull@opensource.altera.com>
New bindings document for FPGA Region to support programming
FPGA's under Device Tree control
Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v9: initial version added to this patchset
v10: s/fpga/FPGA/g
replace DT overlay example with slightly more complicated example
move to staging/simple-fpga-bus
v11: No change in this patch for v11 of the patch set
v12: Moved out of staging.
Changed to use FPGA bridges framework instead of resets
for bridges.
v13: bridge at 0xff20000 -> bridge at ff200000, etc
Leave out directly talking about overlays
Remove regs and clocks directly under simple-fpga-bus in example
Use common "firmware-name" binding instead of "fpga-firmware"
v14: Use firmware-name in bindings description
Call it FPGA Area
Remove bindings that specify FPGA Manager and FPGA Bridges
v15: Cleanup as per Rob's comments
Combine usage doc with bindings document
Document as being Altera specific
Additions and changes to add FPGA Bus
v16: Reworked to document FPGA Regions
rename altera-fpga-bus-fpga-area.txt -> fpga-region.txt
Remove references that made it sound exclusive to Altera
Remove altr, prefix from fpga-bus and fpga-area compatible strings
Added Moritz' usage example with Xilinx
Cleaned up unit addresses
v17: Lots of rewrites to try to make things clearer
Clarify that overlay can be rejected if FPGA isn't programmed
Add external-fpga-config binding already used in u-boot
Change partial-reconfig binding to partial-fpga-config to align
with existing u-boot binding format *-fpga-config
Add a document from Xilinx' website
v18: Fix node names underscores to be hyphens
Fix copy/pasted duplicate nodes in diagram
v19: Fix more underscores
Make FPGA regions to be children of bridges
General cleanup and clarification
v20: Add Rob's reviewed-by
v21: No change for this patch in v21 of patchset
---
.../devicetree/bindings/fpga/fpga-region.txt | 494 +++++++++++++++++++++
1 file changed, 494 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
new file mode 100644
index 0000000..3b32ba1
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
@@ -0,0 +1,494 @@
+FPGA Region Device Tree Binding
+
+Alan Tull 2016
+
+ CONTENTS
+ - Introduction
+ - Terminology
+ - Sequence
+ - FPGA Region
+ - Supported Use Models
+ - Device Tree Examples
+ - Constraints
+
+
+Introduction
+============
+
+FPGA Regions represent FPGA's and partial reconfiguration regions of FPGA's in
+the Device Tree. FPGA Regions provide a way to program FPGAs under device tree
+control.
+
+This device tree binding document hits some of the high points of FPGA usage and
+attempts to include terminology used by both major FPGA manufacturers. This
+document isn't a replacement for any manufacturers specifications for FPGA
+usage.
+
+
+Terminology
+===========
+
+Full Reconfiguration
+ * The entire FPGA is programmed.
+
+Partial Reconfiguration (PR)
+ * A section of an FPGA is reprogrammed while the rest of the FPGA is not
+ affected.
+ * Not all FPGA's support PR.
+
+Partial Reconfiguration Region (PRR)
+ * Also called a "reconfigurable partition"
+ * A PRR is a specific section of a FPGA reserved for reconfiguration.
+ * A base (or static) FPGA image may create a set of PRR's that later may
+ be independently reprogrammed many times.
+ * The size and specific location of each PRR is fixed.
+ * The connections at the edge of each PRR are fixed. The image that is loaded
+ into a PRR must fit and must use a subset of the region's connections.
+ * The busses within the FPGA are split such that each region gets its own
+ branch that may be gated independently.
+
+Persona
+ * Also called a "partial bit stream"
+ * An FPGA image that is designed to be loaded into a PRR. There may be
+ any number of personas designed to fit into a PRR, but only one at at time
+ may be loaded.
+ * A persona may create more regions.
+
+FPGA Bridge
+ * FPGA Bridges gate bus signals between a host and FPGA.
+ * FPGA Bridges should be disabled while the FPGA is being programmed to
+ prevent spurious signals on the cpu bus and to the soft logic.
+ * FPGA bridges may be actual hardware or soft logic on an FPGA.
+ * During Full Reconfiguration, hardware bridges between the host and FPGA
+ will be disabled.
+ * During Partial Reconfiguration of a specific region, that region's bridge
+ will be used to gate the busses. Traffic to other regions is not affected.
+ * In some implementations, the FPGA Manager transparantly handles gating the
+ buses, eliminating the need to show the hardware FPGA bridges in the
+ device tree.
+ * An FPGA image may create a set of reprogrammable regions, each having its
+ own bridge and its own split of the busses in the FPGA.
+
+FPGA Manager
+ * An FPGA Manager is a hardware block that programs an FPGA under the control
+ of a host processor.
+
+Base Image
+ * Also called the "static image"
+ * An FPGA image that is designed to do full reconfiguration of the FPGA.
+ * A base image may set up a set of partial reconfiguration regions that may
+ later be reprogrammed.
+
+ ---------------- ----------------------------------
+ | Host CPU | | FPGA |
+ | | | |
+ | ----| | ----------- -------- |
+ | | H | | |==>| Bridge0 |<==>| PRR0 | |
+ | | W | | | ----------- -------- |
+ | | | | | |
+ | | B |<=====>|<==| ----------- -------- |
+ | | R | | |==>| Bridge1 |<==>| PRR1 | |
+ | | I | | | ----------- -------- |
+ | | D | | | |
+ | | G | | | ----------- -------- |
+ | | E | | |==>| Bridge2 |<==>| PRR2 | |
+ | ----| | ----------- -------- |
+ | | | |
+ ---------------- ----------------------------------
+
+Figure 1: An FPGA set up with a base image that created three regions. Each
+region (PRR0-2) gets its own split of the busses that is independently gated by
+a soft logic bridge (Bridge0-2) in the FPGA. The contents of each PRR can be
+reprogrammed independently while the rest of the system continues to function.
+
+
+Sequence
+========
+
+When a DT overlay that targets a FPGA Region is applied, the FPGA Region will
+do the following:
+
+ 1. Disable appropriate FPGA bridges.
+ 2. Program the FPGA using the FPGA manager.
+ 3. Enable the FPGA bridges.
+ 4. The Device Tree overlay is accepted into the live tree.
+ 5. Child devices are populated.
+
+When the overlay is removed, the child nodes will be removed and the FPGA Region
+will disable the bridges.
+
+
+FPGA Region
+===========
+
+FPGA Regions represent FPGA's and FPGA PR regions in the device tree. An FPGA
+Region brings together the elements needed to program on a running system and
+add the child devices:
+
+ * FPGA Manager
+ * FPGA Bridges
+ * image-specific information needed to to the programming.
+ * child nodes
+
+The intended use is that a Device Tree overlay (DTO) can be used to reprogram an
+FPGA while an operating system is running.
+
+An FPGA Region that exists in the live Device Tree reflects the current state.
+If the live tree shows a "firmware-name" property or child nodes under a FPGA
+Region, the FPGA already has been programmed. A DTO that targets a FPGA Region
+and adds the "firmware-name" property is taken as a request to reprogram the
+FPGA. After reprogramming is successful, the overlay is accepted into the live
+tree.
+
+The base FPGA Region in the device tree represents the FPGA and supports full
+reconfiguration. It must include a phandle to an FPGA Manager. The base
+FPGA region will be the child of one of the hardware bridges (the bridge that
+allows register access) between the cpu and the FPGA. If there are more than
+one bridge to control during FPGA programming, the region will also contain a
+list of phandles to the additional hardware FPGA Bridges.
+
+For partial reconfiguration (PR), each PR region will have an FPGA Region.
+These FPGA regions are children of FPGA bridges which are then children of the
+base FPGA region. The "Full Reconfiguration to add PRR's" example below shows
+this.
+
+If an FPGA Region does not specify a FPGA Manager, it will inherit the FPGA
+Manager specified by its ancestor FPGA Region. This supports both the case
+where the same FPGA Manager is used for all of a FPGA as well the case where
+a different FPGA Manager is used for each region.
+
+FPGA Regions do not inherit their ancestor FPGA regions' bridges. This prevents
+shutting down bridges that are upstream from the other active regions while one
+region is getting reconfigured (see Figure 1 above). During PR, the FPGA's
+hardware bridges remain enabled. The PR regions' bridges will be FPGA bridges
+within the static image of the FPGA.
+
+Required properties:
+- compatible : should contain "fpga-region"
+- fpga-mgr : should contain a phandle to an FPGA Manager. Child FPGA Regions
+ inherit this property from their ancestor regions. A fpga-mgr property
+ in a region will override any inherited FPGA manager.
+- #address-cells, #size-cells, ranges : must be present to handle address space
+ mapping for child nodes.
+
+Optional properties:
+- firmware-name : should contain the name of an FPGA image file located on the
+ firmware search path. If this property shows up in a live device tree
+ it indicates that the FPGA has already been programmed with this image.
+ If this property is in an overlay targeting a FPGA region, it is a
+ request to program the FPGA with that image.
+- fpga-bridges : should contain a list of phandles to FPGA Bridges that must be
+ controlled during FPGA programming along with the parent FPGA bridge.
+ This property is optional if the FPGA Manager handles the bridges.
+ If the fpga-region is the child of a fpga-bridge, the list should not
+ contain the parent bridge.
+- partial-fpga-config : boolean, set if partial reconfiguration is to be done,
+ otherwise full reconfiguration is done.
+- external-fpga-config : boolean, set if the FPGA has already been configured
+ prior to OS boot up.
+- region-unfreeze-timeout-us : The maximum time in microseconds to wait for
+ bridges to successfully become enabled after the region has been
+ programmed.
+- region-freeze-timeout-us : The maximum time in microseconds to wait for
+ bridges to successfully become disabled before the region has been
+ programmed.
+- child nodes : devices in the FPGA after programming.
+
+In the example below, when an overlay is applied targeting fpga-region0,
+fpga_mgr is used to program the FPGA. Two bridges are controlled during
+programming: the parent fpga_bridge0 and fpga_bridge1. Because the region is
+the child of fpga_bridge0, only fpga_bridge1 needs to be specified in the
+fpga-bridges property. During programming, these bridges are disabled, the
+firmware specified in the overlay is loaded to the FPGA using the FPGA manager
+specified in the region. If FPGA programming succeeds, the bridges are
+reenabled and the overlay makes it into the live device tree. The child devices
+are then populated. If FPGA programming fails, the bridges are left disabled
+and the overlay is rejected. The overlay's ranges property maps the lwhps
+bridge's region (0xff200000) and the hps bridge's region (0xc0000000) for use by
+the two child devices.
+
+Example:
+Base tree contains:
+
+ fpga_mgr: fpga-mgr at ff706000 {
+ compatible = "altr,socfpga-fpga-mgr";
+ reg = <0xff706000 0x1000
+ 0xffb90000 0x20>;
+ interrupts = <0 175 4>;
+ };
+
+ fpga_bridge0: fpga-bridge at ff400000 {
+ compatible = "altr,socfpga-lwhps2fpga-bridge";
+ reg = <0xff400000 0x100000>;
+ resets = <&rst LWHPS2FPGA_RESET>;
+ clocks = <&l4_main_clk>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ fpga_region0: fpga-region0 {
+ compatible = "fpga-region";
+ fpga-mgr = <&fpga_mgr>;
+ };
+ };
+
+ fpga_bridge1: fpga-bridge at ff500000 {
+ compatible = "altr,socfpga-hps2fpga-bridge";
+ reg = <0xff500000 0x10000>;
+ resets = <&rst HPS2FPGA_RESET>;
+ clocks = <&l4_main_clk>;
+ };
+
+Overlay contains:
+
+/dts-v1/ /plugin/;
+/ {
+ fragment at 0 {
+ target = <&fpga_region0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ __overlay__ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ firmware-name = "soc_system.rbf";
+ fpga-bridges = <&fpga_bridge1>;
+ ranges = <0x20000 0xff200000 0x100000>,
+ <0x0 0xc0000000 0x20000000>;
+
+ gpio at 10040 {
+ compatible = "altr,pio-1.0";
+ reg = <0x10040 0x20>;
+ altr,gpio-bank-width = <4>;
+ #gpio-cells = <2>;
+ clocks = <2>;
+ gpio-controller;
+ };
+
+ onchip-memory {
+ device_type = "memory";
+ compatible = "altr,onchipmem-15.1";
+ reg = <0x0 0x10000>;
+ };
+ };
+ };
+};
+
+
+Supported Use Models
+====================
+
+In all cases the live DT must have the FPGA Manager, FPGA Bridges (if any), and
+a FPGA Region. The target of the Device Tree Overlay is the FPGA Region. Some
+uses are specific to a FPGA device.
+
+ * No FPGA Bridges
+ In this case, the FPGA Manager which programs the FPGA also handles the
+ bridges behind the scenes. No FPGA Bridge devices are needed for full
+ reconfiguration.
+
+ * Full reconfiguration with hardware bridges
+ In this case, there are hardware bridges between the processor and FPGA that
+ need to be controlled during full reconfiguration. Before the overlay is
+ applied, the live DT must include the FPGA Manager, FPGA Bridges, and a
+ FPGA Region. The FPGA Region is the child of the bridge that allows
+ register access to the FPGA. Additional bridges may be listed in a
+ fpga-bridges property in the FPGA region or in the device tree overlay.
+
+ * Partial reconfiguration with bridges in the FPGA
+ In this case, the FPGA will have one or more PRR's that may be programmed
+ separately while the rest of the FPGA can remain active. To manage this,
+ bridges need to exist in the FPGA that can gate the buses going to each FPGA
+ region while the buses are enabled for other sections. Before any partial
+ reconfiguration can be done, a base FPGA image must be loaded which includes
+ PRR's with FPGA bridges. The device tree should have a FPGA region for each
+ PRR.
+
+Device Tree Examples
+====================
+
+The intention of this section is to give some simple examples, focusing on
+the placement of the elements detailed above, especially:
+ * FPGA Manager
+ * FPGA Bridges
+ * FPGA Region
+ * ranges
+ * target-path or target
+
+For the purposes of this section, I'm dividing the Device Tree into two parts,
+each with its own requirements. The two parts are:
+ * The live DT prior to the overlay being added
+ * The DT overlay
+
+The live Device Tree must contain an FPGA Region, an FPGA Manager, and any FPGA
+Bridges. The FPGA Region's "fpga-mgr" property specifies the manager by phandle
+to handle programming the FPGA. If the FPGA Region is the child of another FPGA
+Region, the parent's FPGA Manager is used. If FPGA Bridges need to be involved,
+they are specified in the FPGA Region by the "fpga-bridges" property. During
+FPGA programming, the FPGA Region will disable the bridges that are in its
+"fpga-bridges" list and will re-enable them after FPGA programming has
+succeeded.
+
+The Device Tree Overlay will contain:
+ * "target-path" or "target"
+ The insertion point where the the contents of the overlay will go into the
+ live tree. target-path is a full path, while target is a phandle.
+ * "ranges"
+ The address space mapping from processor to FPGA bus(ses).
+ * "firmware-name"
+ Specifies the name of the FPGA image file on the firmware search
+ path. The search path is described in the firmware class documentation.
+ * "partial-fpga-config"
+ This binding is a boolean and should be present if partial reconfiguration
+ is to be done.
+ * child nodes corresponding to hardware that will be loaded in this region of
+ the FPGA.
+
+Device Tree Example: Full Reconfiguration without Bridges
+=========================================================
+
+Live Device Tree contains:
+ fpga_mgr0: fpga-mgr at f8007000 {
+ compatible = "xlnx,zynq-devcfg-1.0";
+ reg = <0xf8007000 0x100>;
+ interrupt-parent = <&intc>;
+ interrupts = <0 8 4>;
+ clocks = <&clkc 12>;
+ clock-names = "ref_clk";
+ syscon = <&slcr>;
+ };
+
+ fpga_region0: fpga-region0 {
+ compatible = "fpga-region";
+ fpga-mgr = <&fpga_mgr0>;
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ ranges;
+ };
+
+DT Overlay contains:
+/dts-v1/ /plugin/;
+/ {
+fragment at 0 {
+ target = <&fpga_region0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ __overlay__ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ firmware-name = "zynq-gpio.bin";
+
+ gpio1: gpio at 40000000 {
+ compatible = "xlnx,xps-gpio-1.00.a";
+ reg = <0x40000000 0x10000>;
+ gpio-controller;
+ #gpio-cells = <0x2>;
+ xlnx,gpio-width= <0x6>;
+ };
+ };
+};
+
+Device Tree Example: Full Reconfiguration to add PRR's
+======================================================
+
+The base FPGA Region is specified similar to the first example above.
+
+This example programs the FPGA to have two regions that can later be partially
+configured. Each region has its own bridge in the FPGA fabric.
+
+DT Overlay contains:
+/dts-v1/ /plugin/;
+/ {
+ fragment at 0 {
+ target = <&fpga_region0>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ __overlay__ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ firmware-name = "base.rbf";
+
+ fpga-bridge at 4400 {
+ compatible = "altr,freeze-bridge";
+ reg = <0x4400 0x10>;
+
+ fpga_region1: fpga-region1 {
+ compatible = "fpga-region";
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ ranges;
+ };
+ };
+
+ fpga-bridge at 4420 {
+ compatible = "altr,freeze-bridge";
+ reg = <0x4420 0x10>;
+
+ fpga_region2: fpga-region2 {
+ compatible = "fpga-region";
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ ranges;
+ };
+ };
+ };
+ };
+};
+
+Device Tree Example: Partial Reconfiguration
+============================================
+
+This example reprograms one of the PRR's set up in the previous example.
+
+The sequence that occurs when this overlay is similar to the above, the only
+differences are that the FPGA is partially reconfigured due to the
+"partial-fpga-config" boolean and the only bridge that is controlled during
+programming is the FPGA based bridge of fpga_region1.
+
+/dts-v1/ /plugin/;
+/ {
+ fragment at 0 {
+ target = <&fpga_region1>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ __overlay__ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ firmware-name = "soc_image2.rbf";
+ partial-fpga-config;
+
+ gpio at 10040 {
+ compatible = "altr,pio-1.0";
+ reg = <0x10040 0x20>;
+ clocks = <0x2>;
+ altr,gpio-bank-width = <0x4>;
+ resetvalue = <0x0>;
+ #gpio-cells = <0x2>;
+ gpio-controller;
+ };
+ };
+ };
+};
+
+Constraints
+===========
+
+It is beyond the scope of this document to fully describe all the FPGA design
+constraints required to make partial reconfiguration work[1] [2] [3], but a few
+deserve quick mention.
+
+A persona must have boundary connections that line up with those of the partion
+or region it is designed to go into.
+
+During programming, transactions through those connections must be stopped and
+the connections must be held at a fixed logic level. This can be achieved by
+FPGA Bridges that exist on the FPGA fabric prior to the partial reconfiguration.
+
+--
+[1] www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/ug/ug_partrecon.pdf
+[2] tspace.library.utoronto.ca/bitstream/1807/67932/1/Byma_Stuart_A_201411_MAS_thesis.pdf
+[3] http://www.xilinx.com/support/documentation/sw_manuals/xilinx14_1/ug702.pdf
--
2.10.1
^ permalink raw reply related
* [PATCH v21 0/9] Device Tree support for FPGA Programming
From: Alan Tull @ 2016-10-25 14:55 UTC (permalink / raw)
To: linux-arm-kernel
This patchset supports FPGA programming under the control
of Device Tree overlays.
Changes since v20 are minor:
* s/Mangager/Manager/
* altera-hps2fpga bridge: remove a clk_put
* altera-hps2fpga bridge: don't need to look up l3regs for f2h
bridge
The following were acked so they are no longer in this patch set:
* DT bindings for Altera Freeze Bridges
https://patchwork.kernel.org/patch/9379803/
* DT bindings for Altera SOCFPGA bridges
https://patchwork.kernel.org/patch/9226093/
* DT bindings for Arria 10 FPGA Mgr
https://patchwork.kernel.org/patch/9226111/
* "[PATCH v3] of/overlay: add of overlay notifications"
https://patchwork.kernel.org/patch/8493481/
The patchset is dependent on:
* Pantelis Antonious's dtc changes for dynamic device tree.
https://github.com/pantoniou/dtc.git
* Pantelis' configfs interface patches and fixes
https://github.com/pantoniou/linux-beagle-track-mainline
Alan
Alan Tull (9):
fpga: add bindings document for fpga region
doc: fpga-mgr: add fpga image info to api
add sysfs document for fpga bridge class
fpga-mgr: add fpga image information struct
fpga: add fpga bridge framework
fpga: fpga-region: device tree control for FPGA
ARM: socfpga: fpga bridge driver support
fpga: add altera freeze bridge support
fpga-manager: Add Socfpga Arria10 support
Documentation/ABI/testing/sysfs-class-fpga-bridge | 11 +
.../devicetree/bindings/fpga/fpga-region.txt | 494 +++++++++++++++++
Documentation/fpga/fpga-mgr.txt | 32 +-
drivers/fpga/Kconfig | 36 ++
drivers/fpga/Makefile | 9 +
drivers/fpga/altera-fpga2sdram.c | 180 ++++++
drivers/fpga/altera-freeze-bridge.c | 273 ++++++++++
drivers/fpga/altera-hps2fpga.c | 222 ++++++++
drivers/fpga/fpga-bridge.c | 395 ++++++++++++++
drivers/fpga/fpga-mgr.c | 17 +-
drivers/fpga/fpga-region.c | 603 +++++++++++++++++++++
drivers/fpga/socfpga-a10.c | 556 +++++++++++++++++++
drivers/fpga/socfpga.c | 7 +-
drivers/fpga/zynq-fpga.c | 10 +-
include/linux/fpga/fpga-bridge.h | 60 ++
include/linux/fpga/fpga-mgr.h | 25 +-
16 files changed, 2900 insertions(+), 30 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-bridge
create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
create mode 100644 drivers/fpga/altera-fpga2sdram.c
create mode 100644 drivers/fpga/altera-freeze-bridge.c
create mode 100644 drivers/fpga/altera-hps2fpga.c
create mode 100644 drivers/fpga/fpga-bridge.c
create mode 100644 drivers/fpga/fpga-region.c
create mode 100644 drivers/fpga/socfpga-a10.c
create mode 100644 include/linux/fpga/fpga-bridge.h
--
2.10.1
^ permalink raw reply
* [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Marc Zyngier @ 2016-10-25 14:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477405332.2482.87.camel@baylibre.com>
On 25/10/16 15:22, Jerome Brunet wrote:
> On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote:
>> On 25/10/16 14:08, Jerome Brunet wrote:
>>>
>>> On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
>>>>
>>>>>
>>>>>
>>>> On 25/10/16 10:14, Linus Walleij wrote:
>>>>>
>>>>>
>>>>> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib
>>>>> re.c
>>>>> om> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Isn't this usecase (also as described in the cover letter)
>>>>>>> a
>>>>>>> textbook
>>>>>>> example of when you should be using hierarchical irqdomain?
>>>>>>>
>>>>>>> Please check with Marc et al on hierarchical irqdomains.
>>>>>>
>>>>>> Linus,
>>>>>> Do you mean I should create a new hierarchical irqdomains in
>>>>>> each
>>>>>> of
>>>>>> the two pinctrl instances we have in these SoC, these domains
>>>>>> being
>>>>>> stacked on the one I just added for controller in irqchip ?
>>>>>>
>>>>>> I did not understand this is what you meant when I asked you
>>>>>> the
>>>>>> question at ELCE.
>>>>>
>>>>> Honestly, I do not understand when and where to properly use
>>>>> hierarchical irqdomain, even after Marc's talk at ELC-E.
>>>>
>>>> I probably didn't do that good a job explaining it then. Let's
>>>> try
>>>> again. You want to use hierarchical domains when you want to
>>>> describe
>>>> an
>>>> interrupt whose path traverses multiple controllers without ever
>>>> being
>>>> multiplexed with other signals. As long as you have this 1:1
>>>> relationship between controllers, you can use them.
>>>>
>>>
>>> Linus, Marc,
>>>
>>> The calculation is question here is meant to get the appropriate
>>> hwirq
>>> number from a particular gpio (and deal with the gpios that can't
>>> provide an irq at all).
>>>
>>> If I look at other gpio drivers, many are doing exactly this kind
>>> of
>>> calculation before calling 'irq_create_mapping' in the to_irq
>>> callback.
>>> For example:
>>> - pinctrl/nomadik/pinctrl-abx500.c
>>> - pinctrl/samsung/pinctrl-exynos5440.c
>>>
>>> Some can afford to create all the mappings in the probe and just
>>> call
>>> 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work
>>> here. We
>>> have only 8 upstream irqs for 130+ pins, so only 8 mappings
>>> possible at
>>> a time.
>>>
>>> My understanding is that irqdomain provide a way to map hwirq to
>>> linux
>>> virq (and back), not map gpio number to hwirq, right?
>>
>> But why are those number different? Why don't you use the same
>> namespace? If gpio == hwirq, all your problems are already solved. If
>> you don't find the mapping in the irqdomain, then there is no irq,
>> end
>> of story. What am I missing?
>>
>
> There is a few problems to guarantee that gpio == hwirq.
> 1. We have 2 instances of pinctrl, to guarantee that the linux gpio
> number == hwirq, we would have to guarantee the order in which they are
> probed. At least this my understanding
Maybe I wasn't clear enough, and my use of gpio is probably wrong. So
Linux has a gpio number, which is obviously an abstract number (just
like the Linux irq number). But the pad number, in the context of given
SoC, is constant. So we have:
pad->gpio
hwirq->irq
Why can't you have pad == hwirq, always? This is already what you have
in the irqchip driver. This would simplify a lot of things.
> 2. Inside each instance, we may have banks that can't have irq. We even
> have a bank on meson8b which has a mixed state (the last pins don't
> have irqs, while the first ones do). those banks/pins are still valid
> gpios with gpio numbers. This introduce a shift between the gpio
> numbering and the hwirq.
>
> The point of this calculation is take the offset given to the 'to_irq'
> callback, remove the gpio bank base number and add irq base number.
> There is a few trick added to handled the case in 2.
You seem to assume linear mappings, which is pretty dangerous.
>
> In addition, to call 'irq_find_mapping', we would first have to create
> the mapping, in the probe I suppose. This would call the allocate
> callback of the domain, in which we can allocate only 8 interrupts.
>
> That's why I create the mapping in the .to_irq callback.
Is gpio_to_irq() supposed to allocate an interrupt? Or merely to report
the existence of a mapping?
>
>>>
>>>
>>> Even if I implement an another irqdomain at the gpio level, I would
>>> still have to perform this kind of calculation, one way or the
>>> other.
>>>
>>>>
>>>>>
>>>>> Which is problematic since quite a few GPIO drivers now
>>>>> need to use them.
>>>>>
>>>>> I will review his slides, in the meantime I would say: whatever
>>>>> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
>>>>> could ask that he ACK the entire chain of patches
>>>>> from GIC->specialchip->GPIO.
>>>
>>> Actually this discussion go me thinking about another issue we have
>>> with this hardware.
>>> We are looking for a way to implement support for
>>> IRQ_TYPE_EDGE_BOTH
>>> (needed for things like gpio-keys or mmc card detect).
>>> The controller can do each edge but not both at the same time.
>>> I'm thinking that implementing another irqdomain at the gpio level
>>> would allow to properly check the pad level in the EOI callback
>>> then
>>> set the next expected edge type accordingly (using
>>> 'irq_chip_set_type_parent')
>>>
>>> Would it be acceptable ?
>>
>> I really don't see what another irqdomain brings to the table. This
>> is
>> not a separate piece of HW, so the hwirq:irq mapping is still the
>> same.
>> I fail to see what the benefit is.
>
> The separate piece of hw is the gpio itself.
> The irq-controller (in irqchip) can't read the gpio state because it is
> simply another device.
>
> The domain I'm thinking about wouldn't do much, I reckon.
> It would just register an irqchip which would only implement eoi.
> For everything else, it would rely the parent controller.
> From your explanation, I understood this is the purpose of hierarchy
> domains, For each hw in the chain to handle only what it can, and rely
> on its parent for the rest.
Right. But that doesn't make it more reliable, see below.
>
>>
>>>
>>> It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
>>> similar
>>> way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
>>
>> Being already done doesn't make it reliable. If the line goes low
>> between latching the rising edge and reprogramming the trigger,
>> you've
>> lost at least *two* interrupts (the falling edge and the following
>> rising edge).
>
> If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes
> low between latching rising edge and handling the interrupt, wouldn't
> you miss the falling edge anyway ? The signal is just going too fast
> the HW to handle everything.
That's the role of the HW to ensure that you don't loose any interrupt,
up to the sampling frequency of the controller. Doing it in SW is always
going to be a wonderful case of "it mostly work".
> For the second rising edge, I disagree, it would not be lost, since we
> would read the pad state, get a low level, and reprogram the controller
> for another rising edge.
You always have a race between checking your level and switching the
configuration, which is likely to be slow anyway. In the meantime, the
level has changed and you're dead.
Anyway, I suggest you focus on getting something simple up and running,
and postpone the funky (read broken) stuff for later, once you have
something that looks vaguely sane (because we're not quite there yet).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 4/4] ARM: dts: da850: Add the usb otg device node
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477406345-27192-1-git-send-email-abailon@baylibre.com>
This adds the device tree node for the usb otg
controller present in the da850 family of SoC's.
This also enables the otg usb controller for the lcdk board.
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
arch/arm/boot/dts/da850.dtsi | 15 +++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index 7b8ab21..dca9735 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -158,6 +158,14 @@
rx-num-evt = <32>;
};
+&usb_phy {
+ status = "okay";
+ };
+
+&usb20 {
+ status = "okay";
+};
+
&aemif {
pinctrl-names = "default";
pinctrl-0 = <&nand_pins>;
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..b11d395 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -372,6 +372,21 @@
>;
status = "disabled";
};
+ usb_phy: usb-phy {
+ compatible = "ti,da830-usb-phy";
+ #phy-cells = <1>;
+ status = "disabled";
+ };
+ usb20: usb20 at 200000 {
+ compatible = "ti,da830-musb";
+ reg = <0x200000 0x10000>;
+ interrupts = <58>;
+ interrupt-names = "mc";
+ dr_mode = "otg";
+ phys = <&usb_phy 0>;
+ phy-names = "usb-phy";
+ status = "disabled";
+ };
gpio: gpio at 226000 {
compatible = "ti,dm6441-gpio";
gpio-controller;
--
2.7.3
^ permalink raw reply related
* [PATCH 3/4] usb: musb: da8xx: Add DT support for the DA8xx driver
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477406345-27192-1-git-send-email-abailon@baylibre.com>
From: Petr Kulhavy <petr@barix.com>
This adds DT support for TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver
Signed-off-by: Petr Kulhavy <petr@barix.com>
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
drivers/usb/musb/da8xx.c | 76 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 67 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 210b7e4..d465087 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -6,6 +6,9 @@
* Based on the DaVinci "glue layer" code.
* Copyright (C) 2005-2006 by Texas Instruments
*
+ * DT support
+ * Copyright (c) 2016 Petr Kulhavy <petr@barix.com>
+ *
* This file is part of the Inventra Controller Driver for Linux.
*
* The Inventra Controller Driver for Linux is free software; you
@@ -433,6 +436,21 @@ static int da8xx_musb_exit(struct musb *musb)
return 0;
}
+static inline u8 get_vbus_power(struct device *dev)
+{
+ struct regulator *vbus_supply;
+ int current_uA;
+
+ vbus_supply = regulator_get_optional(dev, "vbus");
+ if (IS_ERR(vbus_supply))
+ return 255;
+ current_uA = regulator_get_current_limit(vbus_supply);
+ regulator_put(vbus_supply);
+ if (current_uA <= 0 || current_uA > 510000)
+ return 255;
+ return current_uA / 1000 / 2;
+}
+
static const struct musb_platform_ops da8xx_ops = {
.quirks = MUSB_DMA_CPPI | MUSB_INDEXED_EP,
.init = da8xx_musb_init,
@@ -458,6 +476,12 @@ static const struct platform_device_info da8xx_dev_info = {
.dma_mask = DMA_BIT_MASK(32),
};
+static const struct musb_hdrc_config da8xx_config = {
+ .ram_bits = 10,
+ .num_eps = 5,
+ .multipoint = 1,
+};
+
static int da8xx_probe(struct platform_device *pdev)
{
struct resource musb_resources[2];
@@ -465,7 +489,9 @@ static int da8xx_probe(struct platform_device *pdev)
struct da8xx_glue *glue;
struct platform_device_info pinfo;
struct clk *clk;
+ struct device_node *np = pdev->dev.of_node;
int ret;
+ struct resource *res;
glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
if (!glue)
@@ -486,6 +512,18 @@ static int da8xx_probe(struct platform_device *pdev)
glue->dev = &pdev->dev;
glue->clk = clk;
+ if (IS_ENABLED(CONFIG_OF) && np) {
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata) {
+ /* FIXME */
+ return -ENOMEM;
+ }
+
+ pdata->config = &da8xx_config;
+ pdata->mode = musb_get_mode(&pdev->dev);
+ pdata->power = get_vbus_power(&pdev->dev);
+ }
+
pdata->platform_ops = &da8xx_ops;
glue->usb_phy = usb_phy_generic_register();
@@ -499,15 +537,21 @@ static int da8xx_probe(struct platform_device *pdev)
memset(musb_resources, 0x00, sizeof(*musb_resources) *
ARRAY_SIZE(musb_resources));
- musb_resources[0].name = pdev->resource[0].name;
- musb_resources[0].start = pdev->resource[0].start;
- musb_resources[0].end = pdev->resource[0].end;
- musb_resources[0].flags = pdev->resource[0].flags;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "failed to get memory.\n");
+ ret = -EINVAL;
+ goto err_unregister_usb_phy;
+ }
+ musb_resources[0] = *res;
- musb_resources[1].name = pdev->resource[1].name;
- musb_resources[1].start = pdev->resource[1].start;
- musb_resources[1].end = pdev->resource[1].end;
- musb_resources[1].flags = pdev->resource[1].flags;
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "failed to get irq.\n");
+ ret = -EINVAL;
+ goto err_unregister_usb_phy;
+ }
+ musb_resources[1] = *res;
pinfo = da8xx_dev_info;
pinfo.parent = &pdev->dev;
@@ -520,9 +564,14 @@ static int da8xx_probe(struct platform_device *pdev)
ret = PTR_ERR_OR_ZERO(glue->musb);
if (ret) {
dev_err(&pdev->dev, "failed to register musb device: %d\n", ret);
- usb_phy_generic_unregister(glue->usb_phy);
+ goto err_unregister_usb_phy;
}
+ return 0;
+
+err_unregister_usb_phy:
+ usb_phy_generic_unregister(glue->usb_phy);
+
return ret;
}
@@ -536,11 +585,20 @@ static int da8xx_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id da8xx_id_table[] = {
+ {
+ .compatible = "ti,da830-musb",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, da8xx_id_table);
+
static struct platform_driver da8xx_driver = {
.probe = da8xx_probe,
.remove = da8xx_remove,
.driver = {
.name = "musb-da8xx",
+ .of_match_table = of_match_ptr(da8xx_id_table),
},
};
--
2.7.3
^ permalink raw reply related
* [PATCH 2/4] usb: musb: core: added helper function for parsing DT
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477406345-27192-1-git-send-email-abailon@baylibre.com>
From: Petr Kulhavy <petr@barix.com>
This adds the function musb_get_mode() to get the DT property "dr_mode"
Signed-off-by: Petr Kulhavy <petr@barix.com>
Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
drivers/usb/musb/musb_core.c | 19 +++++++++++++++++++
drivers/usb/musb/musb_core.h | 5 +++++
2 files changed, 24 insertions(+)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 27dadc0..bba07e7 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -100,6 +100,7 @@
#include <linux/io.h>
#include <linux/dma-mapping.h>
#include <linux/usb.h>
+#include <linux/usb/of.h>
#include "musb_core.h"
#include "musb_trace.h"
@@ -130,6 +131,24 @@ static inline struct musb *dev_to_musb(struct device *dev)
return dev_get_drvdata(dev);
}
+enum musb_mode musb_get_mode(struct device *dev)
+{
+ enum usb_dr_mode mode;
+
+ mode = usb_get_dr_mode(dev);
+ switch (mode) {
+ case USB_DR_MODE_HOST:
+ return MUSB_HOST;
+ case USB_DR_MODE_PERIPHERAL:
+ return MUSB_PERIPHERAL;
+ case USB_DR_MODE_OTG:
+ case USB_DR_MODE_UNKNOWN:
+ default:
+ return MUSB_OTG;
+ }
+}
+EXPORT_SYMBOL_GPL(musb_get_mode);
+
/*-------------------------------------------------------------------------*/
#ifndef CONFIG_BLACKFIN
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 2cb88a49..a406468 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -617,4 +617,9 @@ static inline void musb_platform_post_root_reset_end(struct musb *musb)
musb->ops->post_root_reset_end(musb);
}
+/* gets the "dr_mode" property from DT and converts it into musb_mode
+ * if the property is not found or not recognized returns MUSB_OTG
+ */
+extern enum musb_mode musb_get_mode(struct device *dev);
+
#endif /* __MUSB_CORE_H__ */
--
2.7.3
^ permalink raw reply related
* [PATCH 1/4] dt/bindings: Add binding for the DA8xx MUSB driver
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477406345-27192-1-git-send-email-abailon@baylibre.com>
From: Petr Kulhavy <petr@barix.com>
DT binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver.
Signed-off-by: Petr Kulhavy <petr@barix.com>
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
.../devicetree/bindings/usb/da8xx-usb.txt | 43 ++++++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
diff --git a/Documentation/devicetree/bindings/usb/da8xx-usb.txt b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
new file mode 100644
index 0000000..5663d79
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
@@ -0,0 +1,43 @@
+TI DA8xx MUSB
+~~~~~~~~~~~~~
+For DA8xx/OMAP-L1x/AM17xx/AM18xx platforms.
+
+Required properties:
+~~~~~~~~~~~~~~~~~~~~
+ - compatible : Should be set to "ti,da830-musb".
+
+ - reg: Offset and length of the USB controller register set.
+
+ - interrupts: The USB interrupt number.
+
+ - interrupt-names: Should be set to "mc".
+
+ - dr_mode: The USB operation mode. Should be one of "host", "peripheral" or "otg".
+
+ - phys: Phandle for the PHY device
+
+ - phy-names: Should be "usb-phy"
+
+Optional properties:
+~~~~~~~~~~~~~~~~~~~~
+ - vbus-supply: Phandle to a regulator providing the USB bus power.
+
+Example:
+ usb_phy: usb-phy {
+ compatible = "ti,da830-usb-phy";
+ #phy-cells = <0>;
+ status = "okay";
+ };
+ usb20: usb at 1e00000 {
+ compatible = "ti,da830-musb";
+ reg = <0x00200000 0x10000>;
+ interrupts = <58>;
+ interrupt-names = "mc";
+
+ dr_mode = "host";
+ vbus-supply = <&usb_vbus>;
+ phys = <&usb_phy 0>;
+ phy-names = "usb-phy";
+
+ status = "okay";
+ };
--
2.7.3
^ permalink raw reply related
* [PATCH 0/4] Add DT support for DA8xx
From: Alexandre Bailon @ 2016-10-25 14:39 UTC (permalink / raw)
To: linux-arm-kernel
The purpose of this series is to add DT support to the da8xx USB OTG.
This series should apply and build without any issues but it has
some dependencies on "Add DT support for ohci-da8xx" series.
Without it, the phy init will fail and then the da8xx driver will also fail.
Alexandre Bailon (1):
ARM: dts: da850: Add the usb otg device node
Petr Kulhavy (3):
dt/bindings: Add binding for the DA8xx MUSB driver
usb: musb: core: added helper function for parsing DT
usb: musb: da8xx: Add DT support for the DA8xx driver
.../devicetree/bindings/usb/da8xx-usb.txt | 33 ++++++++++
arch/arm/boot/dts/da850-lcdk.dts | 8 +++
arch/arm/boot/dts/da850.dtsi | 15 +++++
drivers/usb/musb/da8xx.c | 76 +++++++++++++++++++---
drivers/usb/musb/musb_core.c | 19 ++++++
drivers/usb/musb/musb_core.h | 5 ++
6 files changed, 147 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
--
2.7.3
^ permalink raw reply
* [PATCH V3 0/8] IOMMU probe deferral support
From: Robin Murphy @ 2016-10-25 14:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475600632-21289-1-git-send-email-sricharan@codeaurora.org>
Hi Sricharan,
On 04/10/16 18:03, Sricharan R wrote:
> Initial post from Laurent Pinchart[1]. This is
> series calls the dma ops configuration for the devices
> at a generic place so that it works for all busses.
> The dma_configure_ops for a device is now called during
> the device_attach callback just before the probe of the
> bus/driver is called. Similarly dma_deconfigure is called during
> device/driver_detach path.
>
>
> pci_bus_add_devices (platform/amba)(_device_create/driver_register)
> | |
> pci_bus_add_device (device_add/driver_register)
> | |
> device_attach device_initial_probe
> | |
> __device_attach_driver __device_attach_driver
> |
> driver_probe_device
> |
> really_probe
> |
> dma_configure
>
> Similarly on the device/driver_unregister path __device_release_driver is
> called which inturn calls dma_deconfigure.
>
> If the ACPI bus code follows the same, we can add acpi_dma_configure
> at the same place as of_dma_configure.
>
> This series is based on the recently merged Generic DT bindings for
> PCI IOMMUs and ARM SMMU from Robin Murphy robin.murphy at arm.com [2]
>
> This time tested this with platform and pci device for probe deferral
> and reprobe on arm64 based platform. There is an issue on the cleanup
> path for arm64 though, where there is WARN_ON if the dma_ops is reset while
> device is attached to an domain in arch_teardown_dma_ops.
> But with iommu_groups created from the iommu driver, the device is always
> attached to a domain/default_domain. So so the WARN has to be removed/handled
> probably.
I've finally got the chance to take a proper look at this series (sorry
for the delay). First up, with these patches on top of 4.9-rc2, my
little script for unbinding some PCI devices and rebinding them to VFIO
now goes horribly, horribly wrong.
Robin.
[ 39.901592] iommu: Removing device 0000:08:00.0 from group 0
[ 39.907383] ------------[ cut here ]------------
[ 39.911969] WARNING: CPU: 0 PID: 174 at
arch/arm64/mm/dma-mapping.c:856 arch_teardown_dma_ops+0x48/0x68
[ 39.921266] Modules linked in:
[ 39.924290]
[ 39.925766] CPU: 0 PID: 174 Comm: vfio Not tainted 4.9.0-rc2+ #1249
[ 39.931967] Hardware name: ARM Juno development board (r1) (DT)
[ 39.937826] task: ffffffc975ee9900 task.stack: ffffffc974d60000
[ 39.943687] PC is at arch_teardown_dma_ops+0x48/0x68
[ 39.948603] LR is at arch_teardown_dma_ops+0x34/0x68
[ 39.953516] pc : [<ffffff80080948f8>] lr : [<ffffff80080948e4>]
pstate: 60000145
[ 39.960834] sp : ffffffc974d63ca0
[ 39.964112] x29: ffffffc974d63ca0 x28: ffffffc974d60000
[ 39.969377] x27: ffffff80088a2000 x26: 0000000000000040
[ 39.974642] x25: 0000000000000123 x24: ffffffc976a48918
[ 39.979907] x23: ffffffc974d63eb8 x22: ffffff8008db7550
[ 39.985171] x21: 000000000000000d x20: ffffffc9763e9b50
[ 39.990435] x19: ffffffc9763f20a0 x18: 0000000000000010
[ 39.995699] x17: 0000007f99c18018 x16: ffffff80080c0580
[ 40.000964] x15: ffffff8008bb7000 x14: 000137c100013798
[ 40.006228] x13: ffffffffff000000 x12: ffffffffffffffff
[ 40.011492] x11: 0000000000000018 x10: 000000000000000d
[ 40.016757] x9 : 0000000040000000 x8 : 0000000000210d00
[ 40.022021] x7 : 00000049771bc000 x6 : 00000000001f17ed
[ 40.027286] x5 : ffffff80084c4208 x4 : 0000000000000080
[ 40.032551] x3 : ffffffc975ea9800 x2 : ffffffbf25d7aa50
[ 40.037815] x1 : 0000000000000000 x0 : 0000000000000080
[ 40.043078]
[ 40.044549] ---[ end trace 35c1e743d6e6c035 ]---
[ 40.049117] Call trace:
[ 40.051537] Exception stack(0xffffffc974d63ad0 to 0xffffffc974d63c00)
[ 40.057914] 3ac0: ffffffc9763f20a0
0000008000000000
[ 40.065668] 3ae0: ffffffc974d63ca0 ffffff80080948f8 ffffffbf25d7aa40
ffffffc975ea9800
[ 40.073421] 3b00: ffffffc974d60000 000000000002fc80 ffffffc976801e00
ffffffc974d60000
[ 40.081175] 3b20: ffffff80084c4208 0000000000000040 ffffff80088a2000
ffffffc974d60000
[ 40.088928] 3b40: ffffff80084caf78 ffffffc974d60000 ffffffc974d60000
ffffffc974d60000
[ 40.096682] 3b60: ffffffc975ea9800 ffffffc974d60000 0000000000000080
0000000000000000
[ 40.104435] 3b80: ffffffbf25d7aa50 ffffffc975ea9800 0000000000000080
ffffff80084c4208
[ 40.112188] 3ba0: 00000000001f17ed 00000049771bc000 0000000000210d00
0000000040000000
[ 40.119941] 3bc0: 000000000000000d 0000000000000018 ffffffffffffffff
ffffffffff000000
[ 40.127695] 3be0: 000137c100013798 ffffff8008bb7000 ffffff80080c0580
0000007f99c18018
[ 40.135450] [<ffffff80080948f8>] arch_teardown_dma_ops+0x48/0x68
[ 40.141400] [<ffffff8008764a14>] of_dma_deconfigure+0xc/0x18
[ 40.147005] [<ffffff8008552804>] dma_deconfigure+0xc/0x18
[ 40.152353] [<ffffff800853ba10>] __device_release_driver+0x88/0x120
[ 40.158560] [<ffffff800853bacc>] device_release_driver+0x24/0x38
[ 40.164507] [<ffffff800853a868>] unbind_store+0xe8/0x110
[ 40.169767] [<ffffff8008539c70>] drv_attr_store+0x20/0x30
[ 40.175113] [<ffffff800823ab18>] sysfs_kf_write+0x48/0x58
[ 40.180458] [<ffffff8008239ea8>] kernfs_fop_write+0xb0/0x1d8
[ 40.186063] [<ffffff80081c507c>] __vfs_write+0x1c/0x100
[ 40.191237] [<ffffff80081c5e80>] vfs_write+0xa0/0x1b8
[ 40.196239] [<ffffff80081c7274>] SyS_write+0x44/0xa0
[ 40.201155] [<ffffff8008082ef0>] el0_svc_naked+0x24/0x28
[ 40.206703] vfio-pci 0000:08:00.0: Failed to setup iommu ops
[ 40.212382] vfio-pci: probe of 0000:08:00.0 failed with error -22
[ 40.228075] ------------[ cut here ]------------
[ 40.235263] WARNING: CPU: 1 PID: 174 at ./include/linux/kref.h:46
kobject_get+0x64/0x88
[ 40.243181] Modules linked in:
[ 40.246201]
[ 40.247673] CPU: 1 PID: 174 Comm: vfio Tainted: G W
4.9.0-rc2+ #1249
[ 40.255076] Hardware name: ARM Juno development board (r1) (DT)
[ 40.260932] task: ffffffc975ee9900 task.stack: ffffffc974d60000
[ 40.266787] PC is at kobject_get+0x64/0x88
[ 40.270840] LR is at iommu_bus_notifier+0x40/0x110
[ 40.275577] pc : [<ffffff800834d20c>] lr : [<ffffff80084c3fd0>]
pstate: 80000145
[ 40.282894] sp : ffffffc974d63c00
[ 40.286169] x29: ffffffc974d63c00 x28: ffffffc974d60000
[ 40.291431] x27: ffffff80088a2000 x26: 0000000000000040
[ 40.296692] x25: 0000000000000123 x24: ffffffc974c8f418
[ 40.301953] x23: 0000000000000006 x22: ffffffc9763f10a0
[ 40.307214] x21: ffffffc9763e9a00 x20: ffffffc9763f10a0
[ 40.312474] x19: ffffffc9763ebc80 x18: 0000007fd65069e0
[ 40.317734] x17: 0000007f8d0ae3c0 x16: ffffff80081c7230
[ 40.322995] x15: 0000007f8d136588 x14: ffffffffffffffff
[ 40.328255] x13: 0000000000000004 x12: 0000000000000030
[ 40.333515] x11: 0000000000000030 x10: 0101010101010101
[ 40.338775] x9 : feff716475687163 x8 : 7f7f7f7f7f7f7f7f
[ 40.344035] x7 : feff716475687163 x6 : ffffffc976abf400
[ 40.349295] x5 : ffffffc976abf400 x4 : 0000000000000000
[ 40.354555] x3 : ffffff80084c3f90 x2 : ffffffc9763ebcb8
[ 40.359814] x1 : 0000000000000001 x0 : ffffff8008d4f000
[ 40.365074]
[ 40.366542] ---[ end trace 35c1e743d6e6c036 ]---
[ 40.371107] Call trace:
[ 40.373523] Exception stack(0xffffffc974d63a30 to 0xffffffc974d63b60)
[ 40.379895] 3a20: ffffffc9763ebc80
0000008000000000
[ 40.387643] 3a40: ffffffc974d63c00 ffffff800834d20c ffffffc976812400
ffffff8008237d94
[ 40.395391] 3a60: ffffffbf25d78940 ffffffc974d60000 ffffffc975e259d8
0000000000005b81
[ 40.403139] 3a80: ffffffc974d60000 ffffff8008d4b31f ffffff8008b0f000
ffffffc976811c80
[ 40.410887] 3aa0: ffffffc974d60000 ffffffc974d60000 ffffffc974d60000
ffffff8008237000
[ 40.418634] 3ac0: ffffffc975e259d8 ffffff8008b1b9a8 ffffff8008d4f000
0000000000000001
[ 40.426382] 3ae0: ffffffc9763ebcb8 ffffff80084c3f90 0000000000000000
ffffffc976abf400
[ 40.434130] 3b00: ffffffc976abf400 feff716475687163 7f7f7f7f7f7f7f7f
feff716475687163
[ 40.441877] 3b20: 0101010101010101 0000000000000030 0000000000000030
0000000000000004
[ 40.449625] 3b40: ffffffffffffffff 0000007f8d136588 ffffff80081c7230
0000007f8d0ae3c0
[ 40.457372] [<ffffff800834d20c>] kobject_get+0x64/0x88
[ 40.462455] [<ffffff80084c3fd0>] iommu_bus_notifier+0x40/0x110
[ 40.468227] [<ffffff80080da288>] notifier_call_chain+0x50/0x90
[ 40.473997] [<ffffff80080da694>] __blocking_notifier_call_chain+0x4c/0x90
[ 40.480713] [<ffffff80080da6ec>] blocking_notifier_call_chain+0x14/0x20
[ 40.487259] [<ffffff800853b9e4>] __device_release_driver+0x5c/0x120
[ 40.493460] [<ffffff800853bacc>] device_release_driver+0x24/0x38
[ 40.499402] [<ffffff800853a868>] unbind_store+0xe8/0x110
[ 40.504656] [<ffffff8008539c70>] drv_attr_store+0x20/0x30
[ 40.509997] [<ffffff800823ab18>] sysfs_kf_write+0x48/0x58
[ 40.515337] [<ffffff8008239ea8>] kernfs_fop_write+0xb0/0x1d8
[ 40.520936] [<ffffff80081c507c>] __vfs_write+0x1c/0x100
[ 40.526104] [<ffffff80081c5e80>] vfs_write+0xa0/0x1b8
[ 40.531100] [<ffffff80081c7274>] SyS_write+0x44/0xa0
[ 40.536011] [<ffffff8008082ef0>] el0_svc_naked+0x24/0x28
[ 40.541324] ata1.00: disabled
[ 40.544878] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[ 40.550062] sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
hostbyte=0x04 driverbyte=0x00
[ 40.558871] sd 0:0:0:0: [sda] Stopping disk
[ 40.563037] sd 0:0:0:0: [sda] Start/Stop Unit failed: Result:
hostbyte=0x04 driverbyte=0x00
[ 40.586990] Unable to handle kernel paging request at virtual address
0002003e
[ 40.594702] pgd = ffffffc974c80000
[ 40.598165] [0002003e] *pgd=00000009f5102003[ 40.602241] ,
*pud=00000009f5102003
, *pmd=0000000000000000[ 40.607694]
[ 40.609171] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 40.614684] Modules linked in:
[ 40.617712] CPU: 3 PID: 174 Comm: vfio Tainted: G W
4.9.0-rc2+ #1249
[ 40.625118] Hardware name: ARM Juno development board (r1) (DT)
[ 40.630977] task: ffffffc975ee9900 task.stack: ffffffc974d60000
[ 40.636841] PC is at kobject_get+0x14/0x88
[ 40.640897] LR is at iommu_get_domain_for_dev+0x1c/0x48
[ 40.646068] pc : [<ffffff800834d1bc>] lr : [<ffffff80084c3dec>]
pstate: 60000145
[ 40.653387] sp : ffffffc974d63c60
[ 40.656664] x29: ffffffc974d63c60 x28: ffffffc974d60000
[ 40.661928] x27: ffffff80088a2000 x26: 0000000000000040
[ 40.667193] x25: 0000000000000123 x24: ffffffc974c8f418
[ 40.672457] x23: ffffffc974d63eb8 x22: ffffff8008dab568
[ 40.677720] x21: 000000000000000d x20: ffffff8008dab568
[ 40.682984] x19: 0000000000020002 x18: 0000000000000000
[ 40.688246] x17: 0000000000000007 x16: 0000000000000001
[ 40.693509] x15: ffffffc974cd091c x14: ffffffffffffffff
[ 40.698773] x13: ffffffc974cd01cd x12: 0000000000000030
[ 40.704036] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
[ 40.709300] x9 : 0000000040000000 x8 : 0000000000210d00
[ 40.714563] x7 : ffffffc975f95018 x6 : 0000000000000000
[ 40.719826] x5 : 0000000000000000 x4 : ffffffc9763f1210
[ 40.725088] x3 : 0000000000000000 x2 : ffffffc9763f10e8
[ 40.730351] x1 : 0000000000000000 x0 : 0000000000020002
[ 40.735613]
[ 40.737085] Process vfio (pid: 174, stack limit = 0xffffffc974d60020)
[ 40.743460] Stack: (0xffffffc974d63c60 to 0xffffffc974d64000)
[ 40.749150] 3c60: ffffffc974d63c80 ffffff80084c3dec ffffffc9763e9a00
ffffff80085377a4
[ 40.756904] 3c80: ffffffc974d63ca0 ffffff80080948c4 ffffffc9763f10a0
ffffff8008dab568
[ 40.764658] 3ca0: ffffffc974d63cc0 ffffff8008764a14 ffffffc9763f10a0
ffffff8008dab568
[ 40.772411] 3cc0: ffffffc974d63cd0 ffffff8008552804 ffffffc974d63ce0
ffffff800853ba10
[ 40.780165] 3ce0: ffffffc974d63d00 ffffff800853bacc ffffffc9763f1100
ffffffc9763f10a0
[ 40.787918] 3d00: ffffffc974d63d20 ffffff800853a868 ffffff8008d68f18
ffffffc9763f10a0
[ 40.795672] 3d20: ffffffc974d63d50 ffffff8008539c70 000000000000000d
ffffffc974c8f400
[ 40.803425] 3d40: ffffffc9757d5880 0000000000000000 ffffffc974d63d60
ffffff800823ab18
[ 40.811178] 3d60: ffffffc974d63d70 ffffff8008239ea8 ffffffc974d63dc0
ffffff80081c507c
[ 40.818931] 3d80: 000000000000000d 0000000000000000 ffffffc974c8f100
ffffffc974d63eb8
[ 40.826684] 3da0: 000000001285f6a0 0000000000000015 0000000000000123
ffffff80080bf6ac
[ 40.834437] 3dc0: ffffffc974d63e40 ffffff80081c5e80 000000000000000d
0000000000000000
[ 40.842190] 3de0: ffffffc974d63e30 ffffff80080c087c ffffffc974d63e20
ffffff80081c5c0c
[ 40.849943] 3e00: ffffffc974c8f100 0000000000000001 ffffffc974c8f100
ffffffc974d63eb8
[ 40.857696] 3e20: ffffffc974d63e40 ffffff80081c5f48 000000000000000d
ffffffc974c8f100
[ 40.865450] 3e40: ffffffc974d63e80 ffffff80081c7274 ffffffc974c8f100
ffffffc974c8f100
[ 40.873203] 3e60: 000000001285f6a0 000000000000000d 0000000060000000
0000000000000000
[ 40.880956] 3e80: 0000000000000000 ffffff8008082ef0 0000000000000000
0000000000000001
[ 40.888709] 3ea0: ffffffffffffffff 0000007f8d0ae3dc 0000000000000000
0000000000000000
[ 40.896461] 3ec0: 0000000000000001 000000001285f6a0 000000000000000d
0000000000000000
[ 40.904215] 3ee0: ae2e2e2e3f464b49 0000000000000000 000000001285f6b0
39322f392f2f2f2f
[ 40.911968] 3f00: 0000000000000040 fefefeff2f2d2f2f 7f7f7f7f7f7f7f7f
0101010101010101
[ 40.919721] 3f20: 0000000000000002 0000000000000004 ffffffffffffffff
0000007f8d136588
[ 40.927474] 3f40: 0000000000000000 0000007f8d0ae3c0 0000007fd65069e0
00000000004ee000
[ 40.935226] 3f60: 0000000000000001 000000001285f6a0 000000000000000d
0000000000000001
[ 40.942980] 3f80: 0000000000000020 000000001285eed8 00000000004ba158
0000000000000000
[ 40.950732] 3fa0: 0000000000000000 0000007fd6507f30 000000000040e74c
0000007fd6507130
[ 40.958485] 3fc0: 0000007f8d0ae3dc 0000000060000000 0000000000000001
0000000000000040
[ 40.966238] 3fe0: 0000000000000000 0000000000000000 0000002000103a00
4000000010000000
[ 40.973986] Call trace:
[ 40.976405] Exception stack(0xffffffc974d63a90 to 0xffffffc974d63bc0)
[ 40.982780] 3a80: 0000000000020002
0000008000000000
[ 40.990533] 3aa0: ffffffc974d63c60 ffffff800834d1bc ffffffc974d63ae0
ffffff80085377a4
[ 40.998287] 3ac0: ffffffc974d63b10 ffffff8008537424 ffffffc975e3ac28
ffffffc975e3ac38
[ 41.006041] 3ae0: ffffffc974d63b30 ffffff80081737cc ffffffc975e3ac38
ffffff8008da62c0
[ 41.013794] 3b00: ffffffc975e98100 ffffff80085401b0 0000000000000001
ffffff8008540a08
[ 41.021547] 3b20: 00000000000036b8 0000000000000040 0000000000020002
0000000000000000
[ 41.029300] 3b40: ffffffc9763f10e8 0000000000000000 ffffffc9763f1210
0000000000000000
[ 41.037053] 3b60: 0000000000000000 ffffffc975f95018 0000000000210d00
0000000040000000
[ 41.044805] 3b80: 7f7f7f7f7f7f7f7f 0101010101010101 0000000000000030
ffffffc974cd01cd
[ 41.052558] 3ba0: ffffffffffffffff ffffffc974cd091c 0000000000000001
0000000000000007
[ 41.060311] [<ffffff800834d1bc>] kobject_get+0x14/0x88
[ 41.065398] [<ffffff80084c3dec>] iommu_get_domain_for_dev+0x1c/0x48
[ 41.071607] [<ffffff80080948c4>] arch_teardown_dma_ops+0x14/0x68
[ 41.077556] [<ffffff8008764a14>] of_dma_deconfigure+0xc/0x18
[ 41.083161] [<ffffff8008552804>] dma_deconfigure+0xc/0x18
[ 41.088509] [<ffffff800853ba10>] __device_release_driver+0x88/0x120
[ 41.094715] [<ffffff800853bacc>] device_release_driver+0x24/0x38
[ 41.100663] [<ffffff800853a868>] unbind_store+0xe8/0x110
[ 41.105922] [<ffffff8008539c70>] drv_attr_store+0x20/0x30
[ 41.111268] [<ffffff800823ab18>] sysfs_kf_write+0x48/0x58
[ 41.116612] [<ffffff8008239ea8>] kernfs_fop_write+0xb0/0x1d8
[ 41.122216] [<ffffff80081c507c>] __vfs_write+0x1c/0x100
[ 41.127390] [<ffffff80081c5e80>] vfs_write+0xa0/0x1b8
[ 41.132391] [<ffffff80081c7274>] SyS_write+0x44/0xa0
[ 41.137307] [<ffffff8008082ef0>] el0_svc_naked+0x24/0x28
[ 41.142567] Code: 910003fd f9000bf3 aa0003f3 b4000180 (3940f000)
[ 41.148667] ---[ end trace 35c1e743d6e6c037 ]---
Segmentation fault
/ #
^ permalink raw reply
* [PATCH/RFT v2 09/17] regulator: fixed: Add over current event
From: Mark Brown @ 2016-10-25 14:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKXjFTOHcqRFtbXMXSTyUZ3P00eRMz1N3d62Y3NWVZJDGeScZA@mail.gmail.com>
On Tue, Oct 25, 2016 at 02:55:48PM +0200, Axel Haslam wrote:
> To be able to use regulator to handle the overcurrent pin, i need to be able
> to somehow retrieve the over current pin state from the regulator driver.
What makes you say that, none of the existing users need this?
> As i was trying your suggestion, i remembered why i thought i should use
> mode instead of status: Status seems to be for internal regulator driver use,
> there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
> in driver.h and not in consumer.h as REGULATOR_MODE_*
> Would you be ok if i allow consumers to get the status via a new
> "regulator_get_status" call?
What would they do with this information that they can't do with the
existing error notification?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161025/eaaf09dc/attachment.sig>
^ permalink raw reply
* [PATCH] arm64: Neaten show_regs, remove KERN_CONT
From: Mark Rutland @ 2016-10-25 14:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477326477.1984.2.camel@perches.com>
On Mon, Oct 24, 2016 at 09:27:57AM -0700, Joe Perches wrote:
> On Mon, 2016-10-24 at 12:31 +0100, Mark Rutland wrote:
> > On Sun, Oct 23, 2016 at 01:40:49PM -0700, Joe Perches wrote:
> > > commit db4b0710fae9 ("arm64: fix show_regs fallout from KERN_CONT changes")
> > > corrected the KERN_CONT fallout from commit 4bcc595ccd80
> > > ("printk: reinstate KERN_CONT for printing continuation lines"), but
> > > the code still has unnecessary KERN_CONT uses. Remove them.
> >
> > Why are these unnecessary KERN_CONTs a larger problem than duplicating
> > the format string for a third time? Having to duplicate it at all was
> > annoying enough.
>
> Not printing partial lines is the best solution to avoiding
> message output interleaving.
Looking further, it seems that KERN_CONT is terminally broken. The core
code somehow swallows newlines from some KERN_CONT prints in a
non-deterministic fashion, and also appears to insert newlines from thin
air. This happens in the absence of intervening printks.
With the current code in v4.9-rc2, we get output like:
x29: 0000ffffe4938c80 x28: 0000000000000000
x27: 0000000000000000 x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
x23: 0000000000000000 x22: 0000000000000000
x21: 0000000000400470 x20: 0000000000000000
x19: 0000000000000000 x18: 0000ffffe4938b60
x17: 0000000000411000 x16: 0000ffff82f72c9c
x15: 0000ffff830c8000 x14: 0000000000000040
x13: 0000ffff830c8028 x12: 0000000000008738
x11: 0000000000000008
x10: 00000000ffffffff
x9 : 0000ffff830b4e40 x8 : 2f2f2f2f2f2f2f2f
x7 : b3b3bab7acff8b8a x6 : 0000ffff83097aa8
x5 : 54d58839205d3679 x4 : 0000000000000000
x3 : 00000000004005d0 x2 : ffff000000000000
x1 : 0000ffffe4938e08 x0 : ffff000000000000
... or:
x29: 0000fffff6f6a600 x28: 0000000000000000 x27: 0000000000000000 x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000 x23: 0000000000000000 x22: 0000000000000000 x21: 0000000000400470 x20: 0000000000000000 x19: 0000000000000000 x18: 0000fffff6f6a4e0 x17: 0000000000411000 x16: 0000ffffa6e1fc9c x15: 0000ffffa6f75000 x14: 0000000000000040
x13: 0000ffffa6f75028 x12: 0000000000008738 x11: 0000000000000008 x10: 00000000ffffffff
x9 : 0000ffffa6f61e40 x8 : 2f2f2f2f2f2f2f2f x7 : b3b3bab7acff8b8a x6 : 0000ffffa6f44aa8
x5 : 874b6ebb9d5e2f3d x4 : 0000000000000000 x3 : 00000000004005d0 x2 : ffff000000000000
x1 : 0000fffff6f6a788 x0 : ffff000000000000
... and of course, the buffer shown by $(dmesg) or $(demsg -T) is equally
insane, but different.
I found that adding a space prior to newlines prevented them from being
swallowed, but $(dmesg) would still suffer from random additions.
Given all that, unless the core code is changed to as to behave
deterministically at least for trivial cases like this one, I think we
should avoid KERN_CONT like the plague.
So FWIW, so long as you fold in the changes I requested in my other
reply, please add:
Acked-by: Mark Rutland <mark.rutland@arm.com>
... I'll go fix up show_pte() without pr_cont().
Thanks,
Mark.
^ permalink raw reply
* [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Jerome Brunet @ 2016-10-25 14:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <dee26f0b-e175-f8a0-2b73-08e8e324841c@arm.com>
On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote:
> On 25/10/16 14:08, Jerome Brunet wrote:
> >
> > On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
> > >
> > > >
> > > >
> > > On 25/10/16 10:14, Linus Walleij wrote:
> > > >
> > > >
> > > > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib
> > > > re.c
> > > > om> wrote:
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Isn't this usecase (also as described in the cover letter)
> > > > > > a
> > > > > > textbook
> > > > > > example of when you should be using hierarchical irqdomain?
> > > > > >
> > > > > > Please check with Marc et al on hierarchical irqdomains.
> > > > >
> > > > > Linus,
> > > > > Do you mean I should create a new hierarchical irqdomains in
> > > > > each
> > > > > of
> > > > > the two pinctrl instances we have in these SoC, these domains
> > > > > being
> > > > > stacked on the one I just added for controller in irqchip ?
> > > > >
> > > > > I did not understand this is what you meant when I asked you
> > > > > the
> > > > > question at ELCE.
> > > >
> > > > Honestly, I do not understand when and where to properly use
> > > > hierarchical irqdomain, even after Marc's talk at ELC-E.
> > >
> > > I probably didn't do that good a job explaining it then. Let's
> > > try
> > > again. You want to use hierarchical domains when you want to
> > > describe
> > > an
> > > interrupt whose path traverses multiple controllers without ever
> > > being
> > > multiplexed with other signals. As long as you have this 1:1
> > > relationship between controllers, you can use them.
> > >
> >
> > Linus, Marc,
> >
> > The calculation is question here is meant to get the appropriate
> > hwirq
> > number from a particular gpio (and deal with the gpios that can't
> > provide an irq at all).?
> >
> > If I look at other gpio drivers, many are doing exactly this kind
> > of
> > calculation before calling 'irq_create_mapping' in the to_irq
> > callback.
> > For example:
> > - pinctrl/nomadik/pinctrl-abx500.c
> > - pinctrl/samsung/pinctrl-exynos5440.c
> >
> > Some can afford to create all the mappings in the probe and just
> > call
> > 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work
> > here. We
> > have only 8 upstream irqs for 130+ pins, so only 8 mappings
> > possible at
> > a time.?
> >
> > My understanding is that irqdomain provide a way to map hwirq to
> > linux
> > virq (and back), not map gpio number to hwirq, right?
>
> But why are those number different? Why don't you use the same
> namespace? If gpio == hwirq, all your problems are already solved. If
> you don't find the mapping in the irqdomain, then there is no irq,
> end
> of story. What am I missing?
>
There is a few problems to guarantee that gpio == hwirq.
1. We have 2 instances of pinctrl, to guarantee that the linux gpio
number == hwirq, we would have to guarantee the order in which they are
probed. At least this my understanding?
2. Inside each instance, we may have banks that can't have irq. We even
have a bank on meson8b which has a mixed state (the last pins don't
have irqs, while the first ones do). those banks/pins are still valid
gpios with gpio numbers. This introduce a shift between the gpio
numbering and the hwirq.
The point of this calculation is take the offset given to the 'to_irq'
callback, remove the gpio bank base number and add irq base number.
There is a few trick added to handled the case in 2.
In addition, to call 'irq_find_mapping', we would first have to create
the mapping, in the probe I suppose. This would call the allocate
callback of the domain, in which we can allocate only 8 interrupts.
That's why I create the mapping in the .to_irq callback.
> >
> >
> > Even if I implement an another irqdomain at the gpio level, I would
> > still have to perform this kind of calculation, one way or the
> > other.
> >
> > >
> > > >
> > > > Which is problematic since quite a few GPIO drivers now
> > > > need to use them.
> > > >
> > > > I will review his slides, in the meantime I would say: whatever
> > > > Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> > > > could ask that he ACK the entire chain of patches
> > > > from GIC->specialchip->GPIO.
> >
> > Actually this discussion go me thinking about another issue we have
> > with this hardware.
> > We are looking for a way to implement support for
> > IRQ_TYPE_EDGE_BOTH
> > (needed for things like gpio-keys or mmc card detect).?
> > The controller can do each edge but not both at the same time.
> > I'm thinking that implementing another irqdomain at the gpio level
> > would allow to properly check the pad level in the EOI callback
> > then
> > set the next expected edge type accordingly (using
> > 'irq_chip_set_type_parent')
> >
> > Would it be acceptable ?
>
> I really don't see what another irqdomain brings to the table. This
> is
> not a separate piece of HW, so the hwirq:irq mapping is still the
> same.
> I fail to see what the benefit is.
The separate piece of hw is the gpio itself.?
The irq-controller (in irqchip) can't read the gpio state because it is
simply another device.
The domain I'm thinking about wouldn't do much, I reckon.?
It would just register an irqchip which would only implement eoi.
For everything else, it would rely the parent controller.
>From your explanation, I understood this is the purpose of hierarchy
domains, For each hw in the chain to handle only what it can, and rely
on its parent for the rest.
>
> >
> > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
> > similar
> > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
>
> Being already done doesn't make it reliable. If the line goes low
> between latching the rising edge and reprogramming the trigger,
> you've
> lost at least *two* interrupts (the falling edge and the following
> rising edge).
If a 'usual' controller support?IRQ_TYPE_EDGE_BOTH and the line goes
low between latching?rising edge and handling the interrupt, wouldn't
you miss the falling edge anyway ? The signal is just going too fast
the HW to handle everything.
For the second rising edge, I disagree, it would not be lost, since we
would read the pad state, get a low level, and reprogram the controller
for another rising edge.
>
> Thanks,
>
> M.
^ permalink raw reply
* [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2
From: Jean-Francois Moine @ 2016-10-25 14:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025064422.4jua6qmpr7zu3ijt@phenom.ffwll.local>
On Tue, 25 Oct 2016 08:44:22 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:
> > + /* start the subdevices */
> > + ret = component_bind_all(dev, drm);
> > + if (ret < 0)
> > + goto out2;
> > +
> > + ret = drm_dev_register(drm, 0);
>
> This needs to be the very last step in your driver load sequence.
> Kerneldoc explains why. Similar, but inverted for unloading:
> drm_dev_unregister is the very first thing you must call.
Thanks, and also for embedding the drm device.
--
Ken ar c'henta? | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
^ permalink raw reply
* [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2
From: Jean-Francois Moine @ 2016-10-25 14:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161024140419.ah6hywf3b24pj3fv@lukather>
On Mon, 24 Oct 2016 16:04:19 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> Hi,
Hi Maxime,
> On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> > Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> > engine, DE2.
> > This patch adds a DRM video driver for this device.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>
> Output from checkpatch:
> total: 0 errors, 20 warnings, 83 checks, 1799 lines checked
>
> > ---
> > .../bindings/display/sunxi/sunxi-de2.txt | 83 +++
> > drivers/gpu/drm/Kconfig | 2 +
> > drivers/gpu/drm/Makefile | 1 +
> > drivers/gpu/drm/sunxi/Kconfig | 21 +
> > drivers/gpu/drm/sunxi/Makefile | 7 +
> > drivers/gpu/drm/sunxi/de2_crtc.c | 475 +++++++++++++++++
> > drivers/gpu/drm/sunxi/de2_crtc.h | 63 +++
> > drivers/gpu/drm/sunxi/de2_de.c | 591 +++++++++++++++++++++
> > drivers/gpu/drm/sunxi/de2_drm.h | 47 ++
> > drivers/gpu/drm/sunxi/de2_drv.c | 378 +++++++++++++
> > drivers/gpu/drm/sunxi/de2_plane.c | 119 +++++
> > 11 files changed, 1787 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > create mode 100644 drivers/gpu/drm/sunxi/Kconfig
> > create mode 100644 drivers/gpu/drm/sunxi/Makefile
> > create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
> > create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
> > create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
> > create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
> > create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
> > create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
> >
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > new file mode 100644
> > index 0000000..f9cd67a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > @@ -0,0 +1,83 @@
> > +Allwinner sunxi Display Engine 2 subsystem
> > +==========================================
> > +
> > +The sunxi DE2 subsystem contains a display controller (DE2),
>
> sunxi is a made up name, and doesn't really mean anything. You can
> call it either sun8i (because it was introduced in that family).
OK.
> > +one or two LCD controllers (TCON) and their external interfaces.
> > +
> > +Display controller
> > +==================
> > +
> > +Required properties:
> > +
> > +- compatible: value should be one of the following
> > + "allwinner,sun8i-a83t-display-engine"
> > + "allwinner,sun8i-h3-display-engine"
> > +
> > +- clocks: must include clock specifiers corresponding to entries in the
> > + clock-names property.
> > +
> > +- clock-names: must contain
> > + "gate": for DE activation
> > + "clock": DE clock
>
> We've been calling them bus and mod.
I can understand "bus" (which is better than "apb"), but why "mod"?
> > +
> > +- resets: phandle to the reset of the device
> > +
> > +- ports: phandle's to the LCD ports
>
> Please use the OF graph.
These ports are references to the graph of nodes. See
http://www.kernelhub.org/?msg=911825&p=2
[snip]
> > diff --git a/drivers/gpu/drm/sunxi/de2_crtc.c b/drivers/gpu/drm/sunxi/de2_crtc.c
> > new file mode 100644
> > index 0000000..dae0fab
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_crtc.c
> > @@ -0,0 +1,475 @@
> > +/*
> > + * Allwinner DRM driver - DE2 CRTC
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine <moinejf@free.fr>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/component.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <asm/io.h>
> > +#include <linux/of_irq.h>
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +/* I/O map */
> > +
> > +struct tcon {
> > + u32 gctl;
> > +#define TCON_GCTL_TCON_En BIT(31)
> > + u32 gint0;
> > +#define TCON_GINT0_TCON1_Vb_Int_En BIT(30)
> > +#define TCON_GINT0_TCON1_Vb_Int_Flag BIT(14)
> > + u32 gint1;
> > + u32 dum0[13];
> > + u32 tcon0_ctl; /* 0x40 */
> > +#define TCON0_CTL_TCON_En BIT(31)
> > + u32 dum1[19];
> > + u32 tcon1_ctl; /* 0x90 */
> > +#define TCON1_CTL_TCON_En BIT(31)
> > +#define TCON1_CTL_Interlace_En BIT(20)
> > +#define TCON1_CTL_Start_Delay_SHIFT 4
> > +#define TCON1_CTL_Start_Delay_MASK GENMASK(8, 4)
> > + u32 basic0; /* XI/YI */
> > + u32 basic1; /* LS_XO/LS_YO */
> > + u32 basic2; /* XO/YO */
> > + u32 basic3; /* HT/HBP */
> > + u32 basic4; /* VT/VBP */
> > + u32 basic5; /* HSPW/VSPW */
> > + u32 dum2;
> > + u32 ps_sync; /* 0xb0 */
> > + u32 dum3[15];
> > + u32 io_pol; /* 0xf0 */
> > +#define TCON1_IO_POL_IO0_inv BIT(24)
> > +#define TCON1_IO_POL_IO1_inv BIT(25)
> > +#define TCON1_IO_POL_IO2_inv BIT(26)
> > + u32 io_tri;
> > + u32 dum4[2];
> > +
> > + u32 ceu_ctl; /* 0x100 */
> > +#define TCON_CEU_CTL_ceu_en BIT(31)
> > + u32 dum5[3];
> > + u32 ceu_rr;
> > + u32 ceu_rg;
> > + u32 ceu_rb;
> > + u32 ceu_rc;
> > + u32 ceu_gr;
> > + u32 ceu_gg;
> > + u32 ceu_gb;
> > + u32 ceu_gc;
> > + u32 ceu_br;
> > + u32 ceu_bg;
> > + u32 ceu_bb;
> > + u32 ceu_bc;
> > + u32 ceu_rv;
> > + u32 ceu_gv;
> > + u32 ceu_bv;
> > + u32 dum6[45];
> > +
> > + u32 mux_ctl; /* 0x200 */
> > + u32 dum7[63];
> > +
> > + u32 fill_ctl; /* 0x300 */
> > + u32 fill_start0;
> > + u32 fill_end0;
> > + u32 fill_data0;
> > +};
>
> Please use defines instead of the structures.
I think that structures are more readable.
> > +
> > +#define XY(x, y) (((x) << 16) | (y))
> > +
> > +#define tcon_read(base, member) \
> > + readl_relaxed(base + offsetof(struct tcon, member))
> > +#define tcon_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct tcon, member))
> > +
> > +/* vertical blank functions */
> > +static void de2_atomic_flush(struct drm_crtc *crtc,
> > + struct drm_crtc_state *old_state)
> > +{
> > + struct drm_pending_vblank_event *event = crtc->state->event;
> > +
> > + if (event) {
> > + crtc->state->event = NULL;
> > + spin_lock_irq(&crtc->dev->event_lock);
> > + if (drm_crtc_vblank_get(crtc) == 0)
> > + drm_crtc_arm_vblank_event(crtc, event);
> > + else
> > + drm_crtc_send_vblank_event(crtc, event);
> > + spin_unlock_irq(&crtc->dev->event_lock);
> > + }
> > +}
> > +
> > +static irqreturn_t de2_lcd_irq(int irq, void *dev_id)
> > +{
> > + struct lcd *lcd = (struct lcd *) dev_id;
> > + u32 isr;
> > +
> > + isr = tcon_read(lcd->mmio, gint0);
> > +
> > + drm_crtc_handle_vblank(&lcd->crtc);
> > +
> > + tcon_write(lcd->mmio, gint0, isr & ~TCON_GINT0_TCON1_Vb_Int_Flag);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +int de2_enable_vblank(struct drm_device *drm, unsigned crtc)
> > +{
> > + struct priv *priv = drm->dev_private;
> > + struct lcd *lcd = priv->lcds[crtc];
> > +
> > + tcon_write(lcd->mmio, gint0,
> > + tcon_read(lcd->mmio, gint0) |
> > + TCON_GINT0_TCON1_Vb_Int_En);
>
> That's a weird indentation
>
> > + return 0;
> > +}
> > +
> > +void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
> > +{
> > + struct priv *priv = drm->dev_private;
> > + struct lcd *lcd = priv->lcds[crtc];
> > +
> > + tcon_write(lcd->mmio, gint0,
> > + tcon_read(lcd->mmio, gint0) &
> > + ~TCON_GINT0_TCON1_Vb_Int_En);
> > +}
> > +
> > +/* panel functions */
>
> Panel functions? In the CRTC driver?
Yes, dumb panel.
> > +static void de2_set_frame_timings(struct lcd *lcd)
> > +{
> > + struct drm_crtc *crtc = &lcd->crtc;
> > + const struct drm_display_mode *mode = &crtc->mode;
> > + int interlace = mode->flags & DRM_MODE_FLAG_INTERLACE ? 2 : 1;
> > + int start_delay;
> > + u32 data;
> > +
> > + data = XY(mode->hdisplay - 1, mode->vdisplay / interlace - 1);
> > + tcon_write(lcd->mmio, basic0, data);
> > + tcon_write(lcd->mmio, basic1, data);
> > + tcon_write(lcd->mmio, basic2, data);
> > + tcon_write(lcd->mmio, basic3,
> > + XY(mode->htotal - 1,
> > + mode->htotal - mode->hsync_start - 1));
> > + tcon_write(lcd->mmio, basic4,
> > + XY(mode->vtotal * (3 - interlace),
> > + mode->vtotal - mode->vsync_start - 1));
> > + tcon_write(lcd->mmio, basic5,
> > + XY(mode->hsync_end - mode->hsync_start - 1,
> > + mode->vsync_end - mode->vsync_start - 1));
> > +
> > + tcon_write(lcd->mmio, ps_sync, XY(1, 1));
> > +
> > + data = TCON1_IO_POL_IO2_inv;
> > + if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > + data |= TCON1_IO_POL_IO0_inv;
> > + if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > + data |= TCON1_IO_POL_IO1_inv;
> > + tcon_write(lcd->mmio, io_pol, data);
> > +
> > + tcon_write(lcd->mmio, ceu_ctl,
> > + tcon_read(lcd->mmio, ceu_ctl) & ~TCON_CEU_CTL_ceu_en);
> > +
> > + data = tcon_read(lcd->mmio, tcon1_ctl);
> > + if (interlace == 2)
> > + data |= TCON1_CTL_Interlace_En;
> > + else
> > + data &= ~TCON1_CTL_Interlace_En;
> > + tcon_write(lcd->mmio, tcon1_ctl, data);
> > +
> > + tcon_write(lcd->mmio, fill_ctl, 0);
> > + tcon_write(lcd->mmio, fill_start0, mode->vtotal + 1);
> > + tcon_write(lcd->mmio, fill_end0, mode->vtotal);
> > + tcon_write(lcd->mmio, fill_data0, 0);
> > +
> > + start_delay = (mode->vtotal - mode->vdisplay) / interlace - 5;
> > + if (start_delay > 31)
> > + start_delay = 31;
> > + data = tcon_read(lcd->mmio, tcon1_ctl);
> > + data &= ~TCON1_CTL_Start_Delay_MASK;
> > + data |= start_delay << TCON1_CTL_Start_Delay_SHIFT;
> > + tcon_write(lcd->mmio, tcon1_ctl, data);
> > +
> > + tcon_write(lcd->mmio, io_tri, 0x0fffffff);
> > +}
>
> Some comments here would be nice, there's a lot of non trivial things.
... and no documentation. I just set the values I saw in the I/O memory
when running the legacy driver.
> > +
> > +static void de2_crtc_enable(struct drm_crtc *crtc)
> > +{
> > + struct lcd *lcd = crtc_to_lcd(crtc);
> > + struct drm_display_mode *mode = &crtc->mode;
> > +
> > + DRM_DEBUG_DRIVER("\n");
>
> Log something useful, or don't.
This was useful when the driver was crashing only God knew where.
> > +
> > + clk_set_rate(lcd->clk, mode->clock * 1000);
> > +
> > + de2_set_frame_timings(lcd);
> > +
> > + tcon_write(lcd->mmio, tcon1_ctl,
> > + tcon_read(lcd->mmio, tcon1_ctl) | TCON1_CTL_TCON_En);
> > +
> > + de2_de_panel_init(lcd->priv, lcd->num, mode);
>
> panel_init in the CRTC enable? Shouldn't that be in the panel driver?
> or at least the encoder?
I will change to 'dumb panel'.
> > +
> > + drm_mode_debug_printmodeline(mode);
>
> This is already printed by the core.
Not at this time.
[snip]
> > +static int de2_lcd_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node, *tmp, *parent, *port;
> > + struct lcd *lcd;
> > + struct resource *res;
> > + int id, irq, ret;
> > +
> > + id = of_alias_get_id(np, "lcd");
> > + if (id < 0) {
> > + dev_err(dev, "no alias for lcd\n");
> > + id = 0;
> > + }
> > + lcd = devm_kzalloc(dev, sizeof *lcd, GFP_KERNEL);
> > + if (!lcd) {
> > + dev_err(dev, "failed to allocate private data\n");
> > + return -ENOMEM;
> > + }
> > + dev_set_drvdata(dev, lcd);
> > + lcd->dev = dev;
> > + lcd->num = id;
>
> What do you need this number for?
It permits access to the overlay planes in the DE2.
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(dev, "failed to get memory resource\n");
> > + return -EINVAL;
> > + }
> > +
> > + lcd->mmio = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(lcd->mmio)) {
> > + dev_err(dev, "failed to map registers\n");
> > + return PTR_ERR(lcd->mmio);
> > + }
> > +
> > + snprintf(lcd->name, sizeof(lcd->name), "sunxi-lcd%d", id);
> > +
> > + /* possible CRTCs */
> > + parent = np;
> > + tmp = of_get_child_by_name(np, "ports");
> > + if (tmp)
> > + parent = tmp;
> > + port = of_get_child_by_name(parent, "port");
> > + of_node_put(tmp);
> > + if (!port) {
> > + dev_err(dev, "no port node\n");
> > + return -ENXIO;
> > + }
> > + lcd->crtc.port = port;
> > +
> > + lcd->gate = devm_clk_get(dev, "gate"); /* optional */
>
> Having some kind of error checking would still be nice.
And cancel the device creation?
> > +
> > + lcd->clk = devm_clk_get(dev, "clock");
> > + if (IS_ERR(lcd->clk)) {
> > + dev_err(dev, "video clock err %d\n", (int) PTR_ERR(lcd->clk));
> > + ret = PTR_ERR(lcd->clk);
> > + goto err;
> > + }
> > +
> > + lcd->rstc = devm_reset_control_get_optional(dev, NULL);
>
> Ditto.
>
> > +
> > + irq = irq_of_parse_and_map(np, 0);
> > + if (irq <= 0 || irq == NO_IRQ) {
> > + dev_err(dev, "unable to get irq lcd %d\n", id);
> > + ret = -EINVAL;
> > + goto err;
> > + }
>
> You can use platform_get_irq for that.
Right. Thanks.
> > +
> > + if (!IS_ERR(lcd->rstc)) {
> > + ret = reset_control_deassert(lcd->rstc);
> > + if (ret) {
> > + dev_err(dev, "reset deassert err %d\n", ret);
> > + goto err;
> > + }
> > + }
> > +
> > + if (!IS_ERR(lcd->gate)) {
> > + ret = clk_prepare_enable(lcd->gate);
> > + if (ret)
> > + goto err2;
> > + }
> > +
> > + ret = clk_prepare_enable(lcd->clk);
> > + if (ret)
> > + goto err2;
>
> Is there any reason not to do that in the enable / disable? Leaving
> clocks running while the device has no guarantee that it's going to be
> used seems like a waste of resources.
If the machine does not need video (network server, router..), it is simpler
to prevent the video driver to be loaded (DT, module black list...).
> > +
> > + de2_tcon_init(lcd);
> > +
> > + ret = devm_request_irq(dev, irq, de2_lcd_irq, 0,
> > + lcd->name, lcd);
> > + if (ret < 0) {
> > + dev_err(dev, "unable to request irq %d\n", irq);
> > + goto err2;
> > + }
> > +
> > + return component_add(dev, &de2_lcd_ops);
> > +
> > +err2:
> > + if (!IS_ERR_OR_NULL(lcd->rstc))
> > + reset_control_assert(lcd->rstc);
> > + clk_disable_unprepare(lcd->gate);
> > + clk_disable_unprepare(lcd->clk);
> > +err:
> > + of_node_put(lcd->crtc.port);
> > + return ret;
> > +}
> > +
> > +static int de2_lcd_remove(struct platform_device *pdev)
> > +{
> > + struct lcd *lcd = platform_get_drvdata(pdev);
> > +
> > + component_del(&pdev->dev, &de2_lcd_ops);
> > +
> > + if (!IS_ERR_OR_NULL(lcd->rstc))
> > + reset_control_assert(lcd->rstc);
> > + clk_disable_unprepare(lcd->gate);
> > + clk_disable_unprepare(lcd->clk);
> > + of_node_put(lcd->crtc.port);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id de2_lcd_ids[] = {
> > + { .compatible = "allwinner,sun8i-a83t-lcd", },
> > + { }
> > +};
> > +
> > +struct platform_driver de2_lcd_platform_driver = {
> > + .probe = de2_lcd_probe,
> > + .remove = de2_lcd_remove,
> > + .driver = {
> > + .name = "sunxi-de2-lcd",
> > + .of_match_table = of_match_ptr(de2_lcd_ids),
> > + },
> > +};
> > diff --git a/drivers/gpu/drm/sunxi/de2_crtc.h b/drivers/gpu/drm/sunxi/de2_crtc.h
> > new file mode 100644
> > index 0000000..efbe45d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_crtc.h
> > @@ -0,0 +1,63 @@
> > +#ifndef __DE2_CRTC_H__
> > +#define __DE2_CRTC_H__
> > +/*
> > + * Copyright (C) 2016 Jean-Fran?ois Moine
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/reset.h>
> > +#include <drm/drm_plane_helper.h>
> > +
> > +struct priv;
> > +
> > +enum de2_plane2 {
> > + DE2_PRIMARY_PLANE,
> > + DE2_CURSOR_PLANE,
> > + DE2_VI_PLANE,
> > + DE2_N_PLANES,
> > +};
> > +struct lcd {
> > + void __iomem *mmio;
> > +
> > + struct device *dev;
> > + struct drm_crtc crtc;
> > + struct priv *priv; /* DRM/DE private data */
> > +
> > + short num; /* LCD number in hardware */
> > + short crtc_idx; /* CRTC index in drm */
> > +
> > + struct clk *clk;
> > + struct clk *gate;
> > + struct reset_control *rstc;
> > +
> > + char name[16];
> > +
> > + struct drm_pending_vblank_event *event;
> > +
> > + struct drm_plane planes[DE2_N_PLANES];
> > +};
> > +
> > +#define crtc_to_lcd(x) container_of(x, struct lcd, crtc)
> > +
> > +/* in de2_de.c */
> > +void de2_de_enable(struct priv *priv, int lcd_num);
> > +void de2_de_disable(struct priv *priv, int lcd_num);
> > +void de2_de_hw_init(struct priv *priv, int lcd_num);
> > +void de2_de_panel_init(struct priv *priv, int lcd_num,
> > + struct drm_display_mode *mode);
> > +void de2_de_plane_disable(struct priv *priv,
> > + int lcd_num, int plane_ix);
> > +void de2_de_plane_update(struct priv *priv,
> > + int lcd_num, int plane_ix,
> > + struct drm_plane_state *state,
> > + struct drm_plane_state *old_state);
>
> Does it need to be exported?
I don't understand the question.
> > +
> > +/* in de2_plane.c */
> > +int de2_plane_init(struct drm_device *drm, struct lcd *lcd);
> > +
> > +#endif /* __DE2_CRTC_H__ */
> > diff --git a/drivers/gpu/drm/sunxi/de2_de.c b/drivers/gpu/drm/sunxi/de2_de.c
> > new file mode 100644
> > index 0000000..0d8cb62
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_de.c
> > @@ -0,0 +1,591 @@
> > +/*
> > + * ALLWINNER DRM driver - Display Engine 2
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine <moinejf@free.fr>
> > + * Copyright (c) 2016 Allwinnertech Co., Ltd.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <asm/io.h>
> > +#include <drm/drm_gem_cma_helper.h>
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +static DEFINE_SPINLOCK(de_lock);
> > +
> > +#define DE_CLK_RATE_A83T 504000000 /* pll-de */
> > +#define DE_CLK_RATE_H3 432000000 /* de */
>
> This can be set in the DT.
That's what I had in first releases, but, recently, I saw a 'set
parent' code in a clock driver, because 'it is not the plan to expose'
some clocks in the DT. I am glad to move these clock rate settings
back to the DT.
> > +
> > +/* I/O map */
> > +
> > +#define DE_MOD_REG 0x0000 /* 1 bit per LCD */
> > +#define DE_GATE_REG 0x0004
> > +#define DE_RESET_REG 0x0008
> > +#define DE_DIV_REG 0x000c /* 4 bits per LCD */
> > +#define DE_SEL_REG 0x0010
> > +
> > +#define DE_MUX0_BASE 0x00100000
> > +#define DE_MUX1_BASE 0x00200000
> > +
> > +/* MUX registers (addr / MUX base) */
> > +#define DE_MUX_GLB_REGS 0x00000 /* global control */
> > +#define DE_MUX_BLD_REGS 0x01000 /* alpha blending */
> > +#define DE_MUX_CHAN_REGS 0x02000 /* VI/UI overlay channels */
> > +#define DE_MUX_CHAN_SZ 0x1000 /* size of a channel */
> > +#define DE_MUX_VSU_REGS 0x20000 /* VSU */
> > +#define DE_MUX_GSU1_REGS 0x30000 /* GSUs */
> > +#define DE_MUX_GSU2_REGS 0x40000
> > +#define DE_MUX_GSU3_REGS 0x50000
> > +#define DE_MUX_FCE_REGS 0xa0000 /* FCE */
> > +#define DE_MUX_BWS_REGS 0xa2000 /* BWS */
> > +#define DE_MUX_LTI_REGS 0xa4000 /* LTI */
> > +#define DE_MUX_PEAK_REGS 0xa6000 /* PEAK */
> > +#define DE_MUX_ASE_REGS 0xa8000 /* ASE */
> > +#define DE_MUX_FCC_REGS 0xaa000 /* FCC */
> > +#define DE_MUX_DCSC_REGS 0xb0000 /* DCSC/SMBL */
> > +
> > +/* global control */
> > +struct de_glb {
> > + u32 ctl;
> > +#define DE_MUX_GLB_CTL_rt_en BIT(0)
> > +#define DE_MUX_GLB_CTL_finish_irq_en BIT(4)
> > +#define DE_MUX_GLB_CTL_rtwb_port BIT(12)
> > + u32 status;
> > + u32 dbuff;
> > + u32 size;
> > +};
> > +
> > +/* alpha blending */
> > +struct de_bld {
> > + u32 fcolor_ctl; /* 00 */
> > + struct {
> > + u32 fcolor;
> > + u32 insize;
> > + u32 offset;
> > + u32 dum;
> > + } attr[4];
> > + u32 dum0[15]; /* (end of clear offset) */
> > + u32 route; /* 80 */
> > + u32 premultiply;
> > + u32 bkcolor;
> > + u32 output_size;
> > + u32 bld_mode[4];
> > + u32 dum1[4];
> > + u32 ck_ctl; /* b0 */
> > + u32 ck_cfg;
> > + u32 dum2[2];
> > + u32 ck_max[4]; /* c0 */
> > + u32 dum3[4];
> > + u32 ck_min[4]; /* e0 */
> > + u32 dum4[3];
> > + u32 out_ctl; /* fc */
> > +};
> > +
> > +/* VI channel */
> > +struct de_vi {
> > + struct {
> > + u32 attr;
> > +#define VI_CFG_ATTR_en BIT(0)
> > +#define VI_CFG_ATTR_fcolor_en BIT(4)
> > +#define VI_CFG_ATTR_fmt_SHIFT 8
> > +#define VI_CFG_ATTR_fmt_MASK GENMASK(12, 8)
> > +#define VI_CFG_ATTR_ui_sel BIT(15)
> > +#define VI_CFG_ATTR_top_down BIT(23)
> > + u32 size;
> > + u32 coord;
> > +#define VI_N_PLANES 3
> > + u32 pitch[VI_N_PLANES];
> > + u32 top_laddr[VI_N_PLANES];
> > + u32 bot_laddr[VI_N_PLANES];
> > + } cfg[4];
> > + u32 fcolor[4]; /* c0 */
> > + u32 top_haddr[VI_N_PLANES]; /* d0 */
> > + u32 bot_haddr[VI_N_PLANES]; /* dc */
> > + u32 ovl_size[2]; /* e8 */
> > + u32 hori[2]; /* f0 */
> > + u32 vert[2]; /* f8 */
> > +};
> > +
> > +/* UI channel */
> > +struct de_ui {
> > + struct {
> > + u32 attr;
> > +#define UI_CFG_ATTR_en BIT(0)
> > +#define UI_CFG_ATTR_alpmod_SHIFT 1
> > +#define UI_CFG_ATTR_alpmod_MASK GENMASK(2, 1)
> > +#define UI_CFG_ATTR_fcolor_en BIT(4)
> > +#define UI_CFG_ATTR_fmt_SHIFT 8
> > +#define UI_CFG_ATTR_fmt_MASK GENMASK(12, 8)
> > +#define UI_CFG_ATTR_top_down BIT(23)
> > +#define UI_CFG_ATTR_alpha_SHIFT 24
> > +#define UI_CFG_ATTR_alpha_MASK GENMASK(31, 24)
> > + u32 size;
> > + u32 coord;
> > + u32 pitch;
> > + u32 top_laddr;
> > + u32 bot_laddr;
> > + u32 fcolor;
> > + u32 dum;
> > + } cfg[4]; /* 00 */
> > + u32 top_haddr; /* 80 */
> > + u32 bot_haddr;
> > + u32 ovl_size; /* 88 */
> > +};
>
> Please use defines instead of the structures.
Juggling with data in arrays is painful when the offsets are defined by
defines, and structures are so much readable.
> > +
> > +/* coordinates and sizes */
> > +#define XY(x, y) (((y) << 16) | (x))
> > +#define WH(w, h) (((h - 1) << 16) | (w - 1))
> > +
> > +/* UI video formats */
> > +#define DE2_FORMAT_ARGB_8888 0
> > +#define DE2_FORMAT_BGRA_8888 3
> > +#define DE2_FORMAT_XRGB_8888 4
> > +#define DE2_FORMAT_RGB_888 8
> > +#define DE2_FORMAT_BGR_888 9
> > +
> > +/* VI video formats */
> > +#define DE2_FORMAT_YUV422_I_YVYU 1 /* Y-V-Y-U */
> > +#define DE2_FORMAT_YUV422_I_UYVY 2 /* U-Y-V-Y */
> > +#define DE2_FORMAT_YUV422_I_YUYV 3 /* Y-U-Y-V */
> > +#define DE2_FORMAT_YUV422_P 6 /* YYYY UU VV planar */
> > +#define DE2_FORMAT_YUV420_P 10 /* YYYY U V planar */
> > +
> > +#define glb_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_glb, member))
> > +#define glb_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_glb, member))
> > +#define bld_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_bld, member))
> > +#define bld_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_bld, member))
> > +#define ui_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_ui, member))
> > +#define ui_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_ui, member))
> > +#define vi_read(base, member) \
> > + readl_relaxed(base + offsetof(struct de_vi, member))
> > +#define vi_write(base, member, data) \
> > + writel_relaxed(data, base + offsetof(struct de_vi, member))
> > +
> > +static const struct {
> > + char chan;
> > + char layer;
> > + char pipe;
> > +} plane2layer[DE2_N_PLANES] = {
> > + [DE2_PRIMARY_PLANE] = {0, 0, 0},
> > + [DE2_CURSOR_PLANE] = {1, 0, 1},
> > + [DE2_VI_PLANE] = {0, 1, 0},
> > +};
>
> Comments?
This
primary plane is channel 0 (VI), layer 0, pipe 0
cursor plane is channel 1 (UI), layer 0, pipe 1
overlay plane is channel 0 (VI), layer 1, pipe 0
or the full explanation:
Constraints:
The VI channels can do RGB or YUV, while UI channels can do RGB
only.
The LCD 0 has 1 VI channel and 4 UI channels, while
LCD 1 has only 1 VI channel and 1 UI channel.
The cursor must go to a channel bigger than the primary channel,
otherwise it is not transparent.
First try:
Letting the primary plane (usually RGB) in the 2nd channel (UI),
as this is done in the legacy driver, asks for the cursor to go
to the next channel (UI), but this one does not exist in LCD1.
Retained layout:
So, we must use only 2 channels for the same behaviour on LCD0
(H3) and LCD1 (A83T)
The retained combination is:
- primary plane in the first channel (VI),
- cursor plane inthe 2nd channel (UI), and
- overlay plane in the 1st channel (VI).
Note that there could be 3 overlay planes (a channel has 4
layers), but I am not sure that the A83T or the H3 could
support 3 simultaneous video streams...
> > +static inline void de_write(struct priv *priv, int reg, u32 data)
> > +{
> > + writel_relaxed(data, priv->mmio + reg);
> > +}
> > +
> > +static inline u32 de_read(struct priv *priv, int reg)
> > +{
> > + return readl_relaxed(priv->mmio + reg);
> > +}
> > +
> > +static void de_lcd_select(struct priv *priv,
> > + int lcd_num,
> > + void __iomem *mux_o)
> > +{
> > + u32 data;
> > +
> > + /* select the LCD */
> > + data = de_read(priv, DE_SEL_REG);
> > + data &= ~1;
> > + de_write(priv, DE_SEL_REG, data);
> > +
> > + /* double register switch */
> > + glb_write(mux_o + DE_MUX_GLB_REGS, dbuff, 1);
> > +}
> > +
> > +void de2_de_plane_update(struct priv *priv,
> > + int lcd_num, int plane_ix,
> > + struct drm_plane_state *state,
> > + struct drm_plane_state *old_state)
> > +{
> > + struct drm_framebuffer *fb = state->fb;
> > + struct drm_gem_cma_object *gem;
> > + void __iomem *mux_o = priv->mmio;
> > + void __iomem *chan_o;
> > + u32 size = WH(state->crtc_w, state->crtc_h);
> > + u32 coord;
> > + u32 screen_size;
> > + u32 data, fcolor;
> > + u32 ui_sel, alpha_glob;
> > + int chan, layer, x, y;
> > + unsigned fmt;
> > + unsigned long flags;
> > +
> > + chan = plane2layer[plane_ix].chan;
> > + layer = plane2layer[plane_ix].layer;
> > +
> > + mux_o += (lcd_num == 0) ? DE_MUX0_BASE : DE_MUX1_BASE;
> > + chan_o = mux_o;
> > + chan_o += DE_MUX_CHAN_REGS + DE_MUX_CHAN_SZ * chan;
> > +
> > + x = state->crtc_x >= 0 ? state->crtc_x : 0;
> > + y = state->crtc_y >= 0 ? state->crtc_y : 0;
> > + coord = XY(x, y);
> > +
> > + /* handle the cursor move */
> > + if (plane_ix == DE2_CURSOR_PLANE
> > + && fb == old_state->fb) {
> > + spin_lock_irqsave(&de_lock, flags);
> > + de_lcd_select(priv, lcd_num, mux_o);
> > + if (chan == 0)
> > + vi_write(chan_o, cfg[layer].coord, coord);
> > + else
> > + ui_write(chan_o, cfg[layer].coord, coord);
> > + spin_unlock_irqrestore(&de_lock, flags);
> > + return;
> > + }
> > +
> > + gem = drm_fb_cma_get_gem_obj(fb, 0);
> > +
> > + ui_sel = alpha_glob = 0;
> > + switch (fb->pixel_format) {
> > + case DRM_FORMAT_ARGB8888:
> > + fmt = DE2_FORMAT_ARGB_8888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_BGRA8888:
> > + fmt = DE2_FORMAT_BGRA_8888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_XRGB8888:
> > + fmt = DE2_FORMAT_XRGB_8888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + alpha_glob = (1 << UI_CFG_ATTR_alpmod_SHIFT) |
> > + (0xff << UI_CFG_ATTR_alpha_SHIFT);
> > + break;
> > + case DRM_FORMAT_RGB888:
> > + fmt = DE2_FORMAT_RGB_888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_BGR888:
> > + fmt = DE2_FORMAT_BGR_888;
> > + ui_sel = VI_CFG_ATTR_ui_sel;
> > + break;
> > + case DRM_FORMAT_YUYV:
> > + fmt = DE2_FORMAT_YUV422_I_YUYV;
> > + break;
> > + case DRM_FORMAT_YVYU:
> > + fmt = DE2_FORMAT_YUV422_I_YVYU;
> > + break;
> > + case DRM_FORMAT_YUV422:
> > + fmt = DE2_FORMAT_YUV422_P;
> > + break;
> > + case DRM_FORMAT_YUV420:
> > + fmt = DE2_FORMAT_YUV420_P;
> > + break;
> > + case DRM_FORMAT_UYVY:
> > + fmt = DE2_FORMAT_YUV422_I_UYVY;
> > + break;
> > + default:
> > + pr_err("format %.4s not yet treated\n",
> > + (char *) &fb->pixel_format);
> > + return;
> > + }
> > +
> > + spin_lock_irqsave(&de_lock, flags);
> > +
> > + screen_size = plane_ix == DE2_PRIMARY_PLANE ?
> > + size :
> > + glb_read(mux_o + DE_MUX_GLB_REGS, size);
> > +
> > + /* prepare the activation of alpha blending (1 bit per plane) */
> > + fcolor = bld_read(mux_o + DE_MUX_BLD_REGS, fcolor_ctl)
> > + | (0x100 << plane2layer[plane_ix].pipe);
> > +
> > + de_lcd_select(priv, lcd_num, mux_o);
> > +
> > + if (chan == 0) { /* VI channel */
> > + int i;
> > +
> > + data = VI_CFG_ATTR_en | (fmt << VI_CFG_ATTR_fmt_SHIFT) |
> > + ui_sel;
> > + vi_write(chan_o, cfg[layer].attr, data);
> > + vi_write(chan_o, cfg[layer].size, size);
> > + vi_write(chan_o, cfg[layer].coord, coord);
> > + for (i = 0; i < VI_N_PLANES; i++) {
> > + vi_write(chan_o, cfg[layer].pitch[i],
> > + fb->pitches[i] ? fb->pitches[i] :
> > + fb->pitches[0]);
> > + vi_write(chan_o, cfg[layer].top_laddr[i],
> > + gem->paddr + fb->offsets[i]);
> > + vi_write(chan_o, fcolor[layer], 0xff000000);
> > + }
> > + if (layer == 0)
> > + vi_write(chan_o, ovl_size[0], screen_size);
> > +
> > + } else { /* UI channel */
> > + data = UI_CFG_ATTR_en | (fmt << UI_CFG_ATTR_fmt_SHIFT) |
> > + alpha_glob;
> > + ui_write(chan_o, cfg[layer].attr, data);
> > + ui_write(chan_o, cfg[layer].size, size);
> > + ui_write(chan_o, cfg[layer].coord, coord);
> > + ui_write(chan_o, cfg[layer].pitch, fb->pitches[0]);
> > + ui_write(chan_o, cfg[layer].top_laddr,
> > + gem->paddr + fb->offsets[0]);
> > + if (layer == 0)
> > + ui_write(chan_o, ovl_size, screen_size);
> > + }
> > + bld_write(mux_o + DE_MUX_BLD_REGS, fcolor_ctl, fcolor);
> > +
> > + spin_unlock_irqrestore(&de_lock, flags);
> > +}
>
> Splitting that into functions would make it a bit more trivial and
> readable.
Not sure: there is a lot of common data and different I/O accesses.
> > +void de2_de_plane_disable(struct priv *priv,
> > + int lcd_num, int plane_ix)
> > +{
> > + void __iomem *mux_o = priv->mmio;
> > + void __iomem *chan_o;
> > + u32 fcolor;
> > + int chan, layer, chan_disable = 0;
> > + unsigned long flags;
> > +
> > + chan = plane2layer[plane_ix].chan;
> > + layer = plane2layer[plane_ix].layer;
> > +
> > + mux_o += (lcd_num == 0) ? DE_MUX0_BASE : DE_MUX1_BASE;
> > + chan_o = mux_o;
> > + chan_o += DE_MUX_CHAN_REGS + DE_MUX_CHAN_SZ * chan;
> > +
> > + /* (only 2 layers) */
> > + if (chan == 0) {
> > + if (vi_read(chan_o, cfg[1 - layer].attr) == 0)
> > + chan_disable = 1;
> > + } else {
> > + if (ui_read(chan_o, cfg[1 - layer].attr) == 0)
> > + chan_disable = 1;
> > + }
> > +
> > + spin_lock_irqsave(&de_lock, flags);
> > +
> > + fcolor = bld_read(mux_o + DE_MUX_BLD_REGS, fcolor_ctl);
> > +
> > + de_lcd_select(priv, lcd_num, mux_o);
> > +
> > + if (chan == 0)
> > + vi_write(chan_o, cfg[layer].attr, 0);
> > + else
> > + ui_write(chan_o, cfg[layer].attr, 0);
> > +
> > + if (chan_disable)
> > + bld_write(mux_o + DE_MUX_BLD_REGS, fcolor_ctl,
> > + fcolor & ~(0x100 << plane2layer[plane_ix].pipe));
> > +
> > + spin_unlock_irqrestore(&de_lock, flags);
> > +}
>
> Can't you just disable it?
Which 'it'? A layer must be disabled and it is not useful to let the
DE2 processor to scan a pipe (channel) without any layer.
> > +void de2_de_panel_init(struct priv *priv, int lcd_num,
> > + struct drm_display_mode *mode)
> > +{
> > + void __iomem *mux_o = priv->mmio;
> > + u32 size = WH(mode->hdisplay, mode->vdisplay);
> > + unsigned i;
> > + unsigned long flags;
> > +
> > + mux_o += (lcd_num == 0) ? DE_MUX0_BASE : DE_MUX1_BASE;
> > +
> > + DRM_DEBUG_DRIVER("%dx%d\n", mode->hdisplay, mode->vdisplay);
> > +
> > + spin_lock_irqsave(&de_lock, flags);
> > +
> > + de_lcd_select(priv, lcd_num, mux_o);
> > +
> > + glb_write(mux_o + DE_MUX_GLB_REGS, size, size);
> > +
> > + /* set alpha blending */
> > + for (i = 0; i < 4; i++) {
> > + bld_write(mux_o + DE_MUX_BLD_REGS, attr[i].fcolor, 0xff000000);
> > + bld_write(mux_o + DE_MUX_BLD_REGS, attr[i].insize, size);
> > + }
> > + bld_write(mux_o + DE_MUX_BLD_REGS, output_size, size);
> > + bld_write(mux_o + DE_MUX_BLD_REGS, out_ctl,
> > + mode->flags & DRM_MODE_FLAG_INTERLACE ? 2 : 0);
> > +
> > + spin_unlock_irqrestore(&de_lock, flags);
> > +}
> > +
> > +void de2_de_enable(struct priv *priv, int lcd_num)
> > +{
[snip]
> > +}
> > +
> > +void de2_de_disable(struct priv *priv, int lcd_num)
> > +{
> > + u32 data;
> > +
> > + data = ~(1 << lcd_num);
> > + de_write(priv, DE_MOD_REG,
> > + de_read(priv, DE_MOD_REG) & data);
> > + de_write(priv, DE_GATE_REG,
> > + de_read(priv, DE_GATE_REG) & data);
> > + de_write(priv, DE_RESET_REG,
> > + de_read(priv, DE_RESET_REG) & data);
> > +}
> > +
> > +int de2_de_init(struct priv *priv, struct device *dev)
> > +{
> > + struct resource *res;
> > + int ret;
> > +
> > + DRM_DEBUG_DRIVER("\n");
> > +
> > + res = platform_get_resource(to_platform_device(dev),
> > + IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(dev, "failed to get memory resource\n");
> > + return -EINVAL;
> > + }
> > +
> > + priv->mmio = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(priv->mmio)) {
> > + dev_err(dev, "failed to map registers\n");
> > + return PTR_ERR(priv->mmio);
> > + }
> > +
> > + priv->gate = devm_clk_get(dev, "gate"); /* optional */
>
> Error checking
>
> > +
> > + priv->clk = devm_clk_get(dev, "clock");
> > + if (IS_ERR(priv->clk)) {
> > + dev_err(dev, "video clock err %d\n", (int) PTR_ERR(priv->clk));
> > + return PTR_ERR(priv->clk);
> > + }
> > +
> > + priv->rstc = devm_reset_control_get_optional(dev, NULL);
> > +
> > + if (!IS_ERR(priv->rstc)) {
> > + ret = reset_control_deassert(priv->rstc);
> > + if (ret) {
> > + dev_err(dev, "reset deassert err %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + if (!IS_ERR(priv->gate)) {
> > + ret = clk_prepare_enable(priv->gate);
> > + if (ret)
> > + goto err_gate;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret)
> > + goto err_enable;
> > + if (priv->soc_type == SOC_A83T)
> > + clk_set_rate(priv->clk, DE_CLK_RATE_A83T);
> > + else
> > + clk_set_rate(priv->clk, DE_CLK_RATE_H3);
> > +
> > + /* set the A83T clock divider = 500 / 250 */
> > + if (priv->soc_type == SOC_A83T)
> > + de_write(priv, DE_DIV_REG,
> > + 0x00000011); /* div = 2 for both LCDs */
> > +
> > + return 0;
> > +
> > +err_enable:
> > + clk_disable_unprepare(priv->gate);
> > +err_gate:
> > + if (!IS_ERR(priv->rstc))
> > + reset_control_assert(priv->rstc);
> > + return ret;
> > +}
> > +
> > +void de2_de_cleanup(struct priv *priv)
> > +{
> > + clk_disable_unprepare(priv->clk);
> > + clk_disable_unprepare(priv->gate);
> > + if (!IS_ERR(priv->rstc))
> > + reset_control_assert(priv->rstc);
> > +}
[snip]
> > diff --git a/drivers/gpu/drm/sunxi/de2_drv.c b/drivers/gpu/drm/sunxi/de2_drv.c
> > new file mode 100644
> > index 0000000..5daa15c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_drv.c
[snip]
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +/*
> > + * Power management
> > + */
> > +static int de2_pm_suspend(struct device *dev)
> > +{
> > + struct drm_device *drm = dev_get_drvdata(dev);
> > +
> > + drm_kms_helper_poll_disable(drm);
> > + return 0;
> > +}
> > +
> > +static int de2_pm_resume(struct device *dev)
> > +{
> > + struct drm_device *drm = dev_get_drvdata(dev);
> > +
> > + drm_kms_helper_poll_enable(drm);
> > + return 0;
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops de2_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(de2_pm_suspend, de2_pm_resume)
> > +};
>
> Why do you need that? How did you test it? There's no runtime_pm calls
> in your kernel.
That's not tested. I will remove this.
[snip]
> > +static struct platform_driver de2_drm_platform_driver = {
> > + .probe = de2_drm_probe,
> > + .remove = de2_drm_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .pm = &de2_pm_ops,
> > + .of_match_table = de2_drm_of_match,
> > + },
> > +};
> > +
> > +static int __init de2_drm_init(void)
> > +{
> > + int ret;
> > +
> > +/* uncomment to activate the drm traces at startup time */
> > +/* drm_debug = DRM_UT_CORE | DRM_UT_DRIVER | DRM_UT_KMS |
> > + DRM_UT_PRIME | DRM_UT_ATOMIC; */
>
> That's useless.
Right, but it seems that some people don't know how to debug a DRM
driver. This is only a reminder.
> > + DRM_DEBUG_DRIVER("\n");
> > +
> > + ret = platform_driver_register(&de2_lcd_platform_driver);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = platform_driver_register(&de2_drm_platform_driver);
> > + if (ret < 0)
> > + platform_driver_unregister(&de2_lcd_platform_driver);
> > +
> > + return ret;
> > +}
>
> And that really shouldn't be done that way.
May you explain?
> > +static void __exit de2_drm_fini(void)
> > +{
> > + platform_driver_unregister(&de2_lcd_platform_driver);
> > + platform_driver_unregister(&de2_drm_platform_driver);
> > +}
> > +
> > +module_init(de2_drm_init);
> > +module_exit(de2_drm_fini);
> > +
> > +MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>");
> > +MODULE_DESCRIPTION("Allwinner DE2 DRM Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/gpu/drm/sunxi/de2_plane.c b/drivers/gpu/drm/sunxi/de2_plane.c
> > new file mode 100644
> > index 0000000..b338684
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_plane.c
> > @@ -0,0 +1,119 @@
> > +/*
> > + * Allwinner DRM driver - DE2 planes
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine <moinejf@free.fr>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_plane_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +/* plane formats */
> > +static const uint32_t ui_formats[] = {
> > + DRM_FORMAT_ARGB8888,
> > + DRM_FORMAT_BGRA8888,
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_RGB888,
> > + DRM_FORMAT_BGR888,
> > +};
> > +
> > +static const uint32_t vi_formats[] = {
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_YUYV,
> > + DRM_FORMAT_YVYU,
> > + DRM_FORMAT_YUV422,
> > + DRM_FORMAT_YUV420,
> > + DRM_FORMAT_UYVY,
> > + DRM_FORMAT_BGRA8888,
> > + DRM_FORMAT_RGB888,
> > + DRM_FORMAT_BGR888,
> > +};
[snip]
> > +static int de2_one_plane_init(struct drm_device *drm,
> > + struct drm_plane *plane,
> > + int type, int possible_crtcs,
> > + const uint32_t *formats,
> > + int nformats)
> > +{
> > + int ret;
> > +
> > + ret = drm_universal_plane_init(drm, plane, possible_crtcs,
> > + &plane_funcs,
> > + formats, nformats, type, NULL);
> > + if (ret >= 0)
> > + drm_plane_helper_add(plane, &plane_helper_funcs);
> > +
> > + return ret;
> > +}
> > +
> > +int de2_plane_init(struct drm_device *drm, struct lcd *lcd)
> > +{
> > + int ret, possible_crtcs = 1 << lcd->crtc_idx;
> > +
> > + ret = de2_one_plane_init(drm, &lcd->planes[DE2_PRIMARY_PLANE],
> > + DRM_PLANE_TYPE_PRIMARY, possible_crtcs,
> > + ui_formats, ARRAY_SIZE(ui_formats));
> > + if (ret >= 0)
> > + ret = de2_one_plane_init(drm, &lcd->planes[DE2_CURSOR_PLANE],
> > + DRM_PLANE_TYPE_CURSOR, possible_crtcs,
> > + ui_formats, ARRAY_SIZE(ui_formats));
>
> Nothing looks really special about that cursor plane. Any reasion not
> to make it an overlay?
As explained above (channel/layer/pipe plane definitions), the cursor
cannot go in a channel lower or equal to the one of the primary plane.
Then, it must be known and, so, have an explicit plane.
--
Ken ar c'henta? | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
^ permalink raw reply
* [PATCH v9 1/4] soc: mediatek: Refine scpsys to support multiple platform
From: Matthias Brugger @ 2016-10-25 14:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1476953798-23263-2-git-send-email-jamesjj.liao@mediatek.com>
Hi James,
On 10/20/2016 10:56 AM, James Liao wrote:
> -static int scpsys_probe(struct platform_device *pdev)
> +static void init_clks(struct platform_device *pdev, struct clk *clk[CLK_MAX])
I prefer struct clk **clk.
> +{
> + int i;
> +
> + for (i = CLK_NONE + 1; i < CLK_MAX; i++)
> + clk[i] = devm_clk_get(&pdev->dev, clk_names[i]);
> +}
> +
> +static struct scp *init_scp(struct platform_device *pdev,
> + const struct scp_domain_data *scp_domain_data, int num)
> {
> struct genpd_onecell_data *pd_data;
> struct resource *res;
> - int i, j, ret;
> + int i, j;
> struct scp *scp;
> - struct clk *clk[MT8173_CLK_MAX];
> + struct clk *clk[CLK_MAX];
should be *[CLK_MAX - 1] but I would prefer to define in the enum:
CLK_MAX = CLK_VENC_LT,
If you are ok with it, I can fix both of my comments when applying.
Regards,
Matthias
^ permalink raw reply
* [PATCH 2/2] arm64: Support systems without FP/ASIMD
From: Ard Biesheuvel @ 2016-10-25 14:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477403433-2279-3-git-send-email-suzuki.poulose@arm.com>
Hi Suzuki,
On 25 October 2016 at 14:50, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> The arm64 kernel assumes that FP/ASIMD units are always present
> and accesses the FP/ASIMD specific registers unconditionally. This
> could cause problems when they are absent. This patch adds the
> support for kernel handling systems without FP/ASIMD by skipping the
> register access within the kernel. For kvm, we trap the accesses
> to FP/ASIMD and inject an undefined instruction exception to the VM.
>
> The callers of the exported kernel_neon_begin_parital() should
> make sure that the FP/ASIMD is supported.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> ---
> arch/arm64/crypto/aes-ce-ccm-glue.c | 2 +-
> arch/arm64/crypto/aes-ce-cipher.c | 2 ++
> arch/arm64/crypto/ghash-ce-glue.c | 2 ++
> arch/arm64/crypto/sha1-ce-glue.c | 2 ++
> arch/arm64/include/asm/cpufeature.h | 8 +++++++-
> arch/arm64/include/asm/neon.h | 3 ++-
> arch/arm64/kernel/cpufeature.c | 15 +++++++++++++++
> arch/arm64/kernel/fpsimd.c | 14 ++++++++++++++
> arch/arm64/kvm/handle_exit.c | 11 +++++++++++
> arch/arm64/kvm/hyp/hyp-entry.S | 9 ++++++++-
> arch/arm64/kvm/hyp/switch.c | 5 ++++-
> 11 files changed, 68 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
> index f4bf2f2..d001b4e 100644
> --- a/arch/arm64/crypto/aes-ce-ccm-glue.c
> +++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
> @@ -296,7 +296,7 @@ static struct aead_alg ccm_aes_alg = {
>
> static int __init aes_mod_init(void)
> {
> - if (!(elf_hwcap & HWCAP_AES))
> + if (!(elf_hwcap & HWCAP_AES) || !system_supports_fpsimd())
This looks weird to me. All crypto extensionsinstructions except CRC
operate strictly on FP/ASIMD registers, and so support for FP/ASIMD is
implied by having HWCAP_AES. In other words, I think it makes more
sense to sanity check that the info registers are consistent with each
other in core code than modifying each user (which for HWCAP_xxx
includes userland) to double check that the world is sane.
> return -ENODEV;
> return crypto_register_aead(&ccm_aes_alg);
> }
> diff --git a/arch/arm64/crypto/aes-ce-cipher.c b/arch/arm64/crypto/aes-ce-cipher.c
> index f7bd9bf..1a43be2 100644
> --- a/arch/arm64/crypto/aes-ce-cipher.c
> +++ b/arch/arm64/crypto/aes-ce-cipher.c
> @@ -253,6 +253,8 @@ static struct crypto_alg aes_alg = {
>
> static int __init aes_mod_init(void)
> {
> + if (!system_supports_fpsimd())
> + return -ENODEV;
> return crypto_register_alg(&aes_alg);
> }
>
> diff --git a/arch/arm64/crypto/ghash-ce-glue.c b/arch/arm64/crypto/ghash-ce-glue.c
> index 833ec1e..2bc518d 100644
> --- a/arch/arm64/crypto/ghash-ce-glue.c
> +++ b/arch/arm64/crypto/ghash-ce-glue.c
> @@ -144,6 +144,8 @@ static struct shash_alg ghash_alg = {
>
> static int __init ghash_ce_mod_init(void)
> {
> + if (!system_supports_fpsimd())
> + return -ENODEV;
> return crypto_register_shash(&ghash_alg);
> }
>
> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> index aefda98..9f3427a 100644
> --- a/arch/arm64/crypto/sha1-ce-glue.c
> +++ b/arch/arm64/crypto/sha1-ce-glue.c
> @@ -102,6 +102,8 @@ static struct shash_alg alg = {
>
> static int __init sha1_ce_mod_init(void)
> {
> + if (!system_supports_fpsimd())
> + return -ENODEV;
> return crypto_register_shash(&alg);
> }
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index ae5e994..63d739c 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -38,8 +38,9 @@
> #define ARM64_HAS_32BIT_EL0 13
> #define ARM64_HYP_OFFSET_LOW 14
> #define ARM64_MISMATCHED_CACHE_LINE_SIZE 15
> +#define ARM64_HAS_NO_FPSIMD 16
>
> -#define ARM64_NCAPS 16
> +#define ARM64_NCAPS 17
>
> #ifndef __ASSEMBLY__
>
> @@ -236,6 +237,11 @@ static inline bool system_supports_mixed_endian_el0(void)
> return id_aa64mmfr0_mixed_endian_el0(read_system_reg(SYS_ID_AA64MMFR0_EL1));
> }
>
> +static inline bool system_supports_fpsimd(void)
> +{
> + return !cpus_have_const_cap(ARM64_HAS_NO_FPSIMD);
> +}
> +
> #endif /* __ASSEMBLY__ */
>
> #endif
> diff --git a/arch/arm64/include/asm/neon.h b/arch/arm64/include/asm/neon.h
> index 13ce4cc..ad4cdc9 100644
> --- a/arch/arm64/include/asm/neon.h
> +++ b/arch/arm64/include/asm/neon.h
> @@ -9,8 +9,9 @@
> */
>
> #include <linux/types.h>
> +#include <asm/fpsimd.h>
>
> -#define cpu_has_neon() (1)
> +#define cpu_has_neon() system_supports_fpsimd()
>
> #define kernel_neon_begin() kernel_neon_begin_partial(32)
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d577f26..8f22544 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -744,6 +744,14 @@ static bool hyp_offset_low(const struct arm64_cpu_capabilities *entry,
> return idmap_addr > GENMASK(VA_BITS - 2, 0) && !is_kernel_in_hyp_mode();
> }
>
> +static bool has_no_fpsimd(const struct arm64_cpu_capabilities *entry, int __unused)
> +{
> + u64 pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1);
> +
> + return cpuid_feature_extract_signed_field(pfr0,
> + ID_AA64PFR0_FP_SHIFT) < 0;
> +}
> +
> static const struct arm64_cpu_capabilities arm64_features[] = {
> {
> .desc = "GIC system register CPU interface",
> @@ -827,6 +835,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> .def_scope = SCOPE_SYSTEM,
> .matches = hyp_offset_low,
> },
> + {
> + /* FP/SIMD is not implemented */
> + .capability = ARM64_HAS_NO_FPSIMD,
> + .def_scope = SCOPE_SYSTEM,
> + .min_field_value = 0,
> + .matches = has_no_fpsimd,
> + },
> {},
> };
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 975b274..80da5aa 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -127,6 +127,8 @@ void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
>
> void fpsimd_thread_switch(struct task_struct *next)
> {
> + if (!system_supports_fpsimd())
> + return;
> /*
> * Save the current FPSIMD state to memory, but only if whatever is in
> * the registers is in fact the most recent userland FPSIMD state of
> @@ -157,6 +159,8 @@ void fpsimd_thread_switch(struct task_struct *next)
>
> void fpsimd_flush_thread(void)
> {
> + if (!system_supports_fpsimd())
> + return;
> memset(¤t->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
> fpsimd_flush_task_state(current);
> set_thread_flag(TIF_FOREIGN_FPSTATE);
> @@ -168,6 +172,8 @@ void fpsimd_flush_thread(void)
> */
> void fpsimd_preserve_current_state(void)
> {
> + if (!system_supports_fpsimd())
> + return;
> preempt_disable();
> if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
> fpsimd_save_state(¤t->thread.fpsimd_state);
> @@ -181,6 +187,8 @@ void fpsimd_preserve_current_state(void)
> */
> void fpsimd_restore_current_state(void)
> {
> + if (!system_supports_fpsimd())
> + return;
> preempt_disable();
> if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
> struct fpsimd_state *st = ¤t->thread.fpsimd_state;
> @@ -199,6 +207,8 @@ void fpsimd_restore_current_state(void)
> */
> void fpsimd_update_current_state(struct fpsimd_state *state)
> {
> + if (!system_supports_fpsimd())
> + return;
> preempt_disable();
> fpsimd_load_state(state);
> if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
> @@ -228,6 +238,8 @@ static DEFINE_PER_CPU(struct fpsimd_partial_state, softirq_fpsimdstate);
> */
> void kernel_neon_begin_partial(u32 num_regs)
> {
> + if (WARN_ON(!system_supports_fpsimd()))
> + return;
> if (in_interrupt()) {
> struct fpsimd_partial_state *s = this_cpu_ptr(
> in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
> @@ -252,6 +264,8 @@ EXPORT_SYMBOL(kernel_neon_begin_partial);
>
> void kernel_neon_end(void)
> {
> + if (!system_supports_fpsimd())
> + return;
> if (in_interrupt()) {
> struct fpsimd_partial_state *s = this_cpu_ptr(
> in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index fa96fe2..39e055a 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -57,6 +57,16 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
> return 1;
> }
>
> +/*
> + * Guest access to FP/ASIMD registers are routed to this handler only
> + * when the system doesn't support FP/ASIMD.
> + */
> +static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> + kvm_inject_undefined(vcpu);
> + return 1;
> +}
> +
> /**
> * kvm_handle_wfx - handle a wait-for-interrupts or wait-for-event
> * instruction executed by a guest
> @@ -144,6 +154,7 @@ static exit_handle_fn arm_exit_handlers[] = {
> [ESR_ELx_EC_BREAKPT_LOW]= kvm_handle_guest_debug,
> [ESR_ELx_EC_BKPT32] = kvm_handle_guest_debug,
> [ESR_ELx_EC_BRK64] = kvm_handle_guest_debug,
> + [ESR_ELx_EC_FP_ASIMD] = handle_no_fpsimd,
> };
>
> static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index f6d9694..16167d7 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -117,9 +117,16 @@ el1_trap:
> * x2: ESR_EC
> */
>
> - /* Guest accessed VFP/SIMD registers, save host, restore Guest */
> + /*
> + * We trap the first access to the FP/SIMD to save the host context
> + * and restore the guest context lazily.
> + * If FP/SIMD is not implemented, handle the trap and inject an
> + * undefined instruction exception to the guest.
> + */
> +alternative_if_not ARM64_HAS_NO_FPSIMD
> cmp x2, #ESR_ELx_EC_FP_ASIMD
> b.eq __fpsimd_guest_restore
> +alternative_else_nop_endif
>
> mrs x0, tpidr_el2
> mov x1, #ARM_EXCEPTION_TRAP
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index ae7855f..f2d0c4f 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -18,6 +18,7 @@
> #include <linux/types.h>
> #include <asm/kvm_asm.h>
> #include <asm/kvm_hyp.h>
> +#include <asm/fpsimd.h>
>
> static bool __hyp_text __fpsimd_enabled_nvhe(void)
> {
> @@ -73,9 +74,11 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
> * traps are only taken to EL2 if the operation would not otherwise
> * trap to EL1. Therefore, always make sure that for 32-bit guests,
> * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
> + * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
> + * it will cause an exception.
> */
> val = vcpu->arch.hcr_el2;
> - if (!(val & HCR_RW)) {
> + if (!(val & HCR_RW) && system_supports_fpsimd()) {
> write_sysreg(1 << 30, fpexc32_el2);
> isb();
> }
> --
> 2.7.4
>
^ permalink raw reply
* Disabling an interrupt in the handler locks the system up
From: Thomas Gleixner @ 2016-10-25 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <580F647B.5000202@free.fr>
On Tue, 25 Oct 2016, Mason wrote:
> Is the irq_mask() call-back exposed via some module-visible API?
No. And there is no reason to do so.
Thanks,
tglx
^ permalink raw reply
* Disabling an interrupt in the handler locks the system up
From: Mason @ 2016-10-25 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <534c4588-f220-25a3-e7aa-84484f348bd1@arm.com>
On 25/10/2016 12:45, Marc Zyngier wrote:
> On 25/10/16 09:36, Mason wrote:
>> On 25/10/2016 10:29, Sebastian Frias wrote:
>>
>>> On 10/24/2016 06:55 PM, Thomas Gleixner wrote:
>>>
>>>> On Mon, 24 Oct 2016, Mason wrote:
>>>>
>>>>> For the record, setting the IRQ_DISABLE_UNLAZY flag for this device
>>>>> makes the system lock-up disappear.
>>>>
>>>> The way how lazy irq disabling works is:
>>>>
>>>> 1) Interrupt is marked disabled in software, but the hardware is not masked
>>>>
>>>> 2) If the interrupt fires befor the interrupt is reenabled, then it's
>>>> masked at the hardware level in the low level interrupt flow handler.
>>>
>>> Would you mind explaining what is the intention behind?
>>> Because it does not seem obvious why there isn't a direct map between
>>> "disable_irq*()" and "mask_irq()"
>>
>> I had a similar, but slightly different question:
>>
>> What is the difference between struct irq_chip's
>>
>> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL)
>> * @irq_disable: disable the interrupt
>> * @irq_mask: mask an interrupt source
>
> One important difference between disable and mask is that disable is
> perfectly allowed not to care about pending signals, whereas mask must
> preserve an interrupt becoming pending whilst masked.
(For my information)
Is it correct to say that "mask" is supposed to defer any interrupt
until sometime later; while "disable" will simply discard incoming
interrupts, losing them forever.
Is the irq_mask() call-back exposed via some module-visible API?
include/linux/interrupt.h documents mostly enable/disable variants.
extern void disable_irq_nosync(unsigned int irq);
extern bool disable_hardirq(unsigned int irq);
extern void disable_irq(unsigned int irq);
extern void disable_percpu_irq(unsigned int irq);
extern void enable_irq(unsigned int irq);
extern void enable_percpu_irq(unsigned int irq, unsigned int type);
extern bool irq_percpu_is_enabled(unsigned int irq);
extern void irq_wake_thread(unsigned int irq, void *dev_id);
Regards.
^ permalink raw reply
* [PATCH 2/2] arm64: Support systems without FP/ASIMD
From: Suzuki K Poulose @ 2016-10-25 13:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477403433-2279-1-git-send-email-suzuki.poulose@arm.com>
The arm64 kernel assumes that FP/ASIMD units are always present
and accesses the FP/ASIMD specific registers unconditionally. This
could cause problems when they are absent. This patch adds the
support for kernel handling systems without FP/ASIMD by skipping the
register access within the kernel. For kvm, we trap the accesses
to FP/ASIMD and inject an undefined instruction exception to the VM.
The callers of the exported kernel_neon_begin_parital() should
make sure that the FP/ASIMD is supported.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
---
arch/arm64/crypto/aes-ce-ccm-glue.c | 2 +-
arch/arm64/crypto/aes-ce-cipher.c | 2 ++
arch/arm64/crypto/ghash-ce-glue.c | 2 ++
arch/arm64/crypto/sha1-ce-glue.c | 2 ++
arch/arm64/include/asm/cpufeature.h | 8 +++++++-
arch/arm64/include/asm/neon.h | 3 ++-
arch/arm64/kernel/cpufeature.c | 15 +++++++++++++++
arch/arm64/kernel/fpsimd.c | 14 ++++++++++++++
arch/arm64/kvm/handle_exit.c | 11 +++++++++++
arch/arm64/kvm/hyp/hyp-entry.S | 9 ++++++++-
arch/arm64/kvm/hyp/switch.c | 5 ++++-
11 files changed, 68 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index f4bf2f2..d001b4e 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
@@ -296,7 +296,7 @@ static struct aead_alg ccm_aes_alg = {
static int __init aes_mod_init(void)
{
- if (!(elf_hwcap & HWCAP_AES))
+ if (!(elf_hwcap & HWCAP_AES) || !system_supports_fpsimd())
return -ENODEV;
return crypto_register_aead(&ccm_aes_alg);
}
diff --git a/arch/arm64/crypto/aes-ce-cipher.c b/arch/arm64/crypto/aes-ce-cipher.c
index f7bd9bf..1a43be2 100644
--- a/arch/arm64/crypto/aes-ce-cipher.c
+++ b/arch/arm64/crypto/aes-ce-cipher.c
@@ -253,6 +253,8 @@ static struct crypto_alg aes_alg = {
static int __init aes_mod_init(void)
{
+ if (!system_supports_fpsimd())
+ return -ENODEV;
return crypto_register_alg(&aes_alg);
}
diff --git a/arch/arm64/crypto/ghash-ce-glue.c b/arch/arm64/crypto/ghash-ce-glue.c
index 833ec1e..2bc518d 100644
--- a/arch/arm64/crypto/ghash-ce-glue.c
+++ b/arch/arm64/crypto/ghash-ce-glue.c
@@ -144,6 +144,8 @@ static struct shash_alg ghash_alg = {
static int __init ghash_ce_mod_init(void)
{
+ if (!system_supports_fpsimd())
+ return -ENODEV;
return crypto_register_shash(&ghash_alg);
}
diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
index aefda98..9f3427a 100644
--- a/arch/arm64/crypto/sha1-ce-glue.c
+++ b/arch/arm64/crypto/sha1-ce-glue.c
@@ -102,6 +102,8 @@ static struct shash_alg alg = {
static int __init sha1_ce_mod_init(void)
{
+ if (!system_supports_fpsimd())
+ return -ENODEV;
return crypto_register_shash(&alg);
}
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index ae5e994..63d739c 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -38,8 +38,9 @@
#define ARM64_HAS_32BIT_EL0 13
#define ARM64_HYP_OFFSET_LOW 14
#define ARM64_MISMATCHED_CACHE_LINE_SIZE 15
+#define ARM64_HAS_NO_FPSIMD 16
-#define ARM64_NCAPS 16
+#define ARM64_NCAPS 17
#ifndef __ASSEMBLY__
@@ -236,6 +237,11 @@ static inline bool system_supports_mixed_endian_el0(void)
return id_aa64mmfr0_mixed_endian_el0(read_system_reg(SYS_ID_AA64MMFR0_EL1));
}
+static inline bool system_supports_fpsimd(void)
+{
+ return !cpus_have_const_cap(ARM64_HAS_NO_FPSIMD);
+}
+
#endif /* __ASSEMBLY__ */
#endif
diff --git a/arch/arm64/include/asm/neon.h b/arch/arm64/include/asm/neon.h
index 13ce4cc..ad4cdc9 100644
--- a/arch/arm64/include/asm/neon.h
+++ b/arch/arm64/include/asm/neon.h
@@ -9,8 +9,9 @@
*/
#include <linux/types.h>
+#include <asm/fpsimd.h>
-#define cpu_has_neon() (1)
+#define cpu_has_neon() system_supports_fpsimd()
#define kernel_neon_begin() kernel_neon_begin_partial(32)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d577f26..8f22544 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -744,6 +744,14 @@ static bool hyp_offset_low(const struct arm64_cpu_capabilities *entry,
return idmap_addr > GENMASK(VA_BITS - 2, 0) && !is_kernel_in_hyp_mode();
}
+static bool has_no_fpsimd(const struct arm64_cpu_capabilities *entry, int __unused)
+{
+ u64 pfr0 = read_system_reg(SYS_ID_AA64PFR0_EL1);
+
+ return cpuid_feature_extract_signed_field(pfr0,
+ ID_AA64PFR0_FP_SHIFT) < 0;
+}
+
static const struct arm64_cpu_capabilities arm64_features[] = {
{
.desc = "GIC system register CPU interface",
@@ -827,6 +835,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
.def_scope = SCOPE_SYSTEM,
.matches = hyp_offset_low,
},
+ {
+ /* FP/SIMD is not implemented */
+ .capability = ARM64_HAS_NO_FPSIMD,
+ .def_scope = SCOPE_SYSTEM,
+ .min_field_value = 0,
+ .matches = has_no_fpsimd,
+ },
{},
};
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 975b274..80da5aa 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -127,6 +127,8 @@ void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
void fpsimd_thread_switch(struct task_struct *next)
{
+ if (!system_supports_fpsimd())
+ return;
/*
* Save the current FPSIMD state to memory, but only if whatever is in
* the registers is in fact the most recent userland FPSIMD state of
@@ -157,6 +159,8 @@ void fpsimd_thread_switch(struct task_struct *next)
void fpsimd_flush_thread(void)
{
+ if (!system_supports_fpsimd())
+ return;
memset(¤t->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
fpsimd_flush_task_state(current);
set_thread_flag(TIF_FOREIGN_FPSTATE);
@@ -168,6 +172,8 @@ void fpsimd_flush_thread(void)
*/
void fpsimd_preserve_current_state(void)
{
+ if (!system_supports_fpsimd())
+ return;
preempt_disable();
if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
fpsimd_save_state(¤t->thread.fpsimd_state);
@@ -181,6 +187,8 @@ void fpsimd_preserve_current_state(void)
*/
void fpsimd_restore_current_state(void)
{
+ if (!system_supports_fpsimd())
+ return;
preempt_disable();
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
struct fpsimd_state *st = ¤t->thread.fpsimd_state;
@@ -199,6 +207,8 @@ void fpsimd_restore_current_state(void)
*/
void fpsimd_update_current_state(struct fpsimd_state *state)
{
+ if (!system_supports_fpsimd())
+ return;
preempt_disable();
fpsimd_load_state(state);
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
@@ -228,6 +238,8 @@ static DEFINE_PER_CPU(struct fpsimd_partial_state, softirq_fpsimdstate);
*/
void kernel_neon_begin_partial(u32 num_regs)
{
+ if (WARN_ON(!system_supports_fpsimd()))
+ return;
if (in_interrupt()) {
struct fpsimd_partial_state *s = this_cpu_ptr(
in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
@@ -252,6 +264,8 @@ EXPORT_SYMBOL(kernel_neon_begin_partial);
void kernel_neon_end(void)
{
+ if (!system_supports_fpsimd())
+ return;
if (in_interrupt()) {
struct fpsimd_partial_state *s = this_cpu_ptr(
in_irq() ? &hardirq_fpsimdstate : &softirq_fpsimdstate);
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index fa96fe2..39e055a 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -57,6 +57,16 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
return 1;
}
+/*
+ * Guest access to FP/ASIMD registers are routed to this handler only
+ * when the system doesn't support FP/ASIMD.
+ */
+static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+ kvm_inject_undefined(vcpu);
+ return 1;
+}
+
/**
* kvm_handle_wfx - handle a wait-for-interrupts or wait-for-event
* instruction executed by a guest
@@ -144,6 +154,7 @@ static exit_handle_fn arm_exit_handlers[] = {
[ESR_ELx_EC_BREAKPT_LOW]= kvm_handle_guest_debug,
[ESR_ELx_EC_BKPT32] = kvm_handle_guest_debug,
[ESR_ELx_EC_BRK64] = kvm_handle_guest_debug,
+ [ESR_ELx_EC_FP_ASIMD] = handle_no_fpsimd,
};
static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index f6d9694..16167d7 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -117,9 +117,16 @@ el1_trap:
* x2: ESR_EC
*/
- /* Guest accessed VFP/SIMD registers, save host, restore Guest */
+ /*
+ * We trap the first access to the FP/SIMD to save the host context
+ * and restore the guest context lazily.
+ * If FP/SIMD is not implemented, handle the trap and inject an
+ * undefined instruction exception to the guest.
+ */
+alternative_if_not ARM64_HAS_NO_FPSIMD
cmp x2, #ESR_ELx_EC_FP_ASIMD
b.eq __fpsimd_guest_restore
+alternative_else_nop_endif
mrs x0, tpidr_el2
mov x1, #ARM_EXCEPTION_TRAP
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index ae7855f..f2d0c4f 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -18,6 +18,7 @@
#include <linux/types.h>
#include <asm/kvm_asm.h>
#include <asm/kvm_hyp.h>
+#include <asm/fpsimd.h>
static bool __hyp_text __fpsimd_enabled_nvhe(void)
{
@@ -73,9 +74,11 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
* traps are only taken to EL2 if the operation would not otherwise
* trap to EL1. Therefore, always make sure that for 32-bit guests,
* we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
+ * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
+ * it will cause an exception.
*/
val = vcpu->arch.hcr_el2;
- if (!(val & HCR_RW)) {
+ if (!(val & HCR_RW) && system_supports_fpsimd()) {
write_sysreg(1 << 30, fpexc32_el2);
isb();
}
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] arm64: Add hypervisor safe helper for checking constant capabilities
From: Suzuki K Poulose @ 2016-10-25 13:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477403433-2279-1-git-send-email-suzuki.poulose@arm.com>
The hypervisor may not have full access to the kernel data structures
and hence cannot safely use cpus_have_cap() helper for checking the
system capability. Add a safe helper for hypervisors to check a constant
system capability, which *doesn't* fall back to checking the bitmap
maintained by the kernel.
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 758d74f..ae5e994 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -9,8 +9,6 @@
#ifndef __ASM_CPUFEATURE_H
#define __ASM_CPUFEATURE_H
-#include <linux/jump_label.h>
-
#include <asm/hwcap.h>
#include <asm/sysreg.h>
@@ -45,6 +43,8 @@
#ifndef __ASSEMBLY__
+#include <linux/bug.h>
+#include <linux/jump_label.h>
#include <linux/kernel.h>
/* CPU feature register tracking */
@@ -122,6 +122,16 @@ static inline bool cpu_have_feature(unsigned int num)
return elf_hwcap & (1UL << num);
}
+/* System capability check for constant caps */
+static inline bool cpus_have_const_cap(int num)
+{
+ if (__builtin_constant_p(num))
+ return static_branch_unlikely(&cpu_hwcap_keys[num]);
+ BUILD_BUG();
+ /* unreachable */
+ return false;
+}
+
static inline bool cpus_have_cap(unsigned int num)
{
if (num >= ARM64_NCAPS)
@@ -218,7 +228,7 @@ static inline bool cpu_supports_mixed_endian_el0(void)
static inline bool system_supports_32bit_el0(void)
{
- return cpus_have_cap(ARM64_HAS_32BIT_EL0);
+ return cpus_have_const_cap(ARM64_HAS_32BIT_EL0);
}
static inline bool system_supports_mixed_endian_el0(void)
--
2.7.4
^ permalink raw reply related
* [PATCH 0/2] arm64: Support systems without FP/ASIMD
From: Suzuki K Poulose @ 2016-10-25 13:50 UTC (permalink / raw)
To: linux-arm-kernel
This series adds supports to the kernel and KVM hyp to handle
systems without FP/ASIMD properly. At the moment the kernel
doesn't check if the FP unit is available before accessing
the registers (e.g during context switch). Also for KVM,
we trap the FP/ASIMD accesses and handle it by injecting an
undefined instruction into the VM on systems without FP.
Tested on a FVP_Base-AEM-v8A model by disabling VFP on at
least one CPU ( -C clusterX.cpuY.vfp-present=0 ).
Suzuki K Poulose (2):
arm64: Add hypervisor safe helper for checking constant capabilities
arm64: Support systems without FP/ASIMD
arch/arm64/crypto/aes-ce-ccm-glue.c | 2 +-
arch/arm64/crypto/aes-ce-cipher.c | 2 ++
arch/arm64/crypto/ghash-ce-glue.c | 2 ++
arch/arm64/crypto/sha1-ce-glue.c | 2 ++
arch/arm64/include/asm/cpufeature.h | 24 ++++++++++++++++++++----
arch/arm64/include/asm/neon.h | 3 ++-
arch/arm64/kernel/cpufeature.c | 15 +++++++++++++++
arch/arm64/kernel/fpsimd.c | 14 ++++++++++++++
arch/arm64/kvm/handle_exit.c | 11 +++++++++++
arch/arm64/kvm/hyp/hyp-entry.S | 9 ++++++++-
arch/arm64/kvm/hyp/switch.c | 5 ++++-
11 files changed, 81 insertions(+), 8 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH] ARM: sti: stih410-clocks: Add PROC_STFE as a critical clock
From: Peter Griffin @ 2016-10-25 13:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161025100141.GH8574@dell>
Hi Lee,
On Tue, 25 Oct 2016, Lee Jones wrote:
> On Tue, 25 Oct 2016, Peter Griffin wrote:
>
> > Hi Lee,
> >
> > On Tue, 25 Oct 2016, Lee Jones wrote:
> >
> > > On Mon, 24 Oct 2016, Peter Griffin wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > On Mon, 24 Oct 2016, Lee Jones wrote:
> > > > > On Tue, 18 Oct 2016, Peter Griffin wrote:
> > > > >
> > > > > > Once the ST frontend demux HW IP has been enabled, the clock can't
> > > > > > be disabled otherwise the system will hang and the board will
> > > > > > be unserviceable.
> > > > > >
> > > > > > To allow balanced clock enable/disable calls in the driver we use
> > > > > > the critical clock infrastructure to take an extra reference on the
> > > > > > clock so the clock will never actually be disabled.
> > > > >
> > > > > This is an abuse of the critical-clocks framework, and is exactly the
> > > > > type of hack I promised the clk guys I'd try to prevent.
> > > >
> > > > I expect the best way to do this would be to write some documentation on the
> > > > clock-critical DT binding and/or CRITICAL_CLK flag. The only documentation I can
> > > > find currently is with the initial patch series [1] and the comment in
> > > > clk-provider.h of
> > > >
> > > > #define CLK_IS_CRITICAL BIT(11) /* do not gate, ever */
> > > >
> > > > Or the patch decription
> > > >
> > > > "Critical clocks are those which must not be gated, else undefined
> > > > or catastrophic failure would occur. Here we have chosen to
> > > > ensure the prepare/enable counts are correctly incremented, so as
> > > > not to confuse users with enabled clocks with no visible users."
> > > >
> > > > Which is the functionality I want for this clock.
> > >
> > > No, that's not the functionality you want.
> >
> > Yes it is :)
>
> No, it's the functionally that is most convenient.
Nope, the solution you proposed already exists upstream so changing it is not
convienient, it is IMO the correct thing to do and the current upstream solution
is the hack.
Allowing the common CCF to disable this clock during clk_disable_unused is
wrong, and can lead to catastrophic failure which requires a reboot.
IMO Linux needs to be resilient to whatever state the hardware could be left in
by previous software. It can't assume it will be OK to disable this clock, we
*know* there is a HW bug, we *know* that disabling the clock can brick the
board.
So I stick with my initial assertion, if this clock is enabled when Linux is
booted it *should not* be disabled.
>
> > > You want for the clock not
> > > to be RE-gated (big difference). Currently, the STFE clock will never
> > > be gated, even when a) it's not used and b) can actually be disabled.
> > > You're needlessly wasting power here.
> >
> > IMO it is *never* safe for Linux to gate this clock, as you have no idea
> > on the state of the hardware from the primary and secondary bootloaders.
>
> You can say that with any of the clocks on this platform.
This is a odd thing to say. I'm not aware of any other clocks which
will render the platform unusable that aren't already listed as critical for the
STi platform.
>
> > If the clock is enabled when Linux boots, the Linux clock framework *needs*
> > to assume the hardware may have been used in previous boot stages, and it should
> > not attempt to disable the clock.
>
> None of the boot loaders we use do this.
But the Linux kernel isn't just used by us. It is not uncommon for STB
bootloaders to get information from the frontend as part of the boot process.
>
> I have never seen the STFE clock crash a platform.
I have, it leads to the same behaviour as turning off any of the other critical
clocks, the SoC is dead and the board needs to be rebooted.
>
> > > Also, in your use-case there is a visible user, and the prepare/enable
> > > counts will be correct.
> >
> > Your correct there is a visible user, but this is the same as CLK_EXT2F_A9 where
> > there are multiple users of the clock (SPI, I2C, UART etc).
>
> Right, but their clocks can not be turned off, ever. So all the boxes
> are ticked for criticalness. Not the case with your clock.
This clock also should never be turned off by Linux..ever. It is unsafe to do so.
Incidentally this isn't the only platform upstream where the requirement for a
clocks criticalness is dependent on whether it was enabled during boot. Take a
look in bcm/clk-bcm2835.c
/* If the clock wasn't actually enabled at boot, it's not
* critical.
*/
if (!(cprman_read(cprman, data->ctl_reg) & CM_ENABLE))
init.flags &= ~CLK_IS_CRITICAL;
>
> > > > > If this, or
> > > > > any other IP has some quirks (i.e. once enabled, if this clock is
> > > > > subsequently disabled it will have a catastrophic effect on the
> > > > > platform), then they should be worked around in the driver.
> > > > >
> > > > > The correct thing to do here is craft a clk-keep-on flag and ensure it
> > > > > is set to true for the effected platform(s)' platform data.
> > > >
> > > > I'm always wary of creating a driver specific flag, especially when its
> > > > purpose is to do the same thing as an existing mechanism provided by the
> > > > subsystem of not gating the clock.
> > >
> > > Using existing sub-system supplied mechanisms in the way they were not
> > > intended is sub-optimal (read "hacky").
> >
> > I think the scope of this flag has been defined in a very narrow way, which is
> > making you want to suggest lots of hacks in the client driver.
>
> The scope was narrowed intentionally, buy the maintainers. I am
> merely reflecting their views.
What is your recollection of their views? LIke I said before this should be
documented somewhere.
> If you really wish to contend these,
> you should take it up with them.
>
> > > > I can see a couple of problems with what you propose:
> > > >
> > > > 1) You have to put the clk-keep-on flag in every driver which consumes the
> > > > clock. IMO it is much better to have this knowledge in the SoC's
> > > > clock driver so every consumer of the clock automatically benefits.
> > >
> > > That would also be fine(ish). The issue is that this problem is
> > > board specific, so the platform clock driver would have to contain
> > > board level knowledge.
> >
> > It is not board specific. It is a SoC HW bug, so by definition present on all
> > boards which have this SoC.
>
> Okay SoC specific. Is there a SoC specific clock driver?
STi platform clocks are mainly described in DT in stih407/10-clock.dsti, which is SoC
specific, and binds with code in drivers/clk/st. So the marking of a clock as
critical is done on a SoC basis which is what I want.
A quick grep of CLK_IGNORE_UNUSED or CLK_IS_CRITICAL, will show you where the
equivalent code for other platforms is located.
>
> > > Also, if you were to implement this, it would
> > > too mess up reference counting in the core.
>
> This still stands.
I don't understand what you mean here. The point of using CCF and the critical
flag is so the reference counting of the clock is held at 1.
>
> > > > 2) You don't benefit from the CLK_IS_CRITICAL reference counting logic in
> > > > clk.c. So then each driver has to also work around that to get sensible reference
> > > > counts. e.g.
> > > >
> > > > if (!__clk_is_enabled(clk) && pdata->clk-keep-on)
> > > > clk_enable(clk)
> > > >
> > > > Which seems to me to be fighting against the subsystem. Given that the only use of
> > > > _clk_is_enabled() outside drivers/clk is in an old arch/arm/mach-omap2/pm24xx.c
> > > > driver I suspect its use is frowned upon, and it shouldn't really be an EXPORTED_SYMBOL.
> > >
> > > In this instance, since the STFE clock is only used by this IP, I
> > > would choose to handle it in the driver.
> >
> > There are other IPs within this IP block for which upstream drivers don't yet exist.
>
> If they are many and various, we can discuss alternative solutions.
>
> What are they?
You have hardware for Merged Input Blocks, SWTS, TSDMA, Cable Card Stream
Convertor and Tag Counter which is currently unsupported upstream.
>
> > > This can be done using a
> > > single flag stored in pdata which should be fetched using
> > > of_match_device(). This way there is no need for any more API abuse;
> > > either by incorrectly identifying the STFE clock as critical OR
> > > invoking any internal __clk_*() calls.
> > >
> > > Enable the clock once in .probe(), which you already do.
> >
> > But these drivers are by default built as modules, so when you rmmod, and insmod the
> > driver you now have an ever increasing clock reference count. This is the
> > problem I was describing in the previous email, and why what you propose is a
> > bad idea.
>
> Then the variable would have to be exported.
I don't understand why if you had a choice you would choose to put the logic
there over the clock subsystem.
The reasoning is the same as with the I2C & SPI drivers. You don't want to have
to put a platform specific clocking knowledge into each driver, when it
could live centrally in the clock subsystem for the platform. You also don't want to
have to change the driver when a different SoC with the same IP has a different
clock tree.
Not only that CCF *has* to have the knowledge internal to the clock susbsytem so
that it doesn't disable the clock during the clk_disable_unused phase on boot.
>
> > > Then, whenever you do any power saving do:
> > >
> > > suspend()
> > > {
> > > if (!ddata->enable_clk_once)
> > > clk_disable(clk);
> > > }
> > >
> > > resume()
> > > {
> > > if (!ddata->enable_clk_once)
> > > clk_enable(clk);
> > > }
> > >
> > > However, looking at your driver, I think this point might even be
> > > moot, since you don't have any power saving. The only time you
> > > disable the clock is in the error path. Just replace that with a
> > > comment about the platform's unfortunate errata.
> >
> > This is exactly what I want to avoid doing. The driver already has these
> > hacks as it was waiting for the critical clock patches to land so I could remove
> > them and fix the problem properly.
> >
> > Much like you did with the I2C and SPI drivers, where you removed similar hacks
> > and added the clock to the critical clock list.
>
> Right, see above.
>
> > > > [1] https://lkml.org/lkml/2016/1/18/272
> > >
> > > I'm glad you mentioned this. Let's take a look:
> > >
> > > > Some platforms contain clocks which if gated, will cause undefined or
> > > > catastrophic behaviours. As such they are not to be turned off, ever.
> > >
> > > Not the case here.
> > >
> > > This clock *can* be gated and can be turned off *sometimes*.
> >
> > See above, if the clock is on when Linux boots it can never be assumed that it
> > is safe to disable it.
>
> See above.
>
> > > > Many of these such clocks do not have devices, thus device drivers
> > > > where clocks may be enabled and references taken to ensure they stay
> > > > enabled do not exist. Therefore, we must handle these such cases in
> > > > the core.
> > >
> > > This clock *does* have a driver and correct references *can* be
> > > taken.
> >
> > As above this is the same for CLK_EXT2F_A9, which has multiple users in the
> > kernel.
> >
> > The point is we don't wish to have the knowledge in the individual drivers that
> > this clock is critical and can destroy the system if it is disabled.
This point stlll stands.
> >
> > >
> > > [...]
> > >
> > > All I'm saying is, and it's the same thing I've said many times; these
> > > types of issues do not exhibit the same set of symptoms as a critical
> > > clock by definition. Critical clocks are those which references can
> > > not be taken by any other means.
> >
> > That is not how you are using it currently. See CLK_EXT2F_A9 and
> > CLK_TX_ICN_DMU.
So does this one.
> >
> > > Think of the critical clock
> > > framework as a mechanism to circumvent the requirement of writing a
> > > special driver which would *only* handle clocks i.e. an interconnect
> > > driver in the ST case.
> > >
> >
> > You didn't need a new flag for that. CLK_IGNORE_UNUSED already allowed you to not
> > write a specical driver which only handled for example an interconnect clock and
> > would ensure it wouldn't be gated by the disable_unused logic.
> >
> > IMO the point of the critical clock stuff, and what CLK_IGNORE_UNUSED *did not* allow
> > you to do is for situations like CLK_EXT2F_A9, CLK_TX_ICN_DMU and PROC_STFE where users
> > *do* exist in the kernel, but the SoC clock driver has *special* knowledge of the
> > clock tree on the SoC and knows that despite what the drivers are doing with
> > their enable/disable calls the clock should never be disabled.
> >
> > In fact out of what we currently mark as clock-critical on STi platform most
> > of the clocks could have been handled perfectly fine with the CLK_IGNORE_UNUSED
> > flag (apart from the fact that there is no way to set the flag from DT). It is
> > only CLK_EXT2F_A9 and CLK_TX_ICN_DMU which *do* have kernel users where the
> > extra functionality of critical clocks is required (we need the additional
> > reference taken by the clk framework to avoid the kernel drivers from disabling
> > the clock).
And most importantly this one.
>
> Probably best to take your special use-case up with the Clock
> Maintainers. As the author of the critical-clocks patch-set, I would
> say that your use-case does not tick all the boxes for use of the
> critical-clock mechanism, but at the end of the day, it really isn't
> my train-set.
>
I don't think there is anything special about it. If when Linux
boots the clock is enabled you shouldn't disable it, just like the other
critical clocks.
regards,
Peter.
^ 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