Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* PM regression with LED changes in next-20161109
From: Jacek Anaszewski @ 2016-11-12 10:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <d7ddbbb0-b99c-14a3-f462-3534b5ec67e6@redhat.com>

Hi,

On 11/11/2016 08:28 PM, Hans de Goede wrote:
> Hi,
>
> On 11-11-16 18:03, Jacek Anaszewski wrote:
>> On 11/11/2016 01:01 PM, Pavel Machek wrote:
>>> On Thu 2016-11-10 22:34:07, Jacek Anaszewski wrote:
>>>> Hi,
>>>>
>>>> On 11/10/2016 09:29 PM, Pavel Machek wrote:
>>>>> On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
>>>>>> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
>>>>>>> Hi!
>>>>>>>
>>>>>>>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for
>>>>>>>>>>> poll()ing
>>>>>>>>>>> the sysfs brightness attr for changes.") breaks runtime PM
>>>>>>>>>>> for me.
>>>>>>>>>>>
>>>>>>>>>>> On my omap dm3730 based test system, idle power consumption
>>>>>>>>>>> is over 70
>>>>>>>>>>> times higher now with this patch! It goes from about 6mW for
>>>>>>>>>>> the core
>>>>>>>>>>> system to over 440mW during idle meaning there's some busy
>>>>>>>>>>> timer now
>>>>>>>>>>> active.
>>>>>>>>>>>
>>>>>>>>>>> Reverting this patch fixes the issue. Any ideas?
>>>>>>>
>>>>>>> Are you using any LED that toggles with high frequency? Like perhaps
>>>>>>> LED that is lit when CPU is active?
>>>>>>
>>>>>> Yeah one of them seems to have cpu0 as the default trigger.
>>>>>
>>>>> Aha. Its quite obvious we don't want to notify sysfs each time that
>>>>> one is toggled, right?
>>>>>
>>>>> IMO brightness should display max brightness for the trigger, as Hans
>>>>> suggested, anything else is madness for trigger such as cpu activity.
>>>>
>>>> Are you suggesting that we should revert changes introduced
>>>> by below patch?
>>>>
>>>> commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
>>>> Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>>>> Date:   Tue Mar 18 09:47:48 2008 +0000
>>>>
>>>>     leds: Add support to leds with readable status
>>>>
>>>>     Some led hardware allows drivers to query the led state, and
>>>> this patch
>>>>     adds a hook to let the led class take advantage of that
>>>> information when
>>>>     available.
>>>>
>>>>     Without this functionality, when access to the led hardware is not
>>>>     exclusive (i.e. firmware or hardware might change its state
>>>> behind the
>>>>     kernel's back), reality goes out of sync with the led class'
>>>> idea of
>>>> what
>>>>     the led is doing, which is annoying at best.
>>>
>>> Hmm. So userland can read the LED state, and it can get _some_ value
>>> back, but it can not know if it is current state or not.
>>>
>>> I don't think that's a good interface. I see it is from 2008... is
>>> someone using it? Maybe it is too late for revert.
>>
>> I can imagine it being used in flash LED use case. E.g. one
>> could use oneshot trigger to trigger flash strobe, and then
>> he could periodically read brightness file to check, for whatever
>> reason, whether the flash is strobing.
>>
>>> But I'd certainly not extend it with poll.
>>
>> We could add a dedicated file e.g. hw_brightness_change for that
>> (maybe someone will have a better candidate for the file name).
>
> Why a dedicated file? Are we going to mirror brightness here
> wrt r/w (show/store) behavior ? If not userspace now needs
> 2 open fds which is not really nice. If we are and we are
> not going to use poll for something else on brightness itself
> then why not just poll directly on brightness ?

My main concern is that reporting only hw brightness changes
wouldn't be consistent with general brightness file purpose.
One could expect that brightness changes made by triggers
should be also reported.

I'd make it only readable, so it wouldn't mirror brightness
file behavior.

Its purpose would be clear: notify hw brightness changes
and provide the brightness value that was set by the hardware
last time. It implies that this value could be different from
the one the brightness file reports. E.g. hw could have changed
brightness, which could be later updated through brightness
file, but hw_brightness_change would still report brightness level
that was set by the hardware last time. It could be useful
e.g. in case of showing the difference between the desired
value and the currently allowed configuration (e.g. if the
firmware automatically adjusted the value set by the user).

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply

* [PATCH v6 5/9] drm/hisilicon/hibmc: Add crtc for DE
From: Rongrong Zou @ 2016-11-12 10:19 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOw6vbJCu96jcWUDS_zwqGDOa3LiF00adyCijuk7v24Nxe_ivg@mail.gmail.com>

? 2016/11/11 6:14, Sean Paul ??:
> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
>> Add crtc funcs and helper funcs for DE.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 318 ++++++++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c |   6 +
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |   2 +
>>   3 files changed, 326 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> index 9c1a68c..9b5d0d0 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -23,6 +23,7 @@
>>
>>   #include "hibmc_drm_drv.h"
>>   #include "hibmc_drm_regs.h"
>> +#include "hibmc_drm_de.h"
>>   #include "hibmc_drm_power.h"
>
> nit: alphabetize

ok, thanks.

>
>>
>>   /* ---------------------------------------------------------------------- */
>
> Remove

will do, thanks.

>
>> @@ -168,3 +169,320 @@ int hibmc_plane_init(struct hibmc_drm_device *hidev)
>>          drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>>          return 0;
>>   }
>> +
>> +static void hibmc_crtc_enable(struct drm_crtc *crtc)
>> +{
>> +       unsigned int reg;
>> +       /* power mode 0 is default. */
>
> This comment seems to be in the wrong place

will remove it, thanks.

>
>> +       struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>> +
>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>> +
>> +       /* Enable display power gate & LOCALMEM power gate*/
>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>> +       reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>> +       hibmc_set_current_gate(hidev, reg);
>> +       drm_crtc_vblank_on(crtc);
>> +}
>> +
>> +static void hibmc_crtc_disable(struct drm_crtc *crtc)
>> +{
>> +       unsigned int reg;
>> +       struct hibmc_drm_device *hidev = crtc->dev->dev_private;
>> +
>> +       drm_crtc_vblank_off(crtc);
>> +
>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_SLEEP);
>> +
>> +       /* Enable display power gate & LOCALMEM power gate*/
>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(OFF);
>> +       reg |= HIBMC_CURR_GATE_DISPLAY(OFF);
>> +       hibmc_set_current_gate(hidev, reg);
>> +}
>> +
>> +static int hibmc_crtc_atomic_check(struct drm_crtc *crtc,
>> +                                  struct drm_crtc_state *state)
>> +{
>> +       return 0;
>> +}
>
> Caller NULL-checks, no need for stub

thanks for pointing it out.

>
>> +
>> +static unsigned int format_pll_reg(void)
>> +{
>> +       unsigned int pllreg = 0;
>> +       struct panel_pll pll = {0};
>> +
>> +       /* Note that all PLL's have the same format. Here,
>> +        * we just use Panel PLL parameter to work out the bit
>> +        * fields in the register.On returning a 32 bit number, the value can
>> +        * be applied to any PLL in the calling function.
>> +        */
>> +       pllreg |= HIBMC_PLL_CTRL_BYPASS(OFF) & HIBMC_PLL_CTRL_BYPASS_MASK;
>> +       pllreg |= HIBMC_PLL_CTRL_POWER(ON) & HIBMC_PLL_CTRL_POWER_MASK;
>> +       pllreg |= HIBMC_PLL_CTRL_INPUT(OSC) & HIBMC_PLL_CTRL_INPUT_MASK;
>> +       pllreg |= HIBMC_PLL_CTRL_POD(pll.POD) & HIBMC_PLL_CTRL_POD_MASK;
>> +       pllreg |= HIBMC_PLL_CTRL_OD(pll.OD) & HIBMC_PLL_CTRL_OD_MASK;
>> +       pllreg |= HIBMC_PLL_CTRL_N(pll.N) & HIBMC_PLL_CTRL_N_MASK;
>> +       pllreg |= HIBMC_PLL_CTRL_M(pll.M) & HIBMC_PLL_CTRL_M_MASK;
>> +
>> +       return pllreg;
>> +}
>> +
>> +static void set_vclock_hisilicon(struct drm_device *dev, unsigned long pll)
>> +{
>> +       unsigned long tmp0, tmp1;
>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>> +
>> +       /* 1. outer_bypass_n=0 */
>> +       tmp0 = readl(hidev->mmio + CRT_PLL1_HS);
>> +       tmp0 &= 0xBFFFFFFF;
>> +       writel(tmp0, hidev->mmio + CRT_PLL1_HS);
>> +
>> +       /* 2. pll_pd=1?inter_bypass=1 */
>> +       writel(0x21000000, hidev->mmio + CRT_PLL1_HS);
>> +
>> +       /* 3. config pll */
>> +       writel(pll, hidev->mmio + CRT_PLL1_HS);
>> +
>> +       /* 4. delay  */
>> +       mdelay(1);
>
> These should be usleep_range() see
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

This looks better to me. i think a 'usleep_range(1000, 2000)' is ok.

>
>> +
>> +       /* 5. pll_pd =0 */
>> +       tmp1 = pll & ~0x01000000;
>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>> +
>> +       /* 6. delay  */
>> +       mdelay(1);
>> +
>> +       /* 7. inter_bypass=0 */
>> +       tmp1 &= ~0x20000000;
>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>> +
>> +       /* 8. delay  */
>> +       mdelay(1);
>> +
>> +       /* 9. outer_bypass_n=1 */
>> +       tmp1 |= 0x40000000;
>> +       writel(tmp1, hidev->mmio + CRT_PLL1_HS);
>
> This function is a whole lot of magic. Any chance you can pull the
> values out into #defines?

will do. thanks.

>
>> +}
>> +
>> +/* This function takes care the extra registers and bit fields required to
>
> nit: multi-line comments have a leading /* line with the comment
> starting on the following line

thanks for pointing it out.

>
> applies below as well
>
>
>> + *setup a mode in board.
>
> nit: space between * and comment, ie: * setup a mode in board

understood, thanks.

>
> applies to the rest of the comment too
>
>
>> + *Explanation about Display Control register:
>> + *FPGA only supports 7 predefined pixel clocks, and clock select is
>> + *in bit 4:0 of new register 0x802a8.
>> + */
>> +static unsigned int display_ctrl_adjust(struct drm_device *dev,
>> +                                       struct drm_display_mode *mode,
>> +                                       unsigned int ctrl)
>> +{
>> +       unsigned long x, y;
>> +       unsigned long pll1; /* bit[31:0] of PLL */
>> +       unsigned long pll2; /* bit[63:32] of PLL */
>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>> +
>> +       x = mode->hdisplay;
>> +       y = mode->vdisplay;
>> +
>> +       /* Hisilicon has to set up a new register for PLL control
>> +        *(CRT_PLL1_HS & CRT_PLL2_HS).
>> +        */
>> +       if (x == 800 && y == 600) {
>> +               pll1 = CRT_PLL1_HS_40MHZ;
>> +               pll2 = CRT_PLL2_HS_40MHZ;
>> +       } else if (x == 1024 && y == 768) {
>> +               pll1 = CRT_PLL1_HS_65MHZ;
>> +               pll2 = CRT_PLL2_HS_65MHZ;
>> +       } else if (x == 1152 && y == 864) {
>> +               pll1 = CRT_PLL1_HS_80MHZ_1152;
>> +               pll2 = CRT_PLL2_HS_80MHZ;
>> +       } else if (x == 1280 && y == 768) {
>> +               pll1 = CRT_PLL1_HS_80MHZ;
>> +               pll2 = CRT_PLL2_HS_80MHZ;
>> +       } else if (x == 1280 && y == 720) {
>> +               pll1 = CRT_PLL1_HS_74MHZ;
>> +               pll2 = CRT_PLL2_HS_74MHZ;
>> +       } else if (x == 1280 && y == 960) {
>> +               pll1 = CRT_PLL1_HS_108MHZ;
>> +               pll2 = CRT_PLL2_HS_108MHZ;
>> +       } else if (x == 1280 && y == 1024) {
>> +               pll1 = CRT_PLL1_HS_108MHZ;
>> +               pll2 = CRT_PLL2_HS_108MHZ;
>> +       } else if (x == 1600 && y == 1200) {
>> +               pll1 = CRT_PLL1_HS_162MHZ;
>> +               pll2 = CRT_PLL2_HS_162MHZ;
>> +       } else if (x == 1920 && y == 1080) {
>> +               pll1 = CRT_PLL1_HS_148MHZ;
>> +               pll2 = CRT_PLL2_HS_148MHZ;
>> +       } else if (x == 1920 && y == 1200) {
>> +               pll1 = CRT_PLL1_HS_193MHZ;
>> +               pll2 = CRT_PLL2_HS_193MHZ;
>> +       } else /* default to VGA clock */ {
>> +               pll1 = CRT_PLL1_HS_25MHZ;
>> +               pll2 = CRT_PLL2_HS_25MHZ;
>> +       }
>
> This seems like something that should be checked in atomic_check so
> you can be sure the mode is supported.
>
> It would also be nice to pull this out into a separate function (and a
> lookup table if you're feeling adventurous)

a lookup table seems good, thanks.

>
>> +
>> +       writel(pll2, hidev->mmio + CRT_PLL2_HS);
>> +       set_vclock_hisilicon(dev, pll1);
>> +
>> +       /* Hisilicon has to set up the top-left and bottom-right
>> +        * registers as well.
>> +        * Note that normal chip only use those two register for
>> +        * auto-centering mode.
>> +        */
>> +       writel((HIBMC_CRT_AUTO_CENTERING_TL_TOP(0) &
>> +               HIBMC_CRT_AUTO_CENTERING_TL_TOP_MSK) |
>> +              (HIBMC_CRT_AUTO_CENTERING_TL_LEFT(0) &
>> +               HIBMC_CRT_AUTO_CENTERING_TL_LEFT_MSK),
>> +              hidev->mmio + HIBMC_CRT_AUTO_CENTERING_TL);
>> +
>> +       writel((HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM(y - 1) &
>> +               HIBMC_CRT_AUTO_CENTERING_BR_BOTTOM_MASK) |
>> +              (HIBMC_CRT_AUTO_CENTERING_BR_RIGHT(x - 1) &
>> +               HIBMC_CRT_AUTO_CENTERING_BR_RIGHT_MASK),
>> +               hidev->mmio + HIBMC_CRT_AUTO_CENTERING_BR);
>> +
>> +       /* Assume common fields in ctrl have been properly set before
>> +        * calling this function.
>> +        * This function only sets the extra fields in ctrl.
>> +        */
>> +
>> +       /* Set bit 25 of display controller: Select CRT or VGA clock */
>> +       ctrl &= ~HIBMC_CRT_DISP_CTL_CRTSELECT_MASK;
>> +       ctrl &= ~HIBMC_CRT_DISP_CTL_CLOCK_PHASE_MASK;
>> +
>> +       ctrl |= HIBMC_CRT_DISP_CTL_CRTSELECT(CRTSELECT_CRT);
>> +
>> +       /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL, CRTSELECT, CRT);*/
>
> What's the deal with this commented code?

sorry, will clean up.

>
>> +
>> +       /* Set bit 14 of display controller */
>> +       /*ctrl &= FIELD_CLEAR(HIBMC_CRT_DISP_CTL, CLOCK_PHASE);*/
>> +
>> +       /* clock_phase_polarity is 0 */
>> +       ctrl |= HIBMC_CRT_DISP_CTL_CLOCK_PHASE(PHASE_ACTIVE_HIGH);
>> +       /*ctrl = FIELD_SET(ctrl, HIBMC_CRT_DISP_CTL,*/
>> +       /*CLOCK_PHASE, ACTIVE_HIGH);*/
>
> Here too...

ditto.

>
>> +
>> +       writel(ctrl, hidev->mmio + HIBMC_CRT_DISP_CTL);
>> +
>> +       return ctrl;
>> +}
>> +
>> +static void hibmc_crtc_mode_set_nofb(struct drm_crtc *crtc)
>> +{
>> +       unsigned int val;
>> +       struct drm_display_mode *mode = &crtc->state->mode;
>> +       struct drm_device *dev = crtc->dev;
>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>> +
>> +       writel(format_pll_reg(), hidev->mmio + HIBMC_CRT_PLL_CTRL);
>> +       writel((HIBMC_CRT_HORZ_TOTAL_TOTAL(mode->htotal - 1) &
>> +               HIBMC_CRT_HORZ_TOTAL_TOTAL_MASK) |
>> +               (HIBMC_CRT_HORZ_TOTAL_DISPLAY_END(mode->hdisplay - 1) &
>> +               HIBMC_CRT_HORZ_TOTAL_DISPLAY_END_MASK),
>
> You could probably macroize this code to make it more readable


	#define HIBMC_FIELD(field, value) (field(value) & filed##_MASK)

	writel(HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_TOTAL, mode->htotal - 1) |
	       HIBMC_FIELD(HIBMC_CRT_HORZ_TOTAL_DISPLAY_END, mode->hdisplay - 1),
	       hidev->mmio + HIBMC_CRT_HORZ_TOTAL);

Is above ok?
	


>
>> +               hidev->mmio + HIBMC_CRT_HORZ_TOTAL);
>> +
>> +       writel((HIBMC_CRT_HORZ_SYNC_WIDTH(mode->hsync_end - mode->hsync_start)
>> +               & HIBMC_CRT_HORZ_SYNC_WIDTH_MASK) |
>> +               (HIBMC_CRT_HORZ_SYNC_START(mode->hsync_start - 1)
>> +               & HIBMC_CRT_HORZ_SYNC_START_MASK),
>> +               hidev->mmio + HIBMC_CRT_HORZ_SYNC);
>> +
>> +       writel((HIBMC_CRT_VERT_TOTAL_TOTAL(mode->vtotal - 1) &
>> +               HIBMC_CRT_VERT_TOTAL_TOTAL_MASK) |
>> +               (HIBMC_CRT_VERT_TOTAL_DISPLAY_END(mode->vdisplay - 1) &
>> +               HIBMC_CRT_VERT_TOTAL_DISPLAY_END_MASK),
>> +               hidev->mmio + HIBMC_CRT_VERT_TOTAL);
>> +
>> +       writel((HIBMC_CRT_VERT_SYNC_HEIGHT(mode->vsync_end - mode->vsync_start)
>> +               & HIBMC_CRT_VERT_SYNC_HEIGHT_MASK) |
>> +              (HIBMC_CRT_VERT_SYNC_START(mode->vsync_start - 1) &
>> +               HIBMC_CRT_VERT_SYNC_START_MASK),
>> +               hidev->mmio + HIBMC_CRT_VERT_SYNC);
>> +
>> +       val = HIBMC_CRT_DISP_CTL_VSYNC_PHASE(0) &
>> +             HIBMC_CRT_DISP_CTL_VSYNC_PHASE_MASK;
>> +       val |= HIBMC_CRT_DISP_CTL_HSYNC_PHASE(0) &
>> +              HIBMC_CRT_DISP_CTL_HSYNC_PHASE_MASK;
>> +       val |= HIBMC_CRT_DISP_CTL_TIMING(ENABLE);
>> +       val |= HIBMC_CRT_DISP_CTL_PLANE(ENABLE);
>> +
>> +       display_ctrl_adjust(dev, mode, val);
>> +}
>> +
>> +static void hibmc_crtc_atomic_begin(struct drm_crtc *crtc,
>> +                                   struct drm_crtc_state *old_state)
>> +{
>> +       unsigned int reg;
>> +       struct drm_device *dev = crtc->dev;
>> +       struct hibmc_drm_device *hidev = dev->dev_private;
>> +
>> +       hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0);
>> +
>> +       /* Enable display power gate & LOCALMEM power gate*/
>> +       reg = readl(hidev->mmio + HIBMC_CURRENT_GATE);
>> +       reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK;
>> +       reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK;
>> +       reg |= HIBMC_CURR_GATE_DISPLAY(ON);
>> +       reg |= HIBMC_CURR_GATE_LOCALMEM(ON);
>> +       hibmc_set_current_gate(hidev, reg);
>> +
>> +       /* We can add more initialization as needed. */
>> +}
>> +
>> +static void hibmc_crtc_atomic_flush(struct drm_crtc *crtc,
>> +                                   struct drm_crtc_state *old_state)
>> +
>> +{
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&crtc->dev->event_lock, flags);
>> +       if (crtc->state->event)
>> +               drm_crtc_send_vblank_event(crtc, crtc->state->event);
>> +       crtc->state->event = NULL;
>> +
>> +       spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>> +}
>> +
>> +/* These provide the minimum set of functions required to handle a CRTC */
>
> nit: don't need this comment

