Re: [PATCH 2/2] staging: wfx: apply the necessary SDIO quirks for the Silabs WF200

[Ulf Hansson]

On Thu, 17 Feb 2022 at 10:59, Kalle Valo  wrote:
>
> Jerome Pouiller  writes:
>
> > From: Jérôme Pouiller 
> >
> > Until now, the SDIO quirks are applied directly from the driver.
> > However, it is better to apply the quirks before driver probing. So,
> > this patch relocate the quirks in the MMC framework.
>
> It would be good to know how this is better, what's the concrete
> advantage?

The mmc core has a quirk interface for all types of cards
(eMMC/SD/SDIO), which thus keeps these things from sprinkling to
drivers. In some cases, the quirk needs to be applied already during
card initialization, which is earlier than when probing an SDIO func
driver or the MMC block device driver.

Perhaps it's a good idea to explain a bit about this in the commit message.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] staging: wfx: apply SDIO suggestions

[Ulf Hansson]

On Wed, 16 Feb 2022 at 10:31, Jerome Pouiller
 wrote:
>
> From: Jérôme Pouiller 
>
> Hi Ulf, Greg,
>
> The second patch of this series touch to the staging tree and to the MMC
> framework. I don't know what is the rule for these cases, but I think it
> makes more sense to carry this patch with the staging tree.

I don't believe the changes to the mmc core will cause any merge
conflict, so feel free to funnel this through whatever tree makes best
sense.

For the series:
Reviewed-by: Ulf Hansson 

Kind regards
Uffe

>
>
> Jérôme Pouiller (2):
>   staging: wfx: WF200 has no official SDIO IDs
>   staging: wfx: apply the necessary SDIO quirks for the Silabs WF200
>
>  drivers/mmc/core/quirks.h  | 5 +
>  drivers/staging/wfx/bus_sdio.c | 8 ++--
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> --
> 2.34.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Wed, 12 Jan 2022 at 19:24, Jérôme Pouiller
 wrote:
>
> On Wednesday 12 January 2022 18:48:48 CET Pali Rohár wrote:
> > CAUTION: This email originated from outside of the organization. Do not 
> > click links or open attachments unless you recognize the sender and know 
> > the content is safe.
> >
> >
> > On Wednesday 12 January 2022 17:45:45 Jérôme Pouiller wrote:
> > > On Wednesday 12 January 2022 12:43:32 CET Pali Rohár wrote:
> > > >
> > > > On Wednesday 12 January 2022 12:18:58 Jérôme Pouiller wrote:
> > > > > On Wednesday 12 January 2022 11:58:59 CET Pali Rohár wrote:
> > > > > > On Tuesday 11 January 2022 18:14:08 Jerome Pouiller wrote:
> > > > > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, 
> > > > > > > SDIO_DEVICE_ID_SILABS_WF200) },
> > > > > > > + { },
> > > > > > > +};
> > > > > >
> > > > > > Hello! Is this table still required?
> > > > >
> > > > > As far as I understand, if the driver does not provide an id_table, 
> > > > > the
> > > > > probe function won't be never called (see sdio_match_device()).
> > > > >
> > > > > Since, we rely on the device tree, we could replace 
> > > > > SDIO_VENDOR_ID_SILABS
> > > > > and SDIO_DEVICE_ID_SILABS_WF200 by SDIO_ANY_ID. However, it does not 
> > > > > hurt
> > > > > to add an extra filter here.
> > > >
> > > > Now when this particular id is not required, I'm thinking if it is still
> > > > required and it is a good idea to define these SDIO_VENDOR_ID_SILABS
> > > > macros into kernel include files. As it would mean that other broken
> > > > SDIO devices could define these bogus numbers too... And having them in
> > > > common kernel includes files can cause issues... e.g. other developers
> > > > could think that it is correct to use them as they are defined in common
> > > > header files. But as these numbers are not reliable (other broken cards
> > > > may have same ids as wf200) and their usage may cause issues in future.
> > >
> > > In order to make SDIO_VENDOR_ID_SILABS less official, do you prefer to
> > > define it in wfx/bus_sdio.c instead of mmc/sdio_ids.h?
> > >
> > > Or even not defined at all like:
> > >
> > > static const struct sdio_device_id wfx_sdio_ids[] = {
> > >  /* WF200 does not have official VID/PID */
> > >  { SDIO_DEVICE(0x, 0x1000) },
> > >  { },
> > > };
> >
> > This has advantage that it is explicitly visible that this device does
> > not use any officially assigned ids.
>
> Ulf, are you also agree?

Sure, that works for me too.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Wed, 12 Jan 2022 at 12:43, Pali Rohár  wrote:
>
> On Wednesday 12 January 2022 12:18:58 Jérôme Pouiller wrote:
> > On Wednesday 12 January 2022 11:58:59 CET Pali Rohár wrote:
> > > On Tuesday 11 January 2022 18:14:08 Jerome Pouiller wrote:
> > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) 
> > > > },
> > > > + { },
> > > > +};
> > >
> > > Hello! Is this table still required?
> >
> > As far as I understand, if the driver does not provide an id_table, the
> > probe function won't be never called (see sdio_match_device()).
> >
> > Since, we rely on the device tree, we could replace SDIO_VENDOR_ID_SILABS
> > and SDIO_DEVICE_ID_SILABS_WF200 by SDIO_ANY_ID. However, it does not hurt
> > to add an extra filter here.
>
> Now when this particular id is not required, I'm thinking if it is still
> required and it is a good idea to define these SDIO_VENDOR_ID_SILABS
> macros into kernel include files. As it would mean that other broken
> SDIO devices could define these bogus numbers too... And having them in
> common kernel includes files can cause issues... e.g. other developers
> could think that it is correct to use them as they are defined in common
> header files. But as these numbers are not reliable (other broken cards
> may have same ids as wf200) and their usage may cause issues in future.
>
> Ulf, any opinion?

The sdio_match_device() is what is being used to match the device to
its sdio_driver, which is being called from the sdio_bus_type's
->match() callback.

In regards to the DT compatible strings from a drivers'
.of_match_table, that is currently left to be matched by the sdio
driver's ->probe() function internally, by calling
of_driver_match_device().

In other words, I think what Jerome has suggested here seems
reasonable to me. Matching on "SDIO_ANY_ID" would work too, but I
think it's better with a poor filter like SDIO_VENDOR_ID_SILABS*,
rather than none.

An entirely different and new approach would be to extend
sdio_match_device() to call of_driver_match_device() too. However, in
that case we would also need to add a new corresponding ->probe()
callback for the sdio_driver, as the current one takes a const struct
sdio_device_id, which doesn't work when matching on DT compatibles.

>
> Btw, is there any project which maintains SDIO ids, like there is
> pci-ids.ucw.cz for PCI or www.linux-usb.org/usb-ids.html for USB?
>
> > > > +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,
> > > > + }
> > > > +};
> > > > --
> > > > 2.34.1
> > > >
> > >
> >
> >
> > --
> > Jérôme Pouiller

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 01/24] mmc: sdio: add SDIO IDs for Silabs WF200 chip

[Ulf Hansson]

On Tue, 11 Jan 2022 at 18:14, Jerome Pouiller
 wrote:
>
> From: Jérôme Pouiller 
>
> 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.
>
> So, to apply the quirks necessary for the Silabs WF200, we rely on the
> DT rather than on the SDIO VID/PID.
>
> Signed-off-by: Jérôme Pouiller 

I guess the series is getting close to getting queued up?

As an option to make sure $subject patch doesn't cause a problem for
that, I can queue it up and send it for the 5.17-rcs or if Kalle
prefer to carry this in this tree with my ack?

Kalle?

Kind regards
Uffe

