Re: [PATCH v4] sched: fix incorrect wait time and wait count statistics

2015-11-06 Thread Joonwoo Park 
On Fri, Nov 06, 2015 at 02:57:49PM +0100, Peter Zijlstra wrote:
> On Tue, Oct 27, 2015 at 09:46:53PM -0700, Joonwoo Park wrote:
> > @@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned 
> > int new_cpu)
> > WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
> > !p->on_rq);
> >  
> > +   /*
> > +* Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
> > +* because schedstat_wait_{start,end} rebase migrating task's wait_start
> > +* time relying on p->on_rq.
> > +*/
> > +   WARN_ON_ONCE(p->state == TASK_RUNNING &&
> > +p->sched_class == _sched_class &&
> > +(p->on_rq && !task_on_rq_migrating(p)));
> > +
> 
> Why do we have to test p->on_rq? Would not ->state == RUNNING imply
> that?
> 

sched_fork() sets p->state = RUNNING before changing task cpu.
Please let me know if you got better idea.

> > +++ b/kernel/sched/fair.c
> > @@ -737,41 +737,69 @@ static void update_curr_fair(struct rq *rq)
> > update_curr(cfs_rq_of(>curr->se));
> >  }
> >  
> > +#ifdef CONFIG_SCHEDSTATS
> >  static inline void
> >  update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> > +   u64 wait_start = rq_clock(rq_of(cfs_rq));
> >  
> > +   if (entity_is_task(se) && task_on_rq_migrating(task_of(se)) &&
> > +   likely(wait_start > se->statistics.wait_start))
> > +   wait_start -= se->statistics.wait_start;
> > +
> > +   schedstat_set(se->statistics.wait_start, wait_start);
> >  }
> >  
> >  static void
> >  update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> 
> Since this is now all under CONFIG_SCHEDSTAT, would it not make sense
> to do something like:
> 
>   u64 now = rq_clock(rq_of(cfs_rq));
> 
> to avoid the endless calling of that function?
> 
> Also, for that very same reason; would it not make sense to drop the
> schedstat_set() usage below, that would greatly enhance readability.
> 

Agreed.

> > +   if (entity_is_task(se) && task_on_rq_migrating(task_of(se))) {
> > +   /*
> > +* Preserve migrating task's wait time so wait_start time stamp
> > +* can be adjusted to accumulate wait time prior to migration.
> > +*/
> > +   schedstat_set(se->statistics.wait_start,
> > + rq_clock(rq_of(cfs_rq)) -
> > + se->statistics.wait_start);
> > +   return;
> > +   }
> > +
> > schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
> > rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
> > schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);
> > schedstat_set(se->statistics.wait_sum, se->statistics.wait_sum +
> > rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
> > +
> > if (entity_is_task(se)) {
> > trace_sched_stat_wait(task_of(se),
> > rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
> > }
> 
> Is there no means of collapsing the two 'entity_is_task()' branches?
> 

Agreed.  Will spin v5 with these clean up.

Thanks,
Joonwoo

> > schedstat_set(se->statistics.wait_start, 0);
> >  }
> > +#else
> > +static inline void
> > +update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > +{
> > +}
> > +
> > +static inline void
> > +update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > +{
> > +}
> > +#endif

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] mmc: sdhci-msm: Boost controller core clock

2015-11-06 Thread Bjorn Andersson 
On Fri 06 Nov 00:10 PST 2015, Ulf Hansson wrote:

