* Re: Board level compatibility matching
From: Grant Likely @ 2008-07-31 21:09 UTC (permalink / raw)
To: Scott Wood; +Cc: devicetree-discuss, linuxppc-dev
In-Reply-To: <20080731205906.GB17718@ld0162-tx32.am.freescale.net>
On Thu, Jul 31, 2008 at 03:59:06PM -0500, Scott Wood wrote:
> On Thu, Jul 31, 2008 at 02:19:57PM -0600, Grant Likely wrote:
> > - Add a property to the device tree that explicitly specifies the SoC
> > that the board is based on. Something like 'soc-model =
> > "fsl,mpc5200b"' would be appropriate.
>
> Shouldn't that go in the compatible property of the soc node?
Not all SoC's (in particular 4xx SoCs) use an soc node. Some of them
instead just describe the internal busses on the SoC.
> > - Prioritize board ports in the arch/powerpc/platforms directory to
> > identify level-1 machines support from the level-2 ones. Make sure
> > that level-1 stuff always gets probed before level-2 stuff within each
> > SoC family. In all likelyhood, this would probably just involve
> > making sure that board specific machines get linked in before the
> > catchall machine.
>
> I don't think we're too far away from being able to have a catch-all that
> isn't even soc-type-specific -- the main things that come to mind are
> instantiating PCI controllers (should be easily fixed) and finding the
> root IRQ controller (I suppose we could pick a random interrupt
> controller and follow the interrupt tree to its root, though it'd be more
> robust if it were explicitly identified in the device tree).
Perhaps a root-interrupt-controller property?
g.
^ permalink raw reply
* Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Timur Tabi @ 2008-07-31 21:10 UTC (permalink / raw)
To: Jon Smirl; +Cc: Scott Wood, Linux I2C, Linuxppc-dev
In-Reply-To: <9e4733910807311403o63c37366ldae30e873f33a21e@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Wolfgang Grandegger @ 2008-07-31 21:14 UTC (permalink / raw)
To: Jon Smirl; +Cc: Scott Wood, Timur Tabi, Linux I2C, Linuxppc-dev
In-Reply-To: <9e4733910807311403o63c37366ldae30e873f33a21e@mail.gmail.com>
Jon Smirl wrote:
> On 7/31/08, Timur Tabi <timur@freescale.com> 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.
^ permalink raw reply
* Re: [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Timur Tabi @ 2008-07-31 21:17 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: Scott Wood, Linux I2C, Linuxppc-dev
In-Reply-To: <48922B40.3010808@grandegger.com>
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
^ permalink raw reply
* Re: libata badness
From: Kumar Gala @ 2008-07-31 21:58 UTC (permalink / raw)
To: avorontsov; +Cc: linux-ide, Jeff Garzik, Linux Kernel list, linuxppc-dev list
In-Reply-To: <20080731210410.GA623@polina.dev.rtsoft.ru>
I figured out the issue. Its related to the interrupt routing being
conveyed to the code from the device tree. We have a missing 'space'
causing issues.
- k
^ permalink raw reply
* [PATCH] powerpc: Fix whitespace merge in mpc8641 hpcn device tree
From: Kumar Gala @ 2008-07-31 22:10 UTC (permalink / raw)
To: linuxppc-dev
When we coverted the .dts to v1 we lost a space between the irq
and its polarity/sense information. This causes a bit of chaos
as the reset of the blob is off by one cell.
This was noticed by booting and getting errors w/ATA due to
lock of interrupts.
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
in my for-2.6.27 branch
arch/powerpc/boot/dts/mpc8641_hpcn.dts | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc8641_hpcn.dts b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
index ae08761..d665e76 100644
--- a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
+++ b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
@@ -411,7 +411,7 @@
0xe000 0 0 1 &i8259 12 2
0xe100 0 0 2 &i8259 9 2
0xe200 0 0 3 &i8259 10 2
- 0xe300 0 0 4 &i8259 112
+ 0xe300 0 0 4 &i8259 11 2
// IDSEL 0x1d Audio
0xe800 0 0 1 &i8259 6 2
--
1.5.5.1
^ permalink raw reply related
* Re: [PATCH] powerpc: Fix whitespace merge in mpc8641 hpcn device tree
From: Jon Loeliger @ 2008-07-31 22:18 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <Pine.LNX.4.64.0807311709530.10954@blarg.am.freescale.net>
Kumar Gala wrote:
> When we coverted the .dts to v1 we lost a space between the irq
> and its polarity/sense information. This causes a bit of chaos
> as the reset of the blob is off by one cell.
>
>
> diff --git a/arch/powerpc/boot/dts/mpc8641_hpcn.dts b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> index ae08761..d665e76 100644
> --- a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> +++ b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
> @@ -411,7 +411,7 @@
> 0xe000 0 0 1 &i8259 12 2
> 0xe100 0 0 2 &i8259 9 2
> 0xe200 0 0 3 &i8259 10 2
> - 0xe300 0 0 4 &i8259 112
> + 0xe300 0 0 4 &i8259 11 2
>
Ouch. That was probably my typo and fault.
Apologies.
jdl
^ permalink raw reply
* Re: [PATCH] powerpc: Fix whitespace merge in mpc8641 hpcn device tree
From: Kumar Gala @ 2008-07-31 22:28 UTC (permalink / raw)
To: Jon Loeliger; +Cc: linuxppc-dev
In-Reply-To: <48923A43.9040901@freescale.com>
On Jul 31, 2008, at 5:18 PM, Jon Loeliger wrote:
> Kumar Gala wrote:
>> When we coverted the .dts to v1 we lost a space between the irq
>> and its polarity/sense information. This causes a bit of chaos
>> as the reset of the blob is off by one cell.
>>
>> diff --git a/arch/powerpc/boot/dts/mpc8641_hpcn.dts b/arch/powerpc/
>> boot/dts/mpc8641_hpcn.dts
>> index ae08761..d665e76 100644
>> --- a/arch/powerpc/boot/dts/mpc8641_hpcn.dts
>> +++ b/arch/powerpc/boot/dts/mpc8641_hpcn.dts
>> @@ -411,7 +411,7 @@
>> 0xe000 0 0 1 &i8259 12 2
>> 0xe100 0 0 2 &i8259 9 2
>> 0xe200 0 0 3 &i8259 10 2
>> - 0xe300 0 0 4 &i8259 112
>> + 0xe300 0 0 4 &i8259 11 2
>
> Ouch. That was probably my typo and fault.
>
> Apologies.
np. It would be nice to see dtc warn about it, but that would
probably be a bit difficult.
- k
^ permalink raw reply
* Re: [PATCH] powerpc: Fix whitespace merge in mpc8641 hpcn device tree
From: Scott Wood @ 2008-07-31 22:52 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <BF7C9A64-AABC-4624-8282-4BAC2D970009@kernel.crashing.org>
Kumar Gala wrote:
> np. It would be nice to see dtc warn about it, but that would probably
> be a bit difficult.
One thing that macros may bring besides clarity and elimination of
redundancy is the ability to put some semantic checks in the macro
itself (rather than hardcoding it into dtc). At the very least, this
particular bug would show up as the wrong number of arguments to a macro.
-Scott
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Trent Piepho @ 2008-07-31 23:32 UTC (permalink / raw)
To: Timur Tabi; +Cc: Linuxppc-dev, Scott Wood, Linux I2C
In-Reply-To: <4891E06A.4070608@freescale.com>
On Thu, 31 Jul 2008, Timur Tabi wrote:
> 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.
And I decided to program the algorithm. It's not that complex and overall
compiles to less total size than the table method. It doesn't involve some
black box table either.
The real problem, which kept me from making a patch to do this months ago,
is that the source clock that the I2C freq divider applies to is different
for just about every MPC8xxx platform. Not just a different value, but a
totally different internal clock. Sometimes is CCB, sometimes CCB/2,
sometimes tsec2's clock, etc. Sometimes the two i2c controllers don't even
have the same clock. I didn't see any easy way to get this information.
In U-Boot I get it from the global board info struct, but I didn't see
anything like this in Linux.
> > 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.
Yes, I needed a loop. The divider ends up having the form (a + c) << b,
where a is from some set of eight integers between 2 and 15, b has a range
of like 5 to 12, and c is usually zero.
So, I find the divider I want to get, loop over each b, shift the divider
right by b and find the closest (a+c) to that, calculate the error, and
then use the best one.
^ permalink raw reply
* [PATCH] Revert "Merge branch 'x86/iommu' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip into for-linus"
From: Stephen Rothwell @ 2008-07-31 23:43 UTC (permalink / raw)
To: Linus, Andrew Morton
Cc: Ingo, Joerg Roedel, LKML, Jesse Barnes, ppc-dev, sparclinux,
Molnar, David S. Miller
This reverts commit 29111f579f4f3f2a07385f931854ab0527ae7ea5.
This undoes the hasty addition of a global version of iommu_num_pages()
that broke both the powerpc and sparc builds. This function can be
revisited later.
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
arch/x86/kernel/amd_iommu.c | 13 ++++++++-----
arch/x86/kernel/pci-gart_64.c | 11 +++++++----
include/linux/iommu-helper.h | 1 -
lib/iommu-helper.c | 8 --------
4 files changed, 15 insertions(+), 18 deletions(-)
This patch comes from
git revert -m 1 29111f579f4f3f2a07385f931854ab0527ae7ea5
I have test built powerpc ppc64_defconfig and sparc64 defconfig. The only
references to iommu_num_pages() after this is applied are in the powerpc
and sparc code.
Linus, please apply. This is impacting on both powerpc and sparc
development and even the author of the patches said that those patches
were not urgent.
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 22d7d05..7469740 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -29,6 +29,9 @@
#define CMD_SET_TYPE(cmd, t) ((cmd)->data[1] |= ((t) << 28))
+#define to_pages(addr, size) \
+ (round_up(((addr) & ~PAGE_MASK) + (size), PAGE_SIZE) >> PAGE_SHIFT)
+
#define EXIT_LOOP_COUNT 10000000
static DEFINE_RWLOCK(amd_iommu_devtable_lock);
@@ -182,7 +185,7 @@ static int iommu_flush_pages(struct amd_iommu *iommu, u16 domid,
u64 address, size_t size)
{
int s = 0;
- unsigned pages = iommu_num_pages(address, size);
+ unsigned pages = to_pages(address, size);
address &= PAGE_MASK;
@@ -554,8 +557,8 @@ static struct dma_ops_domain *dma_ops_domain_alloc(struct amd_iommu *iommu,
if (iommu->exclusion_start &&
iommu->exclusion_start < dma_dom->aperture_size) {
unsigned long startpage = iommu->exclusion_start >> PAGE_SHIFT;
- int pages = iommu_num_pages(iommu->exclusion_start,
- iommu->exclusion_length);
+ int pages = to_pages(iommu->exclusion_start,
+ iommu->exclusion_length);
dma_ops_reserve_addresses(dma_dom, startpage, pages);
}
@@ -764,7 +767,7 @@ static dma_addr_t __map_single(struct device *dev,
unsigned int pages;
int i;
- pages = iommu_num_pages(paddr, size);
+ pages = to_pages(paddr, size);
paddr &= PAGE_MASK;
address = dma_ops_alloc_addresses(dev, dma_dom, pages);
@@ -799,7 +802,7 @@ static void __unmap_single(struct amd_iommu *iommu,
if ((dma_addr == 0) || (dma_addr + size > dma_dom->aperture_size))
return;
- pages = iommu_num_pages(dma_addr, size);
+ pages = to_pages(dma_addr, size);
dma_addr &= PAGE_MASK;
start = dma_addr;
diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 49285f8..744126e 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -67,6 +67,9 @@ static u32 gart_unmapped_entry;
(((x) & 0xfffff000) | (((x) >> 32) << 4) | GPTE_VALID | GPTE_COHERENT)
#define GPTE_DECODE(x) (((x) & 0xfffff000) | (((u64)(x) & 0xff0) << 28))
+#define to_pages(addr, size) \
+ (round_up(((addr) & ~PAGE_MASK) + (size), PAGE_SIZE) >> PAGE_SHIFT)
+
#define EMERGENCY_PAGES 32 /* = 128KB */
#ifdef CONFIG_AGP
@@ -238,7 +241,7 @@ nonforced_iommu(struct device *dev, unsigned long addr, size_t size)
static dma_addr_t dma_map_area(struct device *dev, dma_addr_t phys_mem,
size_t size, int dir)
{
- unsigned long npages = iommu_num_pages(phys_mem, size);
+ unsigned long npages = to_pages(phys_mem, size);
unsigned long iommu_page = alloc_iommu(dev, npages);
int i;
@@ -301,7 +304,7 @@ static void gart_unmap_single(struct device *dev, dma_addr_t dma_addr,
return;
iommu_page = (dma_addr - iommu_bus_base)>>PAGE_SHIFT;
- npages = iommu_num_pages(dma_addr, size);
+ npages = to_pages(dma_addr, size);
for (i = 0; i < npages; i++) {
iommu_gatt_base[iommu_page + i] = gart_unmapped_entry;
CLEAR_LEAK(iommu_page + i);
@@ -384,7 +387,7 @@ static int __dma_map_cont(struct device *dev, struct scatterlist *start,
}
addr = phys_addr;
- pages = iommu_num_pages(s->offset, s->length);
+ pages = to_pages(s->offset, s->length);
while (pages--) {
iommu_gatt_base[iommu_page] = GPTE_ENCODE(addr);
SET_LEAK(iommu_page);
@@ -467,7 +470,7 @@ gart_map_sg(struct device *dev, struct scatterlist *sg, int nents, int dir)
seg_size += s->length;
need = nextneed;
- pages += iommu_num_pages(s->offset, s->length);
+ pages += to_pages(s->offset, s->length);
ps = s;
}
if (dma_map_cont(dev, start_sg, i - start, sgmap, pages, need) < 0)
diff --git a/include/linux/iommu-helper.h b/include/linux/iommu-helper.h
index f8598f5..c975caf 100644
--- a/include/linux/iommu-helper.h
+++ b/include/linux/iommu-helper.h
@@ -8,4 +8,3 @@ extern unsigned long iommu_area_alloc(unsigned long *map, unsigned long size,
unsigned long align_mask);
extern void iommu_area_free(unsigned long *map, unsigned long start,
unsigned int nr);
-extern unsigned long iommu_num_pages(unsigned long addr, unsigned long len);
diff --git a/lib/iommu-helper.c b/lib/iommu-helper.c
index 889ddce..a3b8d4c 100644
--- a/lib/iommu-helper.c
+++ b/lib/iommu-helper.c
@@ -80,11 +80,3 @@ void iommu_area_free(unsigned long *map, unsigned long start, unsigned int nr)
}
}
EXPORT_SYMBOL(iommu_area_free);
-
-unsigned long iommu_num_pages(unsigned long addr, unsigned long len)
-{
- unsigned long size = roundup((addr & ~PAGE_MASK) + len, PAGE_SIZE);
-
- return size >> PAGE_SHIFT;
-}
-EXPORT_SYMBOL(iommu_num_pages);
--
1.5.6.3
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
^ permalink raw reply related
* Re: [PATCH] Revert "Merge branch 'x86/iommu' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip into for-linus"
From: FUJITA Tomonori @ 2008-07-31 23:51 UTC (permalink / raw)
To: sfr
Cc: joerg.roedel, linux-kernel, jbarnes, davem, linuxppc-dev,
sparclinux, akpm, torvalds, mingo
In-Reply-To: <20080801094323.9601d87b.sfr@canb.auug.org.au>
On Fri, 1 Aug 2008 09:43:23 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> This reverts commit 29111f579f4f3f2a07385f931854ab0527ae7ea5.
>
> This undoes the hasty addition of a global version of iommu_num_pages()
> that broke both the powerpc and sparc builds. This function can be
> revisited later.
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
> arch/x86/kernel/amd_iommu.c | 13 ++++++++-----
> arch/x86/kernel/pci-gart_64.c | 11 +++++++----
> include/linux/iommu-helper.h | 1 -
> lib/iommu-helper.c | 8 --------
> 4 files changed, 15 insertions(+), 18 deletions(-)
>
> This patch comes from
> git revert -m 1 29111f579f4f3f2a07385f931854ab0527ae7ea5
>
> I have test built powerpc ppc64_defconfig and sparc64 defconfig. The only
> references to iommu_num_pages() after this is applied are in the powerpc
> and sparc code.
>
> Linus, please apply. This is impacting on both powerpc and sparc
> development and even the author of the patches said that those patches
> were not urgent.
Ingo has a patch to fix this problem in the x86 tree:
http://marc.info/?l=linux-kernel&m=121754062325903&w=2
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Trent Piepho @ 2008-08-01 0:22 UTC (permalink / raw)
To: Jon Smirl; +Cc: Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <9e4733910807311157q358640ddyef1f14865c069b8@mail.gmail.com>
On Thu, 31 Jul 2008, Jon Smirl wrote:
> 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 mpc8xxxx 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.
There is a huge variation in where the I2C source clock comes from.
Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc. If
I look at u-boot (which might not be entirely correct or complete), I see:
83xx: 5 different clock sources
85xx: 3 different clock sources
86xx: 2 different clock sources
But there's more. Sometimes the two I2C controllers don't use the same
clock! So even if you add 10 globals with different clocks, and then add
code to the mpc i2c driver so if can figure out which one to use given the
platform, it's still not enough because you need to know which controller
the device node is for.
IMHO, what Timur suggested of having u-boot put the source clock into the
i2c node makes the most sense. U-boot has to figure this out, so why
duplicate the work?
Here's my idea:
i2c@0 {
compatible = "fsl-i2c";
bus-frequency = <100000>;
/* Either */
source-clock-frequency = <0>;
/* OR */
source-clock = <&ccb>;
};
bus-frequency is the desired frequency of the i2c bus, i.e. 100 kHz or 400
kHz usually. source-clock-frequency is the the source clock to this i2c
controller. U-Boot can fill this in since it already knows it anyway. Or,
instead of source-clock-frequency, source-clock can be specified as a
phandle to a device which has the clock to use. This could be useful if
Linux can change the clock frequency.
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Trent Piepho @ 2008-08-01 0:46 UTC (permalink / raw)
To: Grant Likely; +Cc: Scott Wood, Linuxppc-dev, Timur Tabi, Linux I2C
In-Reply-To: <20080731195911.GA29610@secretlab.ca>
On Thu, 31 Jul 2008, 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:
> >
> > soc8541@e0000000 {
> > #address-cells = <1>;
> > #size-cells = <1>;
> > device_type = "soc";
> > ranges = <0x0 0xe0000000 0x100000>;
> > reg = <0xe0000000 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().
Except the i2c clock isn't always a based on an interger divider of the CCB
frequency. What's more, it's not always the same for both i2c controllers.
Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would
fsl_get_i2c_freq() figure that out from bus-frequency and
i2c-clock-divider?
> 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.
The i2c controller just uses some system clock that was handy. For each
chip, the designers consult tea leaves to choose a system clock at random
to connect to the i2c controller.
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Trent Piepho @ 2008-08-01 0:57 UTC (permalink / raw)
To: Grant Likely; +Cc: Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <fa686aa40807311335t1dd123d0mc385234ebe55248@mail.gmail.com>
On Thu, 31 Jul 2008, Grant Likely wrote:
> >> 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?
Usually.
> a shared register setting?
Sometimes.
> or a register setting within the i2c device?
Never.
Thinking of it as the CCB divided by 1, 2, 3 isn't really right. It's just
some internal clock that I2C was connected to. Sometimes it's the clock
for the ethernet block of the SoC. Sometimes it comes from some other
block.
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Implement _PAGE_SPECIAL & pte_special() for 32-bit
From: Nick Piggin @ 2008-08-01 1:11 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <Pine.LNX.4.64.0807311345380.8012@blarg.am.freescale.net>
On Thu, Jul 31, 2008 at 01:48:31PM -0500, Kumar Gala wrote:
> Implement _PAGE_SPECIAL and pte_special() for 32-bit powerpc. This bit will
> be used by the fast get_user_pages() to differenciate PTEs that correspond
> to a valid struct page from special mappings that don't such as IO mappings
> obtained via io_remap_pfn_ranges().
>
> We currently only implement this on sub-arch that support SMP or will so
> in the future (6xx, 44x, FSL-BookE) and not (8xx, 40x).
>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
Cool
> ---
> Nick, do you forsee using _PAGE_SPECIAL for other applications that would
> be of interested to non-SMP hw?
>
> We can look at adding it into 8xx and 40x, but was being lazy as I assume
> there is no point.
I don't forsee it being used for anything else, but it is possible I guess.
We currently will also use it in the VM (vm_normal_page), turning
that function into a much more compact and simple version. It doesn't
do a great deal for performance, but you _may_ want to consider using it
just so the entire powerpc architecture takes this same path.
Not a big deal though.
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Trent Piepho @ 2008-08-01 1:16 UTC (permalink / raw)
To: Timur Tabi; +Cc: Scott Wood, Linux I2C, Linuxppc-dev
In-Reply-To: <48922BE4.5030507@freescale.com>
On Thu, 31 Jul 2008, Timur Tabi wrote:
> 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.
I think Wolfgang means u-boot doesn't _read_ information from the device
tree. Put the I2C source clock into the device tree and have u-boot read it
in.
But that simply doesn't work at all. The i2c source clock isn't constant.
It's not a constant divider from the core clock either. Flip a dip switch,
and the divider changes.
But that's irrelevant. U-boot needs to configure the i2c controller to
read SPD data from the DIMM's EPROM, which is necessary to get DRAM
working. That happens long long before the FDT is loaded via TFTP,
y-modem, NFS, or from disk or flash. Might as well have u-boot get the i2c
source clock via Linux's sysfs.
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Jon Smirl @ 2008-08-01 1:19 UTC (permalink / raw)
To: Trent Piepho; +Cc: Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <Pine.LNX.4.58.0807311656250.10341@shell4.speakeasy.net>
On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Thu, 31 Jul 2008, Jon Smirl wrote:
> > 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 mpc8xxxx 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.
>
>
> There is a huge variation in where the I2C source clock comes from.
> Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc. If
> I look at u-boot (which might not be entirely correct or complete), I see:
>
> 83xx: 5 different clock sources
> 85xx: 3 different clock sources
> 86xx: 2 different clock sources
>
> But there's more. Sometimes the two I2C controllers don't use the same
> clock! So even if you add 10 globals with different clocks, and then add
> code to the mpc i2c driver so if can figure out which one to use given the
> platform, it's still not enough because you need to know which controller
> the device node is for.
>
> IMHO, what Timur suggested of having u-boot put the source clock into the
> i2c node makes the most sense. U-boot has to figure this out, so why
> duplicate the work?
>
> Here's my idea:
>
> i2c@0 {
> compatible = "fsl-i2c";
> bus-frequency = <100000>;
>
> /* Either */
> source-clock-frequency = <0>;
> /* OR */
> source-clock = <&ccb>;
> };
Can't we hide a lot of this on platforms where the source clock is not
messed up? For example the mpc5200 doesn't need any of this, the
needed frequency is already available in mpc52xx_find_ipb_freq().
mpc5200 doesn't need any uboot change.
Next would be normal mpc8xxx platforms where i2c is driven by a single
clock, add a uboot filled in parameter in the root node (or I think it
can be computed off of the ones uboot is already filling in). make a
mpc8xxx_find_i2c_freq() function. May not need to change device tree
and uboot.
Finally use this for those days when the tea leaves were especially
bad. Both a device tree and uboot change.
> Except the i2c clock isn't always a based on an integer divider of the CCB
> frequency. What's more, it's not always the same for both i2c controllers.
> Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would
> fsl_get_i2c_freq() figure that out from bus-frequency and
> i2c-clock-divider?
If you get the CCB frequency from uboot and know the chip model, can't
you compute these in the platform code? Then make a
mpc8xxx_find_i2c_freq(cell_index).
I don't see why we want to go through the trouble of having uboot tell
us things about a chip that are fixed in stone. Obviously something
like the frequency of the external crystal needs to be passed up, but
why pass up stuff that can be computed (or recovered by reading a
register)?
I may be using uboot differently, I just use it to boot and removed
support for everything else.
> bus-frequency is the desired frequency of the i2c bus, i.e. 100 kHz or 400
> kHz usually. source-clock-frequency is the the source clock to this i2c
> controller. U-Boot can fill this in since it already knows it anyway. Or,
> instead of source-clock-frequency, source-clock can be specified as a
> phandle to a device which has the clock to use. This could be useful if
> Linux can change the clock frequency.
>
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Trent Piepho @ 2008-08-01 1:36 UTC (permalink / raw)
To: Jon Smirl; +Cc: Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <9e4733910807311819i60872285ga4829c841185fdc0@mail.gmail.com>
On Thu, 31 Jul 2008, Jon Smirl wrote:
> > Here's my idea:
> >
> > i2c@0 {
> > compatible = "fsl-i2c";
> > bus-frequency = <100000>;
> >
> > /* Either */
> > source-clock-frequency = <0>;
> > /* OR */
> > source-clock = <&ccb>;
> > };
>
> Can't we hide a lot of this on platforms where the source clock is not
> messed up? For example the mpc5200 doesn't need any of this, the
> needed frequency is already available in mpc52xx_find_ipb_freq().
> mpc5200 doesn't need any uboot change.
>
> Next would be normal mpc8xxx platforms where i2c is driven by a single
> clock, add a uboot filled in parameter in the root node (or I think it
> can be computed off of the ones uboot is already filling in). make a
> mpc8xxx_find_i2c_freq() function. May not need to change device tree
> and uboot.
>
> Finally use this for those days when the tea leaves were especially
> bad. Both a device tree and uboot change.
If you have to support clock speed in the i2c node anyway, what's the point
of the other options?
> > Except the i2c clock isn't always a based on an integer divider of the CCB
> > frequency. What's more, it's not always the same for both i2c controllers.
> > Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would
> > fsl_get_i2c_freq() figure that out from bus-frequency and
> > i2c-clock-divider?
>
> If you get the CCB frequency from uboot and know the chip model, can't
> you compute these in the platform code? Then make a
> mpc8xxx_find_i2c_freq(cell_index).
You need a bunch of random other device registers (SEC, ethernet, sdhc,
etc.) too.
> I don't see why we want to go through the trouble of having uboot tell
> us things about a chip that are fixed in stone. Obviously something
> like the frequency of the external crystal needs to be passed up, but
> why pass up stuff that can be computed (or recovered by reading a
> register)?
One could also say that if U-boot has to have the code and already
calculated the value, why duplicate the code and the calculation in Linux?
^ permalink raw reply
* RE: mpc744x, Marvell mv6446x kernel guidance please
From: Stephen Horton @ 2008-08-01 1:38 UTC (permalink / raw)
To: Dale Farnsworth; +Cc: linuxppc-embedded
In-Reply-To: <20080730231046.GA26011@farnsworth.org>
Hi Dale,
Thanks for the info; it was very informative. To follow-up on my
previous email:
1. I now have a working interrupt-map section in my .dts. PCI bus
initialization works with no visible errors.
2. I've deciphered the magic numbers in the cpu->pci window
configuration. The required information for disabling and re-enabling
BARS is located in the Marvell User's manual, Rev C, page 365, Table
141.
3. I'm a little smarter on the Ethernet configuration. I understand now
that Linux should actually just know about eth0 the RGMII connection
from Marvell to the Broadcom switch. Its upto me to configure the
additional Broadcom ports with whatever configuration (to meet my PICMG
2.16 requirements). I have all the Ethernet stuff initializing properly
(including eth0) with no errors; unfortunately, when I configure for
bootp and nfs boot over the network, there are no bootp requests coming
out of my board (verified with a sniffer, but link is up according to
Broadcom status bits and remote switch). So, I still have something
wrong here in my kernel code...
Thanks, again,
Stephen
-----Original Message-----
From: Dale Farnsworth [mailto:dale@farnsworth.org]=20
Sent: Wednesday, July 30, 2008 6:11 PM
To: Stephen Horton
Cc: Mark A. Greer; linuxppc-embedded@ozlabs.org
Subject: Re: mpc744x, Marvell mv6446x kernel guidance please
On Wed, Jul 30, 2008 at 08:56:18AM -0500, Stephen Horton wrote:
> Thanks for your kind encouragement. I now have a mostly booting
kernel.
> I have just a few remaining issues to resolve; perhaps you (or others)
> can give me some tips regarding these:
>=20
> 1. In your prpmc2800 .dts configuration, in the PCI bus configuration
> section, you lay-out the IRQ mappings like this:
> interrupt-map =3D <
> /* IDSEL 0x0a */
> 5000 0 0 1 &/mv64x60/pic 50
> 5000 0 0 2 &/mv64x60/pic 51
> I've read the Open Firmware document on Interrupt Mapping, but I still
> don't really understand the first 3 columns (5000 0 0), especially
where
> the first column comes from. Is this just some arbitrarily selected
> offset address for that device on the pci bus?
An address on the PCI bus is represented by 3 cells (96 bits).
Take a look at page 4 of http://www.openbios.org/data/docs/bus.pci.pdf
You'll see that the PCI device is contained in bits 15-11, selected by
the 0xf800 in interrupt-map-mask. The 0x5000 corresponds to device 0xa.
-Dale
^ permalink raw reply
* [PATCH v3] Force printing of 'total_memory' to unsigned long long in ppc_mmu_32.c
From: Tony Breeds @ 2008-08-01 1:38 UTC (permalink / raw)
To: Paul Mackerras, Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <80bdb63106f6dbdbe81a36272ad8da32fd1c1470.1217487365.git.tony@bakeyournoodle.com>
total_memory is a 'phys_addr_t', Which can be either 64 or 32 bits.
Force printing as unsigned long long to silence the warning.
Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
---
Changes since v1:
- correctly use 64bit type as phys_addr_t wont always be 32bits. Thanks to
sfr for showing me the error of my ways ;P
Changes since v2:
- correctly cast the result of the shift NOT just total_memory. Thanks Milton
for catching that.
arch/powerpc/mm/ppc_mmu_32.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/ppc_mmu_32.c b/arch/powerpc/mm/ppc_mmu_32.c
index c53145f..6aa1208 100644
--- a/arch/powerpc/mm/ppc_mmu_32.c
+++ b/arch/powerpc/mm/ppc_mmu_32.c
@@ -236,8 +236,8 @@ void __init MMU_init_hw(void)
Hash_end = (struct hash_pte *) ((unsigned long)Hash + Hash_size);
- printk("Total memory = %ldMB; using %ldkB for hash table (at %p)\n",
- total_memory >> 20, Hash_size >> 10, Hash);
+ printk("Total memory = %lldMB; using %ldkB for hash table (at %p)\n",
+ (unsigned long long)(total_memory >> 20), Hash_size >> 10, Hash);
/*
--
1.5.6.3
^ permalink raw reply related
* Re: [PATCH 5/8] Silence warning in arch/powerpc/mm/ppc_mmu_32.c
From: Tony Breeds @ 2008-08-01 1:40 UTC (permalink / raw)
To: Milton Miller; +Cc: ppcdev, Stephen Rothwell
In-Reply-To: <1c42eb99377e74cc128fa29aee8dc8b8@bga.com>
On Thu, Jul 31, 2008 at 01:54:13AM -0500, Milton Miller wrote:
> But please, cast the result of the shift. Otherwise it will print 0MB
> instead of 4096MB.
Done, patch (v3) on the way. One day I'll get better at this "trivial"
stuff.
Yours Tony
linux.conf.au http://www.marchsouth.org/
Jan 19 - 24 2009 The Australian Linux Technical Conference!
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Jon Smirl @ 2008-08-01 1:44 UTC (permalink / raw)
To: Trent Piepho; +Cc: Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <Pine.LNX.4.58.0807311828580.10341@shell4.speakeasy.net>
On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
> On Thu, 31 Jul 2008, Jon Smirl wrote:
>
> > > Here's my idea:
> > >
> > > i2c@0 {
> > > compatible = "fsl-i2c";
> > > bus-frequency = <100000>;
> > >
> > > /* Either */
> > > source-clock-frequency = <0>;
> > > /* OR */
> > > source-clock = <&ccb>;
> > > };
> >
> > Can't we hide a lot of this on platforms where the source clock is not
> > messed up? For example the mpc5200 doesn't need any of this, the
> > needed frequency is already available in mpc52xx_find_ipb_freq().
> > mpc5200 doesn't need any uboot change.
> >
> > Next would be normal mpc8xxx platforms where i2c is driven by a single
> > clock, add a uboot filled in parameter in the root node (or I think it
> > can be computed off of the ones uboot is already filling in). make a
> > mpc8xxx_find_i2c_freq() function. May not need to change device tree
> > and uboot.
> >
> > Finally use this for those days when the tea leaves were especially
> > bad. Both a device tree and uboot change.
>
>
> If you have to support clock speed in the i2c node anyway, what's the point
> of the other options?
So that I don't have to change my existing mpc5200 systems. mpc5200
has no need for specifying the source clock in each i2c node, hardware
doesn't allow it.
> > > Except the i2c clock isn't always a based on an integer divider of the CCB
> > > frequency. What's more, it's not always the same for both i2c controllers.
> > > Suppose i2c #1 uses CCB times 2/3 and i2c #2 uses CCB/2, how would
> > > fsl_get_i2c_freq() figure that out from bus-frequency and
> > > i2c-clock-divider?
> >
> > If you get the CCB frequency from uboot and know the chip model, can't
> > you compute these in the platform code? Then make a
> > mpc8xxx_find_i2c_freq(cell_index).
>
>
> You need a bunch of random other device registers (SEC, ethernet, sdhc,
> etc.) too.
>
>
> > I don't see why we want to go through the trouble of having uboot tell
> > us things about a chip that are fixed in stone. Obviously something
> > like the frequency of the external crystal needs to be passed up, but
> > why pass up stuff that can be computed (or recovered by reading a
> > register)?
>
>
> One could also say that if U-boot has to have the code and already
> calculated the value, why duplicate the code and the calculation in Linux?
What about the Efika which is mpc5200 based and doesn't use uboot?
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Grant Likely @ 2008-08-01 2:03 UTC (permalink / raw)
To: Jon Smirl; +Cc: Trent Piepho, Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <9e4733910807311819i60872285ga4829c841185fdc0@mail.gmail.com>
On Thu, Jul 31, 2008 at 09:19:51PM -0400, Jon Smirl wrote:
> On 7/31/08, Trent Piepho <xyzzy@speakeasy.org> wrote:
> > On Thu, 31 Jul 2008, Jon Smirl wrote:
> > > 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 mpc8xxxx 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.
> >
> >
> > There is a huge variation in where the I2C source clock comes from.
> > Sometimes it's the system bus, sometimes ethernet, sometimes SEC, etc. If
> > I look at u-boot (which might not be entirely correct or complete), I see:
> >
> > 83xx: 5 different clock sources
> > 85xx: 3 different clock sources
> > 86xx: 2 different clock sources
> >
> > But there's more. Sometimes the two I2C controllers don't use the same
> > clock! So even if you add 10 globals with different clocks, and then add
> > code to the mpc i2c driver so if can figure out which one to use given the
> > platform, it's still not enough because you need to know which controller
> > the device node is for.
> >
> > IMHO, what Timur suggested of having u-boot put the source clock into the
> > i2c node makes the most sense. U-boot has to figure this out, so why
> > duplicate the work?
> >
> > Here's my idea:
> >
> > i2c@0 {
> > compatible = "fsl-i2c";
> > bus-frequency = <100000>;
> >
> > /* Either */
> > source-clock-frequency = <0>;
> > /* OR */
> > source-clock = <&ccb>;
> > };
>
> Can't we hide a lot of this on platforms where the source clock is not
> messed up? For example the mpc5200 doesn't need any of this, the
> needed frequency is already available in mpc52xx_find_ipb_freq().
> mpc5200 doesn't need any uboot change.
Your mixing up device tree layout with implementation details. Device
tree layout must come first. mpc52xx_find_ipb_freq() is just a
convenience function that walks up the device tree looking for a
bus-frequency property.
So, instead of making arguments based on available helper functions,
make your arguments based on how data should be laid out in the device
tree. Currently mpc5200 bindings simply depend on finding a
bus-frequency property in the parent node for determining the input
clock and I don't see any pressing reason to change that (though it
probably needs to be documented better).
However, for the complex cases that Trent and Timur are talking about,
it makes perfect sense to have an optional property in the i2c node
itself that defines a different clock. Once that decision has been made
and documented, then the driver can be modified and the appropriate
helper functions added to adapt the device tree data into something
useful.
Remember (and chant this to yourself). The device tree describes
*hardware*. It does not describe Linux internal implementation details.
g.
^ permalink raw reply
* Re: [i2c] [PATCH] powerpc: i2c-mpc: make speed registers configurable via FDT
From: Jon Smirl @ 2008-08-01 2:35 UTC (permalink / raw)
To: Grant Likely
Cc: Trent Piepho, Linuxppc-dev, Linux I2C, Scott Wood, Timur Tabi
In-Reply-To: <20080801020303.GA30947@secretlab.ca>
On 7/31/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> Your mixing up device tree layout with implementation details. Device
> tree layout must come first. mpc52xx_find_ipb_freq() is just a
> convenience function that walks up the device tree looking for a
> bus-frequency property.
>
> So, instead of making arguments based on available helper functions,
> make your arguments based on how data should be laid out in the device
> tree. Currently mpc5200 bindings simply depend on finding a
> bus-frequency property in the parent node for determining the input
> clock and I don't see any pressing reason to change that (though it
> probably needs to be documented better).
>
> However, for the complex cases that Trent and Timur are talking about,
> it makes perfect sense to have an optional property in the i2c node
> itself that defines a different clock. Once that decision has been made
> and documented, then the driver can be modified and the appropriate
> helper functions added to adapt the device tree data into something
> useful.
I've having trouble with whether these clocks are a system resource or
something that belongs to i2c. If they are a system resource then we
should make nodes in the root and use a phandle in the i2c node to
link to them.
The phandle in the mpc5200 case could be implicit since it is fixed in silicon.
Is this register in a system register bank or an i2c one?
gur->pordevsr2 & MPC85xx_PORDEVSR2_SEC_CFG
>
> Remember (and chant this to yourself). The device tree describes
> *hardware*. It does not describe Linux internal implementation details.
>
>
> g.
>
>
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox