[PATCH 1/8] i2c: i2c-qcom-geni: Provide support for ACPI

[PATCH 1/8] i2c: i2c-qcom-geni: Provide support for ACPI

[Lee Jones]

Add a match table to allow automatic probing of ACPI device
QCOM0220.  Ignore clock attainment errors.  Set default clock
frequency value.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index db075bc0d952..0fa93b448e8d 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
@@ -483,6 +484,12 @@ static const struct i2c_algorithm geni_i2c_algo = {
 	.functionality	= geni_i2c_func,
 };
 
+static const struct acpi_device_id geni_i2c_acpi_match[] = {
+	{ "QCOM0220"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
+
 static int geni_i2c_probe(struct platform_device *pdev)
 {
 	struct geni_i2c_dev *gi2c;
@@ -502,7 +509,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(gi2c->se.base);
 
 	gi2c->se.clk = devm_clk_get(&pdev->dev, "se");
-	if (IS_ERR(gi2c->se.clk)) {
+	if (IS_ERR(gi2c->se.clk) && !ACPI_HANDLE(&pdev->dev)) {
 		ret = PTR_ERR(gi2c->se.clk);
 		dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
 		return ret;
@@ -510,12 +517,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	ret = device_property_read_u32(&pdev->dev, "clock-frequency",
 							&gi2c->clk_freq_out);
-	if (ret) {
+	if (ret && !ACPI_HANDLE(&pdev->dev)) {
 		dev_info(&pdev->dev,
 			"Bus frequency not specified, default to 100kHz.\n");
 		gi2c->clk_freq_out = KHZ(100);
 	}
 
+	if (ACPI_HANDLE(&pdev->dev)) {
+		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(&pdev->dev));
+
+		/* Using default, same as the !ACPI case above */
+		gi2c->clk_freq_out = KHZ(100);
+	}
+
 	gi2c->irq = platform_get_irq(pdev, 0);
 	if (gi2c->irq < 0) {
 		dev_err(&pdev->dev, "IRQ error for i2c-geni\n");
@@ -660,6 +674,7 @@ static struct platform_driver geni_i2c_driver = {
 		.name = "geni_i2c",
 		.pm = &geni_i2c_pm_ops,
 		.of_match_table = geni_i2c_dt_match,
+		.acpi_match_table = ACPI_PTR(geni_i2c_acpi_match),
 	},
 };
 
-- 
2.17.1

[PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

The Qualcomm Geni I2C driver currently probes silently which can be
confusing when debugging potential issues.  Add a low level (INFO)
print when each I2C controller is successfully initially set-up.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 0fa93b448e8d..e27466d77767 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Bjorn Andersson]

On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:

> The Qualcomm Geni I2C driver currently probes silently which can be
> confusing when debugging potential issues.  Add a low level (INFO)
> print when each I2C controller is successfully initially set-up.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 0fa93b448e8d..e27466d77767 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
> +

I would prefer that we do not add such prints, as it would be to accept
the downstream behaviour of spamming the log to the point where no one
will ever look through it.

Regards,
Bjorn

>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

On Tue, 04 Jun 2019, Bjorn Andersson wrote:

> On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> 
> > The Qualcomm Geni I2C driver currently probes silently which can be
> > confusing when debugging potential issues.  Add a low level (INFO)
> > print when each I2C controller is successfully initially set-up.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 0fa93b448e8d..e27466d77767 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
> > +
> 
> I would prefer that we do not add such prints, as it would be to accept
> the downstream behaviour of spamming the log to the point where no one
> will ever look through it.

We should be able to find a middle ground.  Spamming the log with all
sorts of device specific information/debug is obviously not
constructive, but a single liner to advertise that an important
device/controller has been successfully initialised is more helpful
than it is hinderous.

This print was added due to the silent initialisation costing me
several hours of debugging ACPI device/driver code (albeit learning a
lot about ACPI as I go) just to find out that it was already doing the
right thing - just very quietly.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Ard Biesheuvel]

On Wed, 5 Jun 2019 at 09:16, Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 04 Jun 2019, Bjorn Andersson wrote:
>
> > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> >
> > > The Qualcomm Geni I2C driver currently probes silently which can be
> > > confusing when debugging potential issues.  Add a low level (INFO)
> > > print when each I2C controller is successfully initially set-up.
> > >
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > > index 0fa93b448e8d..e27466d77767 100644
> > > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > > @@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
> > >             return ret;
> > >     }
> > >
> > > +   dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
> > > +
> >
> > I would prefer that we do not add such prints, as it would be to accept
> > the downstream behaviour of spamming the log to the point where no one
> > will ever look through it.
>
> We should be able to find a middle ground.  Spamming the log with all
> sorts of device specific information/debug is obviously not
> constructive, but a single liner to advertise that an important
> device/controller has been successfully initialised is more helpful
> than it is hinderous.
>
> This print was added due to the silent initialisation costing me
> several hours of debugging ACPI device/driver code (albeit learning a
> lot about ACPI as I go) just to find out that it was already doing the
> right thing - just very quietly.
>

I agree.

There are numerous EHCI drivers IIRC which, if compiled in,
unconditionally print some blurb, whether you have the hardware or
not, which is pretty annoying.

In this case, however, having a single line per successfully probed
device (containing the dev_name and perhaps the MMIO base address or
some other identifying feature) is pretty useful, and shouldn't be
regarded as log spamming imo. dev_info() honours the 'quiet' kernel
command line parameter, and so you will only see the message if you
actually look at the log.

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

On Wed, 05 Jun 2019, Ard Biesheuvel wrote:

> On Wed, 5 Jun 2019 at 09:16, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 04 Jun 2019, Bjorn Andersson wrote:
> >
> > > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > >
> > > > The Qualcomm Geni I2C driver currently probes silently which can be
> > > > confusing when debugging potential issues.  Add a low level (INFO)
> > > > print when each I2C controller is successfully initially set-up.
> > > >
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > index 0fa93b448e8d..e27466d77767 100644
> > > > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > @@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
> > > >             return ret;
> > > >     }
> > > >
> > > > +   dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
> > > > +
> > >
> > > I would prefer that we do not add such prints, as it would be to accept
> > > the downstream behaviour of spamming the log to the point where no one
> > > will ever look through it.
> >
> > We should be able to find a middle ground.  Spamming the log with all
> > sorts of device specific information/debug is obviously not
> > constructive, but a single liner to advertise that an important
> > device/controller has been successfully initialised is more helpful
> > than it is hinderous.
> >
> > This print was added due to the silent initialisation costing me
> > several hours of debugging ACPI device/driver code (albeit learning a
> > lot about ACPI as I go) just to find out that it was already doing the
> > right thing - just very quietly.
> >
> 
> I agree.
> 
> There are numerous EHCI drivers IIRC which, if compiled in,
> unconditionally print some blurb, whether you have the hardware or
> not, which is pretty annoying.
> 
> In this case, however, having a single line per successfully probed
> device (containing the dev_name and perhaps the MMIO base address or
> some other identifying feature) is pretty useful, and shouldn't be
> regarded as log spamming imo. dev_info() honours the 'quiet' kernel
> command line parameter, and so you will only see the message if you
> actually look at the log.

+999

This is exactly as I see it.

If people want a quiet log/fast boot-up times, they can request it.
Otherwise, it's far more useful to trade a second or two for
important information such as which devices are present/enabled on a
platform.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Johan Hovold]

On Wed, Jun 05, 2019 at 08:16:25AM +0100, Lee Jones wrote:
> On Tue, 04 Jun 2019, Bjorn Andersson wrote:
> 
> > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > 
> > > The Qualcomm Geni I2C driver currently probes silently which can be
> > > confusing when debugging potential issues.  Add a low level (INFO)
> > > print when each I2C controller is successfully initially set-up.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > > index 0fa93b448e8d..e27466d77767 100644
> > > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > > @@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
> > >  		return ret;
> > >  	}
> > >  
> > > +	dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
> > > +
> > 
> > I would prefer that we do not add such prints, as it would be to accept
> > the downstream behaviour of spamming the log to the point where no one
> > will ever look through it.
> 
> We should be able to find a middle ground.  Spamming the log with all
> sorts of device specific information/debug is obviously not
> constructive, but a single liner to advertise that an important
> device/controller has been successfully initialised is more helpful
> than it is hinderous.
> 
> This print was added due to the silent initialisation costing me
> several hours of debugging ACPI device/driver code (albeit learning a
> lot about ACPI as I go) just to find out that it was already doing the
> right thing - just very quietly.

No, we don't add noise like this to the logs just because it may be
useful while debugging. Even one-liners add up.

There are plenty of options for debugging already ranging from adding a
temporary dev_info() to the probe function in question to using dynamic
debugging to have driver core log every successful probe.

And in this case you say the driver was in fact already bound; that can
easily be verified through sysfs too in case things aren't behaving the
way you expect.

Johan

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

On Wed, 05 Jun 2019, Johan Hovold wrote:

> On Wed, Jun 05, 2019 at 08:16:25AM +0100, Lee Jones wrote:
> > On Tue, 04 Jun 2019, Bjorn Andersson wrote:
> > 
> > > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > > 
> > > > The Qualcomm Geni I2C driver currently probes silently which can be
> > > > confusing when debugging potential issues.  Add a low level (INFO)
> > > > print when each I2C controller is successfully initially set-up.
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > index 0fa93b448e8d..e27466d77767 100644
> > > > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > > > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > > > @@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > +	dev_info(&pdev->dev, "Geni-I2C adaptor successfully added\n");
> > > > +
> > > 
> > > I would prefer that we do not add such prints, as it would be to accept
> > > the downstream behaviour of spamming the log to the point where no one
> > > will ever look through it.
> > 
> > We should be able to find a middle ground.  Spamming the log with all
> > sorts of device specific information/debug is obviously not
> > constructive, but a single liner to advertise that an important
> > device/controller has been successfully initialised is more helpful
> > than it is hinderous.
> > 
> > This print was added due to the silent initialisation costing me
> > several hours of debugging ACPI device/driver code (albeit learning a
> > lot about ACPI as I go) just to find out that it was already doing the
> > right thing - just very quietly.
> 
> No, we don't add noise like this to the logs just because it may be
> useful while debugging. Even one-liners add up.

