| Message ID | 20221202031330.94130-2-doug@schmorgal.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-pxav2: Add support for PXA168 | expand |
On 2/12/22 05:13, Doug Brown wrote: > Add a new compatible string for the version 1 controller used in the > PXA168, along with necessary quirks. Use a separate ops struct in > preparation for a silicon bug workaround only necessary on V1. > > Signed-off-by: Doug Brown <doug@schmorgal.com> > --- > drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c > index f18906b5575f..2f9fa0ecbddd 100644 > --- a/drivers/mmc/host/sdhci-pxav2.c > +++ b/drivers/mmc/host/sdhci-pxav2.c > @@ -101,6 +101,14 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width) > writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); > } > > +static const struct sdhci_ops pxav1_sdhci_ops = { > + .set_clock = sdhci_set_clock, > + .get_max_clock = sdhci_pltfm_clk_get_max_clock, > + .set_bus_width = pxav2_mmc_set_bus_width, > + .reset = pxav2_reset, > + .set_uhs_signaling = sdhci_set_uhs_signaling, > +}; > + > static const struct sdhci_ops pxav2_sdhci_ops = { > .set_clock = sdhci_set_clock, > .get_max_clock = sdhci_pltfm_clk_get_max_clock, > @@ -114,6 +122,9 @@ static const struct of_device_id sdhci_pxav2_of_match[] = { > { > .compatible = "mrvl,pxav2-mmc", > }, > + { > + .compatible = "mrvl,pxav1-mmc", > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, sdhci_pxav2_of_match); > @@ -208,7 +219,12 @@ static int sdhci_pxav2_probe(struct platform_device *pdev) > host->mmc->pm_caps |= pdata->pm_caps; > } > > - host->ops = &pxav2_sdhci_ops; > + if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) { > + host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE; > + host->ops = &pxav1_sdhci_ops; > + } else { > + host->ops = &pxav2_sdhci_ops; > + } It would be better to put the information above in a structure and get it with of_device_get_match_data() (instead of of_match_device). Also drivers typically assume there is always a match since that is the only way the driver ->probe() will get run. > > ret = sdhci_add_host(host); > if (ret)
Hi Adrian, On 12/22/2022 8:03 AM, Adrian Hunter wrote: > On 2/12/22 05:13, Doug Brown wrote: >> Add a new compatible string for the version 1 controller used in the >> PXA168, along with necessary quirks. Use a separate ops struct in >> preparation for a silicon bug workaround only necessary on V1. >> >> Signed-off-by: Doug Brown <doug@schmorgal.com> >> --- >> drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c >> index f18906b5575f..2f9fa0ecbddd 100644 >> --- a/drivers/mmc/host/sdhci-pxav2.c >> +++ b/drivers/mmc/host/sdhci-pxav2.c >> @@ -101,6 +101,14 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width) >> writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); >> } >> >> +static const struct sdhci_ops pxav1_sdhci_ops = { >> + .set_clock = sdhci_set_clock, >> + .get_max_clock = sdhci_pltfm_clk_get_max_clock, >> + .set_bus_width = pxav2_mmc_set_bus_width, >> + .reset = pxav2_reset, >> + .set_uhs_signaling = sdhci_set_uhs_signaling, >> +}; >> + >> static const struct sdhci_ops pxav2_sdhci_ops = { >> .set_clock = sdhci_set_clock, >> .get_max_clock = sdhci_pltfm_clk_get_max_clock, >> @@ -114,6 +122,9 @@ static const struct of_device_id sdhci_pxav2_of_match[] = { >> { >> .compatible = "mrvl,pxav2-mmc", >> }, >> + { >> + .compatible = "mrvl,pxav1-mmc", >> + }, >> {}, >> }; >> MODULE_DEVICE_TABLE(of, sdhci_pxav2_of_match); >> @@ -208,7 +219,12 @@ static int sdhci_pxav2_probe(struct platform_device *pdev) >> host->mmc->pm_caps |= pdata->pm_caps; >> } >> >> - host->ops = &pxav2_sdhci_ops; >> + if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) { >> + host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE; >> + host->ops = &pxav1_sdhci_ops; >> + } else { >> + host->ops = &pxav2_sdhci_ops; >> + } > > It would be better to put the information above in a structure and > get it with of_device_get_match_data() (instead of of_match_device). > Also drivers typically assume there is always a match since that > is the only way the driver ->probe() will get run. Thanks for all of your great feedback on this series. That makes sense. I did have one question about this suggestion. There are other parts of sdhci_pxav2_probe() that don't assume there was a match so that it can be set up the old way as a platform_device without CONFIG_OF. I was trying to preserve compatibility by defaulting to pxav2_sdhci_ops if it was set up as a platform_device. Is it all right if I leave a fallback in place for that, or should I just end compatibility with the old way at this point and assume a match in all cases? I don't see any legacy board files that use this driver. > >> >> ret = sdhci_add_host(host); >> if (ret) > Thanks, Doug
On 26/12/22 22:35, Doug Brown wrote: > Hi Adrian, > > On 12/22/2022 8:03 AM, Adrian Hunter wrote: >> On 2/12/22 05:13, Doug Brown wrote: >>> Add a new compatible string for the version 1 controller used in the >>> PXA168, along with necessary quirks. Use a separate ops struct in >>> preparation for a silicon bug workaround only necessary on V1. >>> >>> Signed-off-by: Doug Brown <doug@schmorgal.com> >>> --- >>> drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c >>> index f18906b5575f..2f9fa0ecbddd 100644 >>> --- a/drivers/mmc/host/sdhci-pxav2.c >>> +++ b/drivers/mmc/host/sdhci-pxav2.c >>> @@ -101,6 +101,14 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width) >>> writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); >>> } >>> +static const struct sdhci_ops pxav1_sdhci_ops = { >>> + .set_clock = sdhci_set_clock, >>> + .get_max_clock = sdhci_pltfm_clk_get_max_clock, >>> + .set_bus_width = pxav2_mmc_set_bus_width, >>> + .reset = pxav2_reset, >>> + .set_uhs_signaling = sdhci_set_uhs_signaling, >>> +}; >>> + >>> static const struct sdhci_ops pxav2_sdhci_ops = { >>> .set_clock = sdhci_set_clock, >>> .get_max_clock = sdhci_pltfm_clk_get_max_clock, >>> @@ -114,6 +122,9 @@ static const struct of_device_id sdhci_pxav2_of_match[] = { >>> { >>> .compatible = "mrvl,pxav2-mmc", >>> }, >>> + { >>> + .compatible = "mrvl,pxav1-mmc", >>> + }, >>> {}, >>> }; >>> MODULE_DEVICE_TABLE(of, sdhci_pxav2_of_match); >>> @@ -208,7 +219,12 @@ static int sdhci_pxav2_probe(struct platform_device *pdev) >>> host->mmc->pm_caps |= pdata->pm_caps; >>> } >>> - host->ops = &pxav2_sdhci_ops; >>> + if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) { >>> + host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE; >>> + host->ops = &pxav1_sdhci_ops; >>> + } else { >>> + host->ops = &pxav2_sdhci_ops; >>> + } >> >> It would be better to put the information above in a structure and >> get it with of_device_get_match_data() (instead of of_match_device). >> Also drivers typically assume there is always a match since that >> is the only way the driver ->probe() will get run. > > Thanks for all of your great feedback on this series. That makes sense. > I did have one question about this suggestion. There are other parts of > sdhci_pxav2_probe() that don't assume there was a match so that it can > be set up the old way as a platform_device without CONFIG_OF. I was > trying to preserve compatibility by defaulting to pxav2_sdhci_ops if > it was set up as a platform_device. Is it all right if I leave a > fallback in place for that, or should I just end compatibility with the > old way at this point and assume a match in all cases? I don't see any > legacy board files that use this driver. I didn't think about matching by name. As you say, there doesn't seem to be anything in mainline. I will leave it to you to decide. > >> >>> ret = sdhci_add_host(host); >>> if (ret) >> > > Thanks, > Doug
diff --git a/drivers/mmc/host/sdhci-pxav2.c b/drivers/mmc/host/sdhci-pxav2.c index f18906b5575f..2f9fa0ecbddd 100644 --- a/drivers/mmc/host/sdhci-pxav2.c +++ b/drivers/mmc/host/sdhci-pxav2.c @@ -101,6 +101,14 @@ static void pxav2_mmc_set_bus_width(struct sdhci_host *host, int width) writeb(ctrl, host->ioaddr + SDHCI_HOST_CONTROL); } +static const struct sdhci_ops pxav1_sdhci_ops = { + .set_clock = sdhci_set_clock, + .get_max_clock = sdhci_pltfm_clk_get_max_clock, + .set_bus_width = pxav2_mmc_set_bus_width, + .reset = pxav2_reset, + .set_uhs_signaling = sdhci_set_uhs_signaling, +}; + static const struct sdhci_ops pxav2_sdhci_ops = { .set_clock = sdhci_set_clock, .get_max_clock = sdhci_pltfm_clk_get_max_clock, @@ -114,6 +122,9 @@ static const struct of_device_id sdhci_pxav2_of_match[] = { { .compatible = "mrvl,pxav2-mmc", }, + { + .compatible = "mrvl,pxav1-mmc", + }, {}, }; MODULE_DEVICE_TABLE(of, sdhci_pxav2_of_match); @@ -208,7 +219,12 @@ static int sdhci_pxav2_probe(struct platform_device *pdev) host->mmc->pm_caps |= pdata->pm_caps; } - host->ops = &pxav2_sdhci_ops; + if (match && of_device_is_compatible(dev->of_node, "mrvl,pxav1-mmc")) { + host->quirks |= SDHCI_QUIRK_NO_BUSY_IRQ | SDHCI_QUIRK_32BIT_DMA_SIZE; + host->ops = &pxav1_sdhci_ops; + } else { + host->ops = &pxav2_sdhci_ops; + } ret = sdhci_add_host(host); if (ret)
Add a new compatible string for the version 1 controller used in the PXA168, along with necessary quirks. Use a separate ops struct in preparation for a silicon bug workaround only necessary on V1. Signed-off-by: Doug Brown <doug@schmorgal.com> --- drivers/mmc/host/sdhci-pxav2.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)