Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
[...] >> >> I think a better approach is to use the new sdhci-caps* bindings to >> mask those caps that can't be trusted. And then use the generic mmc >> bindings for speed modes instead. >> >> That should be a safer approach, right!? > > Right. > > But, if I know the caps register bits 63-32 are all zero, > I need not explicitly specify sdhci-caps-mask, right? Maybe not. I don't have a strong opinion here, so l am fine with whatever you choose. Kind regards Uffe
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
[...] >> >> I think a better approach is to use the new sdhci-caps* bindings to >> mask those caps that can't be trusted. And then use the generic mmc >> bindings for speed modes instead. >> >> That should be a safer approach, right!? > > Right. > > But, if I know the caps register bits 63-32 are all zero, > I need not explicitly specify sdhci-caps-mask, right? Maybe not. I don't have a strong opinion here, so l am fine with whatever you choose. Kind regards Uffe
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
Hi Ulf, 2016-12-13 16:52 GMT+09:00 Ulf Hansson: > [...] > + +Optional properties: +For eMMC configuration, supported speed modes are not indicated by the SDHCI +Capabilities Register. Instead, the following properties should be specified +if supported. See mmc.txt for details. +- mmc-ddr-1_8v +- mmc-ddr-1_2v +- mmc-hs200-1_8v +- mmc-hs200-1_2v +- mmc-hs400-1_8v +- mmc-hs400-1_2v >>> >>> There's now a property to override SDHCI capabilities register. Maybe >>> you should use that instead? I'll defer to Ulf. >>> >> >> I did not know this new property. >> >> So, now we have two ways to specify MMC speed mode capabilities >> by only touching DT. > > Let me clarify a bit. > > The point with the new bindings is to be able to override *broken* > SDHCI caps bits. So, not only related to the speed modes. Right. So my approach ([1] Add MMC mode flags directly) basically sounds OK. >> >> [1] Add MMC mode flags directly, like I did. >> [2] Use "sdhci-caps-mask" and "sdhci-caps" >> >> >> The problem for [2] is that eMMC capabilities >> do not perfectly correspond to the SDHCI capabilities register. >> >> +- mmc-hs400-1_8v +- mmc-hs400-1_2v >> >> If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400, >> we can use the bit63 of caps for specifying HS400. >> >> But, this is not defined in the SDHCI standard. >> #define SDHCI_SUPPORT_HS400 0x8000 /* Non-standard */ >> >> >> +- mmc-ddr-1_8v >> >> For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR >> from MMC_CAP_UHS_DDR50 (bit34 of caps) >> >> This is not supported in the current code, but >> if this is a good idea, I can send a patch. >> >> +- mmc-ddr-1_2v >> >> This does not have the corresponding bit, but >> 1.2V is not commonly used, so this is not a fatal problem. >> >> >> >> What I can do at most now, is to delete the >> Optional properties section entirely >> so users can choose [1] or [2] as they like. >> > > I think a better approach is to use the new sdhci-caps* bindings to > mask those caps that can't be trusted. And then use the generic mmc > bindings for speed modes instead. > > That should be a safer approach, right!? Right. But, if I know the caps register bits 63-32 are all zero, I need not explicitly specify sdhci-caps-mask, right? -- Best Regards Masahiro Yamada
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
Hi Ulf, 2016-12-13 16:52 GMT+09:00 Ulf Hansson : > [...] > + +Optional properties: +For eMMC configuration, supported speed modes are not indicated by the SDHCI +Capabilities Register. Instead, the following properties should be specified +if supported. See mmc.txt for details. +- mmc-ddr-1_8v +- mmc-ddr-1_2v +- mmc-hs200-1_8v +- mmc-hs200-1_2v +- mmc-hs400-1_8v +- mmc-hs400-1_2v >>> >>> There's now a property to override SDHCI capabilities register. Maybe >>> you should use that instead? I'll defer to Ulf. >>> >> >> I did not know this new property. >> >> So, now we have two ways to specify MMC speed mode capabilities >> by only touching DT. > > Let me clarify a bit. > > The point with the new bindings is to be able to override *broken* > SDHCI caps bits. So, not only related to the speed modes. Right. So my approach ([1] Add MMC mode flags directly) basically sounds OK. >> >> [1] Add MMC mode flags directly, like I did. >> [2] Use "sdhci-caps-mask" and "sdhci-caps" >> >> >> The problem for [2] is that eMMC capabilities >> do not perfectly correspond to the SDHCI capabilities register. >> >> +- mmc-hs400-1_8v +- mmc-hs400-1_2v >> >> If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400, >> we can use the bit63 of caps for specifying HS400. >> >> But, this is not defined in the SDHCI standard. >> #define SDHCI_SUPPORT_HS400 0x8000 /* Non-standard */ >> >> >> +- mmc-ddr-1_8v >> >> For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR >> from MMC_CAP_UHS_DDR50 (bit34 of caps) >> >> This is not supported in the current code, but >> if this is a good idea, I can send a patch. >> >> +- mmc-ddr-1_2v >> >> This does not have the corresponding bit, but >> 1.2V is not commonly used, so this is not a fatal problem. >> >> >> >> What I can do at most now, is to delete the >> Optional properties section entirely >> so users can choose [1] or [2] as they like. >> > > I think a better approach is to use the new sdhci-caps* bindings to > mask those caps that can't be trusted. And then use the generic mmc > bindings for speed modes instead. > > That should be a safer approach, right!? Right. But, if I know the caps register bits 63-32 are all zero, I need not explicitly specify sdhci-caps-mask, right? -- Best Regards Masahiro Yamada
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
[...] >>> + >>> +Optional properties: >>> +For eMMC configuration, supported speed modes are not indicated by the >>> SDHCI >>> +Capabilities Register. Instead, the following properties should be >>> specified >>> +if supported. See mmc.txt for details. >>> +- mmc-ddr-1_8v >>> +- mmc-ddr-1_2v >>> +- mmc-hs200-1_8v >>> +- mmc-hs200-1_2v >>> +- mmc-hs400-1_8v >>> +- mmc-hs400-1_2v >> >> There's now a property to override SDHCI capabilities register. Maybe >> you should use that instead? I'll defer to Ulf. >> > > I did not know this new property. > > So, now we have two ways to specify MMC speed mode capabilities > by only touching DT. Let me clarify a bit. The point with the new bindings is to be able to override *broken* SDHCI caps bits. So, not only related to the speed modes. > > [1] Add MMC mode flags directly, like I did. > [2] Use "sdhci-caps-mask" and "sdhci-caps" > > > The problem for [2] is that eMMC capabilities > do not perfectly correspond to the SDHCI capabilities register. > > >>> +- mmc-hs400-1_8v >>> +- mmc-hs400-1_2v > > If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400, > we can use the bit63 of caps for specifying HS400. > > But, this is not defined in the SDHCI standard. > #define SDHCI_SUPPORT_HS400 0x8000 /* Non-standard */ > > > >>> +- mmc-ddr-1_8v > > For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR > from MMC_CAP_UHS_DDR50 (bit34 of caps) > > This is not supported in the current code, but > if this is a good idea, I can send a patch. > > >>> +- mmc-ddr-1_2v > > This does not have the corresponding bit, but > 1.2V is not commonly used, so this is not a fatal problem. > > > > What I can do at most now, is to delete the > Optional properties section entirely > so users can choose [1] or [2] as they like. > I think a better approach is to use the new sdhci-caps* bindings to mask those caps that can't be trusted. And then use the generic mmc bindings for speed modes instead. That should be a safer approach, right!? Kind regards Uffe
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
[...] >>> + >>> +Optional properties: >>> +For eMMC configuration, supported speed modes are not indicated by the >>> SDHCI >>> +Capabilities Register. Instead, the following properties should be >>> specified >>> +if supported. See mmc.txt for details. >>> +- mmc-ddr-1_8v >>> +- mmc-ddr-1_2v >>> +- mmc-hs200-1_8v >>> +- mmc-hs200-1_2v >>> +- mmc-hs400-1_8v >>> +- mmc-hs400-1_2v >> >> There's now a property to override SDHCI capabilities register. Maybe >> you should use that instead? I'll defer to Ulf. >> > > I did not know this new property. > > So, now we have two ways to specify MMC speed mode capabilities > by only touching DT. Let me clarify a bit. The point with the new bindings is to be able to override *broken* SDHCI caps bits. So, not only related to the speed modes. > > [1] Add MMC mode flags directly, like I did. > [2] Use "sdhci-caps-mask" and "sdhci-caps" > > > The problem for [2] is that eMMC capabilities > do not perfectly correspond to the SDHCI capabilities register. > > >>> +- mmc-hs400-1_8v >>> +- mmc-hs400-1_2v > > If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400, > we can use the bit63 of caps for specifying HS400. > > But, this is not defined in the SDHCI standard. > #define SDHCI_SUPPORT_HS400 0x8000 /* Non-standard */ > > > >>> +- mmc-ddr-1_8v > > For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR > from MMC_CAP_UHS_DDR50 (bit34 of caps) > > This is not supported in the current code, but > if this is a good idea, I can send a patch. > > >>> +- mmc-ddr-1_2v > > This does not have the corresponding bit, but > 1.2V is not commonly used, so this is not a fatal problem. > > > > What I can do at most now, is to delete the > Optional properties section entirely > so users can choose [1] or [2] as they like. > I think a better approach is to use the new sdhci-caps* bindings to mask those caps that can't be trusted. And then use the generic mmc bindings for speed modes instead. That should be a safer approach, right!? Kind regards Uffe
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
Hi Rob. 2016-12-13 2:14 GMT+09:00 Rob Herring: >> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt >> b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt >> new file mode 100644 >> index 000..750374f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt >> @@ -0,0 +1,30 @@ >> +* Cadence SD/SDIO/eMMC Host Controller >> + >> +Required properties: >> +- compatible: should be "cdns,sd4hc". > > Needs SoC specific compatible strings too. I remember you mentioned the vendor prefix stands for the SoC vendor, not the IP vendor. The compatible prefixed with the IP vendor is prepared for a fallback. I will add "socionext,sd4hc". >> +- reg: offset and length of the register set for the device. >> +- interrupts: a single interrupt specifier. >> +- clocks: phandle to the input clock. >> + >> +Optional properties: >> +For eMMC configuration, supported speed modes are not indicated by the SDHCI >> +Capabilities Register. Instead, the following properties should be >> specified >> +if supported. See mmc.txt for details. >> +- mmc-ddr-1_8v >> +- mmc-ddr-1_2v >> +- mmc-hs200-1_8v >> +- mmc-hs200-1_2v >> +- mmc-hs400-1_8v >> +- mmc-hs400-1_2v > > There's now a property to override SDHCI capabilities register. Maybe > you should use that instead? I'll defer to Ulf. > I did not know this new property. So, now we have two ways to specify MMC speed mode capabilities by only touching DT. [1] Add MMC mode flags directly, like I did. [2] Use "sdhci-caps-mask" and "sdhci-caps" The problem for [2] is that eMMC capabilities do not perfectly correspond to the SDHCI capabilities register. >> +- mmc-hs400-1_8v >> +- mmc-hs400-1_2v If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400, we can use the bit63 of caps for specifying HS400. But, this is not defined in the SDHCI standard. #define SDHCI_SUPPORT_HS400 0x8000 /* Non-standard */ >> +- mmc-ddr-1_8v For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR from MMC_CAP_UHS_DDR50 (bit34 of caps) This is not supported in the current code, but if this is a good idea, I can send a patch. >> +- mmc-ddr-1_2v This does not have the corresponding bit, but 1.2V is not commonly used, so this is not a fatal problem. What I can do at most now, is to delete the Optional properties section entirely so users can choose [1] or [2] as they like. -- Best Regards Masahiro Yamada
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
Hi Rob. 2016-12-13 2:14 GMT+09:00 Rob Herring : >> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt >> b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt >> new file mode 100644 >> index 000..750374f >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt >> @@ -0,0 +1,30 @@ >> +* Cadence SD/SDIO/eMMC Host Controller >> + >> +Required properties: >> +- compatible: should be "cdns,sd4hc". > > Needs SoC specific compatible strings too. I remember you mentioned the vendor prefix stands for the SoC vendor, not the IP vendor. The compatible prefixed with the IP vendor is prepared for a fallback. I will add "socionext,sd4hc". >> +- reg: offset and length of the register set for the device. >> +- interrupts: a single interrupt specifier. >> +- clocks: phandle to the input clock. >> + >> +Optional properties: >> +For eMMC configuration, supported speed modes are not indicated by the SDHCI >> +Capabilities Register. Instead, the following properties should be >> specified >> +if supported. See mmc.txt for details. >> +- mmc-ddr-1_8v >> +- mmc-ddr-1_2v >> +- mmc-hs200-1_8v >> +- mmc-hs200-1_2v >> +- mmc-hs400-1_8v >> +- mmc-hs400-1_2v > > There's now a property to override SDHCI capabilities register. Maybe > you should use that instead? I'll defer to Ulf. > I did not know this new property. So, now we have two ways to specify MMC speed mode capabilities by only touching DT. [1] Add MMC mode flags directly, like I did. [2] Use "sdhci-caps-mask" and "sdhci-caps" The problem for [2] is that eMMC capabilities do not perfectly correspond to the SDHCI capabilities register. >> +- mmc-hs400-1_8v >> +- mmc-hs400-1_2v If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400, we can use the bit63 of caps for specifying HS400. But, this is not defined in the SDHCI standard. #define SDHCI_SUPPORT_HS400 0x8000 /* Non-standard */ >> +- mmc-ddr-1_8v For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR from MMC_CAP_UHS_DDR50 (bit34 of caps) This is not supported in the current code, but if this is a good idea, I can send a patch. >> +- mmc-ddr-1_2v This does not have the corresponding bit, but 1.2V is not commonly used, so this is not a fatal problem. What I can do at most now, is to delete the Optional properties section entirely so users can choose [1] or [2] as they like. -- Best Regards Masahiro Yamada
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
On Thu, Dec 08, 2016 at 09:50:55PM +0900, Masahiro Yamada wrote: > Add a driver for the Cadence SD4HC SD/SDIO/eMMC Controller. > > For SD, it basically relies on the SDHCI standard code. > For eMMC, this driver provides some callbacks to support the > hardware part that is specific to this IP design. > > Signed-off-by: Masahiro Yamada> Acked-by: Adrian Hunter > --- > > Changes in v5: > - Fix calculation of the center of the tuned window > > Changes in v4: > - Override mmc_host_ops.execute_tuning instead of the > .platform_execute_tuning implementation > > Changes in v3: > - Remove unneeded explanation about HRS and SRS from DT binding; > the offsets to HRS/SRS are fixed for this hardware and this is > quite normal, like each hardware has a fixed register view except > the register base. The detailed register map is what the driver > cares about, so no need to explain it in the binding. > > Changes in v2: > - Remove unnecessary "select MMC_SDHCI_IO_ACCESSORS" > > .../devicetree/bindings/mmc/sdhci-cadence.txt | 30 +++ > drivers/mmc/host/Kconfig | 11 + > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-cadence.c | 283 > + > 4 files changed, 325 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > create mode 100644 drivers/mmc/host/sdhci-cadence.c > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > new file mode 100644 > index 000..750374f > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > @@ -0,0 +1,30 @@ > +* Cadence SD/SDIO/eMMC Host Controller > + > +Required properties: > +- compatible: should be "cdns,sd4hc". Needs SoC specific compatible strings too. > +- reg: offset and length of the register set for the device. > +- interrupts: a single interrupt specifier. > +- clocks: phandle to the input clock. > + > +Optional properties: > +For eMMC configuration, supported speed modes are not indicated by the SDHCI > +Capabilities Register. Instead, the following properties should be specified > +if supported. See mmc.txt for details. > +- mmc-ddr-1_8v > +- mmc-ddr-1_2v > +- mmc-hs200-1_8v > +- mmc-hs200-1_2v > +- mmc-hs400-1_8v > +- mmc-hs400-1_2v There's now a property to override SDHCI capabilities register. Maybe you should use that instead? I'll defer to Ulf. > + > +Example: > + emmc: sdhci@5a00 { > + compatible = "cdns,sd4hc"; > + reg = <0x5a00 0x400>; > + interrupts = <0 78 4>; > + clocks = < 4>; > + bus-width = <8>; > + mmc-ddr-1_8v; > + mmc-hs200-1_8v; > + mmc-hs400-1_8v; > + };
Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
On Thu, Dec 08, 2016 at 09:50:55PM +0900, Masahiro Yamada wrote: > Add a driver for the Cadence SD4HC SD/SDIO/eMMC Controller. > > For SD, it basically relies on the SDHCI standard code. > For eMMC, this driver provides some callbacks to support the > hardware part that is specific to this IP design. > > Signed-off-by: Masahiro Yamada > Acked-by: Adrian Hunter > --- > > Changes in v5: > - Fix calculation of the center of the tuned window > > Changes in v4: > - Override mmc_host_ops.execute_tuning instead of the > .platform_execute_tuning implementation > > Changes in v3: > - Remove unneeded explanation about HRS and SRS from DT binding; > the offsets to HRS/SRS are fixed for this hardware and this is > quite normal, like each hardware has a fixed register view except > the register base. The detailed register map is what the driver > cares about, so no need to explain it in the binding. > > Changes in v2: > - Remove unnecessary "select MMC_SDHCI_IO_ACCESSORS" > > .../devicetree/bindings/mmc/sdhci-cadence.txt | 30 +++ > drivers/mmc/host/Kconfig | 11 + > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-cadence.c | 283 > + > 4 files changed, 325 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > create mode 100644 drivers/mmc/host/sdhci-cadence.c > > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > new file mode 100644 > index 000..750374f > --- /dev/null > +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt > @@ -0,0 +1,30 @@ > +* Cadence SD/SDIO/eMMC Host Controller > + > +Required properties: > +- compatible: should be "cdns,sd4hc". Needs SoC specific compatible strings too. > +- reg: offset and length of the register set for the device. > +- interrupts: a single interrupt specifier. > +- clocks: phandle to the input clock. > + > +Optional properties: > +For eMMC configuration, supported speed modes are not indicated by the SDHCI > +Capabilities Register. Instead, the following properties should be specified > +if supported. See mmc.txt for details. > +- mmc-ddr-1_8v > +- mmc-ddr-1_2v > +- mmc-hs200-1_8v > +- mmc-hs200-1_2v > +- mmc-hs400-1_8v > +- mmc-hs400-1_2v There's now a property to override SDHCI capabilities register. Maybe you should use that instead? I'll defer to Ulf. > + > +Example: > + emmc: sdhci@5a00 { > + compatible = "cdns,sd4hc"; > + reg = <0x5a00 0x400>; > + interrupts = <0 78 4>; > + clocks = < 4>; > + bus-width = <8>; > + mmc-ddr-1_8v; > + mmc-hs200-1_8v; > + mmc-hs400-1_8v; > + };