| Message ID | 20190604104455.8877-1-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/8] i2c: i2c-qcom-geni: Provide support for ACPI | expand |
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 >
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 >
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
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
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
> 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.
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
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), }, };
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(-) -- 2.17.1