> ---
>  drivers/mmc/core/quirks.h| 5 +
>  include/linux/mmc/sdio_ids.h | 7 +++
>  2 files changed, 12 insertions(+)
>
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index 20f568727277..f879dc63d936 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -149,6 +149,11 @@ static const struct mmc_fixup __maybe_unused 
> sdio_fixup_methods[] = {
>  static const struct mmc_fixup __maybe_unused sdio_card_init_methods[] = {
> SDIO_FIXUP_COMPATIBLE("ti,wl1251", wl1251_quirk, 0),
>
> +   SDIO_FIXUP_COMPATIBLE("silabs,wf200", add_quirk,
> + MMC_QUIRK_BROKEN_BYTE_MODE_512 |
> + MMC_QUIRK_LENIENT_FN0 |
> + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE),
> +
> END_FIXUP
>  };
>
> diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
> index a85c9f0bd470..483692f3002a 100644
> --- a/include/linux/mmc/sdio_ids.h
> +++ b/include/linux/mmc/sdio_ids.h
> @@ -25,6 +25,13 @@
>   * 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.34.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v9 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

[...]

> +static const struct of_device_id wfx_sdio_of_match[] = {
> +   { .compatible = "silabs,wf200",.data = _wf200 },
> +   { .compatible = "silabs,brd4001a", .data = _brd4001a },
> +   { .compatible = "silabs,brd8022a", .data = _brd8022a },
> +   { .compatible = "silabs,brd8023a", .data = _brd8023a },
> +   { .compatible = "silabs,wfx-sdio", .data = _wfx_sdio },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, wfx_sdio_of_match);
> +
> +static int wfx_sdio_probe(struct sdio_func *func, const struct 
> sdio_device_id *id)
> +{
> +   const struct wfx_platform_data *pdata = 
> of_device_get_match_data(>dev);
> +   struct device_node *np = func->dev.of_node;
> +   struct wfx_sdio_priv *bus;
> +   int ret;
> +
> +   if (func->num != 1) {
> +   dev_err(>dev, "SDIO function number is %d while it 
> should always be 1 (unsupported chip?)\n",
> +   func->num);
> +   return -ENODEV;
> +   }
> +
> +   if (!pdata) {
> +   dev_warn(>dev, "no compatible device found in DT\n");
> +   return -ENODEV;
> +   }
> +
> +   bus = devm_kzalloc(>dev, sizeof(*bus), GFP_KERNEL);
> +   if (!bus)
> +   return -ENOMEM;
> +
> +   bus->func = func;
> +   bus->of_irq = irq_of_parse_and_map(np, 0);
> +   sdio_set_drvdata(func, bus);
> +   func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> + MMC_QUIRK_BROKEN_BYTE_MODE_512;

This should not be needed any more, right?

> +
> +   sdio_claim_host(func);
> +   ret = sdio_enable_func(func);
> +   /* Block of 64 bytes is more efficient than 512B for frame sizes < 4k 
> */
> +   sdio_set_block_size(func, 64);
> +   sdio_release_host(func);
> +   if (ret)
> +   return ret;
> +
> +   bus->core = wfx_init_common(>dev, pdata, _sdio_hwbus_ops, 
> bus);
> +   if (!bus->core) {
> +   ret = -EIO;
> +   goto sdio_release;
> +   }
> +
> +   ret = wfx_probe(bus->core);
> +   if (ret)
> +   goto sdio_release;
> +
> +   return 0;
> +
> +sdio_release:
> +   sdio_claim_host(func);
> +   sdio_disable_func(func);
> +   sdio_release_host(func);
> +   return ret;
> +}

[...]

Other than the above, this looks good to me!


Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Fri, 1 Oct 2021 at 14:31, Kalle Valo  wrote:
>
> Hi Ulf,
>
> sorry for the late reply, my Gnus tells me it took me 24 weeks to reply :)
>
> Ulf Hansson  writes:
>
> > 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. :-)
>
> Indeed, I was omitting a lot of details :) My comment was more like a
> general remark to all different bus techonologies, not just about SDIO.
> And I'm not saying that all wireless drivers do that, but some of them
> do. Though I don't have any numbers how many.
>
> > 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?
>
> In general in the drivers that do we this the firmware is completely
> turned off and all memory is reset during interface down. And firmware
> is started from the scratch during interface up. Also one benefit from
> this is that firmware state is reset, the wireless firmwares are
> notarious being buggy.
>
> > 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?
>
> Yes, that's what I was trying to say. But take all this with grain of
> salt, I'm not very familiar with SDIO! And funnily enough, I checked
> what we do in ath10k_sdio driver during suspend has conflicting code and
> documentation:
>
> /* Empty handlers so that mmc subsystem doesn't remove us entirely during
>  * suspend. We instead follow cfg80211 suspend/resume handlers.
>  */
> static int ath10k_sdio_pm_suspend(struct device *device)
> {
> struct sdio_func *func = dev_to_sdio_func(device);
> struct ath10k_sdio *ar_sdio = sdio_get_drvdata(func);
> struct ath10k *ar = ar_sdio->ar;
> mmc_pm_flag_t pm_flag, pm_caps;
> int ret;
>
> if (!device_may_wakeup(ar->dev))
> return 0;
>
> ath10k_sdio_set_mbox_sleep(ar, true);
>
> pm_flag = MMC_PM_KEEP_POWER;
>
> ret = sdio_set_host_pm_flags(func, pm_flag);
> if (ret) {
> pm_caps = sdio_get_host_pm_caps(func);
> ath10k_warn(ar

Re: [PATCH v7 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Tue, 5 Oct 2021 at 10:14, Jérôme Pouiller  wrote:
>
> On Friday 1 October 2021 17:23:16 CEST Ulf Hansson wrote:
> > On Thu, 30 Sept 2021 at 19:06, Pali Rohár  wrote:
> > > On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> > > > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > > > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > > > >  wrote:
> > > > > >
> > > > > > From: Jérôme Pouiller 
> > > > > >
> > > > > > Signed-off-by: Jérôme Pouiller 
> > > > > > ---
> > > > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 
> > > > > > +
> > > > > >  1 file changed, 261 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
> > > > >
> > > > > [...]
> > > > >
> > > > > > +
> > > > > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > > > > + const struct sdio_device_id *id)
> > > > > > +{
> > > > > > +   struct device_node *np = func->dev.of_node;
> > > > > > +   struct wfx_sdio_priv *bus;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   if (func->num != 1) {
> > > > > > +   dev_err(>dev, "SDIO function number is %d 
> > > > > > while it should always be 1 (unsupported chip?)\n",
> > > > > > +   func->num);
> > > > > > +   return -ENODEV;
> > > > > > +   }
> > > > > > +
> > > > > > +   bus = devm_kzalloc(>dev, sizeof(*bus), GFP_KERNEL);
> > > > > > +   if (!bus)
> > > > > > +   return -ENOMEM;
> > > > > > +
> > > > > > +   if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > > > > +   dev_warn(>dev, "no compatible device found in 
> > > > > > DT\n");
> > > > > > +   return -ENODEV;
> > > > > > +   }
> > > > > > +
> > > > > > +   bus->func = func;
> > > > > > +   bus->of_irq = irq_of_parse_and_map(np, 0);
> > > > > > +   sdio_set_drvdata(func, bus);
> > > > > > +   func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > > > > + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > > > > + MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > > > >
> > > > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > > > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > > > > this.
> > > >
> > > > In the current patch, these quirks are applied only if the device 
> > > > appears
> > > > in the device tree (see the condition above). If I implement them in
> > > > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> > > > detected. Is it what we want?
> > > >
> > > > Note: we already have had a discussion about the strange VID/PID 
> > > > declared
> > > > by this device:
> > > >   https://www.spinics.net/lists/netdev/msg692577.html
> > >
> > > Yes, vendor id 0x is invalid per SDIO spec. So based on this vendor
> > > id, it is not possible to write any quirk in mmc/sdio generic code.
> > >
> > > Ulf, but maybe it could be possible to write quirk based on OF
> > > compatible string?
> >
> > Yes, that would be better in my opinion.
> >
> > We already have DT bindings to describe embedded SDIO cards (a subnode
> > to the mmc controller node), so we should be able to extend that I
> > think.
>
> So, this feature does not yet exist? Do you consider it is a blocker for
> the current patch?

Yes, sorry. I think we should avoid unnecessary hacks in SDIO func
drivers, especially those that deserve to be fixed in the mmc core.

Moreover, we already support the similar thing for eMMC cards, plus
that most parts are already done for SDIO too.

>
> To be honest, I don't really want to take over this change in mmc/core.

I understand. Allow me a couple of days, then I can post a patch to
help you out.

>
> --
> Jérôme Pouiller

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Thu, 30 Sept 2021 at 18:51, Jérôme Pouiller
 wrote:
>
> Hello Ulf,
>
> On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> >  wrote:
> > >
> > > From: Jérôme Pouiller 
> > >
> > > Signed-off-by: Jérôme Pouiller 
> > > ---
> > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +
> > >  1 file changed, 261 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
> >
> > [...]
> >
> > > +
> > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > + const struct sdio_device_id *id)
> > > +{
> > > +   struct device_node *np = func->dev.of_node;
> > > +   struct wfx_sdio_priv *bus;
> > > +   int ret;
> > > +
> > > +   if (func->num != 1) {
> > > +   dev_err(>dev, "SDIO function number is %d while it 
> > > should always be 1 (unsupported chip?)\n",
> > > +   func->num);
> > > +   return -ENODEV;
> > > +   }
> > > +
> > > +   bus = devm_kzalloc(>dev, sizeof(*bus), GFP_KERNEL);
> > > +   if (!bus)
> > > +   return -ENOMEM;
> > > +
> > > +   if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > +   dev_warn(>dev, "no compatible device found in 
> > > DT\n");
> > > +   return -ENODEV;
> > > +   }
> > > +
> > > +   bus->func = func;
> > > +   bus->of_irq = irq_of_parse_and_map(np, 0);
> > > +   sdio_set_drvdata(func, bus);
> > > +   func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > + MMC_QUIRK_BROKEN_BYTE_MODE_512;
> >
> > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > this.
>
> In the current patch, these quirks are applied only if the device appears
> in the device tree (see the condition above). If I implement them in
> drivers/mmc/core/quirks.h they will be applied as soon as the device is
> detected. Is it what we want?
>
> Note: we already have had a discussion about the strange VID/PID declared
> by this device:
>   https://www.spinics.net/lists/netdev/msg692577.html

Please, see my other reply to Pali.

>
>
> [...]
> > > +
> > > +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,
> >
> > Is there no power management? Or do you intend to add that on top?
>
> It seems we already have had this discussion:
>
>   
> https://lore.kernel.org/netdev/CAPDyKFqJf=vUqpQg3suDCadKrFTkQWFTY_qp=+yDK=_lu9g...@mail.gmail.com/#r
>
> In this thread, Kalle said:
> > 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.

Yeah, it's been a while since I looked at this, thanks for the pointer.

[...]

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Thu, 30 Sept 2021 at 19:06, Pali Rohár  wrote:
>
> On Thursday 30 September 2021 18:51:09 Jérôme Pouiller wrote:
> > Hello Ulf,
> >
> > On Thursday 30 September 2021 12:07:55 CEST Ulf Hansson wrote:
> > > On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
> > >  wrote:
> > > >
> > > > From: Jérôme Pouiller 
> > > >
> > > > Signed-off-by: Jérôme Pouiller 
> > > > ---
> > > >  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +
> > > >  1 file changed, 261 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
> > >
> > > [...]
> > >
> > > > +
> > > > +static int wfx_sdio_probe(struct sdio_func *func,
> > > > + const struct sdio_device_id *id)
> > > > +{
> > > > +   struct device_node *np = func->dev.of_node;
> > > > +   struct wfx_sdio_priv *bus;
> > > > +   int ret;
> > > > +
> > > > +   if (func->num != 1) {
> > > > +   dev_err(>dev, "SDIO function number is %d while 
> > > > it should always be 1 (unsupported chip?)\n",
> > > > +   func->num);
> > > > +   return -ENODEV;
> > > > +   }
> > > > +
> > > > +   bus = devm_kzalloc(>dev, sizeof(*bus), GFP_KERNEL);
> > > > +   if (!bus)
> > > > +   return -ENOMEM;
> > > > +
> > > > +   if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> > > > +   dev_warn(>dev, "no compatible device found in 
> > > > DT\n");
> > > > +   return -ENODEV;
> > > > +   }
> > > > +
> > > > +   bus->func = func;
> > > > +   bus->of_irq = irq_of_parse_and_map(np, 0);
> > > > +   sdio_set_drvdata(func, bus);
> > > > +   func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> > > > + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> > > > + MMC_QUIRK_BROKEN_BYTE_MODE_512;
> > >
> > > I would rather see that you add an SDIO_FIXUP for the SDIO card, to
> > > the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
> > > this.
> >
> > In the current patch, these quirks are applied only if the device appears
> > in the device tree (see the condition above). If I implement them in
> > drivers/mmc/core/quirks.h they will be applied as soon as the device is
> > detected. Is it what we want?
> >
> > Note: we already have had a discussion about the strange VID/PID declared
> > by this device:
> >   https://www.spinics.net/lists/netdev/msg692577.html
>
> Yes, vendor id 0x is invalid per SDIO spec. So based on this vendor
> id, it is not possible to write any quirk in mmc/sdio generic code.
>
> Ulf, but maybe it could be possible to write quirk based on OF
> compatible string?

Yes, that would be better in my opinion.

We already have DT bindings to describe embedded SDIO cards (a subnode
to the mmc controller node), so we should be able to extend that I
think.

The main reason why I think it's a good idea, is that we may need to
know (future wise) about quirks from the mmc core point of view,
before the SDIO func driver gets probed.

[...]

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

On Mon, 20 Sept 2021 at 18:12, Jerome Pouiller
 wrote:
>
> From: Jérôme Pouiller 
>
> Signed-off-by: Jérôme Pouiller 
> ---
>  drivers/net/wireless/silabs/wfx/bus_sdio.c | 261 +
>  1 file changed, 261 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

[...]

> +
> +static int wfx_sdio_probe(struct sdio_func *func,
> + const struct sdio_device_id *id)
> +{
> +   struct device_node *np = func->dev.of_node;
> +   struct wfx_sdio_priv *bus;
> +   int ret;
> +
> +   if (func->num != 1) {
> +   dev_err(>dev, "SDIO function number is %d while it 
> should always be 1 (unsupported chip?)\n",
> +   func->num);
> +   return -ENODEV;
> +   }
> +
> +   bus = devm_kzalloc(>dev, sizeof(*bus), GFP_KERNEL);
> +   if (!bus)
> +   return -ENOMEM;
> +
> +   if (!np || !of_match_node(wfx_sdio_of_match, np)) {
> +   dev_warn(>dev, "no compatible device found in DT\n");
> +   return -ENODEV;
> +   }
> +
> +   bus->func = func;
> +   bus->of_irq = irq_of_parse_and_map(np, 0);
> +   sdio_set_drvdata(func, bus);
> +   func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
> + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
> + MMC_QUIRK_BROKEN_BYTE_MODE_512;

I would rather see that you add an SDIO_FIXUP for the SDIO card, to
the sdio_fixup_methods[], in drivers/mmc/core/quirks.h, instead of
this.

> +
> +   sdio_claim_host(func);
> +   ret = sdio_enable_func(func);
> +   /* Block of 64 bytes is more efficient than 512B for frame sizes < 4k 
> */
> +   sdio_set_block_size(func, 64);
> +   sdio_release_host(func);
> +   if (ret)
> +   return ret;
> +
> +   bus->core = wfx_init_common(>dev, _sdio_pdata,
> +   _sdio_hwbus_ops, bus);
> +   if (!bus->core) {
> +   ret = -EIO;
> +   goto sdio_release;
> +   }
> +
> +   ret = wfx_probe(bus->core);
> +   if (ret)
> +   goto sdio_release;
> +
> +   return 0;
> +
> +sdio_release:
> +   sdio_claim_host(func);
> +   sdio_disable_func(func);
> +   sdio_release_host(func);
> +   return ret;
> +}
> +
> +static void wfx_sdio_remove(struct sdio_func *func)
> +{
> +   struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
> +
> +   wfx_release(bus->core);
> +   sdio_claim_host(func);
> +   sdio_disable_func(func);
> +   sdio_release_host(func);
> +}
> +
> +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,

Is there no power management? Or do you intend to add that on top?

> +   }
> +};
> --
> 2.33.0
>

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 08/24] wfx: add bus_sdio.c

[Ulf Hansson]

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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 28/48] soc/tegra: Introduce core power domain driver

[Ulf Hansson]

On Thu, 17 Dec 2020 at 19:07, Dmitry Osipenko  wrote:
>
> NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
> to an external SoC power rail. Core power domain covers vast majority of
> hardware blocks within a Tegra SoC. The voltage of a power domain should
> be set to a value which satisfies all devices within a power domain. Add
> driver for the core power domain in order to manage the voltage state of
> the domain. This allows us to support a system-wide DVFS on Tegra.
>
> Signed-off-by: Dmitry Osipenko 

FYI: from a genpd provider driver point of view, this looks good to
me. However, withholding my ack for the next version, just to make
sure I take a second look.

Kind regards
Uffe

> ---
>  drivers/soc/tegra/Kconfig |   6 ++
>  drivers/soc/tegra/Makefile|   1 +
>  drivers/soc/tegra/core-power-domain.c | 125 ++
>  include/soc/tegra/common.h|   6 ++
>  4 files changed, 138 insertions(+)
>  create mode 100644 drivers/soc/tegra/core-power-domain.c
>
> diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
> index bcd61ae59ba3..fccbc168dd87 100644
> --- a/drivers/soc/tegra/Kconfig
> +++ b/drivers/soc/tegra/Kconfig
> @@ -16,6 +16,7 @@ config ARCH_TEGRA_2x_SOC
> select SOC_TEGRA_COMMON
> select SOC_TEGRA_FLOWCTRL
> select SOC_TEGRA_PMC
> +   select SOC_TEGRA_CORE_POWER_DOMAIN
> select SOC_TEGRA20_VOLTAGE_COUPLER
> select TEGRA_TIMER
> help
> @@ -31,6 +32,7 @@ config ARCH_TEGRA_3x_SOC
> select SOC_TEGRA_COMMON
> select SOC_TEGRA_FLOWCTRL
> select SOC_TEGRA_PMC
> +   select SOC_TEGRA_CORE_POWER_DOMAIN
> select SOC_TEGRA30_VOLTAGE_COUPLER
> select TEGRA_TIMER
> help
> @@ -170,3 +172,7 @@ config SOC_TEGRA20_VOLTAGE_COUPLER
>  config SOC_TEGRA30_VOLTAGE_COUPLER
> bool "Voltage scaling support for Tegra30 SoCs"
> depends on ARCH_TEGRA_3x_SOC || COMPILE_TEST
> +
> +config SOC_TEGRA_CORE_POWER_DOMAIN
> +   bool
> +   select PM_GENERIC_DOMAINS
> diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile
> index 9c809c1814bd..8f1294f954b4 100644
> --- a/drivers/soc/tegra/Makefile
> +++ b/drivers/soc/tegra/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_SOC_TEGRA_PMC) += pmc.o
>  obj-$(CONFIG_SOC_TEGRA_POWERGATE_BPMP) += powergate-bpmp.o
>  obj-$(CONFIG_SOC_TEGRA20_VOLTAGE_COUPLER) += regulators-tegra20.o
>  obj-$(CONFIG_SOC_TEGRA30_VOLTAGE_COUPLER) += regulators-tegra30.o
> +obj-$(CONFIG_SOC_TEGRA_CORE_POWER_DOMAIN) += core-power-domain.o
> diff --git a/drivers/soc/tegra/core-power-domain.c 
> b/drivers/soc/tegra/core-power-domain.c
> new file mode 100644
> index ..7c0cec8c79fd
> --- /dev/null
> +++ b/drivers/soc/tegra/core-power-domain.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NVIDIA Tegra SoC Core Power Domain Driver
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +static struct lock_class_key tegra_core_domain_lock_class;
> +static bool tegra_core_domain_state_synced;
> +
> +static int tegra_genpd_set_performance_state(struct generic_pm_domain *genpd,
> +unsigned int level)
> +{
> +   struct dev_pm_opp *opp;
> +   int err;
> +
> +   opp = dev_pm_opp_find_level_ceil(>dev, );
> +   if (IS_ERR(opp)) {
> +   dev_err(>dev, "failed to find OPP for level %u: %pe\n",
> +   level, opp);
> +   return PTR_ERR(opp);
> +   }
> +
> +   err = dev_pm_opp_set_voltage(>dev, opp);
> +   dev_pm_opp_put(opp);
> +
> +   if (err) {
> +   dev_err(>dev, "failed to set voltage to %duV: %d\n",
> +   level, err);
> +   return err;
> +   }
> +
> +   return 0;
> +}
> +
> +static unsigned int
> +tegra_genpd_opp_to_performance_state(struct generic_pm_domain *genpd,
> +struct dev_pm_opp *opp)
> +{
> +   return dev_pm_opp_get_level(opp);
> +}
> +
> +static int tegra_core_domain_probe(struct platform_device *pdev)
> +{
> +   struct generic_pm_domain *genpd;
> +   struct opp_table *opp_table;
> +   const char *rname = "power";
> +   int err;
> +
> +   genpd = devm_kzalloc(>dev, sizeof(*genpd), GFP_KERNEL);
> +   if (!genpd)
> +   return -ENOMEM;
> +
> +   genpd->name = pdev->dev.of_node->name;
> +   genpd->set_performance_state = tegra_genpd_set_performance_state;
> +   genpd->opp_to_performance_state = 
> tegra_genpd_opp_to_performance_state;
> +
> +   opp_table = devm_pm_opp_set_regulators(>dev, , 1);
> +   if (IS_ERR(opp_table))
> +   return dev_err_probe(>dev, PTR_ERR(opp_table),
> +"failed to set OPP regulator\n");
> +
> +   err = pm_genpd_init(genpd, NULL, false);
> +   if (err) {
> +   dev_err(>dev, 

Re: [PATCH v2 31/48] soc/tegra: regulators: Support Core domain state syncing

[Ulf Hansson]

On Thu, 17 Dec 2020 at 19:07, Dmitry Osipenko  wrote:
>
> The core voltage shall not drop until state of Core domain is synced,
> i.e. all device drivers that use Core domain are loaded and ready.
>
> Support Core domain state syncing. The Core domain driver invokes the
> core-regulator voltage syncing once the state of domain is synced, at
> this point the Core voltage is allowed to go lower.
>
> Signed-off-by: Dmitry Osipenko 

This looks reasonable to me, feel free to add:

Reviewed-by: Ulf Hansson 

Kind regards
Uffe


> ---
>  drivers/soc/tegra/regulators-tegra20.c | 19 ++-
>  drivers/soc/tegra/regulators-tegra30.c | 18 +-
>  2 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/tegra/regulators-tegra20.c 
> b/drivers/soc/tegra/regulators-tegra20.c
> index 367a71a3cd10..e2c11d442591 100644
> --- a/drivers/soc/tegra/regulators-tegra20.c
> +++ b/drivers/soc/tegra/regulators-tegra20.c
> @@ -16,6 +16,8 @@
>  #include 
>  #include 
>
> +#include 
> +
>  struct tegra_regulator_coupler {
> struct regulator_coupler coupler;
> struct regulator_dev *core_rdev;
> @@ -38,6 +40,21 @@ static int tegra20_core_limit(struct 
> tegra_regulator_coupler *tegra,
> int core_cur_uV;
> int err;
>
> +   /*
> +* Tegra20 SoC has critical DVFS-capable devices that are
> +* permanently-active or active at a boot time, like EMC
> +* (DRAM controller) or Display controller for example.
> +*
> +* The voltage of a CORE SoC power domain shall not be dropped below
> +* a minimum level, which is determined by device's clock rate.
> +* This means that we can't fully allow CORE voltage scaling until
> +* the state of all DVFS-critical CORE devices is synced.
> +*/
> +   if (tegra_soc_core_domain_state_synced()) {
> +   pr_info_once("voltage state synced\n");
> +   return 0;
> +   }
> +
> if (tegra->core_min_uV > 0)
> return tegra->core_min_uV;
>
> @@ -58,7 +75,7 @@ static int tegra20_core_limit(struct 
> tegra_regulator_coupler *tegra,
>  */
> tegra->core_min_uV = core_max_uV;
>
> -   pr_info("core minimum voltage limited to %duV\n", tegra->core_min_uV);
> +   pr_info("core voltage initialized to %duV\n", tegra->core_min_uV);
>
> return tegra->core_min_uV;
>  }
> diff --git a/drivers/soc/tegra/regulators-tegra30.c 
> b/drivers/soc/tegra/regulators-tegra30.c
> index 0e776b20f625..42d675b79fa3 100644
> --- a/drivers/soc/tegra/regulators-tegra30.c
> +++ b/drivers/soc/tegra/regulators-tegra30.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>
> +#include 
>  #include 
>
>  struct tegra_regulator_coupler {
> @@ -39,6 +40,21 @@ static int tegra30_core_limit(struct 
> tegra_regulator_coupler *tegra,
> int core_cur_uV;
> int err;
>
> +   /*
> +* Tegra30 SoC has critical DVFS-capable devices that are
> +* permanently-active or active at a boot time, like EMC
> +* (DRAM controller) or Display controller for example.
> +*
> +* The voltage of a CORE SoC power domain shall not be dropped below
> +* a minimum level, which is determined by device's clock rate.
> +* This means that we can't fully allow CORE voltage scaling until
> +* the state of all DVFS-critical CORE devices is synced.
> +*/
> +   if (tegra_soc_core_domain_state_synced()) {
> +   pr_info_once("voltage state synced\n");
> +   return 0;
> +   }
> +
> if (tegra->core_min_uV > 0)
> return tegra->core_min_uV;
>
> @@ -59,7 +75,7 @@ static int tegra30_core_limit(struct 
> tegra_regulator_coupler *tegra,
>  */
> tegra->core_min_uV = core_max_uV;
>
> -   pr_info("core minimum voltage limited to %duV\n", tegra->core_min_uV);
> +   pr_info("core voltage initialized to %duV\n", tegra->core_min_uV);
>
> return tegra->core_min_uV;
>  }
> --
> 2.29.2
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Ulf Hansson]

On Thu, 12 Nov 2020 at 23:14, Dmitry Osipenko  wrote:
>
> 12.11.2020 23:43, Thierry Reding пишет:
> >> The difference in comparison to using voltage regulator directly is
> >> minimal, basically the core-supply phandle is replaced is replaced with
> >> a power-domain phandle in a device tree.
> > These new power-domain handles would have to be added to devices that
> > potentially already have a power-domain handle, right? Isn't that going
> > to cause issues? I vaguely recall that we already have multiple power
> > domains for the XUSB controller and we have to jump through extra hoops
> > to make that work.
>
> I modeled the core PD as a parent of the PMC sub-domains, which
> presumably is a correct way to represent the domains topology.
>
> https://gist.github.com/digetx/dfd92c7f7e0aa6cef20403c4298088d7

That could make sense, it seems.

Anyway, this made me realize that
dev_pm_genpd_set_performance_state(dev) returns -EINVAL, in case the
device's genpd doesn't have the ->set_performance_state() assigned.
This may not be correct. Instead we should likely consider an empty
callback as okay and continue to walk the topology upwards to the
parent domain, etc.

Just wanted to point this out. I intend to post a patch as soon as I
can for this.

[...]

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 01/30] dt-bindings: host1x: Document OPP and voltage regulator properties

[Ulf Hansson]

On Thu, 5 Nov 2020 at 00:44, Dmitry Osipenko  wrote:
>
> Document new DVFS OPP table and voltage regulator properties of the
> Host1x bus and devices sitting on the bus.
>
> Signed-off-by: Dmitry Osipenko 
> ---
>  .../display/tegra/nvidia,tegra20-host1x.txt   | 56 +++
>  1 file changed, 56 insertions(+)
>
> diff --git 
> a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt 
> b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> index 34d993338453..0593c8df70bb 100644
> --- 
> a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> +++ 
> b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt
> @@ -20,6 +20,18 @@ Required properties:
>  - reset-names: Must include the following entries:
>- host1x
>
> +Optional properties:
> +- operating-points-v2: See ../bindings/opp/opp.txt for details.
> +- core-supply: Phandle of voltage regulator of the SoC "core" power domain.
> +
> +For each opp entry in 'operating-points-v2' table of host1x and its modules:
> +- opp-supported-hw: One bitfield indicating:
> +   On Tegra20: SoC process ID mask
> +   On Tegra30+: SoC speedo ID mask
> +
> +   A bitwise AND is performed against the value and if any bit
> +   matches, the OPP gets enabled.
> +
>  Each host1x client module having to perform DMA through the Memory Controller
>  should have the interconnect endpoints set to the Memory Client and External
>  Memory respectively.
> @@ -45,6 +57,8 @@ of the following host1x client modules:
>- interconnect-names: Must include name of the interconnect path for each
>  interconnect entry. Consult TRM documentation for information about
>  available memory clients, see MEMORY CONTROLLER section.
> +  - core-supply: Phandle of voltage regulator of the SoC "core" power domain.
> +  - operating-points-v2: See ../bindings/opp/opp.txt for details.
>

As discussed in the thread for the cover-letter.

We already have DT bindings for power-domains (providers and
consumers). Please use them instead of adding SoC specific bindings to
each peripheral device.

[...]

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Ulf Hansson]

On Sun, 8 Nov 2020 at 13:19, Dmitry Osipenko  wrote:
>
> 05.11.2020 18:22, Dmitry Osipenko пишет:
> > 05.11.2020 12:45, Ulf Hansson пишет:
> > ...
> >> I need some more time to review this, but just a quick check found a
> >> few potential issues...
> >
> > Thank you for starting the review! I'm pretty sure it will take a couple
> > revisions until all the questions will be resolved :)
> >
> >> The "core-supply", that you specify as a regulator for each
> >> controller's device node, is not the way we describe power domains.
> >> Instead, it seems like you should register a power-domain provider
> >> (with the help of genpd) and implement the ->set_performance_state()
> >> callback for it. Each device node should then be hooked up to this
> >> power-domain, rather than to a "core-supply". For DT bindings, please
> >> have a look at Documentation/devicetree/bindings/power/power-domain.yaml
> >> and Documentation/devicetree/bindings/power/power_domain.txt.
> >>
> >> In regards to the "sync state" problem (preventing to change
> >> performance states until all consumers have been attached), this can
> >> then be managed by the genpd provider driver instead.
> >
> > I'll need to take a closer look at GENPD, thank you for the suggestion.
> >
> > Sounds like a software GENPD driver which manages clocks and voltages
> > could be a good idea, but it also could be an unnecessary
> > over-engineering. Let's see..
> >
>
> Hello Ulf and all,
>
> I took a detailed look at the GENPD and tried to implement it. Here is
> what was found:
>
> 1. GENPD framework doesn't aggregate performance requests from the
> attached devices. This means that if deviceA requests performance state
> 10 and then deviceB requests state 3, then framework will set domain's
> state to 3 instead of 10.
>
> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L376

As Viresh also stated, genpd does aggregate the votes. It even
performs aggregation hierarchy (a genpd is allowed to have parent(s)
to model a topology).

>
> 2. GENPD framework has a sync() callback in the genpd.domain structure,
> but this callback isn't allowed to be used by the GENPD implementation.
> The GENPD framework always overrides that callback for its own needs.
> Hence GENPD doesn't allow to solve the bootstrapping
> state-synchronization problem in a nice way.
>
> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L2606

That ->sync() callback isn't the callback you are looking for, it's a
PM domain specific callback - and has other purposes.

To solve the problem you refer to, your genpd provider driver (a
platform driver) should assign its ->sync_state() callback. The
->sync_state() callback will be invoked, when all consumer devices
have been attached (and probed) to their corresponding provider.

You may have a look at drivers/cpuidle/cpuidle-psci-domain.c, to see
an example of how this works. If there is anything unclear, just tell
me and I will try to help.

>
> 3. Tegra doesn't have a dedicated hardware power-controller for the core
> domain, instead there is only an external voltage regulator. Hence we
> will need to create a phony device-tree node for the virtual power
> domain, which is probably a wrong thing to do.

No, this is absolutely the correct thing to do.

This isn't a virtual power domain, it's a real power domain. You only
happen to model the control of it as a regulator, as it fits nicely
with that for *this* SoC. Don't get me wrong, that's fine as long as
the supply is specified only in the power-domain provider node.

On another SoC, you might have a different FW interface for the power
domain provider that doesn't fit well with the regulator. When that
happens, all you need to do is to implement a new power domain
provider and potentially re-define the power domain topology. More
importantly, you don't need to re-invent yet another slew of device
specific bindings - for each SoC.

>
> ===
>
> Perhaps it should be possible to create some hacks to work around
> bullets 2 and 3 in order to achieve what we need for DVFS on Tegra, but
> bullet 1 isn't solvable without changing how the GENPD core works.
>
> Altogether, the GENPD in its current form is a wrong abstraction for a
> system-wide DVFS in a case where multiple devices share power domain and
> this domain is a voltage regulator. The regulator framework is the
> correct abstraction in this case for today.

Well, I admit it's a bit complex. But it solves the problem in a
nicely abstracted way that should work for everybody, at least in my
opinion.

Although, let's not exclude that there are pieces missing in genpd or
the opp layer, as this DVFS feature is rather new - but then we should
just extend/fix it.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Ulf Hansson]

On Thu, 5 Nov 2020 at 12:13, Viresh Kumar  wrote:
>
> On 05-11-20, 11:56, Ulf Hansson wrote:
> > On Thu, 5 Nov 2020 at 11:40, Viresh Kumar  wrote:
> > > Btw, how do we identify if it is a power domain or a regulator ?
>
> To be honest, I was a bit afraid and embarrassed to ask this question,
> and was hoping people to make fun of me in return :)
>
> > Good question. It's not a crystal clear line in between them, I think.
>
> And I was relieved after reading this :)
>
> > A power domain to me, means that some part of a silicon (a group of
> > controllers or just a single piece, for example) needs some kind of
> > resource (typically a power rail) to be enabled to be functional, to
> > start with.
>
> Isn't this what a part of regulator does as well ? i.e.
> enabling/disabling of the regulator or power to a group of
> controllers.

