diff mbox series

[RESEND,4/4] i2c: mpc: always determine I2C clock prescaler at runtime

Message ID 20171207102003.23496-5-asolokha@kb.kras.ru
State Superseded
Headers show
Series i2c: mpc: Clean up clock selection | expand

Commit Message

Arseny Solokha Dec. 7, 2017, 10:20 a.m. UTC 
Remove the facility for setting the prescaler value at compile time
entirely. It was only used for two SoCs, duplicating the actual value
for one of them and setting sometimes bogus value for another. Make all
MPC8xxx SoCs obtain their actual I2C clock prescaler from a single place
in the code.

Signed-off-by: Arseny Solokha <asolokha@kb.kras.ru>
---
 drivers/i2c/busses/i2c-mpc.c | 52 +++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 37 deletions(-)

Comments

Wolfram Sang Dec. 30, 2017, 6:14 p.m. UTC | #1 
>  static const struct of_device_id mpc_i2c_of_match[] = {
>  	{.compatible = "mpc5200-i2c", .data = &mpc_i2c_data_52xx, },
>  	{.compatible = "fsl,mpc5200b-i2c", .data = &mpc_i2c_data_52xx, },
>  	{.compatible = "fsl,mpc5200-i2c", .data = &mpc_i2c_data_52xx, },
>  	{.compatible = "fsl,mpc5121-i2c", .data = &mpc_i2c_data_512x, },
> -	{.compatible = "fsl,mpc8313-i2c", .data = &mpc_i2c_data_8313, },
> -	{.compatible = "fsl,mpc8543-i2c", .data = &mpc_i2c_data_8543, },
> -	{.compatible = "fsl,mpc8544-i2c", .data = &mpc_i2c_data_8544, },

We can't remove compatibles. It may break DTBs. We can change the data
pointer, though.
Arseny Solokha Jan. 10, 2018, 11:36 a.m. UTC | #2 
>>  			/*
>>  			 * Map and check POR Device Status Register 2
>> -			 * (PORDEVSR2) at 0xE0014
>> +			 * (PORDEVSR2) at 0xE0014. Note than while MPC8533
>> +			 * and MPC8544 indicate SEC frequency ratio
>> +			 * configuration as bit 26 in PORDEVSR2, other MPC8xxx
>> +			 * parts may store it differently or may not have it
>> +			 * at all.
>
> So given this comment which you added...
>
>>  			 */
>>  			reg = ioremap(get_immrbase() + *prop + 0x14, 0x4);
>>  			if (!reg)
>>  				printk(KERN_ERR
>>  				       "Error: couldn't map PORDEVSR2\n");
>>  			else
>> -				val = in_be32(reg) & 0x00000080; /* sec-cfg */
>> +				val = in_be32(reg) & 0x00000020; /* sec-cfg */
>
> ... are you really sure there is no ancient device which needs the
> 0x00000080?

Various MPC SoCs indeed have different bit layout of
PORDEVSR2. Obviously not all of them indicate SEC frequency ratio
configuration at all, either in PORDEVSR2 or in some other register, as
not all SoCs contain SEC engine. In this regard,
mpc_i2c_get_sec_cfg_8xxx() is poorly named as it in its current form is
only applicable to a few SoCs from the whole MPC8xxx family.

mpc_i2c_get_sec_cfg_8xxx() is only called from the following snippet:

  else if ((SVR_SOC_VER(svr) == SVR_8533)
          || (SVR_SOC_VER(svr) == SVR_8544))
          /* the above 85xx SoCs have prescaler 3 or 2 */
          prescaler = mpc_i2c_get_sec_cfg_8xxx() ? 3 : 2;

Both MPC8533 and MPC8544 do in fact indicate SEC frequency ratio
configuration as bit 26 in PORDEVSR2 according to [1,2], so I've added
the comment as a precaution for future uses. I can also rename the
function to something like mpc_i2c_get_sec_cfg_8533_8544(), which looks
ugly and doesn't scale.

AFAICS, mask 0x00000080 in the code clearly contradicts to what I read
in the docs.

[1] https://www.nxp.com/docs/en/reference-manual/MPC8533ERM.pdf
[2] https://www.nxp.com/docs/en/reference-manual/MPC8544ERM.pdf

Arsény
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index aac0ec6dc5fc..ad9af3ca35aa 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -78,9 +78,7 @@  struct mpc_i2c_divider {
 };
 
 struct mpc_i2c_data {
-	void (*setup)(struct device_node *node, struct mpc_i2c *i2c,
-		      u32 clock, u32 prescaler);
-	u32 prescaler;
+	void (*setup)(struct device_node *node, struct mpc_i2c *i2c, u32 clock);
 };
 
 static inline void writeccr(struct mpc_i2c *i2c, u32 x)
@@ -201,7 +199,7 @@  static const struct mpc_i2c_divider mpc_i2c_dividers_52xx[] = {
 };
 
 static int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock,
-					  int prescaler, u32 *real_clk)
+					  u32 *real_clk)
 {
 	const struct mpc_i2c_divider *div = NULL;
 	unsigned int pvr = mfspr(SPRN_PVR);
@@ -236,7 +234,7 @@  static int mpc_i2c_get_fdr_52xx(struct device_node *node, u32 clock,
 
 static void mpc_i2c_setup_52xx(struct device_node *node,
 					 struct mpc_i2c *i2c,
-					 u32 clock, u32 prescaler)
+					 u32 clock)
 {
 	int ret, fdr;
 
@@ -246,7 +244,7 @@  static void mpc_i2c_setup_52xx(struct device_node *node,
 		return;
 	}
 
-	ret = mpc_i2c_get_fdr_52xx(node, clock, prescaler, &i2c->real_clk);
+	ret = mpc_i2c_get_fdr_52xx(node, clock, &i2c->real_clk);
 	fdr = (ret >= 0) ? ret : 0x3f; /* backward compatibility */
 
 	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
@@ -258,7 +256,7 @@  static void mpc_i2c_setup_52xx(struct device_node *node,
 #else /* !(CONFIG_PPC_MPC52xx || CONFIG_PPC_MPC512x) */
 static void mpc_i2c_setup_52xx(struct device_node *node,
 					 struct mpc_i2c *i2c,
-					 u32 clock, u32 prescaler)
+					 u32 clock)
 {
 }
 #endif /* CONFIG_PPC_MPC52xx || CONFIG_PPC_MPC512x */
@@ -266,7 +264,7 @@  static void mpc_i2c_setup_52xx(struct device_node *node,
 #ifdef CONFIG_PPC_MPC512x
 static void mpc_i2c_setup_512x(struct device_node *node,
 					 struct mpc_i2c *i2c,
-					 u32 clock, u32 prescaler)
+					 u32 clock)
 {
 	struct device_node *node_ctrl;
 	void __iomem *ctrl;
@@ -289,12 +287,12 @@  static void mpc_i2c_setup_512x(struct device_node *node,
 	}
 
 	/* The clock setup for the 52xx works also fine for the 512x */
-	mpc_i2c_setup_52xx(node, i2c, clock, prescaler);
+	mpc_i2c_setup_52xx(node, i2c, clock);
 }
 #else /* CONFIG_PPC_MPC512x */
 static void mpc_i2c_setup_512x(struct device_node *node,
 					 struct mpc_i2c *i2c,
-					 u32 clock, u32 prescaler)
+					 u32 clock)
 {
 }
 #endif /* CONFIG_PPC_MPC512x */
@@ -388,16 +386,13 @@  static u32 mpc_i2c_get_prescaler_8xxx(void)
 }
 
 static int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock,
-					  u32 prescaler, u32 *real_clk)
+					  u32 *real_clk)
 {
 	const struct mpc_i2c_divider *div = NULL;
+	u32 prescaler = mpc_i2c_get_prescaler_8xxx();
 	u32 divider;
 	int i;
 
-	/* Determine proper divider value */
-	if (!prescaler)
-		prescaler = mpc_i2c_get_prescaler_8xxx();
-
 	if (clock == MPC_I2C_CLOCK_LEGACY) {
 		/* see below - default fdr = 0x1031 -> div = 16 * 3072 */
 		*real_clk = fsl_get_sys_freq() / prescaler / (16 * 3072);
@@ -425,7 +420,7 @@  static int mpc_i2c_get_fdr_8xxx(struct device_node *node, u32 clock,
 
 static void mpc_i2c_setup_8xxx(struct device_node *node,
 					 struct mpc_i2c *i2c,
-					 u32 clock, u32 prescaler)
+					 u32 clock)
 {
 	int ret, fdr;
 
@@ -436,7 +431,7 @@  static void mpc_i2c_setup_8xxx(struct device_node *node,
 		return;
 	}
 
-	ret = mpc_i2c_get_fdr_8xxx(node, clock, prescaler, &i2c->real_clk);
+	ret = mpc_i2c_get_fdr_8xxx(node, clock, &i2c->real_clk);
 	fdr = (ret >= 0) ? ret : 0x1031; /* backward compatibility */
 
 	writeb(fdr & 0xff, i2c->base + MPC_I2C_FDR);
@@ -450,7 +445,7 @@  static void mpc_i2c_setup_8xxx(struct device_node *node,
 #else /* !CONFIG_FSL_SOC */
 static void mpc_i2c_setup_8xxx(struct device_node *node,
 					 struct mpc_i2c *i2c,
-					 u32 clock, u32 prescaler)
+					 u32 clock)
 {
 }
 #endif /* CONFIG_FSL_SOC */
@@ -721,11 +716,11 @@  static int fsl_i2c_probe(struct platform_device *op)
 
 	if (match->data) {
 		const struct mpc_i2c_data *data = match->data;
-		data->setup(op->dev.of_node, i2c, clock, data->prescaler);
+		data->setup(op->dev.of_node, i2c, clock);
 	} else {
 		/* Backwards compatibility */
 		if (of_get_property(op->dev.of_node, "dfsrr", NULL))
-			mpc_i2c_setup_8xxx(op->dev.of_node, i2c, clock, 0);
+			mpc_i2c_setup_8xxx(op->dev.of_node, i2c, clock);
 	}
 
 	prop = of_get_property(op->dev.of_node, "fsl,timeout", &plen);
@@ -817,28 +812,11 @@  static const struct mpc_i2c_data mpc_i2c_data_52xx = {
 	.setup = mpc_i2c_setup_52xx,
 };
 
-static const struct mpc_i2c_data mpc_i2c_data_8313 = {
-	.setup = mpc_i2c_setup_8xxx,
-};
-
-static const struct mpc_i2c_data mpc_i2c_data_8543 = {
-	.setup = mpc_i2c_setup_8xxx,
-	.prescaler = 2,
-};
-
-static const struct mpc_i2c_data mpc_i2c_data_8544 = {
-	.setup = mpc_i2c_setup_8xxx,
-	.prescaler = 3,
-};
-
 static const struct of_device_id mpc_i2c_of_match[] = {
 	{.compatible = "mpc5200-i2c", .data = &mpc_i2c_data_52xx, },
 	{.compatible = "fsl,mpc5200b-i2c", .data = &mpc_i2c_data_52xx, },
 	{.compatible = "fsl,mpc5200-i2c", .data = &mpc_i2c_data_52xx, },
 	{.compatible = "fsl,mpc5121-i2c", .data = &mpc_i2c_data_512x, },
-	{.compatible = "fsl,mpc8313-i2c", .data = &mpc_i2c_data_8313, },
-	{.compatible = "fsl,mpc8543-i2c", .data = &mpc_i2c_data_8543, },
-	{.compatible = "fsl,mpc8544-i2c", .data = &mpc_i2c_data_8544, },
 	/* Backward compatibility */
 	{.compatible = "fsl-i2c", },
 	{},