* [xlnx:xlnx_rebase_v4.14 441/937] drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1854:18: error: 'struct axienet_local' has no member named 'tx_irq'; did you mean 'rtc_irq'?
From: kbuild test robot @ 2018-05-21 15:23 UTC (permalink / raw)
To: linux-arm-kernel
tree: https://github.com/Xilinx/linux-xlnx xlnx_rebase_v4.14
head: 6588700bdabc9bb1a0156770ed2ac41da7d823dd
commit: 27eb94d2931872784a5e740adbddd1b823dc7b95 [441/937] drivers: net: ethernet: TSN: Kconfig and Makefile
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 27eb94d2931872784a5e740adbddd1b823dc7b95
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/net//ethernet/xilinx/xilinx_axienet_main.c: In function 'axienet_start_xmit':
drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1069:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
const struct ethhdr *eth;
^~~~~
drivers/net//ethernet/xilinx/xilinx_axienet_main.c: In function 'axienet_poll_controller':
>> drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1854:18: error: 'struct axienet_local' has no member named 'tx_irq'; did you mean 'rtc_irq'?
disable_irq(lp->tx_irq);
^~~~~~
rtc_irq
>> drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1855:18: error: 'struct axienet_local' has no member named 'rx_irq'; did you mean 'rtc_irq'?
disable_irq(lp->rx_irq);
^~~~~~
rtc_irq
drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1856:21: error: 'struct axienet_local' has no member named 'tx_irq'; did you mean 'rtc_irq'?
axienet_rx_irq(lp->tx_irq, ndev);
^~~~~~
rtc_irq
drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1857:21: error: 'struct axienet_local' has no member named 'rx_irq'; did you mean 'rtc_irq'?
axienet_tx_irq(lp->rx_irq, ndev);
^~~~~~
rtc_irq
drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1858:17: error: 'struct axienet_local' has no member named 'tx_irq'; did you mean 'rtc_irq'?
enable_irq(lp->tx_irq);
^~~~~~
rtc_irq
drivers/net//ethernet/xilinx/xilinx_axienet_main.c:1859:17: error: 'struct axienet_local' has no member named 'rx_irq'; did you mean 'rtc_irq'?
enable_irq(lp->rx_irq);
^~~~~~
rtc_irq
vim +1854 drivers/net//ethernet/xilinx/xilinx_axienet_main.c
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1841
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1842 #ifdef CONFIG_NET_POLL_CONTROLLER
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1843 /**
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1844 * axienet_poll_controller - Axi Ethernet poll mechanism.
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1845 * @ndev: Pointer to net_device structure
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1846 *
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1847 * This implements Rx/Tx ISR poll mechanisms. The interrupts are disabled prior
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1848 * to polling the ISRs and are enabled back after the polling is done.
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1849 */
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1850 static void axienet_poll_controller(struct net_device *ndev)
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1851 {
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1852 struct axienet_local *lp = netdev_priv(ndev);
c0ebb862 Kedareswara rao Appana 2017-09-18 1853
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 @1854 disable_irq(lp->tx_irq);
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 @1855 disable_irq(lp->rx_irq);
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1856 axienet_rx_irq(lp->tx_irq, ndev);
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1857 axienet_tx_irq(lp->rx_irq, ndev);
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1858 enable_irq(lp->tx_irq);
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1859 enable_irq(lp->rx_irq);
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1860 }
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1861 #endif
8a3b7a25 danborkmann at iogearbox.net 2012-01-19 1862
:::::: The code at line 1854 was first introduced by commit
:::::: 8a3b7a252dca9fb28c23b5bf76c49180a2b60d3b drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver
:::::: TO: danborkmann at iogearbox.net <danborkmann@iogearbox.net>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 64342 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180521/375e42cb/attachment-0001.gz>
^ permalink raw reply
* [PATCH v6 5/5] drm/rockchip: support dp training outside dp firmware
From: Enric Balletbo Serra @ 2018-05-21 15:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526895424-22894-5-git-send-email-hl@rock-chips.com>
Hi Lin,
2018-05-21 11:37 GMT+02:00 Lin Huang <hl@rock-chips.com>:
> DP firmware uses fixed phy config values to do training, but some
> boards need to adjust these values to fit for their unique hardware
> design. So get phy config values from dts and use software link training
> instead of relying on firmware, if software training fail, keep firmware
> training as a fallback if sw training fails.
>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> Changes in v2:
> - update patch following Enric suggest
> Changes in v3:
> - use variable fw_training instead sw_training_success
> - base on DP SPCE, if training fail use lower link rate to retry training
> Changes in v4:
> - improve cdn_dp_get_lower_link_rate() and cdn_dp_software_train_link() follow Sean suggest
> Changes in v5:
> - fix some whitespcae issue
> Changes in v6:
> - None
>
> drivers/gpu/drm/rockchip/Makefile | 3 +-
> drivers/gpu/drm/rockchip/cdn-dp-core.c | 24 +-
> drivers/gpu/drm/rockchip/cdn-dp-core.h | 2 +
> drivers/gpu/drm/rockchip/cdn-dp-link-training.c | 420 ++++++++++++++++++++++++
> drivers/gpu/drm/rockchip/cdn-dp-reg.c | 31 +-
> drivers/gpu/drm/rockchip/cdn-dp-reg.h | 38 ++-
> 6 files changed, 505 insertions(+), 13 deletions(-)
> create mode 100644 drivers/gpu/drm/rockchip/cdn-dp-link-training.c
>
> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> index a314e21..b932f62 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -9,7 +9,8 @@ rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>
> rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> -rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
> +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o \
> + cdn-dp-link-training.o
> rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
> rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o
> rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index cce64c1..d9d0d4d 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> @@ -629,11 +629,13 @@ static void cdn_dp_encoder_enable(struct drm_encoder *encoder)
> goto out;
> }
> }
> -
> - ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
> - if (ret) {
> - DRM_DEV_ERROR(dp->dev, "Failed to idle video %d\n", ret);
> - goto out;
> + if (dp->use_fw_training == true) {
You don't need to compare to true. Simply do:
if (dp->use_fw_training)
> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE);
> + if (ret) {
> + DRM_DEV_ERROR(dp->dev,
> + "Failed to idle video %d\n", ret);
> + goto out;
> + }
> }
>
> ret = cdn_dp_config_video(dp);
> @@ -642,11 +644,15 @@ static void cdn_dp_encoder_enable(struct drm_encoder *encoder)
> goto out;
> }
>
> - ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
> - if (ret) {
> - DRM_DEV_ERROR(dp->dev, "Failed to valid video %d\n", ret);
> - goto out;
> + if (dp->use_fw_training == true) {
if (dp->use_fw_training)
> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_VALID);
> + if (ret) {
> + DRM_DEV_ERROR(dp->dev,
> + "Failed to valid video %d\n", ret);
> + goto out;
> + }
> }
> +
> out:
> mutex_unlock(&dp->lock);
> }
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h b/drivers/gpu/drm/rockchip/cdn-dp-core.h
> index 46159b2..77a9793 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.h
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h
> @@ -84,6 +84,7 @@ struct cdn_dp_device {
> bool connected;
> bool active;
> bool suspended;
> + bool use_fw_training;
>
> const struct firmware *fw; /* cdn dp firmware */
> unsigned int fw_version; /* cdn fw version */
> @@ -106,6 +107,7 @@ struct cdn_dp_device {
> u8 ports;
> u8 lanes;
> int active_port;
> + u8 train_set[4];
>
> u8 dpcd[DP_RECEIVER_CAP_SIZE];
> bool sink_has_audio;
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-link-training.c b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
> new file mode 100644
> index 0000000..73c3290
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-link-training.c
> @@ -0,0 +1,420 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> + * Author: Chris Zhong <zyw@rock-chips.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/phy/phy.h>
> +#include <soc/rockchip/rockchip_phy_typec.h>
> +
> +#include "cdn-dp-core.h"
> +#include "cdn-dp-reg.h"
> +
> +static void cdn_dp_set_signal_levels(struct cdn_dp_device *dp)
> +{
> + struct cdn_dp_port *port = dp->port[dp->active_port];
> + struct rockchip_typec_phy *tcphy = phy_get_drvdata(port->phy);
> +
> + int rate = drm_dp_bw_code_to_link_rate(dp->link.rate);
> + u8 swing = (dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) >>
> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
> + u8 pre_emphasis = (dp->train_set[0] & DP_TRAIN_PRE_EMPHASIS_MASK)
> + >> DP_TRAIN_PRE_EMPHASIS_SHIFT;
> +
> + tcphy->typec_phy_config(port->phy, rate, dp->link.num_lanes,
> + swing, pre_emphasis);
> +}
> +
> +static int cdn_dp_set_pattern(struct cdn_dp_device *dp, uint8_t dp_train_pat)
> +{
> + u32 phy_config, global_config;
> + int ret;
> + uint8_t pattern = dp_train_pat & DP_TRAINING_PATTERN_MASK;
> +
> + global_config = NUM_LANES(dp->link.num_lanes - 1) | SST_MODE |
> + GLOBAL_EN | RG_EN | ENC_RST_DIS | WR_VHSYNC_FALL;
> +
> + phy_config = DP_TX_PHY_ENCODER_BYPASS(0) |
> + DP_TX_PHY_SKEW_BYPASS(0) |
> + DP_TX_PHY_DISPARITY_RST(0) |
> + DP_TX_PHY_LANE0_SKEW(0) |
> + DP_TX_PHY_LANE1_SKEW(1) |
> + DP_TX_PHY_LANE2_SKEW(2) |
> + DP_TX_PHY_LANE3_SKEW(3) |
> + DP_TX_PHY_10BIT_ENABLE(0);
> +
> + if (pattern != DP_TRAINING_PATTERN_DISABLE) {
> + global_config |= NO_VIDEO;
> + phy_config |= DP_TX_PHY_TRAINING_ENABLE(1) |
> + DP_TX_PHY_SCRAMBLER_BYPASS(1) |
> + DP_TX_PHY_TRAINING_PATTERN(pattern);
> + }
> +
> + ret = cdn_dp_reg_write(dp, DP_FRAMER_GLOBAL_CONFIG, global_config);
> + if (ret) {
> + DRM_ERROR("fail to set DP_FRAMER_GLOBAL_CONFIG, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + ret = cdn_dp_reg_write(dp, DP_TX_PHY_CONFIG_REG, phy_config);
> + if (ret) {
> + DRM_ERROR("fail to set DP_TX_PHY_CONFIG_REG, error: %d\n",
> + ret);
> + return ret;
> + }
> +
> + ret = cdn_dp_reg_write(dp, DPTX_LANE_EN, BIT(dp->link.num_lanes) - 1);
> + if (ret) {
> + DRM_ERROR("fail to set DPTX_LANE_EN, error: %d\n", ret);
> + return ret;
> + }
> +
> + if (drm_dp_enhanced_frame_cap(dp->dpcd))
> + ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 1);
> + else
> + ret = cdn_dp_reg_write(dp, DPTX_ENHNCD, 0);
> + if (ret)
> + DRM_ERROR("failed to set DPTX_ENHNCD, error: %x\n", ret);
> +
> + return ret;
> +}
> +
> +static u8 cdn_dp_pre_emphasis_max(u8 voltage_swing)
> +{
> + switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
> + return DP_TRAIN_PRE_EMPH_LEVEL_3;
> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
> + return DP_TRAIN_PRE_EMPH_LEVEL_2;
> + case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
> + return DP_TRAIN_PRE_EMPH_LEVEL_1;
> + default:
> + return DP_TRAIN_PRE_EMPH_LEVEL_0;
> + }
> +}
> +
> +static void cdn_dp_get_adjust_train(struct cdn_dp_device *dp,
> + uint8_t link_status[DP_LINK_STATUS_SIZE])
> +{
> + int i;
> + uint8_t v = 0, p = 0;
> + uint8_t preemph_max;
> +
> + for (i = 0; i < dp->link.num_lanes; i++) {
> + v = max(v, drm_dp_get_adjust_request_voltage(link_status, i));
> + p = max(p, drm_dp_get_adjust_request_pre_emphasis(link_status,
> + i));
> + }
> +
> + if (v >= VOLTAGE_LEVEL_2)
> + v = VOLTAGE_LEVEL_2 | DP_TRAIN_MAX_SWING_REACHED;
> +
> + preemph_max = cdn_dp_pre_emphasis_max(v);
> + if (p >= preemph_max)
> + p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> +
> + for (i = 0; i < dp->link.num_lanes; i++)
> + dp->train_set[i] = v | p;
> +}
> +
> +/*
> + * Pick training pattern for channel equalization. Training Pattern 3 for HBR2
> + * or 1.2 devices that support it, Training Pattern 2 otherwise.
> + */
> +static u32 cdn_dp_select_chaneq_pattern(struct cdn_dp_device *dp)
> +{
> + u32 training_pattern = DP_TRAINING_PATTERN_2;
> +
> + /*
> + * cdn dp support HBR2 also support TPS3. TPS3 support is also mandatory
> + * for downstream devices that support HBR2. However, not all sinks
> + * follow the spec.
> + */
> + if (drm_dp_tps3_supported(dp->dpcd))
> + training_pattern = DP_TRAINING_PATTERN_3;
> + else
> + DRM_DEBUG_KMS("5.4 Gbps link rate without sink TPS3 support\n");
> +
> + return training_pattern;
> +}
> +
> +
> +static bool cdn_dp_link_max_vswing_reached(struct cdn_dp_device *dp)
> +{
> + int lane;
> +
> + for (lane = 0; lane < dp->link.num_lanes; lane++)
> + if ((dp->train_set[lane] & DP_TRAIN_MAX_SWING_REACHED) == 0)
> + return false;
> +
> + return true;
> +}
> +
> +static int cdn_dp_update_link_train(struct cdn_dp_device *dp)
> +{
> + int ret;
> +
> + cdn_dp_set_signal_levels(dp);
> +
> + ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET,
> + dp->train_set, dp->link.num_lanes);
> + if (ret != dp->link.num_lanes)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int cdn_dp_set_link_train(struct cdn_dp_device *dp,
> + uint8_t dp_train_pat)
> +{
> + uint8_t buf[sizeof(dp->train_set) + 1];
> + int ret, len;
> +
> + buf[0] = dp_train_pat;
> + if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
> + DP_TRAINING_PATTERN_DISABLE) {
> + /* don't write DP_TRAINING_LANEx_SET on disable */
> + len = 1;
> + } else {
> + /* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
> + memcpy(buf + 1, dp->train_set, dp->link.num_lanes);
> + len = dp->link.num_lanes + 1;
> + }
> +
> + ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_PATTERN_SET,
> + buf, len);
> + if (ret != len)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int cdn_dp_reset_link_train(struct cdn_dp_device *dp,
> + uint8_t dp_train_pat)
> +{
> + int ret;
> +
> + memset(dp->train_set, 0, sizeof(dp->train_set));
> +
> + cdn_dp_set_signal_levels(dp);
> +
> + ret = cdn_dp_set_pattern(dp, dp_train_pat);
> + if (ret)
> + return ret;
> +
> + return cdn_dp_set_link_train(dp, dp_train_pat);
> +}
> +
> +/* Enable corresponding port and start training pattern 1 */
> +static int cdn_dp_link_training_clock_recovery(struct cdn_dp_device *dp)
> +{
> + u8 voltage;
> + u8 link_status[DP_LINK_STATUS_SIZE];
> + u32 voltage_tries, max_vswing_tries;
> + int ret;
> +
> + /* clock recovery */
> + ret = cdn_dp_reset_link_train(dp, DP_TRAINING_PATTERN_1 |
> + DP_LINK_SCRAMBLING_DISABLE);
> + if (ret) {
> + DRM_ERROR("failed to start link train\n");
> + return ret;
> + }
> +
> + voltage_tries = 1;
> + max_vswing_tries = 0;
> + for (;;) {
> + drm_dp_link_train_clock_recovery_delay(dp->dpcd);
> + if (drm_dp_dpcd_read_link_status(&dp->aux, link_status) !=
> + DP_LINK_STATUS_SIZE) {
> + DRM_ERROR("failed to get link status\n");
> + return -EINVAL;
> + }
> +
> + if (drm_dp_clock_recovery_ok(link_status, dp->link.num_lanes)) {
> + DRM_DEBUG_KMS("clock recovery OK\n");
> + return 0;
> + }
> +
> + if (voltage_tries >= 5) {
> + DRM_DEBUG_KMS("Same voltage tried 5 times\n");
> + return -EINVAL;
> + }
> +
> + if (max_vswing_tries >= 1) {
> + DRM_DEBUG_KMS("Max Voltage Swing reached\n");
> + return -EINVAL;
> + }
> +
> + voltage = dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
> +
> + /* Update training set as requested by target */
> + cdn_dp_get_adjust_train(dp, link_status);
> + if (cdn_dp_update_link_train(dp)) {
> + DRM_ERROR("failed to update link training\n");
> + return -EINVAL;
> + }
> +
> + if ((dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) ==
> + voltage)
> + ++voltage_tries;
> + else
> + voltage_tries = 1;
> +
> + if (cdn_dp_link_max_vswing_reached(dp))
> + ++max_vswing_tries;
> + }
> +}
> +
> +static int cdn_dp_link_training_channel_equalization(struct cdn_dp_device *dp)
> +{
> + int tries, ret;
> + u32 training_pattern;
> + uint8_t link_status[DP_LINK_STATUS_SIZE];
> +
> + training_pattern = cdn_dp_select_chaneq_pattern(dp);
> + training_pattern |= DP_LINK_SCRAMBLING_DISABLE;
> +
> + ret = cdn_dp_set_pattern(dp, training_pattern);
> + if (ret)
> + return ret;
> +
> + ret = cdn_dp_set_link_train(dp, training_pattern);
> + if (ret) {
> + DRM_ERROR("failed to start channel equalization\n");
> + return ret;
> + }
> +
> + for (tries = 0; tries < 5; tries++) {
> + drm_dp_link_train_channel_eq_delay(dp->dpcd);
> + if (drm_dp_dpcd_read_link_status(&dp->aux, link_status) !=
> + DP_LINK_STATUS_SIZE) {
> + DRM_ERROR("failed to get link status\n");
> + break;
> + }
> +
> + /* Make sure clock is still ok */
> + if (!drm_dp_clock_recovery_ok(link_status,
> + dp->link.num_lanes)) {
> + DRM_DEBUG_KMS("Clock recovery check failed\n");
> + break;
> + }
> +
> + if (drm_dp_channel_eq_ok(link_status, dp->link.num_lanes)) {
> + DRM_DEBUG_KMS("Channel EQ done\n");
> + return 0;
> + }
> +
> + /* Update training set as requested by target */
> + cdn_dp_get_adjust_train(dp, link_status);
> + if (cdn_dp_update_link_train(dp)) {
> + DRM_ERROR("failed to update link training\n");
> + break;
> + }
> + }
> +
> + /* Try 5 times, else fail and try at lower BW */
> + if (tries == 5)
> + DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
> +
> + return -EINVAL;
> +}
> +
> +static int cdn_dp_stop_link_train(struct cdn_dp_device *dp)
> +{
> + int ret = cdn_dp_set_pattern(dp, DP_TRAINING_PATTERN_DISABLE);
> +
> + if (ret)
> + return ret;
> +
> + return cdn_dp_set_link_train(dp, DP_TRAINING_PATTERN_DISABLE);
> +}
> +
> +static int cdn_dp_get_lower_link_rate(struct cdn_dp_device *dp)
> +{
> + switch (dp->link.rate) {
> + case DP_LINK_BW_1_62:
> + return -EINVAL;
> + case DP_LINK_BW_2_7:
> + dp->link.rate = DP_LINK_BW_1_62;
> + break;
> + case DP_LINK_BW_5_4:
> + dp->link.rate = DP_LINK_BW_2_7;
> + break;
> + default:
> + dp->link.rate = DP_LINK_BW_5_4;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +int cdn_dp_software_train_link(struct cdn_dp_device *dp)
> +{
> + int ret, stop_err;
> + u8 link_config[2];
> + u32 rate, sink_max, source_max;
> +
> + ret = drm_dp_dpcd_read(&dp->aux, DP_DPCD_REV, dp->dpcd,
> + sizeof(dp->dpcd));
> + if (ret < 0) {
> + DRM_DEV_ERROR(dp->dev, "Failed to get caps %d\n", ret);
> + return ret;
> + }
> +
> + source_max = dp->lanes;
> + sink_max = drm_dp_max_lane_count(dp->dpcd);
> + dp->link.num_lanes = min(source_max, sink_max);
> +
> + source_max = drm_dp_bw_code_to_link_rate(CDN_DP_MAX_LINK_RATE);
> + sink_max = drm_dp_max_link_rate(dp->dpcd);
> + rate = min(source_max, sink_max);
> + dp->link.rate = drm_dp_link_rate_to_bw_code(rate);
> +
> + link_config[0] = 0;
> + link_config[1] = 0;
> + if (dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & 0x01)
> + link_config[1] = DP_SET_ANSI_8B10B;
> + drm_dp_dpcd_write(&dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
> +
> + while (true) {
> +
> + /* Write the link configuration data */
> + link_config[0] = dp->link.rate;
> + link_config[1] = dp->link.num_lanes;
> + if (drm_dp_enhanced_frame_cap(dp->dpcd))
> + link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> + drm_dp_dpcd_write(&dp->aux, DP_LINK_BW_SET, link_config, 2);
> +
> + ret = cdn_dp_link_training_clock_recovery(dp);
> + if (ret) {
> + if (!cdn_dp_get_lower_link_rate(dp))
> + continue;
> +
> + DRM_ERROR("training clock recovery failed: %d\n", ret);
> + break;
> + }
> +
> + ret = cdn_dp_link_training_channel_equalization(dp);
> + if (ret) {
> + if (!cdn_dp_get_lower_link_rate(dp))
> + continue;
> +
> + DRM_ERROR("training channel eq failed: %d\n", ret);
> + break;
> + }
> +
> + break;
> + }
> +
> + stop_err = cdn_dp_stop_link_train(dp);
> + if (stop_err) {
> + DRM_ERROR("stop training fail, error: %d\n", stop_err);
> + return stop_err;
> + }
> +
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> index 979355d..e1273e6 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> @@ -17,7 +17,9 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/iopoll.h>
> +#include <linux/phy/phy.h>
> #include <linux/reset.h>
> +#include <soc/rockchip/rockchip_phy_typec.h>
>
> #include "cdn-dp-core.h"
> #include "cdn-dp-reg.h"
> @@ -189,7 +191,7 @@ static int cdn_dp_mailbox_send(struct cdn_dp_device *dp, u8 module_id,
> return 0;
> }
>
> -static int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val)
> {
> u8 msg[6];
>
> @@ -609,6 +611,31 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
> {
> int ret;
>
> + /*
> + * DP firmware uses fixed phy config values to do training, but some
> + * boards need to adjust these values to fit for their unique hardware
> + * design. So if the phy is using custom config values, do software
> + * link training instead of relying on firmware, if software training
> + * fail, keep firmware training as a fallback if sw training fails.
> + */
> + ret = cdn_dp_software_train_link(dp);
> + if (ret) {
> + DRM_DEV_ERROR(dp->dev,
> + "Failed to do software training %d\n", ret);
> + goto do_fw_training;
> + }
If I understand correctly you are changing current behavior. Before
this patch, we use always firmware link training, and after this
patch, we always use software link training. If fails we use the
firmware link training.
AFAIK my Samsung Chromebook Plus works well with firmware link
training, so there are any benefits of use software link training
instead of firmware link training?
Looks to me that we should only do software link training on these
platforms that need it, so on those that define the phy-config
property in their DT and use by default firmware link training.
> + ret = cdn_dp_reg_write(dp, SOURCE_HDTX_CAR, 0xf);
> + if (ret) {
> + DRM_DEV_ERROR(dp->dev,
> + "Failed to write SOURCE_HDTX_CAR register %d\n", ret);
> + goto do_fw_training;
> + }
> + dp->use_fw_training = false;
> + return 0;
> +
> +do_fw_training:
> + dp->use_fw_training = true;
> + DRM_DEV_DEBUG_KMS(dp->dev, "use fw training\n");
> ret = cdn_dp_training_start(dp);
> if (ret) {
> DRM_DEV_ERROR(dp->dev, "Failed to start training %d\n", ret);
> @@ -623,7 +650,7 @@ int cdn_dp_train_link(struct cdn_dp_device *dp)
>
> DRM_DEV_DEBUG_KMS(dp->dev, "rate:0x%x, lanes:%d\n", dp->link.rate,
> dp->link.num_lanes);
> - return ret;
> + return 0;
> }
>
> int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active)
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> index 6580b11..3420771 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> @@ -137,7 +137,7 @@
> #define HPD_EVENT_MASK 0x211c
> #define HPD_EVENT_DET 0x2120
>
> -/* dpyx framer addr */
> +/* dptx framer addr */
> #define DP_FRAMER_GLOBAL_CONFIG 0x2200
> #define DP_SW_RESET 0x2204
> #define DP_FRAMER_TU 0x2208
> @@ -431,6 +431,40 @@
> /* Reference cycles when using lane clock as reference */
> #define LANE_REF_CYC 0x8000
>
> +/* register CM_VID_CTRL */
> +#define LANE_VID_REF_CYC(x) (((x) & (BIT(24) - 1)) << 0)
> +#define NMVID_MEAS_TOLERANCE(x) (((x) & 0xf) << 24)
> +
> +/* register DP_TX_PHY_CONFIG_REG */
> +#define DP_TX_PHY_TRAINING_ENABLE(x) ((x) & 1)
> +#define DP_TX_PHY_TRAINING_TYPE_PRBS7 (0 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_TPS1 (1 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_TPS2 (2 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_TPS3 (3 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_TPS4 (4 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_PLTPAT (5 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_D10_2 (6 << 1)
> +#define DP_TX_PHY_TRAINING_TYPE_HBR2CPAT (8 << 1)
> +#define DP_TX_PHY_TRAINING_PATTERN(x) ((x) << 1)
> +#define DP_TX_PHY_SCRAMBLER_BYPASS(x) (((x) & 1) << 5)
> +#define DP_TX_PHY_ENCODER_BYPASS(x) (((x) & 1) << 6)
> +#define DP_TX_PHY_SKEW_BYPASS(x) (((x) & 1) << 7)
> +#define DP_TX_PHY_DISPARITY_RST(x) (((x) & 1) << 8)
> +#define DP_TX_PHY_LANE0_SKEW(x) (((x) & 7) << 9)
> +#define DP_TX_PHY_LANE1_SKEW(x) (((x) & 7) << 12)
> +#define DP_TX_PHY_LANE2_SKEW(x) (((x) & 7) << 15)
> +#define DP_TX_PHY_LANE3_SKEW(x) (((x) & 7) << 18)
> +#define DP_TX_PHY_10BIT_ENABLE(x) (((x) & 1) << 21)
> +
> +/* register DP_FRAMER_GLOBAL_CONFIG */
> +#define NUM_LANES(x) ((x) & 3)
> +#define SST_MODE (0 << 2)
> +#define RG_EN (0 << 4)
> +#define GLOBAL_EN BIT(3)
> +#define NO_VIDEO BIT(5)
> +#define ENC_RST_DIS BIT(6)
> +#define WR_VHSYNC_FALL BIT(7)
> +
> enum voltage_swing_level {
> VOLTAGE_LEVEL_0,
> VOLTAGE_LEVEL_1,
> @@ -476,6 +510,7 @@ int cdn_dp_set_host_cap(struct cdn_dp_device *dp, u8 lanes, bool flip);
> int cdn_dp_event_config(struct cdn_dp_device *dp);
> u32 cdn_dp_get_event(struct cdn_dp_device *dp);
> int cdn_dp_get_hpd_status(struct cdn_dp_device *dp);
> +int cdn_dp_reg_write(struct cdn_dp_device *dp, u16 addr, u32 val);
> ssize_t cdn_dp_dpcd_write(struct cdn_dp_device *dp, u32 addr,
> u8 *data, u16 len);
> ssize_t cdn_dp_dpcd_read(struct cdn_dp_device *dp, u32 addr,
> @@ -489,4 +524,5 @@ int cdn_dp_config_video(struct cdn_dp_device *dp);
> int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info *audio);
> int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
> int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio);
> +int cdn_dp_software_train_link(struct cdn_dp_device *dp);
> #endif /* _CDN_DP_REG_H */
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 05/15] drm/sun4i: Add TCON TOP driver
From: Jernej Škrabec @ 2018-05-21 15:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521080517.3qmlfajqpn4uw7jv@flea>
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:05:17 CEST je Maxime Ripard napisal(a):
> On Sat, May 19, 2018 at 08:31:17PM +0200, Jernej Skrabec wrote:
> > As already described in DT binding, TCON TOP is responsible for
> > configuring display pipeline. In this initial driver focus is on HDMI
> > pipeline, so TVE and LCD configuration is not implemented.
> >
> > Implemented features:
> > - HDMI source selection
> > - clock driver (TCON and DSI gating)
> > - connecting mixers and TCONS
> >
> > Something similar also existed in previous SoCs, except that it was part
> > of first TCON.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >
> > drivers/gpu/drm/sun4i/Makefile | 3 +-
> > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 256 +++++++++++++++++++++
> > drivers/gpu/drm/sun4i/sun8i_tcon_top.h | 20 ++
> > include/dt-bindings/clock/sun8i-tcon-top.h | 11 +
> > 4 files changed, 289 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > create mode 100644 drivers/gpu/drm/sun4i/sun8i_tcon_top.h
> > create mode 100644 include/dt-bindings/clock/sun8i-tcon-top.h
> >
> > diff --git a/drivers/gpu/drm/sun4i/Makefile
> > b/drivers/gpu/drm/sun4i/Makefile index 2589f4acd5ae..09fbfd6304ba 100644
> > --- a/drivers/gpu/drm/sun4i/Makefile
> > +++ b/drivers/gpu/drm/sun4i/Makefile
> > @@ -16,7 +16,8 @@ sun8i-drm-hdmi-y += sun8i_hdmi_phy_clk.o
> >
> > sun8i-mixer-y += sun8i_mixer.o sun8i_ui_layer.o \
> >
> > sun8i_vi_layer.o sun8i_ui_scaler.o \
> >
> > - sun8i_vi_scaler.o sun8i_csc.o
> > + sun8i_vi_scaler.o sun8i_csc.o \
> > + sun8i_tcon_top.o
> >
> > sun4i-tcon-y += sun4i_crtc.o
> > sun4i-tcon-y += sun4i_dotclock.o
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c new file mode 100644
> > index 000000000000..075a356a6dfa
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > @@ -0,0 +1,256 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (c) 2018 Jernej Skrabec <jernej.skrabec@siol.net> */
> > +
> > +#include <drm/drmP.h>
> > +
> > +#include <dt-bindings/clock/sun8i-tcon-top.h>
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include "sun8i_tcon_top.h"
> > +
> > +#define TCON_TOP_PORT_SEL_REG 0x1C
> > +#define TCON_TOP_PORT_DE0_MSK GENMASK(1, 0)
> > +#define TCON_TOP_PORT_DE1_MSK GENMASK(5, 4)
> > +#define TCON_TOP_PORT_TCON_LCD0 0
> > +#define TCON_TOP_PORT_TCON_LCD1 1
> > +#define TCON_TOP_PORT_TCON_TV0 2
> > +#define TCON_TOP_PORT_TCON_TV1 3
> > +
> > +#define TCON_TOP_GATE_SRC_REG 0x20
> > +#define TCON_TOP_HDMI_SRC_MSK GENMASK(29, 28)
> > +#define TCON_TOP_HDMI_SRC_NONE 0
> > +#define TCON_TOP_HDMI_SRC_TCON_TV0 1
> > +#define TCON_TOP_HDMI_SRC_TCON_TV1 2
> > +#define TCON_TOP_TCON_TV1_GATE 24
> > +#define TCON_TOP_TCON_TV0_GATE 20
> > +#define TCON_TOP_TCON_DSI_GATE 16
> > +
> > +#define CLK_NUM 3
> > +
> > +struct sun8i_tcon_top {
> > + struct clk *bus;
> > + void __iomem *regs;
> > + struct reset_control *rst;
> > +
> > + /*
> > + * spinlock is used for locking access to registers from different
> > + * places - tcon driver and clk subsystem.
> > + */
> > + spinlock_t reg_lock;
> > +};
> > +
> > +struct sun8i_tcon_top_gate {
> > + const char *name;
> > + u8 bit;
> > + int index;
> > +};
> > +
> > +static const struct sun8i_tcon_top_gate gates[] = {
> > + {"bus-tcon-top-dsi", TCON_TOP_TCON_DSI_GATE, CLK_BUS_TCON_TOP_DSI},
> > + {"bus-tcon-top-tv0", TCON_TOP_TCON_TV0_GATE, CLK_BUS_TCON_TOP_TV0},
> > + {"bus-tcon-top-tv1", TCON_TOP_TCON_TV1_GATE, CLK_BUS_TCON_TOP_TV1},
> > +};
> > +
> > +void sun8i_tcon_top_set_hdmi_src(struct sun8i_tcon_top *tcon_top, int
> > tcon) +{
> > + unsigned long flags;
> > + u32 val;
> > +
> > + if (tcon > 1) {
> > + DRM_ERROR("TCON index is too high!\n");
> > + return;
> > + }
> > +
> > + spin_lock_irqsave(&tcon_top->reg_lock, flags);
> > +
> > + val = readl(tcon_top->regs + TCON_TOP_GATE_SRC_REG);
> > + val &= ~TCON_TOP_HDMI_SRC_MSK;
> > + val |= FIELD_PREP(TCON_TOP_HDMI_SRC_MSK,
> > + TCON_TOP_HDMI_SRC_TCON_TV0 + tcon);
> > + writel(val, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
> > +
> > + spin_unlock_irqrestore(&tcon_top->reg_lock, flags);
> > +}
> > +
> > +void sun8i_tcon_top_de_config(struct sun8i_tcon_top *tcon_top,
> > + int mixer, enum tcon_type tcon_type, int tcon)
> > +{
> > + unsigned long flags;
> > + u32 val, reg;
> > +
> > + if (mixer > 1) {
> > + DRM_ERROR("Mixer index is too high!\n");
> > + return;
> > + }
> > +
> > + if (tcon > 1) {
> > + DRM_ERROR("TCON index is too high!\n");
> > + return;
> > + }
> > +
> > + switch (tcon_type) {
> > + case tcon_type_lcd:
> > + val = TCON_TOP_PORT_TCON_LCD0 + tcon;
> > + break;
> > + case tcon_type_tv:
> > + val = TCON_TOP_PORT_TCON_TV0 + tcon;
> > + break;
> > + default:
> > + DRM_ERROR("Invalid TCON type!\n");
> > + return;
> > + }
> > +
> > + spin_lock_irqsave(&tcon_top->reg_lock, flags);
> > +
> > + reg = readl(tcon_top->regs + TCON_TOP_PORT_SEL_REG);
> > + if (mixer == 0) {
> > + reg &= ~TCON_TOP_PORT_DE0_MSK;
> > + reg |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, val);
> > + } else {
> > + reg &= ~TCON_TOP_PORT_DE1_MSK;
> > + reg |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, val);
> > + }
> > + writel(reg, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
> > +
> > + spin_unlock_irqrestore(&tcon_top->reg_lock, flags);
> > +}
> > +
> > +static int sun8i_tcon_top_probe(struct platform_device *pdev)
> > +{
> > + struct clk_hw_onecell_data *clk_data;
> > + struct sun8i_tcon_top *tcon_top;
> > + struct device *dev = &pdev->dev;
> > + struct resource *res;
> > + int ret, i;
> > +
> > + tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
> > + if (!tcon_top)
> > + return -ENOMEM;
> > +
> > + clk_data = devm_kzalloc(&pdev->dev, sizeof(*clk_data) +
> > + sizeof(*clk_data->hws) * CLK_NUM,
> > + GFP_KERNEL);
> > + if (!clk_data)
> > + return -ENOMEM;
> > +
> > + spin_lock_init(&tcon_top->reg_lock);
> > +
> > + tcon_top->rst = devm_reset_control_get(dev, "rst");
> > + if (IS_ERR(tcon_top->rst)) {
> > + dev_err(dev, "Couldn't get our reset line\n");
> > + return PTR_ERR(tcon_top->rst);
> > + }
> > +
> > + tcon_top->bus = devm_clk_get(dev, "bus");
> > + if (IS_ERR(tcon_top->bus)) {
> > + dev_err(dev, "Couldn't get the bus clock\n");
> > + return PTR_ERR(tcon_top->bus);
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + tcon_top->regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(tcon_top->regs))
> > + return PTR_ERR(tcon_top->regs);
> > +
> > + ret = reset_control_deassert(tcon_top->rst);
> > + if (ret) {
> > + dev_err(dev, "Could not deassert ctrl reset control\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(tcon_top->bus);
> > + if (ret) {
> > + dev_err(dev, "Could not enable bus clock\n");
> > + goto err_assert_reset;
> > + }
> > +
> > + /*
> > + * Default register values might have some reserved bits set, which
> > + * prevents TCON TOP from working properly. Set them to 0 here.
> > + */
> > + writel(0, tcon_top->regs + TCON_TOP_PORT_SEL_REG);
> > + writel(0, tcon_top->regs + TCON_TOP_GATE_SRC_REG);
> > +
> > + for (i = 0; i < CLK_NUM; i++) {
> > + const char *parent_name = "bus-tcon-top";
>
> I guess retrieving the parent's clock name at runtime would be more
> flexible.
>
It is, but will it ever be anything else?
> > + struct clk_init_data init;
> > + struct clk_gate *gate;
> > +
> > + gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> > + if (!gate) {
> > + ret = -ENOMEM;
> > + goto err_disable_clock;
> > + }
> > +
> > + init.name = gates[i].name;
> > + init.ops = &clk_gate_ops;
> > + init.flags = CLK_IS_BASIC;
> > + init.parent_names = &parent_name;
> > + init.num_parents = 1;
> > +
> > + gate->reg = tcon_top->regs + TCON_TOP_GATE_SRC_REG;
> > + gate->bit_idx = gates[i].bit;
> > + gate->lock = &tcon_top->reg_lock;
> > + gate->hw.init = &init;
> > +
> > + ret = devm_clk_hw_register(dev, &gate->hw);
> > + if (ret)
> > + goto err_disable_clock;
>
> Isn't it what clk_hw_register_gate is doing?
>
Almost, but not exactly. My goal was to use devm_* functions, so there is no
need to do any special cleanup.
> > + clk_data->hws[gates[i].index] = &gate->hw;
> > + }
> > +
> > + clk_data->num = CLK_NUM;
> > +
> > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
clk_data);
> > + if (ret)
> > + goto err_disable_clock;
> > +
> > + platform_set_drvdata(pdev, tcon_top);
> > +
> > + return 0;
> > +
> > +err_disable_clock:
> > + clk_disable_unprepare(tcon_top->bus);
> > +err_assert_reset:
> > + reset_control_assert(tcon_top->rst);
> > +
> > + return ret;
> > +}
> > +
> > +static int sun8i_tcon_top_remove(struct platform_device *pdev)
> > +{
> > + struct sun8i_tcon_top *tcon_top = platform_get_drvdata(pdev);
> > +
> > + clk_disable_unprepare(tcon_top->bus);
> > + reset_control_assert(tcon_top->rst);
> > +
> > + return 0;
> > +}
> > +
> > +const struct of_device_id sun8i_tcon_top_of_table[] = {
> > + { .compatible = "allwinner,sun8i-r40-tcon-top" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sun8i_tcon_top_of_table);
> > +
> > +static struct platform_driver sun8i_tcon_top_platform_driver = {
> > + .probe = sun8i_tcon_top_probe,
> > + .remove = sun8i_tcon_top_remove,
> > + .driver = {
> > + .name = "sun8i-tcon-top",
> > + .of_match_table = sun8i_tcon_top_of_table,
> > + },
> > +};
> > +module_platform_driver(sun8i_tcon_top_platform_driver);
> > +
> > +MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@siol.net>");
> > +MODULE_DESCRIPTION("Allwinner R40 TCON TOP driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.h
> > b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h new file mode 100644
> > index 000000000000..19126e07d2a6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/* Copyright (c) 2018 Jernej Skrabec <jernej.skrabec@siol.net> */
> > +
> > +#ifndef _SUN8I_TCON_TOP_H_
> > +#define _SUN8I_TCON_TOP_H_
> > +
> > +#include <linux/device.h>
> > +
> > +struct sun8i_tcon_top;
> > +
> > +enum tcon_type {
> > + tcon_type_lcd,
> > + tcon_type_tv,
>
> The usual practice is to have the enum values upper-case.
Ok.
Best regards,
Jernej
^ permalink raw reply
* [PATCH 04/15] dt-bindings: display: sunxi-drm: Add TCON TOP description
From: Jernej Škrabec @ 2018-05-21 15:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521080147.72esi2kwzcvzmetk@flea>
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:01:47 CEST je Maxime Ripard napisal(a):
> Hi,
>
> On Sat, May 19, 2018 at 08:31:16PM +0200, Jernej Skrabec wrote:
> > TCON TOP main purpose is to configure whole display pipeline. It
> > determines relationships between mixers and TCONs, selects source TCON
> > for HDMI, muxes LCD and TV encoder GPIO output,
>
> I'm not sure you mean GPIO here, but rather pin?
Right, I'll fix that.
>
> > selects TV encoder clock source and contains additional TV TCON and
> > DSI gates.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >
> > .../bindings/display/sunxi/sun4i-drm.txt | 20 +++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index
> > 3346c1e2a7a0..a099957ab62a 100644
> > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> >
> > @@ -187,6 +187,26 @@ And on the A23, A31, A31s and A33, you need one more
clock line:
> > - 'lvds-alt': An alternative clock source, separate from the TCON
> > channel 0
> >
> > clock, that can be used to drive the LVDS clock
> >
> > +TCON TOP
> > +--------
> > +
> > +TCON TOPs main purpose is to configure whole display pipeline. It
> > determines +relationships between mixers and TCONs, selects source TCON
> > for HDMI, muxes +LCD and TV encoder GPIO output, selects TV encoder clock
> > source and contains +additional TV TCON and DSI gates.
> > +
> > +Required properties:
> > + - compatible: value must be one of:
> > + * allwinner,sun8i-r40-tcon-top
> > + - reg: base address and size of the memory-mapped region.
> > + - clocks: phandle to the clocks feeding the TCON TOP
> > + * bus: TCON TOP interface clock
> > + - clock-names: clock name mentioned above
> > + - resets: phandle to the reset line driving the DRC
> > + * rst: TCON TOP reset line
> > + - reset-names: reset name mentioned above
> > + - #clock-cells : must contain 1
> > +
>
> I guess you should better describe the OF-graph endpoints, and the
> clocks output. Just using the binding additions here doesn't allow to
> get a clear idea of how the DT should look like.
With my idea of implementation, OF graph is the same as before, so I didn't
mention anything about it.
My idea of dependencies (you should view it in fixed width font):
TCON-TOP
^
|
mixer <-> TCON <-> HDMI
I'll explain my design decision as response to other mail.
Best regards,
Jernej
^ permalink raw reply
* [linux-sunxi] Re: [PATCH 12/15] drm/sun4i: Add support for second clock parent to DW HDMI PHY clk driver
From: Jernej Škrabec @ 2018-05-21 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521081253.cmx2mvfbfybgmtlv@flea>
Hi,
Dne ponedeljek, 21. maj 2018 ob 10:12:53 CEST je Maxime Ripard napisal(a):
> On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote:
> > Expand HDMI PHY clock driver to support second clock parent.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +-
> > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++-
> > drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------
> > 3 files changed, 98 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> > @@ -98,7 +98,8 @@
> >
> > #define SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN BIT(29)
> > #define SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN BIT(28)
> > #define SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 BIT(27)
> >
> > -#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL BIT(26)
> > +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK BIT(26)
> > +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT 26
> >
> > #define SUN8I_HDMI_PHY_PLL_CFG1_PLLEN BIT(25)
> > #define SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(x) ((x) << 22)
> > #define SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(x) ((x) << 20)
> >
> > @@ -190,6 +191,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi
> > *hdmi);
> >
> > void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy);
> > const struct dw_hdmi_phy_ops *sun8i_hdmi_phy_get_ops(void);
> >
> > -int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev);
> > +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev,
> > + bool second_parent);
> >
> > #endif /* _SUN8I_DW_HDMI_H_ */
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
> > b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index deba47ed69d8..7a911f0a3ae3
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
> > @@ -183,7 +183,13 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi
> > *hdmi,>
> > regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG,
> >
> > SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0);
> >
> > - regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, pll_cfg1_init);
> > + /*
> > + * NOTE: We have to be careful not to overwrite PHY parent
> > + * clock selection bit and clock divider.
> > + */
> > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
> > + ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,
> > + pll_cfg1_init);
>
> It feels like it belongs in a separate patch.
Why? clk_set_rate() on HDMI PHY clock is called before this function, so
SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL bit is already set to correct value. Original
code doesn't take this into account and sets it to 0 here in all cases, which
obviously is not right.
If you insist on splitting this in new patch, it has to be applied before
clock changes. It would also need to include "reset PLL clock configuration"
chunk (next change in this patch), otherwise other SoCs with only one parent
could get 1 there, which is obviously wrong. Note that I didn't really tested
if default value of this bit is 0 or 1, but I wouldn't really like to depend
on default values.
>
> > regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG,
> >
> > (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK,
> > pll_cfg2_init);
> >
> > @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct
> > sun8i_hdmi_phy *phy)>
> > SUN8I_HDMI_PHY_ANA_CFG3_SCLEN |
> > SUN8I_HDMI_PHY_ANA_CFG3_SDAEN);
> >
> > + /* reset PLL clock configuration */
> > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0);
> > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0);
> > +
>
> Ditto,
>
> > +
> > + /*
> > + * Even though HDMI PHY clock doesn't have enable/disable
> > + * handlers, we have to enable it. Otherwise it could happen
> > + * that parent PLL is not enabled by clock framework in a
> > + * highly unlikely event when parent PLL is used solely for
> > + * HDMI PHY clock.
> > + */
> > + clk_prepare_enable(phy->clk_phy);
>
> The implementation of the clock doesn't really matter in our API
> usage. If we're using a clock, we have to call
> clk_prepare_enable. That's documented everywhere, and mentionning how
> the clock is implemented is an abstraction leakage and it's
> irrelevant. I'd simply remove the comment here.
>
> And it should be in a separate patch as well.
OK. Should I add Fixes tag, since current code obviously is not by the specs?
It could still get in 4.17...
Best regards,
Jernej
^ permalink raw reply
* [PATCH v2 35/40] iommu/arm-smmu-v3: Add support for PCI ATS
From: Jean-Philippe Brucker @ 2018-05-21 14:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <922474e8-0aa5-e022-0502-f1e51b0d4859@codeaurora.org>
Hi Sinan,
On 19/05/18 18:25, Sinan Kaya wrote:
> Nothing specific about this patch but just a general observation. Last time I
> looked at the code, it seemed to require both ATS and PRI support from a given
> hardware.
>
> I think you can assume that for ATS 1.1 specification but ATS 1.0 specification
> allows a system to have ATS+PASID without PRI.
As far as I know, the latest ATS spec also states that "device that
supports ATS need not support PRI". I'm referring to the version
integrated into PCIe v4.0r1.0, which I think corresponds to ATS 1.1.
> QDF2400 is ATS 1.0 compatible as an example.
>
> Is this an assumption / my misinterpretation?
In this series you can enable ATS and PASID without PRI. The SMMU
enables ATS and PASID in add_device() if supported. Then PRI is only
enabled if users request IOMMU_SVA_FEAT_IOPF in sva_init_device(). If
the device driver pins all DMA memory, it can use PASID without PRI.
Thanks,
Jean
^ permalink raw reply
* [PATCH v2 13/40] vfio: Add support for Shared Virtual Addressing
From: Jean-Philippe Brucker @ 2018-05-21 14:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517165845.00000cc9@huawei.com>
On 17/05/18 16:58, Jonathan Cameron wrote:
>> +static int vfio_iommu_bind_group(struct vfio_iommu *iommu,
>> + struct vfio_group *group,
>> + struct vfio_mm *vfio_mm)
>> +{
>> + int ret;
>> + bool enabled_sva = false;
>> + struct vfio_iommu_sva_bind_data data = {
>> + .vfio_mm = vfio_mm,
>> + .iommu = iommu,
>> + .count = 0,
>> + };
>> +
>> + if (!group->sva_enabled) {
>> + ret = iommu_group_for_each_dev(group->iommu_group, NULL,
>> + vfio_iommu_sva_init);
>> + if (ret)
>> + return ret;
>> +
>> + group->sva_enabled = enabled_sva = true;
>> + }
>> +
>> + ret = iommu_group_for_each_dev(group->iommu_group, &data,
>> + vfio_iommu_sva_bind_dev);
>> + if (ret && data.count > 1)
>
> Are we safe to run this even if data.count == 1? I assume that at
> that point we would always not have an iommu domain associated with the
> device so the initial check would error out.
If data.count == 1, then the first bind didn't succeed. But it's not
necessarily a domain missing, failure to bind may come from various
places. If this vfio_mm was already bound to another device then it
contains a valid PASID and it's safe to call unbind(). Otherwise we call
it with PASID -1 (VFIO_INVALID_PASID) and that's a bit of a grey area.
-1 is currently invalid everywhere, but in the future someone might
implement 32 bits of PASIDs, in which case a bond between this dev and
PASID -1 might exist. But I think it's safe for now, and whoever
redefines VFIO_INVALID_PASID when such implementation appears will also
fix this case.
> Just be nice to get rid of the special casing in this error path as then
> could just do it all under if (ret) and make it visually clearer these
> are different aspects of the same error path.
[...]
>> static long vfio_iommu_type1_ioctl(void *iommu_data,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -1728,6 +2097,44 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>
>> return copy_to_user((void __user *)arg, &unmap, minsz) ?
>> -EFAULT : 0;
>> +
>> + } else if (cmd == VFIO_IOMMU_BIND) {
>> + struct vfio_iommu_type1_bind bind;
>> +
>> + minsz = offsetofend(struct vfio_iommu_type1_bind, flags);
>> +
>> + if (copy_from_user(&bind, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (bind.argsz < minsz)
>> + return -EINVAL;
>> +
>> + switch (bind.flags) {
>> + case VFIO_IOMMU_BIND_PROCESS:
>> + return vfio_iommu_type1_bind_process(iommu, (void *)arg,
>> + &bind);
>
> Can we combine these two blocks given it is only this case statement that is different?
That would be nicer, though I don't know yet what's needed for vSVA (by
Yi Liu on Cc), which will add statements to the switches.
Thanks,
Jean
>
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + } else if (cmd == VFIO_IOMMU_UNBIND) {
>> + struct vfio_iommu_type1_bind bind;
>> +
>> + minsz = offsetofend(struct vfio_iommu_type1_bind, flags);
>> +
>> + if (copy_from_user(&bind, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (bind.argsz < minsz)
>> + return -EINVAL;
>> +
>> + switch (bind.flags) {
>> + case VFIO_IOMMU_BIND_PROCESS:
>> + return vfio_iommu_type1_unbind_process(iommu, (void *)arg,
>> + &bind);
>> + default:
>> + return -EINVAL;
>> + }
>> }
^ permalink raw reply
* [PATCH V7] ARM: dts: da850-evm: Enable LCD and Backlight
From: Sekhar Nori @ 2018-05-21 14:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180518203335.13878-1-aford173@gmail.com>
On Saturday 19 May 2018 02:03 AM, Adam Ford wrote:
> When using the board files the LCD works, but not with the DT.
> This adds enables the original da850-evm to work with the same
> LCD in device tree mode.
>
> The EVM has a gpio for the regulator and a PWM for dimming the
> backlight. The LCD and the vpif display pins are mutually
> exclusive, so if using the LCD, do not load the vpif driver.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
> index 0e82bb988fde..a76c2ddfd23e 100644
> --- a/arch/arm/boot/dts/da850-evm.dts
> +++ b/arch/arm/boot/dts/da850-evm.dts
> @@ -27,6 +27,60 @@
> spi0 = &spi1;
> };
>
> + backlight: backlight-pwm {
> + pinctrl-names = "default";
> + pinctrl-0 = <&ecap2_pins>;
> + power-supply = <&backlight_lcd>;
> + compatible = "pwm-backlight";
> + pwms = <&ecap2 0 50000 0>;
It will be nice to add a comment here: "The PWM here corresponds to
production hardware. The schematic needs to be 1015171 (15 March 2010),
Rev A or newer."
> + brightness-levels = <0 10 20 30 40 50 60 70 80 90 99>;
> + default-brightness-level = <7>;
> + };
> +
> + panel {
> + compatible = "ti,tilcdc,panel";
> + pinctrl-names = "default";
> + pinctrl-0 = <&lcd_pins>;
> + /*
> + * The vpif and the LCD are mutually exclusive.
> + * To enable VPIF, change the status below to 'disabled' then
> + * then change the status of the vpif below to 'okay'
> + */
This results in checkpatch warning:
[PATCH V7] ARM: dts: da850-evm: Enable LCD and Backlight.eml:153: WARNING: Block comments should align the * on each line
[PATCH V7] ARM: dts: da850-evm: Enable LCD and Backlight.eml:239: WARNING: Block comments should align the * on each line
> &vpif {
> pinctrl-names = "default";
> pinctrl-0 = <&vpif_capture_pins>, <&vpif_display_pins>;
> - status = "okay";
> + /*
> + * The vpif and the LCD are mutually exclusive.
> + * To enable VPIF, disable the ti,tilcdc,panel then
> + * changed the status below to 'okay'
> + */
> + status = "disabled";
Are you able to see VPIF is disabled after this? Trying your patch, I
still see VPIF probing[1]. Also, only VPIF display has a conflict with
LCD, correct (capture should be fine)?
Thanks,
Sekhar
[1]
[ 34.739314] adv7343 0-002a: chip found @ 0x54 (DaVinci I2C adapter)
[ 34.881422] vpif_display vpif_display: Pixel details: Width = 720,Height = 480
[ 34.881506] vpif_display vpif_display: channel=5bf2d970,channel->video_dev=5bf2d970
[ 34.883374] vpif_display vpif_display: Pixel details: Width = 720,Height = 480
[ 34.883450] vpif_display vpif_display: channel=16041996,channel->video_dev=16041996
[ 35.370777] tvp514x 0-005d: tvp514x 0-005d decoder driver registered !!
[ 35.470088] vpif_capture vpif_capture: registered sub device tvp514x-0
[ 35.827492] tvp514x 0-005c: tvp514x 0-005c decoder driver registered !!
[ 35.866031] vpif_capture vpif_capture: registered sub device tvp514x-1
[ 35.953752] vpif_capture vpif_capture: VPIF capture driver initialized
# ls /dev/video
video0 video1 video2 video3
^ permalink raw reply
* [PATCH v4 2/3] arm64: dts: renesas: draak: Describe CVBS input
From: Laurent Pinchart @ 2018-05-21 14:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526913942-15426-3-git-send-email-jacopo+renesas@jmondi.org>
Hi Jacopo,
Thank you for the patch.
On Monday, 21 May 2018 17:45:41 EEST Jacopo Mondi wrote:
> Describe CVBS video input through analog video decoder ADV7180
> connected to video input interface VIN4.
>
> The video input signal path is shared with HDMI video input, and
> selected by on-board switches SW-53 and SW-54 with CVBS input selected
> by the default switches configuration.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> v3 -> v4:
> - Use 'adi,adv7180cp' compatible string in place of just 'adi,adv7180'
> as suggested by Laurent.
> - Re-structure CVBS input ports definition according to adv7180cp
> bindings: add port at 0 for composite input, add port at 3 for parallel
> video output.
> - Change node label to 'composite-in' as in Gose board bindings.
>
> v2 -> v3:
> - Add comment to describe the shared input video path.
> - Add my SoB and Niklas' R-b tags.
> ---
> arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 68
> ++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts index 9d73de8..ad59032
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
> @@ -59,6 +59,16 @@
> };
> };
>
> + composite-in {
> + compatible = "composite-video-connector";
> +
> + port {
> + composite_con_in: endpoint {
> + remote-endpoint = <&adv7180_in>;
> + };
> + };
> + };
> +
> memory at 48000000 {
> device_type = "memory";
> /* first 128MB is reserved for secure area. */
> @@ -142,6 +152,11 @@
> groups = "usb0";
> function = "usb0";
> };
> +
> + vin4_pins_cvbs: vin4 {
> + groups = "vin4_data8", "vin4_sync", "vin4_clk";
> + function = "vin4";
> + };
> };
>
> &i2c0 {
> @@ -154,6 +169,39 @@
> reg = <0x50>;
> pagesize = <8>;
> };
> +
> + composite-in at 20 {
> + compatible = "adi,adv7180cp";
> + reg = <0x20>;
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port at 0 {
> + reg = <0>;
> + adv7180_in: endpoint {
> + remote-endpoint = <&composite_con_in>;
> + };
> + };
> +
> + port at 3 {
> + reg = <3>;
> +
> + /*
> + * The VIN4 video input path is shared between
> + * CVBS and HDMI inputs through SW[49-53]
> + * switches.
> + *
> + * CVBS is the default selection, link it to
> + * VIN4 here.
> + */
> + adv7180_out: endpoint {
> + remote-endpoint = <&vin4_in>;
> + };
> + };
> + };
> + };
> };
>
> &i2c1 {
> @@ -246,3 +294,23 @@
> timeout-sec = <60>;
> status = "okay";
> };
> +
> +&vin4 {
> + pinctrl-0 = <&vin4_pins_cvbs>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port at 0 {
> + reg = <0>;
> +
> + vin4_in: endpoint {
> + remote-endpoint = <&adv7180_out>;
> + };
> + };
> + };
> +};
--
Regards,
Laurent Pinchart
^ permalink raw reply
* [PATCH v2 17/40] iommu/arm-smmu-v3: Link domains and devices
From: Jean-Philippe Brucker @ 2018-05-21 14:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517170748.00004927@huawei.com>
On 17/05/18 17:07, Jonathan Cameron wrote:
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -595,6 +595,11 @@ struct arm_smmu_device {
>> struct arm_smmu_master_data {
>> struct arm_smmu_device *smmu;
>> struct arm_smmu_strtab_ent ste;
>> +
>> + struct arm_smmu_domain *domain;
>> + struct list_head list; /* domain->devices */
>
> More meaningful name perhaps to avoid the need for the comment?
Sure
Thanks,
Jean
^ permalink raw reply
* [PATCH 1/2] drm/fourcc: add a 10bits fully packed variant of NV12
From: Ville Syrjälä @ 2018-05-21 14:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180520171705.29690-2-ayaka@soulik.info>
On Mon, May 21, 2018 at 01:17:04AM +0800, Randy Li wrote:
> This pixel format is a fully packed and 10bits variant of NV12.
> A luma pixel would take 10bits in memory, without any
> filled bits between pixels in a stride. The color gamut
> follows the BT.2020 standard.
>
> Signed-off-by: Randy Li <ayaka@soulik.info>
> ---
> drivers/gpu/drm/drm_fourcc.c | 1 +
> include/uapi/drm/drm_fourcc.h | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 5ca6395cd4d3..1f43967c4013 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -173,6 +173,7 @@ const struct drm_format_info *__drm_format_info(u32 format)
> { .format = DRM_FORMAT_UYVY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> { .format = DRM_FORMAT_VYUY, .depth = 0, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 2, .vsub = 1 },
> { .format = DRM_FORMAT_AYUV, .depth = 0, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
> + { .format = DRM_FORMAT_NV12_10LE40, .depth = 0, .num_planes = 2, .cpp = { 1, 2, 0 }, .hsub = 2, .vsub = 2 },
> };
>
> unsigned int i;
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index e04613d30a13..8eabf01e966f 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -140,6 +140,9 @@ extern "C" {
> #define DRM_FORMAT_NV61 fourcc_code('N', 'V', '6', '1') /* 2x1 subsampled Cb:Cr plane */
> #define DRM_FORMAT_NV24 fourcc_code('N', 'V', '2', '4') /* non-subsampled Cr:Cb plane */
> #define DRM_FORMAT_NV42 fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */
> +/* A fully packed variant of NV12_10LE32 */
What does "fully packed" mean? NV12_10LE32 doesn't even exist so
referring to it makes no sense.
Please try to provide an unambiguous description of new formats like we
have for everything else.
> +#define DRM_FORMAT_NV12_10LE40 fourcc_code('R', 'K', '2', '0') /* 2x2 subsampled Cr:Cb plane */
> +
>
> /*
> * 3 plane YCbCr
> --
> 2.14.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
^ permalink raw reply
* [PATCH v2 07/40] iommu: Add a page fault handler
From: Jean-Philippe Brucker @ 2018-05-21 14:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180518110434.150a0e64@jacob-builder>
On 18/05/18 19:04, Jacob Pan wrote:
>> +struct iopf_context {
>> + struct device *dev;
>> + struct iommu_fault_event evt;
>> + struct list_head head;
>> +};
>> +
>> +struct iopf_group {
>> + struct iopf_context last_fault;
>> + struct list_head faults;
>> + struct work_struct work;
>> +};
>> +
> All the page requests in the group should belong to the same device,
> perhaps struct device tracking should be in iopf_group instead of
> iopf_context?
Right, this is a leftover from when we kept a global list of partial
faults. Since the list is now per-device, I can move the dev pointer (I
think I should also rename iopf_context to iopf_fault, if that's all right)
>> +static int iopf_complete(struct device *dev, struct
>> iommu_fault_event *evt,
>> + enum page_response_code status)
>> +{
>> + struct page_response_msg resp = {
>> + .addr = evt->addr,
>> + .pasid = evt->pasid,
>> + .pasid_present = evt->pasid_valid,
>> + .page_req_group_id = evt->page_req_group_id,
>> + .private_data = evt->iommu_private,
>> + .resp_code = status,
>> + };
>> +
>> + return iommu_page_response(dev, &resp);
>> +}
>> +
>> +static enum page_response_code
>> +iopf_handle_single(struct iopf_context *fault)
>> +{
>> + /* TODO */
>> + return -ENODEV;
>> +}
>> +
>> +static void iopf_handle_group(struct work_struct *work)
>> +{
>> + struct iopf_group *group;
>> + struct iopf_context *fault, *next;
>> + enum page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
>> +
>> + group = container_of(work, struct iopf_group, work);
>> +
>> + list_for_each_entry_safe(fault, next, &group->faults, head) {
>> + struct iommu_fault_event *evt = &fault->evt;
>> + /*
>> + * Errors are sticky: don't handle subsequent faults
>> in the
>> + * group if there is an error.
>> + */
> There might be performance benefit for certain device to continue in
> spite of error in that the ATS retry may have less fault. Perhaps a
> policy decision for later enhancement.
Yes I think we should leave it to future work. My current reasoning is
that subsequent requests are more likely to fail as well and reporting
the error is more urgent, but we need real workloads to confirm this.
>> + if (status == IOMMU_PAGE_RESP_SUCCESS)
>> + status = iopf_handle_single(fault);
>> +
>> + if (!evt->last_req)
>> + kfree(fault);
>> + }
>> +
>> + iopf_complete(group->last_fault.dev, &group->last_fault.evt,
>> status);
>> + kfree(group);
>> +}
>> +
>> +/**
>> + * iommu_queue_iopf - IO Page Fault handler
>> + * @evt: fault event
>> + * @cookie: struct device, passed to
>> iommu_register_device_fault_handler.
>> + *
>> + * Add a fault to the device workqueue, to be handled by mm.
>> + */
>> +int iommu_queue_iopf(struct iommu_fault_event *evt, void *cookie)
>> +{
>> + struct iopf_group *group;
>> + struct iopf_context *fault, *next;
>> + struct iopf_device_param *iopf_param;
>> +
>> + struct device *dev = cookie;
>> + struct iommu_param *param = dev->iommu_param;
>> +
>> + if (WARN_ON(!mutex_is_locked(¶m->lock)))
>> + return -EINVAL;
>> +
>> + if (evt->type != IOMMU_FAULT_PAGE_REQ)
>> + /* Not a recoverable page fault */
>> + return IOMMU_PAGE_RESP_CONTINUE;
>> +
>> + /*
>> + * As long as we're holding param->lock, the queue can't be
>> unlinked
>> + * from the device and therefore cannot disappear.
>> + */
>> + iopf_param = param->iopf_param;
>> + if (!iopf_param)
>> + return -ENODEV;
>> +
>> + if (!evt->last_req) {
>> + fault = kzalloc(sizeof(*fault), GFP_KERNEL);
>> + if (!fault)
>> + return -ENOMEM;
>> +
>> + fault->evt = *evt;
>> + fault->dev = dev;
>> +
>> + /* Non-last request of a group. Postpone until the
>> last one */
>> + list_add(&fault->head, &iopf_param->partial);
>> +
>> + return IOMMU_PAGE_RESP_HANDLED;
>> + }
>> +
>> + group = kzalloc(sizeof(*group), GFP_KERNEL);
>> + if (!group)
>> + return -ENOMEM;
>> +
>> + group->last_fault.evt = *evt;
>> + group->last_fault.dev = dev;
>> + INIT_LIST_HEAD(&group->faults);
>> + list_add(&group->last_fault.head, &group->faults);
>> + INIT_WORK(&group->work, iopf_handle_group);
>> +
>> + /* See if we have partial faults for this group */
>> + list_for_each_entry_safe(fault, next, &iopf_param->partial,
>> head) {
>> + if (fault->evt.page_req_group_id ==
>> evt->page_req_group_id)
> If all 9 bit group index are used, there can be lots of PRGs. For
> future enhancement, maybe we can have per group partial list ready to
> go when LPIG comes in? I will be less searching.
Yes, allocating the PRG from the start could be more efficient. I think
it's slightly more complicated code so I'd rather see performance
numbers before implementing it
>> + /* Insert *before* the last fault */
>> + list_move(&fault->head, &group->faults);
>> + }
>> +
> If you already sorted the group list with last fault at the end, why do
> you need a separate entry to track the last fault?
Not sure I understand your question, sorry. Do you mean why the
iopf_group.last_fault? Just to avoid one more kzalloc.
>> +
>> + queue->flush(queue->flush_arg, dev);
>> +
>> + /*
>> + * No need to clear the partial list. All PRGs containing
>> the PASID that
>> + * needs to be decommissioned are whole (the device driver
>> made sure of
>> + * it before this function was called). They have been
>> submitted to the
>> + * queue by the above flush().
>> + */
> So you are saying device driver need to make sure LPIG PRQ is submitted
> in the flush call above such that partial list is cleared?
Not exactly, it's the IOMMU driver that makes sure all LPIG in its
queues are submitted by the above flush call. In more details the flow is:
* Either device driver calls unbind()/sva_device_shutdown(), or the
process exits.
* If the device driver called, then it already told the device to stop
using the PASID. Otherwise we use the mm_exit() callback to tell the
device driver to stop using the PASID.
* In either case, when receiving a stop request from the driver, the
device sends the LPIGs to the IOMMU queue.
* Then, the flush call above ensures that the IOMMU reports the LPIG
with iommu_report_device_fault.
* While submitting all LPIGs for this PASID to the work queue,
ipof_queue_fault also picked up all partial faults, so the partial list
is clean.
Maybe I should improve this comment?
> what if
> there are device failures where device needs to reset (either whole
> function level or PASID level), should there still be a need to clear
> the partial list for all associated PASID/group?
I guess the simplest way out, when resetting the device, is for the
device driver to call iommu_sva_device_shutdown(). Both the IOMMU
driver's sva_device_shutdown() and remove_device() ops should call
iopf_queue_remove_device(), which clears the partial list.
Thanks,
Jean
^ permalink raw reply
* [PATCH v9 12/16] KVM: arm64: Save host SVE context as appropriate
From: Dave Martin @ 2018-05-21 14:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <eac5821b-c5e0-90c9-cdd6-73643662d68d@arm.com>
On Mon, May 21, 2018 at 03:40:02PM +0100, Marc Zyngier wrote:
> On 21/05/18 15:17, Dave Martin wrote:
> > This patch adds SVE context saving to the hyp FPSIMD context switch
> > path. This means that it is no longer necessary to save the host
> > SVE state in advance of entering the guest, when in use.
> >
> > In order to avoid adding pointless complexity to the code, VHE is
> > assumed if SVE is in use. VHE is an architectural prerequisite for
> > SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
> > kernels that support both SVE and KVM.
> >
> > Historically, software models exist that can expose the
> > architecturally invalid configuration of SVE without VHE, so if
> > this situation is detected at kvm_init() time then KVM will be
> > disabled.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> >
> > ---
> >
> > * Stripped the following tags, reviewers please reconfirm:
> >
> > Formerly-Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> > Formerly-Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > Formerly-Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> >
> > (Creation of a new file for this one change may be deemed undesirable,
> > but there didn't seem to be a correct place to put it. There may also
> > be a way around the circular include problem that I missed.)
> >
> > Changes since v8:
> >
> > * Add kvm_arch_check_supported() hook, and move arm64-specific check
> > for SVE-implies-VHE into arch/arm64/.
> >
> > Due to circular header dependency problems, it is difficult to get
> > the prototype for kvm_pr_*() functions in <asm/kvm_host.h>, so this
> > patch puts arm64's kvm_arch_check_supported() hook out of line.
> > This is not a hot function.
> > ---
> > arch/arm/include/asm/kvm_host.h | 1 +
> > arch/arm64/Kconfig | 7 +++++++
> > arch/arm64/include/asm/kvm_host.h | 1 +
> > arch/arm64/kvm/Makefile | 2 +-
> > arch/arm64/kvm/fpsimd.c | 1 -
> > arch/arm64/kvm/hyp/switch.c | 20 +++++++++++++++++++-
> > arch/arm64/kvm/init.c | 33 +++++++++++++++++++++++++++++++++
> > virt/kvm/arm/arm.c | 6 ++++++
> > 8 files changed, 68 insertions(+), 3 deletions(-)
> > create mode 100644 arch/arm64/kvm/init.c
> >
[...]
> > diff --git a/arch/arm64/kvm/init.c b/arch/arm64/kvm/init.c
> > new file mode 100644
> > index 0000000..3b6e730
> > --- /dev/null
> > +++ b/arch/arm64/kvm/init.c
> > @@ -0,0 +1,33 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * arch/arm64/kvm/init.c: KVM initialisation support
> > + *
> > + * Copyright 2018 Arm Limited
> > + * Author: Dave Martin <Dave.Martin@arm.com>
> > + */
> > +#include <linux/errno.h>
> > +#include <linux/kvm_host.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/kvm_host.h>
> > +
> > +/* Additional arch-dependent checks for KVM usability */
> > +int kvm_arch_check_supported(void)
> > +{
> > + /*
> > + * VHE is a prerequisite for SVE in the Arm architecture, and
> > + * Kconfig ensures that if system_supports_sve() here then
> > + * CONFIG_ARM64_VHE is enabled, so if VHE support wasn't already
> > + * detected and enabled, the CPU is architecturally
> > + * noncompliant.
> > + *
> > + * Just in case this mismatch is seen, detect it, warn and give
> > + * up. Supporting this forbidden configuration in Hyp would be
> > + * pointless.
> > + */
> > + if (system_supports_sve() && !has_vhe()) {
> > + kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?");
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
>
> [entering bikeshedding territory]
>
> I'm not exactly keen on this. This feels overkill, and a departure from
> what we've been doing so far.
>
> Why don't you simply have a helper in kvm_host.h that does:
>
> static inline bool kvm_arm_check_sve_valid(void)
> {
> return !system_supports_sve() || has_vhe();
> }
>
> (and a canonical "return true;" implementation for 32bit)
>
> > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > index bee226c..8518df0 100644
> > --- a/virt/kvm/arm/arm.c
> > +++ b/virt/kvm/arm/arm.c
[...]
> > @@ -1574,6 +1576,10 @@ int kvm_arch_init(void *opaque)
> > return -ENODEV;
> > }
> >
> > + err = kvm_arch_check_supported();
> > + if (err)
> > + return err;
> > +
>
> And turn this into:
>
> if (!kvm_arm_check_sve_valid()) {
> kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?");
> return -EINVAL;
> }
>
> It has the advantage of being obvious of what we check for, and doesn't
> add any extra file.
Making the call special-purpose makes it more natural to pull the printk
out of the call, solving the header issues.
This would be a bit gross in core code, but arm.c is supported to be
Arm-specific, so if you prefer this approach it doesn't look too bad
here.
I'm happy to rework along these lines.
Cheers
---Dave
^ permalink raw reply
* [PATCH v2 07/40] iommu: Add a page fault handler
From: Jean-Philippe Brucker @ 2018-05-21 14:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517162555.00002bd3@huawei.com>
On 17/05/18 16:25, Jonathan Cameron wrote:
> On Fri, 11 May 2018 20:06:08 +0100
> Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:
>
>> Some systems allow devices to handle I/O Page Faults in the core mm. For
>> example systems implementing the PCI PRI extension or Arm SMMU stall
>> model. Infrastructure for reporting these recoverable page faults was
>> recently added to the IOMMU core for SVA virtualisation. Add a page fault
>> handler for host SVA.
>>
>> IOMMU driver can now instantiate several fault workqueues and link them to
>> IOPF-capable devices. Drivers can choose between a single global
>> workqueue, one per IOMMU device, one per low-level fault queue, one per
>> domain, etc.
>>
>> When it receives a fault event, supposedly in an IRQ handler, the IOMMU
>> driver reports the fault using iommu_report_device_fault(), which calls
>> the registered handler. The page fault handler then calls the mm fault
>> handler, and reports either success or failure with iommu_page_response().
>> When the handler succeeded, the IOMMU retries the access.
>>
>> The iopf_param pointer could be embedded into iommu_fault_param. But
>> putting iopf_param into the iommu_param structure allows us not to care
>> about ordering between calls to iopf_queue_add_device() and
>> iommu_register_device_fault_handler().
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>
> Hi Jean-Phillipe,
>
> One question below on how we would end up with partial faults left when
> doing the queue remove. Code looks fine, but I'm not seeing how that
> would happen without buggy hardware... + we presumably have to rely on
> the hardware timing out on that request or it's dead anyway...
>> +/**
>> + * iopf_queue_remove_device - Remove producer from fault queue
>> + * @dev: device to remove
>> + *
>> + * Caller makes sure that no more fault is reported for this device, and no more
>> + * flush is scheduled for this device.
>> + *
>> + * Note: safe to call unconditionally on a cleanup path, even if the device
>> + * isn't registered to any IOPF queue.
>> + *
>> + * Return 0 if the device was attached to the IOPF queue
>> + */
>> +int iopf_queue_remove_device(struct device *dev)
>> +{
>> + struct iopf_context *fault, *next;
>> + struct iopf_device_param *iopf_param;
>> + struct iommu_param *param = dev->iommu_param;
>> +
>> + if (!param)
>> + return -EINVAL;
>> +
>> + mutex_lock(¶m->lock);
>> + iopf_param = param->iopf_param;
>> + if (iopf_param) {
>> + refcount_dec(&iopf_param->queue->refs);
>> + param->iopf_param = NULL;
>> + }
>> + mutex_unlock(¶m->lock);
>> + if (!iopf_param)
>> + return -EINVAL;
>> +
>> + list_for_each_entry_safe(fault, next, &iopf_param->partial, head)
>> + kfree(fault);
>> +
>
> Why would we end up here with partials still in the list? Buggy hardware?
Buggy hardware is one possibility. There also is the nasty case of PRI
queue overflow. If the PRI queue is full, then the SMMU discards new
Page Requests from the device. It may discard a 'last' PR of a group
that is already in iopf_param->partial. This group will never be freed
until this cleanup.
The spec dismisses PRIq overflows because the OS is supposed to allocate
PRI credits to devices such that they can't send more than the PRI queue
size. This isn't possible in Linux because we don't know exactly how
many PRI-capable devices will share a queue (the upper limit is 2**32,
and devices may be hotplugged well after we allocated the PRI queue). So
PRIq overflow is possible.
Ideally when detecting a PRIq overflow we need to immediately remove all
partial faults of all devices sharing this queue. Since I can't easily
test that case (my device doesn't do PRGs) and it's complicated code, I
left it as TODO in patch 39, and this is our only chance to free
orphaned page requests.
>> +void iopf_queue_free(struct iopf_queue *queue)
>> +{
>> +
>
> Nitpick : Blank line here doesn't add anything.
Ok
Thanks,
Jean
^ permalink raw reply
* [PATCH v4 3/3] arm64: dts: renesas: draak: Describe HDMI input
From: Jacopo Mondi @ 2018-05-21 14:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526913942-15426-1-git-send-email-jacopo+renesas@jmondi.org>
Describe HDMI input connector and ADV7612 HDMI decoder installed on
R-Car Gen3 Draak board.
The video signal routing to the HDMI decoder to the video input interface
VIN4 is multiplexed with CVBS input path, and enabled/disabled through
on-board switches SW-49, SW-50, SW-51 and SW-52.
As the default board switches configuration connects CVBS input to VIN4,
leave the HDMI decoder unconnected in DTS.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v3 -> v4:
- Add Laurent's R-b tag.
v2 -> v3:
- Add comment on HDMI output port about the shared CVBS/HDMI video path.
- Add Niklas' R-b tag.
---
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 49 ++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index ad59032..1a474f94 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -69,6 +69,17 @@
};
};
+ hdmi-in {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_con_in: endpoint {
+ remote-endpoint = <&adv7612_in>;
+ };
+ };
+ };
+
memory at 48000000 {
device_type = "memory";
/* first 128MB is reserved for secure area. */
@@ -201,6 +212,44 @@
};
};
};
+
+ };
+
+ hdmi-decoder at 4c {
+ compatible = "adi,adv7612";
+ reg = <0x4c>;
+ default-input = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port at 0 {
+ reg = <0>;
+
+ adv7612_in: endpoint {
+ remote-endpoint = <&hdmi_con_in>;
+ };
+ };
+
+ port at 2 {
+ reg = <2>;
+
+ /*
+ * The VIN4 video input path is shared between
+ * CVBS and HDMI inputs through SW[49-53]
+ * switches.
+ *
+ * CVBS is the default selection, leave HDMI
+ * not connected here.
+ */
+ adv7612_out: endpoint {
+ pclk-sample = <0>;
+ hsync-active = <0>;
+ vsync-active = <0>;
+ };
+ };
+ };
};
};
--
2.7.4
^ permalink raw reply related
* [PATCH v4 2/3] arm64: dts: renesas: draak: Describe CVBS input
From: Jacopo Mondi @ 2018-05-21 14:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526913942-15426-1-git-send-email-jacopo+renesas@jmondi.org>
Describe CVBS video input through analog video decoder ADV7180
connected to video input interface VIN4.
The video input signal path is shared with HDMI video input, and
selected by on-board switches SW-53 and SW-54 with CVBS input selected
by the default switches configuration.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
---
v3 -> v4:
- Use 'adi,adv7180cp' compatible string in place of just 'adi,adv7180'
as suggested by Laurent.
- Re-structure CVBS input ports definition according to adv7180cp
bindings: add port at 0 for composite input, add port at 3 for parallel
video output.
- Change node label to 'composite-in' as in Gose board bindings.
v2 -> v3:
- Add comment to describe the shared input video path.
- Add my SoB and Niklas' R-b tags.
---
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 68 ++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
index 9d73de8..ad59032 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77995-draak.dts
@@ -59,6 +59,16 @@
};
};
+ composite-in {
+ compatible = "composite-video-connector";
+
+ port {
+ composite_con_in: endpoint {
+ remote-endpoint = <&adv7180_in>;
+ };
+ };
+ };
+
memory at 48000000 {
device_type = "memory";
/* first 128MB is reserved for secure area. */
@@ -142,6 +152,11 @@
groups = "usb0";
function = "usb0";
};
+
+ vin4_pins_cvbs: vin4 {
+ groups = "vin4_data8", "vin4_sync", "vin4_clk";
+ function = "vin4";
+ };
};
&i2c0 {
@@ -154,6 +169,39 @@
reg = <0x50>;
pagesize = <8>;
};
+
+ composite-in at 20 {
+ compatible = "adi,adv7180cp";
+ reg = <0x20>;
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port at 0 {
+ reg = <0>;
+ adv7180_in: endpoint {
+ remote-endpoint = <&composite_con_in>;
+ };
+ };
+
+ port at 3 {
+ reg = <3>;
+
+ /*
+ * The VIN4 video input path is shared between
+ * CVBS and HDMI inputs through SW[49-53]
+ * switches.
+ *
+ * CVBS is the default selection, link it to
+ * VIN4 here.
+ */
+ adv7180_out: endpoint {
+ remote-endpoint = <&vin4_in>;
+ };
+ };
+ };
+ };
};
&i2c1 {
@@ -246,3 +294,23 @@
timeout-sec = <60>;
status = "okay";
};
+
+&vin4 {
+ pinctrl-0 = <&vin4_pins_cvbs>;
+ pinctrl-names = "default";
+
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port at 0 {
+ reg = <0>;
+
+ vin4_in: endpoint {
+ remote-endpoint = <&adv7180_out>;
+ };
+ };
+ };
+};
--
2.7.4
^ permalink raw reply related
* [PATCH v4 1/3] dt-bindings: media: rcar-vin: Add R8A77995 support
From: Jacopo Mondi @ 2018-05-21 14:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526913942-15426-1-git-send-email-jacopo+renesas@jmondi.org>
Add compatible string for R-Car D3 R8A7795 to list of SoCs supported by
rcar-vin driver.
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas S?derlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
Documentation/devicetree/bindings/media/rcar_vin.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index a19517e1..5c6f2a7 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -22,6 +22,7 @@ on Gen3 platforms to a CSI-2 receiver.
- "renesas,vin-r8a7795" for the R8A7795 device
- "renesas,vin-r8a7796" for the R8A7796 device
- "renesas,vin-r8a77970" for the R8A77970 device
+ - "renesas,vin-r8a77995" for the R8A77995 device
- "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible
device.
--
2.7.4
^ permalink raw reply related
* [PATCH v4 0/3] arm64: dts: Draak: Enable video inputs and VIN4
From: Jacopo Mondi @ 2018-05-21 14:45 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
this series enables HDMI, CVBS and VIN4 for R8A77995 Draak board.
Compared to v3, the analog video decoder ADV7180 is now described in bindings
with the compatible string "adi,adv7180cp" and its port nodes definition
has been res-structured according to the chip bindings as reported by
Laurent on v3.
Bindings have been copi^W^Winspired from Gen2 Gose board, the only Gen2 board
using the "adv7180cp" compatible string.
Example of how to switch to HDMI input is still available here:
git://jmondi.org/linux d3/media-master/salvator-x-dts_csi2/d3-hdmi
And the series still depends on
[PATCH v3 0/9] rcar-vin: Add support for parallel input on Gen3
Thanks
j
Jacopo Mondi (3):
dt-bindings: media: rcar-vin: Add R8A77995 support
arm64: dts: renesas: draak: Describe CVBS input
arm64: dts: renesas: draak: Describe HDMI input
v3 -> v4:
- Use 'adi,adv7180cp' in [3/3] as detailed in the commit message
- Add Laurent's R-b tag to [2/3]
v2 -> v3:
- Add comment to CVBS and HDMI inputs
- Add missing Signed-off-by to [2/3]
- Add Niklas' tag
.../devicetree/bindings/media/rcar_vin.txt | 1 +
arch/arm64/boot/dts/renesas/r8a77995-draak.dts | 117 +++++++++++++++++++++
2 files changed, 118 insertions(+)
--
2.7.4
^ permalink raw reply
* [PATCH v2 05/40] iommu/sva: Track mm changes with an MMU notifier
From: Jean-Philippe Brucker @ 2018-05-21 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517152514.00004247@huawei.com>
On 17/05/18 15:25, Jonathan Cameron wrote:
>> + * already have been removed from the list. Check is someone is
>
> Check if someone...
Ok
Thanks,
Jean
^ permalink raw reply
* [PATCH v2 03/40] iommu/sva: Manage process address spaces
From: Jean-Philippe Brucker @ 2018-05-21 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517150516.000067ca@huawei.com>
On 17/05/18 15:25, Jonathan Cameron wrote:
>> +static struct io_mm *
>> +io_mm_alloc(struct iommu_domain *domain, struct device *dev,
>> + struct mm_struct *mm, unsigned long flags)
>> +{
>> + int ret;
>> + int pasid;
>> + struct io_mm *io_mm;
>> + struct iommu_sva_param *param = dev->iommu_param->sva_param;
>> +
>> + if (!domain->ops->mm_alloc || !domain->ops->mm_free)
>> + return ERR_PTR(-ENODEV);
>> +
>> + io_mm = domain->ops->mm_alloc(domain, mm, flags);
>> + if (IS_ERR(io_mm))
>> + return io_mm;
>> + if (!io_mm)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + /*
>> + * The mm must not be freed until after the driver frees the io_mm
>> + * (which may involve unpinning the CPU ASID for instance, requiring a
>> + * valid mm struct.)
>> + */
>> + mmgrab(mm);
>> +
>> + io_mm->flags = flags;
>> + io_mm->mm = mm;
>> + io_mm->release = domain->ops->mm_free;
>> + INIT_LIST_HEAD(&io_mm->devices);
>> +
>> + idr_preload(GFP_KERNEL);
>> + spin_lock(&iommu_sva_lock);
>> + pasid = idr_alloc(&iommu_pasid_idr, io_mm, param->min_pasid,
>> + param->max_pasid + 1, GFP_ATOMIC);
>
> I'd expect the IDR cleanup to be in io_mm_free as that would 'match'
> against io_mm_alloc but it's in io_mm_release just before the io_mm_free
> call, perhaps move it or am I missing something?
>
> Hmm. This is reworked in patch 5 to use call rcu to do the free. I suppose
> we may be burning an idr entry if we take a while to get round to the
> free.. If so a comment to explain this would be great.
Ok, I'll see if I can come up with some comments for both patch 3 and 5.
>> + io_mm->pasid = pasid;
>> + spin_unlock(&iommu_sva_lock);
>> + idr_preload_end();
>> +
>> + if (pasid < 0) {
>> + ret = pasid;
>> + goto err_free_mm;
>> + }
>> +
>> + /* TODO: keep track of mm. For the moment, abort. */
>
> From later patches, I can now see why we didn't init the kref
> here, but perhaps a comment would make that clear rather than
> people checking it is correctly used throughout? Actually just grab
> the comment from patch 5 and put it in this one and that will
> do the job nicely.
Ok
>> + ret = -ENOSYS;
>> + spin_lock(&iommu_sva_lock);
>> + idr_remove(&iommu_pasid_idr, io_mm->pasid);
>> + spin_unlock(&iommu_sva_lock);
>> +
>> +err_free_mm:
>> + domain->ops->mm_free(io_mm);
>
> Really minor, but you now have io_mm->release set so to keep
> this obviously the same as the io_mm_free path, perhaps call
> that rather than mm_free directly.
Yes, makes sense
>> +static void io_mm_detach_locked(struct iommu_bond *bond)
>> +{
>> + struct iommu_bond *tmp;
>> + bool detach_domain = true;
>> + struct iommu_domain *domain = bond->domain;
>> +
>> + list_for_each_entry(tmp, &domain->mm_list, domain_head) {
>> + if (tmp->io_mm == bond->io_mm && tmp->dev != bond->dev) {
>> + detach_domain = false;
>> + break;
>> + }
>> + }
>> +
>> + domain->ops->mm_detach(domain, bond->dev, bond->io_mm, detach_domain);
>> +
>
> I can't see an immediate reason to have a different order in her to the reverse of
> the attach above. So I think you should be detaching after the list_del calls.
> If there is a reason, can we have a comment so I don't ask on v10.
I don't see a reason either right now, I'll see if it can be moved
>
>> + list_del(&bond->mm_head);
>> + list_del(&bond->domain_head);
>> + list_del(&bond->dev_head);
>> + io_mm_put_locked(bond->io_mm);
>> + /* If an io_mm already exists, use it */
>> + spin_lock(&iommu_sva_lock);
>> + idr_for_each_entry(&iommu_pasid_idr, io_mm, i) {
>> + if (io_mm->mm == mm && io_mm_get_locked(io_mm)) {
>> + /* ... Unless it's already bound to this device */
>> + list_for_each_entry(tmp, &io_mm->devices, mm_head) {
>> + if (tmp->dev == dev) {
>> + bond = tmp;
>
> Using bond for this is clear in a sense, but can we not just use ret
> so it is obvious here that we are going to return -EEXIST?
> At first glance I thought you were going to carry on with this bond
> and couldn't work out why it would ever make sense to have two bonds
> between a device an an io_mm (which it doesn't!)
Yes, using ret is nicer
Thanks,
Jean
^ permalink raw reply
* [PATCH v2 02/40] iommu/sva: Bind process address spaces to devices
From: Jean-Philippe Brucker @ 2018-05-21 14:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180517141058.00001c76@huawei.com>
On 17/05/18 14:10, Jonathan Cameron wrote:
> On Fri, 11 May 2018 20:06:03 +0100
> Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:
>
>> Add bind() and unbind() operations to the IOMMU API. Bind() returns a
>> PASID that drivers can program in hardware, to let their devices access an
>> mm. This patch only adds skeletons for the device driver API, most of the
>> implementation is still missing.
>>
>> IOMMU groups with more than one device aren't supported for SVA at the
>> moment. There may be P2P traffic between devices within a group, which
>> cannot be seen by an IOMMU (note that supporting PASID doesn't add any
>> form of isolation with regard to P2P). Supporting groups would require
>> calling bind() for all bound processes every time a device is added to a
>> group, to perform sanity checks (e.g. ensure that new devices support
>> PASIDs at least as big as those already allocated in the group).
>
> Is it worth adding an explicit comment on this reasoning (or a minimal subset
> of it) at the check for the number of devices in the group?
> It's well laid out here, but might not be so obvious if someone is reading
> the code in the future.
Sure, I'll add something
Thanks,
Jean
^ permalink raw reply
* [PATCH 2/2] rtc: st-lpc: add range
From: kbuild test robot @ 2018-05-21 14:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180520123337.14856-2-alexandre.belloni@bootlin.com>
Hi Alexandre,
I love your patch! Yet something to improve:
[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v4.17-rc6 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alexandre-Belloni/rtc-st-lpc-fix-possible-race-condition/20180521-192317
base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
drivers/rtc/rtc-st-lpc.c: In function 'st_rtc_probe':
>> drivers/rtc/rtc-st-lpc.c:261:5: error: 'struct st_rtc' has no member named 'range_max'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~
In file included from arch/arm/include/asm/div64.h:127:0,
from include/linux/kernel.h:173,
from include/linux/clk.h:16,
from drivers/rtc/rtc-st-lpc.c:17:
>> include/asm-generic/div64.h:226:7: error: lvalue required as left operand of assignment
(n) >>= ilog2(__base); \
^
>> drivers/rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
>> include/asm-generic/div64.h:230:31: warning: large integer implicitly truncated to unsigned type [-Woverflow]
uint32_t __res_lo, __n_lo = (n); \
^
>> drivers/rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
include/asm-generic/div64.h:231:7: error: lvalue required as left operand of assignment
(n) = __div64_const32(n, __base); \
^
>> drivers/rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
include/asm-generic/div64.h:233:14: warning: large integer implicitly truncated to unsigned type [-Woverflow]
__res_lo = (n); \
^
>> drivers/rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
include/asm-generic/div64.h:237:7: error: lvalue required as left operand of assignment
(n) = (uint32_t)(n) / __base; \
^
>> drivers/rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
>> include/asm-generic/div64.h:239:22: error: lvalue required as unary '&' operand
__rem = __div64_32(&(n), __base); \
^
>> drivers/rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
--
drivers//rtc/rtc-st-lpc.c: In function 'st_rtc_probe':
drivers//rtc/rtc-st-lpc.c:261:5: error: 'struct st_rtc' has no member named 'range_max'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~
In file included from arch/arm/include/asm/div64.h:127:0,
from include/linux/kernel.h:173,
from include/linux/clk.h:16,
from drivers//rtc/rtc-st-lpc.c:17:
>> include/asm-generic/div64.h:226:7: error: lvalue required as left operand of assignment
(n) >>= ilog2(__base); \
^
drivers//rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
>> include/asm-generic/div64.h:230:31: warning: large integer implicitly truncated to unsigned type [-Woverflow]
uint32_t __res_lo, __n_lo = (n); \
^
drivers//rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
include/asm-generic/div64.h:231:7: error: lvalue required as left operand of assignment
(n) = __div64_const32(n, __base); \
^
drivers//rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
include/asm-generic/div64.h:233:14: warning: large integer implicitly truncated to unsigned type [-Woverflow]
__res_lo = (n); \
^
drivers//rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
include/asm-generic/div64.h:237:7: error: lvalue required as left operand of assignment
(n) = (uint32_t)(n) / __base; \
^
drivers//rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
>> include/asm-generic/div64.h:239:22: error: lvalue required as unary '&' operand
__rem = __div64_32(&(n), __base); \
^
drivers//rtc/rtc-st-lpc.c:261:19: note: in expansion of macro 'do_div'
rtc->range_max = do_div(U64_MAX, rtc->clkrate);
^~~~~~
vim +261 drivers/rtc/rtc-st-lpc.c
192
193 static int st_rtc_probe(struct platform_device *pdev)
194 {
195 struct device_node *np = pdev->dev.of_node;
196 struct st_rtc *rtc;
197 struct resource *res;
198 uint32_t mode;
199 int ret = 0;
200
201 ret = of_property_read_u32(np, "st,lpc-mode", &mode);
202 if (ret) {
203 dev_err(&pdev->dev, "An LPC mode must be provided\n");
204 return -EINVAL;
205 }
206
207 /* LPC can either run as a Clocksource or in RTC or WDT mode */
208 if (mode != ST_LPC_MODE_RTC)
209 return -ENODEV;
210
211 rtc = devm_kzalloc(&pdev->dev, sizeof(struct st_rtc), GFP_KERNEL);
212 if (!rtc)
213 return -ENOMEM;
214
215 rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
216 if (IS_ERR(rtc->rtc_dev))
217 return PTR_ERR(rtc->rtc_dev);
218
219 spin_lock_init(&rtc->lock);
220
221 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
222 rtc->ioaddr = devm_ioremap_resource(&pdev->dev, res);
223 if (IS_ERR(rtc->ioaddr))
224 return PTR_ERR(rtc->ioaddr);
225
226 rtc->irq = irq_of_parse_and_map(np, 0);
227 if (!rtc->irq) {
228 dev_err(&pdev->dev, "IRQ missing or invalid\n");
229 return -EINVAL;
230 }
231
232 ret = devm_request_irq(&pdev->dev, rtc->irq, st_rtc_handler, 0,
233 pdev->name, rtc);
234 if (ret) {
235 dev_err(&pdev->dev, "Failed to request irq %i\n", rtc->irq);
236 return ret;
237 }
238
239 enable_irq_wake(rtc->irq);
240 disable_irq(rtc->irq);
241
242 rtc->clk = clk_get(&pdev->dev, NULL);
243 if (IS_ERR(rtc->clk)) {
244 dev_err(&pdev->dev, "Unable to request clock\n");
245 return PTR_ERR(rtc->clk);
246 }
247
248 clk_prepare_enable(rtc->clk);
249
250 rtc->clkrate = clk_get_rate(rtc->clk);
251 if (!rtc->clkrate) {
252 dev_err(&pdev->dev, "Unable to fetch clock rate\n");
253 return -EINVAL;
254 }
255
256 device_set_wakeup_capable(&pdev->dev, 1);
257
258 platform_set_drvdata(pdev, rtc);
259
260 rtc->rtc_dev->ops = &st_rtc_ops;
> 261 rtc->range_max = do_div(U64_MAX, rtc->clkrate);
262
263 ret = rtc_register_device(rtc->rtc_dev);
264 if (ret) {
265 clk_disable_unprepare(rtc->clk);
266 return ret;
267 }
268
269 return 0;
270 }
271
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 43343 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180521/7bf5deb2/attachment-0001.gz>
^ permalink raw reply
* [PATCH 6/6] arm64: perf: Add support for chaining counters
From: Suzuki K Poulose @ 2018-05-21 14:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f7ffdcfb-5a32-9a55-32a5-4f8770e23aa0@arm.com>
On 21/05/18 15:00, Robin Murphy wrote:
> On 21/05/18 14:42, Suzuki K Poulose wrote:
>> On 18/05/18 16:57, Suzuki K Poulose wrote:
>>> Hi Robin,
>>>
>>> On 18/05/18 14:49, Robin Murphy wrote:
>>>> On 18/05/18 11:22, Suzuki K Poulose wrote:
>>>>> Add support for chained event counters. PMUv3 allows chaining
>>>>> a pair of adjacent PMU counters (with the lower counter number
>>>>> being always "even"). The low counter is programmed to count
>>>>> the event of interest and the high counter(odd numbered) is
>>>>> programmed with a special event code (0x1e - Chain). Thus
>>>>> we need special allocation schemes to make the full use of
>>>>> available counters. So, we allocate the counters from either
>>>>> ends. i.e, chained counters are allocated from the lower
>>>>> end in pairs of two and the normal counters are allocated
>>>>> from the higher number. Also makes necessary changes to
>>>>> handle the chained events as a single event with 2 counters.
>>>>>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>>>
>>>>> ? /*
>>>>> @@ -845,8 +1016,14 @@ static int __armv8_pmuv3_map_event(struct perf_event *event,
>>>>> ???????????????????????? &armv8_pmuv3_perf_cache_map,
>>>>> ???????????????????????? ARMV8_PMU_EVTYPE_EVENT);
>>>>> -??? if (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES)
>>>>> +??? if (hw_event_id == ARMV8_PMUV3_PERFCTR_CPU_CYCLES) {
>>>>> +??????? /* Prevent chaining for cycle counter */
>>>>
>>>> Why? Sure, we want to avoid executing the chaining logic if we're scheduling a cycles event in the dedicated counter (which is perhaps what the comment above wanted to say), but if one ends up allocated into a regular counter (e.g. if the user asks for multiple cycle counts with different filters), then I don't see any reason to forbid that being chained.
>>>
>>> Ah, I didn't think about that case. I was under the assumption that the
>>> cycles are *only* placed on the cycle counter. I will take care of that.
>>> Thanks for the review.
>>
>> Robin, Mark, Will
>>
>> One potential problem I see with allowing chaining of the cycle counter
>> *and* the promotion of cycle event to 64bit by default is when the user
>> may actually be able to count 1 less event (due to the promotion of
>> cycle event to 64bit and thus forcing to use chain, if the cycle counter
>> is unavailable).
>
> Right, I didn't mean to imply we should inject the "chain" attr automatically for all cycles events, just that we shouldn't be rejecting it if the user does explicitly set it (but then just ignore it if using the dedicated counter).
Right, I was not talking about the automatic "chain" for cycles events. The
problem is we don't know if we would get the "cycle" counter for the given
event until it is "added", at which point we have already decided
whether the event is 32bit or 64bit. So, we cannot really delay the decision
until that. Thats where this comes up. Given a cycle event (without an explicit
chain request), do we treat it as a 64bit event or not ? If we do, we could
1) get the Cycle counter, all is fine.
2) If not, fallback to Chaining. The user looses a counter.
>
>> So one option is to drop automatic promotion of the cycle counter to
>> 64bit and do it only when it is requested by the user and use either the
>> Cycle counter (preferred) or fall back to chaining. That way, the user
>> has the control over the number of events he can count using the given
>> set of counters.
>
> Naively, there doesn't seem to be any inherent harm in always using 64-bit arithmetic for the dedicated counter, but it would mean that with multiple (non-chained) cycles events, some would be taking an interrupt every few seconds while one would effectively never overflow. I guess the question is whether that matters or not.
>
The problem is we can't have a mask per counter as we don't know where
the event would be placed until it is added. Or we should delay the
period initialisation/update to post get_event_idx().
Suzuki
^ permalink raw reply
* [PATCH v9 12/16] KVM: arm64: Save host SVE context as appropriate
From: Marc Zyngier @ 2018-05-21 14:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526912237-25308-13-git-send-email-Dave.Martin@arm.com>
On 21/05/18 15:17, Dave Martin wrote:
> This patch adds SVE context saving to the hyp FPSIMD context switch
> path. This means that it is no longer necessary to save the host
> SVE state in advance of entering the guest, when in use.
>
> In order to avoid adding pointless complexity to the code, VHE is
> assumed if SVE is in use. VHE is an architectural prerequisite for
> SVE, so there is no good reason to turn CONFIG_ARM64_VHE off in
> kernels that support both SVE and KVM.
>
> Historically, software models exist that can expose the
> architecturally invalid configuration of SVE without VHE, so if
> this situation is detected at kvm_init() time then KVM will be
> disabled.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>
> ---
>
> * Stripped the following tags, reviewers please reconfirm:
>
> Formerly-Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
> Formerly-Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Formerly-Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>
> (Creation of a new file for this one change may be deemed undesirable,
> but there didn't seem to be a correct place to put it. There may also
> be a way around the circular include problem that I missed.)
>
> Changes since v8:
>
> * Add kvm_arch_check_supported() hook, and move arm64-specific check
> for SVE-implies-VHE into arch/arm64/.
>
> Due to circular header dependency problems, it is difficult to get
> the prototype for kvm_pr_*() functions in <asm/kvm_host.h>, so this
> patch puts arm64's kvm_arch_check_supported() hook out of line.
> This is not a hot function.
> ---
> arch/arm/include/asm/kvm_host.h | 1 +
> arch/arm64/Kconfig | 7 +++++++
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/Makefile | 2 +-
> arch/arm64/kvm/fpsimd.c | 1 -
> arch/arm64/kvm/hyp/switch.c | 20 +++++++++++++++++++-
> arch/arm64/kvm/init.c | 33 +++++++++++++++++++++++++++++++++
> virt/kvm/arm/arm.c | 6 ++++++
> 8 files changed, 68 insertions(+), 3 deletions(-)
> create mode 100644 arch/arm64/kvm/init.c
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index ac870b2..e627ef8 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -280,6 +280,7 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
>
> struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>
> +static inline int kvm_arch_check_supported(void) { return 0; }
> static inline void kvm_arch_hardware_unsetup(void) {}
> static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index eb2cf49..b0d3820 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1130,6 +1130,7 @@ endmenu
> config ARM64_SVE
> bool "ARM Scalable Vector Extension support"
> default y
> + depends on !KVM || ARM64_VHE
> help
> The Scalable Vector Extension (SVE) is an extension to the AArch64
> execution state which complements and extends the SIMD functionality
> @@ -1155,6 +1156,12 @@ config ARM64_SVE
> booting the kernel. If unsure and you are not observing these
> symptoms, you should assume that it is safe to say Y.
>
> + CPUs that support SVE are architecturally required to support the
> + Virtualization Host Extensions (VHE), so the kernel makes no
> + provision for supporting SVE alongside KVM without VHE enabled.
> + Thus, you will need to enable CONFIG_ARM64_VHE if you want to support
> + KVM in the same kernel image.
> +
> config ARM64_MODULE_PLTS
> bool
> select HAVE_MOD_ARCH_SPECIFIC
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index b3fe730..80f3985 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -405,6 +405,7 @@ static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
> kvm_call_hyp(__kvm_set_tpidr_el2, tpidr_el2);
> }
>
> +int kvm_arch_check_supported(void);
> static inline void kvm_arch_hardware_unsetup(void) {}
> static inline void kvm_arch_sync_events(struct kvm *kvm) {}
> static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 0f2a135..5e66afe 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -17,7 +17,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o $(KVM)/arm/mmio.
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>
> kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
> -kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += init.o hyp.o hyp-init.o handle_exit.o
> kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o
> kvm-$(CONFIG_KVM_ARM_HOST) += vgic-sys-reg-v3.o fpsimd.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index d9b5f73..52185ec 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -59,7 +59,6 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
> */
> void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> {
> - BUG_ON(system_supports_sve());
> BUG_ON(!current->mm);
>
> vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED | KVM_ARM64_HOST_SVE_IN_USE);
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 118f300..a6a8c7d 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -21,6 +21,7 @@
>
> #include <kvm/arm_psci.h>
>
> +#include <asm/cpufeature.h>
> #include <asm/kvm_asm.h>
> #include <asm/kvm_emulate.h>
> #include <asm/kvm_host.h>
> @@ -28,6 +29,7 @@
> #include <asm/kvm_mmu.h>
> #include <asm/fpsimd.h>
> #include <asm/debug-monitors.h>
> +#include <asm/processor.h>
> #include <asm/thread_info.h>
>
> /* Check whether the FP regs were dirtied while in the host-side run loop: */
> @@ -329,6 +331,8 @@ static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> struct kvm_vcpu *vcpu)
> {
> + struct user_fpsimd_state *host_fpsimd = vcpu->arch.host_fpsimd_state;
> +
> if (has_vhe())
> write_sysreg(read_sysreg(cpacr_el1) | CPACR_EL1_FPEN,
> cpacr_el1);
> @@ -339,7 +343,21 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
> isb();
>
> if (vcpu->arch.flags & KVM_ARM64_FP_HOST) {
> - __fpsimd_save_state(vcpu->arch.host_fpsimd_state);
> + /*
> + * In the SVE case, VHE is assumed: it is enforced by
> + * Kconfig and kvm_arch_init().
> + */
> + if (system_supports_sve() &&
> + (vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE)) {
> + struct thread_struct *thread = container_of(
> + host_fpsimd,
> + struct thread_struct, uw.fpsimd_state);
> +
> + sve_save_state(sve_pffr(thread), &host_fpsimd->fpsr);
> + } else {
> + __fpsimd_save_state(host_fpsimd);
> + }
> +
> vcpu->arch.flags &= ~KVM_ARM64_FP_HOST;
> }
>
> diff --git a/arch/arm64/kvm/init.c b/arch/arm64/kvm/init.c
> new file mode 100644
> index 0000000..3b6e730
> --- /dev/null
> +++ b/arch/arm64/kvm/init.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * arch/arm64/kvm/init.c: KVM initialisation support
> + *
> + * Copyright 2018 Arm Limited
> + * Author: Dave Martin <Dave.Martin@arm.com>
> + */
> +#include <linux/errno.h>
> +#include <linux/kvm_host.h>
> +#include <asm/cpufeature.h>
> +#include <asm/kvm_host.h>
> +
> +/* Additional arch-dependent checks for KVM usability */
> +int kvm_arch_check_supported(void)
> +{
> + /*
> + * VHE is a prerequisite for SVE in the Arm architecture, and
> + * Kconfig ensures that if system_supports_sve() here then
> + * CONFIG_ARM64_VHE is enabled, so if VHE support wasn't already
> + * detected and enabled, the CPU is architecturally
> + * noncompliant.
> + *
> + * Just in case this mismatch is seen, detect it, warn and give
> + * up. Supporting this forbidden configuration in Hyp would be
> + * pointless.
> + */
> + if (system_supports_sve() && !has_vhe()) {
> + kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
[entering bikeshedding territory]
I'm not exactly keen on this. This feels overkill, and a departure from
what we've been doing so far.
Why don't you simply have a helper in kvm_host.h that does:
static inline bool kvm_arm_check_sve_valid(void)
{
return !system_supports_sve() || has_vhe();
}
(and a canonical "return true;" implementation for 32bit)
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index bee226c..8518df0 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -16,6 +16,7 @@
> * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> */
>
> +#include <linux/bug.h>
> #include <linux/cpu_pm.h>
> #include <linux/errno.h>
> #include <linux/err.h>
> @@ -41,6 +42,7 @@
> #include <asm/mman.h>
> #include <asm/tlbflush.h>
> #include <asm/cacheflush.h>
> +#include <asm/cpufeature.h>
> #include <asm/virt.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_asm.h>
> @@ -1574,6 +1576,10 @@ int kvm_arch_init(void *opaque)
> return -ENODEV;
> }
>
> + err = kvm_arch_check_supported();
> + if (err)
> + return err;
> +
And turn this into:
if (!kvm_arm_check_sve_valid()) {
kvm_pr_unimpl("SVE system without VHE unsupported. Broken cpu?");
return -EINVAL;
}
It has the advantage of being obvious of what we check for, and doesn't
add any extra file.
Thoughts?
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v9 16/16] KVM: arm64: Invoke FPSIMD context switch trap from C
From: Dave Martin @ 2018-05-21 14:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526912237-25308-1-git-send-email-Dave.Martin@arm.com>
The conversion of the FPSIMD context switch trap code to C has added
some overhead to calling it, due to the need to save registers that
the procedure call standard defines as caller-saved.
So, perhaps it is no longer worth invoking this trap handler quite
so early.
Instead, we can invoke it from fixup_guest_exit(), with little
likelihood of increasing the overhead much further.
As a convenience, this patch gives __hyp_switch_fpsimd() the same
return semantics fixup_guest_exit(). For now there is no
possibility of a spurious FPSIMD trap, so the function always
returns true, but this allows it to be tail-called with a single
return statement.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
---
arch/arm64/kvm/hyp/entry.S | 30 ------------------------------
arch/arm64/kvm/hyp/hyp-entry.S | 19 -------------------
arch/arm64/kvm/hyp/switch.c | 15 +++++++++++++--
3 files changed, 13 insertions(+), 51 deletions(-)
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 40f349b..fad1e16 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -166,33 +166,3 @@ abort_guest_exit_end:
orr x0, x0, x5
1: ret
ENDPROC(__guest_exit)
-
-ENTRY(__fpsimd_guest_restore)
- // x0: esr
- // x1: vcpu
- // x2-x29,lr: vcpu regs
- // vcpu x0-x1 on the stack
- stp x2, x3, [sp, #-144]!
- stp x4, x5, [sp, #16]
- stp x6, x7, [sp, #32]
- stp x8, x9, [sp, #48]
- stp x10, x11, [sp, #64]
- stp x12, x13, [sp, #80]
- stp x14, x15, [sp, #96]
- stp x16, x17, [sp, #112]
- stp x18, lr, [sp, #128]
-
- bl __hyp_switch_fpsimd
-
- ldp x4, x5, [sp, #16]
- ldp x6, x7, [sp, #32]
- ldp x8, x9, [sp, #48]
- ldp x10, x11, [sp, #64]
- ldp x12, x13, [sp, #80]
- ldp x14, x15, [sp, #96]
- ldp x16, x17, [sp, #112]
- ldp x18, lr, [sp, #128]
- ldp x0, x1, [sp, #144]
- ldp x2, x3, [sp], #160
- eret
-ENDPROC(__fpsimd_guest_restore)
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index bffece2..753b9d2 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -113,25 +113,6 @@ el1_hvc_guest:
el1_trap:
get_vcpu_ptr x1, x0
-
- mrs x0, esr_el2
- lsr x0, x0, #ESR_ELx_EC_SHIFT
- /*
- * x0: ESR_EC
- * x1: vcpu pointer
- */
-
- /*
- * 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 x0, #ESR_ELx_EC_FP_ASIMD
- b.eq __fpsimd_guest_restore
-alternative_else_nop_endif
-
mov x0, #ARM_EXCEPTION_TRAP
b __guest_exit
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 4fbee95..2d45bd7 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -328,8 +328,7 @@ static bool __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
}
}
-void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
- struct kvm_vcpu *vcpu)
+static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
{
struct user_fpsimd_state *host_fpsimd = vcpu->arch.host_fpsimd_state;
@@ -369,6 +368,8 @@ void __hyp_text __hyp_switch_fpsimd(u64 esr __always_unused,
fpexc32_el2);
vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
+
+ return true;
}
/*
@@ -390,6 +391,16 @@ static bool __hyp_text fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
if (*exit_code != ARM_EXCEPTION_TRAP)
goto exit;
+ /*
+ * 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.
+ */
+ if (system_supports_fpsimd() &&
+ kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_FP_ASIMD)
+ return __hyp_switch_fpsimd(vcpu);
+
if (!__populate_fault_info(vcpu))
return true;
--
2.1.4
^ permalink raw reply related
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