Skip to contentSkip to conversation

arm: socfpga: fix up a questionable macro for SDMMC

PATCH New dinguyen at opensource.altera.com 3 participants 5 messages Dec 1, 2015 – Dec 3, 2015

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm/socfpga architecture
lore
Share & tools
lore

dinguyen at opensource.altera.com

author
#0 lore raw

PATCH 1/1 arm: socfpga: fix up a questionable macro for SDMMC

From: Dinh Nguyen <[email protected]>

Not sure what made this macro questionable, but edit the macro to be similar to what is used in Linux.

Signed-off-by Dinh Nguyen <[email protected]>
---
 arch/arm/mach-socfpga/include/mach/system_manager.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
arch/arm/mach-socfpga/include/mach/system_manager.h +1 -2
index 618c92f..c0566b0 100644
@@ -201,9 +201,8 @@ struct socfpga_system_manager {
 #define SYSMGR_FPGAINTF_NAND	(1 << 4)
 #define SYSMGR_FPGAINTF_SDMMC	(1 << 5)
 
-/* FIXME: This is questionable macro. */
 #define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)	\
-	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
 
 /* EMAC Group Bit definitions */
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII	0x0
Signature · 1 line
2.6.2
#1 lore raw

PATCH 1/1 arm: socfpga: fix up a questionable macro for SDMMC

Quoted 4 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Not sure what made this macro questionable, but edit the macro to be similar to what is used in Linux.

It should most likely be dissolved and moved into socfpga_dw_mmc.c , since it's used only once in there. What do you think ?

Quoted 11 lines
Signed-off-by Dinh Nguyen <[email protected]>
---
 arch/arm/mach-socfpga/include/mach/system_manager.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
(quoted diff) +0 -0

b/arch/arm/mach-socfpga/include/mach/system_manager.h index 618c92f..c0566b0 100644

arch/arm/mach-socfpga/include/mach/system_manager.h struct socfpga_system_manager { · line 208

Quoted 10 lines
arch/arm/mach-socfpga/include/mach/system_manager.h +1 -2
@@ -201,9 +201,8 @@ struct socfpga_system_manager {
 #define SYSMGR_FPGAINTF_NAND	(1 << 4)
 #define SYSMGR_FPGAINTF_SDMMC	(1 << 5)
 
-/* FIXME: This is questionable macro. */
 #define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)	\
-	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
 
 /* EMAC Group Bit definitions */
 #define SYSMGR_EMACGRP_CTRL_PHYSEL_ENUM_GMII_MII	0x0

Best regards,
Marek Vasut

author
#2 lore raw

PATCH 1/1 arm: socfpga: fix up a questionable macro for SDMMC

Quoted 10 lines · Marek Vasut
Quoted 4 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Not sure what made this macro questionable, but edit the macro to be similar to what is used in Linux.

It should most likely be dissolved and moved into socfpga_dw_mmc.c , since it's used only once in there. What do you think ?

Hmm...if you move this to socfpga_dw_mmc.c then the #if defined(A10) would have to follow as the bit for smplsel is at shift 4 instead of 3.

So if you had this for in system_manager.h, then socfpga_dw_mmc.c doesn't have to change.

arch/arm/mach-socfpga/include/mach/system_manager.h +5 -1
@@ -201,8 +201,13 @@ struct socfpga_system_manager {
 #define SYSMGR_FPGAINTF_NAND   (1 << 4)
 #define SYSMGR_FPGAINTF_SDMMC  (1 << 5)
 
+#if defined(SOCFPGA_GEN5)
 #define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
        ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+#else /* Arria10 */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
+       ((((smplsel) & 0x7) << 4) | (((drvsel) & 0x7) << 0))
+#endif
---

Dinh

#3 lore raw

PATCH 1/1 arm: socfpga: fix up a questionable macro for SDMMC

Quoted 22 lines · Dinh Nguyen
Quoted 11 lines · Marek Vasut

On Wednesday, December 02, 2015 at 12:20:47 AM,
dinguyen at opensource.altera.com

Quoted 4 lines · wrote:

From: Dinh Nguyen <[email protected]>

Not sure what made this macro questionable, but edit the macro to be similar to what is used in Linux.

It should most likely be dissolved and moved into socfpga_dw_mmc.c , since it's used only once in there. What do you think ?

Hmm...if you move this to socfpga_dw_mmc.c then the #if defined(A10) would have to follow as the bit for smplsel is at shift 4 instead of 3.

So if you had this for in system_manager.h, then socfpga_dw_mmc.c doesn't have to change.

arch/arm/mach-socfpga/include/mach/system_manager.h struct socfpga_system_manager { · line 210

Quoted 10 lines
arch/arm/mach-socfpga/include/mach/system_manager.h +5 -0
@@ -201,8 +201,13 @@ struct socfpga_system_manager {
 #define SYSMGR_FPGAINTF_NAND   (1 << 4)
 #define SYSMGR_FPGAINTF_SDMMC  (1 << 5)
 
+#if defined(SOCFPGA_GEN5)
 #define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
        ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+#else /* Arria10 */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
+       ((((smplsel) & 0x7) << 4) | (((drvsel) & 0x7) << 0))
+#endif

Then define the offsets in system_manager.h and then just do writel((smplsel << OFFSET1) | (drvsel << offset2), ...); in the MMC driver. That would work without ifdeffery, right ?

Best regards,
Marek Vasut

#4 lore raw

PATCH 1/1 arm: socfpga: fix up a questionable macro for SDMMC

In reply to Dinh Nguyen
Quoted 21 lines · Dinh Nguyen
Quoted 10 lines · Marek Vasut
Quoted 4 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

Not sure what made this macro questionable, but edit the macro to be similar to what is used in Linux.

It should most likely be dissolved and moved into socfpga_dw_mmc.c , since it's used only once in there. What do you think ?

Hmm...if you move this to socfpga_dw_mmc.c then the #if defined(A10) would have to follow as the bit for smplsel is at shift 4 instead of 3.

So if you had this for in system_manager.h, then socfpga_dw_mmc.c doesn't have to change.

arch/arm/mach-socfpga/include/mach/system_manager.h struct socfpga_system_manager { · line 210

Quoted 10 lines
arch/arm/mach-socfpga/include/mach/system_manager.h +5 -0
@@ -201,8 +201,13 @@ struct socfpga_system_manager {
 #define SYSMGR_FPGAINTF_NAND   (1 << 4)
 #define SYSMGR_FPGAINTF_SDMMC  (1 << 5)
 
+#if defined(SOCFPGA_GEN5)
 #define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
        ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+#else /* Arria10 */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \
+       ((((smplsel) & 0x7) << 4) | (((drvsel) & 0x7) << 0))
+#endif

Also... can we get a rid of << 0 here?

Is it ever valid to pass values >= 8 to this macro?

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html