| Message ID | 1589472657-3930-9-git-send-email-amittomer25@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MMC and DMA support for Actions S700 | expand |
On 14/05/2020 17:10, Amit Singh Tomar wrote: Hi, > This commits adds support for MMC controllers present on Actions S700 SoC, > there are 3 MMC controllers in this SoC which can be used for accessing > SD/EMMC/SDIO cards. > > Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > --- > Changes since RFC: > * No change. > --- > arch/arm64/boot/dts/actions/s700.dtsi | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi > index 56f2f84812cb..3f1fc3e48415 100644 > --- a/arch/arm64/boot/dts/actions/s700.dtsi > +++ b/arch/arm64/boot/dts/actions/s700.dtsi > @@ -258,5 +258,38 @@ > dma-requests = <44>; > clocks = <&cmu CLK_DMAC>; > }; > + > + mmc0: mmc@e0210000 { > + compatible = "actions,owl-mmc"; I was wondering if we should add a SoC specific compatible here, to be on the safe side. The BSP driver seems to differentiate between S900 and S700, although it looks like only to cover some misplaced platform setup. But if we later find a problem, the DTs stay the same, and the driver can easily be fixed. So, using "actions,s700-mmc", "actions,owl-mmc" here, adding this combo to the binding, but leaving the driver alone for now. Cheers, Andre > + reg = <0x0 0xe0210000 0x0 0x4000>; > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cmu CLK_SD0>; > + resets = <&cmu RESET_SD0>; > + dmas = <&dma 2>; > + dma-names = "mmc"; > + status = "disabled"; > + }; > + > + mmc1: mmc@e0214000 { > + compatible = "actions,owl-mmc"; > + reg = <0x0 0xe0214000 0x0 0x4000>; > + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cmu CLK_SD1>; > + resets = <&cmu RESET_SD1>; > + dmas = <&dma 3>; > + dma-names = "mmc"; > + status = "disabled"; > + }; > + > + mmc2: mmc@e0218000 { > + compatible = "actions,owl-mmc"; > + reg = <0x0 0xe0218000 0x0 0x4000>; > + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cmu CLK_SD2>; > + resets = <&cmu RESET_SD2>; > + dmas = <&dma 4>; > + dma-names = "mmc"; > + status = "disabled"; > + }; > }; > }; >
Hi, > I was wondering if we should add a SoC specific compatible here, to be > on the safe side. The BSP driver seems to differentiate between S900 and > S700, although it looks like only to cover some misplaced platform setup. > > But if we later find a problem, the DTs stay the same, and the driver > can easily be fixed. > > So, using "actions,s700-mmc", "actions,owl-mmc" here, adding this combo > to the binding, but leaving the driver alone for now. I think, it can be a good idea to have this extra compatible string that may be needed when MMC driver evolves to support more features which requires data to be differentiated based on SoC. Thanks Amit
Hi, > So, using "actions,s700-mmc", "actions,owl-mmc" here, adding this combo > to the binding, but leaving the driver alone for now. But if we leave this new string from driver , there would be DT validation issue. Are we okay with it ? Thanks -Amit
On 17/05/2020 12:56, Amit Tomer wrote: > Hi, > >> So, using "actions,s700-mmc", "actions,owl-mmc" here, adding this combo >> to the binding, but leaving the driver alone for now. > > But if we leave this new string from driver , there would be DT > validation issue. I don't understand what this has to do with the driver, but I asked above to also change the binding, allowing this compatible string combination. Cheers, Andre > Are we okay with it ? > > Thanks > -Amit >
> I don't understand what this has to do with the driver, but I asked > above to also change the binding, allowing this compatible string > combination. if we add these two strings "actions,s700-mmc", "actions,owl-mmc" to dts file and leave the driver as it. Wouldn't this be mismatch(as driver only has "actions,owl-mmc" and DTS has two strings). Shouldn't that be concerned about ? Thanks -Amit
On 17/05/2020 18:12, Amit Tomer wrote: >> I don't understand what this has to do with the driver, but I asked >> above to also change the binding, allowing this compatible string >> combination. > if we add these two strings "actions,s700-mmc", "actions,owl-mmc" to dts file > and leave the driver as it. Wouldn't this be mismatch(as driver only > has "actions,owl-mmc" > and DTS has two strings). > > Shouldn't that be concerned about ? I recommend reading the DT spec, chapter 2.3.1 "compatible": https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf Cheers, Andre
Hi, > I recommend reading the DT spec, chapter 2.3.1 "compatible": > https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf My point is more about, DT validation is not happy about this situation. For instance when I run dt-validate, do see following: /home/amit/work/kernel_work/linux/arch/arm64/boot/dts/actions/s700-cubieboard7.dt.yaml: mmc@e0210000: compatible: Additional items are not allowed ('actions,s700-mmc' was unexpected) and I am not sure if this is because DT and driver has different number of compatible strings. Thanks Amit
On 0518, Amit Tomer wrote: > Hi, > > > I recommend reading the DT spec, chapter 2.3.1 "compatible": > > https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf > > My point is more about, DT validation is not happy about this situation. > For instance when I run dt-validate, do see following: > > /home/amit/work/kernel_work/linux/arch/arm64/boot/dts/actions/s700-cubieboard7.dt.yaml: > mmc@e0210000: compatible: Additional items are not allowed > ('actions,s700-mmc' was unexpected) > > and I am not sure if this is because DT and driver has different > number of compatible strings. > Yeah, the DT YAML validation tool doesn't allow this. And there were patches removing the additional compatible from DTS. I don't know if the DT fallback is discouraged or not. Anyway, is enough investigation done to justify that we need SoC specific tweaks in the driver? I think I did look into the downstream code for s700/s500 while upstreaming this driver and convinced myself that the same driver could be reused without modifications. Thanks, Mani > Thanks > Amit
On 18/05/2020 07:17, Manivannan Sadhasivam wrote: > On 0518, Amit Tomer wrote: >> Hi, >> >>> I recommend reading the DT spec, chapter 2.3.1 "compatible": >>> https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf >> >> My point is more about, DT validation is not happy about this situation. >> For instance when I run dt-validate, do see following: >> >> /home/amit/work/kernel_work/linux/arch/arm64/boot/dts/actions/s700-cubieboard7.dt.yaml: >> mmc@e0210000: compatible: Additional items are not allowed >> ('actions,s700-mmc' was unexpected) >> >> and I am not sure if this is because DT and driver has different >> number of compatible strings. This has nothing to do with the driver or the actual .dts(i), that's purely a binding issue. > Yeah, the DT YAML validation tool doesn't allow this. And there were patches > removing the additional compatible from DTS. There are tons of examples of how this is done in the .yaml bindings: properties: compatible: oneOf: - const: actions,owl-mmc - items: - const: action,s700-mmc - const: actions,owl-mmc Adding more alternatives would replace the lines after items: - enum - action,s700-mmc - action,s900-mmc - const: actions,owl-mmc > I don't know if the DT fallback is discouraged or not. I don't know if there is an "official" statement on this, but last thing I heard, adding SoC specific compatibles to generic fallback strings was encouraged. Hence my proposal, to add one. > Anyway, is enough investigation done to justify that we need SoC specific tweaks > in the driver? I think I did look into the downstream code for s700/s500 while > upstreaming this driver and convinced myself that the same driver could be > reused without modifications. It wouldn't be the first time some tiny incompatibility would be discovered later, also there is always the chance of a hardware errata. So better safe than sorry. Cheers, Andre.
Hi, Am 18.05.20 um 10:29 schrieb André Przywara: > On 18/05/2020 07:17, Manivannan Sadhasivam wrote: >> I don't know if the DT fallback is discouraged or not. > > I don't know if there is an "official" statement on this, but last thing > I heard, adding SoC specific compatibles to generic fallback strings was > encouraged. Hence my proposal, to add one. I believe the official guidance would be to never be too generic in the first place. I.e., prefer s500 (oldest model tested) over generic owl. But now that we have it, prepending a more specific one (rather than replacing it) is the only way to go. In that case the binding needs to be changed to allow both the old and the recommended new variant, as André points out. Please add a comment to help bindings readers choose which of the two to adopt then. Amit, next time please quote errors you see right away, that could've spared a handful of messages discussing about the driver when it was in fact just a bindings issue (which you had been asked to fix by André). Regards, Andreas
diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi index 56f2f84812cb..3f1fc3e48415 100644 --- a/arch/arm64/boot/dts/actions/s700.dtsi +++ b/arch/arm64/boot/dts/actions/s700.dtsi @@ -258,5 +258,38 @@ dma-requests = <44>; clocks = <&cmu CLK_DMAC>; }; + + mmc0: mmc@e0210000 { + compatible = "actions,owl-mmc"; + reg = <0x0 0xe0210000 0x0 0x4000>; + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cmu CLK_SD0>; + resets = <&cmu RESET_SD0>; + dmas = <&dma 2>; + dma-names = "mmc"; + status = "disabled"; + }; + + mmc1: mmc@e0214000 { + compatible = "actions,owl-mmc"; + reg = <0x0 0xe0214000 0x0 0x4000>; + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cmu CLK_SD1>; + resets = <&cmu RESET_SD1>; + dmas = <&dma 3>; + dma-names = "mmc"; + status = "disabled"; + }; + + mmc2: mmc@e0218000 { + compatible = "actions,owl-mmc"; + reg = <0x0 0xe0218000 0x0 0x4000>; + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cmu CLK_SD2>; + resets = <&cmu RESET_SD2>; + dmas = <&dma 4>; + dma-names = "mmc"; + status = "disabled"; + }; }; };
This commits adds support for MMC controllers present on Actions S700 SoC, there are 3 MMC controllers in this SoC which can be used for accessing SD/EMMC/SDIO cards. Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> --- Changes since RFC: * No change. --- arch/arm64/boot/dts/actions/s700.dtsi | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)