diff mbox series

[07/23] wfx: add bus_sdio.c

Message ID 20201012104648.985256-8-Jerome.Pouiller@silabs.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wfx: get out from the staging area | expand

Commit Message

Jérôme Pouiller Oct. 12, 2020, 10:46 a.m. UTC
From: Jérôme Pouiller <jerome.pouiller@silabs.com>

Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
---
 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

Comments

Pali Rohár Oct. 13, 2020, 8:11 p.m. UTC | #1
Hello!

On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> +#define SDIO_VENDOR_ID_SILABS        0x0000
> +#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?

> +	{ },
> +};
> +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.28.0
>
Jérôme Pouiller Oct. 14, 2020, 11:52 a.m. UTC | #2
Hello Pali,

On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:
> Hello!
> 
> On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> > +#define SDIO_VENDOR_ID_SILABS        0x0000
> > +#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 0000:1000 (as you can see
above). This value is weird. The risk of collision with another device is
high.

So, maybe the device should be probed only if it appears in the DT. Since
WF200 targets embedded platforms, I don't think it is a problem to rely on
DT. You will find another FIXME further in the code about that:

+               dev_warn(&func->dev,
+                        "device is not declared in DT, features will be limited\n");
+               // FIXME: ignore VID/PID and only rely on device tree
+               // return -ENODEV;

However, it wouldn't be usual way to manage SDIO devices (and it is the
reason why the code is commented out).

Anyway, if we choose to rely on the DT, should we also check the VID/PID?

Personally, I am in favor to probe the device only if VID/PID match and if
a DT node is found, even if it is not the usual way.
Pali Rohár Oct. 14, 2020, 12:43 p.m. UTC | #3
On Wednesday 14 October 2020 13:52:15 Jérôme Pouiller wrote:
> Hello Pali,
> 
> On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:
> > Hello!
> > 
> > On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> > > +#define SDIO_VENDOR_ID_SILABS        0x0000
> > > +#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 0000: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 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-pali@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-pali@kernel.org/T/#u

Could you try them and look at /sys/class/mmc_host/ attribute outputs?

> So, maybe the device should be probed only if it appears in the DT. Since
> WF200 targets embedded platforms, I don't think it is a problem to rely on
> DT. You will find another FIXME further in the code about that:
> 
> +               dev_warn(&func->dev,
> +                        "device is not declared in DT, features will be limited\n");
> +               // FIXME: ignore VID/PID and only rely on device tree
> +               // return -ENODEV;
> 
> However, it wouldn't be usual way to manage SDIO devices (and it is the
> reason why the code is commented out).
> 
> Anyway, if we choose to rely on the DT, should we also check the VID/PID?
> 
> Personally, I am in favor to probe the device only if VID/PID match and if
> a DT node is found, even if it is not the usual way.

Normally all sdio devices are hotplugged in linux kernel based on sdio
device and vendor ids. And these ids are unique identifiers of sdio
devices. So should be enough for detection.

Months ago I have checked it and moved all SDIO device and vendor ids
into common include/linux/mmc/sdio_ids.h file. I would like to not have
this "mess" again, which was basically fully cleaned.

I'm adding linux-mmc mailing list and Ulf Hansson to loop.

Ulf, can you look at this "problem"? What do you think about those
"strange" sdio ids?
Jérôme Pouiller Oct. 15, 2020, 2:03 p.m. UTC | #4
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_SILABS        0x0000
> > > > +#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 0000: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 :( .


> 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-pali@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-pali@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:0x0000
    mmc1/mmc1:0001/rca:0x0001
    mmc1/mmc1:0001/device:0x1000
    mmc1/mmc1:0001/mmc1:0001:1/vendor:0x0000
    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:c00v0000d1000
    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@7e300000/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=0000:1000
    mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_REVISION=0.0
    mmc1/mmc1:0001/mmc1:0001:1/uevent:MODALIAS=sdio:c00v0000d1000
    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:0x00200000
    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=0000: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
Ulf Hansson Oct. 16, 2020, 11:30 a.m. UTC | #5
On Mon, 12 Oct 2020 at 12:47, Jerome Pouiller
<Jerome.Pouiller@silabs.com> wrote:
>
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>

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 <jerome.pouiller@silabs.com>
> ---
>  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 000000000000..e06d7e1ebe9c

[...]