One line per device is should not cause an issue.

Problems occur when developers try to print all kinds of device
specifics to the boot log.  A simple, single line for such an
important device/controller has more benefits than drawbacks.

> There are plenty of options for debugging already ranging from adding a
> temporary dev_info() to the probe function in question to using dynamic
> debugging to have driver core log every successful probe.

This is what I ended up doing.  It was time consuming to parse though
a log of that size when you have no paging or keyboard.

> And in this case you say the driver was in fact already bound; that can
> easily be verified through sysfs too in case things aren't behaving the
> way you expect.

Not in a non-booting system with no keyboard you can't. ;)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Johan Hovold]

On Wed, Jun 05, 2019 at 09:20:47AM +0100, Lee Jones wrote:
> On Wed, 05 Jun 2019, Johan Hovold wrote:

> > No, we don't add noise like this to the logs just because it may be
> > useful while debugging. Even one-liners add up.
> 
> One line per device is should not cause an issue.
> 
> Problems occur when developers try to print all kinds of device
> specifics to the boot log.  A simple, single line for such an
> important device/controller has more benefits than drawbacks.

What about the thousands of probe functions which do not currently spam
the logs? If you want to see all successful probes reliably, you tell
driver core to print it.

> > There are plenty of options for debugging already ranging from adding a
> > temporary dev_info() to the probe function in question to using dynamic
> > debugging to have driver core log every successful probe.
> 
> This is what I ended up doing.  It was time consuming to parse though
> a log of that size when you have no paging or keyboard.

With the right command-line option to enable dynamic debugging you get
one line per successful probe, just like you wanted. Or are you now
saying that one-line per device is too much after all? ;)

> > And in this case you say the driver was in fact already bound; that can
> > easily be verified through sysfs too in case things aren't behaving the
> > way you expect.
> 
> Not in a non-booting system with no keyboard you can't. ;)

Fair enough, but the above would still work.

Johan

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

On Wed, 05 Jun 2019, Johan Hovold wrote:

> On Wed, Jun 05, 2019 at 09:20:47AM +0100, Lee Jones wrote:
> > On Wed, 05 Jun 2019, Johan Hovold wrote:
> 
> > > No, we don't add noise like this to the logs just because it may be
> > > useful while debugging. Even one-liners add up.
> > 
> > One line per device is should not cause an issue.
> > 
> > Problems occur when developers try to print all kinds of device
> > specifics to the boot log.  A simple, single line for such an
> > important device/controller has more benefits than drawbacks.
> 
> What about the thousands of probe functions which do not currently spam
> the logs? If you want to see all successful probes reliably, you tell
> driver core to print it.
> 
> > > There are plenty of options for debugging already ranging from adding a
> > > temporary dev_info() to the probe function in question to using dynamic
> > > debugging to have driver core log every successful probe.
> > 
> > This is what I ended up doing.  It was time consuming to parse though
> > a log of that size when you have no paging or keyboard.
> 
> With the right command-line option to enable dynamic debugging you get
> one line per successful probe, just like you wanted. Or are you now
> saying that one-line per device is too much after all? ;)

Which command line option are you pertaining to?

> > > And in this case you say the driver was in fact already bound; that can
> > > easily be verified through sysfs too in case things aren't behaving the
> > > way you expect.
> > 
> > Not in a non-booting system with no keyboard you can't. ;)
> 
> Fair enough, but the above would still work.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Johan Hovold]

On Wed, Jun 05, 2019 at 09:49:21AM +0100, Lee Jones wrote:
> On Wed, 05 Jun 2019, Johan Hovold wrote:
> 
> > On Wed, Jun 05, 2019 at 09:20:47AM +0100, Lee Jones wrote:
> > > On Wed, 05 Jun 2019, Johan Hovold wrote:
 
> > > > There are plenty of options for debugging already ranging from adding a
> > > > temporary dev_info() to the probe function in question to using dynamic
> > > > debugging to have driver core log every successful probe.
> > > 
> > > This is what I ended up doing.  It was time consuming to parse though
> > > a log of that size when you have no paging or keyboard.
> > 
> > With the right command-line option to enable dynamic debugging you get
> > one line per successful probe, just like you wanted. Or are you now
> > saying that one-line per device is too much after all? ;)
> 
> Which command line option are you pertaining to?

To enable dynamic debugging in driver core you could use something like

	CONFIG_CMDLINE="dyndbg=\"func really_probe =p\""

That gives you two printouts per successful probe, for example:

	bus: 'usb-serial': really_probe: probing driver edgeport_ti_1 with device ttyUSB0
	bus: 'usb-serial': really_probe: bound device ttyUSB0 to driver edgeport_ti_1

Or you can of course just change the corresponding pr_debug to pr_info
while debugging.

Johan

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]


> To enable dynamic debugging in driver core you could use something like
> 
> 	CONFIG_CMDLINE="dyndbg=\"func really_probe =p\""
> 
> That gives you two printouts per successful probe, for example:
> 
> 	bus: 'usb-serial': really_probe: probing driver edgeport_ti_1 with device ttyUSB0
> 	bus: 'usb-serial': really_probe: bound device ttyUSB0 to driver edgeport_ti_1

I agree that this scales much better than adding strings to every
driver. Also, the driver core will report failed probes other than
-ENODEV, or?

Regarding this patch, however, I don't care much. I'll let the driver
maintainers decide.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

On Wed, 05 Jun 2019, Wolfram Sang wrote:

> 
> > To enable dynamic debugging in driver core you could use something like
> > 
> > 	CONFIG_CMDLINE="dyndbg=\"func really_probe =p\""
> > 
> > That gives you two printouts per successful probe, for example:
> > 
> > 	bus: 'usb-serial': really_probe: probing driver edgeport_ti_1 with device ttyUSB0
> > 	bus: 'usb-serial': really_probe: bound device ttyUSB0 to driver edgeport_ti_1
> 
> I agree that this scales much better than adding strings to every
> driver. Also, the driver core will report failed probes other than
> -ENODEV, or?
> 
> Regarding this patch, however, I don't care much. I'll let the driver
> maintainers decide.

I've downgraded this to dev_dbg() in v2.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Wed, Jun 05, 2019 at 04:18:12PM +0200, Wolfram Sang wrote:
> 
> > To enable dynamic debugging in driver core you could use something like
> > 
> > 	CONFIG_CMDLINE="dyndbg=\"func really_probe =p\""
> > 
> > That gives you two printouts per successful probe, for example:
> > 
> > 	bus: 'usb-serial': really_probe: probing driver edgeport_ti_1 with device ttyUSB0
> > 	bus: 'usb-serial': really_probe: bound device ttyUSB0 to driver edgeport_ti_1
> 
> I agree that this scales much better than adding strings to every
> driver. Also, the driver core will report failed probes other than
> -ENODEV, or?

Right, errors other than -EPROBE_DEFER, -ENODEV and -ENXIO are always
logged, and the previous three would also be logged with debugging
enabled.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 3/8] pinctrl: msm: Add ability for drivers to supply a reserved GPIO list

[Lee Jones]

When booting MSM based platforms with Device Tree or some ACPI
implementations, it is possible to provide a list of reserved pins
via the 'gpio-reserved-ranges' and 'gpios' properties respectively.
However some ACPI tables are not populated with this information,
thus it has to come from a knowledgable device driver instead.

Here we provide the MSM common driver with additional support to
parse this informtion and correctly populate the widely used
'valid_mask'.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 18 ++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ee8119879c4c..b77f22348907 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -607,8 +607,23 @@ static int msm_gpio_init_valid_mask(struct gpio_chip *chip)
 	int ret;
 	unsigned int len, i;
 	unsigned int max_gpios = pctrl->soc->ngpios;
+	const int *reserved = pctrl->soc->reserved_gpios;
 	u16 *tmp;
 
+	/* Driver provided reserved list overrides DT and ACPI */
+	if (reserved) {
+		bitmap_fill(chip->valid_mask, max_gpios);
+		for (i = 0; i < max_gpios && reserved[i] >= 0; i++) {
+			if (i >= max_gpios || reserved[i] >= max_gpios) {
+				dev_err(pctrl->dev, "invalid list of reserved GPIOs\n");
+				return -EINVAL;
+			}
+			clear_bit(reserved[i], chip->valid_mask);
+		}
+
+		return 0;
+	}
+
 	/* The number of GPIOs in the ACPI tables */
 	len = ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL,
 						   0);
