| Message ID | 20190610084213.1052-1-lee.jones@linaro.org |
|---|---|
| State | Accepted |
| Commit | c9913ac42135cf7f1c8986bcb175c5a7dda126e6 |
| Headers | show |
| Series | [v3,1/8] i2c: i2c-qcom-geni: Provide support for ACPI | expand |
On Mon, 10 Jun 2019 at 10:55, Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 10 Jun 2019, Ard Biesheuvel wrote: > > > On Mon, 10 Jun 2019 at 10:42, Lee Jones <lee.jones@linaro.org> 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> > > > > For the ACPI probing boilerplate: > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > *However*, I really don't like hardcoding reserved GPIOs like this. > > What guarantee do we have that each and every ACPI system > > incorporating the QCOM0217 device has the exact same list of reserved > > GPIOs? > > This is SDM845 specific, so the chances are reduced. > You don't know that. > However, if another SDM845 variant does crop up, also lacking the > "gpios" property, we will have to find another differentiating factor > between them and conduct some matching. What else can you do with > platforms supporting non-complete/non-forthcoming ACPI tables? > Either we don't touch any pins at all if they are not referenced explicitly anywhere, or we parse the PEP tables, which seem to cover some of this information (if Bjorn's analysis is correct)
On Mon, 10 Jun 2019, Ard Biesheuvel wrote: > On Mon, 10 Jun 2019 at 10:55, Lee Jones <lee.jones@linaro.org> wrote: > > > > On Mon, 10 Jun 2019, Ard Biesheuvel wrote: > > > > > On Mon, 10 Jun 2019 at 10:42, Lee Jones <lee.jones@linaro.org> 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> > > > > > > For the ACPI probing boilerplate: > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > > > *However*, I really don't like hardcoding reserved GPIOs like this. > > > What guarantee do we have that each and every ACPI system > > > incorporating the QCOM0217 device has the exact same list of reserved > > > GPIOs? > > > > This is SDM845 specific, so the chances are reduced. > > You don't know that. All the evidence I have to hand tells me that this is the case. Even on very closely related variants Qualcomm uses different H/W blocks for GPIO. > > However, if another SDM845 variant does crop up, also lacking the > > "gpios" property, we will have to find another differentiating factor > > between them and conduct some matching. What else can you do with > > platforms supporting non-complete/non-forthcoming ACPI tables? > > > > Either we don't touch any pins at all if they are not referenced > explicitly anywhere I guess this would require an API change, which is out of scope of this patch-set. Happy to change this implementation later if the subsystem allows for it though. > or we parse the PEP tables, which seem to cover > some of this information (if Bjorn's analysis is correct) Maybe someone can conduct some further work on this when we start to enable or write a driver for the PEP (Windows-compatible System Power Management Controller). The tables for the PEP look pretty complex, so this task would be extremely difficult if not impossible without Qualcomm's help. I wouldn't even know how to extrapolate this information from the tables. > (if Bjorn's analysis is correct) Bjorn is about to provide his Reviewed-by for this implementation. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Mon, 10 Jun 2019 at 11:22, Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 10 Jun 2019, Ard Biesheuvel wrote: > > > On Mon, 10 Jun 2019 at 10:55, Lee Jones <lee.jones@linaro.org> wrote: > > > > > > On Mon, 10 Jun 2019, Ard Biesheuvel wrote: > > > > > > > On Mon, 10 Jun 2019 at 10:42, Lee Jones <lee.jones@linaro.org> 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> > > > > > > > > For the ACPI probing boilerplate: > > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > > > > > *However*, I really don't like hardcoding reserved GPIOs like this. > > > > What guarantee do we have that each and every ACPI system > > > > incorporating the QCOM0217 device has the exact same list of reserved > > > > GPIOs? > > > > > > This is SDM845 specific, so the chances are reduced. > > > > You don't know that. > > All the evidence I have to hand tells me that this is the case. Even > on very closely related variants Qualcomm uses different H/W blocks > for GPIO. > > > > However, if another SDM845 variant does crop up, also lacking the > > > "gpios" property, we will have to find another differentiating factor > > > between them and conduct some matching. What else can you do with > > > platforms supporting non-complete/non-forthcoming ACPI tables? > > > > > > > Either we don't touch any pins at all if they are not referenced > > explicitly anywhere > > I guess this would require an API change, which is out of scope of > this patch-set. Happy to change this implementation later if the > subsystem allows for it though. > > > or we parse the PEP tables, which seem to cover > > some of this information (if Bjorn's analysis is correct) > > Maybe someone can conduct some further work on this when we start to > enable or write a driver for the PEP (Windows-compatible System Power > Management Controller). The tables for the PEP look pretty complex, > so this task would be extremely difficult if not impossible without > Qualcomm's help. I wouldn't even know how to extrapolate this > information from the tables. > > > (if Bjorn's analysis is correct) > > Bjorn is about to provide his Reviewed-by for this implementation. > If Bjorn can live with it, then so can I.
On Mon 10 Jun 01:42 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. As I said in my previous review, the default mode for SDM845 is OTG. For the MTP specifically we specify the default mode to be peripheral (was host). The remaining thing that worries me with this patch is that I do expect that at least one of the USB-C ports is OTG. But I am not able to conclude anything regarding this and host-only is a good default for this type of device, so I suggest that we merge this. Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > 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 Properties > instead. > > This does not change the behaviour of any currently supported devices. > The property is only set on ACPI enabled platforms, thus for H/W > booting DT, unless a 'dr_mode' property is present, the default is > still OTG (On-The-Go) as per [0]. Any new ACPI devices added will > also be able to over-ride this implementation by providing a 'dr_mode' > property in their ACPI tables. In cases where 'dr_mode' is omitted > from the tables AND 'host mode' should not be the default (very > unlikely), then we will have to add some way of choosing between them > at run time - most likely by ACPI HID. > > [0] Documentation/devicetree/bindings/usb/generic.txt > > 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 1e1f12b7991d..55ba04254e38 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -444,6 +444,11 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) > return 0; > } > > +static const struct property_entry dwc3_qcom_acpi_properties[] = { > + PROPERTY_ENTRY_STRING("dr_mode", "host"), > + {} > +}; > + > static int dwc3_qcom_acpi_register_core(struct platform_device *pdev) > { > struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > @@ -488,6 +493,13 @@ static int dwc3_qcom_acpi_register_core(struct platform_device *pdev) > goto out; > } > > + ret = platform_device_add_properties(qcom->dwc3, > + dwc3_qcom_acpi_properties); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to add properties\n"); > + goto out; > + } > + > ret = platform_device_add(qcom->dwc3); > if (ret) > dev_err(&pdev->dev, "failed to add device\n"); > -- > 2.17.1 >
On Mon, Jun 10, 2019 at 09:42:06AM +0100, Lee Jones wrote: > 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> Sadly, there is no cover-letter describing if there is a dependency or not. I assume there is, otherwise I would get the I2C patches only? But what is the suggested way upstream then? Also, the current maintainer entry for this driver looks like: drivers/i2c/busses/i2c-qcom-geni.c: Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT) David Brown <david.brown@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) Alok Chauhan <alokc@codeaurora.org> (supporter:QUALCOMM GENERIC INTERFACE I2C DRIVER) I didn't hear from those people yet, would be great to have their acks.
> There are no cross-subsystem build dependencies on any of these > patches. The only reason they are bundled together in the same > patch-set is for cross-subsystem visibility and understanding. > > There is wide interest in these devices. I see. That would have been a great cover-letter, Lee ;) Thanks for the heads up! > > > Also, the current maintainer entry for this driver looks like: > > > > drivers/i2c/busses/i2c-qcom-geni.c: > > Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT) > > David Brown <david.brown@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) > > Alok Chauhan <alokc@codeaurora.org> (supporter:QUALCOMM GENERIC INTERFACE I2C DRIVER) > > > > I didn't hear from those people yet, would be great to have their acks. > > I will see if I can rouse them from their slumber. Please do. If they are not to reach, we probably need to update the entry...
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index db075bc0d952..9e3b8a98688d 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,14 @@ static const struct i2c_algorithm geni_i2c_algo = { .functionality = geni_i2c_func, }; +#ifdef CONFIG_ACPI +static const struct acpi_device_id geni_i2c_acpi_match[] = { + { "QCOM0220"}, + { }, +}; +MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match); +#endif + static int geni_i2c_probe(struct platform_device *pdev) { struct geni_i2c_dev *gi2c; @@ -502,7 +511,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) && !has_acpi_companion(&pdev->dev)) { ret = PTR_ERR(gi2c->se.clk); dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); return ret; @@ -516,6 +525,9 @@ static int geni_i2c_probe(struct platform_device *pdev) gi2c->clk_freq_out = KHZ(100); } + if (has_acpi_companion(&pdev->dev)) + ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(&pdev->dev)); + gi2c->irq = platform_get_irq(pdev, 0); if (gi2c->irq < 0) { dev_err(&pdev->dev, "IRQ error for i2c-geni\n"); @@ -660,6 +672,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), }, };
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 | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) -- 2.17.1