> +
> +static int wfx_sdio_irq_subscribe(void *priv)
> +{
> +       struct wfx_sdio_priv *bus = priv;
> +       u32 flags;
> +       int ret;
> +       u8 cccr;
> +

I would appreciate a comment about an out-of-band IRQ line. If it's
supported, that is the preferred option to use, else the SDIO IRQs.

> +       if (!bus->of_irq) {
> +               sdio_claim_host(bus->func);
> +               ret = sdio_claim_irq(bus->func, wfx_sdio_irq_handler);
> +               sdio_release_host(bus->func);
> +               return ret;
> +       }
> +
> +       sdio_claim_host(bus->func);
> +       cccr = sdio_f0_readb(bus->func, SDIO_CCCR_IENx, NULL);
> +       cccr |= BIT(0);
> +       cccr |= BIT(bus->func->num);
> +       sdio_f0_writeb(bus->func, cccr, SDIO_CCCR_IENx, NULL);
> +       sdio_release_host(bus->func);
> +       flags = irq_get_trigger_type(bus->of_irq);
> +       if (!flags)
> +               flags = IRQF_TRIGGER_HIGH;
> +       flags |= IRQF_ONESHOT;
> +       return devm_request_threaded_irq(&bus->func->dev, bus->of_irq, NULL,
> +                                        wfx_sdio_irq_handler_ext, flags,
> +                                        "wfx", bus);
> +}
> +

[...]

> +
> +#define SDIO_VENDOR_ID_SILABS        0x0000
> +#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
Ulf Hansson Oct. 16, 2020, 11:54 a.m. UTC | #6
On Thu, 15 Oct 2020 at 16:03, Jérôme Pouiller
<jerome.pouiller@silabs.com> 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_SILABS        0x0000
> > > > > +#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 0000: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-pali@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-pali@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:0x0000
>     mmc1/mmc1:0001/rca:0x0001
>     mmc1/mmc1:0001/device:0x1000
>     mmc1/mmc1:0001/mmc1:0001:1/vendor:0x0000
>     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:c00v0000d1000
>     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@7e300000/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=0000:1000
>     mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_REVISION=0.0
>     mmc1/mmc1:0001/mmc1:0001:1/uevent:MODALIAS=sdio:c00v0000d1000
>     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:0x00200000
>     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=0000: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(), which calls sdio_set_of_node() - we assign the
func->dev.of_node its corresponding child node for the SDIO func.
Allowing the sdio functional driver to be matched with the SDIO func
device.

Perhaps what is missing, is that we may want to avoid probing an
in-correct sdio driver, based upon buggy SDIO ids. I don't think we
take some actions in the mmc core to prevent this, but maybe it's not
a big problem anyway.

When it comes to documenting the buggy SDIO ids, please add some
information about this in the common SDIO headers. It's nice to have a
that information, if any issue comes up later on.

Kind regards
Uffe
Jérôme Pouiller Oct. 16, 2020, 12:16 p.m. UTC | #7
Hello Ulf,

On Friday 16 October 2020 13:30:30 CEST Ulf Hansson wrote:
> On Mon, 12 Oct 2020 at 12:47, Jerome Pouiller
> <Jerome.Pouiller@silabs.com> wrote:
> >
> > From: Jérôme Pouiller <jerome.pouiller@silabs.com>
> 
> Please fill out this commit message to explain a bit more about the
> patch and the HW it enables support for.

This patch belongs to a series[1] that will squashed before to be
committed (Kalle Valo prefer to process like that for this review). So,
I didn't bother to write real commit messages. For the v2, I will take
care to add linux-mmc in copy of the whole series.

[1] https://lore.kernel.org/lkml/20201012104648.985256-1-Jerome.Pouiller@silabs.com/


> > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> > ---
> >  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 000000000000..e06d7e1ebe9c
[...]
> > +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?

Indeed, it is here[2].