@@ -964,6 +979,9 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 
 static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 {
+	if (pctrl->soc->reserved_gpios)
+		return true;
+
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index c12048e54a6f..23b93ae92269 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -121,6 +121,7 @@ struct msm_pinctrl_soc_data {
 	bool pull_no_keeper;
 	const char *const *tiles;
 	unsigned int ntiles;
+	const int *reserved_gpios;
 };
 
 extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
-- 
2.17.1

[PATCH 4/8] pinctrl: qcom: sdm845: Provide ACPI support

[Lee Jones]

This patch provides basic support for booting with ACPI instead
of the currently supported Device Tree.  When doing so there are a
couple of differences which we need to taken into consideration.

Firstly, the SDM850 ACPI tables omit information pertaining to the
4 reserved GPIOs on the platform.  If Linux attempts to touch/
initialise any of these lines, the firmware will restart the
platform.

Secondly, when booting with ACPI, it is expected that the firmware
will set-up things like; Regulators, Clocks, Pin Functions, etc in
their ideal configuration.  Thus, the possible Pin Functions
available to this platform are not advertised when providing the
higher GPIOD/Pinctrl APIs with pin information.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pinctrl/qcom/Kconfig          |  2 +-
 drivers/pinctrl/qcom/pinctrl-sdm845.c | 35 ++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
index 2e66ab72c10b..aafbe932424f 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -168,7 +168,7 @@ config PINCTRL_SDM660
 
 config PINCTRL_SDM845
        tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
-       depends on GPIOLIB && OF
+       depends on GPIOLIB && (OF || ACPI)
        select PINCTRL_MSM
        help
          This is the pinctrl, pinmux, pinconf and gpiolib driver for the
diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index c97f20fca5fd..7188bee3cf3e 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -1277,6 +1278,10 @@ static const struct msm_pingroup sdm845_groups[] = {
 	UFS_RESET(ufs_reset, 0x99f000),
 };
 
+static const int sdm845_acpi_reserved_gpios[] = {
+	0, 1, 2, 3, 81, 82, 83, 84, -1
+};
+
 static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.pins = sdm845_pins,
 	.npins = ARRAY_SIZE(sdm845_pins),
@@ -1284,14 +1289,41 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.nfunctions = ARRAY_SIZE(sdm845_functions),
 	.groups = sdm845_groups,
 	.ngroups = ARRAY_SIZE(sdm845_groups),
+	.reserved_gpios = sdm845_acpi_reserved_gpios,
+	.ngpios = 150,
+};
+
+static const struct msm_pinctrl_soc_data sdm845_acpi_pinctrl = {
+	.pins = sdm845_pins,
+	.npins = ARRAY_SIZE(sdm845_pins),
+	.groups = sdm845_groups,
+	.ngroups = ARRAY_SIZE(sdm845_groups),
+	.reserved_gpios = sdm845_acpi_reserved_gpios,
 	.ngpios = 150,
 };
 
 static int sdm845_pinctrl_probe(struct platform_device *pdev)
 {
-	return msm_pinctrl_probe(pdev, &sdm845_pinctrl);
+	int ret;
+
+	if (pdev->dev.of_node) {
+		ret = msm_pinctrl_probe(pdev, &sdm845_pinctrl);
+	} else if (ACPI_HANDLE(&pdev->dev)) {
+		ret = msm_pinctrl_probe(pdev, &sdm845_acpi_pinctrl);
+	} else {
+		dev_err(&pdev->dev, "DT and ACPI disabled\n");
+		return -EINVAL;
+	}
+
+	return ret;
 }
 
