Re: [PATCH RFC v3] mmc: sdhci-msm: Add support for MSM chipsets
Hi Georgi,
On Tue, 2013-08-20 at 19:44 +0300, Georgi Djakov wrote:
> This platform driver adds the support of Secure Digital Host
> Controller Interface compliant controller in MSM chipsets.
>
> CC: Asutosh Das
> CC: Venkat Gopalakrishnan
> CC: Sahitya Tummala
> CC: Subhash Jadavani
> Signed-off-by: Georgi Djakov
> ---
> Changes from v2:
> - Added DT bindings for clocks
> - Moved voltage regulators data to platform data
> - Removed unneeded includes
> - Removed obsolete and wrapper functions
> - Removed error checking where unnecessary
> - Removed redundant _clk suffix from clock names
> - Just return instead of goto where possible
> - Minor fixes
>
Is this version intermediate step before you address
all previous comments?
> +
> +static const struct of_device_id sdhci_msm_dt_match[] = {
> + {.compatible = "qcom,sdhci-msm"},
Missing termination entry
> +};
> +
> +MODULE_DEVICE_TABLE(of, sdhci_msm_dt_match);
> +
> +static int sdhci_msm_probe(struct platform_device *pdev)
> +{
> + struct sdhci_host *host;
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_msm_host *msm_host;
> + struct resource *core_memres = NULL;
> + int ret = 0, dead = 0;
> + struct pinctrl *pinctrl;
> +
> + msm_host = devm_kzalloc(&pdev->dev, sizeof(struct sdhci_msm_host),
> + GFP_KERNEL);
> + if (!msm_host)
> + return -ENOMEM;
> +
> + host = sdhci_pltfm_init(pdev, &msm_host->sdhci_msm_pdata, 0);
> + if (IS_ERR(host)) {
> + dev_err(mmc_dev(host->mmc), "sdhci_pltfm_init error\n");
> + return PTR_ERR(host);
> + }
> +
> + pltfm_host = sdhci_priv(host);
> + pltfm_host->priv = msm_host;
> + msm_host->mmc = host->mmc;
> + msm_host->pdev = pdev;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret)
Shouldn't sdhci_pltfm_init operation be reverted?
> + return ret;
> +
> + /* Extract platform data */
> + if (pdev->dev.of_node) {
> + msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev);
> + if (!msm_host->pdata) {
> + dev_err(&pdev->dev, "DT parsing error\n");
> + goto pltfm_free;
> + }
> + } else {
> + dev_err(&pdev->dev, "No device tree node\n");
> + goto pltfm_free;
> + }
> +
> + /* Setup pins */
> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> + if (IS_ERR(pinctrl))
> + dev_warn(&pdev->dev, "pins are not configured by the driver\n");
> +
> + /* Setup Clocks */
> +
> + /* Setup SDCC bus voter clock. */
> + msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> + if (!IS_ERR_OR_NULL(msm_host->bus_clk)) {
> + /* Vote for max. clk rate for max. performance */
> + ret = clk_set_rate(msm_host->bus_clk, INT_MAX);
> + if (ret)
> + goto pltfm_free;
> + ret = clk_prepare_enable(msm_host->bus_clk);
> + if (ret)
> + goto pltfm_free;
> + }
> +
> + /* Setup main peripheral bus clock */
> + msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
> + if (!IS_ERR(msm_host->pclk)) {
> + ret = clk_prepare_enable(msm_host->pclk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Main peripheral clock setup failed (%d)\n",
> + ret);
> + goto bus_clk_disable;
> + }
> + }
> +
> + /* Setup SDC MMC clock */
> + msm_host->clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(msm_host->clk)) {
> + ret = PTR_ERR(msm_host->clk);
> + dev_err(&pdev->dev, "SDC MMC clock setup failed (%d)\n", ret);
> + goto pclk_disable;
> + }
> +
> + ret = clk_prepare_enable(msm_host->clk);
> + if (ret)
> + goto pclk_disable;
> +
> + /* Setup regulators */
> + ret = sdhci_msm_vreg_init(&pdev->dev, msm_host->pdata, true);
> + if (ret) {
> + dev_err(&pdev->dev, "Regulator setup failed (%d)\n", ret);
> + goto clk_disable;
> + }
> +
> + /* Reset the core and Enable SDHC mode */
> + core_memres = platform_get_resource_byname(pdev,
> +IORESOURCE_MEM, "core_mem");
> + msm_host->core_mem = devm_ioremap(&pdev->dev, core_memres->start,
> + resource_size(core_memres));
> +
> + if (!msm_host->core_mem) {
> + dev_err(&pdev->dev, "Failed to remap registers\n");
> + ret = -ENOMEM;
> + goto vreg_deinit;
> + }
> +
> + /* Set SW_RST bit in POWER register (Offset 0x0) */
> + writel_relaxed(CORE_SW_RST, msm_host->core_mem + CORE_POWER);
> + /* Set HC_MODE_EN bit in HC_MODE register */
> + writel_relaxed(HC_MODE_EN, (msm_host->core_mem + CORE_HC_MODE));
> +
> + /*
> +
Re: [PATCH RFC v2 2/5] spmi: Linux driver framework for SPMI
On Thu, Aug 22, 2013 at 04:10:54PM -0700, Greg Kroah-Hartman wrote:
> On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote:
> > +static char dbgfs_help[] =
> > + "SPMI Debug-FS support\n"
> > + "\n"
> > + "Hierarchy schema:\n"
> > + "/sys/kernel/debug/spmi\n"
> > + " /help-- Static help text\n"
> > + " /spmi-0 -- Directory for SPMI bus 0\n"
> > + " /spmi-0/0-1 -- Directory for SPMI device '0-1'\n"
> > + " /spmi-0/0-1/address -- Starting register for reads or writes\n"
> > + " /spmi-0/0-1/count-- Number of registers to read (only used
> > for reads)\n"
> > + " /spmi-0/0-1/data -- Initiates the SPMI read (formatted
> > output)\n"
> > + " /spmi-0/0-1/data_raw -- Initiates the SPMI raw read or write\n"
> > + " /spmi-n -- Directory for SPMI bus n\n"
> > + "\n"
> > + "To perform SPMI read or write transactions, you need to first write
> > the\n"
> > + "address of the slave device register to the 'address' file. For
> > read\n"
> > + "transactions, the number of bytes to be read needs to be written to
> > the\n"
> > + "'count' file.\n"
> > + "\n"
> > + "The 'address' file specifies the 20-bit address of a slave device
> > register.\n"
> > + "The upper 4 bits 'address[19..16]' specify the slave identifier (SID)
> > for\n"
> > + "the slave device. The lower 16 bits specify the slave register
> > address.\n"
> > + "\n"
> > + "Reading from the 'data' file will initiate a SPMI read transaction
> > starting\n"
> > + "from slave register 'address' for 'count' number of bytes.\n"
> > + "\n"
> > + "Writing to the 'data' file will initiate a SPMI write transaction
> > starting\n"
> > + "from slave register 'address'. The number of registers written to
> > will\n"
> > + "match the number of bytes written to the 'data' file.\n"
> > + "\n"
> > + "Example: Read 4 bytes starting at register address 0x1234 for SID 2\n"
> > + "\n"
> > + "echo 0x21234 > address\n"
> > + "echo 4 > count\n"
> > + "cat data\n"
> > + "\n"
> > + "Example: Write 3 bytes starting at register address 0x1008 for SID 1\n"
> > + "\n"
> > + "echo 0x11008 > address\n"
> > + "echo 0x01 0x02 0x03 > data\n"
> > + "\n"
> > + "Note that the count file is not used for writes. Since 3 bytes are\n"
> > + "written to the 'data' file, then 3 bytes will be written across the\n"
> > + "SPMI bus.\n\n";
>
> The help file within the kernel is a nice touch :)
>
> I do know the only rule for debugfs is "There are no rules", but this
> looks like you are going to have the way to interact to this bus and
> devices as debugfs, is that correct?
Using debugfs is _a_ way to interact with the controller/slaves, however
it is not _the_ way to do so. The primary interface is the in-kernel
spmi_{read,write,...}_* functions called within the context of a proper
slave driver.
> Or is this only for "debugging"? If so, please document it as such.
It's there because it provides a useful interface for debugging of the
controller code, and for simple peek/poke of the slave registers without
having a full driver in place. Will document this.
> > +void spmi_dfs_controller_add(struct spmi_controller *ctrl)
> > +{
> > + ctrl->dfs_dir = debugfs_create_dir(dev_name(&ctrl->dev),
> > + spmi_debug_root);
> > + WARN_ON(!ctrl->dfs_dir);
>
> Why? What is a user going to be able to do with something like this?
> You do this in a number of places, please provide "valid" error messages
> instead of just kernel stack tracebacks, failing to show the device for
> which the error occured (hint, use dev_err()).
Will do. Thanks.
> Again, never use WARN_ON() as error handling, it's lazy, and wrong.
To be fair to the original author of this code, this was one of the
'cleanups' I implemented. So, I'll take full responsibility for the
laziness. :)
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 5/5] spmi: document the PMIC arbiter SPMI bindings
On 08/09/2013 02:37 PM, Josh Cartwright wrote:
Patch description?
> diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> +Required properties:
> +- compatible : should be "qcom,spmi-pmic-arb".
> +- reg-names : should be "core", "intr", "cnfg"
> +- reg : offset and length of the PMIC Arbiter Core register map.
> +- reg : offset and length of the PMIC Arbiter Interrupt controller register
> map.
> +- reg : offset and length of the PMIC Arbiter Configuration register map.
This seems like it's defining the "reg" property 3 times each with a
different meaning. It'd be better to say something like:
reg : register specifier. Must contain 3 entries, in the following
order: core registers, interrupt register, configuration registers.
> + qcom,spmi@fc4c {
...
> + qcom,pm8841@4 {
Node names typically don't include a vendor prefix. For the first
instance above, I think just "spmi@fc4c" or even just "spmi" would
be appropriate here; the latter being best in the case where there's
only 1 SPMI controller and hence no need to include the unit address for
uniqueness.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation
On 08/22/2013 01:59 PM, Josh Cartwright wrote: > Signed-off-by: Josh Cartwright > --- > I'm introducing this as an RFC, because there are set of assumptions > made in this binding spec, that currently hold true for the supported > controller/addressing scheme for the Snapdragon 800 series, but don't > necessarily hold true for SPMI in general. > > 1. No use of Group Slave Identifiers (GSIDs) > (SPMI allows for a slave to belong to zero or more groups specified > by GSID, however this feature isn't currently implemented) > > 2. No specification of Master Identifier (MID) > (A "system integrator" allocates to each master a 2-bit MID, this > currently isn't being specified, as it isn't needed by software for > the PMIC Arb; not sure if this is of use to other SPMI controllers) > > 3. Single SPMI master per controller > > Effectively, only a subset of possible SPMI configurations are specified > in this document. > > So, if it's considered necessary to provide a generic SPMI binding > specification, is it acceptable to only define a subset at this time, > expanding only when necessary, or shall I expand the definition to at > least address 1 & 2, even though they are of no use in the current > implementation? It's best to define the whole thing from the start if possible. It's easier to ensure the whole binding is consistent, and nothing has been left out. However, it's probably OK to define a subset binding initially and then expand it later, as long as some thought it put into how it can be expanded in a way that is 100% compatible: old DTs will still operate with new kernels and perhaps even new DTs will still operate with old kernels. That said, if the thought is put in to ensure that's possible, it's probably just as easy to define the whole binding from the start. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
