On Wednesday 29 January 2014, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Wednesday 29 January 2014 02:56 AM, Arnd Bergmann wrote:
> > On Tuesday 28 January 2014, Kishon Vijay Abraham I wrote:
> >>> I have a common set of registers, which need to be programmed
> >>> differently for PCIe and SATA during phy init/exit.
> >>
> >> One way is differentiate using different compatible strings fro pcie and
> >> sata
> >> and use of_device_is_compatible to select a particular path.
> >
> > But if the IP block is the same, the compatible string should be
> > identical.
>
> Actually we define the compatible for 'device' no?. Here the same IP is
> configured differently as different devices in SoCs.
> >
> >>> Therefore, in the init/exit routine of phy_ops, I need some way of
> >>> identifying that phy_init/exit has been called from PCIe driver or
> >>> SATA driver.
> >>
> >> In this case you'll be actually registering two different PHYs (each for
> >> pcie
> >> and sata), so your phy_get should give you the only the appropriate phy.
> >
> > I would instead recommend making the mode of the PHY device the
> > argument to the phy handle in DT, so that the sata node uses
> >
> > phys = <&phyA 0>;
> >
> > and the PCIe node uses
> >
> > phys = <&phyB 1>;
> >
> > Then the binding for the phy defines that an argument of '0' means sata
> > mode,
> > while '1' means pcie mode, plus you should define all other valid modes.
>
> Anyway phyA and phyB points to different nodes and just from phyA and phyB we
> should be able to tell whether it is sata or pcie.
>
> We can just have a property in phyA to specify it is SATA and phyB to specify
> it is PCIE.
> phyA {
> compatible="phy-pipe3";
> .
> .
> type=<SATA>;
> }
> phyB {
> compatible="phy-pipe3";
> .
> .
> type=<PCIE>;
> }
> Then in probe
> of_property_read_u32(node, "type", &pipe3->type);
But this would be contrary to how we handle all other such devices.
> In phy_init function we can follow different path for SATA and PCIE using the
> type
>
> static int pipe3_init(struct phy *x) {
> struct pipe3 *phy = phy_get_drvdata(x);
>
> switch (phy->type) {
> case SATA:
> /* do sata phy initialization here*/
> break;
> case PCIE:
> /* do pcie phy initialization here*/
> break;
> default:
> dev_err(phy->dev, "phy type not supported\n");
> }
>
> return 0;
> }
I understand that it can be done, but that doesn't make it a good idea.
Take interrupt controllers as another example: the irqchip node provides
a set of identical resources (interrupt lines) that are connected to
various other parts of the SoC and external sources. You have to somehow
configure each line (edge/level, polarity, ...), but we intentionally
keep the configuration out of the node that describes the irq chip and
instead put it into the nodes that use it.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