+static const struct acpi_device_id sdm845_pinctrl_acpi_match[] = {
+	{ "QCOM0217"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, sdm845_pinctrl_acpi_match);
+
 static const struct of_device_id sdm845_pinctrl_of_match[] = {
 	{ .compatible = "qcom,sdm845-pinctrl", },
 	{ },
@@ -1302,6 +1334,7 @@ static struct platform_driver sdm845_pinctrl_driver = {
 		.name = "sdm845-pinctrl",
 		.pm = &msm_pinctrl_dev_pm_ops,
 		.of_match_table = sdm845_pinctrl_of_match,
+		.acpi_match_table = ACPI_PTR(sdm845_pinctrl_acpi_match),
 	},
 	.probe = sdm845_pinctrl_probe,
 	.remove = msm_pinctrl_remove,
-- 
2.17.1

Re: [PATCH 4/8] pinctrl: qcom: sdm845: Provide ACPI support

[Bjorn Andersson]

On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:

> This patch provides basic support for booting with ACPI instead
> of the currently supported Device Tree.  When doing so there are a
> couple of differences which we need to taken into consideration.
> 
> Firstly, the SDM850 ACPI tables omit information pertaining to the
> 4 reserved GPIOs on the platform.  If Linux attempts to touch/
> initialise any of these lines, the firmware will restart the
> platform.
> 
> Secondly, when booting with ACPI, it is expected that the firmware
> will set-up things like; Regulators, Clocks, Pin Functions, etc in
> their ideal configuration.  Thus, the possible Pin Functions
> available to this platform are not advertised when providing the
> higher GPIOD/Pinctrl APIs with pin information.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pinctrl/qcom/Kconfig          |  2 +-
>  drivers/pinctrl/qcom/pinctrl-sdm845.c | 35 ++++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> index 2e66ab72c10b..aafbe932424f 100644
> --- a/drivers/pinctrl/qcom/Kconfig
> +++ b/drivers/pinctrl/qcom/Kconfig
> @@ -168,7 +168,7 @@ config PINCTRL_SDM660
>  
>  config PINCTRL_SDM845
>         tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
> -       depends on GPIOLIB && OF
> +       depends on GPIOLIB && (OF || ACPI)
>         select PINCTRL_MSM
>         help
>           This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> index c97f20fca5fd..7188bee3cf3e 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -1277,6 +1278,10 @@ static const struct msm_pingroup sdm845_groups[] = {
>  	UFS_RESET(ufs_reset, 0x99f000),
>  };
>  
> +static const int sdm845_acpi_reserved_gpios[] = {
> +	0, 1, 2, 3, 81, 82, 83, 84, -1
> +};
> +
>  static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
>  	.pins = sdm845_pins,
>  	.npins = ARRAY_SIZE(sdm845_pins),
> @@ -1284,14 +1289,41 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
>  	.nfunctions = ARRAY_SIZE(sdm845_functions),
>  	.groups = sdm845_groups,
>  	.ngroups = ARRAY_SIZE(sdm845_groups),
> +	.reserved_gpios = sdm845_acpi_reserved_gpios,

The reason why put these in DT is because the list is board/firmware
dependent. E.g. the firmware on db845c does not support the peripherals
that sits on these 8 pins and as such these are not reserved.

But given that the two structs looks identical now, did you perhaps not
intend to add.reserved_gpios for the non-ACPI case?

Regards,
Bjorn

> +	.ngpios = 150,
> +};
> +
> +static const struct msm_pinctrl_soc_data sdm845_acpi_pinctrl = {
> +	.pins = sdm845_pins,
> +	.npins = ARRAY_SIZE(sdm845_pins),
> +	.groups = sdm845_groups,
> +	.ngroups = ARRAY_SIZE(sdm845_groups),
> +	.reserved_gpios = sdm845_acpi_reserved_gpios,
>  	.ngpios = 150,
>  };
>  
>  static int sdm845_pinctrl_probe(struct platform_device *pdev)
>  {
> -	return msm_pinctrl_probe(pdev, &sdm845_pinctrl);
> +	int ret;
> +
> +	if (pdev->dev.of_node) {
> +		ret = msm_pinctrl_probe(pdev, &sdm845_pinctrl);
> +	} else if (ACPI_HANDLE(&pdev->dev)) {
> +		ret = msm_pinctrl_probe(pdev, &sdm845_acpi_pinctrl);
> +	} else {
> +		dev_err(&pdev->dev, "DT and ACPI disabled\n");
> +		return -EINVAL;
> +	}
> +
> +	return ret;
>  }
>  
> +static const struct acpi_device_id sdm845_pinctrl_acpi_match[] = {
> +	{ "QCOM0217"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, sdm845_pinctrl_acpi_match);
> +
>  static const struct of_device_id sdm845_pinctrl_of_match[] = {
>  	{ .compatible = "qcom,sdm845-pinctrl", },
>  	{ },
> @@ -1302,6 +1334,7 @@ static struct platform_driver sdm845_pinctrl_driver = {
>  		.name = "sdm845-pinctrl",
>  		.pm = &msm_pinctrl_dev_pm_ops,
>  		.of_match_table = sdm845_pinctrl_of_match,
> +		.acpi_match_table = ACPI_PTR(sdm845_pinctrl_acpi_match),
>  	},
>  	.probe = sdm845_pinctrl_probe,
>  	.remove = msm_pinctrl_remove,
> -- 
> 2.17.1
> 

Re: [PATCH 4/8] pinctrl: qcom: sdm845: Provide ACPI support

[Lee Jones]

On Tue, 04 Jun 2019, Bjorn Andersson wrote:

> On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> 
> > This patch provides basic support for booting with ACPI instead
> > of the currently supported Device Tree.  When doing so there are a
> > couple of differences which we need to taken into consideration.
> > 
> > Firstly, the SDM850 ACPI tables omit information pertaining to the
> > 4 reserved GPIOs on the platform.  If Linux attempts to touch/
> > initialise any of these lines, the firmware will restart the
> > platform.
> > 
> > Secondly, when booting with ACPI, it is expected that the firmware
> > will set-up things like; Regulators, Clocks, Pin Functions, etc in
> > their ideal configuration.  Thus, the possible Pin Functions
> > available to this platform are not advertised when providing the
> > higher GPIOD/Pinctrl APIs with pin information.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pinctrl/qcom/Kconfig          |  2 +-
> >  drivers/pinctrl/qcom/pinctrl-sdm845.c | 35 ++++++++++++++++++++++++++-
> >  2 files changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> > index 2e66ab72c10b..aafbe932424f 100644
> > --- a/drivers/pinctrl/qcom/Kconfig
> > +++ b/drivers/pinctrl/qcom/Kconfig
> > @@ -168,7 +168,7 @@ config PINCTRL_SDM660
> >  
> >  config PINCTRL_SDM845
> >         tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
> > -       depends on GPIOLIB && OF
> > +       depends on GPIOLIB && (OF || ACPI)
> >         select PINCTRL_MSM
> >         help
> >           This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > index c97f20fca5fd..7188bee3cf3e 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > @@ -1277,6 +1278,10 @@ static const struct msm_pingroup sdm845_groups[] = {
> >  	UFS_RESET(ufs_reset, 0x99f000),
> >  };
> >  
> > +static const int sdm845_acpi_reserved_gpios[] = {
> > +	0, 1, 2, 3, 81, 82, 83, 84, -1
> > +};
> > +
> >  static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
> >  	.pins = sdm845_pins,
> >  	.npins = ARRAY_SIZE(sdm845_pins),
> > @@ -1284,14 +1289,41 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
> >  	.nfunctions = ARRAY_SIZE(sdm845_functions),
> >  	.groups = sdm845_groups,
> >  	.ngroups = ARRAY_SIZE(sdm845_groups),
> > +	.reserved_gpios = sdm845_acpi_reserved_gpios,
> 
> The reason why put these in DT is because the list is board/firmware
> dependent. E.g. the firmware on db845c does not support the peripherals
> that sits on these 8 pins and as such these are not reserved.

If we need to be more particular about which platform(s) this affects,
we could add matching based on their differences (some ACPI HID or F/W
version/descriptor, etc) as and when we enable them for booting with
ACPI.

> But given that the two structs looks identical now, did you perhaps not
> intend to add.reserved_gpios for the non-ACPI case?

Given your example above, I think it's best that we let the
configuration tables advertise these in the first instance.  I only
add them here because it is not possible to obtain them from
elsewhere.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/8] pinctrl: qcom: sdm845: Provide ACPI support

[Bjorn Andersson]

On Wed 05 Jun 00:31 PDT 2019, Lee Jones wrote:

> On Tue, 04 Jun 2019, Bjorn Andersson wrote:
> 
> > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > 
> > > This patch provides basic support for booting with ACPI instead
> > > of the currently supported Device Tree.  When doing so there are a
> > > couple of differences which we need to taken into consideration.
> > > 
> > > Firstly, the SDM850 ACPI tables omit information pertaining to the
> > > 4 reserved GPIOs on the platform.  If Linux attempts to touch/
> > > initialise any of these lines, the firmware will restart the
> > > platform.
> > > 
> > > Secondly, when booting with ACPI, it is expected that the firmware
> > > will set-up things like; Regulators, Clocks, Pin Functions, etc in
> > > their ideal configuration.  Thus, the possible Pin Functions
> > > available to this platform are not advertised when providing the
> > > higher GPIOD/Pinctrl APIs with pin information.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/pinctrl/qcom/Kconfig          |  2 +-
> > >  drivers/pinctrl/qcom/pinctrl-sdm845.c | 35 ++++++++++++++++++++++++++-
> > >  2 files changed, 35 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> > > index 2e66ab72c10b..aafbe932424f 100644
> > > --- a/drivers/pinctrl/qcom/Kconfig
> > > +++ b/drivers/pinctrl/qcom/Kconfig
> > > @@ -168,7 +168,7 @@ config PINCTRL_SDM660
> > >  
> > >  config PINCTRL_SDM845
> > >         tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
> > > -       depends on GPIOLIB && OF
> > > +       depends on GPIOLIB && (OF || ACPI)
> > >         select PINCTRL_MSM
> > >         help
> > >           This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > > diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > > index c97f20fca5fd..7188bee3cf3e 100644
> > > --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > > +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > > @@ -3,6 +3,7 @@
> > >   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > >   */
> > >  
> > > +#include <linux/acpi.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > > @@ -1277,6 +1278,10 @@ static const struct msm_pingroup sdm845_groups[] = {
> > >  	UFS_RESET(ufs_reset, 0x99f000),
> > >  };
> > >  
> > > +static const int sdm845_acpi_reserved_gpios[] = {
> > > +	0, 1, 2, 3, 81, 82, 83, 84, -1
> > > +};
> > > +
> > >  static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
> > >  	.pins = sdm845_pins,
> > >  	.npins = ARRAY_SIZE(sdm845_pins),
> > > @@ -1284,14 +1289,41 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
> > >  	.nfunctions = ARRAY_SIZE(sdm845_functions),
> > >  	.groups = sdm845_groups,
> > >  	.ngroups = ARRAY_SIZE(sdm845_groups),
> > > +	.reserved_gpios = sdm845_acpi_reserved_gpios,
> > 
> > The reason why put these in DT is because the list is board/firmware
> > dependent. E.g. the firmware on db845c does not support the peripherals
> > that sits on these 8 pins and as such these are not reserved.
> 
> If we need to be more particular about which platform(s) this affects,
> we could add matching based on their differences (some ACPI HID or F/W
> version/descriptor, etc) as and when we enable them for booting with
> ACPI.
> 

You're making an assumption that all SDM845 (the platform) devices using
ACPI will have this list of GPIOs reserved for secure firmware to use,
this is questionable but I don't have any better suggestion.

But you do this by adding a new struct msm_pinctrl_soc_data
sdm845_acpi_pinctrl, specifically for the ACPI case. And then, on the
line I object to here, you add this list as the list of reserved GPIOs
for the DT case as well.

> > But given that the two structs looks identical now, did you perhaps not
> > intend to add.reserved_gpios for the non-ACPI case?
> 
> Given your example above, I think it's best that we let the
> configuration tables advertise these in the first instance.  I only
> add them here because it is not possible to obtain them from
> elsewhere.
> 

Then add it for ACPI only - which I still presume you intended to do by
adding sdm845_acpi_pinctrl (which is now identical to sdm845_pinctrl).

Regards,
Bjorn

Re: [PATCH 4/8] pinctrl: qcom: sdm845: Provide ACPI support

[Lee Jones]

On Wed, 05 Jun 2019, Bjorn Andersson wrote:

> On Wed 05 Jun 00:31 PDT 2019, Lee Jones wrote:
> 
> > On Tue, 04 Jun 2019, Bjorn Andersson wrote:
> > 
> > > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > > 
> > > > This patch provides basic support for booting with ACPI instead
> > > > of the currently supported Device Tree.  When doing so there are a
> > > > couple of differences which we need to taken into consideration.
> > > > 
> > > > Firstly, the SDM850 ACPI tables omit information pertaining to the
> > > > 4 reserved GPIOs on the platform.  If Linux attempts to touch/
> > > > initialise any of these lines, the firmware will restart the
> > > > platform.
> > > > 
> > > > Secondly, when booting with ACPI, it is expected that the firmware
> > > > will set-up things like; Regulators, Clocks, Pin Functions, etc in
> > > > their ideal configuration.  Thus, the possible Pin Functions
> > > > available to this platform are not advertised when providing the
> > > > higher GPIOD/Pinctrl APIs with pin information.
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/pinctrl/qcom/Kconfig          |  2 +-
> > > >  drivers/pinctrl/qcom/pinctrl-sdm845.c | 35 ++++++++++++++++++++++++++-
> > > >  2 files changed, 35 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> > > > index 2e66ab72c10b..aafbe932424f 100644
> > > > --- a/drivers/pinctrl/qcom/Kconfig
> > > > +++ b/drivers/pinctrl/qcom/Kconfig
> > > > @@ -168,7 +168,7 @@ config PINCTRL_SDM660
> > > >  
> > > >  config PINCTRL_SDM845
> > > >         tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
> > > > -       depends on GPIOLIB && OF
> > > > +       depends on GPIOLIB && (OF || ACPI)
> > > >         select PINCTRL_MSM
> > > >         help
> > > >           This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > > > diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > > > index c97f20fca5fd..7188bee3cf3e 100644
> > > > --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > > > +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> > > > @@ -3,6 +3,7 @@
> > > >   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > > >   */
> > > >  
> > > > +#include <linux/acpi.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > > @@ -1277,6 +1278,10 @@ static const struct msm_pingroup sdm845_groups[] = {
> > > >  	UFS_RESET(ufs_reset, 0x99f000),
> > > >  };
> > > >  
> > > > +static const int sdm845_acpi_reserved_gpios[] = {
> > > > +	0, 1, 2, 3, 81, 82, 83, 84, -1
> > > > +};
> > > > +
> > > >  static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
> > > >  	.pins = sdm845_pins,
> > > >  	.npins = ARRAY_SIZE(sdm845_pins),
> > > > @@ -1284,14 +1289,41 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
> > > >  	.nfunctions = ARRAY_SIZE(sdm845_functions),
> > > >  	.groups = sdm845_groups,
> > > >  	.ngroups = ARRAY_SIZE(sdm845_groups),
> > > > +	.reserved_gpios = sdm845_acpi_reserved_gpios,
> > > 
> > > The reason why put these in DT is because the list is board/firmware
> > > dependent. E.g. the firmware on db845c does not support the peripherals
> > > that sits on these 8 pins and as such these are not reserved.
> > 
> > If we need to be more particular about which platform(s) this affects,
> > we could add matching based on their differences (some ACPI HID or F/W
> > version/descriptor, etc) as and when we enable them for booting with
> > ACPI.
> > 
> 
> You're making an assumption that all SDM845 (the platform) devices using
> ACPI will have this list of GPIOs reserved for secure firmware to use,
> this is questionable but I don't have any better suggestion.

Yes, I am, since this is the first and currently only device which
ticks those boxes.  If/when there are others AND if they require a
different configuration, we can look at the differences and conduct
some suitable matching on them at the time.

> But you do this by adding a new struct msm_pinctrl_soc_data
> sdm845_acpi_pinctrl, specifically for the ACPI case. And then, on the
> line I object to here, you add this list as the list of reserved GPIOs
> for the DT case as well.