[2] https://lore.kernel.org/lkml/20201012104648.985256-5-Jerome.Pouiller@silabs.com/
Kalle Valo Nov. 2, 2020, 4:02 p.m. UTC | #8
Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Thu, 15 Oct 2020 at 16:03, Jérôme Pouiller
> <jerome.pouiller@silabs.com> 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_SILABS        0x0000
>> > > > > +#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 0000: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-pali@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-pali@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:0x0000
>>     mmc1/mmc1:0001/rca:0x0001
>>     mmc1/mmc1:0001/device:0x1000
>>     mmc1/mmc1:0001/mmc1:0001:1/vendor:0x0000
>>     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:c00v0000d1000
>>     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@7e300000/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=0000:1000
>>     mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_REVISION=0.0
>>     mmc1/mmc1:0001/mmc1:0001:1/uevent:MODALIAS=sdio:c00v0000d1000
>>     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:0x00200000
>>     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=0000: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(), which calls sdio_set_of_node() - we assign the
> func->dev.of_node its corresponding child node for the SDIO func.
> Allowing the sdio functional driver to be matched with the SDIO func
> device.
>
> Perhaps what is missing, is that we may want to avoid probing an
> in-correct sdio driver, based upon buggy SDIO ids. I don't think we
> take some actions in the mmc core to prevent this, but maybe it's not
> a big problem anyway.
>
> When it comes to documenting the buggy SDIO ids, please add some
> information about this in the common SDIO headers. It's nice to have a
> that information, if any issue comes up later on.

And if there's some special setup (DT etc) needed to get the wireless
driver working that should be documented in the driver side as well. The
expectation is that an upstream driver is just plug and play, and if
it's not there should be clear documentation how to enable the driver.
diff mbox series

