Re: [RFC PATCH] USB: misc: Add usb_hub_pwr driver
Hi Greg, On Wed, Sep 02, 2020 at 08:07:44AM +0200, Greg Kroah-Hartman wrote: > On Tue, Sep 01, 2020 at 01:21:43PM -0700, Matthias Kaehlcke wrote: > > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > > index da39bddb0604..2bd02388ca62 100644 > > --- a/drivers/usb/misc/Makefile > > +++ b/drivers/usb/misc/Makefile > > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY)+= chaoskey.o > > > > obj-$(CONFIG_USB_SISUSBVGA)+= sisusbvga/ > > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > > +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o > > Why is this 2 files? Why can't it just be one? It's effectively two drivers that work together, it seemed cleaner to me to have a file for every driver. The 'usb_hub_psupply' driver (which probably should be named 'onboard_usb_hub' or similar) would even be useful on its own (taking care of powering the hub on and potentially other setup actions) with a bit of rework. > And isn't this much the same thing as many of the other "misc" hub > controller drivers we have? There are some commonalities, however most of these drivers seem to target USB hubs with an I2C bus, using custom 'commands' for initialization and putting the hub in/out of standby. The drivers in this patch have two goals: - provide a generic (configurable) solution for powering up/initializing a USB hub - enable support for powering down a USB hub during system suspend > And to make it easier to review, can you split out the device tree > descriptions so that the DT maintainers can review that? Sure, I wasn't sure if this is the right approach, hence I only sent an RFC without formal bindings to get initial feedback. As of now it seems that you are at least not frontally opposed it ;-)
Re: [RFC PATCH] USB: misc: Add usb_hub_pwr driver
Hi Peter, On Wed, Sep 02, 2020 at 05:31:06AM +, Peter Chen wrote: > On 20-09-01 13:21:43, Matthias Kaehlcke wrote: > > The driver combo usb_hub_pwr/usb_hub_psupply allows to control > > the power supply of an onboard USB hub. > > > > The drivers address two issues: > > - a USB hub needs to be powered before it can be discovered > > - battery powered devices may want to switch the USB hub off > >during suspend to extend battery life > > > > The regulator of the hub is controlled by the usb_hub_psupply > > platform driver. The regulator is switched on when the platform > > device is initialized, which enables discovery of the hub. The > > driver provides an external interface to enable/disable the > > power supply which is used by the usb_hub_pwr driver. > > > > The usb_hub_pwr extends the generic USB hub driver. The device is > > initialized when the hub is discovered by the USB subsystem. It > > uses the usb_hub_psupply interface to make its own request to > > enable the regulator (increasing the use count to 2). > > > > During system suspend usb_hub_pwr checks if any wakeup capable > > devices are connected to the hub. If not it 'disables' the hub > > regulator (decreasing the use count to 1, hence the regulator > > stays enabled for now). When the usb_hub_psupply device suspends > > it disables the hub regulator unconditionally (decreasing the use > > count to 0 or 1, depending on the actions of usb_hub_pwr). This > > is done to allow the usb_hub_pwr device to control the state of > > the regulator during system suspend. > > > > Upon resume usb_hub_psupply enables the regulator again, the > > usb_hub_pwr device does the same if it disabled the regulator > > during resume. > > Hi Matthias, > > I did similar several years ago [1], but the concept (power sequence) > doesn't be accepted by power maintainer. Yeah, I saw that, I think I even reviewed or tested some early version of it :) > Your patch introduce an new way to fix this long-term issue, I have an > idea to fix it more generally. > - Create a table (say usb_pm_table) for USB device which needs to do > initial power on and power management during suspend suspend/resume based > on VID and PID, example: usb/core/quirks.c > - After hub (both roothub and intermediate hub) device is created, search > the DT node under this hub, and see if the device is in usb_pm_table. If > it is in it, create a platform device, say usb-power-supply, and the > related driver is like your usb_hub_psupply.c, the parent of this device > is controller device. This part isn't clear to me. How would the DT look like? Would it have a single node per physical hub chip or one node for each 'logical' hub? Similarly, would there be a single plaform device or multiple? I guess when registering the platform device we could pass it the corresponding DT node, to allow it to determine its regulators, GPIOs, etc during probe. > - After this usb-power-supply device is probed, do initial power on at > probe, eg, clock, regulator, reset-gpio. > - This usb-power-supply device system suspend operation should be called after > onboard device has suspended since it is created before it. No runtime PM ops > are needed for it. > - When the hub is removed, delete this platform device. What exactly is removal? It seems once the hub is 'removed' it could never be probed again because the platform device that is in charge of the initialization is only created when the USB controller is initialized. I have the impression that the platform device would have to exist as long as the USB controller.
Re: [RFC PATCH] USB: misc: Add usb_hub_pwr driver
Hi Doug,
On Tue, Sep 01, 2020 at 02:21:49PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Sep 1, 2020 at 1:21 PM Matthias Kaehlcke wrote:
> >
> > The driver combo usb_hub_pwr/usb_hub_psupply allows to control
> > the power supply of an onboard USB hub.
> >
> > The drivers address two issues:
> > - a USB hub needs to be powered before it can be discovered
> > - battery powered devices may want to switch the USB hub off
> >during suspend to extend battery life
> >
> > The regulator of the hub is controlled by the usb_hub_psupply
> > platform driver. The regulator is switched on when the platform
> > device is initialized, which enables discovery of the hub. The
> > driver provides an external interface to enable/disable the
> > power supply which is used by the usb_hub_pwr driver.
> >
> > The usb_hub_pwr extends the generic USB hub driver. The device is
> > initialized when the hub is discovered by the USB subsystem. It
> > uses the usb_hub_psupply interface to make its own request to
> > enable the regulator (increasing the use count to 2).
> >
> > During system suspend usb_hub_pwr checks if any wakeup capable
> > devices are connected to the hub. If not it 'disables' the hub
> > regulator (decreasing the use count to 1, hence the regulator
> > stays enabled for now). When the usb_hub_psupply device suspends
> > it disables the hub regulator unconditionally (decreasing the use
> > count to 0 or 1, depending on the actions of usb_hub_pwr). This
> > is done to allow the usb_hub_pwr device to control the state of
> > the regulator during system suspend.
> >
> > Upon resume usb_hub_psupply enables the regulator again, the
> > usb_hub_pwr device does the same if it disabled the regulator
> > during resume.
> >
> > Co-developed-by: Ravi Chandra Sadineni
> > Signed-off-by: Ravi Chandra Sadineni
> > Signed-off-by: Matthias Kaehlcke
> > ---
> > The driver currently only supports a single power supply. This should
> > work for most/many configurations/hubs, support for multiple power
> > supplies can be added later if needed.
> >
> > No DT bindings are included since this is just a RFC. Here is a DT
> > example:
> >
> > usb_hub_psupply: usb-hub-psupply {
> > compatible = "linux,usb_hub_psupply";
> > vdd-supply = <_hub>;
> > };
>
> Definitely bikeshedding, but I would name this differently. The
> name/compatible you have makes this sound as if it's a software
> concept that we're sticking into DT. That's generally discouraged.
> ...if we name it slightly different then I think the driver can work
> the same but be more in the spirit of DT describing hardware.
> Specifically, I think it'd be better as:
>
> usb_hub: usb-hub {
> compatible = "realtek,rts5411", "onboard-usb-hub";
> vdd-supply = <_hub>;
> };
>
> Now we're describing hardware that's on the board. We have a RTS5411
> hub and we've described its power supply. There's also precedent for
> describing an on-board USB hub in a lop-level node like this
> (smsc,usb3503).
You are right, it definitely describes existing hardware, not some
linux-ism.
I also envisioned that there might be eventually device specific
compatible strings to support multiple regulators or even hub specific
power on/off sequences.
> Now, I know what you're saying: we're already describing this hub
> underneath the USB port node below. I think this is OK to have
> different aspects of the device described in two places in the DT,
> though of course I could be corrected by someone more knowledgeable.
>
>
> > _1_dwc3 {
> > /* 2.0 hub on port 1 */
> > hub@1 {
> > compatible = "usbbda,5411";
>
> What is "usbbda"? I would probably just call this:
>
> compatible = "realtek,rts5411-usb2", "onboard-usb-hub-usb2"
That's from the USB bindings:
Required properties for device nodes:
- compatible: "usbVID,PID", where VID is the vendor id and PID the product id.
The textual representation of VID and PID shall be in lower case hexadecimal
with leading zeroes suppressed. The other compatible strings from the above
standard binding could also be used, but a device adhering to this binding
may leave out all except for "usbVID,PID".
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/usb/usb-device.txt
>
>
> > reg = <1>;
> > psupply = <_hub_psupply>;
>
> Calling this psupply sounds a bit too much like you're referring to a
> regulator (with the
Re: [PATCH] arm64: dts: qcom: sc7180: Add 'sustainable_power' for CPU thermal zones
On Tue, Sep 01, 2020 at 03:45:52PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Sep 1, 2020 at 2:33 PM Matthias Kaehlcke wrote:
> >
> > Hi Doug,
> >
> > On Tue, Sep 01, 2020 at 01:19:10PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Tue, Sep 1, 2020 at 10:07 AM Matthias Kaehlcke
> > > wrote:
> > > >
> > > > On Thu, Aug 13, 2020 at 11:30:33AM -0700, Matthias Kaehlcke wrote:
> > > > > The 'sustainable_power' attribute provides an estimate of the
> > > > > sustained
> > > > > power that can be dissipated at the desired control temperature. One
> > > > > could argue that this value is not necessarily the same for all
> > > > > devices
> > > > > with the same SoC, which may have different form factors or thermal
> > > > > designs. However there are reasons to specify a (default) value at SoC
> > > > > level for SC7180: most importantly, if no value is specified at all
> > > > > the
> > > > > power_allocator thermal governor (aka 'IPA') estimates a value, using
> > > > > the
> > > > > minimum power of all cooling devices of the zone, which can result in
> > > > > overly aggressive thermal throttling. For most devices an approximate
> > > > > conservative value should be more useful than the minimum guesstimate
> > > > > of power_allocator. Devices that need a different value can overwrite
> > > > > it in their .dts. Also the thermal zones for SC7180 have a
> > > > > high
> > > > > level of granularity (essentially one for each function block), which
> > > > > makes it more likely that the default value just works for many
> > > > > devices.
> > > > >
> > > > > The values correspond to 1901 MHz for the big cores, and 1804 MHz for
> > > > > the small cores. The values were determined by limiting the CPU
> > > > > frequencies to different max values and launching a bunch of processes
> > > > > that cause high CPU load ('while true; do true; done &' is simple and
> > > > > does a good job). A frequency is deemed sustainable if the CPU
> > > > > temperatures don't rise (consistently) above the second trip point
> > > > > ('control temperature', 95 degC in this case). Once the highest
> > > > > sustainable frequency is found, the sustainable power can be
> > > > > calculated
> > > > > by multiplying the energy consumption per core at this frequency
> > > > > (which
> > > > > can be found in /sys/kernel/debug/energy_model/) with the number of
> > > > > cores that are specified as cooling devices.
> > > > >
> > > > > The sustainable frequencies were determined at room temperature
> > > > > on a device without heat sink or other passive cooling elements.
> > >
> > > I'm curious: was this a bare board, or a device in a case? Hrm, I'm
> > > not sure which one would be worse at heat dissipation, but I would
> > > imagine that being inside a plastic case might be worse?
> >
> > This was with a device in a plastic case.
> >
> > > > > Signed-off-by: Matthias Kaehlcke
> > > > > ---
> > > > > If maintainers think 'sustainable_power' should be specified at
> > > > > device level (with which I conceptually agree) I'm fine with
> > > > > doing that, just seemed it could be useful to have a reasonable
> > > > > 'default' at SoC level in this case.
> > > >
> > > > Any comments on this?
> > >
> > > I'm not massively familiar with this area of the code, but I guess I
> > > shouldn't let that stop me from having an opinion! :-P
> > >
> > > * I would agree that it seems highly unlikely that someone would put
> > > one of these chips in a device that could only dissipate the heat from
> > > the lowest OPP, so having some higher estimate definitely makes sense.
> > >
> > > * In terms of the numbers here, I believe that you're claiming that we
> > > can dissipate 768 mW * 6 + 1202 mW * 2 = ~7 Watts of power.
> >
> > No, I'm claiming it's 768 mW + 1202 mW = ~2 W.
> >
> > SC7180 has a 6 thermal zones for the 6 little cores and 4 zones for the
> > 2 big cores. Each of these thermal zones uses either all little or all big
> > cores as cooling devices, hence the power sustainable power
Re: [PATCH] arm64: dts: qcom: sc7180: Add 'sustainable_power' for CPU thermal zones
Hi Doug,
On Tue, Sep 01, 2020 at 01:19:10PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Sep 1, 2020 at 10:07 AM Matthias Kaehlcke wrote:
> >
> > On Thu, Aug 13, 2020 at 11:30:33AM -0700, Matthias Kaehlcke wrote:
> > > The 'sustainable_power' attribute provides an estimate of the sustained
> > > power that can be dissipated at the desired control temperature. One
> > > could argue that this value is not necessarily the same for all devices
> > > with the same SoC, which may have different form factors or thermal
> > > designs. However there are reasons to specify a (default) value at SoC
> > > level for SC7180: most importantly, if no value is specified at all the
> > > power_allocator thermal governor (aka 'IPA') estimates a value, using the
> > > minimum power of all cooling devices of the zone, which can result in
> > > overly aggressive thermal throttling. For most devices an approximate
> > > conservative value should be more useful than the minimum guesstimate
> > > of power_allocator. Devices that need a different value can overwrite
> > > it in their .dts. Also the thermal zones for SC7180 have a high
> > > level of granularity (essentially one for each function block), which
> > > makes it more likely that the default value just works for many devices.
> > >
> > > The values correspond to 1901 MHz for the big cores, and 1804 MHz for
> > > the small cores. The values were determined by limiting the CPU
> > > frequencies to different max values and launching a bunch of processes
> > > that cause high CPU load ('while true; do true; done &' is simple and
> > > does a good job). A frequency is deemed sustainable if the CPU
> > > temperatures don't rise (consistently) above the second trip point
> > > ('control temperature', 95 degC in this case). Once the highest
> > > sustainable frequency is found, the sustainable power can be calculated
> > > by multiplying the energy consumption per core at this frequency (which
> > > can be found in /sys/kernel/debug/energy_model/) with the number of
> > > cores that are specified as cooling devices.
> > >
> > > The sustainable frequencies were determined at room temperature
> > > on a device without heat sink or other passive cooling elements.
>
> I'm curious: was this a bare board, or a device in a case? Hrm, I'm
> not sure which one would be worse at heat dissipation, but I would
> imagine that being inside a plastic case might be worse?
This was with a device in a plastic case.
> > > Signed-off-by: Matthias Kaehlcke
> > > ---
> > > If maintainers think 'sustainable_power' should be specified at
> > > device level (with which I conceptually agree) I'm fine with
> > > doing that, just seemed it could be useful to have a reasonable
> > > 'default' at SoC level in this case.
> >
> > Any comments on this?
>
> I'm not massively familiar with this area of the code, but I guess I
> shouldn't let that stop me from having an opinion! :-P
>
> * I would agree that it seems highly unlikely that someone would put
> one of these chips in a device that could only dissipate the heat from
> the lowest OPP, so having some higher estimate definitely makes sense.
>
> * In terms of the numbers here, I believe that you're claiming that we
> can dissipate 768 mW * 6 + 1202 mW * 2 = ~7 Watts of power.
No, I'm claiming it's 768 mW + 1202 mW = ~2 W.
SC7180 has a 6 thermal zones for the 6 little cores and 4 zones for the
2 big cores. Each of these thermal zones uses either all little or all big
cores as cooling devices, hence the power sustainable power of the
individual zones doesn't add up. 768 mW corresponds to 6x 128 mW (aka all
little cores at 1.8 GHz), and 1202 mW to 2x 601 mW (both big cores at 1.9 GHz).
> My memory
> of how much power we could dissipate in previous laptops I worked on
> is a little fuzzy, but that doesn't seem insane for a passively-cooled
> laptop. However, I think someone could conceivably put this chip in a
> smaller form factor. In such a case, it seems like we'd want these
> things to sum up to ~2000 (if it would ever make sense for someone to
> put this chip in a phone) or ~4000 (if it would ever make sense for
> someone to put this chip in a small tablet).
See above, the sustainable power with this patch only adds up to ~2000.
It is possible though that it would be lower in a smaller form factor
device.
I'd be ok with posting something lower for SC7180 (it would be a guess
though) and use the specific numbers in the device specific DT.
> It seems possible that,
> to achieve this, we might have to tweak the
>
[RFC PATCH] USB: misc: Add usb_hub_pwr driver
The driver combo usb_hub_pwr/usb_hub_psupply allows to control
the power supply of an onboard USB hub.
The drivers address two issues:
- a USB hub needs to be powered before it can be discovered
- battery powered devices may want to switch the USB hub off
during suspend to extend battery life
The regulator of the hub is controlled by the usb_hub_psupply
platform driver. The regulator is switched on when the platform
device is initialized, which enables discovery of the hub. The
driver provides an external interface to enable/disable the
power supply which is used by the usb_hub_pwr driver.
The usb_hub_pwr extends the generic USB hub driver. The device is
initialized when the hub is discovered by the USB subsystem. It
uses the usb_hub_psupply interface to make its own request to
enable the regulator (increasing the use count to 2).
During system suspend usb_hub_pwr checks if any wakeup capable
devices are connected to the hub. If not it 'disables' the hub
regulator (decreasing the use count to 1, hence the regulator
stays enabled for now). When the usb_hub_psupply device suspends
it disables the hub regulator unconditionally (decreasing the use
count to 0 or 1, depending on the actions of usb_hub_pwr). This
is done to allow the usb_hub_pwr device to control the state of
the regulator during system suspend.
Upon resume usb_hub_psupply enables the regulator again, the
usb_hub_pwr device does the same if it disabled the regulator
during resume.
Co-developed-by: Ravi Chandra Sadineni
Signed-off-by: Ravi Chandra Sadineni
Signed-off-by: Matthias Kaehlcke
---
The driver currently only supports a single power supply. This should
work for most/many configurations/hubs, support for multiple power
supplies can be added later if needed.
No DT bindings are included since this is just a RFC. Here is a DT
example:
usb_hub_psupply: usb-hub-psupply {
compatible = "linux,usb_hub_psupply";
vdd-supply = <_hub>;
};
_1_dwc3 {
/* 2.0 hub on port 1 */
hub@1 {
compatible = "usbbda,5411";
reg = <1>;
psupply = <_hub_psupply>;
};
/* 3.0 hub on port 2 */
hub@2 {
compatible = "usbbda,411";
reg = <2>;
psupply = <_hub_psupply>;
};
};
drivers/usb/misc/Kconfig | 14 +++
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/usb_hub_psupply.c | 112 ++
drivers/usb/misc/usb_hub_psupply.h | 9 ++
drivers/usb/misc/usb_hub_pwr.c | 177 +
5 files changed, 313 insertions(+)
create mode 100644 drivers/usb/misc/usb_hub_psupply.c
create mode 100644 drivers/usb/misc/usb_hub_psupply.h
create mode 100644 drivers/usb/misc/usb_hub_pwr.c
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 6818ea689cd9..79ed50e6a7bf 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -275,3 +275,17 @@ config USB_CHAOSKEY
To compile this driver as a module, choose M here: the
module will be called chaoskey.
+
+config USB_HUB_PWR
+ tristate "Control power supply for onboard USB hubs"
+ depends on PM
+ help
+ Say Y here if you want to control the power supply of an
+ onboard USB hub. The driver switches the power supply of the
+ hub on, to make sure the hub can be discovered. During system
+ suspend the power supply is switched off, unless a wakeup
+ capable device is connected to the hub. This may reduce power
+ consumption on battery powered devices.
+
+ To compile this driver as a module, choose M here: the
+ module will be called usb_hub_pwr.
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index da39bddb0604..2bd02388ca62 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY)+= chaoskey.o
obj-$(CONFIG_USB_SISUSBVGA)+= sisusbvga/
obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o
+obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o
diff --git a/drivers/usb/misc/usb_hub_psupply.c
b/drivers/usb/misc/usb_hub_psupply.c
new file mode 100644
index ..6a155ae1f831
--- /dev/null
+++ b/drivers/usb/misc/usb_hub_psupply.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020, Google LLC
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct usb_hub_psupply_dev {
+ struct regulator *vdd;
+};
+
+int usb_hub_psupply_on(struct device *dev)
+{
+ struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev);
+ int err;
+
+ err = regulator_enable(usb_hub_psupply->vdd);
+ if (err) {
+ dev_err(dev, "failed to enable regulator: %d\n", err);
+ return err;
+ }
+
+ return 0;
+
+}
+EXPORT_SYMBOL_GPL(usb_hub_psupply_on);
+
Re: [PATCH] arm64: dts: qcom: sc7180: Add 'sustainable_power' for CPU thermal zones
On Thu, Aug 13, 2020 at 11:30:33AM -0700, Matthias Kaehlcke wrote:
> The 'sustainable_power' attribute provides an estimate of the sustained
> power that can be dissipated at the desired control temperature. One
> could argue that this value is not necessarily the same for all devices
> with the same SoC, which may have different form factors or thermal
> designs. However there are reasons to specify a (default) value at SoC
> level for SC7180: most importantly, if no value is specified at all the
> power_allocator thermal governor (aka 'IPA') estimates a value, using the
> minimum power of all cooling devices of the zone, which can result in
> overly aggressive thermal throttling. For most devices an approximate
> conservative value should be more useful than the minimum guesstimate
> of power_allocator. Devices that need a different value can overwrite
> it in their .dts. Also the thermal zones for SC7180 have a high
> level of granularity (essentially one for each function block), which
> makes it more likely that the default value just works for many devices.
>
> The values correspond to 1901 MHz for the big cores, and 1804 MHz for
> the small cores. The values were determined by limiting the CPU
> frequencies to different max values and launching a bunch of processes
> that cause high CPU load ('while true; do true; done &' is simple and
> does a good job). A frequency is deemed sustainable if the CPU
> temperatures don't rise (consistently) above the second trip point
> ('control temperature', 95 degC in this case). Once the highest
> sustainable frequency is found, the sustainable power can be calculated
> by multiplying the energy consumption per core at this frequency (which
> can be found in /sys/kernel/debug/energy_model/) with the number of
> cores that are specified as cooling devices.
>
> The sustainable frequencies were determined at room temperature
> on a device without heat sink or other passive cooling elements.
>
> Signed-off-by: Matthias Kaehlcke
> ---
> If maintainers think 'sustainable_power' should be specified at
> device level (with which I conceptually agree) I'm fine with
> doing that, just seemed it could be useful to have a reasonable
> 'default' at SoC level in this case.
Any comments on this?
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Satya,
On Thu, Aug 27, 2020 at 08:37:33PM +0530, ska...@codeaurora.org wrote:
> Hi Matthias,
>
> On 2020-08-26 22:10, Matthias Kaehlcke wrote:
> > Hi Satya,
> >
> > On Wed, Aug 26, 2020 at 09:35:15PM +0530, ska...@codeaurora.org wrote:
> > > Hi Matthias,
> > >
> > > On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > > > On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> > > > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > > > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > > > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > > > > configuration to match Bluetooth module.
> > > > > > >
> > > > > > > Signed-off-by: satya priya
> > > > > > > Reviewed-by: Akash Asthana
> > > > > > > ---
> > > > > > > Changes in V2:
> > > > > > > - This patch adds sleep state for BT UART. Newly added in V2.
> > > > > > >
> > > > > > > Changes in V3:
> > > > > > > - Remove "output-high" for TX from both sleep and default states
> > > > > > >as it is not required. Configure pull-up for TX in sleep state.
> > > > > > >
> > > > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > > > +++--
> > > > > > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > index d8b5507..806f626 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > > > @@ -473,20 +473,20 @@
> > > > > > >
> > > > > > > _uart3_default {
> > > > > > > pinconf-cts {
> > > > > > > - /*
> > > > > > > - * Configure a pull-down on 38 (CTS) to match the pull
> > > > > > > of
> > > > > > > - * the Bluetooth module.
> > > > > > > - */
> > > > > > > + /* Configure no pull on 38 (CTS) to match Bluetooth
> > > > > > > module */
> > > > > > > pins = "gpio38";
> > > > > > > - bias-pull-down;
> > > > > > > - output-high;
> > > > > > > + bias-disable;
> > > > > >
> > > > > > I think it should be ok in functional terms, but I don't like the
> > > > > > rationale
> > > > > > and also doubt the change is really needed.
> > > > > >
> > > > > > If the pull is removed to match the Bluetooth module, then that
> > > > > > sounds
> > > > > > as
> > > > > > if the signal was floating on the the BT side, which I think is not
> > > > > > the
> > > > > > case.
> > > > > > Yes, according to the datasheet there is no pull when the BT
> > > > > > controller
> > > > > > is
> > > > > > active, but then it drives the signal actively to either high or
> > > > > > low.
> > > > > > There
> > > > > > seems to be no merit in 'matching' the Bluetooth side in this case,
> > > > > > if
> > > > > > the
> > > > > > signal was really floating on the BT side we would definitely not
> > > > > > want
> > > > > > this.
> > > > > >
> > > > > > In a reply to v2 you said:
> > > > > >
> > > > > > > Recently on cherokee we worked with BT team and came to an
> > > > > > > agreement
> > > > > > > to
> > > > > > > keep no-pull from our side in order to not conflict with their
> > > > > > > pull in
> > > > > > > any state.
> > > > > >
> > > > > > What are these conflicting pull states?
> > > > > >
> > > > > > The WCN3998 datasheet has a pull-do
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
Hi Satya,
On Wed, Aug 26, 2020 at 09:35:15PM +0530, ska...@codeaurora.org wrote:
> Hi Matthias,
>
> On 2020-08-25 22:08, Matthias Kaehlcke wrote:
> > On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> > > On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > > > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > > configuration to match Bluetooth module.
> > > > >
> > > > > Signed-off-by: satya priya
> > > > > Reviewed-by: Akash Asthana
> > > > > ---
> > > > > Changes in V2:
> > > > > - This patch adds sleep state for BT UART. Newly added in V2.
> > > > >
> > > > > Changes in V3:
> > > > > - Remove "output-high" for TX from both sleep and default states
> > > > >as it is not required. Configure pull-up for TX in sleep state.
> > > > >
> > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > > > +++--
> > > > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > index d8b5507..806f626 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > @@ -473,20 +473,20 @@
> > > > >
> > > > > _uart3_default {
> > > > > pinconf-cts {
> > > > > - /*
> > > > > - * Configure a pull-down on 38 (CTS) to match the pull
> > > > > of
> > > > > - * the Bluetooth module.
> > > > > - */
> > > > > + /* Configure no pull on 38 (CTS) to match Bluetooth
> > > > > module */
> > > > > pins = "gpio38";
> > > > > - bias-pull-down;
> > > > > - output-high;
> > > > > + bias-disable;
> > > >
> > > > I think it should be ok in functional terms, but I don't like the
> > > > rationale
> > > > and also doubt the change is really needed.
> > > >
> > > > If the pull is removed to match the Bluetooth module, then that sounds
> > > > as
> > > > if the signal was floating on the the BT side, which I think is not the
> > > > case.
> > > > Yes, according to the datasheet there is no pull when the BT controller
> > > > is
> > > > active, but then it drives the signal actively to either high or low.
> > > > There
> > > > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > > > the
> > > > signal was really floating on the BT side we would definitely not want
> > > > this.
> > > >
> > > > In a reply to v2 you said:
> > > >
> > > > > Recently on cherokee we worked with BT team and came to an agreement
> > > > > to
> > > > > keep no-pull from our side in order to not conflict with their pull in
> > > > > any state.
> > > >
> > > > What are these conflicting pull states?
> > > >
> > > > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > > > boot mode, and no pull in active mode. In reset and boot mode the host
> > > > config with a pull down would match, and no pull in active mode doesn't
> > > > conflict with the pull-down on the host UART. My understanding is that
> > > > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > > > RTS the pull on the host side shouldn't matter.
> > > >
> > >
> > > yes, I agree with you, the pinconf pulls are weak. As this is driven
> > > by BT
> > > SoC (pull on HOST side shouldn't matter), we are not mentioning any
> > > bias
> > > configuration from our side and simply putting it as no-pull, just
> > > to not
> > > conflict in any case. It seems that the rationale mentioned is a bit
> > > confusing i will change it to clearly specify why we are configuring
> > > no-pull.
> > >
> > > > Is this change actually related with wakeup support? I have the
> &
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Tue, Aug 25, 2020 at 06:42:28PM +0530, ska...@codeaurora.org wrote:
> On 2020-08-21 22:52, Matthias Kaehlcke wrote:
> > On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> > > Add sleep pin ctrl for BT uart, and also change the bias
> > > configuration to match Bluetooth module.
> > >
> > > Signed-off-by: satya priya
> > > Reviewed-by: Akash Asthana
> > > ---
> > > Changes in V2:
> > > - This patch adds sleep state for BT UART. Newly added in V2.
> > >
> > > Changes in V3:
> > > - Remove "output-high" for TX from both sleep and default states
> > >as it is not required. Configure pull-up for TX in sleep state.
> > >
> > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> > > +++--
> > > 1 file changed, 45 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > index d8b5507..806f626 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > @@ -473,20 +473,20 @@
> > >
> > > _uart3_default {
> > > pinconf-cts {
> > > - /*
> > > - * Configure a pull-down on 38 (CTS) to match the pull of
> > > - * the Bluetooth module.
> > > - */
> > > + /* Configure no pull on 38 (CTS) to match Bluetooth module */
> > > pins = "gpio38";
> > > - bias-pull-down;
> > > - output-high;
> > > + bias-disable;
> >
> > I think it should be ok in functional terms, but I don't like the
> > rationale
> > and also doubt the change is really needed.
> >
> > If the pull is removed to match the Bluetooth module, then that sounds
> > as
> > if the signal was floating on the the BT side, which I think is not the
> > case.
> > Yes, according to the datasheet there is no pull when the BT controller
> > is
> > active, but then it drives the signal actively to either high or low.
> > There
> > seems to be no merit in 'matching' the Bluetooth side in this case, if
> > the
> > signal was really floating on the BT side we would definitely not want
> > this.
> >
> > In a reply to v2 you said:
> >
> > > Recently on cherokee we worked with BT team and came to an agreement
> > > to
> > > keep no-pull from our side in order to not conflict with their pull in
> > > any state.
> >
> > What are these conflicting pull states?
> >
> > The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
> > boot mode, and no pull in active mode. In reset and boot mode the host
> > config with a pull down would match, and no pull in active mode doesn't
> > conflict with the pull-down on the host UART. My understanding is that
> > the pinconf pulls are weak pulls, so as soon as the BT chip drives its
> > RTS the pull on the host side shouldn't matter.
> >
>
> yes, I agree with you, the pinconf pulls are weak. As this is driven by BT
> SoC (pull on HOST side shouldn't matter), we are not mentioning any bias
> configuration from our side and simply putting it as no-pull, just to not
> conflict in any case. It seems that the rationale mentioned is a bit
> confusing i will change it to clearly specify why we are configuring
> no-pull.
>
> > Is this change actually related with wakeup support? I have the
> > impression
> > that multiple things are conflated in this patch. If some of the changes
> > are just fixing/improving other things they should be in a separate
> > patch,
> > which could be part of this series, otherwise it's really hard to
> > distinguish between the pieces that are actually relevant for wakeup and
> > the rest.
> >
> > Independently of whether the changes are done in a single or multiple
> > patches, the commit log should include details on why the changes are
> > necessary, especially when there are not explantatory comments in the
> > DT/code itself (e.g. the removal of 'output-high', which seems correct
> > to me, but no reason is given why it is done).
> >
>
> This change is not related to wakeup support, I will make it a separate
> patch, will also mention the details in commit text.
>
> > > };
> > >
> > > pinconf-rts {
> > > - /* We'll drive 39 (RTS), so no pull */
> > > + /*
> > > +
Re: [PATCH] interconnect: Show bandwidth for disabled paths as zero in debugfs
On Wed, Jul 29, 2020 at 10:50:12AM -0700, Matthias Kaehlcke wrote: > For disabled paths the 'interconnect_summary' in debugfs currently shows > the orginally requested bandwidths. This is confusing, since the bandwidth > requests aren't active. Instead show the bandwidths for disabled > paths/requests as zero. > > Signed-off-by: Matthias Kaehlcke ping, any comments on this?
Re: [PATCH V3 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Thu, Aug 20, 2020 at 07:21:06PM +0530, satya priya wrote:
> Add sleep pin ctrl for BT uart, and also change the bias
> configuration to match Bluetooth module.
>
> Signed-off-by: satya priya
> Reviewed-by: Akash Asthana
> ---
> Changes in V2:
> - This patch adds sleep state for BT UART. Newly added in V2.
>
> Changes in V3:
> - Remove "output-high" for TX from both sleep and default states
>as it is not required. Configure pull-up for TX in sleep state.
>
> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 54
> +++--
> 1 file changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index d8b5507..806f626 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -473,20 +473,20 @@
>
> _uart3_default {
> pinconf-cts {
> - /*
> - * Configure a pull-down on 38 (CTS) to match the pull of
> - * the Bluetooth module.
> - */
> + /* Configure no pull on 38 (CTS) to match Bluetooth module */
> pins = "gpio38";
> - bias-pull-down;
> - output-high;
> + bias-disable;
I think it should be ok in functional terms, but I don't like the rationale
and also doubt the change is really needed.
If the pull is removed to match the Bluetooth module, then that sounds as
if the signal was floating on the the BT side, which I think is not the case.
Yes, according to the datasheet there is no pull when the BT controller is
active, but then it drives the signal actively to either high or low. There
seems to be no merit in 'matching' the Bluetooth side in this case, if the
signal was really floating on the BT side we would definitely not want this.
In a reply to v2 you said:
> Recently on cherokee we worked with BT team and came to an agreement to
> keep no-pull from our side in order to not conflict with their pull in
> any state.
What are these conflicting pull states?
The WCN3998 datasheet has a pull-down on RTS (WCN3998 side) in reset and
boot mode, and no pull in active mode. In reset and boot mode the host
config with a pull down would match, and no pull in active mode doesn't
conflict with the pull-down on the host UART. My understanding is that
the pinconf pulls are weak pulls, so as soon as the BT chip drives its
RTS the pull on the host side shouldn't matter.
Is this change actually related with wakeup support? I have the impression
that multiple things are conflated in this patch. If some of the changes
are just fixing/improving other things they should be in a separate patch,
which could be part of this series, otherwise it's really hard to
distinguish between the pieces that are actually relevant for wakeup and
the rest.
Independently of whether the changes are done in a single or multiple
patches, the commit log should include details on why the changes are
necessary, especially when there are not explantatory comments in the
DT/code itself (e.g. the removal of 'output-high', which seems correct
to me, but no reason is given why it is done).
> };
>
> pinconf-rts {
> - /* We'll drive 39 (RTS), so no pull */
> + /*
> + * Configure pull-down on 39 (RTS). This is needed to avoid a
> + * floating pin which could mislead Bluetooth controller
> + * with UART RFR state (READY/NOT_READY).
> + */
> pins = "gpio39";
> drive-strength = <2>;
> - bias-disable;
> + bias-pull-down;
> };
[copy of my comment on v2]
I'm a bit at a loss here, about two things:
RTS is an output pin controlled by the UART. IIUC if the UART port is active
and hardware flow control is enabled the RTS signal is either driven to high
or low, but not floating.
Now lets assume I'm wrong with the above and RTS can be floating. We only want
the BT SoC to send data when the host UART is ready to receive them, right?
RTS is an active low signal, hence by configuring it as a pull-down the BT
SoC can send data regardless of whether the host UART actually asserts RTS,
so the host UART may not be ready to receive it. I would argue that if there
is really such a thing as a floating RTS signal then it should have a pull-up,
to prevent the BT SoC from sending data at any time.
I'm not an expert in UART communication and pinconf, so it could be that I
got something wrong, but as of now it seems to me that no pull is the correct
config for RTS.
>
> pinconf-tx {
> @@ -494,7 +494,43 @@
> pins = "gpio40";
> drive-strength = <2>;
> bias-disable;
> - output-high;
> + };
> +
> + pinconf-rx {
> + /*
> + * Configure a pull-up on 41 (RX). This is needed to avoid
> + * garbage data when the TX pin of the Bluetooth module is
> +
Re: [PATCH V3 1/3] arm64: dts: sc7180: Add wakeup support over UART RX
On Thu, Aug 20, 2020 at 07:21:05PM +0530, satya priya wrote:
> Add the necessary pinctrl and interrupts to make UART
> wakeup capable.
>
> Signed-off-by: satya priya
> Reviewed-by: Akash Asthana
> ---
> Changes in V2:
> - As per Matthias's comment added wakeup support for all the UARTs
>of SC7180.
>
> Changes in V3:
> - No change.
>
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 98
> ++--
> 1 file changed, 84 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index d46b383..855b13e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>
> ...
>
> + qup_uart0_sleep: qup-uart0-sleep {
> + pinmux {
> + pins = "gpio34", "gpio35",
> +"gpio36", "gpio37";
> + function = "gpio";
What is the reason that the GPIO function needs to be selected in sleep mode
to support wakeup?
This should be explained in the commit message unless it is evident.
Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Thu, Aug 20, 2020 at 05:49:53PM +0530, ska...@codeaurora.org wrote:
> Hi Matthias,
>
> On 2020-08-19 21:43, Matthias Kaehlcke wrote:
> > On Wed, Aug 19, 2020 at 07:19:25PM +0530, ska...@codeaurora.org wrote:
> > > On 2020-08-17 23:31, Matthias Kaehlcke wrote:
> > > > On Fri, Jul 24, 2020 at 09:28:01AM +0530, satya priya wrote:
> > > > > Add sleep pin ctrl for BT uart, and also change the bias
> > > > > configuration to match Bluetooth module.
> > > > >
> > > > > Signed-off-by: satya priya
> > > > > ---
> > > > > Changes in V2:
> > > > > - This patch adds sleep state for BT UART. Newly added in V2.
> > > > >
> > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 42
> > > > > -
> > > > > 1 file changed, 36 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > index 26cc491..bc919f2 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > > > @@ -469,20 +469,50 @@
> > > > >
> > > > > _uart3_default {
> > > > > pinconf-cts {
> > > > > - /*
> > > > > - * Configure a pull-down on 38 (CTS) to match the pull
> > > > > of
> > > > > - * the Bluetooth module.
> > > > > - */
> > > > > + /* Configure no pull on 38 (CTS) to match Bluetooth
> > > > > module */
> > > >
> > > > Has the pull from the Bluetooth module been removed or did the previous
> > > > config
> > > > incorrectly claim that the Bluetooth module has a pull-down?
> > > >
> > >
> > > The previous config was incorrect, so we corrected it to match the
> > > pull of
> > > BT.
> >
> > The pull config of the BT controller varies depending on its state,
> > could
> > you clarify which state you intend to match?
> >
>
> Since this line is driven by BT SoC, they could change their pull(although
> it's less likely). Recently on cherokee we worked with BT team and came to
> an agreement to keep no-pull from our side in order to not conflict with
> their pull in any state.
>
> > >
> > > > > pins = "gpio38";
> > > > > + bias-disable;
> > > > > + };
> > > > > +
> > > > > + pinconf-rts {
> > > > > + /* We'll drive 39 (RTS), so configure pull-down */
> > > > > + pins = "gpio39";
> > > > > + drive-strength = <2>;
> > > > > bias-pull-down;
> > > > > + };
> > > > > +
> > > > > + pinconf-tx {
> > > > > + /* We'll drive 40 (TX), so no pull */
> > > >
> > > > The rationales for RTS and TX contradict each other. According to the
> > > > comment
> > > > the reason to configure a pull-down on RTS is that it is driven by the
> > > > host.
> > > > Then for TX the reason to configure no pull is that it is driven by the
> > > > host.
> > > >
> > > > Please make sure the comments *really* describe the rationale, otherwise
> > > > they
> > > > are just confusing.
> > >
> > > The rationale for RTS is that we don't want it to be floating and
> > > want to
> > > make sure that it is pulled down, to receive bytes. Will modify the
> > > comment
> > > mentioning the same.
> >
> > Could you clarify what you mean with "to receive bytes"?
> >
>
> When we keep no-pull(floating), sometimes it may be pulled high and UART
> flow will be turned off(RFR_NOT_READY), due to this BT SoC wont be able to
> send data even though host is ready.
I'm a bit at a loss here, about two things:
RTS is an output pin controlled by the UART. IIUC if the UART port is active
and hardware flow control is enabled the RTS signal is either driven to high
or low, but not floating.
Now lets assume I'm wrong with the above and RTS can be floating. We only want
the BT SoC to send data when the host UART is ready to receive them, right?
RTS is an active low signal, hence by configuring it as a pull-down the BT
SoC can send data regardless of whether the host UART actually asserts RTS,
so the host UART may not be ready to receive it. I would argue that if there
is really such a thing as a floating RTS signal then it should have a pull-up,
to prevent the BT SoC from sending data at any time.
I'm not an expert in UART communication and pinconf, so it could be that I
got something wrong, but as of now it seems to me that no pull is the correct
config for RTS.
Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Wed, Aug 19, 2020 at 07:19:25PM +0530, ska...@codeaurora.org wrote:
> On 2020-08-17 23:31, Matthias Kaehlcke wrote:
> > On Fri, Jul 24, 2020 at 09:28:01AM +0530, satya priya wrote:
> > > Add sleep pin ctrl for BT uart, and also change the bias
> > > configuration to match Bluetooth module.
> > >
> > > Signed-off-by: satya priya
> > > ---
> > > Changes in V2:
> > > - This patch adds sleep state for BT UART. Newly added in V2.
> > >
> > > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 42
> > > -
> > > 1 file changed, 36 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > index 26cc491..bc919f2 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > > @@ -469,20 +469,50 @@
> > >
> > > _uart3_default {
> > > pinconf-cts {
> > > - /*
> > > - * Configure a pull-down on 38 (CTS) to match the pull of
> > > - * the Bluetooth module.
> > > - */
> > > + /* Configure no pull on 38 (CTS) to match Bluetooth module */
> >
> > Has the pull from the Bluetooth module been removed or did the previous
> > config
> > incorrectly claim that the Bluetooth module has a pull-down?
> >
>
> The previous config was incorrect, so we corrected it to match the pull of
> BT.
The pull config of the BT controller varies depending on its state, could
you clarify which state you intend to match?
>
> > > pins = "gpio38";
> > > + bias-disable;
> > > + };
> > > +
> > > + pinconf-rts {
> > > + /* We'll drive 39 (RTS), so configure pull-down */
> > > + pins = "gpio39";
> > > + drive-strength = <2>;
> > > bias-pull-down;
> > > + };
> > > +
> > > + pinconf-tx {
> > > + /* We'll drive 40 (TX), so no pull */
> >
> > The rationales for RTS and TX contradict each other. According to the
> > comment
> > the reason to configure a pull-down on RTS is that it is driven by the
> > host.
> > Then for TX the reason to configure no pull is that it is driven by the
> > host.
> >
> > Please make sure the comments *really* describe the rationale, otherwise
> > they
> > are just confusing.
>
> The rationale for RTS is that we don't want it to be floating and want to
> make sure that it is pulled down, to receive bytes. Will modify the comment
> mentioning the same.
Could you clarify what you mean with "to receive bytes"?
Thanks
Matthias
Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Mon, Aug 17, 2020 at 11:01:58AM -0700, Matthias Kaehlcke wrote:
> On Fri, Jul 24, 2020 at 09:28:01AM +0530, satya priya wrote:
> > Add sleep pin ctrl for BT uart, and also change the bias
> > configuration to match Bluetooth module.
> >
> > Signed-off-by: satya priya
> > ---
> > Changes in V2:
> > - This patch adds sleep state for BT UART. Newly added in V2.
> >
> > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 42
> > -
> > 1 file changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > index 26cc491..bc919f2 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> > @@ -469,20 +469,50 @@
> >
> > _uart3_default {
> > pinconf-cts {
> > - /*
> > -* Configure a pull-down on 38 (CTS) to match the pull of
> > -* the Bluetooth module.
> > -*/
> > + /* Configure no pull on 38 (CTS) to match Bluetooth module */
>
> Has the pull from the Bluetooth module been removed or did the previous config
> incorrectly claim that the Bluetooth module has a pull-down?
>
> > pins = "gpio38";
> > + bias-disable;
> > + };
> > +
> > + pinconf-rts {
> > + /* We'll drive 39 (RTS), so configure pull-down */
> > + pins = "gpio39";
> > + drive-strength = <2>;
> > bias-pull-down;
> > + };
> > +
> > + pinconf-tx {
> > + /* We'll drive 40 (TX), so no pull */
>
> The rationales for RTS and TX contradict each other. According to the comment
> the reason to configure a pull-down on RTS is that it is driven by the host.
> Then for TX the reason to configure no pull is that it is driven by the host.
>
> Please make sure the comments *really* describe the rationale, otherwise they
> are just confusing.
Ok, let's try to reason about the configurations.
I didn't find the datasheet for the WCN3991, but my understanding is that
it is an evolution of the WCN3998, so probably the states of the UART pins
are the same (signal names from the BT chip perspective):
active reset
CTSNP PD
RTSNP PD
RX NP PU
TX NP PD
Since this patch changes the DT let's use the signal names from the host side
in the following.
> RTS: NP => PD
I can see that this could make sense, a floating pin could indicate
the Bluetooth controller that the host is ready to receive data, when it is
not.
> CTS: PD => NP
>From a signalling perspective this should be no problem, since the WCN399x
has a pull-down on its RTS signal in reset, and otherwise will drive it.
IIUC there should be no power leakage without a pull, so I think this
should be ok.
> TX: +output-high
IIUC this only has an impact when the pin is in GPIO mode, i.e. in the sleep
config. If that's correct, does it even make sense to specify it in the default
config?
Besides that, what is the reason for this change? I was told in another forum
that Qualcomm found this to fix problems at UART initialization and wakeup,
without really understanding why. That's not great.
I'm no expert in this area, but my guess is that forcing the TX signal to high
in certain states is needed to not have it floating (no pull is configured),
which could generate garbage on the Bluetooth RX side. But is it really
necessary to actively drive it to high? Wouldn't it be enough to configure a
pull-up when it isn't actively driven (i.e. in sleep mode)?
In a quick test wakeup from Bluetooth worked when configuring a pull-up only in
sleep mode. Could you test this on your side or provide a rationale why TX needs
to be actively driven to high?
Thanks
Matthias
Re: [PATCH V2 2/3] arm64: dts: qcom: sc7180: Add sleep pin ctrl for BT uart
On Fri, Jul 24, 2020 at 09:28:01AM +0530, satya priya wrote:
> Add sleep pin ctrl for BT uart, and also change the bias
> configuration to match Bluetooth module.
>
> Signed-off-by: satya priya
> ---
> Changes in V2:
> - This patch adds sleep state for BT UART. Newly added in V2.
>
> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 42
> -
> 1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 26cc491..bc919f2 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -469,20 +469,50 @@
>
> _uart3_default {
> pinconf-cts {
> - /*
> - * Configure a pull-down on 38 (CTS) to match the pull of
> - * the Bluetooth module.
> - */
> + /* Configure no pull on 38 (CTS) to match Bluetooth module */
Has the pull from the Bluetooth module been removed or did the previous config
incorrectly claim that the Bluetooth module has a pull-down?
> pins = "gpio38";
> + bias-disable;
> + };
> +
> + pinconf-rts {
> + /* We'll drive 39 (RTS), so configure pull-down */
> + pins = "gpio39";
> + drive-strength = <2>;
> bias-pull-down;
> + };
> +
> + pinconf-tx {
> + /* We'll drive 40 (TX), so no pull */
The rationales for RTS and TX contradict each other. According to the comment
the reason to configure a pull-down on RTS is that it is driven by the host.
Then for TX the reason to configure no pull is that it is driven by the host.
Please make sure the comments *really* describe the rationale, otherwise they
are just confusing.
Re: [PATCH V2 3/3] tty: serial: qcom_geni_serial: Fix the UART wakeup issue
On Fri, Jul 24, 2020 at 09:28:02AM +0530, satya priya wrote: > As a part of system suspend we call uart_port_suspend from the > Serial driver, which calls set_mctrl passing mctrl as NULL. This > makes RFR high(NOT_READY) during suspend. > > Due to this BT SoC is not able to send wakeup bytes to UART during > suspend. Included if check for non-suspend case to keep RFR low > during suspend. > > Signed-off-by: satya priya > --- > Changes in V2: > - This patch fixes the UART flow control issue during suspend. >Newly added in V2. > > drivers/tty/serial/qcom_geni_serial.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c > b/drivers/tty/serial/qcom_geni_serial.c > index 07b7b6b..7108dfc 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -242,7 +242,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port > *uport, > if (mctrl & TIOCM_LOOP) > port->loopback = RX_TX_CTS_RTS_SORTED; > > - if (!(mctrl & TIOCM_RTS)) > + if ((!(mctrl & TIOCM_RTS)) && (!(uport->suspended))) Why all these parentheses, instead of: if (!(mctrl & TIOCM_RTS) && !uport->suspended) ?
[PATCH] arm64: dts: qcom: sc7180: Add 'sustainable_power' for CPU thermal zones
The 'sustainable_power' attribute provides an estimate of the sustained
power that can be dissipated at the desired control temperature. One
could argue that this value is not necessarily the same for all devices
with the same SoC, which may have different form factors or thermal
designs. However there are reasons to specify a (default) value at SoC
level for SC7180: most importantly, if no value is specified at all the
power_allocator thermal governor (aka 'IPA') estimates a value, using the
minimum power of all cooling devices of the zone, which can result in
overly aggressive thermal throttling. For most devices an approximate
conservative value should be more useful than the minimum guesstimate
of power_allocator. Devices that need a different value can overwrite
it in their .dts. Also the thermal zones for SC7180 have a high
level of granularity (essentially one for each function block), which
makes it more likely that the default value just works for many devices.
The values correspond to 1901 MHz for the big cores, and 1804 MHz for
the small cores. The values were determined by limiting the CPU
frequencies to different max values and launching a bunch of processes
that cause high CPU load ('while true; do true; done &' is simple and
does a good job). A frequency is deemed sustainable if the CPU
temperatures don't rise (consistently) above the second trip point
('control temperature', 95 degC in this case). Once the highest
sustainable frequency is found, the sustainable power can be calculated
by multiplying the energy consumption per core at this frequency (which
can be found in /sys/kernel/debug/energy_model/) with the number of
cores that are specified as cooling devices.
The sustainable frequencies were determined at room temperature
on a device without heat sink or other passive cooling elements.
Signed-off-by: Matthias Kaehlcke
---
If maintainers think 'sustainable_power' should be specified at
device level (with which I conceptually agree) I'm fine with
doing that, just seemed it could be useful to have a reasonable
'default' at SoC level in this case.
arch/arm64/boot/dts/qcom/sc7180.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index d46b3833e52f..23f84639d6b9 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3320,6 +3320,7 @@ cpu0-thermal {
polling-delay = <0>;
thermal-sensors = < 1>;
+ sustainable-power = <768>;
trips {
cpu0_alert0: trip-point0 {
@@ -3368,6 +3369,7 @@ cpu1-thermal {
polling-delay = <0>;
thermal-sensors = < 2>;
+ sustainable-power = <768>;
trips {
cpu1_alert0: trip-point0 {
@@ -3416,6 +3418,7 @@ cpu2-thermal {
polling-delay = <0>;
thermal-sensors = < 3>;
+ sustainable-power = <768>;
trips {
cpu2_alert0: trip-point0 {
@@ -3464,6 +3467,7 @@ cpu3-thermal {
polling-delay = <0>;
thermal-sensors = < 4>;
+ sustainable-power = <768>;
trips {
cpu3_alert0: trip-point0 {
@@ -3512,6 +3516,7 @@ cpu4-thermal {
polling-delay = <0>;
thermal-sensors = < 5>;
+ sustainable-power = <768>;
trips {
cpu4_alert0: trip-point0 {
@@ -3560,6 +3565,7 @@ cpu5-thermal {
polling-delay = <0>;
thermal-sensors = < 6>;
+ sustainable-power = <768>;
trips {
cpu5_alert0: trip-point0 {
@@ -3608,6 +3614,7 @@ cpu6-thermal {
polling-delay = <0>;
thermal-sensors = < 9>;
+ sustainable-power = <1202>;
trips {
cpu6_alert0: trip-point0 {
@@ -3648,6 +3655,7 @@ cpu7-thermal {
polling-delay = <0>;
thermal-sensors = < 10>;
+ sustainable-power = <1202>;
trips {
cpu7_alert0: trip-point0 {
@@ -3688,6 +3696,7 @@ cpu8-thermal {
polling-delay = <0>;
thermal-sensors = < 11>;
+ sustainable-power = <1202>;
Re: [PATCH v2 1/3] usb: dwc3: core: Host wake up support from system suspend
On Thu, Jul 09, 2020 at 12:40:15AM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown in host mode so that it can be wake up by devices.
> Added need_phy_for_wakeup flag to distinugush resume path and hs_phy_flags
> to check connection status and set phy mode and configure interrupts.
>
> Signed-off-by: Sandeep Maheswaram
> ---
> drivers/usb/dwc3/core.c | 47 ---
> drivers/usb/dwc3/core.h | 2 ++
> 2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 25c686a7..eb7c225 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -31,12 +31,14 @@
> #include
> #include
> #include
> +#include
>
> #include "core.h"
> #include "gadget.h"
> #include "io.h"
>
> #include "debug.h"
> +#include "../host/xhci.h"
>
> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
>
> @@ -1627,10 +1629,36 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> return ret;
> }
>
> +static void dwc3_set_phy_speed_flags(struct dwc3 *dwc)
> +{
> +
> + int i, num_ports;
> + u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> + struct xhci_hcd *xhci_hcd = hcd_to_xhci(hcd);
> +
> + dwc->hs_phy_flags &= ~(PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_LS);
> +
> + reg = readl(_hcd->cap_regs->hcs_params1);
> +
> + num_ports = HCS_MAX_PORTS(reg);
> + for (i = 0; i < num_ports; i++) {
> + reg = readl(_hcd->op_regs->port_status_base + i * 0x04);
> + if (reg & PORT_PE) {
> + if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg))
> + dwc->hs_phy_flags |= PHY_MODE_USB_HOST_HS;
> + else if (DEV_LOWSPEED(reg))
> + dwc->hs_phy_flags |= PHY_MODE_USB_HOST_LS;
> + }
> + }
> + phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_flags);
> +}
> +
> static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> {
> unsigned long flags;
> u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> @@ -1643,9 +1671,12 @@ static int dwc3_suspend_common(struct dwc3 *dwc,
> pm_message_t msg)
> dwc3_core_exit(dwc);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> + dwc3_set_phy_speed_flags(dwc);
> if (!PMSG_IS_AUTO(msg)) {
> - dwc3_core_exit(dwc);
> - break;
> + if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> + dwc->need_phy_for_wakeup = true;
> + else
> + dwc->need_phy_for_wakeup = false;
> }
>
> /* Let controller to suspend HSPHY before PHY driver suspends */
> @@ -1705,11 +1736,13 @@ static int dwc3_resume_common(struct dwc3 *dwc,
> pm_message_t msg)
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> if (!PMSG_IS_AUTO(msg)) {
> - ret = dwc3_core_init_for_resume(dwc);
> - if (ret)
> - return ret;
> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> - break;
> + if (!dwc->need_phy_for_wakeup) {
> + ret = dwc3_core_init_for_resume(dwc);
> + if (ret)
> + return ret;
> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> + break;
> + }
> }
> /* Restore GUSB2PHYCFG bits that were modified in suspend */
> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 013f42a..5367d510e 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1094,6 +1094,8 @@ struct dwc3 {
> struct phy *usb3_generic_phy;
>
> boolphys_ready;
> + boolneed_phy_for_wakeup;
> + unsigned inths_phy_flags;
>
> struct ulpi *ulpi;
> boolulpi_ready;
Should this include a check for the 'wakeup-source' DT attribute as in
xhci-mtk.c, to make wakeup support optional?
Re: [PATCH V2] arm64: dts: qcom: sc7180: Add bandwidth votes for eMMC and SDcard
On Wed, Aug 12, 2020 at 04:26:08PM +0530, sbh...@codeaurora.org wrote:
> On 2020-08-11 22:38, Matthias Kaehlcke wrote:
> > On Tue, Jul 28, 2020 at 04:49:05PM +0530, sbh...@codeaurora.org wrote:
> > > On 2020-07-28 00:40, Matthias Kaehlcke wrote:
> > > > Hi,
> > > >
> > > > On Mon, Jul 27, 2020 at 12:20:38PM +0530, sbh...@codeaurora.org wrote:
> > > > > On 2020-07-24 22:40, Matthias Kaehlcke wrote:
> > > > > > Hi Shaik,
> > > > > >
> > > > > > On Tue, Jul 21, 2020 at 04:16:21PM +0530, Shaik Sajida Bhanu wrote:
> > > > > > > From: Pradeep P V K
> > > > > > >
> > > > > > > Add the bandwidth domain supporting performance state and
> > > > > > > the corresponding OPP tables for the sdhc device on sc7180.
> > > > > > >
> > > > > > > Signed-off-by: Pradeep P V K
> > > > > > > Signed-off-by: Shaik Sajida Bhanu
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since V1:
> > > > > > > - Incorporated review comments by Bjorn Andersson.
> > > > > > > ---
> > > > > > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 15 +++
> > > > > > > 1 file changed, 15 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > > > b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > > > index 68f9894..d78a066 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > > > @@ -684,6 +684,9 @@
> > > > > > > clocks = < GCC_SDCC1_APPS_CLK>,
> > > > > > > < GCC_SDCC1_AHB_CLK>;
> > > > > > > clock-names = "core", "iface";
> > > > > > > + interconnects = <_noc MASTER_EMMC
> > > > > > > _virt SLAVE_EBI1>,
> > > > > > > + <_noc MASTER_APPSS_PROC _noc
> > > > > > > SLAVE_EMMC_CFG>;
> > > > > > > + interconnect-names = "sdhc-ddr","cpu-sdhc";
> > > > > > > power-domains = < SC7180_CX>;
> > > > > > > operating-points-v2 = <_opp_table>;
> > > > > > >
> > > > > > > @@ -704,11 +707,15 @@
> > > > > > > opp-1 {
> > > > > > > opp-hz = /bits/ 64 <1>;
> > > > > > > required-opps =
> > > > > > > <_opp_low_svs>;
> > > > > > > + opp-peak-kBps = <10 10>;
> > > > > > > + opp-avg-kBps = <10 5>;
> > > > > > > };
> > > > > > >
> > > > > > > opp-38400 {
> > > > > > > opp-hz = /bits/ 64 <38400>;
> > > > > > > required-opps =
> > > > > > > <_opp_svs_l1>;
> > > > > > > + opp-peak-kBps = <60 90>;
> > > > > > > + opp-avg-kBps = <261438 30>;
> > > > > > > };
> > > > > > > };
> > > > > > > };
> > > > > > > @@ -2476,6 +2483,10 @@
> > > > > > > clocks = < GCC_SDCC2_APPS_CLK>,
> > > > > > > < GCC_SDCC2_AHB_CLK>;
> > > > > > > clock-names = "core", "iface";
> > > > > > > +
> > > > > > > + interconnects = <_noc MASTER_SDCC_2
> > > > > > > _virt SLAVE_EBI1>,
> > > > > > > + <_noc MASTER_APPSS_PROC _noc
> > > > > > > SLAVE_SDCC_2>;
>
Re: [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early
On Mon, Aug 10, 2020 at 12:36:19PM +0530, Rajendra Nayak wrote:
> dev_pm_opp_set_rate() can now be called with freq = 0 inorder
> to either drop performance or bandwidth votes or to disable
> regulators on platforms which support them.
> In such cases, a subsequent call to dev_pm_opp_set_rate() with
> the same frequency ends up returning early because 'old_freq == freq'
> Instead make it fall through and put back the dropped performance
> and bandwidth votes and/or enable back the regulators.
>
> Fixes: cd7ea582 ("opp: Make dev_pm_opp_set_rate() handle freq = 0 to drop
> performance votes")
> Reported-by: Sajida Bhanu
> Signed-off-by: Rajendra Nayak
Tested-by: Matthias Kaehlcke
Originally-reported-by: Matthias Kaehlcke
https://patchwork.kernel.org/patch/11675369/#23514895 :P
Re: [PATCH V2] arm64: dts: qcom: sc7180: Add bandwidth votes for eMMC and SDcard
On Tue, Jul 28, 2020 at 04:49:05PM +0530, sbh...@codeaurora.org wrote:
> On 2020-07-28 00:40, Matthias Kaehlcke wrote:
> > Hi,
> >
> > On Mon, Jul 27, 2020 at 12:20:38PM +0530, sbh...@codeaurora.org wrote:
> > > On 2020-07-24 22:40, Matthias Kaehlcke wrote:
> > > > Hi Shaik,
> > > >
> > > > On Tue, Jul 21, 2020 at 04:16:21PM +0530, Shaik Sajida Bhanu wrote:
> > > > > From: Pradeep P V K
> > > > >
> > > > > Add the bandwidth domain supporting performance state and
> > > > > the corresponding OPP tables for the sdhc device on sc7180.
> > > > >
> > > > > Signed-off-by: Pradeep P V K
> > > > > Signed-off-by: Shaik Sajida Bhanu
> > > > > ---
> > > > >
> > > > > Changes since V1:
> > > > > - Incorporated review comments by Bjorn Andersson.
> > > > > ---
> > > > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 15 +++
> > > > > 1 file changed, 15 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > index 68f9894..d78a066 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > > @@ -684,6 +684,9 @@
> > > > > clocks = < GCC_SDCC1_APPS_CLK>,
> > > > > < GCC_SDCC1_AHB_CLK>;
> > > > > clock-names = "core", "iface";
> > > > > + interconnects = <_noc MASTER_EMMC
> > > > > _virt SLAVE_EBI1>,
> > > > > + <_noc MASTER_APPSS_PROC _noc
> > > > > SLAVE_EMMC_CFG>;
> > > > > + interconnect-names = "sdhc-ddr","cpu-sdhc";
> > > > > power-domains = < SC7180_CX>;
> > > > > operating-points-v2 = <_opp_table>;
> > > > >
> > > > > @@ -704,11 +707,15 @@
> > > > > opp-1 {
> > > > > opp-hz = /bits/ 64 <1>;
> > > > > required-opps =
> > > > > <_opp_low_svs>;
> > > > > + opp-peak-kBps = <10 10>;
> > > > > + opp-avg-kBps = <10 5>;
> > > > > };
> > > > >
> > > > > opp-38400 {
> > > > > opp-hz = /bits/ 64 <38400>;
> > > > > required-opps =
> > > > > <_opp_svs_l1>;
> > > > > + opp-peak-kBps = <60 90>;
> > > > > + opp-avg-kBps = <261438 30>;
> > > > > };
> > > > > };
> > > > > };
> > > > > @@ -2476,6 +2483,10 @@
> > > > > clocks = < GCC_SDCC2_APPS_CLK>,
> > > > > < GCC_SDCC2_AHB_CLK>;
> > > > > clock-names = "core", "iface";
> > > > > +
> > > > > + interconnects = <_noc MASTER_SDCC_2
> > > > > _virt SLAVE_EBI1>,
> > > > > + <_noc MASTER_APPSS_PROC _noc
> > > > > SLAVE_SDCC_2>;
> > > > > + interconnect-names = "sdhc-ddr","cpu-sdhc";
> > > > > power-domains = < SC7180_CX>;
> > > > > operating-points-v2 = <_opp_table>;
> > > > >
> > > > > @@ -2489,11 +2500,15 @@
> > > > > opp-1 {
> > > > > opp-hz = /bits/ 64 <1>;
> > > > > required-opps =
> > > > > <_opp_low_svs>;
> > > > > +
[PATCH] interconnect: Show bandwidth for disabled paths as zero in debugfs
For disabled paths the 'interconnect_summary' in debugfs currently shows
the orginally requested bandwidths. This is confusing, since the bandwidth
requests aren't active. Instead show the bandwidths for disabled
paths/requests as zero.
Signed-off-by: Matthias Kaehlcke
---
drivers/interconnect/core.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 609e206bf598..3491175304d1 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -55,12 +55,18 @@ static int icc_summary_show(struct seq_file *s, void *data)
icc_summary_show_one(s, n);
hlist_for_each_entry(r, >req_list, req_node) {
+ u32 avg_bw = 0, peak_bw = 0;
+
if (!r->dev)
continue;
+ if (r->enabled) {
+ avg_bw = r->avg_bw;
+ peak_bw = r->peak_bw;
+ }
+
seq_printf(s, " %-27s %12u %12u %12u\n",
- dev_name(r->dev), r->tag, r->avg_bw,
- r->peak_bw);
+ dev_name(r->dev), r->tag, avg_bw,
peak_bw);
}
}
}
--
2.28.0.rc0.142.g3c755180ce-goog
Re: [PATCH 6/6] arm64: dts: qcom: sc7180: Increase the number of interconnect cells
On Thu, Jul 23, 2020 at 04:09:42PM +0300, Georgi Djakov wrote: > From: Sibi Sankar > > Increase the number of interconnect-cells, as now we can include > the tag information. The consumers can specify the path tag as an > additional argument to the endpoints. > > Signed-off-by: Sibi Sankar > Signed-off-by: Georgi Djakov Tested-by: Matthias Kaehlcke Reviewed-by: Matthias Kaehlcke
Re: [PATCH 5/6] interconnect: qcom: sc7180: Replace xlate with xlate_extended
On Thu, Jul 23, 2020 at 04:09:41PM +0300, Georgi Djakov wrote: > From: Sibi Sankar > > Use the qcom_icc_xlate_extended() in order to parse tags, that are > specified as an additional arguments to the path endpoints in DT. > > Signed-off-by: Sibi Sankar > Signed-off-by: Georgi Djakov Tested-by: Matthias Kaehlcke Reviewed-by: Matthias Kaehlcke
Re: [PATCH 4/6] arm64: dts: qcom: sdm845: Increase the number of interconnect cells
On Mon, Jul 27, 2020 at 04:28:35PM +0530, Sibi Sankar wrote: > On 2020-07-23 18:39, Georgi Djakov wrote: > > Increase the number of interconnect-cells, as now we can include > > the tag information. The consumers can specify the path tag as an > > additional argument to the endpoints. > > Tested-by: Sibi Sankar > Reviewed-by: Sibi Sankar > > https://patchwork.kernel.org/patch/11655409/ > I'll replace the tag ids with the > macros once ^^ lands. Great, I was going to ask about that :) Reviewed-by: Matthias Kaehlcke
Re: [PATCH 3/6] interconnect: qcom: sdm845: Replace xlate with xlate_extended
On Thu, Jul 23, 2020 at 04:09:39PM +0300, Georgi Djakov wrote: > Use the qcom_icc_xlate_extended() in order to parse tags, that are > specified as an additional arguments to the path endpoints in DT. > > Signed-off-by: Georgi Djakov Reviewed-by: Matthias Kaehlcke
Re: [PATCH 2/6] interconnect: qcom: Implement xlate_extended() to parse tags
On Thu, Jul 23, 2020 at 04:09:38PM +0300, Georgi Djakov wrote:
> Implement a function to parse the arguments of the "interconnects" DT
> property and populate the interconnect path tags if this information
> is available.
>
> Signed-off-by: Georgi Djakov
> ---
> drivers/interconnect/qcom/icc-rpmh.c | 27 +++
> drivers/interconnect/qcom/icc-rpmh.h | 1 +
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c
> b/drivers/interconnect/qcom/icc-rpmh.c
> index 3ac5182c9ab2..44144fabec32 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -6,6 +6,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include "bcm-voter.h"
> #include "icc-rpmh.h"
> @@ -92,6 +94,31 @@ int qcom_icc_set(struct icc_node *src, struct icc_node
> *dst)
> }
> EXPORT_SYMBOL_GPL(qcom_icc_set);
>
> +struct icc_node_data *qcom_icc_xlate_extended(struct of_phandle_args *spec,
> void *data)
> +{
> + struct icc_node_data *ndata;
> + struct icc_node *node;
> +
> + if (!spec)
> + return ERR_PTR(-EINVAL);
> +
> + node = of_icc_xlate_onecell(spec, data);
> + if (IS_ERR(node))
> + return ERR_CAST(node);
> +
> + ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
> + if (!ndata)
> + return ERR_PTR(-ENOMEM);
> +
> + ndata->node = node;
> +
> + if (spec->args_count == 2)
> + ndata->tag = spec->args[1];
Would a higher number of arguments (currently) be an error? Is so,
should it be reported?
Re: [PATCH 1/6] interconnect: Introduce xlate_extended() callback
On Thu, Jul 23, 2020 at 04:09:37PM +0300, Georgi Djakov wrote: > Currently there is the xlate() callback, which is provider-specific is > used for mapping the nodes from phandle arguments. This is fine for simple > mappings, but the phandle arguments could contain an additional data, such > as tag information. Let's create another callback xlate_extended() for the > cases where providers want also populate the tagging data. > > Signed-off-by: Georgi Djakov Reviewed-by: Matthias Kaehlcke Tested-by: Matthias Kaehlcke
Re: [PATCH v11 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Mon, Jul 27, 2020 at 10:36:36PM +0530, Sandeep Maheswaram wrote: > Add interconnect support in dwc3-qcom driver to vote for bus > bandwidth. > > This requires for two different paths - from USB to > DDR. The other is from APPS to USB. > > Signed-off-by: Sandeep Maheswaram > Signed-off-by: Chandana Kishori Chiluveru Reviewed-by: Matthias Kaehlcke
Re: [PATCH v10 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Mon, Jul 27, 2020 at 12:17:19PM -0700, Matthias Kaehlcke wrote: > On Thu, Jul 23, 2020 at 11:57:36PM +0530, Sandeep Maheswaram wrote: > > Add interconnect support in dwc3-qcom driver to vote for bus > > bandwidth. > > > > This requires for two different paths - from USB to > > DDR. The other is from APPS to USB. > > > > Signed-off-by: Sandeep Maheswaram > > Signed-off-by: Chandana Kishori Chiluveru > > Reviewed-by: Matthias Kaehlcke Sorry, that was meant for v11
Re: [PATCH v10 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Thu, Jul 23, 2020 at 11:57:36PM +0530, Sandeep Maheswaram wrote: > Add interconnect support in dwc3-qcom driver to vote for bus > bandwidth. > > This requires for two different paths - from USB to > DDR. The other is from APPS to USB. > > Signed-off-by: Sandeep Maheswaram > Signed-off-by: Chandana Kishori Chiluveru Reviewed-by: Matthias Kaehlcke
Re: [PATCH V2] arm64: dts: qcom: sc7180: Add bandwidth votes for eMMC and SDcard
Hi,
On Mon, Jul 27, 2020 at 12:20:38PM +0530, sbh...@codeaurora.org wrote:
> On 2020-07-24 22:40, Matthias Kaehlcke wrote:
> > Hi Shaik,
> >
> > On Tue, Jul 21, 2020 at 04:16:21PM +0530, Shaik Sajida Bhanu wrote:
> > > From: Pradeep P V K
> > >
> > > Add the bandwidth domain supporting performance state and
> > > the corresponding OPP tables for the sdhc device on sc7180.
> > >
> > > Signed-off-by: Pradeep P V K
> > > Signed-off-by: Shaik Sajida Bhanu
> > > ---
> > >
> > > Changes since V1:
> > > - Incorporated review comments by Bjorn Andersson.
> > > ---
> > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > index 68f9894..d78a066 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > @@ -684,6 +684,9 @@
> > > clocks = < GCC_SDCC1_APPS_CLK>,
> > > < GCC_SDCC1_AHB_CLK>;
> > > clock-names = "core", "iface";
> > > + interconnects = <_noc MASTER_EMMC _virt
> > > SLAVE_EBI1>,
> > > + <_noc MASTER_APPSS_PROC _noc
> > > SLAVE_EMMC_CFG>;
> > > + interconnect-names = "sdhc-ddr","cpu-sdhc";
> > > power-domains = < SC7180_CX>;
> > > operating-points-v2 = <_opp_table>;
> > >
> > > @@ -704,11 +707,15 @@
> > > opp-1 {
> > > opp-hz = /bits/ 64 <1>;
> > > required-opps = <_opp_low_svs>;
> > > + opp-peak-kBps = <10 10>;
> > > + opp-avg-kBps = <10 5>;
> > > };
> > >
> > > opp-38400 {
> > > opp-hz = /bits/ 64 <38400>;
> > > required-opps = <_opp_svs_l1>;
> > > + opp-peak-kBps = <60 90>;
> > > + opp-avg-kBps = <261438 30>;
> > > };
> > > };
> > > };
> > > @@ -2476,6 +2483,10 @@
> > > clocks = < GCC_SDCC2_APPS_CLK>,
> > > < GCC_SDCC2_AHB_CLK>;
> > > clock-names = "core", "iface";
> > > +
> > > + interconnects = <_noc MASTER_SDCC_2 _virt
> > > SLAVE_EBI1>,
> > > + <_noc MASTER_APPSS_PROC _noc
> > > SLAVE_SDCC_2>;
> > > + interconnect-names = "sdhc-ddr","cpu-sdhc";
> > > power-domains = < SC7180_CX>;
> > > operating-points-v2 = <_opp_table>;
> > >
> > > @@ -2489,11 +2500,15 @@
> > > opp-1 {
> > > opp-hz = /bits/ 64 <1>;
> > > required-opps = <_opp_low_svs>;
> > > + opp-peak-kBps = <16 10>;
> > > + opp-avg-kBps = <8 5>;
> > > };
> > >
> > > opp-20200 {
> > > opp-hz = /bits/ 64 <20200>;
> > > required-opps = <_opp_svs_l1>;
> > > + opp-peak-kBps = <20 12>;
> > > + opp-avg-kBps = <10 6>;
> > > };
> > > };
> > > };
> >
> > Does the sdhci-msm driver actually have BW scaling support at this
> > point?
> >
>
> yes
>
> > There is commit 4ece9795be56 ("mmc: sdhci-msm: Add interconnect
> > bandwidth scaling support"), whose commit message says "make sure
> > interconnect driver is
Re: [PATCH V2] arm64: dts: qcom: sc7180: Add bandwidth votes for eMMC and SDcard
Hi Shaik,
On Tue, Jul 21, 2020 at 04:16:21PM +0530, Shaik Sajida Bhanu wrote:
> From: Pradeep P V K
>
> Add the bandwidth domain supporting performance state and
> the corresponding OPP tables for the sdhc device on sc7180.
>
> Signed-off-by: Pradeep P V K
> Signed-off-by: Shaik Sajida Bhanu
> ---
>
> Changes since V1:
> - Incorporated review comments by Bjorn Andersson.
> ---
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 68f9894..d78a066 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -684,6 +684,9 @@
> clocks = < GCC_SDCC1_APPS_CLK>,
> < GCC_SDCC1_AHB_CLK>;
> clock-names = "core", "iface";
> + interconnects = <_noc MASTER_EMMC _virt
> SLAVE_EBI1>,
> + <_noc MASTER_APPSS_PROC _noc
> SLAVE_EMMC_CFG>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> power-domains = < SC7180_CX>;
> operating-points-v2 = <_opp_table>;
>
> @@ -704,11 +707,15 @@
> opp-1 {
> opp-hz = /bits/ 64 <1>;
> required-opps = <_opp_low_svs>;
> + opp-peak-kBps = <10 10>;
> + opp-avg-kBps = <10 5>;
> };
>
> opp-38400 {
> opp-hz = /bits/ 64 <38400>;
> required-opps = <_opp_svs_l1>;
> + opp-peak-kBps = <60 90>;
> + opp-avg-kBps = <261438 30>;
> };
> };
> };
> @@ -2476,6 +2483,10 @@
> clocks = < GCC_SDCC2_APPS_CLK>,
> < GCC_SDCC2_AHB_CLK>;
> clock-names = "core", "iface";
> +
> + interconnects = <_noc MASTER_SDCC_2 _virt
> SLAVE_EBI1>,
> + <_noc MASTER_APPSS_PROC _noc
> SLAVE_SDCC_2>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> power-domains = < SC7180_CX>;
> operating-points-v2 = <_opp_table>;
>
> @@ -2489,11 +2500,15 @@
> opp-1 {
> opp-hz = /bits/ 64 <1>;
> required-opps = <_opp_low_svs>;
> + opp-peak-kBps = <16 10>;
> + opp-avg-kBps = <8 5>;
> };
>
> opp-20200 {
> opp-hz = /bits/ 64 <20200>;
> required-opps = <_opp_svs_l1>;
> + opp-peak-kBps = <20 12>;
> + opp-avg-kBps = <10 6>;
> };
> };
> };
Does the sdhci-msm driver actually have BW scaling support at this point?
There is commit 4ece9795be56 ("mmc: sdhci-msm: Add interconnect
bandwidth scaling support"), whose commit message says "make sure
interconnect driver is ready before handling interconnect scaling.".
I haven't seen any patch adding the scaling support (supposedly by
adding dev_pm_opp_set_bw() calls?). Did I miss it? If not it seems
it would make sense to post it in a series together with this patch,
as far as I can tell this patch alone does nothing in practical terms.
grep sdhc /sys/kernel/debug/interconnect/interconnect_summary
8804000.sdhci 000
7c4000.sdhci 000
7c4000.sdhci 000
8804000.sdhci 000
...
Re: [PATCH v10 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi Sandeep,
On Thu, Jul 23, 2020 at 11:57:36PM +0530, Sandeep Maheswaram wrote:
> Add interconnect support in dwc3-qcom driver to vote for bus
> bandwidth.
>
> This requires for two different paths - from USB to
> DDR. The other is from APPS to USB.
>
> Signed-off-by: Sandeep Maheswaram
> Signed-off-by: Chandana Kishori Chiluveru
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 127
> ++-
> 1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index e1e78e9..712efb7 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -43,6 +44,14 @@
> #define SDM845_QSCRATCH_SIZE 0x400
> #define SDM845_DWC3_CORE_SIZE0xcd00
>
> +/* Interconnect path bandwidths in MBps */
> +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240)
> +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700)
> +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000)
> +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500)
> +#define APPS_USB_AVG_BW 0
> +#define APPS_USB_PEAK_BW MBps_to_icc(40)
> +
> struct dwc3_acpi_pdata {
> u32 qscratch_base_offset;
> u32 qscratch_base_size;
> @@ -76,6 +85,8 @@ struct dwc3_qcom {
> enum usb_dr_modemode;
> boolis_suspended;
> boolpm_suspended;
> + struct icc_path *icc_path_ddr;
> + struct icc_path *icc_path_apps;
> };
>
> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> @@ -190,6 +201,103 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom
> *qcom)
> return 0;
> }
>
> +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom)
> +{
> + int ret;
> +
> + ret = icc_enable(qcom->icc_path_ddr);
> + if (ret)
> + return ret;
> +
> + ret = icc_enable(qcom->icc_path_apps);
> + if (ret)
> + return icc_disable(qcom->icc_path_ddr);
You are returning the result of icc_disable(), but it should be the
previous error. Just do
icc_disable(qcom->icc_path_ddr);
and use the below statement for returning (if not it should be 'return 0').
> +
> + return ret;
> +}
> +
> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom)
> +{
> + int ret;
> +
> + ret = icc_disable(qcom->icc_path_ddr);
> + if (ret)
> + return ret;
> +
> + ret = icc_disable(qcom->icc_path_apps);
> + if (ret)
> + goto err_reenable_memory_path;
Please make the error handling in _enable() and _disable() symmetrical, either
call icc_enable/disable() directly or use a goto in both functions (IMO the goto
is not needed in this case, it makes the code more complex rather than
simplifying it).
> +
> + return 0;
> +
> + /* Re-enable things in the event of an error */
> +err_reenable_memory_path:
> + dwc3_qcom_interconnect_enable(qcom);
Why this function which disables both paths and not just
icc_enable(qcom->icc_path_ddr), analogous to dwc3_qcom_interconnect_enable()?
> +
> + return ret;
> +}
> +
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> + * and set bandwidhth.
> + * @qcom:Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + int ret;
> +
> + qcom->icc_path_ddr = of_icc_get(dev, "usb-ddr");
> + if (IS_ERR(qcom->icc_path_ddr)) {
> + dev_err(dev, "failed to get usb-ddr path: %ld\n",
> + PTR_ERR(qcom->icc_path_ddr));
> + return PTR_ERR(qcom->icc_path_ddr);
> + }
> +
> + qcom->icc_path_apps = of_icc_get(dev, "apps-usb");
> + if (IS_ERR(qcom->icc_path_apps)) {
> + dev_err(dev, "failed to get apps-usb path: %ld\n",
> + PTR_ERR(qcom->icc_path_apps));
> + return PTR_ERR(qcom->icc_path_apps);
> + }
> +
> + if (usb_get_maximum_speed(>dwc3->dev) >= USB_SPEED_SUPER ||
> + usb_get_maximum_speed(>dwc3->dev) ==
> USB_SPEED_UNKNOWN)
> + ret = icc_set_bw(qcom->icc_path_ddr,
> + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> + else
> + ret = icc_set_bw(qcom->icc_path_ddr,
> + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
> +
> + if (ret) {
> + dev_err(dev, "failed to set bandwidth for usb-ddr path: %d\n",
> ret);
> + return ret;
> + }
> +
> + ret = icc_set_bw(qcom->icc_path_apps,
> + APPS_USB_AVG_BW, APPS_USB_PEAK_BW);
> +
nit: remove empty line, the call and the if block belong together.
> + if (ret) {
> + dev_err(dev, "failed to set bandwidth for
Re: [PATCH v2 2/3] usb: dwc3: qcom: Configure wakeup interrupts and set genpd active wakeup flag
Hi Sandeep,
On Thu, Jul 09, 2020 at 12:40:16AM +0530, Sandeep Maheswaram wrote:
> configure interrupts based on hs_phy_flag. Set genpd active wakeup flag
> for usb gdsc if wakeup capable devices are connected.
as Stephen remarked, please describe why this patch is doing what
it is doing.
> Signed-off-by: Sandeep Maheswaram
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 73
> ++--
> 1 file changed, 57 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 1dfd024..8902670 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -16,9 +16,11 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
>
> #include "core.h"
>
> @@ -192,21 +194,34 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom
> *qcom)
>
> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> {
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +
> if (qcom->hs_phy_irq) {
> disable_irq_wake(qcom->hs_phy_irq);
> disable_irq_nosync(qcom->hs_phy_irq);
> }
nit: add empty line
> + if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
> + if (qcom->dp_hs_phy_irq) {
> + disable_irq_wake(qcom->dp_hs_phy_irq);
> + disable_irq_nosync(qcom->dp_hs_phy_irq);
> + }
> + } else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
> + if (qcom->dm_hs_phy_irq) {
> + disable_irq_wake(qcom->dm_hs_phy_irq);
> + disable_irq_nosync(qcom->dm_hs_phy_irq);
> + }
> + } else {
>
delete empty line
> - if (qcom->dp_hs_phy_irq) {
> - disable_irq_wake(qcom->dp_hs_phy_irq);
> - disable_irq_nosync(qcom->dp_hs_phy_irq);
> - }
> + if (qcom->dp_hs_phy_irq) {
> + disable_irq_wake(qcom->dp_hs_phy_irq);
> + disable_irq_nosync(qcom->dp_hs_phy_irq);
> + }
>
> - if (qcom->dm_hs_phy_irq) {
> - disable_irq_wake(qcom->dm_hs_phy_irq);
> - disable_irq_nosync(qcom->dm_hs_phy_irq);
> + if (qcom->dm_hs_phy_irq) {
> + disable_irq_wake(qcom->dm_hs_phy_irq);
> + disable_irq_nosync(qcom->dm_hs_phy_irq);
> + }
> }
> -
> if (qcom->ss_phy_irq) {
> disable_irq_wake(qcom->ss_phy_irq);
> disable_irq_nosync(qcom->ss_phy_irq);
> @@ -215,21 +230,34 @@ static void dwc3_qcom_disable_interrupts(struct
> dwc3_qcom *qcom)
>
> static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> {
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +
> if (qcom->hs_phy_irq) {
> enable_irq(qcom->hs_phy_irq);
> enable_irq_wake(qcom->hs_phy_irq);
> }
nit: add empty line
> + if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
> + if (qcom->dp_hs_phy_irq) {
> + enable_irq(qcom->dp_hs_phy_irq);
> + enable_irq_wake(qcom->dp_hs_phy_irq);
> + }
> + } else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
> + if (qcom->dm_hs_phy_irq) {
> + enable_irq(qcom->dm_hs_phy_irq);
> + enable_irq_wake(qcom->dm_hs_phy_irq);
> + }
> + } else {
>
delete empty line
> - if (qcom->dp_hs_phy_irq) {
> - enable_irq(qcom->dp_hs_phy_irq);
> - enable_irq_wake(qcom->dp_hs_phy_irq);
> - }
> + if (qcom->dp_hs_phy_irq) {
> + enable_irq(qcom->dp_hs_phy_irq);
> + enable_irq_wake(qcom->dp_hs_phy_irq);
> + }
>
> - if (qcom->dm_hs_phy_irq) {
> - enable_irq(qcom->dm_hs_phy_irq);
> - enable_irq_wake(qcom->dm_hs_phy_irq);
> + if (qcom->dm_hs_phy_irq) {
> + enable_irq(qcom->dm_hs_phy_irq);
> + enable_irq_wake(qcom->dm_hs_phy_irq);
> + }
> }
> -
> if (qcom->ss_phy_irq) {
> enable_irq(qcom->ss_phy_irq);
> enable_irq_wake(qcom->ss_phy_irq);
> @@ -240,6 +268,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> {
> u32 val;
> int i;
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> + struct generic_pm_domain *genpd;
> +
> + genpd = pd_to_genpd(qcom->dev->pm_domain);
nit: assign in declaration?
> +
> + if (genpd && usb_wakeup_enabled_descendants(hcd->self.root_hub))
> + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>
> if (qcom->is_suspended)
> return 0;
> @@ -261,6 +297,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> {
> int ret;
> int i;
> + struct generic_pm_domain
Re: [PATCH v2 1/3] usb: dwc3: core: Host wake up support from system suspend
Hi Sandeep,
On Thu, Jul 09, 2020 at 12:40:15AM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown in host mode so that it can be wake up by devices.
> Added need_phy_for_wakeup flag to distinugush resume path and hs_phy_flags
> to check connection status and set phy mode and configure interrupts.
>
> Signed-off-by: Sandeep Maheswaram
> ---
> drivers/usb/dwc3/core.c | 47 ---
> drivers/usb/dwc3/core.h | 2 ++
> 2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 25c686a7..eb7c225 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -31,12 +31,14 @@
> #include
> #include
> #include
> +#include
>
> #include "core.h"
> #include "gadget.h"
> #include "io.h"
>
> #include "debug.h"
> +#include "../host/xhci.h"
>
> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
>
> @@ -1627,10 +1629,36 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> return ret;
> }
>
> +static void dwc3_set_phy_speed_flags(struct dwc3 *dwc)
> +{
> +
> + int i, num_ports;
> + u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> + struct xhci_hcd *xhci_hcd = hcd_to_xhci(hcd);
> +
> + dwc->hs_phy_flags &= ~(PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_LS);
Where is hs_phy_flags initialized? As far as I can tell it isn't, hence when
dwc3_set_phy_speed_flags() is executed the first time it is 0 (from
devm_kzalloc()), and after the '&=' it is still 0. The next time it will have
whatever value it was set to in the below loop, which is then cleared by
the '&='. It seems you could as well just write 'dwc->hs_phy_flags = 0',
which is clearer, unless the field is used in some other way that isn't
obvious to me.
> +
> + reg = readl(_hcd->cap_regs->hcs_params1);
> +
> + num_ports = HCS_MAX_PORTS(reg);
> + for (i = 0; i < num_ports; i++) {
> + reg = readl(_hcd->op_regs->port_status_base + i * 0x04);
> + if (reg & PORT_PE) {
> + if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg))
> + dwc->hs_phy_flags |= PHY_MODE_USB_HOST_HS;
> + else if (DEV_LOWSPEED(reg))
> + dwc->hs_phy_flags |= PHY_MODE_USB_HOST_LS;
Is another entry for DEV_SUPERSPEED needed?
> + }
> + }
> + phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_flags);
> +}
> +
> static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> {
> unsigned long flags;
> u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> @@ -1643,9 +1671,12 @@ static int dwc3_suspend_common(struct dwc3 *dwc,
> pm_message_t msg)
> dwc3_core_exit(dwc);
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> + dwc3_set_phy_speed_flags(dwc);
> if (!PMSG_IS_AUTO(msg)) {
> - dwc3_core_exit(dwc);
> - break;
> + if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> + dwc->need_phy_for_wakeup = true;
> + else
> + dwc->need_phy_for_wakeup = false;
> }
>
> /* Let controller to suspend HSPHY before PHY driver suspends */
> @@ -1705,11 +1736,13 @@ static int dwc3_resume_common(struct dwc3 *dwc,
> pm_message_t msg)
> break;
> case DWC3_GCTL_PRTCAP_HOST:
> if (!PMSG_IS_AUTO(msg)) {
> - ret = dwc3_core_init_for_resume(dwc);
> - if (ret)
> - return ret;
> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST);
> - break;
> + if (!dwc->need_phy_for_wakeup) {
> + ret = dwc3_core_init_for_resume(dwc);
Before this patch we had the combo dwc3_core_exit() /
dwc3_core_init_for_resume(),
now it is only dwc3_core_init_for_resume() for !dwc->need_phy_for_wakeup.
Doesn't this cause trouble with enable counts, e.g. with
clk_bulk_prepare_enable()
being called in dwc3_core_init_for_resume(), without the corresponding
clk_bulk_disable_unprepare() calls in dwc3_core_exit()?
Re: [PATCH v9 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi Sandeep,
On Tue, Jul 21, 2020 at 01:14:48PM +0530, Sandeep Maheswaram wrote:
> Add interconnect support in dwc3-qcom driver to vote for bus
> bandwidth.
>
> This requires for two different paths - from USB to
> DDR. The other is from APPS to USB.
>
> Signed-off-by: Sandeep Maheswaram
> Signed-off-by: Chandana Kishori Chiluveru
> Reviewed-by: Matthias Kaehlcke
^
Please remove this tag for now (should have requested this earlier). It
seems we are very close, but apparently the review is still/again ongoing.
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 127
> ++-
> 1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index e1e78e9..82e08ff 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -43,6 +44,14 @@
> #define SDM845_QSCRATCH_SIZE 0x400
> #define SDM845_DWC3_CORE_SIZE0xcd00
>
> +/* Interconnect path bandwidths in MBps */
> +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240)
> +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700)
> +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000)
> +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500)
> +#define APPS_USB_AVG_BW 0
> +#define APPS_USB_PEAK_BW MBps_to_icc(40)
> +
> struct dwc3_acpi_pdata {
> u32 qscratch_base_offset;
> u32 qscratch_base_size;
> @@ -76,6 +85,8 @@ struct dwc3_qcom {
> enum usb_dr_modemode;
> boolis_suspended;
> boolpm_suspended;
> + struct icc_path *icc_path_ddr;
> + struct icc_path *icc_path_apps;
> };
>
> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> @@ -190,6 +201,101 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom
> *qcom)
> return 0;
> }
>
> +/* Bandwidth levels are set in dwc3_qcom_interconnect_init ,
> + * so just "enable" interconnects.
> + */
nits:
- the common format for functions in this file is:
/**
*
*/
- remove blank before the ','
That said, not all functions in this file have documentation, and this
comment doesn't seem to add much, so you could consider to just remove
it.
> +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom)
> +{
> + int ret;
> +
> + ret = icc_enable(qcom->icc_path_ddr);
> + if (ret)
> + return ret;
> +
> + ret = icc_enable(qcom->icc_path_apps);
> + if (ret)
> + return ret;
You changed the logic here: in v8 the DDR path would be disabled again
if enabling the APPS patch failed. I think it would be preferable to
keep the enable state of the two ICC paths in sync if possible.
If not, just do
return icc_enable(qcom->icc_path_apps);
instead of
if (ret)
return ret;
return ret;
> +
> + return ret;
> +}
> +
> +/*Disabling the interconnect, will set the bandwidth to 0 */
nit: add blank before 'Disabling'.
Same as above, the comment doesn't add much value, you can consider
to remove it.
> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom)
> +{
> + int ret;
> +
> + ret = icc_disable(qcom->icc_path_ddr);
> + if (ret)
> + return ret;
> +
> + ret = icc_disable(qcom->icc_path_apps);
> + if (ret)
> + return ret;
Same as above, either reenable the DDR path in case of failure, or simplify
the code.
> +
> + return ret;
> +}
> +
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> + * and set bandwidhth.
> + * @qcom:Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + int ret;
> +
> + qcom->icc_path_ddr = of_icc_get(dev, "usb-ddr");
> + if (IS_ERR(qcom->icc_path_ddr)) {
> + dev_err(dev, "failed to get usb-ddr path: %ld\n",
> + PTR_ERR(qcom->icc_path_ddr));
> + return PTR_ERR(qcom->icc_path_ddr);
> + }
> +
> + qcom->icc_path_apps = of_icc_get(dev, "apps-usb");
> + if (IS_ERR(qcom->icc_path_apps)) {
> + dev_err(dev, "failed to get apps-usb path: %ld\n",
> + PTR_ERR(qcom->icc_path_apps));
> + return PTR_ERR(qcom->icc_path_apps);
> + }
&g
Re: [PATCH] soc: qcom: geni: Fix NULL pointer dereference
Please make sure to cc the linux-arm-msm@vger list for patches of
Qualcomm code.
On Fri, Jul 17, 2020 at 08:02:22PM +0530, Akash Asthana wrote:
> pdev struct doesn't exits for the devices whose status are disabled
s/exits/exist/
> from DT node, in such cases NULL is returned from 'of_find_device_by_node'
> Later when we try to get drvdata from pdev struct NULL pointer dereference
> is triggered.
>
> Add a NULL check for return values to fix the issue.
>
> We were hitting this issue when one of QUP is disabled.
>
> Fixes: 048eb908a1f2 ("soc: qcom-geni-se: Add interconnect support to fix
> earlycon crash")
> Reported-by: Sai Prakash Ranjan
> Signed-off-by: Akash Asthana
> ---
> drivers/soc/qcom/qcom-geni-se.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 355d503..6e5fe65 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -820,6 +820,7 @@ void geni_remove_earlycon_icc_vote(void)
> struct geni_wrapper *wrapper;
> struct device_node *parent;
> struct device_node *child;
> + struct platform_device *wrapper_pdev;
nit: since there is no other 'pdev' in this function you could just name
it 'pdev', which is less clunky. The variable is only used immediately
after it is assigned, so it's clear from the context that it refers to
the 'wrapper'.
Reviewed-by: Matthias Kaehlcke
Re: [PATCH] drm: msm: a6xx: fix gpu failure after system resume
On Tue, Jul 14, 2020 at 06:55:30PM +0530, Akhil P Oommen wrote: > On targets where GMU is available, GMU takes over the ownership of GX GDSC > during its initialization. So, take a refcount on the GX PD on behalf of > GMU before we initialize it. This makes sure that nobody can collapse the > GX GDSC once GMU owns the GX GDSC. This patch fixes some weird failures > during GPU wake up during system resume. > > Signed-off-by: Akhil P Oommen I went through a few dozen suspend/resume cycles on SC7180 and didn't run into the kernel panic that typically occurs after a few iterations without this patch. Reported-by: Matthias Kaehlcke Tested-by: Matthias Kaehlcke On which tree is this patch based on? I had to apply it manually because 'git am' is unhappy when I try to apply it: error: sha1 information is lacking or useless (drivers/gpu/drm/msm/adreno/a6xx_gmu.c). error: could not build fake ancestor Both upstream and drm-msm are in my remotes and synced, so I suspect it's some private tree. Please make sure to base patches on the corresponding maintainer tree or upstream, whichs makes life easier for maintainers, testers and reviewers.
Re: [v1] drm/msm/dpu: add support for clk and bw scaling for display
On Tue, Jul 14, 2020 at 04:39:47PM +0530, kalya...@codeaurora.org wrote: > On 2020-07-14 06:42, Matthias Kaehlcke wrote: > > On Thu, Jun 18, 2020 at 07:38:41PM +0530, Kalyan Thota wrote: > > > This change adds support to scale src clk and bandwidth as > > > per composition requirements. > > > > > > Interconnect registration for bw has been moved to mdp > > > device node from mdss to facilitate the scaling. > > > > > > Changes in v1: > > > - Address armv7 compilation issues with the patch (Rob) > > > > > > Signed-off-by: Kalyan Thota > > > > It seems this is an evolution of this series: > > https://patchwork.kernel.org/project/linux-arm-msm/list/?series=265351 > > > > Are the DT bits of the series still valid? If so please include them in > > the > > series, otherwise please add DT patches to allow folks to test and > > review, > > and get them landed in Bjorn's tree after the driver changes have > > landed. > > Hi, > > Yes the patch is dependent on the DT changes, should i add them with depends > tag in the commit text ? > https://patchwork.kernel.org/patch/11470785/ > https://patchwork.kernel.org/patch/11470789/ This patch doesn't really depend on the DT changes. I would suggest to make this a series of 3 patches, just like the original series linked above.
Re: [v1] drm/msm/dpu: add support for clk and bw scaling for display
On Thu, Jun 18, 2020 at 07:38:41PM +0530, Kalyan Thota wrote: > This change adds support to scale src clk and bandwidth as > per composition requirements. > > Interconnect registration for bw has been moved to mdp > device node from mdss to facilitate the scaling. > > Changes in v1: > - Address armv7 compilation issues with the patch (Rob) > > Signed-off-by: Kalyan Thota It seems this is an evolution of this series: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=265351 Are the DT bits of the series still valid? If so please include them in the series, otherwise please add DT patches to allow folks to test and review, and get them landed in Bjorn's tree after the driver changes have landed.
Re: [PATCH 3/3] arm64: dts: sc7180: Add OPP tables and power-domains for venus
On Thu, Jul 02, 2020 at 02:26:14PM +0530, Rajendra Nayak wrote:
>
> On 7/1/2020 10:24 PM, Matthias Kaehlcke wrote:
> > On Wed, Jul 01, 2020 at 05:10:38PM +0530, Rajendra Nayak wrote:
> > > Add the OPP tables in order to be able to vote on the performance state
> > > of a power-domain
> > >
> > > Signed-off-by: Rajendra Nayak
> > > ---
> > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 35
> > > +--
> > > 1 file changed, 33 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > index ad57df2..738a741 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > @@ -2392,8 +2392,10 @@
> > > reg = <0 0x0aa0 0 0xff000>;
> > > interrupts = ;
> > > power-domains = < VENUS_GDSC>,
> > > - < VCODEC0_GDSC>;
> > > - power-domain-names = "venus", "vcodec0";
> > > + < VCODEC0_GDSC>,
> > > + < SC7180_CX>;
> > > + power-domain-names = "venus", "vcodec0", "opp-pd";
> > > + operating-points-v2 = <_opp_table>;
> > > clocks = < VIDEO_CC_VENUS_CTL_CORE_CLK>,
> > >< VIDEO_CC_VENUS_AHB_CLK>,
> > >< VIDEO_CC_VENUS_CTL_AXI_CLK>,
> > > @@ -2414,6 +2416,35 @@
> > > video-encoder {
> > > compatible = "venus-encoder";
> > > };
> > > +
> > > + venus_opp_table: venus-opp-table {
> > > + compatible = "operating-points-v2";
> > > +
> > > + opp-2 {
> > > + opp-hz = /bits/ 64 <15000>;
> > > + required-opps = <_opp_low_svs>;
> > > + };
> > > +
> > > + opp-32000 {
> > > + opp-hz = /bits/ 64 <27000>;
> > > + required-opps = <_opp_svs>;
> > > + };
> > > +
> > > + opp-38000 {
> > > + opp-hz = /bits/ 64 <34000>;
> > > + required-opps = <_opp_svs_l1>;
> > > + };
> > > +
> > > + opp-44400 {
> > > + opp-hz = /bits/ 64 <43400>;
> > > + required-opps = <_opp_nom>;
> > > + };
> > > +
> > > + opp-53300 {
> > > + opp-hz = /bits/ 64 <5>;
> > > + required-opps = <_opp_turbo>;
> > > + };
> >
> > the labels of the OPP nodes don't match the specified frequencies
>
> Oops, I'll fix and respin.
ping, it seems the respin is still pending
Re: [PATCH V1] mmc: sdhci-msm: Set IO pins in low power state during suspend
Hi,
On Fri, Jul 10, 2020 at 04:28:36PM +0530, Veerabhadrarao Badiganti wrote:
> Hi Mathias,
>
> On 7/10/2020 6:22 AM, Matthias Kaehlcke wrote:
> > Hi,
> >
> > On Wed, Jul 08, 2020 at 06:41:20PM +0530, Veerabhadrarao Badiganti wrote:
> > > Configure SDHC IO pins with low power configuration when the driver
> > > is in suspend state.
> > >
> > > Signed-off-by: Veerabhadrarao Badiganti
> > > ---
> > > drivers/mmc/host/sdhci-msm.c | 17 +
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> > > index 392d41d57a6e..efd2bae1430c 100644
> > > --- a/drivers/mmc/host/sdhci-msm.c
> > > +++ b/drivers/mmc/host/sdhci-msm.c
> > > @@ -15,6 +15,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include "sdhci-pltfm.h"
> > > #include "cqhci.h"
> > > @@ -1352,6 +1353,19 @@ static void sdhci_msm_set_uhs_signaling(struct
> > > sdhci_host *host,
> > > sdhci_msm_hs400(host, >ios);
> > > }
> > > +static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool
> > > level)
> > > +{
> > > + struct platform_device *pdev = msm_host->pdev;
> > > + int ret;
> > > +
> > > + if (level)
> > > + ret = pinctrl_pm_select_default_state(>dev);
> > > + else
> > > + ret = pinctrl_pm_select_sleep_state(>dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> > > {
> > > if (IS_ERR(mmc->supply.vmmc))
> > > @@ -1596,6 +1610,9 @@ static void sdhci_msm_handle_pwr_irq(struct
> > > sdhci_host *host, int irq)
> > > ret = sdhci_msm_set_vqmmc(msm_host, mmc,
> > > pwr_state & REQ_BUS_ON);
> > > if (!ret)
> > > + ret = sdhci_msm_set_pincfg(msm_host,
> > > + pwr_state & REQ_BUS_ON);
> > > + if (!ret)
> > > irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> > > else
> > > irq_ack |= CORE_PWRCTL_BUS_FAIL;
> > I happened to have a debug patch in my tree which logs when regulators
> > are enabled/disabled, with this patch I see the SD card regulator
> > toggling constantly after returning from the first system suspend.
> >
> > I added more logs:
> >
> > [ 1156.085819] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
> > [ 1156.248936] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
> > [ 1156.301989] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
> > [ 1156.462383] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
> > [ 1156.525988] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
> > [ 1156.670372] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
> > [ 1156.717935] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
> > [ 1156.878122] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
> > [ 1156.928134] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
> >
> > This is on an SC7180 platform. It doesn't run an upstream kernel though,
> > but v5.4 with plenty of upstream patches.
> I have verified this on couple of sc7180 targets (on Chrome platform with
> Chrome kernel).
> But didn't see any issue. Its working as expected.
Did you test system suspend too? At least in the Chrome OS kernel tree system
suspend is not supported yet in the main branch, you'd need a pile of 30+
extra patches to get it to work. This is expected to change soon though :)
> Let me know if you are observing this issue constantly on multiple boards, I
> will share you
> a debug patch to check it further.
I currently have only one board with the SD card slot populated, I might
get another one next week.
The toggling occurs only when no SD card is inserted.
Re: [PATCH V1] mmc: sdhci-msm: Set IO pins in low power state during suspend
Hi,
On Wed, Jul 08, 2020 at 06:41:20PM +0530, Veerabhadrarao Badiganti wrote:
> Configure SDHC IO pins with low power configuration when the driver
> is in suspend state.
>
> Signed-off-by: Veerabhadrarao Badiganti
> ---
> drivers/mmc/host/sdhci-msm.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 392d41d57a6e..efd2bae1430c 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include "sdhci-pltfm.h"
> #include "cqhci.h"
> @@ -1352,6 +1353,19 @@ static void sdhci_msm_set_uhs_signaling(struct
> sdhci_host *host,
> sdhci_msm_hs400(host, >ios);
> }
>
> +static int sdhci_msm_set_pincfg(struct sdhci_msm_host *msm_host, bool level)
> +{
> + struct platform_device *pdev = msm_host->pdev;
> + int ret;
> +
> + if (level)
> + ret = pinctrl_pm_select_default_state(>dev);
> + else
> + ret = pinctrl_pm_select_sleep_state(>dev);
> +
> + return ret;
> +}
> +
> static int sdhci_msm_set_vmmc(struct mmc_host *mmc)
> {
> if (IS_ERR(mmc->supply.vmmc))
> @@ -1596,6 +1610,9 @@ static void sdhci_msm_handle_pwr_irq(struct sdhci_host
> *host, int irq)
> ret = sdhci_msm_set_vqmmc(msm_host, mmc,
> pwr_state & REQ_BUS_ON);
> if (!ret)
> + ret = sdhci_msm_set_pincfg(msm_host,
> + pwr_state & REQ_BUS_ON);
> + if (!ret)
> irq_ack |= CORE_PWRCTL_BUS_SUCCESS;
> else
> irq_ack |= CORE_PWRCTL_BUS_FAIL;
I happened to have a debug patch in my tree which logs when regulators
are enabled/disabled, with this patch I see the SD card regulator
toggling constantly after returning from the first system suspend.
I added more logs:
[ 1156.085819] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
[ 1156.248936] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
[ 1156.301989] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
[ 1156.462383] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
[ 1156.525988] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
[ 1156.670372] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
[ 1156.717935] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
[ 1156.878122] DBG: sdhci_msm_set_pincfg: level = 1 (ret: 0)
[ 1156.928134] DBG: sdhci_msm_set_pincfg: level = 0 (ret: 0)
This is on an SC7180 platform. It doesn't run an upstream kernel though,
but v5.4 with plenty of upstream patches.
Re: [PATCH v8 2/2] arm64: dts: qcom: sc7180: Add maximum speed property for DWC3 USB node
On Thu, Jul 09, 2020 at 09:30:12PM +0530, Sandeep Maheswaram wrote: > Adding maximum speed property for DWC3 USB node which can be used > for setting interconnect bandwidth. > > Signed-off-by: Sandeep Maheswaram > --- > arch/arm64/boot/dts/qcom/sc7180.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi > b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index 2be81a2..753e1a1 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -2544,6 +2544,7 @@ > snps,dis_enblslpm_quirk; > phys = <_1_hsphy>, <_1_ssphy>; > phy-names = "usb2-phy", "usb3-phy"; > + maximum-speed = "super-speed"; > }; > }; It shouldn't be strictly necessary if you use super-speed as default max-speed in the driver, but it also does no harm. Reviewed-by: Matthias Kaehlcke You might want to add it for other platforms too.
Re: [PATCH v8 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi, inline one more thing I forgot to comment on in my previous mail On Thu, Jul 09, 2020 at 09:30:11PM +0530, Sandeep Maheswaram wrote: > Add interconnect support in dwc3-qcom driver to vote for bus > bandwidth. > > This requires for two different paths - from USB master to > DDR slave. The other is from APPS master to USB slave. > > Signed-off-by: Sandeep Maheswaram > Signed-off-by: Chandana Kishori Chiluveru > Reviewed-by: Matthias Kaehlcke > --- > drivers/usb/dwc3/dwc3-qcom.c | 127 > ++- > 1 file changed, 125 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 1dfd024..5532988 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > ... > > @@ -648,6 +763,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > goto depopulate; > } > > + qcom->max_speed = usb_get_maximum_speed(>dwc3->dev); What if the function returns USB_SPEED_UNKNOWN? You need a reasonable default value for that case, which I think would be USB_SPEED_SUPER (i.e. the controller would work properly at super speed, though the interconnects would consume a bit more power than necessary lower speed modes).
Re: [PATCH v8 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi Sandeep,
On Thu, Jul 09, 2020 at 09:30:11PM +0530, Sandeep Maheswaram wrote:
> Add interconnect support in dwc3-qcom driver to vote for bus
> bandwidth.
>
> This requires for two different paths - from USB master to
> DDR slave. The other is from APPS master to USB slave.
>
> Signed-off-by: Sandeep Maheswaram
> Signed-off-by: Chandana Kishori Chiluveru
> Reviewed-by: Matthias Kaehlcke
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 127
> ++-
> 1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 1dfd024..5532988 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -43,6 +44,14 @@
> #define SDM845_QSCRATCH_SIZE 0x400
> #define SDM845_DWC3_CORE_SIZE0xcd00
>
> +/* Interconnect path bandwidths in MBps */
> +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240)
> +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700)
> +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000)
> +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500)
> +#define APPS_USB_AVG_BW 0
> +#define APPS_USB_PEAK_BW MBps_to_icc(40)
> +
> struct dwc3_acpi_pdata {
> u32 qscratch_base_offset;
> u32 qscratch_base_size;
> @@ -73,9 +82,12 @@ struct dwc3_qcom {
>
> const struct dwc3_acpi_pdata *acpi_pdata;
>
> + enum usb_device_speed max_speed;
> enum usb_dr_modemode;
> boolis_suspended;
> boolpm_suspended;
> + struct icc_path *usb_ddr_icc_path;
> + struct icc_path *apps_usb_icc_path;
nit: the names are a bit clunky, the 'usb' part is redundant. You could name
'icc_path_ddr' and 'icc_path_apps' or add an anonymous struct:
struct {
struct icc_path ddr;
struct icc_path apps;
};
not super-important, but since it looks like you have to respin anyway it's
something to consider.
> };
>
> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> @@ -190,6 +202,99 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom
> *qcom)
> return 0;
> }
>
> +/* Currently we only use bandwidth level, so just "enable" interconnects */
> +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom)
> +{
> + int ret;
> +
> + if (qcom->max_speed >= USB_SPEED_SUPER)
> + ret = icc_set_bw(qcom->usb_ddr_icc_path,
> + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> + else
> + ret = icc_set_bw(qcom->usb_ddr_icc_path,
> + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
> +
> + if (ret)
> + return ret;
> +
> + ret = icc_set_bw(qcom->apps_usb_icc_path,
> + APPS_USB_AVG_BW, APPS_USB_PEAK_BW);
> + if (ret)
> + icc_set_bw(qcom->usb_ddr_icc_path, 0, 0);
> +
The helpers icc_enable/disable() were recently added. With that you only
have to set the bandwidth once (unless it changes) and then use icc_disable()
to set it to 0 and icc_enable() to restore it.
With icc_enable/disable() the above code would move to
dwc3_qcom_interconnect_init(). It would also make it unnecessary to
have a 'max_speed' field in struct dwc3_qcom.
With that it might not be worth to keep dwc3_qcom_interconnect_enable/disable(),
since they will be relatively short and only have a single caller.
> + return ret;
> +}
> +
> +/* To disable an interconnect, we just set its bandwidth to 0 */
> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom)
> +{
> + int ret;
> +
> + ret = icc_set_bw(qcom->usb_ddr_icc_path, 0, 0);
> + if (ret)
> + return ret;
> +
> + ret = icc_set_bw(qcom->apps_usb_icc_path, 0, 0);
> + if (ret)
> + goto err_reenable_memory_path;
> +
> + return 0;
> +
> + /* Re-enable things in the event of an error */
> +err_reenable_memory_path:
> + dwc3_qcom_interconnect_enable(qcom);
> +
> + return ret;
> +}
> +
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> + * @qcom:Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + int ret;
> +
> + if (!device_is_bound(>dwc3->dev))
> + return -EPROBE_DEFER;
This is the main reason you
Re: [PATCH 1/2] dt-bindings: interconnect: Add generic qcom bindings
On Mon, Jun 22, 2020 at 09:05:14PM -0700, Mike Tipton wrote: > Add generic qcom interconnect bindings that are common across platforms. In > particular, these include QCOM_ICC_TAG_* macros that clients can use when > calling icc_set_tag(). > > Signed-off-by: Mike Tipton > --- > include/dt-bindings/interconnect/qcom,icc.h | 26 + > 1 file changed, 26 insertions(+) > create mode 100644 include/dt-bindings/interconnect/qcom,icc.h > > diff --git a/include/dt-bindings/interconnect/qcom,icc.h > b/include/dt-bindings/interconnect/qcom,icc.h > new file mode 100644 > index ..cd34f36d > --- /dev/null > +++ b/include/dt-bindings/interconnect/qcom,icc.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2020, The Linux Foundation. All rights reserved. > + */ > + > +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_ICC_H > +#define __DT_BINDINGS_INTERCONNECT_QCOM_ICC_H > + > +/* > + * The AMC bucket denotes constraints that are applied to hardware when > + * icc_set_bw() completes, whereas the WAKE and SLEEP constraints are applied > + * when the execution environment transitions between active and low power > mode. > + */ > +#define QCOM_ICC_BUCKET_AMC 0 > +#define QCOM_ICC_BUCKET_WAKE 1 > +#define QCOM_ICC_BUCKET_SLEEP2 > +#define QCOM_ICC_NUM_BUCKETS 3 > + > +#define QCOM_ICC_TAG_AMC (1 << QCOM_ICC_BUCKET_AMC) > +#define QCOM_ICC_TAG_WAKE(1 << QCOM_ICC_BUCKET_WAKE) > +#define QCOM_ICC_TAG_SLEEP (1 << QCOM_ICC_BUCKET_SLEEP) > +#define QCOM_ICC_TAG_ACTIVE_ONLY (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE) > +#define QCOM_ICC_TAG_ALWAYS (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE |\ > + QCOM_ICC_TAG_SLEEP) > + > +#endif Would it make sense to squash the two patches of this series into a single patch? This would make it more evident that this was moved from drivers/interconnect/qcom/icc-rpmh.h and avoid duplicate definitions if only this patch was applied.
Re: [PATCH] tty: serial: qcom-geni-serial: Drop the icc bw votes in suspend for console
On Thu, Jul 09, 2020 at 03:07:00PM +0530, Rajendra Nayak wrote: > When using the geni-serial as console, its important to be > able to hit the lowest possible power state in suspend, > even with no_console_suspend. > The only thing that prevents it today on platforms like the sc7180 > is the interconnect BW votes, which we certainly don't need when > the system is in suspend. So in the suspend handler mark them as > ACTIVE_ONLY (0x3) and on resume switch them back to the ALWAYS tag (0x7) > > Signed-off-by: Rajendra Nayak Tested-by: Matthias Kaehlcke
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Tue, Jul 07, 2020 at 10:41:24AM +0530, Sandeep Maheswaram (Temp) wrote:
>
> On 7/1/2020 4:12 AM, Matthias Kaehlcke wrote:
> > On Tue, Jun 16, 2020 at 01:38:49PM -0700, Matthias Kaehlcke wrote:
> > > On Tue, Jun 16, 2020 at 10:22:47AM +0530, Sandeep Maheswaram (Temp) wrote:
> > > > On 6/16/2020 1:12 AM, Matthias Kaehlcke wrote:
> > > > > On Thu, Jun 04, 2020 at 04:16:31AM -0700, Stephen Boyd wrote:
> > > > > > Quoting Sandeep Maheswaram (Temp) (2020-06-04 02:43:09)
> > > > > > > On 6/3/2020 11:06 PM, Stephen Boyd wrote:
> > > > > > > > Quoting Sandeep Maheswaram (2020-03-31 22:15:43)
> > > > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > index 1dfd024..d33ae86 100644
> > > > > > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > > > @@ -285,6 +307,101 @@ static int dwc3_qcom_resume(struct
> > > > > > > > > dwc3_qcom *qcom)
> > > > > > > > >return 0;
> > > > > > > > > }
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * dwc3_qcom_interconnect_init() - Get interconnect path
> > > > > > > > > handles
> > > > > > > > > + * @qcom: Pointer to the concerned usb
> > > > > > > > > core.
> > > > > > > > > + *
> > > > > > > > > + */
> > > > > > > > > +static int dwc3_qcom_interconnect_init(struct dwc3_qcom
> > > > > > > > > *qcom)
> > > > > > > > > +{
> > > > > > > > > + struct device *dev = qcom->dev;
> > > > > > > > > + int ret;
> > > > > > > > > +
> > > > > > > > > + if (!device_is_bound(>dwc3->dev))
> > > > > > > > > + return -EPROBE_DEFER;
> > > > > > > > How is this supposed to work? I see that this was added in an
> > > > > > > > earlier
> > > > > > > > revision of this patch series but there isn't any mention of why
> > > > > > > > device_is_bound() is used here. It would be great if there was
> > > > > > > > a comment
> > > > > > > > detailing why this is necessary. It sounds like maximum_speed is
> > > > > > > > important?
> > > > > > > >
> > > > > > > > Furthermore, dwc3_qcom_interconnect_init() is called by
> > > > > > > > dwc3_qcom_probe() which is the function that registers the
> > > > > > > > device for
> > > > > > > > qcom->dwc3->dev. If that device doesn't probe between the time
> > > > > > > > it is
> > > > > > > > registered by dwc3_qcom_probe() and this function is called
> > > > > > > > then we'll
> > > > > > > > fail dwc3_qcom_probe() with -EPROBE_DEFER. And that will remove
> > > > > > > > the
> > > > > > > > qcom->dwc3->dev device from the platform bus because we call
> > > > > > > > of_platform_depopulate() on the error path of dwc3_qcom_probe().
> > > > > > > >
> > > > > > > > So isn't this whole thing racy and can potentially lead us to a
> > > > > > > > driver
> > > > > > > > probe loop where the wrapper (dwc3_qcom) and the core (dwc3)
> > > > > > > > are probing
> > > > > > > > and we're trying to time it just right so that driver for dwc3
> > > > > > > > binds
> > > > > > > > before we setup interconnects? I don't know if dwc3 can
> > > > > > > > communicate to
> > > > > > > > the wrapper but that would be more of a direct way to do this.
> > > > > > > > Or maybe
> > > > > > > > the wrapper should try to read the DT property for maximum
> > > > > > > > speed and
> > > > > > > > fal
Re: [PATCH v2 2/4] drm/msm: dsi: Use OPP API to set clk/perf state
On Thu, Jul 02, 2020 at 04:39:09PM +0530, Rajendra Nayak wrote:
> On SDM845 and SC7180 DSI needs to express a performance state
> requirement on a power domain depending on the clock rates.
> Use OPP table from DT to register with OPP framework and use
> dev_pm_opp_set_rate() to set the clk/perf state.
>
> dev_pm_opp_set_rate() is designed to be equivalent to clk_set_rate()
> for devices without an OPP table, hence the change works fine
> on devices/platforms which only need to set a clock rate.
>
> Signed-off-by: Rajendra Nayak
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 26 --
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 11ae5b8..09e16b8 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -111,6 +112,9 @@ struct msm_dsi_host {
> struct clk *pixel_clk_src;
> struct clk *byte_intf_clk;
>
> + struct opp_table *opp_table;
> + bool has_opp_table;
> +
> u32 byte_clk_rate;
> u32 pixel_clk_rate;
> u32 esc_clk_rate;
> @@ -512,9 +516,10 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host
> *msm_host)
> DBG("Set clk rates: pclk=%d, byteclk=%d",
> msm_host->mode->clock, msm_host->byte_clk_rate);
>
> - ret = clk_set_rate(msm_host->byte_clk, msm_host->byte_clk_rate);
> + ret = dev_pm_opp_set_rate(_host->pdev->dev,
> + msm_host->byte_clk_rate);
> if (ret) {
> - pr_err("%s: Failed to set rate byte clk, %d\n", __func__, ret);
> + pr_err("%s: dev_pm_opp_set_rate failed %d\n", __func__, ret);
> return ret;
> }
>
> @@ -658,6 +663,8 @@ int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host)
>
> void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host)
> {
> + /* Drop the performance state vote */
> + dev_pm_opp_set_rate(_host->pdev->dev, 0);
> clk_disable_unprepare(msm_host->esc_clk);
> clk_disable_unprepare(msm_host->pixel_clk);
> if (msm_host->byte_intf_clk)
> @@ -1879,6 +1886,18 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
> goto fail;
> }
>
> + msm_host->opp_table = dev_pm_opp_set_clkname(>dev, "byte");
> + if (IS_ERR(msm_host->opp_table))
> + return PTR_ERR(msm_host->opp_table);
> + /* OPP table is optional */
> + ret = dev_pm_opp_of_add_table(>dev);
> + if (!ret) {
> + msm_host->has_opp_table = true;
> + } else if (ret != -ENODEV) {
> + dev_err(>dev, "invalid OPP table in device tree\n");
dev_pm_opp_put_clkname(msm_host->opp_table);
> + return ret;
> + }
With the missing _put_clkname() fixed:
Reviewed-by: Matthias Kaehlcke
Re: [PATCH 3/3] arm64: dts: sc7180: Add qspi opps and power-domains
On Fri, Jul 03, 2020 at 03:11:33PM +0530, Rajendra Nayak wrote: > Add the power domain supporting performance state and the corresponding > OPP tables for the qspi device on sc7180 > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH 2/3] arm64: dts: sdm845: Add qspi opps and power-domains
On Fri, Jul 03, 2020 at 03:11:32PM +0530, Rajendra Nayak wrote: > Add the power domain supporting performance state and the corresponding > OPP tables for the qspi device on sdm845 > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH] drm/msm: handle for EPROBE_DEFER for of_icc_get
On Wed, Jul 01, 2020 at 01:13:34PM -0400, Jonathan Marek wrote: > On 7/1/20 1:12 PM, Matthias Kaehlcke wrote: > > Hi Jonathan, > > > > On Tue, Jun 30, 2020 at 11:08:41PM -0400, Jonathan Marek wrote: > > > Check for EPROBE_DEFER instead of silently not using icc if the msm driver > > > probes before the interconnect driver. > > > > Agreed with supporting deferred ICC probing. > > > > > Only check for EPROBE_DEFER because of_icc_get can return other errors > > > that > > > we want to ignore (ENODATA). > > > > What would be the -ENODATA case? > > > > The of_icc_get for the ocmem_icc_path can return -ENODATA when the ocmem > path is not specified (it is optional and only relevant for a3xx/a4xx). Thanks for the clarification! In this case it seems reasonable to me to return any error for the 'gfx-mem' path and all errors except -ENODATA for 'ocmem'.
Re: [PATCH] drm/msm: handle for EPROBE_DEFER for of_icc_get
Hi Jonathan, On Tue, Jun 30, 2020 at 11:08:41PM -0400, Jonathan Marek wrote: > Check for EPROBE_DEFER instead of silently not using icc if the msm driver > probes before the interconnect driver. Agreed with supporting deferred ICC probing. > Only check for EPROBE_DEFER because of_icc_get can return other errors that > we want to ignore (ENODATA). What would be the -ENODATA case? If the 'interconnects' property is not specified of_icc_get() returns NULL, shouldn't all (or most) errors be propagated rather than staying silent? Thanks Matthias
Re: [PATCH 3/3] arm64: dts: sc7180: Add OPP tables and power-domains for venus
On Wed, Jul 01, 2020 at 05:10:38PM +0530, Rajendra Nayak wrote:
> Add the OPP tables in order to be able to vote on the performance state
> of a power-domain
>
> Signed-off-by: Rajendra Nayak
> ---
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 35 +--
> 1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index ad57df2..738a741 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -2392,8 +2392,10 @@
> reg = <0 0x0aa0 0 0xff000>;
> interrupts = ;
> power-domains = < VENUS_GDSC>,
> - < VCODEC0_GDSC>;
> - power-domain-names = "venus", "vcodec0";
> + < VCODEC0_GDSC>,
> + < SC7180_CX>;
> + power-domain-names = "venus", "vcodec0", "opp-pd";
> + operating-points-v2 = <_opp_table>;
> clocks = < VIDEO_CC_VENUS_CTL_CORE_CLK>,
>< VIDEO_CC_VENUS_AHB_CLK>,
>< VIDEO_CC_VENUS_CTL_AXI_CLK>,
> @@ -2414,6 +2416,35 @@
> video-encoder {
> compatible = "venus-encoder";
> };
> +
> + venus_opp_table: venus-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-2 {
> + opp-hz = /bits/ 64 <15000>;
> + required-opps = <_opp_low_svs>;
> + };
> +
> + opp-32000 {
> + opp-hz = /bits/ 64 <27000>;
> + required-opps = <_opp_svs>;
> + };
> +
> + opp-38000 {
> + opp-hz = /bits/ 64 <34000>;
> + required-opps = <_opp_svs_l1>;
> + };
> +
> + opp-44400 {
> + opp-hz = /bits/ 64 <43400>;
> + required-opps = <_opp_nom>;
> + };
> +
> + opp-53300 {
> + opp-hz = /bits/ 64 <5>;
> + required-opps = <_opp_turbo>;
> + };
the labels of the OPP nodes don't match the specified frequencies
Re: [PATCH 2/3] arm64: dts: sdm845: Add OPP tables and power-domains for venus
On Wed, Jul 01, 2020 at 05:10:37PM +0530, Rajendra Nayak wrote: > Add the OPP tables in order to be able to vote on the performance state of > a power-domain. > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH 2/4] drm/msm: dsi: Use OPP API to set clk/perf state
On Tue, Jun 30, 2020 at 05:26:14PM +0530, Rajendra Nayak wrote:
> On SDM845 DSI needs to express a perforamnce state
nit: performance
> requirement on a power domain depending on the clock rates.
> Use OPP table from DT to register with OPP framework and use
> dev_pm_opp_set_rate() to set the clk/perf state.
>
> Signed-off-by: Rajendra Nayak
> ---
> drivers/gpu/drm/msm/dsi/dsi.h | 2 ++
> drivers/gpu/drm/msm/dsi/dsi_cfg.c | 4 +--
> drivers/gpu/drm/msm/dsi/dsi_host.c | 58
> ++
> 3 files changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 4de771d..ba7583c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -180,10 +180,12 @@ int msm_dsi_runtime_suspend(struct device *dev);
> int msm_dsi_runtime_resume(struct device *dev);
> int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host);
> int dsi_link_clk_set_rate_v2(struct msm_dsi_host *msm_host);
> +int dsi_link_clk_set_rate_6g_v2(struct msm_dsi_host *msm_host);
> int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
> int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
> void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
> void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
> +void dsi_link_clk_disable_6g_v2(struct msm_dsi_host *msm_host);
> int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
> int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
> void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index 813d69d..773c4fe 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -210,9 +210,9 @@ static const struct msm_dsi_host_cfg_ops
> msm_dsi_6g_host_ops = {
> };
>
> static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_host_ops = {
> - .link_clk_set_rate = dsi_link_clk_set_rate_6g,
> + .link_clk_set_rate = dsi_link_clk_set_rate_6g_v2,
> .link_clk_enable = dsi_link_clk_enable_6g,
> - .link_clk_disable = dsi_link_clk_disable_6g,
> + .link_clk_disable = dsi_link_clk_disable_6g_v2,
> .clk_init_ver = dsi_clk_init_6g_v2,
> .tx_buf_alloc = dsi_tx_buf_alloc_6g,
> .tx_buf_get = dsi_tx_buf_get_6g,
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 11ae5b8..890531c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -111,6 +112,9 @@ struct msm_dsi_host {
> struct clk *pixel_clk_src;
> struct clk *byte_intf_clk;
>
> + struct opp_table *opp_table;
> + bool has_opp_table;
> +
> u32 byte_clk_rate;
> u32 pixel_clk_rate;
> u32 esc_clk_rate;
> @@ -537,6 +541,38 @@ int dsi_link_clk_set_rate_6g(struct msm_dsi_host
> *msm_host)
> return 0;
> }
>
> +int dsi_link_clk_set_rate_6g_v2(struct msm_dsi_host *msm_host)
> +{
> + int ret;
> + struct device *dev = _host->pdev->dev;
> +
> + DBG("Set clk rates: pclk=%d, byteclk=%d",
> + msm_host->mode->clock, msm_host->byte_clk_rate);
> +
> + ret = dev_pm_opp_set_rate(dev, msm_host->byte_clk_rate);
> + if (ret) {
> + pr_err("%s: dev_pm_opp_set_rate failed %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = clk_set_rate(msm_host->pixel_clk, msm_host->pixel_clk_rate);
> + if (ret) {
> + pr_err("%s: Failed to set rate pixel clk, %d\n", __func__, ret);
> + return ret;
> + }
> +
> + if (msm_host->byte_intf_clk) {
> + ret = clk_set_rate(msm_host->byte_intf_clk,
> +msm_host->byte_clk_rate / 2);
> + if (ret) {
> + pr_err("%s: Failed to set rate byte intf clk, %d\n",
> +__func__, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
xThis function is essentially the same as dsi_link_clk_set_rate_6g(),
except for the use of dev_pm_opp_set_rate() instead of clk_set_rate().
IIUC dev_pm_opp_set_rate() just calls clk_set_rate() if the device has
no OPP table. If that's correct you could just call dev_pm_opp_set_rate()
in dsi_link_clk_set_rate_6g().
/*
* For IO devices which require an OPP on some platforms/SoCs
* while just needing to scale the clock on some others
* we look for empty OPP tables with just a clock handle and
* scale only the clk. This makes dev_pm_opp_set_rate()
* equivalent to a clk_set_rate()
*/
if (!_get_opp_count(opp_table)) {
ret = _generic_set_opp_clk_only(dev, clk, freq);
goto put_opp_table;
}
Re: [PATCH 4/4] arm64: dts: sc7180: Add DSI and MDP OPP tables and power-domains
On Tue, Jun 30, 2020 at 05:26:16PM +0530, Rajendra Nayak wrote: > Add the OPP tables for DSI and MDP based on the perf state/clk > requirements, and add the power-domains property to specify the > scalable power domain. > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH 3/4] arm64: dts: sdm845: Add DSI and MDP OPP tables and power-domains
On Tue, Jun 30, 2020 at 05:26:15PM +0530, Rajendra Nayak wrote:
> Add the OPP tables for DSI and MDP based on the perf state/clk
> requirements, and add the power-domains property to specify the
> scalable power domain.
>
> Signed-off-by: Rajendra Nayak
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 59
>
> 1 file changed, 59 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 8eb5a31..b6afeb2 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -3296,6 +3296,35 @@
> #power-domain-cells = <1>;
> };
>
> + dsi_opp_table: dsi-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-1920 {
> + opp-hz = /bits/ 64 <1920>;
> + required-opps = <_opp_min_svs>;
> + };
> +
> + opp-18000 {
> + opp-hz = /bits/ 64 <18000>;
> + required-opps = <_opp_low_svs>;
> + };
> +
> + opp-27500 {
> + opp-hz = /bits/ 64 <27500>;
> + required-opps = <_opp_svs>;
> + };
> +
> + opp-32858 {
> + opp-hz = /bits/ 64 <32858>;
> + required-opps = <_opp_svs_l1>;
> + };
> +
> + opp-35800 {
> + opp-hz = /bits/ 64 <35800>;
> + required-opps = <_opp_nom>;
> + };
> + };
> +
I still don't like the shared OPP tables to be positioned inmidst of the
device nodes, but it seems we currently don't have a better convention.
> mdss: mdss@ae0 {
> compatible = "qcom,sdm845-mdss";
> reg = <0 0x0ae0 0 0x1000>;
> @@ -3340,6 +3369,8 @@
> <
> DISP_CC_MDSS_VSYNC_CLK>;
> assigned-clock-rates = <3>,
> <1920>;
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SDM845_CX>;
>
> interrupt-parent = <>;
> interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> @@ -3364,6 +3395,30 @@
> };
> };
> };
> +
> + mdp_opp_table: mdp-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-1920 {
> + opp-hz = /bits/ 64 <1920>;
> + required-opps =
> <_opp_min_svs>;
> + };
> +
> + opp-171428571 {
> + opp-hz = /bits/ 64 <171428571>;
> + required-opps =
> <_opp_low_svs>;
> + };
> +
> + opp-34400 {
> + opp-hz = /bits/ 64 <34400>;
> + required-opps =
> <_opp_svs_l1>;
> + };
> +
> + opp-43000 {
> + opp-hz = /bits/ 64 <43000>;
> + required-opps =
> <_opp_nom>;
> + };
> + };
> };
>
> dsi0: dsi@ae94000 {
> @@ -3386,6 +3441,8 @@
> "core",
> "iface",
> "bus";
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SDM845_CX>;
>
> phys = <_phy>;
> phy-names = "dsi";
> @@ -3450,6 +3507,8 @@
> "core",
> "iface",
> "bus";
> + operating-points-v2 = <_opp_table>;
> + power-domains = < SDM845_CX>;
>
> phys = <_phy>;
> phy-names = "dsi";
Reviewed-by: Matthias Kaehlcke
Re: [PATCH 4/4] arm64: dts: sc7180: Add sdhc opps and power-domains
On Tue, Jun 30, 2020 at 02:15:12PM +0530, Rajendra Nayak wrote: > Add the power domain supporting performance state and the corresponding > OPP tables for the sdhc device on sc7180. > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH 3/4] arm64: dts: sdm845: Add sdhc opps and power-domains
On Tue, Jun 30, 2020 at 02:15:11PM +0530, Rajendra Nayak wrote: > Add the power domain supporting performance state and the corresponding > OPP tables for the sdhc device on sdm845. > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH 2/4] arm64: dts: sc7180: Add OPP table for all qup devices
On Tue, Jun 30, 2020 at 02:15:10PM +0530, Rajendra Nayak wrote: > qup has a requirement to vote on the performance state of the CX domain > in sc7180 devices. Add OPP tables for these and also add power-domains > property for all qup instances for uart and spi. > i2c does not support scaling and uses a fixed clock. > > Signed-off-by: Rajendra Nayak Reviewed-by: Matthias Kaehlcke
Re: [PATCH 1/4] arm64: dts: sdm845: Add OPP table for all qup devices
On Tue, Jun 30, 2020 at 02:15:09PM +0530, Rajendra Nayak wrote: > qup has a requirement to vote on the performance state of the CX domain > in sdm845 devices. Add OPP tables for these and also add power-domains > property for all qup instances for uart and spi. > i2c does not support scaling and uses a fixed clock. > > Signed-off-by: Rajendra Nayak > Signed-off-by: Stephen Boyd Reviewed-by: Matthias Kaehlcke
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Tue, Jun 16, 2020 at 01:38:49PM -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 16, 2020 at 10:22:47AM +0530, Sandeep Maheswaram (Temp) wrote:
> >
> > On 6/16/2020 1:12 AM, Matthias Kaehlcke wrote:
> > > On Thu, Jun 04, 2020 at 04:16:31AM -0700, Stephen Boyd wrote:
> > > > Quoting Sandeep Maheswaram (Temp) (2020-06-04 02:43:09)
> > > > > On 6/3/2020 11:06 PM, Stephen Boyd wrote:
> > > > > > Quoting Sandeep Maheswaram (2020-03-31 22:15:43)
> > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > index 1dfd024..d33ae86 100644
> > > > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > > @@ -285,6 +307,101 @@ static int dwc3_qcom_resume(struct
> > > > > > > dwc3_qcom *qcom)
> > > > > > > return 0;
> > > > > > >}
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> > > > > > > + * @qcom: Pointer to the concerned usb core.
> > > > > > > + *
> > > > > > > + */
> > > > > > > +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> > > > > > > +{
> > > > > > > + struct device *dev = qcom->dev;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (!device_is_bound(>dwc3->dev))
> > > > > > > + return -EPROBE_DEFER;
> > > > > > How is this supposed to work? I see that this was added in an
> > > > > > earlier
> > > > > > revision of this patch series but there isn't any mention of why
> > > > > > device_is_bound() is used here. It would be great if there was a
> > > > > > comment
> > > > > > detailing why this is necessary. It sounds like maximum_speed is
> > > > > > important?
> > > > > >
> > > > > > Furthermore, dwc3_qcom_interconnect_init() is called by
> > > > > > dwc3_qcom_probe() which is the function that registers the device
> > > > > > for
> > > > > > qcom->dwc3->dev. If that device doesn't probe between the time it is
> > > > > > registered by dwc3_qcom_probe() and this function is called then
> > > > > > we'll
> > > > > > fail dwc3_qcom_probe() with -EPROBE_DEFER. And that will remove the
> > > > > > qcom->dwc3->dev device from the platform bus because we call
> > > > > > of_platform_depopulate() on the error path of dwc3_qcom_probe().
> > > > > >
> > > > > > So isn't this whole thing racy and can potentially lead us to a
> > > > > > driver
> > > > > > probe loop where the wrapper (dwc3_qcom) and the core (dwc3) are
> > > > > > probing
> > > > > > and we're trying to time it just right so that driver for dwc3 binds
> > > > > > before we setup interconnects? I don't know if dwc3 can communicate
> > > > > > to
> > > > > > the wrapper but that would be more of a direct way to do this. Or
> > > > > > maybe
> > > > > > the wrapper should try to read the DT property for maximum speed and
> > > > > > fallback to a worst case high bandwidth value if it can't figure it
> > > > > > out
> > > > > > itself without help from dwc3 core.
> > > > > >
> > > > > This was added in V4 to address comments from Matthias in V3
> > > > >
> > > > > https://patchwork.kernel.org/patch/11148587/
> > > > >
> > > > Yes, that why I said:
> > > >
> > > > "I see that this was added in an earlier
> > > > revision of this patch series but there isn't any mention of why
> > > > device_is_bound() is used here. It would be great if there was a
> > > > comment
> > > > detailing why this is necessary. It sounds like maximum_speed is
> > > > important?"
> > > >
> > > > Can you please respond to the rest of my email?
> > > I agree with Stephen that using device_
Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state
On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: > > On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: > > > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > > > > > > Wait, so *some* of the series should go together but not other bits? > > > > But you want them split up for some reason? > > > > > Yes, this will almost certainly be the case, even if not for this patch. > > > I brought this up earlier > > > (https://patchwork.kernel.org/cover/11604623/#23428709). > > > > I'm not really reading any of this stuff for the series as a whole, as > > far as I could tell I'd reviewed all my bits and was hoping whatever > > random platform stuff needs sorting out was going to be sorted out so I > > stopped getting copied on revisions :( > > Sorry this caused you extra work, I only fully realized this when the series > was basically ready to land :( > > Avoiding unnecessary revision spam is another good reason to not combine > technically unrelated patches in a single series. > > If I notice similar series in the future I'll try to bring it up early. > > > > For the QSPI patch you could argue to just take it through QCOM since the > > > SPI > > > patch of this series goes through this tree, up to you, I just want to > > > make > > > sure everybody is on the same page. > > > > If there are some part of this that don't have a connection with the > > rest of the series and should be applied separately please split them > > out and send them separately so it's clear what's going on. > > Rajendra, IIUC you have to re-spin this series anyway, please split it > up in self-contained chunks. One more thing: when you do the split it seems it would make sense to include the DT changes that were initially part of this series (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=278691=*)
Re: [PATCH v3 04/17] arm64: dts: sc7180: Add OPP table for all qup devices
Hi Rajendra,
On Tue, Apr 28, 2020 at 07:02:52PM +0530, Rajendra Nayak wrote:
> qup has a requirement to vote on the performance state of the CX domain
> in sc7180 devices. Add OPP tables for these and also add power-domains
> property for all qup instances.
>
> Signed-off-by: Rajendra Nayak
> ---
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 79
>
> 1 file changed, 79 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 998f101..efba600 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -417,6 +417,25 @@
> status = "disabled";
> };
>
> + qup_opp_table: qup-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-7500 {
> + opp-hz = /bits/ 64 <7500>;
> + required-opps = <_opp_low_svs>;
> + };
> +
> + opp-1 {
> + opp-hz = /bits/ 64 <1>;
> + required-opps = <_opp_svs>;
> + };
> +
> + opp-12800 {
> + opp-hz = /bits/ 64 <12800>;
> + required-opps = <_opp_nom>;
> + };
> + };
> +
> qupv3_id_0: geniqup@8c {
> compatible = "qcom,geni-se-qup";
> reg = <0 0x008c 0 0x6000>;
no entries for i2c0?
> @@ -452,6 +471,8 @@
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -463,6 +484,8 @@
> pinctrl-names = "default";
> pinctrl-0 = <_uart0_default>;
> interrupts = ;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -476,6 +499,8 @@
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -489,6 +514,8 @@
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -500,6 +527,8 @@
> pinctrl-names = "default";
> pinctrl-0 = <_uart1_default>;
> interrupts = ;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -513,6 +542,8 @@
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -524,6 +555,8 @@
> pinctrl-names = "default";
> pinctrl-0 = <_uart2_default>;
> interrupts = ;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -537,6 +570,8 @@
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
> + power-domains = < SC7180_CX>;
> + operating-points-v2 = <_opp_table>;
> status = "disabled";
> };
>
> @@ -550,6 +585,8 @@
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
> +
Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state
On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: > > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > > > > Wait, so *some* of the series should go together but not other bits? > > > But you want them split up for some reason? > > > Yes, this will almost certainly be the case, even if not for this patch. > > I brought this up earlier > > (https://patchwork.kernel.org/cover/11604623/#23428709). > > I'm not really reading any of this stuff for the series as a whole, as > far as I could tell I'd reviewed all my bits and was hoping whatever > random platform stuff needs sorting out was going to be sorted out so I > stopped getting copied on revisions :( Sorry this caused you extra work, I only fully realized this when the series was basically ready to land :( Avoiding unnecessary revision spam is another good reason to not combine technically unrelated patches in a single series. If I notice similar series in the future I'll try to bring it up early. > > For the QSPI patch you could argue to just take it through QCOM since the > > SPI > > patch of this series goes through this tree, up to you, I just want to make > > sure everybody is on the same page. > > If there are some part of this that don't have a connection with the > rest of the series and should be applied separately please split them > out and send them separately so it's clear what's going on. Rajendra, IIUC you have to re-spin this series anyway, please split it up in self-contained chunks.
Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state
On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 10:39:48AM -0700, Matthias Kaehlcke wrote: > > On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote: > > > > Aren't there dependencies on earlier patches in the series? > > > Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set > > clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made > > by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state", > > however that's not true for this patch. > > Wait, so *some* of the series should go together but not other bits? > But you want them split up for some reason? Yes, this will almost certainly be the case, even if not for this patch. I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). It seems very unlikely to me that the DRM patches will go through the QCOM tree. The venus patch also doesn't have any dependencies and is more likely to cause conflicts if it lands through QCOM instead of it's maintainer tree. For the QSPI patch you could argue to just take it through QCOM since the SPI patch of this series goes through this tree, up to you, I just want to make sure everybody is on the same page. > > I wonder if it would have been better to split this series into individual > > patches/mini-series, to avoid this kind of confusion. > > Yes, if there's no dependencies then bundling things up into a series > just causes confusion.
Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state
On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 10:09:33AM -0700, Matthias Kaehlcke wrote: > > Hi Mark, > > > > do you plan to land this in your tree? > > > > I know you hate contentless pings, but since you acked this patch and > > usually don't seem to do that when patches go through your tree I want > > to make sure we aren't in a situation where everybody thinks that the > > patch will go through someone else's tree. > > Aren't there dependencies on earlier patches in the series? Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state", however that's not true for this patch. I wonder if it would have been better to split this series into individual patches/mini-series, to avoid this kind of confusion. > In general if someone acks something for their tree that means they don't > expect to apply it themselves. Yes, that was my understanding and prompted me to clarify this with you. The patch could go through the QCOM tree, but to my knowledge there is no reason for it. Btw, the patch "[V8,7/8] spi: spi-qcom-qspi: Add interconnect support" (https://patchwork.kernel.org/patch/11620285/) is in a similar situation. Another patch of the series for the 'spi-geni-qcom' driver has to go through the QCOM change due to changes in geni, but the QSPI driver doesn't use geni and could therefore go through your tree. Ultimately I don't really care too much through which tree the patches land as long as you and Bjorn agree on it :)
Re: [PATCH v6 6/6] spi: spi-qcom-qspi: Use OPP API to set clk/perf state
Hi Mark,
do you plan to land this in your tree?
I know you hate contentless pings, but since you acked this patch and
usually don't seem to do that when patches go through your tree I want
to make sure we aren't in a situation where everybody thinks that the
patch will go through someone else's tree.
Thanks
Matthias
On Mon, Jun 15, 2020 at 05:32:44PM +0530, Rajendra Nayak wrote:
> QSPI needs to vote on a performance state of a power domain depending on
> the clock rate. Add support for it by specifying the perf state/clock rate
> as an OPP table in device tree.
>
> Signed-off-by: Rajendra Nayak
> Reviewed-by: Matthias Kaehlcke
> Acked-by: Mark Brown
> Cc: Alok Chauhan
> Cc: Akash Asthana
> Cc: linux-...@vger.kernel.org
> ---
> No functional change in v6, rebased over 5.8-rc1
>
> drivers/spi/spi-qcom-qspi.c | 28 +++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> index 3c4f83b..ef51982 100644
> --- a/drivers/spi/spi-qcom-qspi.c
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -139,6 +140,8 @@ struct qcom_qspi {
> struct device *dev;
> struct clk_bulk_data *clks;
> struct qspi_xfer xfer;
> + struct opp_table *opp_table;
> + bool has_opp_table;
> /* Lock to protect xfer and IRQ accessed registers */
> spinlock_t lock;
> };
> @@ -235,7 +238,7 @@ static int qcom_qspi_transfer_one(struct spi_master
> *master,
> speed_hz = xfer->speed_hz;
>
> /* In regular operation (SBL_EN=1) core must be 4x transfer clock */
> - ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4);
> + ret = dev_pm_opp_set_rate(ctrl->dev, speed_hz * 4);
> if (ret) {
> dev_err(ctrl->dev, "Failed to set core clk %d\n", ret);
> return ret;
> @@ -481,6 +484,20 @@ static int qcom_qspi_probe(struct platform_device *pdev)
> master->handle_err = qcom_qspi_handle_err;
> master->auto_runtime_pm = true;
>
> + ctrl->opp_table = dev_pm_opp_set_clkname(>dev, "core");
> + if (IS_ERR(ctrl->opp_table)) {
> + ret = PTR_ERR(ctrl->opp_table);
> + goto exit_probe_master_put;
> + }
> + /* OPP table is optional */
> + ret = dev_pm_opp_of_add_table(>dev);
> + if (!ret) {
> + ctrl->has_opp_table = true;
> + } else if (ret != -ENODEV) {
> + dev_err(>dev, "invalid OPP table in device tree\n");
> + goto exit_probe_master_put;
> + }
> +
> pm_runtime_enable(dev);
>
> ret = spi_register_master(master);
> @@ -488,6 +505,9 @@ static int qcom_qspi_probe(struct platform_device *pdev)
> return 0;
>
> pm_runtime_disable(dev);
> + if (ctrl->has_opp_table)
> + dev_pm_opp_of_remove_table(>dev);
> + dev_pm_opp_put_clkname(ctrl->opp_table);
>
> exit_probe_master_put:
> spi_master_put(master);
> @@ -498,11 +518,15 @@ static int qcom_qspi_probe(struct platform_device *pdev)
> static int qcom_qspi_remove(struct platform_device *pdev)
> {
> struct spi_master *master = platform_get_drvdata(pdev);
> + struct qcom_qspi *ctrl = spi_master_get_devdata(master);
>
> /* Unregister _before_ disabling pm_runtime() so we stop transfers */
> spi_unregister_master(master);
>
> pm_runtime_disable(>dev);
> + if (ctrl->has_opp_table)
> + dev_pm_opp_of_remove_table(>dev);
> + dev_pm_opp_put_clkname(ctrl->opp_table);
>
> return 0;
> }
> @@ -512,6 +536,8 @@ static int __maybe_unused
> qcom_qspi_runtime_suspend(struct device *dev)
> struct spi_master *master = dev_get_drvdata(dev);
> struct qcom_qspi *ctrl = spi_master_get_devdata(master);
>
> + /* Drop the performance state vote */
> + dev_pm_opp_set_rate(dev, 0);
> clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);
>
> return 0;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
Re: [PATCH v3 2/6] drm: msm: a6xx: send opp instead of a frequency
Hi,
On Thu, Jun 18, 2020 at 10:52:09AM -0700, Rob Clark wrote:
> On Fri, Jun 5, 2020 at 9:26 PM Sharat Masetty wrote:
> >
> > This patch changes the plumbing to send the devfreq recommended opp rather
> > than the frequency. Also consolidate and rearrange the code in a6xx to set
> > the GPU frequency and the icc vote in preparation for the upcoming
> > changes for GPU->DDR scaling votes.
> >
> > Signed-off-by: Sharat Masetty
> > ---
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 62
> > +++
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 +-
> > drivers/gpu/drm/msm/msm_gpu.c | 3 +-
> > drivers/gpu/drm/msm/msm_gpu.h | 3 +-
> > 4 files changed, 38 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 748cd37..2d8124b 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -100,17 +100,30 @@ bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu)
> > A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));
> > }
> >
> > -static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> > +void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
> > {
> > - struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);
> > - struct adreno_gpu *adreno_gpu = _gpu->base;
> > - struct msm_gpu *gpu = _gpu->base;
> > - int ret;
> > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> > + struct a6xx_gmu *gmu = _gpu->gmu;
> > + u32 perf_index;
> > + unsigned long gpu_freq;
> > + int ret = 0;
> > +
> > + gpu_freq = dev_pm_opp_get_freq(opp);
> > +
> > + if (gpu_freq == gmu->freq)
> > + return;
> > +
> > + for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1;
> > perf_index++)
> > + if (gpu_freq == gmu->gpu_freqs[perf_index])
> > + break;
> > +
> > + gmu->current_perf_index = perf_index;
> >
> > gmu_write(gmu, REG_A6XX_GMU_DCVS_ACK_OPTION, 0);
> >
> > gmu_write(gmu, REG_A6XX_GMU_DCVS_PERF_SETTING,
> > - ((3 & 0xf) << 28) | index);
> > + ((3 & 0xf) << 28) | perf_index);
> >
> > /*
> > * Send an invalid index as a vote for the bus bandwidth and let the
> > @@ -126,7 +139,7 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu,
> > int index)
> > if (ret)
> > dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
> >
> > - gmu->freq = gmu->gpu_freqs[index];
> > + gmu->freq = gmu->gpu_freqs[perf_index];
> >
> > /*
> > * Eventually we will want to scale the path vote with the
> > frequency but
> > @@ -135,25 +148,6 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu,
> > int index)
> > icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));
> > }
> >
> > -void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)
> > -{
> > - struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > - struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> > - struct a6xx_gmu *gmu = _gpu->gmu;
> > - u32 perf_index = 0;
> > -
> > - if (freq == gmu->freq)
> > - return;
> > -
> > - for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1;
> > perf_index++)
> > - if (freq == gmu->gpu_freqs[perf_index])
> > - break;
> > -
> > - gmu->current_perf_index = perf_index;
> > -
> > - __a6xx_gmu_set_freq(gmu, perf_index);
> > -}
>
> this does end up conflicting a bit with some of the newer stuff that
> landed this cycle, in particular "drm/msm/a6xx: HFI v2 for A640 and
> A650"
>
> Adding Jonathan on CC since I think he will want to test this on
> a650/a640 as well..
Sharat, please send an updated version that is rebased on the latest drm-msm.
Thanks
Matthias
Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change
On Mon, Jun 22, 2020 at 01:46:48PM +0530, Sibi Sankar wrote: > Add support to parse optional OPP table attached to the cpu node when > the OPP bandwidth values are populated. This allows for scaling of > DDR/L3 bandwidth levels with frequency change. > > Signed-off-by: Sibi Sankar Reviewed-by: Matthias Kaehlcke
Re: [PATCH] driver core:Export the symbol device_is_bound
On Thu, Jun 18, 2020 at 11:33:49AM -0600, Rob Herring wrote: > On Thu, Jun 18, 2020 at 10:51 AM Matthias Kaehlcke wrote: > > > > On Thu, Jun 18, 2020 at 05:58:20PM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Jun 18, 2020 at 08:45:55AM -0700, Matthias Kaehlcke wrote: > > > > Hi Greg, > > > > > > > > On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote: > > > > > On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote: > > > > > > Export the symbol device_is_bound so that it can be used by the > > > > > > modules. > > > > > > > > > > What modules need this? > > > > > > > > drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers'). > > > > > > Why wasn't that said here? No context is not good :( > > > > Agreed, this patch should probably have been part of a series to establish > > the context. > > > > > > Short summary: QCOM dwc3 support is split in two drivers, the core dwc3 > > > > driver and the QCOM specific parts. dwc3-qcom is probed first (through > > > > a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate() > > > > to probe the core part. After a successful return from _populate() the > > > > driver assumes that the core device is fully initialized. However the > > > > latter is not correct, the driver core doesn't propagate errors from > > > > probe() to platform_populate(). The dwc3-qcom driver would use > > > > device_is_bound() to make sure the core device was probed successfully. > > > > > > why does the dwc3-qcom driver care? > > > > Currently the dwc3-qcom driver uses the core device to determine if the > > controller is used in peripheral mode and it runtime resumes the XHCI > > device when it sees a wakeup interrupt. > > > > The WIP patch to add interconnect support relies on the core driver > > to determine the max speed of the controller. > > > > > And why is the driver split in a way that requires such "broken" > > > structures? Why can't that be fixed instead? > > > > It seems determining the mode could be easily changed by getting it through > > the pdev, as in st_dwc3_probe(). Not sure about the other two parts, > > determining the maximum speed can involve evaluating hardware registers, > > which currently are 'owned' by the core driver. > > > > Manu or Sandeep who know the hardware and the driver better than me might > > have ideas on how to improve things. > > We never should have had this split either in the DT binding nor > driver(s) as if the SoC wrapper crap and licensed IP block are > independent things. The thing to do here is either make the DWC3 code > a library which drivers call (e.g. SDHCI) or add hooks into the DWC3 > driver for platform specifics (e.g. Designware PCI). Neither is a > simple solution though. Sounds reasonable, but as you say it's likely not a short term solution. If someone ever picked it up maybe they could create a new driver (with plenty of copied code from the current driver) and start with supporting a single platform (with multi-platform support in the driver architecture). Other drivers could then be migrated one by one by folks who have the hardware to test. Duplicate code is definitely not desirable, but it's probably more feasible than migrating all the drivers in one big bang. I guess for now we are stuck with the race in the dwc3-qcom driver ...
Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change
On Wed, Jun 17, 2020 at 10:13:21PM +0530, Sibi Sankar wrote:
> On 2020-06-17 03:41, Matthias Kaehlcke wrote:
> > Hi Sibi,
> >
> > after doing the review I noticed that Viresh replied on the cover letter
> > that he picked the series up for v5.9, so I'm not sure if it makes sense
> > to send a v7.
> >
> > On Wed, Jun 17, 2020 at 02:35:00AM +0530, Sibi Sankar wrote:
> >
> > > > > @@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct
> > > > > device *cpu_dev,
> > > > >
> > > > > if (freq != prev_freq && core_count != LUT_TURBO_IND) {
> > > > > table[i].frequency = freq;
> > > > > - dev_pm_opp_add(cpu_dev, freq * 1000, volt);
> > > > > + qcom_cpufreq_update_opp(cpu_dev, freq, volt);
> > > >
> > > > This is the cross-validation mentioned above, right? Shouldn't it
> > > > include
> > > > a check of the return value?
> > >
> > > Yes, this is the cross-validation step,
> > > we adjust the voltage if opp-tables are
> > > present/added successfully and enable
> > > them, else we would just do a add opp.
> > > We don't want to exit early on a single
> > > opp failure. We will error out a bit
> > > later if the opp-count ends up to be
> > > zero.
> >
> > At least an error/warning message would seem convenient when
> > adjusting/adding
> > an OPP fails, otherwise you would only notice by looking at the sysfs
> > attributes (if you'd even spot a single/few OPPs to be missing).
>
> I did consider the case where adjust
> voltage fails and we do report the
> freq for which it fails for as well.
> If adding a OPP fails we will still
> it being listed in the sysfs cpufreq
> scaling_available_frequencies since
> it lists the freq_table in khz there
> instead.
Ah, right, I missed that v6 added the error log to
qcom_cpufreq_update_opp(), please ignore my comment :)
Re: [PATCH] driver core:Export the symbol device_is_bound
On Thu, Jun 18, 2020 at 05:58:20PM +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 18, 2020 at 08:45:55AM -0700, Matthias Kaehlcke wrote: > > Hi Greg, > > > > On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote: > > > On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote: > > > > Export the symbol device_is_bound so that it can be used by the modules. > > > > > > What modules need this? > > > > drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers'). > > Why wasn't that said here? No context is not good :( Agreed, this patch should probably have been part of a series to establish the context. > > Short summary: QCOM dwc3 support is split in two drivers, the core dwc3 > > driver and the QCOM specific parts. dwc3-qcom is probed first (through > > a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate() > > to probe the core part. After a successful return from _populate() the > > driver assumes that the core device is fully initialized. However the > > latter is not correct, the driver core doesn't propagate errors from > > probe() to platform_populate(). The dwc3-qcom driver would use > > device_is_bound() to make sure the core device was probed successfully. > > why does the dwc3-qcom driver care? Currently the dwc3-qcom driver uses the core device to determine if the controller is used in peripheral mode and it runtime resumes the XHCI device when it sees a wakeup interrupt. The WIP patch to add interconnect support relies on the core driver to determine the max speed of the controller. > And why is the driver split in a way that requires such "broken" > structures? Why can't that be fixed instead? It seems determining the mode could be easily changed by getting it through the pdev, as in st_dwc3_probe(). Not sure about the other two parts, determining the maximum speed can involve evaluating hardware registers, which currently are 'owned' by the core driver. Manu or Sandeep who know the hardware and the driver better than me might have ideas on how to improve things.
Re: [PATCH] driver core:Export the symbol device_is_bound
Hi Greg,
On Thu, Jun 18, 2020 at 10:14:43AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> > Export the symbol device_is_bound so that it can be used by the modules.
>
> What modules need this?
drivers/usb/dwc3/dwc3-qcom.c (and probably other dwc3 'wrappers').
Short summary: QCOM dwc3 support is split in two drivers, the core dwc3
driver and the QCOM specific parts. dwc3-qcom is probed first (through
a DT entry or ACPI), dwc3_qcom_probe() then calls of_platform_populate()
to probe the core part. After a successful return from _populate() the
driver assumes that the core device is fully initialized. However the
latter is not correct, the driver core doesn't propagate errors from
probe() to platform_populate(). The dwc3-qcom driver would use
device_is_bound() to make sure the core device was probed successfully.
Related patches:
"usb: dwc3: qcom: Make sure core device is fully initialized before it is used"
https://lore.kernel.org/patchwork/patch/1257279/
"usb: dwc3: qcom: Add interconnect support in dwc3 driver"
https://patchwork.kernel.org/patch/11468647/
> > This change was suggested to solve the allmodconfig build error on adding
> > the patch https://lore.kernel.org/patchwork/patch/1218628/
> >
> > Signed-off-by: Sandeep Maheswaram
> > ---
> > drivers/base/dd.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 9a1d940..65d16ce 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -337,6 +337,7 @@ bool device_is_bound(struct device *dev)
> > {
> > return dev->p && klist_node_attached(>p->knode_driver);
> > }
> > +EXPORT_SYMBOL_GPL(device_is_bound);
>
> If a driver needs to use this, something is really wrong with it. What
> happens right after this, the state could have changed?
>
> So, no, sorry, this is not a good idea.
Agreed that what some dwc3 'wrapper' drivers do is brittle, and that using
device_is_bound() is only a bandaid, that doesn't address the entire issue.
Do you have any suggestions on how this could be properly addressed?
Thanks
Matthias
Re: [PATCH v6 0/6] DVFS for IO devices on sdm845 and sc7180
What is the plan for landing these, it seems not all must/should go through the QCOM tree. My guesses: tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state spi: spi-geni-qcom: Use OPP API to set clk/perf state QCOM tree due to shared dependency on change in include/linux/qcom-geni-se.h drm/msm/dpu: Use OPP API to set clk/perf state drm/msm: dsi: Use OPP API to set clk/perf state drm/msm tree media: venus: core: Add support for opp tables/perf voting venus tree spi: spi-qcom-qspi: Use OPP API to set clk/perf state SPI tree Does this make sense or are there any dependencies I'm missing? Thanks Matthias On Mon, Jun 15, 2020 at 05:32:38PM +0530, Rajendra Nayak wrote: > Changes in v6: > 1. rebased on 5.8-rc1, no functional change. > > Changes in v5: > 1. Opp cleanup path fixed up across drivers > > Changes in v4: > 1. Fixed all review feedback on v3 > 2. Dropped the dts patches, will post as a seperate series once > driver changes are reviewed and merged. > The driver changes without DT updates to include OPP tables will > have zero functional change. > 3. Dropped the mmc/sdhc patch, which is a standalone patch. will > repost if needed seperately. > > Changes in v3: > 1. Added better error handling for dev_pm_opp_of_add_table() > 2. Some minor changes and fixes in 'PATCH 12/17' as compared to v2 > 3. Dropped the mmc patch picked up by Ulf [2] > > Changes in v2: > 1. Added error handling for dev_pm_opp_set_clkname() > and dev_pm_opp_of_add_table() > 2. Used dev_pm_opp_put_clkname() in the cleanup path > 3. Dropped the OPP patch pulled in by Viresh [1] > 4. Dropped the UFS patches since they had some major rework > needed because of changes that were merged in the merge window > and I don't have a UFS device currently to validate the changes. > > We have had support added in the OPP core for a while now to support > DVFS for IO devices, and this series uses that infrastructure to > add DVFS support for various IO devices in sdm845 and sc7180 SoCs. > > [1] https://lkml.org/lkml/2020/4/14/98 > [2] https://lore.kernel.org/patchwork/patch/1226381/ > > Rajendra Nayak (6): > tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state > spi: spi-geni-qcom: Use OPP API to set clk/perf state > drm/msm/dpu: Use OPP API to set clk/perf state > drm/msm: dsi: Use OPP API to set clk/perf state > media: venus: core: Add support for opp tables/perf voting > spi: spi-qcom-qspi: Use OPP API to set clk/perf state > > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 3 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 26 +++- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 4 ++ > drivers/gpu/drm/msm/dsi/dsi.h | 2 + > drivers/gpu/drm/msm/dsi/dsi_cfg.c | 4 +- > drivers/gpu/drm/msm/dsi/dsi_host.c | 58 > ++ > drivers/media/platform/qcom/venus/core.c | 43 --- > drivers/media/platform/qcom/venus/core.h | 5 +++ > drivers/media/platform/qcom/venus/pm_helpers.c | 54 ++-- > drivers/spi/spi-geni-qcom.c| 26 ++-- > drivers/spi/spi-qcom-qspi.c| 28 - > drivers/tty/serial/qcom_geni_serial.c | 34 --- > include/linux/qcom-geni-se.h | 4 ++ > 13 files changed, 268 insertions(+), 23 deletions(-) > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
Re: [PATCH] usb: dwc3: qcom: Make sure core device is fully initialized before it is used
Hi Stephen,
On Wed, Jun 17, 2020 at 12:49:13PM -0700, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2020-06-16 13:37:37)
> > dwc3_qcom_of_register_core() uses of_platform_populate() to add
> > the dwc3 core device. The driver core will try to probe the device,
> > however this might fail (e.g. due to deferred probing) and
> > of_platform_populate() would still return 0 if the device was
> > successully added to the platform bus. Verify that the core device
> > is actually bound to its driver before using it, defer probing of the
> > dwc3_qcom device if the core device isn't ready (yet).
> >
> > Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver").
> > Signed-off-by: Matthias Kaehlcke
> > ---
> > depends on:
> > https://lore.kernel.org/patchwork/patch/1251661/ ("driver core:Export
> > the symbol device_is_bound")
> >
> > drivers/usb/dwc3/dwc3-qcom.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 1dfd024cd06b..5a9036b050c6 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -537,6 +537,16 @@ static int dwc3_qcom_of_register_core(struct
> > platform_device *pdev)
> > return -ENODEV;
> > }
> >
> > + /*
> > +* A successful return from of_platform_populate() only guarantees
> > that
> > +* the core device was added to the platform bus, however it might
> > not
> > +* be bound to its driver (e.g. due to deferred probing). This
> > driver
> > +* requires the core device to be fully initialized, so defer
> > probing
> > +* if it isn't ready (yet).
> > +*/
> > + if (!device_is_bound(>dwc3->dev))
> > + return -EPROBE_DEFER;
>
> Isn't this still broken? i.e. the dwc3 core driver may bind much later
> and then device_is_bound() will return an error here and then we'll
> return to the caller, dwc3_qcom_probe(), and that will depopulate the
> device with of_platform_depopulate(). It seems like we need to run some
> sort of wait for driver to be bound function instead of a one-shot check
> for the driver being bound.
My understanding is that the probing is done synchronously and either done,
failed or deferred when returning from of_platform_populate(). Ideally we
would be able to differentiate between a failure and deferral, and not defer
probing in case of an error, however I'm not aware of a way to do this with
the current driver framework.
The call flow is:
of_platform_populate
of_platform_bus_create
of_platform_device_create_pdata
of_device_add
device_add
bus_probe_device
device_initial_probe
__device_attach
__device_attach_driver
driver_probe_device
really_probe
->probe()
> Also, what about acpi? That has the same problem but it isn't covered by
> the dwc3_qcom_of_register_core() function.
I wouldn't be surprised if it had the same problem. I'm not familiar with ACPI
though and don't have a device for testing, hence I limited the patch to the
platform device.
Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change
Hi Sibi,
after doing the review I noticed that Viresh replied on the cover letter
that he picked the series up for v5.9, so I'm not sure if it makes sense
to send a v7.
On Wed, Jun 17, 2020 at 02:35:00AM +0530, Sibi Sankar wrote:
> > > @@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct
> > > device *cpu_dev,
> > >
> > > if (freq != prev_freq && core_count != LUT_TURBO_IND) {
> > > table[i].frequency = freq;
> > > - dev_pm_opp_add(cpu_dev, freq * 1000, volt);
> > > + qcom_cpufreq_update_opp(cpu_dev, freq, volt);
> >
> > This is the cross-validation mentioned above, right? Shouldn't it
> > include
> > a check of the return value?
>
> Yes, this is the cross-validation step,
> we adjust the voltage if opp-tables are
> present/added successfully and enable
> them, else we would just do a add opp.
> We don't want to exit early on a single
> opp failure. We will error out a bit
> later if the opp-count ends up to be
> zero.
At least an error/warning message would seem convenient when adjusting/adding
an OPP fails, otherwise you would only notice by looking at the sysfs
attributes (if you'd even spot a single/few OPPs to be missing).
Re: [PATCH] driver core:Export the symbol device_is_bound
On Wed, Jun 03, 2020 at 12:09:52AM +0530, Sandeep Maheswaram wrote:
> Export the symbol device_is_bound so that it can be used by the modules.
> This change was suggested to solve the allmodconfig build error on adding
> the patch https://lore.kernel.org/patchwork/patch/1218628/
nit: the last two lines aren't particularly interesting in the commit log,
this could be mentioned below '---'.
It might make sense to group the two patches in a series, which would make
the dependency more evident.
> Signed-off-by: Sandeep Maheswaram
> ---
> drivers/base/dd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9a1d940..65d16ce 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -337,6 +337,7 @@ bool device_is_bound(struct device *dev)
> {
> return dev->p && klist_node_attached(>p->knode_driver);
> }
> +EXPORT_SYMBOL_GPL(device_is_bound);
>
> static void driver_bound(struct device *dev)
> {
Reviewed-by: Matthias Kaehlcke
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Tue, Jun 16, 2020 at 10:22:47AM +0530, Sandeep Maheswaram (Temp) wrote:
>
> On 6/16/2020 1:12 AM, Matthias Kaehlcke wrote:
> > On Thu, Jun 04, 2020 at 04:16:31AM -0700, Stephen Boyd wrote:
> > > Quoting Sandeep Maheswaram (Temp) (2020-06-04 02:43:09)
> > > > On 6/3/2020 11:06 PM, Stephen Boyd wrote:
> > > > > Quoting Sandeep Maheswaram (2020-03-31 22:15:43)
> > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > index 1dfd024..d33ae86 100644
> > > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > @@ -285,6 +307,101 @@ static int dwc3_qcom_resume(struct dwc3_qcom
> > > > > > *qcom)
> > > > > > return 0;
> > > > > >}
> > > > > > +
> > > > > > +/**
> > > > > > + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> > > > > > + * @qcom: Pointer to the concerned usb core.
> > > > > > + *
> > > > > > + */
> > > > > > +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> > > > > > +{
> > > > > > + struct device *dev = qcom->dev;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (!device_is_bound(>dwc3->dev))
> > > > > > + return -EPROBE_DEFER;
> > > > > How is this supposed to work? I see that this was added in an earlier
> > > > > revision of this patch series but there isn't any mention of why
> > > > > device_is_bound() is used here. It would be great if there was a
> > > > > comment
> > > > > detailing why this is necessary. It sounds like maximum_speed is
> > > > > important?
> > > > >
> > > > > Furthermore, dwc3_qcom_interconnect_init() is called by
> > > > > dwc3_qcom_probe() which is the function that registers the device for
> > > > > qcom->dwc3->dev. If that device doesn't probe between the time it is
> > > > > registered by dwc3_qcom_probe() and this function is called then we'll
> > > > > fail dwc3_qcom_probe() with -EPROBE_DEFER. And that will remove the
> > > > > qcom->dwc3->dev device from the platform bus because we call
> > > > > of_platform_depopulate() on the error path of dwc3_qcom_probe().
> > > > >
> > > > > So isn't this whole thing racy and can potentially lead us to a driver
> > > > > probe loop where the wrapper (dwc3_qcom) and the core (dwc3) are
> > > > > probing
> > > > > and we're trying to time it just right so that driver for dwc3 binds
> > > > > before we setup interconnects? I don't know if dwc3 can communicate to
> > > > > the wrapper but that would be more of a direct way to do this. Or
> > > > > maybe
> > > > > the wrapper should try to read the DT property for maximum speed and
> > > > > fallback to a worst case high bandwidth value if it can't figure it
> > > > > out
> > > > > itself without help from dwc3 core.
> > > > >
> > > > This was added in V4 to address comments from Matthias in V3
> > > >
> > > > https://patchwork.kernel.org/patch/11148587/
> > > >
> > > Yes, that why I said:
> > >
> > > "I see that this was added in an earlier
> > > revision of this patch series but there isn't any mention of why
> > > device_is_bound() is used here. It would be great if there was a comment
> > > detailing why this is necessary. It sounds like maximum_speed is
> > > important?"
> > >
> > > Can you please respond to the rest of my email?
> > I agree with Stephen that using device_is_bound() isn't a good option
> > in this case, when I suggested it I wasn't looking at the big picture
> > of how probing the core driver is triggered, sorry about that.
> >
> > Reading the speed from the DT with usb_get_maximum_speed() as Stephen
> > suggests would be an option, the inconvenient is that we then
> > essentially require the property to be defined, while the core driver
> > gets a suitable value from hardware registers. Not sure if the wrapper
> > driver could read from the same registers.
> >
> >
[PATCH] usb: dwc3: qcom: Make sure core device is fully initialized before it is used
dwc3_qcom_of_register_core() uses of_platform_populate() to add
the dwc3 core device. The driver core will try to probe the device,
however this might fail (e.g. due to deferred probing) and
of_platform_populate() would still return 0 if the device was
successully added to the platform bus. Verify that the core device
is actually bound to its driver before using it, defer probing of the
dwc3_qcom device if the core device isn't ready (yet).
Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver").
Signed-off-by: Matthias Kaehlcke
---
depends on:
https://lore.kernel.org/patchwork/patch/1251661/ ("driver core:Export
the symbol device_is_bound")
drivers/usb/dwc3/dwc3-qcom.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 1dfd024cd06b..5a9036b050c6 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -537,6 +537,16 @@ static int dwc3_qcom_of_register_core(struct
platform_device *pdev)
return -ENODEV;
}
+ /*
+* A successful return from of_platform_populate() only guarantees that
+* the core device was added to the platform bus, however it might not
+* be bound to its driver (e.g. due to deferred probing). This driver
+* requires the core device to be fully initialized, so defer probing
+* if it isn't ready (yet).
+*/
+ if (!device_is_bound(>dwc3->dev))
+ return -EPROBE_DEFER;
+
return 0;
}
--
2.27.0.290.gba653c62da-goog
Re: [PATCH V4 1/2] mmc: sdhci-msm: Add interconnect bandwidth scaling support
On Tue, Jun 09, 2020 at 02:07:25PM +0530, Pradeep P V K wrote: > Interconnect bandwidth scaling support is now added as a > part of OPP. So, make sure interconnect driver is ready > before handling interconnect scaling. > > Signed-off-by: Pradeep P V K > Reviewed-by: Sibi Sankar Reviewed-by: Matthias Kaehlcke Do you plan to send also patches that add the necessary DT entries? I'm particularly interested in SC7180.
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On Thu, Jun 04, 2020 at 04:16:31AM -0700, Stephen Boyd wrote:
> Quoting Sandeep Maheswaram (Temp) (2020-06-04 02:43:09)
> >
> > On 6/3/2020 11:06 PM, Stephen Boyd wrote:
> > > Quoting Sandeep Maheswaram (2020-03-31 22:15:43)
> > >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > >> index 1dfd024..d33ae86 100644
> > >> --- a/drivers/usb/dwc3/dwc3-qcom.c
> > >> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > >> @@ -285,6 +307,101 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> > >> return 0;
> > >> }
> > >>
> > >> +
> > >> +/**
> > >> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> > >> + * @qcom: Pointer to the concerned usb core.
> > >> + *
> > >> + */
> > >> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> > >> +{
> > >> + struct device *dev = qcom->dev;
> > >> + int ret;
> > >> +
> > >> + if (!device_is_bound(>dwc3->dev))
> > >> + return -EPROBE_DEFER;
> > > How is this supposed to work? I see that this was added in an earlier
> > > revision of this patch series but there isn't any mention of why
> > > device_is_bound() is used here. It would be great if there was a comment
> > > detailing why this is necessary. It sounds like maximum_speed is
> > > important?
> > >
> > > Furthermore, dwc3_qcom_interconnect_init() is called by
> > > dwc3_qcom_probe() which is the function that registers the device for
> > > qcom->dwc3->dev. If that device doesn't probe between the time it is
> > > registered by dwc3_qcom_probe() and this function is called then we'll
> > > fail dwc3_qcom_probe() with -EPROBE_DEFER. And that will remove the
> > > qcom->dwc3->dev device from the platform bus because we call
> > > of_platform_depopulate() on the error path of dwc3_qcom_probe().
> > >
> > > So isn't this whole thing racy and can potentially lead us to a driver
> > > probe loop where the wrapper (dwc3_qcom) and the core (dwc3) are probing
> > > and we're trying to time it just right so that driver for dwc3 binds
> > > before we setup interconnects? I don't know if dwc3 can communicate to
> > > the wrapper but that would be more of a direct way to do this. Or maybe
> > > the wrapper should try to read the DT property for maximum speed and
> > > fallback to a worst case high bandwidth value if it can't figure it out
> > > itself without help from dwc3 core.
> > >
> > This was added in V4 to address comments from Matthias in V3
> >
> > https://patchwork.kernel.org/patch/11148587/
> >
>
> Yes, that why I said:
>
> "I see that this was added in an earlier
> revision of this patch series but there isn't any mention of why
> device_is_bound() is used here. It would be great if there was a comment
> detailing why this is necessary. It sounds like maximum_speed is
> important?"
>
> Can you please respond to the rest of my email?
I agree with Stephen that using device_is_bound() isn't a good option
in this case, when I suggested it I wasn't looking at the big picture
of how probing the core driver is triggered, sorry about that.
Reading the speed from the DT with usb_get_maximum_speed() as Stephen
suggests would be an option, the inconvenient is that we then
essentially require the property to be defined, while the core driver
gets a suitable value from hardware registers. Not sure if the wrapper
driver could read from the same registers.
One option could be to poll device_is_bound() for 100 ms (or so), with
sleeps between polls. It's not elegant but would probably work if we
don't find a better solution.
Re: [PATCH v6 5/5] cpufreq: qcom: Disable fast switch when scaling DDR/L3
On Sat, Jun 06, 2020 at 03:03:32AM +0530, Sibi Sankar wrote: > Disable fast switch when the opp-tables required for scaling DDR/L3 > are populated. > > Signed-off-by: Sibi Sankar not sure a separate patch is needed for this, but anyway: Reviewed-by: Matthias Kaehlcke
Re: [PATCH v6 4/5] cpufreq: qcom: Update the bandwidth levels on frequency change
Hi Sibi,
On Sat, Jun 06, 2020 at 03:03:31AM +0530, Sibi Sankar wrote:
> Add support to parse optional OPP table attached to the cpu node when
> the OPP bandwidth values are populated. This allows for scaling of
> DDR/L3 bandwidth levels with frequency change.
>
> Signed-off-by: Sibi Sankar
> ---
>
> v6:
> * Add global flag to distinguish between voltage update and opp add.
>Use the same flag before trying to scale ddr/l3 bw [Viresh]
> * Use dev_pm_opp_find_freq_ceil to grab all opps [Viresh]
> * Move dev_pm_opp_of_find_icc_paths into probe [Viresh]
>
> v5:
> * Use dev_pm_opp_adjust_voltage instead [Viresh]
> * Misc cleanup
>
> v4:
> * Split fast switch disable into another patch [Lukasz]
>
> drivers/cpufreq/qcom-cpufreq-hw.c | 82 ++-
> 1 file changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
> b/drivers/cpufreq/qcom-cpufreq-hw.c
> index fc92a8842e252..8fa6ab6e0e4b6 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -30,6 +31,48 @@
>
> static unsigned long cpu_hw_rate, xo_rate;
> static struct platform_device *global_pdev;
> +static bool icc_scaling_enabled;
It seem you rely on 'icc_scaling_enabled' to be initialized to 'false'.
This works during the first initialization, but not if the 'device' is
unbound/rebound. In theory things shouldn't be different in a succesive
initialization, however for robustness the variable should be explicitly
set to 'false' somewhere in the code path (_probe(), _read_lut(), ...).
> +static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
> +unsigned long freq_khz)
> +{
> + unsigned long freq_hz = freq_khz * 1000;
> + struct dev_pm_opp *opp;
> + struct device *dev;
> + int ret;
> +
> + dev = get_cpu_device(policy->cpu);
> + if (!dev)
> + return -ENODEV;
> +
> + opp = dev_pm_opp_find_freq_exact(dev, freq_hz, true);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + ret = dev_pm_opp_set_bw(dev, opp);
> + dev_pm_opp_put(opp);
> + return ret;
> +}
> +
> +static int qcom_cpufreq_update_opp(struct device *cpu_dev,
> +unsigned long freq_khz,
> +unsigned long volt)
> +{
> + unsigned long freq_hz = freq_khz * 1000;
> + int ret;
> +
> + /* Skip voltage update if the opp table is not available */
> + if (!icc_scaling_enabled)
> + return dev_pm_opp_add(cpu_dev, freq_hz, volt);
> +
> + ret = dev_pm_opp_adjust_voltage(cpu_dev, freq_hz, volt, volt, volt);
> + if (ret) {
> + dev_err(cpu_dev, "Voltage update failed freq=%ld\n", freq_khz);
> + return ret;
> + }
> +
> + return dev_pm_opp_enable(cpu_dev, freq_hz);
> +}
>
> static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> unsigned int index)
> @@ -39,6 +82,9 @@ static int qcom_cpufreq_hw_target_index(struct
> cpufreq_policy *policy,
>
> writel_relaxed(index, perf_state_reg);
>
> + if (icc_scaling_enabled)
> + qcom_cpufreq_set_bw(policy, freq);
> +
> arch_set_freq_scale(policy->related_cpus, freq,
> policy->cpuinfo.max_freq);
> return 0;
> @@ -89,11 +135,31 @@ static int qcom_cpufreq_hw_read_lut(struct device
> *cpu_dev,
> u32 data, src, lval, i, core_count, prev_freq = 0, freq;
> u32 volt;
> struct cpufreq_frequency_table *table;
> + struct dev_pm_opp *opp;
> + unsigned long rate;
> + int ret;
>
> table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
> if (!table)
> return -ENOMEM;
>
> + ret = dev_pm_opp_of_add_table(cpu_dev);
> + if (!ret) {
> + /* Disable all opps and cross-validate against LUT */
nit: IIUC the cross-validation doesn't happen in this branch, so the
comment is a bit misleading. Maybe change it to "Disable all opps to
cross-validate against the LUT {below,later}".
> + icc_scaling_enabled = true;
> + for (rate = 0; ; rate++) {
> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, );
> + if (IS_ERR(opp))
> + break;
> +
> + dev_pm_opp_put(opp);
> + dev_pm_opp_disable(cpu_dev, rate);
> + }
> + } else if (ret != -ENODEV) {
> + dev_err(cpu_dev, "Invalid opp table in device tree\n");
> + return ret;
> + }
> +
> for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> data = readl_relaxed(base + REG_FREQ_LUT +
> i * LUT_ROW_SIZE);
> @@ -112,7 +178,7 @@ static int qcom_cpufreq_hw_read_lut(struct device
> *cpu_dev,
>
>
Re: [PATCH v6 3/5] OPP: Add and export helper to set bandwidth
On Sat, Jun 06, 2020 at 03:03:30AM +0530, Sibi Sankar wrote: > Add and export 'dev_pm_opp_set_bw' to set the bandwidth > levels associated with an OPP. > > Signed-off-by: Sibi Sankar Reviewed-by: Matthias Kaehlcke
[PATCH] Bluetooth: hci_qca: Simplify determination of serial clock on/off state from votes
The serial clocks should be on when there is a vote for at least one
of the clocks (RX or TX), and off when there is no 'on' vote. The
current logic to determine the combined state is a bit redundant
in the code paths for different types of votes, use a single
statement in the common path instead.
Signed-off-by: Matthias Kaehlcke
---
drivers/bluetooth/hci_qca.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 836949d827ee9..997ddab26a33b 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -289,25 +289,21 @@ static void serial_clock_vote(unsigned long vote, struct
hci_uart *hu)
case HCI_IBS_TX_VOTE_CLOCK_ON:
qca->tx_vote = true;
qca->tx_votes_on++;
- new_vote = true;
break;
case HCI_IBS_RX_VOTE_CLOCK_ON:
qca->rx_vote = true;
qca->rx_votes_on++;
- new_vote = true;
break;
case HCI_IBS_TX_VOTE_CLOCK_OFF:
qca->tx_vote = false;
qca->tx_votes_off++;
- new_vote = qca->rx_vote | qca->tx_vote;
break;
case HCI_IBS_RX_VOTE_CLOCK_OFF:
qca->rx_vote = false;
qca->rx_votes_off++;
- new_vote = qca->rx_vote | qca->tx_vote;
break;
default:
@@ -315,6 +311,8 @@ static void serial_clock_vote(unsigned long vote, struct
hci_uart *hu)
return;
}
+ new_vote = qca->rx_vote | qca->tx_vote;
+
if (new_vote != old_vote) {
if (new_vote)
__serial_clock_on(hu->tty);
--
2.27.0.278.ge193c7cf3a9-goog
Re: [PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed
Hi Bala,
On Sat, Jun 06, 2020 at 06:23:13PM +0530, bgoda...@codeaurora.org wrote:
> Hi Matthias,
>
> On 2020-06-06 00:16, Matthias Kaehlcke wrote:
> > qca_suspend() removes the vote for the UART TX clock after
> > writing an IBS sleep request to the serial buffer. This is
> > not a good idea since there is no guarantee that the request
> > has been sent at this point. Instead remove the vote after
> > successfully entering IBS sleep. This also fixes the issue
> > of the vote being removed in case of an aborted suspend due
> > to a failure of entering IBS sleep.
> >
> > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support")
> > Signed-off-by: Matthias Kaehlcke
> > ---
> >
> > drivers/bluetooth/hci_qca.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index ece9f91cc3deb..b1d82d32892e9 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> >
> > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
> > qca->ibs_sent_slps++;
> > -
> > - qca_wq_serial_tx_clock_vote_off(>ws_tx_vote_off);
> > break;
> >
> > case HCI_IBS_TX_ASLEEP:
> > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP,
> > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS));
> >
> > - if (ret > 0)
> > + if (ret > 0) {
> > + qca_wq_serial_tx_clock_vote_off(>ws_tx_vote_off);
> [Bala]: qca_wq_serial_tx_clock_vote_off votes for Tx clock off, when both Tx
> clock and Rx clock voted to off.
> then only actual call to clock off is called.
> https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L312
> I would recommend to vote Tx clock off after sending SLEEP BYTE from HOST TO
> BT SOC.
Are you suggesting to move the vote after _wait_until_sent() and before
waiting for RX_SLEEP?
I think if the vote is done before RX_SLEEP and going to RX_SLEEP fails the
variables qca->tx_vote and qca->tx_votes_off would have the wrong state, even
if that doesn't lead to actually switching the clock off. I might be missing
something though, I'm not very familiar with this part.
Re: [PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending
Hi Bala,
On Sat, Jun 06, 2020 at 06:27:59PM +0530, bgoda...@codeaurora.org wrote:
> Hi matthias,
>
> On 2020-06-06 00:16, Matthias Kaehlcke wrote:
> > qca_suspend() calls serdev_device_wait_until_sent() regardless of
> > whether a transfer is pending. While it does no active harm since
> > the function should return immediately it makes the code more
> > confusing. Add a flag to track whether a transfer is pending and
> > only call serdev_device_wait_until_sent() is needed.
> >
> > Signed-off-by: Matthias Kaehlcke
> > ---
> >
> > drivers/bluetooth/hci_qca.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index b1d82d32892e9..90ffd8ca1fb0d 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -2050,6 +2050,7 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> > struct hci_uart *hu = >serdev_hu;
> > struct qca_data *qca = hu->priv;
> > unsigned long flags;
> > + bool tx_pending = false;
> > int ret = 0;
> > u8 cmd;
> >
> > @@ -2083,6 +2084,7 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> >
> > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
> > qca->ibs_sent_slps++;
> > + tx_pending = true;
> > break;
> >
> > case HCI_IBS_TX_ASLEEP:
> > @@ -2099,8 +2101,10 @@ static int __maybe_unused qca_suspend(struct
> > device *dev)
> > if (ret < 0)
> > goto error;
> >
> > - serdev_device_wait_until_sent(hu->serdev,
> > - msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> > + if (tx_pending) {
> [Bala]: Good idea why don't we move this call to switch case under
> HCI_IBS_TX_AWAKE
> https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L1994
I agree that this would make the code easier to follow, however the
reason this wasn't done in the first place is (probably) that the
IBS spinlock is held during the switch/case block.
In principle the unlock could be done before calling _wait_until_sent(),
but then the unlock also needs to happen in the other 'case' branches.
It's not ideal, but also not necessarily worse than introducing
'tx_pending', the repeated unlocks might be preferable in terms of
readability.
> i.e. i would recommend below sequence
>
> 1. Send SLEEP BYTE
> 2. wait for some time to write SLEEP Byte on Tx line
> 3. call for Tx clock off qca_wq_serial_tx_clock_vote_off
>
> > + serdev_device_wait_until_sent(hu->serdev,
> > +
> > msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
> > + }
> >
> > /* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is
> > going
> > * to sleep, so that the packet does not wake the system later.
[PATCH 3/3] Bluetooth: hci_qca: Refactor error handling in qca_suspend()
If waiting for IBS sleep times out jump to the error handler, this is
easier to read than multiple 'if' branches and a fall through to the
error handler.
Signed-off-by: Matthias Kaehlcke
---
drivers/bluetooth/hci_qca.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 90ffd8ca1fb0d..cf76f128e9834 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2109,18 +2109,16 @@ static int __maybe_unused qca_suspend(struct device
*dev)
/* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is going
* to sleep, so that the packet does not wake the system later.
*/
-
ret = wait_event_interruptible_timeout(qca->suspend_wait_q,
qca->rx_ibs_state == HCI_IBS_RX_ASLEEP,
msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS));
-
- if (ret > 0) {
- qca_wq_serial_tx_clock_vote_off(>ws_tx_vote_off);
- return 0;
+ if (ret == 0) {
+ ret = -ETIMEDOUT;
+ goto error;
}
- if (ret == 0)
- ret = -ETIMEDOUT;
+ qca_wq_serial_tx_clock_vote_off(>ws_tx_vote_off);
+ return 0;
error:
clear_bit(QCA_SUSPENDING, >flags);
--
2.27.0.278.ge193c7cf3a9-goog
[PATCH 0/3]
This series includes a fix for a possible race in qca_suspend() and some minor refactoring of the same function. Matthias Kaehlcke (3): Bluetooth: hci_qca: Only remove TX clock vote after TX is completed Bluetooth: hci_qca: Skip serdev wait when no transfer is pending Bluetooth: hci_qca: Refactor error handling in qca_suspend() drivers/bluetooth/hci_qca.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) -- 2.27.0.278.ge193c7cf3a9-goog
[PATCH 2/3] Bluetooth: hci_qca: Skip serdev wait when no transfer is pending
qca_suspend() calls serdev_device_wait_until_sent() regardless of
whether a transfer is pending. While it does no active harm since
the function should return immediately it makes the code more
confusing. Add a flag to track whether a transfer is pending and
only call serdev_device_wait_until_sent() is needed.
Signed-off-by: Matthias Kaehlcke
---
drivers/bluetooth/hci_qca.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b1d82d32892e9..90ffd8ca1fb0d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2050,6 +2050,7 @@ static int __maybe_unused qca_suspend(struct device *dev)
struct hci_uart *hu = >serdev_hu;
struct qca_data *qca = hu->priv;
unsigned long flags;
+ bool tx_pending = false;
int ret = 0;
u8 cmd;
@@ -2083,6 +2084,7 @@ static int __maybe_unused qca_suspend(struct device *dev)
qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
qca->ibs_sent_slps++;
+ tx_pending = true;
break;
case HCI_IBS_TX_ASLEEP:
@@ -2099,8 +2101,10 @@ static int __maybe_unused qca_suspend(struct device *dev)
if (ret < 0)
goto error;
- serdev_device_wait_until_sent(hu->serdev,
- msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
+ if (tx_pending) {
+ serdev_device_wait_until_sent(hu->serdev,
+
msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS));
+ }
/* Wait for HCI_IBS_SLEEP_IND sent by device to indicate its Tx is going
* to sleep, so that the packet does not wake the system later.
--
2.27.0.278.ge193c7cf3a9-goog
[PATCH 1/3] Bluetooth: hci_qca: Only remove TX clock vote after TX is completed
qca_suspend() removes the vote for the UART TX clock after
writing an IBS sleep request to the serial buffer. This is
not a good idea since there is no guarantee that the request
has been sent at this point. Instead remove the vote after
successfully entering IBS sleep. This also fixes the issue
of the vote being removed in case of an aborted suspend due
to a failure of entering IBS sleep.
Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support")
Signed-off-by: Matthias Kaehlcke
---
drivers/bluetooth/hci_qca.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index ece9f91cc3deb..b1d82d32892e9 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct device *dev)
qca->tx_ibs_state = HCI_IBS_TX_ASLEEP;
qca->ibs_sent_slps++;
-
- qca_wq_serial_tx_clock_vote_off(>ws_tx_vote_off);
break;
case HCI_IBS_TX_ASLEEP:
@@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct device *dev)
qca->rx_ibs_state == HCI_IBS_RX_ASLEEP,
msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS));
- if (ret > 0)
+ if (ret > 0) {
+ qca_wq_serial_tx_clock_vote_off(>ws_tx_vote_off);
return 0;
+ }
if (ret == 0)
ret = -ETIMEDOUT;
--
2.27.0.278.ge193c7cf3a9-goog
Re: [PATCH V2 1/2] mmc: sdhci-msm: Add interconnect bandwidth scaling support
On Thu, Jun 04, 2020 at 04:44:42PM +0530, Pradeep P V K wrote:
> Interconnect bandwidth scaling support is now added as a
> part of OPP [1]. So, make sure interconnect driver is ready
> before handling interconnect scaling.
>
> This change is based on
> [1] [Patch v8] Introduce OPP bandwidth bindings
> (https://lkml.org/lkml/2020/5/12/493)
>
> [2] [Patch v3] mmc: sdhci-msm: Fix error handling
> for dev_pm_opp_of_add_table()
> (https://lkml.org/lkml/2020/5/5/491)
>
> Signed-off-by: Pradeep P V K
> ---
> drivers/mmc/host/sdhci-msm.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b277dd7..a13ff1b 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
>
> #include "sdhci-pltfm.h"
> #include "cqhci.h"
> @@ -2070,6 +2071,18 @@ static int sdhci_msm_probe(struct platform_device
> *pdev)
> }
> msm_host->bulk_clks[0].clk = clk;
>
> + /* Make sure that ICC driver is ready for interconnect bandwdith
> + * scaling before registering the device for OPP.
> + */
> + ret = dev_pm_opp_of_find_icc_paths(>dev, NULL);
> + if (ret) {
> + if (ret == -EPROBE_DEFER)
> + dev_info(>dev, "defer icc path: %d\n", ret);
I already commented on this on v1:
This log seems to add little more than noise, or are there particular reasons
why it is useful in this driver? Most drivers just return silently in case of
deferred probing.
If you think the log is really needed please explain why.
Re: [PATCH V1 1/2] mmc: sdhci-msm: Add interconnect bandwidth scaling support
Hi Pradeep,
On Wed, Jun 03, 2020 at 02:39:35PM +0530, Pradeep P V K wrote:
> Interconnect bandwidth scaling support is now added as a
> part of OPP [1]. So, make sure interconnect driver is ready
> before handling interconnect scaling.
>
> This change is based on
> [1] [Patch v8] Introduce OPP bandwidth bindings
> (https://lkml.org/lkml/2020/5/12/493)
>
> [2] [Patch v3] mmc: sdhci-msm: Fix error handling
> for dev_pm_opp_of_add_table()
> (https://lkml.org/lkml/2020/5/5/491)
>
> Signed-off-by: Pradeep P V K
> ---
> drivers/mmc/host/sdhci-msm.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b277dd7..bf95484 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
>
> #include "sdhci-pltfm.h"
> #include "cqhci.h"
> @@ -1999,6 +2000,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
> struct sdhci_pltfm_host *pltfm_host;
> struct sdhci_msm_host *msm_host;
> struct clk *clk;
> + struct icc_path *sdhc_path;
nit: The 'sdhc_' prefix doesn't provide any useful information, change it
to 'icc_path', which makes evident what it is?
> int ret;
> u16 host_version, core_minor;
> u32 core_version, config;
> @@ -2070,6 +2072,20 @@ static int sdhci_msm_probe(struct platform_device
> *pdev)
> }
> msm_host->bulk_clks[0].clk = clk;
>
> + /* Make sure that ICC driver is ready for interconnect bandwdith
> + * scaling before registering the device for OPP.
> + */
> + sdhc_path = of_icc_get(>dev, NULL);
> + ret = PTR_ERR_OR_ZERO(sdhc_path);
> + if (ret) {
nit: IMO it would be clearer to do this instead of using PTR_ERR_OR_ZERO()
(as for the OPP table below):
if (IS_ERR(sdhc_path)) {
ret = PTR_ERR(sdhc_path);
> + if (ret == -EPROBE_DEFER)
> + dev_info(>dev, "defer icc path: %d\n", ret);
This log seems to add little more than noise, or are there particular reasons
why it is useful in this driver? Most drivers just return silently in case of
deferred probing.
> + else
> + dev_err(>dev, "failed to get icc path:%d\n", ret);
> + goto bus_clk_disable;
> + }
> + icc_put(sdhc_path);
> +
> msm_host->opp_table = dev_pm_opp_set_clkname(>dev, "core");
> if (IS_ERR(msm_host->opp_table)) {
> ret = PTR_ERR(msm_host->opp_table);
> --
> 1.9.1
>
Re: [PATCH v4] Bluetooth: hci_qca: Improve controller ID info log level
On Fri, May 29, 2020 at 10:46:13PM +0800, Zijun Hu wrote: > Controller ID info got by VSC EDL_PATCH_GETVER is very > important, so improve its log level from DEBUG to INFO. > > Signed-off-by: Zijun Hu As requested earlier, please add the tags from previous versions (in this case my 'Reviewed-by' tag from v2/v3), unless the new patch has substantial changes. Reviewed-by: Matthias Kaehlcke