Ohhhh, now I see what you're getting at.  Yes, that's a mistake left
over from testing.  That needs removing -- good spot.

> > > But given that the two structs looks identical now, did you perhaps not
> > > intend to add.reserved_gpios for the non-ACPI case?
> > 
> > Given your example above, I think it's best that we let the
> > configuration tables advertise these in the first instance.  I only
> > add them here because it is not possible to obtain them from
> > elsewhere.
> > 
> 
> Then add it for ACPI only - which I still presume you intended to do by
> adding sdm845_acpi_pinctrl (which is now identical to sdm845_pinctrl).

We're arguing about the same thing - sorry for the confusion.

I will fix this.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/8] soc: qcom: geni: Add support for ACPI

[Lee Jones]

When booting with ACPI as the active set of configuration tables,
all; clocks, regulators, pin functions ect are expected to be at
their ideal values/levels/rates, thus the associated frameworks
are unavailable.  Ensure calls to these APIs are shielded when
ACPI is enabled.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/soc/qcom/qcom-geni-se.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 6b8ef01472e9..cff0a413e59a 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
@@ -450,6 +451,9 @@ int geni_se_resources_off(struct geni_se *se)
 {
 	int ret;
 
+	if (ACPI_HANDLE(se->dev))
+		return 0;
+
 	ret = pinctrl_pm_select_sleep_state(se->dev);
 	if (ret)
 		return ret;
@@ -487,6 +491,9 @@ int geni_se_resources_on(struct geni_se *se)
 {
 	int ret;
 
+	if (ACPI_HANDLE(se->dev))
+		return 0;
+
 	ret = geni_se_clks_on(se);
 	if (ret)
 		return ret;
@@ -724,12 +731,14 @@ static int geni_se_probe(struct platform_device *pdev)
 	if (IS_ERR(wrapper->base))
 		return PTR_ERR(wrapper->base);
 
-	wrapper->ahb_clks[0].id = "m-ahb";
-	wrapper->ahb_clks[1].id = "s-ahb";
-	ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
-	if (ret) {
-		dev_err(dev, "Err getting AHB clks %d\n", ret);
-		return ret;
+	if (!ACPI_HANDLE(&pdev->dev)) {
+		wrapper->ahb_clks[0].id = "m-ahb";
+		wrapper->ahb_clks[1].id = "s-ahb";
+		ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
+		if (ret) {
+			dev_err(dev, "Err getting AHB clks %d\n", ret);
+			return ret;
+		}
 	}
 
 	dev_set_drvdata(dev, wrapper);
-- 
2.17.1

[PATCH 6/8] usb: dwc3: qcom: Add support for booting with ACPI

[Lee Jones]

In Linux, the DWC3 core exists as its own independent platform device.
Thus when describing relationships in Device Tree, the current default
boot configuration table option, the DWC3 core often resides as a child
of the platform specific node.  Both of which are given their own
address space descriptions and the drivers can be mostly agnostic to
each other.

However, other Operating Systems have taken a more monolithic approach,
which is evident in the configuration ACPI tables for the Qualcomm
Snapdragon SDM850, where all DWC3 (core and platform) components are
described under a single IO memory region.

To ensure successful booting using the supplied ACPI tables, we need to
devise a way to chop up the address regions provided and subsequently
register the DWC3 core with the resultant information, which is
precisely what this patch aims to achieve.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/usb/dwc3/Kconfig     |   2 +-
 drivers/usb/dwc3/dwc3-qcom.c | 194 ++++++++++++++++++++++++++++++-----
 2 files changed, 170 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 2b1494460d0c..6dab3fd1e233 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -116,7 +116,7 @@ config USB_DWC3_ST
 config USB_DWC3_QCOM
 	tristate "Qualcomm Platform"
 	depends on EXTCON && (ARCH_QCOM || COMPILE_TEST)
-	depends on OF
+	depends on (OF || ACPI)
 	default USB_DWC3
 	help
 	  Some Qualcomm SoCs use DesignWare Core IP for USB2/3
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 184df4daa590..349bf549ee44 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -4,6 +4,7 @@
  * Inspired by dwc3-of-simple.c
  */
 
+#include <linux/acpi.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/clk.h>
@@ -38,6 +39,20 @@
 #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
 #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
 
+#define SDM845_QSCRATCH_BASE_OFFSET		0xf8800
+#define SDM845_QSCRATCH_SIZE			0x400
+#define SDM845_DWC3_CORE_SIZE			0xcd00
+
+struct dwc3_acpi_pdata {
+	u32			qscratch_base_offset;
+	u32			qscratch_base_size;
+	u32			dwc3_core_base_size;
+	int			hs_phy_irq_index;
+	int			dp_hs_phy_irq_index;
+	int			dm_hs_phy_irq_index;
+	int			ss_phy_irq_index;
+};
+
 struct dwc3_qcom {
 	struct device		*dev;
 	void __iomem		*qscratch_base;
@@ -56,6 +71,8 @@ struct dwc3_qcom {
 	struct notifier_block	vbus_nb;
 	struct notifier_block	host_nb;
 
+	struct dwc3_acpi_pdata	*acpi_pdata;
+
 	enum usb_dr_mode	mode;
 	bool			is_suspended;
 	bool			pm_suspended;
@@ -300,12 +317,32 @@ static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
 			  PIPE_UTMI_CLK_DIS);
 }
 
+static int dwc3_qcom_get_irq(struct platform_device *pdev,
+			     const char *name, int num)
+{
+	if (pdev->dev.of_node) {
+		return platform_get_irq_byname(pdev, name);
+	} else if (ACPI_HANDLE(&pdev->dev)) {
+		return platform_get_irq(pdev, num);
+	} else
+		dev_err(&pdev->dev, "Neither ACPI nor DT enabled\n");
+
+	return -EINVAL;
+}
+
 static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 {
 	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
 	int irq, ret;
 
-	irq = platform_get_irq_byname(pdev, "hs_phy_irq");
+	if (ACPI_HANDLE(&pdev->dev) && !qcom->acpi_pdata) {
+		dev_err(&pdev->dev, "No ACPI platform data supplied\n");
+		return -EINVAL;
+	}
+
+	irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
+				qcom->acpi_pdata ?
+				qcom->acpi_pdata->hs_phy_irq_index : -1);
 	if (irq > 0) {
 		/* Keep wakeup interrupts disabled until suspend */
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
@@ -320,7 +357,9 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		qcom->hs_phy_irq = irq;
 	}
 
-	irq = platform_get_irq_byname(pdev, "dp_hs_phy_irq");
+	irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
+				qcom->acpi_pdata ?
+				qcom->acpi_pdata->dp_hs_phy_irq_index : -1);
 	if (irq > 0) {
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
@@ -334,7 +373,9 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		qcom->dp_hs_phy_irq = irq;
 	}
 
-	irq = platform_get_irq_byname(pdev, "dm_hs_phy_irq");
+	irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
+				qcom->acpi_pdata ?
+				qcom->acpi_pdata->dm_hs_phy_irq_index : -1);
 	if (irq > 0) {
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
@@ -348,7 +389,9 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
 		qcom->dm_hs_phy_irq = irq;
 	}
 
-	irq = platform_get_irq_byname(pdev, "ss_phy_irq");
+	irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
+				qcom->acpi_pdata ?
+				qcom->acpi_pdata->ss_phy_irq_index : -1);
 	if (irq > 0) {
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
@@ -373,7 +416,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
 
 	qcom->num_clocks = count;
 
-	if (!count)
+	if (!count || ACPI_HANDLE(dev))
 		return 0;
 
 	qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
@@ -409,12 +452,28 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
 	return 0;
 }
 
+static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
+	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
+	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
+	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
+	.hs_phy_irq_index = 1,
+	.dp_hs_phy_irq_index = 4,
+	.dm_hs_phy_irq_index = 3,
+	.ss_phy_irq_index = 2
+};
+
+static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
+	{ "QCOM2430", (unsigned long)&sdm845_acpi_pdata },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
+
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
 	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
 	struct device		*dev = &pdev->dev;
 	struct dwc3_qcom	*qcom;
-	struct resource		*res;
+	struct resource		*res, *parent_res = NULL, *child_res = NULL;
 	int			ret, i;
 	bool			ignore_pipe_clk;
 
@@ -425,6 +484,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, qcom);
 	qcom->dev = &pdev->dev;
 
