| Message ID | 0887d84402f796d1e7361261b88ec6057fbb0065.1571510481.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | OpenPandora: make wl1251 connected to mmc3 sdio port of OpenPandora work again | expand |
On Sat, 19 Oct 2019 at 20:42, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Pandora_wl1251_init_card was used to do special pdata based > setup of the sdio mmc interface. This does no longer work with > v4.7 and later. A fix requires a device tree based mmc3 setup. > > Therefore we move the special setup to omap_hsmmc.c instead > of calling some pdata supplied init_card function. > > The new code checks for a DT child node compatible to wl1251 > so it will not affect other MMC3 use cases. > > Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: <stable@vger.kernel.org> # 4.7.0 > --- > drivers/mmc/host/omap_hsmmc.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 952fa4063ff8..03ba80bcf319 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -1512,6 +1512,27 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card) > > if (mmc_pdata(host)->init_card) > mmc_pdata(host)->init_card(card); > + else if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { > + struct device_node *np = mmc_dev(mmc)->of_node; > + > + np = of_get_compatible_child(np, "ti,wl1251"); > + if (np) { > + /* > + * We have TI wl1251 attached to MMC3. Pass this information to > + * SDIO core because it can't be probed by normal methods. > + */ > + > + dev_info(host->dev, "found wl1251\n"); > + card->quirks |= MMC_QUIRK_NONSTD_SDIO; > + card->cccr.wide_bus = 1; > + card->cis.vendor = 0x104c; > + card->cis.device = 0x9066; > + card->cis.blksize = 512; > + card->cis.max_dtr = 24000000; > + card->ocr = 0x80; These things should really be figured out by the mmc core during SDIO card initialization itself, not via the host ops ->init_card() callback. That is just poor hack, which in the long run should go away. Moreover, I think we should add a subnode to the host node in the DT, to describe the embedded SDIO card, rather than parsing the subnode for the SDIO func - as that seems wrong to me. To add a subnode for the SDIO card, we already have a binding that I think we should extend. Please have a look at Documentation/devicetree/bindings/mmc/mmc-card.txt. If you want an example of how to implement this for your case, do a git grep "broken-hpi" in the driver/mmc/core/, I think it will tell you more of what I have in mind. > + of_node_put(np); > + } > + } > } > > static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > -- > 2.19.1 > Kind regards Uffe
Hi Ulf, > Am 30.10.2019 um 16:51 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > >> + >> + np = of_get_compatible_child(np, "ti,wl1251"); >> + if (np) { >> + /* >> + * We have TI wl1251 attached to MMC3. Pass this information to >> + * SDIO core because it can't be probed by normal methods. >> + */ >> + >> + dev_info(host->dev, "found wl1251\n"); >> + card->quirks |= MMC_QUIRK_NONSTD_SDIO; >> + card->cccr.wide_bus = 1; >> + card->cis.vendor = 0x104c; >> + card->cis.device = 0x9066; >> + card->cis.blksize = 512; >> + card->cis.max_dtr = 24000000; >> + card->ocr = 0x80; > > These things should really be figured out by the mmc core during SDIO > card initialization itself, not via the host ops ->init_card() > callback. That is just poor hack, which in the long run should go > away. Yes, I agree. But I am just the poor guy who is trying to fix really broken code with as low effort as possible. I don't even have a significant clue what this code is exactly doing and what the magic values mean. They were setup by pandora_wl1251_init_card() in the same way so that I have just moved the code here and make it called in (almost) the same situation. > Moreover, I think we should add a subnode to the host node in the DT, > to describe the embedded SDIO card, rather than parsing the subnode > for the SDIO func - as that seems wrong to me. You mean a second subnode? The wl1251 is the child node of the mmc node and describes the SDIO card. We just check if it is a wl1251 or e.g. wl1837 or something else or even no child. > To add a subnode for the SDIO card, we already have a binding that I > think we should extend. Please have a look at > Documentation/devicetree/bindings/mmc/mmc-card.txt. > > If you want an example of how to implement this for your case, do a > git grep "broken-hpi" in the driver/mmc/core/, I think it will tell > you more of what I have in mind. So while I agree that it should be improved in the long run, we should IMHO fix the hack first (broken since v4.9!), even if it remains a hack for now. Improving this part seems to be quite independent and focussed on the mmc subsystem, while the other patches involve other subsystems. Maybe should we make a REVISIT note in the code? Or add something to the commit message about the idea how it should be done right? > >> + of_node_put(np); >> + } >> + } >> } >> >> static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable) >> -- >> 2.19.1 >> > > Kind regards > Uffe BR and thanks, Nikolaus
On Wed, 30 Oct 2019 at 18:25, H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Hi Ulf, > > > Am 30.10.2019 um 16:51 schrieb Ulf Hansson <ulf.hansson@linaro.org>: > > > >> + > >> + np = of_get_compatible_child(np, "ti,wl1251"); > >> + if (np) { > >> + /* > >> + * We have TI wl1251 attached to MMC3. Pass this information to > >> + * SDIO core because it can't be probed by normal methods. > >> + */ > >> + > >> + dev_info(host->dev, "found wl1251\n"); > >> + card->quirks |= MMC_QUIRK_NONSTD_SDIO; > >> + card->cccr.wide_bus = 1; > >> + card->cis.vendor = 0x104c; > >> + card->cis.device = 0x9066; > >> + card->cis.blksize = 512; > >> + card->cis.max_dtr = 24000000; > >> + card->ocr = 0x80; > > > > These things should really be figured out by the mmc core during SDIO > > card initialization itself, not via the host ops ->init_card() > > callback. That is just poor hack, which in the long run should go > > away. > > Yes, I agree. > > But I am just the poor guy who is trying to fix really broken code with > as low effort as possible. I see. Thanks for looking at this mess! In general, as long as we improve the code, I am happy to move forward. However, my main concern at this point, is to make sure we get the DT bindings and the DTS files updated correctly. We don't want to come back to this again. > > I don't even have a significant clue what this code is exactly doing and what > the magic values mean. They were setup by pandora_wl1251_init_card() in the > same way so that I have just moved the code here and make it called in (almost) > the same situation. Okay! > > > Moreover, I think we should add a subnode to the host node in the DT, > > to describe the embedded SDIO card, rather than parsing the subnode > > for the SDIO func - as that seems wrong to me. > > You mean a second subnode? > > The wl1251 is the child node of the mmc node and describes the SDIO card. > We just check if it is a wl1251 or e.g. wl1837 or something else or even > no child. The reason why I brought this up, was because there are sometimes cases where an SDIO card is shared between more than one SDIO func. WiFi+Bluetooth for example, but if I am correct, that is not the case for wl1251? That said, I am happy to continue with your approach. > > > To add a subnode for the SDIO card, we already have a binding that I > > think we should extend. Please have a look at > > Documentation/devicetree/bindings/mmc/mmc-card.txt. > > > > If you want an example of how to implement this for your case, do a > > git grep "broken-hpi" in the driver/mmc/core/, I think it will tell > > you more of what I have in mind. > > So while I agree that it should be improved in the long run, we should > IMHO fix the hack first (broken since v4.9!), even if it remains a hack > for now. Improving this part seems to be quite independent and focussed > on the mmc subsystem, while the other patches involve other subsystems. I agree. > > Maybe should we make a REVISIT note in the code? Or add something to > the commit message about the idea how it should be done right? Just add a note that we should move this DT parsing of the subnode to the mmc core, but that we are leaving that as a future improvement. That's good enough. Then I can have a look as a second step, and when I get some time, to move this to the mmc core. However, there is one thing I would like you to add to the series. That is: In the struct omap_hsmmc_platform_data, there is an ->init_card() callback. Beyond the changes of this series, there is no longer any users of that, unless I am mistaken. Going forward, let's make sure it doesn't get used again, so can you please remove it!? [...] Kind regards Uffe
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 952fa4063ff8..03ba80bcf319 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1512,6 +1512,27 @@ static void omap_hsmmc_init_card(struct mmc_host *mmc, struct mmc_card *card) if (mmc_pdata(host)->init_card) mmc_pdata(host)->init_card(card); + else if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) { + struct device_node *np = mmc_dev(mmc)->of_node; + + np = of_get_compatible_child(np, "ti,wl1251"); + if (np) { + /* + * We have TI wl1251 attached to MMC3. Pass this information to + * SDIO core because it can't be probed by normal methods. + */ + + dev_info(host->dev, "found wl1251\n"); + card->quirks |= MMC_QUIRK_NONSTD_SDIO; + card->cccr.wide_bus = 1; + card->cis.vendor = 0x104c; + card->cis.device = 0x9066; + card->cis.blksize = 512; + card->cis.max_dtr = 24000000; + card->ocr = 0x80; + of_node_put(np); + } + } } static void omap_hsmmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
Pandora_wl1251_init_card was used to do special pdata based setup of the sdio mmc interface. This does no longer work with v4.7 and later. A fix requires a device tree based mmc3 setup. Therefore we move the special setup to omap_hsmmc.c instead of calling some pdata supplied init_card function. The new code checks for a DT child node compatible to wl1251 so it will not affect other MMC3 use cases. Fixes: 81eef6ca9201 ("mmc: omap_hsmmc: Use dma_request_chan() for requesting DMA channel") Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> Cc: <stable@vger.kernel.org> # 4.7.0 --- drivers/mmc/host/omap_hsmmc.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)