> On 6 November 2015 at 02:42, Bjorn Andersson  wrote:
> > On Mon, Jul 6, 2015 at 4:53 AM, Ivan T. Ivanov  
> > wrote:
> >> Ensure SDCC is working with maximum clock otherwise card
> >> detection could be extremely slow, up to 7 seconds.
> >>
> >> Signed-off-by: Ivan T. Ivanov 
> >> Reviewed-by: Georgi Djakov 
> >> Acked-by: Stephen Boyd 
> >> ---
> >>
> >> Changes since v0:
> >> - s/falied/failed in warning message.
> >>
> >>  drivers/mmc/host/sdhci-msm.c | 5 +
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> >> index 4a09f76..4bcee03 100644
> >> --- a/drivers/mmc/host/sdhci-msm.c
> >> +++ b/drivers/mmc/host/sdhci-msm.c
> >> @@ -489,6 +489,11 @@ static int sdhci_msm_probe(struct platform_device 
> >> *pdev)
> >> goto pclk_disable;
> >> }
> >>
> >> +   /* Vote for maximum clock rate for maximum performance */
> >> +   ret = clk_set_rate(msm_host->clk, INT_MAX);
> >> +   if (ret)
> >> +   dev_warn(>dev, "core clock boost failed\n");
> >> +
> >
> > On my 8974AC devices this results in GCC_SDCC1_APPS_CLK changing from
> > 100MHz to 200MHz for my eMMC. Unfortunately this results in the
> > following error:
> >
> > [5.103241] mmcblk0: retrying because a re-tune was needed
> > [5.109270] mmcblk0: error -84 transferring data, sector 5816322,
> > nr 2, cmd response 0x900, card status 0xc00
> >
> > Looking at the board specification it's stated that these card should
> > run in DDR50, so I've tried specifying "max-frequency" in the dt. I
> > verified in sdhci_set_clock() that we get a divisor of 4, but the
> > result is a repetition of:
> 
> I don't follow. Are you saying that changing the clock frequency to
> 200MHz caused the card to be initialized in HS200 mode instead of
> DDR50?
> 

No, we clock the sdhci block at 100MHz, the host->max_clk is 200MHz and
the divisor in sdhci_set_clock() becomes 1. So if I read this correctly
we're running HS200 at 100MHz.

Bumping the clock rate to 200MHz at the block doesn't affect the max_clk
and hence we're trying to run the bus at 200MHz.

I therefor tried to just set "max-frequency" to 50MHz, getting the
divider to be 4 and the below error.

So I assume it just happened to work at 100MHz, but 200MHz is way off
from the 50MHz the board is designed and tested for.


Unfortunately I don't have the equipment to measure these assumptions :/

> >
> > [1.702312] mmc1: Switching to 3.3V signalling voltage failed
> > [1.837987] mmc1: power class selection to bus width 8 ddr 0 failed
> > [1.846227] mmc1: error -110 whilst initialising MMC card
> > [1.946303] mmc1: Reset 0x1 never completed.
> >
> > I tried to disable HS200 by specifying SDHCI_QUIRK2_BROKEN_HS200. This
> > makes the card come up nicely again.
> >
> >
> > Is there any other way to force the link to DDR50? Is it acceptable to
> > expose the broken-hs200 quirk in dt so I can use that for now?
> 
> We already have these DT bindings.
> - mmc-ddr-1_8v: eMMC high-speed DDR mode(1.8V I/O) is supported
> - mmc-ddr-1_2v: eMMC high-speed DDR mode(1.2V I/O) is supported
> - mmc-hs200-1_8v: eMMC HS200 mode(1.8V I/O) is supported
> - mmc-hs200-1_2v: eMMC HS200 mode(1.2V I/O) is supported
> 
> Can't we use these instead?
> 

These are all additive and the hardware is correctly advertising that
it's capable of supporting SDR104, which implies HS200. Dropping this
assumption (or unsetting these bits) drops us down to the high-speed
rates and things work again.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: chipidea: msm: Use posted data writes on AHB

2015-11-06 Thread Peter Chen 
On Fri, Nov 06, 2015 at 12:04:06AM -0600, Andy Gross wrote:
> This patch sets the AHBMODE to allow for posted data writes.  This
> results in higher performance.
> 
> Signed-off-by: Andy Gross 
> ---
>  drivers/usb/chipidea/ci_hdrc_msm.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c 
> b/drivers/usb/chipidea/ci_hdrc_msm.c
> index d79ecc0..3889809 100644
> --- a/drivers/usb/chipidea/ci_hdrc_msm.c
> +++ b/drivers/usb/chipidea/ci_hdrc_msm.c
> @@ -25,7 +25,8 @@ static void ci_hdrc_msm_notify_event(struct ci_hdrc *ci, 
> unsigned event)
>   case CI_HDRC_CONTROLLER_RESET_EVENT:
>   dev_dbg(dev, "CI_HDRC_CONTROLLER_RESET_EVENT received\n");
>   writel(0, USB_AHBBURST);
> - writel(0, USB_AHBMODE);
> + /* use AHB transactor, allow posted data writes */
> + writel(0x8, USB_AHBMODE);
>   usb_phy_init(ci->usb_phy);
>   break;
>   case CI_HDRC_CONTROLLER_STOPPED_EVENT:
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