+	if (ACPI_HANDLE(dev)) {
+		const struct acpi_device_id *match;
+
+		match = acpi_match_device(dwc3_qcom_acpi_match, dev);
+		if (!match || !match->driver_data) {
+			dev_err(&pdev->dev, "no supporting ACPI device data\n");
+			return -EINVAL;
+		}
+		qcom->acpi_pdata = (struct dwc3_acpi_pdata *)match->driver_data;
+	}
+
 	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
 	if (IS_ERR(qcom->resets)) {
 		ret = PTR_ERR(qcom->resets);
@@ -454,7 +524,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	qcom->qscratch_base = devm_ioremap_resource(dev, res);
+
+	if (ACPI_HANDLE(dev)) {
+		parent_res = kmemdup(res, sizeof(struct resource), GFP_KERNEL);
+		if (!parent_res)
+			return -ENOMEM;
+
+		parent_res->start = res->start +
+			qcom->acpi_pdata->qscratch_base_offset;
+		parent_res->end = parent_res->start +
+			qcom->acpi_pdata->qscratch_base_size;
+	} else {
+		parent_res = res;
+	}
+
+	qcom->qscratch_base = devm_ioremap_resource(dev, parent_res);
 	if (IS_ERR(qcom->qscratch_base)) {
 		dev_err(dev, "failed to map qscratch, err=%d\n", ret);
 		ret = PTR_ERR(qcom->qscratch_base);
@@ -462,13 +546,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	}
 
 	ret = dwc3_qcom_setup_irq(pdev);
-	if (ret)
-		goto clk_disable;
-
-	dwc3_np = of_get_child_by_name(np, "dwc3");
-	if (!dwc3_np) {
-		dev_err(dev, "failed to find dwc3 core child\n");
-		ret = -ENODEV;
+	if (ret) {
+		dev_err(dev, "failed to setup IRQs, err=%d\n", ret);
 		goto clk_disable;
 	}
 
@@ -481,17 +560,74 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ignore_pipe_clk)
 		dwc3_qcom_select_utmi_clk(qcom);
 
-	ret = of_platform_populate(np, NULL, NULL, dev);
-	if (ret) {
-		dev_err(dev, "failed to register dwc3 core - %d\n", ret);
-		goto clk_disable;
-	}
+	if (ACPI_HANDLE(dev)) {
+		int irq;
 
-	qcom->dwc3 = of_find_device_by_node(dwc3_np);
-	if (!qcom->dwc3) {
-		dev_err(&pdev->dev, "failed to get dwc3 platform device\n");
-		ret = -ENODEV;
-		goto depopulate;
+		qcom->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO);
+		if (!qcom->dwc3) {
+			ret = -ENOMEM;
+			goto clk_disable;
+		}
+
+		qcom->dwc3->dev.parent = dev;
+		qcom->dwc3->dev.type = dev->type;
+		qcom->dwc3->dev.dma_mask = dev->dma_mask;
+		qcom->dwc3->dev.dma_parms = dev->dma_parms;
+		qcom->dwc3->dev.coherent_dma_mask = dev->coherent_dma_mask;
+
+		child_res = kcalloc(2, sizeof(*child_res), GFP_KERNEL);
+		if (!child_res) {
+			ret = -ENOMEM;
+			goto platform_unalloc;
+		}
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res) {
+			dev_err(&pdev->dev, "failed to get memory resource\n");
+			ret = -ENODEV;
+			goto platform_unalloc;
+		}
+
+		child_res[0].flags = res->flags;
+		child_res[0].start = res->start;
+		child_res[0].end = child_res[0].start +
+			qcom->acpi_pdata->dwc3_core_base_size;
+
+		irq = platform_get_irq(pdev, 0);
+		child_res[1].flags = IORESOURCE_IRQ;
+		child_res[1].start = child_res[1].end = irq;
+
+		ret = platform_device_add_resources(qcom->dwc3, child_res, 2);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to add resources\n");
+			goto platform_unalloc;
+		}
+
+		ret = platform_device_add(qcom->dwc3);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to add device\n");
+			goto platform_unalloc;
+		}
+	} else {
+		dwc3_np = of_get_child_by_name(np, "dwc3");
+		if (!dwc3_np) {
+			dev_err(dev, "failed to find dwc3 core child\n");
+			ret = -ENODEV;
+			goto clk_disable;
+		}
+
+		ret = of_platform_populate(np, NULL, NULL, dev);
+		if (ret) {
+			dev_err(dev, "failed to register dwc3 core - %d\n", ret);
+			goto clk_disable;
+		}
+
+		qcom->dwc3 = of_find_device_by_node(dwc3_np);
+		if (!qcom->dwc3) {
+			dev_err(&pdev->dev, "failed to get dwc3 platform device\n");
+			ret = -ENODEV;
+			goto depopulate;
+		}
 	}
 
 	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
@@ -514,7 +650,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	return 0;
 
 depopulate:
-	of_platform_depopulate(&pdev->dev);
+platform_unalloc:
+	if (child_res)
+		kfree(child_res);
+
+	if (np)
+		of_platform_depopulate(&pdev->dev);
+	else
+		platform_device_put(pdev);
 clk_disable:
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {
 		clk_disable_unprepare(qcom->clks[i]);
@@ -608,6 +751,7 @@ static struct platform_driver dwc3_qcom_driver = {
 		.name	= "dwc3-qcom",
 		.pm	= &dwc3_qcom_dev_pm_ops,
 		.of_match_table	= dwc3_qcom_of_match,
+		.acpi_match_table = ACPI_PTR(dwc3_qcom_acpi_match),
 	},
 };
 
-- 
2.17.1

Re: [PATCH 6/8] usb: dwc3: qcom: Add support for booting with ACPI

[Bjorn Andersson]

On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
[..]
> @@ -373,7 +416,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
>  
>  	qcom->num_clocks = count;
>  
> -	if (!count)
> +	if (!count || ACPI_HANDLE(dev))
>  		return 0;

Afaict you call this with count = of_count_phandle_with_args(), which
should be 0. But why not skip calling this at all?

>  
>  	qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
> @@ -409,12 +452,28 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
>  	return 0;
>  }
>  
> +static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
> +	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
> +	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
> +	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
> +	.hs_phy_irq_index = 1,
> +	.dp_hs_phy_irq_index = 4,
> +	.dm_hs_phy_irq_index = 3,
> +	.ss_phy_irq_index = 2
> +};
> +
> +static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
> +	{ "QCOM2430", (unsigned long)&sdm845_acpi_pdata },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);

Analog to of_device_get_match_data() there seems to be a
acpi_device_get_match_data(), if you use this you should be able to
have you acpi_device_id array next to the of_device_id.

> +
>  static int dwc3_qcom_probe(struct platform_device *pdev)

It seems that all that's left unconditional on ACPI_HANDLE() in this
function are the optional pieces and the tail. Wouldn't it be cleaner to
split it out in different functions?

Regards,
Bjorn

Re: [PATCH 6/8] usb: dwc3: qcom: Add support for booting with ACPI

[Lee Jones]

On Tue, 04 Jun 2019, Bjorn Andersson wrote:

> On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> [..]
> > @@ -373,7 +416,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
> >  
> >  	qcom->num_clocks = count;
> >  
> > -	if (!count)
> > +	if (!count || ACPI_HANDLE(dev))
> >  		return 0;
> 
> Afaict you call this with count = of_count_phandle_with_args(), which
> should be 0. But why not skip calling this at all?

Actually count can be <0, which is why I must have needed it at the
beginning.  There is another patch in this set which checks for
errors, thus the ACPI_HANDLE() call should now be superfluous.  I
will test and remove it.

> >  	qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
> > @@ -409,12 +452,28 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
> >  	return 0;
> >  }
> >  
> > +static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
> > +	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
> > +	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
> > +	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
> > +	.hs_phy_irq_index = 1,
> > +	.dp_hs_phy_irq_index = 4,
> > +	.dm_hs_phy_irq_index = 3,
> > +	.ss_phy_irq_index = 2
> > +};
> > +
> > +static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
> > +	{ "QCOM2430", (unsigned long)&sdm845_acpi_pdata },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
> 
> Analog to of_device_get_match_data() there seems to be a
> acpi_device_get_match_data(), if you use this you should be able to
> have you acpi_device_id array next to the of_device_id.

Do you mean "Analogous"?

I will try to group them, thanks.

> > +
> >  static int dwc3_qcom_probe(struct platform_device *pdev)
> 
> It seems that all that's left unconditional on ACPI_HANDLE() in this
> function are the optional pieces and the tail. Wouldn't it be cleaner to
> split it out in different functions?

There are ~50 lines of shared code in dwc3_qcom_probe(), most of it is
interspersed between the configuration table (DT, ACPI) pieces, which
is why it's formatted in the current way.

I can split a few things out into separate functions if you think
it'll help.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 6/8] usb: dwc3: qcom: Add support for booting with ACPI

[Lee Jones]

On Wed, 05 Jun 2019, Lee Jones wrote:

> On Tue, 04 Jun 2019, Bjorn Andersson wrote:
> 
> > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > [..]
> > > @@ -373,7 +416,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
> > >  
> > >  	qcom->num_clocks = count;
> > >  
> > > -	if (!count)
> > > +	if (!count || ACPI_HANDLE(dev))
> > >  		return 0;
> > 
> > Afaict you call this with count = of_count_phandle_with_args(), which
> > should be 0. But why not skip calling this at all?
> 
> Actually count can be <0, which is why I must have needed it at the
> beginning.  There is another patch in this set which checks for
> errors, thus the ACPI_HANDLE() call should now be superfluous.  I
> will test and remove it.

Just looked into this - it is still required.

of_count_phandle_with_args() returns an error not to be heeded in the
ACPI case.  So the logic goes:

[This patch]
 * It's fine to boot DT with no clocks to initialise (return 0)
 * There are no clocks to enable when booting ACPI (return 0)

[Another patch]
 * It's not fine to boot DT and for 'count < 0' (return count)

> > >  	qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
> > > @@ -409,12 +452,28 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
> > >  	return 0;
> > >  }
> > >  
> > > +static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
> > > +	.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
> > > +	.qscratch_base_size = SDM845_QSCRATCH_SIZE,
> > > +	.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
> > > +	.hs_phy_irq_index = 1,
> > > +	.dp_hs_phy_irq_index = 4,
> > > +	.dm_hs_phy_irq_index = 3,
> > > +	.ss_phy_irq_index = 2
> > > +};
> > > +
> > > +static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
> > > +	{ "QCOM2430", (unsigned long)&sdm845_acpi_pdata },
> > > +	{ },
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
> > 
> > Analog to of_device_get_match_data() there seems to be a
> > acpi_device_get_match_data(), if you use this you should be able to
> > have you acpi_device_id array next to the of_device_id.
> 
> Do you mean "Analogous"?
> 
> I will try to group them, thanks.
> 
> > > +
> > >  static int dwc3_qcom_probe(struct platform_device *pdev)
> > 
> > It seems that all that's left unconditional on ACPI_HANDLE() in this
> > function are the optional pieces and the tail. Wouldn't it be cleaner to
> > split it out in different functions?
> 
> There are ~50 lines of shared code in dwc3_qcom_probe(), most of it is
> interspersed between the configuration table (DT, ACPI) pieces, which
> is why it's formatted in the current way.
> 
> I can split a few things out into separate functions if you think
> it'll help.
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Lee Jones]

