Re: [PATCH v5 08/24] wfx: add bus_sdio.c
On Wed, 7 Apr 2021 at 14:00, Kalle Valo wrote:
>
> Ulf Hansson writes:
>
> >> If I follow what has been done in other drivers I would write something
> >> like:
> >>
> >> static int wfx_sdio_suspend(struct device *dev)
> >> {
> >> struct sdio_func *func = dev_to_sdio_func(dev);
> >> struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
> >>
> >> config_reg_write_bits(bus->core, CFG_IRQ_ENABLE_DATA, 0);
> >> // Necessary to keep device firmware in RAM
> >> return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
> >
> > This will tell the mmc/sdio core to keep the SDIO card powered on
> > during system suspend. Thus, it doesn't need to re-initialize it at
> > system resume - and the firmware should not need to be re-programmed.
> >
> > On the other hand, if you don't plan to support system wakeups, it
> > would probably be better to power off the card, to avoid wasting
> > energy while the system is suspended. I assume that means you need to
> > re-program the firmware as well. Normally, it's these kinds of things
> > that need to be managed from a ->resume() callback.
>
> Many mac80211 drivers do so that the device is powered off during
> interface down (ifconfig wlan0 down), and as mac80211 does interface
> down automatically during suspend, suspend then works without extra
> handlers.
That sounds simple. :-)
Would you mind elaborating on what is actually being powered off at
interface down - and thus also I am curious what happens at a typical
interface up?
Even if we don't want to use system wakeups (wake-on-lan), the SDIO
core and the SDIO func driver still need to somewhat agree on how to
manage the power for the card during system suspend, I think.
For example, for a non-removable SDIO card, the SDIO/MMC core may
decide to power off the card in system suspend. Then it needs to
restore power to the card and re-initialize it at system resume, of
course. This doesn't mean that the actual corresponding struct device
for it, gets removed/re-added, thus the SDIO func driver isn't being
re-probed after the system has resumed. Although, since the SDIO card
was re-initialized, it's likely that the FW may need to be
re-programmed after the system has been resumed.
Are you saying that re-programming the FW is always happening at
interface up, when there are none system suspend/resume callbacks
assigned for the SDIO func driver?
Kind regards
Uffe
Re: [PATCH v5 08/24] wfx: add bus_sdio.c
On Tue, 23 Mar 2021 at 18:53, Jérôme Pouiller
wrote:
>
> On Tuesday 23 March 2021 15:11:56 CET Ulf Hansson wrote:
> > On Mon, 22 Mar 2021 at 18:14, Jérôme Pouiller
> > wrote:
> > > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote:
> > > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller
> > > > wrote:
> > > > >
> > > > > From: Jérôme Pouiller
> > > > >
> > > > > Signed-off-by: Jérôme Pouiller
> > > > > ---
> > > > > drivers/net/wireless/silabs/wfx/bus_sdio.c | 259
> > > > > +
> > > > > 1 file changed, 259 insertions(+)
> > > > > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > >
> > > > [...]
> > > >
> > > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS,
> > > > > SDIO_DEVICE_ID_SILABS_WF200) },
> > > > > + { },
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > > > > +
> > > > > +struct sdio_driver wfx_sdio_driver = {
> > > > > + .name = "wfx-sdio",
> > > > > + .id_table = wfx_sdio_ids,
> > > > > + .probe = wfx_sdio_probe,
> > > > > + .remove = wfx_sdio_remove,
> > > > > + .drv = {
> > > > > + .owner = THIS_MODULE,
> > > > > + .of_match_table = wfx_sdio_of_match,
> > > >
> > > > It's not mandatory to support power management, like system
> > > > suspend/resume. However, as this looks like this is a driver for an
> > > > embedded SDIO device, you probably want this.
> > > >
> > > > If that is the case, please assign the dev_pm_ops here and implement
> > > > the ->suspend|resume() callbacks.
> > >
> > > I have no platform to test suspend/resume, so I have only a
> > > theoretical understanding of this subject.
> >
> > I see.
> >
> > >
> > > I understanding is that with the current implementation, the
> > > device will be powered off on suspend and then totally reset
> > > (including reloading of the firmware) on resume. I am wrong?
> >
> > You are correct, for a *removable* SDIO card. In this case, the
> > mmc/sdio core will remove the corresponding SDIO card/device and its
> > corresponding SDIO func devices at system suspend. It will then be
> > redetected at system resume (and the SDIO func driver re-probed).
> >
> > Although, as this is an embedded SDIO device, per definition it's not
> > a removable card (MMC_CAP_NONREMOVABLE should be set for the
> > corresponding mmc host), the SDIO card will stick around and instead
> > the ->suspend|resume() callback needs to be implemented for the SDIO
> > func driver.
>
> If I follow what has been done in other drivers I would write something
> like:
>
> static int wfx_sdio_suspend(struct device *dev)
> {
> struct sdio_func *func = dev_to_sdio_func(dev);
> struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
>
> config_reg_write_bits(bus->core, CFG_IRQ_ENABLE_DATA, 0);
> // Necessary to keep device firmware in RAM
> return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
This will tell the mmc/sdio core to keep the SDIO card powered on
during system suspend. Thus, it doesn't need to re-initialize it at
system resume - and the firmware should not need to be re-programmed.
On the other hand, if you don't plan to support system wakeups, it
would probably be better to power off the card, to avoid wasting
energy while the system is suspended. I assume that means you need to
re-program the firmware as well. Normally, it's these kinds of things
that need to be managed from a ->resume() callback.
> }
>
> However, why not the implementation below?
>
> static int wfx_sdio_suspend(struct device *dev)
> {
> struct sdio_func *func = dev_to_sdio_func(dev);
>
> wfx_sdio_remove(func);
I don't know what wfx_sdio_remove() does, but for sure you would need
a ->resume() callback to make it possible to restore power/firmware.
> return 0;
> }
>
> In both cases, I worry to provide these functions without being able to
> test them.
Alright, let's simply leave this driver without having the PM
callbacks assigned. I guess we can revisit this at some later point.
The mmc core will log a message about the missing callbacks, in case
someone tries to execute system suspend/resume when the driver has
been probed.
Kind regards
Uffe
Re: [PATCH v5 08/24] wfx: add bus_sdio.c
On Mon, 22 Mar 2021 at 18:14, Jérôme Pouiller
wrote:
>
> Hello Ulf,
>
> On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote:
> > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller
> > wrote:
> > >
> > > From: Jérôme Pouiller
> > >
> > > Signed-off-by: Jérôme Pouiller
> > > ---
> > > drivers/net/wireless/silabs/wfx/bus_sdio.c | 259 +
> > > 1 file changed, 259 insertions(+)
> > > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> >
> > [...]
> >
> > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200)
> > > },
> > > + { },
> > > +};
> > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > > +
> > > +struct sdio_driver wfx_sdio_driver = {
> > > + .name = "wfx-sdio",
> > > + .id_table = wfx_sdio_ids,
> > > + .probe = wfx_sdio_probe,
> > > + .remove = wfx_sdio_remove,
> > > + .drv = {
> > > + .owner = THIS_MODULE,
> > > + .of_match_table = wfx_sdio_of_match,
> >
> > It's not mandatory to support power management, like system
> > suspend/resume. However, as this looks like this is a driver for an
> > embedded SDIO device, you probably want this.
> >
> > If that is the case, please assign the dev_pm_ops here and implement
> > the ->suspend|resume() callbacks.
>
> I have no platform to test suspend/resume, so I have only a
> theoretical understanding of this subject.
I see.
>
> I understanding is that with the current implementation, the
> device will be powered off on suspend and then totally reset
> (including reloading of the firmware) on resume. I am wrong?
You are correct, for a *removable* SDIO card. In this case, the
mmc/sdio core will remove the corresponding SDIO card/device and its
corresponding SDIO func devices at system suspend. It will then be
redetected at system resume (and the SDIO func driver re-probed).
Although, as this is an embedded SDIO device, per definition it's not
a removable card (MMC_CAP_NONREMOVABLE should be set for the
corresponding mmc host), the SDIO card will stick around and instead
the ->suspend|resume() callback needs to be implemented for the SDIO
func driver.
>
> This behavior sounds correct to me. You would expect something
> more?
Yes, see above.
Kind regards
Uffe
Re: [PATCH v5 08/24] wfx: add bus_sdio.c
On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller
wrote:
>
> From: Jérôme Pouiller
>
> Signed-off-by: Jérôme Pouiller
> ---
> drivers/net/wireless/silabs/wfx/bus_sdio.c | 259 +
> 1 file changed, 259 insertions(+)
> create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
[...]
> +static const struct sdio_device_id wfx_sdio_ids[] = {
> + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> + { },
> +};
> +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> +
> +struct sdio_driver wfx_sdio_driver = {
> + .name = "wfx-sdio",
> + .id_table = wfx_sdio_ids,
> + .probe = wfx_sdio_probe,
> + .remove = wfx_sdio_remove,
> + .drv = {
> + .owner = THIS_MODULE,
> + .of_match_table = wfx_sdio_of_match,
It's not mandatory to support power management, like system
suspend/resume. However, as this looks like this is a driver for an
embedded SDIO device, you probably want this.
If that is the case, please assign the dev_pm_ops here and implement
the ->suspend|resume() callbacks.
[...]
Kind regards
Uffe
Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter
On Mon, 9 Nov 2020 at 16:54, Rafael J. Wysocki wrote: > > On Mon, Nov 9, 2020 at 4:50 PM Ulf Hansson wrote: > > > > On Mon, 9 Nov 2020 at 16:20, Rafael J. Wysocki wrote: > > > > > > On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong > > > wrote: > > > > > > > > In many case, we need to check return value of pm_runtime_get_sync, but > > > > it brings a trouble to the usage counter processing. Many callers forget > > > > to decrease the usage counter when it failed. It has been discussed a > > > > lot[0][1]. So we add a function to deal with the usage counter for > > > > better > > > > coding. > > > > > > > > [0]https://lkml.org/lkml/2020/6/14/88 > > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao@zju.edu.cn/ > > > > Signed-off-by: Zhang Qilong > > > > --- > > > > include/linux/pm_runtime.h | 30 ++ > > > > 1 file changed, 30 insertions(+) > > > > > > > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > > > > index 4b708f4e8eed..6549ce764400 100644 > > > > --- a/include/linux/pm_runtime.h > > > > +++ b/include/linux/pm_runtime.h > > > > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct > > > > device *dev) > > > > return __pm_runtime_resume(dev, RPM_GET_PUT); > > > > } > > > > > > > > +/** > > > > + * pm_runtime_general_get - Bump up usage counter of a device and > > > > resume it. > > > > + * @dev: Target device. > > > > + * > > > > + * Increase runtime PM usage counter of @dev first, and carry out > > > > runtime-resume > > > > + * of it synchronously. If __pm_runtime_resume return negative > > > > value(device is in > > > > + * error state), we to need decrease the usage counter before it > > > > return. If > > > > + * __pm_runtime_resume return positive value, it means the runtime of > > > > device has > > > > + * already been in active state, and we let the new wrapper return > > > > zero instead. > > > > + * > > > > + * The possible return values of this function is zero or negative > > > > value. > > > > + * zero: > > > > + *- it means resume succeeed or runtime of device has already been > > > > active, the > > > > + * runtime PM usage counter of @dev remains incremented. > > > > + * negative: > > > > + *- it means failure and the runtime PM usage counter of @dev has > > > > been balanced. > > > > > > The kerneldoc above is kind of noisy and it is hard to figure out what > > > the helper really does from it. > > > > > > You could basically say something like "Resume @dev synchronously and > > > if that is successful, increment its runtime PM usage counter. Return > > > 0 if the runtime PM usage counter of @dev has been incremented or a > > > negative error code otherwise." > > > > > > > + */ > > > > +static inline int pm_runtime_general_get(struct device *dev) > > > > > > What about pm_runtime_resume_and_get()? > > > > We already have pm_runtime_get_if_active() - so perhaps > > pm_runtime_get_if_suspended() could be an option as well? > > It doesn't work this way, though. > > The "get" happens even if the device has not been suspended. Yes, that's right - so pm_runtime_resume_and_get() is probably the best we can pick then. Kind regards Uffe
Re: [PATCH v2 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter
On Mon, 9 Nov 2020 at 16:20, Rafael J. Wysocki wrote:
>
> On Mon, Nov 9, 2020 at 4:00 PM Zhang Qilong wrote:
> >
> > In many case, we need to check return value of pm_runtime_get_sync, but
> > it brings a trouble to the usage counter processing. Many callers forget
> > to decrease the usage counter when it failed. It has been discussed a
> > lot[0][1]. So we add a function to deal with the usage counter for better
> > coding.
> >
> > [0]https://lkml.org/lkml/2020/6/14/88
> > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520095148.10995-1-dinghao@zju.edu.cn/
> > Signed-off-by: Zhang Qilong
> > ---
> > include/linux/pm_runtime.h | 30 ++
> > 1 file changed, 30 insertions(+)
> >
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index 4b708f4e8eed..6549ce764400 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -386,6 +386,36 @@ static inline int pm_runtime_get_sync(struct device
> > *dev)
> > return __pm_runtime_resume(dev, RPM_GET_PUT);
> > }
> >
> > +/**
> > + * pm_runtime_general_get - Bump up usage counter of a device and resume
> > it.
> > + * @dev: Target device.
> > + *
> > + * Increase runtime PM usage counter of @dev first, and carry out
> > runtime-resume
> > + * of it synchronously. If __pm_runtime_resume return negative
> > value(device is in
> > + * error state), we to need decrease the usage counter before it return. If
> > + * __pm_runtime_resume return positive value, it means the runtime of
> > device has
> > + * already been in active state, and we let the new wrapper return zero
> > instead.
> > + *
> > + * The possible return values of this function is zero or negative value.
> > + * zero:
> > + *- it means resume succeeed or runtime of device has already been
> > active, the
> > + * runtime PM usage counter of @dev remains incremented.
> > + * negative:
> > + *- it means failure and the runtime PM usage counter of @dev has been
> > balanced.
>
> The kerneldoc above is kind of noisy and it is hard to figure out what
> the helper really does from it.
>
> You could basically say something like "Resume @dev synchronously and
> if that is successful, increment its runtime PM usage counter. Return
> 0 if the runtime PM usage counter of @dev has been incremented or a
> negative error code otherwise."
>
> > + */
> > +static inline int pm_runtime_general_get(struct device *dev)
>
> What about pm_runtime_resume_and_get()?
We already have pm_runtime_get_if_active() - so perhaps
pm_runtime_get_if_suspended() could be an option as well?
>
> > +{
> > + int ret = 0;
>
> This extra initialization is not necessary.
>
> You can initialize ret to the __pm_runtime_resume() return value right away.
>
> > +
> > + ret = __pm_runtime_resume(dev, RPM_GET_PUT);
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(dev);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * pm_runtime_put - Drop device usage counter and queue up "idle check" if
> > 0.
> > * @dev: Target device.
> > --
Kind regards
Uffe
Re: [PATCH v2 01/24] mmc: sdio: add SDIO IDs for Silabs WF200 chip
On Tue, 20 Oct 2020 at 14:58, Jerome Pouiller wrote: > > From: Jérôme Pouiller > > Add Silabs SDIO ID to sdio_ids.h. > > Note that the values used by Silabs are uncommon. A driver cannot fully > rely on the SDIO PnP. It should also check if the device is declared in > the DT. > > Signed-off-by: Jérôme Pouiller Acked-by: Ulf Hansson Kind regards Uffe > --- > include/linux/mmc/sdio_ids.h | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h > index 12036619346c..20a48162f7fc 100644 > --- a/include/linux/mmc/sdio_ids.h > +++ b/include/linux/mmc/sdio_ids.h > @@ -25,6 +25,11 @@ > * Vendors and devices. Sort key: vendor first, device next. > */ > > +// Silabs does not use a reliable vendor ID. To avoid conflicts, the driver > +// won't probe the device if it is not also declared in the DT. > +#define SDIO_VENDOR_ID_SILABS 0x > +#define SDIO_DEVICE_ID_SILABS_WF2000x1000 > + > #define SDIO_VENDOR_ID_STE 0x0020 > #define SDIO_DEVICE_ID_STE_CW1200 0x2280 > > -- > 2.28.0 >
Re: [PATCH 07/23] wfx: add bus_sdio.c
On Thu, 15 Oct 2020 at 16:03, Jérôme Pouiller
wrote:
>
> On Wednesday 14 October 2020 14:43:34 CEST Pali Rohár wrote:
> > On Wednesday 14 October 2020 13:52:15 Jérôme Pouiller wrote:
> > > On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:
> > > > On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> > > > > +#define SDIO_VENDOR_ID_SILABS0x
> > > > > +#define SDIO_DEVICE_ID_SILABS_WF200 0x1000
> > > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS,
> > > > > SDIO_DEVICE_ID_SILABS_WF200) },
> > > >
> > > > Please move ids into common include file include/linux/mmc/sdio_ids.h
> > > > where are all SDIO ids. Now all drivers have ids defined in that file.
> > > >
> > > > > + // FIXME: ignore VID/PID and only rely on device tree
> > > > > + // { SDIO_DEVICE(SDIO_ANY_ID, SDIO_ANY_ID) },
> > > >
> > > > What is the reason for ignoring vendor and device ids?
> > >
> > > The device has a particularity, its VID/PID is :1000 (as you can see
> > > above). This value is weird. The risk of collision with another device is
> > > high.
> >
> > Those ids looks strange. You are from Silabs, can you check internally
> > in Silabs if ids are really correct? And which sdio vendor id you in
> > Silabs got assigned for your products?
>
> I confirm these ids are the ones burned in the WF200. We have to deal with
> that :( .
Yep. Unfortunately this isn't so uncommon when targeting the embedded
types of devices.
The good thing is, that we already have bindings allowing us to specify this.
>
>
> > I know that sdio devices with multiple functions may have different sdio
> > vendor/device id particular function and in common CIS (function 0).
> >
> > Could not be a problem that on one place is vendor/device id correct and
> > on other place is that strange value?
> >
> > I have sent following patch (now part of upstream kernel) which exports
> > these ids to userspace:
> > https://lore.kernel.org/linux-mmc/20200527110858.17504-2-p...@kernel.org/T/#u
> >
> > Also for debugging ids and information about sdio cards, I sent another
> > patch which export additional data:
> > https://lore.kernel.org/linux-mmc/20200727133837.19086-1-p...@kernel.org/T/#u
> >
> > Could you try them and look at /sys/class/mmc_host/ attribute outputs?
>
> Here is:
>
> # cd /sys/class/mmc_host/ && grep -r . mmc1/
> mmc1/power/runtime_suspended_time:0
> grep: mmc1/power/autosuspend_delay_ms: Input/output error
> mmc1/power/runtime_active_time:0
> mmc1/power/control:auto
> mmc1/power/runtime_status:unsupported
> mmc1/mmc1:0001/vendor:0x
> mmc1/mmc1:0001/rca:0x0001
> mmc1/mmc1:0001/device:0x1000
> mmc1/mmc1:0001/mmc1:0001:1/vendor:0x
> mmc1/mmc1:0001/mmc1:0001:1/device:0x1000
> grep: mmc1/mmc1:0001/mmc1:0001:1/info4: No data available
> mmc1/mmc1:0001/mmc1:0001:1/power/runtime_suspended_time:0
> grep: mmc1/mmc1:0001/mmc1:0001:1/power/autosuspend_delay_ms: Input/output
> error
> mmc1/mmc1:0001/mmc1:0001:1/power/runtime_active_time:0
> mmc1/mmc1:0001/mmc1:0001:1/power/control:auto
> mmc1/mmc1:0001/mmc1:0001:1/power/runtime_status:unsupported
> mmc1/mmc1:0001/mmc1:0001:1/class:0x00
> grep: mmc1/mmc1:0001/mmc1:0001:1/info2: No data available
> mmc1/mmc1:0001/mmc1:0001:1/modalias:sdio:c00vd1000
> mmc1/mmc1:0001/mmc1:0001:1/revision:0.0
> mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_NAME=mmc
> mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_FULLNAME=/soc/sdhci@7e30/mmc@1
> mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_COMPATIBLE_0=silabs,wfx-sdio
> mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_COMPATIBLE_N=1
> mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_CLASS=00
> mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_ID=:1000
> mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_REVISION=0.0
> mmc1/mmc1:0001/mmc1:0001:1/uevent:MODALIAS=sdio:c00vd1000
> grep: mmc1/mmc1:0001/mmc1:0001:1/info3: No data available
> grep: mmc1/mmc1:0001/mmc1:0001:1/info1: No data available
> mmc1/mmc1:0001/ocr:0x0020
> grep: mmc1/mmc1:0001/info4: No data available
> mmc1/mmc1:0001/power/runtime_suspended_time:0
> grep: mmc1/mmc1:0001/power/autosuspend_delay_ms: Input/output error
> mmc1/mmc1:0001/power/runtime_active_time:0
> mmc1/mmc1:0001/power/control:auto
> mmc1/mmc1:0001/power/runtime_status:unsupported
> grep: mmc1/mmc1:0001/info2: No data available
> mmc1/mmc1:0001/type:SDIO
> mmc1/mmc1:0001/revision:0.0
> mmc1/mmc1:0001/uevent:MMC_TYPE=SDIO
> mmc1/mmc1:0001/uevent:SDIO_ID=:1000
> mmc1/mmc1:0001/uevent:SDIO_REVISION=0.0
> grep: mmc1/mmc1:0001/info3: No data available
> grep: mmc1/mmc1:0001/info1: No data available
>
>
Please have a look at the
Documentation/devicetree/bindings/mmc/mmc-controller.yaml, there you
find that from a child node of the mmc host's node, we can specify an
embedded SDIO functional device.
In sdio_add_func(), whi
Re: [PATCH 07/23] wfx: add bus_sdio.c
On Mon, 12 Oct 2020 at 12:47, Jerome Pouiller
wrote:
>
> From: Jérôme Pouiller
Please fill out this commit message to explain a bit more about the
patch and the HW it enables support for.
>
> Signed-off-by: Jérôme Pouiller
> ---
> drivers/net/wireless/silabs/wfx/bus_sdio.c | 269 +
> 1 file changed, 269 insertions(+)
> create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
>
> diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c
> b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> new file mode 100644
> index ..e06d7e1ebe9c
[...]
> +
> +static int wfx_sdio_irq_subscribe(void *priv)
> +{
> + struct wfx_sdio_priv *bus = priv;
> + u32 flags;
> + int ret;
> + u8 cccr;
> +
I would appreciate a comment about an out-of-band IRQ line. If it's
supported, that is the preferred option to use, else the SDIO IRQs.
> + if (!bus->of_irq) {
> + sdio_claim_host(bus->func);
> + ret = sdio_claim_irq(bus->func, wfx_sdio_irq_handler);
> + sdio_release_host(bus->func);
> + return ret;
> + }
> +
> + sdio_claim_host(bus->func);
> + cccr = sdio_f0_readb(bus->func, SDIO_CCCR_IENx, NULL);
> + cccr |= BIT(0);
> + cccr |= BIT(bus->func->num);
> + sdio_f0_writeb(bus->func, cccr, SDIO_CCCR_IENx, NULL);
> + sdio_release_host(bus->func);
> + flags = irq_get_trigger_type(bus->of_irq);
> + if (!flags)
> + flags = IRQF_TRIGGER_HIGH;
> + flags |= IRQF_ONESHOT;
> + return devm_request_threaded_irq(&bus->func->dev, bus->of_irq, NULL,
> +wfx_sdio_irq_handler_ext, flags,
> +"wfx", bus);
> +}
> +
[...]
> +
> +#define SDIO_VENDOR_ID_SILABS0x
> +#define SDIO_DEVICE_ID_SILABS_WF200 0x1000
> +static const struct sdio_device_id wfx_sdio_ids[] = {
> + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> + // FIXME: ignore VID/PID and only rely on device tree
> + // { SDIO_DEVICE(SDIO_ANY_ID, SDIO_ANY_ID) },
> + { },
> +};
> +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
I will comment about the sdio IDs separately, replying to the other
thread between you and Pali.
> +
> +struct sdio_driver wfx_sdio_driver = {
> + .name = "wfx-sdio",
> + .id_table = wfx_sdio_ids,
> + .probe = wfx_sdio_probe,
> + .remove = wfx_sdio_remove,
> + .drv = {
> + .owner = THIS_MODULE,
> + .of_match_table = wfx_sdio_of_match,
> + }
> +};
I couldn't find where you call sdio_register|unregister_driver(), but
maybe that's done from another patch in series?
Kind regards
Uffe
Re: [PATCH] dt-bindings: Another round of adding missing 'additionalProperties'
On Sat, 3 Oct 2020 at 01:41, Rob Herring wrote: > > Another round of wack-a-mole. The json-schema default is additional > unknown properties are allowed, but for DT all properties should be > defined. > > Cc: Thierry Reding > Cc: Linus Walleij > Cc: Stephen Boyd > Cc: Shawn Guo > Cc: Bjorn Andersson > Cc: Baolin Wang > Cc: Guenter Roeck > Cc: Jonathan Cameron > Cc: Mauro Carvalho Chehab > Cc: Laurent Pinchart > Cc: Lee Jones > Cc: Ulf Hansson > Cc: "David S. Miller" > Cc: Bjorn Helgaas > Cc: Vinod Koul > Cc: Liam Girdwood > Cc: Mark Brown > Cc: Greg Kroah-Hartman > Cc: Daniel Lezcano > Cc: linux-...@vger.kernel.org > Cc: dri-de...@lists.freedesktop.org > Cc: linux-...@vger.kernel.org > Cc: linux-g...@vger.kernel.org > Cc: linux-hw...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: openipmi-develo...@lists.sourceforge.net > Cc: linux-l...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-m...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: netdev@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: linux-remotep...@vger.kernel.org > Cc: linux-ser...@vger.kernel.org > Cc: alsa-de...@alsa-project.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Rob Herring > --- > > I'll take this thru the DT tree. > [...] > .../bindings/mmc/mmc-pwrseq-emmc.yaml | 2 ++ > .../bindings/mmc/mmc-pwrseq-sd8787.yaml | 2 ++ > .../bindings/mmc/mmc-pwrseq-simple.yaml | 2 ++ Acked-by: Ulf Hansson Kind regards Uffe
Re: [patch 24/35] net: brcmfmac: Replace in_interrupt()
On Mon, 28 Sep 2020 at 09:35, Arend Van Spriel wrote: > > + Uffe > > On 9/27/2020 9:49 PM, Thomas Gleixner wrote: > > @@ -85,7 +85,7 @@ static void brcmf_sdiod_ib_irqhandler(st > > > > brcmf_dbg(INTR, "IB intr triggered\n"); > > > > - brcmf_sdio_isr(sdiodev->bus); > > + brcmf_sdio_isr(sdiodev->bus, false); > > } > > Hi Uffe, > > I assume the above code is okay, but want to confirm. Is the SDIO > interrupt guaranteed to be on a worker thread? Correct. As a matter of fact, the sdio irqs can be delivered through a couple of different paths. The legacy (scheduled for removal), is from a dedicated kthread. The more "modern" way is either from the context of a threaded IRQ handler or via a workqueue. However, there are also so-called out of band SDIO irqs, typically routed via a separate GPIO line. This isn't managed by the MMC/SDIO subsystem, but the SDIO functional driver itself. Kind regards Uffe
Re: linux-next: build failure after merge of the mmc tree
+ Linus
On Tue, 2 Jun 2020 at 05:44, Stephen Rothwell wrote:
>
> Hi all,
>
> After merging the mmc tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
>
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c: In function
> 'brcmf_sdiod_probe':
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:915:7: error:
> 'SDIO_DEVICE_ID_CYPRESS_4373' undeclared (first use in this function); did
> you mean 'SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373'?
> 915 | case SDIO_DEVICE_ID_CYPRESS_4373:
> | ^~~
> | SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c:915:7: note: each
> undeclared identifier is reported only once for each function it appears in
>
> Caused by commit
>
> 1eb911258805 ("mmc: sdio: Fix Cypress SDIO IDs macros in common include
> file")
>
> interacting with commit
>
> 2a7621ded321 ("brcmfmac: set F2 blocksize for 4373")
>
> from the net-next tree.
>
> I have applied the following merge fix patch.
>
> From: Stephen Rothwell
> Date: Tue, 2 Jun 2020 13:41:04 +1000
> Subject: [PATCH] mmc: sdio: merge fix for "brcmfmac: set F2 blocksize for
> 4373"
>
> Signed-off-by: Stephen Rothwell
Thanks Stephen for fixing and reporting about this!
Looks like the fix is rather trivial, so I assume Linus can
cherry-pick your patch, while merging my pull request for mmc for
v5.8. In any case, I will monitor the process and send a fix on top,
if needed.
Kind regards
Uffe
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index e718bd466830..46346cb3bc84 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -912,7 +912,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev
> *sdiodev)
> goto out;
> }
> switch (sdiodev->func2->device) {
> - case SDIO_DEVICE_ID_CYPRESS_4373:
> + case SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373:
> f2_blksz = SDIO_4373_FUNC2_BLOCKSIZE;
> break;
> case SDIO_DEVICE_ID_BROADCOM_4359:
> --
> 2.26.2
>
> --
> Cheers,
> Stephen Rothwell
Re: [PATCH 04/17] dt-bindings: mmc: renesas,sdhi: Document r8a7742 support
On Fri, 15 May 2020 at 17:09, Lad Prabhakar wrote: > > Document SDHI controller for RZ/G1H (R8A7742) SoC, which is compatible > with R-Car Gen2 SoC family. > > Signed-off-by: Lad Prabhakar > Reviewed-by: Marian-Cristian Rotariu > Applied for next, thanks! Kind regards Uffe > --- > Documentation/devicetree/bindings/mmc/renesas,sdhi.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/mmc/renesas,sdhi.txt > b/Documentation/devicetree/bindings/mmc/renesas,sdhi.txt > index e6cc478..0ca9a62 100644 > --- a/Documentation/devicetree/bindings/mmc/renesas,sdhi.txt > +++ b/Documentation/devicetree/bindings/mmc/renesas,sdhi.txt > @@ -7,6 +7,7 @@ Required properties: > "renesas,sdhi-r7s9210" - SDHI IP on R7S9210 SoC > "renesas,sdhi-r8a73a4" - SDHI IP on R8A73A4 SoC > "renesas,sdhi-r8a7740" - SDHI IP on R8A7740 SoC > + "renesas,sdhi-r8a7742" - SDHI IP on R8A7742 SoC > "renesas,sdhi-r8a7743" - SDHI IP on R8A7743 SoC > "renesas,sdhi-r8a7744" - SDHI IP on R8A7744 SoC > "renesas,sdhi-r8a7745" - SDHI IP on R8A7745 SoC > -- > 2.7.4 >
Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac
On 5 April 2018 at 15:10, Kalle Valo wrote:
> Ulf Hansson writes:
>
>> On 20 March 2018 at 10:55, Kalle Valo wrote:
>>> Arend van Spriel writes:
>>>
>>>>>> If I get it right, you mean something like this:
>>>>>>
>>>>>> mmc3: mmc@1c12000 {
>>>>>> ...
>>>>>> broken-sg-support;
>>>>>> sd-head-align = 4;
>>>>>> sd-sgentry-align = 512;
>>>>>>
>>>>>> brcmf: wifi@1 {
>>>>>> ...
>>>>>> };
>>>>>> };
>>>>>>
>>>>>> Where dt: bindings documentation for these entries should reside?
>>>>>> In generic MMC bindings? Well, this is the very special case and
>>>>>> mmc-linux maintainer will unlikely to accept these changes.
>>>>>> Also, extra kernel code modification might be required. It could make
>>>>>> quite trivial change much more complex.
>>>>>
>>>>> If the MMC maintainers are not copied on this patch series, it will
>>>>> likely be hard for them to identify this patch series and chime in...
>>>>
>>>> The main question is whether this is indeed a "very special case" as
>>>> Alexey claims it to be or that it is likely to be applicable to other
>>>> device and host combinations as you are suggesting.
>>>>
>>>> If these properties are imposed by the host or host controller it
>>>> would make sense to have these in the mmc bindings.
>>>
>>> BTW, last year we were discussing something similar (I mean related to
>>> alignment requirements) with ath10k SDIO patches and at the time the
>>> patch submitter was proposing to have a bounce buffer in ath10k to
>>> workaround that. I don't remember the details anymore, they are on the
>>> ath10k mailing list archive if anyone is curious to know, but I would
>>> not be surprised if they are similar as here. So there might be a need
>>> to solve this in a generic way (but not sure of course as I haven't
>>> checked the details).
>>
>> I re-call something about these as well, here are the patches. Perhaps
>> I should pick some of them up...
>>
>> https://patchwork.kernel.org/patch/10123137/
>> https://patchwork.kernel.org/patch/10123139/
>> https://patchwork.kernel.org/patch/10123141/
>> https://patchwork.kernel.org/patch/10123143/
>
> Actually I was talking about a different patch, found it now:
>
> ath10k_sdio: DMA bounce buffers for read write
>
> https://patchwork.kernel.org/patch/9979543/
Ah, yes. This is about buffer alignment, particularly when using DMA.
Normally there should be no constraint on the alignment, if the
mmc/sdio controller driver would implement a fallback mechanism from
DMA to PIO mode, in case the buffer can't be used for DMA.
However, I know about cases where simply PIO doesn't work because of
broken HW and in many cases the mmc drivers don't implement the
fallback to PIO even if they could.
Moreover, it seems reasonable to anyway have a way for mmc driver to
inform upper layers about DMA buffer alignment constraints, as to be
able to use DMA as long as possible.
Thoughts?
Kind regards
Uffe
Re: [PATCH 00/15] ARM: pxa: switch to DMA slave maps
On 4 April 2018 at 21:56, Boris Brezillon wrote: > On Wed, 04 Apr 2018 21:49:26 +0200 > Robert Jarzmik wrote: > >> Ulf Hansson writes: >> >> > On 2 April 2018 at 16:26, Robert Jarzmik wrote: >> >> Hi, >> >> >> >> This serie is aimed at removing the dmaengine slave compat use, and >> >> transfer >> >> knowledge of the DMA requestors into architecture code. >> >> As this looks like a patch bomb, each maintainer expressing for his tree >> >> either >> >> an Ack or "I want to take through my tree" will be spared in the next >> >> iterations >> >> of this serie. >> > >> > Perhaps an option is to send this hole series as PR for 3.17 rc1, that >> > would removed some churns and make this faster/easier? Well, if you >> > receive the needed acks of course. >> For 3.17-rc1 it looks a bit optimistic with the review time ... If I have all > > Especially since 3.17-rc1 has been released more than 3 years ago :-), > but I guess you meant 4.17-rc1. Yeah, I realize that I was a bit lost in time yesterday. Even more people have been having fun about it (me too). :-) Kind regards Uffe
Re: [PATCH 00/15] ARM: pxa: switch to DMA slave maps
On 2 April 2018 at 16:26, Robert Jarzmik wrote: > Hi, > > This serie is aimed at removing the dmaengine slave compat use, and transfer > knowledge of the DMA requestors into architecture code. > > This was discussed/advised by Arnd a couple of years back, it's almost time. > > The serie is divided in 3 phasees : > - phase 1 : patch 1/15 and patch 2/15 >=> this is the preparation work > - phase 2 : patches 3/15 .. 10/15 >=> this is the switch of all the drivers >=> this one will require either an Ack of the maintainers or be taken by > them > once phase 1 is merged > - phase 3 : patches 11/15 >=> this is the last part, cleanup and removal of export of the DMA filter > function > > As this looks like a patch bomb, each maintainer expressing for his tree > either > an Ack or "I want to take through my tree" will be spared in the next > iterations > of this serie. Perhaps an option is to send this hole series as PR for 3.17 rc1, that would removed some churns and make this faster/easier? Well, if you receive the needed acks of course. For the mmc change: Acked-by: Ulf Hansson Kind regards Uffe > > Several of these changes have been tested on actual hardware, including : > - pxamci > - pxa_camera > - smc* > - ASoC and SSP > > Happy review. > > Robert Jarzmik (15): > dmaengine: pxa: use a dma slave map > ARM: pxa: add dma slave map > mmc: pxamci: remove the dmaengine compat need > media: pxa_camera: remove the dmaengine compat need > mtd: nand: pxa3xx: remove the dmaengine compat need > net: smc911x: remove the dmaengine compat need > net: smc91x: remove the dmaengine compat need > ASoC: pxa: remove the dmaengine compat need > net: irda: pxaficp_ir: remove the dmaengine compat need > ata: pata_pxa: remove the dmaengine compat need > dmaengine: pxa: document pxad_param > dmaengine: pxa: make the filter function internal > ARM: pxa: remove the DMA IO resources > ARM: pxa: change SSP devices allocation > ARM: pxa: change SSP DMA channels allocation > > arch/arm/mach-pxa/devices.c | 269 > ++ > arch/arm/mach-pxa/devices.h | 14 +- > arch/arm/mach-pxa/include/mach/audio.h| 12 ++ > arch/arm/mach-pxa/pxa25x.c| 4 +- > arch/arm/mach-pxa/pxa27x.c| 4 +- > arch/arm/mach-pxa/pxa3xx.c| 5 +- > arch/arm/plat-pxa/ssp.c | 50 +- > drivers/ata/pata_pxa.c| 10 +- > drivers/dma/pxa_dma.c | 13 +- > drivers/media/platform/pxa_camera.c | 22 +-- > drivers/mmc/host/pxamci.c | 29 +--- > drivers/mtd/nand/pxa3xx_nand.c| 10 +- > drivers/net/ethernet/smsc/smc911x.c | 16 +- > drivers/net/ethernet/smsc/smc91x.c| 12 +- > drivers/net/ethernet/smsc/smc91x.h| 1 - > drivers/staging/irda/drivers/pxaficp_ir.c | 14 +- > include/linux/dma/pxa-dma.h | 20 +-- > include/linux/platform_data/mmp_dma.h | 4 + > include/linux/pxa2xx_ssp.h| 4 +- > sound/arm/pxa2xx-ac97.c | 14 +- > sound/arm/pxa2xx-pcm-lib.c| 6 +- > sound/soc/pxa/pxa-ssp.c | 5 +- > sound/soc/pxa/pxa2xx-ac97.c | 32 +--- > 23 files changed, 196 insertions(+), 374 deletions(-) > > -- > 2.11.0 >
Re: [PATCH net-next v2 2/2] dt: bindings: add new dt entries for brcmfmac
On 20 March 2018 at 10:55, Kalle Valo wrote:
> Arend van Spriel writes:
>
If I get it right, you mean something like this:
mmc3: mmc@1c12000 {
...
broken-sg-support;
sd-head-align = 4;
sd-sgentry-align = 512;
brcmf: wifi@1 {
...
};
};
Where dt: bindings documentation for these entries should reside?
In generic MMC bindings? Well, this is the very special case and
mmc-linux maintainer will unlikely to accept these changes.
Also, extra kernel code modification might be required. It could make
quite trivial change much more complex.
>>>
>>> If the MMC maintainers are not copied on this patch series, it will
>>> likely be hard for them to identify this patch series and chime in...
>>
>> The main question is whether this is indeed a "very special case" as
>> Alexey claims it to be or that it is likely to be applicable to other
>> device and host combinations as you are suggesting.
>>
>> If these properties are imposed by the host or host controller it
>> would make sense to have these in the mmc bindings.
>
> BTW, last year we were discussing something similar (I mean related to
> alignment requirements) with ath10k SDIO patches and at the time the
> patch submitter was proposing to have a bounce buffer in ath10k to
> workaround that. I don't remember the details anymore, they are on the
> ath10k mailing list archive if anyone is curious to know, but I would
> not be surprised if they are similar as here. So there might be a need
> to solve this in a generic way (but not sure of course as I haven't
> checked the details).
I re-call something about these as well, here are the patches. Perhaps
I should pick some of them up...
https://patchwork.kernel.org/patch/10123137/
https://patchwork.kernel.org/patch/10123139/
https://patchwork.kernel.org/patch/10123141/
https://patchwork.kernel.org/patch/10123143/
Kind regards
Uffe
Re: [v16, 0/7] Fix eSDHC host version register bug
- i2c-list
On 9 November 2016 at 04:14, Yangbo Lu wrote:
> This patchset is used to fix a host version register bug in the
> T4240-R1.0-R2.0
> eSDHC controller. To match the SoC version and revision, 15 previous version
> patchsets had tried many methods but all of them were rejected by reviewers.
> Such as
> - dts compatible method
> - syscon method
> - ifdef PPC method
> - GUTS driver getting SVR method
> Anrd suggested a soc_device_match method in v10, and this is the only
> available
> method left now. This v11 patchset introduces the soc_device_match interface
> in
> soc driver.
>
> The first four patches of Yangbo are to add the GUTS driver. This is used to
> register a soc device which contain soc version and revision information.
> The other three patches introduce the soc_device_match method in soc driver
> and apply it on esdhc driver to fix this bug.
>
> ---
> Changes for v15:
> - Dropped patch 'dt: bindings: update Freescale DCFG compatible'
> since the work had been done by below patch on ShawnGuo's linux
> tree.
> 'dt-bindings: fsl: add LS1043A/LS1046A/LS2080A compatible for SCFG
>and DCFG'
> - Fixed error code issue in guts driver
> Changes for v16:
> - Dropped patch 'powerpc/fsl: move mpc85xx.h to include/linux/fsl'
> - Added a bug-fix patch from Geert
> ---
>
> Arnd Bergmann (1):
> base: soc: introduce soc_device_match() interface
>
> Geert Uytterhoeven (1):
> base: soc: Check for NULL SoC device attributes
>
> Yangbo Lu (5):
> ARM64: dts: ls2080a: add device configuration node
> dt: bindings: move guts devicetree doc out of powerpc directory
> soc: fsl: add GUTS driver for QorIQ platforms
> MAINTAINERS: add entry for Freescale SoC drivers
> mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
>
> .../bindings/{powerpc => soc}/fsl/guts.txt | 3 +
> MAINTAINERS| 11 +-
> arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 6 +
> drivers/base/Kconfig | 1 +
> drivers/base/soc.c | 70 ++
> drivers/mmc/host/Kconfig | 1 +
> drivers/mmc/host/sdhci-of-esdhc.c | 20 ++
> drivers/soc/Kconfig| 3 +-
> drivers/soc/fsl/Kconfig| 18 ++
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/guts.c | 236
> +
> include/linux/fsl/guts.h | 125 ++-
> include/linux/sys_soc.h| 3 +
> 13 files changed, 447 insertions(+), 51 deletions(-)
> rename Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt (91%)
> create mode 100644 drivers/soc/fsl/Kconfig
> create mode 100644 drivers/soc/fsl/guts.c
>
> --
> 2.1.0.27.g96db324
>
Thanks, applied on my mmc tree for next!
I noticed that some DT compatibles weren't documented, according to
checkpatch. Please fix that asap!
Kind regards
Ulf Hansson
Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend
On 1 November 2016 at 05:19, Rafael J. Wysocki wrote:
> On Thursday, October 27, 2016 01:53:03 PM Ulf Hansson wrote:
>> On 27 October 2016 at 13:41, Geert Uytterhoeven wrote:
>> > Hi Ulf,
>> >
>> > On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson
>> > wrote:
>> >> The smsc911c driver puts its device into low power state when entering
>> >> system suspend. Although it doesn't update the device's runtime PM status
>> >> to RPM_SUSPENDED, which causes problems for a parent device.
>> >>
>> >> In particular, when the runtime PM status of the parent is requested to be
>> >> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>> >> forbidden to runtime suspend a device, which has an active child.
>> >>
>> >> Fix this by updating the runtime PM status of the smsc911x device to
>> >> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>> >> action by runtime resuming the device and thus also the parent.
>> >
>> > Thanks for your patch!
>> >
>> > The changelog sounds quite innocent, but this does fix a system crash
>> > during resume from s2ram.
>> >
>> >> Signed-off-by: Ulf Hansson
>> >> Tested-by: Geert Uytterhoeven
>> >> Cc: Steve Glendinning
>> >> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with
>> >> an active child")
>> >
>> > While the abovementioned commit made the problem visible, the root cause
>> > was present before, right?
>>
>> Yes.
>>
>> >
>> >> ---
>> >>
>> >> Note that the commit this change fixes is currently queued for 4.10 via
>> >> Rafael's linux-pm tree. So this fix should go via that tree as well.
>> >
>> > Alternatively, this could go in in v4.9 to avoid the problem from ever
>> > appearing in upstream?
>>
>> Makes perfect sense! In that case we should remove the fixes tag.
>>
>> Rafael, can you pick this up for 4.9 rc[n]?
>
> If that is to go into 4.9-rc, it really should go in via the networking tree,
> because there is no PM dependency for it as of today.
>
> I can rearrange my 4.10 queue to put this one before the runtime PM commit
> exposing the problem in smsc911x, though.
As we spoked at LPC today, I don't mind if you take care of this
through your tree.
Kind regards
Uffe
Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend
On 27 October 2016 at 13:54, Geert Uytterhoeven wrote:
> Hi Ulf,
>
> On Thu, Oct 27, 2016 at 1:53 PM, Ulf Hansson wrote:
>> On 27 October 2016 at 13:41, Geert Uytterhoeven wrote:
>>> On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson wrote:
>>>> The smsc911c driver puts its device into low power state when entering
>>>> system suspend. Although it doesn't update the device's runtime PM status
>>>> to RPM_SUSPENDED, which causes problems for a parent device.
>>>>
>>>> In particular, when the runtime PM status of the parent is requested to be
>>>> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>>>> forbidden to runtime suspend a device, which has an active child.
>>>>
>>>> Fix this by updating the runtime PM status of the smsc911x device to
>>>> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>>>> action by runtime resuming the device and thus also the parent.
>>>
>>> Thanks for your patch!
>>>
>>> The changelog sounds quite innocent, but this does fix a system crash
>>> during resume from s2ram.
>>>
>>>> Signed-off-by: Ulf Hansson
>>>> Tested-by: Geert Uytterhoeven
>>>> Cc: Steve Glendinning
>>>> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with
>>>> an active child")
>>>
>>> While the abovementioned commit made the problem visible, the root cause
>>> was present before, right?
>>
>> Yes.
>>
>>>> ---
>>>>
>>>> Note that the commit this change fixes is currently queued for 4.10 via
>>>> Rafael's linux-pm tree. So this fix should go via that tree as well.
>>>
>>> Alternatively, this could go in in v4.9 to avoid the problem from ever
>>> appearing in upstream?
>>
>> Makes perfect sense! In that case we should remove the fixes tag.
>>
>> Rafael, can you pick this up for 4.9 rc[n]?
>
> Actually I was thinking about DaveM and the network tree instead.
Well, that would work as well.
Although, perhaps it becomes easier if Rafael deals with this, as it
gives him better control of when below change also can go in.
https://patchwork.kernel.org/patch/9375061
Rafael, please tell what you prefer?
Kind regards
Uffe
[PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend
The smsc911c driver puts its device into low power state when entering
system suspend. Although it doesn't update the device's runtime PM status
to RPM_SUSPENDED, which causes problems for a parent device.
In particular, when the runtime PM status of the parent is requested to be
updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
forbidden to runtime suspend a device, which has an active child.
Fix this by updating the runtime PM status of the smsc911x device to
RPM_SUSPENDED during system suspend. In system resume, let's reverse that
action by runtime resuming the device and thus also the parent.
Signed-off-by: Ulf Hansson
Tested-by: Geert Uytterhoeven
Cc: Steve Glendinning
Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an
active child")
---
Note that the commit this change fixes is currently queued for 4.10 via
Rafael's linux-pm tree. So this fix should go via that tree as well.
---
drivers/net/ethernet/smsc/smsc911x.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c
b/drivers/net/ethernet/smsc/smsc911x.c
index e9b8579..65fca9c 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev)
PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+
return 0;
}
@@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev)
struct smsc911x_data *pdata = netdev_priv(ndev);
unsigned int to = 100;
+ pm_runtime_enable(dev);
+ pm_runtime_resume(dev);
+
/* Note 3.11 from the datasheet:
* "When the LAN9220 is in a power saving state, a write of any
* data to the BYTE_TEST register will wake-up the device."
--
1.9.1
Re: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend
On 27 October 2016 at 13:41, Geert Uytterhoeven wrote:
> Hi Ulf,
>
> On Thu, Oct 27, 2016 at 1:23 PM, Ulf Hansson wrote:
>> The smsc911c driver puts its device into low power state when entering
>> system suspend. Although it doesn't update the device's runtime PM status
>> to RPM_SUSPENDED, which causes problems for a parent device.
>>
>> In particular, when the runtime PM status of the parent is requested to be
>> updated to RPM_SUSPENDED, the runtime PM core prevent this, because it's
>> forbidden to runtime suspend a device, which has an active child.
>>
>> Fix this by updating the runtime PM status of the smsc911x device to
>> RPM_SUSPENDED during system suspend. In system resume, let's reverse that
>> action by runtime resuming the device and thus also the parent.
>
> Thanks for your patch!
>
> The changelog sounds quite innocent, but this does fix a system crash
> during resume from s2ram.
>
>> Signed-off-by: Ulf Hansson
>> Tested-by: Geert Uytterhoeven
>> Cc: Steve Glendinning
>> Fixes: 8b1107b85efd ("PM / Runtime: Don't allow to suspend a device with an
>> active child")
>
> While the abovementioned commit made the problem visible, the root cause
> was present before, right?
Yes.
>
>> ---
>>
>> Note that the commit this change fixes is currently queued for 4.10 via
>> Rafael's linux-pm tree. So this fix should go via that tree as well.
>
> Alternatively, this could go in in v4.9 to avoid the problem from ever
> appearing in upstream?
Makes perfect sense! In that case we should remove the fixes tag.
Rafael, can you pick this up for 4.9 rc[n]?
Kind regards
Uffe
Re: [v12, 0/8] Fix eSDHC host version register bug
On 21 September 2016 at 08:57, Yangbo Lu wrote:
> This patchset is used to fix a host version register bug in the
> T4240-R1.0-R2.0
> eSDHC controller. To match the SoC version and revision, 10 previous version
> patchsets had tried many methods but all of them were rejected by reviewers.
> Such as
> - dts compatible method
> - syscon method
> - ifdef PPC method
> - GUTS driver getting SVR method
> Anrd suggested a soc_device_match method in v10, and this is the only
> available
> method left now. This v11 patchset introduces the soc_device_match interface
> in
> soc driver.
>
> The first six patches of Yangbo are to add the GUTS driver. This is used to
> register a soc device which contain soc version and revision information.
> The other two patches introduce the soc_device_match method in soc driver
> and apply it on esdhc driver to fix this bug.
>
> Arnd Bergmann (1):
> base: soc: introduce soc_device_match() interface
>
> Yangbo Lu (7):
> dt: bindings: update Freescale DCFG compatible
> ARM64: dts: ls2080a: add device configuration node
> dt: bindings: move guts devicetree doc out of powerpc directory
> powerpc/fsl: move mpc85xx.h to include/linux/fsl
> soc: fsl: add GUTS driver for QorIQ platforms
> MAINTAINERS: add entry for Freescale SoC drivers
> mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
>
> Documentation/devicetree/bindings/arm/fsl.txt | 6 +-
> .../bindings/{powerpc => soc}/fsl/guts.txt | 3 +
> MAINTAINERS| 11 +-
> arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 6 +
> arch/powerpc/kernel/cpu_setup_fsl_booke.S | 2 +-
> arch/powerpc/sysdev/fsl_pci.c | 2 +-
> drivers/base/Kconfig | 1 +
> drivers/base/soc.c | 66 ++
> drivers/clk/clk-qoriq.c| 3 +-
> drivers/i2c/busses/i2c-mpc.c | 2 +-
> drivers/iommu/fsl_pamu.c | 3 +-
> drivers/mmc/host/Kconfig | 1 +
> drivers/mmc/host/sdhci-of-esdhc.c | 20 ++
> drivers/net/ethernet/freescale/gianfar.c | 2 +-
> drivers/soc/Kconfig| 2 +-
> drivers/soc/fsl/Kconfig| 19 ++
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/guts.c | 257
> +
> include/linux/fsl/guts.h | 125 ++
> .../asm/mpc85xx.h => include/linux/fsl/svr.h | 4 +-
> include/linux/sys_soc.h| 3 +
> 21 files changed, 478 insertions(+), 61 deletions(-)
> rename Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt (91%)
> create mode 100644 drivers/soc/fsl/Kconfig
> create mode 100644 drivers/soc/fsl/guts.c
> rename arch/powerpc/include/asm/mpc85xx.h => include/linux/fsl/svr.h (97%)
>
> --
> 2.1.0.27.g96db324
>
This looks good to me! I am not sure which tree you want this to be
picked up through, but unless no other volunteers I can take it
through my mmc tree.
Although, before considering to apply, I need an ack from Scott/Arnd
for the guts driver in patch 5/8 and I need an ack from Greg for patch
7/8, where the soc_device_match() interface is added (seems like you
didn't add him on cc/to).
Kind regards
Uffe
Re: [v11, 7/8] base: soc: introduce soc_device_match() interface
On 6 September 2016 at 10:28, Yangbo Lu wrote:
> We keep running into cases where device drivers want to know the exact
> version of the a SoC they are currently running on. In the past, this has
> usually been done through a vendor specific API that can be called by a
> driver, or by directly accessing some kind of version register that is
> not part of the device itself but that belongs to a global register area
> of the chip.
>
> Common reasons for doing this include:
>
> - A machine is not using devicetree or similar for passing data about
> on-chip devices, but just announces their presence using boot-time
> platform devices, and the machine code itself does not care about the
> revision.
>
> - There is existing firmware or boot loaders with existing DT binaries
> with generic compatible strings that do not identify the particular
> revision of each device, but the driver knows which SoC revisions
> include which part.
>
> - A prerelease version of a chip has some quirks and we are using the same
> version of the bootloader and the DT blob on both the prerelease and the
> final version. An update of the DT binding seems inappropriate because
> that would involve maintaining multiple copies of the dts and/or
> bootloader.
>
> This patch introduces the soc_device_match() interface that is meant to
> work like of_match_node() but instead of identifying the version of a
> device, it identifies the SoC itself using a vendor-agnostic interface.
>
> Unlike of_match_node(), we do not do an exact string compare but instead
> use glob_match() to allow wildcards in strings.
Overall, this change make sense to me, although some minor comment below.
>
> Signed-off-by: Arnd Bergmann
> Signed-off-by: Yangbo Lu
> ---
> Changes for v11:
> - Added this patch for soc match
> ---
> drivers/base/Kconfig| 1 +
> drivers/base/soc.c | 61
> +
> include/linux/sys_soc.h | 3 +++
> 3 files changed, 65 insertions(+)
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 98504ec..f1591ad2 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -225,6 +225,7 @@ config GENERIC_CPU_AUTOPROBE
>
> config SOC_BUS
> bool
> + select GLOB
>
> source "drivers/base/regmap/Kconfig"
>
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 75b98aa..5c4e84a 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
>
> static DEFINE_IDA(soc_ida);
>
> @@ -168,3 +169,63 @@ static void __exit soc_bus_unregister(void)
> bus_unregister(&soc_bus_type);
> }
> module_exit(soc_bus_unregister);
> +
> +static int soc_device_match_one(struct device *dev, void *arg)
> +{
> + struct soc_device *soc_dev = container_of(dev, struct soc_device,
> dev);
> + const struct soc_device_attribute *match = arg;
> +
> + if (match->machine &&
> + !glob_match(match->machine, soc_dev->attr->machine))
> + return 0;
> +
> + if (match->family &&
> + !glob_match(match->family, soc_dev->attr->family))
> + return 0;
> +
> + if (match->revision &&
> + !glob_match(match->revision, soc_dev->attr->revision))
> + return 0;
> +
> + if (match->soc_id &&
> + !glob_match(match->soc_id, soc_dev->attr->soc_id))
> + return 0;
> +
> + return 1;
> +}
> +
> +/*
> + * soc_device_match - identify the SoC in the machine
> + * @matches: zero-terminated array of possible matches
Perhaps also express the constraint on the matching entries. As you
need at least one of the ->machine(), ->family(), ->revision() or
->soc_id() callbacks implemented, right!?
> + *
> + * returns the first matching entry of the argument array, or NULL
> + * if none of them match.
> + *
> + * This function is meant as a helper in place of of_match_node()
> + * in cases where either no device tree is available or the information
> + * in a device node is insufficient to identify a particular variant
> + * by its compatible strings or other properties. For new devices,
> + * the DT binding should always provide unique compatible strings
> + * that allow the use of of_match_node() instead.
> + *
> + * The calling function can use the .data entry of the
> + * soc_device_attribute to pass a structure or function pointer for
> + * each entry.
I don't get the use case behind this, could you elaborate?
Perhaps we should postpone adding the .data entry until we actually
see a need for it?
> + */
> +const struct soc_device_attribute *soc_device_match(
> + const struct soc_device_attribute *matches)
> +{
> + struct device *dev;
> + int ret;
> +
> + for (ret = 0; ret == 0; matches++) {
This loop looks a bit weird and unsafe.
1) Perhaps using a while loop makes this more readable?
2) As this is an exported API, I guess validation of the ->matches
Re: [v10, 7/7] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
On 26 May 2016 at 06:05, Yangbo Lu wrote: > Hi Uffe, > > Could we merge this patchset? ... > It has been a long time to wait for Arnd's response... > > Thanks a lot. > > As we are still in the merge window I won't queue anything but fixes. Let's give Arnd another week or so to respond. Kind regards Uffe > Best regards, > Yangbo Lu > > >> -Original Message- >> From: Yangbo Lu >> Sent: Friday, May 20, 2016 2:06 PM >> To: 'Scott Wood'; Arnd Bergmann; linux-arm-ker...@lists.infradead.org >> Cc: linuxppc-...@lists.ozlabs.org; Mark Rutland; >> devicet...@vger.kernel.org; ulf.hans...@linaro.org; Russell King; Bhupesh >> Sharma; netdev@vger.kernel.org; Joerg Roedel; Kumar Gala; linux- >> m...@vger.kernel.org; linux-ker...@vger.kernel.org; Yang-Leo Li; >> io...@lists.linux-foundation.org; Rob Herring; linux-...@vger.kernel.org; >> Claudiu Manoil; Santosh Shilimkar; Xiaobo Xie; linux-...@vger.kernel.org; >> Qiang Zhao >> Subject: RE: [v10, 7/7] mmc: sdhci-of-esdhc: fix host version for T4240- >> R1.0-R2.0 >> >> Hi Arnd, >> >> Any comments? >> Please reply when you see the email since we want this eSDHC issue to be >> fixed soon. >> >> All the patches are Freescale-specific and is to fix the eSDHC driver. >> Could we let them merged first if you were talking about a new way of >> abstracting getting SoC version. >> >> >> Thanks :) >> >> >> Best regards, >> Yangbo Lu >> >
Re: [v9, 6/7] MAINTAINERS: add entry for Freescale SoC driver
On 4 May 2016 at 05:24, Yangbo Lu wrote: > Add maintainer entry for Freescale SoC driver including > the QE library and the GUTS driver now. Also add maintainer > for QE library. > > Signed-off-by: Yangbo Lu So I need an ack from Scott and Qiang for this one, then I intend to queue up the series. Kind regards Uffe > --- > Changes for v8: > - Added this patch > Changes for v9: > - Added linux-arm mail list > - Removed GUTS driver entry > --- > MAINTAINERS | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 42e65d1..ce91db7 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -4622,9 +4622,18 @@ F: drivers/net/ethernet/freescale/fec_ptp.c > F: drivers/net/ethernet/freescale/fec.h > F: Documentation/devicetree/bindings/net/fsl-fec.txt > > +FREESCALE SOC DRIVER > +M: Scott Wood > +L: linuxppc-...@lists.ozlabs.org > +L: linux-arm-ker...@lists.infradead.org > +S: Maintained > +F: drivers/soc/fsl/ > +F: include/linux/fsl/ > + > FREESCALE QUICC ENGINE LIBRARY > +M: Qiang Zhao > L: linuxppc-...@lists.ozlabs.org > -S: Orphan > +S: Maintained > F: drivers/soc/fsl/qe/ > F: include/soc/fsl/*qe*.h > F: include/soc/fsl/*ucc*.h > -- > 2.1.0.27.g96db324 >
Re: [v7, 0/5] Fix eSDHC host version register bug
>> >> I was about to queue this for next, when I noticed that checkpatch is >> complaining/warning lots about these patches. Can you please a round of >> checkpatch and fix what makes sense. >> >> Kind regards >> Uffe > > [Lu Yangbo-B47093] Sorry about this, Uffe... No worries! > Should I ignore the warnings that update MAINTAINERS? drivers/soc/fsl/guts.c isn't part of the MAINTAINERS file, it should be. I also realize that the FREESCALE QUICC ENGINE LIBRARY section drivers/soc/fsl/qe/* also need an active maintainer, as it's currently orphan. Perhaps we should have create a new section for drivers/soc/fsl/* instead that covers all of the above? Maybe you or Scott are interested to pick it up? I also noted that, "include/linux/fsl/" isn't present in MAINTAINERS, please add that as well. > Regarding the 'undocumented' warning, I will added a patch updates doc before > all the patches, Ok? Yes, good! > > Thanks a lot :) > Kind regards Uffe
Re: [v7, 0/5] Fix eSDHC host version register bug
On 1 April 2016 at 05:07, Yangbo Lu wrote:
> This patchset is used to fix a host version register bug in the
> T4240-R1.0-R2.0
> eSDHC controller. To get the SoC version and revision, it's needed to add the
> GUTS driver to access the global utilities registers.
>
> So, the first three patches are to add the GUTS driver.
> The following two patches are to enable GUTS driver support to get SVR in
> eSDHC
> driver and fix host version for T4240.
>
> Yangbo Lu (5):
> ARM64: dts: ls2080a: add device configuration node
> soc: fsl: add GUTS driver for QorIQ platforms
> dt: move guts devicetree doc out of powerpc directory
> powerpc/fsl: move mpc85xx.h to include/linux/fsl
> mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
>
> .../bindings/{powerpc => soc}/fsl/guts.txt | 3 +
> arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi | 6 ++
> arch/powerpc/kernel/cpu_setup_fsl_booke.S | 2 +-
> drivers/clk/clk-qoriq.c| 3 +-
> drivers/i2c/busses/i2c-mpc.c | 2 +-
> drivers/iommu/fsl_pamu.c | 3 +-
> drivers/mmc/host/Kconfig | 1 +
> drivers/mmc/host/sdhci-of-esdhc.c | 23
> drivers/net/ethernet/freescale/gianfar.c | 2 +-
> drivers/soc/Kconfig| 2 +-
> drivers/soc/fsl/Kconfig| 8 ++
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/guts.c | 119
> +
> include/linux/fsl/guts.h | 98 -
> .../asm/mpc85xx.h => include/linux/fsl/svr.h | 4 +-
> 15 files changed, 219 insertions(+), 58 deletions(-)
> rename Documentation/devicetree/bindings/{powerpc => soc}/fsl/guts.txt (91%)
> create mode 100644 drivers/soc/fsl/Kconfig
> create mode 100644 drivers/soc/fsl/guts.c
> rename arch/powerpc/include/asm/mpc85xx.h => include/linux/fsl/svr.h (97%)
>
> --
> 2.1.0.27.g96db324
>
I was about to queue this for next, when I noticed that checkpatch is
complaining/warning lots about these patches. Can you please a round
of checkpatch and fix what makes sense.
Kind regards
Uffe