It could, but it shouldn't.

>
> Over that the regulator does voltage/current scaling as well, which
> normally the power domains don't do (though we did that in
> performance-state case).
>
> > If there are operating points involved, that's also a
> > clear indication to me, that it's not a regular regulator.
>
> Is there any example of that? I hope by OPP you meant both freq and
> voltage here. I am not sure if I know of a case where a power domain
> handles both of them.

It may be both voltage and frequency - but in some cases only voltage.
>From HW point of view, many ARM legacy platforms have power domains
that work like this.

As you know, the DVFS case has in many years not been solved in a
generic way, but mostly via platform specific hacks.

The worst ones are probably those hacking clock drivers (which myself
also have contributed to). Have a look at clk_prcmu_opp_prepare(), for
example, which is used by the UX500 platform. Another option has been
to use the devfreq framework, but it has limitations in regards to
this too.

That said, I am hoping that people start moving towards the
deploying/implementing DVFS through the power-domain approach,
together with the OPPs. Maybe there are still some pieces missing from
an infrastructure point of view, but that should become more evident
as more starts using it.

>
> > Maybe we should try to specify this more exactly in some
> > documentation, somewhere.
>
> I think yes, it is very much required. And in absence of that I think,
> many (or most) of the platforms that also need to scale the voltage
> would have modeled their hardware as a regulator and not a PM domain.
>
> What I always thought was:
>
> - Module that can just enable/disable power to a block of SoC is a
>   power domain.
>
> - Module that can enable/disable as well as scale voltage is a
>   regulator.
>
> And so I thought that this patchset has done the right thing. This
> changed a bit with the qcom stuff where the IP to be configured was in
> control of RPM and not Linux and so we couldn't add it as a regulator.
> If it was controlled by Linux, it would have been a regulator in
> kernel for sure :)

In my view, DT bindings have consistently been pushed back during the
year, if they have tried to model power domains as regulator supplies
from consumer device nodes. Hence, people have tried other things, as
I mentioned above.

I definitely agree that we need to update some documentations,
explaining things more exactly. Additionally, it seems like a talk at
some conferences should make sense, as a way to spread the word.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Ulf Hansson]

On Thu, 5 Nov 2020 at 11:40, Viresh Kumar  wrote:
>
> On 05-11-20, 11:34, Ulf Hansson wrote:
> > I am not objecting about scaling the voltage through a regulator,
> > that's fine to me. However, encoding a power domain as a regulator
> > (even if it may seem like a regulator) isn't. Well, unless Mark Brown
> > has changed his mind about this.
> >
> > In this case, it seems like the regulator supply belongs in the
> > description of the power domain provider.
>
> Okay, I wasn't sure if it is a power domain or a regulator here. Btw,
> how do we identify if it is a power domain or a regulator ?

Good question. It's not a crystal clear line in between them, I think.

A power domain to me, means that some part of a silicon (a group of
controllers or just a single piece, for example) needs some kind of
resource (typically a power rail) to be enabled to be functional, to
start with. If there are operating points involved, that's also a
clear indication to me, that it's not a regular regulator.

Maybe we should try to specify this more exactly in some
documentation, somewhere.

>
> > > In case of Qcom earlier (when we added the performance-state stuff),
> > > the eventual hardware was out of kernel's control and we didn't wanted
> > > (allowed) to model it as a virtual regulator just to pass the votes to
> > > the RPM. And so we did what we did.
> > >
> > > But if the hardware (where the voltage is required to be changed) is
> > > indeed a regulator and is modeled as one, then what Dmitry has done
> > > looks okay. i.e. add a supply in the device's node and microvolt
> > > property in the DT entries.
> >
> > I guess I haven't paid enough attention how power domain regulators
> > are being described then. I was under the impression that the CPUfreq
> > case was a bit specific - and we had legacy bindings to stick with.
> >
> > Can you point me to some other existing examples of where power domain
> > regulators are specified as a regulator in each device's node?
>
> No, I thought it is a regulator here and not a power domain.

Okay, thanks!

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Ulf Hansson]

On Thu, 5 Nov 2020 at 11:06, Viresh Kumar  wrote:
>
> On 05-11-20, 10:45, Ulf Hansson wrote:
> > + Viresh
>
> Thanks Ulf. I found a bug in OPP core because you cc'd me here :)

Happy to help. :-)

>
> > On Thu, 5 Nov 2020 at 00:44, Dmitry Osipenko  wrote:
> > I need some more time to review this, but just a quick check found a
> > few potential issues...
> >
> > The "core-supply", that you specify as a regulator for each
> > controller's device node, is not the way we describe power domains.
>
> Maybe I misunderstood your comment here, but there are two ways of
> scaling the voltage of a device depending on if it is a regulator (and
> can be modeled as one in the kernel) or a power domain.

I am not objecting about scaling the voltage through a regulator,
that's fine to me. However, encoding a power domain as a regulator
(even if it may seem like a regulator) isn't. Well, unless Mark Brown
has changed his mind about this.

In this case, it seems like the regulator supply belongs in the
description of the power domain provider.

>
> In case of Qcom earlier (when we added the performance-state stuff),
> the eventual hardware was out of kernel's control and we didn't wanted
> (allowed) to model it as a virtual regulator just to pass the votes to
> the RPM. And so we did what we did.
>
> But if the hardware (where the voltage is required to be changed) is
> indeed a regulator and is modeled as one, then what Dmitry has done
> looks okay. i.e. add a supply in the device's node and microvolt
> property in the DT entries.

I guess I haven't paid enough attention how power domain regulators
are being described then. I was under the impression that the CPUfreq
case was a bit specific - and we had legacy bindings to stick with.

Can you point me to some other existing examples of where power domain
regulators are specified as a regulator in each device's node?

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Ulf Hansson]

+ Viresh

On Thu, 5 Nov 2020 at 00:44, Dmitry Osipenko  wrote:
>
> Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces
> power consumption and heating of the Tegra chips. Tegra SoC has multiple
> hardware units which belong to a core power domain of the SoC and share
> the core voltage. The voltage must be selected in accordance to a minimum
> requirement of every core hardware unit.
>
> The minimum core voltage requirement depends on:
>
>   1. Clock enable state of a hardware unit.
>   2. Clock frequency.
>   3. Unit's internal idling/active state.
>
> This series is tested on Acer A500 (T20), AC100 (T20), Nexus 7 (T30) and
> Ouya (T30) devices. I also added voltage scaling to the Ventana (T20) and
> Cardhu (T30) boards which are tested by NVIDIA's CI farm. Tegra30 is now up
> to 5C cooler on Nexus 7 and stays cool on Ouya (instead of becoming burning
> hot) while system is idling. It should be possible to improve this further
> by implementing a more advanced power management features for the kernel
> drivers.
>
> The DVFS support is opt-in for all boards, meaning that older DTBs will
> continue to work like they did it before this series. It should be possible
> to easily add the core voltage scaling support for Tegra114+ SoCs based on
> this grounding work later on, if anyone will want to implement it.
>
> WARNING(!) This series is made on top of the memory interconnect patches
>which are currently under review [1]. The Tegra EMC driver
>and devicetree-related patches need to be applied on top of
>the ICC series.
>
> [1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=212196
>
> Dmitry Osipenko (30):
>   dt-bindings: host1x: Document OPP and voltage regulator properties
>   dt-bindings: mmc: tegra: Document OPP and voltage regulator properties
>   dt-bindings: pwm: tegra: Document OPP and voltage regulator properties
>   media: dt: bindings: tegra-vde: Document OPP and voltage regulator
> properties
>   dt-binding: usb: ci-hdrc-usb2:  Document OPP and voltage regulator
> properties
>   dt-bindings: usb: tegra-ehci: Document OPP and voltage regulator
> properties
>   soc/tegra: Add sync state API
>   soc/tegra: regulators: Support Tegra SoC device sync state API
>   soc/tegra: regulators: Fix lockup when voltage-spread is out of range
>   regulator: Allow skipping disabled regulators in
> regulator_check_consumers()
>   drm/tegra: dc: Support OPP and SoC core voltage scaling
>   drm/tegra: gr2d: Correct swapped device-tree compatibles
>   drm/tegra: gr2d: Support OPP and SoC core voltage scaling
>   drm/tegra: gr3d: Support OPP and SoC core voltage scaling
>   drm/tegra: hdmi: Support OPP and SoC core voltage scaling
>   gpu: host1x: Support OPP and SoC core voltage scaling
>   mmc: sdhci-tegra: Support OPP and core voltage scaling
>   pwm: tegra: Support OPP and core voltage scaling
>   media: staging: tegra-vde: Support OPP and SoC core voltage scaling
>   usb: chipidea: tegra: Support OPP and SoC core voltage scaling
>   usb: host: ehci-tegra: Support OPP and SoC core voltage scaling
>   memory: tegra20-emc: Support Tegra SoC device state syncing
>   memory: tegra30-emc: Support Tegra SoC device state syncing
>   ARM: tegra: Add OPP tables for Tegra20 peripheral devices
>   ARM: tegra: Add OPP tables for Tegra30 peripheral devices
>   ARM: tegra: ventana: Add voltage supplies to DVFS-capable devices
>   ARM: tegra: paz00: Add voltage supplies to DVFS-capable devices
>   ARM: tegra: acer-a500: Add voltage supplies to DVFS-capable devices
>   ARM: tegra: cardhu-a04: Add voltage supplies to DVFS-capable devices
>   ARM: tegra: nexus7: Add voltage supplies to DVFS-capable devices
>
>  .../display/tegra/nvidia,tegra20-host1x.txt   |  56 +++
>  .../bindings/media/nvidia,tegra-vde.txt   |  12 +
>  .../bindings/mmc/nvidia,tegra20-sdhci.txt |  12 +
>  .../bindings/pwm/nvidia,tegra20-pwm.txt   |  13 +
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |   4 +
>  .../bindings/usb/nvidia,tegra20-ehci.txt  |   2 +
>  .../boot/dts/tegra20-acer-a500-picasso.dts|  30 +-
>  arch/arm/boot/dts/tegra20-paz00.dts   |  40 +-
>  .../arm/boot/dts/tegra20-peripherals-opp.dtsi | 386 
>  arch/arm/boot/dts/tegra20-ventana.dts |  65 ++-
>  arch/arm/boot/dts/tegra20.dtsi|  14 +
>  .../tegra30-asus-nexus7-grouper-common.dtsi   |  23 +
>  arch/arm/boot/dts/tegra30-cardhu-a04.dts  |  44 ++
>  .../arm/boot/dts/tegra30-peripherals-opp.dtsi | 415 ++
>  arch/arm/boot/dts/tegra30.dtsi|  13 +
>  drivers/gpu/drm/tegra/Kconfig |   1 +
>  drivers/gpu/drm/tegra/dc.c| 138 +-
>  drivers/gpu/drm/tegra/dc.h|   5 +
>  drivers/gpu/drm/tegra/gr2d.c  | 140 +-
>  drivers/gpu/drm/tegra/gr3d.c  | 136 ++
>  drivers/gpu/drm/tegra/hdmi.c  |  63 ++-
>  

Re: [PATCH v2 01/24] mmc: sdio: add SDIO IDs for Silabs WF200 chip

[Ulf Hansson]

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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 07/23] wfx: add bus_sdio.c

[Ulf Hansson]

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(), 

Re: [PATCH 07/23] wfx: add bus_sdio.c

[Ulf Hansson]

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(>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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 00/17] wilc1000: move out of staging

[Ulf Hansson]

On Wed, 1 Jul 2020 at 09:55, Pali Rohár  wrote:
>
> On Tuesday 30 June 2020 03:17:01 ajay.kat...@microchip.com wrote:
> > On 29/06/20 6:56 pm, Pali Rohár wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> > > the content is safe
> > >
> > > On Tuesday 23 June 2020 11:00:04 ajay.kat...@microchip.com wrote:
> > >> This patch series is to review and move wilc1000 driver out of staging.
> > >> Most of the review comments received in [1] & [2] are addressed in the
> > >> latest code.
> > >> Please review and provide your inputs.
> > >
> > > Hello Ajay! Could you please move SDIO vendor/device ID definitions from
> > > driver code wilc1000/sdio.c to common file include/linux/mmc/sdio_ids.h?
> > >
> >
> > Currently, the wilc1000 driver movement changes are present in topic
> > branch and yet to be merged to master branch. Would it be okay to submit
> > the new patch once driver is merged to 'wireless-driver-next' master and
> > branch is pulled into Greg's staging repo.
>
> I think it should be OK. But maybe Ulf as maintainer of mmc subsystem
> could have opinion or react on this.

That should be fine. Just keep me on cc so I can ack it. Potentially
we may get some mergeconflict between the trees, but let's resolve
that if/when that happens.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC RFT PATCH 0/4] gpiolib: speed up GPIO array processing

[Ulf Hansson]

On 21 August 2018 at 01:43, Janusz Krzysztofik  wrote:
>
> This series is a follow up of the former "mtd: rawnand: ams-delta: Use
> gpio-omap accessors for data I/O" which already contained some changes
> to gpiolib.  Those previous attempts were commented by Borris Brezillon
> who suggested using GPIO API modified to accept bitmaps, and by Linus
> Walleij who suggested still more great ideas for further immprovement
> of the proposed API changes - thanks!
>
> The goal is to boost performans of get/set array functions while
> processing GPIO arrays which represent pins of a signle chip in
> hardware order.  If resulting performance is close to PIO, GPIO API
> can be used for data I/O without much loss of speed.
>
> Created and tested on a low end Amstrad Delta board with NAND driver
> updated to use GPIO API for data I/O.  Performance degrade compared to
> PIO is much better than before the optimization but still not quite
> satisfactory.
>
>
> Janusz Krzysztofik (4):
>   gpiolib: Pass bitmaps, not integer arrays, to get/set array
>   gpiolib: Identify arrays matching GPIO hardware
>   gpiolib: Pass array info to get/set array functions
>   gpiolib: Implement fast processing path in get/set array
>
>
>  Documentation/driver-api/gpio/board.rst |   15 +
>  Documentation/driver-api/gpio/consumer.rst  |   48 +++-
>  drivers/auxdisplay/hd44780.c|   64 +++---
>  drivers/bus/ts-nbus.c   |   25 --
>  drivers/gpio/gpio-max3191x.c|   23 +-
>  drivers/gpio/gpiolib.c  |  279 
> ++--
>  drivers/gpio/gpiolib.h  |   15 +
>  drivers/i2c/muxes/i2c-mux-gpio.c|5
>  drivers/mmc/core/pwrseq_simple.c|   15 -
>  drivers/mux/gpio.c  |7
>  drivers/net/phy/mdio-mux-gpio.c |5
>  drivers/pcmcia/soc_common.c |   14 -
>  drivers/phy/motorola/phy-mapphone-mdm6600.c |   21 +-
>  drivers/staging/iio/adc/ad7606.c|   12 -
>  drivers/tty/serial/serial_mctrl_gpio.c  |9
>  include/linux/gpio/consumer.h   |   35 ++-
>  16 files changed, 410 insertions(+), 182 deletions(-)
>

For the mmc related changes:

Acked-by: Ulf Hansson 

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 13/20] mmc: Remove depends on HAS_DMA in case of platform dependency

[Ulf Hansson]

On 17 April 2018 at 19:49, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Remove dependencies on HAS_DMA where a Kconfig symbol depends on another
> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
> In most cases this other symbol is an architecture or platform specific
> symbol, or PCI.
>
> Generic symbols and drivers without platform dependencies keep their
> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
> cannot work anyway.
>
> This simplifies the dependencies, and allows to improve compile-testing.
>
> Signed-off-by: Geert Uytterhoeven <ge...@linux-m68k.org>
> Reviewed-by: Mark Brown <broo...@kernel.org>
> Acked-by: Robin Murphy <robin.mur...@arm.com>
> Acked-by: Ulf Hansson <ulf.hans...@linaro.org>

Thanks, applied for next!

Kind regrds
Uffe

> ---
> v3:
>   - Add Acked-by,
>   - Rebase to v4.17-rc1,
>
> v2:
>   - Add Reviewed-by, Acked-by,
>   - Drop RFC state,
>   - Split per subsystem.
> ---
>  drivers/mmc/host/Kconfig | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 9589f9c9046f14b1..3978d0418958bf6b 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -358,7 +358,6 @@ config MMC_MESON_MX_SDIO
> tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller 
> support"
> depends on ARCH_MESON || COMPILE_TEST
> depends on COMMON_CLK
> -   depends on HAS_DMA
> depends on OF
> help
>   This selects support for the SD/MMC Host Controller on
> @@ -401,7 +400,6 @@ config MMC_OMAP
>
>  config MMC_OMAP_HS
> tristate "TI OMAP High Speed Multimedia Card Interface support"
> -   depends on HAS_DMA
> depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || COMPILE_TEST
> help
>   This selects the TI OMAP High Speed Multimedia card Interface.
> @@ -511,7 +509,6 @@ config MMC_DAVINCI
>
>  config MMC_GOLDFISH
> tristate "goldfish qemu Multimedia Card Interface support"
> -   depends on HAS_DMA
> depends on GOLDFISH || COMPILE_TEST
> help
>   This selects the Goldfish Multimedia card Interface emulation
> @@ -605,7 +602,7 @@ config MMC_SDHI
>
>  config MMC_SDHI_SYS_DMAC
> tristate "DMA for SDHI SD/SDIO controllers using SYS-DMAC"
> -   depends on MMC_SDHI && HAS_DMA
> +   depends on MMC_SDHI
> default MMC_SDHI if (SUPERH || ARM)
> help
>   This provides DMA support for SDHI SD/SDIO controllers
> @@ -615,7 +612,7 @@ config MMC_SDHI_SYS_DMAC
>  config MMC_SDHI_INTERNAL_DMAC
> tristate "DMA for SDHI SD/SDIO controllers using on-chip bus 
> mastering"
> depends on ARM64 || COMPILE_TEST
> -   depends on MMC_SDHI && HAS_DMA
> +   depends on MMC_SDHI
> default MMC_SDHI if ARM64
> help
>   This provides DMA support for SDHI SD/SDIO controllers
> @@ -669,7 +666,6 @@ config MMC_CAVIUM_THUNDERX
>
>  config MMC_DW
> tristate "Synopsys DesignWare Memory Card Interface"
> -   depends on HAS_DMA
> depends on ARC || ARM || ARM64 || MIPS || COMPILE_TEST
> help
>   This selects support for the Synopsys DesignWare Mobile Storage IP
> @@ -748,7 +744,6 @@ config MMC_DW_ZX
>
>  config MMC_SH_MMCIF
> tristate "SuperH Internal MMCIF support"
> -   depends on HAS_DMA
> depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> help
>   This selects the MMC Host Interface controller (MMCIF) found in 
> various
> @@ -868,7 +863,6 @@ config MMC_TOSHIBA_PCI
>  config MMC_BCM2835
> tristate "Broadcom BCM2835 SDHOST MMC Controller support"
> depends on ARCH_BCM2835 || COMPILE_TEST
> -   depends on HAS_DMA
> help
>   This selects the BCM2835 SDHOST MMC controller. If you have
>   a BCM2835 platform with SD or MMC devices, say Y or M here.
> --
> 2.7.4
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/15] ARM: pxa: switch to DMA slave maps

[Ulf Hansson]

On 4 April 2018 at 21:56, Boris Brezillon <boris.brezil...@bootlin.com> wrote:
> On Wed, 04 Apr 2018 21:49:26 +0200
> Robert Jarzmik <robert.jarz...@free.fr> wrote:
>
>> Ulf Hansson <ulf.hans...@linaro.org> writes:
>>
>> > On 2 April 2018 at 16:26, Robert Jarzmik <robert.jarz...@free.fr> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/15] ARM: pxa: switch to DMA slave maps

[Ulf Hansson]

On 2 April 2018 at 16:26, Robert Jarzmik <robert.jarz...@free.fr> 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 <ulf.hans...@linaro.org>

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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 13/21] mmc: Remove depends on HAS_DMA in case of platform dependency

[Ulf Hansson]

On 16 March 2018 at 14:51, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Remove dependencies on HAS_DMA where a Kconfig symbol depends on another
> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
> In most cases this other symbol is an architecture or platform specific
> symbol, or PCI.
>
> Generic symbols and drivers without platform dependencies keep their
> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
> cannot work anyway.
>
> This simplifies the dependencies, and allows to improve compile-testing.
>
> Signed-off-by: Geert Uytterhoeven <ge...@linux-m68k.org>
> Reviewed-by: Mark Brown <broo...@kernel.org>
> Acked-by: Robin Murphy <robin.mur...@arm.com>

Acked-by: Ulf Hansson <ulf.hans...@linaro.org>

