Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430245 > unrolled thread

Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

Started byBrian Norris <briannorris@chromium.org>
First post2016-06-24 01:40 +0200
Last post2016-06-28 01:30 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal  PCIe PHY Brian Norris <briannorris@chromium.org> - 2016-06-24 01:40 +0200
    Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal  PCIe PHY Shawn Lin <shawn.lin@rock-chips.com> - 2016-06-24 03:40 +0200
      Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal  PCIe PHY Brian Norris <briannorris@chromium.org> - 2016-06-28 01:30 +0200
    Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal  PCIe PHY Kishon Vijay Abraham I <kishon@ti.com> - 2016-06-27 07:30 +0200
      Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal  PCIe PHY Brian Norris <briannorris@chromium.org> - 2016-06-28 01:30 +0200

#1430245 — Re: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY

FromBrian Norris <briannorris@chromium.org>
Date2016-06-24 01:40 +0200
SubjectRe: [PATCH v3 2/2] phy: add a driver for the Rockchip SoC internal PCIe PHY
Message-ID<rNmo1-it-7@gated-at.bofh.it>
Hi,

On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
> >On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> >>On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> >>>On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
> >>>>This patch to add a generic PHY driver for rockchip PCIe PHY.
> >>>>Access the PHY via registers provided by GRF (general register
> >>>>files) module.
> >>>>
> >>>>Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >>>>---
> >>>>
> >>>>Changes in v3: None
> >>>>Changes in v2: None
> >>>>
[...]
> >>>>diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
> >>>>new file mode 100644
> >>>>index 0000000..bc6cd17
> >>>>--- /dev/null
> >>>>+++ b/drivers/phy/phy-rockchip-pcie.c
> >>>>@@ -0,0 +1,378 @@

[...]

> >>>>+void rockchip_pcie_phy_laneoff(struct phy *phy)
> >>>>+{
> >>>>+    u32 status;
> >>>>+    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> >>>>+    int i;
> >>>>+
> >>>>+    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> >>>>+        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
> >>>>+        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
> >>>>+               PHY_LANE_RX_DET_TH))
> >>>>+            pr_debug("lane %d is used\n", i);
> >>>>+        else
> >>>>+            regmap_write(rk_phy->reg_base,
> >>>>+                     rk_phy->phy_data->pcie_laneoff,
> >>>>+                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
> >>>>+                           PHY_LANE_IDLE_MASK,
> >>>>+                           PHY_LANE_IDLE_A_SHIFT + i));
> >>>>+    }
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);

Shawn, I can't find an example of how you planned to use this (though I
can make educated guesses). As such, it's possible there's some
misunderstanding. Maybe you can include a sample patch for the PCIe
controller driver?

Related: it might make sense to have the PCIe controller and PHY
drivers/bindings all in the same patch series (with proper threading,
which we already talked about off-list).

> >>>Er.. don't use export symbols from phy driver. I think it would be nice if you
> >>>can model the driver in such a way that the PCIe driver can control individual
> >>>phy's.
> >>>
> >>
> >>Yes, I was trying to look for a way not to export symbols from
> >>phy... But I failed to find it as there at least need three
> >>interaction between controller and phy which made me believe we
> >>at least need to export one symbol without adding new API for phy.

My interpretation of the above is that Shawn means we might turn off up
to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).

> >That can be managed by implementing a small state machine within the PHY driver.
> 
> I don't understand your point of implementing a small state machine
> within the PHY driver.

I'm not 100% sure I understand, but I think I have a reasonable
interpretation below.

> Do you mean I need to call vaarious of power_on/off and count the
> on/off times to decide the state machine?
> 
> I would appreciate it If you could elaborate this a bit more or
> show me a example. :)

My interpretation: rather than associating a single PCIe controller
device with a single struct phy that controls up to 4 lanes, Kishon is
suggesting you should have this driver implement 4 phy objects, one for
each lane. You'd need to add #phy-cells = <1> to the DT binding, and
implement an ->of_xlate() hook so we can associate/address them
properly. Then the PCIe controller would call phy_power_off() on each
lane that's not used.

The state machine would come into play because you have additional power
savings to utilize, but only when all PHYs are off. So the state machine
would just track how many of the lane PHYs are still on, and when the
count reaches 0, you call reset_control_assert(rk_phy->phy_rst).

The DT for this would be:

	pcie0: pcie@f8000000 {
		compatible = "rockchip,rk3399-pcie";
		...
		phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
		phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3";
		...
	};

	pcie_phy: pcie-phy {
		compatible = "rockchip,rk3399-pcie-phy";
		...
		#phy-cells = <1>;
		...
	};

