Re: 答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Lian Minghuan-B31939 <B31939@freescale.com>]

Hi Arnd,

I am sorry for missed reply style and thank you very much for fixing it.
please see my comments inline.

Thanks,
Minghuan

On 2014年09月05日 08:44, Arnd Bergmann wrote:
> On Friday 05 September 2014 07:22:08 Minghuan.Lian@freescale.com wrote:
>> Hi Arnd,
>>
>> Please see my comments inline.
> Please use the usual reply style in the future, using '>' characters to
> properly give attribution. I'm fixing this up manually in my reply
> here, so others can follow the discussion.
>
>>> On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:
>>>> +
>>>> +#define PCIE_LS1021A_BASE    0x3400000
>>>> +#define PCIE_LS1021A_REG_SIZE        0x0100000
>>> This does not belong here. All addresses must come from DT,
>>> and you should not do the calculation below based on the address.
>> [Minghuan] LS1021A contains two PCI controllers. I use them to
>> calculation the PCI controller index.
>> There are several separate registers for PEX1 and PEX2 in SCFG
>> register space. It is hard to get them address from DTS. Could
>> you tell me a way to get PCI controller index?
> The easiest way would be to put the index as a property in the
> device tree itself.
[Minghuan] I want to use  scfg = <&scfg 0> you mentioned below.
>>> +struct ls_pcie {
>>> +     struct list_head node;
>>> +     struct device *dev;
>>> +     struct pci_bus *bus;
>>> +     void __iomem *dbi;
>>> +     void __iomem *scfg;
>>> +     struct pcie_port pp;
>>> +     int id;
>>> +     int index;
>>> +     int irq;
>>> +     int msi_irq;
>>> +     int pme_irq;
>>> +};
>> irq and pme_irq seem to be completely unused, so better
>> not add them until they are actually used.
>> [Minghuan] Ok, I will remove them.
>>
>> The scfg registers seem to belong to another device that
>> is responsible for multiple instances. Unfortunately,
>> this "fsl,ls1021a-scfg" device is not documented anywhere
>> that I can see.
>>
>> Is this some sort of syscon node, or is it specific to the
>> pcie controller(s)?
>>
>> [Minghaun] SCFG provides SoC specific configuration and status
>> registers for the device including PCI USB eTSEC SATA ...
>> The platform patches that contains SCFG code are being upstream.
> This sounds like it should be a syscon node, and you should use
> syscon_regmap_lookup_by_phandle() to find the scfg node from each
> of those drivers, rather than scanning the DT yourself in each
> of the drivers using it.
>
> This can also be a place to put the index, such as
>
> 	scfg = <&scfg 0>;
>
> for the first PCIe node. The probe function then extracts both
> the phandle for the syscon and the index from that property.
[Minghuan] I discussed with my colleague. They worry about performance 
degradation if using regmap API,
because there are some fast device use scfg. We tend to use a simple way 
to map andread/write scfg directly.
>>> +static int ls_pcie_link_up(struct pcie_port *pp)
>>> +{
>>> +     u32 rc, tmp;
>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>> +
>>> +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +
>>> +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
>>> +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
>>> +
>>> +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
>>> +
>>> +     if (rc < LTSSM_PCIE_L0)
>>> +             return 0;
>>> +
>>> +     return 1;
>>> +}
>> This looks like it is really a phy driver, although a pretty minimal
>> one.
>>
>> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
>> and status registers for the device including PCI USB eTSEC SATA ...
> I'm guessing that a lot of that is phy related information though.
> A nicer way to deal with this, compared to using syscon directly is
> to have a phy driver for your chip, and have that deal with all
> the phy related setup. In this case you would have a reference in
> the client driver like
>
> 	phys = <&scfgphy LS1021A_PHY_PCIE0>;
> 	phy-names = "pcie";
>
> And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
> to that phy node, and then you can use the phy API to perform actions on
> it (init, power_on, power_off, exit).
>
> This keeps all knowledge about the phy registers inside of the scfg area
> in one place, and you only have to add a new phy driver for a future SoC
> that has the same PCIe core but different scfg.
[Minghuan] SCFG does not contains power_on power_off or other PCI 
control registers.
We only  get link up and MSI related status via SCFG. So I think it is 
not enough to call scfg
PCIe phy.
>>> +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
>>> +{
>>> +     return SCFG_SPIMSICR_ADDR;
>>> +}
>>> +
>>> +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
>>> +{
>>> +     struct ls_pcie *pcie = to_ls_pcie(pp);
>>> +
>>> +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
>>> +             return MSI_LS1021A_DATA(pcie->index);
>>> +
>>> +     return pos;
>>> +}
>> My impression is that you have two distinct MSI controller
>> implementations, one for LS1021A and the other one for everything
>> else. How about using separate pcie_host_ops for them, possibly
>> also moving them into separate files?
>>
>> A good way to deal with this is to put the pointers to the two
>> pcie_host_ops into the data fields of the ls_pcie_of_match table.
>>
>> [Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
>> both PEX1 and PEX use the same MSI address, but different MSI message data.
> I mean LS1021 is different from other chips here, since you compare
> the pcie->id value.
[Minghuan] I see. I will change it.
> 	Arnd