On Thu, Jan 30, 2014 at 09:34:19PM +0800, Arnd Bergmann wrote:
> On Thursday 30 January 2014, Mohit Kumar wrote:
>
> > @@ -80,6 +80,57 @@
> > status = "disabled";
> > };
> >
> > + pcie0: pcie@b1000000 {
> > + compatible = "st,spear1340-pcie", "snps,dw-pcie";
> > + reg = <0xb1000000 0x4000>;
> > + interrupts = <0 68 0x4>;
> > + pcie_is_gen1 = <0>;
> > + num-lanes = <1>;
> > + phys = <&miphy0 1 0>;
> > + phy-names = "pcie-phy";
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > + device_type = "pci";
> > + ranges = <0x00000800 0 0x80000000 0x80000000 0 0x00020000 /* configuration space */
> > + 0x81000000 0 0 0x80020000 0 0x00010000 /* downstream I/O */
> > + 0x82000000 0 0x80030000 0xc0030000 0 0x0ffd0000>; /* non-prefetchable memory */
> > + status = "disabled";
> > + };
>
> Shouldn't there be more than one interrupt? Normally each root port has
> four legacy IRQs, in order to support bridge devices.
>
> > + spear13xx_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > + if (IS_ERR(spear13xx_pcie->phy)) {
> > + dev_err(dev, "Could not get PHY\n");
> > + return -EPROBE_DEFER;
> > + }
>
> I think you should only return -EPROBE_DEFER if you got that
> error from the PHY layer. If there is some other problem
> with getting the PHY, you want that returned as a fatal
> error here and not retry the PCIe probe function.
OK.
>
> > diff --git a/drivers/phy/phy-spear13xx-sata-pcie.c b/drivers/phy/phy-spear13xx-sata-pcie.c
> > index 6adfa64..5eabf51 100644
> > --- a/drivers/phy/phy-spear13xx-sata-pcie.c
> > +++ b/drivers/phy/phy-spear13xx-sata-pcie.c
> > @@ -71,6 +71,80 @@
> > #define SPEAR1340_PCIE_SATA_MIPHY_CFG_PCIE \
> > (SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
> > SPEAR1340_MIPHY_PLL_RATIO_TOP(25))
>
> Please split this out into a separate patch.
ok.
>
> > +static int spear1310_pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
> > +{
> > + u32 mask, val;
> > +
> > + regmap_update_bits(phypriv->misc, SPEAR1310_PCIE_MIPHY_CFG_1,
> > + SPEAR1310_PCIE_SATA_MIPHY_CFG_PCIE_MASK,
> > + SPEAR1310_PCIE_SATA_MIPHY_CFG_PCIE);
> > +
> > + switch (phypriv->id) {
> > + case 0:
> > + mask = SPEAR1310_PCIE_CFG_MASK(0);
> > + val = SPEAR1310_PCIE_CFG_VAL(0);
> > + break;
> > + case 1:
> > + mask = SPEAR1310_PCIE_CFG_MASK(1);
> > + val = SPEAR1310_PCIE_CFG_VAL(1);
> > + break;
> > + case 2:
> > + mask = SPEAR1310_PCIE_CFG_MASK(2);
> > + val = SPEAR1310_PCIE_CFG_VAL(2);
> > + break;
>
> Ah, so this is what the ID gets used for. I would indeed encode this in the
> phy node, unlike the configuration of whether it's used for PCIe or SATA.
ok.. ll use "phy_id = <n>;" in phy node.
>
> > +static int pcie_miphy_init(struct spear13xx_phy_priv *phypriv)
> > +{
> > + if (of_machine_is_compatible("st,spear1340"))
> > + return spear1340_pcie_miphy_init(phypriv);
> > + else if (of_machine_is_compatible("st,spear1310"))
> > + return spear1310_pcie_miphy_init(phypriv);
> > + else
> > + return -EINVAL;
> > +}
>
> You should never check global properties such as the machine compatible
> value to make local decisions. You have two options here: Either use
> two different compatible strings for the phy node, or encode the
> difference in another property. If the only difference between spear1310
> and spear1340 is the location of the register within the "misc" syscon
> space, a good represenation would be to put the register offset next
> to the syscon phandle in the same property. That way it could transparently
> work for future SoCs.
Currently, there is only difference in misc syscon space. But, as I
said few phy register definition might also change in different soc.
Ok.. I ll go with first option, ie different compatible string for
different socs. It seems most logical also.
Regards
Pratyush
>
> Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
