linux-kernel - Re: [PATCH v3] usb: phy-samsung-usb: Simplify PMU register handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 30 Jul 2013 09:25:51 +0300
From:	Felipe Balbi <balbi@...com>
To:	Julius Werner <jwerner@...omium.org>
CC:	Felipe Balbi <balbi@...com>, <linux-kernel@...r.kernel.org>,
	<linux-usb@...r.kernel.org>, <linux-samsung-soc@...r.kernel.org>,
	Tomasz Figa <t.figa@...sung.com>,
	Vivek Gautam <gautam.vivek@...sung.com>,
	<devicetree@...r.kernel.org>,
	Sylwester Nawrocki <sylvester.nawrocki@...il.com>
Subject: Re: [PATCH v3] usb: phy-samsung-usb: Simplify PMU register handling

On Mon, Jul 29, 2013 at 02:44:13PM -0700, Julius Werner wrote:
> This patch simplifies the way the phy-samsung-usb code finds the correct
> power management register to enable PHY clock gating. Previously, the
> code would calculate the register address from a device tree supplied
> base address and add an offset based on the PHY type.
> 
> Since every PHY has its own device tree entry and needs only one
> register, we can just encode the address itself in the device tree and
> remove the diffentiation in the code. The bitmask needed to specify the
> bit within that register stays in place, allowing support for platforms
> like s3c64xx that use different bits within the same register. Due to
> this simplification, the whole complication of a Samsung-specific USB
> PHY type can be removed from the PHY driver.
> 
> Signed-off-by: Julius Werner <jwerner@...omium.org>

alright, let's try to split this patch down, it's too large.

> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index ef57277..5a754d7 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -473,7 +473,7 @@
>  		ranges;
>  
>  		usbphy-sys {
> -			reg = <0x10040704 0x8>;
> +			reg = <0x10040704 0x4>;
>  		};
>  	};
>  
> @@ -505,7 +505,7 @@
>  		ranges;
>  
>  		usbphy-sys {
> -			reg = <0x10040704 0x8>,
> +			reg = <0x10040708 0x4>,
>  			      <0x10050230 0x4>;
>  		};
>  	};
> diff --git a/drivers/usb/phy/phy-samsung-usb.c b/drivers/usb/phy/phy-samsung-usb.c
> index ac025ca..32c5264 100644
> --- a/drivers/usb/phy/phy-samsung-usb.c
> +++ b/drivers/usb/phy/phy-samsung-usb.c
> @@ -27,7 +27,6 @@
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> -#include <linux/usb/samsung_usb_phy.h>
>  
>  #include "phy-samsung-usb.h"
>  
> @@ -42,9 +41,9 @@ int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
>  		return -ENODEV;
>  	}
>  
> -	sphy->pmuregs = of_iomap(usbphy_sys, 0);
> +	sphy->pmureg = of_iomap(usbphy_sys, 0);

this rename can go in a separate patch.

> @@ -75,35 +74,25 @@ EXPORT_SYMBOL_GPL(samsung_usbphy_parse_dt);
>   */
>  void samsung_usbphy_set_isolation_4210(struct samsung_usbphy *sphy, bool on)
>  {
> -	void __iomem *reg = NULL;
>  	u32 reg_val;
> -	u32 en_mask = 0;
>  
> -	if (!sphy->pmuregs) {
> +	if (!sphy->pmureg) {
>  		dev_warn(sphy->dev, "Can't set pmu isolation\n");
>  		return;
>  	}
>  
> -	if (sphy->phy_type == USB_PHY_TYPE_DEVICE) {
> -		reg = sphy->pmuregs + sphy->drv_data->devphy_reg_offset;
> -		en_mask = sphy->drv_data->devphy_en_mask;
> -	} else if (sphy->phy_type == USB_PHY_TYPE_HOST) {
> -		reg = sphy->pmuregs + sphy->drv_data->hostphy_reg_offset;
> -		en_mask = sphy->drv_data->hostphy_en_mask;
> -	}
> 
> -	reg_val = readl(reg);
> +	reg_val = readl(sphy->pmureg);
>  
>  	if (on)
> -		reg_val &= ~en_mask;
> +		reg_val &= ~sphy->drv_data->phy_en_mask;
>  	else
> -		reg_val |= en_mask;
> +		reg_val |= sphy->drv_data->phy_en_mask;

removing the if case up there and updating this if here could be a patch
of its own.

> @@ -111,7 +100,7 @@ EXPORT_SYMBOL_GPL(samsung_usbphy_set_isolation_4210);
>  /*
>   * Configure the mode of working of usb-phy here: HOST/DEVICE.
>   */
> -void samsung_usbphy_cfg_sel(struct samsung_usbphy *sphy)
> +void samsung_usbphy_cfg_sel(struct samsung_usbphy *sphy, bool device_mode)

passing this extra parameter could be a patch of its own.

> @@ -122,31 +111,15 @@ void samsung_usbphy_cfg_sel(struct samsung_usbphy *sphy)
>  
>  	reg = readl(sphy->sysreg);
>  
> -	if (sphy->phy_type == USB_PHY_TYPE_DEVICE)
> +	if (device_mode)
>  		reg &= ~EXYNOS_USB20PHY_CFG_HOST_LINK;
> -	else if (sphy->phy_type == USB_PHY_TYPE_HOST)
> +	else
>  		reg |= EXYNOS_USB20PHY_CFG_HOST_LINK;
>  
>  	writel(reg, sphy->sysreg);
>  }
>  EXPORT_SYMBOL_GPL(samsung_usbphy_cfg_sel);
>  
> -/*
> - * PHYs are different for USB Device and USB Host.
> - * This make sure that correct PHY type is selected before
> - * any operation on PHY.
> - */
> -int samsung_usbphy_set_type(struct usb_phy *phy,
> -				enum samsung_usb_phy_type phy_type)
> -{
> -	struct samsung_usbphy *sphy = phy_to_sphy(phy);
> -
> -	sphy->phy_type = phy_type;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(samsung_usbphy_set_type);

removing this function should be a patch of its own.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)