On Thu, 15 Oct 2020 at 16:03, Jérôme Pouiller
[off-list ref] wrote:

On Wednesday 14 October 2020 14:43:34 CEST Pali Rohár wrote:

quoted

On Wednesday 14 October 2020 13:52:15 Jérôme Pouiller wrote:

quoted

On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:

quoted

On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:

quoted

+#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.

quoted

+     // 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.

quoted

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

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

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