| Message ID | 3550912.kR0pejLP0h@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Heiko Stübner <heiko@sntech.de> writes: > Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang: >> Add power domain drivers based on generic power domain for >> Rockchip platform, and support RK3288. >> >> Verified on url = >> https://chromium.googlesource.com/chromiumos/third_party/kernel. >> >> At the moment,there are mass of products are using the driver. >> I believe the driver can happy work for next kernel. > > I've taken a look at the driver and here are some global remarks: > > (1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks > bisectability, as the driver itself in patch 2 also includes the header and > would thus fail to compile if the later patch 3 is missing. > Ideally I think the header addition should be a separate patch itself, so that > we can possibly share it between driver and dts branches. > So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes. > > > (2) The dts-changes in patch 3 should also add any necessary power-domain > assignment on devices if they're still missing, so that we don't introduce > regressions. In my case my work-in-progress edp died because the powerdomain > was turned off automatically it seems. > > > (3) more like wondering @Kevin or so, is there some more generic place for a > power-domain driver nowadays? I think the preference has been to put these under drivers/soc/<vendor> for now, so they can shared across arm32 and arm64. > (4) As Tomasz remarked previously the dts should represent the hardware and > the power-domains are part of the pmu. There is a recent addition from Linus > Walleij, called simple-mfd [a] that is supposed to get added real early for > kernel 4.2. So I'd think the power-domains should use that and the patchset > modified to include the changes shown below [b]? > > (5) Keven Hilman and Tomasz had reservations about all the device clocks > being listed in the power-domains itself in the previous versions. I don't see > a comment from them yet changing that view. Correct. > Their wish was to get the clocks by reading the clocks from the device nodes, > though I see a problem on how to handle devices that do not have any bindings > at all yet. > > Kevin, Tomasz any new thoughts? I don't see any issues with devices that don't have bindings, as all that would be needed would be to simple device nodes with a clock property. I wouldn't even matter if those devices had device drivers. Kevin
Hi Kevin, Heiko Thanks for your comments. Sorry for delay reply. ? 2015?04?28? 02:28, Kevin Hilman ??: > Heiko Stübner <heiko@sntech.de> writes: > >> Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang: >>> Add power domain drivers based on generic power domain for >>> Rockchip platform, and support RK3288. >>> >>> Verified on url = >>> https://chromium.googlesource.com/chromiumos/third_party/kernel. >>> >>> At the moment,there are mass of products are using the driver. >>> I believe the driver can happy work for next kernel. >> I've taken a look at the driver and here are some global remarks: >> >> (1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks >> bisectability, as the driver itself in patch 2 also includes the header and >> would thus fail to compile if the later patch 3 is missing. >> Ideally I think the header addition should be a separate patch itself, so that >> we can possibly share it between driver and dts branches. >> So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes. OK, done. >> >> (2) The dts-changes in patch 3 should also add any necessary power-domain >> assignment on devices if they're still missing, so that we don't introduce >> regressions. In my case my work-in-progress edp died because the powerdomain >> was turned off automatically it seems. OK, I will list that devices. At the moment, I don't find the EDP driver for rockchip. (I think the EDP driver hasn't a upstream). Anyway, I will test it on https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14, Meanwhile work on next-kernel. >> >> (3) more like wondering @Kevin or so, is there some more generic place for a >> power-domain driver nowadays? > I think the preference has been to put these under drivers/soc/<vendor> for now, > so they can shared across arm32 and arm64. > Interesting. Do you want to put the domain driver into /driver/soc/rockchip? I guess the efuse driver ...is also do that. Perhaps, it's a good select in the future. >> (4) As Tomasz remarked previously the dts should represent the hardware and >> the power-domains are part of the pmu. There is a recent addition from Linus >> Walleij, called simple-mfd [a] that is supposed to get added real early for >> kernel 4.2. So I'd think the power-domains should use that and the patchset >> modified to include the changes shown below [b]? >> >> (5) Keven Hilman and Tomasz had reservations about all the device clocks >> being listed in the power-domains itself in the previous versions. I don't see >> a comment from them yet changing that view. > Correct. How about this patch? https://patchwork.kernel.org/patch/5145241/ I will do that. Maybe, do you have more suggestions? >> Their wish was to get the clocks by reading the clocks from the device nodes, >> though I see a problem on how to handle devices that do not have any bindings >> at all yet. >> >> Kevin, Tomasz any new thoughts? > I don't see any issues with devices that don't have bindings, as all > that would be needed would be to simple device nodes with a clock > property. I wouldn't even matter if those devices had device drivers. > > Kevin > > >
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi index 9696f51..b9ba79013 100644 --- a/arch/arm/boot/dts/rk3288.dtsi +++ b/arch/arm/boot/dts/rk3288.dtsi @@ -549,8 +549,66 @@ }; pmu: power-management@ff730000 { - compatible = "rockchip,rk3288-pmu", "syscon"; + compatible = "rockchip,rk3288-pmu", "syscon", "simple-mfd"; reg = <0xff730000 0x100>; + + power: power-controller { + compatible = "rockchip,rk3288-power-controller"; + #power-domain-cells = <1>; + rockchip,pmu = <&pmu>; + #address-cells = <1>; + #size-cells = <0>; + + pd_gpu { + reg = <RK3288_PD_GPU>; + clocks = <&cru ACLK_GPU>; + }; + + pd_hevc { + reg = <RK3288_PD_HEVC>; + clocks = <&cru ACLK_HEVC>, + <&cru SCLK_HEVC_CABAC>, + <&cru SCLK_HEVC_CORE>, + <&cru HCLK_HEVC>; + }; + + pd_vio { + reg = <RK3288_PD_VIO>; + clocks = <&cru ACLK_IEP>, + <&cru ACLK_ISP>, + <&cru ACLK_RGA>, + <&cru ACLK_VIP>, + <&cru ACLK_VOP0>, + <&cru ACLK_VOP1>, + <&cru DCLK_VOP0>, + <&cru DCLK_VOP1>, + <&cru HCLK_IEP>, + <&cru HCLK_ISP>, + <&cru HCLK_RGA>, + <&cru HCLK_VIP>, + <&cru HCLK_VOP0>, + <&cru HCLK_VOP1>, + <&cru PCLK_EDP_CTRL>, + <&cru PCLK_HDMI_CTRL>, + <&cru PCLK_LVDS_PHY>, + <&cru PCLK_MIPI_CSI>, + <&cru PCLK_MIPI_DSI0>, + <&cru PCLK_MIPI_DSI1>, + <&cru SCLK_EDP_24M>, + <&cru SCLK_EDP>, + <&cru SCLK_HDMI_CEC>, + <&cru SCLK_HDMI_HDCP>, + <&cru SCLK_ISP_JPE>, + <&cru SCLK_ISP>, + <&cru SCLK_RGA>; + }; + + pd_video { + reg = <RK3288_PD_VIDEO>; + clocks = <&cru ACLK_VCODEC>, + <&cru HCLK_VCODEC>; + }; + }; }; sgrf: syscon@ff740000 { @@ -1316,62 +1374,4 @@ }; }; }; - - power: power-controller { - compatible = "rockchip,rk3288-power-controller"; - #power-domain-cells = <1>; - rockchip,pmu = <&pmu>; - #address-cells = <1>; - #size-cells = <0>; - - pd_gpu { - reg = <RK3288_PD_GPU>; - clocks = <&cru ACLK_GPU>; - }; - - pd_hevc { - reg = <RK3288_PD_HEVC>; - clocks = <&cru ACLK_HEVC>, - <&cru SCLK_HEVC_CABAC>, - <&cru SCLK_HEVC_CORE>, - <&cru HCLK_HEVC>; - }; - - pd_vio { - reg = <RK3288_PD_VIO>; - clocks = <&cru ACLK_IEP>, - <&cru ACLK_ISP>, - <&cru ACLK_RGA>, - <&cru ACLK_VIP>, - <&cru ACLK_VOP0>, - <&cru ACLK_VOP1>, - <&cru DCLK_VOP0>, - <&cru DCLK_VOP1>, - <&cru HCLK_IEP>, - <&cru HCLK_ISP>, - <&cru HCLK_RGA>, - <&cru HCLK_VIP>, - <&cru HCLK_VOP0>, - <&cru HCLK_VOP1>, - <&cru PCLK_EDP_CTRL>, - <&cru PCLK_HDMI_CTRL>, - <&cru PCLK_LVDS_PHY>, - <&cru PCLK_MIPI_CSI>, - <&cru PCLK_MIPI_DSI0>, - <&cru PCLK_MIPI_DSI1>, - <&cru SCLK_EDP_24M>, - <&cru SCLK_EDP>, - <&cru SCLK_HDMI_CEC>, - <&cru SCLK_HDMI_HDCP>, - <&cru SCLK_ISP_JPE>, - <&cru SCLK_ISP>, - <&cru SCLK_RGA>; - }; - - pd_video { - reg = <RK3288_PD_VIDEO>; - clocks = <&cru ACLK_VCODEC>, - <&cru HCLK_VCODEC>; - }; - }; }; diff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c index 92d2569..f3d87c21 100644 --- a/arch/arm/mach-rockchip/pm_domains.c +++ b/arch/arm/mach-rockchip/pm_domains.c @@ -377,6 +377,7 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct device_node *node; + struct device *parent; struct rockchip_pmu *pmu; const struct of_device_id *match; const struct rockchip_pmu_info *pmu_info; @@ -410,9 +411,13 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev) pmu->genpd_data.domains = pmu->domains; pmu->genpd_data.num_domains = pmu_info->num_domains; - node = of_parse_phandle(np, "rockchip,pmu", 0); - pmu->regmap = syscon_node_to_regmap(node); - of_node_put(node); + parent = dev->parent; + if (!parent) { + dev_err(dev, "no parent for syscon LED\n"); + return -ENODEV; + } + + pmu->regmap = syscon_node_to_regmap(parent->of_node); if (IS_ERR(pmu->regmap)) { error = PTR_ERR(pmu->regmap); dev_err(dev, "failed to get PMU regmap: %d\n", error);