Re: [PATCH v4 3/4] dt-bindings: Add Qualcomm USB SuperSpeed PHY bindings
On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote: > Binding description for Qualcomm's Synopsys 1.0.0 SuperSpeed phy > controller embedded in QCS404. > > Based on Sriharsha Allenki's original > definitions. > > Signed-off-by: Jorge Ramirez-Ortiz > --- > .../bindings/phy/qcom,snps-usb-ssphy.txt | 79 +++ > 1 file changed, 79 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt > > diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt > b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt > new file mode 100644 > index ..354e6f9cef62 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt > @@ -0,0 +1,79 @@ > +Qualcomm Synopsys 1.0.0 SS phy controller > +=== > + > +Qualcomm 1.0.0 SS phy controller supports SuperSpeed USB connectivity on > +some Qualcomm platforms. > + > +Required properties: > + > +- compatible: > +Value type: > +Definition: Should contain "qcom,snps-usb-ssphy". Per Rob's request make this: Should contain "qcom,qcs404-snps-usb-ssphy" and "qcom,snps-usb-ssphy" You can then leave the driver matching on qcom,snps-usb-ssphy for now and if we ever find this to be incompatible with other platforms we can make the driver match on the platform-specific compatible. > + > +- reg: > +Value type: > +Definition: USB PHY base address and length of the register map. > + > +- #phy-cells: > +Value type: > +Definition: Should be 0. See phy/phy-bindings.txt for details. > + > +- clocks: > +Value type: > +Definition: See clock-bindings.txt section "consumers". List of > + three clock specifiers for reference, phy core and > + pipe clocks. > + > +- clock-names: > +Value type: > +Definition: Names of the clocks in 1-1 correspondence with the "clocks" > + property. Must contain "ref", "phy" and "pipe". > + > +- vdd-supply: > +Value type: > +Definition: phandle to the regulator VDD supply node. > + > +- vdda1p8-supply: > +Value type: > +Definition: phandle to the regulator 1.8V supply node. > + > +Optional properties: > + > +- resets: > +Value type: > +Definition: See reset.txt section "consumers". Specifiers for COM and > + PHY resets. > + > +- reset-names: > +Value type: > +Definition: Names of the resets in 1-1 correspondence with the "resets" > + property. Must contain "com" and "phy" if the property is > + specified. > + > +Required child nodes: > + > +- usb connector node as defined in bindings/connector/usb-connector.txt > + containing the property vbus-supply. > + > +Example: > + > +usb3_phy: usb3-phy@78000 { > + compatible = "qcom,snps-usb-ssphy"; > + reg = <0x78000 0x400>; > + #phy-cells = <0>; > + clocks = <&rpmcc RPM_SMD_LN_BB_CLK>, > + <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>, > + <&gcc GCC_USB3_PHY_PIPE_CLK>; > + clock-names = "ref", "phy", "pipe"; > + resets = <&gcc GCC_USB3_PHY_BCR>, > + <&gcc GCC_USB3PHY_PHY_BCR>; > + reset-names = "com", "phy"; > + vdd-supply = <&vreg_l3_1p05>; > + vdda1p8-supply = <&vreg_l5_1p8>; > + usb3_c_connector: usb3-c-connector { The USB-C connector is attached both to the HS and SS PHYs, so I think you should represent this external to this node and use of_graph to query it. So the connector should look similar to example 2 in connector/usb-connector.txt. Regards, Bjorn > + compatible = "usb-c-connector"; > + label = "USB-C"; > + type = "micro"; > + vbus-supply = <&usb3_vbus_reg>; > + }; > +}; > -- > 2.20.1 >
Re: [PATCH v2 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings
On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote: > Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY SuperSpeed > controller embedded in QCS404. > > Based on Sriharsha Allenki's original > definitions. > > Signed-off-by: Jorge Ramirez-Ortiz > --- > .../devicetree/bindings/usb/qcom,usb-ssphy.txt | 73 > ++ > 1 file changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > > diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > new file mode 100644 > index 000..8ef6e39 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > @@ -0,0 +1,73 @@ > +Qualcomm Synopsys 1.0.0 SS phy controller > +=== > + > +Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm > +chipsets It's based on Synopsys IP, but it's Qualcomm's version, and isn't the 1.0.0 Qualcomm's version number for this block? Also I think it "provides SuperSpeed USB connectivity on some Qualcomm platforms". > + > +Required properties: > + > +- compatible: > +Value type: > +Definition: Should contain "qcom,usb-ssphy". > + > +- reg: > +Value type: > +Definition: USB PHY base address and length of the register map. > + > +- #phy-cells: > +Value type: > +Definition: Should be 0. See phy/phy-bindings.txt for details. > + > +- clocks: > +Value type: > +Definition: See clock-bindings.txt section "consumers". List of > + three clock specifiers for reference, phy core and > + pipe clocks. > + > +- clock-names: > +Value type: > +Definition: Names of the clocks in 1-1 correspondence with the "clocks" > + property. Must contain "ref", "phy" and "pipe". > + > +- vdd-supply: > +Value type: > +Definition: phandle to the regulator VDD supply node. > + > +- vdda1p8-supply: > +Value type: > +Definition: phandle to the regulator 1.8V supply node. > + > + > +Optional child nodes: > + > +- vbus-supply: > +Value type: > +Definition: phandle to the VBUS supply node. > + > +- resets: > +Value type: > +Definition: See reset.txt section "consumers". PHY reset specifiers > + for phy core and COR resets. > + > +- reset-names: > +Value type: > +Definition: Names of the resets in 1-1 correspondence with the "resets" > + property. Must contain "com" and "phy". Perhaps "Must contain both com and phy, if property is specified", to clarify that it's all or nothing. Looks good otherwise. Regards, Bjorn
Re: [PATCH v2 2/2] phy: qualcomm: usb: Add Super-Speed PHY driver
On Tue 29 Jan 03:35 PST 2019, Jorge Ramirez-Ortiz wrote: > diff --git a/drivers/phy/qualcomm/phy-qcom-usb-ss.c > b/drivers/phy/qualcomm/phy-qcom-usb-ss.c > new file mode 100644 > index 000..e6ae96e > --- /dev/null > +++ b/drivers/phy/qualcomm/phy-qcom-usb-ss.c > @@ -0,0 +1,347 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2012-2014,2017 The Linux Foundation. All rights reserved. > + * Copyright (c) 2018, Linaro Limited > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PHY_CTRL00x6C > +#define PHY_CTRL10x70 > +#define PHY_CTRL20x74 > +#define PHY_CTRL40x7C > + > +/* PHY_CTRL bits */ > +#define REF_PHY_EN BIT(0) > +#define LANE0_PWR_ON BIT(2) > +#define SWI_PCS_CLK_SEL BIT(4) > +#define TST_PWR_DOWN BIT(4) > +#define PHY_RESETBIT(7) > + > +enum phy_vdd_ctrl { ENABLE, DISABLE, }; Use bool to describe boolean values. > +enum phy_regulator { VDD, VDDA1P8, }; It doesn't look like you need either of these if you remove the set_voltage calls. > + > +struct ssphy_priv { > + void __iomem *base; > + struct device *dev; > + struct reset_control *reset_com; > + struct reset_control *reset_phy; > + struct regulator *vbus; > + struct regulator_bulk_data *regs; > + int num_regs; > + struct clk_bulk_data *clks; > + int num_clks; > + enum phy_mode mode; > +}; > + > +static inline void qcom_ssphy_updatel(void __iomem *addr, u32 mask, u32 val) > +{ > + writel((readl(addr) & ~mask) | val, addr); > +} > + > +static inline int qcom_ssphy_vbus_enable(struct regulator *vbus) > +{ > + return !regulator_is_enabled(vbus) ? regulator_enable(vbus) : 0; regulator_is_enabled() will check if the actual regulator is on, not if you already voted for it. So if something else is enabling the vbus regulator you will skip your enable and be in the mercy of them not releasing it. Presumably there's only one consumer of this particular regulator, but it's a bad habit. Please keep track of this drivers requested state in this driver, if qcom_ssphy_vbus_ctrl() is called not only for the state changes. > +} > + > +static inline int qcom_ssphy_vbus_disable(struct regulator *vbus) > +{ > + return regulator_is_enabled(vbus) ? regulator_disable(vbus) : 0; > +} > + > +static int qcom_ssphy_vdd_ctrl(struct ssphy_priv *priv, enum phy_vdd_ctrl > ctrl) As discussed on IRC, I think you should just leave the voltage constraints to DeviceTree. > +{ > + const int vdd_min = ctrl == ENABLE ? 105 : 0; > + const int vdd_max = 105; > + int ret; > + > + ret = regulator_set_voltage(priv->regs[VDD].consumer, vdd_min, vdd_max); > + if (ret) > + dev_err(priv->dev, "Failed to set regulator vdd to %d\n", > + vdd_min); > + > + return ret; > +} [..] > +static int qcom_ssphy_power_on(struct phy *phy) > +{ > + struct ssphy_priv *priv = phy_get_drvdata(phy); > + int ret; > + > + ret = qcom_ssphy_vdd_ctrl(priv, ENABLE); > + if (ret) > + return ret; > + > + ret = regulator_bulk_enable(priv->num_regs, priv->regs); > + if (ret) > + goto err1; > + > + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); > + if (ret) > + goto err2; > + > + ret = qcom_ssphy_vbus_ctrl(priv->vbus, priv->mode); > + if (ret) > + goto err3; > + > + ret = qcom_ssphy_do_reset(priv); > + if (ret) > + goto err4; > + > + writeb(SWI_PCS_CLK_SEL, priv->base + PHY_CTRL0); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE0_PWR_ON, LANE0_PWR_ON); > + qcom_ssphy_updatel(priv->base + PHY_CTRL2, REF_PHY_EN, REF_PHY_EN); > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, TST_PWR_DOWN, 0); > + > + return 0; > +err4: Name your labels based on what they do, e.g. err_disable_vbus. > + if (priv->vbus && priv->mode != PHY_MODE_INVALID) > + qcom_ssphy_vbus_disable(priv->vbus); qcom_ssphy_vbus_ctrl() above was either enabling or disabling vbus, but here you're directly calling disable to unroll that. It think the result is correct (in host this reverts to disabled and in gadget it's a no-op), but I'm not sure I like the design of sometimes calling straight to the vbus enable/disable and sometimes to the wrapper function. > +err3: err_clk_disable > + clk_bulk_disable_unprepare(priv->num_clks, priv->clks); > +err2: > + regulator_bulk_disable(priv->num_regs, priv->regs); > +err1: > + qcom_ssphy_vdd_ctrl(priv, DISABLE); > + > + return ret; > +} > + > +static int qcom_ssphy_power_off(struct phy *phy) > +{ > + struct ssphy_priv *priv = phy_get_drvdata(phy); > + > + qcom_ssphy_updatel(priv->base + PHY_CTRL4, LANE
Re: [PATCH v2 6/6] arm64: dts: qcom: msm8998: Add USB-related nodes
On Mon 14 Jan 08:37 PST 2019, Jeffrey Hugo wrote: > Add nodes for USB and related PHYs. > > Signed-off-by: Jeffrey Hugo Reviewed-by: Bjorn Andersson Regards, Bjorn > --- > arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi | 22 > arch/arm64/boot/dts/qcom/msm8998.dtsi | 92 > +++ > 2 files changed, 114 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi > b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi > index 50e9033..dc703fc 100644 > --- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi > @@ -257,3 +257,25 @@ > pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>; > pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>; > }; > + > +&usb1 { > + status = "okay"; > +}; > + > +&usb1_dwc3 { > + dr_mode = "host"; /* Force to host until we have Type-C hooked up */ > +}; > + > +&usb1_hsphy { > + status = "okay"; > + > + vdda-pll-supply = <&vreg_l12a_1p8>; > + vdda-phy-dpdm-supply = <&vreg_l24a_3p075>; > +}; > + > +&usb1_qmpphy { > + status = "okay"; > + > + vdda-phy-supply = <&vreg_l1a_0p875>; > + vdda-pll-supply = <&vreg_l2a_1p2>; > +}; > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi > b/arch/arm64/boot/dts/qcom/msm8998.dtsi > index 8d41b69..706b303 100644 > --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi > @@ -540,6 +540,11 @@ > reg = <0x78 0x621c>; > #address-cells = <1>; > #size-cells = <1>; > + > + qusb2_hstx_trim: hstx-trim@423a { > + reg = <0x423a 0x1>; > + bits = <0 4>; > + }; > }; > > gcc: clock-controller@10 { > @@ -607,6 +612,93 @@ > #mbox-cells = <1>; > }; > > + usb1: usb@a8f8800 { > + compatible = "qcom,msm8998-dwc3", "qcom,dwc3"; > + reg = <0xa8f8800 0x400>; > + status = "disabled"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clocks = <&gcc GCC_CFG_NOC_USB3_AXI_CLK>, > + <&gcc GCC_USB30_MASTER_CLK>, > + <&gcc GCC_AGGRE1_USB3_AXI_CLK>, > + <&gcc GCC_USB30_MOCK_UTMI_CLK>, > + <&gcc GCC_USB30_SLEEP_CLK>; > + clock-names = "cfg_noc", "core", "iface", "mock_utmi", > + "sleep"; > + > + assigned-clocks = <&gcc GCC_USB30_MOCK_UTMI_CLK>, > + <&gcc GCC_USB30_MASTER_CLK>; > + assigned-clock-rates = <1920>, <12000>; > + > + interrupts = , > + ; > + interrupt-names = "hs_phy_irq", "ss_phy_irq"; > + > + power-domains = <&gcc USB_30_GDSC>; > + > + resets = <&gcc GCC_USB_30_BCR>; > + > + usb1_dwc3: dwc3@a80 { > + compatible = "snps,dwc3"; > + reg = <0xa80 0xcd00>; > + interrupts = ; > + snps,dis_u2_susphy_quirk; > + snps,dis_enblslpm_quirk; > + phys = <&usb1_hsphy>, <&usb1_ssphy>; > + phy-names = "usb2-phy", "usb3-phy"; > + snps,has-lpm-erratum; > + snps,hird-threshold = /bits/ 8 <0x10>; > + }; > + }; > + > + usb1_qmpphy: phy@c01 { > + compatible = "qcom,msm8998-qmp-usb3-phy"; > + reg = <0xc01 0x18c>; > + status = "disabled"; > + #clock-cells = <1>; > +
Re: [PATCH v2 5/6] usb: dwc3: qcom: Add support for MSM8998
On Mon 14 Jan 08:37 PST 2019, Jeffrey Hugo wrote: > Add a MSM8998 specific DT compatible so that we can properly bind to the > device and enable the USB controller. > Reviewed-by: Bjorn Andersson > Signed-off-by: Jeffrey Hugo > --- > drivers/usb/dwc3/dwc3-qcom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index a6d0203..184df4d 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -595,6 +595,7 @@ static int __maybe_unused dwc3_qcom_runtime_resume(struct > device *dev) > static const struct of_device_id dwc3_qcom_of_match[] = { > { .compatible = "qcom,dwc3" }, > { .compatible = "qcom,msm8996-dwc3" }, > + { .compatible = "qcom,msm8998-dwc3" }, > { .compatible = "qcom,sdm845-dwc3" }, > { } > }; > -- > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, > Inc. > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. >
Re: [PATCH v2 4/6] phy: qcom-qmp: Add QMP V3 USB3 PHY support for msm8998
On Mon 14 Jan 08:36 PST 2019, Jeffrey Hugo wrote: > MSM8998 contains a single QMP v3 USB3 phy similar to the existing sdm845 > support. > > Signed-off-by: Jeffrey Hugo Reviewed-by: Bjorn Andersson Regards, Bjorn
Re: [PATCH v2 3/6] phy: qcom-qusb2: Add QUSB2 PHY support for msm8998
On Mon 14 Jan 08:36 PST 2019, Jeffrey Hugo wrote: > MSM8998 contains one QUSB2 PHY which is very similar to the existing > sdm845 support. > Reviewed-by: Bjorn Andersson > Signed-off-by: Jeffrey Hugo > --- > drivers/phy/qualcomm/phy-qcom-qusb2.c | 41 > +++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c > b/drivers/phy/qualcomm/phy-qcom-qusb2.c > index 9177989f..e5e4f36 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c > +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c > @@ -152,6 +152,32 @@ enum qusb2phy_reg_layout { > QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00), > }; > > +static const unsigned int msm8998_regs_layout[] = { > + [QUSB2PHY_PLL_CORE_INPUT_OVERRIDE] = 0xa8, > + [QUSB2PHY_PLL_STATUS] = 0x1a0, > + [QUSB2PHY_PORT_TUNE1] = 0x23c, > + [QUSB2PHY_PORT_TUNE2] = 0x240, > + [QUSB2PHY_PORT_TUNE3] = 0x244, > + [QUSB2PHY_PORT_TUNE4] = 0x248, > + [QUSB2PHY_PORT_TEST1] = 0x24c, > + [QUSB2PHY_PORT_TEST2] = 0x250, > + [QUSB2PHY_PORT_POWERDOWN] = 0x210, > + [QUSB2PHY_INTR_CTRL] = 0x22c, > +}; > + > +static const struct qusb2_phy_init_tbl msm8998_init_tbl[] = { > + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_ANALOG_CONTROLS_TWO, 0x13), > + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_CLOCK_INVERTERS, 0x7c), > + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_CMODE, 0x80), > + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_LOCK_DELAY, 0x0a), > + > + QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE1, 0xa5), > + QUSB2_PHY_INIT_CFG_L(QUSB2PHY_PORT_TUNE2, 0x09), > + > + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_DIGITAL_TIMERS_TWO, 0x19), > +}; > + > + > static const unsigned int sdm845_regs_layout[] = { > [QUSB2PHY_PLL_CORE_INPUT_OVERRIDE] = 0xa8, > [QUSB2PHY_PLL_STATUS] = 0x1a0, > @@ -221,6 +247,18 @@ struct qusb2_phy_cfg { > .autoresume_en = BIT(3), > }; > > +static const struct qusb2_phy_cfg msm8998_phy_cfg = { > + .tbl= msm8998_init_tbl, > + .tbl_num= ARRAY_SIZE(msm8998_init_tbl), > + .regs = msm8998_regs_layout, > + > + .disable_ctrl = POWER_DOWN, > + .mask_core_ready = CORE_READY_STATUS, > + .has_pll_override = true, > + .autoresume_en = BIT(0), > + .update_tune1_with_efuse = true, > +}; > + > static const struct qusb2_phy_cfg sdm845_phy_cfg = { > .tbl= sdm845_init_tbl, > .tbl_num= ARRAY_SIZE(sdm845_init_tbl), > @@ -734,6 +772,9 @@ static int qusb2_phy_exit(struct phy *phy) > .compatible = "qcom,msm8996-qusb2-phy", > .data = &msm8996_phy_cfg, > }, { > + .compatible = "qcom,msm8998-qusb2-phy", > + .data = &msm8998_phy_cfg, > + }, { > .compatible = "qcom,sdm845-qusb2-phy", > .data = &sdm845_phy_cfg, > }, > -- > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, > Inc. > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. >
Re: [PATCH v2 2/6] dt-bindings: usb: Add support for msm8998
On Mon 14 Jan 08:36 PST 2019, Jeffrey Hugo wrote: > msm8998 USB has a dwc3 controller just like the existing sdm845 support > Reviewed-by: Bjorn Andersson > Signed-off-by: Jeffrey Hugo > --- > Documentation/devicetree/bindings/usb/qcom,dwc3.txt | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > index 95afdcf..cb695aa 100644 > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt > @@ -4,6 +4,7 @@ Required properties: > - compatible:Compatible list, contains > "qcom,dwc3" > "qcom,msm8996-dwc3" for msm8996 SOC. > + "qcom,msm8998-dwc3" for msm8998 SOC. > "qcom,sdm845-dwc3" for sdm845 SOC. > - reg: Offset and length of register set for QSCRATCH > wrapper > - power-domains: specifies a phandle to PM domain provider node > -- > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, > Inc. > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. >
Re: [PATCH v2 1/6] dt-bindings: phy-qcom: Add support for msm8998 usb
On Mon 14 Jan 08:36 PST 2019, Jeffrey Hugo wrote: > USB on msm8998 utilizes the QUSB2 and QMP phys, similar to sdm845. > > Signed-off-by: Jeffrey Hugo > --- > Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 5 + > Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt > b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt > index 41a1074..6061b6f 100644 > --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt > +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt > @@ -9,6 +9,7 @@ Required properties: > "qcom,ipq8074-qmp-pcie-phy" for PCIe phy on IPQ8074 > "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996, > "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996, > +"qcom,msm8998-qmp-usb3-phy" for USB3 QMP V3 phy on msm8998, > "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845, > "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845, > "qcom,sdm845-qmp-ufs-phy" for UFS QMP phy on sdm845. > @@ -42,6 +43,8 @@ Required properties: > "aux", "cfg_ahb", "ref". > For "qcom,msm8996-qmp-usb3-phy" must contain: > "aux", "cfg_ahb", "ref". > + For "qcom,msm8998-qmp-usb3-phy" must contain: > + "aux", "cfg_ahb", "ref". > For "qcom,sdm845-qmp-usb3-phy" must contain: > "aux", "cfg_ahb", "ref", "com_aux". > For "qcom,sdm845-qmp-usb3-uni-phy" must contain: > @@ -60,6 +63,8 @@ Required properties: > For "qcom,msm8996-qmp-pcie-phy" must contain: > "phy", "common", "cfg". > For "qcom,msm8996-qmp-usb3-phy" must contain > + "phy", "common". Odd indentation. Apart from this, you have my Reviewed-by: Bjorn Andersson Regards, Bjorn > + For "qcom,msm8998-qmp-usb3-phy" must contain > "phy", "common". > For "qcom,sdm845-qmp-usb3-phy" must contain: > "phy", "common". > diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > index 03025d9..fe29f9e 100644 > --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > @@ -6,6 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on > Qualcomm chipsets. > Required properties: > - compatible: compatible list, contains > "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996, > +"qcom,msm8998-qusb2-phy" for 10nm PHY on msm8998, > "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845. > > - reg: offset and length of the PHY register set. > -- > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, > Inc. > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. >
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Wed 12 Sep 08:08 PDT 2018, Arnd Bergmann wrote: [..] > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index a76b963a7e50..02aefb2b2d47 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -285,7 +285,7 @@ static const struct file_operations rpmsg_eptdev_fops = { > .write = rpmsg_eptdev_write, > .poll = rpmsg_eptdev_poll, > .unlocked_ioctl = rpmsg_eptdev_ioctl, > - .compat_ioctl = rpmsg_eptdev_ioctl, > + .compat_ioctl = generic_compat_ioctl_ptrarg, > }; > > static ssize_t name_show(struct device *dev, struct device_attribute *attr, > @@ -446,7 +446,7 @@ static const struct file_operations rpmsg_ctrldev_fops = { > .open = rpmsg_ctrldev_open, > .release = rpmsg_ctrldev_release, > .unlocked_ioctl = rpmsg_ctrldev_ioctl, > - .compat_ioctl = rpmsg_ctrldev_ioctl, > + .compat_ioctl = generic_compat_ioctl_ptrarg, > }; > For rpmsg part Acked-by: Bjorn Andersson Regards, Bjorn
Re: [PATCH 5/6] phy: qcom-usb-hs: Fix unbalanced notifier registration
On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote: > Phy power on/off cycle can happen several times during device life. > We then need to balance the extcon notifier registration accordingly. > > Fixes: f0b5c2c96370 ("phy: qcom-usb-hs: Replace the extcon API") > Signed-off-by: Loic Poulain > --- > drivers/phy/qualcomm/phy-qcom-usb-hs.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c > b/drivers/phy/qualcomm/phy-qcom-usb-hs.c > index 2d0c70b..92e9d94 100644 > --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c > +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c > @@ -181,6 +181,12 @@ static int qcom_usb_hs_phy_power_off(struct phy *phy) > { > struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy); > > + if (uphy->vbus_edev) { > + devm_extcon_unregister_notifier(&uphy->ulpi->dev, > + uphy->vbus_edev, EXTCON_USB, > + &uphy->vbus_notify); As I presume the power_on should always be matched with a power_off I wonder if it really is appropriate to use the devres version of this API. Should we drop the devm_ from registration in power_on as well? > + } > + > regulator_disable(uphy->v3p3); > regulator_disable(uphy->v1p8); > clk_disable_unprepare(uphy->sleep_clk); Regards, Bjorn
Re: [PATCH 1/6] usb: chipidea: Add dynamic pinctrl selection
On Tue 21 Aug 06:55 PDT 2018, Loic Poulain wrote: > Some hardware implementations require to configure pins differently > according to the USB role (host/device), this can be an update of the > pins routing or a simple GPIO value change. > > This patch introduces new optional "host" and "device" pinctrls. > If these pinctrls are defined by the device, they are respectively > selected on host/device role start. > > If a default pinctrl exist, it is restored on host/device role stop. > The implementation looks reasonable, but we're actually just toggling a gpio using pinctrl states. Do you see any reason not to control this mux through the gpio api? Regards, Bjorn > Signed-off-by: Loic Poulain > --- > drivers/usb/chipidea/core.c | 19 +++ > drivers/usb/chipidea/host.c | 9 + > drivers/usb/chipidea/udc.c | 9 + > include/linux/usb/chipidea.h | 6 ++ > 4 files changed, 43 insertions(+) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 85fc6db..03e52fc 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -46,6 +46,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -723,6 +724,24 @@ static int ci_get_platdata(struct device *dev, > else > cable->connected = false; > } > + > + platdata->pctl = devm_pinctrl_get(dev); > + if (!IS_ERR(platdata->pctl)) { > + struct pinctrl_state *p; > + > + p = pinctrl_lookup_state(platdata->pctl, "default"); > + if (!IS_ERR(p)) > + platdata->pins_default = p; > + > + p = pinctrl_lookup_state(platdata->pctl, "host"); > + if (!IS_ERR(p)) > + platdata->pins_host = p; > + > + p = pinctrl_lookup_state(platdata->pctl, "device"); > + if (!IS_ERR(p)) > + platdata->pins_device = p; > + } > + > return 0; > } > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > index af45aa32..55dbd49 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > > #include "../host/ehci.h" > > @@ -150,6 +151,10 @@ static int host_start(struct ci_hdrc *ci) > } > } > > + if (ci->platdata->pins_host) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_host); > + > ret = usb_add_hcd(hcd, 0, 0); > if (ret) { > goto disable_reg; > @@ -194,6 +199,10 @@ static void host_stop(struct ci_hdrc *ci) > } > ci->hcd = NULL; > ci->otg.host = NULL; > + > + if (ci->platdata->pins_host && ci->platdata->pins_default) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_default); > } > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 9852ec5..c04384e 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include "ci.h" > #include "udc.h" > @@ -1965,6 +1966,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci) > > static int udc_id_switch_for_device(struct ci_hdrc *ci) > { > + if (ci->platdata->pins_device) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_device); > + > if (ci->is_otg) > /* Clear and enable BSV irq */ > hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE, > @@ -1983,6 +1988,10 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci) > hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS, OTGSC_BSVIS); > > ci->vbus_active = 0; > + > + if (ci->platdata->pins_device && ci->platdata->pins_default) > + pinctrl_select_state(ci->platdata->pctl, > + ci->platdata->pins_default); > } > > /** > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > index 07f9936..63758c3 100644 > --- a/include/linux/usb/chipidea.h > +++ b/include/linux/usb/chipidea.h > @@ -77,6 +77,12 @@ struct ci_hdrc_platform_data { > struct ci_hdrc_cablevbus_extcon; > struct ci_hdrc_cableid_extcon; > u32 phy_clkgate_delay_us; > + > + /* pins */ > + struct pinctrl *pctl; > + struct pinctrl_state *pins_default; > + struct pinctrl_state *pins_host; > + struct pinctrl_state *pins_device; > }; > > /* Default offset of capability registers */ > -- > 2.7.4 >
Re: [PATCH v7 1/4] reset: Add APIs to manage array of resets
On Fri 20 Oct 05:20 PDT 2017, Philipp Zabel wrote: > Hi, > > On Thu, 2017-10-19 at 11:54 -0700, Bjorn Andersson wrote: > > On Wed 19 Jul 08:59 PDT 2017, Philipp Zabel wrote: > > > > > From: Vivek Gautam > > > > > > Many devices may want to request a bunch of resets and control them. So > > > it's better to manage them as an array. Add APIs to _get() an array of > > > reset_control, reusing the _assert(), _deassert(), and _reset() APIs for > > > single reset controls. Since reset controls already may control multiple > > > reset lines with a single hardware bit, from the user perspective, reset > > > control arrays are not at all different from single reset controls. > > > Note that these APIs don't guarantee that the reset lines managed in the > > > array are handled in any particular order. > > > > > > Cc: Felipe Balbi > > > Cc: Jon Hunter > > > Signed-off-by: Vivek Gautam > > > [p.za...@pengutronix.de: changed API to hide reset control arrays behind > > > struct reset_control] > > > Signed-off-by: Philipp Zabel > > > > This looks more or less identical to how regulators and clocks already > > deals with resources in bulk; see regulator_bulk_data and clk_bulk_data > > and their associated functions. > > > > I would really like to see that you follow this model, to make it easier > > for developers to work with and use the various subsystems. > > These APIs have two undesirable (in this case) properties; the driver > has to know the number of resets and their identifiers in advance, and > singular resets and bulk reset arrays can't be used interchangeably. As a writer of device drivers as well as dts files I greatly appreciate when this expectations is encoded in the kernel, so that it is clear when the DT node is missing some resource - rather than having random reboots because of spelling mistakes or variations between hardware revisions. We tend to express these things explicitly in the kernel, as magic interfaces makes things harder to debug. > Both are not well suited to this use case, which is "triggering one or > any number of anonymous resets together". > Triggering one is just a special case of N. But this does not change the fact that the reset framework interface looks and function in a fundamentally different way than the clock and regulator equivalents, which will be confusing - in particular since most drivers will use 2 or 3 of these. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] usb: host: remove ehci-msm.c
On Thu 26 Oct 08:06 PDT 2017, Alan Stern wrote: > On Thu, 26 Oct 2017, Alex Elder wrote: > > > No Qualcomm SoC requires the "ehci-msm.c" code any more. So remove it. > > What about old Qualcomm SoCs? What should they use instead? > These drivers where unfortunately broken by design (host and gadget mode where implemented in separate drivers that where racing) and the ChipIdea driver has been updated to support driving the host-mode on these platforms as well. So all platforms existing upstream should be covered by the remaining drivers. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/4] reset: Add APIs to manage array of resets
On Wed 19 Jul 08:59 PDT 2017, Philipp Zabel wrote: > From: Vivek Gautam > > Many devices may want to request a bunch of resets and control them. So > it's better to manage them as an array. Add APIs to _get() an array of > reset_control, reusing the _assert(), _deassert(), and _reset() APIs for > single reset controls. Since reset controls already may control multiple > reset lines with a single hardware bit, from the user perspective, reset > control arrays are not at all different from single reset controls. > Note that these APIs don't guarantee that the reset lines managed in the > array are handled in any particular order. > > Cc: Felipe Balbi > Cc: Jon Hunter > Signed-off-by: Vivek Gautam > [p.za...@pengutronix.de: changed API to hide reset control arrays behind > struct reset_control] > Signed-off-by: Philipp Zabel This looks more or less identical to how regulators and clocks already deals with resources in bulk; see regulator_bulk_data and clk_bulk_data and their associated functions. I would really like to see that you follow this model, to make it easier for developers to work with and use the various subsystems. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] remoteproc: make device_type const
On Sat 19 Aug 01:22 PDT 2017, Bhumika Goyal wrote: > Make this const as it is only stored in the type field of a device > structure, which is const. > Done using Coccinelle. > > Signed-off-by: Bhumika Goyal Applied, thanks. Regards, Bjorn > --- > drivers/remoteproc/remoteproc_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c > b/drivers/remoteproc/remoteproc_core.c > index 364ef28..48b2c5d 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1360,7 +1360,7 @@ static void rproc_type_release(struct device *dev) > kfree(rproc); > } > > -static struct device_type rproc_type = { > +static const struct device_type rproc_type = { > .name = "remoteproc", > .release= rproc_type_release, > }; > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] usb: chipidea: Configure DMA properties and ops from DT
On Fri 21 Oct 10:38 PDT 2016, Stephen Boyd wrote: > On 10/21, Bjorn Andersson wrote: > > hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent > > memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated > > manually > > it will not have any dma_mem or dma_ops assigned, which makes the > > dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves > > this by assigning the dma_mem and dma_ops based on the parent's DeviceTree > > node. > > > > Cc: Stephen Boyd > > Signed-off-by: Bjorn Andersson > > --- > > > > Hi Peter, > > > > After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm > > systems I realized that we never concluded on this patch. Unfortunately I > > can't > > find it in my mailbox either, so resending it to restart the discussion. > > > > I thought we were going to go down the route that Arnd has been > pushing[1]? That should work, but I haven't tried it yet and > there are some more fixes on top from Sriram. I think Sriram is > taking over the patch now? > > [1] https://patchwork.kernel.org/patch/9319527/ Thanks for the pointer, I've heard about it but couldn't find it. It does make me further wonder about the multi-device model of these drivers, but I agree with you that it looks like the patch would solve our issue. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND PATCH] usb: chipidea: Configure DMA properties and ops from DT
hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated manually it will not have any dma_mem or dma_ops assigned, which makes the dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves this by assigning the dma_mem and dma_ops based on the parent's DeviceTree node. Cc: Stephen Boyd Signed-off-by: Bjorn Andersson --- Hi Peter, After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm systems I realized that we never concluded on this patch. Unfortunately I can't find it in my mailbox either, so resending it to restart the discussion. Regards, Bjorn drivers/usb/chipidea/core.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 69426e644d17..6218d83cca25 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -62,6 +62,7 @@ #include #include #include +#include #include #include #include @@ -837,6 +838,9 @@ struct platform_device *ci_hdrc_add_device(struct device *dev, pdev->dev.dma_parms = dev->dma_parms; dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); + if (IS_ENABLED(CONFIG_OF) && dev->of_node) + of_dma_configure(&pdev->dev, dev->of_node); + ret = platform_device_add_resources(pdev, res, nres); if (ret) goto err; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/21] usb: chipidea: msm: Mux over secondary phy at the right time
On Sun 26 Jun 00:28 PDT 2016, Stephen Boyd wrote: > We need to pick the correct phy at runtime based on how the SoC > has been wired onto the board. If the secondary phy is used, take > it out of reset and mux over to it by writing into the TCSR > register. Make sure to do this on reset too, because this > register is reset to the default value (primary phy) after the > RESET bit is set in USBCMD. > > Cc: Peter Chen > Cc: Greg Kroah-Hartman > Signed-off-by: Stephen Boyd > --- > drivers/usb/chipidea/ci_hdrc_msm.c | 78 > +++--- > 1 file changed, 73 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c > b/drivers/usb/chipidea/ci_hdrc_msm.c [..] > > +static int ci_hdrc_msm_mux_phy(struct ci_hdrc_msm *ci, > +struct platform_device *pdev) > +{ > + struct regmap *regmap; > + struct device_node *syscon; > + struct device *dev = &pdev->dev; > + u32 off, val; > + int ret; > + > + syscon = of_parse_phandle(dev->of_node, "phy-select", 0); > + if (!syscon) > + return 0; > + > + regmap = syscon_node_to_regmap(syscon); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + ret = of_property_read_u32_index(dev->of_node, "phy-select", 1, &off); > + if (ret < 0) { > + dev_err(dev, "no offset in syscon\n"); > + return -EINVAL; > + } > + > + ret = of_property_read_u32_index(dev->of_node, "phy-select", 2, &val); > + if (ret < 0) { > + dev_err(dev, "no value in syscon\n"); > + return -EINVAL; > + } > + > + ret = regmap_write(regmap, off, val); I recently found out (thanks to a comment from Srinivas) that you can drop the last two error checks by using of_parse_phandle_with_fixed_args() as in: struct of_phandle_args args; ret = of_parse_phandle_with_fixed_args(dev->of_node, "phy-select", 2, 0, &args); if (ret < 0) ... regmap = syscon_node_to_regmap(args.np); of_node_put(args.np); if (IS_ERR(regmap)) ... ret = regmap_write(regmap, args.args[0], args.args[1]); > + if (ret) > + return ret; > + > + ci->secondary_phy = !!val; > + if (ci->secondary_phy) { > + val = readl_relaxed(ci->base + HS_PHY_SEC_CTRL); > + val |= HS_PHY_DIG_CLAMP_N; > + writel_relaxed(val, ci->base + HS_PHY_SEC_CTRL); > + } > + > + return 0; > +} > + > static int ci_hdrc_msm_probe(struct platform_device *pdev) > { > struct ci_hdrc_msm *ci; > struct platform_device *plat_ci; > struct clk *clk; > struct reset_control *reset; > + struct resource *res; > + void __iomem *base; Doesn't look like you need "base". > + resource_size_t size; > int ret; > > dev_dbg(&pdev->dev, "ci_hdrc_msm_probe\n"); > @@ -76,6 +132,15 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev) > if (IS_ERR(clk)) > return PTR_ERR(clk); > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + size = resource_size(res); > + ci->base = base = devm_ioremap(&pdev->dev, res->start, size); > + if (!base) > + return -ENOMEM; Replace these two snippets with: res = platform_get_resource(pdev, IORESOURCE_MEM, 0); ci->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(ci->base)) return PTR_ERR(ci->base); > + > reset_control_assert(reset); > usleep_range(1, 12000); > reset_control_deassert(reset); Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/21] of: device: Support loading a module with OF based modalias
On Sun 26 Jun 00:28 PDT 2016, Stephen Boyd wrote: > In the case of ULPI devices, we want to be able to load the > driver before registering the device so that we don't get stuck > in a loop waiting for the phy module to appear and failing usb > controller probe. Currently we request the ulpi module via the > ulpi ids, but in the DT case we might need to request it with the > OF based modalias instead. Add a common function that allows > anyone to request a module with the OF based modalias. > > Cc: Rob Herring > Cc: > Signed-off-by: Stephen Boyd > --- > drivers/of/device.c | 50 > +++ > include/linux/of_device.h | 6 ++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index fd5cfad7c403..f275e5beb736 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -226,6 +226,56 @@ ssize_t of_device_get_modalias(struct device *dev, char > *str, ssize_t len) > return tsize; > } > > +static ssize_t of_device_modalias_size(struct device *dev) > +{ > + const char *compat; > + int cplen, i; > + ssize_t csize; > + > + if ((!dev) || (!dev->of_node)) > + return -ENODEV; > + > + /* Name & Type */ > + csize = 5 + strlen(dev->of_node->name) + strlen(dev->of_node->type); It would be clearer if you replaced 5 with strlen("of:NT"), but... > + > + /* Get compatible property if any */ > + compat = of_get_property(dev->of_node, "compatible", &cplen); > + if (!compat) > + return csize; > + > + /* Find true end (we tolerate multiple \0 at the end */ > + for (i = (cplen - 1); i >= 0 && !compat[i]; i--) > + cplen--; > + if (!cplen) > + return csize; > + cplen++; > + > + /* Check space (need cplen+1 chars including final \0) */ > + return csize + cplen; > +} ...if I understand of_device_get_modalias() correctly you should be able to replace this function with: size = of_device_get_modalias(dev, NULL, 0); snprintf() will not write to NULL, csize will be larger than 0 so tsize will be returned before it will memcpy() to the buffer. > + > +int of_device_request_module(struct device *dev) > +{ > + char *str; > + ssize_t size; > + int ret; > + > + size = of_device_modalias_size(dev); > + if (size < 0) > + return size; > + > + str = kmalloc(size + 1, GFP_KERNEL); > + if (!str) > + return -ENOMEM; > + > + of_device_get_modalias(dev, str, size); > + str[size] = '\0'; > + ret = request_module(str); > + kfree(str); > + > + return ret; > +} > + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT
On Tue, Mar 8, 2016 at 11:52 AM, Li Yang wrote: > On Wed, Mar 2, 2016 at 4:59 PM, Li Yang wrote: >> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson >> wrote: >>> On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote: >>> >>>> >>>> >>>> On 22/02/16 05:32, Bjorn Andersson wrote: >>>> >On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set >>>> >to be able to do DMA allocations, so use the of_dma_configure() helper >>>> >to populate the dma properties and assign an appropriate dma_ops. [..] >>>> None of the drivers call of_dma_configure() explicitly, which makes me feel >>>> that we are doing something wrong. TBH, this should be handled in more >>>> generic way rather than driver like this having an explicit call to >>>> of_dma_configure(). >>>> >>> >>> I agree, trying to figure out if it should be inherited or something. >> >> I also agree. We need address it in a more generic way. I did a >> search for platform_device_add()/platform_device_register() in the >> kernel source code. I found a lot of them and many could be also >> doing DMA. Looks like it is still too early to assume every device is >> already getting dma_ops set through bus probe. Otherwise, many >> drivers are potentially broken by this assumption. > > Any further comment on this topic? I added the linux-arm mailing list > which was missing from previous discussion. > I had the chance to go through this with Arnd and the verdict is that devices not described in DT should not do DMA (or allocate buffers for doing DMA). So I believe the solution is to fall back on Peter's description; the chipidea driver is the core driver and the Qualcomm code should just be a platform layer. My suggestion is that we turn the chipidea core into a set of APIs that can called by the platform specific pieces. That way we will have the chipidea core be the device described in the DT. I'll try to find some time to prototype this after Connect. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT
On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote: > > > On 22/02/16 05:32, Bjorn Andersson wrote: > >On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set > >to be able to do DMA allocations, so use the of_dma_configure() helper > >to populate the dma properties and assign an appropriate dma_ops. > > > >Signed-off-by: Bjorn Andersson > >--- > > drivers/usb/chipidea/core.c | 4 > > 1 file changed, 4 insertions(+) > > > >diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > >index 7404064b9bbc..047b9d4e67aa 100644 > >--- a/drivers/usb/chipidea/core.c > >+++ b/drivers/usb/chipidea/core.c > >@@ -62,6 +62,7 @@ > > #include > > #include > > #include > >+#include > > #include > > #include > > #include > >@@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct device > >*dev, > > pdev->dev.dma_parms = dev->dma_parms; > > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > > >+if (IS_ENABLED(CONFIG_OF) && dev->of_node) > >+of_dma_configure(&pdev->dev, dev->of_node); > >+ > Would we hit the same issue if we are on non Device tree platforms like ACPI > or platform device style itself? > As far as I can see, yes. > > > ret = platform_device_add_resources(pdev, res, nres); > > if (ret) > > goto err; > > > > I think this is the side effect of commit > 1dccb598df549d892b6450c261da54cdd7af44b4(arm64: simplify dma_get_ops) > I agree, before that we would have hit: __generic_dma_ops() { .. else if (acpi_disabled) return dma_ops; ... } with dma_ops being swiotlb_dma_ops from arm64_dma_init(). But this would not have saved us in the ACPI case, i.e. the result would have been as with my suggested patch. Poking Arnd here to see if he has any input. > None of the drivers call of_dma_configure() explicitly, which makes me feel > that we are doing something wrong. TBH, this should be handled in more > generic way rather than driver like this having an explicit call to > of_dma_configure(). > I agree, trying to figure out if it should be inherited or something. > > On the other hand, I think we could also solve the issue by using correct > device(parent device) while allocating dma/dma-pool. > Unfortunately this solves the allocation part, but in udc-core we try to map and unmap buffers based on the gadget's parent pointer (i.e. the chipidea device). I'm still puzzled to why the chipidea lives as a child device of the msm device; but as this is a rather common structure I believe this still needs to be figured out. @Arnd, the Qualcomm (msm) chipidea driver instantiates a chipidea device, which tries to do DMA mapping operations on ARM64 and fails, because we don't have dma_ops specified on the child. Using of_dma_configure() we can populate the child with appropriate settings and ops, but we would be the first driver doing so. Do you have any pointers to follow on what to do here? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT
On Sun 21 Feb 22:02 PST 2016, Peter Chen wrote: > On Sun, Feb 21, 2016 at 09:32:13PM -0800, Bjorn Andersson wrote: > > On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set > > to be able to do DMA allocations, so use the of_dma_configure() helper > > to populate the dma properties and assign an appropriate dma_ops. > > > > Signed-off-by: Bjorn Andersson > > --- > > drivers/usb/chipidea/core.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index 7404064b9bbc..047b9d4e67aa 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -62,6 +62,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct > > device *dev, > > pdev->dev.dma_parms = dev->dma_parms; > > dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); > > > > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) > > + of_dma_configure(&pdev->dev, dev->of_node); > > + > > ret = platform_device_add_resources(pdev, res, nres); > > if (ret) > > goto err; > > Just would like to confirm, it will not affect the default behavior > which the "dma-ranges" is not set at those platforms? > If I read the code correctly, the only difference if you don't specify dma-ranges, dma-coherent or specify an iommu is that the dma_ops gets assigned. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: chipidea: Configure DMA properties and ops from DT
On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set to be able to do DMA allocations, so use the of_dma_configure() helper to populate the dma properties and assign an appropriate dma_ops. Signed-off-by: Bjorn Andersson --- drivers/usb/chipidea/core.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 7404064b9bbc..047b9d4e67aa 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -62,6 +62,7 @@ #include #include #include +#include #include #include #include @@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct device *dev, pdev->dev.dma_parms = dev->dma_parms; dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask); + if (IS_ENABLED(CONFIG_OF) && dev->of_node) + of_dma_configure(&pdev->dev, dev->of_node); + ret = platform_device_add_resources(pdev, res, nres); if (ret) goto err; -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] usb: phy: msm: fix connect/disconnect bug for dragonboard OTG port
On Fri 20 Nov 15:47 PST 2015, Tim Bird wrote: > Add support for async_irq to wake up driver from low power mode. > Without this, the power management code never calls resume. > Remove a spurious interrupt enable in the driver resume function. > > Signed-off-by: Tim Bird Sorry Tim for missing these things and not jumping into the discussion before. > --- > drivers/usb/phy/phy-msm-usb.c | 17 - > include/linux/usb/msm_hsusb.h | 1 + > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c [..] > @@ -1732,6 +1731,12 @@ static int msm_otg_probe(struct platform_device *pdev) > return motg->irq; > } > > + motg->async_irq = platform_get_irq_byname(pdev, "async"); > + if (motg->async_irq < 0) { > + dev_err(&pdev->dev, "platform_get_irq for async irq failed\n"); This is optional, so I must object to this being dev_err(). Except for development purposes logging this is useless, can't we make it a dev_dbg? > + motg->async_irq = 0; > + } > + [..] > diff --git a/include/linux/usb/msm_hsusb.h b/include/linux/usb/msm_hsusb.h > index 8c8f685..08c67a3 100644 > --- a/include/linux/usb/msm_hsusb.h > +++ b/include/linux/usb/msm_hsusb.h > @@ -164,6 +164,7 @@ struct msm_otg { > struct usb_phy phy; > struct msm_otg_platform_data *pdata; > int irq; > + int async_irq; This should be added to the kerneldoc above. > struct clk *clk; > struct clk *pclk; > struct clk *core_clk; Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Device-mainlining] [PATCH v4 1/5] gadget: Introduce the notifier functions
On Fri, Oct 2, 2015 at 10:23 AM, Felipe Balbi via device-mainlining wrote: [..] > The real difficulty here is coming up with an interface which we're agreeing > to > supporting for the next 30 years. Whatever that is, as soon as it gets exposed > to userland, we will have to support it. > Isn't this the main reason for not creating a user space ABI now? Running with the in-kernel hooks would allow us to get things working now, we can change it as we wish, we can explore the difficulties and we can create an ABI from that - if we need to - that might have a chance to work for the next 30 years. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Rename regulator_set_optimum_mode
On Wed, Feb 11, 2015 at 7:35 PM, Bjorn Andersson wrote: > Changing the name of the regulator_set_optimum_mode() to > regulator_set_load() better reflects that the API is doing. > Any comments on this? I'm going to propose a patch to the mmc framework calling this api, so it would be good to know before I add another consumer of the api. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] Rename regulator_set_optimum_mode
Changing the name of the regulator_set_optimum_mode() to regulator_set_load() better reflects that the API is doing. This series does the name change and move the current consumers over. Bjorn Andersson (5): regulator: s/regulator_set_optimum_mode/regulator_set_load/ ufs: Rename of regulator_set_optimum_mode usb: phy: ab8500-usb: Rename regulator_set_optimum_mode usb: phy: phy-msm-usb: Rename regulator_set_optimum_mode regulator: Drop temporary regulator_set_optimum_mode wrapper Documentation/power/regulator/consumer.txt | 2 +- drivers/regulator/core.c | 8 drivers/scsi/ufs/ufshcd.c | 27 +++ drivers/usb/phy/phy-ab8500-usb.c | 4 ++-- drivers/usb/phy/phy-msm-usb.c | 15 +-- include/linux/regulator/consumer.h | 5 ++--- 6 files changed, 21 insertions(+), 40 deletions(-) -- 1.8.2.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/5] usb: phy: ab8500-usb: Rename regulator_set_optimum_mode
The function regulator_set_optimum_mode() is changing name to regulator_set_load(), so update the code accordingly. Signed-off-by: Bjorn Andersson --- drivers/usb/phy/phy-ab8500-usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/phy/phy-ab8500-usb.c b/drivers/usb/phy/phy-ab8500-usb.c index 0b1bd23..f5b3b92 100644 --- a/drivers/usb/phy/phy-ab8500-usb.c +++ b/drivers/usb/phy/phy-ab8500-usb.c @@ -277,7 +277,7 @@ static void ab8500_usb_regulator_enable(struct ab8500_usb *ab) dev_err(ab->dev, "Failed to set the Vintcore to 1.3V, ret=%d\n", ret); - ret = regulator_set_optimum_mode(ab->v_ulpi, 28000); + ret = regulator_set_load(ab->v_ulpi, 28000); if (ret < 0) dev_err(ab->dev, "Failed to set optimum mode (ret=%d)\n", ret); @@ -317,7 +317,7 @@ static void ab8500_usb_regulator_disable(struct ab8500_usb *ab) ab->saved_v_ulpi, ret); } - ret = regulator_set_optimum_mode(ab->v_ulpi, 0); + ret = regulator_set_load(ab->v_ulpi, 0); if (ret < 0) dev_err(ab->dev, "Failed to set optimum mode (ret=%d)\n", ret); -- 1.8.2.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] usb: phy: phy-msm-usb: Rename regulator_set_optimum_mode
The function regulator_set_optimum_mode() is changing name to regulator_set_load(), so update the code accordingly. Signed-off-by: Bjorn Andersson --- drivers/usb/phy/phy-msm-usb.c | 15 +-- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/drivers/usb/phy/phy-msm-usb.c b/drivers/usb/phy/phy-msm-usb.c index 000fd89..6ed67ea 100644 --- a/drivers/usb/phy/phy-msm-usb.c +++ b/drivers/usb/phy/phy-msm-usb.c @@ -142,27 +142,22 @@ static int msm_hsusb_ldo_set_mode(struct msm_otg *motg, int on) int ret = 0; if (on) { - ret = regulator_set_optimum_mode(motg->v1p8, - USB_PHY_1P8_HPM_LOAD); + ret = regulator_set_load(motg->v1p8, USB_PHY_1P8_HPM_LOAD); if (ret < 0) { pr_err("Could not set HPM for v1p8\n"); return ret; } - ret = regulator_set_optimum_mode(motg->v3p3, - USB_PHY_3P3_HPM_LOAD); + ret = regulator_set_load(motg->v3p3, USB_PHY_3P3_HPM_LOAD); if (ret < 0) { pr_err("Could not set HPM for v3p3\n"); - regulator_set_optimum_mode(motg->v1p8, - USB_PHY_1P8_LPM_LOAD); + regulator_set_load(motg->v1p8, USB_PHY_1P8_LPM_LOAD); return ret; } } else { - ret = regulator_set_optimum_mode(motg->v1p8, - USB_PHY_1P8_LPM_LOAD); + ret = regulator_set_load(motg->v1p8, USB_PHY_1P8_LPM_LOAD); if (ret < 0) pr_err("Could not set LPM for v1p8\n"); - ret = regulator_set_optimum_mode(motg->v3p3, - USB_PHY_3P3_LPM_LOAD); + ret = regulator_set_load(motg->v3p3, USB_PHY_3P3_LPM_LOAD); if (ret < 0) pr_err("Could not set LPM for v3p3\n"); } -- 1.8.2.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html