Re: [PATCH 0/3] Add R8A77970/Eagle PFC support
On Fri, Nov 10, 2017 at 11:02:18PM +0300, Sergei Shtylyov wrote: > Hello! > > Here's the set of 3 patches against Simon Horman's 'renesas.git' repo's > 'renesas-devel-20171110-v4.14-rc8' tag. We're adding the R8A77970 PFC node > and then describing the pins for SCIF0 and EtherAVB devices declared earlier. > These patches depend on the R8A77970 PFC suport in order to work properly. > > [1/3] arm64: dts: renesas: r8a77970: add PFC support > [2/3] arm64: dts: renesas: eagle: add SCIF0 pins > [3/3] arm64: dts: renesas: eagle: add EtherAVB pins Hi Sergei, I have marked these patches as deferred pending acceptance of the PFC driver. Please repost or otherwise ping me once that dependency has been accepted.
Re: [PATCH V3] PCI: rcar: Use runtime PM to control controller clock
On Fri, Nov 10, 2017 at 10:54:11PM +0100, Marek Vasut wrote: > From: Dien Pham > > The controller clock can be switched off during suspend/resume, > let runtime PM take care of that. > > Signed-off-by: Dien Pham > Signed-off-by: Hien Dang > Signed-off-by: Marek Vasut > Cc: Geert Uytterhoeven > Cc: Phil Edworthy > Cc: Simon Horman > Cc: Wolfram Sang > Cc: linux-renesas-soc@vger.kernel.org > To: linux-...@vger.kernel.org Acked-by: Simon Horman
Re: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling
On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote:
> From: Phil Edworthy
>
> Most PCIe host controllers support L0s and L1 power states via ASPM.
> The R-Car hardware only supports L0s, so when the system suspends and
> resumes we have to manually handle L1.
>
> When the system suspends, cards can put themselves into L1 and send a
> PM_ENTER_L1 DLLP to the host controller. At this point, we can no longer
> access the card's config registers.
>
> The R-Car host controller will handle taking cards out of L1 as long as
> the host controller has also been transitioned to L1 link state.
>
> Ideally, we would detect the PM_ENTER_L1 DLLP using an interrupt and
> transition the host to L1 immediately. However, this patch just ensures
> that we can talk to cards after they have gone into L1.
> When attempting a config access, it checks to see if the card has gone
> into L1, and if so, does the same for the host controller.
>
> This is based on a patch by Hien Dang
>
> Signed-off-by: Phil Edworthy
> Signed-off-by: Marek Vasut
> Cc: Geert Uytterhoeven
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
Acked-by: Simon Horman
> ---
> V2: - Drop extra parenthesis
> - Use GENMASK()
> - Fix comment "The HW will handle coming of of L1.", s/of of/out of/
> ---
> drivers/pci/host/pcie-rcar.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index ab61829db389..068bf9067ec1 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -92,6 +92,13 @@
> #define MACCTLR 0x011058
> #define SPEED_CHANGEBIT(24)
> #define SCRAMBLE_DISABLEBIT(27)
> +#define PMSR 0x01105c
> +#define L1FAEG BIT(31)
> +#define PM_ENTER_L1RX BIT(23)
> +#define PMSTATE GENMASK(18, 16)
> +#define PMSTATE_L1 GENMASK(17, 16)
> +#define PMCTLR 0x011060
> +#define L1_INIT BIT(31)
> #define MACS2R 0x011078
> #define MACCGSPSETR 0x011084
> #define SPCNGRSNBIT(31)
> @@ -191,6 +198,7 @@ static int rcar_pcie_config_access(struct rcar_pcie *pcie,
> unsigned int devfn, int where, u32 *data)
> {
> int dev, func, reg, index;
> + u32 val;
>
> dev = PCI_SLOT(devfn);
> func = PCI_FUNC(devfn);
> @@ -232,6 +240,22 @@ static int rcar_pcie_config_access(struct rcar_pcie
> *pcie,
> if (pcie->root_bus_nr < 0)
> return PCIBIOS_DEVICE_NOT_FOUND;
>
> + /*
> + * If we are not in L1 link state and we have received PM_ENTER_L1 DLLP,
> + * transition to L1 link state. The HW will handle coming out of L1.
> + */
> + val = rcar_pci_read_reg(pcie, PMSR);
> + if (val & PM_ENTER_L1RX && (val & PMSTATE) != PMSTATE_L1) {
> + rcar_pci_write_reg(pcie, L1_INIT, PMCTLR);
> +
> + /* Wait until we are in L1 */
> + while (!(val & L1FAEG))
> + val = rcar_pci_read_reg(pcie, PMSR);
> +
> + /* Clear flags indicating link has transitioned to L1 */
> + rcar_pci_write_reg(pcie, L1FAEG | PM_ENTER_L1RX, PMSR);
> + }
> +
> /* Clear errors */
> rcar_pci_write_reg(pcie, rcar_pci_read_reg(pcie, PCIEERRFR), PCIEERRFR);
>
> --
> 2.11.0
>
Re: [PATCH V2 3/5] PCI: rcar: Add the initialization of PCIe link in resume_noirq
On Fri, Nov 10, 2017 at 10:58:41PM +0100, Marek Vasut wrote:
> From: Kazufumi Ikeda
>
> Reestablish the PCIe link very early in the resume process in case it
> went down to prevent PCI accesses from hanging the bus. Such accesses
> can happen early in the PCI resume process, in the resume_noirq, thus
> the link must be reestablished in the resume_noirq callback of the
> driver.
>
> Signed-off-by: Kazufumi Ikeda
> Signed-off-by: Gaku Inami
> Signed-off-by: Marek Vasut
> Cc: Geert Uytterhoeven
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
Acked-by: Simon Horman
> ---
> V2: - Use BIT() macro for (1 << n)
> - Since polling in rcar_pcie_wait_for_dl() uses udelay(), do not
> add extra changes to this function anymore
> - Make resume_noirq return early and clean up parenthesis therein
> ---
> drivers/pci/host/pcie-rcar.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 811e8194ef74..ab61829db389 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -43,6 +43,7 @@
>
> /* Transfer control */
> #define PCIETCTLR0x02000
> +#define DL_DOWN BIT(3)
> #define CFINIT 1
> #define PCIETSTR 0x02004
> #define DATA_LINK_ACTIVE1
> @@ -1107,6 +1108,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> pcie = pci_host_bridge_priv(bridge);
>
> pcie->dev = dev;
> + platform_set_drvdata(pdev, pcie);
>
> INIT_LIST_HEAD(&pcie->resources);
>
> @@ -1167,10 +1169,28 @@ static int rcar_pcie_probe(struct platform_device
> *pdev)
> return err;
> }
>
> +static int rcar_pcie_resume_noirq(struct device *dev)
> +{
> + struct rcar_pcie *pcie = dev_get_drvdata(dev);
> +
> + if (rcar_pci_read_reg(pcie, PMSR) &&
> + !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
> + return 0;
> +
> + /* Re-establish the PCIe link */
> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> + return rcar_pcie_wait_for_dl(pcie);
> +}
> +
> +static const struct dev_pm_ops rcar_pcie_pm_ops = {
> + .resume_noirq = rcar_pcie_resume_noirq,
> +};
> +
> static struct platform_driver rcar_pcie_driver = {
> .driver = {
> .name = "rcar-pcie",
> .of_match_table = rcar_pcie_of_match,
> + .pm = &rcar_pcie_pm_ops,
> .suppress_bind_attrs = true,
> },
> .probe = rcar_pcie_probe,
> --
> 2.11.0
>
Re: [PATCH V2 2/5] PCI: rcar: Clean up the macros
On Fri, Nov 10, 2017 at 10:58:40PM +0100, Marek Vasut wrote: > Just clean up the macros in the RCar PCI driver, no functional change. Could you describe the cleanup in slightly more detail? My reading is 1. Use BIT() macro 2. tidy up whitespace.
Re: [PATCH V2 1/5] PCI: rcar: Poll more often in rcar_pcie_wait_for_dl()
On Fri, Nov 10, 2017 at 10:58:39PM +0100, Marek Vasut wrote:
> The data link active signal usually takes ~20 uSec to be asserted,
> poll the bit more often to avoid useless delays in this function.
>
> Signed-off-by: Marek Vasut
> Cc: Geert Uytterhoeven
> Cc: Phil Edworthy
> Cc: Simon Horman
> Cc: Wolfram Sang
> Cc: linux-renesas-soc@vger.kernel.org
Acked-by: Simon Horman
> ---
> V2: New patch
> ---
> drivers/pci/host/pcie-rcar.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index e00f865952d5..351e1276b90a 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -531,13 +531,13 @@ static void phy_write_reg(struct rcar_pcie *pcie,
>
> static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> {
> - unsigned int timeout = 10;
> + unsigned int timeout = 1;
>
> while (timeout--) {
> if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
> return 0;
>
> - msleep(5);
> + udelay(5);
> }
>
> return -ETIMEDOUT;
> --
> 2.11.0
>
Re: [PATCH 3/3] PCI: rcar: Add the suspend/resume for pcie-rcar driver
On Fri, Nov 10, 2017 at 10:53:07PM +0100, Marek Vasut wrote:
> On 11/10/2017 10:09 AM, Simon Horman wrote:
> > On Wed, Nov 08, 2017 at 10:28:06AM +0100, Marek Vasut wrote:
> >> From: Kazufumi Ikeda
> >>
> >> This adds the suspend/resume supports for pcie-rcar. The resume handler
> >> reprogram the hardware based on the software state kept in specific
> >> device structures. Also it doesn't need to save any registers.
> >>
> >> Signed-off-by: Kazufumi Ikeda
> >> Signed-off-by: Gaku Inami
> >> Signed-off-by: Marek Vasut
> >> Cc: Geert Uytterhoeven
> >> Cc: Simon Horman
> >> Cc: Wolfram Sang
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> ---
> >> drivers/pci/host/pcie-rcar.c | 86
> >> +++-
> >> 1 file changed, 78 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> >> index 2b28292de93a..7a9e30185c79 100644
> >> --- a/drivers/pci/host/pcie-rcar.c
> >> +++ b/drivers/pci/host/pcie-rcar.c
> >> @@ -471,6 +471,36 @@ static void rcar_pcie_force_speedup(struct rcar_pcie
> >> *pcie)
> >> (macsr & LINK_SPEED) == LINK_SPEED_5_0GTS ? "5" : "2.5");
> >> }
> >>
> >> +static int rcar_pcie_hw_enable(struct rcar_pcie *pcie)
> >
> > This function always returns 0 and the value is not checked by the caller.
> > Can we change the return type to void?
>
> Yes, done
>
> >> +{
> >> + struct resource_entry *win;
> >> + LIST_HEAD(res);
> >> + int i = 0;
> >> +
> >> + /* Try setting 5 GT/s link speed */
> >
> > What if it fails?
>
> If it fails, we're back at 2.5 GT/s . The rcar_pcie_force_speedup()
> first checks if the PCIe IP can do 5 GT/s at all. Only if so, tries to
> initiate transition to 5 GT/s operation , checks whether that succeeded
> and if it failed, falls back to 2.5 GT/s .
Thanks, got it.
> >> + rcar_pcie_force_speedup(pcie);
> >> +
> >> + /* Setup PCI resources */
> >> + resource_list_for_each_entry(win, &pcie->resources) {
> >> + struct resource *res = win->res;
> >> +
> >> + if (!res->flags)
> >> + continue;
> >> +
> >> + switch (resource_type(res)) {
> >> + case IORESOURCE_IO:
> >> + case IORESOURCE_MEM:
> >> + rcar_pcie_setup_window(i, pcie, res);
> >> + i++;
> >> + break;
> >> + default:
> >> + continue;
> >
> > Can the default case be omitted?
>
> Sure
>
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int rcar_pcie_enable(struct rcar_pcie *pcie)
> >> {
> >>struct device *dev = pcie->dev;
> >> @@ -872,11 +902,25 @@ static const struct irq_domain_ops msi_domain_ops = {
> >>.map = rcar_msi_map,
> >> };
> >>
> >> +static void rcar_pcie_hw_enable_msi(struct rcar_pcie *pcie)
> >> +{
> >> + struct rcar_msi *msi = &pcie->msi;
> >> + unsigned long base;
> >> +
> >> + /* setup MSI data target */
> >> + base = virt_to_phys((void *)msi->pages);
> >
> > Why do you need to cast to void *?
> > I expect such casting can be done implicitly.
>
> Because __get_free_pages() returns unsigned long and that's what's used
> to assign msi->pages . And virt_to_phys() expects void * instead, thus
> the cast.
Right, but I don't think one should ever need to explicitly cast
to or from void *. What mean is, can you just remove "(void *)" without
changing any behaviour?
>
> >> +
> >> + rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
> >> + rcar_pci_write_reg(pcie, 0, PCIEMSIAUR);
> >> +
> >> + /* enable all MSI interrupts */
> >> + rcar_pci_write_reg(pcie, 0x, PCIEMSIIER);
> >> +}
> >> +
> >> static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
> >> {
> >>struct device *dev = pcie->dev;
> >>struct rcar_msi *msi = &pcie->msi;
> >> - unsigned long base;
> >>int err, i;
> >>
> >>mutex_init(&msi->lock);
> >> @@ -915,13 +959,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie
> >> *pcie)
> >>
> >>/* setup MSI data target */
> >>msi->pages = __get_free_pages(GFP_KERNEL, 0);
> >> - base = virt_to_phys((void *)msi->pages);
> >> -
> >> - rcar_pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
> >> - rcar_pci_write_reg(pcie, 0, PCIEMSIAUR);
> >> -
> >> - /* enable all MSI interrupts */
> >> - rcar_pci_write_reg(pcie, 0x, PCIEMSIIER);
> >> + rcar_pcie_hw_enable_msi(pcie);
> >>
> >>return 0;
> >>
> >> @@ -1202,6 +1240,37 @@ static int rcar_pcie_probe(struct platform_device
> >> *pdev)
> >>return err;
> >> }
> >>
> >> +static int rcar_pcie_resume(struct device *dev)
> >> +{
> >> + struct rcar_pcie *pcie = dev_get_drvdata(dev);
> >> + unsigned int data;
> >> + int err;
> >> + int (*hw_init_fn)(struct rcar_pcie *);
> >
> > Please sort local variables in reverse xmas tree order.
>
> OK
>
> >> +
> >> + err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node);
> >> + if (err)
> >> + return 0;
> >> +
> >> + /* Failure to get a link
Re: [PATCH 1/3] PCI: rcar: Add the initialization of PCIe link in resume_noirq
On Fri, Nov 10, 2017 at 10:14:33PM +0100, Marek Vasut wrote:
> On 11/10/2017 09:59 AM, Simon Horman wrote:
> > Hi Marek,
>
> Hi Simon,
>
> > On Wed, Nov 08, 2017 at 10:28:04AM +0100, Marek Vasut wrote:
> >> From: Kazufumi Ikeda
> >>
> >> Reestablish the PCIe link very early in the resume process in case it
> >> went down to prevent PCI accesses from hanging the bus. Such accesses
> >> can happen early in the PCI resume process, in the resume_noirq, thus
> >> the link must be reestablished in the resume_noirq callback of the
> >> driver.
> >>
> >> Signed-off-by: Kazufumi Ikeda
> >> Signed-off-by: Gaku Inami
> >> Signed-off-by: Marek Vasut
> >> Cc: Geert Uytterhoeven
> >> Cc: Simon Horman
> >> Cc: Wolfram Sang
> >> Cc: linux-renesas-soc@vger.kernel.org
> >
> > For patch-sets (with more than one patch) please provide a cover-letter.
> > The --cover-letter option to git format-patch can help.
> >
> >> ---
> >> drivers/pci/host/pcie-rcar.c | 31 ---
> >> 1 file changed, 28 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> >> index 889603783f01..aa588a7d4811 100644
> >> --- a/drivers/pci/host/pcie-rcar.c
> >> +++ b/drivers/pci/host/pcie-rcar.c
> >> @@ -43,6 +43,7 @@
> >>
> >> /* Transfer control */
> >> #define PCIETCTLR 0x02000
> >> +#define DL_DOWN (1 << 3)
> >
> > Can you use the BIT() macro here?
>
> Yes, I'll also add a patch to the series to convert all the others to
> the BIT() macro, to keep things consistent.
Great, thanks.
> >> #define CFINIT 1
> >> #define PCIETSTR 0x02004
> >> #define DATA_LINK_ACTIVE 1
> >> @@ -529,7 +530,7 @@ static void phy_write_reg(struct rcar_pcie *pcie,
> >>phy_wait_for_ack(pcie);
> >> }
> >>
> >> -static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie)
> >> +static int rcar_pcie_wait_for_dl(struct rcar_pcie *pcie, int atomic)
> >
> > As Sergei mentioned bool seems like a more appropriate type for atomic.
>
> I can actually get rid of this altogether.
Nice :)
> >> {
> >>unsigned int timeout = 10;
> >>
> >> @@ -537,7 +538,10 @@ static int rcar_pcie_wait_for_dl(struct rcar_pcie
> >> *pcie)
> >>if ((rcar_pci_read_reg(pcie, PCIETSTR) & DATA_LINK_ACTIVE))
> >>return 0;
> >>
> >> - msleep(5);
> >> + if (atomic)
> >> + mdelay(5);
> >> + else
> >> + msleep(5);
> >
> > If we must delay, then I suppose this is reasonable.
>
> See above
>
> >>}
> >>
> >>return -ETIMEDOUT;
> >> @@ -595,7 +599,7 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
> >>rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >>
> >>/* This will timeout if we don't have a link. */
> >> - err = rcar_pcie_wait_for_dl(pcie);
> >> + err = rcar_pcie_wait_for_dl(pcie, 0);
> >>if (err)
> >>return err;
> >>
> >> @@ -1110,6 +1114,7 @@ static int rcar_pcie_probe(struct platform_device
> >> *pdev)
> >>pcie = pci_host_bridge_priv(bridge);
> >>
> >>pcie->dev = dev;
> >> + platform_set_drvdata(pdev, pcie);
> >>
> >>INIT_LIST_HEAD(&pcie->resources);
> >>
> >> @@ -1173,10 +1178,30 @@ static int rcar_pcie_probe(struct platform_device
> >> *pdev)
> >>return err;
> >> }
> >>
> >> +static int rcar_pcie_resume_noirq(struct device *dev)
> >> +{
> >> + struct rcar_pcie *pcie = dev_get_drvdata(dev);
> >> + u32 val = rcar_pci_read_reg(pcie, PMSR);
> >> + int ret = 0;
> >> +
> >> + if ((val == 0) || (rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN)) {
> >
> > Please remove the unnecessary parentheses from the line above.
> >
> > Also, I would prefer if the function returned early.
> > Something like (completely untested!):
>
> This should work.
>
> > if (rcar_pci_read_reg(pcie, PMSR) &&
> > !(rcar_pci_read_reg(pcie, PCIETCTLR) & DL_DOWN))
> > return 0;
> >
> > rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> > return rcar_pcie_wait_for_dl(pcie, 1);
> >
> >> + /* Re-establish the PCIe link */
> >> + rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
> >> + ret = rcar_pcie_wait_for_dl(pcie, 1);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static const struct dev_pm_ops rcar_pcie_pm_ops = {
> >> + .resume_noirq = rcar_pcie_resume_noirq,
> >> +};
> >> +
> >> static struct platform_driver rcar_pcie_driver = {
> >>.driver = {
> >>.name = "rcar-pcie",
> >>.of_match_table = rcar_pcie_of_match,
> >> + .pm = &rcar_pcie_pm_ops,
> >>.suppress_bind_attrs = true,
> >>},
> >>.probe = rcar_pcie_probe,
> >> --
> >> 2.11.0
> >>
>
>
> --
> Best regards,
> Marek Vasut
>
Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines
Hi Kieran,
On Sunday, 12 November 2017 18:38:31 EET Kieran Bingham wrote:
> On 12/11/17 04:28, Laurent Pinchart wrote:
> > On Wednesday, 20 September 2017 12:16:54 EET Kieran Bingham wrote:
> >> When used as part of a display pipeline, the VSP is stopped and
> >> restarted explicitly by the DU from its suspend and resume handlers.
> >> There is thus no need to stop or restart pipelines in the VSP suspend
> >> and resume handlers, and doing so would cause the hardware to be
> >> left in a misconfigured state.
> >>
> >> Ensure that the VSP suspend and resume handlers do not affect DRM
> >> based pipelines.
> >
> > s/DRM-base/DRM-based/
>
> -ENOMATCH
This was of course meant to be s/DRM based/DRM-based/ :-)
> >> Signed-off-by: Kieran Bingham
> >> ---
> >>
> >> drivers/media/platform/vsp1/vsp1_drv.c | 16 ++--
> >> 1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> >> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..ed25ba9d551b
> >> 100644
> >> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> >> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> >> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct
> >> device *dev) {
> >>
> >>struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >>
> >> - vsp1_pipelines_suspend(vsp1);
> >> + /*
> >> + * When used as part of a display pipeline, the VSP is stopped and
> >> + * restarted explicitly by the DU
> >
> > s/DU/DU./
> >
> >> + */
> >> + if (!vsp1->drm)
> >> + vsp1_pipelines_suspend(vsp1);
> >> +
> >>
> >>pm_runtime_force_suspend(vsp1->dev);
> >>
> >>return 0;
> >>
> >> @@ -582,7 +588,13 @@ static int __maybe_unused vsp1_pm_resume(struct
> >> device
> >> *dev) struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >>
> >>pm_runtime_force_resume(vsp1->dev);
> >>
> >> - vsp1_pipelines_resume(vsp1);
> >> +
> >> + /*
> >> + * When used as part of a display pipeline, the VSP is stopped and
> >> + * restarted explicitly by the DU
> >
> > s/DU/DU./
> >
> > Apart from that,
> >
> > Reviewed-by: Laurent Pinchart
>
> Thanks,
>
> I'll add the full-stops and repost a v2.1 with your RB tag.
>
> >> + */
> >> + if (!vsp1->drm)
> >> + vsp1_pipelines_resume(vsp1);
> >>
> >>return 0;
> >>
> >> }
--
Regards,
Laurent Pinchart
Re: [PATCH v2] media: vsp1: Prevent suspending and resuming DRM pipelines
Hi Laurent,
On 12/11/17 04:28, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Wednesday, 20 September 2017 12:16:54 EET Kieran Bingham wrote:
>> When used as part of a display pipeline, the VSP is stopped and
>> restarted explicitly by the DU from its suspend and resume handlers.
>> There is thus no need to stop or restart pipelines in the VSP suspend
>> and resume handlers, and doing so would cause the hardware to be
>> left in a misconfigured state.
>>
>> Ensure that the VSP suspend and resume handlers do not affect DRM
>> based pipelines.
>
> s/DRM-base/DRM-based/
-ENOMATCH
>
>> Signed-off-by: Kieran Bingham
>> ---
>> drivers/media/platform/vsp1/vsp1_drv.c | 16 ++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
>> b/drivers/media/platform/vsp1/vsp1_drv.c index 962e4c304076..ed25ba9d551b
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drv.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>> @@ -571,7 +571,13 @@ static int __maybe_unused vsp1_pm_suspend(struct device
>> *dev) {
>> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>
>> -vsp1_pipelines_suspend(vsp1);
>> +/*
>> + * When used as part of a display pipeline, the VSP is stopped and
>> + * restarted explicitly by the DU
>
> s/DU/DU./
>
>> + */
>> +if (!vsp1->drm)
>> +vsp1_pipelines_suspend(vsp1);
>> +
>> pm_runtime_force_suspend(vsp1->dev);
>>
>> return 0;
>> @@ -582,7 +588,13 @@ static int __maybe_unused vsp1_pm_resume(struct device
>> *dev) struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>
>> pm_runtime_force_resume(vsp1->dev);
>> -vsp1_pipelines_resume(vsp1);
>> +
>> +/*
>> + * When used as part of a display pipeline, the VSP is stopped and
>> + * restarted explicitly by the DU
>
> s/DU/DU./
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart
Thanks,
I'll add the full-stops and repost a v2.1 with your RB tag.
>
>> + */
>> +if (!vsp1->drm)
>> +vsp1_pipelines_resume(vsp1);
>>
>> return 0;
>> }
>