When booting with Device Tree, the current default boot configuration
table option, the request to boot via 'host mode' comes from the
"dr_mode" property.  A property of the same name can be used inside
ACPI tables too.  However it is missing from the SDM845's ACPI tables
so we have to supply this information using Platform Device Properites
instead.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 349bf549ee44..f21fdd6cdd1a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -468,6 +468,11 @@ static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
 
+static const struct property_entry dwc3_qcom_acpi_properties[] = {
+	PROPERTY_ENTRY_STRING("dr_mode", "host"),
+	{}
+};
+
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
 	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
@@ -603,6 +608,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 			goto platform_unalloc;
 		}
 
+		ret = platform_device_add_properties(qcom->dwc3,
+						     dwc3_qcom_acpi_properties);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to add properties\n");
+			goto platform_unalloc;
+		}
+
 		ret = platform_device_add(qcom->dwc3);
 		if (ret) {
 			dev_err(&pdev->dev, "failed to add device\n");
-- 
2.17.1

Re: [PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Bjorn Andersson]

On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:

> When booting with Device Tree, the current default boot configuration
> table option, the request to boot via 'host mode' comes from the
> "dr_mode" property.

This has been the default on the MTP, but this is changing as this is
causing issues when connected downstream from a hub (the typical
development case for the primary USB port of a phone like device) and
more importantly we don't have support for the PMIC blocks that control
VBUS.

Once these issues are resolved the dr_mode would be "otg".

> A property of the same name can be used inside
> ACPI tables too.  However it is missing from the SDM845's ACPI tables
> so we have to supply this information using Platform Device Properites
> instead.
> 

Afaict this would install a fall-back property, so in the case that we
have specified dr_mode in DT (or ACPI) that would take precedence. So
the commit message should reflect that this redefines the default choice
to be "host", rather than "otg".

Which is in conflict with what's described for dr_mode in
Documentation/devicetree/bindings/usb/generic.txt


And this driver is used on a range of different Qualcomm platforms, so I
don't think this is SDM845 specific.

Regards,
Bjorn

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 349bf549ee44..f21fdd6cdd1a 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -468,6 +468,11 @@ static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
>  
> +static const struct property_entry dwc3_qcom_acpi_properties[] = {
> +	PROPERTY_ENTRY_STRING("dr_mode", "host"),
> +	{}
> +};
> +
>  static int dwc3_qcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
> @@ -603,6 +608,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  			goto platform_unalloc;
>  		}
>  
> +		ret = platform_device_add_properties(qcom->dwc3,
> +						     dwc3_qcom_acpi_properties);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to add properties\n");
> +			goto platform_unalloc;
> +		}
> +
>  		ret = platform_device_add(qcom->dwc3);
>  		if (ret) {
>  			dev_err(&pdev->dev, "failed to add device\n");
> -- 
> 2.17.1
> 

Re: [PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Lee Jones]

On Wed, 05 Jun 2019, Bjorn Andersson wrote:

> On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> 
> > When booting with Device Tree, the current default boot configuration
> > table option, the request to boot via 'host mode' comes from the
> > "dr_mode" property.
> 
> This has been the default on the MTP, but this is changing as this is
> causing issues when connected downstream from a hub (the typical
> development case for the primary USB port of a phone like device) and
> more importantly we don't have support for the PMIC blocks that control
> VBUS.

My point is not about which mode is currently chosen.  It's more about
the capability of choosing which mode is appropriate for a given
system via DT.

> Once these issues are resolved the dr_mode would be "otg".

OTG doesn't work on this H/W, so we need to specify "host" mode.

> > A property of the same name can be used inside
> > ACPI tables too.  However it is missing from the SDM845's ACPI tables
> > so we have to supply this information using Platform Device Properites
> > instead.
> > 
> 
> Afaict this would install a fall-back property, so in the case that we
> have specified dr_mode in DT (or ACPI) that would take precedence. So

That's correct.

> the commit message should reflect that this redefines the default choice
> to be "host", rather than "otg".

No problem.

> Which is in conflict with what's described for dr_mode in
> Documentation/devicetree/bindings/usb/generic.txt

This implementation only affects ACPI based platforms.  When booting
with DT, the description in that DT related document is still
accurate.

> And this driver is used on a range of different Qualcomm platforms, so I
> don't think this is SDM845 specific.

ACPI based platforms?

All the ones I've seen use the XHCI USB driver directly ("PNP0D10").
 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 349bf549ee44..f21fdd6cdd1a 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -468,6 +468,11 @@ static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
> >  
> > +static const struct property_entry dwc3_qcom_acpi_properties[] = {
> > +	PROPERTY_ENTRY_STRING("dr_mode", "host"),
> > +	{}
> > +};
> > +
> >  static int dwc3_qcom_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
> > @@ -603,6 +608,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  			goto platform_unalloc;
> >  		}
> >  
> > +		ret = platform_device_add_properties(qcom->dwc3,
> > +						     dwc3_qcom_acpi_properties);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "failed to add properties\n");
> > +			goto platform_unalloc;
> > +		}
> > +
> >  		ret = platform_device_add(qcom->dwc3);
> >  		if (ret) {
> >  			dev_err(&pdev->dev, "failed to add device\n");

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Jeffrey Hugo]

On 6/5/2019 2:34 AM, Lee Jones wrote:
> On Wed, 05 Jun 2019, Bjorn Andersson wrote:
> 
>> On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
>>
>>> When booting with Device Tree, the current default boot configuration
>>> table option, the request to boot via 'host mode' comes from the
>>> "dr_mode" property.
>>
>> This has been the default on the MTP, but this is changing as this is
>> causing issues when connected downstream from a hub (the typical
>> development case for the primary USB port of a phone like device) and
>> more importantly we don't have support for the PMIC blocks that control
>> VBUS.
> 
> My point is not about which mode is currently chosen.  It's more about
> the capability of choosing which mode is appropriate for a given
> system via DT.
> 
>> Once these issues are resolved the dr_mode would be "otg".
> 
> OTG doesn't work on this H/W, so we need to specify "host" mode.

How have you made that determination?

> 
>>> A property of the same name can be used inside
>>> ACPI tables too.  However it is missing from the SDM845's ACPI tables
>>> so we have to supply this information using Platform Device Properites
>>> instead.
>>>
>>
>> Afaict this would install a fall-back property, so in the case that we
>> have specified dr_mode in DT (or ACPI) that would take precedence. So
> 
> That's correct.
> 
>> the commit message should reflect that this redefines the default choice
>> to be "host", rather than "otg".
> 
> No problem. >
>> Which is in conflict with what's described for dr_mode in
>> Documentation/devicetree/bindings/usb/generic.txt
> 
> This implementation only affects ACPI based platforms.  When booting
> with DT, the description in that DT related document is still
> accurate.
> 
>> And this driver is used on a range of different Qualcomm platforms, so I
>> don't think this is SDM845 specific.
> 
> ACPI based platforms?
> 
> All the ones I've seen use the XHCI USB driver directly ("PNP0D10").
>   
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>   drivers/usb/dwc3/dwc3-qcom.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>>> index 349bf549ee44..f21fdd6cdd1a 100644
>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>> @@ -468,6 +468,11 @@ static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
>>>   };
>>>   MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
>>>   
>>> +static const struct property_entry dwc3_qcom_acpi_properties[] = {
>>> +	PROPERTY_ENTRY_STRING("dr_mode", "host"),
>>> +	{}
>>> +};
>>> +
>>>   static int dwc3_qcom_probe(struct platform_device *pdev)
>>>   {
>>>   	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
>>> @@ -603,6 +608,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>>   			goto platform_unalloc;
>>>   		}
>>>   
>>> +		ret = platform_device_add_properties(qcom->dwc3,
>>> +						     dwc3_qcom_acpi_properties);
>>> +		if (ret < 0) {
>>> +			dev_err(&pdev->dev, "failed to add properties\n");
>>> +			goto platform_unalloc;
>>> +		}
>>> +
>>>   		ret = platform_device_add(qcom->dwc3);
>>>   		if (ret) {
>>>   			dev_err(&pdev->dev, "failed to add device\n");
> 


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Lee Jones]

On Wed, 05 Jun 2019, Jeffrey Hugo wrote:

> On 6/5/2019 2:34 AM, Lee Jones wrote:
> > On Wed, 05 Jun 2019, Bjorn Andersson wrote:
> > 
> > > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > > 
> > > > When booting with Device Tree, the current default boot configuration
> > > > table option, the request to boot via 'host mode' comes from the
> > > > "dr_mode" property.
> > > 
> > > This has been the default on the MTP, but this is changing as this is
> > > causing issues when connected downstream from a hub (the typical
> > > development case for the primary USB port of a phone like device) and
> > > more importantly we don't have support for the PMIC blocks that control
> > > VBUS.
> > 
> > My point is not about which mode is currently chosen.  It's more about
> > the capability of choosing which mode is appropriate for a given
> > system via DT.
> > 
> > > Once these issues are resolved the dr_mode would be "otg".
> > 
> > OTG doesn't work on this H/W, so we need to specify "host" mode.
> 
> How have you made that determination?

I enabled OTG and plugged in all of the things.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Bjorn Andersson]

On Wed 05 Jun 01:34 PDT 2019, Lee Jones wrote:

