| Message ID | 1393524848-8207-2-git-send-email-lho@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Feb 27, 2014 at 11:14:05AM -0700, Loc Ho wrote: > This patch adds function set_speed to the generic PHY framework operation > structure. This function can be called to instruct the PHY underlying layer > at specified lane to configure for specified speed in hertz. why ? looks like clk_set_rate() is your friend here. Can you be more descriptive of the use case ? When will this be used ?
Hi Balbi, > On Thu, Feb 27, 2014 at 11:14:05AM -0700, Loc Ho wrote: >> This patch adds function set_speed to the generic PHY framework operation >> structure. This function can be called to instruct the PHY underlying layer >> at specified lane to configure for specified speed in hertz. > > why ? looks like clk_set_rate() is your friend here. Can you be more > descriptive of the use case ? When will this be used ? > The phy_set_speed is used to configure the operation speed of the PHY at run-time. The clock interface in general is used to configure the clock input to the IP. I don't believe they are the same thing. Maybe it will be clear in my response to your second email -Loc
Hi, On Thu, Feb 27, 2014 at 11:57:44AM -0800, Loc Ho wrote: > > On Thu, Feb 27, 2014 at 11:14:05AM -0700, Loc Ho wrote: > >> This patch adds function set_speed to the generic PHY framework operation > >> structure. This function can be called to instruct the PHY underlying layer > >> at specified lane to configure for specified speed in hertz. > > > > why ? looks like clk_set_rate() is your friend here. Can you be more > > descriptive of the use case ? When will this be used ? > > > > The phy_set_speed is used to configure the operation speed of the PHY > at run-time. The clock interface in general is used to configure the > clock input to the IP. I don't believe they are the same thing. Maybe > it will be clear in my response to your second email The problem with this is that you end up adding SATA-specific details to something which is supposed to be generic. After negoatiation, don't you get any interrupt from your PHY indicating that link speed negotiation is done ? Or is that IRQ only on AHCI IP ? cheers
Hi, >> >> This patch adds function set_speed to the generic PHY framework operation >> >> structure. This function can be called to instruct the PHY underlying layer >> >> at specified lane to configure for specified speed in hertz. >> > >> > why ? looks like clk_set_rate() is your friend here. Can you be more >> > descriptive of the use case ? When will this be used ? >> > >> >> The phy_set_speed is used to configure the operation speed of the PHY >> at run-time. The clock interface in general is used to configure the >> clock input to the IP. I don't believe they are the same thing. Maybe >> it will be clear in my response to your second email > > The problem with this is that you end up adding SATA-specific details to > something which is supposed to be generic. Setting the operation speed is pretty generic from an interface point of view. An generic multi-purpose PHY can support multiple speed. If the upper layer wish to operate at an specified speed (say for testing purpose and etc), it can be allowed. > After negoatiation, don't you > get any interrupt from your PHY indicating that link speed negotiation > is done ? Or is that IRQ only on AHCI IP ? There is NO interrupt from the PHY. The IRQ is assoicated with the AHCI IP. With SATA host flow, it starts off with an COMRESET to start the link negotiation. At that point, it will poll for completion. Any other concerns? -Loc
Hi, On Thu, Feb 27, 2014 at 01:09:57PM -0800, Loc Ho wrote: > >> >> This patch adds function set_speed to the generic PHY framework operation > >> >> structure. This function can be called to instruct the PHY underlying layer > >> >> at specified lane to configure for specified speed in hertz. > >> > > >> > why ? looks like clk_set_rate() is your friend here. Can you be more > >> > descriptive of the use case ? When will this be used ? > >> > > >> > >> The phy_set_speed is used to configure the operation speed of the PHY > >> at run-time. The clock interface in general is used to configure the > >> clock input to the IP. I don't believe they are the same thing. Maybe > >> it will be clear in my response to your second email > > > > The problem with this is that you end up adding SATA-specific details to > > something which is supposed to be generic. > > Setting the operation speed is pretty generic from an interface point > of view. An generic multi-purpose PHY can support multiple speed. If no it's not. Specially when you consider that your "speed" argument can be just about anything and depending on the underlying bus, it *will* be treated differently. Note that e.g. in OMAP devices the exact *same* PHY IP is used for PCIe, SATA and USB... now, let's assume for the sake of argument that we were to implement ->set_speed() in that environment, how do you plan to do that ? a 6MHz arguments isn't valid from USB stand point and could mean different things in PCIe or SATA. > the upper layer wish to operate at an specified speed (say for testing > purpose and etc), it can be allowed. anything for testing purposes, should be limited to test scenarios. > > After negoatiation, don't you > > get any interrupt from your PHY indicating that link speed negotiation > > is done ? Or is that IRQ only on AHCI IP ? > > There is NO interrupt from the PHY. The IRQ is assoicated with the > AHCI IP. With SATA host flow, it starts off with an COMRESET to start > the link negotiation. At that point, it will poll for completion. > > Any other concerns? hey, calm down... just trying to prevent us from applying something which isn't truly generic and I don't think "->set_speed" is generic enough. The semantics of the "speed" argument won't be valid for all use cases. I can already see people using that to pass USB_SPEED_{LOW,FULL,HIGH,SUPER}, instead of actual speed numbers. We wil end up with a mess to handle from a PHY driver, specially in cases such as OMAP where, as mentioned above, the same IP is used for SATA, PCIe and USB3.
HI, >> >> >> This patch adds function set_speed to the generic PHY framework operation >> >> >> structure. This function can be called to instruct the PHY underlying layer >> >> >> at specified lane to configure for specified speed in hertz. >> >> > >> >> > why ? looks like clk_set_rate() is your friend here. Can you be more >> >> > descriptive of the use case ? When will this be used ? >> >> > >> >> >> >> The phy_set_speed is used to configure the operation speed of the PHY >> >> at run-time. The clock interface in general is used to configure the >> >> clock input to the IP. I don't believe they are the same thing. Maybe >> >> it will be clear in my response to your second email >> > >> > The problem with this is that you end up adding SATA-specific details to >> > something which is supposed to be generic. >> >> Setting the operation speed is pretty generic from an interface point >> of view. An generic multi-purpose PHY can support multiple speed. If > > no it's not. Specially when you consider that your "speed" argument can > be just about anything and depending on the underlying bus, it *will* be > treated differently. Note that e.g. in OMAP devices the exact *same* PHY > IP is used for PCIe, SATA and USB... now, let's assume for the sake of > argument that we were to implement ->set_speed() in that environment, > how do you plan to do that ? a 6MHz arguments isn't valid from USB stand > point and could mean different things in PCIe or SATA. > Correct me if I am wrong here. If the same PHY is used for PCIe, SATA, and USB, would you not need to indicate what speed the PHY should be operated at - unless the underlying IP magically negotiate and configured automatically? If so, what about PHY isn't so intelligent? How should you suggest that we handle these? >> the upper layer wish to operate at an specified speed (say for testing >> purpose and etc), it can be allowed. > > anything for testing purposes, should be limited to test scenarios. Testing purpose is only one use case. Another use case is to limit the speed so that I can confirm the driver actually works with various speed of the device and handle correctly. > >> > After negoatiation, don't you >> > get any interrupt from your PHY indicating that link speed negotiation >> > is done ? Or is that IRQ only on AHCI IP ? >> >> There is NO interrupt from the PHY. The IRQ is assoicated with the >> AHCI IP. With SATA host flow, it starts off with an COMRESET to start >> the link negotiation. At that point, it will poll for completion. >> >> Any other concerns? > > hey, calm down... just trying to prevent us from applying something > which isn't truly generic and I don't think "->set_speed" is generic > enough. The semantics of the "speed" argument won't be valid for all use > cases. > > I can already see people using that to pass > USB_SPEED_{LOW,FULL,HIGH,SUPER}, instead of actual speed numbers. We wil > end up with a mess to handle from a PHY driver, specially in cases such > as OMAP where, as mentioned above, the same IP is used for SATA, PCIe > and USB3. > This PHY isn't as "intelligent" as other PHY's. What would you suggest as I need an method to indicate to the underlying PHY that I want to operate at an specified speed? -Loc
Hi Felipe and Kishon, >>> >> >> This patch adds function set_speed to the generic PHY framework operation >>> >> >> structure. This function can be called to instruct the PHY underlying layer >>> >> >> at specified lane to configure for specified speed in hertz. >>> >> > >>> >> > why ? looks like clk_set_rate() is your friend here. Can you be more >>> >> > descriptive of the use case ? When will this be used ? >>> >> > >>> >> >>> >> The phy_set_speed is used to configure the operation speed of the PHY >>> >> at run-time. The clock interface in general is used to configure the >>> >> clock input to the IP. I don't believe they are the same thing. Maybe >>> >> it will be clear in my response to your second email >>> > >>> > The problem with this is that you end up adding SATA-specific details to >>> > something which is supposed to be generic. >>> >>> Setting the operation speed is pretty generic from an interface point >>> of view. An generic multi-purpose PHY can support multiple speed. If >> >> no it's not. Specially when you consider that your "speed" argument can >> be just about anything and depending on the underlying bus, it *will* be >> treated differently. Note that e.g. in OMAP devices the exact *same* PHY >> IP is used for PCIe, SATA and USB... now, let's assume for the sake of >> argument that we were to implement ->set_speed() in that environment, >> how do you plan to do that ? a 6MHz arguments isn't valid from USB stand >> point and could mean different things in PCIe or SATA. >> > > Correct me if I am wrong here. If the same PHY is used for PCIe, SATA, > and USB, would you not need to indicate what speed the PHY should be > operated at - unless the underlying IP magically negotiate and > configured automatically? If so, what about PHY isn't so intelligent? > How should you suggest that we handle these? > >>> the upper layer wish to operate at an specified speed (say for testing >>> purpose and etc), it can be allowed. >> >> anything for testing purposes, should be limited to test scenarios. > > Testing purpose is only one use case. Another use case is to limit the > speed so that I can confirm the driver actually works with various > speed of the device and handle correctly. > >> >>> > After negoatiation, don't you >>> > get any interrupt from your PHY indicating that link speed negotiation >>> > is done ? Or is that IRQ only on AHCI IP ? >>> >>> There is NO interrupt from the PHY. The IRQ is assoicated with the >>> AHCI IP. With SATA host flow, it starts off with an COMRESET to start >>> the link negotiation. At that point, it will poll for completion. >>> >>> Any other concerns? >> >> hey, calm down... just trying to prevent us from applying something >> which isn't truly generic and I don't think "->set_speed" is generic >> enough. The semantics of the "speed" argument won't be valid for all use >> cases. >> >> I can already see people using that to pass >> USB_SPEED_{LOW,FULL,HIGH,SUPER}, instead of actual speed numbers. We wil >> end up with a mess to handle from a PHY driver, specially in cases such >> as OMAP where, as mentioned above, the same IP is used for SATA, PCIe >> and USB3. >> > > This PHY isn't as "intelligent" as other PHY's. What would you suggest > as I need an method to indicate to the underlying PHY that I want to > operate at an specified speed? Sorry if I am pinging you guys too fast here. I am look from an solution and open to any solution in which it is acceptable for your original intent of the generic PHY framework. I understand that you don't believe set_speed is generic enough and may not apply to omap. Or if you prefer we can try changing the init function to take an initial MAX speed? -Loc
Hi, >>>> >> >> This patch adds function set_speed to the generic PHY framework operation >>>> >> >> structure. This function can be called to instruct the PHY underlying layer >>>> >> >> at specified lane to configure for specified speed in hertz. >>>> >> > >>>> >> > why ? looks like clk_set_rate() is your friend here. Can you be more >>>> >> > descriptive of the use case ? When will this be used ? >>>> >> > >>>> >> >>>> >> The phy_set_speed is used to configure the operation speed of the PHY >>>> >> at run-time. The clock interface in general is used to configure the >>>> >> clock input to the IP. I don't believe they are the same thing. Maybe >>>> >> it will be clear in my response to your second email >>>> > >>>> > The problem with this is that you end up adding SATA-specific details to >>>> > something which is supposed to be generic. >>>> >>>> Setting the operation speed is pretty generic from an interface point >>>> of view. An generic multi-purpose PHY can support multiple speed. If >>> >>> no it's not. Specially when you consider that your "speed" argument can >>> be just about anything and depending on the underlying bus, it *will* be >>> treated differently. Note that e.g. in OMAP devices the exact *same* PHY >>> IP is used for PCIe, SATA and USB... now, let's assume for the sake of >>> argument that we were to implement ->set_speed() in that environment, >>> how do you plan to do that ? a 6MHz arguments isn't valid from USB stand >>> point and could mean different things in PCIe or SATA. >>> >> >> Correct me if I am wrong here. If the same PHY is used for PCIe, SATA, >> and USB, would you not need to indicate what speed the PHY should be >> operated at - unless the underlying IP magically negotiate and >> configured automatically? If so, what about PHY isn't so intelligent? >> How should you suggest that we handle these? >> >>>> the upper layer wish to operate at an specified speed (say for testing >>>> purpose and etc), it can be allowed. >>> >>> anything for testing purposes, should be limited to test scenarios. >> >> Testing purpose is only one use case. Another use case is to limit the >> speed so that I can confirm the driver actually works with various >> speed of the device and handle correctly. >> >>> >>>> > After negoatiation, don't you >>>> > get any interrupt from your PHY indicating that link speed negotiation >>>> > is done ? Or is that IRQ only on AHCI IP ? >>>> >>>> There is NO interrupt from the PHY. The IRQ is assoicated with the >>>> AHCI IP. With SATA host flow, it starts off with an COMRESET to start >>>> the link negotiation. At that point, it will poll for completion. >>>> >>>> Any other concerns? >>> >>> hey, calm down... just trying to prevent us from applying something >>> which isn't truly generic and I don't think "->set_speed" is generic >>> enough. The semantics of the "speed" argument won't be valid for all use >>> cases. >>> >>> I can already see people using that to pass >>> USB_SPEED_{LOW,FULL,HIGH,SUPER}, instead of actual speed numbers. We wil >>> end up with a mess to handle from a PHY driver, specially in cases such >>> as OMAP where, as mentioned above, the same IP is used for SATA, PCIe >>> and USB3. >>> >> >> This PHY isn't as "intelligent" as other PHY's. What would you suggest >> as I need an method to indicate to the underlying PHY that I want to >> operate at an specified speed? > > Sorry if I am pinging you guys too fast here. I am look from an > solution and open to any solution in which it is acceptable for your > original intent of the generic PHY framework. I understand that you > don't believe set_speed is generic enough and may not apply to omap. > Or if you prefer we can try changing the init function to take an > initial MAX speed? > For the initial version, I will remove support for Gen1/Gen2 until we come up with an solution. Then post patches that will address individual errata's. -Loc
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 645c867..5451b6d 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -258,6 +258,36 @@ int phy_power_off(struct phy *phy) EXPORT_SYMBOL_GPL(phy_power_off); /** + * phy_set_speed - set an specified lane at an specified speed + * @phy: phy instance to be set + * @lane: zero-based lane index + * @speed: operating speed in hz + * + * Returns 0 if successful, -ENOTSUPP if not supported, + * -EINVAL if parameter out of range. + */ +int phy_set_speed(struct phy *phy, int lane, u64 speed) +{ + int ret = -ENOTSUPP; + + mutex_lock(&phy->mutex); + if (phy->ops->set_speed) { + ret = phy->ops->set_speed(phy, lane, speed); + if (ret < 0) { + dev_err(&phy->dev, "phy set speed failed --> %d\n", + ret); + goto out; + } + } + +out: + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_set_speed); + +/** * of_phy_get() - lookup and obtain a reference to a phy by phandle * @dev: device that requests this phy * @index: the index of the phy diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index e273e5a..4eb589c 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -27,6 +27,7 @@ struct phy; * @exit: operation to be performed while exiting * @power_on: powering on the phy * @power_off: powering off the phy + * @set_speed: set operation speed in hz * @owner: the module owner containing the ops */ struct phy_ops { @@ -34,6 +35,7 @@ struct phy_ops { int (*exit)(struct phy *phy); int (*power_on)(struct phy *phy); int (*power_off)(struct phy *phy); + int (*set_speed)(struct phy *phy, int lane, u64 speed); struct module *owner; }; @@ -145,6 +147,7 @@ static inline void phy_set_bus_width(struct phy *phy, int bus_width) { phy->attrs.bus_width = bus_width; } +int phy_set_speed(struct phy *phy, int lane, u64 speed); struct phy *phy_get(struct device *dev, const char *string); struct phy *devm_phy_get(struct device *dev, const char *string); void phy_put(struct phy *phy); @@ -227,6 +230,11 @@ static inline void phy_set_bus_width(struct phy *phy, int bus_width) return; } +static inline int phy_set_speed(struct phy *phy, int lane, u64 speed) +{ + return -ENOSYS; +} + static inline struct phy *phy_get(struct device *dev, const char *string) { return ERR_PTR(-ENOSYS);
This patch adds function set_speed to the generic PHY framework operation structure. This function can be called to instruct the PHY underlying layer at specified lane to configure for specified speed in hertz. Signed-off-by: Loc Ho <lho@apm.com> --- drivers/phy/phy-core.c | 30 ++++++++++++++++++++++++++++++ include/linux/phy/phy.h | 8 ++++++++ 2 files changed, 38 insertions(+), 0 deletions(-)