Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   =?ISO-8859-1?Q?J=E9r=F4me?= Pouiller <jerome.pouiller@silabs.com>
To:     Ulf Hansson <ulf.hansson@linaro.org>,
        "H. Nikolaus Schaller" <hns@goldelico.com>
Cc:     Avri Altman <avri.altman@wdc.com>,
        Shawn Lin <shawn.lin@rock-chips.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bean Huo <beanhuo@micron.com>, linux-mmc@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@openphoenux.org>, kernel@pyra-handheld.com,
        Tony Lindgren <tony@atomide.com>,
        Linux-OMAP <linux-omap@vger.kernel.org>
Subject: Re: [RFC] mmc: core: transplant ti,wl1251 quirks from to be retired omap_hsmmc
Date:   Thu, 28 Oct 2021 10:59:17 +0200
Message-ID: <2013308.OSlt1BDEiP@pc-42>
Organization: Silicon Labs
In-Reply-To: <470A96FD-DB24-4C32-BC9F-AE2F617FBF2D@goldelico.com>
References: <8ecc5c79c1dd0627d570ede31e18c860786cacca.1633519499.git.hns@goldelico.com> <CAPDyKFp47sAXhM2s5HOqV2wLf-kYRhdqSdzcn7a62ZW23SSPdg@mail.gmail.com> <470A96FD-DB24-4C32-BC9F-AE2F617FBF2D@goldelico.com>
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?E44jxA7hC6DyuP1CzC3H5zdE3FwtW6VaXTA//l1JsL5cEssQLfWicUQqU5?=
 =?iso-8859-1?Q?gdeRC1kJrbGYkUm+dp+Qj2StSsPXqBnLZ8F20/z5vVRbVvopA5ZixtkGaY?=
 =?iso-8859-1?Q?17J8Hrou2sigo3ePPooRCH9d4Tx804bD9DJB4gTs+u60d/+lAMs9Wd78Vu?=
 =?iso-8859-1?Q?JTdYvuVJIbbCw8Pvusld4yCH4nTQ/Cs0YGfpVNXAMTD2uzIptTOHW7+IIH?=
 =?iso-8859-1?Q?gXjM2gGvshFt76o1COUxi1KRDMSVhaa+y4HlkRByr02vS0fIajGhmCcAcf?=
 =?iso-8859-1?Q?PBxCew3tZ/pAiUfauVySincaFCi0VQMmbxE+CdtsVYyOkuHhqf2+qr3Ybo?=
 =?iso-8859-1?Q?hpzGHXQDJWeMa16rOp7/wfWI93ppSGY/8IPkgSjJ3yLGE4s2mAuWek0LJT?=
 =?iso-8859-1?Q?Ve+aJQTPqJmifUgOmgojq+JjABsteJSz3Iinj9zr7NKu5mKE2U4zG2RV0s?=
 =?iso-8859-1?Q?LBv3mdtAd5lfWtQcCtTG9rDbwAZbNBmNJCWng5EFas6ZPwYLNg27W0l80q?=
 =?iso-8859-1?Q?oEX7C1pYYHnJoBCYJdnnCQjdgVoctDQyhnAeZ0sacTvDzNe9WHPJdqLBww?=
 =?iso-8859-1?Q?2TTqPlC0U4f5/gVuo8iCIQnrcb+i6fWU72rl30sn0Nt6dWReouFqy5nJ2z?=
 =?iso-8859-1?Q?HW7DiVqRXQr+qPX9ZD9os0IOp7kcYpOEBtk9JtjtNbX/+oTOvklI4Ca6l0?=
 =?iso-8859-1?Q?9VUc6OvF8B7JDTTLVmhThcEjEdxsb/7rptrw6v70naFaOiul0e04yFrgth?=
 =?iso-8859-1?Q?CqaXw2a6xvEVUAdaSQ7gHoi8cY5YSo8t8o2w1z8Clu97JYs6kO9p6Ggcxl?=
 =?iso-8859-1?Q?BAGmRYMy3osHXoCPFnmeGP0vT7e/bzP7zpbmP0fZtSbaiycWl5Hnu/z/8f?=
 =?iso-8859-1?Q?G4wjNDphXQ5/FsO2qHwUlRajhjmE4v5TBz7T9MstnDkuXCm2uhjI0/3xyZ?=
 =?iso-8859-1?Q?fs28YYhzZ4mdcRmTD4uVd0ahjkMCmyktBDb0EWONNnS5qLHgxWmvMVdLtA?=
 =?iso-8859-1?Q?Kydin+JaOAYMKmOOLheNlUVVk47ZaCeR4upwTjp1U+C+oiDVtPXSgXQQYT?=
 =?iso-8859-1?Q?6dY4qKvbHchtYoxoqjVm+j04gjUi4Mo3fXt1hsFGyuoUDUQlRyJz05Et4f?=
 =?iso-8859-1?Q?CMzvtj2cZPz1h4FSVeyBWYhHiqvxAS4swNn+RE9ev9DYJK2m9Xfaxk0xHN?=
 =?iso-8859-1?Q?MrpeM5X+weZ3UiFqmcEaDyb4qN4GOQNwfmMSzWBP/ZUvWLLpFp5sgweAz6?=
 =?iso-8859-1?Q?HL1eZsjvTGNBSC9+gPgR4wWxYZ9GST7LwemkW+BtzfT/1oDqTtA3J/HW20?=
 =?iso-8859-1?Q?ivkDknUWMoISik8+tXtWtaP5js1dfDxbtqEJ9OoHqr9d4VvuUkiOf4FpUz?=
 =?iso-8859-1?Q?Obq3glXkjdFsaAQi09GFJ7SFIpm3gbhfQZGmLfKGnKvefuNQAia+e4GhCs?=
 =?iso-8859-1?Q?qdJh7JCL+dc0tjRaMNrPI6KwUlYuwu06p9zYbmHZZqZb7xu6dhmGF2dNxb?=
 =?iso-8859-1?Q?WmLCO+xV3fIIyN0MvWle3AoszldaQQyHcfrGPFtu8j4IrOJ5zzJTNhhtkt?=
 =?iso-8859-1?Q?FgHK++aQJTUtS71iQr5LLoOFNVza68igiDYrgwV2ftYGsM1MHVyCoT0H1e?=
 =?iso-8859-1?Q?JBpzdtx25m3dzI5bdmIZiKI/1Mk14+oF3rUELIQnVACoj179wi+LwMs6yc?=
 =?iso-8859-1?Q?zYSHNgqq5FWilDWgiY0=3D?=