> ---
> v2:
>   - Add Reviewed-by, Acked-by,
>   - Drop RFC state,
>   - Split per subsystem.
> ---
>  drivers/mmc/host/Kconfig | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 620c2d90a646f387..f6d43348b4a3e5d4 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -358,7 +358,6 @@ config MMC_MESON_MX_SDIO
> tristate "Amlogic Meson6/Meson8/Meson8b SD/MMC Host Controller 
> support"
> depends on ARCH_MESON || COMPILE_TEST
> depends on COMMON_CLK
> -   depends on HAS_DMA
> depends on OF
> help
>   This selects support for the SD/MMC Host Controller on
> @@ -401,7 +400,6 @@ config MMC_OMAP
>
>  config MMC_OMAP_HS
> tristate "TI OMAP High Speed Multimedia Card Interface support"
> -   depends on HAS_DMA
> depends on ARCH_OMAP2PLUS || ARCH_KEYSTONE || COMPILE_TEST
> help
>   This selects the TI OMAP High Speed Multimedia card Interface.
> @@ -511,7 +509,6 @@ config MMC_DAVINCI
>
>  config MMC_GOLDFISH
> tristate "goldfish qemu Multimedia Card Interface support"
> -   depends on HAS_DMA
> depends on GOLDFISH || COMPILE_TEST
> help
>   This selects the Goldfish Multimedia card Interface emulation
> @@ -605,7 +602,7 @@ config MMC_SDHI
>
>  config MMC_SDHI_SYS_DMAC
> tristate "DMA for SDHI SD/SDIO controllers using SYS-DMAC"
> -   depends on MMC_SDHI && HAS_DMA
> +   depends on MMC_SDHI
> default MMC_SDHI if (SUPERH || ARM)
> help
>   This provides DMA support for SDHI SD/SDIO controllers
> @@ -615,7 +612,7 @@ config MMC_SDHI_SYS_DMAC
>  config MMC_SDHI_INTERNAL_DMAC
> tristate "DMA for SDHI SD/SDIO controllers using on-chip bus 
> mastering"
> depends on ARM64 || COMPILE_TEST
> -   depends on MMC_SDHI && HAS_DMA
> +   depends on MMC_SDHI
> default MMC_SDHI if ARM64
> help
>   This provides DMA support for SDHI SD/SDIO controllers
> @@ -688,7 +685,6 @@ config MMC_CAVIUM_THUNDERX
>
>  config MMC_DW
> tristate "Synopsys DesignWare Memory Card Interface"
> -   depends on HAS_DMA
> depends on ARC || ARM || ARM64 || MIPS || COMPILE_TEST
> help
>   This selects support for the Synopsys DesignWare Mobile Storage IP
> @@ -758,7 +754,6 @@ config MMC_DW_ZX
>
>  config MMC_SH_MMCIF
> tristate "SuperH Internal MMCIF support"
> -   depends on HAS_DMA
> depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> help
>   This selects the MMC Host Interface controller (MMCIF) found in 
> various
> @@ -878,7 +873,6 @@ config MMC_TOSHIBA_PCI
>  config MMC_BCM2835
> tristate "Broadcom BCM2835 SDHOST MMC Controller support"
> depends on ARCH_BCM2835 || COMPILE_TEST
> -   depends on HAS_DMA
> help
>   This selects the BCM2835 SDHOST MMC controller. If you have
>   a BCM2835 platform with SD or MMC devices, say Y or M here.
> --
> 2.7.4
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: Add mmc_force_detect_change_begin / _end functions

[Ulf Hansson]

[...]

>> > I'd like to know if any progress has been made on that problem (I may
>> > have missed patches).
>> > Had you had the time to look at the issue?
>>
>> I have looked at the issue, but not manage to cook some patches for it.
>>
>> However, it's on my top of my TODO list for mmc. No promises, but
>> perhaps and hopefully I manage to get something posted during the
>> coming release cycle.
>>
>
> Cool! If you ever need some testing, I'd be glad to test your patches
> (even if they are in a draft/RFC state).
>
> Also, when you send patches, I'd appreciate being Cc'ed so that I can
> put my Tested-by :) Thanks!

Absolutely! I appreciate it.

Br
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: Add mmc_force_detect_change_begin / _end functions

[Ulf Hansson]

On 8 February 2018 at 15:59, Quentin Schulz <quentin.sch...@bootlin.com> wrote:
> Hi Ulf,
>
> On Wed, Aug 30, 2017 at 03:43:49PM +0200, Ulf Hansson wrote:
>> On 30 August 2017 at 14:44, Hans de Goede <hdego...@redhat.com> wrote:
>> > Hi,
>> >
>> >
>> > On 21-07-17 16:35, Quentin Schulz wrote:
>> >>
>> >> From: Hans de Goede <hdego...@redhat.com>
>> >>
>> >> Some sdio devices have a multiple stage bring-up process. Specifically
>> >> the esp8089 (for which an out of tree driver is available) loads firmware
>> >> on the first call to its sdio-drivers' probe function and then resets
>> >> the device causing it to reboot from its RAM with the new firmware.
>> >>
>> >> When this sdio device reboots it comes back up in 1 bit 400 KHz mode
>> >> again, and we need to walk through the whole ios negatiation and sdio
>> >> setup
>> >> again.
>> >>
>> >> There are 2 problems with this:
>> >>
>> >> 1) Typically these devices are soldered onto some (ARM) tablet / SBC
>> >> PCB and as such are described in devicetree as "non-removable", which
>> >> causes the mmc-core to scan them only once and not poll for the device
>> >> dropping of the bus. Normally this is the right thing todo but in the
>> >> eso8089 example we need the mmc-core to notice the module has disconnected
>> >> (since it is now in 1 bit mode again it will not talk to the host in 4 bit
>> >> mode). This can be worked around by using "broken-cd" in devicetree
>> >> instead of "non-removable", but that is not a proper fix since the device
>> >> really is non-removable.
>> >>
>> >> 2) When the mmc-core detects the device has disconnected it will poweroff
>> >> the device, causing the RAM loaded firmware to be lost. This can be worked
>> >> around in devicetree by using regulator-always-on (and avoiding the use of
>> >> mmc-pwrseq), but again that is more of a hack then a proper fix.
>> >>
>> >> This commmit fixes 1) by adding a mmc_force_detect_change function which
>> >> will cause scanning for device removal / insertion until a new device is
>> >> detected. 2) Is fixed by a keep_power flag to the mmc_force_detect_change
>> >> function which when set causes the mmc-core to keep the power to the
>> >> device
>> >> on during the rescan.
>> >>
>> >> Cc: Icenowy Zheng <icen...@aosc.xyz>
>> >> Cc: Maxime Ripard <maxime.rip...@free-electrons.com>
>> >> Cc: Chen-Yu Tsai <w...@csie.org>
>> >> Signed-off-by: Hans de Goede <hdego...@redhat.com>
>> >
>> >
>> > So when I posted this patch quite a while back, there was some discussion
>> > about this and a consensus that this is not the right solution.
>> >
>> > So first of all lets describe the problem:
>> >
>> > The esp8089 sdio wifi chip is really an ARM core with a wifi phy
>> > connected to it (as many wifi chipsets are).
>> >
>> > But this one comes up in some really generic sdio capable boot-loader
>> > mode and we need to feed it firmware and then reboot it into the
>> > new firmware.
>> >
>> > The reboot is where the problems happens. It seems to fallback
>> > from the negotiated 4 wire sdio mode to single wire spi mode then.
>> >
>> > The out of tree version of the driver deals with this by not setting
>> > the non-removable flag as well as setting the broken_cd flag so that
>> > the mmc core polls the device, after the reboot the poll fails
>> > because the mmc-controller and the esp8089 are using a different
>> > amount of wires so the mmc-cmd the poll uses times out.
>> >
>> > After which the esp8089 drivers remove function gets called, and
>> > the mmc stack re-discovers the esp8089 by restarting the whole
>> > number of wires (and speed) used negotiation. After which the
>> > esp8089 driver's probe function gets called (again) and on
>> > firmware loading is has set a global flag, so now it actually
>> > acts as a wifi driver rather then trying to load the firmware
>> > a second time.
>> >
>> > Since I did not want to rely on broken_cd polling I came up
>> > with the hack which is this patch.
>> >
>> > So when this patch was first discussed we came to the conclusion
>> > that 

Re: [PATCH 2/2] mmc: Add mmc_force_detect_change_begin / _end functions

[Ulf Hansson]

On 30 August 2017 at 14:44, Hans de Goede  wrote:
> Hi,
>
>
> On 21-07-17 16:35, Quentin Schulz wrote:
>>
>> From: Hans de Goede 
>>
>> Some sdio devices have a multiple stage bring-up process. Specifically
>> the esp8089 (for which an out of tree driver is available) loads firmware
>> on the first call to its sdio-drivers' probe function and then resets
>> the device causing it to reboot from its RAM with the new firmware.
>>
>> When this sdio device reboots it comes back up in 1 bit 400 KHz mode
>> again, and we need to walk through the whole ios negatiation and sdio
>> setup
>> again.
>>
>> There are 2 problems with this:
>>
>> 1) Typically these devices are soldered onto some (ARM) tablet / SBC
>> PCB and as such are described in devicetree as "non-removable", which
>> causes the mmc-core to scan them only once and not poll for the device
>> dropping of the bus. Normally this is the right thing todo but in the
>> eso8089 example we need the mmc-core to notice the module has disconnected
>> (since it is now in 1 bit mode again it will not talk to the host in 4 bit
>> mode). This can be worked around by using "broken-cd" in devicetree
>> instead of "non-removable", but that is not a proper fix since the device
>> really is non-removable.
>>
>> 2) When the mmc-core detects the device has disconnected it will poweroff
>> the device, causing the RAM loaded firmware to be lost. This can be worked
>> around in devicetree by using regulator-always-on (and avoiding the use of
>> mmc-pwrseq), but again that is more of a hack then a proper fix.
>>
>> This commmit fixes 1) by adding a mmc_force_detect_change function which
>> will cause scanning for device removal / insertion until a new device is
>> detected. 2) Is fixed by a keep_power flag to the mmc_force_detect_change
>> function which when set causes the mmc-core to keep the power to the
>> device
>> on during the rescan.
>>
>> Cc: Icenowy Zheng 
>> Cc: Maxime Ripard 
>> Cc: Chen-Yu Tsai 
>> Signed-off-by: Hans de Goede 
>
>
> So when I posted this patch quite a while back, there was some discussion
> about this and a consensus that this is not the right solution.
>
> So first of all lets describe the problem:
>
> The esp8089 sdio wifi chip is really an ARM core with a wifi phy
> connected to it (as many wifi chipsets are).
>
> But this one comes up in some really generic sdio capable boot-loader
> mode and we need to feed it firmware and then reboot it into the
> new firmware.
>
> The reboot is where the problems happens. It seems to fallback
> from the negotiated 4 wire sdio mode to single wire spi mode then.
>
> The out of tree version of the driver deals with this by not setting
> the non-removable flag as well as setting the broken_cd flag so that
> the mmc core polls the device, after the reboot the poll fails
> because the mmc-controller and the esp8089 are using a different
> amount of wires so the mmc-cmd the poll uses times out.
>
> After which the esp8089 drivers remove function gets called, and
> the mmc stack re-discovers the esp8089 by restarting the whole
> number of wires (and speed) used negotiation. After which the
> esp8089 driver's probe function gets called (again) and on
> firmware loading is has set a global flag, so now it actually
> acts as a wifi driver rather then trying to load the firmware
> a second time.
>
> Since I did not want to rely on broken_cd polling I came up
> with the hack which is this patch.
>
> So when this patch was first discussed we came to the conclusion
> that what we really need is some sort of mmc_reprobe_device
> function which the driver can call from probe which will
> redo the number of wires (and speed) used negotiation,
> while keeping the sdio_function device as is so that probe can
> simply continue after this and we also don't need the ugly
> global flag.
>
> The idea would be for this function to be some wrapper
> around mmc_init_card() which resets the ios settings as is
> normally done on remove and then call mmc_init_card()
> passing in the existing card the same way as is done
> one resume, so that the existing card / sdio_function
> devices get reused.
>
> IIRC Ulf would look into writing this mmc_reprobe_device
> function and then I would test it with the esp8089, but
> Ulf never got around to writing the function and I ended
> up working on other things too.

Thanks for summary!

Just to let you know, I haven't forgot about this problem. I am
planning for a major update of the SDIO for power management support,
within a not too far future.
The issue described above, is then also one of the things I also plan
to look into.

However, if there anybody that wants to hack on this, feel free to do it!

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org

Re: [PATCH v2 5/7] staging: board: Add support for devices with complex dependencies

[Ulf Hansson]

On 4 September 2015 at 17:03, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Fri, 4 Sep 2015, Ulf Hansson wrote:
>> On 3 September 2015 at 15:35, Geert Uytterhoeven <ge...@linux-m68k.org> 
>> wrote:
>> > On Thu, Sep 3, 2015 at 2:53 PM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
>> >> On 17 June 2015 at 10:38, Geert Uytterhoeven <geert+rene...@glider.be> 
>> >> wrote:
>> >>> Add support for easy registering of one ore more platform devices that
>> >>> may:
>> >>>   - need clocks that are described in DT,
>> >>>   - be part of a PM Domain.
>> >
>> >>> diff --git a/drivers/staging/board/board.c 
>> >>> b/drivers/staging/board/board.c
>> >>> index 8712f566b31196e0..29d456e29f38feac 100644
>> >>> --- a/drivers/staging/board/board.c
>> >>> +++ b/drivers/staging/board/board.c
>> >
>> >>> +int __init board_staging_register_device(const struct board_staging_dev 
>> >>> *dev)
>> >>> +{
>> >>> +   struct platform_device *pdev = dev->pdev;
>> >>> +   unsigned int i;
>> >>> +   int error;
>> >>> +
>> >>> +   pr_debug("Trying to register device %s\n", pdev->name);
>> >>> +   if (board_staging_dt_node_available(pdev->resource,
>> >>> +   pdev->num_resources)) {
>> >>> +   pr_warn("Skipping %s, already in DT\n", pdev->name);
>> >>> +   return -EEXIST;
>> >>> +   }
>> >>> +
>> >>> +   board_staging_gic_fixup_resources(pdev->resource, 
>> >>> pdev->num_resources);
>> >>> +
>> >>> +   for (i = 0; i < dev->nclocks; i++)
>> >>> +   board_staging_register_clock(>clocks[i]);
>> >>> +
>> >>> +   error = platform_device_register(pdev);
>> >>> +   if (error) {
>> >>> +   pr_err("Failed to register device %s (%d)\n", pdev->name,
>> >>> +  error);
>> >>> +   return error;
>> >>> +   }
>> >>> +
>> >>> +   if (dev->domain)
>> >>> +   __pm_genpd_name_add_device(dev->domain, >dev, 
>> >>> NULL);
>> >>
>> >> Urgh, this managed to slip through my filters.
>> >>
>> >> It seems like we almost managed to remove all users of the
>> >> "..._name_add..." APIs for genpd. If hasn't been for $subject patch.
>> >> :-)
>> >>
>> >> Now, I realize this is already too late here, but let's try to fix
>> >> this before it turns into a bigger issue.
>> >>
>> >> Geert, do you think it's possible to convert into using the non-named
>> >> bases APIs?
>> >
>> > That will be difficult. This code is meant to use drivers that are not yet
>> > DT-aware on DT-based systems. Hence it uses platform devices with named PM
>> > domains, while the PM domains are described in DT.
>> > I don't think there's another way to look up a PM domain by name, is there?
>>
>> As a matter of fact there are, especially for those genpds that has
>> been created through DT as in this case. The API to use is
>> of_genpd_get_from_provider() to find the struct generic_pm_domain.
>
> Thanks!
>
>> Yes, I do realize that you need to manage the parsing of the domain
>> name to make sure it's the one you want, but I would rather keep that
>> "hack" in this driver than in the generic API.
>
> OK. It turned out not to be that complex, cfr. the patch below.
> For now this supports PM domains with "#power-domain-cells = <0>" only,
> but of course it can be extended if the need arises.
>
> I've also tried:
>
> np = of_find_compatible_node(NULL, NULL, "renesas,sysc-rmobile");
> np = of_find_node_by_name(np, "a4lc");
>
> (the second step is needed because of the domain hierarchy in DT), but that
> would require adding the compatible string to struct board_staging_dev,
> and doesn't work if you have multiple identical power providers.
>
> I hope you like it?
>
> From 5fb11904845eb929a5b3382a9d5153c151cde1cf Mon Sep 17 00:00:00 2001
> From: Geert Uytterhoeven <geert+rene...@glider.be>
> Da

Re: [PATCH v2 5/7] staging: board: Add support for devices with complex dependencies

[Ulf Hansson]

On 3 September 2015 at 15:35, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Thu, Sep 3, 2015 at 2:53 PM, Ulf Hansson <ulf.hans...@linaro.org> wrote:
>> On 17 June 2015 at 10:38, Geert Uytterhoeven <geert+rene...@glider.be> wrote:
>>> Add support for easy registering of one ore more platform devices that
>>> may:
>>>   - need clocks that are described in DT,
>>>   - be part of a PM Domain.
>
>>> diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
>>> index 8712f566b31196e0..29d456e29f38feac 100644
>>> --- a/drivers/staging/board/board.c
>>> +++ b/drivers/staging/board/board.c
>
>>> +int __init board_staging_register_device(const struct board_staging_dev 
>>> *dev)
>>> +{
>>> +   struct platform_device *pdev = dev->pdev;
>>> +   unsigned int i;
>>> +   int error;
>>> +
>>> +   pr_debug("Trying to register device %s\n", pdev->name);
>>> +   if (board_staging_dt_node_available(pdev->resource,
>>> +   pdev->num_resources)) {
>>> +   pr_warn("Skipping %s, already in DT\n", pdev->name);
>>> +   return -EEXIST;
>>> +   }
>>> +
>>> +   board_staging_gic_fixup_resources(pdev->resource, 
>>> pdev->num_resources);
>>> +
>>> +   for (i = 0; i < dev->nclocks; i++)
>>> +   board_staging_register_clock(>clocks[i]);
>>> +
>>> +   error = platform_device_register(pdev);
>>> +   if (error) {
>>> +   pr_err("Failed to register device %s (%d)\n", pdev->name,
>>> +  error);
>>> +   return error;
>>> +   }
>>> +
>>> +   if (dev->domain)
>>> +   __pm_genpd_name_add_device(dev->domain, >dev, NULL);
>>
>> Urgh, this managed to slip through my filters.
>>
>> It seems like we almost managed to remove all users of the
>> "..._name_add..." APIs for genpd. If hasn't been for $subject patch.
>> :-)
>>
>> Now, I realize this is already too late here, but let's try to fix
>> this before it turns into a bigger issue.
>>
>> Geert, do you think it's possible to convert into using the non-named
>> bases APIs?
>
> That will be difficult. This code is meant to use drivers that are not yet
> DT-aware on DT-based systems. Hence it uses platform devices with named PM
> domains, while the PM domains are described in DT.
> I don't think there's another way to look up a PM domain by name, is there?

As a matter of fact there are, especially for those genpds that has
been created through DT as in this case. The API to use is
of_genpd_get_from_provider() to find the struct generic_pm_domain.

Yes, I do realize that you need to manage the parsing of the domain
name to make sure it's the one you want, but I would rather keep that
"hack" in this driver than in the generic API.

>
> This code is meant to go away, once all drivers are converted to DT, or
> considered obsolete.

Well, who knows *when* that is going to happen. :-)

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 5/7] staging: board: Add support for devices with complex dependencies

[Ulf Hansson]