Greg, another related changes is at host driver, pick it up
directly please, thanks.

Acked-by: Peter Chen 

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: host: ehci-msm: Use posted data writes on AHB

2015-11-06 Thread Georgi Djakov 
On 11/06/2015 08:04 AM, Andy Gross wrote:
> This patch sets the AHBMODE to allow for posted data writes.  This
> results in higher performance.
> 
> Signed-off-by: Andy Gross 

With these patches I see significant improvement in throughput
on my db410c board.

Tested-by: Georgi Djakov 

> ---
>  drivers/usb/host/ehci-msm.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index c4f84c8..c23e285 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -57,8 +57,8 @@ static int ehci_msm_reset(struct usb_hcd *hcd)
>  
>   /* bursts of unspecified length. */
>   writel(0, USB_AHBBURST);
> - /* Use the AHB transactor */
> - writel(0, USB_AHBMODE);
> + /* Use the AHB transactor, allow posted data writes */
> + writel(0x8, USB_AHBMODE);
>   /* Disable streaming mode and select host mode */
>   writel(0x13, USB_USBMODE);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: host: ehci-msm: Use posted data writes on AHB

2015-11-06 Thread Alan Stern 
On Fri, 6 Nov 2015, Georgi Djakov wrote:

> On 11/06/2015 08:04 AM, Andy Gross wrote:
> > This patch sets the AHBMODE to allow for posted data writes.  This
> > results in higher performance.
> > 
> > Signed-off-by: Andy Gross 
> 
> With these patches I see significant improvement in throughput
> on my db410c board.
> 
> Tested-by: Georgi Djakov 
> 
> > ---
> >  drivers/usb/host/ehci-msm.c |4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> > index c4f84c8..c23e285 100644
> > --- a/drivers/usb/host/ehci-msm.c
> > +++ b/drivers/usb/host/ehci-msm.c
> > @@ -57,8 +57,8 @@ static int ehci_msm_reset(struct usb_hcd *hcd)
> >  
> > /* bursts of unspecified length. */
> > writel(0, USB_AHBBURST);
> > -   /* Use the AHB transactor */
> > -   writel(0, USB_AHBMODE);
> > +   /* Use the AHB transactor, allow posted data writes */
> > +   writel(0x8, USB_AHBMODE);
> > /* Disable streaming mode and select host mode */
> > writel(0x13, USB_USBMODE);

Acked-by: Alan Stern 

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Remove the need for qcom,{msm-id,board-id,pmic-id}

2015-11-06 Thread Kevin Hilman 
Hi Stephen,

Stephen Boyd  writes:

> This patchset documents a compatible string format that encodes
> all the information that was being encoded in qcom specific DT
> properties in downstream msm kernels. The goal being to come
> up with a format that will allow us to express the information
> we want to express without requiring the use of vendor specific
> properties. An updated dtbTool will be released after these new
> bindings are accepted so that users can work with non-updateable
> bootloaders.
>
> This is an attempt to resolve a discussion around March of this year[1]
> where the arm-soc maintainers suggested we express this information
> through the board's compatible string.

Thanks for working on this.  This indeed looks like the right way to
handle this, including updating dtbTool.  Very nice.

I guess the DT maintainers should have a crack at the bindings, but
otherwise it looks good.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] devicetree: bindings: Document qcom board compatible format

2015-11-06 Thread Andy Gross 
On Mon, Oct 26, 2015 at 02:25:10PM -0700, Stephen Boyd wrote:
> Some qcom based bootloaders identify the dtb blob based on a set
> of device properties like SoC, platform, PMIC, and revisions of
> those components. In downstream kernels, these values are added
> to the different component dtsi files (i.e. pmic dtsi file, SoC
> dtsi file, board dtsi file, etc.) via qcom specific DT
> properties. The dtb files are parsed by a program called dtbTool
> that picks out these properties and creates a table of contents
> binary blob with the property information and some offsets into
> the concatenation of all the dtbs (termed a QCDT image).
> 
> The suggestion is to do this via the board compatible string
> instead, because these qcom specific properties are never used by
> the kernel. Add a document describing the format of the
> compatible string that encodes all this information that's
> currently encoded in the qcom,{msm-id,board-id,pmic-id}
> properties in downstream devicetrees. Future bootloaders may be
> updated to look at the compatible field instead of looking for
> the table of contents image. For non-updateable bootloaders, a
> new dtbTool program will parse the compatible string and generate
> a QCDT image from it.
> 
> Signed-off-by: Stephen Boyd 
> ---

