mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
Subsystems (4)
v2 cover mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
This patch adds support for the APM X-Gene SoC SDHC controller to Arasan SDHCI driver.
v2:
* Update binding documentation
* Change compatible string from "xgene,arasan" to "apm,arasan"
---
Loc Ho (3):
Documentation: Update Arasan SDHC documentation for the APM X-Gene
SoC SDHC DTS binding
mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI
driver
arm64: Add APM X-Gene SoC SDHC controller DTS entry
.../devicetree/bindings/mmc/arasan,sdhci.txt | 16 ++- arch/arm64/boot/dts/apm-storm.dtsi | 8 ++ drivers/mmc/host/Kconfig | 4 +- drivers/mmc/host/sdhci-of-arasan.c | 126 ++++++++++++++++++-- 4 files changed, 140 insertions(+), 14 deletions(-)
PATCH v2 1/3 Documentation: Update Arasan SDHC documentation for the APM X-Gene SoC SDHC DTS binding
This patch updates Arasan SDHC documentation for the APM X-Gene SoC SDHC controller DTS binding.
--- .../devicetree/bindings/mmc/arasan,sdhci.txt | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/mmc/arasan,sdhci.txt +12 -4
@@ -8,14 +8,22 @@ Device Tree Bindings for the Arasan SDHCI Controller [3] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt Required Properties: - - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' - - reg: From mmc bindings: Register location and length. - - clocks: From clock bindings: Handles to clock inputs. - - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb" + - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or + 'apm,arasan,sdhci-8.9a' + - reg: First resource is the SDHC register location and length. + Second optional resource is the bridge translation register location + and length if required. - interrupts: Interrupt specifier - interrupt-parent: Phandle for the interrupt controller that services interrupts for this device. +Optional Properties: + - clocks: For format, refer to clock bindings. It requires two clocks if + specified - AHB clock and SDHC clock. + - clock-names: For format, refer to clock bindings. The clock corresponding + to the AHBC clock must be named "clk_ahb". The clock + corresponding to the SDHC clock must be named "clk_xin". + Example: sdhci at e0100000 { compatible = "arasan,sdhci-8.9a";
Signature · 1 line
1.5.5
PATCH v2 2/3 mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
This patch adds support for the APM X-Gene SoC SDHC controller to Arasan SDHCI driver.
--- drivers/mmc/host/Kconfig | 4 +- drivers/mmc/host/sdhci-of-arasan.c | 126 +++++++++++++++++++++++++++++++++--- 2 files changed, 120 insertions(+), 10 deletions(-)
drivers/mmc/host/Kconfig +3 -1
@@ -108,9 +108,11 @@ config MMC_SDHCI_OF_ARASAN tristate "SDHCI OF support for the Arasan SDHCI controllers" depends on MMC_SDHCI_PLTFM depends on OF + select MMC_SDHCI_IO_ACCESSORS help This selects the Arasan Secure Digital Host Controller Interface - (SDHCI). This hardware is found e.g. in Xilinx' Zynq SoC. + (SDHCI). This hardware is found e.g. in Xilinx' Zynq and X-Gene + SoC. If you have a controller with this interface, say Y or M here.
drivers/mmc/host/sdhci-of-arasan.c +117 -9
181 lines not on this page · raw message
Signature · 1 line
1.5.5
PATCH v2 3/3 arm64: Add APM X-Gene SoC SDHC controller DTS entry
This patch adds APM X-Gene SoC SDHC controller DTS entry.
--- arch/arm64/boot/dts/apm-storm.dtsi | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
arch/arm64/boot/dts/apm-storm.dtsi +8 -0
@@ -436,5 +436,13 @@ phys = <&phy3 0>; phy-names = "sata-phy"; }; + + sdhc0: sdhc at 1c000000 { + compatible = "apm,arasan,sdhci-8.9a"; + reg = <0x0 0x1c000000 0x0 0x100>, + <0x0 0x1f2a0094 0x0 0x10>; + interrupts = <0x0 0x49 0x4>; + no-1-8-v; + }; }; };
Signature · 1 line
1.5.5
PATCH v2 2/3 mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
In reply to Loc HoQuoted 31 lines · Loc Ho
This patch adds support for the APM X-Gene SoC SDHC controller to Arasan SDHCI driver.
Signed-off-by Loc Ho <[email protected]>--- drivers/mmc/host/Kconfig | 4 +- drivers/mmc/host/sdhci-of-arasan.c | 126 +++++++++++++++++++++++++++++++++--- 2 files changed, 120 insertions(+), 10 deletions(-)drivers/mmc/host/Kconfig +3
-1@@ -108,9 +108,11 @@ config MMC_SDHCI_OF_ARASAN tristate "SDHCI OF support for the Arasan SDHCI controllers" depends on MMC_SDHCI_PLTFM depends on OF + select MMC_SDHCI_IO_ACCESSORS help This selects the Arasan Secure Digital Host Controller Interface - (SDHCI). This hardware is found e.g. in Xilinx' Zynq SoC. + (SDHCI). This hardware is found e.g. in Xilinx' Zynq and X-Gene + SoC. If you have a controller with this interface, say Y or M here.drivers/mmc/host/sdhci-of-arasan.c +0
-0
drivers/mmc/host/sdhci-of-arasan.c line 24
Quoted 5 lines
drivers/mmc/host/sdhci-of-arasan.c +2
-0@@ -20,6 +20,8 @@*/ #include <linux/module.h> +#include <linux/dma-mapping.h> +#include <linux/of_device.h>
Keep headers sorted.
Quoted 6 lines
drivers/mmc/host/sdhci-of-arasan.c +0
-0#include "sdhci-pltfm.h" #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c@@ -34,6 +36,19 @@*/ struct sdhci_arasan_data {
drivers/mmc/host/sdhci-of-arasan.c line 47
Quoted 10 lines
drivers/mmc/host/sdhci-of-arasan.c +9
-0struct clk *clk_ahb; +struct platform_device *pdev; +void __iomem *ahb_aim_csr; +const struct sdhci_arasan_ahb_ops *ahb_ops; +}; + +/** +* struct sdhci_arasan_ahb_ops +* @init_ahb Initialize translation bus +* @xlat_addr Set up an 64-bit addressing translation
This definitely breaking kernel-doc format. Please fix.
Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c +4
-0+*/ +struct sdhci_arasan_ahb_ops { +int (*init_ahb)(struct sdhci_arasan_data *data); +void (*xlat_addr)(struct sdhci_arasan_data *data,u64 dma_addr); };static unsigned int sdhci_arasan_get_timeout_clock (struct sdhci_host *host)
drivers/mmc/host/sdhci-of-arasan.c static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) · line 75
Quoted 10 lines
drivers/mmc/host/sdhci-of-arasan.c +7
-0@@ -51,7 +66,21 @@ static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)return freq; } +static void sdhci_arasan_writel (struct sdhci_host *host,u32 val,int reg) +{ +struct sdhci_pltfm_host *pltfm_host =sdhci_priv (host); +struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv; + +if (reg ==SDHCI_DMA_ADDRESS ) { +if (sdhci_arasan->ahb_ops && sdhci_arasan->ahb_ops->xlat_addr)
just one if here.
drivers/mmc/host/sdhci-of-arasan.c static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) · line 80
Quoted 5 lines
drivers/mmc/host/sdhci-of-arasan.c +5
-0+ sdhci_arasan->ahb_ops->xlat_addr (sdhci_arasan, +sg_dma_address (host->data->sg)); + } +writel (val, host->ioaddr + reg); +}
The same code was submitted in v1 and Arnd commented it but you are still keeping it here. Why?
Quoted 7 lines
drivers/mmc/host/sdhci-of-arasan.c +2
-0+static struct sdhci_ops sdhci_arasan_ops = { + .write_l = sdhci_arasan_writel, .get_max_clock = sdhci_pltfm_clk_get_max_clock, .get_timeout_clock = sdhci_arasan_get_timeout_clock, };
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 159
Quoted 10 lines
drivers/mmc/host/sdhci-of-arasan.c +7
-0@@ -121,13 +150,83 @@ static int sdhci_arasan_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend, sdhci_arasan_resume); +static int sdhci_arasan_xgene_init_ahb (struct sdhci_arasan_data *data) +{ +#define AIM_SIZE_CTL_OFFSET 0x00000004 +#define AIM_EN_N_WR (src) (((u32) (src) << 31) & 0x80000000) +#define ARSB_WR (src) (((u32) (src) << 24) & 0x0f000000) +#define AWSB_WR (src) (((u32) (src) << 20) & 0x00f00000) +#define AIM_MASK_N_WR (src) (((u32) (src)) & 0x000fffff)
Remove that one more space between #define and name.
I am not fan of having these defines just here - move them to the top.
Also these macros are used just here at one location. Isn't it better just to define that BITS you want to setup instead of these macros which are hardly to read?
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 170
Quoted 11 lines
drivers/mmc/host/sdhci-of-arasan.c +11
-0+ +struct sdhci_host *host =platform_get_drvdata (data->pdev); +int ret; + +if (!data->ahb_aim_csr) +return 0 ; + +/* +* Setup AHB AIM windows ctrl register. The lower 32-bit is left +* at 0 while the upper bit are programmed when the buffer address +( is set from function sdhci_arasn_writel.
broken comment.
Quoted 19 lines
drivers/mmc/host/sdhci-of-arasan.c +19
-0+*/ +writel (AIM_EN_N_WR (1 ) |ARSB_WR (1 ) |AWSB_WR (1 ) |AIM_MASK_N_WR (0 ), + data->ahb_aim_csr +AIM_SIZE_CTL_OFFSET ); + +/* Set DMA mask */ + ret =dma_set_mask_and_coherent (&data->pdev->dev,DMA_BIT_MASK (64 )); +if (ret) { +dev_err (&data->pdev->dev,"Unable to set dma mask\n" ); +return ret; + } + +/* +* This shouldn't be necessary. Just in case the FW doesn't +* configure disable ADMA support as we can't support multiple +* DMA buffer whose address is 64-bit. The AHB translation bridge +* only has 8 entry max and that is required to be shared and +* upper layer can pass more than 8 buffer pointers. +*/ + host->quirks |=SDHCI_QUIRK_BROKEN_ADMA ;
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 199
Quoted 10 lines
drivers/mmc/host/sdhci-of-arasan.c +10
-0+ +return 0 ; +} + +static void sdhci_arasn_xgene_xlat_addr (struct sdhci_arasan_data *data, +u64 dma_addr) +{ +#define AIM_AXI_HI_OFFSET 0x0000000c +#define AIM_AXI_ADDRESS_HI_N_WR (src) \ + (((u32) (src) << 20) & 0xfff00000)
ditto with indentation.
20 should be shift0xfff00000 is mask.
Also you should take this opportunity and add function description in kernel doc to be exactly clear what this function is doing.
Quoted 35 lines
drivers/mmc/host/sdhci-of-arasan.c +24
-1+ +if (!data->ahb_aim_csr) +return ; + +writel (AIM_AXI_ADDRESS_HI_N_WR (dma_addr >>32 ), + data->ahb_aim_csr +AIM_AXI_HI_OFFSET ); +} + +static const struct sdhci_arasan_ahb_ops xgene_ahb_ops = { + .init_ahb = sdhci_arasan_xgene_init_ahb, + .xlat_addr = sdhci_arasn_xgene_xlat_addr, +}; + +static const struct of_device_id sdhci_arasan_of_match[] = { + { .compatible ="arasan,sdhci-8.9a" }, + { .compatible ="apm,arasan,sdhci-8.9a" , .data = &xgene_ahb_ops }, + { } +}; +MODULE_DEVICE_TABLE (of, sdhci_arasan_of_match); +static int sdhci_arasan_probe (struct platform_device *pdev) {int ret; -struct clk *clk_xin; +struct clk *clk_xin =NULL ;struct sdhci_host *host;struct sdhci_pltfm_host *pltfm_host;struct sdhci_arasan_data *sdhci_arasan; +const struct of_device_id *of_id = +of_match_device (sdhci_arasan_of_match, &pdev->dev); +struct resource *res; sdhci_arasan =devm_kzalloc (&pdev->dev,sizeof (*sdhci_arasan),GFP_KERNEL );
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_probe(struct platform_device *pdev) · line 240
Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c +3
-2@@ -136,8 +235,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev) sdhci_arasan->clk_ahb =devm_clk_get (&pdev->dev,"clk_ahb" );if (IS_ERR (sdhci_arasan->clk_ahb)) { -dev_err (&pdev->dev,"clk_ahb clock not found.\n" ); -return PTR_ERR (sdhci_arasan->clk_ahb); +/* Clock is optional */ + sdhci_arasan->clk_ahb =NULL ; + goto skip_clk;
Clocks are optional for your SoC but they are necessary for Zynq.
You can't just skip clocks for zynq because if DT is wrong clocks won't be enabled and even checked.
Does it mean that there are no clocks for this IP?
Or do you miss clock driver?
Thanks,
Michal
PATCH v2 2/3 mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
Hi,
Quoted 8 lines
Quoted 8 lines
#define
SDHCI_ARASAN_CLK_CTRL_OFFSET0x2cdrivers/mmc/host/sdhci-of-arasan.c +3
-0@@ -34,6 +36,19 @@*/ struct sdhci_arasan_data {struct clk *clk_ahb; +struct platform_device *pdev; +void __iomem *ahb_aim_csr; +const struct sdhci_arasan_ahb_ops *ahb_ops;
Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c line 47
Quoted 6 lines
drivers/mmc/host/sdhci-of-arasan.c +6
-0+}; + +/** +* struct sdhci_arasan_ahb_ops +* @init_ahb Initialize translation bus +* @xlat_addr Set up an 64-bit addressing translation This definitely breaking kernel-doc format. Please fix.
I will add the missing ":".
Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) · line 80
Quoted 5 lines
drivers/mmc/host/sdhci-of-arasan.c +5
-0+ sdhci_arasan->ahb_ops->xlat_addr (sdhci_arasan, +sg_dma_address (host->data->sg)); + } +writel (val, host->ioaddr + reg); +}The same code was submitted in v1 and Arnd commented it but you are still keeping it here. Why?
I responded back to Arnd. If I move this code to IO MMU, what do I do when we enable the actual IO MMU? The structure for the bus type only has one IO MMU pointer. We would need to IO MMU pointer and not sure how the IO MMU framework would handle this.
Quoted 22 lines
Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c +2
-0+static struct sdhci_ops sdhci_arasan_ops = { + .write_l = sdhci_arasan_writel, .get_max_clock = sdhci_pltfm_clk_get_max_clock, .get_timeout_clock = sdhci_arasan_get_timeout_clock, };drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 159
Quoted 10 lines
drivers/mmc/host/sdhci-of-arasan.c +7
-0@@ -121,13 +150,83 @@ static int sdhci_arasan_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend, sdhci_arasan_resume); +static int sdhci_arasan_xgene_init_ahb (struct sdhci_arasan_data *data) +{ +#define AIM_SIZE_CTL_OFFSET 0x00000004 +#define AIM_EN_N_WR (src) (((u32) (src) << 31) & 0x80000000) +#define ARSB_WR (src) (((u32) (src) << 24) & 0x0f000000) +#define AWSB_WR (src) (((u32) (src) << 20) & 0x00f00000) +#define AIM_MASK_N_WR (src) (((u32) (src)) & 0x000fffff)Remove that one more space between #define and name.
I am not fan of having these defines just here - move them to the top.
Quoted 3 lines
Also these macros are used just here at one location. Isn't it better just to define that BITS you want to setup instead of these macros which are hardly to read?
The only field that is single bit is AIM_EN_N_WR. All others are multiple bit fields. Either I write them in the code or use these defines. Would it be better if I just write them in the code?
Quoted 11 lines
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 199
Quoted 6 lines
drivers/mmc/host/sdhci-of-arasan.c +6
-0+static void sdhci_arasn_xgene_xlat_addr (struct sdhci_arasan_data *data, +u64 dma_addr) +{ +#define AIM_AXI_HI_OFFSET 0x0000000c +#define AIM_AXI_ADDRESS_HI_N_WR (src) \ + (((u32) (src) << 20) & 0xfff00000)ditto with indentation.
20 should be shift0xfff00000is mask.
Quoted 2 lines
Also you should take this opportunity and add function description in kernel doc to be exactly clear what this function is doing.
The function is pretty small, simply and don't believe it needs function description.
Quoted 37 lines
Quoted 37 lines
drivers/mmc/host/sdhci-of-arasan.c +24
-1+ +if (!data->ahb_aim_csr) +return ; + +writel (AIM_AXI_ADDRESS_HI_N_WR (dma_addr >>32 ), + data->ahb_aim_csr +AIM_AXI_HI_OFFSET ); +} + +static const struct sdhci_arasan_ahb_ops xgene_ahb_ops = { + .init_ahb = sdhci_arasan_xgene_init_ahb, + .xlat_addr = sdhci_arasn_xgene_xlat_addr, +}; + +static const struct of_device_id sdhci_arasan_of_match[] = { + { .compatible ="arasan,sdhci-8.9a" }, + { .compatible ="apm,arasan,sdhci-8.9a" , .data = &xgene_ahb_ops }, + { } +}; +MODULE_DEVICE_TABLE (of, sdhci_arasan_of_match); +static int sdhci_arasan_probe (struct platform_device *pdev) {int ret; -struct clk *clk_xin; +struct clk *clk_xin =NULL ;struct sdhci_host *host;struct sdhci_pltfm_host *pltfm_host;struct sdhci_arasan_data *sdhci_arasan; +const struct of_device_id *of_id = +of_match_device (sdhci_arasan_of_match, &pdev->dev); +struct resource *res; sdhci_arasan =devm_kzalloc (&pdev->dev,sizeof (*sdhci_arasan),GFP_KERNEL );@@ -136,8 +235,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
Quoted 12 lines
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_probe(struct platform_device *pdev) · line 240
Quoted 7 lines
drivers/mmc/host/sdhci-of-arasan.c +3
-2sdhci_arasan->clk_ahb =devm_clk_get (&pdev->dev,"clk_ahb" );if (IS_ERR (sdhci_arasan->clk_ahb)) { -dev_err (&pdev->dev,"clk_ahb clock not found.\n" ); -return PTR_ERR (sdhci_arasan->clk_ahb); +/* Clock is optional */ + sdhci_arasan->clk_ahb =NULL ; + goto skip_clk;Clocks are optional for your SoC but they are necessary for Zynq.
You can't just skip clocks for zynq because if DT is wrong clocks won't be enabled and even checked.
Does it mean that there are no clocks for this IP?
The clock for X-Gene is enabled by the time Linux boot. As the clock in programmed in the SDHC register, it knows the clock frequency.
Quoted 1 line
Or do you miss clock driver?
The clock will be configured by the FW and left out intentionally as this will also support ACPI boot.
-Loc
PATCH v2 2/3 mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
Hi,
Quoted 27 lines · Loc Ho
Hi,
Quoted 8 lines
Quoted 8 lines
#define
SDHCI_ARASAN_CLK_CTRL_OFFSET0x2cdrivers/mmc/host/sdhci-of-arasan.c +3
-0@@ -34,6 +36,19 @@*/ struct sdhci_arasan_data {struct clk *clk_ahb; +struct platform_device *pdev; +void __iomem *ahb_aim_csr; +const struct sdhci_arasan_ahb_ops *ahb_ops;Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c line 47
Quoted 6 lines
drivers/mmc/host/sdhci-of-arasan.c +6
-0+}; + +/** +* struct sdhci_arasan_ahb_ops +* @init_ahb Initialize translation bus +* @xlat_addr Set up an 64-bit addressing translation This definitely breaking kernel-doc format. Please fix.
I will add the missing ":".
Quoted 6 lines
Quoted 5 lines
drivers/mmc/host/sdhci-of-arasan.c +5
-0+ sdhci_arasan->ahb_ops->xlat_addr (sdhci_arasan, +sg_dma_address (host->data->sg)); + } +writel (val, host->ioaddr + reg); +}
Quoted 7 lines
Quoted 2 lines
The same code was submitted in v1 and Arnd commented it but you are still keeping it here. Why?
I responded back to Arnd. If I move this code to IO MMU, what do I do when we enable the actual IO MMU? The structure for the bus type only has one IO MMU pointer. We would need to IO MMU pointer and not sure how the IO MMU framework would handle this.
Then good time to start investigating this.
Quoted 26 lines
Quoted 22 lines
Quoted 8 lines
drivers/mmc/host/sdhci-of-arasan.c +2
-0+static struct sdhci_ops sdhci_arasan_ops = { + .write_l = sdhci_arasan_writel, .get_max_clock = sdhci_pltfm_clk_get_max_clock, .get_timeout_clock = sdhci_arasan_get_timeout_clock, };drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 159
Quoted 10 lines
drivers/mmc/host/sdhci-of-arasan.c +7
-0@@ -121,13 +150,83 @@ static int sdhci_arasan_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend, sdhci_arasan_resume); +static int sdhci_arasan_xgene_init_ahb (struct sdhci_arasan_data *data) +{ +#define AIM_SIZE_CTL_OFFSET 0x00000004 +#define AIM_EN_N_WR (src) (((u32) (src) << 31) & 0x80000000) +#define ARSB_WR (src) (((u32) (src) << 24) & 0x0f000000) +#define AWSB_WR (src) (((u32) (src) << 20) & 0x00f00000) +#define AIM_MASK_N_WR (src) (((u32) (src)) & 0x000fffff)Remove that one more space between #define and name.
I am not fan of having these defines just here - move them to the top.Quoted 3 lines
Also these macros are used just here at one location. Isn't it better just to define that BITS you want to setup instead of these macros which are hardly to read?
Quoted 5 lines
The only field that is single bit is
AIM_EN_N_WR. All others are multiple bit fields. Either I write them in the code or use these defines. Would it be better if I just write them in the code?From my point of view will be the best to compose one macro like
#define AIM_EN_N_WR BIT(31)
...
#define AIM...INIT_VAL (AIM_EN_N_WR |... )
Quoted 11 lines
Quoted 11 lines
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 199
Quoted 6 lines
drivers/mmc/host/sdhci-of-arasan.c +6
-0+static void sdhci_arasn_xgene_xlat_addr (struct sdhci_arasan_data *data, +u64 dma_addr) +{ +#define AIM_AXI_HI_OFFSET 0x0000000c +#define AIM_AXI_ADDRESS_HI_N_WR (src) \ + (((u32) (src) << 20) & 0xfff00000)ditto with indentation.
20 should be shift0xfff00000is mask.
Quoted 5 lines
Quoted 2 lines
Also you should take this opportunity and add function description in kernel doc to be exactly clear what this function is doing.
The function is pretty small, simply and don't believe it needs function description.
That documentation is for everybody not just for you who write the code. Simple description will be useful.
Quoted 49 lines
Quoted 36 lines
Quoted 36 lines
drivers/mmc/host/sdhci-of-arasan.c +24
-1+ +if (!data->ahb_aim_csr) +return ; + +writel (AIM_AXI_ADDRESS_HI_N_WR (dma_addr >>32 ), + data->ahb_aim_csr +AIM_AXI_HI_OFFSET ); +} + +static const struct sdhci_arasan_ahb_ops xgene_ahb_ops = { + .init_ahb = sdhci_arasan_xgene_init_ahb, + .xlat_addr = sdhci_arasn_xgene_xlat_addr, +}; + +static const struct of_device_id sdhci_arasan_of_match[] = { + { .compatible ="arasan,sdhci-8.9a" }, + { .compatible ="apm,arasan,sdhci-8.9a" , .data = &xgene_ahb_ops }, + { } +}; +MODULE_DEVICE_TABLE (of, sdhci_arasan_of_match); +static int sdhci_arasan_probe (struct platform_device *pdev) {int ret; -struct clk *clk_xin; +struct clk *clk_xin =NULL ;struct sdhci_host *host;struct sdhci_pltfm_host *pltfm_host;struct sdhci_arasan_data *sdhci_arasan; +const struct of_device_id *of_id = +of_match_device (sdhci_arasan_of_match, &pdev->dev); +struct resource *res; sdhci_arasan =devm_kzalloc (&pdev->dev,sizeof (*sdhci_arasan),GFP_KERNEL );@@ -136,8 +235,9 @@ static int sdhci_arasan_probe(struct platform_device *pdev)Quoted 12 lines
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_probe(struct platform_device *pdev) · line 240
Quoted 7 lines
drivers/mmc/host/sdhci-of-arasan.c +3
-2sdhci_arasan->clk_ahb =devm_clk_get (&pdev->dev,"clk_ahb" );if (IS_ERR (sdhci_arasan->clk_ahb)) { -dev_err (&pdev->dev,"clk_ahb clock not found.\n" ); -return PTR_ERR (sdhci_arasan->clk_ahb); +/* Clock is optional */ + sdhci_arasan->clk_ahb =NULL ; + goto skip_clk;Clocks are optional for your SoC but they are necessary for Zynq.
You can't just skip clocks for zynq because if DT is wrong clocks won't be enabled and even checked.
Does it mean that there are no clocks for this IP?
Quoted 7 lines
The clock for X-Gene is enabled by the time Linux boot. As the clock in programmed in the SDHC register, it knows the clock frequency.
Quoted 1 line
Or do you miss clock driver?
The clock will be configured by the FW and left out intentionally as this will also support ACPI boot.
Not a problem if this is what you like but you can't just break Zynq by this change which is what you are doing right now.
Thanks,
Michal
PATCH v2 2/3 mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
In reply to Michal SimekQuoted 12 lines · On Wednesday 04 June 2014 08:09:12 Michal Simek wrote:
drivers/mmc/host/sdhci-of-arasan.c static int sdhci_arasan_resume(struct device *dev) · line 199
Quoted 7 lines
drivers/mmc/host/sdhci-of-arasan.c +7
-0+ +static void sdhci_arasn_xgene_xlat_addr (struct sdhci_arasan_data *data, +u64 dma_addr) +{ +#define AIM_AXI_HI_OFFSET 0x0000000c +#define AIM_AXI_ADDRESS_HI_N_WR (src) \ + (((u32) (src) << 20) & 0xfff00000)ditto with indentation.
20 should be shift0xfff00000is mask.
Quoted 2 lines
Also you should take this opportunity and add function description in kernel doc to be exactly clear what this function is doing.
Actually this should just be configured in the dma-ranges property I think. It's not the driver's business to do the dma translation.
Arnd
v2 cover mmc: Add APM X-Gene SoC SDHC controller support to Arasan SDHCI driver
In reply to Loc HoQuoted 6 lines · Loc Ho
This patch adds support for the APM X-Gene SoC SDHC controller to Arasan SDHCI driver.
v2:
* Update binding documentation
* Change compatible string from "xgene,arasan" to "apm,arasan"
Please write these changes directly to the patch version. Hard to guess what you have changed in every patch.
Thanks,
Michal
Signature · 14 lines
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 263 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140604/d87d43c2/attachment.sig>