DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: "Jérôme Pouiller" <jerome.pouiller@silabs.com>
Cc: driverdevel <devel@driverdev.osuosl.org>,
	DTML <devicetree@vger.kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	netdev <netdev@vger.kernel.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	"Pali Rohár" <pali@kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	"Kalle Valo" <kvalo@codeaurora.org>
Subject: Re: [PATCH 07/23] wfx: add bus_sdio.c
Date: Fri, 16 Oct 2020 13:54:48 +0200
Message-ID: <CAPDyKFqCn386r4ecLDnMQmxrAZCvU9r=-eY71UUNpXWNxKOz2g@mail.gmail.com> (raw)
In-Reply-To: <2444203.ROLCPKctRj@pc-42>

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

  reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-12 10:46 [PATCH 00/23] wfx: get out from the staging area Jerome Pouiller
2020-10-12 10:46 ` [PATCH 01/23] dt-bindings: introduce silabs,wfx.yaml Jerome Pouiller
2020-10-13 16:49   ` Rob Herring
2020-10-14 13:49     ` Jérôme Pouiller
2020-11-02 15:58       ` Kalle Valo
2020-10-12 10:46 ` [PATCH 02/23] wfx: add Makefile/Kconfig Jerome Pouiller
2020-10-12 10:46 ` [PATCH 03/23] wfx: add wfx.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 04/23] wfx: add main.c/main.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 05/23] wfx: add bus.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 06/23] wfx: add bus_spi.c Jerome Pouiller
2020-10-12 10:46 ` [PATCH 07/23] wfx: add bus_sdio.c Jerome Pouiller
2020-10-13 20:11   ` Pali Rohár
2020-10-14 11:52     ` Jérôme Pouiller
2020-10-14 12:43       ` Pali Rohár
2020-10-15 14:03         ` Jérôme Pouiller
2020-10-16 11:54           ` Ulf Hansson [this message]
2020-11-02 16:02             ` Kalle Valo
2020-10-16 11:30   ` Ulf Hansson
2020-10-16 12:16     ` Jérôme Pouiller
2020-10-12 10:46 ` [PATCH 08/23] wfx: add hwio.c/hwio.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 09/23] wfx: add fwio.c/fwio.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 10/23] wfx: add bh.c/bh.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 11/23] wfx: add hif_api_*.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 12/23] wfx: add hif_tx*.c/hif_tx*.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 13/23] wfx: add key.c/key.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 14/23] wfx: add hif_rx.c/hif_rx.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 15/23] wfx: add data_rx.c/data_rx.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 16/23] wfx: add queue.c/queue.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 17/23] wfx: add data_tx.c/data_tx.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 18/23] wfx: add sta.c/sta.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 19/23] wfx: add scan.c/scan.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 20/23] wfx: add debug.c/debug.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 21/23] wfx: add traces.h Jerome Pouiller
2020-10-12 10:46 ` [PATCH 22/23] wfx: remove from the staging area Jerome Pouiller
2020-10-12 10:46 ` [PATCH 23/23] wfx: get out " Jerome Pouiller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAPDyKFqCn386r4ecLDnMQmxrAZCvU9r=-eY71UUNpXWNxKOz2g@mail.gmail.com' \
    --to=ulf.hansson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=devel@driverdev.osuosl.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jerome.pouiller@silabs.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pali@kernel.org \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git