Patch

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 000000000000..e06d7e1ebe9c
--- /dev/null
+++ b/drivers/net/wireless/silabs/wfx/bus_sdio.c
@@ -0,0 +1,269 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SDIO interface.
+ *
+ * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
+ * Copyright (c) 2010, ST-Ericsson
+ */
+#include <linux/module.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/sdio_func.h>
+#include <linux/mmc/card.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/irq.h>
+
+#include "bus.h"
+#include "wfx.h"
+#include "hwio.h"
+#include "main.h"
+#include "bh.h"
+
+static const struct wfx_platform_data wfx_sdio_pdata = {
+	.file_fw = "wfm_wf200",
+	.file_pds = "wf200.pds",
+};
+
+struct wfx_sdio_priv {
+	struct sdio_func *func;
+	struct wfx_dev *core;
+	u8 buf_id_tx;
+	u8 buf_id_rx;
+	int of_irq;
+};
+
+static int wfx_sdio_copy_from_io(void *priv, unsigned int reg_id,
+				 void *dst, size_t count)
+{
+	struct wfx_sdio_priv *bus = priv;
+	unsigned int sdio_addr = reg_id << 2;
+	int ret;
+
+	WARN(reg_id > 7, "chip only has 7 registers");
+	WARN(((uintptr_t)dst) & 3, "unaligned buffer size");
+	WARN(count & 3, "unaligned buffer address");
+
+	/* Use queue mode buffers */
+	if (reg_id == WFX_REG_IN_OUT_QUEUE)
+		sdio_addr |= (bus->buf_id_rx + 1) << 7;
+	ret = sdio_memcpy_fromio(bus->func, dst, sdio_addr, count);
+	if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE)
+		bus->buf_id_rx = (bus->buf_id_rx + 1) % 4;
+
+	return ret;
+}
+
+static int wfx_sdio_copy_to_io(void *priv, unsigned int reg_id,
+			       const void *src, size_t count)
+{
+	struct wfx_sdio_priv *bus = priv;
+	unsigned int sdio_addr = reg_id << 2;
+	int ret;
+
+	WARN(reg_id > 7, "chip only has 7 registers");
+	WARN(((uintptr_t)src) & 3, "unaligned buffer size");
+	WARN(count & 3, "unaligned buffer address");
+
+	/* Use queue mode buffers */
+	if (reg_id == WFX_REG_IN_OUT_QUEUE)
+		sdio_addr |= bus->buf_id_tx << 7;
+	// FIXME: discards 'const' qualifier for src
+	ret = sdio_memcpy_toio(bus->func, sdio_addr, (void *)src, count);
+	if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE)
+		bus->buf_id_tx = (bus->buf_id_tx + 1) % 32;
+
+	return ret;
+}
+
+static void wfx_sdio_lock(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	sdio_claim_host(bus->func);
+}
+
+static void wfx_sdio_unlock(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	sdio_release_host(bus->func);
+}
+
+static void wfx_sdio_irq_handler(struct sdio_func *func)
+{
+	struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
+
+	wfx_bh_request_rx(bus->core);
+}
+
+static irqreturn_t wfx_sdio_irq_handler_ext(int irq, void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	sdio_claim_host(bus->func);
+	wfx_bh_request_rx(bus->core);
+	sdio_release_host(bus->func);
+	return IRQ_HANDLED;
+}
+
+static int wfx_sdio_irq_subscribe(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+	u32 flags;
+	int ret;
+	u8 cccr;
+
+	if (!bus->of_irq) {
+		sdio_claim_host(bus->func);
+		ret = sdio_claim_irq(bus->func, wfx_sdio_irq_handler);
+		sdio_release_host(bus->func);
+		return ret;
+	}
+
+	sdio_claim_host(bus->func);
+	cccr = sdio_f0_readb(bus->func, SDIO_CCCR_IENx, NULL);
+	cccr |= BIT(0);
+	cccr |= BIT(bus->func->num);
+	sdio_f0_writeb(bus->func, cccr, SDIO_CCCR_IENx, NULL);
+	sdio_release_host(bus->func);
+	flags = irq_get_trigger_type(bus->of_irq);
+	if (!flags)
+		flags = IRQF_TRIGGER_HIGH;
+	flags |= IRQF_ONESHOT;
+	return devm_request_threaded_irq(&bus->func->dev, bus->of_irq, NULL,
+					 wfx_sdio_irq_handler_ext, flags,
+					 "wfx", bus);
+}
+
+static int wfx_sdio_irq_unsubscribe(void *priv)
+{
+	struct wfx_sdio_priv *bus = priv;
+	int ret;
+
+	if (bus->of_irq)
+		devm_free_irq(&bus->func->dev, bus->of_irq, bus);
+	sdio_claim_host(bus->func);
+	ret = sdio_release_irq(bus->func);
+	sdio_release_host(bus->func);
+	return ret;
+}
+
+static size_t wfx_sdio_align_size(void *priv, size_t size)
+{
+	struct wfx_sdio_priv *bus = priv;
+
+	return sdio_align_size(bus->func, size);
+}
+
+static const struct hwbus_ops wfx_sdio_hwbus_ops = {
+	.copy_from_io = wfx_sdio_copy_from_io,
+	.copy_to_io = wfx_sdio_copy_to_io,
+	.irq_subscribe = wfx_sdio_irq_subscribe,
+	.irq_unsubscribe = wfx_sdio_irq_unsubscribe,
+	.lock			= wfx_sdio_lock,
+	.unlock			= wfx_sdio_unlock,
+	.align_size		= wfx_sdio_align_size,
+};
+
+static const struct of_device_id wfx_sdio_of_match[] = {
+	{ .compatible = "silabs,wfx-sdio" },
+	{ .compatible = "silabs,wf200" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, wfx_sdio_of_match);
+
+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(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n",
+			func->num);
+		return -ENODEV;
+	}
+
+	bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL);
+	if (!bus)
+		return -ENOMEM;
+
+	if (np) {
+		if (!of_match_node(wfx_sdio_of_match, np)) {
+			dev_warn(&func->dev, "no compatible device found in DT\n");
+			return -ENODEV;
+		}
+		bus->of_irq = irq_of_parse_and_map(np, 0);
+	} else {
+		dev_warn(&func->dev,
+			 "device is not declared in DT, features will be limited\n");
+		// FIXME: ignore VID/PID and only rely on device tree
+		// return -ENODEV;
+	}
+
+	bus->func = func;
+	sdio_set_drvdata(func, bus);
+	func->card->quirks |= MMC_QUIRK_LENIENT_FN0 |
+			      MMC_QUIRK_BLKSZ_FOR_BYTE_MODE |
+			      MMC_QUIRK_BROKEN_BYTE_MODE_512;
+
+	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)
+		goto err0;
+
+	bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
+				    &wfx_sdio_hwbus_ops, bus);
+	if (!bus->core) {
+		ret = -EIO;
+		goto err1;
+	}
+
+	ret = wfx_probe(bus->core);
+	if (ret)
+		goto err1;
+
+	return 0;
+
+err1:
+	sdio_claim_host(func);
+	sdio_disable_func(func);
+	sdio_release_host(func);
+err0:
+	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);
+}
+
+#define SDIO_VENDOR_ID_SILABS        0x0000
+#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);
+
+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,
+	}
+};