linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Grant Likely <grant.likely@secretlab.ca>
To: John Rigby <jrigby@freescale.com>
Cc: linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 5121 pci 1/3] powerpc: 83xx: pci: Remove need for get_immrbase from mpc83xx_add_bridge.
Date: Tue, 12 Aug 2008 23:09:24 -0600	[thread overview]
Message-ID: <20080813050924.GD17587@secretlab.ca> (raw)
In-Reply-To: <1218130587-31176-1-git-send-email-jrigby@freescale.com>

On Thu, Aug 07, 2008 at 11:36:25AM -0600, John Rigby wrote:
> Modify mpc83xx_add_bridge to get config space register base address from the device
> tree instead of immr + hardcoded offset.
> 
> 83xx pci nodes have these changes:
>     register properties now contain two address length tuples:
> 	First is the pci bridge register base, this has always been there.
> 	Second is the config base, this is new.
>     The primary pci bus should have the "primary" property.
> 
> These are documented in Documentation/powerpc/dts-bindings/fsl/83xx-512x-pci.txt

Looks mostly good to me.  I only have one comment on the device tree
binding...

> diff --git a/Documentation/powerpc/dts-bindings/fsl/83xx-512x-pci.txt b/Documentation/powerpc/dts-bindings/fsl/83xx-512x-pci.txt
> new file mode 100644
> index 0000000..51214a0
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/fsl/83xx-512x-pci.txt
> @@ -0,0 +1,43 @@
> +* Freescale 83xx and 512x PCI bridges
> +
> +Freescale 83xx and 512x SOCs include the same pci bridge core.
> +
> +83xx/512x specific notes:
> +- reg: should contain two address length tuples
> +    The first is for the internal pci bridge registers
> +    The second is for the pci config space access registers
> +- primary: 
> +    This property should be present for the primary pci bridge

Can you use something like 'fsl,primary-pci-bridge' instead?  'primary'
is a little too generic for my taste.  Also, the purpose of identifying
one of the PCI bridges as primary should be documented (This is me
pushing against encoding Linux internal implementation details into the
device tree, I suspect that 'primary' doesn't belong in the device tree
at all).

  parent reply	other threads:[~2008-08-13  5:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-07 17:36 [PATCH 5121 pci 1/3] powerpc: 83xx: pci: Remove need for get_immrbase from mpc83xx_add_bridge John Rigby
2008-08-07 17:36 ` [PATCH 5121 pci 2/3] powerpc: 5121: Add PCI support John Rigby
2008-08-07 17:36   ` [PATCH 5121 pci 3/3] powerpc: pci: 5121: Hide pci bridge John Rigby
2008-08-13  5:16     ` Grant Likely
2008-08-14  3:19       ` Kumar Gala
2008-08-13  5:12   ` [PATCH 5121 pci 2/3] powerpc: 5121: Add PCI support Grant Likely
2008-08-13  5:09 ` Grant Likely [this message]
2008-08-13 16:06   ` [PATCH 5121 pci 1/3] powerpc: 83xx: pci: Remove need for get_immrbase from mpc83xx_add_bridge John Rigby
2008-08-13 16:10     ` Scott Wood
2008-08-13 16:20       ` Grant Likely
2008-08-13 16:23     ` Grant Likely
2008-08-13 16:30       ` John Rigby

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080813050924.GD17587@secretlab.ca \
    --to=grant.likely@secretlab.ca \
    --cc=jrigby@freescale.com \
    --cc=linuxppc-dev@ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).