* [PATCH v5] drm/mediatek: fixed the calc method of data rate per lane
From: Daniel Kurtz @ 2016-11-18 3:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479361006.13083.7.camel@mtksdaap41>
Hi CK,
On Thu, Nov 17, 2016 at 1:36 PM, CK Hu <ck.hu@mediatek.com> wrote:
> Hi, Jitao:
>
>
> On Wed, 2016-11-16 at 11:20 +0800, Jitao Shi wrote:
>> Tune dsi frame rate by pixel clock, dsi add some extra signal (i.e.
>> Tlpx, Ths-prepare, Ths-zero, Ths-trail,Ths-exit) when enter and exit LP
>> mode, those signals will cause h-time larger than normal and reduce FPS.
>> So need to multiply a coefficient to offset the extra signal's effect.
>> coefficient = ((htotal*bpp/lane_number)+Tlpx+Ths_prep+Ths_zero+
>> Ths_trail+Ths_exit)/(htotal*bpp/lane_number)
>>
>> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
>
> It looks good to me.
> But this patch conflict with [1] which is one patch of MT2701 series. I
> want to apply MT2701 patches first, so please help to refine this patch
> based on MT2701 patches.
I don't think the MT2701 DSI patches are quite ready yet (I just
reviewed the one below).
Can we instead land Jitao's small targeted change first, and then
rebase the MT2701 series on top.
Thanks,
-Dan
>
> [1] https://patchwork.kernel.org/patch/9422821/
>
> Regards,
> CK
>
>> ---
>> Change since v4:
>> - tune the calc comment more clear.
>> - define the phy timings as constants.
>>
>> Chnage since v3:
>> - wrapp the commit msg.
>> - fix alignment of some lines.
>>
>> Change since v2:
>> - move phy timing back to dsi_phy_timconfig.
>>
>> Change since v1:
>> - phy_timing2 and phy_timing3 refer clock cycle time.
>> - define values of LPX HS_PRPR HS_ZERO HS_TRAIL TA_GO TA_SURE TA_GET DA_HS_EXIT.
>> ---
>>
>
^ permalink raw reply
* [PATCH v9 09/10] drm/mediatek: update DSI sub driver flow for sending commands to panel
From: Daniel Kurtz @ 2016-11-18 3:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478865346-19043-10-git-send-email-yt.shen@mediatek.com>
Hi YT,
Sorry for the very late review.
My biggest problem with this patch is it describes itself as adding
support for a new use case "DSI -> panel", but makes many changes to
the existing working flow "DSI -> bridge -> panel".
If these changes are really needed, or improve the existing flow, I'd
expect to see those changes added first in a preparatory patch,
followed by a second smaller, simpler
patch that adds any additional functionality required to enable the new flow.
See detailed comments inline.
On Fri, Nov 11, 2016 at 7:55 PM, YT Shen <yt.shen@mediatek.com> wrote:
>
> This patch update enable/disable flow of DSI module and MIPI TX module.
> Original flow works on there is a bridge chip: DSI -> bridge -> panel.
> In this case: DSI -> panel, the DSI sub driver flow should be updated.
> We need to initialize DSI first so that we can send commands to panel.
>
> Signed-off-by: shaoming chen <shaoming.chen@mediatek.com>
> Signed-off-by: YT Shen <yt.shen@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 110 ++++++++++++++++++++++++++-------
> drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 32 +++++-----
> 2 files changed, 103 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 860b84f..12a1206 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -94,6 +94,8 @@
> #define DSI_RACK 0x84
> #define RACK BIT(0)
>
> +#define DSI_MEM_CONTI 0x90
> +
> #define DSI_PHY_LCCON 0x104
> #define LC_HS_TX_EN BIT(0)
> #define LC_ULPM_EN BIT(1)
> @@ -126,6 +128,10 @@
> #define CLK_HS_POST (0xff << 8)
> #define CLK_HS_EXIT (0xff << 16)
>
> +#define DSI_VM_CMD_CON 0x130
> +#define VM_CMD_EN BIT(0)
> +#define TS_VFP_EN BIT(5)
> +
> #define DSI_CMDQ0 0x180
> #define CONFIG (0xff << 0)
> #define SHORT_PACKET 0
> @@ -219,12 +225,12 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
> writel(timcon3, dsi->regs + DSI_PHY_TIMECON3);
> }
>
> -static void mtk_dsi_enable(struct mtk_dsi *dsi)
> +static void mtk_dsi_engine_enable(struct mtk_dsi *dsi)
I don't think we need to change these names.
> {
> mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, DSI_EN);
> }
>
> -static void mtk_dsi_disable(struct mtk_dsi *dsi)
> +static void mtk_dsi_engine_disable(struct mtk_dsi *dsi)
> {
> mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, 0);
> }
> @@ -249,7 +255,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> * mipi_ratio is mipi clk coefficient for balance the pixel clk in mipi.
> * we set mipi_ratio is 1.05.
> */
> - dsi->data_rate = dsi->vm.pixelclock * 3 * 21 / (1 * 1000 * 10);
> + dsi->data_rate = dsi->vm.pixelclock * 12 * 21;
> + dsi->data_rate /= (dsi->lanes * 1000 * 10);
> + dev_info(dev, "set mipitx's data rate: %dMHz\n", dsi->data_rate);
I don't think we want to spam the log like this. Use dev_dbg.... or
use the DRM_() messaging like elsewhere in this driver?
>
> ret = clk_set_rate(dsi->hs_clk, dsi->data_rate * 1000000);
> if (ret < 0) {
> @@ -271,7 +279,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> goto err_disable_engine_clk;
> }
>
> - mtk_dsi_enable(dsi);
> + mtk_dsi_engine_enable(dsi);
> mtk_dsi_reset_engine(dsi);
> mtk_dsi_phy_timconfig(dsi);
>
> @@ -289,7 +297,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> static void mtk_dsi_clk_ulp_mode_enter(struct mtk_dsi *dsi)
> {
> mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_HS_TX_EN, 0);
> - mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, 0);
> + mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, LC_ULPM_EN);
What does this change do?
It looks like a pure bug fix (ie, previoulsy we were'nt actually
enabling ULP MODE before).
If so, can you please move it to a separate preliminary patch.
> }
>
> static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi)
> @@ -302,7 +310,7 @@ static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi)
> static void mtk_dsi_lane0_ulp_mode_enter(struct mtk_dsi *dsi)
> {
> mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_HS_TX_EN, 0);
> - mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, 0);
> + mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, LD0_ULPM_EN);
Same here.
> }
>
> static void mtk_dsi_lane0_ulp_mode_leave(struct mtk_dsi *dsi)
> @@ -338,11 +346,21 @@ static void mtk_dsi_set_mode(struct mtk_dsi *dsi)
> if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) &&
> !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE))
> vid_mode = BURST_MODE;
> + else
> + vid_mode = SYNC_EVENT_MODE;
So, when do we use SYNC_PULSE_MODE (set just before the 'if')?
> }
>
> writel(vid_mode, dsi->regs + DSI_MODE_CTRL);
> }
>
> +static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
> +{
> + writel(0x3c, dsi->regs + DSI_MEM_CONTI);
Please use #defined constants, especially if this register is a bit field.
Also, this looks like new behavior which doesn't seem related to
changing the enable order.
If this is a general fix, please use a separate patch.
> +
> + mtk_dsi_mask(dsi, DSI_VM_CMD_CON, VM_CMD_EN, VM_CMD_EN);
> + mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
> +}
> +
> static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
> {
> struct videomode *vm = &dsi->vm;
> @@ -399,6 +417,9 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
> break;
> }
>
> + tmp_reg |= (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) << 6;
> + tmp_reg |= (dsi->mode_flags & MIPI_DSI_MODE_EOT_PACKET) >> 3;
> +
ditto
> writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
> }
>
> @@ -477,6 +498,16 @@ static void mtk_dsi_start(struct mtk_dsi *dsi)
> writel(1, dsi->regs + DSI_START);
> }
>
> +static void mtk_dsi_stop(struct mtk_dsi *dsi)
> +{
> + writel(0, dsi->regs + DSI_START);
> +}
> +
> +static void mtk_dsi_set_cmd_mode(struct mtk_dsi *dsi)
> +{
> + writel(CMD_MODE, dsi->regs + DSI_MODE_CTRL);
> +}
> +
> static void mtk_dsi_set_interrupt_enable(struct mtk_dsi *dsi)
> {
> u32 inten = LPRX_RD_RDY_INT_FLAG | CMD_DONE_INT_FLAG | VM_DONE_INT_FLAG;
> @@ -506,7 +537,7 @@ static s32 mtk_dsi_wait_for_irq_done(struct mtk_dsi *dsi, u32 irq_flag,
> if (ret == 0) {
> dev_info(dsi->dev, "Wait DSI IRQ(0x%08x) Timeout\n", irq_flag);
>
> - mtk_dsi_enable(dsi);
> + mtk_dsi_engine_enable(dsi);
> mtk_dsi_reset_engine(dsi);
> }
>
> @@ -535,6 +566,17 @@ static irqreturn_t mtk_dsi_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static s32 mtk_dsi_switch_to_cmd_mode(struct mtk_dsi *dsi, u8 irq_flag, u32 t)
> +{
> + mtk_dsi_irq_data_clear(dsi, irq_flag);
> + mtk_dsi_set_cmd_mode(dsi);
> +
> + if (!mtk_dsi_wait_for_irq_done(dsi, irq_flag, t))
> + return -1;
No, use a real linux errno, and return an int, and print an error
message if this is unexpected.
> + else
> + return 0;
> +}
> +
> static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
> {
> if (WARN_ON(dsi->refcount == 0))
> @@ -543,11 +585,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
> if (--dsi->refcount != 0)
> return;
>
> - mtk_dsi_lane0_ulp_mode_enter(dsi);
> - mtk_dsi_clk_ulp_mode_enter(dsi);
> -
> - mtk_dsi_disable(dsi);
> -
> clk_disable_unprepare(dsi->engine_clk);
> clk_disable_unprepare(dsi->digital_clk);
>
> @@ -561,35 +598,45 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> if (dsi->enabled)
> return;
>
> - if (dsi->panel) {
> - if (drm_panel_prepare(dsi->panel)) {
> - DRM_ERROR("failed to setup the panel\n");
> - return;
> - }
> - }
> -
> ret = mtk_dsi_poweron(dsi);
> if (ret < 0) {
> DRM_ERROR("failed to power on dsi\n");
> return;
> }
>
> + usleep_range(20000, 21000);
> +
Why are you adding a 20 ms delay where there was none before?
> mtk_dsi_rxtx_control(dsi);
> + mtk_dsi_phy_timconfig(dsi);
> + mtk_dsi_ps_control_vact(dsi);
> + mtk_dsi_set_vm_cmd(dsi);
> + mtk_dsi_config_vdo_timing(dsi);
> + mtk_dsi_set_interrupt_enable(dsi);
>
> + mtk_dsi_engine_enable(dsi);
> mtk_dsi_clk_ulp_mode_leave(dsi);
> mtk_dsi_lane0_ulp_mode_leave(dsi);
> mtk_dsi_clk_hs_mode(dsi, 0);
> - mtk_dsi_set_mode(dsi);
>
> - mtk_dsi_ps_control_vact(dsi);
> - mtk_dsi_config_vdo_timing(dsi);
> - mtk_dsi_set_interrupt_enable(dsi);
> + if (dsi->panel) {
> + if (drm_panel_prepare(dsi->panel)) {
> + DRM_ERROR("failed to prepare the panel\n");
> + return;
> + }
> + }
>
> mtk_dsi_set_mode(dsi);
> mtk_dsi_clk_hs_mode(dsi, 1);
>
> mtk_dsi_start(dsi);
>
> + if (dsi->panel) {
> + if (drm_panel_enable(dsi->panel)) {
> + DRM_ERROR("failed to enable the panel\n");
In case of error, you must undo everything done to this point. At least:
(1) unprepare the panel
(2) stop dsi
(3) poweroff dsi
> + return;
> + }
> + }
> +
> dsi->enabled = true;
> }
>
> @@ -605,6 +652,21 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
> }
> }
>
> + mtk_dsi_stop(dsi);
> + mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
This function can return an error, so please check it. Although,
there probably isn't much you can do here about it.
> +
> + if (dsi->panel) {
> + if (drm_panel_unprepare(dsi->panel)) {
> + DRM_ERROR("failed to unprepare the panel\n");
> + return;
I think you should probably just ignore this error and continue
disabling dsi, since it isn't really recoverable and you can't roll
back and re-enable dsi.
> + }
> + }
> +
> + mtk_dsi_reset_engine(dsi);
> + mtk_dsi_lane0_ulp_mode_enter(dsi);
> + mtk_dsi_clk_ulp_mode_enter(dsi);
> + mtk_dsi_engine_disable(dsi);
> +
> mtk_dsi_poweroff(dsi);
>
> dsi->enabled = false;
> @@ -845,7 +907,7 @@ static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi)
> if (timeout_ms == 0) {
> dev_info(dsi->dev, "polling dsi wait not busy timeout!\n");
>
> - mtk_dsi_enable(dsi);
> + mtk_dsi_engine_enable(dsi);
> mtk_dsi_reset_engine(dsi);
> }
> }
> diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> index 108d31a..34e95c6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> @@ -177,7 +177,9 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
>
> dev_dbg(mipi_tx->dev, "prepare: %u Hz\n", mipi_tx->data_rate);
>
> - if (mipi_tx->data_rate >= 500000000) {
> + if (mipi_tx->data_rate > 1250000000) {
> + return -EINVAL;
> + } else if (mipi_tx->data_rate >= 500000000) {
Capping the max data rate looks like an unrelated fix.
> txdiv = 1;
> txdiv0 = 0;
> txdiv1 = 0;
> @@ -201,6 +203,10 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
> return -EINVAL;
> }
>
> + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_TOP_CON,
> + RG_DSI_LNT_IMP_CAL_CODE | RG_DSI_LNT_HS_BIAS_EN,
> + (8 << 4) | RG_DSI_LNT_HS_BIAS_EN);
> +
> mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_BG_CON,
> RG_DSI_VOUT_MSK |
> RG_DSI_BG_CKEN | RG_DSI_BG_CORE_EN,
> @@ -210,24 +216,18 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
>
> usleep_range(30, 100);
>
> - mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_TOP_CON,
> - RG_DSI_LNT_IMP_CAL_CODE | RG_DSI_LNT_HS_BIAS_EN,
> - (8 << 4) | RG_DSI_LNT_HS_BIAS_EN);
> -
> - mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_CON,
> - RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN);
> + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_CON,
> + RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN,
> + RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN);
Changing from set_bits to update_bits does not do anything. Please
leave this alone.
>
> mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_PWR,
> RG_DSI_MPPLL_SDM_PWR_ON |
> RG_DSI_MPPLL_SDM_ISO_EN,
> RG_DSI_MPPLL_SDM_PWR_ON);
>
> - mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
> - RG_DSI_MPPLL_PLL_EN);
> -
Why don't you need to disable the PLL first now?
> mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
> - RG_DSI_MPPLL_TXDIV0 | RG_DSI_MPPLL_TXDIV1 |
> - RG_DSI_MPPLL_PREDIV,
> + RG_DSI_MPPLL_PREDIV | RG_DSI_MPPLL_TXDIV0 |
> + RG_DSI_MPPLL_TXDIV1 | RG_DSI_MPPLL_POSDIV,
> (txdiv0 << 3) | (txdiv1 << 5));
If I read this right, the only thing you are changing is clearing
"RG_DSI_MPPLL_POSDIV".
This would be more clear if you kept the field order: TXDIV0, TXDIV1, PREDIV.
And why are you making this change in this patch?
>
> /*
> @@ -242,10 +242,12 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
> 26000000);
> writel(pcw, mipi_tx->regs + MIPITX_DSI_PLL_CON2);
>
> - mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_PLL_CON1,
> - RG_DSI_MPPLL_SDM_FRA_EN);
> + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON1,
> + RG_DSI_MPPLL_SDM_FRA_EN,
> + RG_DSI_MPPLL_SDM_FRA_EN);
AFAICT, this change does not do anything but make the code more confusing.
>
> - mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_PLL_CON0, RG_DSI_MPPLL_PLL_EN);
> + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
> + RG_DSI_MPPLL_PLL_EN, RG_DSI_MPPLL_PLL_EN);
AFAICT, this change does not do anything but make the code more confusing.
>
> usleep_range(20, 100);
>
> --
> 1.9.1
>
^ permalink raw reply
* [PATCH 1/2] phy: rockchip-inno-usb2: fix uninitialized tmout variable
From: wlf @ 2016-11-18 3:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161116142259.2123506-1-arnd@arndb.de>
Hi Arnd,
? 2016?11?16? 22:22, Arnd Bergmann ??:
> The newly added OTG support has an obvious uninitialized variable
> access that gcc warns about:
>
> drivers/phy/phy-rockchip-inno-usb2.c: In function 'rockchip_chg_detect_work':
> drivers/phy/phy-rockchip-inno-usb2.c:717:7: error: 'tmout' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This replaces the use of the uninitialized variable with what
> the value was in the previous USB_CHG_STATE_WAIT_FOR_DCD
> state.
>
> Fixes: 0c42fe48fd23 ("phy: rockchip-inno-usb2: support otg-port for rk3399")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c
> index eb89de59b68f..2f99ec95079c 100644
> --- a/drivers/phy/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
> @@ -714,7 +714,7 @@ static void rockchip_chg_detect_work(struct work_struct *work)
> delay = CHG_SECONDARY_DET_TIME;
> rphy->chg_state = USB_CHG_STATE_PRIMARY_DONE;
> } else {
> - if (tmout) {
> + if (rphy->dcd_retries == CHG_DCD_MAX_RETRIES) {
> /* floating charger found */
> rphy->chg_type = POWER_SUPPLY_TYPE_USB_DCP;
> rphy->chg_state = USB_CHG_STATE_DETECTED;
Thanks very much for your help.
Reviewed-by: William Wu <wulf@rock-chips.com>
Best Regards,
wulf
^ permalink raw reply
* System/uncore PMUs and unit aggregation
From: Leeder, Neil @ 2016-11-18 3:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117181708.GT22855@arm.com>
Thanks for opening up the discussion on this Will.
For the Qualcomm L2 driver, one objection I had to exposing each unit is
that there are so many of them - the minimum starting point is a dozen,
so trying to start 9 counters on each means a perf command line
specifying 100+ events. Future chips are only likely to increase that.
There is a single CPU node so from an end-user perspective it would
seems to make sense to also have a single L2 node. perf already has the
ability to create events on multiple units using cpumask, aggregate the
results, and split them out per unit with perf stat -a -A, so the user
can get that granularity. Exposing separate units would make userspace
duplicate a lot of that functionality. This may rely on each uncore unit
being associated with a CPU, which is the case with L2.
I agree with your comments regarding groups and I can see that a
standard way of representing topology could be useful - in this case,
which CPUs are within the same L2 cluster. Perhaps a list of cpumasks,
one per L2 unit.
On 11/17/2016 1:17 PM, Will Deacon wrote:
[...]
> 3. Summing the counters across units is only permitted if the units
> can all be started and stopped atomically. Otherwise, the counters
> should be exposed individually. It's up to the driver author to
> decide what makes sense to sum.
If I understand your your point 3 correctly, I'm not sure about the need
to start and stop them all atomically. That seems to be a tighter
requirement than we require for CPU PMUs. For them, perf stat -a creates
events/groups on each CPU, then starts and stops them sequentially and
sums the results. If that model is acceptable for the CPU to collect and
aggregate counts, that should be the same bar that uncore PMUs need to
reach. In the L2 case, the driver isn't summing the results, it's still
perf doing it, so I may be misinterpreting your comment about where the
summation is permitted.
Thanks,
Neil
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH V8 3/3] stm: Mark the functions of writing buffer with notrace
From: Chunyan Zhang @ 2016-11-18 3:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479438454-28650-1-git-send-email-zhang.chunyan@linaro.org>
If CONFIG_STM_SOURCE_FTRACE is selected, Function trace data can be writen
to sink via STM, all functions that related to writing data packets to
STM should be marked 'notrace' to avoid being traced by Ftrace, otherwise
the program would stall into an endless loop.
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
---
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
drivers/hwtracing/intel_th/sth.c | 11 +++++++----
drivers/hwtracing/stm/core.c | 7 ++++---
drivers/hwtracing/stm/dummy_stm.c | 2 +-
include/linux/stm.h | 4 ++--
5 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 49e0f1b..b7543bd 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -406,7 +406,7 @@ static long stm_generic_set_options(struct stm_data *stm_data,
return 0;
}
-static ssize_t stm_generic_packet(struct stm_data *stm_data,
+static ssize_t notrace stm_generic_packet(struct stm_data *stm_data,
unsigned int master,
unsigned int channel,
unsigned int packet,
diff --git a/drivers/hwtracing/intel_th/sth.c b/drivers/hwtracing/intel_th/sth.c
index e1aee61..b034446 100644
--- a/drivers/hwtracing/intel_th/sth.c
+++ b/drivers/hwtracing/intel_th/sth.c
@@ -67,10 +67,13 @@ static void sth_iowrite(void __iomem *dest, const unsigned char *payload,
}
}
-static ssize_t sth_stm_packet(struct stm_data *stm_data, unsigned int master,
- unsigned int channel, unsigned int packet,
- unsigned int flags, unsigned int size,
- const unsigned char *payload)
+static ssize_t notrace sth_stm_packet(struct stm_data *stm_data,
+ unsigned int master,
+ unsigned int channel,
+ unsigned int packet,
+ unsigned int flags,
+ unsigned int size,
+ const unsigned char *payload)
{
struct sth_device *sth = container_of(stm_data, struct sth_device, stm);
struct intel_th_channel __iomem *out =
diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 51f81d6..37d3bcb 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -425,7 +425,7 @@ static int stm_file_assign(struct stm_file *stmf, char *id, unsigned int width)
return ret;
}
-static ssize_t stm_write(struct stm_data *data, unsigned int master,
+static ssize_t notrace stm_write(struct stm_data *data, unsigned int master,
unsigned int channel, const char *buf, size_t count)
{
unsigned int flags = STP_PACKET_TIMESTAMPED;
@@ -1121,8 +1121,9 @@ void stm_source_unregister_device(struct stm_source_data *data)
}
EXPORT_SYMBOL_GPL(stm_source_unregister_device);
-int stm_source_write(struct stm_source_data *data, unsigned int chan,
- const char *buf, size_t count)
+int notrace stm_source_write(struct stm_source_data *data,
+ unsigned int chan,
+ const char *buf, size_t count)
{
struct stm_source_device *src = data->src;
struct stm_device *stm;
diff --git a/drivers/hwtracing/stm/dummy_stm.c b/drivers/hwtracing/stm/dummy_stm.c
index a86612d..c5f94ca 100644
--- a/drivers/hwtracing/stm/dummy_stm.c
+++ b/drivers/hwtracing/stm/dummy_stm.c
@@ -21,7 +21,7 @@
#include <linux/slab.h>
#include <linux/stm.h>
-static ssize_t
+static ssize_t notrace
dummy_stm_packet(struct stm_data *stm_data, unsigned int master,
unsigned int channel, unsigned int packet, unsigned int flags,
unsigned int size, const unsigned char *payload)
diff --git a/include/linux/stm.h b/include/linux/stm.h
index 8369d8a..210ff22 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -133,7 +133,7 @@ int stm_source_register_device(struct device *parent,
struct stm_source_data *data);
void stm_source_unregister_device(struct stm_source_data *data);
-int stm_source_write(struct stm_source_data *data, unsigned int chan,
- const char *buf, size_t count);
+int notrace stm_source_write(struct stm_source_data *data, unsigned int chan,
+ const char *buf, size_t count);
#endif /* _STM_H_ */
--
2.7.4
^ permalink raw reply related
* [PATCH V8 2/3] stm class: ftrace: Add ftrace-export-over-stm driver
From: Chunyan Zhang @ 2016-11-18 3:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479438454-28650-1-git-send-email-zhang.chunyan@linaro.org>
This patch adds a driver that models itself as an stm_source called
stm_ftrace. Once the stm device and stm_ftrace have been linked via
sysfs, the driver registers itself as a trace_export and everything
passed to the interface from Ftrace subsystem will end up in the STM
trace engine.
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
drivers/hwtracing/stm/Kconfig | 11 ++++++
drivers/hwtracing/stm/Makefile | 2 +
drivers/hwtracing/stm/ftrace.c | 87 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 100 insertions(+)
create mode 100644 drivers/hwtracing/stm/ftrace.c
diff --git a/drivers/hwtracing/stm/Kconfig b/drivers/hwtracing/stm/Kconfig
index 847a39b..723e2d9 100644
--- a/drivers/hwtracing/stm/Kconfig
+++ b/drivers/hwtracing/stm/Kconfig
@@ -39,4 +39,15 @@ config STM_SOURCE_HEARTBEAT
If you want to send heartbeat messages over STM devices,
say Y.
+config STM_SOURCE_FTRACE
+ tristate "Copy the output from kernel Ftrace to STM engine"
+ depends on FUNCTION_TRACER
+ help
+ This option can be used to copy the output from kernel Ftrace
+ to STM engine. Enabling this option will introduce a slight
+ timing effect.
+
+ If you want to send kernel Ftrace messages over STM devices,
+ say Y.
+
endif
diff --git a/drivers/hwtracing/stm/Makefile b/drivers/hwtracing/stm/Makefile
index a9ce3d4..3abd84c 100644
--- a/drivers/hwtracing/stm/Makefile
+++ b/drivers/hwtracing/stm/Makefile
@@ -6,6 +6,8 @@ obj-$(CONFIG_STM_DUMMY) += dummy_stm.o
obj-$(CONFIG_STM_SOURCE_CONSOLE) += stm_console.o
obj-$(CONFIG_STM_SOURCE_HEARTBEAT) += stm_heartbeat.o
+obj-$(CONFIG_STM_SOURCE_FTRACE) += stm_ftrace.o
stm_console-y := console.o
stm_heartbeat-y := heartbeat.o
+stm_ftrace-y := ftrace.o
diff --git a/drivers/hwtracing/stm/ftrace.c b/drivers/hwtracing/stm/ftrace.c
new file mode 100644
index 0000000..bd126a7
--- /dev/null
+++ b/drivers/hwtracing/stm/ftrace.c
@@ -0,0 +1,87 @@
+/*
+ * Simple kernel driver to link kernel Ftrace and an STM device
+ * Copyright (c) 2016, Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * STM Ftrace will be registered as a trace_export.
+ */
+
+#include <linux/module.h>
+#include <linux/stm.h>
+#include <linux/trace.h>
+
+#define STM_FTRACE_NR_CHANNELS 1
+#define STM_FTRACE_CHAN 0
+
+static int stm_ftrace_link(struct stm_source_data *data);
+static void stm_ftrace_unlink(struct stm_source_data *data);
+
+static struct stm_ftrace {
+ struct stm_source_data data;
+ struct trace_export ftrace;
+} stm_ftrace = {
+ .data = {
+ .name = "ftrace",
+ .nr_chans = STM_FTRACE_NR_CHANNELS,
+ .link = stm_ftrace_link,
+ .unlink = stm_ftrace_unlink,
+ },
+};
+
+/**
+ * stm_ftrace_write() - write data to STM via 'stm_ftrace' source
+ * @buf: buffer containing the data packet
+ * @len: length of the data packet
+ */
+static void notrace
+stm_ftrace_write(const void *buf, unsigned int len)
+{
+ stm_source_write(&stm_ftrace.data, STM_FTRACE_CHAN, buf, len);
+}
+
+static int stm_ftrace_link(struct stm_source_data *data)
+{
+ struct stm_ftrace *sf = container_of(data, struct stm_ftrace, data);
+
+ sf->ftrace.write = stm_ftrace_write;
+
+ return register_ftrace_export(&sf->ftrace);
+}
+
+static void stm_ftrace_unlink(struct stm_source_data *data)
+{
+ struct stm_ftrace *sf = container_of(data, struct stm_ftrace, data);
+
+ unregister_ftrace_export(&sf->ftrace);
+}
+
+static int __init stm_ftrace_init(void)
+{
+ int ret;
+
+ ret = stm_source_register_device(NULL, &stm_ftrace.data);
+ if (ret)
+ pr_err("Failed to register stm_source - ftrace.\n");
+
+ return ret;
+}
+
+static void __exit stm_ftrace_exit(void)
+{
+ stm_source_unregister_device(&stm_ftrace.data);
+}
+
+module_init(stm_ftrace_init);
+module_exit(stm_ftrace_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("stm_ftrace driver");
+MODULE_AUTHOR("Chunyan Zhang <zhang.chunyan@linaro.org>");
--
2.7.4
^ permalink raw reply related
* [PATCH V8 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only
From: Chunyan Zhang @ 2016-11-18 3:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479438454-28650-1-git-send-email-zhang.chunyan@linaro.org>
Currently Function traces can be only exported to ring buffer, this
patch added trace_export concept which can process traces and export
them to a registered destination as an addition to the current only
one output of Ftrace - i.e. ring buffer.
In this way, if we want Function traces to be sent to other destination
rather than ring buffer only, we just need to register a new trace_export
and implement its own .write() function for writing traces to storage.
With this patch, only Function trace (trace type is TRACE_FN)
is supported.
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
include/linux/trace.h | 28 +++++++++++
kernel/trace/trace.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 156 insertions(+), 1 deletion(-)
create mode 100644 include/linux/trace.h
diff --git a/include/linux/trace.h b/include/linux/trace.h
new file mode 100644
index 0000000..9330a58
--- /dev/null
+++ b/include/linux/trace.h
@@ -0,0 +1,28 @@
+#ifndef _LINUX_TRACE_H
+#define _LINUX_TRACE_H
+
+#ifdef CONFIG_TRACING
+/*
+ * The trace export - an export of Ftrace output. The trace_export
+ * can process traces and export them to a registered destination as
+ * an addition to the current only output of Ftrace - i.e. ring buffer.
+ *
+ * If you want traces to be sent to some other place rather than ring
+ * buffer only, just need to register a new trace_export and implement
+ * its own .write() function for writing traces to the storage.
+ *
+ * next - pointer to the next trace_export
+ * write - copy traces which have been delt with ->commit() to
+ * the destination
+ */
+struct trace_export {
+ struct trace_export __rcu *next;
+ void (*write)(const void *, unsigned int);
+};
+
+int register_ftrace_export(struct trace_export *export);
+int unregister_ftrace_export(struct trace_export *export);
+
+#endif /* CONFIG_TRACING */
+
+#endif /* _LINUX_TRACE_H */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8696ce6..038291d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -40,6 +40,7 @@
#include <linux/poll.h>
#include <linux/nmi.h>
#include <linux/fs.h>
+#include <linux/trace.h>
#include <linux/sched/rt.h>
#include "trace.h"
@@ -2128,6 +2129,129 @@ void trace_buffer_unlock_commit_regs(struct trace_array *tr,
ftrace_trace_userstack(buffer, flags, pc);
}
+static void
+trace_process_export(struct trace_export *export,
+ struct ring_buffer_event *event)
+{
+ struct trace_entry *entry;
+ unsigned int size = 0;
+
+ entry = ring_buffer_event_data(event);
+ size = ring_buffer_event_length(event);
+ export->write(entry, size);
+}
+
+static DEFINE_MUTEX(ftrace_export_lock);
+
+static struct trace_export __rcu *ftrace_exports_list __read_mostly;
+
+static DEFINE_STATIC_KEY_FALSE(ftrace_exports_enabled);
+
+static inline void ftrace_exports_enable(void)
+{
+ static_branch_enable(&ftrace_exports_enabled);
+}
+
+static inline void ftrace_exports_disable(void)
+{
+ static_branch_disable(&ftrace_exports_enabled);
+}
+
+void ftrace_exports(struct ring_buffer_event *event)
+{
+ struct trace_export *export;
+
+ preempt_disable_notrace();
+
+ export = rcu_dereference_raw_notrace(ftrace_exports_list);
+ while (export) {
+ trace_process_export(export, event);
+ export = rcu_dereference_raw_notrace(export->next);
+ }
+
+ preempt_enable_notrace();
+}
+
+static inline void
+add_trace_export(struct trace_export **list, struct trace_export *export)
+{
+ rcu_assign_pointer(export->next, *list);
+ /*
+ * We are entering export into the list but another
+ * CPU might be walking that list. We need to make sure
+ * the export->next pointer is valid before another CPU sees
+ * the export pointer included into the list.
+ */
+ rcu_assign_pointer(*list, export);
+}
+
+static inline int
+rm_trace_export(struct trace_export **list, struct trace_export *export)
+{
+ struct trace_export **p;
+
+ for (p = list; *p != NULL; p = &(*p)->next)
+ if (*p == export)
+ break;
+
+ if (*p != export)
+ return -1;
+
+ rcu_assign_pointer(*p, (*p)->next);
+
+ return 0;
+}
+
+static inline void
+add_ftrace_export(struct trace_export **list, struct trace_export *export)
+{
+ if (*list == NULL)
+ ftrace_exports_enable();
+
+ add_trace_export(list, export);
+}
+
+static inline int
+rm_ftrace_export(struct trace_export **list, struct trace_export *export)
+{
+ int ret;
+
+ ret = rm_trace_export(list, export);
+ if (*list == NULL)
+ ftrace_exports_disable();
+
+ return ret;
+}
+
+int register_ftrace_export(struct trace_export *export)
+{
+ if (WARN_ON_ONCE(!export->write))
+ return -1;
+
+ mutex_lock(&ftrace_export_lock);
+
+ add_ftrace_export(&ftrace_exports_list, export);
+
+ mutex_unlock(&ftrace_export_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(register_ftrace_export);
+
+int unregister_ftrace_export(struct trace_export *export)
+{
+ int ret;
+
+ mutex_lock(&ftrace_export_lock);
+
+ ret = rm_ftrace_export(&ftrace_exports_list, export);
+
+ mutex_unlock(&ftrace_export_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(unregister_ftrace_export);
+
void
trace_function(struct trace_array *tr,
unsigned long ip, unsigned long parent_ip, unsigned long flags,
@@ -2146,8 +2270,11 @@ trace_function(struct trace_array *tr,
entry->ip = ip;
entry->parent_ip = parent_ip;
- if (!call_filter_check_discard(call, entry, buffer, event))
+ if (!call_filter_check_discard(call, entry, buffer, event)) {
+ if (static_branch_unlikely(&ftrace_exports_enabled))
+ ftrace_exports(event);
__buffer_unlock_commit(buffer, event);
+ }
}
#ifdef CONFIG_STACKTRACE
--
2.7.4
^ permalink raw reply related
* [PATCH V8 0/3] Integration of function trace with System Trace IP blocks
From: Chunyan Zhang @ 2016-11-18 3:07 UTC (permalink / raw)
To: linux-arm-kernel
IP blocks allowing a variety of trace sources to log debugging
information to a pre-defined area have been introduced on a couple of
architecture [1][2]. These system trace blocks (also known as STM)
typically follow the MIPI STPv2 protocol [3] and provide a system wide
logging facility to any device, running a kernel or not, with access
to the block's log entry port(s). Since each trace message has a
timestamp, it is possible to correlate events happening in the entire
system rather than being confined to the logging facility of a single
entity.
This patchset is trying to use STM IP blocks to store function tracing
information produced by Ftrace and I'm taking the Function trace
(trace type is TRACE_FN) as the example in this patchset, but other
types of traces also can be supported.
Logging information generated by the Ftrace subsystem to STM and gathered
in the sink device can be used in conjunction with trace data from other
board components, also collected in the same trace sink.
This example is using ARM coresight STM but the same would apply to any
other architecture wishing to do the same.
Comments would be greatly appreciated.
Thanks,
Chunyan
[1]. https://lwn.net/Articles/674746/
[2]. http://lxr.free-electrons.com/source/drivers/hwtracing/intel_th/
[3]. http://mipi.org/specifications/debug#STP
Changes from v7:
* Addressed comments from Steven Rostedt:
- Removed unnessecery code;
- Shrinked the dependence of 'STM_SOURCE_FTRACE' from 'TRACING' to 'FUNCTION_TRACER';
- Changed the parameter type from char* to void*, that makes more sense.
Changes from v6:
* Rebased on v4.9-rc1;
* Removed unused the declaration and header file including from trace.h
which was added in patch 1 of this series;
* Revised a bit the comments in trace.h .
Changes from v5:
* Addressed comments from Steven Rostedt:
- Removed .commit() from trace_export;
- Changed to directly call trace_process_export() instead of trace_export::commit();
- Used 'ring_buffer_event_length(event)' instead to get the trace size;
- Removed trace_export pointer from trace_array structure.
* Revised commit message a little to make the description more accurate.
Changes from v4:
* Addressed comments from Steven Rostedt:
- Removed 'inline' annotations from the function which is called via function pointer;
- Removed useless components from structure trace_export;
- Added '__rcu' annotation to the RCU variables;
- Used 'rcu_assign_pointer' to do every RCU assignment;
- Added WARN_ON_ONCE() when the .write() is not assigned;
- In order to reduce the overhead caused by adding this feature, this revision used an
global array instead of a big "if statement" to get the size of trace entry according
to the trace type.
- In order to keep the current logic unchanged, made ftrace_exports() only being called if
there's something have been added, i.e. if trace_export to stm_ftrace has been added in
this patchset.
Changes from v3:
* Addressed comments from Steven Rostedt:
- Added comments for the element 'name' and 'next' of trace_export;
- Changed to use RCU functions instead of mutex in function callback;
- Moved the check for name to register trace_export function;
* Renamed 'trace_function_exports' to 'trace_exports' since its
implementation is not only for function_trace; The similar changes on
'add_trace_export' and 'rm_trace_export'.
Changes from v2:
* Rebased on v4.8-rc1.
* Added trace_export class, and make traces can be exported to not only
ring buffer but also other area such as STM.
* Made stm_ftrace as an trace_export.
* More detailed changes are described in change log of each patch.
Changes from v1:
* Addressed comments from Alexander Shishkin:
- Modified some ambiguous change logs.
- Decoupled stm_ftrace and trace_output interface to STM.
- Changed the file name from stm_ftrace.c to stm/ftrace.c.
- Implemented link/unlink hooks for stm_ftrace.
* Removed useless header file include from stm/ftrace.c
* Added Acked-by from Steven Rostedt on 4/4.
Chunyan Zhang (3):
tracing: add a possibility of exporting function trace to other places
instead of ring buffer only
stm class: ftrace: Add ftrace-export-over-stm driver
stm: Mark the functions of writing buffer with notrace
drivers/hwtracing/coresight/coresight-stm.c | 2 +-
drivers/hwtracing/intel_th/sth.c | 11 ++-
drivers/hwtracing/stm/Kconfig | 11 +++
drivers/hwtracing/stm/Makefile | 2 +
drivers/hwtracing/stm/core.c | 7 +-
drivers/hwtracing/stm/dummy_stm.c | 2 +-
drivers/hwtracing/stm/ftrace.c | 87 +++++++++++++++++++
include/linux/stm.h | 4 +-
include/linux/trace.h | 28 ++++++
kernel/trace/trace.c | 129 +++++++++++++++++++++++++++-
10 files changed, 271 insertions(+), 12 deletions(-)
create mode 100644 drivers/hwtracing/stm/ftrace.c
create mode 100644 include/linux/trace.h
--
2.7.4
^ permalink raw reply
* [RFC 6/6] ARM: dts: am57xx-beagle-x15-common: enable etnaviv
From: Nishanth Menon @ 2016-11-18 2:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-6-robertcnelson@gmail.com>
On 11/17/2016 08:44 PM, Robert Nelson wrote:
again.. a short commit message at least please? :)
> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> CC: Julien <jboulnois@gmail.com>
> CC: Nishanth Menon <nm@ti.com>
> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> ---
> arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> index 6df7829..3bc47be 100644
> --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> @@ -97,6 +97,12 @@
> #cooling-cells = <2>;
> };
>
> + gpu-subsystem {
A) do we want to make things clear that this is gpu subsystem for gc320?
B) How about other platforms that could equally reuse?
> + compatible = "ti,gc320-gpu-subsystem";
> + cores = <&bb2d>;
> + status = "okay";
> + };
> +
> hdmi0: connector {
> compatible = "hdmi-connector";
> label = "hdmi";
> @@ -190,6 +196,11 @@
> >;
> };
> };
> +
> +&bb2d {
> + status = "okay";
> +};
> +
> &i2c1 {
> status = "okay";
> clock-frequency = <400000>;
>
--
Regards,
Nishanth Menon
^ permalink raw reply
* [RFC 3/6] Documentation: dt: add bindings for ti bb2d
From: Nishanth Menon @ 2016-11-18 2:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-3-robertcnelson@gmail.com>
On 11/17/2016 08:44 PM, Robert Nelson wrote:
> From: Gowtham Tammana <g-tammana@ti.com>
>
> Add documentation for device tree bindings description for
> 2D GPU blitter module present in Texas Instruments family of SoCs.
>
> Signed-off-by: Gowtham Tammana <g-tammana@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
Might want to point at the source of the patch -> just for the record,
this came from TI vendor kernel..
> Documentation/devicetree/bindings/gpu/ti-bb2d.txt | 27 +++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpu/ti-bb2d.txt
>
> diff --git a/Documentation/devicetree/bindings/gpu/ti-bb2d.txt b/Documentation/devicetree/bindings/gpu/ti-bb2d.txt
> new file mode 100644
> index 0000000..af47488
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/ti-bb2d.txt
> @@ -0,0 +1,27 @@
> +* Texas Instruments BB2D blitter module
> +
> +This binding describes the 2D BitBlit (BB2D) graphics accelerator
> +subsystem based on the GC320 core from Vivante Corporation available
> +in Texas Instruments SoCs.
> +
> +Required properties:
> + - compatible: value should take the following format:
> + "ti,<soc>-bb2d", "vivante,<gpuversion>"
> + accepted values:
> + (a) "ti,dra7-bb2d", "vivante,gc320" for TI DRA7xx / AM57x
> +
> + - reg : base address and length of BB2D IP registers
> + - interrupts : BB2D interrupt line number
> + - ti,hwmods : name of the hwmod associated with BB2D module
> + - clocks : handle to BB2D functional clock
> + - clock-names : fclk
> +
> +Example for DRA7x SoC:
> + bb2d: bb2d at 59000000 {
> + compatible = "ti,dra7-bb2d", "vivante,gc320";
> + reg = <0x59000000 0x0700>;
> + interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
> + ti,hwmods = "bb2d";
> + clocks = <&dpll_core_h24x2_ck>;
> + clock-names = "fclk";
> + };
>
--
Regards,
Nishanth Menon
^ permalink raw reply
* [RFC 1/6] drm/etnaviv: add binding for the gc320 found in ti socs
From: Nishanth Menon @ 2016-11-18 2:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
On 11/17/2016 08:44 PM, Robert Nelson wrote:
Could we write at least a oneline commit message? :)
Might want to state that since the TI gpu systems are a mixed bunch
and certain SoCs may have more than 1 GPU integrated, we indicate
clearly the rev here?
> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> CC: Julien <jboulnois@gmail.com>
> CC: Christian Gmeiner <christian.gmeiner@gmail.com>
> CC: Russell King <rmk+kernel@arm.linux.org.uk>
> CC: Lucas Stach <l.stach@pengutronix.de>
> CC: Nishanth Menon <nm@ti.com>
> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> ---
> Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt | 1 +
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> index ed5e0a7..9fa259d 100644
> --- a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> +++ b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> @@ -8,6 +8,7 @@ Required properties:
> - compatible: Should be one of
> "fsl,imx-gpu-subsystem"
> "marvell,dove-gpu-subsystem"
> + "ti,gc320-gpu-subsystem"
> - cores: Should contain a list of phandles pointing to Vivante GPU devices
>
> example:
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index a6799b0..ce51270 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -653,6 +653,7 @@ static int etnaviv_pdev_remove(struct platform_device *pdev)
> static const struct of_device_id dt_match[] = {
> { .compatible = "fsl,imx-gpu-subsystem" },
> { .compatible = "marvell,dove-gpu-subsystem" },
> + { .compatible = "ti,gc320-gpu-subsystem" },
> {}
> };
> MODULE_DEVICE_TABLE(of, dt_match);
>
--
Regards,
Nishanth Menon
^ permalink raw reply
* [RFC 2/6] drm/etnaviv: allow building etnaviv on omap devices
From: Nishanth Menon @ 2016-11-18 2:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-2-robertcnelson@gmail.com>
On 11/17/2016 08:44 PM, Robert Nelson wrote:
Could we write at least a oneline commit message? :)
> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> CC: Christian Gmeiner <christian.gmeiner@gmail.com>
> CC: Russell King <rmk+kernel@arm.linux.org.uk>
> CC: Lucas Stach <l.stach@pengutronix.de>
> ---
> drivers/gpu/drm/etnaviv/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
> index 2cde7a5..b776f41 100644
> --- a/drivers/gpu/drm/etnaviv/Kconfig
> +++ b/drivers/gpu/drm/etnaviv/Kconfig
> @@ -2,7 +2,7 @@
> config DRM_ETNAVIV
> tristate "ETNAVIV (DRM support for Vivante GPU IP cores)"
> depends on DRM
> - depends on ARCH_MXC || ARCH_DOVE
> + depends on ARCH_MXC || ARCH_DOVE || ARCH_OMAP2PLUS
> select SHMEM
> select TMPFS
> select IOMMU_API
>
--
Regards,
Nishanth Menon
^ permalink raw reply
* [RFC 1/6] drm/etnaviv: add binding for the gc320 found in ti socs
From: Robert Nelson @ 2016-11-18 2:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
Sorry, messed up the cover-header, patches 3 & 4 in this series where
cherry picked form ti's v4.4.x tree:
http://git.ti.com/cgit/cgit.cgi/ti-linux-kernel/ti-linux-kernel.git/commit/arch/arm/boot/dts/dra7.dtsi?h=ti-linux-4.4.y&id=8067f5a5619ce45657d3729ab3adb9e5b1294f0d
http://git.ti.com/cgit/cgit.cgi/ti-linux-kernel/ti-linux-kernel.git/commit/Documentation/devicetree/bindings/gpu/ti-bb2d.txt?h=ti-linux-4.4.y&id=ddadad0828f8e5ad7e89b11dd243249228ff2997
"ti,gc320-gpu-subsystem" seems like the best middle ground option, the
gc320 is found on one omap4770, omap5, dra7..
"ti,omap-gpu-subsystem" might clash with the naming for the sgx544 3D
core found on these devices, then some day, ti could use
"ti,sgx<num>-gpu-subsystem"
For BeagleBoard.org & BeagleBoard-x15 users we have this working in
the updated images.
For people looking to build the packages, i have it up here:
https://gist.github.com/RobertCNelson/fc6d07157b0fcc13b9c28c5832fdc74b
Regards,
On Thu, Nov 17, 2016 at 8:44 PM, Robert Nelson <robertcnelson@gmail.com> wrote:
> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> CC: Julien <jboulnois@gmail.com>
> CC: Christian Gmeiner <christian.gmeiner@gmail.com>
> CC: Russell King <rmk+kernel@arm.linux.org.uk>
> CC: Lucas Stach <l.stach@pengutronix.de>
> CC: Nishanth Menon <nm@ti.com>
> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> ---
> Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt | 1 +
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> index ed5e0a7..9fa259d 100644
> --- a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> +++ b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
> @@ -8,6 +8,7 @@ Required properties:
> - compatible: Should be one of
> "fsl,imx-gpu-subsystem"
> "marvell,dove-gpu-subsystem"
> + "ti,gc320-gpu-subsystem"
> - cores: Should contain a list of phandles pointing to Vivante GPU devices
>
> example:
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index a6799b0..ce51270 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -653,6 +653,7 @@ static int etnaviv_pdev_remove(struct platform_device *pdev)
> static const struct of_device_id dt_match[] = {
> { .compatible = "fsl,imx-gpu-subsystem" },
> { .compatible = "marvell,dove-gpu-subsystem" },
> + { .compatible = "ti,gc320-gpu-subsystem" },
> {}
> };
> MODULE_DEVICE_TABLE(of, dt_match);
> --
> 2.10.2
>
--
Robert Nelson
https://rcn-ee.com/
^ permalink raw reply
* [RFC 6/6] ARM: dts: am57xx-beagle-x15-common: enable etnaviv
From: Robert Nelson @ 2016-11-18 2:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
CC: Julien <jboulnois@gmail.com>
CC: Nishanth Menon <nm@ti.com>
CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
CC: Tony Lindgren <tony@atomide.com>
---
arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
index 6df7829..3bc47be 100644
--- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
+++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
@@ -97,6 +97,12 @@
#cooling-cells = <2>;
};
+ gpu-subsystem {
+ compatible = "ti,gc320-gpu-subsystem";
+ cores = <&bb2d>;
+ status = "okay";
+ };
+
hdmi0: connector {
compatible = "hdmi-connector";
label = "hdmi";
@@ -190,6 +196,11 @@
>;
};
};
+
+&bb2d {
+ status = "okay";
+};
+
&i2c1 {
status = "okay";
clock-frequency = <400000>;
--
2.10.2
^ permalink raw reply related
* [RFC 5/6] ARM: dts: dra7: add vivante for bb2d module
From: Robert Nelson @ 2016-11-18 2:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
CC: Julien <jboulnois@gmail.com>
CC: Nishanth Menon <nm@ti.com>
CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
CC: Tony Lindgren <tony@atomide.com>
---
arch/arm/boot/dts/dra7.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 43488b6..22bd0a5 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -960,7 +960,7 @@
};
bb2d: bb2d at 59000000 {
- compatible = "ti,dra7-bb2d";
+ compatible = "ti,dra7-bb2d","vivante,gc";
reg = <0x59000000 0x0700>;
interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
ti,hwmods = "bb2d";
--
2.10.2
^ permalink raw reply related
* [RFC 4/6] ARM: dts: dra7: add entry for bb2d module
From: Robert Nelson @ 2016-11-18 2:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
From: Gowtham Tammana <g-tammana@ti.com>
BB2D entry is added to the dts file. Crossbar index number is used
for interrupt mapping.
Signed-off-by: Gowtham Tammana <g-tammana@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/boot/dts/dra7.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index addb753..43488b6 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -959,6 +959,16 @@
ti,hwmods = "dmm";
};
+ bb2d: bb2d at 59000000 {
+ compatible = "ti,dra7-bb2d";
+ reg = <0x59000000 0x0700>;
+ interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ ti,hwmods = "bb2d";
+ clocks = <&dpll_core_h24x2_ck>;
+ clock-names = "fclk";
+ status = "disabled";
+ };
+
i2c1: i2c at 48070000 {
compatible = "ti,omap4-i2c";
reg = <0x48070000 0x100>;
--
2.10.2
^ permalink raw reply related
* [RFC 3/6] Documentation: dt: add bindings for ti bb2d
From: Robert Nelson @ 2016-11-18 2:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
From: Gowtham Tammana <g-tammana@ti.com>
Add documentation for device tree bindings description for
2D GPU blitter module present in Texas Instruments family of SoCs.
Signed-off-by: Gowtham Tammana <g-tammana@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
Documentation/devicetree/bindings/gpu/ti-bb2d.txt | 27 +++++++++++++++++++++++
1 file changed, 27 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpu/ti-bb2d.txt
diff --git a/Documentation/devicetree/bindings/gpu/ti-bb2d.txt b/Documentation/devicetree/bindings/gpu/ti-bb2d.txt
new file mode 100644
index 0000000..af47488
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/ti-bb2d.txt
@@ -0,0 +1,27 @@
+* Texas Instruments BB2D blitter module
+
+This binding describes the 2D BitBlit (BB2D) graphics accelerator
+subsystem based on the GC320 core from Vivante Corporation available
+in Texas Instruments SoCs.
+
+Required properties:
+ - compatible: value should take the following format:
+ "ti,<soc>-bb2d", "vivante,<gpuversion>"
+ accepted values:
+ (a) "ti,dra7-bb2d", "vivante,gc320" for TI DRA7xx / AM57x
+
+ - reg : base address and length of BB2D IP registers
+ - interrupts : BB2D interrupt line number
+ - ti,hwmods : name of the hwmod associated with BB2D module
+ - clocks : handle to BB2D functional clock
+ - clock-names : fclk
+
+Example for DRA7x SoC:
+ bb2d: bb2d at 59000000 {
+ compatible = "ti,dra7-bb2d", "vivante,gc320";
+ reg = <0x59000000 0x0700>;
+ interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ ti,hwmods = "bb2d";
+ clocks = <&dpll_core_h24x2_ck>;
+ clock-names = "fclk";
+ };
--
2.10.2
^ permalink raw reply related
* [RFC 2/6] drm/etnaviv: allow building etnaviv on omap devices
From: Robert Nelson @ 2016-11-18 2:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118024436.13447-1-robertcnelson@gmail.com>
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
CC: Christian Gmeiner <christian.gmeiner@gmail.com>
CC: Russell King <rmk+kernel@arm.linux.org.uk>
CC: Lucas Stach <l.stach@pengutronix.de>
---
drivers/gpu/drm/etnaviv/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
index 2cde7a5..b776f41 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -2,7 +2,7 @@
config DRM_ETNAVIV
tristate "ETNAVIV (DRM support for Vivante GPU IP cores)"
depends on DRM
- depends on ARCH_MXC || ARCH_DOVE
+ depends on ARCH_MXC || ARCH_DOVE || ARCH_OMAP2PLUS
select SHMEM
select TMPFS
select IOMMU_API
--
2.10.2
^ permalink raw reply related
* [RFC 1/6] drm/etnaviv: add binding for the gc320 found in ti socs
From: Robert Nelson @ 2016-11-18 2:44 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
CC: Julien <jboulnois@gmail.com>
CC: Christian Gmeiner <christian.gmeiner@gmail.com>
CC: Russell King <rmk+kernel@arm.linux.org.uk>
CC: Lucas Stach <l.stach@pengutronix.de>
CC: Nishanth Menon <nm@ti.com>
CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
CC: Tony Lindgren <tony@atomide.com>
---
Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt | 1 +
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
index ed5e0a7..9fa259d 100644
--- a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
+++ b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt
@@ -8,6 +8,7 @@ Required properties:
- compatible: Should be one of
"fsl,imx-gpu-subsystem"
"marvell,dove-gpu-subsystem"
+ "ti,gc320-gpu-subsystem"
- cores: Should contain a list of phandles pointing to Vivante GPU devices
example:
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index a6799b0..ce51270 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -653,6 +653,7 @@ static int etnaviv_pdev_remove(struct platform_device *pdev)
static const struct of_device_id dt_match[] = {
{ .compatible = "fsl,imx-gpu-subsystem" },
{ .compatible = "marvell,dove-gpu-subsystem" },
+ { .compatible = "ti,gc320-gpu-subsystem" },
{}
};
MODULE_DEVICE_TABLE(of, dt_match);
--
2.10.2
^ permalink raw reply related
* [PATCH] iommu: mtk: add common-clk dependency
From: Honghui Zhang @ 2016-11-18 2:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117233502.GW25626@codeaurora.org>
On Thu, 2016-11-17 at 15:35 -0800, Stephen Boyd wrote:
> On 11/17, Honghui Zhang wrote:
> > On Wed, 2016-11-16 at 11:38 -0800, Stephen Boyd wrote:
> > > On 11/16, Arnd Bergmann wrote:
> > > > After the MT2701 clock driver was added, we get a harmless warning for
> > > > the iommu driver that selects it, when compile-testing without
> > > > COMMON_CLK.
> > > >
> > > > warning: (MTK_IOMMU_V1) selects COMMON_CLK_MT2701_IMGSYS which has unmet direct dependencies (COMMON_CLK)
> > > >
> > > > Adding a dependency on COMMON_CLK avoids the warning.
> > > >
> > > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support")
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Hm.. why is an iommu driver selecting a clk driver? They should
> > > be using standard clk APIs so it's not like they need it for
> > > build time correctness. Shouldn't we drop the selects instead?
> > > Those look to have been introduced a few kernel versions ago, but
> > > they were selecting options that didn't exist until a few days
> > > ago when I merged the mediatek clk driver. The clk options are
> > > user-visible, so it should be possible to select them in the
> > > configuration phase.
> > >
> >
> > Hi, Stephen,
> > I'm a bit out of date of the current clock code. Mediatek IOMMU v1
> > driver will need smi driver to enable iommu clients. And smi driver is
> > also respond to enable/disable the susbsys clocks for multi-media HW.
> > The relationship between iommu and smi is like the graphics below[1].
> >
> > EMI (External Memory Interface)
> > |
> > m4u (Multimedia Memory Management Unit)
> > |
> > SMI Common(Smart Multimedia Interface Common)
> > |
> > +----------------+-------
> > | |
> > | |
> > SMI larb0 SMI larb1 ... SoCs have several SMI local
> > arbiter(larb).
> > (display) (vdec)
> > | |
> > | |
> > +-----+-----+ +----+----+
> > | | | | | |
> > | | |... | | | ... There are different ports in each
> > larb.
> > | | | | | |
> > OVL0 RDMA0 WDMA0 MC PP VLD
> >
> >
> > When enable SMI driver it will need those subsys clock provider.
> > But those clocks providers are disabled in default. Since it's needed by
> > smi driver, and smi was select by MTK_IOMMU_V1, I figure it should be
> > select by MTK_IOMMU_V1 too.
>
> Ok I understand all that, but I don't understand why that means
> we need to have select statements for clk drivers still. If
> anything, that logic would mean the SMI driver should select clk
> drivers. I hope it isn't doing that.
>
OK, I guess the only reason of "SMI driver select clk driver" is to
avoid runtime error. Maybe this is not necessary since runtime errors
should be guaranteed by defconfig.
Your propose of just remove the select statements is good enough for
this problem, thanks.
> BTW, I don't understand the mtk_smi_larb_get() API. It looks like
> we expect the SMI driver to probe and succeed before the
> mtk_smi_larb_get() function is called. That seems fairly brittle
> in the face of probe defer or device ordering changes.
>
Sharp eyes.
As a matter of fact, we are struggling on this problem for the
moment[1], I guess the recently merged device link's patch may help with
this, but I didn't have a change to try that yet.
> The SMI driver actually looks like a bus driver for an
> interconnect as well, but drivers/memory is for memory
> controllers? Odd but I can get over that.
>
This have been discussed long times ago, seems the current folder is
where no one object[2][3].
[1]https://lkml.org/lkml/2016/11/15/232
[2]https://lists.linuxfoundation.org/pipermail/iommu/2015-March/012497.html
[3]https://lists.linuxfoundation.org/pipermail/iommu/2015-March/012498.html
^ permalink raw reply
* [PATCH] drm/sun4i: Only count TCON endpoints as valid outputs
From: Chen-Yu Tsai @ 2016-11-18 2:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117190254.d4doghspvz6ma6x2@lukather>
On Fri, Nov 18, 2016 at 3:02 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Nov 16, 2016 at 05:37:31PM +0800, Chen-Yu Tsai wrote:
>> The sun4i DRM driver counts the number of endpoints it found and
>> registers the whole DRM pipeline if any endpoints are found.
>>
>> However, if the TCON and its child endpoints (LCD panels, TV encoder,
>> HDMI encoder, MIPI DSI encoder, etc.) aren't found, that means we
>> don't have any usable CRTCs, and the display pipeline is incomplete
>> and useless.
>
> If some node set as available is not probed, then yes, it does, but
> I'm not really sure how it's a problem. Quite the opposite actually.
Actually the problem occurs when the TCON is _not_ available, but
the other endpoints preceding it are.
>> The whole DRM display pipeline should only be registered and enabled
>> if there are proper outputs available.
>
> Unless I've misunderstood what you're saying, we could have the
> writeback for example that would just need the display engine.
Yeah, I guess that complicates things...
>> The debug message "Queued %d outputs on pipeline %d\n" is also telling.
>>
>> This patch makes the driver only count enabled TCON endpoints. If
>> none are found, the DRM pipeline is not used. This avoids screwing
>> up the simple framebuffer provided by the bootloader in cases where
>> we aren't able to support the display with the DRM subsystem, due
>> to lack of panel or bridge drivers, or just lack of progress.
>
> The framebuffer is removed only at bind time, which means that all the
> drivers have probed already. Lack of progress isn't an issue here,
> since the node simply won't be there, and we wouldn't have it in the
> component lists. And lack of drivers shouldn't be an issue either,
> since in order for bind to be called, all the drivers would have
> gone through their probe.
>
> So I'm not really sure what it fixes.
To recap, on sun6i I had enabled the display engine node by default
in the dtsi, along with the backend and drc. The tcon is disabled
by default, so it doesn't get added to the list of components.
The available components get probed, binded, and simplefb gets
pushed out.
I suppose disabling the display engine by default would be better?
At least simplefb still works.
Regards
ChenYu
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply
* spin_lock behavior with ARM64 big.Little/HMP
From: Vikram Mulukutla @ 2016-11-18 2:22 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
This isn't really a bug report, but just a description of a
frequency/IPC
dependent behavior that I'm curious if we should worry about. The
behavior
is exposed by questionable design so I'm leaning towards don't-care.
Consider these threads running in parallel on two ARM64 CPUs running
mainline
Linux:
(Ordering of lines between the two columns does not indicate a sequence
of
execution. Assume flag=0 initially.)
LittleARM64_CPU @ 300MHz (e.g.A53) | BigARM64_CPU @ 1.5GHz (e.g. A57)
-------------------------------------+----------------------------------
spin_lock_irqsave(s) | local_irq_save()
/* critical section */
flag = 1 | spin_lock(s)
spin_unlock_irqrestore(s) | while (!flag) {
| spin_unlock(s)
| cpu_relax();
| spin_lock(s)
| }
| spin_unlock(s)
| local_irq_restore()
I see a livelock occurring where the LittleCPU is never able to acquire
the
lock, and the BigCPU is stuck forever waiting on 'flag' to be set.
Even with ticket spinlocks, this bit of code can cause a livelock (or
very
long delays) if BigCPU runs fast enough. Afaics this can only happen if
the
LittleCPU is unable to put its ticket in the queue (i.e, increment the
next
field) since the store-exclusive keeps failing.
The problem is not present on SMP, and is mitigated by adding enough
additional clock cycles between the unlock and lock in the loop running
on the BigCPU. On big.Little, if both threads are scheduled on the same
cluster within the same clock domain, the problem is avoided.
Now the infinite loop may seem like questionable design but the problem
isn't entirely hypothetical; if BigCPU calls hrtimer_cancel with
interrupts disabled, this scenario can result if the hrtimer is about
to run on a littleCPU. It's of course possible that there's just enough
intervening code for the problem to not occur. At the very least it
seems
that loops like the one running in the BigCPU above should come with a
WARN_ON(irqs_disabled) or with a sufficient udelay() instead of the
cpu_relax.
Thoughts?
Thanks,
Vikram
^ permalink raw reply
* [PATCH] drm/sun4i: tcon: Initialize regmap after enabling bus clocks
From: Chen-Yu Tsai @ 2016-11-18 2:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161117190501.hjpwposqzidwhwdc@lukather>
On Fri, Nov 18, 2016 at 3:05 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Nov 16, 2016 at 05:37:32PM +0800, Chen-Yu Tsai wrote:
>> If we attempt to read/write the TCON registers before the bus clock
>> is enabled, those accesses get ignored.
>>
>> In practice this almost never occurs because U-boot had already enabled
>> the bus clock as part of its firmware provided framebuffer (simplefb).
>>
>> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>
>> I was looking around the DRM driver and noticed this sequence was off.
>>
>> ---
>> drivers/gpu/drm/sun4i/sun4i_tcon.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index c6afb2448655..8c2db65ea229 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -506,16 +506,16 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>> return ret;
>> }
>>
>> - ret = sun4i_tcon_init_regmap(dev, tcon);
>> + ret = sun4i_tcon_init_clocks(dev, tcon);
>> if (ret) {
>> - dev_err(dev, "Couldn't init our TCON regmap\n");
>> + dev_err(dev, "Couldn't init our TCON clocks\n");
>> goto err_assert_reset;
>> }
>>
>> - ret = sun4i_tcon_init_clocks(dev, tcon);
>> + ret = sun4i_tcon_init_regmap(dev, tcon);
>> if (ret) {
>> - dev_err(dev, "Couldn't init our TCON clocks\n");
>> - goto err_assert_reset;
>> + dev_err(dev, "Couldn't init our TCON regmap\n");
>> + goto err_free_clocks;
>> }
>
> That won't work either. sun4i_tcon_init_clocks requires the regmap to
> be enabled before it calls sun4i_dclk_create.
>
> Maybe we should just move that call outside of sun4i_tcon_init_clocks
> and put it directly into the bind then.
That makes sense. I'll send a v2.
ChenYu
^ permalink raw reply
* [GIT PULL 2/3] ARM64: dts: exynos: DT for v4.10
From: Olof Johansson @ 2016-11-18 1:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478629589-7520-4-git-send-email-krzk@kernel.org>
On Tue, Nov 08, 2016 at 08:26:29PM +0200, Krzysztof Kozlowski wrote:
> Hi,
>
> Exynos5433 + two boards using it. Mobile boards! :)
>
> I am really happy to push it. I know that it has been a lot of effort
> in Samsung to mainline this.
>
> Best regards,
> Krzysztof
>
>
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git tags/samsung-dt64-4.10
>
> for you to fetch changes up to 8ac46fc57df82efbc19194dddd335b6c7a960c31:
>
> arm64: dts: exynos: Add dts file for Exynos5433-based TM2E board (2016-11-03 22:19:57 +0200)
>
> ----------------------------------------------------------------
> Finally, I am really pleased to announce adding support for Exynos5433 ARMv8
> SoC along with two boards. A lot of Samsung people contributed into this
> but the final work and commits were done by Chanwoo Choi.
>
> This means that for v4.10 we got:
> 1. Exynos5433 DTSI.
> 2. Two boards: TM2 and TM2E. These are (almost fully) working mobile phones.
Awesome! Looks like TM2 is a Tizen reference board? Great to see the support,
even if it's taken a while.
-Olof
^ permalink raw reply
* [GIT PULL 1/3] ARM: dts: exynos: DT for v4.10
From: Olof Johansson @ 2016-11-18 1:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478629589-7520-3-git-send-email-krzk@kernel.org>
Hi,
On Tue, Nov 08, 2016 at 08:26:28PM +0200, Krzysztof Kozlowski wrote:
> Hi,
>
> Hurray! New board! ... Exynos4415 slowly is going away.
>
> Best regards,
> Krzysztof
>
>
> The following changes since commit 1001354ca34179f3db924eb66672442a173147dc:
>
> Linux 4.9-rc1 (2016-10-15 12:17:50 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git tags/samsung-dt-4.10
>
> for you to fetch changes up to 05a3589f46f913fbe91704f12fdca46a0eb0a27b:
>
> ARM: dts: exynos: Add SCU device node to exynos4.dtsi (2016-11-05 17:39:50 +0200)
>
> ----------------------------------------------------------------
> Samsung DeviceTree update for v4.10:
> 1. Add TOPEET itop core and Elite boards, based on Exynos4412.
> 2. Remove the Exynos4415 DTSI. We did not have any mainlined boards
> using it. I am also not aware of any popular out-of-tree boards using it.
> 3. Add Snoop Control Unit node for Exynos4.
> 4. Minor cleanups.
Merged, thanks.
-Olof
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox