Query: Phy: How to find consumer device on dt platform

Query: Phy: How to find consumer device on dt platform

[Pratyush Anand]

Hi Kishon,

I have a phy driver which has to be used by two different consumer
driver, say pcie and sata.

I have a common set of registers, which need to be programmed
differently for PCIe and SATA during phy init/exit.

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.

Please let me know, if a way already exist.

Regards
Pratyush

Re: Query: Phy: How to find consumer device on dt platform

[Kishon Vijay Abraham I]

Hi,

On Tuesday 28 January 2014 07:43 PM, Pratyush Anand wrote:
> Hi Kishon,
> 
> I have a phy driver which has to be used by two different consumer
> driver, say pcie and sata.

I assume you have multiple instances of the same IP and you have a single
driver for it?
> 
> 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.
> 
> 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.

Cheers
Kishon

Re: Query: Phy: How to find consumer device on dt platform

[Arnd Bergmann]

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.

> > 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.

	Arnd

Re: Query: Phy: How to find consumer device on dt platform

[Kishon Vijay Abraham I]

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);

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;
}

Cheers
Kishon

Re: Query: Phy: How to find consumer device on dt platform

[Pratyush Anand]

Hi Arnd / Kishon,

Thanks for your inputs.

On Wed, Jan 29, 2014 at 01:41:56PM +0800, 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.

Probably, it may  not help in this case. How would *phys* defining as
above with PCIe/SATA node help phy driver to decide whether current
phy instance is associated with PCIe or SATA. Actually, there is no
way to pass information from phy consumer driver(pcie/sata driver in
this case) to phy driver.

> 
> 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 have multiple instances (say 3) of same phy, which can be
programmed either for pcie or for sata. We have multiple instances of
ahci and pcie controller. phy_n will be connected to either ahci_n or
pcie_n.

What Kishon has suggested here is exactly what I was thinking.
I think, we should go with this.

> 
> 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);
> 
> 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;
> }
> 
> Cheers
> Kishon

Regards
Pratyush

Re: Query: Phy: How to find consumer device on dt platform

[Arnd Bergmann]

On Wednesday 29 January 2014, Pratyush Anand wrote:
> On Wed, Jan 29, 2014 at 01:41:56PM +0800, Kishon Vijay Abraham I wrote:
> > > 
> > > 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.
> 
> Probably, it may  not help in this case. How would *phys* defining as
> above with PCIe/SATA node help phy driver to decide whether current
> phy instance is associated with PCIe or SATA. Actually, there is no
> way to pass information from phy consumer driver(pcie/sata driver in
> this case) to phy driver.

I don't understand what is unclear about my example where I do just that.
The argument (0 or 1) gets passed into the driver's xlate function
when the consumer calls of_phy_get().

> > 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 have multiple instances (say 3) of same phy, which can be
> programmed either for pcie or for sata. We have multiple instances of
> ahci and pcie controller. phy_n will be connected to either ahci_n or
> pcie_n.
> 
> What Kishon has suggested here is exactly what I was thinking.
> I think, we should go with this.

I still find it highly inconsistent.

	Arnd

Re: Query: Phy: How to find consumer device on dt platform

[Pratyush Anand]

On Wed, Jan 29, 2014 at 10:24:35PM +0800, Arnd Bergmann wrote:
> On Wednesday 29 January 2014, Pratyush Anand wrote:
> > On Wed, Jan 29, 2014 at 01:41:56PM +0800, Kishon Vijay Abraham I wrote:
> > > > 
> > > > 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.
> > 
> > Probably, it may  not help in this case. How would *phys* defining as
> > above with PCIe/SATA node help phy driver to decide whether current
> > phy instance is associated with PCIe or SATA. Actually, there is no
> > way to pass information from phy consumer driver(pcie/sata driver in
> > this case) to phy driver.
> 
> I don't understand what is unclear about my example where I do just that.
> The argument (0 or 1) gets passed into the driver's xlate function
> when the consumer calls of_phy_get().

May be I did not understand this mechanism earlier.
I got it now. 

Will do this way :)

Thanks for explaining it.

Regards
Pratyush

> 
> > > 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 have multiple instances (say 3) of same phy, which can be
> > programmed either for pcie or for sata. We have multiple instances of
> > ahci and pcie controller. phy_n will be connected to either ahci_n or
> > pcie_n.
> > 
> > What Kishon has suggested here is exactly what I was thinking.
> > I think, we should go with this.
> 
> I still find it highly inconsistent.
> 
> 	Arnd

Re: Query: Phy: How to find consumer device on dt platform

[Arnd Bergmann]

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