(See Documentation/devicetree/bindings/phy/phy-bindings.txt for the
#phy-cells explanation.)

Is that close to what you're suggesting, Kishon? Seems reasonable enough
to me, even if it's slightly more complicated.

Brian

[toc] | [next] | [standalone]

#1430278

FromShawn Lin <shawn.lin@rock-chips.com>
Date2016-06-24 03:40 +0200
Message-ID<rNog9-1uZ-9@gated-at.bofh.it>
In reply to#1430245
在 2016/6/24 7:37, Brian Norris 写道:
> Hi,
>
> On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
>> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
>>> On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
>>>> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
>>>>> On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
>>>>>> This patch to add a generic PHY driver for rockchip PCIe PHY.
>>>>>> Access the PHY via registers provided by GRF (general register
>>>>>> files) module.
>>>>>>
>>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
> [...]
>>>>>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
>>>>>> new file mode 100644
>>>>>> index 0000000..bc6cd17
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/phy/phy-rockchip-pcie.c
>>>>>> @@ -0,0 +1,378 @@
>
> [...]
>
>>>>>> +void rockchip_pcie_phy_laneoff(struct phy *phy)
>>>>>> +{
>>>>>> +    u32 status;
>>>>>> +    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>>>> +    int i;
>>>>>> +
>>>>>> +    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>>>>>> +        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
>>>>>> +        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
>>>>>> +               PHY_LANE_RX_DET_TH))
>>>>>> +            pr_debug("lane %d is used\n", i);
>>>>>> +        else
>>>>>> +            regmap_write(rk_phy->reg_base,
>>>>>> +                     rk_phy->phy_data->pcie_laneoff,
>>>>>> +                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
>>>>>> +                           PHY_LANE_IDLE_MASK,
>>>>>> +                           PHY_LANE_IDLE_A_SHIFT + i));
>>>>>> +    }
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
>
> Shawn, I can't find an example of how you planned to use this (though I
> can make educated guesses). As such, it's possible there's some
> misunderstanding. Maybe you can include a sample patch for the PCIe
> controller driver?

It will be called after rockchip_pcie_init_port for phy to disable
the unused lanes.

>
> Related: it might make sense to have the PCIe controller and PHY
> drivers/bindings all in the same patch series (with proper threading,
> which we already talked about off-list).
>
>>>>> Er.. don't use export symbols from phy driver. I think it would be nice if you
>>>>> can model the driver in such a way that the PCIe driver can control individual
>>>>> phy's.
>>>>>
>>>>
>>>> Yes, I was trying to look for a way not to export symbols from
>>>> phy... But I failed to find it as there at least need three
>>>> interaction between controller and phy which made me believe we
>>>> at least need to export one symbol without adding new API for phy.
>
> My interpretation of the above is that Shawn means we might turn off up
> to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).

yes, pcie drivers support up to 4 lanes. But the device may only
support x2. This is beyound the awareness of PCIe controller, so pcie
controller can't tell which one(s) should be turned off.

>
>>> That can be managed by implementing a small state machine within the PHY driver.
>>
>> I don't understand your point of implementing a small state machine
>> within the PHY driver.
>
> I'm not 100% sure I understand, but I think I have a reasonable
> interpretation below.
>
>> Do you mean I need to call vaarious of power_on/off and count the
>> on/off times to decide the state machine?
>>
>> I would appreciate it If you could elaborate this a bit more or
>> show me a example. :)
>
> My interpretation: rather than associating a single PCIe controller
> device with a single struct phy that controls up to 4 lanes, Kishon is
> suggesting you should have this driver implement 4 phy objects, one for
> each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> implement an ->of_xlate() hook so we can associate/address them
> properly. Then the PCIe controller would call phy_power_off() on each
> lane that's not used.

As I say above, even we have 4 phy objects, PCIe controller still
doesn't know which one(s) to be turned off, so you have to call 4 times
phy_power_off if I understand it correctly. This doesn't look
okay to me.

