Re: [PATCH 3/6] usb: phy: samsung: Consolidate reference clock rate handling
Hi Felipe,
On Wednesday 27 of March 2013 15:19:58 Felipe Balbi wrote:
Hi,
On Tue, Mar 26, 2013 at 03:53:12PM +0100, Tomasz Figa wrote:
@@ -307,6 +310,7 @@ static int samsung_usb3phy_remove(struct
platform_device *pdev)
static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
.cpu_type = TYPE_EXYNOS5250,
.devphy_en_mask = EXYNOS_USBPHY_ENABLE,
+ .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12,
why isn't this just a clk_get_rate() ???
As the name suggests, this is a function to get appropriate CLKSEL value to
write into PHYCLK register for reference clock rate on particular platform
(clk_get_rate is used inside to get the rate).
Best regards,
--
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Kernel and System Framework
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] usb: phy: samsung: Consolidate reference clock rate handling
Hi,
On Wed, Mar 27, 2013 at 02:26:08PM +0100, Tomasz Figa wrote:
Hi Felipe,
On Wednesday 27 of March 2013 15:19:58 Felipe Balbi wrote:
Hi,
On Tue, Mar 26, 2013 at 03:53:12PM +0100, Tomasz Figa wrote:
@@ -307,6 +310,7 @@ static int samsung_usb3phy_remove(struct
platform_device *pdev)
static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
.cpu_type = TYPE_EXYNOS5250,
.devphy_en_mask = EXYNOS_USBPHY_ENABLE,
+ .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12,
why isn't this just a clk_get_rate() ???
As the name suggests, this is a function to get appropriate CLKSEL value to
write into PHYCLK register for reference clock rate on particular platform
(clk_get_rate is used inside to get the rate).
alright, then you don't need this function pointer at all, look at both
your rate_to_clksel functions (quoted below):
| +int samsung_usbphy_rate_to_clksel_64xx(struct samsung_usbphy *sphy,
| + unsigned long rate)
| +{
| + unsigned int clksel;
| +
| + switch (rate) {
| + case 12 * MHZ:
| + clksel = PHYCLK_CLKSEL_12M;
| + break;
| + case 24 * MHZ:
| + clksel = PHYCLK_CLKSEL_24M;
| + break;
| + case 48 * MHZ:
| + clksel = PHYCLK_CLKSEL_48M;
| + break;
| + default:
| + dev_err(sphy-dev,
| + Invalid reference clock frequency: %lu\n, rate);
| + return -EINVAL;
| + }
| +
| + return clksel;
| +}
| +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_64xx);
| +
| +int samsung_usbphy_rate_to_clksel_4x12(struct samsung_usbphy *sphy,
| + unsigned long rate)
| +{
| + unsigned int clksel;
| +
| + switch (rate) {
| + case 9600 * KHZ:
| + clksel = FSEL_CLKSEL_9600K;
| + break;
| + case 10 * MHZ:
| + clksel = FSEL_CLKSEL_10M;
| + break;
| + case 12 * MHZ:
| + clksel = FSEL_CLKSEL_12M;
| + break;
| + case 19200 * KHZ:
| + clksel = FSEL_CLKSEL_19200K;
| + break;
| + case 20 * MHZ:
| + clksel = FSEL_CLKSEL_20M;
| + break;
| + case 24 * MHZ:
| + clksel = FSEL_CLKSEL_24M;
| + break;
| + case 50 * MHZ:
| + clksel = FSEL_CLKSEL_50M;
| + break;
| + default:
| + dev_err(sphy-dev,
| + Invalid reference clock frequency: %lu\n, rate);
| + return -EINVAL;
| + }
| +
| + return clksel;
| +}
| +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_4x12);
They both return the same thing and test the same thing. You clearly
don't need this function pointer. The only thing you need to be careful
is that different platforms will have different clock rates, but that
can just as easily be turned into a generic check.
I don't see the need for $SUBJECT.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 3/6] usb: phy: samsung: Consolidate reference clock rate handling
On Wednesday 27 of March 2013 15:31:49 Felipe Balbi wrote:
Hi,
On Wed, Mar 27, 2013 at 02:26:08PM +0100, Tomasz Figa wrote:
Hi Felipe,
On Wednesday 27 of March 2013 15:19:58 Felipe Balbi wrote:
Hi,
On Tue, Mar 26, 2013 at 03:53:12PM +0100, Tomasz Figa wrote:
@@ -307,6 +310,7 @@ static int samsung_usb3phy_remove(struct
platform_device *pdev)
static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
.cpu_type = TYPE_EXYNOS5250,
.devphy_en_mask = EXYNOS_USBPHY_ENABLE,
+ .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12,
why isn't this just a clk_get_rate() ???
As the name suggests, this is a function to get appropriate CLKSEL value
to
write into PHYCLK register for reference clock rate on particular platform
(clk_get_rate is used inside to get the rate).
alright, then you don't need this function pointer at all, look at both
your rate_to_clksel functions (quoted below):
| +int samsung_usbphy_rate_to_clksel_64xx(struct samsung_usbphy *sphy,
| + unsigned long
| rate)
| +{
| + unsigned int clksel;
| +
| + switch (rate) {
| + case 12 * MHZ:
| + clksel = PHYCLK_CLKSEL_12M;
Please note the PHYCLK_CLKSEL_ prefix here...
| + break;
| + case 24 * MHZ:
| + clksel = PHYCLK_CLKSEL_24M;
| + break;
| + case 48 * MHZ:
| + clksel = PHYCLK_CLKSEL_48M;
| + break;
| + default:
| + dev_err(sphy-dev,
| + Invalid reference clock frequency: %lu\n, rate);
| + return -EINVAL;
| + }
| +
| + return clksel;
| +}
| +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_64xx);
| +
| +int samsung_usbphy_rate_to_clksel_4x12(struct samsung_usbphy *sphy,
| + unsigned long
| rate)
| +{
| + unsigned int clksel;
| +
| + switch (rate) {
| + case 9600 * KHZ:
| + clksel = FSEL_CLKSEL_9600K;
| + break;
| + case 10 * MHZ:
| + clksel = FSEL_CLKSEL_10M;
| + break;
| + case 12 * MHZ:
| + clksel = FSEL_CLKSEL_12M;
..and then FSEL_CLKSEL_ here. They have different values. (Their names are a
bit unfortunate, though...)
Best regards,
--
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Kernel and System Framework
| + break;
| + case 19200 * KHZ:
| + clksel = FSEL_CLKSEL_19200K;
| + break;
| + case 20 * MHZ:
| + clksel = FSEL_CLKSEL_20M;
| + break;
| + case 24 * MHZ:
| + clksel = FSEL_CLKSEL_24M;
| + break;
| + case 50 * MHZ:
| + clksel = FSEL_CLKSEL_50M;
| + break;
| + default:
| + dev_err(sphy-dev,
| + Invalid reference clock frequency: %lu\n, rate);
| + return -EINVAL;
| + }
| +
| + return clksel;
| +}
| +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_4x12);
They both return the same thing and test the same thing. You clearly
don't need this function pointer. The only thing you need to be careful
is that different platforms will have different clock rates, but that
can just as easily be turned into a generic check.
I don't see the need for $SUBJECT.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] usb: phy: samsung: Consolidate reference clock rate handling
Hi,
On Wed, Mar 27, 2013 at 02:39:08PM +0100, Tomasz Figa wrote:
On Wednesday 27 of March 2013 15:31:49 Felipe Balbi wrote:
Hi,
On Wed, Mar 27, 2013 at 02:26:08PM +0100, Tomasz Figa wrote:
Hi Felipe,
On Wednesday 27 of March 2013 15:19:58 Felipe Balbi wrote:
Hi,
On Tue, Mar 26, 2013 at 03:53:12PM +0100, Tomasz Figa wrote:
@@ -307,6 +310,7 @@ static int samsung_usb3phy_remove(struct
platform_device *pdev)
static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
.cpu_type = TYPE_EXYNOS5250,
.devphy_en_mask = EXYNOS_USBPHY_ENABLE,
+ .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12,
why isn't this just a clk_get_rate() ???
As the name suggests, this is a function to get appropriate CLKSEL value
to
write into PHYCLK register for reference clock rate on particular platform
(clk_get_rate is used inside to get the rate).
alright, then you don't need this function pointer at all, look at both
your rate_to_clksel functions (quoted below):
| +int samsung_usbphy_rate_to_clksel_64xx(struct samsung_usbphy *sphy,
| + unsigned long
| rate)
| +{
| + unsigned int clksel;
| +
| + switch (rate) {
| + case 12 * MHZ:
| + clksel = PHYCLK_CLKSEL_12M;
Please note the PHYCLK_CLKSEL_ prefix here...
| + break;
| + case 24 * MHZ:
| + clksel = PHYCLK_CLKSEL_24M;
| + break;
| + case 48 * MHZ:
| + clksel = PHYCLK_CLKSEL_48M;
| + break;
| + default:
| + dev_err(sphy-dev,
| + Invalid reference clock frequency: %lu\n, rate);
| + return -EINVAL;
| + }
| +
| + return clksel;
| +}
| +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_64xx);
| +
| +int samsung_usbphy_rate_to_clksel_4x12(struct samsung_usbphy *sphy,
| + unsigned long
| rate)
| +{
| + unsigned int clksel;
| +
| + switch (rate) {
| + case 9600 * KHZ:
| + clksel = FSEL_CLKSEL_9600K;
| + break;
| + case 10 * MHZ:
| + clksel = FSEL_CLKSEL_10M;
| + break;
| + case 12 * MHZ:
| + clksel = FSEL_CLKSEL_12M;
..and then FSEL_CLKSEL_ here. They have different values. (Their names are a
bit unfortunate, though...)
indeed, my eyes failed there. So I agree with the patch :-)
sorry for the noise.
--
balbi
signature.asc
Description: Digital signature
[PATCH 3/6] usb: phy: samsung: Consolidate reference clock rate handling
This patch cleans up handling of reference clock rate in Samsung USB PHY
drivers. It is mostly a cosmetic change but improves error handling in
case of failing to get reference clock or invalid clock rate.
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/usb/phy/phy-samsung-usb.c | 114 ++---
drivers/usb/phy/phy-samsung-usb.h | 7 +++
drivers/usb/phy/phy-samsung-usb2.c | 8 ++-
drivers/usb/phy/phy-samsung-usb3.c | 6 +-
4 files changed, 86 insertions(+), 49 deletions(-)
diff --git a/drivers/usb/phy/phy-samsung-usb.c
b/drivers/usb/phy/phy-samsung-usb.c
index 62cdb7e..c40ea32 100644
--- a/drivers/usb/phy/phy-samsung-usb.c
+++ b/drivers/usb/phy/phy-samsung-usb.c
@@ -162,13 +162,76 @@ int samsung_usbphy_set_type(struct usb_phy *phy,
}
EXPORT_SYMBOL_GPL(samsung_usbphy_set_type);
+int samsung_usbphy_rate_to_clksel_64xx(struct samsung_usbphy *sphy,
+ unsigned long rate)
+{
+ unsigned int clksel;
+
+ switch (rate) {
+ case 12 * MHZ:
+ clksel = PHYCLK_CLKSEL_12M;
+ break;
+ case 24 * MHZ:
+ clksel = PHYCLK_CLKSEL_24M;
+ break;
+ case 48 * MHZ:
+ clksel = PHYCLK_CLKSEL_48M;
+ break;
+ default:
+ dev_err(sphy-dev,
+ Invalid reference clock frequency: %lu\n, rate);
+ return -EINVAL;
+ }
+
+ return clksel;
+}
+EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_64xx);
+
+int samsung_usbphy_rate_to_clksel_4x12(struct samsung_usbphy *sphy,
+ unsigned long rate)
+{
+ unsigned int clksel;
+
+ switch (rate) {
+ case 9600 * KHZ:
+ clksel = FSEL_CLKSEL_9600K;
+ break;
+ case 10 * MHZ:
+ clksel = FSEL_CLKSEL_10M;
+ break;
+ case 12 * MHZ:
+ clksel = FSEL_CLKSEL_12M;
+ break;
+ case 19200 * KHZ:
+ clksel = FSEL_CLKSEL_19200K;
+ break;
+ case 20 * MHZ:
+ clksel = FSEL_CLKSEL_20M;
+ break;
+ case 24 * MHZ:
+ clksel = FSEL_CLKSEL_24M;
+ break;
+ case 50 * MHZ:
+ clksel = FSEL_CLKSEL_50M;
+ break;
+ default:
+ dev_err(sphy-dev,
+ Invalid reference clock frequency: %lu\n, rate);
+ return -EINVAL;
+ }
+
+ return clksel;
+}
+EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_4x12);
+
/*
* Returns reference clock frequency selection value
*/
int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy)
{
struct clk *ref_clk;
- int refclk_freq = 0;
+ unsigned long rate;
+ int refclk_freq;
/*
* In exynos5250 USB host and device PHY use
@@ -183,52 +246,9 @@ int samsung_usbphy_get_refclk_freq(struct samsung_usbphy
*sphy)
return PTR_ERR(ref_clk);
}
- if (sphy-drv_data-cpu_type == TYPE_EXYNOS5250) {
- /* set clock frequency for PLL */
- switch (clk_get_rate(ref_clk)) {
- case 9600 * KHZ:
- refclk_freq = FSEL_CLKSEL_9600K;
- break;
- case 10 * MHZ:
- refclk_freq = FSEL_CLKSEL_10M;
- break;
- case 12 * MHZ:
- refclk_freq = FSEL_CLKSEL_12M;
- break;
- case 19200 * KHZ:
- refclk_freq = FSEL_CLKSEL_19200K;
- break;
- case 20 * MHZ:
- refclk_freq = FSEL_CLKSEL_20M;
- break;
- case 50 * MHZ:
- refclk_freq = FSEL_CLKSEL_50M;
- break;
- case 24 * MHZ:
- default:
- /* default reference clock */
- refclk_freq = FSEL_CLKSEL_24M;
- break;
- }
- } else {
- switch (clk_get_rate(ref_clk)) {
- case 12 * MHZ:
- refclk_freq = PHYCLK_CLKSEL_12M;
- break;
- case 24 * MHZ:
- refclk_freq = PHYCLK_CLKSEL_24M;
- break;
- case 48 * MHZ:
- refclk_freq = PHYCLK_CLKSEL_48M;
- break;
- default:
- if (sphy-drv_data-cpu_type == TYPE_S3C64XX)
- refclk_freq = PHYCLK_CLKSEL_48M;
- else
- refclk_freq = PHYCLK_CLKSEL_24M;
- break;
- }
- }
+ rate =