> On Wed, 05 Jun 2019, Bjorn Andersson wrote:
> 
> > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > 
> > > When booting with Device Tree, the current default boot configuration
> > > table option, the request to boot via 'host mode' comes from the
> > > "dr_mode" property.
> > 
> > This has been the default on the MTP, but this is changing as this is
> > causing issues when connected downstream from a hub (the typical
> > development case for the primary USB port of a phone like device) and
> > more importantly we don't have support for the PMIC blocks that control
> > VBUS.
> 
> My point is not about which mode is currently chosen.  It's more about
> the capability of choosing which mode is appropriate for a given
> system via DT.
> 
> > Once these issues are resolved the dr_mode would be "otg".
> 
> OTG doesn't work on this H/W, so we need to specify "host" mode.
> 

My objection is that when you say "this H/W" you mean a particular
product, but you're making this decision for all SDM845 based products
using ACPI.

I don't know if there is a Windows phone based on SDM845, but if there
is then I don't think forcing it to host would be correct.

> > > A property of the same name can be used inside
> > > ACPI tables too.  However it is missing from the SDM845's ACPI tables
> > > so we have to supply this information using Platform Device Properites
> > > instead.
> > > 
> > 
> > Afaict this would install a fall-back property, so in the case that we
> > have specified dr_mode in DT (or ACPI) that would take precedence. So
> 
> That's correct.
> 
> > the commit message should reflect that this redefines the default choice
> > to be "host", rather than "otg".
> 
> No problem.
> 
> > Which is in conflict with what's described for dr_mode in
> > Documentation/devicetree/bindings/usb/generic.txt
> 
> This implementation only affects ACPI based platforms.  When booting
> with DT, the description in that DT related document is still
> accurate.
> 

You're right, I got lost between the patches and the sprinkled if
(ACPI_HANDLE()) in the probe. This is only added for ACPI.

> > And this driver is used on a range of different Qualcomm platforms, so I
> > don't think this is SDM845 specific.
> 
> ACPI based platforms?
> 
> All the ones I've seen use the XHCI USB driver directly ("PNP0D10").
>  

MSM8998 (835) has the same controller, so this should affect those
laptops as well.

Regards,
Bjorn

> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/usb/dwc3/dwc3-qcom.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index 349bf549ee44..f21fdd6cdd1a 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -468,6 +468,11 @@ static const struct acpi_device_id dwc3_qcom_acpi_match[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(acpi, dwc3_qcom_acpi_match);
> > >  
> > > +static const struct property_entry dwc3_qcom_acpi_properties[] = {
> > > +	PROPERTY_ENTRY_STRING("dr_mode", "host"),
> > > +	{}
> > > +};
> > > +
> > >  static int dwc3_qcom_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device_node	*np = pdev->dev.of_node, *dwc3_np;
> > > @@ -603,6 +608,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > >  			goto platform_unalloc;
> > >  		}
> > >  
> > > +		ret = platform_device_add_properties(qcom->dwc3,
> > > +						     dwc3_qcom_acpi_properties);
> > > +		if (ret < 0) {
> > > +			dev_err(&pdev->dev, "failed to add properties\n");
> > > +			goto platform_unalloc;
> > > +		}
> > > +
> > >  		ret = platform_device_add(qcom->dwc3);
> > >  		if (ret) {
> > >  			dev_err(&pdev->dev, "failed to add device\n");
> 
> -- 
> Lee Jones [?????????]
> Linaro Services Technical Lead
> Linaro.org ??? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 7/8] usb: dwc3: qcom: Start USB in 'host mode' on the SDM845

[Lee Jones]

On Wed, 05 Jun 2019, Bjorn Andersson wrote:

> On Wed 05 Jun 01:34 PDT 2019, Lee Jones wrote:
> 
> > On Wed, 05 Jun 2019, Bjorn Andersson wrote:
> > 
> > > On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:
> > > 
> > > > When booting with Device Tree, the current default boot configuration
> > > > table option, the request to boot via 'host mode' comes from the
> > > > "dr_mode" property.
> > > 
> > > This has been the default on the MTP, but this is changing as this is
> > > causing issues when connected downstream from a hub (the typical
> > > development case for the primary USB port of a phone like device) and
> > > more importantly we don't have support for the PMIC blocks that control
> > > VBUS.
> > 
> > My point is not about which mode is currently chosen.  It's more about
> > the capability of choosing which mode is appropriate for a given
> > system via DT.
> > 
> > > Once these issues are resolved the dr_mode would be "otg".
> > 
> > OTG doesn't work on this H/W, so we need to specify "host" mode.
> 
> My objection is that when you say "this H/W" you mean a particular
> product, but you're making this decision for all SDM845 based products
> using ACPI.
> 
> I don't know if there is a Windows phone based on SDM845, but if there
> is then I don't think forcing it to host would be correct.

You mean if someone wanted to boot Linux on a Windows phone?  Not sure
how likely that is, but even if a) there is an SDM845 based Windows
phone and b) someone is crazy enough to run Linux on it, it should be
trivial for them to conduct some device matching and choose a
different property based on the result.

[...]

> > > And this driver is used on a range of different Qualcomm platforms, so I
> > > don't think this is SDM845 specific.
> > 
> > ACPI based platforms?
> > 
> > All the ones I've seen use the XHCI USB driver directly ("PNP0D10").
> 
> MSM8998 (835) has the same controller, so this should affect those
> laptops as well.

This would also be the correct configuration for them too.  OTG
doesn't make much sense for a laptop form factor.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 8/8] usb: dwc3: qcom: Improve error handling

[Lee Jones]

dwc3_qcom_clk_init() is called with of_count_phandle_with_args() as an
argument.  If of_count_phandle_with_args() returns an error, the number
of clocks will be a negative value and will lead to undefined behaviour.

Ensure we check for an error before attempting to blindly use the value.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index f21fdd6cdd1a..633482926497 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -419,6 +419,9 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
 	if (!count || ACPI_HANDLE(dev))
 		return 0;
 
+	if (count < 0)
+		return count;
+
 	qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
 				  sizeof(struct clk *), GFP_KERNEL);
 	if (!qcom->clks)
-- 
2.17.1

Re: [PATCH 8/8] usb: dwc3: qcom: Improve error handling

[Bjorn Andersson]

On Tue 04 Jun 03:44 PDT 2019, Lee Jones wrote:

> dwc3_qcom_clk_init() is called with of_count_phandle_with_args() as an
> argument.  If of_count_phandle_with_args() returns an error, the number
> of clocks will be a negative value and will lead to undefined behaviour.
> 
> Ensure we check for an error before attempting to blindly use the value.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index f21fdd6cdd1a..633482926497 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -419,6 +419,9 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
>  	if (!count || ACPI_HANDLE(dev))
>  		return 0;
>  
> +	if (count < 0)
> +		return count;
> +
>  	qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
>  				  sizeof(struct clk *), GFP_KERNEL);
>  	if (!qcom->clks)
> -- 
> 2.17.1
> 

[PATCH 1/8] i2c: i2c-qcom-geni: Provide support for ACPI

[Lee Jones]

Add a match table to allow automatic probing of ACPI device
QCOM0220.  Ignore clock attainment errors.  Set default clock
frequency value.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index db075bc0d952..0fa93b448e8d 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
@@ -483,6 +484,12 @@ static const struct i2c_algorithm geni_i2c_algo = {
 	.functionality	= geni_i2c_func,
 };
 
+static const struct acpi_device_id geni_i2c_acpi_match[] = {
+	{ "QCOM0220"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
+
 static int geni_i2c_probe(struct platform_device *pdev)
 {
 	struct geni_i2c_dev *gi2c;
@@ -502,7 +509,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(gi2c->se.base);
 
 	gi2c->se.clk = devm_clk_get(&pdev->dev, "se");
-	if (IS_ERR(gi2c->se.clk)) {
+	if (IS_ERR(gi2c->se.clk) && !ACPI_HANDLE(&pdev->dev)) {
 		ret = PTR_ERR(gi2c->se.clk);
 		dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
 		return ret;
@@ -510,12 +517,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	ret = device_property_read_u32(&pdev->dev, "clock-frequency",
 							&gi2c->clk_freq_out);
-	if (ret) {
+	if (ret && !ACPI_HANDLE(&pdev->dev)) {
 		dev_info(&pdev->dev,
 			"Bus frequency not specified, default to 100kHz.\n");
 		gi2c->clk_freq_out = KHZ(100);
 	}
 
+	if (ACPI_HANDLE(&pdev->dev)) {
+		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(&pdev->dev));
+
+		/* Using default, same as the !ACPI case above */
+		gi2c->clk_freq_out = KHZ(100);
+	}
+
 	gi2c->irq = platform_get_irq(pdev, 0);
 	if (gi2c->irq < 0) {
 		dev_err(&pdev->dev, "IRQ error for i2c-geni\n");
@@ -660,6 +674,7 @@ static struct platform_driver geni_i2c_driver = {
 		.name = "geni_i2c",
 		.pm = &geni_i2c_pm_ops,
 		.of_match_table = geni_i2c_dt_match,
+		.acpi_match_table = ACPI_PTR(geni_i2c_acpi_match),
 	},
 };
 
-- 
2.17.1

[PATCH 2/8] i2c: i2c-qcom-geni: Signify successful driver probe

[Lee Jones]

The Qualcomm Geni I2C driver currently probes silently which can be
confusing when debugging potential issues.  Add a low level (INFO)
print when each I2C controller is successfully initially set-up.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 0fa93b448e8d..720131c40fe0 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -598,6 +598,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	dev_dbg(&pdev->dev, "Geni-I2C adaptor successfully added\n");
+
 	return 0;
 }
 
-- 
2.17.1