Looks workable.

Reviewed-by: Andy Gross 

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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] sched: fix incorrect wait time and wait count statistics

2015-11-06 Thread Peter Zijlstra 
On Tue, Oct 27, 2015 at 09:46:53PM -0700, Joonwoo Park wrote:
> @@ -1272,6 +1272,15 @@ void set_task_cpu(struct task_struct *p, unsigned int 
> new_cpu)
>   WARN_ON_ONCE(p->state != TASK_RUNNING && p->state != TASK_WAKING &&
>   !p->on_rq);
>  
> + /*
> +  * Migrating fair class task must have p->on_rq = TASK_ON_RQ_MIGRATING,
> +  * because schedstat_wait_{start,end} rebase migrating task's wait_start
> +  * time relying on p->on_rq.
> +  */
> + WARN_ON_ONCE(p->state == TASK_RUNNING &&
> +  p->sched_class == _sched_class &&
> +  (p->on_rq && !task_on_rq_migrating(p)));
> +

Why do we have to test p->on_rq? Would not ->state == RUNNING imply
that?

> +++ b/kernel/sched/fair.c
> @@ -737,41 +737,69 @@ static void update_curr_fair(struct rq *rq)
>   update_curr(cfs_rq_of(>curr->se));
>  }
>  
> +#ifdef CONFIG_SCHEDSTATS
>  static inline void
>  update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
> + u64 wait_start = rq_clock(rq_of(cfs_rq));
>  
> + if (entity_is_task(se) && task_on_rq_migrating(task_of(se)) &&
> + likely(wait_start > se->statistics.wait_start))
> + wait_start -= se->statistics.wait_start;
> +
> + schedstat_set(se->statistics.wait_start, wait_start);
>  }
>  
>  static void
>  update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {

Since this is now all under CONFIG_SCHEDSTAT, would it not make sense
to do something like:

u64 now = rq_clock(rq_of(cfs_rq));

to avoid the endless calling of that function?

Also, for that very same reason; would it not make sense to drop the
schedstat_set() usage below, that would greatly enhance readability.

> + if (entity_is_task(se) && task_on_rq_migrating(task_of(se))) {
> + /*
> +  * Preserve migrating task's wait time so wait_start time stamp
> +  * can be adjusted to accumulate wait time prior to migration.
> +  */
> + schedstat_set(se->statistics.wait_start,
> +   rq_clock(rq_of(cfs_rq)) -
> +   se->statistics.wait_start);
> + return;
> + }
> +
>   schedstat_set(se->statistics.wait_max, max(se->statistics.wait_max,
>   rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start));
>   schedstat_set(se->statistics.wait_count, se->statistics.wait_count + 1);
>   schedstat_set(se->statistics.wait_sum, se->statistics.wait_sum +
>   rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
> +
>   if (entity_is_task(se)) {
>   trace_sched_stat_wait(task_of(se),
>   rq_clock(rq_of(cfs_rq)) - se->statistics.wait_start);
>   }

Is there no means of collapsing the two 'entity_is_task()' branches?

>   schedstat_set(se->statistics.wait_start, 0);
>  }
> +#else
> +static inline void
> +update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +{
> +}
> +
> +static inline void
> +update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +{
> +}
> +#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] PCI: qcom: Add Qualcomm PCIe controller driver

2015-11-06 Thread Bjorn Andersson 
On Mon 04 May 05:42 PDT 2015, Stanimir Varbanov wrote:

> The PCIe driver reuse the Designware common code for host
> and MSI initialization, and also program the Qualcomm
> application specific registers.
> 

I want to get the ethernet on the ifc6410 running and this seems like
the last patchset for the PCIe.

> Signed-off-by: Stanimir Varbanov 
[..]
> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
> new file mode 100644
> index 000..4f083c6
> --- /dev/null
> +++ b/drivers/pci/host/pcie-qcom.c
> @@ -0,0 +1,677 @@
> +/*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.

Bump the year, it's the future now :)

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
[..]
> +
> +struct qcom_pcie {
> + struct pcie_port pp;
> + struct device *dev;

You already have this device pointer in pp->dev.

> + union pcie_resources res;
> + void __iomem *parf;
> + void __iomem *dbi;
> + void __iomem *elbi;
> + struct phy *phy;
> + struct gpio_desc *reset;
> + unsigned int version;
> +};
> +
> +#define to_qcom_pcie(x)  container_of(x, struct qcom_pcie, pp)
> +
> +static inline void
> +writel_masked(void __iomem *addr, u32 clear_mask, u32 set_mask)
> +{
> + u32 val = readl(addr);
> +
> + val &= ~clear_mask;
> + val |= set_mask;
> + writel(val, addr);
> +}

There are no case where you do clear and set, so I would like that you
just inline this function where you need it. It adds 2 extra lines at a
few places, but a lot of clarity.

> +
> +static void qcom_ep_reset_assert_deassert(struct qcom_pcie *pcie, int assert)
> +{
> + int val, active_low;
> +
> + if (IS_ERR_OR_NULL(pcie->reset))
> + return;

This will be NULL or valid here, never ERR. So it's fine to call
gpiod_set_value() with this pointer without the check.

> +
> + active_low = gpiod_is_active_low(pcie->reset);
> +

gpiod_set_value() checks for FLAG_ACTIVE_LOW and will invert the value
first thing. So you do not need to do this manually.

> + if (assert)
> + val = !!active_low;
> + else
> + val = !active_low;
> +
> + gpiod_set_value(pcie->reset, val);
> +
> + usleep_range(PERST_DELAY_MIN_US, PERST_DELAY_MAX_US);

This doesn't seem to be called in a hot path, so you should be able to
simply msleep(1) here.

> +}
> +
> +static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> +{
> + qcom_ep_reset_assert_deassert(pcie, 1);

If we're down to this function just being a gpiod_set_value() and a
sleep we can inline it here instead of having this proxy function.

> +}
> +
> +static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> +{
> + qcom_ep_reset_assert_deassert(pcie, 0);

Same here.

> +}
> +
> +static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
> +{
> + struct pcie_port *pp = arg;
> +
> + return dw_handle_msi_irq(pp);
> +}
> +
> +static int qcom_pcie_link_up(struct pcie_port *pp)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pp);
> + u32 val = readl(pcie->dbi + PCIE20_CAP_LINKCTRLSTATUS);
> +
> + return val & BIT(29) ? 1 : 0;

return !!(val & BIT(29));

Do we know what this bit 29 is?

> +}
> +
[..]
> +
> +static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
> +{
> + struct qcom_pcie_resources_v1 *res = >res.v1;
> + struct device *dev = pcie->dev;
> +
> + res->vdda = devm_regulator_get(dev, "vdda");
> + if (IS_ERR(res->vdda))
> + return PTR_ERR(res->vdda);
> +
> + res->iface = devm_clk_get(dev, "iface");
> + if (IS_ERR(res->iface))
> + return PTR_ERR(res->iface);
> +
> + res->aux = devm_clk_get(dev, "aux");
> + if (IS_ERR(res->aux) && PTR_ERR(res->aux) == -EPROBE_DEFER)

PTR_ERR() == x implies IS_ERR(), so you don't need both.

> + return -EPROBE_DEFER;
> + else if (IS_ERR(res->aux))
> + res->aux = NULL;
> +
> + res->master_bus = devm_clk_get(dev, "master_bus");
> + if (IS_ERR(res->master_bus))
> + return PTR_ERR(res->master_bus);
> +
> + res->slave_bus = devm_clk_get(dev, "slave_bus");
> + if (IS_ERR(res->slave_bus))
> + return PTR_ERR(res->slave_bus);
> +
> + res->core = devm_reset_control_get(dev, "core");
> + if (IS_ERR(res->core))
> + return PTR_ERR(res->core);
> +
> + return 0;
> +}
> +
> +static int qcom_pcie_enable_link_training(struct pcie_port *pp)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pp);
> + struct device