On 17 June 2015 at 10:38, Geert Uytterhoeven  wrote:
> Add support for easy registering of one ore more platform devices that
> may:
>   - need clocks that are described in DT,
>   - be part of a PM Domain.
>
> All these dependencies are optional.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> v2:
>   - Drop support for pinctrl, use standard pinctrl in DT instead,
>   - Drop support for configured GPIOs, use "gpio-hog" in DT instead,
>   - Use clk_add_alias() instead of open coding,
>   - Update for changed function names,
>   - Drop RFC status.
> ---
>  drivers/staging/board/board.c | 56 
> +++
>  drivers/staging/board/board.h | 20 
>  2 files changed, 76 insertions(+)
>
> diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
> index 8712f566b31196e0..29d456e29f38feac 100644
> --- a/drivers/staging/board/board.c
> +++ b/drivers/staging/board/board.c
> @@ -9,6 +9,7 @@
>
>  #define pr_fmt(fmt)"board_staging: "  fmt
>
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -16,6 +17,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>
>  #include "board.h"
>
> @@ -118,3 +121,56 @@ void __init board_staging_gic_fixup_resources(struct 
> resource *res,
> for (i = 0; i < nres; i++)
> gic_fixup_resource([i]);
>  }
> +
> +int __init board_staging_register_clock(const struct board_staging_clk *bsc)
> +{
> +   int error;
> +
> +   pr_debug("Aliasing clock %s for con_id %s dev_id %s\n", bsc->clk,
> +bsc->con_id, bsc->dev_id);
> +   error = clk_add_alias(bsc->con_id, bsc->dev_id, bsc->clk, NULL);
> +   if (error)
> +   pr_err("Failed to alias clock %s (%d)\n", bsc->clk, error);
> +
> +   return error;
> +}
> +
> +int __init board_staging_register_device(const struct board_staging_dev *dev)
> +{
> +   struct platform_device *pdev = dev->pdev;
> +   unsigned int i;
> +   int error;
> +
> +   pr_debug("Trying to register device %s\n", pdev->name);
> +   if (board_staging_dt_node_available(pdev->resource,
> +   pdev->num_resources)) {
> +   pr_warn("Skipping %s, already in DT\n", pdev->name);
> +   return -EEXIST;
> +   }
> +
> +   board_staging_gic_fixup_resources(pdev->resource, 
> pdev->num_resources);
> +
> +   for (i = 0; i < dev->nclocks; i++)
> +   board_staging_register_clock(>clocks[i]);
> +
> +   error = platform_device_register(pdev);
> +   if (error) {
> +   pr_err("Failed to register device %s (%d)\n", pdev->name,
> +  error);
> +   return error;
> +   }
> +
> +   if (dev->domain)
> +   __pm_genpd_name_add_device(dev->domain, >dev, NULL);

Urgh, this managed to slip through my filters.

It seems like we almost managed to remove all users of the
"..._name_add..." APIs for genpd. If hasn't been for $subject patch.
:-)

Now, I realize this is already too late here, but let's try to fix
this before it turns into a bigger issue.

Geert, do you think it's possible to convert into using the non-named
bases APIs?

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] sdhci: rtsx: fix 64 BIT DMA quirks

[Ulf Hansson]

On 7 April 2015 at 05:32,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 rts5250 chip failed handle 64 bit ADMA for address below 4G.
 Add 64 BIT quirks to disable this feature.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn

Thanks! Applied, with a minor updated commit message header.

Kind regards
Uffe

 ---
  drivers/mmc/host/sdhci-pci.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
 index 0342775..ae8e450 100644
 --- a/drivers/mmc/host/sdhci-pci.c
 +++ b/drivers/mmc/host/sdhci-pci.c
 @@ -650,6 +650,7 @@ static int rtsx_probe_slot(struct sdhci_pci_slot *slot)

  static const struct sdhci_pci_fixes sdhci_rtsx = {
 .quirks2= SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
 +   SDHCI_QUIRK2_BROKEN_64_BIT_DMA |
 SDHCI_QUIRK2_BROKEN_DDR50,
 .probe_slot = rtsx_probe_slot,
  };
 --
 1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] mmc: rtsx: add check before sending request

[Ulf Hansson]

On 14 January 2015 at 04:09,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 Add check before sending request can make request return faster.
 - finish request if no card exist
   This can make request finish faster, especial for some sdio card,
   when card removed, there still a lot of command pending,
   if we check card exist and stop request, the card will disappear
   faster in user space.

 - check sg_count before long data xfer
   modify sg_count type unsigned - int, because dma_map_sg() return
   int, and this value can be negative to indicate some error occurs.

 Micky Ching (2):
   mmc: rtsx: finish request if no card exist
   mmc: rtsx: check sg_count before long data xfer

  drivers/mmc/host/rtsx_pci_sdmmc.c | 20 
  1 file changed, 16 insertions(+), 4 deletions(-)

 --
 1.9.1


Thanks! Applied for next.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/6] mfd: rtsx: add func to split u32 into register

[Ulf Hansson]

On 20 January 2015 at 15:47, Lee Jones lee.jo...@linaro.org wrote:
 On Tue, 23 Dec 2014, micky_ch...@realsil.com.cn wrote:

 From: Micky Ching micky_ch...@realsil.com.cn

 Add helper function to write u32 to registers, if we want to put u32
 value to 4 continuous register, this can help us reduce tedious work.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
 Acked-by: Lee Jones lee.jo...@linaro.org
 ---
  include/linux/mfd/rtsx_pci.h | 9 +
  1 file changed, 9 insertions(+)

 Applied, thanks.

This one has already been queued by you, for 3.19, and it's in Linus tree. :-)

For your reference, I have queued the mmc patches which depends on
$subject patch for 3.20.

Kind regards
Uffe



 diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h
 index 74346d5..9234449 100644
 --- a/include/linux/mfd/rtsx_pci.h
 +++ b/include/linux/mfd/rtsx_pci.h
 @@ -558,6 +558,7 @@
  #define SD_SAMPLE_POINT_CTL  0xFDA7
  #define SD_PUSH_POINT_CTL0xFDA8
  #define SD_CMD0  0xFDA9
 +#define   SD_CMD_START   0x40
  #define SD_CMD1  0xFDAA
  #define SD_CMD2  0xFDAB
  #define SD_CMD3  0xFDAC
 @@ -967,4 +968,12 @@ static inline u8 *rtsx_pci_get_cmd_data(struct rtsx_pcr 
 *pcr)
   return (u8 *)(pcr-host_cmds_ptr);
  }

 +static inline void rtsx_pci_write_be32(struct rtsx_pcr *pcr, u16 reg, u32 
 val)
 +{
 + rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, reg, 0xFF, val  24);
 + rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, reg + 1, 0xFF, val  16);
 + rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, reg + 2, 0xFF, val  8);
 + rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, reg + 3, 0xFF, val);
 +}
 +
  #endif

 --
 Lee Jones
 Linaro STMicroelectronics Landing Team Lead
 Linaro.org │ Open source software for ARM SoCs
 Follow Linaro: Facebook | Twitter | Blog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/6] mfd: rtsx: add func to split u32 into register

[Ulf Hansson]

On 8 December 2014 at 10:57, Lee Jones lee.jo...@linaro.org wrote:
 On Fri, 05 Dec 2014, micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 Add helper function to write u32 to registers, if we want to put u32
 value to 4 continuous register, this can help us reduce tedious work.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
 Acked-by: Lee Jones lee.jo...@linaro.org
 ---
  include/linux/mfd/rtsx_pci.h | 9 +
  1 file changed, 9 insertions(+)

 Applied, thanks.

Lee,

I suppose you queued this for 3.19?

I don't intend to queue anything more before the merge window and thus
the mmc patches can later be handled without any dependence to mfd, I
guess!?

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 0/2] mmc: rtsx: add support for sdio card

[Ulf Hansson]

On 2 December 2014 at 02:36,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 v3:
   rtsx_pci_sdmmc.c:
 - dump_reg_range
   - remove unused pointer check
   - fix start index

I can't find v3.

Kind regards
Uffe

 v2:
   rtsx_pci.h:
 - remove unused rtsx_pci_write_le32
 - add SD_CMD_START
   rtsx_pci_sdmmc.c:
 - dump_reg_range
   - alloc data on stack
 - remove forward declaration
 - use SD_CMD_START replace magic number 0x40
 - move initialize assignment to error handling

 This patch is used to change transfer mode for sdio card support
 by SD interface.


 Micky Ching (2):
   mfd: rtsx: add func to split u32 into register
   mmc: rtsx: add support for sdio card

  drivers/mmc/host/rtsx_pci_sdmmc.c | 522 
 +-
  include/linux/mfd/rtsx_pci.h  |   9 +
  2 files changed, 296 insertions(+), 235 deletions(-)

 --
 1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] mmc: core: Add new power_mode MMC_POWER_UNDEFINED

[Ulf Hansson]

On 24 September 2014 11:07, Roger Tseng rogera...@realtek.com wrote:
 Define new macro MMC_POWER_UNDEFINED for power_mode in struct mmc_ios.
 It will also be set as the initial value of host-ios.power_mode in
 mmc_start_host().

 For hosts with MMC_CAP2_NO_PRESCAN_POWERUP, this makes the later
 mmc_power_off() do real power-off things instead of NOP, and further
 prevents state messed up in cards that was already initialized(eg. by
 BIOS of UEFI driver).

 Signed-off-by: Roger Tseng rogera...@realtek.com

Thanks! Applied for next!

I changes some minor parts of the commit message and set the author of
the patch to Roger Tseng rogera...@realtek.com instead of
rogera...@realtek.com. Please fix that in for future patches.

Kind regards
Uffe

 ---
  drivers/mmc/core/core.c  |1 +
  include/linux/mmc/host.h |1 +
  2 files changed, 2 insertions(+)

 diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
 index d03a080fb9cd..7dad1a1adf18 100644
 --- a/drivers/mmc/core/core.c
 +++ b/drivers/mmc/core/core.c
 @@ -2489,6 +2489,7 @@ void mmc_start_host(struct mmc_host *host)
  {
 host-f_init = max(freqs[0], host-f_min);
 host-rescan_disable = 0;
 +   host-ios.power_mode = MMC_POWER_UNDEFINED;
 if (host-caps2  MMC_CAP2_NO_PRESCAN_POWERUP)
 mmc_power_off(host);
 else
 diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
 index 7960424d0bc0..b3bfa609816a 100644
 --- a/include/linux/mmc/host.h
 +++ b/include/linux/mmc/host.h
 @@ -42,6 +42,7 @@ struct mmc_ios {
  #define MMC_POWER_OFF  0
  #define MMC_POWER_UP   1
  #define MMC_POWER_ON   2
 +#define MMC_POWER_UNDEFINED3

 unsigned char   bus_width;  /* data bus width */

 --
 1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx_pci: Set power related cap2 macros

[Ulf Hansson]

On 24 September 2014 11:07, Roger Tseng rogera...@realtek.com wrote:
 Set MMC_CAP2_NO_PRESCAN_POWERUP and MMC_CAP2_FULL_PWR_CYCLE for
 rtsx_pci_sdmmc and rtsx_usb_sdmmc to reflect properties of Realtek
 card reader hosts.

 Signed-off-by: Roger Tseng rogera...@realtek.com

Thanks! Applied for next!

Kind regards
Uffe

 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |1 +
  drivers/mmc/host/rtsx_usb_sdmmc.c |1 +
  2 files changed, 2 insertions(+)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index dfde4a210238..d49460b5ff07 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -1292,6 +1292,7 @@ static void realtek_init_host(struct realtek_pci_sdmmc 
 *host)
 mmc-caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED |
 MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST |
 MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25;
 +   mmc-caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE;
 mmc-max_current_330 = 400;
 mmc-max_current_180 = 800;
 mmc-ops = realtek_pci_sdmmc_ops;
 diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c 
 b/drivers/mmc/host/rtsx_usb_sdmmc.c
 index 5d3766e792f0..a884631d7eea 100644
 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
 @@ -1329,6 +1329,7 @@ static void rtsx_usb_init_host(struct rtsx_usb_sdmmc 
 *host)
 MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST |
 MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
 MMC_CAP_NEEDS_POLL;
 +   mmc-caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE;

 mmc-max_current_330 = 400;
 mmc-max_current_180 = 800;
 --
 1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mmc: rtsx: add card power off during probe

[Ulf Hansson]

[...]


 In that case, don't forget to enable MMC_CAP2_FULL_PWR_CYCLE.

 
  if MMC_CAP2_NO_PRESCAN_POWERUP enable, will call mmc_power_off() at start,
  then it will check ios.power_mode, but the state is MMC_POWER_OFF and just
  return.

 Uhh, that's right! So, I wonder why we invokes mmc_power_off() from
 that path at all.

 Hmm, I think we should change the behavior in mmc_start_host(), like below:
 1) Add a MMC_POWER_UNDEFINED state which is what the power state
 should be assigned to at allocation.
 2 ) From mmc_start_host(), invoke mmc_power_off() when
 MMC_CAP2_NO_PRESCAN_POWERUP and MMC_CAP2_FULL_PWR_CYCLE is set.

 Would that work?
 Yes. I have confirmed this by following changes. The MMC_POWER_UNDEFINED
 designation in mmc_start_host() will eventually cause a power-off
 operation.

 But I wonder if we need to additionally check MMC_CAP2_FULL_PWR_CYCLE
 before calling mmc_power_off()?

The intent from my side was to keep the current behaviour for those
that already used MMC_CAP2_NO_PRESCAN_POWERUP, but it's s not a big
deal.

So, let's try your proposal, thus don't check MMC_CAP2_FULL_PWR_CYCLE.

Can you repost new version of your patches and please split them up on
core and host separately.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mmc: rtsx: add card power off during probe

[Ulf Hansson]

On 12 September 2014 03:39,  micky_ch...@realsil.com.cn wrote:
 From: Roger Tseng rogera...@realtek.com

 Some platform have both UEFI driver and MFD/mmc driver, if entering
 linux while card in the slot, the card power is already on, and rtsx-mmc
 driver have no chance to make card power off. This will lead UHSI card
 failed to enter UHSI mode.

 It is hard to control the UEFI driver leaving state, so we power off the
 card power during probe.

 Signed-off-by: Roger Tseng rogera...@realtek.com
 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index dfde4a2..57b0796 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -1341,8 +1341,13 @@ static int rtsx_pci_sdmmc_drv_probe(struct 
 platform_device *pdev)
 host-pcr = pcr;
 host-mmc = mmc;
 host-pdev = pdev;
 -   host-power_state = SDMMC_POWER_OFF;
 INIT_WORK(host-work, sd_request);
 +   sd_power_off(host);
 +   /*
 +* ref: SD spec 3.01: 6.4.1.2 Power On or Power Cycle
 +*/
 +   usleep_range(1000, 2000);
 +

This won't work in cases were you power off eMMC cards, unless you can
do a full power cycle - cut both VCC and VCCQ. Can you?

There are also another option you might want to use,
MMC_CAP2_NO_PRESCAN_POWERUP. But again, it must only be used for those
hosts that you are able to do a full power cycle for.

Kind regards
Uffe

 platform_set_drvdata(pdev, host);
 pcr-slots[RTSX_SD_CARD].p_dev = pdev;
 pcr-slots[RTSX_SD_CARD].card_event = rtsx_pci_sdmmc_card_event;
 --
 1.7.9.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] mmc: rtsx_pci_sdmmc: fix incorrect last byte in R2 response

[Ulf Hansson]