>
> The state machine would come into play because you have additional power
> savings to utilize, but only when all PHYs are off. So the state machine
> would just track how many of the lane PHYs are still on, and when the
> count reaches 0, you call reset_control_assert(rk_phy->phy_rst).
>
> The DT for this would be:
>
> 	pcie0: pcie@f8000000 {
> 		compatible = "rockchip,rk3399-pcie";
> 		...
> 		phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> 		phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3";
> 		...
> 	};
>
> 	pcie_phy: pcie-phy {
> 		compatible = "rockchip,rk3399-pcie-phy";
> 		...
> 		#phy-cells = <1>;
> 		...
> 	};
>
> (See Documentation/devicetree/bindings/phy/phy-bindings.txt for the
> #phy-cells explanation.)
>
> Is that close to what you're suggesting, Kishon? Seems reasonable enough
> to me, even if it's slightly more complicated.
>
> Brian
>
>
>


-- 
Best Regards
Shawn Lin

[toc] | [prev] | [next] | [standalone]

#1432355

FromBrian Norris <briannorris@chromium.org>
Date2016-06-28 01:30 +0200
Message-ID<rOO8x-76d-5@gated-at.bofh.it>
In reply to#1430278
Hi Shawn,

On Fri, Jun 24, 2016 at 09:37:41AM +0800, Shawn Lin wrote:
> 在 2016/6/24 7:37, Brian Norris 写道:
> >On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
> >>在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
> >>>On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> >>>>On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> >>>>>On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:

> >>>>>>+void rockchip_pcie_phy_laneoff(struct phy *phy)
> >>>>>>+{
> >>>>>>+    u32 status;
> >>>>>>+    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
> >>>>>>+    int i;
> >>>>>>+
> >>>>>>+    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
> >>>>>>+        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
> >>>>>>+        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
> >>>>>>+               PHY_LANE_RX_DET_TH))
> >>>>>>+            pr_debug("lane %d is used\n", i);
> >>>>>>+        else
> >>>>>>+            regmap_write(rk_phy->reg_base,
> >>>>>>+                     rk_phy->phy_data->pcie_laneoff,
> >>>>>>+                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
> >>>>>>+                           PHY_LANE_IDLE_MASK,
> >>>>>>+                           PHY_LANE_IDLE_A_SHIFT + i));
> >>>>>>+    }
> >>>>>>+}
> >>>>>>+EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
> >
> >Shawn, I can't find an example of how you planned to use this (though I
> >can make educated guesses). As such, it's possible there's some
> >misunderstanding. Maybe you can include a sample patch for the PCIe
> >controller driver?
> 
> It will be called after rockchip_pcie_init_port for phy to disable
> the unused lanes.

A real, working patch would be nice. FWIW, when I try the following
diff, my PCIe WiFi dies gloriously:


diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 661d6e04af71..fbb3dd8da0c7 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -373,6 +373,8 @@ static struct pci_ops rockchip_pcie_ops = {
 	.write = rockchip_pcie_wr_conf,
 };
 