will delete, thanks.

>
>> +static const struct drm_crtc_funcs hibmc_crtc_funcs = {
>> +       .page_flip = drm_atomic_helper_page_flip,
>> +       .set_config = drm_atomic_helper_set_config,
>> +       .destroy = drm_crtc_cleanup,
>> +       .reset = drm_atomic_helper_crtc_reset,
>> +       .atomic_duplicate_state =  drm_atomic_helper_crtc_duplicate_state,
>> +       .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> +};
>> +
>> +static const struct drm_crtc_helper_funcs hibmc_crtc_helper_funcs = {
>> +       .enable         = hibmc_crtc_enable,
>> +       .disable        = hibmc_crtc_disable,
>> +       .mode_set_nofb  = hibmc_crtc_mode_set_nofb,
>> +       .atomic_check   = hibmc_crtc_atomic_check,
>> +       .atomic_begin   = hibmc_crtc_atomic_begin,
>> +       .atomic_flush   = hibmc_crtc_atomic_flush,
>> +};
>> +
>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev)
>> +{
>> +       struct drm_device *dev = hidev->dev;
>> +       struct drm_crtc *crtc = &hidev->crtc;
>> +       struct drm_plane *plane = &hidev->plane;
>> +       int ret;
>> +
>> +       ret = drm_crtc_init_with_planes(dev, crtc, plane,
>> +                                       NULL, &hibmc_crtc_funcs, NULL);
>> +       if (ret) {
>> +               DRM_ERROR("failed to init crtc.\n");
>
> print return code

agreed, thanks.

>
>> +               return ret;
>> +       }
>> +
>> +       drm_mode_crtc_set_gamma_size(crtc, 256);
>
> check return code

agreed though none of other drivers has done this,
thanks.

>
>> +       drm_crtc_helper_add(crtc, &hibmc_crtc_helper_funcs);
>> +       return 0;
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 7d96583..303cd36 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -119,6 +119,12 @@ static int hibmc_kms_init(struct hibmc_drm_device *hidev)
>>                  return ret;
>>          }
>>
>> +       ret = hibmc_crtc_init(hidev);
>> +       if (ret) {
>> +               DRM_ERROR("failed to init crtc.\n");
>> +               return ret;
>> +       }
>
> Typically the plane is initialized internally in the crtc driver. I
> think this is a good design pattern, and you should probably use it.
>
> So how about squashing this down with the plane patch and keeping the
> plane inside hibmc_drm_de.c?

understood after i looked at intel_display.c, this file will be merged
with patch 4/9, and the tile will be: 'drm/hisilicon/hibmc: Add display
engine'.

>
>> +
>>          return 0;
>>   }
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 49e39d2..5731ec2 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -46,6 +46,7 @@ struct hibmc_drm_device {
>>          /* drm */
>>          struct drm_device  *dev;
>>          struct drm_plane plane;
>
> I don't think you should be keeping track of plane here. plane is only
> used in the crtc init, which should be addressed by the previous
> comment.

so allocate with devm_kzalloc(sizeof(*plane)) and remove it from
hibmc_drm_device?

>
>
>> +       struct drm_crtc crtc;
>
> crtc is only used in the irq handler, so you could remove this here
> and just call drm_handle_vblank(dev, 0) in the handler.

so allocate with devm_kzalloc(sizeof(*crtc)) and remove it from
hibmc_drm_device, when driver unload drm_crtc_cleanup() will be
called and finally memory will be freed before quit.

>
>
>>          bool mode_config_initialized;
>>
>>          /* ttm */
>> @@ -85,6 +86,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>>   #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>
>>   int hibmc_plane_init(struct hibmc_drm_device *hidev);
>> +int hibmc_crtc_init(struct hibmc_drm_device *hidev);
>>   int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>   void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> linuxarm mailing list
> linuxarm at huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>


-- 
Regards, Rongrong

^ permalink raw reply

* [PATCH v2 2/2] arm64: dts: Add ARM PMU node for exynos7
From: Alim Akhtar @ 2016-11-12 10:17 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478945832-1826-1-git-send-email-alim.akhtar@samsung.com>

This patch adds ARM Performance Monitor Unit dt node for exynos7.
PMU provides various statistics on the operation of the CPU and
memory system at runtime, which are very useful when debugging or
profiling code. This enables the same.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos7.dtsi |   10 ++++++++++
 1 file changed, 10 insertions(+)

Changes since v1:
* Added "interrupt-affinity" property as per Robin Murphy review comment.

diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
index 396ffb9..09e7a05b 100644
--- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
@@ -472,6 +472,16 @@
 			status = "disabled";
 		};
 
+		arm-pmu {
+			compatible = "arm,cortex-a57-pmu", "arm,armv8-pmuv3";
+			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-affinity = <&cpu_atlas0>, <&cpu_atlas1>,
+					     <&cpu_atlas2>, <&cpu_atlas3>;
+		};
+
 		timer {
 			compatible = "arm,armv8-timer";
 			interrupts = <GIC_PPI 13
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH v2 1/2] arm64: dts: Add level for cpu dt node for exynos7
From: Alim Akhtar @ 2016-11-12 10:17 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds level for cpu dt node, so that these levels can be used
as a phandle whenever required. For example, adding a "interrupt-affinity"
for arm pmu node.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 arch/arm64/boot/dts/exynos/exynos7.dtsi |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
index e0d0d01..396ffb9 100644
--- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
@@ -35,28 +35,28 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		cpu at 0 {
+		cpu_atlas0: cpu at 0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a57", "arm,armv8";
 			reg = <0x0>;
 			enable-method = "psci";
 		};
 
-		cpu at 1 {
+		cpu_atlas1: cpu at 1 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a57", "arm,armv8";
 			reg = <0x1>;
 			enable-method = "psci";
 		};
 
-		cpu at 2 {
+		cpu_atlas2: cpu at 2 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a57", "arm,armv8";
 			reg = <0x2>;
 			enable-method = "psci";
 		};
 
-		cpu at 3 {
+		cpu_atlas3: cpu at 3 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a57", "arm,armv8";
 			reg = <0x3>;
-- 
1.7.10.4

^ permalink raw reply related

* PM regression with LED changes in next-20161109
From: Hans de Goede @ 2016-11-12  8:03 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161111221224.GB10983@amd>

Hi,

On 11-11-16 23:12, Pavel Machek wrote:
> Hi!
>
> Reason #1:
>
>>>> Hmm. So userland can read the LED state, and it can get _some_ value
>>>> back, but it can not know if it is current state or not.

That is not correct, the current behavior for eading the brightness
atrribute is to always return the current state.

>> Why a dedicated file? Are we going to mirror brightness here
>> wrt r/w (show/store) behavior ? If not userspace now needs
>> 2 open fds which is not really nice. If we are and we are
>> not going to use poll for something else on brightness itself
>> then why not just poll directly on brightness ?
>
> Reason #1 is above.

See my reply above.

> Reason #2 is "if userspace sees brightness file, it can not know if
> the notifications on change actually work or not".

If it needs to know that it can simply check the kernel version.

> Reason #3 is that you broke Tony's system. Polling does not make sense
> when trigger such as "CPU in use" is active.

Have you seen v4 of my patch? It fixes this while keeping the
polling on the brightness attribute itself, it basically goes
back (more or less) to v1 of my patch which did not have this
problem. I never wanted notification of trigger / blinking
changes because I already feared Tony's problem would happen.

> Reason #4 is that there are really two brightnesses:
>
> 1) maximum brightness trigger is going to use
>
> 2) current brightness
>
> Currently writing to "brightness" file changes 1), but reading returns
> 2) when available.

Right and Jacek has already said that we cannot change the
reading behavior on the brightness file because of ABI concerns.

So if anything we need a new blink_brightness file or such,
which when read shows the maximum brightness when blinking or
triggers are active. Note that we already have a max_brightness
file which is the actual maximum brightness the led supports.

Since the existing ABI behavior is for the existing brightness
file to return the *current* brightness, please explain to me
how polling on say the new blink_brightness file would make
sense to detect changes in the current brightness ?

> So, feel free to propose better interface. One that solves #1..#4
> above.

Proposal 1:

v4 of my patch, see the list. It solves all but #4, which
is out of scope for my patch, feel free to submit a patch to
solve #4 (with a new sysfs attr).

Proposal 2:

Add a new "user_brightness" file, which shows the last brightness
as set by the user, this would show the read behavior we really
want of brightness: show the real brightness when not blinking /
triggers are active, show the brightness used when on when
blinking / triggers are active.

And then we could add poll support on this new user_brightness
file, thus avoiding the problem with the extra cpu-load on
notifications on blinking / triggers.

Regards,

Hans

^ permalink raw reply

* [PATCH 10/10] ARM: dts: sun8i-h3: orange-pi-pc: Enable audio codec
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The Orange Pi PC routes the LINEOUT pins to the audio out jack on the
board. The onboard microphone is routed to MIC1, with MBIAS providing
power.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
index 3ec971285aa3..2b1e68d0f2fa 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
@@ -90,6 +90,14 @@
 	};
 };
 
+&codec {
+	allwinner,audio-routing =
+		"Line Out", "LINEOUT",
+		"MIC1", "Mic",
+		"Mic",  "MBIAS";
+	status = "okay";
+};
+
 &ehci1 {
 	status = "okay";
 };
-- 
2.10.2

^ permalink raw reply related

* [PATCH 09/10] ARM: dts: sun8i-h3: Add device nodes for audio codec and its analog controls
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

Now that we support the audio codec found on the Allwinner H3 SoC, add
device nodes for it.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-h3.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index c38b028cac83..ceec979f57e4 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -485,6 +485,20 @@
 			status = "disabled";
 		};
 
+		codec: codec at 01c22c00 {
+			#sound-dai-cells = <0>;
+			compatible = "allwinner,sun8i-h3-codec";
+			reg = <0x01c22c00 0x400>;
+			interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CODEC>, <&ccu CLK_AC_DIG>;
+			clock-names = "apb", "codec";
+			resets = <&ccu RST_BUS_CODEC>;
+			dmas = <&dma 15>, <&dma 15>;
+			dma-names = "rx", "tx";
+			allwinner,codec-analog-controls = <&codec_analog>;
+			status = "disabled";
+		};
+
 		uart0: serial at 01c28000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28000 0x400>;
@@ -600,6 +614,11 @@
 			#reset-cells = <1>;
 		};
 