X-OriginatorOrg: silabs.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4e74c73d-dbaa-4f0d-0c33-08d999f13dd6
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5657.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Oct 2021 08:59:25.4914
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 54dbd822-5231-4b20-944d-6f4abcd541fb
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4cYu0UTUPfPT6hBG0aLZllb2tjRXqtxrtcIu4wy0mr2/u35yLy7uIbkjj+RbR4eAonl9bUiQB7U64FdAxDSBkQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5612
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Nikolaus,

On Thursday 28 October 2021 09:08:50 CEST H. Nikolaus Schaller wrote:
> > Am 27.10.2021 um 23:31 schrieb Ulf Hansson <ulf.hansson@linaro.org>:
> > On Wed, 27 Oct 2021 at 19:01, H. Nikolaus Schaller <hns@goldelico.com> =
wrote:
> >>> Am 26.10.2021 um 20:08 schrieb H. Nikolaus Schaller <hns@goldelico.co=
m>:
> >>>> As a matter of fact, the similar problem that you are looking to
> >>>> address (applying card quirks based on DT compatibility strings), is
> >>>> partly being taken care of in another series [1], being discussed
> >>>> right now. I think the solution for the ti,wl1251 should be based up=
on
> >>>> that too. Please have a look and see if you can play with that!?
> >>>
> >>> That is interesting.
> >>> Yes, maybe it can be the basis. At least for finding the chip and dri=
ver.
> >>
> >> I have done a first experiment.
> >>
> >> It seems as if the series [1] does the opposite of what we need... It =
just
> >> skips entries in struct mmc_fixup if the DT does *not* match.
> >
> > Ohh, I didn't look that close. In that case the code isn't doing what
> > it *should*. The point is really to match on the compatible string and
> > then add quirks if that is found.
>=20
> That is what I had expected.

Note I have not tested this code. My primary goal was to submit the idea. I
think I will be able to send a true PR next week.


> > Let me have a closer look - and for sure, I am willing to help if neede=
d.

I confirm it does not have the expected behavior. !mmc_fixup_of_compatible_=
match()
should be mmc_fixup_of_compatible_match(), sorry.


[...]
> >> What I don't get from the code is how cis.vendor or cis.device can be
> >> initialized from device tree for a specific device. As far as I see it=
 can
> >> only be checked for and some quirks can be set from a table if vendor =
and
> >> device read from the CIS registers do match.
> >
> > Yes. I thought that should be possible, but maybe it is not?
>=20
> It seems to be a hen or egg issue here. MMC_QUIRK_NONSTD_SDIO should be s=
et
> before we can match by vendor and device or compatible. But it can't be s=
et
> late.

I think you can add a new fixup table that could be applied earlier (as you
do in your suggestion below).


> >> Instead, we want to match DT and define some values for an otherwise u=
nknown
> >> device (i.e. we can't match by vendor or other methods) to help to ini=
tialize
> >> the interface. So in mmc_fixup_device it is too late and we need somet=
hing
> >> running earlier, based purely on device tree information...
> >
> > Okay, I will have a closer look. Maybe we need to extend the card
> > quirks interface a bit to make it suitable for this case too.
>=20
> Combining your suggestions we could do roughly:
>=20
> in mmc_sdio_init_card():
>=20
>         if (host->ops->init_card)
>                 host->ops->init_card(host, card);
>         else
>                 mmc_fixup_device(host, sdio_prepare_fixups_methods);

I think I mostly agree, but why you don't call mmc_fixup_device() if
init_card is defined? (BTW, mmc_fixup_device() takes a card as
first parameter)


> Next we need a location for the sdio_prepare_fixups_methods table and fun=
ctions.
>=20
> For "ti,wl1251" we would then provide the entry in the table and a functi=
on doing
> the setup. But where should these be defined? Likely not in a header file=
 like
> quirks.h? But there is no quirks.c.

I think you can place your function in drivers/mmc/core/card.h. There are
already add_quirk(), add_limit_rate_quirk(), add_quirk_mmc(), etc...


--=20
J=E9r=F4me Pouiller