+extern void rockchip_pcie_phy_laneoff(struct phy *phy);
+
 /**
  * rockchip_pcie_init_port - Initialize hardware
  * @port: PCIe port information
@@ -533,6 +535,8 @@ static int rockchip_pcie_init_port(struct rockchip_pcie_port *port)
 	pcie_write(port, 0x0,
 		   PCIE_CORE_AXI_CONF_BASE + PCIE_CORE_OB_REGION_DESC1);
 
+	rockchip_pcie_phy_laneoff(port->phy);
+
 	return 0;
 }
 

> >Related: it might make sense to have the PCIe controller and PHY
> >drivers/bindings all in the same patch series (with proper threading,
> >which we already talked about off-list).
> >
> >>>>>Er.. don't use export symbols from phy driver. I think it would be nice if you
> >>>>>can model the driver in such a way that the PCIe driver can control individual
> >>>>>phy's.
> >>>>>
> >>>>
> >>>>Yes, I was trying to look for a way not to export symbols from
> >>>>phy... But I failed to find it as there at least need three
> >>>>interaction between controller and phy which made me believe we
> >>>>at least need to export one symbol without adding new API for phy.
> >
> >My interpretation of the above is that Shawn means we might turn off up
> >to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).
> 
> yes, pcie drivers support up to 4 lanes. But the device may only
> support x2. This is beyound the awareness of PCIe controller, so pcie
> controller can't tell which one(s) should be turned off.

Are you sure the PCIe controller can't know? I'm pretty sure it has know
how many lanes are in use. And your controller even has this coded in
it:

	status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE);
	status =  0x1 << ((status >> PCIE_CORE_PL_CONF_LANE_SHIFT) &
			   PCIE_CORE_PL_CONF_LANE_MASK);
	dev_dbg(port->dev, "current link width is x%d\n", status);

So, doesn't that mean that out of lanes {0, 1, 2, 3}, that every lane
other than 0 <= lane < width is unused? So you can just disable those,
and have the same effect as the rockchip_pcie_phy_laneoff() function?

> >>>That can be managed by implementing a small state machine within the PHY driver.
> >>
> >>I don't understand your point of implementing a small state machine
> >>within the PHY driver.
> >
> >I'm not 100% sure I understand, but I think I have a reasonable
> >interpretation below.
> >
> >>Do you mean I need to call vaarious of power_on/off and count the
> >>on/off times to decide the state machine?
> >>
> >>I would appreciate it If you could elaborate this a bit more or
> >>show me a example. :)
> >
> >My interpretation: rather than associating a single PCIe controller
> >device with a single struct phy that controls up to 4 lanes, Kishon is
> >suggesting you should have this driver implement 4 phy objects, one for
> >each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> >implement an ->of_xlate() hook so we can associate/address them
> >properly. Then the PCIe controller would call phy_power_off() on each
> >lane that's not used.
> 
> As I say above, even we have 4 phy objects, PCIe controller still
> doesn't know which one(s) to be turned off, so you have to call 4 times
> phy_power_off if I understand it correctly. This doesn't look
> okay to me.

Brian

[toc] | [prev] | [next] | [standalone]

#1431722

FromKishon Vijay Abraham I <kishon@ti.com>
Date2016-06-27 07:30 +0200
Message-ID<rOxhn-4pl-3@gated-at.bofh.it>
In reply to#1430245
Hi,

On Friday 24 June 2016 05:07 AM, Brian Norris wrote:
> Hi,
> 
> On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
>> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
>>> On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
>>>> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
>>>>> On Thursday 16 June 2016 06:52 AM, Shawn Lin wrote:
>>>>>> This patch to add a generic PHY driver for rockchip PCIe PHY.
>>>>>> Access the PHY via registers provided by GRF (general register
>>>>>> files) module.
>>>>>>
>>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
> [...]
>>>>>> diff --git a/drivers/phy/phy-rockchip-pcie.c b/drivers/phy/phy-rockchip-pcie.c
>>>>>> new file mode 100644
>>>>>> index 0000000..bc6cd17
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/phy/phy-rockchip-pcie.c
>>>>>> @@ -0,0 +1,378 @@
> 
> [...]
> 
>>>>>> +void rockchip_pcie_phy_laneoff(struct phy *phy)
>>>>>> +{
>>>>>> +    u32 status;
>>>>>> +    struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>>>> +    int i;
>>>>>> +
>>>>>> +    for (i = 0; i < PHY_MAX_LANE_NUM; i++) {
>>>>>> +        status = phy_rd_cfg(rk_phy, PHY_LANE_A_STATUS + i);
>>>>>> +        if (!((status >> PHY_LANE_RX_DET_SHIFT) &
>>>>>> +               PHY_LANE_RX_DET_TH))
>>>>>> +            pr_debug("lane %d is used\n", i);
>>>>>> +        else
>>>>>> +            regmap_write(rk_phy->reg_base,
>>>>>> +                     rk_phy->phy_data->pcie_laneoff,
>>>>>> +                     HIWORD_UPDATE(PHY_LANE_IDLE_OFF,
>>>>>> +                           PHY_LANE_IDLE_MASK,
>>>>>> +                           PHY_LANE_IDLE_A_SHIFT + i));
>>>>>> +    }
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(rockchip_pcie_phy_laneoff);
> 
> Shawn, I can't find an example of how you planned to use this (though I
> can make educated guesses). As such, it's possible there's some
> misunderstanding. Maybe you can include a sample patch for the PCIe
> controller driver?
> 
> Related: it might make sense to have the PCIe controller and PHY
> drivers/bindings all in the same patch series (with proper threading,
> which we already talked about off-list).
> 
>>>>> Er.. don't use export symbols from phy driver. I think it would be nice if you
>>>>> can model the driver in such a way that the PCIe driver can control individual
>>>>> phy's.
>>>>>
>>>>
>>>> Yes, I was trying to look for a way not to export symbols from
>>>> phy... But I failed to find it as there at least need three
>>>> interaction between controller and phy which made me believe we
>>>> at least need to export one symbol without adding new API for phy.
> 
> My interpretation of the above is that Shawn means we might turn off up
> to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).
> 
>>> That can be managed by implementing a small state machine within the PHY driver.
>>
>> I don't understand your point of implementing a small state machine
>> within the PHY driver.
> 
> I'm not 100% sure I understand, but I think I have a reasonable
> interpretation below.
> 
>> Do you mean I need to call vaarious of power_on/off and count the
>> on/off times to decide the state machine?
>>
>> I would appreciate it If you could elaborate this a bit more or
>> show me a example. :)
> 
> My interpretation: rather than associating a single PCIe controller
> device with a single struct phy that controls up to 4 lanes, Kishon is
> suggesting you should have this driver implement 4 phy objects, one for
> each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> implement an ->of_xlate() hook so we can associate/address them
> properly. Then the PCIe controller would call phy_power_off() on each
> lane that's not used.
> 
> The state machine would come into play because you have additional power
> savings to utilize, but only when all PHYs are off. So the state machine
> would just track how many of the lane PHYs are still on, and when the
> count reaches 0, you call reset_control_assert(rk_phy->phy_rst).
> 
> The DT for this would be:
> 
> 	pcie0: pcie@f8000000 {
> 		compatible = "rockchip,rk3399-pcie";
> 		...
> 		phys = <&pcie_phy 0>, <&pcie_phy 1>, <&pcie_phy 2>, <&pcie_phy 3>;
> 		phy-names = "pcie-lane0", "pcie-lane1", "pcie-lane2", "pcie-lane3";
> 		...
> 	};
> 
> 	pcie_phy: pcie-phy {
> 		compatible = "rockchip,rk3399-pcie-phy";
> 		...
> 		#phy-cells = <1>;
> 		...
> 	};
> 
> (See Documentation/devicetree/bindings/phy/phy-bindings.txt for the
> #phy-cells explanation.)
> 
> Is that close to what you're suggesting, Kishon? Seems reasonable enough
> to me, even if it's slightly more complicated.

That's pretty much what I had in mind. Thanks for putting this down in an
elaborate manner.

Thanks
Kishon

[toc] | [prev] | [next] | [standalone]

#1432354

FromBrian Norris <briannorris@chromium.org>
Date2016-06-28 01:30 +0200
Message-ID<rOO8x-76d-7@gated-at.bofh.it>
In reply to#1431722
Hi Kishon,

On Mon, Jun 27, 2016 at 10:54:19AM +0530, Kishon Vijay Abraham I wrote:
> On Friday 24 June 2016 05:07 AM, Brian Norris wrote:
> > On Thu, Jun 23, 2016 at 10:30:17AM +0800, Shawn Lin wrote:
> >> 在 2016/6/20 14:36, Kishon Vijay Abraham I 写道:
> >>> On Monday 20 June 2016 06:28 AM, Shawn Lin wrote:
> >>>> On 2016/6/17 21:08, Kishon Vijay Abraham I wrote:
> >>>>> Er.. don't use export symbols from phy driver. I think it would be nice if you
> >>>>> can model the driver in such a way that the PCIe driver can control individual
> >>>>> phy's.
> >>>>>
> >>>>
> >>>> Yes, I was trying to look for a way not to export symbols from
> >>>> phy... But I failed to find it as there at least need three
> >>>> interaction between controller and phy which made me believe we
> >>>> at least need to export one symbol without adding new API for phy.
> > 
> > My interpretation of the above is that Shawn means we might turn off up
> > to 3 different lanes (i.e., 3 of 4 supported lanes might be unused).
> > 
> >>> That can be managed by implementing a small state machine within the PHY driver.
> >>
> >> I don't understand your point of implementing a small state machine
> >> within the PHY driver.
> > 
> > I'm not 100% sure I understand, but I think I have a reasonable
> > interpretation below.
> > 
> >> Do you mean I need to call vaarious of power_on/off and count the
> >> on/off times to decide the state machine?
> >>
> >> I would appreciate it If you could elaborate this a bit more or
> >> show me a example. :)
> > 
> > My interpretation: rather than associating a single PCIe controller
> > device with a single struct phy that controls up to 4 lanes, Kishon is
> > suggesting you should have this driver implement 4 phy objects, one for
> > each lane. You'd need to add #phy-cells = <1> to the DT binding, and
> > implement an ->of_xlate() hook so we can associate/address them
> > properly. Then the PCIe controller would call phy_power_off() on each
> > lane that's not used.
> > 
> > The state machine would come into play because you have additional power
> > savings to utilize, but only when all PHYs are off. So the state machine
> > would just track how many of the lane PHYs are still on, and when the
> > count reaches 0, you call reset_control_assert(rk_phy->phy_rst).

[...]

> That's pretty much what I had in mind. Thanks for putting this down in an
> elaborate manner.

No problem. Glad we understand your suggestion now.

It remains to be seen whether we can reasonably utilize this suggestion
though. According to Shawn, the PCIe controller doesn't have anough
information to be able to utilize this model effectively. But I have my
doubts, which I've posted in reply to Shawn.

Regards,
Brian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web