On Thursday 27 February 2014 02:15 AM, Loc Ho wrote:
Hi,
+config PHY_XGENE
+ tristate "APM X-Gene 15Gbps PHY support"
+ depends on ARM64 || COMPILE_TEST
+ select GENERIC_PHY
depends on HAS_IOMEM and CONFIG_OF..
I will make it depends as "HAS_IOMEM && OF && (ARM64 || COMPILE_TEST)
+/* PLL Clock Macro Unit (CMU) CSR accessing from SDS indirectly */
+#define CMU_REG0 0x00000
+#define CMU_REG0_PLL_REF_SEL_MASK 0x00002000
+#define CMU_REG0_PLL_REF_SEL_SET(dst, src) \
+ (((dst) & ~0x00002000) | (((u32)(src) << 0xd) & 0x00002000))
using decimals for shift value would be better. No strong feeling though.
I will change to integer value.
+/*
+ * For chip earlier than A3 version, enable this flag.
+ * To enable, pass boot argument phy_xgene.preA3Chip=1
+ */
+static int preA3Chip;
+MODULE_PARM_DESC(preA3Chip, "Enable pre-A3 chip support (1=enable 0=disable)");
+module_param_named(preA3Chip, preA3Chip, int, 0444);
Do we need to have module param for this? I mean we can differentiate between
different chip versions in dt data only.
This is only required for the short term. Once all the pre-A3 system
are replaced, there isn't an need for this. For those who still has an
pre-A3 silicon system, this would provide an short term solution for
them. DT isn't quite correct here. This is an global thing. I guess I
can OR all node. If it is still better to put in the DT, let me know
and I will move it.
+
+static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
+ u32 indirect_data_reg, u32 addr, u32 data)
+{
+ u32 val;
+ u32 cmd;
+
+ cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
+ cmd = CFG_IND_ADDR_SET(cmd, addr);
This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK' should be set then it should be part of the second argument. From the macro 'CFG_IND_ADDR_SET' the first argument should be more like the current value present in the register right? I feel the macro (CFG_IND_ADDR_SET) is not used in the way it is intended to.
The macro XXX_SET is intended to update an field within the register.
The update field is returned. The first assignment lines are setting
another field. Those two lines can be written as:
cmd = 0;
cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
#define CFG_IND_ADDR_SET(dst, src) \
(((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0))
From this macro the first argument should be the present value in that
register. Here you reset the address bits and write the new address bits.
IMO the first argument should be the value in 'csr_base +
indirect_cmd_reg'. So it resets the address bits in 'csr_base +
indirect_cmd_reg' and write down the new address bits.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