+		codec_analog: codec-analog at 01f015c0 {
+			compatible = "allwinner,sun8i-h3-codec-analog";
+			reg = <0x01f015c0 0x4>;
+		};
+
 		ir: ir at 01f02000 {
 			compatible = "allwinner,sun5i-a13-ir";
 			clocks = <&apb0_gates 1>, <&ir_clk>;
-- 
2.10.2

^ permalink raw reply related

* [PATCH 08/10] ASoC: sun4i-codec: Add support for H3 codec
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The codec on the H3 is similar to the one found on the A31. One key
difference is the analog path controls are routed through the PRCM
block. This is supported by the sun8i-codec-analog driver, and tied
into this codec driver with the audio card's aux_dev.

In addition, the H3 has no HP (headphone) and HBIAS support, and no
MIC3 input. The FIFO related registers are slightly rearranged.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 .../devicetree/bindings/sound/sun4i-codec.txt      |  3 +
 sound/soc/sunxi/sun4i-codec.c                      | 71 ++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/sun4i-codec.txt b/Documentation/devicetree/bindings/sound/sun4i-codec.txt
index f7a548b604fc..3033bd8aab0f 100644
--- a/Documentation/devicetree/bindings/sound/sun4i-codec.txt
+++ b/Documentation/devicetree/bindings/sound/sun4i-codec.txt
@@ -6,6 +6,7 @@ Required properties:
 		- "allwinner,sun6i-a31-codec"
 		- "allwinner,sun7i-a20-codec"
 		- "allwinner,sun8i-a23-codec"
+		- "allwinner,sun8i-h3-codec"
 - reg: must contain the registers location and length
 - interrupts: must contain the codec interrupt
 - dmas: DMA channels for tx and rx dma. See the DMA client binding,
@@ -23,6 +24,7 @@ Optional properties:
 Required properties for the following compatibles:
 		- "allwinner,sun6i-a31-codec"
 		- "allwinner,sun8i-a23-codec"
+		- "allwinner,sun8i-h3-codec"
 - resets: phandle to the reset control for this device
 - allwinner,audio-routing: A list of the connections between audio components.
 			   Each entry is a pair of strings, the first being the
@@ -52,6 +54,7 @@ Required properties for the following compatibles:
 
 Required properties for the following compatibles:
 		- "allwinner,sun8i-a23-codec"
+		- "allwinner,sun8i-h3-codec"
 - allwinner,codec-analog-controls: A phandle to the codec analog controls
 				   block in the PRCM.
 
diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 3c5ef1724163..1a5acadf65d7 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -217,6 +217,13 @@
 #define SUN8I_A23_CODEC_DAC_TXCNT		(0x1c)
 #define SUN8I_A23_CODEC_ADC_RXCNT		(0x20)
 
+/* TX FIFO moved on H3 */
+#define SUN8I_H3_CODEC_DAC_TXDATA		(0x20)
+#define SUN8I_H3_CODEC_DAC_DBG			(0x48)
+#define SUN8I_H3_CODEC_ADC_DBG			(0x4c)
+
+/* TODO H3 DAP (Digital Audio Processing) bits */
+
 struct sun4i_codec {
 	struct device	*dev;
 	struct regmap	*regmap;
@@ -1293,6 +1300,44 @@ static struct snd_soc_card *sun8i_a23_codec_create_card(struct device *dev)
 	return card;
 };
 
+static struct snd_soc_card *sun8i_h3_codec_create_card(struct device *dev)
+{
+	struct snd_soc_card *card;
+	int ret;
+
+	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
+	if (!card)
+		return ERR_PTR(-ENOMEM);
+
+	aux_dev.codec_of_node = of_parse_phandle(dev->of_node,
+						 "allwinner,codec-analog-controls",
+						 0);
+	if (!aux_dev.codec_of_node) {
+		dev_err(dev, "Can't find analog controls for codec.\n");
+		return ERR_PTR(-EINVAL);
+	};
+
+	card->dai_link = sun4i_codec_create_link(dev, &card->num_links);
+	if (!card->dai_link)
+		return ERR_PTR(-ENOMEM);
+
+	card->dev		= dev;
+	card->name		= "H3 Audio Codec";
+	card->dapm_widgets	= sun6i_codec_card_dapm_widgets;
+	card->num_dapm_widgets	= ARRAY_SIZE(sun6i_codec_card_dapm_widgets);
+	card->dapm_routes	= sun8i_codec_card_routes;
+	card->num_dapm_routes	= ARRAY_SIZE(sun8i_codec_card_routes);
+	card->aux_dev		= &aux_dev;
+	card->num_aux_devs	= 1;
+	card->fully_routed	= true;
+
+	ret = snd_soc_of_parse_audio_routing(card, "allwinner,audio-routing");
+	if (ret)
+		dev_warn(dev, "failed to parse audio-routing: %d\n", ret);
+
+	return card;
+};
+
 static const struct regmap_config sun4i_codec_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
@@ -1321,6 +1366,13 @@ static const struct regmap_config sun8i_a23_codec_regmap_config = {
 	.max_register	= SUN8I_A23_CODEC_ADC_RXCNT,
 };
 
+static const struct regmap_config sun8i_h3_codec_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= SUN8I_H3_CODEC_ADC_DBG,
+};
+
 struct sun4i_codec_quirks {
 	const struct regmap_config *regmap_config;
 	const struct snd_soc_codec_driver *codec;
@@ -1369,6 +1421,21 @@ static const struct sun4i_codec_quirks sun8i_a23_codec_quirks = {
 	.has_reset	= true,
 };
 
+static const struct sun4i_codec_quirks sun8i_h3_codec_quirks = {
+	.regmap_config	= &sun8i_h3_codec_regmap_config,
+	/*
+	 * TODO Share the codec structure with A23 for now.
+	 * This should be split out when adding digital audio
+	 * processing support for the H3.
+	 */
+	.codec		= &sun8i_a23_codec_codec,
+	.create_card	= sun8i_h3_codec_create_card,
+	.reg_adc_fifoc	= REG_FIELD(SUN6I_CODEC_ADC_FIFOC, 0, 31),
+	.reg_dac_txdata	= SUN8I_H3_CODEC_DAC_TXDATA,
+	.reg_adc_rxdata	= SUN6I_CODEC_ADC_RXDATA,
+	.has_reset	= true,
+};
+
 static const struct of_device_id sun4i_codec_of_match[] = {
 	{
 		.compatible = "allwinner,sun4i-a10-codec",
@@ -1386,6 +1453,10 @@ static const struct of_device_id sun4i_codec_of_match[] = {
 		.compatible = "allwinner,sun8i-a23-codec",
 		.data = &sun8i_a23_codec_quirks,
 	},
+	{
+		.compatible = "allwinner,sun8i-h3-codec",
+		.data = &sun8i_h3_codec_quirks,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, sun4i_codec_of_match);
-- 
2.10.2

^ permalink raw reply related

* [PATCH 07/10] ARM: dts: sun8i-a23: q8-tablet: Enable internal audio codec
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The A23 Q8 tablets have an internal mono speaker w/ external amp
which has a shutdown control tied to a GPIO pin. Both the speaker
amp and the headphone jack are tied to the HP output pins. While
the speaker is mono, the headset jack is stereo. Unfortunately
the driver does not support automatic switching of this.

In addition, the headset is DC coupled, or "direct drive" enabled.
The headset's microphone is tied to MIC2 with HBIAS providing power.
A separate internal microphone is tied to MIC1 with MBIAS providing
power.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-a23-q8-tablet.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a23-q8-tablet.dts b/arch/arm/boot/dts/sun8i-a23-q8-tablet.dts
index 956320a6cc78..3ab5c0c09d93 100644
--- a/arch/arm/boot/dts/sun8i-a23-q8-tablet.dts
+++ b/arch/arm/boot/dts/sun8i-a23-q8-tablet.dts
@@ -48,3 +48,26 @@
 	model = "Q8 A23 Tablet";
 	compatible = "allwinner,q8-a23", "allwinner,sun8i-a23";
 };
+
+&codec {
+	pinctrl-0 = <&codec_pa_pin>;
+	allwinner,pa-gpios = <&pio 7 9 GPIO_ACTIVE_HIGH>; /* PH9 */
+	allwinner,audio-routing =
+		"Headphone", "HP",
+		"Headphone", "HPCOM",
+		"Speaker", "HP",
+		"MIC1", "Mic",
+		"MIC2", "Headset Mic",
+		"Mic",  "MBIAS",
+		"Headset Mic", "HBIAS";
+	status = "okay";
+};
+
+&pio {
+	codec_pa_pin: codec_pa_pin at 0 {
+		allwinner,pins = "PH9";
+		allwinner,function = "gpio_out";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+	};
+};
-- 
2.10.2

^ permalink raw reply related

* [PATCH 06/10] ARM: dts: sun8i-a23: Add device node for internal audio codec
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

Now that we have a device tree binding and driver for the A23's
internal audio codec, add a device node for it.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-a23.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a23.dtsi b/arch/arm/boot/dts/sun8i-a23.dtsi
index 54d045dab825..4d1f929780a8 100644
--- a/arch/arm/boot/dts/sun8i-a23.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23.dtsi
@@ -48,6 +48,22 @@
 	memory {
 		reg = <0x40000000 0x40000000>;
 	};
+
+	soc at 01c00000 {
+		codec: codec at 01c22c00 {
+			#sound-dai-cells = <0>;
+			compatible = "allwinner,sun8i-a23-codec";
+			reg = <0x01c22c00 0x400>;
+			interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CODEC>, <&ccu CLK_AC_DIG>;
+			clock-names = "apb", "codec";
+			resets = <&ccu RST_BUS_CODEC>;
+			dmas = <&dma 15>, <&dma 15>;
+			dma-names = "rx", "tx";
+			allwinner,codec-analog-controls = <&codec_analog>;
+			status = "disabled";
+		};
+	};
 };
 
 &ccu {
-- 
2.10.2

^ permalink raw reply related

* [PATCH 05/10] ARM: dts: sun8i: Add codec analog path controls node in PRCM for A23/A33
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

On the A23/A33, the internal codec's analog path controls are located in
the PRCM node.

Add a sub-device node to the PRCM for it.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-a23-a33.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
index 300a1bd5a6ec..ac0d281f4489 100644
--- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi
@@ -553,6 +553,10 @@
 				compatible = "allwinner,sun6i-a31-clock-reset";
 				#reset-cells = <1>;
 			};
+
+			codec_analog: codec-analog {
+				compatible = "allwinner,sun8i-a23-codec-analog";
+			};
 		};
 
 		cpucfg at 01f01c00 {
-- 
2.10.2

^ permalink raw reply related

* [PATCH 04/10] ASoC: sun4i-codec: Add support for A23 codec
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The codec in the A23 is similar to the one found on the A31. One key
difference is the analog path controls are routed through the PRCM
block. This is supported by the sun8i-codec-analog driver, and tied
into this codec driver with the audio card's aux_dev.

In addition, the A23 does not have LINEOUT, and it does not support
headset jack detection or buttons.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 .../devicetree/bindings/sound/sun4i-codec.txt      |  11 ++-
 sound/soc/sunxi/sun4i-codec.c                      | 108 +++++++++++++++++++++
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/sun4i-codec.txt b/Documentation/devicetree/bindings/sound/sun4i-codec.txt
index d91a95377f49..f7a548b604fc 100644
--- a/Documentation/devicetree/bindings/sound/sun4i-codec.txt
+++ b/Documentation/devicetree/bindings/sound/sun4i-codec.txt
@@ -5,6 +5,7 @@ Required properties:
 		- "allwinner,sun4i-a10-codec"
 		- "allwinner,sun6i-a31-codec"
 		- "allwinner,sun7i-a20-codec"
+		- "allwinner,sun8i-a23-codec"
 - reg: must contain the registers location and length
 - interrupts: must contain the codec interrupt
 - dmas: DMA channels for tx and rx dma. See the DMA client binding,
@@ -21,6 +22,7 @@ Optional properties:
 
 Required properties for the following compatibles:
 		- "allwinner,sun6i-a31-codec"
+		- "allwinner,sun8i-a23-codec"
 - resets: phandle to the reset control for this device
 - allwinner,audio-routing: A list of the connections between audio components.
 			   Each entry is a pair of strings, the first being the
@@ -31,10 +33,10 @@ Required properties for the following compatibles:
 			   "HP"
 			   "HPCOM"
 			   "LINEIN"
-			   "LINEOUT"
+			   "LINEOUT"	(not on sun8i-a23)
 			   "MIC1"
 			   "MIC2"
-			   "MIC3"
+			   "MIC3"	(sun6i-a31 only)
 
 			   Microphone biases from the SoC:
 			   "HBIAS"
@@ -48,6 +50,11 @@ Required properties for the following compatibles:
 			   "Mic"
 			   "Speaker"
 
+Required properties for the following compatibles:
+		- "allwinner,sun8i-a23-codec"
+- allwinner,codec-analog-controls: A phandle to the codec analog controls
+				   block in the PRCM.
+
 Example:
 codec: codec at 01c22c00 {
 	#sound-dai-cells = <0>;
diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 6379efd21f00..3c5ef1724163 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -213,6 +213,10 @@
 
 /* TODO sun6i DAP (Digital Audio Processing) bits */
 
+/* FIFO counters moved on A23 */
+#define SUN8I_A23_CODEC_DAC_TXCNT		(0x1c)
+#define SUN8I_A23_CODEC_ADC_RXCNT		(0x20)
+
 struct sun4i_codec {
 	struct device	*dev;
 	struct regmap	*regmap;
@@ -1067,6 +1071,32 @@ static struct snd_soc_codec_driver sun6i_codec_codec = {
 	},
 };
 
+/* sun8i A23 codec */
+static const struct snd_kcontrol_new sun8i_a23_codec_codec_controls[] = {
+	SOC_SINGLE_TLV("DAC Playback Volume", SUN4I_CODEC_DAC_DPC,
+		       SUN4I_CODEC_DAC_DPC_DVOL, 0x3f, 1,
+		       sun6i_codec_dvol_scale),
+};
+
+static const struct snd_soc_dapm_widget sun8i_a23_codec_codec_widgets[] = {
+	/* Digital parts of the ADCs */
+	SND_SOC_DAPM_SUPPLY("ADC Enable", SUN6I_CODEC_ADC_FIFOC,
+			    SUN6I_CODEC_ADC_FIFOC_EN_AD, 0, NULL, 0),
+	/* Digital parts of the DACs */
+	SND_SOC_DAPM_SUPPLY("DAC Enable", SUN4I_CODEC_DAC_DPC,
+			    SUN4I_CODEC_DAC_DPC_EN_DA, 0, NULL, 0),
+
+};
+
+static struct snd_soc_codec_driver sun8i_a23_codec_codec = {
+	.component_driver = {
+		.controls		= sun8i_a23_codec_codec_controls,
+		.num_controls		= ARRAY_SIZE(sun8i_a23_codec_codec_controls),
+		.dapm_widgets		= sun8i_a23_codec_codec_widgets,
+		.num_dapm_widgets	= ARRAY_SIZE(sun8i_a23_codec_codec_widgets),
+	},
+};
+
 static const struct snd_soc_component_driver sun4i_codec_component = {
 	.name = "sun4i-codec",
 };
@@ -1206,6 +1236,63 @@ static struct snd_soc_card *sun6i_codec_create_card(struct device *dev)
 	return card;
 };
 
+/* Connect digital side enables to analog side widgets */
+static const struct snd_soc_dapm_route sun8i_codec_card_routes[] = {
+	/* ADC Routes */
+	{ "Left ADC", NULL, "ADC Enable" },
+	{ "Right ADC", NULL, "ADC Enable" },
+	{ "Codec Capture", NULL, "Left ADC" },
+	{ "Codec Capture", NULL, "Right ADC" },
+
+	/* DAC Routes */
+	{ "Left DAC", NULL, "DAC Enable" },
+	{ "Right DAC", NULL, "DAC Enable" },
+	{ "Left DAC", NULL, "Codec Playback" },
+	{ "Right DAC", NULL, "Codec Playback" },
+};
+
+static struct snd_soc_aux_dev aux_dev = {
+	.name = "Codec Analog Controls",
+};
+
+static struct snd_soc_card *sun8i_a23_codec_create_card(struct device *dev)
+{
+	struct snd_soc_card *card;
+	int ret;
+
+	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
+	if (!card)
+		return ERR_PTR(-ENOMEM);
+
+	aux_dev.codec_of_node = of_parse_phandle(dev->of_node,
+						 "allwinner,codec-analog-controls",
+						 0);
+	if (!aux_dev.codec_of_node) {
+		dev_err(dev, "Can't find analog controls for codec.\n");
+		return ERR_PTR(-EINVAL);
+	};
+
+	card->dai_link = sun4i_codec_create_link(dev, &card->num_links);
+	if (!card->dai_link)
+		return ERR_PTR(-ENOMEM);
+
+	card->dev		= dev;
+	card->name		= "A23 Audio Codec";
+	card->dapm_widgets	= sun6i_codec_card_dapm_widgets;
+	card->num_dapm_widgets	= ARRAY_SIZE(sun6i_codec_card_dapm_widgets);
+	card->dapm_routes	= sun8i_codec_card_routes;
+	card->num_dapm_routes	= ARRAY_SIZE(sun8i_codec_card_routes);
+	card->aux_dev		= &aux_dev;
+	card->num_aux_devs	= 1;
+	card->fully_routed	= true;
+
+	ret = snd_soc_of_parse_audio_routing(card, "allwinner,audio-routing");
+	if (ret)
+		dev_warn(dev, "failed to parse audio-routing: %d\n", ret);
+
+	return card;
+};
+
 static const struct regmap_config sun4i_codec_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
@@ -1227,6 +1314,13 @@ static const struct regmap_config sun7i_codec_regmap_config = {
 	.max_register	= SUN7I_CODEC_AC_MIC_PHONE_CAL,
 };
 
+static const struct regmap_config sun8i_a23_codec_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= SUN8I_A23_CODEC_ADC_RXCNT,
+};
+
 struct sun4i_codec_quirks {
 	const struct regmap_config *regmap_config;
 	const struct snd_soc_codec_driver *codec;
@@ -1265,6 +1359,16 @@ static const struct sun4i_codec_quirks sun7i_codec_quirks = {
 	.reg_adc_rxdata	= SUN4I_CODEC_ADC_RXDATA,
 };
 
+static const struct sun4i_codec_quirks sun8i_a23_codec_quirks = {
+	.regmap_config	= &sun8i_a23_codec_regmap_config,
+	.codec		= &sun8i_a23_codec_codec,
+	.create_card	= sun8i_a23_codec_create_card,
+	.reg_adc_fifoc	= REG_FIELD(SUN6I_CODEC_ADC_FIFOC, 0, 31),
+	.reg_dac_txdata	= SUN4I_CODEC_DAC_TXDATA,
+	.reg_adc_rxdata	= SUN6I_CODEC_ADC_RXDATA,
+	.has_reset	= true,
+};
+
 static const struct of_device_id sun4i_codec_of_match[] = {
 	{
 		.compatible = "allwinner,sun4i-a10-codec",
@@ -1278,6 +1382,10 @@ static const struct of_device_id sun4i_codec_of_match[] = {
 		.compatible = "allwinner,sun7i-a20-codec",
 		.data = &sun7i_codec_quirks,
 	},
+	{
+		.compatible = "allwinner,sun8i-a23-codec",
+		.data = &sun8i_a23_codec_quirks,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, sun4i_codec_of_match);
-- 
2.10.2

^ permalink raw reply related

* [PATCH 03/10] mfd: sun6i-prcm: Add codec analog controls sub-device for Allwinner A23
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The PRCM block on the A23 contains a message box like interface to
the registers for the analog path controls of the internal codec.

Add a sub-device for it.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/mfd/sun6i-prcm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mfd/sun6i-prcm.c b/drivers/mfd/sun6i-prcm.c
index 011fcc555945..4abbf2ede944 100644
--- a/drivers/mfd/sun6i-prcm.c
+++ b/drivers/mfd/sun6i-prcm.c
@@ -12,6 +12,9 @@
 #include <linux/init.h>
 #include <linux/of.h>
 
+#define SUN8I_CODEC_ANALOG_BASE	0x1c0
+#define SUN8I_CODEC_ANALOG_END	(SUN8I_CODEC_ANALOG_BASE + 0x4 - 1)
+
 struct prcm_data {
 	int nsubdevs;
 	const struct mfd_cell *subdevs;
@@ -57,6 +60,14 @@ static const struct resource sun6i_a31_apb0_rstc_res[] = {
 	},
 };
 
+static const struct resource sun8i_codec_analog_res[] = {
+	{
+		.start	= SUN8I_CODEC_ANALOG_BASE,
+		.end	= SUN8I_CODEC_ANALOG_END,
+		.flags	= IORESOURCE_MEM,
+	},
+};
+
 static const struct mfd_cell sun6i_a31_prcm_subdevs[] = {
 	{
 		.name = "sun6i-a31-ar100-clk",
@@ -109,6 +120,12 @@ static const struct mfd_cell sun8i_a23_prcm_subdevs[] = {
 		.num_resources = ARRAY_SIZE(sun6i_a31_apb0_rstc_res),
 		.resources = sun6i_a31_apb0_rstc_res,
 	},
+	{
+		.name		= "sun8i-codec-analog",
+		.of_compatible	= "allwinner,sun8i-a23-codec-analog",
+		.num_resources	= ARRAY_SIZE(sun8i_codec_analog_res),
+		.resources	= sun8i_codec_analog_res,
+	},
 };
 
 static const struct prcm_data sun6i_a31_prcm_data = {
-- 
2.10.2

^ permalink raw reply related

* [PATCH 02/10] ASoC: sunxi: Add support for A23/A33/H3 codec's analog path controls
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The internal codec on A23/A33/H3 is split into 2 parts. The
analog path controls are routed through an embedded custom register
bus accessed through the PRCM block.

The SoCs share a common set of inputs, outputs, and audio paths.
The following table lists the differences.

    ----------------------------------------
    | Feature \ SoC |  A23  |  A33  |  H3  |
    ----------------------------------------
    | Headphone     |   v   |   v   |      |
    ----------------------------------------
    | Line Out      |       |       |  v   |
    ----------------------------------------
    | Phone In/Out  |   v   |   v   |      |
    ----------------------------------------

Add an ASoC component driver for it. This should be tied to the codec
audio card as an auxiliary device. This patch adds the commont paths
and controls, and variant specific headphone out and line out.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 sound/soc/sunxi/Kconfig              |   8 +
 sound/soc/sunxi/Makefile             |   1 +
 sound/soc/sunxi/sun8i-codec-analog.c | 665 +++++++++++++++++++++++++++++++++++
 3 files changed, 674 insertions(+)
 create mode 100644 sound/soc/sunxi/sun8i-codec-analog.c

diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
index dd2368297fd3..6c344e16aca4 100644
--- a/sound/soc/sunxi/Kconfig
+++ b/sound/soc/sunxi/Kconfig
@@ -9,6 +9,14 @@ config SND_SUN4I_CODEC
 	  Select Y or M to add support for the Codec embedded in the Allwinner
 	  A10 and affiliated SoCs.
 
+config SND_SUN8I_CODEC_ANALOG
+	tristate "Allwinner sun8i Codec Analog Controls Support"
+	depends on MACH_SUN8I || COMPILE_TEST
+	select REGMAP
+	help
+	  Say Y or M if you want to add support for the analog controls for
+	  the codec embedded in newer Allwinner SoCs.
+
 config SND_SUN4I_I2S
 	tristate "Allwinner A10 I2S Support"
 	select SND_SOC_GENERIC_DMAENGINE_PCM
diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
index 604c7b842837..241c0df9ca0c 100644
--- a/sound/soc/sunxi/Makefile
+++ b/sound/soc/sunxi/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_SND_SUN4I_CODEC) += sun4i-codec.o
 obj-$(CONFIG_SND_SUN4I_I2S) += sun4i-i2s.o
 obj-$(CONFIG_SND_SUN4I_SPDIF) += sun4i-spdif.o
+obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) += sun8i-codec-analog.o
diff --git a/sound/soc/sunxi/sun8i-codec-analog.c b/sound/soc/sunxi/sun8i-codec-analog.c
new file mode 100644
index 000000000000..222bbd440b1e
--- /dev/null
+++ b/sound/soc/sunxi/sun8i-codec-analog.c
@@ -0,0 +1,665 @@
+/*
+ * This driver supports the analog controls for the internal codec
+ * found in Allwinner's A31s, A23, A33 and H3 SoCs.
+ *
+ * Copyright 2016 Chen-Yu Tsai <wens@csie.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <sound/soc.h>
+#include <sound/soc-dapm.h>
+#include <sound/tlv.h>
+
+/* Codec analog control register offsets and bit fields */
+#define SUN8I_ADDA_HP_VOLC		0x00
+#define SUN8I_ADDA_HP_VOLC_PA_CLK_GATE		7
+#define SUN8I_ADDA_HP_VOLC_HP_VOL		0
+#define SUN8I_ADDA_LOMIXSC		0x01
+#define SUN8I_ADDA_LOMIXSC_MIC1			6
+#define SUN8I_ADDA_LOMIXSC_MIC2			5
+#define SUN8I_ADDA_LOMIXSC_PHONE		4
+#define SUN8I_ADDA_LOMIXSC_PHONEN		3
+#define SUN8I_ADDA_LOMIXSC_LINEINL		2
+#define SUN8I_ADDA_LOMIXSC_DACL			1
+#define SUN8I_ADDA_LOMIXSC_DACR			0
+#define SUN8I_ADDA_ROMIXSC		0x02
+#define SUN8I_ADDA_ROMIXSC_MIC1			6
+#define SUN8I_ADDA_ROMIXSC_MIC2			5
+#define SUN8I_ADDA_ROMIXSC_PHONE		4
+#define SUN8I_ADDA_ROMIXSC_PHONEP		3
+#define SUN8I_ADDA_ROMIXSC_LINEINR		2
+#define SUN8I_ADDA_ROMIXSC_DACR			1
+#define SUN8I_ADDA_ROMIXSC_DACL			0
+#define SUN8I_ADDA_DAC_PA_SRC		0x03
+#define SUN8I_ADDA_DAC_PA_SRC_DACAREN		7
+#define SUN8I_ADDA_DAC_PA_SRC_DACALEN		6
+#define SUN8I_ADDA_DAC_PA_SRC_RMIXEN		5
+#define SUN8I_ADDA_DAC_PA_SRC_LMIXEN		4
+#define SUN8I_ADDA_DAC_PA_SRC_RHPPAMUTE		3
+#define SUN8I_ADDA_DAC_PA_SRC_LHPPAMUTE		2
+#define SUN8I_ADDA_DAC_PA_SRC_RHPIS		1
+#define SUN8I_ADDA_DAC_PA_SRC_LHPIS		0
+#define SUN8I_ADDA_PHONEIN_GCTRL	0x04
+#define SUN8I_ADDA_PHONEIN_GCTRL_PHONEPG	4
+#define SUN8I_ADDA_PHONEIN_GCTRL_PHONENG	0
+#define SUN8I_ADDA_LINEIN_GCTRL		0x05
+#define SUN8I_ADDA_LINEIN_GCTRL_LINEING		4
+#define SUN8I_ADDA_LINEIN_GCTRL_PHONEG		0
+#define SUN8I_ADDA_MICIN_GCTRL		0x06
+#define SUN8I_ADDA_MICIN_GCTRL_MIC1G		4
+#define SUN8I_ADDA_MICIN_GCTRL_MIC2G		0
+#define SUN8I_ADDA_PAEN_HP_CTRL		0x07
+#define SUN8I_ADDA_PAEN_HP_CTRL_HPPAEN		7
+#define SUN8I_ADDA_PAEN_HP_CTRL_LINEOUTEN	7	/* H3 specific */
+#define SUN8I_ADDA_PAEN_HP_CTRL_HPCOM_FC	5
+#define SUN8I_ADDA_PAEN_HP_CTRL_COMPTEN		4
+#define SUN8I_ADDA_PAEN_HP_CTRL_PA_ANTI_POP_CTRL	2
+#define SUN8I_ADDA_PAEN_HP_CTRL_LTRNMUTE	1
+#define SUN8I_ADDA_PAEN_HP_CTRL_RTLNMUTE	0
+#define SUN8I_ADDA_PHONEOUT_CTRL	0x08
+#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTG	5
+#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTEN	4
+#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUT_MIC1	3
+#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUT_MIC2	2
+#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUT_RMIX	1
+#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUT_LMIX	0
+#define SUN8I_ADDA_PHONE_GAIN_CTRL	0x09
+#define SUN8I_ADDA_PHONE_GAIN_CTRL_LINEOUT_VOL	3
+#define SUN8I_ADDA_PHONE_GAIN_CTRL_PHONEPREG	0
+#define SUN8I_ADDA_MIC2G_CTRL		0x0a
+#define SUN8I_ADDA_MIC2G_CTRL_MIC2AMPEN		7
+#define SUN8I_ADDA_MIC2G_CTRL_MIC2BOOST		4
+#define SUN8I_ADDA_MIC2G_CTRL_LINEOUTLEN	3
+#define SUN8I_ADDA_MIC2G_CTRL_LINEOUTREN	2
+#define SUN8I_ADDA_MIC2G_CTRL_LINEOUTLSRC	1
+#define SUN8I_ADDA_MIC2G_CTRL_LINEOUTRSRC	0
+#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL	0x0b
+#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_HMICBIASEN	7
+#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MMICBIASEN	6
+#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_HMICBIAS_MODE	5
+#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MIC1AMPEN		3
+#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MIC1BOOST		0
+#define SUN8I_ADDA_LADCMIXSC		0x0c
+#define SUN8I_ADDA_LADCMIXSC_MIC1		6
+#define SUN8I_ADDA_LADCMIXSC_MIC2		5
+#define SUN8I_ADDA_LADCMIXSC_PHONE		4
+#define SUN8I_ADDA_LADCMIXSC_PHONEN		3
+#define SUN8I_ADDA_LADCMIXSC_LINEINL		2
+#define SUN8I_ADDA_LADCMIXSC_OMIXRL		1
+#define SUN8I_ADDA_LADCMIXSC_OMIXRR		0
+#define SUN8I_ADDA_RADCMIXSC		0x0d
+#define SUN8I_ADDA_RADCMIXSC_MIC1		6
+#define SUN8I_ADDA_RADCMIXSC_MIC2		5
+#define SUN8I_ADDA_RADCMIXSC_PHONE		4
+#define SUN8I_ADDA_RADCMIXSC_PHONEP		3
+#define SUN8I_ADDA_RADCMIXSC_LINEINR		2
+#define SUN8I_ADDA_RADCMIXSC_OMIXR		1
+#define SUN8I_ADDA_RADCMIXSC_OMIXL		0
+#define SUN8I_ADDA_RES			0x0e
+#define SUN8I_ADDA_RES_MMICBIAS_SEL		4
+#define SUN8I_ADDA_RES_PA_ANTI_POP_CTRL		0
+#define SUN8I_ADDA_ADC_AP_EN		0x0f
+#define SUN8I_ADDA_ADC_AP_EN_ADCREN		7
+#define SUN8I_ADDA_ADC_AP_EN_ADCLEN		6
+#define SUN8I_ADDA_ADC_AP_EN_ADCG		0
+
+/* Analog control register access bits */
+#define ADDA_PR			0x0		/* PRCM base + 0x1c0 */
+#define ADDA_PR_RESET			BIT(28)
+#define ADDA_PR_WRITE			BIT(24)
+#define ADDA_PR_ADDR_SHIFT		16
+#define ADDA_PR_ADDR_MASK		GENMASK(4, 0)
+#define ADDA_PR_DATA_IN_SHIFT		8
+#define ADDA_PR_DATA_IN_MASK		GENMASK(7, 0)
+#define ADDA_PR_DATA_OUT_SHIFT		0
+#define ADDA_PR_DATA_OUT_MASK		GENMASK(7, 0)
+
+/* regmap access bits */
+static int adda_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	void __iomem *base = (void __iomem *)context;
+	u32 tmp;
+
+	/* De-assert reset */
+	writel(readl(base) | ADDA_PR_RESET, base);
+
+	/* Clear write bit */
+	writel(readl(base) & ~ADDA_PR_WRITE, base);
+
+	/* Set register address */
+	tmp = readl(base);
+	tmp &= ~(ADDA_PR_ADDR_MASK << ADDA_PR_ADDR_SHIFT);
+	tmp |= (reg & ADDA_PR_ADDR_MASK) << ADDA_PR_ADDR_SHIFT;
+	writel(tmp, base);
+
+	/* Read back value */
+	*val = readl(base) & ADDA_PR_DATA_OUT_MASK;
+
+	return 0;
+}
+
+static int adda_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	void __iomem *base = (void __iomem *)context;
+	u32 tmp;
+
+	/* De-assert reset */
+	writel(readl(base) | ADDA_PR_RESET, base);
+
+	/* Set register address */
+	tmp = readl(base);
+	tmp &= ~(ADDA_PR_ADDR_MASK << ADDA_PR_ADDR_SHIFT);
+	tmp |= (reg & ADDA_PR_ADDR_MASK) << ADDA_PR_ADDR_SHIFT;
+	writel(tmp, base);
+
+	/* Set data to write */
+	tmp = readl(base);
+	tmp &= ~(ADDA_PR_DATA_IN_MASK << ADDA_PR_DATA_IN_SHIFT);
+	tmp |= (val & ADDA_PR_DATA_IN_MASK) << ADDA_PR_DATA_IN_SHIFT;
+	writel(tmp, base);
+
+	/* Set write bit to signal a write */
+	writel(readl(base) | ADDA_PR_WRITE, base);
+
+	/* Clear write bit */
+	writel(readl(base) & ~ADDA_PR_WRITE, base);
+
+	return 0;
+}
+
+static const struct regmap_config adda_pr_regmap_cfg = {
+	.name		= "adda-pr",
+	.reg_bits	= 5,
+	.reg_stride	= 1,
+	.val_bits	= 8,
+	.reg_read	= adda_reg_read,
+	.reg_write	= adda_reg_write,
+	.fast_io	= true,
+	.max_register	= 24,
+};
+
+/* mixer controls */
+static const struct snd_kcontrol_new sun8i_codec_mixer_controls[] = {
+	SOC_DAPM_DOUBLE_R("DAC Playback Switch",
+			  SUN8I_ADDA_LOMIXSC,
+			  SUN8I_ADDA_ROMIXSC,
+			  SUN8I_ADDA_LOMIXSC_DACL, 1, 0),
+	SOC_DAPM_DOUBLE_R("DAC Reversed Playback Switch",
+			  SUN8I_ADDA_LOMIXSC,
+			  SUN8I_ADDA_ROMIXSC,
+			  SUN8I_ADDA_LOMIXSC_DACR, 1, 0),
+	SOC_DAPM_DOUBLE_R("Line In Playback Switch",
+			  SUN8I_ADDA_LOMIXSC,
+			  SUN8I_ADDA_ROMIXSC,
+			  SUN8I_ADDA_LOMIXSC_LINEINL, 1, 0),
+	SOC_DAPM_DOUBLE_R("Mic1 Playback Switch",
+			  SUN8I_ADDA_LOMIXSC,
+			  SUN8I_ADDA_ROMIXSC,
+			  SUN8I_ADDA_LOMIXSC_MIC1, 1, 0),
+	SOC_DAPM_DOUBLE_R("Mic2 Playback Switch",
+			  SUN8I_ADDA_LOMIXSC,
+			  SUN8I_ADDA_ROMIXSC,
+			  SUN8I_ADDA_LOMIXSC_MIC2, 1, 0),
+};
+
+/* ADC mixer controls */
+static const struct snd_kcontrol_new sun8i_codec_adc_mixer_controls[] = {
+	SOC_DAPM_DOUBLE_R("Mixer Capture Switch",
+			  SUN8I_ADDA_LADCMIXSC,
+			  SUN8I_ADDA_RADCMIXSC,
+			  SUN8I_ADDA_LADCMIXSC_OMIXRL, 1, 0),
+	SOC_DAPM_DOUBLE_R("Mixer Reversed Capture Switch",
+			  SUN8I_ADDA_LADCMIXSC,
+			  SUN8I_ADDA_RADCMIXSC,
+			  SUN8I_ADDA_LADCMIXSC_OMIXRR, 1, 0),
+	SOC_DAPM_DOUBLE_R("Line In Capture Switch",
+			  SUN8I_ADDA_LADCMIXSC,
+			  SUN8I_ADDA_RADCMIXSC,
+			  SUN8I_ADDA_LADCMIXSC_LINEINL, 1, 0),
+	SOC_DAPM_DOUBLE_R("Mic1 Capture Switch",
+			  SUN8I_ADDA_LADCMIXSC,
+			  SUN8I_ADDA_RADCMIXSC,
+			  SUN8I_ADDA_LADCMIXSC_MIC1, 1, 0),
+	SOC_DAPM_DOUBLE_R("Mic2 Capture Switch",
+			  SUN8I_ADDA_LADCMIXSC,
+			  SUN8I_ADDA_RADCMIXSC,
+			  SUN8I_ADDA_LADCMIXSC_MIC2, 1, 0),
+};
+
+/* volume / mute controls */
+static const DECLARE_TLV_DB_SCALE(sun8i_codec_out_mixer_pregain_scale,
+				  -450, 150, 0);
+static const DECLARE_TLV_DB_RANGE(sun8i_codec_mic_gain_scale,
+	0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
+	1, 7, TLV_DB_SCALE_ITEM(2400, 300, 0),
+);
+
+static const struct snd_kcontrol_new sun8i_codec_common_controls[] = {
+	/* Mixer pre-gains */
+	SOC_SINGLE_TLV("Line In Playback Volume", SUN8I_ADDA_LINEIN_GCTRL,
+		       SUN8I_ADDA_LINEIN_GCTRL_LINEING,
+		       0x7, 0, sun8i_codec_out_mixer_pregain_scale),
+	SOC_SINGLE_TLV("Mic1 Playback Volume", SUN8I_ADDA_MICIN_GCTRL,
+		       SUN8I_ADDA_MICIN_GCTRL_MIC1G,
+		       0x7, 0, sun8i_codec_out_mixer_pregain_scale),
+	SOC_SINGLE_TLV("Mic2 Playback Volume",
+		       SUN8I_ADDA_MICIN_GCTRL, SUN8I_ADDA_MICIN_GCTRL_MIC2G,
+		       0x7, 0, sun8i_codec_out_mixer_pregain_scale),
+
+	/* Microphone Amp boost gains */
+	SOC_SINGLE_TLV("Mic1 Boost Volume", SUN8I_ADDA_MIC1G_MICBIAS_CTRL,
+		       SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MIC1BOOST, 0x7, 0,
+		       sun8i_codec_mic_gain_scale),
+	SOC_SINGLE_TLV("Mic2 Boost Volume", SUN8I_ADDA_MIC2G_CTRL,
+		       SUN8I_ADDA_MIC2G_CTRL_MIC2BOOST, 0x7, 0,
+		       sun8i_codec_mic_gain_scale),
+
+	/* ADC */
+	SOC_SINGLE_TLV("ADC Gain Capture Volume", SUN8I_ADDA_ADC_AP_EN,
+		       SUN8I_ADDA_ADC_AP_EN_ADCG, 0x7, 0,
+		       sun8i_codec_out_mixer_pregain_scale),
+};
+
+static const struct snd_soc_dapm_widget sun8i_codec_common_widgets[] = {
+	/* ADC */
+	SND_SOC_DAPM_ADC("Left ADC", NULL, SUN8I_ADDA_ADC_AP_EN,
+			 SUN8I_ADDA_ADC_AP_EN_ADCLEN, 0),
+	SND_SOC_DAPM_ADC("Right ADC", NULL, SUN8I_ADDA_ADC_AP_EN,
+			 SUN8I_ADDA_ADC_AP_EN_ADCREN, 0),
+
+	/* DAC */
+	SND_SOC_DAPM_DAC("Left DAC", NULL, SUN8I_ADDA_DAC_PA_SRC,
+			 SUN8I_ADDA_DAC_PA_SRC_DACALEN, 0),
+	SND_SOC_DAPM_DAC("Right DAC", NULL, SUN8I_ADDA_DAC_PA_SRC,
+			 SUN8I_ADDA_DAC_PA_SRC_DACAREN, 0),
+	/*
+	 * Due to this component and the codec belonging to separate DAPM
+	 * contexts, we need to manually link the above widgets to their
+	 * stream widgets@the card level.
+	 */
+
+	/* Line In */
+	SND_SOC_DAPM_INPUT("LINEIN"),
+
+	/* Microphone inputs */
+	SND_SOC_DAPM_INPUT("MIC1"),
+	SND_SOC_DAPM_INPUT("MIC2"),
+
+	/* Microphone Bias */
+	SND_SOC_DAPM_SUPPLY("MBIAS", SUN8I_ADDA_MIC1G_MICBIAS_CTRL,
+			    SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MMICBIASEN,
+			    0, NULL, 0),
+
+	/* Mic input path */
+	SND_SOC_DAPM_PGA("Mic1 Amplifier", SUN8I_ADDA_MIC1G_MICBIAS_CTRL,
+			 SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MIC1AMPEN, 0, NULL, 0),
+	SND_SOC_DAPM_PGA("Mic2 Amplifier", SUN8I_ADDA_MIC2G_CTRL,
+			 SUN8I_ADDA_MIC2G_CTRL_MIC2AMPEN, 0, NULL, 0),
+
+	/* Mixers */
+	SND_SOC_DAPM_MIXER("Left Mixer", SUN8I_ADDA_DAC_PA_SRC,
+			   SUN8I_ADDA_DAC_PA_SRC_LMIXEN, 0,
+			   sun8i_codec_mixer_controls,
+			   ARRAY_SIZE(sun8i_codec_mixer_controls)),
+	SND_SOC_DAPM_MIXER("Right Mixer", SUN8I_ADDA_DAC_PA_SRC,
+			   SUN8I_ADDA_DAC_PA_SRC_RMIXEN, 0,
+			   sun8i_codec_mixer_controls,
+			   ARRAY_SIZE(sun8i_codec_mixer_controls)),
+	SND_SOC_DAPM_MIXER("Left ADC Mixer", SUN8I_ADDA_ADC_AP_EN,
+			   SUN8I_ADDA_ADC_AP_EN_ADCLEN, 0,
+			   sun8i_codec_adc_mixer_controls,
+			   ARRAY_SIZE(sun8i_codec_adc_mixer_controls)),
+	SND_SOC_DAPM_MIXER("Right ADC Mixer", SUN8I_ADDA_ADC_AP_EN,
+			   SUN8I_ADDA_ADC_AP_EN_ADCREN, 0,
+			   sun8i_codec_adc_mixer_controls,
+			   ARRAY_SIZE(sun8i_codec_adc_mixer_controls)),
+};
+
+static const struct snd_soc_dapm_route sun8i_codec_common_routes[] = {
+	/* Microphone Routes */
+	{ "Mic1 Amplifier", NULL, "MIC1"},
+	{ "Mic2 Amplifier", NULL, "MIC2"},
+
+	/* Left Mixer Routes */
+	{ "Left Mixer", "DAC Playback Switch", "Left DAC" },
+	{ "Left Mixer", "DAC Reversed Playback Switch", "Right DAC" },
+	{ "Left Mixer", "Line In Playback Switch", "LINEIN" },
+	{ "Left Mixer", "Mic1 Playback Switch", "Mic1 Amplifier" },
+	{ "Left Mixer", "Mic2 Playback Switch", "Mic2 Amplifier" },
+
+	/* Right Mixer Routes */
+	{ "Right Mixer", "DAC Playback Switch", "Right DAC" },
+	{ "Right Mixer", "DAC Reversed Playback Switch", "Left DAC" },
+	{ "Right Mixer", "Line In Playback Switch", "LINEIN" },
+	{ "Right Mixer", "Mic1 Playback Switch", "Mic1 Amplifier" },
+	{ "Right Mixer", "Mic2 Playback Switch", "Mic2 Amplifier" },
+
+	/* Left ADC Mixer Routes */
+	{ "Left ADC Mixer", "Mixer Capture Switch", "Left Mixer" },
+	{ "Left ADC Mixer", "Mixer Reversed Capture Switch", "Right Mixer" },
+	{ "Left ADC Mixer", "Line In Capture Switch", "LINEIN" },
+	{ "Left ADC Mixer", "Mic1 Capture Switch", "Mic1 Amplifier" },
+	{ "Left ADC Mixer", "Mic2 Capture Switch", "Mic2 Amplifier" },
+
+	/* Right ADC Mixer Routes */
+	{ "Right ADC Mixer", "Mixer Capture Switch", "Right Mixer" },
+	{ "Right ADC Mixer", "Mixer Reversed Capture Switch", "Left Mixer" },
+	{ "Right ADC Mixer", "Line In Capture Switch", "LINEIN" },
+	{ "Right ADC Mixer", "Mic1 Capture Switch", "Mic1 Amplifier" },
+	{ "Right ADC Mixer", "Mic2 Capture Switch", "Mic2 Amplifier" },
+
+	/* ADC Routes */
+	{ "Left ADC", NULL, "Left ADC Mixer" },
+	{ "Right ADC", NULL, "Right ADC Mixer" },
+};
+
+/* headphone specific controls, widgets, and routes */
+static const DECLARE_TLV_DB_SCALE(sun8i_codec_hp_vol_scale, -6300, 100, 1);
+static const struct snd_kcontrol_new sun8i_codec_headphone_controls[] = {
+	SOC_SINGLE_TLV("Headphone Playback Volume",
+		       SUN8I_ADDA_HP_VOLC,
+		       SUN8I_ADDA_HP_VOLC_HP_VOL, 0x3f, 0,
+		       sun8i_codec_hp_vol_scale),
+	SOC_DOUBLE("Headphone Playback Switch",
+		   SUN8I_ADDA_DAC_PA_SRC,
+		   SUN8I_ADDA_DAC_PA_SRC_LHPPAMUTE,
+		   SUN8I_ADDA_DAC_PA_SRC_RHPPAMUTE, 1, 0),
+};
+
+static const char * const sun8i_codec_hp_src_enum_text[] = {
+	"DAC", "Mixer",
+};
+
+static SOC_ENUM_DOUBLE_DECL(sun8i_codec_hp_src_enum,
+			    SUN8I_ADDA_DAC_PA_SRC,
+			    SUN8I_ADDA_DAC_PA_SRC_LHPIS,
+			    SUN8I_ADDA_DAC_PA_SRC_RHPIS,
+			    sun8i_codec_hp_src_enum_text);
+
+static const struct snd_kcontrol_new sun8i_codec_hp_src[] = {
+	SOC_DAPM_ENUM("Headphone Source Playback Route",
+		      sun8i_codec_hp_src_enum),
+};
+
+static const struct snd_soc_dapm_widget sun8i_codec_headphone_widgets[] = {
+	SND_SOC_DAPM_MUX("Headphone Source Playback Route",
+			 SND_SOC_NOPM, 0, 0, sun8i_codec_hp_src),
+	SND_SOC_DAPM_OUT_DRV("Headphone Amp", SUN8I_ADDA_PAEN_HP_CTRL,
+			     SUN8I_ADDA_PAEN_HP_CTRL_HPPAEN, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("HPCOM Protection", SUN8I_ADDA_PAEN_HP_CTRL,
+			    SUN8I_ADDA_PAEN_HP_CTRL_COMPTEN, 0, NULL, 0),
+	SND_SOC_DAPM_REG(snd_soc_dapm_supply, "HPCOM", SUN8I_ADDA_PAEN_HP_CTRL,
+			 SUN8I_ADDA_PAEN_HP_CTRL_HPCOM_FC, 0x3, 0x3, 0),
+	SND_SOC_DAPM_OUTPUT("HP"),
+};
+
+static const struct snd_soc_dapm_route sun8i_codec_headphone_routes[] = {
+	{ "Headphone Source Playback Route", "DAC", "Left DAC" },
+	{ "Headphone Source Playback Route", "DAC", "Right DAC" },
+	{ "Headphone Source Playback Route", "Mixer", "Left Mixer" },
+	{ "Headphone Source Playback Route", "Mixer", "Right Mixer" },
+	{ "Headphone Amp", NULL, "Headphone Source Playback Route" },
+	{ "HPCOM", NULL, "HPCOM Protection" },
+	{ "HP", NULL, "Headphone Amp" },
+};
+
+static int sun8i_codec_add_headphone(struct snd_soc_component *cmpnt)
+{
+	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(cmpnt);
+	struct device *dev = cmpnt->dev;
+	int ret;
+
+	ret = snd_soc_add_component_controls(cmpnt,
+					     sun8i_codec_headphone_controls,
+					     ARRAY_SIZE(sun8i_codec_headphone_controls));
+	if (ret) {
+		dev_err(dev, "Failed to add Headphone controls: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dapm_new_controls(dapm, sun8i_codec_headphone_widgets,
+					ARRAY_SIZE(sun8i_codec_headphone_widgets));
+	if (ret) {
+		dev_err(dev, "Failed to add Headphone DAPM widgets: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dapm_add_routes(dapm, sun8i_codec_headphone_routes,
+				      ARRAY_SIZE(sun8i_codec_headphone_routes));
+	if (ret) {
+		dev_err(dev, "Failed to add Headphone DAPM routes: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+/* hmic specific widget */
+static const struct snd_soc_dapm_widget sun8i_codec_hmic_widgets[] = {
+	SND_SOC_DAPM_SUPPLY("HBIAS", SUN8I_ADDA_MIC1G_MICBIAS_CTRL,
+			    SUN8I_ADDA_MIC1G_MICBIAS_CTRL_HMICBIASEN,
+			    0, NULL, 0),
+};
+
+static int sun8i_codec_add_hmic(struct snd_soc_component *cmpnt)
+{
+	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(cmpnt);
+	struct device *dev = cmpnt->dev;
+	int ret;
+
+	ret = snd_soc_dapm_new_controls(dapm, sun8i_codec_hmic_widgets,
+					ARRAY_SIZE(sun8i_codec_hmic_widgets));
+	if (ret)
+		dev_err(dev, "Failed to add Mic3 DAPM widgets: %d\n", ret);
+
+	return ret;
+}
+
+/* line out specific controls, widgets and routes */
+static const DECLARE_TLV_DB_RANGE(sun8i_codec_lineout_vol_scale,
+	0, 1, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
+	2, 31, TLV_DB_SCALE_ITEM(-4350, 150, 0),
+);
+static const struct snd_kcontrol_new sun8i_codec_lineout_controls[] = {
+	SOC_SINGLE_TLV("Line Out Playback Volume",
+		       SUN8I_ADDA_PHONE_GAIN_CTRL,
+		       SUN8I_ADDA_PHONE_GAIN_CTRL_LINEOUT_VOL, 0x1f, 0,
+		       sun8i_codec_lineout_vol_scale),
+	SOC_DOUBLE("Line Out Playback Switch",
+		   SUN8I_ADDA_MIC2G_CTRL,
+		   SUN8I_ADDA_MIC2G_CTRL_LINEOUTLEN,
+		   SUN8I_ADDA_MIC2G_CTRL_LINEOUTREN, 1, 0),
+};
+
+static const char * const sun8i_codec_lineout_src_enum_text[] = {
+	"Stereo", "Mono Differential",
+};
+
+static SOC_ENUM_DOUBLE_DECL(sun8i_codec_lineout_src_enum,
+			    SUN8I_ADDA_MIC2G_CTRL,
+			    SUN8I_ADDA_MIC2G_CTRL_LINEOUTLSRC,
+			    SUN8I_ADDA_MIC2G_CTRL_LINEOUTRSRC,
+			    sun8i_codec_lineout_src_enum_text);
+
+static const struct snd_kcontrol_new sun8i_codec_lineout_src[] = {
+	SOC_DAPM_ENUM("Line Out Source Playback Route",
+		      sun8i_codec_lineout_src_enum),
+};
+
+static const struct snd_soc_dapm_widget sun8i_codec_lineout_widgets[] = {
+	SND_SOC_DAPM_MUX("Line Out Source Playback Route",
+			 SND_SOC_NOPM, 0, 0, sun8i_codec_lineout_src),
+	/* It is unclear if this is a buffer or gate, model it as a supply */
+	SND_SOC_DAPM_SUPPLY("Line Out Enable", SUN8I_ADDA_PAEN_HP_CTRL,
+			    SUN8I_ADDA_PAEN_HP_CTRL_LINEOUTEN, 0, NULL, 0),
+	SND_SOC_DAPM_OUTPUT("LINEOUT"),
+};
+
+static const struct snd_soc_dapm_route sun8i_codec_lineout_routes[] = {
+	{ "Line Out Source Playback Route", "Stereo", "Left Mixer" },
+	{ "Line Out Source Playback Route", "Stereo", "Right Mixer" },
+	{ "Line Out Source Playback Route", "Mono Differential", "Left Mixer" },
+	{ "Line Out Source Playback Route", "Mono Differential", "Right Mixer" },
+	{ "LINEOUT", NULL, "Line Out Source Playback Route" },
+	{ "LINEOUT", NULL, "Line Out Enable", },
+};
+
+static int sun8i_codec_add_lineout(struct snd_soc_component *cmpnt)
+{
+	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(cmpnt);
+	struct device *dev = cmpnt->dev;
+	int ret;
+
+	ret = snd_soc_add_component_controls(cmpnt,
+					     sun8i_codec_lineout_controls,
+					     ARRAY_SIZE(sun8i_codec_lineout_controls));
+	if (ret) {
+		dev_err(dev, "Failed to add Line Out controls: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dapm_new_controls(dapm, sun8i_codec_lineout_widgets,
+					ARRAY_SIZE(sun8i_codec_lineout_widgets));
+	if (ret) {
+		dev_err(dev, "Failed to add Line Out DAPM widgets: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dapm_add_routes(dapm, sun8i_codec_lineout_routes,
+				      ARRAY_SIZE(sun8i_codec_lineout_routes));
+	if (ret) {
+		dev_err(dev, "Failed to add Line Out DAPM routes: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+struct sun8i_codec_analog_quirks {
+	bool has_headphone;
+	bool has_hmic;
+	bool has_lineout;
+};
+
+static const struct sun8i_codec_analog_quirks sun8i_a23_quirks = {
+	.has_headphone	= true,
+	.has_hmic	= true,
+};
+
+static const struct sun8i_codec_analog_quirks sun8i_h3_quirks = {
+	.has_lineout	= true,
+};
+
+static int sun8i_codec_analog_cmpnt_probe(struct snd_soc_component *cmpnt)
+{
+	struct device *dev = cmpnt->dev;
+	const struct sun8i_codec_analog_quirks *quirks;
+	int ret;
+
+	/*
+	 * This would never return NULL unless someone directly registers a
+	 * platform device matching this driver's name, without specifying a
+	 * device tree node.
+	 */
+	quirks = of_device_get_match_data(dev);
+
+	/* Add controls, widgets, and routes for individual features */
+
+	if (quirks->has_headphone) {
+		ret = sun8i_codec_add_headphone(cmpnt);
+		if (ret)
+			return ret;
+	}
+
+	if (quirks->has_hmic) {
+		sun8i_codec_add_hmic(cmpnt);
+		if (ret)
+			return ret;
+	}
+
+	if (quirks->has_lineout) {
+		ret = sun8i_codec_add_lineout(cmpnt);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver sun8i_codec_analog_cmpnt_drv = {
+	.controls		= sun8i_codec_common_controls,
+	.num_controls		= ARRAY_SIZE(sun8i_codec_common_controls),
+	.dapm_widgets		= sun8i_codec_common_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(sun8i_codec_common_widgets),
+	.dapm_routes		= sun8i_codec_common_routes,
+	.num_dapm_routes	= ARRAY_SIZE(sun8i_codec_common_routes),
+	.probe			= sun8i_codec_analog_cmpnt_probe,
+};
+
+static const struct of_device_id sun8i_codec_analog_of_match[] = {
+	{
+		.compatible = "allwinner,sun8i-a23-codec-analog",
+		.data = &sun8i_a23_quirks,
+	},
+	{
+		.compatible = "allwinner,sun8i-h3-codec-analog",
+		.data = &sun8i_h3_quirks,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, sun8i_codec_analog_of_match);
+
+static int sun8i_codec_analog_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct regmap *regmap;
+	void __iomem *base;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base)) {
+		dev_err(&pdev->dev, "Failed to map the registers\n");
+		return PTR_ERR(base);
+	}
+
+	regmap = devm_regmap_init(&pdev->dev, NULL, base, &adda_pr_regmap_cfg);
+	if (IS_ERR(regmap)) {
+		dev_err(&pdev->dev, "Failed to create regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	return devm_snd_soc_register_component(&pdev->dev,
+					       &sun8i_codec_analog_cmpnt_drv,
+					       NULL, 0);
+}
+
+static struct platform_driver sun8i_codec_analog_driver = {
+	.driver = {
+		.name = "sun8i-codec-analog",
+		.of_match_table = sun8i_codec_analog_of_match,
+	},
+	.probe = sun8i_codec_analog_probe,
+};
+module_platform_driver(sun8i_codec_analog_driver);
+
+MODULE_DESCRIPTION("Allwinner internal codec analog controls driver");
+MODULE_AUTHOR("Chen-Yu Tsai <wens@csie.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:sun8i-codec-analog");
-- 
2.10.2

^ permalink raw reply related

* [PATCH 01/10] ASoC: sunxi: Add bindings for A23/A33/H3 codec's analog path controls
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112064648.26779-1-wens@csie.org>

The internal codec on A23/A33/H3 is split into 2 parts. The
analog path controls are routed through an embedded custom register
bus accessed through the PRCM block.

The SoCs share a common set of inputs, outputs, and audio paths.
The following table lists the differences.

    ----------------------------------------
    | Feature \ SoC |  A23  |  A33  |  H3  |
    ----------------------------------------
    | Headphone     |   v   |   v   |      |
    ----------------------------------------
    | Line Out      |       |       |  v   |
    ----------------------------------------
    | Phone In/Out  |   v   |   v   |      |
    ----------------------------------------

Add a binding for this hardware.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 .../devicetree/bindings/sound/sun8i-codec-analog.txt     | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt

diff --git a/Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt b/Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt
new file mode 100644
index 000000000000..779b735781ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt
@@ -0,0 +1,16 @@
+* Allwinner Codec Analog Controls
+
+Required properties:
+- compatible: must be one of the following compatibles:
+		- "allwinner,sun8i-a23-codec-analog"
+		- "allwinner,sun8i-h3-codec-analog"
+
+Required properties if not a sub-node of the PRCM node:
+- reg: must contain the registers location and length
+
+Example:
+prcm: prcm at 01f01400 {
+	codec_analog: codec-analog {
+		compatible = "allwinner,sun8i-a23-codec-analog";
+	};
+};
-- 
2.10.2

^ permalink raw reply related

* [PATCH 00/10] ASoC: sunxi: Add support for audio codec in A23/H3 SoCs
From: Chen-Yu Tsai @ 2016-11-12  6:46 UTC (permalink / raw)
  To: linux-arm-kernel

Hi everyone,

This series adds support for the audio codec found in Allwinner A23 and
H3 SoCs. The design and data paths are similar to the audio codec found
in earlier SoCs such as the A31. The analog audio paths are symmetrical
with left/right channels and down-mix selectors for mono differential
output.

What deviates from previous SoCs is that the analog path controls have
been moved to a separate control bus, accessed through a message box
like register interface in the PRCM block. This necessitates writing
a separate component driver for it, which is then tied into the sound
card as an ASoC auxiliary device.

Patches 1 and 2 add the binding and driver for the analog path control
block. This is the more complete version. Previously I provided a not
fully tested version to Mylene Josserand from Free Electrons to use
with the A33. Their version was then trimmed down and switched to 
SOC_DAPM_SINGLE controls. Mine implements all the commonly used paths,
and uses the new stereo SOC_DAPM_DOUBLE controls. I also have a separate
patch for "Phone In" in case anyone wants to test them. It is not included
as my hardware does not use that input.

As for the A33, the analog controls are exactly the same as the A23, so
this driver can be reused, but the PCM and DAI bits are completely
different.

Patch 3 adds the analog path controls block to the sun6i-prcm driver as
a sub-device, for the A23. The H3 currently does not use the PRCM driver.

Patch 4 adds PCM and card support for the A23 codec to the sun4i-codec
driver.

Patch 5 adds a device node for the analog path controls block to the A23
dtsi.

Patch 6 adds a device node for the audio codec, and the phandle for the
analog path controls block to the A23 dtsi.

Patch 7 enables the audio codec for the A23 Q8 tablets. On these tablets
the headphone output is driven in DC coupled, or "direct drive", mode.

Patch 8 adds PCM and card support for the H3 codec to the sun4i-codec
driver.

Patch 9 adds device nodes for the audio codec and analog path controls
block to the H3 dtsi.

Patch 10 enables the audio codec on the Orange Pi PC. The audio output
jack on the board is tied to the line out pins on the SoC.


Please take a look and let me know what you think.

In addition, the sun4i-codec driver is getting pretty large. Maybe we
want to split the different parts into different files?


Regards
ChenYu


Chen-Yu Tsai (10):
  ASoC: sunxi: Add bindings for A23/A33/H3 codec's analog path controls
  ASoC: sunxi: Add support for A23/A33/H3 codec's analog path controls
  mfd: sun6i-prcm: Add codec analog controls sub-device for Allwinner
    A23
  ASoC: sun4i-codec: Add support for A23 codec
  ARM: dts: sun8i: Add codec analog path controls node in PRCM for
    A23/A33
  ARM: dts: sun8i-a23: Add device node for internal audio codec
  ARM: dts: sun8i-a23: q8-tablet: Enable internal audio codec
  ASoC: sun4i-codec: Add support for H3 codec
  ARM: dts: sun8i-h3: Add device nodes for audio codec and its analog
    controls
  ARM: dts: sun8i-h3: orange-pi-pc: Enable audio codec

 .../devicetree/bindings/sound/sun4i-codec.txt      |  14 +-
 .../bindings/sound/sun8i-codec-analog.txt          |  16 +
 arch/arm/boot/dts/sun8i-a23-a33.dtsi               |   4 +
 arch/arm/boot/dts/sun8i-a23-q8-tablet.dts          |  23 +
 arch/arm/boot/dts/sun8i-a23.dtsi                   |  16 +
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |   8 +
 arch/arm/boot/dts/sun8i-h3.dtsi                    |  19 +
 drivers/mfd/sun6i-prcm.c                           |  17 +
 sound/soc/sunxi/Kconfig                            |   8 +
 sound/soc/sunxi/Makefile                           |   1 +
 sound/soc/sunxi/sun4i-codec.c                      | 179 ++++++
 sound/soc/sunxi/sun8i-codec-analog.c               | 665 +++++++++++++++++++++
 12 files changed, 968 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/sun8i-codec-analog.txt
 create mode 100644 sound/soc/sunxi/sun8i-codec-analog.c

-- 
2.10.2

^ permalink raw reply

* [PATCH RFC] mm: Add debug_virt_to_phys()
From: Will Deacon @ 2016-11-12  5:43 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112004449.30566-1-f.fainelli@gmail.com>

On Fri, Nov 11, 2016 at 04:44:43PM -0800, Florian Fainelli wrote:
> When CONFIG_DEBUG_VM is turned on, virt_to_phys() maps to
> debug_virt_to_phys() which helps catch vmalloc space addresses being
> passed. This is helpful in debugging bogus drivers that just assume
> linear mappings all over the place.
> 
> For ARM, ARM64, Unicore32 and Microblaze, the architectures define
> __virt_to_phys() as being the functional implementation of the address
> translation, so we special case the debug stub to call into
> __virt_to_phys directly.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm/include/asm/memory.h      |  4 ++++
>  arch/arm64/include/asm/memory.h    |  4 ++++
>  include/asm-generic/memory_model.h |  4 ++++
>  mm/debug.c                         | 15 +++++++++++++++
>  4 files changed, 27 insertions(+)

What's the interaction between this and the DEBUG_VIRTUAL patches from Laura?

http://lkml.kernel.org/r/20161102210054.16621-7-labbott at redhat.com

They seem to be tackling the exact same problem afaict.

Will

^ permalink raw reply

* [PATCH] ARM: dts: vfxxx: Enable DMA for DSPI2 and DSPI3
From: maitysanchayan at gmail.com @ 2016-11-12  5:16 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOMZO5BAE=6pE9DMGhXTMm5CeLh+DmhSqGtdSYHf8+yyZkfEVQ@mail.gmail.com>

On 16-11-11 20:46:12, Fabio Estevam wrote:
> On Thu, Nov 10, 2016 at 9:45 AM, Sanchayan Maity
> <maitysanchayan@gmail.com> wrote:
> > Enable DMA for DSPI2 and DSPI3 on Vybrid.
> 
> You missed your Signed-off-by line.

Argh...Sorry about that...Will send a follow patch with this fixed.

- Sanchayan.

^ permalink raw reply

* [PATCH v6 4/9] drm/hisilicon/hibmc: Add plane for DE
From: Rongrong Zou @ 2016-11-12  5:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAOw6vb+kibtOkViwB4+gEn=+u14esSoNVavhrasNxtrCoWhyJQ@mail.gmail.com>

? 2016/11/11 5:53, Sean Paul ??:
> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
>> Add plane funcs and helper funcs for DE.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Kconfig         |   1 +
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile        |   2 +-
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 170 ++++++++++++++++++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h  |  29 ++++
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c |  51 ++++++-
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |   5 +
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c     |   6 +
>>   7 files changed, 261 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> index bcb8c18..380622a 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> @@ -1,6 +1,7 @@
>>   config DRM_HISI_HIBMC
>>          tristate "DRM Support for Hisilicon Hibmc"
>>          depends on DRM && PCI
>> +       select DRM_KMS_HELPER
>>          select DRM_TTM
>>
>>          help
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 810a37e..72e107e 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,5 @@
>>   ccflags-y := -Iinclude/drm
>> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o
>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o
>>
>>   obj-$(CONFIG_DRM_HISI_HIBMC)   +=hibmc-drm.o
>>   #obj-y += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> new file mode 100644
>> index 0000000..9c1a68c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -0,0 +1,170 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <zourongrong@huawei.com>
>> + *     Rongrong Zou <zourongrong@gmail.com>
>> + *     Jianhua Li <lijianhua@huawei.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_plane_helper.h>
>> +
>> +#include "hibmc_drm_drv.h"
>> +#include "hibmc_drm_regs.h"
>> +#include "hibmc_drm_power.h"
>> +
>> +/* ---------------------------------------------------------------------- */
>
> Remove

ok, will do, thanks.

>
>> +
>> +static int hibmc_plane_atomic_check(struct drm_plane *plane,
>> +                                   struct drm_plane_state *state)
>> +{
>> +       struct drm_framebuffer *fb = state->fb;
>> +       struct drm_crtc *crtc = state->crtc;
>> +       struct drm_crtc_state *crtc_state;
>> +       u32 src_x = state->src_x >> 16;
>> +       u32 src_y = state->src_y >> 16;
>> +       u32 src_w = state->src_w >> 16;
>> +       u32 src_h = state->src_h >> 16;
>> +       int crtc_x = state->crtc_x;
>> +       int crtc_y = state->crtc_y;
>> +       u32 crtc_w = state->crtc_w;
>> +       u32 crtc_h = state->crtc_h;
>
> I don't think you gain anything with the crtc_* vars

It would work well, but looks redundant and not simple enough,
will delete them, thanks.

>
>> +
>> +       if (!crtc || !fb)
>> +               return 0;
>> +
>> +       crtc_state = drm_atomic_get_crtc_state(state->state, crtc);
>> +       if (IS_ERR(crtc_state))
>> +               return PTR_ERR(crtc_state);
>> +
>> +       if (src_w != crtc_w || src_h != crtc_h) {
>> +               DRM_ERROR("Scale not support!!!\n");
>
> I like the enthusiasm, but I think DRM_DEBUG_ATOMIC would be better

I'm sorry, can you explain why here should be an DRM_DEBUG_ATOMIC,
when this condition is hit, it is really an error and atomic_commit will
abort with failure.

>
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (src_x + src_w > fb->width ||
>> +           src_y + src_h > fb->height)
>
> These should be already covered in drm_atomic_plane_check

understood, thanks.

>
>> +               return -EINVAL;
>> +
>> +       if (crtc_x < 0 || crtc_y < 0)
>
> Print DRM_DEBUG_ATOMIC message here

agreed. thanks.

>
>> +               return -EINVAL;
>> +
>> +       if (crtc_x + crtc_w > crtc_state->adjusted_mode.hdisplay ||
>> +           crtc_y + crtc_h > crtc_state->adjusted_mode.vdisplay)
>
> DRM_DEBUG_ATOMIC here too

ditto.

>
>> +               return -EINVAL;
>> +
>> +       return 0;
>> +}
>> +
>> +static void hibmc_plane_atomic_update(struct drm_plane *plane,
>> +                                     struct drm_plane_state *old_state)
>> +{
>> +       struct drm_plane_state  *state  = plane->state;
>> +       u32 reg;
>> +       int ret;
>> +       u64 gpu_addr = 0;
>> +       unsigned int line_l;
>> +       struct hibmc_drm_device *hidev =
>> +               (struct hibmc_drm_device *)plane->dev->dev_private;
>> +
>
> nit: extra line

will delete, thanks.

>> +       struct hibmc_framebuffer *hibmc_fb;
>> +       struct hibmc_bo *bo;
>> +
>> +       hibmc_fb = to_hibmc_framebuffer(state->fb);
>> +       bo = gem_to_hibmc_bo(hibmc_fb->obj);
>> +       ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
>> +       if (ret)
>
> Print error

agreed, thanks.

>
>> +               return;
>> +
>> +       hibmc_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr);
>
> Check return value

ok, thanks.

>
>> +       if (ret) {
>> +               ttm_bo_unreserve(&bo->bo);
>> +               return;
>> +       }
>> +
>> +       ttm_bo_unreserve(&bo->bo);
>
> Move this up before the conditional so you don't have to call it in
> both branches

understood, thanks.

>
>> +
>> +       writel(gpu_addr, hidev->mmio + HIBMC_CRT_FB_ADDRESS);
>> +
>> +       reg = state->fb->width * (state->fb->bits_per_pixel >> 3);
>> +       /* now line_pad is 16 */
>> +       reg = PADDING(16, reg);
>> +
>> +       line_l = state->fb->width * state->fb->bits_per_pixel / 8;
>
> above, you >> 3. here you / 8, pick one?

i prefer /8 because it is more readable to human, although it is less effective
in executing.

>
>> +       line_l = PADDING(16, line_l);
>> +       writel((HIBMC_CRT_FB_WIDTH_WIDTH(reg) & HIBMC_CRT_FB_WIDTH_WIDTH_MASK) |
>> +              (HIBMC_CRT_FB_WIDTH_OFFS(line_l) & HIBMC_CRT_FB_WIDTH_OFFS_MASK),
>> +              hidev->mmio + HIBMC_CRT_FB_WIDTH);
>> +
>> +       /* SET PIXEL FORMAT */
>> +       reg = readl(hidev->mmio + HIBMC_CRT_DISP_CTL);
>> +       reg = reg & ~HIBMC_CRT_DISP_CTL_FORMAT_MASK;
>> +       reg = reg | (HIBMC_CRT_DISP_CTL_FORMAT(state->fb->bits_per_pixel >> 4) &
>> +                    HIBMC_CRT_DISP_CTL_FORMAT_MASK);
>> +       writel(reg, hidev->mmio + HIBMC_CRT_DISP_CTL);
>> +}
>> +
>> +static void hibmc_plane_atomic_disable(struct drm_plane *plane,
>> +                                      struct drm_plane_state *old_state)
>> +{
>> +}
>
> The caller checks for NULL, no need to stub

thanks for pointing it out,
will remove.

Regards,
Rongrong.

>
>> +
>> +static const u32 channel_formats1[] = {
>> +       DRM_FORMAT_RGB565, DRM_FORMAT_BGR565, DRM_FORMAT_RGB888,
>> +       DRM_FORMAT_BGR888, DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
>> +       DRM_FORMAT_RGBA8888, DRM_FORMAT_BGRA8888, DRM_FORMAT_ARGB8888,
>> +       DRM_FORMAT_ABGR8888
>> +};
>> +
>> +static struct drm_plane_funcs hibmc_plane_funcs = {
>> +       .update_plane   = drm_atomic_helper_update_plane,
>> +       .disable_plane  = drm_atomic_helper_disable_plane,
>> +       .set_property = drm_atomic_helper_plane_set_property,
>> +       .destroy = drm_plane_cleanup,
>> +       .reset = drm_atomic_helper_plane_reset,
>> +       .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> +       .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> +};
>> +
>> +static const struct drm_plane_helper_funcs hibmc_plane_helper_funcs = {
>> +       .atomic_check = hibmc_plane_atomic_check,
>> +       .atomic_update = hibmc_plane_atomic_update,
>> +       .atomic_disable = hibmc_plane_atomic_disable,
>> +};
>> +
>> +int hibmc_plane_init(struct hibmc_drm_device *hidev)
>> +{
>> +       struct drm_device *dev = hidev->dev;
>> +       struct drm_plane *plane = &hidev->plane;
>> +       int ret = 0;
>> +
>> +       /*
>> +        * plane init
>> +        * TODO: Now only support primary plane, overlay planes
>> +        * need to do.
>> +        */
>> +       ret = drm_universal_plane_init(dev, plane, 1, &hibmc_plane_funcs,
>> +                                      channel_formats1,
>> +                                      ARRAY_SIZE(channel_formats1),
>> +                                      DRM_PLANE_TYPE_PRIMARY,
>> +                                      NULL);
>> +       if (ret) {
>> +               DRM_ERROR("fail to init plane!!!\n");
>> +               return ret;
>> +       }
>> +
>> +       drm_plane_helper_add(plane, &hibmc_plane_helper_funcs);
>> +       return 0;
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h
>> new file mode 100644
>> index 0000000..4ce0d7b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h
>> @@ -0,0 +1,29 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + *     Rongrong Zou <zourongrong@huawei.com>
>> + *     Rongrong Zou <zourongrong@gmail.com>
>> + *     Jianhua Li <lijianhua@huawei.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#ifndef HIBMC_DRM_DE_H
>> +#define HIBMC_DRM_DE_H
>> +
>> +struct panel_pll {
>> +       unsigned long M;
>> +       unsigned long N;
>> +       unsigned long OD;
>> +       unsigned long POD;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 5ac7a7e..7d96583 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -18,6 +18,7 @@
>>
>>   #include <linux/module.h>
>>   #include <linux/console.h>
>> +#include <drm/drm_crtc_helper.h>
>>
>>   #include "hibmc_drm_drv.h"
>>   #include "hibmc_drm_regs.h"
>> @@ -47,8 +48,8 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>>   }
>>
>>   static struct drm_driver hibmc_driver = {
>> -       .driver_features        = DRIVER_GEM,
>> -
>> +       .driver_features        = DRIVER_GEM | DRIVER_MODESET |
>> +                                 DRIVER_ATOMIC,
>>          .fops                   = &hibmc_fops,
>>          .name                   = "hibmc",
>>          .date                   = "20160828",
>> @@ -70,6 +71,7 @@ static int hibmc_pm_suspend(struct device *dev)
>>          struct drm_device *drm_dev = pci_get_drvdata(pdev);
>>          struct hibmc_drm_device *hidev = drm_dev->dev_private;
>>
>> +       drm_kms_helper_poll_disable(drm_dev);
>>          drm_fb_helper_set_suspend_unlocked(&hidev->fbdev->helper, 1);
>>
>>          return 0;
>> @@ -81,7 +83,9 @@ static int hibmc_pm_resume(struct device *dev)
>>          struct drm_device *drm_dev = pci_get_drvdata(pdev);
>>          struct hibmc_drm_device *hidev = drm_dev->dev_private;
>>
>> +       drm_helper_resume_force_mode(drm_dev);
>>          drm_fb_helper_set_suspend_unlocked(&hidev->fbdev->helper, 0);
>> +       drm_kms_helper_poll_enable(drm_dev);
>>
>>          return 0;
>>   }
>> @@ -91,6 +95,41 @@ static int hibmc_pm_resume(struct device *dev)
>>                                  hibmc_pm_resume)
>>   };
>>
>> +static int hibmc_kms_init(struct hibmc_drm_device *hidev)
>> +{
>> +       int ret;
>> +
>> +       drm_mode_config_init(hidev->dev);
>> +       hidev->mode_config_initialized = true;
>> +
>> +       hidev->dev->mode_config.min_width = 0;
>> +       hidev->dev->mode_config.min_height = 0;
>> +       hidev->dev->mode_config.max_width = 1920;
>> +       hidev->dev->mode_config.max_height = 1440;
>> +
>> +       hidev->dev->mode_config.fb_base = hidev->fb_base;
>> +       hidev->dev->mode_config.preferred_depth = 24;
>> +       hidev->dev->mode_config.prefer_shadow = 0;
>> +
>> +       hidev->dev->mode_config.funcs = (void *)&hibmc_mode_funcs;
>> +
>> +       ret = hibmc_plane_init(hidev);
>> +       if (ret) {
>> +               DRM_ERROR("fail to init plane!!!\n");
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void hibmc_kms_fini(struct hibmc_drm_device *hidev)
>> +{
>> +       if (hidev->mode_config_initialized) {
>> +               drm_mode_config_cleanup(hidev->dev);
>> +               hidev->mode_config_initialized = false;
>> +       }
>> +}
>> +
>>   static int hibmc_hw_config(struct hibmc_drm_device *hidev)
>>   {
>>          unsigned int reg;
>> @@ -183,6 +222,7 @@ static int hibmc_unload(struct drm_device *dev)
>>          struct hibmc_drm_device *hidev = dev->dev_private;
>>
>>          hibmc_fbdev_fini(hidev);
>> +       hibmc_kms_fini(hidev);
>>          hibmc_mm_fini(hidev);
>>          hibmc_hw_fini(hidev);
>>          dev->dev_private = NULL;
>> @@ -208,6 +248,13 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags)
>>          if (ret)
>>                  goto err;
>>
>> +       ret = hibmc_kms_init(hidev);
>> +       if (ret)
>> +               goto err;
>> +
>> +       /* reset all the states of crtc/plane/encoder/connector */
>> +       drm_mode_config_reset(dev);
>> +
>>          ret = hibmc_fbdev_init(hidev);
>>          if (ret)
>>                  goto err;
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index a40e9a7..49e39d2 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -45,6 +45,8 @@ struct hibmc_drm_device {
>>
>>          /* drm */
>>          struct drm_device  *dev;
>> +       struct drm_plane plane;
>> +       bool mode_config_initialized;
>>
>>          /* ttm */
>>          struct {
>> @@ -82,6 +84,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>>
>>   #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>>
>> +int hibmc_plane_init(struct hibmc_drm_device *hidev);
>>   int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
>>   void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
>>
>> @@ -102,4 +105,6 @@ int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
>>                             u32 handle, u64 *offset);
>>   int hibmc_mmap(struct file *filp, struct vm_area_struct *vma);
>>
>> +extern const struct drm_mode_config_funcs hibmc_mode_funcs;
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> index 9822f62..beb4d76 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> @@ -554,3 +554,9 @@ struct hibmc_framebuffer *
>>          }
>>          return &hibmc_fb->fb;
>>   }
>> +
>> +const struct drm_mode_config_funcs hibmc_mode_funcs = {
>> +       .atomic_check = drm_atomic_helper_check,
>> +       .atomic_commit = drm_atomic_helper_commit,
>> +       .fb_create = hibmc_user_framebuffer_create,
>> +};
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> linuxarm mailing list
> linuxarm at huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>

^ permalink raw reply

* [PATCH v2 2/9] arm64: dts: rockchip: add pd_sd power node for rk3399
From: Shawn Lin @ 2016-11-12  4:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478697721-2323-3-git-send-email-wxt@rock-chips.com>

Hi Caesar,

On 2016/11/9 21:21, Caesar Wang wrote:
> From: zhangqing <zhangqing@rock-chips.com>
>
> 1.add pd node for RK3399 Soc
> 2.create power domain tree
> 3.add qos node for domain
> 4.add the pd_sd consumers node

I'm no sure if it is worth spliting out a seperated
patch as it looks to me that you was doing 4 things within
one patch, but anyway

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
> Changes in v2:
> - v1 on https://patchwork.kernel.org/patch/9322553/
> - Reviewed-on: https://chromium-review.googlesource.com/386483
> - Verified on ChromeOS kernel4.4
>
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index b401176..e5b5b3d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -253,6 +253,7 @@
>  			 <&cru SCLK_SDMMC_DRV>, <&cru SCLK_SDMMC_SAMPLE>;
>  		clock-names = "biu", "ciu", "ciu-drive", "ciu-sample";
>  		fifo-depth = <0x100>;
> +		power-domains = <&power RK3399_PD_SD>;
>  		status = "disabled";
>  	};
>
> @@ -691,6 +692,11 @@
>  		status = "disabled";
>  	};
>
> +	qos_sd: qos at ffa74000 {
> +		compatible = "syscon";
> +		reg = <0x0 0xffa74000 0x0 0x20>;
> +	};
> +
>  	qos_emmc: qos at ffa58000 {
>  		compatible = "syscon";
>  		reg = <0x0 0xffa58000 0x0 0x20>;
> @@ -839,6 +845,12 @@
>  				clocks = <&cru ACLK_GMAC>;
>  				pm_qos = <&qos_gmac>;
>  			};
> +			pd_sd at RK3399_PD_SD {
> +				reg = <RK3399_PD_SD>;
> +				clocks = <&cru HCLK_SDMMC>,
> +					 <&cru SCLK_SDMMC>;
> +				pm_qos = <&qos_sd>;
> +			};
>  			pd_vio at RK3399_PD_VIO {
>  				reg = <RK3399_PD_VIO>;
>  				#address-cells = <1>;
>


-- 
Best Regards
Shawn Lin

^ permalink raw reply

* [PATCH v2 1/9] arm64: dts: rockchip: add eMMC's power domain support for rk3399
From: Shawn Lin @ 2016-11-12  4:22 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478697721-2323-2-git-send-email-wxt@rock-chips.com>

On 2016/11/9 21:21, Caesar Wang wrote:
> From: Ziyuan Xu <xzy.xu@rock-chips.com>
>
> Control power domain for eMMC via genpd to reduce power consumption.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Ziyuan Xu <xzy.xu@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>

It was verified on my rk3399 evb with kernel4.4, so
free feel to add my tag,

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

BTW, it seems my reply is bounced form Yakir's address, so please
remove him from CC list if he changed his mail address.

> ---
>
> Changes in v2:
> - Reviewed-on: https://chromium-review.googlesource.com/376558
> - Verified on ChromeOS kernel4.4
>
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index cbb7f8b..b401176 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -269,6 +269,7 @@
>  		#clock-cells = <0>;
>  		phys = <&emmc_phy>;
>  		phy-names = "phy_arasan";
> +		power-domains = <&power RK3399_PD_EMMC>;
>  		status = "disabled";
>  	};
>
> @@ -690,6 +691,11 @@
>  		status = "disabled";
>  	};
>
> +	qos_emmc: qos at ffa58000 {
> +		compatible = "syscon";
> +		reg = <0x0 0xffa58000 0x0 0x20>;
> +	};
> +
>  	qos_gmac: qos at ffa5c000 {
>  		compatible = "syscon";
>  		reg = <0x0 0xffa5c000 0x0 0x20>;
> @@ -823,6 +829,11 @@
>  			};
>
>  			/* These power domains are grouped by VD_LOGIC */
> +			pd_emmc at RK3399_PD_EMMC {
> +				reg = <RK3399_PD_EMMC>;
> +				clocks = <&cru ACLK_EMMC>;
> +				pm_qos = <&qos_emmc>;
> +			};
>  			pd_gmac at RK3399_PD_GMAC {
>  				reg = <RK3399_PD_GMAC>;
>  				clocks = <&cru ACLK_GMAC>;
>


-- 
Best Regards
Shawn Lin

^ permalink raw reply

* [PATCH RFC] mm: Add debug_virt_to_phys()
From: Florian Fainelli @ 2016-11-12  3:39 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <alpine.LFD.2.20.1611112034520.1618@knanqh.ubzr>

Le 11/11/2016 ? 17:49, Nicolas Pitre a ?crit :
> On Fri, 11 Nov 2016, Florian Fainelli wrote:
> 
>> When CONFIG_DEBUG_VM is turned on, virt_to_phys() maps to
>> debug_virt_to_phys() which helps catch vmalloc space addresses being
>> passed. This is helpful in debugging bogus drivers that just assume
>> linear mappings all over the place.
>>
>> For ARM, ARM64, Unicore32 and Microblaze, the architectures define
>> __virt_to_phys() as being the functional implementation of the address
>> translation, so we special case the debug stub to call into
>> __virt_to_phys directly.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  arch/arm/include/asm/memory.h      |  4 ++++
>>  arch/arm64/include/asm/memory.h    |  4 ++++
>>  include/asm-generic/memory_model.h |  4 ++++
>>  mm/debug.c                         | 15 +++++++++++++++
>>  4 files changed, 27 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index 76cbd9c674df..448dec9b8b00 100644
>> --- a/arch/arm/include/asm/memory.h
>> +++ b/arch/arm/include/asm/memory.h
>> @@ -260,11 +260,15 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>>   * translation for translating DMA addresses.  Use the driver
>>   * DMA support - see dma-mapping.h.
>>   */
>> +#ifndef CONFIG_DEBUG_VM
>>  #define virt_to_phys virt_to_phys
>>  static inline phys_addr_t virt_to_phys(const volatile void *x)
>>  {
>>  	return __virt_to_phys((unsigned long)(x));
>>  }
>> +#else
>> +#define virt_to_phys debug_virt_to_phys
>> +#endif
> [...]
> 
> Why don't you do something more like:
> 
>  static inline phys_addr_t virt_to_phys(const volatile void *x)
>  {
> +        debug_virt_to_phys(x);
>          return __virt_to_phys((unsigned long)(x));
>  }
> 
> [...]
> 
> static inline void debug_virt_to_phys(const void *address)
> {
> #ifdef CONFIG_DEBUG_VM
>         BUG_ON(is_vmalloc_addr(address));
> #endif
> }
> 
> ?

This is how I started doing it initially, but to get the
is_vmalloc_addr() definition, we need to include linux/mm.h and then
everything falls apart because of the include and dependencies chain. We
could open code the is_vmalloc_addr() check because that's simple
enough, but we still need VMALLOC_START and VMALLOC_END and to get there
we need to include pgtable.h, and there are still some inclusion
problems in doing so.

The other reason was to avoid putting the same checks in architecture
specific code, except for those like ARM/ARM64/Unicore32/Microblaze
where I could not find a simple way to undefined virt_to_phys and
redefine it to debug_virt_to_phys.

Do you see an other way around this? Thanks!
-- 
Florian

^ permalink raw reply

* [PATCH V6 2/3] ACPI: Add support for ResourceSource/IRQ domain mapping
From: agustinv at codeaurora.org @ 2016-11-12  3:01 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <5825C8A2.3040302@linaro.org>

Hey Lorenzo, Hanjun,

On 2016-11-11 08:33, Hanjun Guo wrote:
> Hi Lorenzo,
> 
> On 11/11/2016 01:58 AM, Lorenzo Pieralisi wrote:
>> On Thu, Nov 10, 2016 at 10:02:35AM -0500, agustinv at codeaurora.org 
>> wrote:
>>> Hey Hanjun,
>>> 
>>> On 2016-11-09 21:36, Hanjun Guo wrote:
>>>> Hi Marc, Rafael, Lorenzo,
>>>> 
>>>> Since we agreed to add a probe deferral if we failed to get irq
>>>> resources which mirroring the DT does (patch 1 in this patch set),
>>>> I think the last blocker to make things work both for Agustin and
>>>> me [1] is this patch, which makes the interrupt producer and 
>>>> consumer
>>>> work in ACPI, we have two different solution for one thing, we'd 
>>>> happy
>>>> to work together for one solution, could you give some suggestions
>>>> please?
>>>> 
>>>> [1]: 
>>>> https://mail-archive.com/linux-kernel at vger.kernel.org/msg1257419.html
>>>> 
>>>> Agustin, I have some comments below.
>>>> 
>>>> On 2016/10/29 4:48, Agustin Vega-Frias wrote:
>>>>> This allows irqchip drivers to associate an ACPI DSDT device to
>>>>> an IRQ domain and provides support for using the ResourceSource
>>>>> in Extended IRQ Resources to find the domain and map the IRQs
>>>>> specified on that domain.
>>>>> 
>>>>> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
>>>>> ---
>>>>> drivers/acpi/Makefile    |   1 +
>>>>> drivers/acpi/irqdomain.c | 119
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>> 
>>>> Could we just reuse the gsi.c and not introduce a new
>>>> file, probably we can change the gsi.c to irqdomain.c
>>>> or something similar, then reuse the code in gsi.c.
>>> 
>>> I was thinking just that after we chatted off-list.
>> 
>> Yes, that's a fair point.
>> 
>>> I might revisit and see what I come up with given that we already 
>>> have
>>> a device argument and we could pass the IRQ source there.
>> 
>> I agree with the approach taken by this patch, I do not like much
>> passing around struct acpi_resource_source *source (in particular
>> the dummy struct) I do not think it is needed, I will comment on
>> the code.
> 
> thanks for your time to have a look:)
> 
>> 
>> Hopefully there is not any buggy FW out there that does use the
>> resource source inappropriately otherwise we will notice on x86/ia64
>> (ie you can't blame FW if it breaks the kernel) but I suspect the
>> only way to find out is by trying, the patch has to go through 
>> Rafael's
>> review anyway before getting there so it is fine.
> 
> I think we can avoid that by not touching the logic that x86/ia64
> already used, but only adding interrupt producer/consumer function.

I looked at this more today and implemented a new patch that I plan to
test over the weekend, but I wanted to let you know the approach I am
pursuing.

On the new patch use of ResourceSource when parsing ACPI Extended IRQ
Resources is conditional on CONFIG_ACPI_GENERIC_GSI. The reason for this
is two fold:

1. Since we wanted to reduce duplication and place the new APIs on the
    same source file as acpi_register_gsi, which is already under that
    config flag.
2. So the patch does not have effect on platforms not using the generic
    GSI support, including x86/ia64.

If support for this is needed outside platforms using the generic GSI
implementation, we can move these APIs out to their own source file
and eliminate the CONFIG_ACPI_GENERIC_GSI conditionality.

I'll send the new patch, hopefully some time tomorrow, but please let
me know if you have concerns with this approach.

Thanks,
Agustin

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

^ permalink raw reply

* [RFC 00/17] clk: Add per-controller locks to fix deadlocks
From: Stephen Boyd @ 2016-11-12  2:38 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <7933d51e-92a8-ca6d-84f0-70b22fe17568@samsung.com>

On 11/04, Marek Szyprowski wrote:
> Hi Stephen,
> 
> Krzysztof has left Samsung, but we would like to continue this task, because
> the ABBA dead-locks related to global prepare lock becomes more and more
> problematic for us to workaround.

Hmm. Ok. Thanks for the info.

> 
> On 2016-09-09 02:24, Stephen Boyd wrote:
> 
> >So I'm not very fond of this design because the locking scheme is
> >pretty much out of the hands of the framework and can be easily
> >broken.
> 
> Well, switching from a single global lock to more granular locking
> is always a good approach. Please note that the global lock sooner
> or later became a serious bottleneck if one wants to make somehow
> more aggressive power management and clock gating.

I'm not so sure switching from a global lock to a more granular
lock is _always_ a great idea. Sometimes simpler code is better,
even if it doesn't scale to a million clk nodes. The largest
systems I've seen only have clocks in the hundreds, and a
majority of those aren't rate changing in parallel, so it's not
like we're suffering from VFS type scalability problems here with
tens of thousands of inodes.

That isn't to say I don't agree there's a scalability problem
here, but I'd like to actually see numbers to prove that there's
some sort of scalability problem before making drastic changes.

> 
> >  I'm biased of course, because I'd prefer we go with my
> >wwmutex design of per-clk locks[1]. Taking locks in any order
> >works fine there, and we resolve quite a few long standing
> >locking problems that we have while improving scalability. The
> >problem there is that we don't get the recursive mutex design
> >(maybe that's a benefit!).
> 
> Do you have any plan to continue working on your approach? per-clk
> wwmutex looks like an overkill on the first glance, but that's probably
> the only working solution if you want to get rid of recursive locks.
> I'm still not really convinced that we really need wwmutex here,
> especially if it is possible to guarantee the same order of locking
> operations inside the clock core. This requires a bit of cooperation
> from clock providers (with proper documentation and a list of
> DO/DON'T it shouldn't be that hard).

So far I haven't gotten around to resurrecting the wwmutex stuff.
If you have interest in doing it that's great. Having a locking
scheme with rules of DO/DON'T sounds brittle to me, unless it can
be automated to find problems. I know that I'm not going to spend
time policing that.

> 
> >Once a clk_op reenters the framework
> >with consumer APIs and tries to grab the same lock we deadlock.
> >This is why I've been slowly splitting consumers from providers
> >so we can easily identify these cases. If we had something like
> >coordinated clk rate switching, we could get rid of clk_ops
> >reentering the framework and avoid this problem (and we really do
> >need to do that).
> 
> I'm not sure that this makes really sense split consumers and
> providers. You will get recursive calls to clk core anyway with
> consumers calls if you are implementing i2c clock, for which an i2c
> bus driver does it's own clock gating (i2c controller uses
> consumer clk api).
> 
> 

I suppose this is a different topic. Regardless of the recursive
call or not, we can easily see that a clk consumer is also a clk
provider and just knowing that is useful. Once we know that, we
can look to see if they're calling clk consumer APIs from their
provider callbacks which is not desired because it makes it
impossible to get rid of the recursive lock design. If the lock
is per-clock, then recursion doesn't happen when the provider is
also a consumer. If it does, that's bad and lockdep should tell
us.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply

* [PATCH RFC] mm: Add debug_virt_to_phys()
From: Nicolas Pitre @ 2016-11-12  1:49 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161112004449.30566-1-f.fainelli@gmail.com>

On Fri, 11 Nov 2016, Florian Fainelli wrote:

> When CONFIG_DEBUG_VM is turned on, virt_to_phys() maps to
> debug_virt_to_phys() which helps catch vmalloc space addresses being
> passed. This is helpful in debugging bogus drivers that just assume
> linear mappings all over the place.
> 
> For ARM, ARM64, Unicore32 and Microblaze, the architectures define
> __virt_to_phys() as being the functional implementation of the address
> translation, so we special case the debug stub to call into
> __virt_to_phys directly.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm/include/asm/memory.h      |  4 ++++
>  arch/arm64/include/asm/memory.h    |  4 ++++
>  include/asm-generic/memory_model.h |  4 ++++
>  mm/debug.c                         | 15 +++++++++++++++
>  4 files changed, 27 insertions(+)
> 
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 76cbd9c674df..448dec9b8b00 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -260,11 +260,15 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>   * translation for translating DMA addresses.  Use the driver
>   * DMA support - see dma-mapping.h.
>   */
> +#ifndef CONFIG_DEBUG_VM
>  #define virt_to_phys virt_to_phys
>  static inline phys_addr_t virt_to_phys(const volatile void *x)
>  {
>  	return __virt_to_phys((unsigned long)(x));
>  }
> +#else
> +#define virt_to_phys debug_virt_to_phys
> +#endif
[...]

Why don't you do something more like:

 static inline phys_addr_t virt_to_phys(const volatile void *x)
 {
+        debug_virt_to_phys(x);
         return __virt_to_phys((unsigned long)(x));
 }

[...]

static inline void debug_virt_to_phys(const void *address)
{
#ifdef CONFIG_DEBUG_VM
        BUG_ON(is_vmalloc_addr(address));
#endif
}

?


Nicolas

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox