Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote: On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote: Wolfgang Grandegger wrote: I know but we still need an algorithm for MPC52xx and MPC82xx as well. That's true, but I still think hard-coding values of DFSR and FDR in the device tree is not a good way to do this. I agree, it should encode real frequencies, not raw register values. Digging deeper I'm frightened by plenty of platform specific code. We would need: - one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors (already available from Timur's U-Boot implementation) - one table of divider,fdr values for the MPC5200 rev A. - one table of divider,fdr values for the MPC5200 rev B. (the Rev. B has two more pre-scaler bits). - furthermore, there are various mpc-specific I2C clock sources: MPC82xx : fsl_get_sys_freq() MPC5200 : IPB MPC83xx : fsl_get_sys_freq() MPC8540/41/60/55,MPC8610: fsl_get_sys_freq() MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2 MPC8544 : fsl_get_sys_freq()/2 or /3 It would make sense to hand-over the I2C frequency from U-Boot to Linux. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Grant Likely wrote:
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
Wolfgang Grandegger wrote:
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the
device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
Aren't the tables in the manual there just to make it easy for a human
to pick out the line they want? For a computer you'd program the
formula that was used to create the tables.
I agree that it took me half an hour to figure out the formula that
was needed to compute the i2s clocks, but once you figure out the
formula it solves all of the cases and no one needs to read the manual
any more. The i2c formula may even need a small loop which compares
different solutions looking for the smallest error term. But it's a
small space to search.
These device tree flags should be removed, the driver can ask the
platform code what CPU it is running on.
if (of_get_property(op-node, dfsrr, NULL))
i2c-flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
if (of_device_is_compatible(op-node, fsl,mpc5200-i2c) ||
of_device_is_compatible(op-node, mpc5200-i2c))
i2c-flags |= FSL_I2C_DEV_CLOCK_5200;
static void mpc_i2c_setclock(struct mpc_i2c *i2c)
{
/* Set clock and filters */
if (i2c-flags FSL_I2C_DEV_SEPARATE_DFSRR) {
writeb(0x31, i2c-base + MPC_I2C_FDR);
writeb(0x10, i2c-base + MPC_I2C_DFSRR);
} else if (i2c-flags FSL_I2C_DEV_CLOCK_5200)
writeb(0x3f, i2c-base + MPC_I2C_FDR);
else
writel(0x1031, i2c-base + MPC_I2C_FDR);
}
These defines shouldn't be here, they should get the offset from the
right header file for the CPU. But it appears that structures for the
i2c memory map haven't been done for the various CPUs.
#define MPC_I2C_FDR 0x04
#define MPC_I2C_CR 0x08
#define MPC_I2C_SR 0x0c
#define MPC_I2C_DR 0x10
#define MPC_I2C_DFSRR 0x14
There appears to be one for i2x8xxx but not the other CPUs.
/* I2C
*/
typedef struct i2c {
u_char i2c_i2mod;
charres1[3];
u_char i2c_i2add;
charres2[3];
u_char i2c_i2brg;
charres3[3];
u_char i2c_i2com;
charres4[3];
u_char i2c_i2cer;
charres5[3];
u_char i2c_i2cmr;
charres6[0x8b];
} i2c8xx_t;
- furthermore, there are various mpc-specific I2C clock sources:
MPC82xx : fsl_get_sys_freq()
MPC5200 : IPB
MPC83xx : fsl_get_sys_freq()
MPC8540/41/60/55,MPC8610: fsl_get_sys_freq()
MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2
MPC8544 : fsl_get_sys_freq()/2 or /3
It would make sense to hand-over the I2C frequency from U-Boot to
Linux.
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: Aren't the tables in the manual there just to make it easy for a human to pick out the line they want? For a computer you'd program the formula that was used to create the tables. Actually, the tables in the manuals are just an example of what can be programmed. They don't cover all cases. The tables assume a specific value of DFSR. For 8xxx, you want to use AN2919 to figure out how to really program the device. The table I generated for U-Boot is based on the calculations outlined in AN2919. I debated trying to implement the actual algorithm, but decided that pre-calculating the values was better. The algorithm is very convoluted. I agree that it took me half an hour to figure out the formula that was needed to compute the i2s clocks, but once you figure out the formula it solves all of the cases and no one needs to read the manual any more. The i2c formula may even need a small loop which compares different solutions looking for the smallest error term. But it's a small space to search. My understanding is that the algorithm itself is different on 8xxx vs. 52xx. It might be possible to combine 5200A and 5200B into one table/algorithm, but I don't think you can combine it with the 8xxx table. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 5:51 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote: Grant Likely wrote: On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote: Wolfgang Grandegger wrote: I know but we still need an algorithm for MPC52xx and MPC82xx as well. That's true, but I still think hard-coding values of DFSR and FDR in the device tree is not a good way to do this. I agree, it should encode real frequencies, not raw register values. Digging deeper I'm frightened by plenty of platform specific code. We would need: - one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors (already available from Timur's U-Boot implementation) - one table of divider,fdr values for the MPC5200 rev A. - one table of divider,fdr values for the MPC5200 rev B. (the Rev. B has two more pre-scaler bits). - furthermore, there are various mpc-specific I2C clock sources: MPC82xx : fsl_get_sys_freq() MPC5200 : IPB MPC83xx : fsl_get_sys_freq() MPC8540/41/60/55,MPC8610: fsl_get_sys_freq() MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2 MPC8544 : fsl_get_sys_freq()/2 or /3 It would make sense to hand-over the I2C frequency from U-Boot to Linux. U-Boot isn't always available and there are plenty of 5200 and 8xxx boards out there which will never have U-Boot reflashed to provide this data. Also, there are boards that don't even use U-Boot. I don't want to go down this path. It is the drivers *job* to understand how to set these registers. If you're careful, the table doesn't need to be huge. It can be marked as initdata and conditionally compiled depending on which architectures are compiled in. You should use .data in the driver's of_device_id table to provide machine specific ops for setting clocking to avoid a maze of if/else statements. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote:
If you're careful, the table doesn't need to be huge. It can be
marked as initdata and conditionally compiled depending on which
architectures are compiled in. You should use .data in the driver's
of_device_id table to provide machine specific ops for setting
clocking to avoid a maze of if/else statements.
Does this look ok for the mpc5200 i2c struct?
/* I2C Registers */
struct mpc52xx_i2c {
u8 madr;/* I2C + 0x00 */
u8 reserved1[3];/* I2C + 0x01 */
u8 mfdr;/* I2C + 0x04 */
u8 reserved2[3];/* I2C + 0x05 */
u8 mcr; /* I2C + 0x08 */
u8 reserved3[3];/* I2C + 0x09 */
u8 msr; /* I2C + 0x0c */
u8 reserved4[3];/* I2C + 0x0d */
u8 mdr; /* I2C + 0x10 */
u8 reserved5[15]; /* I2C + 0x11 */
u8 interrupt; /* I2C + 0x20 */
u8 reserved6[3];/* I2C + 0x21 */
u8 mifr;/* I2C + 0x24 */
};
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote:
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Grant Likely wrote:
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
Wolfgang Grandegger wrote:
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in the
device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
Aren't the tables in the manual there just to make it easy for a human
to pick out the line they want? For a computer you'd program the
formula that was used to create the tables.
I have the formulas to create the tables, also for the MPC5200 Rev. A
and B. That was not my point. I'm worried about arch specific code in
i2c-mpc.c. It should go somewhere to arch/powerpc.
I agree that it took me half an hour to figure out the formula that
was needed to compute the i2s clocks, but once you figure out the
formula it solves all of the cases and no one needs to read the manual
any more. The i2c formula may even need a small loop which compares
different solutions looking for the smallest error term. But it's a
small space to search.
These device tree flags should be removed, the driver can ask the
platform code what CPU it is running on.
if (of_get_property(op-node, dfsrr, NULL))
i2c-flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
if (of_device_is_compatible(op-node, fsl,mpc5200-i2c) ||
of_device_is_compatible(op-node, mpc5200-i2c))
i2c-flags |= FSL_I2C_DEV_CLOCK_5200;
static void mpc_i2c_setclock(struct mpc_i2c *i2c)
{
/* Set clock and filters */
if (i2c-flags FSL_I2C_DEV_SEPARATE_DFSRR) {
writeb(0x31, i2c-base + MPC_I2C_FDR);
writeb(0x10, i2c-base + MPC_I2C_DFSRR);
} else if (i2c-flags FSL_I2C_DEV_CLOCK_5200)
writeb(0x3f, i2c-base + MPC_I2C_FDR);
else
writel(0x1031, i2c-base + MPC_I2C_FDR);
}
These defines shouldn't be here, they should get the offset from the
right header file for the CPU. But it appears that structures for the
i2c memory map haven't been done for the various CPUs.
#define MPC_I2C_FDR 0x04
#define MPC_I2C_CR 0x08
#define MPC_I2C_SR 0x0c
#define MPC_I2C_DR 0x10
#define MPC_I2C_DFSRR 0x14
There appears to be one for i2x8xxx but not the other CPUs.
/* I2C
*/
typedef struct i2c {
u_char i2c_i2mod;
charres1[3];
u_char i2c_i2add;
charres2[3];
u_char i2c_i2brg;
charres3[3];
u_char i2c_i2com;
charres4[3];
u_char i2c_i2cer;
charres5[3];
u_char i2c_i2cmr;
charres6[0x8b];
} i2c8xx_t;
The I2C interface for the MPC5200 is not compatible with the one for the
MPC83/4/5/6xx, AFAIK.
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote: On Thu, Jul 31, 2008 at 5:51 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote: Grant Likely wrote: On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote: Wolfgang Grandegger wrote: I know but we still need an algorithm for MPC52xx and MPC82xx as well. That's true, but I still think hard-coding values of DFSR and FDR in the device tree is not a good way to do this. I agree, it should encode real frequencies, not raw register values. Digging deeper I'm frightened by plenty of platform specific code. We would need: - one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors (already available from Timur's U-Boot implementation) - one table of divider,fdr values for the MPC5200 rev A. - one table of divider,fdr values for the MPC5200 rev B. (the Rev. B has two more pre-scaler bits). - furthermore, there are various mpc-specific I2C clock sources: MPC82xx : fsl_get_sys_freq() MPC5200 : IPB MPC83xx : fsl_get_sys_freq() MPC8540/41/60/55,MPC8610: fsl_get_sys_freq() MPC8543/45/47/48/68, MPC8641: fsl_get_sys_freq()/2 MPC8544 : fsl_get_sys_freq()/2 or /3 It would make sense to hand-over the I2C frequency from U-Boot to Linux. U-Boot isn't always available and there are plenty of 5200 and 8xxx boards out there which will never have U-Boot reflashed to provide this data. Also, there are boards that don't even use U-Boot. I don't want to go down this path. It is the drivers *job* to understand how to set these registers. If you're careful, the table doesn't need to be huge. It can be marked as initdata and conditionally compiled depending on which architectures are compiled in. You should use .data in the driver's of_device_id table to provide machine specific ops for setting clocking to avoid a maze of if/else statements. Yep, that makes sense. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 11:22 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Grant Likely wrote:
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
Wolfgang Grandegger wrote:
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in
the
device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We
would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
Aren't the tables in the manual there just to make it easy for a human
to pick out the line they want? For a computer you'd program the
formula that was used to create the tables.
I have the formulas to create the tables, also for the MPC5200 Rev. A and B.
Oh, hey, even better.
That was not my point. I'm worried about arch specific code in i2c-mpc.c. It
should go somewhere to arch/powerpc.
i2c-mpc *is* arch specific. I really don't think you need to worry
about adding a block of code for each supported SoC family. Just
change the of_match table to look something like this:
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = fsl,mpc8260-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8349-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8540-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8543-i2c, .data =
fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = fsl,mpc8544-i2c, .data =
fsl_i2c_mpc8xxx_div3_set_freq, },
/* keep this only for older device trees with some support
code to figure out
what .data should have pointed to. */
{.compatible = fsl-i2c, },
{},
};
MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
There appears to be one for i2x8xxx but not the other CPUs.
/* I2C
*/
typedef struct i2c {
u_char i2c_i2mod;
charres1[3];
u_char i2c_i2add;
charres2[3];
u_char i2c_i2brg;
charres3[3];
u_char i2c_i2com;
charres4[3];
u_char i2c_i2cer;
charres5[3];
u_char i2c_i2cmr;
charres6[0x8b];
} i2c8xx_t;
The I2C interface for the MPC5200 is not compatible with the one for the
MPC83/4/5/6xx, AFAIK.
Ignore that table, I mistook MPC8xx for MPC8xxx. That is a MPC8xx table.
Wolfgang.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote:
On Thu, Jul 31, 2008 at 11:06 AM, Jon Smirl [EMAIL PROTECTED] wrote:
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote:
If you're careful, the table doesn't need to be huge. It can be
marked as initdata and conditionally compiled depending on which
architectures are compiled in. You should use .data in the driver's
of_device_id table to provide machine specific ops for setting
clocking to avoid a maze of if/else statements.
Does this look ok for the mpc5200 i2c struct?
/* I2C Registers */
struct mpc52xx_i2c {
u8 madr;/* I2C + 0x00 */
u8 reserved1[3];/* I2C + 0x01 */
u8 mfdr;/* I2C + 0x04 */
u8 reserved2[3];/* I2C + 0x05 */
u8 mcr; /* I2C + 0x08 */
u8 reserved3[3];/* I2C + 0x09 */
u8 msr; /* I2C + 0x0c */
u8 reserved4[3];/* I2C + 0x0d */
u8 mdr; /* I2C + 0x10 */
u8 reserved5[15]; /* I2C + 0x11 */
u8 interrupt; /* I2C + 0x20 */
u8 reserved6[3];/* I2C + 0x21 */
u8 mifr;/* I2C + 0x24 */
};
Ugh. I hate all the registers defined in structures thing done for
5200, but I guess it is better to stick with established convention
than do it differently.
Isn't it better than adding random numbers to a pointer and having to
worry about what the pointer is pointing at so that it will multiply
by the right word size? That's a mess when the registers are different
lengths.
A common i2c struct might be a better idea that adds in the dfsrr
register might be better.
You can set the flags for dfsrr use in these set_freq routines too
since they select on CPU type.
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = fsl,mpc8260-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8349-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8540-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8543-i2c, .data =
fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = fsl,mpc8544-i2c, .data =
fsl_i2c_mpc8xxx_div3_set_freq, },
Yes, I think this is okay (but I haven't double checked the values; I
trust you).
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote:
On Thu, Jul 31, 2008 at 11:22 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Grant Likely wrote:
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote:
Wolfgang Grandegger wrote:
I know but we still need an algorithm for MPC52xx and MPC82xx as well.
That's true, but I still think hard-coding values of DFSR and FDR in
the
device
tree is not a good way to do this.
I agree, it should encode real frequencies, not raw register values.
Digging deeper I'm frightened by plenty of platform specific code. We
would
need:
- one table of divider,fdr,dfsr values for the MPC82/3/5/6xx processors
(already available from Timur's U-Boot implementation)
- one table of divider,fdr values for the MPC5200 rev A.
- one table of divider,fdr values for the MPC5200 rev B.
(the Rev. B has two more pre-scaler bits).
Aren't the tables in the manual there just to make it easy for a human
to pick out the line they want? For a computer you'd program the
formula that was used to create the tables.
I have the formulas to create the tables, also for the MPC5200 Rev. A and B.
Oh, hey, even better.
That was not my point. I'm worried about arch specific code in i2c-mpc.c. It
should go somewhere to arch/powerpc.
i2c-mpc *is* arch specific. I really don't think you need to worry
about adding a block of code for each supported SoC family. Just
change the of_match table to look something like this:
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = fsl,mpc8260-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8349-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8540-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8543-i2c, .data =
fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = fsl,mpc8544-i2c, .data =
fsl_i2c_mpc8xxx_div3_set_freq, },
/* keep this only for older device trees with some support
code to figure out
what .data should have pointed to. */
{.compatible = fsl-i2c, },
{},
};
MODULE_DEVICE_TABLE(of, mpc_i2c_of_match);
Cool, this would also make the dfsrr property obsolete. Just the
MPC8544 needs more attention because the I2C clock can be programmed to
be freq/2 or freq/3.
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote:
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = fsl,mpc8260-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8349-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8540-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8543-i2c, .data =
fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = fsl,mpc8544-i2c, .data =
fsl_i2c_mpc8xxx_div3_set_freq, },
So we need to update this table every time there's a new SOC? All 83xx, 85xx,
and 86xx SOCs use the same table. I'd prefer an implementation that does need a
specific entry for each variant of 8[356]xx.
--
Timur Tabi
Linux kernel developer at Freescale
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Timur Tabi wrote:
Grant Likely wrote:
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200b_set_freq, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200_set_freq, },
{.compatible = fsl,mpc8260-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8349-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8540-i2c, .data = fsl_i2c_mpc8xxx_set_freq, },
{.compatible = fsl,mpc8543-i2c, .data =
fsl_i2c_mpc8xxx_div2_set_freq, },
{.compatible = fsl,mpc8544-i2c, .data =
fsl_i2c_mpc8xxx_div3_set_freq, },
So we need to update this table every time there's a new SOC? All 83xx, 85xx,
and 86xx SOCs use the same table. I'd prefer an implementation that does need a
specific entry for each variant of 8[356]xx.
We could add a property source-clock-divider = 2/3 if it's needed!?
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger [EMAIL PROTECTED] wrote: We could add a property source-clock-divider = 2/3 if it's needed!? fsl,source-clock-divider But, yes, this is a good solution (assuming that it is a board or SoC characteristic, and not just a choice that the driver has the option of making on it's own). g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: We could add a property source-clock-divider = 2/3 if it's needed!? How about we just get U-Boot to put the core frequency in the device tree? -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote: On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger [EMAIL PROTECTED] wrote: We could add a property source-clock-divider = 2/3 if it's needed!? fsl,source-clock-divider But, yes, this is a good solution (assuming that it is a board or SoC characteristic, and not just a choice that the driver has the option of making on it's own). I was going to suggest the actual clock frequency of the I2C device. It would be value of gd-i2c1_clk or gd-i2c2_clk from U-Boot. The actual divider value is meaningless. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote: This is a solved problem. The device tree simple claims compatibility with an older part that has the identical register-level interface. That would assume that the clock frequency is the only thing that decides compatibility, which may technically be true now, but I don't think it's a good idea. I don't understand what's wrong with simply specifying the actual clock frequency that the device uses? It varies from SOC to SOC, but U-Boot calculates today already: #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \ defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555) gd-i2c1_clk = sys_info.freqSystemBus; #elif defined(CONFIG_MPC8544) /* * On the 8544, the I2C clock is the same as the SEC clock. This can be * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See * 4.4.3.3 of the 8544 RM. Note that this might actually work for all * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544. */ if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG) gd-i2c1_clk = sys_info.freqSystemBus / 3; else gd-i2c1_clk = sys_info.freqSystemBus / 2; #else /* Most 85xx SOCs use CCB/2, so this is the default behavior. */ gd-i2c1_clk = sys_info.freqSystemBus / 2; #endif gd-i2c2_clk = gd-i2c1_clk; We need this ugliness in U-Boot. Let's take advantage of this and do something clean and elegant in the device tree. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 01:10:30PM -0500, Timur Tabi wrote: Grant Likely wrote: On Thu, Jul 31, 2008 at 12:07 PM, Wolfgang Grandegger [EMAIL PROTECTED] wrote: We could add a property source-clock-divider = 2/3 if it's needed!? fsl,source-clock-divider But, yes, this is a good solution (assuming that it is a board or SoC characteristic, and not just a choice that the driver has the option of making on it's own). I was going to suggest the actual clock frequency of the I2C device. It would be value of gd-i2c1_clk or gd-i2c2_clk from U-Boot. The actual divider value is meaningless. I'm cool with that too, as long as it gets documented g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 01:13:10PM -0500, Timur Tabi wrote: Grant Likely wrote: This is a solved problem. The device tree simple claims compatibility with an older part that has the identical register-level interface. That would assume that the clock frequency is the only thing that decides compatibility, which may technically be true now, but I don't think it's a good idea. No it doesn't, it depends on the register interface to decide compatibility. Clock interface is part of that. I suggested encoding the clock divider directly in compatible (implicit in the SoC version), but it doesn't have to be that way. If clock freq is obtained from another property or some other method then that is okay too. What is important is that if compatibility is claimed, then it must really be compatible! If some new part appears in the future that breaks our assumptions, then we'll need to rework the driver anyway and the new part will *not* claim compatibility with the old. I don't understand what's wrong with simply specifying the actual clock frequency that the device uses? It varies from SOC to SOC, but U-Boot calculates today already: There is nothing wrong with it (as long as we agree and it gets documented). I certainly don't have a problem with doing it this way. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Timur Tabi [EMAIL PROTECTED] wrote:
Grant Likely wrote:
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what
specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
That answer is dependent on the actual clock input to the device, which is
external to the device. I wouldn't call the input frequency a property of
the
I2C device.
I suggested encoding
the clock divider directly in compatible (implicit in the SoC version),
but it doesn't have to be that way. If clock freq is obtained from
another property or some other method then that is okay too.
I think we agree on that.
There is nothing wrong with it (as long as we agree and it gets
documented). I certainly don't have a problem with doing it this way.
I propose the property clock-frequency, like this:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 0;
cell-index = 0;
compatible = fsl-i2c;
reg = 0x3000 0x100;
interrupts = 14 0x8;
interrupt-parent = ipic;
dfsrr;
The existence of the dfsrr and fsl,mpc5200-i2c attributes imply to me
that the compatible strings were not done correctly. If these devices
really were compatible we wouldn't need extra attributes to tell them
apart.
So I'm with Grant on adding compatible strings sufficient to remove
the need for dfsrr and fsl,mpc5200-i2c.
Something like...
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200, },
{.compatible = fsl,mpc8xxx-i2c, .data = fsl_i2c_dfsrr, },
As for the source clock, how about creating a new global like
ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
right clock value into the variable. For mpc8 get it from uboot.
mpc5200 can easily compute it from ppc_proc_freq and checking how the
ipb divider is set. That will move the clock problem out of the i2c
driver.
I'd like to move towards a more generic uboot that gets the processor
minimally running and then use the device tree to customize as many
things as possible. But that's just my opinion.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Timur Tabi wrote:
Grant Likely wrote:
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
If it affects the values you need to write to the registers to achieve a
given result, how is it not a difference in the register interface?
I propose the property clock-frequency, like this:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 0;
cell-index = 0;
compatible = fsl-i2c;
reg = 0x3000 0x100;
interrupts = 14 0x8;
interrupt-parent = ipic;
dfsrr;
clock-frequency = 0xblablabla; -- added by U-Boot
};
A clock-frequency property is OK, and is in line with what we do in
other types of nodes. However, in the long run it might be nice to
introduce some sort of clock binding where, for example, the i2c node
can point to a clock elsewhere in the device tree as an input clock.
That way, less knowledge is required by the firmware to poke values all
over the place, and it also allows one to describe situations where the
frequency of the input clock can change (such as in low-power modes).
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote:
The existence of the dfsrr and fsl,mpc5200-i2c attributes imply to me
that the compatible strings were not done correctly. If these devices
really were compatible we wouldn't need extra attributes to tell them
apart.
I agree with that.
So I'm with Grant on adding compatible strings sufficient to remove
the need for dfsrr and fsl,mpc5200-i2c.
Let's just make sure we don't break backwards compatibility.
Something like...
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200, },
{.compatible = fsl,mpc8xxx-i2c, .data = fsl_i2c_dfsrr, },
That seems ok.
As for the source clock, how about creating a new global like
ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
right clock value into the variable. For mpc8 get it from uboot.
mpc5200 can easily compute it from ppc_proc_freq and checking how the
ipb divider is set. That will move the clock problem out of the i2c
driver.
I don't want to differentiate between 52xx and 8xxx any more than we have to.
If 8xxx has the clock frequency in the device tree, then 52xx should have it
there, too.
For backwards compatibility purposes, we can have the driver provide a
hard-coded value of some kind if the property does not exist.
I'd like to move towards a more generic uboot that gets the processor
minimally running and then use the device tree to customize as many
things as possible. But that's just my opinion.
U-Boot needs to configure the I2C bus speed because U-Boot uses the I2C. We
should capitalize on that by taking the information that U-Boot has calculated
and re-use it.
--
Timur Tabi
Linux kernel developer at Freescale
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Scott Wood [EMAIL PROTECTED] wrote:
Timur Tabi wrote:
Grant Likely wrote:
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what
specific
values to put in the FDR and DFSR registers to get a desired I2C bus
speed.
If it affects the values you need to write to the registers to achieve a
given result, how is it not a difference in the register interface?
I propose the property clock-frequency, like this:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 0;
cell-index = 0;
compatible = fsl-i2c;
reg = 0x3000 0x100;
interrupts = 14 0x8;
interrupt-parent = ipic;
dfsrr;
clock-frequency = 0xblablabla; -- added by
U-Boot
};
A clock-frequency property is OK, and is in line with what we do in other
types of nodes. However, in the long run it might be nice to introduce some
sort of clock binding where, for example, the i2c node can point to a clock
elsewhere in the device tree as an input clock.
That way, less knowledge is required by the firmware to poke values all
over the place, and it also allows one to describe situations where the
frequency of the input clock can change (such as in low-power modes).
PowerPC,[EMAIL PROTECTED] {
timebase-frequency = 0; /* From Bootloader */
bus-frequency = 0;/* From Bootloader */
clock-frequency = 0; /* From Bootloader */
};
The mpc5200 code already has mpc52xx_find_ipb_freq() to get it.
I should grep before suggesting things.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 01:35:51PM -0500, Timur Tabi wrote:
Grant Likely wrote:
No it doesn't, it depends on the register interface to decide
compatibility. Clock interface is part of that.
I don't think so. The interface for programming the clock registers is
identical on all 8[356]xx parts. The only thing that matters is what specific
values to put in the FDR and DFSR registers to get a desired I2C bus speed.
That answer is dependent on the actual clock input to the device, which is
external to the device. I wouldn't call the input frequency a property of the
I2C device.
Okay, I accept that argument. Make input frequency a property and be
done with it.
I suggested encoding
the clock divider directly in compatible (implicit in the SoC version),
but it doesn't have to be that way. If clock freq is obtained from
another property or some other method then that is okay too.
I think we agree on that.
indeed.
There is nothing wrong with it (as long as we agree and it gets
documented). I certainly don't have a problem with doing it this way.
I propose the property clock-frequency, like this:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 0;
cell-index = 0;
compatible = fsl-i2c;
reg = 0x3000 0x100;
interrupts = 14 0x8;
interrupt-parent = ipic;
dfsrr;
clock-frequency = 0xblablabla; -- added by U-Boot
};
I'm okay with this.
Note that the dfsrr property already differentiates between 8xxx and 52xx, so
maybe we don't need any other device tree changes.
The whole dfsrr property is a really bad idea, and it just replaces the
equally bad idea of an mpc5200-clocking property which is currently in
use. This bit should definitely be determined via compatible.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Scott Wood wrote: Timur Tabi wrote: Grant Likely wrote: No it doesn't, it depends on the register interface to decide compatibility. Clock interface is part of that. I don't think so. The interface for programming the clock registers is identical on all 8[356]xx parts. The only thing that matters is what specific values to put in the FDR and DFSR registers to get a desired I2C bus speed. If it affects the values you need to write to the registers to achieve a given result, how is it not a difference in the register interface? I think we're splitting hairs, here. The code for actually programming the registers is the same, regardless of the input frequency. It's just that the input frequency is a value needed to calculate the right value. But like I said, I don't think this distinction is that relevant. A clock-frequency property is OK, and is in line with what we do in other types of nodes. However, in the long run it might be nice to introduce some sort of clock binding where, for example, the i2c node can point to a clock elsewhere in the device tree as an input clock. The only problem with that is that the actual input clock to the I2C device is not the same as any other device. It's a unique clock. Look at the code I had to write to figure out this clock just on 85xx: /* * The base clock for I2C depends on the actual SOC. Unfortunately, * there is no pattern that can be used to determine the frequency, so * the only choice is to look up the actual SOC number and use the value * for that SOC. This information is taken from application note * AN2919. */ #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \ defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555) gd-i2c1_clk = sys_info.freqSystemBus; #elif defined(CONFIG_MPC8544) /* * On the 8544, the I2C clock is the same as the SEC clock. This can be * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See * 4.4.3.3 of the 8544 RM. Note that this might actually work for all * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544. */ if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG) gd-i2c1_clk = sys_info.freqSystemBus / 3; else gd-i2c1_clk = sys_info.freqSystemBus / 2; #else /* Most 85xx SOCs use CCB/2, so this is the default behavior. */ gd-i2c1_clk = sys_info.freqSystemBus / 2; #endif This is just for 85xx, and it only applies to the I2C devices. That way, less knowledge is required by the firmware to poke values all over the place, and it also allows one to describe situations where the frequency of the input clock can change (such as in low-power modes). I don't think it's possible to do what you want to do. I2C clocking is completely screwed up, and that's just the way the hardware was designed. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Timur Tabi wrote: Scott Wood wrote: A clock-frequency property is OK, and is in line with what we do in other types of nodes. However, in the long run it might be nice to introduce some sort of clock binding where, for example, the i2c node can point to a clock elsewhere in the device tree as an input clock. The only problem with that is that the actual input clock to the I2C device is not the same as any other device. It's a unique clock. Look at the code I had to write to figure out this clock just on 85xx: IIRC, only the divider is unique, and the divider that is applied to the input clock can be specified in the i2c node (either implicitly in compatible, or explicitly via a property). -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 02:57:25PM -0400, Jon Smirl wrote:
On 7/31/08, Timur Tabi [EMAIL PROTECTED] wrote:
Grant Likely wrote:
I propose the property clock-frequency, like this:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 0;
cell-index = 0;
compatible = fsl-i2c;
reg = 0x3000 0x100;
interrupts = 14 0x8;
interrupt-parent = ipic;
dfsrr;
The existence of the dfsrr and fsl,mpc5200-i2c attributes imply to me
that the compatible strings were not done correctly. If these devices
really were compatible we wouldn't need extra attributes to tell them
apart.
Yes, exactly right.
So I'm with Grant on adding compatible strings sufficient to remove
the need for dfsrr and fsl,mpc5200-i2c.
Something like...
static const struct of_device_id mpc_i2c_of_match[] = {
{.compatible = fsl,mpc5200b-i2c, .data = fsl_i2c_mpc5200, },
{.compatible = fsl,mpc5200-i2c, .data = fsl_i2c_mpc5200, },
{.compatible = fsl,mpc8xxx-i2c, .data = fsl_i2c_dfsrr, },
Yes, but I don't agree with the last entry in this list.
fsl,mpc8xxx-i2c is not a good value. Use an actual part number
instead and have newer devices claim compatibility with the old.
The problem with 8xxx is that you don't know if/when another 8xxx
will arrive on the scene which does not conform to already made
assumptions. If you specify an exact SoC part, then the problem goes
away without any downside.
As for the source clock, how about creating a new global like
ppc_proc_freq called ppc_ipb_freq. The platform code can then set the
right clock value into the variable. For mpc8 get it from uboot.
mpc5200 can easily compute it from ppc_proc_freq and checking how the
ipb divider is set. That will move the clock problem out of the i2c
driver.
In general, I don't like adding additional global or machine vars to
Linux. I just don't think it in necessary and it can quickly get out of
control. Instead, For 5200 I've exported a mpc52xx specific function
that returns the clock frequency. The function can be adapted over time
to handle new cases on the 52xx platform and it get compiled out if 5200
is not in use.
As an alternative, clock-frequency could be made an optional property.
If it isn't specified, then get the bus-frequency property from the
parent node instead.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: No, the source clock is not identical for all 8[356]xx. Some use half or even a third of the SOC clock frequency. The platform clock divided by 2 or 3 *is* the source clock to the I2C. That's what I'm talking about. Linux must determine the real source clock frequency somehow. We may introduce the SOC property i2c-clock-frequency, which could be fixed up by U-Boot or a pre-loader (in case U-Boot is not used). Like for other frequency properties as well. That's what I'm proposing, but drop the i2c-. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 09:54:48PM +0200, Wolfgang Grandegger wrote:
Thinking more about it, it would be best to add the property
i2c-clock-divider to the soc node and implement fsl_get_i2c_freq() in
a similar way:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 1;
device_type = soc;
ranges = 0x0 0xe000 0x10;
reg = 0xe000 0x1000; // CCSRBAR 1M
bus-frequency = 0;
i2c-clock-divider = 2;
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
Ugh. This is specifically related to the i2c device, so please place
the property in the i2c device. Remember, device tree design is not
about what will make the implementation simplest, but rather about what
describes the hardware in the best way.
Now, if you can argue that i2c-clock-frequency is actually a separate
clock domain defined at the SoC level, not the i2c device level, then I
will change my opinion.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: But clock-frequency, aka bus-frequency, is already used by fsl_get_sys_freq(): http://lxr.linux.no/linux+v2.6.26/arch/powerpc/sysdev/fsl_soc.c#L80 So? clock-frequency is a per-node property. I use it in the codec node on the 8610 (mpc8610_hpcd.dts). It does not mean platform clock frequency. U-Boot could then fixup that value like bus-frequency() and the i2c-mpc driver simply calls fsl_get_i2c_freq(). This is just more complicated than it needs to be. Why should the I2C driver fetch the platform clock and the divider from the parent node, and then do additional math, when it could just get the value it needs right from the node it's probing? Besides, U-Boot does not currently store the divider value. Look at the code I've posted twice already - it stores the frequency in i2c1_clk. So now I would need to create another variable in the gd_t to store the divider? No thanks. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Timur Tabi wrote: Wolfgang Grandegger wrote: But clock-frequency, aka bus-frequency, is already used by fsl_get_sys_freq(): http://lxr.linux.no/linux+v2.6.26/arch/powerpc/sysdev/fsl_soc.c#L80 So? clock-frequency is a per-node property. I use it in the codec node on the 8610 (mpc8610_hpcd.dts). It does not mean platform clock frequency. U-Boot could then fixup that value like bus-frequency() and the i2c-mpc driver simply calls fsl_get_i2c_freq(). This is just more complicated than it needs to be. Why should the I2C driver fetch the platform clock and the divider from the parent node, and then do additional math, when it could just get the value it needs right from the node it's probing? I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. The first one is common for all I2C devices, the second can be different. What properties would you like to use for defining both? Besides, U-Boot does not currently store the divider value. Look at the code I've posted twice already - it stores the frequency in i2c1_clk. So now I would need to create another variable in the gd_t to store the divider? No thanks. OK, that's an argument but it's biased by U-Boot. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote:
On Thu, Jul 31, 2008 at 09:54:48PM +0200, Wolfgang Grandegger wrote:
Thinking more about it, it would be best to add the property
i2c-clock-divider to the soc node and implement fsl_get_i2c_freq() in
a similar way:
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 1;
device_type = soc;
ranges = 0x0 0xe000 0x10;
reg = 0xe000 0x1000; // CCSRBAR 1M
bus-frequency = 0;
i2c-clock-divider = 2;
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
Ugh. This is specifically related to the i2c device, so please place
the property in the i2c device. Remember, device tree design is not
about what will make the implementation simplest, but rather about what
describes the hardware in the best way.
Now, if you can argue that i2c-clock-frequency is actually a separate
clock domain defined at the SoC level, not the i2c device level, then I
will change my opinion.
Is it not exactly that? For me it's not a per I2C device property, at least.
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Timur Tabi wrote: Wolfgang Grandegger wrote: I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. There are two frequencies: 1) The frequency of the input clock to the I2C device, after it has gone through a divider. This is what I call the I2C clock frequency and what I think belongs in the clock-frequency property. This is usually the platform clock divided by 1, 2, or 3. OK. 2) The speed of the I2C bus, as seen by devices on that bus. This is usually 400KHz. Which should be defined with the property current-speed, right? Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. There are two frequencies: 1) The frequency of the input clock to the I2C device, after it has gone through a divider. This is what I call the I2C clock frequency and what I think belongs in the clock-frequency property. This is usually the platform clock divided by 1, 2, or 3. 2) The speed of the I2C bus, as seen by devices on that bus. This is usually 400KHz. The first one is common for all I2C devices, the second can be different. What properties would you like to use for defining both? The platform clock has no value to the I2C hardware, so I don't care anything about it. Besides, U-Boot does not currently store the divider value. Look at the code I've posted twice already - it stores the frequency in i2c1_clk. So now I would need to create another variable in the gd_t to store the divider? No thanks. OK, that's an argument but it's biased by U-Boot. As long as a method that is favorable to U-Boot does not put any undo hardship on non-U-Boot methods, I would say that it is the preferred method. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: Is it not exactly that? For me it's not a per I2C device property, at least. Of course it's a per-I2C device property. The input frequency to the I2C device is only seen by the I2C device, and no other device. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 2:19 PM, Timur Tabi [EMAIL PROTECTED] wrote: Wolfgang Grandegger wrote: I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. There are two frequencies: 1) The frequency of the input clock to the I2C device, after it has gone through a divider. This is what I call the I2C clock frequency and what I think belongs in the clock-frequency property. This is usually the platform clock divided by 1, 2, or 3. 2) The speed of the I2C bus, as seen by devices on that bus. This is usually 400KHz. analogous example: serial nodes use two properties; clock-frequency and current-speed, one for clock frequency routed into the device and one for the current baud rate. It is completely relevant to put the effective clock frequency into the i2c device node. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Timur Tabi wrote:
Wolfgang Grandegger wrote:
But clock-frequency, aka bus-frequency, is already used by
fsl_get_sys_freq():
http://lxr.linux.no/linux+v2.6.26/arch/powerpc/sysdev/fsl_soc.c#L80
So? clock-frequency is a per-node property. I use it in the codec node
on the
8610 (mpc8610_hpcd.dts). It does not mean platform clock frequency.
U-Boot could then fixup that value like bus-frequency() and the i2c-mpc
driver simply calls fsl_get_i2c_freq().
This is just more complicated than it needs to be. Why should the I2C
driver
fetch the platform clock and the divider from the parent node, and then do
additional math, when it could just get the value it needs right from the
node
it's probing?
I'm a bit confused. The frequency of the I2C source clock and the real I2C
clock frequency are two different things. The first one is common for all
I2C devices, the second can be different. What properties would you like to
use for defining both?
For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get
the source clock for the i2c devices. Each i2c node in the device tree
can then specify clock-frequency = 40; or let it default. You
have the input clock and the desired clock, do some math and you can
set FDR.
For the 8xxx chips there is no simple solution for obtain the input
clock for the i2c section. Maybe create something like i2c-frequency
and set it from uboot. The make another accessor function like
mpc8xxx_find_i2c_freq().
PowerPC,[EMAIL PROTECTED] {
timebase-frequency = 0; /* From Bootloader */
bus-frequency = 0;/* From Bootloader */
clock-frequency = 0; /* From Bootloader */
i2c-frequency = 0;/* From Bootloader */
};
Instead of creating i2c-frequency in the device tree, you could put
this piece of code in the the mpc8xxx platform driver and use it to
implement mpc8xxx_find_i2c_freq(). Read the PORDEVSR2_SEC_CFG bit back
from whatever uboot set it too.
#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555)
gd-i2c1_clk = sys_info.freqSystemBus;
#elif defined(CONFIG_MPC8544)
/*
* On the 8544, the I2C clock is the same as the SEC clock. This can be
* either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See
* 4.4.3.3 of the 8544 RM. Note that this might actually work for all
* 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the
* PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544.
*/
if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG)
gd-i2c1_clk = sys_info.freqSystemBus / 3;
else
gd-i2c1_clk = sys_info.freqSystemBus / 2;
#else
/* Most 85xx SOCs use CCB/2, so this is the default behavior. */
gd-i2c1_clk = sys_info.freqSystemBus / 2;
#endif
gd-i2c2_clk = gd-i2c1_clk;
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 2:32 PM, Jon Smirl [EMAIL PROTECTED] wrote: On 7/31/08, Wolfgang Grandegger [EMAIL PROTECTED] wrote: Timur Tabi wrote: Wolfgang Grandegger wrote: But clock-frequency, aka bus-frequency, is already used by fsl_get_sys_freq(): http://lxr.linux.no/linux+v2.6.26/arch/powerpc/sysdev/fsl_soc.c#L80 So? clock-frequency is a per-node property. I use it in the codec node on the 8610 (mpc8610_hpcd.dts). It does not mean platform clock frequency. U-Boot could then fixup that value like bus-frequency() and the i2c-mpc driver simply calls fsl_get_i2c_freq(). This is just more complicated than it needs to be. Why should the I2C driver fetch the platform clock and the divider from the parent node, and then do additional math, when it could just get the value it needs right from the node it's probing? I'm a bit confused. The frequency of the I2C source clock and the real I2C clock frequency are two different things. The first one is common for all I2C devices, the second can be different. What properties would you like to use for defining both? How is the divider controlled? Is it a fixed property of the SoC? a shared register setting? or a register setting within the i2c device? (My answer depends on the layout) g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: 2) The speed of the I2C bus, as seen by devices on that bus. This is usually 400KHz. Which should be defined with the property current-speed, right? I would use something like bus-speed, but yes. The word current shouldn't be in a property string. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get the source clock for the i2c devices. Each i2c node in the device tree can then specify clock-frequency = 40; or let it default. You 400KHz is the *speed* of the I2C bus, so let's be sure to use speed in the property name. Reserve the word bus for the input clock to the I2C device. #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \ defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555) gd-i2c1_clk = sys_info.freqSystemBus; #elif defined(CONFIG_MPC8544) /* * On the 8544, the I2C clock is the same as the SEC clock. This can be * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See * 4.4.3.3 of the 8544 RM. Note that this might actually work for all * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544. */ if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG) gd-i2c1_clk = sys_info.freqSystemBus / 3; else gd-i2c1_clk = sys_info.freqSystemBus / 2; #else /* Most 85xx SOCs use CCB/2, so this is the default behavior. */ gd-i2c1_clk = sys_info.freqSystemBus / 2; #endif gd-i2c2_clk = gd-i2c1_clk; I think the whole point is to eliminate duplicating this code in the Linux driver. It's hideously ugly, so it should be limited as much as possible. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Timur Tabi [EMAIL PROTECTED] wrote: Jon Smirl wrote: For mpc5200 it is easy, mpc52xx_find_ipb_freq() already exists to get the source clock for the i2c devices. Each i2c node in the device tree can then specify clock-frequency = 40; or let it default. You 400KHz is the *speed* of the I2C bus, so let's be sure to use speed in the property name. Reserve the word bus for the input clock to the I2C device. #if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \ defined(CONFIG_MPC8560) || defined(CONFIG_MPC8555) gd-i2c1_clk = sys_info.freqSystemBus; #elif defined(CONFIG_MPC8544) /* * On the 8544, the I2C clock is the same as the SEC clock. This can be * either CCB/2 or CCB/3, depending on the value of cfg_sec_freq. See * 4.4.3.3 of the 8544 RM. Note that this might actually work for all * 85xx, but only the 8544 has cfg_sec_freq, so it's unknown if the * PORDEVSR2_SEC_CFG bit is 0 on all 85xx boards that are not an 8544. */ if (gur-pordevsr2 MPC85xx_PORDEVSR2_SEC_CFG) gd-i2c1_clk = sys_info.freqSystemBus / 3; else gd-i2c1_clk = sys_info.freqSystemBus / 2; #else /* Most 85xx SOCs use CCB/2, so this is the default behavior. */ gd-i2c1_clk = sys_info.freqSystemBus / 2; #endif gd-i2c2_clk = gd-i2c1_clk; I think the whole point is to eliminate duplicating this code in the Linux driver. It's hideously ugly, so it should be limited as much as possible. It wouldn't go into the i2c driver, it would go into the mpc8xxx platform driver. Why is it bad to put it into the mpc8xxx platform driver? It is an accurate description of the mpc8xxx platform isn't it? -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote: How is the divider controlled? Is it a fixed property of the SoC? Yes. The divider is either 1, 2, or 3, and the only way to know which one it is is to look up the specific SOC model number. And depending on the SOC model, there may also be a register that needs to be looked up. a shared register setting? or a register setting within the i2c device? The I2C device itself has no idea what the divider is. It only sees the result of the divider. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Thu, Jul 31, 2008 at 03:37:07PM -0500, Timur Tabi wrote: Grant Likely wrote: How is the divider controlled? Is it a fixed property of the SoC? Yes. The divider is either 1, 2, or 3, and the only way to know which one it is is to look up the specific SOC model number. And depending on the SOC model, there may also be a register that needs to be looked up. a shared register setting? or a register setting within the i2c device? The I2C device itself has no idea what the divider is. It only sees the result of the divider. Then that absolutely suggests to me that either the final clock or the divider should be encoded in the i2c node; not in the soc node. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: It wouldn't go into the i2c driver, it would go into the mpc8xxx platform driver. Why is it bad to put it into the mpc8xxx platform driver? It is an accurate description of the mpc8xxx platform isn't it? There's no need to put that code in the platform driver because U-Boot will put the final result in the device tree! That way, we won't need to update the platform driver *and* U-Boot for any new SOC. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/31/08, Grant Likely [EMAIL PROTECTED] wrote: On Thu, Jul 31, 2008 at 03:37:07PM -0500, Timur Tabi wrote: Grant Likely wrote: How is the divider controlled? Is it a fixed property of the SoC? Yes. The divider is either 1, 2, or 3, and the only way to know which one it is is to look up the specific SOC model number. And depending on the SOC model, there may also be a register that needs to be looked up. a shared register setting? or a register setting within the i2c device? The I2C device itself has no idea what the divider is. It only sees the result of the divider. Then that absolutely suggests to me that either the final clock or the divider should be encoded in the i2c node; not in the soc node. Isn't there a single global divider that generates all the i2c source clocks? You don't want to copy a global value into each i2c node. Aren't we talking about the /2 or /3 or /1 divider that appears to be randomly implemented on various members of the mpc8xxx family? g. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: Isn't there a single global divider that generates all the i2c source clocks? You don't want to copy a global value into each i2c node. Why not? The compatible is pretty much always going to be the same for the i2c node, but we copy that, as well. Likewise for the clock-frequency of serial nodes. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: Isn't there a single global divider that generates all the i2c source clocks? You don't want to copy a global value into each i2c node. Why not? There are only two I2C devices, and it's theoretically possible for them to have different input clock frequencies. Keeping it in the I2C node allows the I2C driver to reference a property directly in the node that its probing. Aren't we talking about the /2 or /3 or /1 divider that appears to be randomly implemented on various members of the mpc8xxx family? Yes. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: On 7/31/08, Timur Tabi [EMAIL PROTECTED] wrote: Jon Smirl wrote: Isn't there a single global divider that generates all the i2c source clocks? You don't want to copy a global value into each i2c node. Why not? There are only two I2C devices, and it's theoretically possible for them to have different input clock frequencies. Keeping it in the I2C node allows the I2C driver to reference a property directly in the node that its probing. But that's the same as saying we should copy the system clock frequency into all of the PSC nodes because we might implement hardware where they aren't all clocked off from the same input clock source. Aren't we talking about the /2 or /3 or /1 divider that appears to be randomly implemented on various members of the mpc8xxx family? I don't this these dividers or clocks need to be exposed at all if you'd just put that ugly code snippet into your platform driver. U-Boot does not (yet) use the FDT and it has therefore to use that ugly code to derive the I2C input clock frequency. I think its completely legal to put that hardware specific information into the FDT and get rid of such code. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jon Smirl wrote: But that's the same as saying we should copy the system clock frequency into all of the PSC nodes because we might implement hardware where they aren't all clocked off from the same input clock source. The I2C clock is only visible to the I2C devices. The system clock is seen by many devices. There's the difference. Aren't we talking about the /2 or /3 or /1 divider that appears to be randomly implemented on various members of the mpc8xxx family? I don't this these dividers or clocks need to be exposed at all if you'd just put that ugly code snippet into your platform driver. That's why I don't think the divider belongs in the device tree. Just put the actual resulting clock frequency in the device tree. Besides, putting that snippet in the platform driver *is* exposing it. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: U-Boot does not (yet) use the FDT and it has therefore to use that ugly code to derive the I2C input clock frequency. I think its completely legal to put that hardware specific information into the FDT and get rid of such code. Huh? U-Boot initializes several fields and creates several properties in the FDT today, so what's wrong with adding another one? The clock frequencies have always been calculated by U-Boot, because putting them in the device tree is a bad idea. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, Jul 25, 2008 at 05:34:49PM +0200, Wolfgang Grandegger wrote: Grant Likely wrote: On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote: I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is not defined for the I2C node (instead of the debatable default values). This is a perfectly valid option. Personally, I'd prefer it encoded in the device tree, but if it looks like a valid speed has already been programmed in then I'm cool with the driver just preserving that. If it turns out to causes problems the we can always change the code to be more conservative later. How should the Linux driver decide if the registers have been already set by the boot-loader? The reset-values might be good as well. Therefore, if clock-frequency is not specified, the driver may simply not touch the fdr and dfsr registers (overtaking the values from the boot-loader). I'm okay with that. If the property isn't present then it is probably okay to just assume that it is usable. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, Jul 25, 2008 at 11:19:41AM -0500, Timur Tabi wrote: Wolfgang Grandegger wrote: I know but we still need an algorithm for MPC52xx and MPC82xx as well. That's true, but I still think hard-coding values of DFSR and FDR in the device tree is not a good way to do this. I agree, it should encode real frequencies, not raw register values. g. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Hi Wolfgang,
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties fdr and dfsrr for the
corresponding I2C registers to make the speed configurable via FDT,
e.g.:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
reg = 0x3100 0x100;
interrupts = 43 2;
interrupt-parent = mpic;
dfsrr = 0x20;
fdr = 0x03;
};
Would it be possible to use the standard property clock-frequency for this
and calculate the register settings in the driver?
Thanks,
Jochen
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Jochen Friedrich wrote:
Hi Wolfgang,
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties fdr and dfsrr for the
corresponding I2C registers to make the speed configurable via FDT,
e.g.:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
reg = 0x3100 0x100;
interrupts = 43 2;
interrupt-parent = mpic;
dfsrr = 0x20;
fdr = 0x03;
};
Would it be possible to use the standard property clock-frequency for this
and calculate the register settings in the driver?
Almost everything is possible in software, just for what price ;-).
U-Boot has some code in drivers/i2c/fsl_i2c.c to determine reasonable
fdr and dfsrr values for the MPC83/5/6xx boards. For the MPC82xx and
MPC85xx it's even more sophisticated.
I was also thinking to just overtake the U-Boot settings if fdt and
dfsrr is not defined for the I2C node (instead of the debatable default
values).
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Jochen Friedrich wrote:
Hi Wolfgang,
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties fdr and dfsrr for the
corresponding I2C registers to make the speed configurable via FDT, e.g.:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
reg = 0x3100 0x100;
interrupts = 43 2;
interrupt-parent = mpic;
dfsrr = 0x20;
fdr = 0x03;
};
Would it be possible to use the standard property clock-frequency for
this
and calculate the register settings in the driver?
Yes, please use something like clock-frequency or current-speed and do
the calculation.
Almost everything is possible in software, just for what price ;-). U-Boot
has some code in drivers/i2c/fsl_i2c.c to determine reasonable fdr and dfsrr
values for the MPC83/5/6xx boards. For the MPC82xx and MPC85xx it's even
more sophisticated.
I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is
not defined for the I2C node (instead of the debatable default values).
This is a perfectly valid option. Personally, I'd prefer it encoded
in the device tree, but if it looks like a valid speed has already
been programmed in then I'm cool with the driver just preserving that.
If it turns out to causes problems the we can always change the code
to be more conservative later.
Thanks,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely [EMAIL PROTECTED] wrote: Yes, please use something like clock-frequency or current-speed and do the calculation. Ditto. I already wrote the code that does that for U-Boot, so all you need to do is port it. Although I'm curious, if U-Boot already programs the speed, why does the driver program it again? -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
On 7/25/08, Timur Tabi [EMAIL PROTECTED] wrote:
On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely [EMAIL PROTECTED] wrote:
Yes, please use something like clock-frequency or current-speed and do
the calculation.
Ditto. I already wrote the code that does that for U-Boot, so all you
need to do is port it.
I calculate the register values in the i2s driver. There is the issue
of requesting a frequency the hardware can't make exactly (Freescale -
more FractionalN dividers please!).
if (dir == SND_SOC_CLOCK_OUT) {
psc_i2s-sysclk = freq;
if (clk_id == MPC52xx_CLK_CELLSLAVE) {
psc_i2s-sicr |= MPC52xx_PSC_SICR_CELLSLAVE |
MPC52xx_PSC_SICR_GENCLK;
} else { /* MPC52xx_CLK_INTERNAL */
psc_i2s-sicr = ~MPC52xx_PSC_SICR_CELLSLAVE;
psc_i2s-sicr |= MPC52xx_PSC_SICR_GENCLK;
clkdiv = ppc_proc_freq / freq;
err = ppc_proc_freq % freq;
if (err freq / 2)
clkdiv++;
dev_dbg(psc_i2s-dev, psc_i2s_set_sysclk(clkdiv %d
freq error=%ldHz)\n,
clkdiv, (ppc_proc_freq / clkdiv -
freq));
return mpc52xx_set_psc_clkdiv(psc_i2s-dai.id + 1,
clkdiv);
}
}
if (psc_i2s-sysclk) {
framesync = bits * 2;
bitclk = (psc_i2s-sysclk) / (params_rate(params) * framesync);
/* bitclk field is byte swapped due to mpc5200/b compatibility
*/
value = ((framesync - 1) 24) |
(((bitclk - 1) 0xFF) 16) | ((bitclk - 1) 0xFF00);
dev_dbg(psc_i2s-dev, %s(substream=%p) rate=%i sysclk=%i
framesync=%i bitclk=%i reg=%X\n,
__FUNCTION__, substream, params_rate(params),
psc_i2s-sysclk,
framesync, bitclk, value);
out_be32(psc_i2s-psc_regs-ccr, value);
out_8(psc_i2s-psc_regs-ctur, bits - 1);
}
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Timur Tabi wrote: On Fri, Jul 25, 2008 at 8:12 AM, Grant Likely [EMAIL PROTECTED] wrote: Yes, please use something like clock-frequency or current-speed and do the calculation. Ditto. I already wrote the code that does that for U-Boot, so all you need to do is port it. I know but we still need an algorithm for MPC52xx and MPC82xx as well. Although I'm curious, if U-Boot already programs the speed, why does the driver program it again? Maybe because it's not obvious for the driver if the registers have already been configured by the boot-loader. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Grant Likely wrote:
On Fri, Jul 25, 2008 at 5:04 AM, Wolfgang Grandegger [EMAIL PROTECTED] wrote:
Jochen Friedrich wrote:
Hi Wolfgang,
The I2C driver for the MPC currently uses a fixed speed hard-coded into
the driver. This patch adds the FDT properties fdr and dfsrr for the
corresponding I2C registers to make the speed configurable via FDT, e.g.:
[EMAIL PROTECTED] {
compatible = fsl-i2c;
reg = 0x3100 0x100;
interrupts = 43 2;
interrupt-parent = mpic;
dfsrr = 0x20;
fdr = 0x03;
};
Would it be possible to use the standard property clock-frequency for
this
and calculate the register settings in the driver?
Yes, please use something like clock-frequency or current-speed and do
the calculation.
Almost everything is possible in software, just for what price ;-). U-Boot
has some code in drivers/i2c/fsl_i2c.c to determine reasonable fdr and dfsrr
values for the MPC83/5/6xx boards. For the MPC82xx and MPC85xx it's even
more sophisticated.
I was also thinking to just overtake the U-Boot settings if fdt and dfsrr is
not defined for the I2C node (instead of the debatable default values).
This is a perfectly valid option. Personally, I'd prefer it encoded
in the device tree, but if it looks like a valid speed has already
been programmed in then I'm cool with the driver just preserving that.
If it turns out to causes problems the we can always change the code
to be more conservative later.
How should the Linux driver decide if the registers have been already
set by the boot-loader? The reset-values might be good as well.
Therefore, if clock-frequency is not specified, the driver may simply
not touch the fdr and dfsr registers (overtaking the values from the
boot-loader).
What do you think.
Wolfgang.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
Wolfgang Grandegger wrote: I know but we still need an algorithm for MPC52xx and MPC82xx as well. That's true, but I still think hard-coding values of DFSR and FDR in the device tree is not a good way to do this. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