On 15 August 2014 08:06,  rogera...@realtek.com wrote:
 From: Roger Tseng rogera...@realtek.com

 Current code erroneously fill the last byte of R2 response with an undefined
 value. In addition, the controller actually 'offloads' the last byte
 (CRC7, end bit) while receiving R2 response and thus it's impossible to get 
 the
 actual value. This could cause mmc stack to obtain inconsistent CID from the
 same card after resume and misidentify it as a different card.

 Fix by assigning dummy CRC and end bit: {7'b0, 1} = 0x1 to the last byte of 
 R2.

Thanks! Applied for next.

Kind regards
Uffe


 Cc: sta...@vger.kernel.org # v3.8+
 Fixes: ff984e57d36e (mmc: Add realtek pcie sdmmc host driver)
 Signed-off-by: Roger Tseng rogera...@realtek.com
 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |7 +++
  1 file changed, 7 insertions(+)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index dfde4a210238..b2537e2f26b1 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -412,6 +412,13 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc 
 *host,
 }

 if (rsp_type == SD_RSP_TYPE_R2) {
 +   /*
 +* The controller offloads the last byte {CRC-7, end bit 1'b1}
 +* of response type R2. Assign dummy CRC, 0, and end bit to 
 the
 +* byte(ptr[16], goes into the LSB of resp[3] later).
 +*/
 +   ptr[16] = 1;
 +
 for (i = 0; i  4; i++) {
 cmd-resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 dev_dbg(sdmmc_dev(host), cmd-resp[%d] = 0x%08x\n,
 --
 1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx_usb_sdmmc: fix incorrect last byte in R2 response

[Ulf Hansson]

On 15 August 2014 08:06,  rogera...@realtek.com wrote:
 From: Roger Tseng rogera...@realtek.com

 Current code erroneously fill the last byte of R2 response with an undefined
 value. In addition, the controller actually 'offloads' the last byte
 (CRC7, end bit) while receiving R2 response and thus it's impossible to get 
 the
 actual value. This could cause mmc stack to obtain inconsistent CID from the
 same card after resume and misidentify it as a different card.

 Fix by assigning dummy CRC and end bit: {7'b0, 1} = 0x1 to the last byte of 
 R2.

 Cc: sta...@vger.kernel.org # v3.16+
 Fixes: c7f6558d84af (mmc: Add realtek USB sdmmc host driver)
 Signed-off-by: Roger Tseng rogera...@realtek.com

Thanks! Applied for next.

Kind regards
Uffe


 ---
  drivers/mmc/host/rtsx_usb_sdmmc.c |7 +++
  1 file changed, 7 insertions(+)

 diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c 
 b/drivers/mmc/host/rtsx_usb_sdmmc.c
 index 5d3766e792f0..d9153a7d160d 100644
 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
 @@ -435,6 +435,13 @@ static void sd_send_cmd_get_rsp(struct rtsx_usb_sdmmc 
 *host,
 }

 if (rsp_type == SD_RSP_TYPE_R2) {
 +   /*
 +* The controller offloads the last byte {CRC-7, end bit 1'b1}
 +* of response type R2. Assign dummy CRC, 0, and end bit to 
 the
 +* byte(ptr[16], goes into the LSB of resp[3] later).
 +*/
 +   ptr[16] = 1;
 +
 for (i = 0; i  4; i++) {
 cmd-resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 dev_dbg(sdmmc_dev(host), cmd-resp[%d] = 0x%08x\n,
 --
 1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Ulf Hansson]

On 14 August 2014 08:06, Roger Tseng rogera...@realtek.com wrote:
 On Wed, 2014-08-13 at 17:09 +0200, Ulf Hansson wrote:
 On 11 August 2014 10:32,  rogera...@realtek.com wrote:
  From: Roger Tseng rogera...@realtek.com
 
  Current code erroneously fill the last byte of R2 response with an 
  undefined
  value. In addition, it is impossible to obtain the real values since the
  controller actually 'offloads' the last byte(CRC7, end bit) while 
  receiving R2
  response. This could cause mmc stack to obtain inconsistent CID from the 
  same
  card after resume and misidentify it as a different card.
 
  Fix by assigning a dummy value 0x01 to the last byte of R2 response.
 
  Signed-off-by: Roger Tseng rogera...@realtek.com

 Thanks! Queued for 3.18.

 I guess this should go for stable as well?
 Yes. However, since rtsx_usb* is present in 3.16 and later, this patch
 will not apply on 3.15.y or older. Should I separately send an adapted
 version to stable?

I haven't pushed this externally yet. I will drop the patch from my 3.18 branch.

Then, let's split the patch into two, one for usb and one for pci -
that should simplify patch management.

Additionally, you should include a Cc tag with proper hashmark telling
which kernel each patch should be included into.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Ulf Hansson]

On 11 August 2014 10:32,  rogera...@realtek.com wrote:
 From: Roger Tseng rogera...@realtek.com

 Current code erroneously fill the last byte of R2 response with an undefined
 value. In addition, it is impossible to obtain the real values since the
 controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
 response. This could cause mmc stack to obtain inconsistent CID from the same
 card after resume and misidentify it as a different card.

 Fix by assigning a dummy value 0x01 to the last byte of R2 response.

 Signed-off-by: Roger Tseng rogera...@realtek.com

Thanks! Queued for 3.18.

I guess this should go for stable as well?

Kind regards
Uffe


 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |1 +
  drivers/mmc/host/rtsx_usb_sdmmc.c |1 +
  2 files changed, 2 insertions(+)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index dfde4a2..54849d8 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -412,6 +412,7 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc 
 *host,
 }

 if (rsp_type == SD_RSP_TYPE_R2) {
 +   ptr[16] = 1;
 for (i = 0; i  4; i++) {
 cmd-resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 dev_dbg(sdmmc_dev(host), cmd-resp[%d] = 0x%08x\n,
 diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c 
 b/drivers/mmc/host/rtsx_usb_sdmmc.c
 index 5d3766e..ca08df1 100644
 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
 @@ -435,6 +435,7 @@ static void sd_send_cmd_get_rsp(struct rtsx_usb_sdmmc 
 *host,
 }

 if (rsp_type == SD_RSP_TYPE_R2) {
 +   ptr[16] = 1;
 for (i = 0; i  4; i++) {
 cmd-resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 dev_dbg(sdmmc_dev(host), cmd-resp[%d] = 0x%08x\n,
 --
 1.7.10.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Ulf Hansson]

On 6 June 2014 09:05,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 Add support for non-blocking request, pre_req() runs dma_map_sg() and
 post_req() runs dma_unmap_sg(). This patch can increase card read/write
 speed, especially for high speed card and slow speed CPU.

 Test on intel i3(800MHz - 2.3GHz) performance mode(2.3GHz), SD card
 clock 208MHz

 run dd if=/dev/mmcblk0 of=/dev/null bs=64k count=1024
 before:
 67108864 bytes (67 MB) copied, 0.85427 s, 78.6 MB/s
 after:
 67108864 bytes (67 MB) copied, 0.74799 s, 89.7 MB/s

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn

Acked-by: Ulf Hansson ulf.hans...@linaro.org

I assume Lee will pick this patchset through his mfd tree then. If
not, ping me and I will help.

Kind regards
Uffe

 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |  133 
 +++--
  1 file changed, 127 insertions(+), 6 deletions(-)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index 1c68e0d..a2c0858 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -24,6 +24,7 @@
  #include linux/highmem.h
  #include linux/delay.h
  #include linux/platform_device.h
 +#include linux/workqueue.h
  #include linux/mmc/host.h
  #include linux/mmc/mmc.h
  #include linux/mmc/sd.h
 @@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {
 struct rtsx_pcr *pcr;
 struct mmc_host *mmc;
 struct mmc_request  *mrq;
 +   struct workqueue_struct *workq;
 +#define SDMMC_WORKQ_NAME   rtsx_pci_sdmmc_workq

 +   struct work_struct  work;
 struct mutexhost_mutex;

 u8  ssc_depth;
 @@ -48,6 +52,11 @@ struct realtek_pci_sdmmc {
 int power_state;
  #define SDMMC_POWER_ON 1
  #define SDMMC_POWER_OFF0
 +
 +   unsigned intsg_count;
 +   s32 cookie;
 +   unsigned intcookie_sg_count;
 +   boolusing_cookie;
  };

  static inline struct device *sdmmc_dev(struct realtek_pci_sdmmc *host)
 @@ -86,6 +95,77 @@ static void sd_print_debug_regs(struct realtek_pci_sdmmc 
 *host)
  #define sd_print_debug_regs(host)
  #endif /* DEBUG */

 +/*
 + * sd_pre_dma_transfer - do dma_map_sg() or using cookie
 + *
 + * @pre: if called in pre_req()
 + * return:
 + * 0 - do dma_map_sg()
 + * 1 - using cookie
 + */
 +static int sd_pre_dma_transfer(struct realtek_pci_sdmmc *host,
 +   struct mmc_data *data, bool pre)
 +{
 +   struct rtsx_pcr *pcr = host-pcr;
 +   int read = data-flags  MMC_DATA_READ;
 +   int count = 0;
 +   int using_cookie = 0;
 +
 +   if (!pre  data-host_cookie  data-host_cookie != host-cookie) {
 +   dev_err(sdmmc_dev(host),
 +   error: data-host_cookie = %d, host-cookie = %d\n,
 +   data-host_cookie, host-cookie);
 +   data-host_cookie = 0;
 +   }
 +
 +   if (pre || data-host_cookie != host-cookie) {
 +   count = rtsx_pci_dma_map_sg(pcr, data-sg, data-sg_len, 
 read);
 +   } else {
 +   count = host-cookie_sg_count;
 +   using_cookie = 1;
 +   }
 +
 +   if (pre) {
 +   host-cookie_sg_count = count;
 +   if (++host-cookie  0)
 +   host-cookie = 1;
 +   data-host_cookie = host-cookie;
 +   } else {
 +   host-sg_count = count;
 +   }
 +
 +   return using_cookie;
 +}
 +
 +static void sdmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 +   bool is_first_req)
 +{
 +   struct realtek_pci_sdmmc *host = mmc_priv(mmc);
 +   struct mmc_data *data = mrq-data;
 +
 +   if (data-host_cookie) {
 +   dev_err(sdmmc_dev(host),
 +   error: reset data-host_cookie = %d\n,
 +   data-host_cookie);
 +   data-host_cookie = 0;
 +   }
 +
 +   sd_pre_dma_transfer(host, data, true);
 +   dev_dbg(sdmmc_dev(host), pre dma sg: %d\n, host-cookie_sg_count);
 +}
 +
 +static void sdmmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 +   int err)
 +{
 +   struct realtek_pci_sdmmc *host = mmc_priv(mmc);
 +   struct rtsx_pcr *pcr = host-pcr;
 +   struct mmc_data *data = mrq-data;
 +   int read = data-flags  MMC_DATA_READ;
 +
 +   rtsx_pci_dma_unmap_sg(pcr, data-sg, data-sg_len, read);
 +   data-host_cookie = 0;
 +}
 +
  static int sd_read_data(struct realtek_pci_sdmmc *host, u8 *cmd, u16 
 byte_cnt,
 u8 *buf, int buf_len, int timeout)
  {
 @@ -415,7 +495,7 @@ static int sd_rw_multi(struct realtek_pci_sdmmc *host, 
 struct mmc_request *mrq)

 rtsx_pci_send_cmd_no_wait(pcr);

 -   err = rtsx_pci_transfer_data(pcr, data-sg, data-sg_len, read, 
 1);
 +   err = rtsx_pci_dma_transfer

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Ulf Hansson]

On 18 June 2014 03:17, micky micky_ch...@realsil.com.cn wrote:
 On 06/17/2014 03:45 PM, Ulf Hansson wrote:

 On 17 June 2014 03:04, micky micky_ch...@realsil.com.cn wrote:

 On 06/16/2014 08:40 PM, Ulf Hansson wrote:

 On 16 June 2014 11:09, micky micky_ch...@realsil.com.cn wrote:

 On 06/16/2014 04:42 PM, Ulf Hansson wrote:

 @@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {

   struct rtsx_pcr *pcr;
   struct mmc_host *mmc;
   struct mmc_request  *mrq;
 +   struct workqueue_struct *workq;
 +#define SDMMC_WORKQ_NAME   rtsx_pci_sdmmc_workq

 +   struct work_struct  work;

 I am trying to understand why you need a work/workqueue to implement
 this feature. Is that really the case?

 Could you elaborate on the reasons?

 Hi Uffe,

 we need return as fast as possible in mmc_host_ops
 request(ops-request)
 callback,
 so the mmc core can continue handle next request.
 when next request everything is ready, it will wait previous done(if
 not
 done),
 then call ops-request().

 we can't use atomic context, because we use mutex_lock() to protect

 ops-request should never executed in atomic context. Is that your
 concern?

 Yes.

 Okay. Unless I missed your point, I don't think you need the
 work/workqueue.

 any other method?


 Because, ops-request isn't ever executed in atomic context. That's
 due to the mmc core, which handles the async mechanism, are waiting
 for a completion variable in process context, before it invokes the
 ops-request() callback.

 That completion variable will be kicked, from your host driver, when
 you invoke mmc_request_done(), .

 Sorry, I don't understand here, how kicked?

mmc_request_done()
-mrq-done()
-mmc_wait_done()
-complete(mrq-completion);


 I think the flow is:
 - not wait for first req
 - init mrq-done
 - ops-request() --- A.rtsx: start queue
 work.
 - continue fetch next req
 - prepare next req ok,
 - wait previous done.--- B.(mmc_request_done() may be called
 at any time from A to B)
 - init mrq-done
 - ops-request() --- C.rtsx: start queue
 next work.
 ...
 and seems no problem.

Right, I don't think there are any _problem_ by using the workqueue as
you have implemented, but I am questioning if it's correct. Simply
because I don't think there are any reasons to why you need a
workqueue, it doesn't solve any problem for you - it just adds
overhead.

Kind regards
Ulf Hansson
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Ulf Hansson]

On 18 June 2014 12:08, micky micky_ch...@realsil.com.cn wrote:
 On 06/18/2014 03:39 PM, Ulf Hansson wrote:

 On 18 June 2014 03:17, micky micky_ch...@realsil.com.cn wrote:

 On 06/17/2014 03:45 PM, Ulf Hansson wrote:

 On 17 June 2014 03:04, micky micky_ch...@realsil.com.cn wrote:

 On 06/16/2014 08:40 PM, Ulf Hansson wrote:

 On 16 June 2014 11:09, micky micky_ch...@realsil.com.cn wrote:

 On 06/16/2014 04:42 PM, Ulf Hansson wrote:

 @@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {

struct rtsx_pcr *pcr;
struct mmc_host *mmc;
struct mmc_request  *mrq;
 +   struct workqueue_struct *workq;
 +#define SDMMC_WORKQ_NAME   rtsx_pci_sdmmc_workq

 +   struct work_struct  work;

 I am trying to understand why you need a work/workqueue to implement
 this feature. Is that really the case?

 Could you elaborate on the reasons?

 Hi Uffe,

 we need return as fast as possible in mmc_host_ops
 request(ops-request)
 callback,
 so the mmc core can continue handle next request.
 when next request everything is ready, it will wait previous done(if
 not
 done),
 then call ops-request().

 we can't use atomic context, because we use mutex_lock() to protect

 ops-request should never executed in atomic context. Is that your
 concern?

 Yes.

 Okay. Unless I missed your point, I don't think you need the
 work/workqueue.

 any other method?

 Because, ops-request isn't ever executed in atomic context. That's
 due to the mmc core, which handles the async mechanism, are waiting
 for a completion variable in process context, before it invokes the
 ops-request() callback.

 That completion variable will be kicked, from your host driver, when
 you invoke mmc_request_done(), .

 Sorry, I don't understand here, how kicked?

 mmc_request_done()
  -mrq-done()
  -mmc_wait_done()
  -complete(mrq-completion);

 I think the flow is:
 - not wait for first req
 - init mrq-done
 - ops-request() --- A.rtsx: start queue
 work.
 - continue fetch next req
 - prepare next req ok,
 - wait previous done.--- B.(mmc_request_done() may be
 called
 at any time from A to B)
 - init mrq-done
 - ops-request() --- C.rtsx: start queue
 next work.
 ...
 and seems no problem.

 Right, I don't think there are any _problem_ by using the workqueue as
 you have implemented, but I am questioning if it's correct. Simply
 because I don't think there are any reasons to why you need a
 workqueue, it doesn't solve any problem for you - it just adds
 overhead.

 Hi Uffe,

 we have two driver under mfd, the rtsx-mmc and rtsx-ms,
 we use mutex lock(pcr_mutex) to protect resource,
 when we handle mmc request, we need hold the mutex until we finish the
 request,
 so it will not interruptted by rtsx-ms request.

Ahh, I see. Now, _that_ explains why you want the workqueue. :-) Thanks!


 If we not use workq, once the request hold the mutex, we have to wait until
 the request finish,
 then release mutex, so the mmc core is blocking at here.
 To implement nonblocking request, we have to use workq.

One minor suggestion below, please consider this as an optimization
which goes outside the context of this patch.

There are cases when I think you should be able to skip the overhead
from scheduling the work from -request(). Those cases can be
described as when the mutex are available which can be tested by using
mutex_trylock().

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Ulf Hansson]

On 17 June 2014 03:04, micky micky_ch...@realsil.com.cn wrote:
 On 06/16/2014 08:40 PM, Ulf Hansson wrote:

 On 16 June 2014 11:09, micky micky_ch...@realsil.com.cn wrote:

 On 06/16/2014 04:42 PM, Ulf Hansson wrote:

 @@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {

  struct rtsx_pcr *pcr;
  struct mmc_host *mmc;
  struct mmc_request  *mrq;
 +   struct workqueue_struct *workq;
 +#define SDMMC_WORKQ_NAME   rtsx_pci_sdmmc_workq

 +   struct work_struct  work;

 I am trying to understand why you need a work/workqueue to implement
 this feature. Is that really the case?

 Could you elaborate on the reasons?

 Hi Uffe,

 we need return as fast as possible in mmc_host_ops request(ops-request)
 callback,
 so the mmc core can continue handle next request.
 when next request everything is ready, it will wait previous done(if not
 done),
 then call ops-request().

 we can't use atomic context, because we use mutex_lock() to protect

 ops-request should never executed in atomic context. Is that your
 concern?

 Yes.

Okay. Unless I missed your point, I don't think you need the work/workqueue.

Because, ops-request isn't ever executed in atomic context. That's
due to the mmc core, which handles the async mechanism, are waiting
for a completion variable in process context, before it invokes the
ops-request() callback.

That completion variable will be kicked, from your host driver, when
you invoke mmc_request_done(), .

Kind regards
Uffe



 resource, and we have to hold the lock during handle request.
 So I use workq, we just queue a work and return in ops-request(),
 The mmc core can continue without blocking at ops-request().

 Best Regards.
 micky.

 Kind regards
 Uffe
 .


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Ulf Hansson]

On 6 June 2014 09:05,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 Add support for non-blocking request, pre_req() runs dma_map_sg() and
 post_req() runs dma_unmap_sg(). This patch can increase card read/write
 speed, especially for high speed card and slow speed CPU.

 Test on intel i3(800MHz - 2.3GHz) performance mode(2.3GHz), SD card
 clock 208MHz

 run dd if=/dev/mmcblk0 of=/dev/null bs=64k count=1024
 before:
 67108864 bytes (67 MB) copied, 0.85427 s, 78.6 MB/s
 after:
 67108864 bytes (67 MB) copied, 0.74799 s, 89.7 MB/s

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |  133 
 +++--
  1 file changed, 127 insertions(+), 6 deletions(-)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index 1c68e0d..a2c0858 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -24,6 +24,7 @@
  #include linux/highmem.h
  #include linux/delay.h
  #include linux/platform_device.h
 +#include linux/workqueue.h
  #include linux/mmc/host.h
  #include linux/mmc/mmc.h
  #include linux/mmc/sd.h
 @@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {
 struct rtsx_pcr *pcr;
 struct mmc_host *mmc;
 struct mmc_request  *mrq;
 +   struct workqueue_struct *workq;
 +#define SDMMC_WORKQ_NAME   rtsx_pci_sdmmc_workq

 +   struct work_struct  work;

I am trying to understand why you need a work/workqueue to implement
this feature. Is that really the case?

Could you elaborate on the reasons?

Kind regards
Uffe

 struct mutexhost_mutex;

 u8  ssc_depth;
 @@ -48,6 +52,11 @@ struct realtek_pci_sdmmc {
 int power_state;
  #define SDMMC_POWER_ON 1
  #define SDMMC_POWER_OFF0
 +
 +   unsigned intsg_count;
 +   s32 cookie;
 +   unsigned intcookie_sg_count;
 +   boolusing_cookie;
  };

  static inline struct device *sdmmc_dev(struct realtek_pci_sdmmc *host)
 @@ -86,6 +95,77 @@ static void sd_print_debug_regs(struct realtek_pci_sdmmc 
 *host)
  #define sd_print_debug_regs(host)
  #endif /* DEBUG */

 +/*
 + * sd_pre_dma_transfer - do dma_map_sg() or using cookie
 + *
 + * @pre: if called in pre_req()
 + * return:
 + * 0 - do dma_map_sg()
 + * 1 - using cookie
 + */
 +static int sd_pre_dma_transfer(struct realtek_pci_sdmmc *host,
 +   struct mmc_data *data, bool pre)
 +{
 +   struct rtsx_pcr *pcr = host-pcr;
 +   int read = data-flags  MMC_DATA_READ;
 +   int count = 0;
 +   int using_cookie = 0;
 +
 +   if (!pre  data-host_cookie  data-host_cookie != host-cookie) {
 +   dev_err(sdmmc_dev(host),
 +   error: data-host_cookie = %d, host-cookie = %d\n,
 +   data-host_cookie, host-cookie);
 +   data-host_cookie = 0;
 +   }
 +
 +   if (pre || data-host_cookie != host-cookie) {
 +   count = rtsx_pci_dma_map_sg(pcr, data-sg, data-sg_len, 
 read);
 +   } else {
 +   count = host-cookie_sg_count;
 +   using_cookie = 1;
 +   }
 +
 +   if (pre) {
 +   host-cookie_sg_count = count;
 +   if (++host-cookie  0)
 +   host-cookie = 1;
 +   data-host_cookie = host-cookie;
 +   } else {
 +   host-sg_count = count;
 +   }
 +
 +   return using_cookie;
 +}
 +
 +static void sdmmc_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 +   bool is_first_req)
 +{
 +   struct realtek_pci_sdmmc *host = mmc_priv(mmc);
 +   struct mmc_data *data = mrq-data;
 +
 +   if (data-host_cookie) {
 +   dev_err(sdmmc_dev(host),
 +   error: reset data-host_cookie = %d\n,
 +   data-host_cookie);
 +   data-host_cookie = 0;
 +   }
 +
 +   sd_pre_dma_transfer(host, data, true);
 +   dev_dbg(sdmmc_dev(host), pre dma sg: %d\n, host-cookie_sg_count);
 +}
 +
 +static void sdmmc_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 +   int err)
 +{
 +   struct realtek_pci_sdmmc *host = mmc_priv(mmc);
 +   struct rtsx_pcr *pcr = host-pcr;
 +   struct mmc_data *data = mrq-data;
 +   int read = data-flags  MMC_DATA_READ;
 +
 +   rtsx_pci_dma_unmap_sg(pcr, data-sg, data-sg_len, read);
 +   data-host_cookie = 0;
 +}
 +
  static int sd_read_data(struct realtek_pci_sdmmc *host, u8 *cmd, u16 
 byte_cnt,
 u8 *buf, int buf_len, int timeout)
  {
 @@ -415,7 +495,7 @@ static int sd_rw_multi(struct realtek_pci_sdmmc *host, 
 struct mmc_request *mrq)

 rtsx_pci_send_cmd_no_wait(pcr);

 -   err = rtsx_pci_transfer_data(pcr, data-sg, data-sg_len, read, 
 1);
 +   err = 

Re: [PATCH 2/2] mmc: rtsx: add support for async request

[Ulf Hansson]

On 16 June 2014 11:09, micky micky_ch...@realsil.com.cn wrote:
 On 06/16/2014 04:42 PM, Ulf Hansson wrote:

 @@ -36,7 +37,10 @@ struct realtek_pci_sdmmc {
  struct rtsx_pcr *pcr;
  struct mmc_host *mmc;
  struct mmc_request  *mrq;
 +   struct workqueue_struct *workq;
 +#define SDMMC_WORKQ_NAME   rtsx_pci_sdmmc_workq
 
 +   struct work_struct  work;

 I am trying to understand why you need a work/workqueue to implement
 this feature. Is that really the case?

 Could you elaborate on the reasons?

 Hi Uffe,

 we need return as fast as possible in mmc_host_ops request(ops-request)
 callback,
 so the mmc core can continue handle next request.
 when next request everything is ready, it will wait previous done(if not
 done),
 then call ops-request().

 we can't use atomic context, because we use mutex_lock() to protect

ops-request should never executed in atomic context. Is that your concern?

 resource, and we have to hold the lock during handle request.
 So I use workq, we just queue a work and return in ops-request(),
 The mmc core can continue without blocking at ops-request().

 Best Regards.
 micky.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] mfd: rtsx: add dma transfer function

[Ulf Hansson]

On 16 June 2014 14:20, Lee Jones lee.jo...@linaro.org wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 rtsx driver using a single function for transfer data, dma map/unmap are
 placed in one fix function. We need map/unmap dma in different place(for
 mmc async driver), so add three function for dma map, dma transfer and
 dma unmap.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
 ---
  drivers/mfd/rtsx_pcr.c   |   76 
 ++
  include/linux/mfd/rtsx_pci.h |6 
  2 files changed, 54 insertions(+), 28 deletions(-)

 I don't see any glaring issues with this patch.  Does it rely on the
 first patch, or vise versa, or can it just be applied?

The mmc part in patch2 relies on this one, but please go ahead and
apply the mfd patch if you see it good.

I can later provide my ack for the mmc parts, in patch2, when it's a
reviewed properly and thus you can take it through your tree.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] mmc: rtsx: Revert mmc: rtsx: modify error handleandremove smatch warnings

[Ulf Hansson]

On 8 May 2014 09:55, Lee Jones lee.jo...@linaro.org wrote:
 It seems Chris is too busy to responding, so would you help to pick
 this patch for 3.15 fix.

 H... that's pretty bad timing.  I've literally just sent the
 pull-request containing the MFD/MMC fixup branch to Linus.  I guess
 Ulf will have to do it.

There seem to be some confusion here. :-)

This patch is _only_ intended for Chris' mmc_next branch, so it
shouldn't go for 3.15 fixes. Or better, Chris should drop the patch
this one is reverting.

Likely we will see a merge problem in Stephen Rothwell's linux-next
tree, until Chris' drops this patch - causing Stephen to not merge the
mmc tree for a while. I suppose that's the best we can do, until Chris
shows up again.

Kind regards
Ulf Hansson


 From: Micky Chingmicky_ch...@realsil.com.cn
 
 This reverts commit 1f7b581b3ffcb2a8437397a02f4af89fa6934d08.
 
 The patch depend on commit c42deffd5b53c9e583d83c7964854ede2f12410d
 mmc: rtsx: add support for pre_req and post_req, but the previous
 patch was discard. So we have to delete the patch.
 
 Signed-off-by: Micky Chingmicky_ch...@realsil.com.cn
 Acked-by: Ulf Hanssonulf.hans...@linaro.org


 --
 Lee Jones
 Linaro STMicroelectronics Landing Team Lead
 Linaro.org │ Open source software for ARM SoCs
 Follow Linaro: Facebook | Twitter | Blog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mmc: rtsx: usb backend needs LED support

[Ulf Hansson]

 From: Roger Tseng rogera...@realtek.com
 Date: Wed, 30 Apr 2014 11:11:25 +0800
 Subject: [PATCH] mmc: rtsx: fix possible linking error if built-in

 rtsx_usb_sdmmc module uses the LED classdev if available, but the code
 failed
 to consider the situation that it is built-in and the LED classdev is a
 module,
 leading to following linking error:

LD  init/built-in.o
 drivers/built-in.o: In function `rtsx_usb_sdmmc_drv_remove':
 rtsx_usb_sdmmc.c:(.text+0x2a018e): undefined reference to
 `led_classdev_unregister'
 drivers/built-in.o: In function `rtsx_usb_sdmmc_drv_probe':
 rtsx_usb_sdmmc.c:(.text+0x2a197e): undefined reference to
 `led_classdev_register'

 Fix by excluding such condition when defining macro
 RTSX_USB_USE_LEDS_CLASS.

 Signed-off-by: Roger Tseng rogera...@realtek.com


 Acked-by: Ulf Hansson ulf.hans...@linaro.org


 Would this patch be merged into linux-next or Lee's mfd.git
 ib-mfd-mmc-memstick-3.16 branch?

This is for Chris' tree.

Kind regards
Ulf Hansson


 By the way, should I resend my version?


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] mmc: rtsx: fix possible linking error if built-in

[Ulf Hansson]

On 8 May 2014 10:27, Arnd Bergmann a...@arndb.de wrote:
 rtsx_usb_sdmmc module uses the LED classdev if available, but the code
 failed to consider the situation that it is built-in and the LED classdev is a
 module, leading to following linking error:

LD  init/built-in.o
 drivers/built-in.o: In function `rtsx_usb_sdmmc_drv_remove':
 rtsx_usb_sdmmc.c:(.text+0x2a018e): undefined reference to
 `led_classdev_unregister'
 drivers/built-in.o: In function `rtsx_usb_sdmmc_drv_probe':
 rtsx_usb_sdmmc.c:(.text+0x2a197e): undefined reference to
 `led_classdev_register'

 Fix by excluding such condition when defining macro RTSX_USB_USE_LEDS_CLASS.

 Signed-off-by: Roger Tseng rogera...@realtek.com
 Signed-off-by: Arnd Bergmann a...@arndb.de
 Acked-by: Ulf Hansson ulf.hans...@linaro.org
 ---
 v2 by Arnd: change the second #ifdef as well.

Thanks Arnd, Roger - I will include this one in PR that I send to
Chris in the next few days.

Kind regards
Ulf Hansson


 diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c 
 b/drivers/mmc/host/rtsx_usb_sdmmc.c
 index e11fafa..5d3766e 100644
 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
 @@ -34,7 +34,8 @@
  #include linux/mfd/rtsx_usb.h
  #include asm/unaligned.h

 -#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 +#if defined(CONFIG_LEDS_CLASS) || (defined(CONFIG_LEDS_CLASS_MODULE)  \
 +   defined(CONFIG_MMC_REALTEK_USB_MODULE))
  #include linux/leds.h
  #include linux/workqueue.h
  #define RTSX_USB_USE_LEDS_CLASS
 @@ -59,7 +60,7 @@ struct rtsx_usb_sdmmc {

 unsigned char   power_mode;

 -#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 +#ifdef RTSX_USB_USE_LEDS_CLASS
 struct led_classdev led;
 charled_name[32];
 struct work_struct  led_work;

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] mmc: rtsx: Revert mmc: rtsx: add support for pre_req and post_req

[Ulf Hansson]

On 29 April 2014 09:36, Ulf Hansson ulf.hans...@linaro.org wrote:
 On 29 April 2014 03:54,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 This reverts commit c42deffd5b53c9e583d83c7964854ede2f12410d.

 commit mmc: rtsx: add support for pre_req and post_req did use
 mutex_unlock() in tasklet, but mutex_unlock() can't used in
 tasklet(atomic context). The driver need use mutex to avoid concurrency,
 so we can't use tasklet here, the patch need to be removed.

 The spinlock host-lock and pcr-lock may deadlock, one way to solve the
 deadlock is remove host-lock in sd_isr_done_transfer(), but if using
 workqueue the we can avoid using the spinlock and also avoid the problem.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn

 Acked-by: Ulf Hansson ulf.hans...@linaro.org

Hi Lee,

Would you mind to pick this up and send it for 3.15 fixes?

The other revert [PATCH 1/2] mmc: rtsx: Revert mmc: rtsx: modify
error handle and remove smatch warnings, needs to go through the mmc
tree.

Kind regards
Uffe
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] mmc: rtsx: Revert mmc: rtsx: modify error handle and remove smatch warnings

[Ulf Hansson]

On 29 April 2014 03:54,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 This reverts commit 1f7b581b3ffcb2a8437397a02f4af89fa6934d08.

 The patch depend on commit c42deffd5b53c9e583d83c7964854ede2f12410d
 mmc: rtsx: add support for pre_req and post_req, but the previous
 patch was discard. So we have to delete the patch.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn

Acked-by: Ulf Hansson ulf.hans...@linaro.org

The patch this is reverting has been recently queued for 3.16. So we
may either apply the revert or just drop the patch from the mmc-next
branch.

Kind regards
Ulf Hansson

 ---
  drivers/mmc/host/rtsx_pci_sdmmc.c |  119 
 +
  1 file changed, 54 insertions(+), 65 deletions(-)

 diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c 
 b/drivers/mmc/host/rtsx_pci_sdmmc.c
 index 09340b9..76cfdcc 100644
 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
 +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
 @@ -81,24 +81,25 @@ static inline void sd_clear_error(struct 
 realtek_pci_sdmmc *host)
  }

  #ifdef DEBUG
 -static inline void sd_print_reg(struct realtek_pci_sdmmc *host, u16 reg)
 -{
 -   u8 val = 0;
 -
 -   if (rtsx_pci_read_register(host-pcr, reg, val)  0)
 -   dev_dbg(sdmmc_dev(host), read 0x%04x failed\n, reg);
 -   else
 -   dev_dbg(sdmmc_dev(host), 0x%04X: 0x%02x\n, reg, val);
 -}
 -
  static void sd_print_debug_regs(struct realtek_pci_sdmmc *host)
  {
 +   struct rtsx_pcr *pcr = host-pcr;
 u16 i;
 +   u8 *ptr;
 +
 +   /* Print SD host internal registers */
 +   rtsx_pci_init_cmd(pcr);
 +   for (i = 0xFDA0; i = 0xFDAE; i++)
 +   rtsx_pci_add_cmd(pcr, READ_REG_CMD, i, 0, 0);
 +   for (i = 0xFD52; i = 0xFD69; i++)
 +   rtsx_pci_add_cmd(pcr, READ_REG_CMD, i, 0, 0);
 +   rtsx_pci_send_cmd(pcr, 100);

 +   ptr = rtsx_pci_get_cmd_data(pcr);
 for (i = 0xFDA0; i = 0xFDAE; i++)
 -   sd_print_reg(host, i);
 +   dev_dbg(sdmmc_dev(host), 0x%04X: 0x%02x\n, i, *(ptr++));
 for (i = 0xFD52; i = 0xFD69; i++)
 -   sd_print_reg(host, i);
 +   dev_dbg(sdmmc_dev(host), 0x%04X: 0x%02x\n, i, *(ptr++));
  }
  #else
  #define sd_print_debug_regs(host)
 @@ -124,27 +125,19 @@ static void sd_request_timeout(unsigned long host_addr)
 spin_lock_irqsave(host-lock, flags);

 if (!host-mrq) {
 -   dev_err(sdmmc_dev(host), error: request not exist\n);
 -   spin_unlock_irqrestore(host-lock, flags);
 -   return;
 +   dev_err(sdmmc_dev(host), error: no request exist\n);
 +   goto out;
 }

 -   if (host-cmd  host-data)
 -   dev_err(sdmmc_dev(host), error: cmd and data conflict\n);
 -
 -   if (host-cmd) {
 +   if (host-cmd)
 host-cmd-error = -ETIMEDOUT;
 -   dev_dbg(sdmmc_dev(host), timeout for cmd %d\n,
 -   host-cmd-opcode);
 -   tasklet_schedule(host-cmd_tasklet);
 -   }
 -
 -   if (host-data) {
 +   if (host-data)
 host-data-error = -ETIMEDOUT;
 -   dev_dbg(sdmmc_dev(host), timeout for data transfer\n);
 -   tasklet_schedule(host-data_tasklet);
 -   }

 +   dev_dbg(sdmmc_dev(host), timeout for request\n);
 +
 +out:
 +   tasklet_schedule(host-finish_tasklet);
 spin_unlock_irqrestore(host-lock, flags);
  }

 @@ -164,8 +157,7 @@ static void sd_finish_request(unsigned long host_addr)
 mrq = host-mrq;
 if (!mrq) {
 dev_err(sdmmc_dev(host), error: no request need finish\n);
 -   spin_unlock_irqrestore(host-lock, flags);
 -   return;
 +   goto out;
 }

 cmd = mrq-cmd;
 @@ -175,6 +167,11 @@ static void sd_finish_request(unsigned long host_addr)
 (mrq-stop  mrq-stop-error) ||
 (cmd  cmd-error) || (data  data-error);

 +   if (any_error) {
 +   rtsx_pci_stop_cmd(pcr);
 +   sd_clear_error(host);
 +   }
 +
 if (data) {
 if (any_error)
 data-bytes_xfered = 0;
 @@ -191,6 +188,7 @@ static void sd_finish_request(unsigned long host_addr)
 host-cmd = NULL;
 host-data = NULL;

 +out:
 spin_unlock_irqrestore(host-lock, flags);
 mutex_unlock(pcr-pcr_mutex);
 mmc_request_done(host-mmc, mrq);
 @@ -375,11 +373,8 @@ static void sd_send_cmd(struct realtek_pci_sdmmc *host, 
 struct mmc_command *cmd)
 if (cmd-opcode == SD_SWITCH_VOLTAGE) {
 err = rtsx_pci_write_register(pcr, SD_BUS_STAT,
 0xFF, SD_CLK_TOGGLE_EN);
 -   if (err  0) {
 -   rtsx_pci_write_register(pcr, SD_BUS_STAT,
 -   SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0);
 +   if (err  0

Re: [PATCH 2/2] mmc: rtsx: Revert mmc: rtsx: add support for pre_req and post_req

[Ulf Hansson]

On 29 April 2014 03:54,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 This reverts commit c42deffd5b53c9e583d83c7964854ede2f12410d.

 commit mmc: rtsx: add support for pre_req and post_req did use
 mutex_unlock() in tasklet, but mutex_unlock() can't used in
 tasklet(atomic context). The driver need use mutex to avoid concurrency,
 so we can't use tasklet here, the patch need to be removed.

 The spinlock host-lock and pcr-lock may deadlock, one way to solve the
 deadlock is remove host-lock in sd_isr_done_transfer(), but if using
 workqueue the we can avoid using the spinlock and also avoid the problem.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn

Acked-by: Ulf Hansson ulf.hans...@linaro.org

 ---
  drivers/mfd/rtsx_pcr.c|  132 
  drivers/mmc/host/rtsx_pci_sdmmc.c |  418 
 ++---
  include/linux/mfd/rtsx_common.h   |1 -
  include/linux/mfd/rtsx_pci.h  |6 -
  4 files changed, 109 insertions(+), 448 deletions(-)

 diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
 index c9de3d5..1d15735 100644
 --- a/drivers/mfd/rtsx_pcr.c
 +++ b/drivers/mfd/rtsx_pcr.c
 @@ -338,28 +338,58 @@ int rtsx_pci_transfer_data(struct rtsx_pcr *pcr, struct 
 scatterlist *sglist,
 int num_sg, bool read, int timeout)
  {
 struct completion trans_done;
 -   int err = 0, count;
 +   u8 dir;
 +   int err = 0, i, count;
 long timeleft;
 unsigned long flags;
 +   struct scatterlist *sg;
 +   enum dma_data_direction dma_dir;
 +   u32 val;
 +   dma_addr_t addr;
 +   unsigned int len;
 +
 +   dev_dbg((pcr-pci-dev), -- %s: num_sg = %d\n, __func__, num_sg);
 +
 +   /* don't transfer data during abort processing */
 +   if (pcr-remove_pci)
 +   return -EINVAL;
 +
 +   if ((sglist == NULL) || (num_sg = 0))
 +   return -EINVAL;

 -   count = rtsx_pci_dma_map_sg(pcr, sglist, num_sg, read);
 +   if (read) {
 +   dir = DEVICE_TO_HOST;
 +   dma_dir = DMA_FROM_DEVICE;
 +   } else {
 +   dir = HOST_TO_DEVICE;
 +   dma_dir = DMA_TO_DEVICE;
 +   }
 +
 +   count = dma_map_sg((pcr-pci-dev), sglist, num_sg, dma_dir);
 if (count  1) {
 dev_err((pcr-pci-dev), scatterlist map failed\n);
 return -EINVAL;
 }
 dev_dbg((pcr-pci-dev), DMA mapping count: %d\n, count);

 +   val = ((u32)(dir  0x01)  29) | TRIG_DMA | ADMA_MODE;
 +   pcr-sgi = 0;
 +   for_each_sg(sglist, sg, count, i) {
 +   addr = sg_dma_address(sg);
 +   len = sg_dma_len(sg);
 +   rtsx_pci_add_sg_tbl(pcr, addr, len, i == count - 1);
 +   }

 spin_lock_irqsave(pcr-lock, flags);

 pcr-done = trans_done;
 pcr-trans_result = TRANS_NOT_READY;
 init_completion(trans_done);
 +   rtsx_pci_writel(pcr, RTSX_HDBAR, pcr-host_sg_tbl_addr);
 +   rtsx_pci_writel(pcr, RTSX_HDBCTLR, val);

 spin_unlock_irqrestore(pcr-lock, flags);

 -   rtsx_pci_dma_transfer(pcr, sglist, count, read);
 -
 timeleft = wait_for_completion_interruptible_timeout(
 trans_done, msecs_to_jiffies(timeout));
 if (timeleft = 0) {
 @@ -383,7 +413,7 @@ out:
 pcr-done = NULL;
 spin_unlock_irqrestore(pcr-lock, flags);

 -   rtsx_pci_dma_unmap_sg(pcr, sglist, num_sg, read);
 +   dma_unmap_sg((pcr-pci-dev), sglist, num_sg, dma_dir);

 if ((err  0)  (err != -ENODEV))
 rtsx_pci_stop_cmd(pcr);
 @@ -395,73 +425,6 @@ out:
  }
  EXPORT_SYMBOL_GPL(rtsx_pci_transfer_data);

 -int rtsx_pci_dma_map_sg(struct rtsx_pcr *pcr, struct scatterlist *sglist,
 -   int num_sg, bool read)
 -{
 -   enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 -
 -   if (pcr-remove_pci)
 -   return -EINVAL;
 -
 -   if ((sglist == NULL) || num_sg  1)
 -   return -EINVAL;
 -
 -   return dma_map_sg((pcr-pci-dev), sglist, num_sg, dir);
 -}
 -EXPORT_SYMBOL_GPL(rtsx_pci_dma_map_sg);
 -
 -int rtsx_pci_dma_unmap_sg(struct rtsx_pcr *pcr, struct scatterlist *sglist,
 -   int num_sg, bool read)
 -{
 -   enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 -
 -   if (pcr-remove_pci)
 -   return -EINVAL;
 -
 -   if (sglist == NULL || num_sg  1)
 -   return -EINVAL;
 -
 -   dma_unmap_sg((pcr-pci-dev), sglist, num_sg, dir);
 -   return num_sg;
 -}
 -EXPORT_SYMBOL_GPL(rtsx_pci_dma_unmap_sg);
 -
 -int rtsx_pci_dma_transfer(struct rtsx_pcr *pcr, struct scatterlist *sglist,
 -   int sg_count, bool read)
 -{
 -   struct scatterlist *sg;
 -   dma_addr_t addr;
 -   unsigned int len;
 -   int i;
 -   u32 val;
 -   u8 dir = read ? DEVICE_TO_HOST : HOST_TO_DEVICE;
 -   unsigned long

Re: [PATCH] mmc: rtsx: usb backend needs LED support

[Ulf Hansson]

On 29 April 2014 11:45, Arnd Bergmann a...@arndb.de wrote:
 [hijacking the thread since it has the right Cc list already, sorry]

 I stumbled over this doing randconfig builds on linux-next

 8--

 From c11f54f1e5ea0557e076867ca31c90bcb20e3e0c Mon Sep 17 00:00:00 2001
 From: Arnd Bergmann a...@arndb.de
 Date: Tue, 29 Apr 2014 11:41:40 +0200
 Subject: [PATCH] mmc: rtsx: usb backend needs LED support

 Building the rtsx USB driver requires the LED classdev base
 support, otherwise we get this build error:

 drivers/built-in.o: In function `rtsx_usb_sdmmc_drv_remove':
 :(.text+0x806480): undefined reference to `led_classdev_unregister'
 drivers/built-in.o: In function `rtsx_usb_sdmmc_drv_probe':
 :(.text+0x806708): undefined reference to `led_classdev_register'

 This adds an explicit dependency in Kconfig

I think the proper solution is to fix the dependency in the driver code instead.

There are already some ifdefs hackery for making it optional to use
leds, apparently that's not working properly.

Kind regards
Ulf Hansson


 Signed-off-by: Arnd Bergmann a...@arndb.de

 diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
 index 92d91fe..66aedf3 100644
 --- a/drivers/mmc/host/Kconfig
 +++ b/drivers/mmc/host/Kconfig
 @@ -696,6 +696,7 @@ config MMC_REALTEK_PCI
  config MMC_REALTEK_USB
 tristate Realtek USB SD/MMC Card Interface Driver
 depends on MFD_RTSX_USB
 +   depends on LEDS_CLASS
 help
   Say Y here to include driver code to support SD/MMC card interface
   of Realtek RTS5129/39 series card reader

 --
 To unsubscribe from this list: send the line unsubscribe linux-mmc in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] mmc: rtsx: Revert mmc: rtsx: add support for pre_req and post_req

[Ulf Hansson]

On 28 April 2014 09:36,  micky_ch...@realsil.com.cn wrote:
 From: Micky Ching micky_ch...@realsil.com.cn

 This reverts commit c42deffd5b53c9e583d83c7964854ede2f12410d.

 commit mmc: rtsx: add support for pre_req and post_req did use
 mutex_unlock() in tasklet, but mutex_unlock() can't used in
 tasklet(atomic context). The driver need use mutex to avoid concurrency,
 so we can't use tasklet here, the patch need to be removed.

 The spinlock host-lock and pcr-lock may deadlock, one way to solve the
 deadlock is remove host-lock in sd_isr_done_transfer(), but if using
 workqueue the we can avoid using the spinlock and also avoid the problem.

 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn

Acked-by: Ulf Hansson ulf.hans...@linaro.org

Chris, could you pick up this for fixes instead of queuing it for 3.16?

Kind regards
Uffe

 ---
  drivers/mfd/rtsx_pcr.c|  132 
  drivers/mmc/host/rtsx_pci_sdmmc.c |  418 
 ++---
  include/linux/mfd/rtsx_common.h   |1 -
  include/linux/mfd/rtsx_pci.h  |6 -
  4 files changed, 109 insertions(+), 448 deletions(-)

 diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
 index c9de3d5..1d15735 100644
 --- a/drivers/mfd/rtsx_pcr.c
 +++ b/drivers/mfd/rtsx_pcr.c
 @@ -338,28 +338,58 @@ int rtsx_pci_transfer_data(struct rtsx_pcr *pcr, struct 
 scatterlist *sglist,
 int num_sg, bool read, int timeout)
  {
 struct completion trans_done;
 -   int err = 0, count;
 +   u8 dir;
 +   int err = 0, i, count;
 long timeleft;
 unsigned long flags;
 +   struct scatterlist *sg;
 +   enum dma_data_direction dma_dir;
 +   u32 val;
 +   dma_addr_t addr;
 +   unsigned int len;
 +
 +   dev_dbg((pcr-pci-dev), -- %s: num_sg = %d\n, __func__, num_sg);
 +
 +   /* don't transfer data during abort processing */
 +   if (pcr-remove_pci)
 +   return -EINVAL;
 +
 +   if ((sglist == NULL) || (num_sg = 0))
 +   return -EINVAL;

 -   count = rtsx_pci_dma_map_sg(pcr, sglist, num_sg, read);
 +   if (read) {
 +   dir = DEVICE_TO_HOST;
 +   dma_dir = DMA_FROM_DEVICE;
 +   } else {
 +   dir = HOST_TO_DEVICE;
 +   dma_dir = DMA_TO_DEVICE;
 +   }
 +
 +   count = dma_map_sg((pcr-pci-dev), sglist, num_sg, dma_dir);
 if (count  1) {
 dev_err((pcr-pci-dev), scatterlist map failed\n);
 return -EINVAL;
 }
 dev_dbg((pcr-pci-dev), DMA mapping count: %d\n, count);

 +   val = ((u32)(dir  0x01)  29) | TRIG_DMA | ADMA_MODE;
 +   pcr-sgi = 0;
 +   for_each_sg(sglist, sg, count, i) {
 +   addr = sg_dma_address(sg);
 +   len = sg_dma_len(sg);
 +   rtsx_pci_add_sg_tbl(pcr, addr, len, i == count - 1);
 +   }

 spin_lock_irqsave(pcr-lock, flags);

 pcr-done = trans_done;
 pcr-trans_result = TRANS_NOT_READY;
 init_completion(trans_done);
 +   rtsx_pci_writel(pcr, RTSX_HDBAR, pcr-host_sg_tbl_addr);
 +   rtsx_pci_writel(pcr, RTSX_HDBCTLR, val);

 spin_unlock_irqrestore(pcr-lock, flags);

 -   rtsx_pci_dma_transfer(pcr, sglist, count, read);
 -
 timeleft = wait_for_completion_interruptible_timeout(
 trans_done, msecs_to_jiffies(timeout));
 if (timeleft = 0) {
 @@ -383,7 +413,7 @@ out:
 pcr-done = NULL;
 spin_unlock_irqrestore(pcr-lock, flags);

 -   rtsx_pci_dma_unmap_sg(pcr, sglist, num_sg, read);
 +   dma_unmap_sg((pcr-pci-dev), sglist, num_sg, dma_dir);

 if ((err  0)  (err != -ENODEV))
 rtsx_pci_stop_cmd(pcr);
 @@ -395,73 +425,6 @@ out:
  }
  EXPORT_SYMBOL_GPL(rtsx_pci_transfer_data);

 -int rtsx_pci_dma_map_sg(struct rtsx_pcr *pcr, struct scatterlist *sglist,
 -   int num_sg, bool read)
 -{
 -   enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 -
 -   if (pcr-remove_pci)
 -   return -EINVAL;
 -
 -   if ((sglist == NULL) || num_sg  1)
 -   return -EINVAL;
 -
 -   return dma_map_sg((pcr-pci-dev), sglist, num_sg, dir);
 -}
 -EXPORT_SYMBOL_GPL(rtsx_pci_dma_map_sg);
 -
 -int rtsx_pci_dma_unmap_sg(struct rtsx_pcr *pcr, struct scatterlist *sglist,
 -   int num_sg, bool read)
 -{
 -   enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 -
 -   if (pcr-remove_pci)
 -   return -EINVAL;
 -
 -   if (sglist == NULL || num_sg  1)
 -   return -EINVAL;
 -
 -   dma_unmap_sg((pcr-pci-dev), sglist, num_sg, dir);
 -   return num_sg;
 -}
 -EXPORT_SYMBOL_GPL(rtsx_pci_dma_unmap_sg);
 -
 -int rtsx_pci_dma_transfer(struct rtsx_pcr *pcr, struct scatterlist *sglist,
 -   int sg_count, bool read)
 -{
 -   struct scatterlist *sg;
 -   dma_addr_t addr;
 -   unsigned int len;
 -   int i

Re: [PATCH] mmc: rtsx: Revert mmc: rtsx: add support for pre_req and post_req

[Ulf Hansson]

On 28 April 2014 14:29, Lee Jones lee.jo...@linaro.org wrote:
  From: Micky Ching micky_ch...@realsil.com.cn
 
  This reverts commit c42deffd5b53c9e583d83c7964854ede2f12410d.
 
  Why was this patch even merged without an MFD Ack?
 
  commit mmc: rtsx: add support for pre_req and post_req did use
  mutex_unlock() in tasklet, but mutex_unlock() can't used in
  tasklet(atomic context). The driver need use mutex to avoid concurrency,
  so we can't use tasklet here, the patch need to be removed.
 
  The spinlock host-lock and pcr-lock may deadlock, one way to solve the
  deadlock is remove host-lock in sd_isr_done_transfer(), but if using
  workqueue the we can avoid using the spinlock and also avoid the problem.
 
  Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
  ---
   drivers/mfd/rtsx_pcr.c|  132 
   drivers/mmc/host/rtsx_pci_sdmmc.c |  418 
  ++---
   include/linux/mfd/rtsx_common.h   |1 -
   include/linux/mfd/rtsx_pci.h  |6 -
   4 files changed, 109 insertions(+), 448 deletions(-)
 
  This patch should be sent to the -rcs as soon as possible:

 Do you mind helping out doing it here Lee? Not sure how busy Chris is
 at the moment.

 I don't mind. I just need a maintainer Ack from one of you guys.

Thanks for helping out Lee. Please hold on until we sorted out exactly
what should be reverted, it seems like some additional patch should be
removed/reverted as well.

Kind regards
Uffe


 
  Acked-by: Lee Jones lee.jo...@linaro.org

 --
 Lee Jones
 Linaro STMicroelectronics Landing Team Lead
 Linaro.org │ Open source software for ARM SoCs
 Follow Linaro: Facebook | Twitter | Blog
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v7 0/4] Add modules for realtek USB card reader

[Ulf Hansson]

On 16 April 2014 05:19, Roger rogera...@realtek.com wrote:
 On 04/11/2014 06:36 PM, Oliver Neukum wrote:

 On Fri, 2014-04-11 at 11:28 +0100, Lee Jones wrote:

 From: Roger Tseng rogera...@realtek.com

 This patchset adds modules to support Realtek USB vendor specific class
 flash
 card reader: one base module in MFD subsystem and two host modules in
 both mmc
 and memstick subsystems. The architecture is similar to rtsx_pci.

 This work is done primarily to replace the staging driver:
 staging/rts5139,
 which doesn't utilize mmc nor memstick subsystems. Once the patchset or
 its
 revision is applied, we may need Greg's help to remove the staging one.


 Looks good to me.


 Is that an Ack?

 Acked-by: Oliver oneu...@suse.de

 Sorry
 Oliver

 Lee,

 Would you apply all of these patches?

I have sent a pull request to Chris, containing the patch with the new
mmc driver. The rest I am fine with if Lee can take though the mfd
tree.

Kind regards
Ulf Hansson



 Best regards,
 Roger Tseng
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 3/4] mmc: Add realtek USB sdmmc host driver

[Ulf Hansson]

On 9 April 2014 08:16,  rogera...@realtek.com wrote:
 From: Roger Tseng rogera...@realtek.com

 Realtek USB SD/MMC host driver provides mmc host support based on the Realtek
 USB card reader MFD driver.

 Signed-off-by: Roger Tseng rogera...@realtek.com
 ---
  drivers/mmc/host/Kconfig  |7 +
  drivers/mmc/host/Makefile |1 +
  drivers/mmc/host/rtsx_usb_sdmmc.c | 1462 
 +
  3 files changed, 1470 insertions(+)
  create mode 100644 drivers/mmc/host/rtsx_usb_sdmmc.c

 diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
 index 1384f67..1c01df4 100644
 --- a/drivers/mmc/host/Kconfig
 +++ b/drivers/mmc/host/Kconfig
 @@ -689,3 +689,10 @@ config MMC_REALTEK_PCI
 help
   Say Y here to include driver code to support SD/MMC card interface
   of Realtek PCI-E card reader
 +
 +config MMC_REALTEK_USB
 +   tristate Realtek USB SD/MMC Card Interface Driver
 +   depends on MFD_RTSX_USB
 +   help
 + Say Y here to include driver code to support SD/MMC card interface
 + of Realtek RTS5129/39 series card reader
 diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
 index 3483b6b..8194317 100644
 --- a/drivers/mmc/host/Makefile
 +++ b/drivers/mmc/host/Makefile
 @@ -53,6 +53,7 @@ obj-$(CONFIG_MMC_USHC)+= ushc.o
  obj-$(CONFIG_MMC_WMT)  += wmt-sdmmc.o

  obj-$(CONFIG_MMC_REALTEK_PCI)  += rtsx_pci_sdmmc.o
 +obj-$(CONFIG_MMC_REALTEK_USB)  += rtsx_usb_sdmmc.o

  obj-$(CONFIG_MMC_SDHCI_PLTFM)  += sdhci-pltfm.o
  obj-$(CONFIG_MMC_SDHCI_CNS3XXX)+= sdhci-cns3xxx.o
 diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c 
 b/drivers/mmc/host/rtsx_usb_sdmmc.c
 new file mode 100644
 index 000..37f58b5
 --- /dev/null
 +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
 @@ -0,0 +1,1462 @@
 +/* Realtek USB SD/MMC Card Interface driver
 + *
 + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU General Public License version 2
 + * as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful, but
 + * WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 + * General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License along
 + * with this program; if not, see http://www.gnu.org/licenses/.
 + *
 + * Author:
 + *   Roger Tseng rogera...@realtek.com
 + */
 +
 +#include linux/module.h
 +#include linux/slab.h
 +#include linux/delay.h
 +#include linux/platform_device.h
 +#include linux/usb.h
 +#include linux/mmc/host.h
 +#include linux/mmc/mmc.h
 +#include linux/mmc/sd.h
 +#include linux/mmc/sdio.h
 +#include linux/mmc/card.h
 +#include linux/scatterlist.h
 +#include linux/pm_runtime.h
 +
 +#include linux/mfd/rtsx_usb.h
 +#include asm/unaligned.h
 +
 +#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 +#include linux/leds.h
 +#include linux/workqueue.h
 +#define RTSX_USB_USE_LEDS_CLASS
 +#endif
 +
 +struct rtsx_usb_sdmmc {
 +   struct platform_device  *pdev;
 +   struct rtsx_ucr *ucr;
 +   struct mmc_host *mmc;
 +   struct mmc_request  *mrq;
 +
 +   struct mutexhost_mutex;
 +
 +   u8  ssc_depth;
 +   unsigned intclock;
 +   boolvpclk;
 +   booldouble_clk;
 +   boolhost_removal;
 +   boolcard_exist;
 +   boolinitial_mode;
 +   boolddr_mode;
 +
 +   unsigned char   power_mode;
 +
 +#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 +   struct led_classdev led;
 +   charled_name[32];
 +   struct work_struct  led_work;
 +#endif
 +};
 +
 +static inline struct device *sdmmc_dev(struct rtsx_usb_sdmmc *host)
 +{
 +   return (host-pdev-dev);
 +}
 +
 +static inline void sd_clear_error(struct rtsx_usb_sdmmc *host)
 +{
 +   struct rtsx_ucr *ucr = host-ucr;
 +   rtsx_usb_ep0_write_register(ucr, CARD_STOP,
 + SD_STOP | SD_CLR_ERR,
 + SD_STOP | SD_CLR_ERR);
 +
 +   rtsx_usb_clear_dma_err(ucr);
 +   rtsx_usb_clear_fsm_err(ucr);
 +}
 +
 +#ifdef DEBUG
 +static void sd_print_debug_regs(struct rtsx_usb_sdmmc *host)
 +{
 +   struct rtsx_ucr *ucr = host-ucr;
 +   u8 val = 0;
 +
 +   rtsx_usb_ep0_read_register(ucr, SD_STAT1, val);
 +   dev_dbg(sdmmc_dev(host), SD_STAT1: 0x%x\n, val);
 +   rtsx_usb_ep0_read_register(ucr, SD_STAT2, val);
 +   dev_dbg(sdmmc_dev(host), SD_STAT2: 0x%x\n, val);
 +   rtsx_usb_ep0_read_register(ucr, SD_BUS_STAT, val);
 +   

Re: [PATCH v4 2/3] mmc: Add realtek USB sdmmc host driver

[Ulf Hansson]

On 12 February 2014 11:00,  rogera...@realtek.com wrote:
 From: Roger Tseng rogera...@realtek.com

 Realtek USB SD/MMC host driver provides mmc host support based on the Realtek
 USB card reader MFD driver.

 Signed-off-by: Roger Tseng rogera...@realtek.com

Reviewed-by: Ulf Hansson ulf.hans...@linaro.org


 ---
  drivers/mmc/host/Kconfig  |7 +
  drivers/mmc/host/Makefile |1 +
  drivers/mmc/host/rtsx_usb_sdmmc.c | 1462 
 +
  3 files changed, 1470 insertions(+)
  create mode 100644 drivers/mmc/host/rtsx_usb_sdmmc.c

 diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
 index 7fc5099..16f9a23 100644
 --- a/drivers/mmc/host/Kconfig
 +++ b/drivers/mmc/host/Kconfig
 @@ -665,3 +665,10 @@ config MMC_REALTEK_PCI
 help
   Say Y here to include driver code to support SD/MMC card interface
   of Realtek PCI-E card reader
 +
 +config MMC_REALTEK_USB
 +   tristate Realtek USB SD/MMC Card Interface Driver
 +   depends on MFD_RTSX_USB
 +   help
 + Say Y here to include driver code to support SD/MMC card interface
 + of Realtek RTS5129/39 series card reader
 diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
 index c41d0c3..9d8d765 100644
 --- a/drivers/mmc/host/Makefile
 +++ b/drivers/mmc/host/Makefile
 @@ -51,6 +51,7 @@ obj-$(CONFIG_MMC_USHC)+= ushc.o
  obj-$(CONFIG_MMC_WMT)  += wmt-sdmmc.o

  obj-$(CONFIG_MMC_REALTEK_PCI)  += rtsx_pci_sdmmc.o
 +obj-$(CONFIG_MMC_REALTEK_USB)  += rtsx_usb_sdmmc.o

  obj-$(CONFIG_MMC_SDHCI_PLTFM)  += sdhci-pltfm.o
  obj-$(CONFIG_MMC_SDHCI_CNS3XXX)+= sdhci-cns3xxx.o
 diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c 
 b/drivers/mmc/host/rtsx_usb_sdmmc.c
 new file mode 100644
 index 000..37f58b5
 --- /dev/null
 +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
 @@ -0,0 +1,1462 @@
 +/* Realtek USB SD/MMC Card Interface driver
 + *
 + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU General Public License version 2
 + * as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful, but
 + * WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 + * General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License along
 + * with this program; if not, see http://www.gnu.org/licenses/.
 + *
 + * Author:
 + *   Roger Tseng rogera...@realtek.com
 + */
 +
 +#include linux/module.h
 +#include linux/slab.h
 +#include linux/delay.h
 +#include linux/platform_device.h
 +#include linux/usb.h
 +#include linux/mmc/host.h
 +#include linux/mmc/mmc.h
 +#include linux/mmc/sd.h
 +#include linux/mmc/sdio.h
 +#include linux/mmc/card.h
 +#include linux/scatterlist.h
 +#include linux/pm_runtime.h
 +
 +#include linux/mfd/rtsx_usb.h
 +#include asm/unaligned.h
 +
 +#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 +#include linux/leds.h
 +#include linux/workqueue.h
 +#define RTSX_USB_USE_LEDS_CLASS
 +#endif
 +
 +struct rtsx_usb_sdmmc {
 +   struct platform_device  *pdev;
 +   struct rtsx_ucr *ucr;
 +   struct mmc_host *mmc;
 +   struct mmc_request  *mrq;
 +
 +   struct mutexhost_mutex;
 +
 +   u8  ssc_depth;
 +   unsigned intclock;
 +   boolvpclk;
 +   booldouble_clk;
 +   boolhost_removal;
 +   boolcard_exist;
 +   boolinitial_mode;
 +   boolddr_mode;
 +
 +   unsigned char   power_mode;
 +
 +#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
 +   struct led_classdev led;
 +   charled_name[32];
 +   struct work_struct  led_work;
 +#endif
 +};
 +
 +static inline struct device *sdmmc_dev(struct rtsx_usb_sdmmc *host)
 +{
 +   return (host-pdev-dev);
 +}
 +
 +static inline void sd_clear_error(struct rtsx_usb_sdmmc *host)
 +{
 +   struct rtsx_ucr *ucr = host-ucr;
 +   rtsx_usb_ep0_write_register(ucr, CARD_STOP,
 + SD_STOP | SD_CLR_ERR,
 + SD_STOP | SD_CLR_ERR);
 +
 +   rtsx_usb_clear_dma_err(ucr);
 +   rtsx_usb_clear_fsm_err(ucr);
 +}
 +
 +#ifdef DEBUG
 +static void sd_print_debug_regs(struct rtsx_usb_sdmmc *host)
 +{
 +   struct rtsx_ucr *ucr = host-ucr;
 +   u8 val = 0;
 +
 +   rtsx_usb_ep0_read_register(ucr, SD_STAT1, val);
 +   dev_dbg(sdmmc_dev(host), SD_STAT1: 0x%x\n, val);
 +   rtsx_usb_ep0_read_register(ucr, SD_STAT2, val);
 +   dev_dbg(sdmmc_dev(host), SD_STAT2: 0x%x\n, val

Re: [PATCH v3 2/3] mmc: Add realtek USB sdmmc host driver

[Ulf Hansson]

On 11 February 2014 10:27, Roger rogera...@realtek.com wrote:
 On 02/10/2014 10:58 PM, Ulf Hansson wrote:

 On 6 February 2014 15:35,  rogera...@realtek.com wrote:

 From: Roger Tseng rogera...@realtek.com

 Realtek USB SD/MMC host driver provides mmc host support based on the
 Realtek
 USB card reader MFD driver.

 Signed-off-by: Roger Tseng rogera...@realtek.com
 ---
   drivers/mmc/host/Kconfig  |7 +
   drivers/mmc/host/Makefile |1 +
   drivers/mmc/host/rtsx_usb_sdmmc.c | 1500
 +
   3 files changed, 1508 insertions(+)
   create mode 100644 drivers/mmc/host/rtsx_usb_sdmmc.c

 [snip]

 +#ifdef CONFIG_PM_RUNTIME


 There are stubs for pm_runtime* functions, thus the ifdefs can be removed.
 Please go though the complete patch and remove all instances.

 +   pm_runtime_put(sdmmc_dev(host));


 I don't know so much about USB mmc hosts hardware, but I just wanted
 to find out if I have understood this correct.

 You can't do fine grained power management of the USB parent device,
 since it needs to be runtime resumed to be able keep the power the
 card? Once it becomes runtime suspended, the power to the card will
 thus also be dropped?

 Yes, and to keep some internal state of the controller.

Okay.

But the internal state of the controller should be possible to restore
at runtime_resume, so that should not be the reason, right?


 [snip]

 +#ifdef CONFIG_PM


 I suppose this should be CONFIG_PM_SLEEP?

 ...

 +   err = mmc_suspend_host(mmc);


 This won't compile. The mmc_suspend_host API has been removed.

 ...

 +   return mmc_resume_host(mmc);


 This won't compile. The mmc_resume_host API has been removed.

 ...

 +static struct platform_driver rtsx_usb_sdmmc_driver = {
 +   .probe  = rtsx_usb_sdmmc_drv_probe,
 +   .remove = rtsx_usb_sdmmc_drv_remove,
 +   .id_table   = rtsx_usb_sdmmc_ids,
 +   .suspend= rtsx_usb_sdmmc_suspend,
 +   .resume = rtsx_usb_sdmmc_resume,


 Please use the modern pm_ops instead of the legacy suspend/resume
 callbacks.
 I suggest you then also switch to use the SIMPLE_DEV_PM_OPS macro.


 I just missed the removal of mmc_suspend|resume_host.

 I'll remove all these unnecessary mmc host pm things as done in commit
 ff71c4bcb0af2730d047989e485303ae4e1ce794 for drivers of our PCIe devices.

 Thanks for pointing this out.

 Kind regards
 Ulf Hansson
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/3] mfd: Add realtek USB card reader driver

[Ulf Hansson]

On 11 February 2014 10:40, Roger rogera...@realtek.com wrote:
 On 02/10/2014 07:30 PM, Lee Jones wrote:

 From: Roger Tseng rogera...@realtek.com

 Realtek USB card reader provides a channel to transfer command or data to
 flash
 memory cards. This driver exports host instances for mmc and memstick
 subsystems
 and handles basic works.

 Signed-off-by: Roger Tseng rogera...@realtek.com
 ---
   drivers/mfd/Kconfig  |  10 +
   drivers/mfd/Makefile |   1 +
   drivers/mfd/rtsx_usb.c   | 760
 +++
   include/linux/mfd/rtsx_usb.h | 628 +++
   4 files changed, 1399 insertions(+)
   create mode 100644 drivers/mfd/rtsx_usb.c
   create mode 100644 include/linux/mfd/rtsx_usb.h


 Applied with Greg's Ack.

 Thanks. But I have to modify some places in PATCH 2/3 and 3/3 according to
 Ulf's suggestion. I will resend v4 later but currently I don't plan to
 change the 1/3 part(for mfd) in it.

 Would you wait for and re-apply the v4 submission or give any advice?

Just send a v4 of PATCH 2/3 and 3/3, that should be fine I think.

Kind regards
Ulf Hansson
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel