[PATCH] Add MPC837xEMDS PCIE RC mode support

Kumar Gala kumar.gala at freescale.com
Fri Nov 30 20:05:55 EST 2007


>>> +
>>> +     pci2 at e0009000 {
>>
>> I agree w/Olof.  This should be pcie at e0009000
>>>
>>> +             interrupt-map-mask = <f800 0 0 7>;
>>> +             msi-available-ranges = <43 4 51 52 56 57 58 59>;
>>> +             interrupt-map = <
>>> +                     0000 0 0 1 &ipic 1 8
>>> +                     0000 0 0 2 &ipic 1 8
>>> +                     0000 0 0 3 &ipic 1 8
>>> +                     0000 0 0 4 &ipic 1 8
>>> +             >;
>>> +             interrupt-parent = < &ipic >;
>>> +             interrupts = <1 8>;
>>> +             bus-range = <0 0>;
>>> +             ranges = <02000000 0 A8000000 A8000000 0 10000000
>>> +                       01000000 0 00000000 B8000000 0 00800000>;
>>> +             clock-frequency = <0>;
>>> +             #interrupt-cells = <1>;
>>> +             #size-cells = <2>;
>>> +             #address-cells = <3>;
>>> +             reg = <e0009000 00001000
>>> +                    a0000000 08000000>;
>>
>> Shouldn't the reg size for the cfg space be 256M?
>
> 256M is a little too big for kernel.

what do you mean too big?  Aren't you losing access to some bus/dev/fn  
than?

>>> diff --git a/arch/powerpc/platforms/83xx/Kconfig b/arch/powerpc/
>>> platforms/83xx/Kconfig
>>> index 0c61e7a..00154c5 100644
>>> --- a/arch/powerpc/platforms/83xx/Kconfig
>>> +++ b/arch/powerpc/platforms/83xx/Kconfig
>>> @@ -87,3 +87,10 @@ config PPC_MPC837x
>>>      select PPC_INDIRECT_PCI
>>>      select FSL_SERDES
>>>      default y if MPC837x_MDS
>>> +
>>> +config PPC_MPC83XX_PCIE
>>> +     bool "MPC837X PCI Express support"
>>> +     depends on PCIEPORTBUS && PPC_MPC837x
>>> +     default n
>>> +     help
>>> +       Enables MPC837x PCI express RC mode
>>
>> This should be a hidden config that is just selected by PPC_MPC837x
>> instead of a choice.  Also drop the depends on.
>>
>
> In the dts file, the PCIE is default enabled. So, we should provide
> another way to disable the PCIE.
> Modify and recompile the dts is a little unkind to user.

Why do you something beyond CONFIG_PCI.  if you don't want PCIe but do  
want PCI the extra code for PCIe isn't going to kill you.

>>> diff --git a/arch/powerpc/platforms/83xx/mpc83xx.h b/arch/powerpc/
>>> platforms/83xx/mpc83xx.h
>>> index b778cb4..2078da7 100644
>>> --- a/arch/powerpc/platforms/83xx/mpc83xx.h
>>> +++ b/arch/powerpc/platforms/83xx/mpc83xx.h
>>> @@ -43,12 +43,18 @@
>>> #define PORTSCX_PTS_UTMI           0x00000000
>>> #define PORTSCX_PTS_ULPI           0x80000000
>>>
>>> +/* PCIE Registers */
>>> +#define PEX_LTSSM_STAT               0x404
>>> +#define PEX_LTSSM_STAT_L0    0x16
>>> +#define PEX_GCLK_RATIO               0x440
>>> +
>>
>> just move these into the .c file.
>>
>>>
>>> /*
>>> * Declaration for the various functions exported by the
>>> * mpc83xx_* files. Mostly for use by mpc83xx_setup
>>> */
>>> -
>>> -extern int mpc83xx_add_bridge(struct device_node *dev);
>>> +#define PPC_83XX_PCI 0x1
>>> +#define PPC_83XX_PCIE        0x2
>>> +extern int mpc83xx_add_bridge(struct device_node *dev, int flags);
>>> extern void mpc83xx_restart(char *cmd);
>>> extern long mpc83xx_time_init(void);
>>> extern int mpc834x_usb_cfg(void);
>>> diff --git a/arch/powerpc/platforms/83xx/pci.c b/arch/powerpc/
>>> platforms/83xx/pci.c
>>> index 80425d7..0b52b2e 100644
>>> --- a/arch/powerpc/platforms/83xx/pci.c
>>> +++ b/arch/powerpc/platforms/83xx/pci.c
>>> @@ -25,6 +25,8 @@
>>> #include <asm/prom.h>
>>> #include <sysdev/fsl_soc.h>
>>>
>>> +#include "mpc83xx.h"
>>> +
>>> #undef DEBUG
>>>
>>> #ifdef DEBUG
>>> @@ -33,13 +35,157 @@
>>> #define DBG(x...)
>>> #endif
>>>
>>> -int __init mpc83xx_add_bridge(struct device_node *dev)
>>> +#if defined(CONFIG_PPC_MPC83XX_PCIE)
>>> +
>>> +/* PCIE config space Read/Write routines */
>>
>> should really be called something like mpc83xx_pciex_read_config
>>>
>>> +static int direct_read_config_pcie(struct pci_bus *bus,
>>> +                     uint devfn, int offset, int len, u32 *val)
>>> +{
>>> +     struct pci_controller *hose = bus->sysdata;
>>> +     void __iomem *cfg_addr;
>>> +     u32 bus_no;
>>> +
>>> +     if (hose->indirect_type & PPC_INDIRECT_TYPE_NO_PCIE_LINK)
>>> +             return PCIBIOS_DEVICE_NOT_FOUND;
>>> +
>>> +     if (ppc_md.pci_exclude_device)
>>> +             if (ppc_md.pci_exclude_device(hose, bus->number,
>> devfn))
>>> +                     return PCIBIOS_DEVICE_NOT_FOUND;
>>> +
>>> +     switch (len) {
>>> +     case 2:
>>> +             if (offset & 1)
>>> +                     return -EINVAL;
>>> +             break;
>>> +     case 4:
>>> +     if (offset & 3)
>>> +             return -EINVAL;
>>> +             break;
>>> +     }
>>
>> fix formatting.
>>
>>>
>>> +
>>> +     pr_debug("_read_cfg_pcie: bus=%d devfn=%x off=%x len=%x\n",
>>> +             bus->number, devfn, offset, len);
>>> +
>>> +     if (bus->number == hose->first_busno) {
>>> +             if (devfn & 0xf8)
>>> +                     return PCIBIOS_DEVICE_NOT_FOUND;
>>
>> what is the 0xf8 all about?
>>
>
> The pcie only have one link, so the dev number only can be 0, as well
> the function number can be 0 ~ 7.

I don't follow what the number of links has to do with dev number.

>>> +     addr = mbase + (cfg_addr & ~PAGE_MASK);
>>> +     hose->cfg_addr = addr;
>>> +     hose->ops = &direct_pcie_ops;
>>
>> what's the point of this function, why not just fold it into
>> mpc83xx_setup_pcie
>>
>>>
>>> +}
>>> +
>>> +static void __init mpc83xx_setup_pcie(struct pci_controller *hose,
>>> +                     struct resource *reg, struct resource
>> *cfg_space)
>>> +{
>>> +     void __iomem *hose_cfg_base;
>>> +     u32 val;
>>> +
>>> +     hose_cfg_base = ioremap(reg->start, reg->end - reg->start +
>> 1);
>>> +
>>> +     val = in_le32(hose_cfg_base + PEX_LTSSM_STAT);
>>> +     if (val < PEX_LTSSM_STAT_L0)
>>> +             hose->indirect_type |=
>> PPC_INDIRECT_TYPE_NO_PCIE_LINK;
>>> +
>>> +     setup_direct_pcie(hose, cfg_space->start,
>>> +                     cfg_space->end - cfg_space->start + 1);
>>> +}
>>> +#endif /* CONFIG_PPC_MPC83XX_PCIE */
>>> +
>>
>> We should do the same think fsl_pci does for primary, its passed
>> into
>> _add_bridge.  Also, can we not do what fsl_pci does and use PCI
>> capabilities to determine we are a PCIe controller (and drop passing
>> it in via flags).
>>
>
> The mpc837x PCIE controller is a little different from mpc85xx.
> It can not be access via PCI configure read/write. It only can be
> accessed via memory-mapped read/write from core.

You don't have access to your own config space?

- k




More information about the Linuxppc-dev mailing list