Re: [PATCH v3 0/4] Raspberry Pi power domains
On 17 December 2015 at 21:11, Arnd Bergmann wrote: > On Thursday 17 December 2015 11:03:47 Eric Anholt wrote: >> >> >> >> .../bindings/arm/bcm/raspberrypi,bcm2835-power.txt | 47 >> >> arch/arm/boot/dts/bcm2835-rpi.dtsi | 11 + >> >> arch/arm/boot/dts/bcm2835.dtsi | 2 +- >> >> arch/arm/mach-bcm/Kconfig | 10 + >> >> arch/arm/mach-bcm/Makefile | 1 + >> >> arch/arm/mach-bcm/raspberrypi-power.c | 247 >> >> + >> >> include/dt-bindings/arm/raspberrypi-power.h| 41 >> >> include/soc/bcm2835/raspberrypi-firmware.h | 2 + >> >> 8 files changed, 360 insertions(+), 1 deletion(-) >> >> create mode 100644 >> >> Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-power.txt >> >> create mode 100644 arch/arm/mach-bcm/raspberrypi-power.c >> >> create mode 100644 include/dt-bindings/arm/raspberrypi-power.h >> >> >> >> -- >> >> 2.6.2 >> >> >> > >> > Besides a nitpick for patch2, I would also reverse the order of patch3 >> > and patch2. DT docs should go in before the actual parsing of the new >> > bindings/compatibles. >> > >> > Anyway, for the hole series, you may add my: >> > >> > Reviewed-by: Ulf Hansson >> >> Would your tree be pulling the series (since it's power domains), or >> should I (since it's SOC stuff)? >> > > All of the above files go through the arm-soc tree by default. That should work as I don't see any other dependency, unless Eric/Alexander think there is. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] Raspberry Pi power domains
On 15 December 2015 at 22:40, Eric Anholt wrote: > Since the pm_genpd_exit() patch is still going through review, and > other drivers in the tree just ignore the error cases, Ulf offered to > merge the series as a builtin driver not depending on that interface. > We still avoid dangling pointer references, by just continuing with > probing if of_genpd_add_provider_onecell() fails. We can easily go > back and update the driver when a pm_genpd_exit() lands. > > Alexander Aring (3): > ARM: bcm2835: add rpi power domain driver > dt-bindings: add rpi power domain driver bindings > ARM: bcm2835: Add the Raspberry Pi power domain driver to the DT. > > Eric Anholt (1): > ARM: bcm2835: Define two new packets from the latest firmware. > > .../bindings/arm/bcm/raspberrypi,bcm2835-power.txt | 47 > arch/arm/boot/dts/bcm2835-rpi.dtsi | 11 + > arch/arm/boot/dts/bcm2835.dtsi | 2 +- > arch/arm/mach-bcm/Kconfig | 10 + > arch/arm/mach-bcm/Makefile | 1 + > arch/arm/mach-bcm/raspberrypi-power.c | 247 > + > include/dt-bindings/arm/raspberrypi-power.h| 41 > include/soc/bcm2835/raspberrypi-firmware.h | 2 + > 8 files changed, 360 insertions(+), 1 deletion(-) > create mode 100644 > Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-power.txt > create mode 100644 arch/arm/mach-bcm/raspberrypi-power.c > create mode 100644 include/dt-bindings/arm/raspberrypi-power.h > > -- > 2.6.2 > Besides a nitpick for patch2, I would also reverse the order of patch3 and patch2. DT docs should go in before the actual parsing of the new bindings/compatibles. Anyway, for the hole series, you may add my: Reviewed-by: Ulf Hansson Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/4] ARM: bcm2835: add rpi power domain driver
On 17 December 2015 at 01:26, Eric Anholt wrote: > From: Alexander Aring > > This patch adds support for several power domains on Raspberry Pi, > including USB (so it can be enabled even if the bootloader didn't do > it), and graphics. > > This patch is the combined work of Eric Anholt (who wrote USB support > inside of the Raspberry Pi firmware driver, and wrote the non-USB > domain support) and Alexander Aring (who separated the original USB > work out from the firmware driver). > > Signed-off-by: Alexander Aring > Signed-off-by: Eric Anholt > --- > > v2: Add support for power domains other than USB, using the new > firmware interface, reword commit message (changes by Eric) > > v3: Restructure as a builtin driver, and drop > of_genpd_add_provider_onecell error handling to avoid > pm_genpd_exit() dependency until that API can be settled. Clean > up copyright header, add missing ISP initialization, and fix typo > in transposer's name. > > v4: Move to drivers/soc/bcm/, move include to dt-bindings/soc/, set up > Makefile for the drivers/soc/bcm following clk/'s model. > > drivers/soc/Kconfig | 1 + > drivers/soc/Makefile| 1 + > drivers/soc/bcm/Kconfig | 9 + > drivers/soc/bcm/Makefile| 1 + > drivers/soc/bcm/raspberrypi-power.c | 247 > > include/dt-bindings/soc/raspberrypi-power.h | 41 + There is currently a directory which I think may be better. include/dt-bindings/power/* Besides this nitpick, I think this looks good. You may add my: Reviewed-by: Ulf Hansson Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS
On 16 December 2015 at 12:40, Jon Hunter wrote: > > On 16/12/15 09:47, Ulf Hansson wrote: >> On 16 December 2015 at 10:40, Jon Hunter wrote: >>> Hi Ulf, >>> >>> On 15/12/15 19:54, Ulf Hansson wrote: >>>> On 4 December 2015 at 15:57, Jon Hunter wrote: >>>>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices >>>>> dependent upon a particular power-domain are only probed when that power >>>>> domain has been powered up, requires that PM is made mandatory for tegra >>>>> 64-bit devices and so select this option for tegra as well. >>>>> >>>>> Signed-off-by: Jon Hunter >>>>> --- >>>>> arch/arm64/Kconfig.platforms | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms >>>>> index 9806324fa215..e0b5bd0aff0f 100644 >>>>> --- a/arch/arm64/Kconfig.platforms >>>>> +++ b/arch/arm64/Kconfig.platforms >>>>> @@ -93,6 +93,8 @@ config ARCH_TEGRA >>>>> select GENERIC_CLOCKEVENTS >>>>> select HAVE_CLK >>>>> select PINCTRL >>>>> + select PM >>>>> + select PM_GENERIC_DOMAINS >>>> >>>> If you still want to allow ARCH_TEGRA to run without PM, you should >>>> probably change to: >>>> >>>> select PM_GENERIC_DOMAINS if PM >>> >>> Per the changelog this is deliberate. If we allow !PM, then there is a >>> potential that you could probe a device when the power domain is not >>> powered on. I understand that some SoCs turn on all the power-domains >>> when !PM but this will not work for tegra because we don't register the >>> power domain until later in the boot and so we are relying upon probe >>> deferral to defer the probe of devices that use power-domains. >> >> So what you are saying is that adding the PM domain support, will fix >> some devices to become successfully probed as they were broken before? > > Not exactly. There is a legacy tegra_powergate_sequence_power_up() that > has been used to date to get around this. However, by migrating to GENPD > we really need to make PM mandatory, otherwise you could attempt to > probe a device in a power domain that is not powered. In other words, > you are probing blindly. I know some SoCs do this, but that does not > seem very robust. Thank for the clarification. I agree. You may add my: Reviewed-by: Ulf Hansson Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] ARM: bcm2835: add rpi power domain driver
On 16 December 2015 at 02:27, Krzysztof Kozlowski wrote: > 2015-12-16 10:11 GMT+09:00 Sebastian Reichel : >> Hi, >> >> On Tue, Dec 15, 2015 at 04:53:31PM -0800, Eric Anholt wrote: >>> >>> What motivated the location of this power domain driver in >>> >>> arch/arm/mach-bcm? Should not we have this in drivers/power/ or >>> >>> somewhere in drivers/ at the very least? >>> >> >>> >> ls stronly suggests that power contains drivers for power supplies and >>> >> batteries, not power domains. >> >> Indeed it's used for fuel gauges and chargers, but also for >> reboot/powerdown and adaptive voltage scaling, so another >> subdirectory for power-domains wouldn't be that odd. >> >>> >> There are 6 power domain drivers in >>> >> arch/arm, 3 in drivers/clk, and 3 in drivers/soc. >>> > >>> > If we ever have to support a different architecture which happens to use >>> > a similar power domain, then we want it to be in a location which makes >>> > it easy for sharing it in the first place. As it stands today, it does >>> > not seem useful to me to have this code in arch/arm/mach-bcm/ at all. >>> > >>> > Maybe there is room from a drivers/power/domains/ of some kind? >> >> I like the idea, but let's include generic power domain maintainers >> in this discussion, as I suggested here (I got a power domain driver >> patch for drivers/power just a few days ago): >> >> https://lkml.org/lkml/2015/12/15/815 >> >> Also somebody would have to step up to maintain that directory. > > This could go into drivers/soc. We put there a lot of mach-specific > stuff which we want to make a little more generic (like generic enough > multiplatform, multiarchitecture etc). Rockchip has its own power > domains there. Dove and Mediatek seem as well but I am not sure. Some > other architectures keep this still in arm/mach (exynos, ux500, zx, > imx, s34c64xx, shmobile) but this looks more of like a legacy choice. Agree, drivers/soc is good. > > However, since the generic power domains have its own maintainers entry > and reside under drivers/base/power, maybe making a separate directory > for power domains drivers makes sense? That could work as well, but I have no strong opinion. Perhaps it would become a bit more clear, although in that case I would also move drivers/base/power/domain* in there. If that happens, I am willing to help maintain it. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS
On 16 December 2015 at 10:40, Jon Hunter wrote: > Hi Ulf, > > On 15/12/15 19:54, Ulf Hansson wrote: >> On 4 December 2015 at 15:57, Jon Hunter wrote: >>> Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices >>> dependent upon a particular power-domain are only probed when that power >>> domain has been powered up, requires that PM is made mandatory for tegra >>> 64-bit devices and so select this option for tegra as well. >>> >>> Signed-off-by: Jon Hunter >>> --- >>> arch/arm64/Kconfig.platforms | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms >>> index 9806324fa215..e0b5bd0aff0f 100644 >>> --- a/arch/arm64/Kconfig.platforms >>> +++ b/arch/arm64/Kconfig.platforms >>> @@ -93,6 +93,8 @@ config ARCH_TEGRA >>> select GENERIC_CLOCKEVENTS >>> select HAVE_CLK >>> select PINCTRL >>> + select PM >>> + select PM_GENERIC_DOMAINS >> >> If you still want to allow ARCH_TEGRA to run without PM, you should >> probably change to: >> >> select PM_GENERIC_DOMAINS if PM > > Per the changelog this is deliberate. If we allow !PM, then there is a > potential that you could probe a device when the power domain is not > powered on. I understand that some SoCs turn on all the power-domains > when !PM but this will not work for tegra because we don't register the > power domain until later in the boot and so we are relying upon probe > deferral to defer the probe of devices that use power-domains. So what you are saying is that adding the PM domain support, will fix some devices to become successfully probed as they were broken before? If that's the case, this makes all good sense! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB
On 9 December 2015 at 09:55, Arnd Bergmann wrote: > On Wednesday 09 December 2015 16:12:24 Peter Chen wrote: >> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote: >> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote: >> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote: >> > > > This something we don't have to define ad-hoc. The hub hangs off an USB >> > > > controller, right? The "Open Firmware recommended practice: USB" >> > > > document already describes how to represent USB devices in a generic >> > > > manner: >> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps >> > > > >> > > > Is there a reason not to reuse this? >> > > > >> > > > The usb hub node would be a child of the usb controller node, and it >> > > > could use >> > > > compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */ >> > > >> > > Good point, I had not thought of that when I looked at the patches. >> > > >> > > Yes, let's do this way. I don't know if we ever implemented the simple >> > > patch to associate a USB device with a device_node, but if not, then >> > > let's do it now for this driver. A lot of people have asked for it in >> > > the past. >> > >> > Agreed. Also, some hubs have I2C buses as well, but I still think under >> > the USB bus is the right place. >> > >> > However, one complication here is often (probably this case) these >> > addtional signals need to be controlled before the device enumerates. >> > >> >> Yes, I did not find a way to let the USB bus code handle it, so I had to >> write a platform driver to do it > > Looping in Ulf, he solved the same problem for SDIO devices recently, > and probably remembers the details best. > Thanks Arnd! Yes, that was a kind of a long outstanding issue we have had for SDIO devices. Several generic attempts was made to have a framework/library available to support so called "power sequences" for exactly the same reasons as above. Others and myself failed to get those attempts accepted. Instead, I invented a mmc subsystem specific DT based solution, the "mmc-pwrseq". DT documentation: Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt Documentation/devicetree/bindings/mmc/mmc.txt Long story short: The mmc host device may contain a phandle to a mmc-pwrseq, which will describe the resources needed to power on/off the SDIO card. The code is available at: drivers/mmc/core/pwrseq* We didn't implement this as platform driver, but that was mostly because I initially wanted things to be simple. Although, nothing prevents us from converting to this as a follow up, which would make the solution a bit less "hacky". Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS
On 4 December 2015 at 15:57, Jon Hunter wrote: > Enable PM_GENERIC_DOMAINS for tegra 64-bit devices. To ensure that devices > dependent upon a particular power-domain are only probed when that power > domain has been powered up, requires that PM is made mandatory for tegra > 64-bit devices and so select this option for tegra as well. > > Signed-off-by: Jon Hunter > --- > arch/arm64/Kconfig.platforms | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms > index 9806324fa215..e0b5bd0aff0f 100644 > --- a/arch/arm64/Kconfig.platforms > +++ b/arch/arm64/Kconfig.platforms > @@ -93,6 +93,8 @@ config ARCH_TEGRA > select GENERIC_CLOCKEVENTS > select HAVE_CLK > select PINCTRL > + select PM > + select PM_GENERIC_DOMAINS If you still want to allow ARCH_TEGRA to run without PM, you should probably change to: select PM_GENERIC_DOMAINS if PM > select RESET_CONTROLLER > help > This enables support for the NVIDIA Tegra SoC family. > -- > 2.1.4 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/5] power: domain: add pm_genpd_exit
+Russell
[...]
>> >> +void pm_genpd_exit(struct generic_pm_domain *genpd)
>> >> +{
>> >> + if (IS_ERR_OR_NULL(genpd))
>> >> + return;
>> >> +
>> >> + /* check if domain is still in registered inside the pm subsystem */
>> >> + WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
>> >> + !list_empty(&genpd->slave_links) ||
>> >> + !list_empty(&genpd->dev_list));
>> >> +
>> >
>> > Why not return an error here? Seems bad to remove it, if it could still
>> > be referenced by other domains.
>>
>> I had pointed this out as well in an earlier review.
>>
>
> I talked with Ulf Hansson about such handling and there exists currently
> no handling to remove the pm_genpd on error handling (what our use case
> is for RPi domains).
>
> The real solution would be a "pm_genpd_exit_recursive" functionality to
> remove subdomains, etc -> simple everything. Anway I am not a expert into
> power domain functionality and this simple approach was enough to him to
> add "something" which we have actually a lack of support.
>
>
[...]
Just to be clear on my view. I think Russell made a good summary of
how we should implement this API [1].
We should return an error, instead of WARN_ON_ONCE and continue.
Perhaps you can do both a WARN *and* return an error.
Regarding the similar patch from Jon Hunter, I would suggest we focus
on Alexander's $subject patch and try to it queued for 4.5. Please
send a new version.
Also, as other SoC genpd driver isn't using a "pm_genpd_exit()" API,
it shouldn't prevent neither Alexander/Eric or Jon from proceeding
with the RPI/Tegra genpd drivers. Once the new API is in place you can
update these drivers to properly deal with error handling and undo
things from pm_genpd_init().
Kind regards
Uffe
[1]
https://lkml.org/lkml/2015/12/9/308
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/4] power: domain: add pm_genpd_exit
On 29 November 2015 at 17:59, Alexander Aring wrote:
> This patch adds function pm_genpd_exit for undo a pm_genpd_init. This
> is useful for multiple power domains while probing. If the probing fails
> after one pm_genpd_init was called we need to undo all previous
> registrations of generic pm domains inside the gpd_list list.
>
> There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
> registered power domains and not registered domains, the driver can use
> this mechanism to have an array with registered and non-registered power
> domains, where non-registered power domains are NULL.
>
> Cc: Rafael J. Wysocki
> Cc: Kevin Hilman
> Cc: Ulf Hansson
> Cc: Pavel Machek
> Cc: Len Brown
> Cc: Greg Kroah-Hartman
> Signed-off-by: Alexander Aring
Acked-by: Ulf Hansson
Kind regards
Uffe
> ---
> drivers/base/power/domain.c | 22 ++
> include/linux/pm_domain.h | 4
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e03b1ad..311cb0e 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1509,6 +1509,28 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
> }
> EXPORT_SYMBOL_GPL(pm_genpd_init);
>
> +/**
> + * pm_genpd_exit - Uninitialize a generic I/O PM domain object.
> + * @genpd: PM domain object to uninitialize.
> + */
> +void pm_genpd_exit(struct generic_pm_domain *genpd)
> +{
> + if (IS_ERR_OR_NULL(genpd))
> + return;
> +
> + /* check if domain is still in registered inside the pm subsystem */
> + WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
> +!list_empty(&genpd->slave_links) ||
> +!list_empty(&genpd->dev_list));
> +
> + mutex_lock(&gpd_list_lock);
> + list_del(&genpd->gpd_list_node);
> + mutex_unlock(&gpd_list_lock);
> +
> + mutex_destroy(&genpd->lock);
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_exit);
> +
> #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> /*
> * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ba4ced3..5020b36 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -123,6 +123,7 @@ extern int pm_genpd_remove_subdomain(struct
> generic_pm_domain *genpd,
> struct generic_pm_domain *target);
> extern void pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> +extern void pm_genpd_exit(struct generic_pm_domain *genpd);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -161,6 +162,9 @@ static inline void pm_genpd_init(struct generic_pm_domain
> *genpd,
> struct dev_power_governor *gov, bool is_off)
> {
> }
> +static inline void pm_genpd_exit(struct generic_pm_domain *genpd)
> +{
> +}
> #endif
>
> static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
> --
> 2.6.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: bcm2835: add rpi power domain driver
[...] > diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig > index 8c53c55..20479d7 100644 > --- a/arch/arm/mach-bcm/Kconfig > +++ b/arch/arm/mach-bcm/Kconfig > @@ -134,6 +134,16 @@ config ARCH_BCM2835 > This enables support for the Broadcom BCM2835 SoC. This SoC is > used in the Raspberry Pi and Roku 2 devices. > > +config RASPBERRYPI_POWER You don't need a new Kconfig option I think. If you fold in the below "select" under ARCH_BCM2835, that should work as well, right? select PM_GENERIC_DOMAINS if (RASPBERRYPI_FIRMWARE && PM && OF) > + bool "Raspberry Pi power domain driver" > + depends on ARCH_BCM2835 > + depends on RASPBERRYPI_FIRMWARE > + select PM_GENERIC_DOMAINS if PM > + select PM_GENERIC_DOMAINS_OF if PM > + help > + This enables support for the RPi power domains which can be enabled > + or disabled via the RPi firmware. > + > config ARCH_BCM_63XX > bool "Broadcom BCM63xx DSL SoC" if ARCH_MULTI_V7 > depends on MMU > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile > index 892261f..fec2d6b 100644 > --- a/arch/arm/mach-bcm/Makefile > +++ b/arch/arm/mach-bcm/Makefile > @@ -36,6 +36,7 @@ endif > > # BCM2835 > obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o > +obj-$(CONFIG_RASPBERRYPI_POWER)+= raspberrypi-power.o According to above, then this should become: obj-$(CONFIG_PM_GENERIC_DOMAINS) += raspberrypi-power.o [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] power: domain: add pm_genpd_uninit
On 19 November 2015 at 19:08, Alexander Aring wrote:
> This patch adds function pm_genpd_uninit for undo a pm_genpd_init. This
> is useful for multiple power domains while probing. If the probing fails
> after one pm_genpd_init was called we need to undo all previous
> registrations of generic pm domains inside the gpd_list list.
>
> There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
> registered power domains and not registered domains, the driver can use
> this mechanism to have an array with registered and non-registered power
> domains, where non-registered power domains are NULL.
>
> Cc: Rafael J. Wysocki
> Cc: Kevin Hilman
> Cc: Ulf Hansson
> Cc: Pavel Machek
> Cc: Len Brown
> Cc: Greg Kroah-Hartman
> Signed-off-by: Alexander Aring
> ---
> drivers/base/power/domain.c | 22 ++
> include/linux/pm_domain.h | 4
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e03b1ad..24f54b8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1509,6 +1509,28 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
> }
> EXPORT_SYMBOL_GPL(pm_genpd_init);
>
> +/**
> + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object.
> + * @genpd: PM domain object to initialize.
> + */
> +void pm_genpd_uninit(struct generic_pm_domain *genpd)
> +{
> + if (IS_ERR_OR_NULL(genpd))
> + return;
> +
> + /* check if domain is still in registered inside the pm subsystem */
> + WARN_ON_ONCE(!list_empty(&genpd->master_links) ||
> +!list_empty(&genpd->slave_links) ||
> +!list_empty(&genpd->dev_list));
> +
We did discuss about verifying that the genpd mustn't have a
corresponding DT provider. I do realize that it becomes a bit complex
to verify that, unless we decide to add some handle to it within the
genpd struct.
Anyway, perhaps this minimal effort is good enough as is. Especially
since we won't be able to handle the error cases, besides giving a
WARN.
> + mutex_lock(&gpd_list_lock);
> + list_del(&genpd->gpd_list_node);
> + mutex_unlock(&gpd_list_lock);
> +
> + mutex_destroy(&genpd->lock);
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_uninit);
> +
> #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> /*
> * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index ba4ced3..df84a45 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -123,6 +123,7 @@ extern int pm_genpd_remove_subdomain(struct
> generic_pm_domain *genpd,
> struct generic_pm_domain *target);
> extern void pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> +extern void pm_genpd_uninit(struct generic_pm_domain *genpd);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -161,6 +162,9 @@ static inline void pm_genpd_init(struct generic_pm_domain
> *genpd,
> struct dev_power_governor *gov, bool is_off)
> {
> }
> +static inline void pm_genpd_uninit(struct generic_pm_domain *genpd)
> +{
> +}
> #endif
>
> static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
> --
> 2.6.1
>
So, I am fine with this, but let's see if other people have objections.
Acked-by: Ulf Hansson
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power
[...] >> > This one to not write an invalid voltage in the power control register >> > even if we have an external regulator for vmmc. >> > >> >> As I stated earlier, according to the SDHCI spec in the section for >> the Power Control Register. Bit 0 needs to be set when communicating >> with the card as it will for >> example enable the clock. >> > > I am okay with bit 0. I don't want to change this part, it will be done > later in sdhci_set_power(). My concern is only about bit 3-1, I want to > go through the switch statement. For those variants that have a VMMC and don't care about the other bits (1->3), it means executing code that isn't needed. Instead, as I have been telling people several times by now, let's convert the "sdhci core" into a library, so each variant can pick and do what suite them best. > >> I suspect if I apply your patch several sdhci variants would break, >> don't you think? > > I wouldn't sign it with my blood but I don't think so. It seems they > don't care about the SD bus Voltage since they work with an unsupported > voltage. You may very well be right, that it doesn't break anything. But in this case I really don't want to take the risk. As stated above, the proper solution would be that sdhci_set_power() should be split up in smaller pieces, where each piece may become a library function. Each host variant can then decide what to use. Future wise, that would mean when changing a library function, it will affect the subset of the sdhci variants that actually use it and not *all* sdhci variants. Moreover it will lead to optimized code. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power
On 24 November 2015 at 14:12, Ludovic Desroches wrote: > On Tue, Nov 24, 2015 at 12:01:53PM +0100, Ulf Hansson wrote: >> On 24 November 2015 at 10:23, Ludovic Desroches >> wrote: >> > Hi Ulf, >> > >> > On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote: >> >> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote: >> > >> > [...] >> > >> >> > Now, this discussion was interesting, but I forgot what problem you >> >> > actually where trying to solve? :-) >> >> >> >> There is this discussion because of two things: >> >> - Fixing something I consider as a bug: when I have a VMMC, only >> >> setting/clearing bit 0. Our controller strictly obeys to the spec and >> >> check the 'SD Bus Voltage Select' field. Since we put a reserved value >> >> (000), the Power On is not performed. >> >> - I was trying to get help to understand what is this 'SD Bus Voltage'. >> >> For our controller and sdhci_set_power(), it seems to stand for VMMC. >> >> For me, everything concerning bus voltage is related to VQMMC, so I was >> >> disappointed. >> > >> > Do you plan to take the patch for VMMC? If yes, I will send a new patch >> > for the device tree (I'll only add vmmc, not vqmmc as discussed); if >> > not, forget these two patches. >> > >> >> Which patch do you refer to for "VMMC"? >> > > This one to not write an invalid voltage in the power control register > even if we have an external regulator for vmmc. > As I stated earlier, according to the SDHCI spec in the section for the Power Control Register. Bit 0 needs to be set when communicating with the card as it will for example enable the clock. I suspect if I apply your patch several sdhci variants would break, don't you think? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power
On 24 November 2015 at 10:23, Ludovic Desroches wrote: > Hi Ulf, > > On Mon, Nov 09, 2015 at 05:30:26PM +0100, Ludovic Desroches wrote: >> On Mon, Nov 09, 2015 at 05:00:46PM +0100, Ulf Hansson wrote: > > [...] > >> > Now, this discussion was interesting, but I forgot what problem you >> > actually where trying to solve? :-) >> >> There is this discussion because of two things: >> - Fixing something I consider as a bug: when I have a VMMC, only >> setting/clearing bit 0. Our controller strictly obeys to the spec and >> check the 'SD Bus Voltage Select' field. Since we put a reserved value >> (000), the Power On is not performed. >> - I was trying to get help to understand what is this 'SD Bus Voltage'. >> For our controller and sdhci_set_power(), it seems to stand for VMMC. >> For me, everything concerning bus voltage is related to VQMMC, so I was >> disappointed. > > Do you plan to take the patch for VMMC? If yes, I will send a new patch > for the device tree (I'll only add vmmc, not vqmmc as discussed); if > not, forget these two patches. > Which patch do you refer to for "VMMC"? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/19] mmc: core/host: enable support for the standard "wakeup-source" property
On 21 October 2015 at 12:10, Sudeep Holla wrote: > Though the mmc core driver should/will continue to support the legacy > "enable-sdio-wakeup" property to enable SDIO as the wakeup source, we > need to add support for the new standard property "wakeup-source". > > This patch adds support for "wakeup-source" property in addition to the > existing "enable-sdio-wakeup" property. > > Cc: Ulf Hansson > Cc: Adrian Hunter > Cc: linux-...@vger.kernel.org > Signed-off-by: Sudeep Holla Thanks, applied for next! Kind regards Uffe > --- > drivers/mmc/core/host.c| 3 ++- > drivers/mmc/host/sdhci-pltfm.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 5466f25f0281..85222bb56c73 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -513,7 +513,8 @@ int mmc_of_parse(struct mmc_host *host) > host->caps2 |= MMC_CAP2_FULL_PWR_CYCLE; > if (of_property_read_bool(np, "keep-power-in-suspend")) > host->pm_caps |= MMC_PM_KEEP_POWER; > - if (of_property_read_bool(np, "enable-sdio-wakeup")) > + if (of_property_read_bool(np, "wakeup-source") || > + of_property_read_bool(np, "enable-sdio-wakeup")) /* legacy */ > host->pm_caps |= MMC_PM_WAKE_SDIO_IRQ; > if (of_property_read_bool(np, "mmc-ddr-1_8v")) > host->caps |= MMC_CAP_1_8V_DDR; > diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c > index a207f5aaf62f..f00374bdafc9 100644 > --- a/drivers/mmc/host/sdhci-pltfm.c > +++ b/drivers/mmc/host/sdhci-pltfm.c > @@ -108,7 +108,8 @@ void sdhci_get_of_property(struct platform_device *pdev) > if (of_find_property(np, "keep-power-in-suspend", NULL)) > host->mmc->pm_caps |= MMC_PM_KEEP_POWER; > > - if (of_find_property(np, "enable-sdio-wakeup", NULL)) > + if (of_property_read_bool(np, "wakeup-source") || > + of_property_read_bool(np, "enable-sdio-wakeup")) /* legacy */ > host->mmc->pm_caps |= MMC_PM_WAKE_SDIO_IRQ; > } > #else > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 1/2] power: domain: add pm_genpd_uninit
On 11 November 2015 at 19:00, Alexander Aring wrote: > Hi, > > On Thu, Nov 05, 2015 at 03:34:45PM +0100, Alexander Aring wrote: >> >> What do you suggest to me for e.g. the raspberrypi power domain driver, >> also simple ignore such error handling? >> > > ping, I also can add some WARN_ON_ONCE, if the list for sub-domains, > etc. are not empty. This would then report about wrong use of > pm_genpd_uninit. > > - Alex Sorry for the delay. I think what you suggest would be an okay solution, at least it will improve the current behaviour. We should verify for sub-domains, attached devices, and if the genpd has an of-provider. That's all I can think of right now, but there may be other things as well. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 1/2] power: domain: add pm_genpd_uninit
[...]
>> >
>> > +/**
>> > + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object.
>> > + * @genpd: PM domain object to initialize.
>> > + */
>> > +void pm_genpd_uninit(struct generic_pm_domain *genpd)
>> > +{
>> > + if (IS_ERR_OR_NULL(genpd))
>> > + return;
>> > +
>> > + mutex_lock(&gpd_list_lock);
>> > + list_del(&genpd->gpd_list_node);
>> > + mutex_unlock(&gpd_list_lock);
>>
>> This is too fragile. You don't protect from the cases below.
>>
>> 1. The genpd may have devices attached to it.
>> 2. The genpd may have subdomains.
>>
>> To deal with these case, that's when it becomes more complex which I
>> guess is the reason to why nobody really cared until now.
>>
>
> Can we not just undo the things which pm_genpd_init does, then let the
> driver to deal with all other uninitialize of e.g. "subdomains" which might
> the driver has initialize before? We know there are no slaves or something
> else which is empty because pm_genpd_init runs INIT_LIST_HEAD, or not?
>
> To make such handling above I would add some:
>
> "pm_genpd_uninit_recursive" function, which cleanup everything from a
> power domain, e.g. subdomains, etc and other lists.
>
> What do you suggest to me for e.g. the raspberrypi power domain driver,
> also simple ignore such error handling?
No, let's give it try so see if can improve the situation.
>
>
> For my current use-case I can remove the failure between pm_genpd_init by
> simple getting all "power states" (the bool is_off for pm_genpd_init),
> before calling pm_genpd_init. In this case I don't have any failure between
> calling pm_genpd_init when another pm_genpd_init was before.
>
> Looks like this:
>
> 1. get all states from firmware to get parameter "is_off", which can fail.
>We should get here the states for the power domains which we want to
>register only.
>
> 2. Then call pm_genpd_init, which cannot be fail between another
>pm_genpd_init. (There are only void functions in the middle, which cannot
>fail). If I call pm_genpd_init, I use "is_off" variable from the step
>of "1.".
>
>
> But then I have still the call of "of_genpd_add_provider_onecell" at the end
> of probing which can fail. So this is also not a valid solution, because
> I need to undo everything before.
Okay, got it.
>
>> Moreover, I think there are some more structures to "uninitialize"
>> besides just unlinking the genpd struct from the gpd list. For example
>> a mutex_destroy() should be done.
>>
>
> yes, of course.
>
> - Alex
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 16/16] mmc: host: omap_hsmmc: use "mmc_of_parse_voltage" to get ocr_avail
On 11 November 2015 at 11:26, Roger Quadros wrote: > Hi, > > On 25/08/15 17:50, Ulf Hansson wrote: >> On 3 August 2015 at 14:26, Kishon Vijay Abraham I wrote: >>> From: Roger Quadros >>> >>> For platforms that doesn't have explicit regulator control in MMC, >>> populate voltage-ranges in MMC device tree node and use >>> mmc_of_parse_voltage to get ocr_avail >> >> I don't like this. >> >> If we are able to fetch the OCR mask via an external regulator, that >> shall be done. > > Agreed. >> >> I think the mmc_of_parse_voltage() API and the corresponding DT >> binding it parses, should be used for those HW when we don't have an >> external regulator to use. For example if the MMC controller itself >> somehow controls the voltage levels. Is that really the case for you? > > What shall be done if there is no software control of the external regulator > and it is fixed at a certain voltage? I think you can model that as a so called fixed regulator. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power
On 9 November 2015 at 15:40, Ludovic Desroches wrote: > On Mon, Nov 09, 2015 at 03:12:46PM +0100, Ulf Hansson wrote: >> On 9 November 2015 at 14:23, Ludovic Desroches >> wrote: >> > On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote: >> >> When there is a vmmc regulator, only SD Bus Power is set to 1 in the >> >> Power Control Register. It means SD Bus Voltage Select field is set to 0 >> >> that is a reserved value. The SD Host Controller specification says: >> >> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD >> >> Bus Voltage Select.' and 'If the Host Driver selects an unsupported >> >> voltage in the SD B?us Voltage Select field, the Host Controller may >> >> ignore writes to SD Bus Power and keep its value at zero." >> >> >> >> Having an external regulator means the SD Bus Voltage Select is useless >> >> but if the Host Controller strictly follows the specification then we >> >> need to set a valid value. >> > >> > Ulf, >> > >> > What is your opinion about this patch? >> > >> > If the 'no regulator found' message is turned in debug message, I can get >> > rid of my vmmc regulator but I really think that writing only >> >> I expect you mean vqmmc? > > I don't mean vmmc. In the sdhci_set_power function, we are using vmmc. > I feel not confortable with it because the power control register > contains 'SD Bus' fields so it should depend on vqmmc not vmmc. > >> >> > SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not >> > setting the bus voltage is a quirk! >> >> I don't really follow. >> >> I read the SDHCI spec and the section for the Power Control Register. >> Bit 0 needs to be set when communicating with the card as it will for >> example enables the clock. Before setting bit0 you must decide what >> signal level to use, which is done by writing to bit 1->3. >> > > Right. But when having vmmc supply we do: > sdhci_writeb(host, SDHCI_POWER_ON, SDHCI_POWER_CONTROL) or > sdhci_writeb(host, 0, SDHCI_POWER_CONTROL) so we loose the signal level, > isn't it? > >> If SDHCI monitors the power state (MMC_POWER_UP|ON|OFF) when its >> ->set_ios() callback are invoked and in combination of using the >> ->start_signal_voltage_switch() callback to change the signal voltage >> level, this *should* work out nicely. >> > > It is my turn to not follow! We write into the Power Control Register > only in sdhci_set_power(). May I miss a callback or something else? > sdhci_do_start_signal_voltage_switch doesn't modify the Power Control > Register. > >> Now, looking at the related code in sdhci, I am kind of surprised that >> it works. :-) Though, again I don't have the in-depth knowledge about >> sdhci. >> > > Me too, I am starting to dig into the sdhci spec and some points are > not crystal clear. > Okay, I am finally starting to understand some of your concern. According to the spec, the Power Control Register should control the signal voltage/bus voltage. As UHS mode was added to the spec, it seems like the Power Control Register couldn't cover all new cases, as why Host Control 2 register needed to be added. The Host Control 2 register, is what sdhci_do_start_signal_voltage_switch() uses to change the signal voltage level, which all makes sense to me. For sdhci_set_power(); it seems to use the Power Control Register to control the power to the card (VDD/VMMC). Indeed this looks *really* weird/wrong. I wonder if it's working because of luck, intentional violation of the SDHCI spec or because of special variants. Especially when looking into the case when you *don't* have a VMMC regulator several strange quirks exists in sdhci_set_power(). In the case when you *have* a VMMC, I think just setting/clearing bit 0 (SDHCI_POWER_ON) and then bail out, is probably working with modern HW because it's likely the only thing needed. Now, this discussion was interesting, but I forgot what problem you actually where trying to solve? :-) Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci: set bus voltage before enabling bus power
On 9 November 2015 at 14:23, Ludovic Desroches wrote: > On Fri, Nov 06, 2015 at 04:59:29PM +0100, Ludovic Desroches wrote: >> When there is a vmmc regulator, only SD Bus Power is set to 1 in the >> Power Control Register. It means SD Bus Voltage Select field is set to 0 >> that is a reserved value. The SD Host Controller specification says: >> 'SD Bus Power: Before setting this bit, the SD Host Driver shall set SD >> Bus Voltage Select.' and 'If the Host Driver selects an unsupported >> voltage in the SD B?us Voltage Select field, the Host Controller may >> ignore writes to SD Bus Power and keep its value at zero." >> >> Having an external regulator means the SD Bus Voltage Select is useless >> but if the Host Controller strictly follows the specification then we >> need to set a valid value. > > Ulf, > > What is your opinion about this patch? > > If the 'no regulator found' message is turned in debug message, I can get > rid of my vmmc regulator but I really think that writing only I expect you mean vqmmc? > SDHCI_POWER_ON is opposite to the sdhci spec. I would say that not > setting the bus voltage is a quirk! I don't really follow. I read the SDHCI spec and the section for the Power Control Register. Bit 0 needs to be set when communicating with the card as it will for example enables the clock. Before setting bit0 you must decide what signal level to use, which is done by writing to bit 1->3. If SDHCI monitors the power state (MMC_POWER_UP|ON|OFF) when its ->set_ios() callback are invoked and in combination of using the ->start_signal_voltage_switch() callback to change the signal voltage level, this *should* work out nicely. Now, looking at the related code in sdhci, I am kind of surprised that it works. :-) Though, again I don't have the in-depth knowledge about sdhci. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2] mmc: sdhci: potentially bad behavior when using vmmc supply
On 9 November 2015 at 14:14, Ludovic Desroches wrote: > On Mon, Nov 09, 2015 at 11:50:50AM +0100, Ulf Hansson wrote: >> [...] >> >> >> >> >> This doesn't seems like a case where a gpio regulator should be used >> >> and I am not sure what problem it would solve. Beside to suppress the >> >> log warnings (actually those aren't warnings but informations). >> >> >> >> Isn't sdhci_do_start_signal_voltage_switch() doing what you need here? >> >> >> > >> > It is. I am only wondering the best way to describe the hardware: >> > - No regulator but I have the 'no vqmmc regulator not found' message which >> > is a bit annoying and which can be interpreted as an issue for someone >> > who has no knowledge about this stuff. >> >> Hmm, should we turn them into debug messages? Both regulators are optional. >> > > I think so. From my point of view, it is useless to indicate that > something optionnal is missing. Okay. Please go ahead and send a patch. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2] mmc: sdhci: potentially bad behavior when using vmmc supply
[...] >> >> This doesn't seems like a case where a gpio regulator should be used >> and I am not sure what problem it would solve. Beside to suppress the >> log warnings (actually those aren't warnings but informations). >> >> Isn't sdhci_do_start_signal_voltage_switch() doing what you need here? >> > > It is. I am only wondering the best way to describe the hardware: > - No regulator but I have the 'no vqmmc regulator not found' message which > is a bit annoying and which can be interpreted as an issue for someone > who has no knowledge about this stuff. Hmm, should we turn them into debug messages? Both regulators are optional. > - Describe the regulator since there is one on my board. But it is not a > fixed regulator and even if it's close to a gpio one it is not. If it's driven by SDHCI internal logic, I would leave it to that. There are no need to describe it at all. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2] mmc: sdhci: potentially bad behavior when using vmmc supply
[...] >> > Is the regulator-gpio usage the right thing to do for vqmmc? In my case it >> > is >> > not really driven by a gpio but by a pio from the sdhci device. In the >> > binding, >> >> What's a "pio"? >> >> What do you mean by the it's driven from the sdhci device? >> > > Sorry I mean sdhci device from the SoC point of view, I should say > controller. So yes the signal is driven by the controller. > >> Is it the internal HW logic of the sdhci controller that manages the >> IO voltage? And this logic can be controlled via certain register bits >> in the SDHCI controller? >> > > Yes, it depends of the value of the '1.8V Signaling Enable' value in the > host control 2 register. > >> > declaring the gpio is an option so I thought using this regulator fits my >> > need. >> >> In quite many cases it makes sense to model this though a gpio >> regulator. For example when you use a level shifter circuit. Those >> normally have gpio pin routed to control the voltage level output for >> the signals. For example switching between 1.8V and 2.9V. >> > > I agree, my concern is to know if I can consider it as a 'general' pio > since it is driven by the sdhci controller. This doesn't seems like a case where a gpio regulator should be used and I am not sure what problem it would solve. Beside to suppress the log warnings (actually those aren't warnings but informations). Isn't sdhci_do_start_signal_voltage_switch() doing what you need here? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2] mmc: sdhci: potentially bad behavior when using vmmc supply
On 6 November 2015 at 16:59, Ludovic Desroches wrote: > Hi, > > I would like to have some feedback for these two patches. I have two > questions. > > 1. > > To suppress the warnings telling me I have no vmmc and vqmmc supplies, I have > added them and discovered that adding a vmmc supply involve only writing 1 in > the Power Control Register involves enabling the SD Bus Power with a non valid > value for the SD Bus Voltage. If the host controllers strictly follows the > specification, it shall not enable SD Bus Power. So enough if it seems useless > to configure SD Bus Voltage because we have an external regulator, do it. I can't give you a detailed answer about the sdhci HW as I only have limited knowledge. What I can say is that people have been trying to fix all kind of crazy corner cases by adding quirks and callbacks. This seems like yet another one. So, by turning sdhci into a set of library functions you could easier pick and decide to what suites your particular variant. In this case it seems like would have relied on using the external regulators to control voltages, instead of some internal sdhci registers. > > By the way, I am curious to understand what is really the SD Bus Voltage. I > mean talking about bus voltage makes me thinking more about vqmmc than vmmc. > Is it only bad naming or do I miss something? > > From the specification, there is this figure: > > HOST > --- > | 3.3V| VDD > | | Power SW ||-- > | | | | > | -| | | > | | | | > | -- -- | > | | Ref. Volt. |-| Regulator/Selector | | > | -- -- | > |3.3V/1.8V | | > | | | > | |--- | > | | | | | | > | | R R _ | > | | | | _ | > | | | | | | > | | | | /// | > |---| | | CLK > || |)--)--|-- > | --- |Multi|| | | CMD > | |Random Logic Circuits|---|Drive|)-|-- > | --- |I/O || | DAT > || |--|-- > |--- | > --- > > In my mind vmmc is the 3.3V signal and vqmmc is the 3.3V/1.8V signal, so why > talking about bus voltage? "IO voltage", "bus voltage", "VQMMC", etc they all mean the same thing to me. It's the voltage level of the signals going to the card. > > > 2. > > Is the regulator-gpio usage the right thing to do for vqmmc? In my case it is > not really driven by a gpio but by a pio from the sdhci device. In the > binding, What's a "pio"? What do you mean by the it's driven from the sdhci device? Is it the internal HW logic of the sdhci controller that manages the IO voltage? And this logic can be controlled via certain register bits in the SDHCI controller? > declaring the gpio is an option so I thought using this regulator fits my > need. In quite many cases it makes sense to model this though a gpio regulator. For example when you use a level shifter circuit. Those normally have gpio pin routed to control the voltage level output for the signals. For example switching between 1.8V and 2.9V. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 1/2] power: domain: add pm_genpd_uninit
On 3 November 2015 at 23:45, Alexander Aring wrote:
> This patch adds function pm_genpd_uninit for undo a pm_genpd_init. This
> is useful for multiple power domains while probing. If the probing fails
> after one pm_genpd_init was called we need to undo all previous
> registrations of generic pm domains inside the gpd_list list.
Yes, agree. Although I think it's a bit mote complicated than what you
suggest in this simple approach. :-)
>
> There is a check on IS_ERR_OR_NULL(genpd) which is useful to check again
> registered power domains and not registered domains, the driver can use
> this mechanism to have an array with registered and non-registered power
> domains, where non-registered power domains are NULL.
>
> Cc: Rafael J. Wysocki
> Cc: Kevin Hilman
> Cc: Ulf Hansson
> Cc: Pavel Machek
> Cc: Len Brown
> Cc: Greg Kroah-Hartman
> Signed-off-by: Alexander Aring
> ---
> drivers/base/power/domain.c | 15 +++
> include/linux/pm_domain.h | 4
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 16550c6..65b9d1a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1730,6 +1730,21 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
> }
> EXPORT_SYMBOL_GPL(pm_genpd_init);
>
> +/**
> + * pm_genpd_uninit - Uninitialize a generic I/O PM domain object.
> + * @genpd: PM domain object to initialize.
> + */
> +void pm_genpd_uninit(struct generic_pm_domain *genpd)
> +{
> + if (IS_ERR_OR_NULL(genpd))
> + return;
> +
> + mutex_lock(&gpd_list_lock);
> + list_del(&genpd->gpd_list_node);
> + mutex_unlock(&gpd_list_lock);
This is too fragile. You don't protect from the cases below.
1. The genpd may have devices attached to it.
2. The genpd may have subdomains.
To deal with these case, that's when it becomes more complex which I
guess is the reason to why nobody really cared until now.
Moreover, I think there are some more structures to "uninitialize"
besides just unlinking the genpd struct from the gpd list. For example
a mutex_destroy() should be done.
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_uninit);
> +
> #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> /*
> * Device Tree based PM domain providers.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b1cf7e7..45d4f7a 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -143,6 +143,7 @@ extern int pm_genpd_detach_cpuidle(struct
> generic_pm_domain *genpd);
> extern int pm_genpd_name_detach_cpuidle(const char *name);
> extern void pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> +extern void pm_genpd_uninit(struct generic_pm_domain *genpd);
>
> extern int pm_genpd_poweron(struct generic_pm_domain *genpd);
> extern int pm_genpd_name_poweron(const char *domain_name);
> @@ -212,6 +213,9 @@ static inline void pm_genpd_init(struct generic_pm_domain
> *genpd,
> struct dev_power_governor *gov, bool is_off)
> {
> }
> +static inline void pm_genpd_uninit(struct generic_pm_domain *genpd)
> +{
> +}
> static inline int pm_genpd_poweron(struct generic_pm_domain *genpd)
> {
> return -ENOSYS;
> --
> 2.6.1
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/10] Add tune support of Mediatek MMC driver
On 27 October 2015 at 07:24, Chaotian Jing wrote: > Change in v3: > Fix checkpatch errors and warnings for patch 8 > Split patch 9, make DT parts enabling hw reset separately > > Change in v2: > Drop the 400mhz and use assigned-clock-parents to instead > Split the original tune patch to several independent patches > Re-write the mmc_send_tuning() > Fix GPD checksum error > Move the HS400 setting to ops->prepare_hs400_tuning() > Modify SD driving settings > > Change in v1: > Add DT bindings for eMMC hardware reset > Add pinctrl of data strobe pin for HS400 mode > Modify eMMC driving settings > Add 400mhz source clock for HS400 mode > Add eMMC HS200/HS400 mode support > Add SD SDR50/SDR104 mode support > Add implement of tune function with CMD19/CMD21 > > Chaotian Jing (10): > mmc: core: Add DT bindings for eMMC hardware reset support > mmc: dt-bindings: update Mediatek MMC bindings > mmc: mediatek: make cmd_ints_mask to const > mmc: mediatek: change the argument "ddr" to "timing" > mmc: mediatek: fix got GPD checksum error interrupt when data transfer > mmc: mediatek: add implement of ops->hw_reset() > arm64: dts: mediatek: add eMMC hw reset support > mmc: mmc: extend the mmc_send_tuning() > mmc: mediatek: add HS400 support > arm64: dts: mediatek: add HS200/HS400/SDR50/SDR104 support > > Documentation/devicetree/bindings/mmc/mmc.txt| 1 + > Documentation/devicetree/bindings/mmc/mtk-sd.txt | 11 +- > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 27 +- > drivers/mmc/core/host.c | 2 + > drivers/mmc/core/mmc_ops.c | 8 +- > drivers/mmc/host/dw_mmc-exynos.c | 4 +- > drivers/mmc/host/dw_mmc.c| 2 +- > drivers/mmc/host/dw_mmc.h| 2 +- > drivers/mmc/host/mtk-sd.c| 304 > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 6 +- > drivers/mmc/host/sdhci-msm.c | 2 +- > drivers/mmc/host/sdhci-sirf.c| 2 +- > include/linux/mmc/core.h | 2 +- > 13 files changed, 322 insertions(+), 51 deletions(-) > > -- > 1.8.1.1.dirty > Chaotian, I have applied this except the ARM patches (patch7 and patch10) as I think those should go via ARM SoC (unless I get acks for them). Regarding patch8, it didn't apply since it needed a re-base. This time it was trivial to fix, so I decided to pick it up anyway. Future wise I also advise you to significantly trim the cc/to list when posting patches. Likely what happens when that many people are requested for input, is that *none* cares. :-) Thanks and kind regards! Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/19] ARM: dts: at91: replace gpio-key,wakeup with wakeup-source property
On 21 October 2015 at 12:25, Sudeep Holla wrote: > > > On 21/10/15 11:21, Nicolas Ferre wrote: >> >> Le 21/10/2015 12:10, Sudeep Holla a écrit : >>> >>> Though the keyboard driver for GPIO buttons(gpio-keys) will continue to >>> check for/support the legacy "gpio-key,wakeup" boolean property to >>> enable gpio buttons as wakeup source, "wakeup-source" is the new >>> standard binding. >>> >>> This patch replaces the legacy "gpio-key,wakeup" with the unified >>> "wakeup-source" property in order to avoid any futher copy-paste >>> duplication. >>> >>> Cc: Nicolas Ferre >> >> >> I'm not against this if the whole series goes further. >> Acked-by: Nicolas Ferre >> > > Thanks ! > >> I suspect that we would need to take this patch with us on the AT91 >> branches that would go to arm-soc. Is it the intentions? >> > > Yes that was my intention for splitting the patches per SoC group. > Many SoC maintainers prefer that. Hold on! All patches that changes the DT parsing to accept the "standardized wakeup-source" binding, need to be merged upstream before corresponding DTS changes. Therefore, I suggest we go for a two step approach, starting with changes affecting the DT parsing/documentation for various drivers/subsystem and perhaps we can reach 4.4 for these. Then the DTS changes can go in at any later point. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] DT: mmc: sh_mmcif: document R8A779[34] support
On 20 October 2015 at 22:19, Sergei Shtylyov wrote: > Hello. > > On 10/16/2015 04:07 PM, Ulf Hansson wrote: > >>> Renesas R8A7794 SoC also has the MMCIF controller... >>> >>> Signed-off-by: Sergei Shtylyov > > >> Thanks, applied for next. > > >Oops, just noticed the subject was stale. I wasn't adding R8A7793 support > in that version. Is it possible to fix? I have updated the subject, no worries! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 0/9] Add tune support of Mediatek MMC driver
On 20 October 2015 at 11:13, Chaotian Jing wrote: > Change in v2: > Drop the 400mhz and use assigned-clock-parents to instead > Split the original tune patch to several independent patches > Re-write the mmc_send_tuning() > Fix GPD checksum error > Move the HS400 setting to ops->prepare_hs400_tuning() > Modify SD driving settings > > Change in v1: > Add DT bindings for eMMC hardware reset > Add pinctrl of data strobe pin for HS400 mode > Modify eMMC driving settings > Add 400mhz source clock for HS400 mode > Add eMMC HS200/HS400 mode support > Add SD SDR50/SDR104 mode support > Add implement of tune function with CMD19/CMD21 > > Chaotian Jing (9): > mmc: core: Add DT bindings for eMMC hardware reset support > mmc: dt-bindings: update Mediatek MMC bindings > mmc: mediatek: make cmd_ints_mask to const > mmc: mediatek: change the argument "ddr" to "timing" > mmc: mediatek: fix got GPD checksum error interrupt when data transfer > mmc: mediatek: add implement of ops->hw_reset() > mmc: mmc: extend the mmc_send_tuning() > mmc: mediatek: add HS400 support > arm64: dts: mediatek: add HS200/HS400/SDR50/SDR104 support > > Documentation/devicetree/bindings/mmc/mmc.txt| 1 + > Documentation/devicetree/bindings/mmc/mtk-sd.txt | 11 +- > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 27 ++- > drivers/mmc/core/host.c | 2 + > drivers/mmc/core/mmc_ops.c | 8 +- > drivers/mmc/host/dw_mmc-exynos.c | 4 +- > drivers/mmc/host/dw_mmc.c| 2 +- > drivers/mmc/host/dw_mmc.h| 2 +- > drivers/mmc/host/mtk-sd.c| 296 > --- > drivers/mmc/host/sdhci-esdhc-imx.c | 6 +- > drivers/mmc/host/sdhci-msm.c | 2 +- > drivers/mmc/host/sdhci-sirf.c| 2 +- > include/linux/mmc/core.h | 2 +- > 13 files changed, 314 insertions(+), 51 deletions(-) > > -- > 1.8.1.1.dirty I have reviewed the patches, I think they overall looks good! Some comments though. You need to split patch 9, the DT parts enabling hw reset shall go in separately. Regarding the hw-reset changes in patch1, patch6 and patch9, I believe I requested you to separate the HW reset changes from $subject patchset as they are unrelated, please do this. Running checkpatch, it gave me warnings and errors for patch 8. Patch7 didn't apply to my next branch. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
[...] >>> > + ret = phy_power_on(sdhci_arasan->phy); This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()? Similar comment applies to phy_exit() and phy_power_off(). >>> >>> >>> >>> Both are okay. It depends how the phy-driver implement the four >>> interfaces. >>> From my case, power_on for arasan's phy driver firstly enable phy's clk >>> and >>> open power-domain, then programme phy internal registers to activate phy. >>> Without enabling phy's clk and power-domain, we cannot do phy_init since >>> phy >>> can't be accessed by CPU. >>> >>> But here, I think you are right. It does look a bit weird. >>> I think the better way is to remove phy_power_on here, and let phy-driver >>> do >>> power_on in phy_init internally. Similarly in the remove call. >> >> >> That makes sense to me! I think it would also follow other phy clients >> use of the phy API. >> >> What makes me wonder though is from a power management point of view. >> *When* do you need to have phy initialized and powered? >> > > Whenever controller need to communicate with card, must init/power_on phy > firstly. > >> 1) For a removable card can leave the phy uninitialised or powered >> off, but still detect card insertion/removal via GPIO? Is that a valid >> scenario for you? >> > > Theoretically, it is. Although my soc don't use phy for removable card, I > also consider how to handle this case. Should we add a hook > for sdhci_get_cd, and initialize phy if it's non-removeable device or > removeable card in slot ? Doesn't seem like a good idea. > > Also seems not always work if we just use SDHCI_PRESENT_STATE to detect card > insertion/removal without phy in active state. As you SoC don't use removable card, let's just leave this for now. Thanks for sharing your thoughts. > >> 2) Considering the runtime PM case for the sdhci device. Typically you >> can gate clocks etc at runtime suspend to save power, but what about >> the phy? Can you power off it in runtime suspend? > > > yes, we can power off it in runtime suspend. So we can append some patches > later to introduce runtime pm for sdhci-of-arasan? Yes, that's fine. I would thus expect that you want to do phy power off/on from the runtime PM callbacks, right!? If that's the case I think $subject patch should deal with *both* phy init and phy power on during ->probe(). Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
[...] >> I understand the phy is optional, but you still need to handle the >> EPROBE_DEFER case. >> >> Perhaps you should also use devm_phy_optional_get() instead!? > > > I already changed it in version-2 [1]. :) > phy is mandatory for sdhci-arasan,5.1. > > [1]: https://patchwork.kernel.org/patch/7173251/ Oh, apologize for reviewing the old version! > >> >>> + ret = phy_power_on(sdhci_arasan->phy); >> >> >> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()? >> >> Similar comment applies to phy_exit() and phy_power_off(). > > > Both are okay. It depends how the phy-driver implement the four interfaces. > From my case, power_on for arasan's phy driver firstly enable phy's clk and > open power-domain, then programme phy internal registers to activate phy. > Without enabling phy's clk and power-domain, we cannot do phy_init since phy > can't be accessed by CPU. > > But here, I think you are right. It does look a bit weird. > I think the better way is to remove phy_power_on here, and let phy-driver do > power_on in phy_init internally. Similarly in the remove call. That makes sense to me! I think it would also follow other phy clients use of the phy API. What makes me wonder though is from a power management point of view. *When* do you need to have phy initialized and powered? 1) For a removable card can leave the phy uninitialised or powered off, but still detect card insertion/removal via GPIO? Is that a valid scenario for you? 2) Considering the runtime PM case for the sdhci device. Typically you can gate clocks etc at runtime suspend to save power, but what about the phy? Can you power off it in runtime suspend? [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] DT: mmc: sh_mmcif: fix "compatible" property text
On 16 October 2015 at 00:39, Sergei Shtylyov
wrote:
> The "compatible" property text contradicts even the example given in the MMCIF
> binding document itself; moreover, the Renesas MMCIF driver only matches on
> the generic "compatible" string and doesn't look for the SoC specific strings
> at all. Thus describe "renesas,sh-mmcif" as a fallback value.
>
> Fixes: b4c27763d749 ("mmc: sh_mmcif: Document DT bindings")
> Signed-off-by: Sergei Shtylyov
Thanks, applied for next.
Kind regards
Uffe
>
> ---
> The patch is against Ulf Hansson's 'mmc.git' repo's 'fixes' or 'next'
> branches.
>
> Changes in version 3:
> - reworded the "compatible" property description to look like the majority of
> the Renesas bindings.
>
> Changes in version 2:
> - kept the SoC specific "compatible" property values mandatory and made the
> generic string a fallback.
>
> Documentation/devicetree/bindings/mmc/renesas,mmcif.txt |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: mmc/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> ===
> --- mmc.orig/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> +++ mmc/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> @@ -6,11 +6,11 @@ and the properties used by the MMCIF dev
>
> Required properties:
>
> -- compatible: must contain one of the following
> +- compatible: should be "renesas,mmcif-", "renesas,sh-mmcif" as a
> + fallback. Examples with are:
> - "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs
> - "renesas,mmcif-r8a7790" for the MMCIF found in r8a7790 SoCs
> - "renesas,mmcif-r8a7791" for the MMCIF found in r8a7791 SoCs
> - - "renesas,sh-mmcif" for the generic MMCIF
>
> - clocks: reference to the functional clock
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] DT: mmc: sh_mmcif: document R8A779[34] support
On 16 October 2015 at 00:40, Sergei Shtylyov wrote: > Renesas R8A7794 SoC also has the MMCIF controller... > > Signed-off-by: Sergei Shtylyov Thanks, applied for next. Kind regards Uffe > > --- > The patch is against Ulf Hansson's 'mmc.git' repo's 'next' branches plus the > patch I posted earlier today... > > Changes in version 3: > - resolved reject. > > Changes in version 2: > - deferred R8A7793 support to the patch posted earlier by Ulrich Hecht; > - fixed typo in the changelog. > > Documentation/devicetree/bindings/mmc/renesas,mmcif.txt |1 + > 1 file changed, 1 insertion(+) > > Index: mmc/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt > === > --- mmc.orig/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt > +++ mmc/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt > @@ -11,6 +11,7 @@ Required properties: > - "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs > - "renesas,mmcif-r8a7790" for the MMCIF found in r8a7790 SoCs > - "renesas,mmcif-r8a7791" for the MMCIF found in r8a7791 SoCs > + - "renesas,mmcif-r8a7794" for the MMCIF found in r8a7794 SoCs > > - clocks: reference to the functional clock > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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] mmc: core: Allow specifying current consumption
On 13 October 2015 at 03:00, Bjorn Andersson
wrote:
> This allows us to specify expected current consumption of the vmmc and
> vqmmc regulators. This is needed to bring the supplying regulators out
> of their low-power-mode while accessing the mmc.
This indeed makes sense, still I need to think a bit more on this.
For example, can we allow these regulators to enter low power again at
some times? If so, when and what should that current value be.
Moreover, wouldn't vmmc|vqmmc-active-current be depending what eMMC/SD
card that is attached?
>
> Signed-off-by: Bjorn Andersson
> ---
>
> The sd specification states that if the host can provide more than 150mA this
> should be indicated in the "XPC value in the argument of ACMD41" for SDXC
> cards. Should this be tied in with the added property as well?
>
> Changes since v1:
> - Property name and description updated to clarify intention
>
> Documentation/devicetree/bindings/mmc/mmc.txt | 2 ++
> drivers/mmc/core/core.c | 6 ++
> drivers/mmc/core/host.c | 4
> include/linux/mmc/host.h | 2 ++
> 4 files changed, 14 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt
> b/Documentation/devicetree/bindings/mmc/mmc.txt
> index 0384fc3f64e8..7514083a9f55 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc.txt
> @@ -47,6 +47,8 @@ Optional properties:
> - mmc-hs400-1_2v: eMMC HS400 mode(1.2V I/O) is supported
> - dsr: Value the card's (optional) Driver Stage Register (DSR) should be
>programmed with. Valid range: [0 .. 0x].
> +- vmmc-active-current: current required from the vmmc regulator, in uA
> +- vqmmc-active-current: current required from the vqmmc regulator, in uA
>
> *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers
> line
> polarity properties, we have to fix the meaning of the "normal" and
> "inverted"
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 23fa221ce803..603f40136306 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1469,12 +1469,18 @@ int mmc_regulator_get_supply(struct mmc_host *mmc)
> mmc->ocr_avail = ret;
> else
> dev_warn(dev, "Failed getting OCR mask: %d\n", ret);
> +
> + if (mmc->supply.vmmc_current)
> + regulator_set_load(mmc->supply.vmmc,
> mmc->supply.vmmc_current);
> }
>
> if (IS_ERR(mmc->supply.vqmmc)) {
> if (PTR_ERR(mmc->supply.vqmmc) == -EPROBE_DEFER)
> return -EPROBE_DEFER;
> dev_info(dev, "No vqmmc regulator found\n");
> + } else {
> + if (mmc->supply.vqmmc_current)
> + regulator_set_load(mmc->supply.vqmmc,
> mmc->supply.vqmmc_current);
> }
>
> return 0;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 970e6906930b..ede508d63fb5 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -235,6 +235,10 @@ int mmc_of_parse(struct mmc_host *host)
> host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> }
>
> + /* Parse regulator load requests */
> + of_property_read_u32(np, "vmmc-active-current",
> &host->supply.vmmc_current);
> + of_property_read_u32(np, "vqmmc-active-current",
> &host->supply.vqmmc_current);
> +
> /* Parse Write Protection */
> ro_cap_invert = of_property_read_bool(np, "wp-inverted");
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 8673ffe3d86e..640d8dbb8559 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -197,6 +197,8 @@ struct mmc_pwrseq;
> struct mmc_supply {
> struct regulator *vmmc; /* Card power supply */
> struct regulator *vqmmc;/* Optional Vccq supply */
> + u32 vmmc_current; /* Requested current for vmmc */
> + u32 vqmmc_current; /* Requested current for vqmmc */
> };
>
> struct mmc_host {
> --
> 2.4.2
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
On 11 September 2015 at 10:54, Shawn Lin wrote:
> This patch adds Generic PHY access for sdhci-of-arasan. Driver
> can get PHY handler from dt-binding, and power-on/init the PHY.
> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.
>
> Signed-off-by: Shawn Lin
> ---
>
> drivers/mmc/host/sdhci-of-arasan.c | 90
> ++
> 1 file changed, 90 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> b/drivers/mmc/host/sdhci-of-arasan.c
> index 621c3f4..fdd71c7 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -21,6 +21,7 @@
>
> #include
> #include
> +#include
> #include "sdhci-pltfm.h"
>
> #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
> @@ -35,6 +36,7 @@
> */
> struct sdhci_arasan_data {
> struct clk *clk_ahb;
> + struct phy *phy;
> };
>
> static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> @@ -67,6 +69,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
>
> #ifdef CONFIG_PM_SLEEP
> /**
> + * sdhci_arasan_suspend_phy - Suspend phy method for the driver
> + * @phy:Handler of phy structure
> + * Returns 0 on success and error value on error
> + *
> + * Put the phy in a deactive state.
> + */
> +static int sdhci_arasan_suspend_phy(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_exit(phy);
> + if (ret < 0)
> + goto err_phy_exit;
This odd to me. First you do phy_exit() then phy_power_off(). It seems
like it should be in the opposite order.
Moreover I wonder why phy_exit() is needed, I expected that to be
called at ->remove() only!?
> +
> + ret = phy_power_off(phy);
> + if (ret < 0)
> + goto err_phy_pwr_off;
> +
> + return 0;
> +
> +err_phy_pwr_off:
> + phy_power_on(phy);
> +err_phy_exit:
> + phy_init(phy);
> + return ret;
> +}
> +
> +/**
> + * sdhci_arasan_resume_phy - Resume phy method for the driver
> + * @phy:Handler of phy structure
> + * Returns 0 on success and error value on error
> + *
> + * Put the phy in a active state.
> + */
> +static int sdhci_arasan_resume_phy(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_power_on(phy);
> + if (ret < 0)
> + goto err_phy_pwr_on;
> +
> + ret = phy_init(phy);
> + if (ret < 0)
> + goto err_phy_init;
> +
Similar comment as above.
> + return 0;
> +
> +err_phy_init:
> + phy_exit(phy);
> +err_phy_pwr_on:
> + phy_power_off(phy);
> + return ret;
> +}
> +
> +/**
> * sdhci_arasan_suspend - Suspend method for the driver
> * @dev: Address of the device structure
> * Returns 0 on success and error value on error
> @@ -88,6 +146,12 @@ static int sdhci_arasan_suspend(struct device *dev)
> clk_disable(pltfm_host->clk);
> clk_disable(sdhci_arasan->clk_ahb);
>
> + if (sdhci_arasan->phy) {
> + ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy);
> + if (ret < 0)
> + return ret;
> + }
This means you will suspend the phy after you have disabled the
clocks. Of course I can't tell whether that okay, but it doesn't
follow the same sequence as in ->probe(). To me that indicates that
either probe or suspend/resume could be broken.
> +
> return 0;
> }
>
> @@ -119,6 +183,12 @@ static int sdhci_arasan_resume(struct device *dev)
> return ret;
> }
>
> + if (sdhci_arasan->phy) {
> + ret = sdhci_arasan_resume_phy(sdhci_arasan->phy);
> + if (ret < 0)
> + return ret;
> + }
> +
> return sdhci_resume_host(host);
> }
> #endif /* ! CONFIG_PM_SLEEP */
> @@ -163,6 +233,26 @@ static int sdhci_arasan_probe(struct platform_device
> *pdev)
> goto clk_dis_ahb;
> }
>
> + sdhci_arasan->phy = devm_phy_get(&pdev->dev, "phy_arasan");
> + if (!IS_ERR(sdhci_arasan->phy)) {
I understand the phy is optional, but you still need to handle the
EPROBE_DEFER case.
Perhaps you should also use devm_phy_optional_get() instead!?
> + ret = phy_power_on(sdhci_arasan->phy);
This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?
Similar comment applies to phy_exit() and phy_power_off().
> + if (ret < 0) {
> + dev_err(&pdev->dev, "phy_power_on err.\n");
> + phy_power_off(sdhci_arasan->phy);
> + goto clk_dis_ahb;
> + }
> +
> + ret = phy_init(sdhci_arasan->phy);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "phy_init err.\n");
> + phy_exit(sdhci_arasan->phy);
> + phy_power_off(sdhci_arasan->phy);
> + goto clk_dis_ahb;
> + }
> + } else {
This else isn't needed. When you a
Re: [PATCH 3/4] mmc: mediatek: Add tune support
[...]
>> >
>> > struct clk *src_clk;/* msdc source clock */
>> > + struct clk *src_clk_parent; /* src_clk's parent */
>> > + struct clk *hs400_src; /* 400Mhz source clock */
>>
>> Hmm, so you need to control the upper level clocks. Can you elaborate
>> on why this is needed?
>>
> hs400 is DDR200, in our host design, if the mode is DDR(HS400), if want
> to get 200Mhz mmc bus clock frequency, the minimum source clock is
> double of the mmc bus clock, So, we need it for HS400 mode with 200Mhz
> bus clock.
Thanks for clarifying.
[...]
> flags = readl(host->base + MSDC_INTEN);
>> > sdr_clr_bits(host->base + MSDC_INTEN, flags);
>> > - if (ddr) { /* may need to modify later */
>> > - mode = 0x2; /* ddr mode and use divisor */
>> > + sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_HS400_CK_MODE);
>> > + if (timing == MMC_TIMING_UHS_DDR50 ||
>> > + timing == MMC_TIMING_MMC_DDR52 ||
>>
>> So, no support for HS200?
>>
> HS200 is the same with other SDR modes, so it will be handled at else..
Okay, nice!
So, your the driver currently supports HS200, but without need for tuning!?
[...]
>> > +static struct msdc_delay_phase get_best_delay(u32 delay)
>> > +{
>> > + int start = 0, len = 0;
>> > + int start_final = 0, len_final = 0;
>> > + u8 final_phase = 0xff;
>> > + struct msdc_delay_phase delay_phase;
>> > +
>> > + if (delay == 0) {
>> > + pr_err("phase error: [map:%x]\n", delay);
>>
>> Please use dev_err|warn|dbg|info instead.
>>
> As you know, this function is just only parse the argument "u32 delay",
> it do not bind with any device.
You may just add a msdc_host * as a parameter to the function, that
would solve this.
[...]
>> > +static int msdc_send_tuning(struct mmc_host *host, u32 opcode, int
>> > *cmd_error)
>>
>> I think you can remove this function and use mmc_send_tuning() instead.
> Hmm, I also noticed that there was a mmc_send_tuning, but, I need to get
> the cmd_error when tune cmd response, in this case, do not care the data
> error.
Well, if you need to extend the mmc_send_tuning() API to suite your
needs, let's do that instead of duplicating code.
>>
>> > +{
>> > + struct mmc_request mrq = {NULL};
>> > + struct mmc_command cmd = {0};
>> > + struct mmc_data data = {0};
>> > + struct scatterlist sg;
>> > + struct mmc_ios *ios = &host->ios;
>> > + int size, err = 0;
>> > + u8 *data_buf;
>> > +
>> > + if (ios->bus_width == MMC_BUS_WIDTH_8)
>> > + size = 128;
>> > + else if (ios->bus_width == MMC_BUS_WIDTH_4)
>> > + size = 64;
>> > + else
>> > + return -EINVAL;
>> > +
>> > + data_buf = kzalloc(size, GFP_KERNEL);
>> > + if (!data_buf)
>> > + return -ENOMEM;
>> > +
>> > + mrq.cmd = &cmd;
>> > + mrq.data = &data;
>> > +
>> > + cmd.opcode = opcode;
>> > + cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
>> > +
>> > + data.blksz = size;
>> > + data.blocks = 1;
>> > + data.flags = MMC_DATA_READ;
>> > +
>> > + /*
>> > +* According to the tuning specs, Tuning process
>> > +* is normally shorter 40 executions of CMD19,
>> > +* and timeout value should be shorter than 150 ms
>> > +*/
>> > + data.timeout_ns = 150 * NSEC_PER_MSEC;
>> > +
>> > + data.sg = &sg;
>> > + data.sg_len = 1;
>> > + sg_init_one(&sg, data_buf, size);
>> > +
>> > + mmc_wait_for_req(host, &mrq);
>> > +
>> > + if (cmd_error)
>> > + *cmd_error = cmd.error;
>> > +
>> > + if (cmd.error) {
>> > + err = cmd.error;
>> > + goto out;
>> > + }
>> > +
>> > + if (data.error) {
>> > + err = data.error;
>> > + goto out;
>> > + }
>> > +
>> > +out:
>> > + kfree(data_buf);
>> > + return err;
>> > +}
>> > +
[...]
>> > + host->src_clk_parent = clk_get_parent(host->src_clk);
>>
>> Don't you need to check the return value here?
>>
> will check it.
>> > + host->hs400_src = devm_clk_get(&pdev->dev, "400mhz");
>>
>> That's a really weird conid for a clock. If it's not too late to
>> change, please do that!
>>
>> > + if (IS_ERR(host->hs400_src)) {
>> > + dev_dbg(&pdev->dev, "Cannot find 400mhz at dts!\n");
>> > + } else if (clk_set_parent(host->src_clk_parent, host->hs400_src) <
>> > 0) {
>> > + dev_err(host->dev, "Failed to set 400mhz source clock!\n");
>> > + ret = -EINVAL;
>>
>> I think it seems more apropriate to use the return value from
>> clk_set_parent() instead of inventing your own return value.
>>
> OK.
>> > + goto host_free;
>> > + }
>>
>> It seems like you don't need to store the src_clk_parent and the
>> hs400_src in the host struct, as you are only using it during
>> ->probe().
> OK,will remove the membe
Re: [PATCH 3/4] mmc: mediatek: Add tune support
On 13 October 2015 at 11:37, Chaotian Jing wrote:
> Add CMD19/CMD21 support for EMMC/SD/SDIO tuning
> Add HS400 mode support
>
> Signed-off-by: Chaotian Jing
> ---
> drivers/mmc/host/mtk-sd.c | 359
> ++
> 1 file changed, 332 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index b2e89d3..f12a50a 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -64,6 +65,7 @@
> #define SDC_RESP20x48
> #define SDC_RESP30x4c
> #define SDC_BLK_NUM 0x50
> +#define EMMC_IOCON 0x7c
> #define SDC_ACMD_RESP0x80
> #define MSDC_DMA_SA 0x90
> #define MSDC_DMA_CTRL0x98
> @@ -71,6 +73,8 @@
> #define MSDC_PATCH_BIT 0xb0
> #define MSDC_PATCH_BIT1 0xb4
> #define MSDC_PAD_TUNE0xec
> +#define PAD_DS_TUNE 0x188
> +#define EMMC50_CFG0 0x208
>
>
> /*--*/
> /* Register Mask
> */
> @@ -87,6 +91,7 @@
> #define MSDC_CFG_CKSTB (0x1 << 7) /* R */
> #define MSDC_CFG_CKDIV (0xff << 8)/* RW */
> #define MSDC_CFG_CKMOD (0x3 << 16)/* RW */
> +#define MSDC_CFG_HS400_CK_MODE (0x1 << 18)/* RW */
>
> /* MSDC_IOCON mask */
> #define MSDC_IOCON_SDR104CKS(0x1 << 0) /* RW */
> @@ -204,6 +209,17 @@
> #define MSDC_PATCH_BIT_SPCPUSH(0x1 << 29) /* RW */
> #define MSDC_PATCH_BIT_DECRCTMO (0x1 << 30) /* RW */
>
> +#define MSDC_PAD_TUNE_DATRRDLY (0x1f << 8) /* RW */
> +#define MSDC_PAD_TUNE_CMDRDLY(0x1f << 16) /* RW */
> +
> +#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> +#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> +#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> +
> +#define EMMC50_CFG_PADCMD_LATCHCK (0x1 << 0) /* RW */
> +#define EMMC50_CFG_CRCSTS_EDGE(0x1 << 3) /* RW */
> +#define EMMC50_CFG_CFCSTS_SEL (0x1 << 4) /* RW */
> +
> #define REQ_CMD_EIO (0x1 << 0)
> #define REQ_CMD_TMO (0x1 << 1)
> #define REQ_DAT_ERR (0x1 << 2)
> @@ -219,6 +235,7 @@
> #define CMD_TIMEOUT (HZ/10 * 5)/* 100ms x5 */
> #define DAT_TIMEOUT (HZ* 5)/* 1000ms x5 */
>
> +#define DELAY_MAX 32 /* PAD dalay cells */
/s/dalay/delay
Can we have some more explaination around this define. Perhaps a more
self-explaining name would be enough.-
>
> /*--*/
> /* Descriptor Structure
> */
>
> /*--*/
> @@ -265,6 +282,14 @@ struct msdc_save_para {
> u32 pad_tune;
> u32 patch_bit0;
> u32 patch_bit1;
> + u32 pad_ds_tune;
> + u32 emmc50_cfg0;
> +};
> +
> +struct msdc_delay_phase {
> + u8 maxlen;
> + u8 start;
> + u8 final_phase;
> };
>
> struct msdc_host {
> @@ -293,12 +318,15 @@ struct msdc_host {
> int irq;/* host interrupt */
>
> struct clk *src_clk;/* msdc source clock */
> + struct clk *src_clk_parent; /* src_clk's parent */
> + struct clk *hs400_src; /* 400Mhz source clock */
Hmm, so you need to control the upper level clocks. Can you elaborate
on why this is needed?
> struct clk *h_clk; /* msdc h_clk */
> u32 mclk; /* mmc subsystem clock frequency */
> u32 src_clk_freq; /* source clock frequency */
> u32 sclk; /* SD/MS bus clock frequency */
> - bool ddr;
> + unsigned char timing;
> bool vqmmc_enabled;
> + u32 hs400_ds_delay;
> struct msdc_save_para save_para; /* used when gate HCLK */
> };
>
> @@ -353,7 +381,10 @@ static void msdc_reset_hw(struct msdc_host *host)
> static void msdc_cmd_next(struct msdc_host *host,
> struct mmc_request *mrq, struct mmc_command *cmd);
>
> -static u32 data_ints_mask = MSDC_INTEN_XFER_COMPL | MSDC_INTEN_DATTMO |
> +static const u32 cmd_ints_mask = MSDC_INTEN_CMDRDY | MSDC_INTEN_RSPCRCERR |
> + MSDC_INTEN_CMDTMO | MSDC_INTEN_ACMDRDY |
> + MSDC_INTEN_ACMDCRCERR | MSDC_INTEN_ACMDTMO;
This belongs to separate code improvement patch.
> +static const u32 data_ints_mask = MSDC_INTEN_XFER_COMPL | MSDC_INTEN_DATTMO |
> MSDC_INTEN_DATCRCERR | MSDC_INTEN_DMA_BDCSERR |
> MSDC_INTEN_DMA_GPDCSERR | MSDC_INTEN_DMA_PROTECT;
>
> @@ -485,7 +516,7 @@ static void msdc_ungate_clock(struct msdc_host *host)
> cpu_relax();
> }
>
> -static void msdc_set_mclk(struct msdc_host *host, int ddr, u32 hz)
> +static void msdc_set_mclk(struct msdc_host *host, unsigned char ti
Re: [RFC PATCH v3 1/3] mmc: sprd: Add MMC host driver for Spreadtrum SoC
[...] >> Thanks for clarifying! >> >> You have some differences towards the "standard" sdhci variant, but >> that doesn't mean you should go off and implement a new driver from >> scratch, instead you should create a new sdhci variant and re-use code >> from the generic sdhci driver. >> >> The current problem with such approach, is that the sdhci driver isn't >> designed as a library but instead a driver consisting of too many >> quirks and callbacks. While you start to adopt your driver towards >> sdhci, you will need to add yet another bunch of new quirks and >> callbacks to suite your hw. >> >> Now, as the number of callbacks and quirks continues to increase I >> will sooner or later give up maintaining it, as each line of code will >> depend on a quirk. So, we need to start turning sdhci into a library >> *right now*! Russell King, has pointed out this several times as well, >> but unfortunate I haven't yet seen anyone willing to help out in this >> field. >> >> I would of course be very happy if you would like to have a look at >> that, but I realize it's a difficult task, So, unless you are happy >> with taking on such a challenge, I suggest you go for an intermediate >> step, which thus means convert your driver to a sdhci variant driver >> and add the quirks/callbacks you need to suite your hw. >> >> [...] >> >> Kind regards >> Uffe > > Thanks for kindly suggestion! > I think it's a good idea to turn sdhci into a library. But for me it's > too difficult to > take on such a challenge. However, I will try my best to support you to do > it. Yes, I totally understand and thanks for your support. > > As you suggested, I will consider converting our eMMC host driver to a > sdhci variant driver. However, our controller has some features, which > differentiate it from standard sd host controller. For example, our controller > doesn't have such functions as follows: tuning or re-tuning, Power Control > Register, PIO or ADMA transfer mode, UHS-II and so on. So, if we use sdchi > variant driver right now, I think it has a litter redundancy. I realize that, but I would very much appreciate if you give it try - I think it should be doable. Of course, you will need to change the "sdhci core" to suite your needs and normally people do that via adding callbacks and quirks. Perhaps you can keep my request in mind of turning sdhci into a library and thus limit the number of added quirks and callbacks... > > Now our sdio team are discussing improving our eMMC host controller, we are > making it more standardized. But you know, changing a IP block is a long > process. Maybe it will take us about one or two years. So what do you think > if we use ourself eMMC host driver right now, and convert it when our new host > controller is ready. > Well, that won't help the current HW so I would encourage you to do the "sdhci variant" work anyway. Likely it will also benefit you when you try to upstream the next variant of the driver to cope with your new HW. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v3 1/3] mmc: sprd: Add MMC host driver for Spreadtrum SoC
+Russell On 28 September 2015 at 09:18, Hongtao Wu wrote: >>> > > On Thu, Sep 10, 2015 at 9:28 PM, Ulf Hansson wrote: >> >> On 14 August 2015 at 18:55, Hongtao Wu wrote: >> > the Spreadtrum MMC host driver is used to supply EMMC, SD, and >> > SDIO types of memory cards >> >> Perhaps some more information about the controller. Are there any >> specific features it support or doesn't support!? > > Thanks for kindly reply. > Yes, spreadtrum MMC host controllers have some specific features as follows: > (1) We don't have controls for sampling clock tuning and re-tuning, we take > place of them with three registers as follows: >(a) CLK_WR_DL(Offset 080h): Data write clock delay line. >(b) CLK_RD_POS_DL(Offset 0x84h): Posedge data read clock delay line. >(c) CLK_RD_NEG_DL(Offset 088h): Negedge data read clock delay line. > > (2) We don't have Power Control Register(Offset 029h), all our controller's > power > come from PMIC, rather than directly from CPU. > > (3) We don't have bit[6](Card Insertion), bit[7] (Card Removal), bit[8] > (Card Interrupt) > and bit[12:9] in Normal Interrupt Status Register(Offset 030h). Because the > detect > gpio pin doesn't connect to the register of our host controller. So we can't > operate bit[18:16](Card Detect Pin Level, Card State stable and Card > Inserted) > in Present State Register(Offset 024h). Thanks for clarifying! You have some differences towards the "standard" sdhci variant, but that doesn't mean you should go off and implement a new driver from scratch, instead you should create a new sdhci variant and re-use code from the generic sdhci driver. The current problem with such approach, is that the sdhci driver isn't designed as a library but instead a driver consisting of too many quirks and callbacks. While you start to adopt your driver towards sdhci, you will need to add yet another bunch of new quirks and callbacks to suite your hw. Now, as the number of callbacks and quirks continues to increase I will sooner or later give up maintaining it, as each line of code will depend on a quirk. So, we need to start turning sdhci into a library *right now*! Russell King, has pointed out this several times as well, but unfortunate I haven't yet seen anyone willing to help out in this field. I would of course be very happy if you would like to have a look at that, but I realize it's a difficult task, So, unless you are happy with taking on such a challenge, I suggest you go for an intermediate step, which thus means convert your driver to a sdhci variant driver and add the quirks/callbacks you need to suite your hw. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
On 17 September 2015 at 17:20, Shawn Lin wrote: > On 2015/9/17 19:44, Ulf Hansson wrote: >> >> On 14 September 2015 at 08:29, Shawn Lin wrote: >>> >>> This patch adds Generic PHY access for sdhci-of-arasan. Driver >>> can get PHY handler from dt-binding, and power-on/init the PHY. >>> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled. >>> Currently, it's just mandatory for arasan,sdhci-5.1. >> >> >> I am trying to understand how a PHY can be used together with a >> MMC/SD/SDIO controller. Normally the card connector doesn't hold any >> intelligence, so I wonder if PHY is correctly used here. >> >> Could you try to explain, HW-wise, what role the PHY has for you? >> > > Thanks for comment, Ulf. > > It's the first time we introduce phy into mmc subsystem,also it's my first > time to use phy for emmc in real Soc. > so definitely we need to discuss it deliberately. > > From my view, phy is an active-power, differential sampling and more > well-designed analog *IO component* for ultra-highspeed device to guarantee > its signal integrity, like USB, UFS, DDR-RAM,MIPI and so on. > > (1)Firstly it contains DLL to generate delayline and phase for sampling data > from mmc devices. When sdhci controller executes tuning and sends out tuning > sequence, and PHY can auto change its delayline and phase > in order to test if this "sample timing" is okay. If not, CRC err is > generate back to the controller. Then after SDHCI tune timing for 40 times, > also PHY have scanned all the "sample windows" by trying different delayline > and phase combination, and finally auto-select the best sample timing for > the "sample windows". And sdhci HOST CONTROL > 2 Register[6:7] is controlled by PHY in my case. > > (2)Then phy can programming source/sink impedance to avoid signal reflex. > Given that JEDEC has come to HS533(highspeed 533MHz), this function is > imperative to be implemented just like the role of ODT for DDR-RAM's PHY. > > (3)Phy is integrated with diff pull-up/down resistance value and open-drain > type for HW design. > > (4)Phy can also have some enable-bit to decide whether all mmc > signal(clk/data/cmd/strobe) can output to the devices or not. Thanks for elaborating. It seems like using phy is justified in your case. > > > For the card, even for host controller itself, phy can just be regarded as a > need-to-do-something-before-used GPIO. > So, before we start initializing card, we need to power up PHY, enable clk > for it and configure all the stuffs. Also we need to power down it for > power-saving when into suspend. That's all we need to do and that's all the > generic phy framework had provided by four interface: > phy_init/phy_exit/phy_power_on/phy_power_off. > > > Sincerely welcome any comments here to make things better. :) > > I will get back to your patch and review it in detail as soon as can. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
On 14 September 2015 at 08:29, Shawn Lin wrote:
> This patch adds Generic PHY access for sdhci-of-arasan. Driver
> can get PHY handler from dt-binding, and power-on/init the PHY.
> Also we add pm ops for PHY here if CONFIG_PM_SLEEP is enabled.
> Currently, it's just mandatory for arasan,sdhci-5.1.
I am trying to understand how a PHY can be used together with a
MMC/SD/SDIO controller. Normally the card connector doesn't hold any
intelligence, so I wonder if PHY is correctly used here.
Could you try to explain, HW-wise, what role the PHY has for you?
Kind regards
Uffe
>
> Signed-off-by: Shawn Lin
>
> Serise-changes: 2
> - Keep phy as a mandatory requirement for arasan,sdhci-5.1
>
> ---
>
> Changes in v2: None
>
> drivers/mmc/host/sdhci-of-arasan.c | 97
> ++
> 1 file changed, 97 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> b/drivers/mmc/host/sdhci-of-arasan.c
> index 75379cb..2c13ef8 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -21,6 +21,7 @@
>
> #include
> #include
> +#include
> #include "sdhci-pltfm.h"
>
> #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
> @@ -35,6 +36,7 @@
> */
> struct sdhci_arasan_data {
> struct clk *clk_ahb;
> + struct phy *phy;
> };
>
> static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> @@ -70,6 +72,62 @@ static struct sdhci_pltfm_data sdhci_arasan_pdata = {
>
> #ifdef CONFIG_PM_SLEEP
> /**
> + * sdhci_arasan_suspend_phy - Suspend phy method for the driver
> + * @phy:Handler of phy structure
> + * Returns 0 on success and error value on error
> + *
> + * Put the phy in a deactive state.
> + */
> +static int sdhci_arasan_suspend_phy(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_exit(phy);
> + if (ret < 0)
> + goto err_phy_exit;
> +
> + ret = phy_power_off(phy);
> + if (ret < 0)
> + goto err_phy_pwr_off;
> +
> + return 0;
> +
> +err_phy_pwr_off:
> + phy_power_on(phy);
> +err_phy_exit:
> + phy_init(phy);
> + return ret;
> +}
> +
> +/**
> + * sdhci_arasan_resume_phy - Resume phy method for the driver
> + * @phy:Handler of phy structure
> + * Returns 0 on success and error value on error
> + *
> + * Put the phy in a active state.
> + */
> +static int sdhci_arasan_resume_phy(struct phy *phy)
> +{
> + int ret;
> +
> + ret = phy_power_on(phy);
> + if (ret < 0)
> + goto err_phy_pwr_on;
> +
> + ret = phy_init(phy);
> + if (ret < 0)
> + goto err_phy_init;
> +
> + return 0;
> +
> +err_phy_init:
> + phy_exit(phy);
> +err_phy_pwr_on:
> + phy_power_off(phy);
> + return ret;
> +}
> +
> +/**
> * sdhci_arasan_suspend - Suspend method for the driver
> * @dev: Address of the device structure
> * Returns 0 on success and error value on error
> @@ -91,6 +149,12 @@ static int sdhci_arasan_suspend(struct device *dev)
> clk_disable(pltfm_host->clk);
> clk_disable(sdhci_arasan->clk_ahb);
>
> + if (sdhci_arasan->phy) {
> + ret = sdhci_arasan_suspend_phy(sdhci_arasan->phy);
> + if (ret < 0)
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -122,6 +186,12 @@ static int sdhci_arasan_resume(struct device *dev)
> return ret;
> }
>
> + if (sdhci_arasan->phy) {
> + ret = sdhci_arasan_resume_phy(sdhci_arasan->phy);
> + if (ret < 0)
> + return ret;
> + }
> +
> return sdhci_resume_host(host);
> }
> #endif /* ! CONFIG_PM_SLEEP */
> @@ -166,6 +236,33 @@ static int sdhci_arasan_probe(struct platform_device
> *pdev)
> goto clk_dis_ahb;
> }
>
> + sdhci_arasan->phy = NULL;
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "arasan,sdhci-5.1")) {
> + sdhci_arasan->phy = devm_phy_get(&pdev->dev,
> +"phy_arasan");
> + if (IS_ERR(sdhci_arasan->phy)) {
> + ret = -ENODEV;
> + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
> + goto clk_dis_ahb;
> + }
> +
> + ret = phy_power_on(sdhci_arasan->phy);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "phy_power_on err.\n");
> + phy_power_off(sdhci_arasan->phy);
> + goto clk_dis_ahb;
> + }
> +
> + ret = phy_init(sdhci_arasan->phy);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "phy_init err.\n");
> + phy_exit(sdhci_arasan->phy);
> + phy_power_off(sdhci_arasan->phy);
> + goto clk_di
Re: [PATCH-v2 0/3] mmc: sdhci-pxav3: Fix tabbing issue
On 7 September 2015 at 13:31, Vaibhav Hiremath wrote: > Trivial patch-series, which fixes the tabbing issue in the driver, > uses the BIT macro for bit fields and prints notice on -EPROBE_DEFER > in sdhci_add_host() function on regulator unavailability. > > V1 => V2 > > - Fixed all comments from Joe, mostly alignment changes > - Separated BIT macro usage patch into new one. > - changed error to kernel notice for EPROBE_DEFER in sdhci_add_host() > > Note: This patch-series should get merged before another series - > > [PATCH-v2 0/7] mmc: sdhci-pxav3: Enable support for PXA1928 SDCHI controller > > Vaibhav Hiremath (3): > mmc: sdhci-pxav3: Fix tabbing issue > mmc: sdhci-pxav3: Use BIT macro for bit field definitions > mmc: sdhci: print notice on -EPROBE_DEFER in sdhci_add_host() fn > > drivers/mmc/host/sdhci-pxav3.c | 46 > +- > drivers/mmc/host/sdhci.c | 5 - > 2 files changed, 27 insertions(+), 24 deletions(-) > > -- > 1.9.1 > Hi Vaibhav, FYI, I won't be picking up any of these patches. Primarily because I don't think they improves the code and other people also seems to agree to that. Regarding patch1 and similar patches which deals with only fixing checkpatch warnings/errors. In most cases I don't like such changes, as they makes it harder to use "git blame" when you want to find out which commit that introduced a change. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v3 1/3] mmc: sprd: Add MMC host driver for Spreadtrum SoC
On 14 August 2015 at 18:55, Hongtao Wu wrote:
> the Spreadtrum MMC host driver is used to supply EMMC, SD, and
> SDIO types of memory cards
Perhaps some more information about the controller. Are there any
specific features it support or doesn't support!?
Moreover it would be nice to know a bit more what this patch contains.
For example, you have some debugfs support, what's that?
>
> Signed-off-by: Billows Wu(WuHongtao)
> ---
> drivers/mmc/host/Kconfig |6 +
> drivers/mmc/host/Makefile |1 +
> drivers/mmc/host/sprd_sdhost.c | 1202
>
> drivers/mmc/host/sprd_sdhost.h | 615
Just by looking at number of lines here, it tells me that the size of
header file is just way too big. It's half the size of the c-file,
that just can't be right. :-)
Just to make it clear. I don't like "one-liner" wrapper functions nor
macros. If you remove these kind of stuff, the total size would shrink
significantly I belive.
> drivers/mmc/host/sprd_sdhost_debugfs.c | 212 ++
> drivers/mmc/host/sprd_sdhost_debugfs.h | 27 +
I had a brief look at the debugfs support, let me suggest that we
handle that in a separate patch.
Perhaps, some of that code can also be made more generic and used by
the mmc core instead.
> 6 files changed, 2063 insertions(+)
> create mode 100644 drivers/mmc/host/sprd_sdhost.c
> create mode 100644 drivers/mmc/host/sprd_sdhost.h
> create mode 100644 drivers/mmc/host/sprd_sdhost_debugfs.c
> create mode 100644 drivers/mmc/host/sprd_sdhost_debugfs.h
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index fd9a58e..c43d938 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -264,6 +264,12 @@ config MMC_SDHCI_SPEAR
>
> If you have a controller with this interface, say Y or M here.
>
> +config SPRD_MMC_SDHOST
> + tristate "Spreadtrum SDIO host Controller support"
> + help
> + This selects the SDIO Host Controller in spreadtrum platform
> +
> + If you have a controller with this interface, say Y or M here.
> If unsure, say N.
>
> config MMC_SDHCI_S3C_DMA
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index e928d61..e00227f 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_MMC_SDHCI_BCM2835) +=
> sdhci-bcm2835.o
> obj-$(CONFIG_MMC_SDHCI_IPROC) += sdhci-iproc.o
> obj-$(CONFIG_MMC_SDHCI_MSM)+= sdhci-msm.o
> obj-$(CONFIG_MMC_SDHCI_ST) += sdhci-st.o
> +obj-$(CONFIG_SPRD_MMC_SDHOST) += sprd_sdhost.o sprd_sdhost_debugfs.o
>
> ifeq ($(CONFIG_CB710_DEBUG),y)
> CFLAGS-cb710-mmc+= -DDEBUG
> diff --git a/drivers/mmc/host/sprd_sdhost.c b/drivers/mmc/host/sprd_sdhost.c
> new file mode 100644
> index 000..95639a3
> --- /dev/null
> +++ b/drivers/mmc/host/sprd_sdhost.c
> @@ -0,0 +1,1202 @@
> +/*
> + * linux/drivers/mmc/host/sprd_sdhost.c - Secure Digital Host Controller
> + * Interface driver
> + *
> + * Copyright (C) 2015 Spreadtrum corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or (at
> + * your option) any later version.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "sprd_sdhost.h"
> +#include "sprd_sdhost_debugfs.h"
> +
> +#define DRIVER_NAME "sdhost"
> +#define SDHOST_CAPS \
Please remove this define it makes it harder to read the code.
> + (MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | \
> + MMC_CAP_ERASE | MMC_CAP_UHS_SDR50 | \
> + MMC_CAP_CMD23 | MMC_CAP_HW_RESET)
> +
> +struct sdhost_caps_data {
> + char *name;
> + u32 ocr_avail;
> + u32 caps;
> + u32 caps2;
> + u32 pm_caps;
> + u32 base_clk;
> + u32 signal_default_voltage;
> +};
I don't think you need this struct, as most of this data is already
present it struct mmc_host. And for that small pieces that isn't you
might as well just add into your struct sdhost_host.
> +
> +struct sdhost_caps_data caps_info_map[] = {
This is the wrong way of how to assign capabilties. Instead
mmc_of_parse() parses your DT configuration and assign the
corresponding caps in the struct mmc_host.
> + {
> + .name = "sd",
> + .ocr_avail = MMC_VDD_29_30 | MMC_VDD_30_31,
ocr_avail should be fetched from external regulators via
mmc_regulator_get_supply() API. I don't think you need this at all
then, right?
> + .caps = SDHOST_CAPS,
> + .caps2 = MMC_CAP2_HC_ERASE_SZ,
> + .signal_default_voltage = 300,
Again, if this is
Re: [RFC PATCH v3 0/3] Add MMC host driver for Spreadtrum SoC
Hi Hongtao,
On 14 August 2015 at 18:55, Hongtao Wu wrote:
> This patch adds MMC host driver for Spreadtrum SoC.
> The following coding style may be not meet kernel coding style.
I don't think you have any reason to why you shouldn't follow the
kernel coding style. Please change!
> I am not sure this kind of coding style is better or worse.
> 1) A macro that represent some bits of a register is added a prefix "__",
> for example:
> #define SDHOST_16_HOST_CTRL_2 0x3E
> #define __TIMING_MODE_SDR12 0x
> #define __TIMING_MODE_SDR25 0x0001
> #define __TIMING_MODE_SDR50 0x0002
> I think it is more useful to distinguish a register from a bit of this
> register.
> 2) A function in order to operate a register is also added a prefix "_".
> If the functions(A) call other function(B), we added a prefix "__" before
> B,
> for example:
> static inline void _sdhost_enable_int(struct sdhost_host *host, u32 mask)
> {
> __local_writel(mask, host, SDHOST_32_INT_ST_EN);
> __local_writel(mask, host, SDHOST_32_INT_SIG_EN);
> }
> I think this make the relationship of the function call more explicit.
>
> Changes in v3:
> - add Spreadtrum MMC DT bindings.
> - add MMC nodes in Spreadtrum DT files
> - release resources when there is an error or removing MMC host driver.
>
> Changes in v2:
> - delete some redundant mdelay()
> - add error handling in some functions.
>
> Billows Wu(HongtaoWu) (3):
> mmc: sprd: add MMC host driver for Spreadtrum SoC
> Documentation: add Spreadtrum MMC DT bindings.
> DT: add MMC nodes in Spreadtrum DT files.
>
> Documentation/devicetree/bindings/mmc/sprd-mmc.txt | 46 +
> arch/arm64/boot/dts/sprd/sc9836-openphone.dts | 24 +
> arch/arm64/boot/dts/sprd/sharkl64.dtsi | 44 +
> drivers/mmc/host/Kconfig |6 +
> drivers/mmc/host/Makefile |1 +
> drivers/mmc/host/sprd_sdhost.c | 1202
>
> drivers/mmc/host/sprd_sdhost.h | 615 ++
> drivers/mmc/host/sprd_sdhost_debugfs.c | 212
> drivers/mmc/host/sprd_sdhost_debugfs.h | 27 +
> 9 files changed, 2177 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/sprd-mmc.txt
> create mode 100644 drivers/mmc/host/sprd_sdhost.c
> create mode 100644 drivers/mmc/host/sprd_sdhost.h
> create mode 100644 drivers/mmc/host/sprd_sdhost_debugfs.c
> create mode 100644 drivers/mmc/host/sprd_sdhost_debugfs.h
>
> --
> 1.7.9.5
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 5/7] staging: board: Add support for devices with complex dependencies
On 4 September 2015 at 17:03, Geert Uytterhoeven wrote:
> Hi Ulf,
>
> On Fri, 4 Sep 2015, Ulf Hansson wrote:
>> On 3 September 2015 at 15:35, Geert Uytterhoeven
>> wrote:
>> > On Thu, Sep 3, 2015 at 2:53 PM, Ulf Hansson wrote:
>> >> On 17 June 2015 at 10:38, Geert Uytterhoeven
>> >> wrote:
>> >>> Add support for easy registering of one ore more platform devices that
>> >>> may:
>> >>> - need clocks that are described in DT,
>> >>> - be part of a PM Domain.
>> >
>> >>> diff --git a/drivers/staging/board/board.c
>> >>> b/drivers/staging/board/board.c
>> >>> index 8712f566b31196e0..29d456e29f38feac 100644
>> >>> --- a/drivers/staging/board/board.c
>> >>> +++ b/drivers/staging/board/board.c
>> >
>> >>> +int __init board_staging_register_device(const struct board_staging_dev
>> >>> *dev)
>> >>> +{
>> >>> + struct platform_device *pdev = dev->pdev;
>> >>> + unsigned int i;
>> >>> + int error;
>> >>> +
>> >>> + pr_debug("Trying to register device %s\n", pdev->name);
>> >>> + if (board_staging_dt_node_available(pdev->resource,
>> >>> + pdev->num_resources)) {
>> >>> + pr_warn("Skipping %s, already in DT\n", pdev->name);
>> >>> + return -EEXIST;
>> >>> + }
>> >>> +
>> >>> + board_staging_gic_fixup_resources(pdev->resource,
>> >>> pdev->num_resources);
>> >>> +
>> >>> + for (i = 0; i < dev->nclocks; i++)
>> >>> + board_staging_register_clock(&dev->clocks[i]);
>> >>> +
>> >>> + error = platform_device_register(pdev);
>> >>> + if (error) {
>> >>> + pr_err("Failed to register device %s (%d)\n", pdev->name,
>> >>> + error);
>> >>> + return error;
>> >>> + }
>> >>> +
>> >>> + if (dev->domain)
>> >>> + __pm_genpd_name_add_device(dev->domain, &pdev->dev,
>> >>> NULL);
>> >>
>> >> Urgh, this managed to slip through my filters.
>> >>
>> >> It seems like we almost managed to remove all users of the
>> >> "..._name_add..." APIs for genpd. If hasn't been for $subject patch.
>> >> :-)
>> >>
>> >> Now, I realize this is already too late here, but let's try to fix
>> >> this before it turns into a bigger issue.
>> >>
>> >> Geert, do you think it's possible to convert into using the non-named
>> >> bases APIs?
>> >
>> > That will be difficult. This code is meant to use drivers that are not yet
>> > DT-aware on DT-based systems. Hence it uses platform devices with named PM
>> > domains, while the PM domains are described in DT.
>> > I don't think there's another way to look up a PM domain by name, is there?
>>
>> As a matter of fact there are, especially for those genpds that has
>> been created through DT as in this case. The API to use is
>> of_genpd_get_from_provider() to find the struct generic_pm_domain.
>
> Thanks!
>
>> Yes, I do realize that you need to manage the parsing of the domain
>> name to make sure it's the one you want, but I would rather keep that
>> "hack" in this driver than in the generic API.
>
> OK. It turned out not to be that complex, cfr. the patch below.
> For now this supports PM domains with "#power-domain-cells = <0>" only,
> but of course it can be extended if the need arises.
>
> I've also tried:
>
> np = of_find_compatible_node(NULL, NULL, "renesas,sysc-rmobile");
> np = of_find_node_by_name(np, "a4lc");
>
> (the second step is needed because of the domain hierarchy in DT), but that
> would require adding the compatible string to struct board_staging_dev,
> and doesn't work if you have multiple identical power providers.
>
> I hope you like it?
>
> From 5fb11904845eb929a5b3382a9d5153c151cde1cf Mon Sep 17 00:00:00 2001
> From: Geert Uytterhoeven
> Date: Fri, 4 Sep 2015 16:52:33 +0200
> Subject: [PATCH/RFC] staging: board: M
Re: [PATCH v3 0/2] regulator: Fix pbias regulator enable
On 4 September 2015 at 14:00, Kishon Vijay Abraham I wrote: > vsel_reg and enable_reg of the pbias regulator descriptor should actually > have the offset from syscon. > > However after > "ARM: dts: : add minimal l4 bus layout with control module > support" > vsel_reg and enable_reg started to have the absolute address because > of address translation that happens due to pbias node made as the > child node of syscon. This breaks the pbias regulator enable. > > This series adds the 'offset' to be populated in vsel_reg and enable_reg > in the pbias driver itself. > > Changes from v2: > *) Squashed all the dt patches into a single patch > > Changes from v1: > *) Fixed Tony's review comments on adding a 'comment' for adding offset in >the driver and adding a warning for using platform_get_resource. > *) Added Tony's Acked-by. > > Tested these patches against mmc -next in omap4 panda, omap3 beagle xm, > dra72 and omap5 uevm > > Kishon Vijay Abraham I (2): > regulator: pbias: program pbias register offset in pbias driver > ARM: dts: : use "ti,pbias-" compatible > string for pbias > > .../bindings/regulator/pbias-regulator.txt |7 ++- > arch/arm/boot/dts/dra7.dtsi|2 +- > arch/arm/boot/dts/omap2430.dtsi|2 +- > arch/arm/boot/dts/omap3.dtsi |2 +- > arch/arm/boot/dts/omap4.dtsi |2 +- > arch/arm/boot/dts/omap5.dtsi |2 +- > drivers/regulator/pbias-regulator.c| 56 > +--- > 7 files changed, 61 insertions(+), 12 deletions(-) > > -- > 1.7.9.5 > Okay, just to be clear on the way forward. I spoked with Mark Brown offlist, and he will/has picked up the regulator patch and will send it as fix for the 4.3 rc[n]. Regarding the ARM patch here, I guess Tony might as well handle it and send through arm-soc, especially since the regression won't be fixed within my mmc tree anyway. So, I am going to leave my next branch as is - and thus relying on that the regression for OMAP will be fixed in some the 4.3 rc[n]. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: ux500: fix typo in regulator names
On 5 August 2015 at 14:22, Tomeu Vizoso wrote:
> Apparently an extra _reg suffix was appended to the end of the names of
> regulators ab8500_ext2_reg and ab8500_ext3_reg.
>
> Signed-off-by: Tomeu Vizoso
You should probably include Linus Walleij as he is the maintainer of ux500.
Acked-by: Ulf Hansson
> ---
> arch/arm/boot/dts/ste-snowball.dts | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/ste-snowball.dts
> b/arch/arm/boot/dts/ste-snowball.dts
> index 32a5ccb14e7e..07c0e346942c 100644
> --- a/arch/arm/boot/dts/ste-snowball.dts
> +++ b/arch/arm/boot/dts/ste-snowball.dts
> @@ -360,11 +360,11 @@
> regulator-name =
> "ab8500-ext-supply1";
> };
>
> - ab8500_ext2_reg_reg: ab8500_ext2 {
> + ab8500_ext2_reg: ab8500_ext2 {
> regulator-name =
> "ab8500-ext-supply2";
> };
>
> - ab8500_ext3_reg_reg: ab8500_ext3 {
> + ab8500_ext3_reg: ab8500_ext3 {
> regulator-name =
> "ab8500-ext-supply3";
> };
> };
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 5/7] staging: board: Add support for devices with complex dependencies
On 3 September 2015 at 15:35, Geert Uytterhoeven wrote:
> Hi Ulf,
>
> On Thu, Sep 3, 2015 at 2:53 PM, Ulf Hansson wrote:
>> On 17 June 2015 at 10:38, Geert Uytterhoeven wrote:
>>> Add support for easy registering of one ore more platform devices that
>>> may:
>>> - need clocks that are described in DT,
>>> - be part of a PM Domain.
>
>>> diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
>>> index 8712f566b31196e0..29d456e29f38feac 100644
>>> --- a/drivers/staging/board/board.c
>>> +++ b/drivers/staging/board/board.c
>
>>> +int __init board_staging_register_device(const struct board_staging_dev
>>> *dev)
>>> +{
>>> + struct platform_device *pdev = dev->pdev;
>>> + unsigned int i;
>>> + int error;
>>> +
>>> + pr_debug("Trying to register device %s\n", pdev->name);
>>> + if (board_staging_dt_node_available(pdev->resource,
>>> + pdev->num_resources)) {
>>> + pr_warn("Skipping %s, already in DT\n", pdev->name);
>>> + return -EEXIST;
>>> + }
>>> +
>>> + board_staging_gic_fixup_resources(pdev->resource,
>>> pdev->num_resources);
>>> +
>>> + for (i = 0; i < dev->nclocks; i++)
>>> + board_staging_register_clock(&dev->clocks[i]);
>>> +
>>> + error = platform_device_register(pdev);
>>> + if (error) {
>>> + pr_err("Failed to register device %s (%d)\n", pdev->name,
>>> + error);
>>> + return error;
>>> + }
>>> +
>>> + if (dev->domain)
>>> + __pm_genpd_name_add_device(dev->domain, &pdev->dev, NULL);
>>
>> Urgh, this managed to slip through my filters.
>>
>> It seems like we almost managed to remove all users of the
>> "..._name_add..." APIs for genpd. If hasn't been for $subject patch.
>> :-)
>>
>> Now, I realize this is already too late here, but let's try to fix
>> this before it turns into a bigger issue.
>>
>> Geert, do you think it's possible to convert into using the non-named
>> bases APIs?
>
> That will be difficult. This code is meant to use drivers that are not yet
> DT-aware on DT-based systems. Hence it uses platform devices with named PM
> domains, while the PM domains are described in DT.
> I don't think there's another way to look up a PM domain by name, is there?
As a matter of fact there are, especially for those genpds that has
been created through DT as in this case. The API to use is
of_genpd_get_from_provider() to find the struct generic_pm_domain.
Yes, I do realize that you need to manage the parsing of the domain
name to make sure it's the one you want, but I would rather keep that
"hack" in this driver than in the generic API.
>
> This code is meant to go away, once all drivers are converted to DT, or
> considered obsolete.
Well, who knows *when* that is going to happen. :-)
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 5/7] staging: board: Add support for devices with complex dependencies
On 17 June 2015 at 10:38, Geert Uytterhoeven wrote:
> Add support for easy registering of one ore more platform devices that
> may:
> - need clocks that are described in DT,
> - be part of a PM Domain.
>
> All these dependencies are optional.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> v2:
> - Drop support for pinctrl, use standard pinctrl in DT instead,
> - Drop support for configured GPIOs, use "gpio-hog" in DT instead,
> - Use clk_add_alias() instead of open coding,
> - Update for changed function names,
> - Drop RFC status.
> ---
> drivers/staging/board/board.c | 56
> +++
> drivers/staging/board/board.h | 20
> 2 files changed, 76 insertions(+)
>
> diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c
> index 8712f566b31196e0..29d456e29f38feac 100644
> --- a/drivers/staging/board/board.c
> +++ b/drivers/staging/board/board.c
> @@ -9,6 +9,7 @@
>
> #define pr_fmt(fmt)"board_staging: " fmt
>
> +#include
> #include
> #include
> #include
> @@ -16,6 +17,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include "board.h"
>
> @@ -118,3 +121,56 @@ void __init board_staging_gic_fixup_resources(struct
> resource *res,
> for (i = 0; i < nres; i++)
> gic_fixup_resource(&res[i]);
> }
> +
> +int __init board_staging_register_clock(const struct board_staging_clk *bsc)
> +{
> + int error;
> +
> + pr_debug("Aliasing clock %s for con_id %s dev_id %s\n", bsc->clk,
> +bsc->con_id, bsc->dev_id);
> + error = clk_add_alias(bsc->con_id, bsc->dev_id, bsc->clk, NULL);
> + if (error)
> + pr_err("Failed to alias clock %s (%d)\n", bsc->clk, error);
> +
> + return error;
> +}
> +
> +int __init board_staging_register_device(const struct board_staging_dev *dev)
> +{
> + struct platform_device *pdev = dev->pdev;
> + unsigned int i;
> + int error;
> +
> + pr_debug("Trying to register device %s\n", pdev->name);
> + if (board_staging_dt_node_available(pdev->resource,
> + pdev->num_resources)) {
> + pr_warn("Skipping %s, already in DT\n", pdev->name);
> + return -EEXIST;
> + }
> +
> + board_staging_gic_fixup_resources(pdev->resource,
> pdev->num_resources);
> +
> + for (i = 0; i < dev->nclocks; i++)
> + board_staging_register_clock(&dev->clocks[i]);
> +
> + error = platform_device_register(pdev);
> + if (error) {
> + pr_err("Failed to register device %s (%d)\n", pdev->name,
> + error);
> + return error;
> + }
> +
> + if (dev->domain)
> + __pm_genpd_name_add_device(dev->domain, &pdev->dev, NULL);
Urgh, this managed to slip through my filters.
It seems like we almost managed to remove all users of the
"..._name_add..." APIs for genpd. If hasn't been for $subject patch.
:-)
Now, I realize this is already too late here, but let's try to fix
this before it turns into a bigger issue.
Geert, do you think it's possible to convert into using the non-named
bases APIs?
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 0/6] regulator: Fix pbias regulator enable
+Olof On 3 September 2015 at 08:50, Kishon Vijay Abraham I wrote: > vsel_reg and enable_reg of the pbias regulator descriptor should actually > have the offset from syscon. > > However after > "ARM: dts: : add minimal l4 bus layout with control module > support" > vsel_reg and enable_reg started to have the absolute address because > of address translation that happens due to pbias node made as the > child node of syscon. This breaks the pbias regulator enable. > > This series adds the 'offset' to be populated in vsel_reg and enable_reg > in the pbias driver itself. > > Changes from v1: > *) Fixed Tony's review comments on adding a 'comment' for adding offset in >the driver and adding a warning for using platform_get_resource. > *) Added Tony's Acked-by. > > Tested these patches against mmc -next in omap4 panda, omap3 beagle xm, > dra72 and omap5 uevm > > Kishon Vijay Abraham I (6): > regulator: pbias: program pbias register offset in pbias driver > ARM: dts: dra7: use "ti,pbias-dra7" compatible string for pbias > ARM: dts: omap243x: use "ti,pbias-omap2" compatible string for pbias > ARM: dts: omap3: use "ti,pbias-omap3" compatible string for pbias > ARM: dts: omap4: use "ti,pbias-omap4" compatible string for pbias > ARM: dts: omap5: use "ti,pbias-omap5" compatible string for pbias > > .../bindings/regulator/pbias-regulator.txt |7 ++- > arch/arm/boot/dts/dra7.dtsi|2 +- > arch/arm/boot/dts/omap2430.dtsi|2 +- > arch/arm/boot/dts/omap3.dtsi |2 +- > arch/arm/boot/dts/omap4.dtsi |2 +- > arch/arm/boot/dts/omap5.dtsi |2 +- > drivers/regulator/pbias-regulator.c| 56 > +--- > 7 files changed, 61 insertions(+), 12 deletions(-) > > -- > 1.7.9.5 > I have recently queued another patchset [1] for the mmc omap driver for 4.3 through my mmc tree for which Olof Johansson reported a regression [2] for Panda ES with multi_v7_defconfig. Kishon, could you please clarify if $subject patchset solves that regression reported by Olof? Or perhaps Olof can run a test? Finally, perhaps it's better if we queue this through my mmc tree since we would then be able to avoid the regression - if I put $subject patchset before [1], right? Then I need an ack from Mark for the regulator patch. Please tell me if you guys prefer another way. Kind regards Uffe [1] http://permalink.gmane.org/gmane.linux.kernel/2027789 [2] http://www.spinics.net/lists/linux-mmc/msg33146.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [v4,1/6] mmc: dt: add DT binding for big endian controller
On 27 August 2015 at 20:39, Li Leo wrote: >> > Or Maybe there is another method, use conditional compilation. The previous >> method would delete the MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER to >> use accessors defined in sdhci-esdhc.h, and add dependency of ARM. This >> method could use 'select if ' to compile LE or BE accessors >> according ARCH. >> >> I don't really follow your suggestion. >> >> Isn't the problem that you need the >> MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER for configurations and for >> some you don't? More precisely, for those where you don't you would >> rather just use the regulator ioread* functions since those will >> internally deal with the endianess in runtime? >> >> If you can make these decisions at compile time an depending of the >> ARCH - I believe I would be fine with that as well. > > I know it is a little bit weird. But the endian of the peripheral device is > not always consistent with the type/endian of the core. Yes got it. So it seems relying on the ARCH in compile time isn't going to be a long term solution. Then it's better to have a default mode and then via DT be able to change that, right? As you suggested above. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [v4,1/6] mmc: dt: add DT binding for big endian controller
[...]
>
> What I had in mind is replacing in_be32() with a esdhc_ops->readl() in this
> function. And the esdhc_ops->readl() would be assigned at the beginning
> according endian.
>
> static u32 esdhc_readl(struct sdhci_host *host, int reg)
> {
> u32 ret;
>
> ret = in_be32(host->ioaddr + reg);
> /*
> * The bit of ADMA flag in eSDHC is not compatible with standard
> * SDHC register, so set fake flag SDHCI_CAN_DO_ADMA2 when ADMA is
> * supported by eSDHC.
> * And for many FSL eSDHC controller, the reset value of field
> * SDHCI_CAN_DO_ADMA1 is one, but some of them can't support ADMA,
> * only these vendor version is greater than 2.2/0x12 support ADMA.
> * For FSL eSDHC, must aligned 4-byte, so use 0xFC to read the
> * the verdor version number, oxFE is SDHCI_HOST_VERSION.
> */
> if ((reg == SDHCI_CAPABILITIES) && (ret & SDHCI_CAN_DO_ADMA1)) {
> u32 tmp = in_be32(host->ioaddr + SDHCI_SLOT_INT_STATUS);
> tmp = (tmp & SDHCI_VENDOR_VER_MASK) >> SDHCI_VENDOR_VER_SHIFT;
> if (tmp > VENDOR_V_22)
> ret |= SDHCI_CAN_DO_ADMA2;
> }
>
> return ret;
> }
>
> The LE and BE accessors could be defined in sdhci-esdhc.h for
> esdhc_ops->readl() as below.
> static u32 esdhc_be32bs_readl(struct sdhci_host *host, int reg)
> {
>return ioread32be(host->ioaddr + reg);
> }
>
> static u32 esdhc_le32bs_readl(struct sdhci_host *host, int reg)
> {
>return ioread32(host->ioaddr + reg);
> }
>
> Do you think it is ok for you?
It becomes a bit "hacky", but currently I can't think of a better solution.
>
>
> Or Maybe there is another method, use conditional compilation. The previous
> method would delete the MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER to use
> accessors defined in sdhci-esdhc.h, and add dependency of ARM. This method
> could use 'select if ' to compile LE or BE accessors
> according ARCH.
I don't really follow your suggestion.
Isn't the problem that you need the
MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER for configurations and for
some you don't? More precisely, for those where you don't you would
rather just use the regulator ioread* functions since those will
internally deal with the endianess in runtime?
If you can make these decisions at compile time an depending of the
ARCH - I believe I would be fine with that as well.
[...]
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [v4,1/6] mmc: dt: add DT binding for big endian controller
On 27 August 2015 at 15:34, Ulf Hansson wrote:
> [...]
>
>>
>> What I had in mind is replacing in_be32() with a esdhc_ops->readl() in this
>> function. And the esdhc_ops->readl() would be assigned at the beginning
>> according endian.
>>
>> static u32 esdhc_readl(struct sdhci_host *host, int reg)
>> {
>> u32 ret;
>>
>> ret = in_be32(host->ioaddr + reg);
>> /*
>> * The bit of ADMA flag in eSDHC is not compatible with standard
>> * SDHC register, so set fake flag SDHCI_CAN_DO_ADMA2 when ADMA is
>> * supported by eSDHC.
>> * And for many FSL eSDHC controller, the reset value of field
>> * SDHCI_CAN_DO_ADMA1 is one, but some of them can't support ADMA,
>> * only these vendor version is greater than 2.2/0x12 support ADMA.
>> * For FSL eSDHC, must aligned 4-byte, so use 0xFC to read the
>> * the verdor version number, oxFE is SDHCI_HOST_VERSION.
>> */
>> if ((reg == SDHCI_CAPABILITIES) && (ret & SDHCI_CAN_DO_ADMA1)) {
>> u32 tmp = in_be32(host->ioaddr + SDHCI_SLOT_INT_STATUS);
>> tmp = (tmp & SDHCI_VENDOR_VER_MASK) >>
>> SDHCI_VENDOR_VER_SHIFT;
>> if (tmp > VENDOR_V_22)
>> ret |= SDHCI_CAN_DO_ADMA2;
>> }
>>
>> return ret;
>> }
>>
>> The LE and BE accessors could be defined in sdhci-esdhc.h for
>> esdhc_ops->readl() as below.
>> static u32 esdhc_be32bs_readl(struct sdhci_host *host, int reg)
>> {
>>return ioread32be(host->ioaddr + reg);
>> }
>>
>> static u32 esdhc_le32bs_readl(struct sdhci_host *host, int reg)
>> {
>>return ioread32(host->ioaddr + reg);
>> }
>>
>> Do you think it is ok for you?
>
> It becomes a bit "hacky", but currently I can't think of a better solution.
>
>>
>>
>> Or Maybe there is another method, use conditional compilation. The previous
>> method would delete the MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER to use
>> accessors defined in sdhci-esdhc.h, and add dependency of ARM. This method
>> could use 'select if ' to compile LE or BE accessors
>> according ARCH.
>
> I don't really follow your suggestion.
>
> Isn't the problem that you need the
> MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER for configurations and for
> some you don't? More precisely, for those where you don't you would
> rather just use the regulator ioread* functions since those will
/s/regulator/regular
> internally deal with the endianess in runtime?
>
> If you can make these decisions at compile time an depending of the
> ARCH - I believe I would be fine with that as well.
>
> [...]
>
> Kind regards
> Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mmc: mediatek: Add online-tuning support of EMMC/SD
On 26 August 2015 at 10:40, Chaotian Jing wrote: > On Tue, 2015-08-25 at 14:07 +0200, Ulf Hansson wrote: >> On 17 August 2015 at 14:01, Chaotian Jing wrote: >> > Hi Ulf, >> > Thanks, please see my comment: >> > On Mon, 2015-08-17 at 13:31 +0200, Ulf Hansson wrote: >> >> On 12 August 2015 at 10:24, Chaotian Jing >> >> wrote: >> >> > Schedule a workqueue to do tuning when CRC error >> >> > Call mmc_hw_reset to re-init card when data timeout >> >> >> >> Thanks to Adrian Hunter, the mmc core already supports re-tuning for >> >> the above scenarios through the mmc_retune_*() APIs. >> >> >> >> SDHCI driver has already adopted to use that feature, you should do >> >> that for the mtk-sd driver as well. >> >> >> >> Kind regards >> >> Uffe >> >> >> > I also noticed that the mmc core already supports re-tuning, but it is >> > not suitable for our host. >> > For EMMC, the CMD21 only support HS200/HS400 mode, for SD card, CMD19 >> > only support SDR50/SDR104 mode, but in our host, even 50Mhz clock >> > frequency may also occur CRC error, Cannot find a parameter that can >> > cover all SD cards which running at 50Mhz, even 25Mhz, will occur CRC >> > error for stress test, DDR50 mode is worse. >> >> I don't follow. You may run for example HS200 in lower speed, nothing >> will prevent tuning and re-tuning from happen for these scenarios. >> >> Or you are talking about other speed modes than HS200/400 and >> SDR50/104? If so, which speed modes are these? >> > Yes, I am talking about other speed modes. > For SD card, the speed mode is High Speed, for EMMC, the speed may be > High Speed or DDR mode. > If CRC error occurs at these speed modes, the MMC core layer will never > handler it. even propagate the -EILSEQ towards to the mmc core, it will > not start mmc_retune. That's true and that's because the MMC/SD/SDIO specs states it. Moreover, there a no tuning ever executed even while initializing such cards. I think you need to elaborate more on what kind of "tuning" your controller needs for these legacy speed modes, can you please do that? Currently the mmc block layer has request retry mechanism when encountering IO errors. That may even reset or power cycle the card, via mmc_hw_reset(). Isn't that sufficient? >> BTW, there are currently a patch being discussed which is about adding >> tuning for DDR mode. Please have look. >> http://www.spinics.net/lists/arm-kernel/msg438434.html >> >> Regarding re-tuning on CRC errors, that's already supported by the mmc >> core. More precisely when a host driver returns -EILSEQ for a request. >> >> > By the way,there are too many tune parameters need try for response, >> > read data, write crc status CRC error, these parameters are >> > multidimensional, it is hard to find a best parameter, and, try >> > thousands of parameters will take long time. >> >> As I see, it's your responsibility from the host driver to propagate >> the proper error code towards the mmc core. If you encounter an error >> that you want to trigger a retune, just return -EILSEQ. >> >> Moreover, if you see a need to extend the tuning/re-tuning support in >> the mmc core to suit your need - I am definitely open to look into >> that. More importantly, I don't want to see host specific hacks trying >> to deal with this. >> > In addition, the CMD21 is only valid for HS200 mode, do not support > HS400, So that there is no chance to tune the HS400 read/write data with > current mmc core layer codes. > As you see, in our platform, tune of HS200 and HS400 is different, but > if use the CMD21, it will never work at HS400 mode, even with > "prepare_hs400_tuning", but it prepare what ? what we need is that > running at HS400 mode and tune the read/write parameters, but not HS200, > because for data Rx path, the tune result of HS200 is meaningless for > HS400. So, should we perhaps add another host_ops callback to satisfy your need for HS400? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 16/16] mmc: host: omap_hsmmc: use "mmc_of_parse_voltage" to get ocr_avail
On 3 August 2015 at 14:26, Kishon Vijay Abraham I wrote:
> From: Roger Quadros
>
> For platforms that doesn't have explicit regulator control in MMC,
> populate voltage-ranges in MMC device tree node and use
> mmc_of_parse_voltage to get ocr_avail
I don't like this.
If we are able to fetch the OCR mask via an external regulator, that
shall be done.
I think the mmc_of_parse_voltage() API and the corresponding DT
binding it parses, should be used for those HW when we don't have an
external regulator to use. For example if the MMC controller itself
somehow controls the voltage levels. Is that really the case for you?
Kind regards
Uffe
>
> Signed-off-by: Roger Quadros
> Signed-off-by: Lokesh Vutla
> Signed-off-by: Murali Karicheri
> Signed-off-by: Franklin S Cooper Jr
> Signed-off-by: Kishon Vijay Abraham I
> ---
> .../devicetree/bindings/mmc/ti-omap-hsmmc.txt |2 ++
> drivers/mmc/host/omap_hsmmc.c |9 -
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index 76bf087..2408e87 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -22,6 +22,8 @@ ti,dual-volt: boolean, supports dual voltage cards
> ti,non-removable: non-removable slot (like eMMC)
> ti,needs-special-reset: Requires a special softreset sequence
> ti,needs-special-hs-handling: HSMMC IP needs special setting for handling
> High Speed
> +voltage-ranges: Specify the voltage range supported if regulator framework
> +isn't enabled.
> dmas: List of DMA specifiers with the controller specific format
> as described in the generic DMA client binding. A tx and rx
> specifier is required.
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 15973f1..d884d8f 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -2184,7 +2184,13 @@ static int omap_hsmmc_probe(struct platform_device
> *pdev)
> goto err_irq;
> }
>
> - mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
> + if (!mmc_pdata(host)->ocr_mask) {
> + ret = mmc_of_parse_voltage(pdev->dev.of_node,
> &mmc->ocr_avail);
> + if (ret)
> + goto err_parse_voltage;
> + } else {
> + mmc->ocr_avail = mmc_pdata(host)->ocr_mask;
> + }
>
> omap_hsmmc_disable_irq(host);
>
> @@ -2224,6 +2230,7 @@ static int omap_hsmmc_probe(struct platform_device
> *pdev)
>
> err_slot_name:
> mmc_remove_host(mmc);
> +err_parse_voltage:
> omap_hsmmc_reg_put(host);
> err_irq:
> device_init_wakeup(&pdev->dev, false);
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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] mmc: sunxi: Don't start commands while the card is busy
On 25 August 2015 at 14:09, Hans de Goede wrote: > Hi, > > > On 25-08-15 14:05, Ulf Hansson wrote: >> >> On 1 August 2015 at 11:01, Hans de Goede wrote: >>> >>> Hi, >>> >>> On 21-07-15 14:15, Ulf Hansson wrote: >>>> >>>> >>>> On 10 July 2015 at 17:14, Hans de Goede wrote: >>>>> >>>>> >>>>> Some sdio wifi modules have not been working reliable with the >>>>> sunxi-mmc >>>>> host code. This turns out to be caused by it starting new commands >>>>> while >>>>> the card signals that it is still busy processing a previous command. >>>> >>>> >>>> >>>> Which are those commands? >>> >>> >>> >>> The code were things get stuck when not waiting for the busy signal uses >>> the following sdio helper functions: >>> >>> sdio_readb/sdio_writeb >>> sdio_f0_readb/sdio_f0_writeb >>> sdio_readw/sdio_writew >>> sdio_readl/sdio_writel >>> sdio_readsb >>> sdio_memcpy_fromio/sdio_memcpy_toio >>> >>> And directly issues the following command: >>> >>> mmc_dat.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ; >>> mmc_cmd.opcode = SD_IO_RW_EXTENDED; >>> mmc_cmd.arg = write ? 1<<31 : 0;/* write flag */ >>> mmc_cmd.arg |= (fn & 0x7) << 28;/* SDIO func num */ >>> mmc_cmd.arg |= 1<<27; /* block mode */ >>> /* for function 1 the addr will be incremented */ >>> mmc_cmd.arg |= (fn == 1) ? 1<<26 : 0; >>> mmc_cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC; >>> >>> I do not know if the busy wait is necessary for all >>> of these, but the hack in the android kernel code, >>> which inserts calls to a wait_card_busy function directly >>> into: drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>> >>> Does so for all of these. >>> >>>> Or more interesting, which response types do >>>> >>>> these commands expect? >>>> I don't think this is a sunxi specific issue, I have seen similar >>>> issues for other host controllers. >>> >>> >>> >>> Agreed, recent dw-mmc patches address the same issue, also involving >>> broadcom sdio wifi chips. >>> >>>> I am thinking that perhaps this should be managed by the mmc core >>>> instead of local hacks to sunxi. Potentially we could make the core to >>>> invoke the already existing host_ops->card_busy() callback when >>>> needed... >>> >>> >>> >>> I'm fine with solving this either way, implementing host_ops->card_busy() >>> for sunxi is easy, and if the core os modified to call that function >>> before issue sdio io ops (which seems to be the common thing here), >>> then that indeed is better then having the sunxi code always do >>> the busy check. >> >> >> Okay, so let's make the core to call the ->card_busy() callback and >> then it's up to each host driver to implement that callback. > > > Ok, do you plan to do a patch for this, or do you expect to get > one submitted to you ? I prefer to get a patch submitted to me. My bandwidth has been limited and still have a lot to catch up with... Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mmc: mediatek: Add online-tuning support of EMMC/SD
On 17 August 2015 at 14:01, Chaotian Jing wrote: > Hi Ulf, > Thanks, please see my comment: > On Mon, 2015-08-17 at 13:31 +0200, Ulf Hansson wrote: >> On 12 August 2015 at 10:24, Chaotian Jing wrote: >> > Schedule a workqueue to do tuning when CRC error >> > Call mmc_hw_reset to re-init card when data timeout >> >> Thanks to Adrian Hunter, the mmc core already supports re-tuning for >> the above scenarios through the mmc_retune_*() APIs. >> >> SDHCI driver has already adopted to use that feature, you should do >> that for the mtk-sd driver as well. >> >> Kind regards >> Uffe >> > I also noticed that the mmc core already supports re-tuning, but it is > not suitable for our host. > For EMMC, the CMD21 only support HS200/HS400 mode, for SD card, CMD19 > only support SDR50/SDR104 mode, but in our host, even 50Mhz clock > frequency may also occur CRC error, Cannot find a parameter that can > cover all SD cards which running at 50Mhz, even 25Mhz, will occur CRC > error for stress test, DDR50 mode is worse. I don't follow. You may run for example HS200 in lower speed, nothing will prevent tuning and re-tuning from happen for these scenarios. Or you are talking about other speed modes than HS200/400 and SDR50/104? If so, which speed modes are these? BTW, there are currently a patch being discussed which is about adding tuning for DDR mode. Please have look. http://www.spinics.net/lists/arm-kernel/msg438434.html Regarding re-tuning on CRC errors, that's already supported by the mmc core. More precisely when a host driver returns -EILSEQ for a request. > By the way,there are too many tune parameters need try for response, > read data, write crc status CRC error, these parameters are > multidimensional, it is hard to find a best parameter, and, try > thousands of parameters will take long time. As I see, it's your responsibility from the host driver to propagate the proper error code towards the mmc core. If you encounter an error that you want to trigger a retune, just return -EILSEQ. Moreover, if you see a need to extend the tuning/re-tuning support in the mmc core to suit your need - I am definitely open to look into that. More importantly, I don't want to see host specific hacks trying to deal with this. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v1] mmc: sdhci-of-arasan: Add the support for sdhci-5.1
On 18 August 2015 at 01:40, Shawn Lin wrote:
> Hi Ulf,
>
>
> On 2015/8/12 13:07, Michal Simek wrote:
>>
>> +linux-mmc
>>
>> On 08/11/2015 04:53 PM, Michal Simek wrote:
>>>
>>> On 08/11/2015 09:46 AM, Shawn Lin wrote:
This patch adds the compatible string in sdhci-of-arasan.c to
support sdhci-arasan5.1 version of controller. No documented
controller IP version is found in the TRM, so we use ths version
of command queueing engine integrated into this controller by arasan
to specify our controller.
Signed-off-by: Shawn Lin
---
Changes in v1:
- Remove redundant SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN for "arasan,
sdhci-5.1" since
SDHCI will check "host->max_clk == 0" and let driver get it from
host->ops->get_max_clock.
Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 2 +-
drivers/mmc/host/sdhci-of-arasan.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 7e94903..da541c3 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -9,7 +9,7 @@ Device Tree Bindings for the Arasan SDHCI Controller
Required Properties:
- compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
-'arasan,sdhci-4.9a'
+'arasan,sdhci-4.9a' or 'arasan,sdhci-5.1'
- reg: From mmc bindings: Register location and length.
- clocks: From clock bindings: Handles to clock inputs.
- clock-names: From clock bindings: Tuple including "clk_xin" and
"clk_ahb"
diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index ef5a7d2..75379cb 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -217,6 +217,7 @@ static int sdhci_arasan_remove(struct
platform_device *pdev)
static const struct of_device_id sdhci_arasan_of_match[] = {
{ .compatible = "arasan,sdhci-8.9a" },
+ { .compatible = "arasan,sdhci-5.1" },
{ .compatible = "arasan,sdhci-4.9a" },
{ }
};
>>>
>>> Acked-by: Michal Simek
>
>
> Could you pull this patch into your repository?
>
Thanks, applied for next!
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 0/3] introduce driver for the Atmel SDMMC
On 29 July 2015 at 16:22, Ludovic Desroches wrote: > Hi, > > This set of patches introduce a driver for the new Atmel SDMMC device avaible > on SAMA5D2 SoC. > > There is also a resend of an old patch which has not been taken. Ulf asked > for some reviews since it could impact all sdhci devices but nobody did it... > This patch is not necessary for patch 2/3. It only fixes a special use case. > If > there are objections about it, drop it, I don't want to delay the Atmel SDMMC > driver inclusion only for this patch. > > Changes: > - from v1: > - update license > > Ludovic Desroches (3): > mmc: sdhci: switch from programmable clock mode to divided one if > needed > mmc: sdhci-of-at91: introduce driver for the Atmel SDMMC > MAINTAINERS: add entry for Atmel sdhci-of-at91 driver > > .../devicetree/bindings/mmc/sdhci-atmel.txt| 21 +++ > MAINTAINERS| 6 + > drivers/mmc/host/Kconfig | 8 + > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-of-at91.c | 192 > + > drivers/mmc/host/sdhci.c | 29 +++- > 6 files changed, 248 insertions(+), 9 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-atmel.txt > create mode 100644 drivers/mmc/host/sdhci-of-at91.c > > -- > 2.5.0 > Thanks, applied all three patches for next! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/6] mmc: imx: a few fixes and new feature
On 11 August 2015 at 13:38, Haibo Chen wrote: > Changes for v6: > -remove duplicate code in esdhc_set_uhs_signaling(). > -fix a typo for patch-2. > -make commit log of patch-3 more specific. > > Haibo Chen (6): > mmc: sdhci-esdhc-imx: add imx7d support and support HS400 > mmc: sdhci-esdhc-imx: add tuning-step setting support > mmc: sdhci-esdhc-imx: add imx7d support in bingding doc > ARM: dts: imx7d-sdb: add eMMC5.0 support > mmc: sdhci-esdhc-imx: set back the burst_length_enable bit to 1 > mmc: sdhci-esdhc-imx: change default watermark level and burst length > > .../devicetree/bindings/mmc/fsl-imx-esdhc.txt | 6 ++ > arch/arm/boot/dts/imx7d-sdb.dts| 13 +++ > drivers/mmc/host/sdhci-esdhc-imx.c | 114 > - > include/linux/platform_data/mmc-esdhc-imx.h| 1 + > 4 files changed, 130 insertions(+), 4 deletions(-) > > -- > 1.9.1 > I have applied this for next - except patch4 as it needs and ack from the IMX SOC maintainer, or it may be better to take that patch through ARM soc... Regarding patch3, which document the new bindings and the compatible string, I moved this to be the first patch and fixed the spelling of the commit message header. Thanks! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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] mmc: sunxi: Don't start commands while the card is busy
On 1 August 2015 at 11:01, Hans de Goede wrote: > Hi, > > On 21-07-15 14:15, Ulf Hansson wrote: >> >> On 10 July 2015 at 17:14, Hans de Goede wrote: >>> >>> Some sdio wifi modules have not been working reliable with the sunxi-mmc >>> host code. This turns out to be caused by it starting new commands while >>> the card signals that it is still busy processing a previous command. >> >> >> Which are those commands? > > > The code were things get stuck when not waiting for the busy signal uses > the following sdio helper functions: > > sdio_readb/sdio_writeb > sdio_f0_readb/sdio_f0_writeb > sdio_readw/sdio_writew > sdio_readl/sdio_writel > sdio_readsb > sdio_memcpy_fromio/sdio_memcpy_toio > > And directly issues the following command: > > mmc_dat.flags = write ? MMC_DATA_WRITE : MMC_DATA_READ; > mmc_cmd.opcode = SD_IO_RW_EXTENDED; > mmc_cmd.arg = write ? 1<<31 : 0;/* write flag */ > mmc_cmd.arg |= (fn & 0x7) << 28;/* SDIO func num */ > mmc_cmd.arg |= 1<<27; /* block mode */ > /* for function 1 the addr will be incremented */ > mmc_cmd.arg |= (fn == 1) ? 1<<26 : 0; > mmc_cmd.flags = MMC_RSP_SPI_R5 | MMC_RSP_R5 | MMC_CMD_ADTC; > > I do not know if the busy wait is necessary for all > of these, but the hack in the android kernel code, > which inserts calls to a wait_card_busy function directly > into: drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c > > Does so for all of these. > >> Or more interesting, which response types do >> >> these commands expect? >> I don't think this is a sunxi specific issue, I have seen similar >> issues for other host controllers. > > > Agreed, recent dw-mmc patches address the same issue, also involving > broadcom sdio wifi chips. > >> I am thinking that perhaps this should be managed by the mmc core >> instead of local hacks to sunxi. Potentially we could make the core to >> invoke the already existing host_ops->card_busy() callback when >> needed... > > > I'm fine with solving this either way, implementing host_ops->card_busy() > for sunxi is easy, and if the core os modified to call that function > before issue sdio io ops (which seems to be the common thing here), > then that indeed is better then having the sunxi code always do > the busy check. Okay, so let's make the core to call the ->card_busy() callback and then it's up to each host driver to implement that callback. As an optimization, we might consider (in a separate step) to add MMC_RSP_BUSY to the response type. I realize that would somewhat be a violation of the spec, but apparently the SDIO spec isn't really clear on this area. > >> Within this context, could I ask whether you controller supports IRQ >> based HW-busy detection? Or you need polling of the status register? > > > I'm afraid that we need to poll the status register. I've been unable > to find an irq flag corresponding to this. Okay, thanks for the info. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [v4,1/6] mmc: dt: add DT binding for big endian controller
On 25 August 2015 at 12:27, Lu Y.B. wrote: > Please see my comments below. > >> -Original Message- >> From: Li Yang-Leo-R58472 >> Sent: Tuesday, August 25, 2015 6:56 AM >> To: Lu Yangbo-B47093 >> Cc: linux-...@vger.kernel.org; ulf.hans...@linaro.org; >> devicetree@vger.kernel.org >> Subject: Re: [v4,1/6] mmc: dt: add DT binding for big endian controller >> >> > Add specification of 'big-endian' property. if the controller is big >> > endian mode, specify this property in device tree node. >> > The default endian mode is little endian. >> >> You shouldn't make the default endian mode to be little endian because in >> the original binding without this new property we are actually expecting >> big-endian hardware. Making little-endian the default is breaking the >> compatibility with the original binding. >> >> Regards, >> Leo > > Thanks a lot, Leo. > > Hi Uffe, > > Since it doesn't make sense to extend the common MMC DT parser, > mmc_of_parse(), to fetch the endian mode, > I'd like to move endian fetching code and all LE/BE accessors definition to > eSDHC driver. Please them parts of the sdhci generic code, as library functions. > > And the default endian would be set to big-endian again for eSDHC. That's okay, just make sure it's properly documented in the DT binding doc for esdhc. > > Use function pointers that are assigned in probe according endian mode to > avoid checking endian every time. > I'd also like to separate eMMC DDR mode patch from this patchset. Would send > it with other UHS-1 speed mode patches together. Makes perfect sense! So in your sdhci host specific driver, you will find out the endian type and make use of the corresponding library functions gets assigned in the host ops which is provided to sdhci_pltfm_init. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 00/16] omap_hsmmc: regulator usage cleanup and fixes
On 3 August 2015 at 14:26, Kishon Vijay Abraham I wrote: > Changes from v1: > *) return on -EPROBE_DEFER and other fatal errors. (Don't return only >if the return value is -ENODEV) > *) Remove the beagle x15 dts patch. It can be part of a different >series. > *) Avoid using regulator_is_enabled for vqmmc since if the regulator >is shared and the other users are not using regulator_is_enabled >then there can be unbalanced regulator_enable/regulator_disable > > This patch series does the following > *) Uses devm_regulator_get_optional() for vmmc and then removes the >CONFIG_REGULATOR check altogether. > *) return on -EPROBE_DEFER and any other fatal errors > *) enable/disable vmmc_aux regulator based on prior state > > I've pushed this patch series to > git://git.ti.com/linux-phy/linux-phy.git mmc_regulator_cleanup_fixes_v2 > > Please note the branch also has the pbias fixes [1] & [2]. > [1] -> https://lkml.org/lkml/2015/7/27/358 > [2] -> https://lkml.org/lkml/2015/7/27/391 > > This series is in preparation for implementing the voltage switch > sequence so that UHS cards can be supported. > > Did basic read/write test in J6, J6 Eco, Beagle-x15, AM437x EVM, > Beaglebone black, OMAP5 uEVM and OMAP4 PANDA. > > Kishon Vijay Abraham I (15): > mmc: host: omap_hsmmc: use devm_regulator_get_optional() for vmmc > mmc: host: omap_hsmmc: return on fatal errors from omap_hsmmc_reg_get > mmc: host: omap_hsmmc: cleanup omap_hsmmc_reg_get() > mmc: host: omap_hsmmc: use the ocrmask provided by the vmmc regulator > mmc: host: omap_hsmmc: use mmc_host's vmmc and vqmmc > mmc: host: omap_hsmmc: remove unnecessary pbias set_voltage > mmc: host: omap_hsmmc: return error if any of the regulator APIs fail > mmc: host: omap_hsmmc: add separate functions for enable/disable > supply > mmc: host: omap_hsmmc: add separate function to set pbias > mmc: host: omap_hsmmc: avoid pbias regulator enable on power off > mmc: host: omap_hsmmc: don't use ->set_power to set initial regulator > state > mmc: host: omap_hsmmc: enable/disable vmmc_aux regulator based on > previous state > mmc: host: omap_hsmmc: use regulator_is_enabled to find pbias status > mmc: host: omap_hsmmc: use ios->vdd for setting vmmc voltage > mmc: host: omap_hsmmc: remove CONFIG_REGULATOR check > > Roger Quadros (1): > mmc: host: omap_hsmmc: use "mmc_of_parse_voltage" to get ocr_avail > > .../devicetree/bindings/mmc/ti-omap-hsmmc.txt |2 + > drivers/mmc/host/omap_hsmmc.c | 340 > +--- > 2 files changed, 224 insertions(+), 118 deletions(-) > > -- > 1.7.9.5 > I tried to apply this on top of my next branch, but it didn't work. Could you please rebase and send a v3!? While you do that, please add Tony's tested-by tag as well. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] mmc: mediatek: Add online-tuning support of EMMC/SD
On 12 August 2015 at 10:24, Chaotian Jing wrote:
> Schedule a workqueue to do tuning when CRC error
> Call mmc_hw_reset to re-init card when data timeout
Thanks to Adrian Hunter, the mmc core already supports re-tuning for
the above scenarios through the mmc_retune_*() APIs.
SDHCI driver has already adopted to use that feature, you should do
that for the mtk-sd driver as well.
Kind regards
Uffe
>
> Signed-off-by: Chaotian Jing
> ---
> drivers/mmc/host/mtk-sd.c | 162
> ++
> 1 file changed, 150 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 7153500..eb44fe1 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -290,6 +291,10 @@ struct msdc_host {
> struct pinctrl_state *pins_default;
> struct pinctrl_state *pins_uhs;
> struct delayed_work req_timeout;
> + struct workqueue_struct *repeat_workqueue;
> + struct work_struct repeat_req;
> + struct mmc_request *repeat_mrq;
> + u32 tune_cmd_counter;
> int irq;/* host interrupt */
>
> struct clk *src_clk;/* msdc source clock */
> @@ -353,7 +358,10 @@ static void msdc_reset_hw(struct msdc_host *host)
> static void msdc_cmd_next(struct msdc_host *host,
> struct mmc_request *mrq, struct mmc_command *cmd);
>
> -static u32 data_ints_mask = MSDC_INTEN_XFER_COMPL | MSDC_INTEN_DATTMO |
> +static const u32 cmd_ints_mask = MSDC_INTEN_CMDRDY | MSDC_INTEN_RSPCRCERR |
> + MSDC_INTEN_CMDTMO | MSDC_INTEN_ACMDRDY |
> + MSDC_INTEN_ACMDCRCERR | MSDC_INTEN_ACMDTMO;
> +static const u32 data_ints_mask = MSDC_INTEN_XFER_COMPL | MSDC_INTEN_DATTMO |
> MSDC_INTEN_DATCRCERR | MSDC_INTEN_DMA_BDCSERR |
> MSDC_INTEN_DMA_GPDCSERR | MSDC_INTEN_DMA_PROTECT;
>
> @@ -690,6 +698,18 @@ static void msdc_request_done(struct msdc_host *host,
> struct mmc_request *mrq)
> msdc_track_cmd_data(host, mrq->cmd, mrq->data);
> if (mrq->data)
> msdc_unprepare_data(host, mrq);
> + if (host->error && host->mmc->card &&
> + !mmc_card_sdio(host->mmc->card)) {
> + if (mrq->cmd->error == (unsigned int)-EILSEQ ||
> + (mrq->stop && mrq->stop->error == (unsigned int)-EILSEQ)
> ||
> + (mrq->sbc && mrq->sbc->error == (unsigned int)-EILSEQ) ||
> + (mrq->data && mrq->data->error)) {
> + host->repeat_mrq = mrq;
> + queue_work(host->repeat_workqueue, &host->repeat_req);
> + return;
> + }
> + }
> + host->tune_cmd_counter = 0;
> mmc_request_done(host->mmc, mrq);
>
> pm_runtime_mark_last_busy(host->dev);
> @@ -725,11 +745,7 @@ static bool msdc_cmd_done(struct msdc_host *host, int
> events,
> if (done)
> return true;
>
> - sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_CMDRDY |
> - MSDC_INTEN_RSPCRCERR | MSDC_INTEN_CMDTMO |
> - MSDC_INTEN_ACMDRDY | MSDC_INTEN_ACMDCRCERR |
> - MSDC_INTEN_ACMDTMO);
> - writel(cmd->arg, host->base + SDC_ARG);
> + sdr_clr_bits(host->base + MSDC_INTEN, cmd_ints_mask);
>
> if (cmd->flags & MMC_RSP_PRESENT) {
> if (cmd->flags & MMC_RSP_136) {
> @@ -819,10 +835,7 @@ static void msdc_start_command(struct msdc_host *host,
> rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
> mod_delayed_work(system_wq, &host->req_timeout, DAT_TIMEOUT);
>
> - sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_CMDRDY |
> - MSDC_INTEN_RSPCRCERR | MSDC_INTEN_CMDTMO |
> - MSDC_INTEN_ACMDRDY | MSDC_INTEN_ACMDCRCERR |
> - MSDC_INTEN_ACMDTMO);
> + sdr_set_bits(host->base + MSDC_INTEN, cmd_ints_mask);
> writel(cmd->arg, host->base + SDC_ARG);
> writel(rawcmd, host->base + SDC_CMD);
> }
> @@ -942,6 +955,8 @@ static bool msdc_data_xfer_done(struct msdc_host *host,
> u32 events,
>
> if (events & MSDC_INT_DATTMO)
> data->error = -ETIMEDOUT;
> + else if (events & MSDC_INT_DATCRCERR)
> + data->error = -EILSEQ;
>
> dev_err(host->dev, "%s: cmd=%d; blocks=%d",
> __func__, mrq->cmd->opcode, data->blocks);
> @@ -1113,8 +1128,8 @@ static void msdc_init_hw(struct msdc_host *host)
>
> writel(0, host->base + MSDC_PAD_TUNE);
> writel(0, host->base + MSDC_IOCON);
> - sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_DDLSEL, 1);
> - writel(0x403c004f, host->base + MSDC_PAT
Re: [PATCH v3 00/16] ARM: shmobile: Add CPG/MSTP Clock Domain
> [6] ARM: shmobile: r7s72100 dtsi: Add CPG/MSTP Clock Domain > [7] ARM: shmobile: r8a7778 dtsi: Add CPG/MSTP Clock Domain > [8] ARM: shmobile: r8a7779 dtsi: Add CPG/MSTP Clock Domain > [9] ARM: shmobile: r8a7790 dtsi: Add CPG/MSTP Clock Domain > [10] ARM: shmobile: r8a7791 dtsi: Add CPG/MSTP Clock Domain > [11] ARM: shmobile: r8a7793 dtsi: Add CPG/MSTP Clock Domain > [12] ARM: shmobile: r8a7794 dtsi: Add CPG/MSTP Clock Domain > [13] drivers: sh: Disable legacy default PM Domain on emev2 > [14] drivers: sh: Disable PM runtime for multi-platform ARM with genpd > [15] clk: shmobile: mstp: Consider "zb_clk" suitable for power management > [16] ARM: shmobile: R-Mobile: Use CPG/MSTP Clock Domain attach/detach > helpers > > .../bindings/clock/renesas,r8a7778-cpg-clocks.txt | 29 ++- > .../bindings/clock/renesas,r8a7779-cpg-clocks.txt | 30 +++- > .../clock/renesas,rcar-gen2-cpg-clocks.txt | 26 ++- > .../bindings/clock/renesas,rz-cpg-clocks.txt | 29 ++- > arch/arm/boot/dts/r7s72100.dtsi| 19 + > arch/arm/boot/dts/r8a7778.dtsi | 22 ++ > arch/arm/boot/dts/r8a7779.dtsi | 23 ++ > arch/arm/boot/dts/r8a7790.dtsi | 79 +-- > arch/arm/boot/dts/r8a7791.dtsi | 81 +-- > arch/arm/boot/dts/r8a7793.dtsi | 7 ++ > arch/arm/boot/dts/r8a7794.dtsi | 28 +++ > arch/arm/mach-shmobile/Kconfig | 2 + > arch/arm/mach-shmobile/pm-rmobile.c| 35 + > drivers/clk/shmobile/clk-mstp.c| 90 > ++ > drivers/clk/shmobile/clk-r8a7778.c | 2 + > drivers/clk/shmobile/clk-r8a7779.c | 2 + > drivers/clk/shmobile/clk-rcar-gen2.c | 2 + > drivers/clk/shmobile/clk-rz.c | 3 + > drivers/sh/pm_runtime.c| 19 ++--- > include/linux/clk/shmobile.h | 12 +++ > 20 files changed, 472 insertions(+), 68 deletions(-) > > -- Unless it's too late; for the series - feel free to add: Reviewed-by: Ulf Hansson Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/2] mmc: host: sdhci: Disable 1.8V signaling for arasan 4.9a version of SDHCI controller.
On 19 June 2015 at 14:28, Suman Tripathi wrote:
> Hi ,
>
> On Fri, Jun 19, 2015 at 5:30 PM, Suman Tripathi wrote:
>> This patch disables the 1.8V signaling for arasan 4.9a version
>> of SDHCI controller with the help SDHCI_QUIRK2_NO_1_8_V quirk.
>>
>> Signed-off-by: Suman Tripathi
>> ---
>> ---
>> drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index 21c0c08..4c99ea4 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -171,7 +171,8 @@ static int sdhci_arasan_probe(struct platform_device
>> *pdev)
>>
>> if (of_device_is_compatible(pdev->dev.of_node, "arasan,sdhci-4.9a"))
>> {
>> host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT;
>> - host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
>> + host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23 |
>> +SDHCI_QUIRK2_NO_1_8_V;
>> }
>
>
> I think I am wrong here. No-1.8v is something related to board
> regulator circuitry not related to IP version. So it should be dts
> driven.
Agree!
So I guess that will change patch1 and this one can be completely dropped?
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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] mmc: sunxi: Don't start commands while the card is busy
On 10 July 2015 at 17:14, Hans de Goede wrote:
> Some sdio wifi modules have not been working reliable with the sunxi-mmc
> host code. This turns out to be caused by it starting new commands while
> the card signals that it is still busy processing a previous command.
Which are those commands? Or more interesting, which response types do
these commands expect?
I don't think this is a sunxi specific issue, I have seen similar
issues for other host controllers.
I am thinking that perhaps this should be managed by the mmc core
instead of local hacks to sunxi. Potentially we could make the core to
invoke the already existing host_ops->card_busy() callback when
needed...
Within this context, could I ask whether you controller supports IRQ
based HW-busy detection? Or you need polling of the status register?
>
> This commit fixes this, thereby fixing the wifi reliability issues on
> the Cubietruck and other sunxi boards using sdio wifi.
>
> Reported-by: Eugene K
> Suggested-by: Eugene K
> Cc: Eugene K
> Cc: Arend van Spriel
> Signed-off-by: Hans de Goede
> ---
> Changes in v2:
> -Properly accredit Eugene K for coming up with the fix for this
> ---
> drivers/mmc/host/sunxi-mmc.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 4d3e1ff..daa90b7 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -289,6 +289,24 @@ static int sunxi_mmc_init_host(struct mmc_host *mmc)
> return 0;
> }
>
> +/* Wait for card to report ready before starting a new cmd */
> +static int sunxi_mmc_wait_card_ready(struct sunxi_mmc_host *host)
> +{
> + unsigned long expire = jiffies + msecs_to_jiffies(500);
> + u32 rval;
> +
> + do {
> + rval = mmc_readl(host, REG_STAS);
> + } while (time_before(jiffies, expire) && (rval &
> SDXC_CARD_DATA_BUSY));
> +
> + if (rval & SDXC_CARD_DATA_BUSY) {
> + dev_err(mmc_dev(host->mmc), "Error R1 ready timeout\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host,
> struct mmc_data *data)
> {
> @@ -383,6 +401,8 @@ static void sunxi_mmc_send_manual_stop(struct
> sunxi_mmc_host *host,
> u32 arg, cmd_val, ri;
> unsigned long expire = jiffies + msecs_to_jiffies(1000);
>
> + sunxi_mmc_wait_card_ready(host);
> +
> cmd_val = SDXC_START | SDXC_RESP_EXPIRE |
> SDXC_STOP_ABORT_CMD | SDXC_CHECK_RESPONSE_CRC;
>
> @@ -597,6 +617,11 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host
> *host, u32 oclk_en)
> {
> unsigned long expire = jiffies + msecs_to_jiffies(250);
> u32 rval;
> + int ret;
> +
> + ret = sunxi_mmc_wait_card_ready(host);
> + if (ret)
> + return ret;
>
> rval = mmc_readl(host, REG_CLKCR);
> rval &= ~(SDXC_CARD_CLOCK_ON | SDXC_LOW_POWER_ON);
> @@ -785,6 +810,13 @@ static void sunxi_mmc_request(struct mmc_host *mmc,
> struct mmc_request *mrq)
> return;
> }
>
> + ret = sunxi_mmc_wait_card_ready(host);
> + if (ret) {
> + mrq->cmd->error = ret;
> + mmc_request_done(mmc, mrq);
> + return;
> + }
> +
> if (data) {
> ret = sunxi_mmc_map_dma(host, data);
> if (ret < 0) {
> --
> 2.4.3
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: dt: update ti,am33xx-hsmmc swakeup workaround
On 7 July 2015 at 19:53, Andreas Fenkart wrote:
> Before 5b83b2234be6733cf the driver was hard coding the wakeup irq to
> be active low. The generic pm wakeirq does not override the active
> high/low parameter, hence it must be specified correctly in the
> device tree.
> Mind that SDIO IRQ is active low as defined in the SDIO specification
>
> Signed-off-by: Andreas Fenkart
Thanks, applied!
Kind regards
Uffe
> ---
> Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> index 76bf087..74166a0 100644
> --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt
> @@ -102,7 +102,7 @@ not every application needs SDIO irq, e.g. MMC cards.
> pinctrl-1 = <&mmc1_idle>;
> pinctrl-2 = <&mmc1_sleep>;
> ...
> - interrupts-extended = <&intc 64 &gpio2 28 0>;
> + interrupts-extended = <&intc 64 &gpio2 28 GPIO_ACTIVE_LOW>;
> };
>
> mmc1_idle : pinmux_cirq_pin {
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver
[...]
>>> +#include
>>
>> clk-provider.h, why?
>
>
> The following is needed.
>
> _clk_get_name(clk)
I see, you need it for for the dev_dbg().
I think you shall use "%pC" as the formatting string for the dev_dbg()
message, since that will take care of printing the clock name for you.
[...]
>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>> power_on)
>>> +{
>>> + int i;
>>> +
>>> + mutex_lock(&pd->pmu->mutex);
>>
>> I don't quite understand why you need a lock, what are you protecting and
>> why?
>
>
> Says:
> [ 3551.678762] mali ffa3.gpu: starting device in power domain 'pd_gpu'
> [ 3551.899180] mali ffa3.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.905958] mali ffa3.gpu: starting device in power domain 'pd_gpu'
> [ 3551.912832] mali ffa3.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.919730] rockchip_pd_power:139: mutex_lock
> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
> [ 3563.827295] rockchip_pd_power:139: mutex_lock
> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
> [ 3563.836216] mali ffa3.gpu: starting device in power domain 'pd_gpu'
> [ 3564.058989] mali ffa3.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.065659] mali ffa3.gpu: starting device in power domain 'pd_gpu'
> [ 3564.072354] mali ffa3.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.079047] rockchip_pd_power:139: mutex_lock
> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
> [ 3611.665726] rockchip_pd_power:139: mutex_lock
> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
> [ 3611.674692] mali ffa3.gpu: starting device in power domain 'pd_gpu'
> [ 3611.899160] mali ffa3.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.905938] mali ffa3.gpu: starting device in power domain 'pd_gpu'
> [ 3611.912818] mali ffa3.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.919689] rockchip_pd_power:139: mutex_lock
> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
> [ 3623.848296] rockchip_pd_power:139: mutex_lock
> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>
>
>
>
>>> +
>>> + if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>> + for (i = 0; i < pd->num_clks; i++)
>>> + clk_enable(pd->clks[i]);
>>> +
>>> + if (!power_on) {
>>> + /* FIXME: add code to save AXI_QOS */
>>> +
>>> + /* if powering down, idle request to NIU first */
>>> + rockchip_pmu_set_idle_request(pd, true);
>>> + }
>>> +
>>> + rockchip_do_pmu_set_power_domain(pd, power_on);
>>> +
>>> + if (power_on) {
>>> + /* if powering up, leave idle mode */
>>> + rockchip_pmu_set_idle_request(pd, false);
>>> +
>>> + /* FIXME: add code to restore AXI_QOS */
>>> + }
>>> +
>>> + for (i = pd->num_clks - 1; i >= 0; i--)
>>> + clk_disable(pd->clks[i]);
>>> + }
>>> +
>>> + mutex_unlock(&pd->pmu->mutex);
>>> + return 0;
>>> +}
>>> +
>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>> +{
>>> + struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
>>> +
>>> + return rockchip_pd_power(pd, true);
>>> +}
>>> +
>>> +static int rockchip_pd_power_off(struct generic_pm_domain *domain)
>>> +{
>>> + struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
>>> +
>>> + return rockchip_pd_power(pd, false);
>>> +}
>>> +
>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>> + struct device *dev)
>>> +{
>>> + struct clk *clk;
>>> + int i;
>>> + int error;
>>> +
>>> + dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>> +
>>> + error = pm_clk_create(dev);
>>> + if (error) {
>>> + dev_err(dev, "pm_clk_create failed %d\n", error);
>>> + return error;
>>> + }
>>> +
>>> + i = 0;
>>> + while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>
>> This loop adds all available device clocks to the PM clock list. I
>> wonder if this could be considered as a common thing and if so, we
>> might want to extend the pm_clk API with this.
>
>
> There are several reasons as follows:
>
> Firstly, the clocks need be turned off to save power when
> the system enter the suspend state. So we need to enumerate the clocks
> in the dts. In order to power domain can turn on and off.
>
> Secondly, the reset-circuit should reset be synchronous on rk3288, then
> sync revoked.
> So we need to enable clocks of all devices.
I think you misinterpreted my previous answer. Instead of fetching all
clocks for a device and manually create the pm_clk list as you do
here, I was suggesting to make this a part of the pm clk API.
I would happily support such an API, but I can't tell what the other
ma
Re: [PATCH v6 0/7] Add Mediatek MMC driver
On 17 June 2015 at 23:01, Paul Gortmaker wrote: > On Tue, Jun 16, 2015 at 4:03 AM, Ulf Hansson wrote: >> On 15 June 2015 at 13:20, Chaotian Jing wrote: >>> This series enables MMC support on the MT8135/MT8173 platform. >>> MT8135 has 5 MMC controllers and MT8173 has 4 MMC controllers. >>> >>> Changes base on Ulf's comments >>> Make hclk mandatory in the driver code >>> Change host->hclk to host->src_clk_freq >>> Add linux/pm.h >>> >>> Chaotian Jing (3): >>> mmc: dt-bindings: add Mediatek MMC bindings >>> mmc: mediatek: Add Mediatek MMC driver. > > This commit breaks when merged to linux-next since it gets > exposed as an implicit module.h user that does not include > that header: > >drivers/mmc/host/mtk-sd.c:1459:1: note: in expansion of macro > 'module_platform_driver' > module_platform_driver(mt_msdc_driver); > ^ >include/linux/device.h:1296:1: error: type defaults to 'int' in > declaration of 'module_init' [-Werror=implicit-int] > module_init(__driver##_init); \ > ^ > > Since the code doesn't exist in mainline, I can't fix it from > within my header cleanup series, so can you please add > an appropriate include of module.h to the above? > > Also, I noticed the Makefile addition from this commit > is whitespace mangled; might as well fix that too if > rebasing to fix the include problem. > > Thanks, > Paul. Paul, appreciate your feedback! I have re-based my next branch and updated the commit according to your suggestions. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/7] Add Mediatek MMC driver
On 15 June 2015 at 13:20, Chaotian Jing wrote: > This series enables MMC support on the MT8135/MT8173 platform. > MT8135 has 5 MMC controllers and MT8173 has 4 MMC controllers. > > Changes base on Ulf's comments > Make hclk mandatory in the driver code > Change host->hclk to host->src_clk_freq > Add linux/pm.h > > Chaotian Jing (3): > mmc: dt-bindings: add Mediatek MMC bindings > mmc: mediatek: Add Mediatek MMC driver > mmc: mediatek: Add PM support for MMC driver > > Eddie Huang (2): > arm64: dts: mediatek: Add MT8173 MMC dts > arm64: mediatek: Add Mediatek MMC support in defconfig > > Yingjoe Chen (2): > ARM: mediatek: dts: Add emmc support to mt8135 > ARM: multi_v7_defconfig: Enable Mediatek MMC support multi-v7 > > Documentation/devicetree/bindings/mmc/mtk-sd.txt | 32 + > arch/arm/boot/dts/mt8135-evbp1.dts | 158 +++ > arch/arm/boot/dts/mt8135.dtsi| 55 + > arch/arm/configs/multi_v7_defconfig |1 + > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 126 ++ > arch/arm64/boot/dts/mediatek/mt8173.dtsi | 45 +- > arch/arm64/configs/defconfig |1 + > drivers/mmc/host/Kconfig |8 + > drivers/mmc/host/Makefile|1 + > drivers/mmc/host/mtk-sd.c| 1461 > ++ > include/linux/mmc/core.h |1 + > 11 files changed, 1888 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/mmc/mtk-sd.txt > create mode 100644 drivers/mmc/host/mtk-sd.c > > -- > 1.8.1.1.dirty > Hi Chaotian, I have applied patch 1->3 for my next branch, which thus includes the mmc driver parts. I could potentially also pick the ARM patches, but then those needs to be acked from the ARM SoC folks. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/5] soc: Mediatek: Add SCPSYS power domain driver
On 9 June 2015 at 10:47, Sascha Hauer wrote:
> This adds a power domain driver for the Mediatek SCPSYS unit.
>
> The System Control Processor System (SCPSYS) has several power
> management related tasks in the system. The tasks include thermal
> measurement, dynamic voltage frequency scaling (DVFS), interrupt
> filter and lowlevel sleep control. The System Power Manager (SPM)
> inside the SCPSYS is for the MTCMOS power domain control.
>
> For now this driver only adds power domain support, the more
> advanced features are not yet supported. The driver implements
> the generic PM domain device tree bindings, the first user will
> most likely be the Mediatek AFE audio driver.
>
> Signed-off-by: Sascha Hauer
> ---
> drivers/soc/mediatek/Kconfig | 9 +
> drivers/soc/mediatek/Makefile| 1 +
> drivers/soc/mediatek/mtk-scpsys.c| 490
> +++
> include/dt-bindings/power/mt8173-power.h | 15 +
> 4 files changed, 515 insertions(+)
> create mode 100644 drivers/soc/mediatek/mtk-scpsys.c
> create mode 100644 include/dt-bindings/power/mt8173-power.h
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index e4f37a3..9a61b54 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -18,3 +18,12 @@ config MTK_PMIC_WRAP
> Say yes here to add support for MediaTek PMIC Wrapper found
> on different MediaTek SoCs. The PMIC wrapper is a proprietary
> hardware to connect the PMIC.
> +
> +config MTK_SCPSYS
> + bool "MediaTek SCPSYS Support"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
How about also depending on "PM" and selecting PM_GENERIC_DOMAINS,
would that work?
> + select REGMAP
> + select MTK_INFRACFG
> + help
> + Say yes here to add support for the MediaTek SCPSYS power domain
> + driver.
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 3fa940f..12998b0 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> +obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c
> b/drivers/soc/mediatek/mtk-scpsys.c
> new file mode 100644
> index 000..b9eed37
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -0,0 +1,490 @@
> +/*
> + * Copyright (c) 2015 Pengutronix, Sascha Hauer
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SPM_VDE_PWR_CON0x0210
> +#define SPM_MFG_PWR_CON0x0214
> +#define SPM_VEN_PWR_CON0x0230
> +#define SPM_ISP_PWR_CON0x0238
> +#define SPM_DIS_PWR_CON0x023c
> +#define SPM_VEN2_PWR_CON 0x0298
> +#define SPM_AUDIO_PWR_CON 0x029c
> +#define SPM_MFG_2D_PWR_CON 0x02c0
> +#define SPM_MFG_ASYNC_PWR_CON 0x02c4
> +#define SPM_USB_PWR_CON0x02cc
> +#define SPM_PWR_STATUS 0x060c
> +#define SPM_PWR_STATUS_2ND 0x0610
> +
> +#define PWR_RST_B_BIT BIT(0)
> +#define PWR_ISO_BITBIT(1)
> +#define PWR_ON_BIT BIT(2)
> +#define PWR_ON_2ND_BIT BIT(3)
> +#define PWR_CLK_DIS_BITBIT(4)
> +
> +#define PWR_STATUS_DISPBIT(3)
> +#define PWR_STATUS_MFG BIT(4)
> +#define PWR_STATUS_ISP BIT(5)
> +#define PWR_STATUS_VDECBIT(7)
> +#define PWR_STATUS_VENC_LT BIT(20)
> +#define PWR_STATUS_VENCBIT(21)
> +#define PWR_STATUS_MFG_2D BIT(22)
> +#define PWR_STATUS_MFG_ASYNC BIT(23)
> +#define PWR_STATUS_AUDIO BIT(24)
> +#define PWR_STATUS_USB BIT(25)
> +
> +enum clk_id {
> + MT8173_CLK_NONE,
> + MT8173_CLK_MM,
> + MT8173_CLK_MFG,
> + MT8173_CLK_MAX = MT8173_CLK_MFG,
> +};
> +
> +struct scp_domain_data {
> + const char *name;
> + u32 sta_mask;
> + int ctl_offs;
> + u32 sram_pdn_bits;
> + u32 sram_pdn_ack_bits;
> + u32 bus_prot_mask;
> + enum clk_id clk_id;
> +};
> +
> +static const struct scp_domain_data scp_domain_data[
Re: [PATCH v5 1/7] mmc: dt-bindings: add Mediatek MMC bindings
On 10 June 2015 at 04:24, Chaotian Jing wrote:
> Document the device-tree binding of Mediatek MMC host
>
> Signed-off-by: Chaotian Jing
> ---
> Documentation/devicetree/bindings/mmc/mtk-sd.txt | 32
>
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/mtk-sd.txt
>
> diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> new file mode 100644
> index 000..a1adfa4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> @@ -0,0 +1,32 @@
> +* MTK MMC controller
> +
> +The MTK MSDC can act as a MMC controller
> +to support MMC, SD, and SDIO types of memory cards.
> +
> +This file documents differences between the core properties in mmc.txt
> +and the properties used by the msdc driver.
> +
> +Required properties:
> +- compatible: Should be "mediatek,mt8173-mmc","mediatek,mt8135-mmc"
> +- interrupts: Should contain MSDC interrupt number
> +- clocks: MSDC source clock, HCLK
> +- clock-names: "source", "hclk"
According to the mmc driver, hclk is treated as an optional clock.
Therefore I think you should list it under an "Optional properties"
section instead.
> +- pinctrl-names: should be "default", "state_uhs"
> +- pinctrl-0: should contain default/high speed pin ctrl
> +- pinctrl-1: should contain uhs mode pin ctrl
> +- vmmc-supply: power to the Core
> +- vqmmc-supply: power to the IO
> +
> +Examples:
> +mmc0: mmc@1123 {
> + compatible = "mediatek,mt8173-mmc", "mediatek,mt8135-mmc";
> + reg = <0 0x1123 0 0x108>;
> + interrupts = ;
> + vmmc-supply = <&mt6397_vemc_3v3_reg>;
> + vqmmc-supply = <&mt6397_vio18_reg>;
> + clocks = <&pericfg CLK_PERI_MSDC30_0>, <&topckgen
> CLK_TOP_MSDC50_0_H_SEL>;
> + clock-names = "source", "hclk";
> + pinctrl-names = "default", "state_uhs";
> + pinctrl-0 = <&mmc0_pins_default>;
> + pinctrl-1 = <&mmc0_pins_uhs>;
> +};
> --
> 1.8.1.1.dirty
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/7] mmc: mediatek: Add PM support for MMC driver
On 10 June 2015 at 04:24, Chaotian Jing wrote: > Add PM support for Mediatek MMC driver > Save/restore registers when PM > > Signed-off-by: Chaotian Jing > --- > drivers/mmc/host/mtk-sd.c | 88 > +-- > 1 file changed, 85 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > index 8de3299..62e966f 100644 > --- a/drivers/mmc/host/mtk-sd.c > +++ b/drivers/mmc/host/mtk-sd.c > @@ -22,6 +22,7 @@ > #include > #include > #include Add pm.h as well. > +#include > #include > #include > [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/7] mmc: mediatek: Add Mediatek MMC driver
[...]
> +static int msdc_drv_probe(struct platform_device *pdev)
> +{
> + struct mmc_host *mmc;
> + struct msdc_host *host;
> + struct resource *res;
> + int ret;
> +
> + if (!pdev->dev.of_node) {
> + dev_err(&pdev->dev, "No DT found\n");
> + return -EINVAL;
> + }
> + /* Allocate MMC host for this device */
> + mmc = mmc_alloc_host(sizeof(struct msdc_host), &pdev->dev);
> + if (!mmc)
> + return -ENOMEM;
> +
> + host = mmc_priv(mmc);
> + ret = mmc_of_parse(mmc);
> + if (ret)
> + goto host_free;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + host->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(host->base)) {
> + ret = PTR_ERR(host->base);
> + goto host_free;
> + }
> +
> + ret = mmc_regulator_get_supply(mmc);
> + if (ret == -EPROBE_DEFER)
> + goto host_free;
> +
> + host->src_clk = devm_clk_get(&pdev->dev, "source");
> + if (IS_ERR(host->src_clk)) {
> + ret = PTR_ERR(host->src_clk);
> + goto host_free;
> + }
> +
> + host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> + if (IS_ERR(host->h_clk)) {
> + /* host->h_clk is optional, Only for MSDC0/3 at MT8173 */
> + dev_dbg(&pdev->dev,
> + "Invalid hclk from the device tree!\n");
> + }
> +
> + host->irq = platform_get_irq(pdev, 0);
> + if (host->irq < 0) {
> + ret = -EINVAL;
> + goto host_free;
> + }
> +
> + host->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(host->pinctrl)) {
> + ret = PTR_ERR(host->pinctrl);
> + dev_err(&pdev->dev, "Cannot find pinctrl!\n");
> + goto host_free;
> + }
> +
> + host->pins_default = pinctrl_lookup_state(host->pinctrl, "default");
> + if (IS_ERR(host->pins_default)) {
> + ret = PTR_ERR(host->pins_default);
> + dev_err(&pdev->dev, "Cannot find pinctrl default!\n");
> + goto host_free;
> + }
> +
> + host->pins_uhs = pinctrl_lookup_state(host->pinctrl, "state_uhs");
> + if (IS_ERR(host->pins_uhs)) {
> + ret = PTR_ERR(host->pins_uhs);
> + dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> + goto host_free;
> + }
> +
> + host->dev = &pdev->dev;
> + host->mmc = mmc;
> + host->hclk = clk_get_rate(host->src_clk);
This looks odd. The clock rate for the source clock is assigned as the
speed of hclk!? Why?
> + /* Set host parameters to mmc */
> + mmc->ops = &mt_msdc_ops;
> + mmc->f_min = host->hclk / (4 * 255);
> +
> + mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> + /* MMC core transfer sizes tunable parameters */
> + mmc->max_segs = MAX_BD_NUM;
> + mmc->max_seg_size = BDMA_DESC_BUFLEN;
> + mmc->max_blk_size = 2048;
> + mmc->max_req_size = 512 * 1024;
> + mmc->max_blk_count = mmc->max_req_size / 512;
> + host->dma_mask = DMA_BIT_MASK(32);
> + mmc_dev(mmc)->dma_mask = &host->dma_mask;
> +
> + host->timeout_clks = 3 * 1048576;
> + host->dma.gpd = dma_alloc_coherent(&pdev->dev,
> + sizeof(struct mt_gpdma_desc),
> + &host->dma.gpd_addr, GFP_KERNEL);
> + host->dma.bd = dma_alloc_coherent(&pdev->dev,
> + MAX_BD_NUM * sizeof(struct mt_bdma_desc),
> + &host->dma.bd_addr, GFP_KERNEL);
> + if (!host->dma.gpd || !host->dma.bd) {
> + ret = -ENOMEM;
> + goto release_mem;
> + }
> + msdc_init_gpd_bd(host, &host->dma);
> + INIT_DELAYED_WORK(&host->req_timeout, msdc_request_timeout);
> + spin_lock_init(&host->lock);
> +
> + platform_set_drvdata(pdev, mmc);
> + msdc_ungate_clock(host);
> + msdc_init_hw(host);
> +
> + ret = devm_request_irq(&pdev->dev, host->irq, msdc_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT, pdev->name, host);
> + if (ret)
> + goto release;
> +
> + ret = mmc_add_host(mmc);
> + if (ret)
> + goto release;
> +
> + return 0;
> +
> +release:
> + platform_set_drvdata(pdev, NULL);
> + msdc_deinit_hw(host);
> + msdc_gate_clock(host);
> +release_mem:
> + if (host->dma.gpd)
> + dma_free_coherent(&pdev->dev,
> + sizeof(struct mt_gpdma_desc),
> + host->dma.gpd, host->dma.gpd_addr);
> + if (host->dma.bd)
> + dma_free_coherent(&pdev->dev,
> + MAX_BD_NUM * sizeof(struct mt_bdma_desc),
> + host->dma.bd, host->dma.bd_addr);
> +host_free:
> + mmc_free_host(m
Re: [PATCH RESEND v7 2/2] mmc: host: sdhci: Add support to disable SDR104/SDR50/DDR50 based on capability register 0.
On 8 June 2015 at 10:37, Ulf Hansson wrote: > [...] > >>> > >>> >Can you test this patch on imx SoC ? >>> > >>> >>> (Your email have some format issue.) >> >> Yeah missed to sent in plain text mode. >> >>> >>> I have tested this patch and it does not break imx SoC. >>> You can add my tag. >>> Tested-by: Dong Aisheng >> >> Thanks Dong !! >> >>> >>> However, it looks to me SDHCI_CAN_VDD_180 is only indicating the host VDD >>> capabiliies, not IO voltage capability. > > I think Dong is correct. I don't think SDHCI_CAN_VDD_180 is not /s /is not /is > related to UHS modes at all. > > At least the name of the field (SDHCI_CAN_VDD_180) indicates it's > about VDD/VCC, the core power and not the IO voltage. > >> Are you sure on this ?? If SDHCI host VDD is 1.8V then the cards are >> also capable to operate at 1.8V ? Didn't understand what you mean by >> IO voltage capability >> >> >>> SD3.0 cards require 1.8v IO voltage support. >>> So should this bit affect SD3.0 support? >> >> The preset value resgister says that SDR modes requires 1.8V and we >> disable the modes based on capability or quirk. > > It requires 1.8V *IO voltage*, not VDD/VCC. > > [...] > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v7 2/2] mmc: host: sdhci: Add support to disable SDR104/SDR50/DDR50 based on capability register 0.
[...] >> > >> >Can you test this patch on imx SoC ? >> > >> >> (Your email have some format issue.) > > Yeah missed to sent in plain text mode. > >> >> I have tested this patch and it does not break imx SoC. >> You can add my tag. >> Tested-by: Dong Aisheng > > Thanks Dong !! > >> >> However, it looks to me SDHCI_CAN_VDD_180 is only indicating the host VDD >> capabiliies, not IO voltage capability. I think Dong is correct. I don't think SDHCI_CAN_VDD_180 is not related to UHS modes at all. At least the name of the field (SDHCI_CAN_VDD_180) indicates it's about VDD/VCC, the core power and not the IO voltage. > Are you sure on this ?? If SDHCI host VDD is 1.8V then the cards are > also capable to operate at 1.8V ? Didn't understand what you mean by > IO voltage capability > > >> SD3.0 cards require 1.8v IO voltage support. >> So should this bit affect SD3.0 support? > > The preset value resgister says that SDR modes requires 1.8V and we > disable the modes based on capability or quirk. It requires 1.8V *IO voltage*, not VDD/VCC. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/7] mmc: mediatek: Add Mediatek MMC driver
On 27 May 2015 at 13:34, Chaotian Jing wrote:
> On Tue, 2015-05-26 at 14:33 +0200, Ulf Hansson wrote:
>> [...]
>>
>> >> >> > +{
>> >> >> > + unsigned long tmo = jiffies + msecs_to_jiffies(20);
>> >> >> > +
>> >> >> > + while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
>> >> >> > + && time_before(jiffies, tmo))
>> >> >> > + continue;
>> >> >> > +
>> >> >> > + if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
>> >> >> > + dev_err(host->dev, "CMD bus busy detected\n");
>> >> >> > + host->error |= REQ_CMD_BUSY;
>> >> >> > + msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
>> >> >> > + return false;
>> >> >> > + }
>> >> >> > +
>> >> >> > + if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
>> >> >> > + /* R1B or with data, should check SDCBUSY */
>> >> >> > + while (readl(host->base + SDC_STS) & SDC_STS_SDCBUSY)
>> >> >> > + cpu_relax();
>> >> >> > + }
>> >> >>
>> >> >> MSDC seems to be handling card busy detection in HW, right?
>> >> >>
>> >> > Do not have this ability, HW only know if CMD/DAT is low, but do not
>> >> > have any interrupt for it,
>> >>
>> >> I see, but doesn't the above polling mean that msdc will not propagate
>> >> the response until the card have stopped signal busy? That's what
>> >> MMC_CAP_WAIT_WHILE_BUSY shall be used for.
>> >>
>> > As you see, we only check the "busy state" BEFORE issue a R1B command or
>> > with data command, but do not check if AFTER the request was done, that
>> > would do not match the "MMC_CAP_WAIT_WHILE_BUSY"(eg. CMD5 to sleep)
>>
>> Okay, fair enough. Still, I don't understand why you want to do that?
>>
> Below is the description of these 2 bits:
> 1 CMDBSY CMDBUSY SD Command line busy status
> S/W should always read this bit to make sure the command line is not
> busy before sending the next command.
> If the command is R1B or data read/write command, S/W should check
> SDCBUSY bit too.
> Note: When Auto command 12 is enabled, this bit will be asserted
> immediately after SDC_CMD is written and de-asserted after auto-command
> 12 finishes.
> 1'b0: No transmission is going on CMD line on SD bus
> 1'b1: There exists transmission going on CMD line on SD bus
> 0 SDCBSY SDCBUSY SD controller busy status
> 1'b0: SD controller is idle
> 1'b1: SD controller is busy
>
Thanks, for elaborating. I get it know. :-)
>> > In addition, about CMD5, I find that the suspend/resume flow of EMMC is
>> > stranger in new kernel version, when suspend, it may issue CMD5 to enter
>> > sleep mode, then power off MMC, but when resume, it will
>> > re-initialization, So that why need do the redundant CMD5 in suspend ?
>>
>> Both CMD0 and CMD5 are valid as wakeup commands.
>>
>> To be able to use CMD5, the VCCQ regulator must be kept enabled in the
>> sleep state. That's when it becomes a bit tricky, due to the range of
>> different host drivers and SoCs for how VCCQ is managed.
>>
>> To be safe we have chosen to use CMD0, since it works for *all* cases
>> no matter if VCCQ get gated or not.
>>
>> Moreover, using CMD5 as the wakeup command would require added
>> complexity in the code dealing with suspend/resume. I don't think the
>> effort is worth it, at least until someone has proven that the resume
>> time is greatly decreased by using CMD5.
>>
>> >
>> >> Perhaps you should remove the above polling, and rely on the MMC core
>> >> to poll with CMD13 instead?
>> > before any read/write command, core will issue CMD13 to confirm card
>> > status, here is just only do double confirm to avoid HW issue.
>>
>> What HW issue and why do you need to double confirm? It seems strange.
> Some fake SD card, even the CMD13 get card status is in transfer state,
> but when we do read/write, the data0 was pull low.
Okay. So in this case your mmc host driver will take care of this. That's fine.
My concern is then how other mmc host drivers will deal with these
kind of cards that violates the spec. It would be nice if you could
add a card quirk for these cards, if you still recall what cards that
were causing these issues.
>
> By the way, do you think need do some spinlock protect in our code ?
> I cannot find a good reason to use spin lock, it seems no need
> it,although there will be many thread(mmcqd, mmcqd/0boot1,) and the irq
> handler.
>From mmc core layer point of view, you shouldn't be needing any spinlocks.
Whether your driver has a potential concurrent access of the same
registers, I don't know. If that's the case you probably want to
protect this from happening.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver
On 24 April 2015 at 10:07, Caesar Wang wrote:
> In order to meet high performance and low power requirements, a power
> management unit is designed or saving power when RK3288 in low power
> mode.
> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>
> Signed-off-by: jinkun.hong
> Signed-off-by: Caesar Wang
> ---
>
> arch/arm/mach-rockchip/Kconfig | 1 +
> arch/arm/mach-rockchip/Makefile | 1 +
> arch/arm/mach-rockchip/pm_domains.c | 506
>
> 3 files changed, 508 insertions(+)
> create mode 100644 arch/arm/mach-rockchip/pm_domains.c
>
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index ae4eb7c..578206b 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
> select ROCKCHIP_TIMER
> select ARM_GLOBAL_TIMER
> select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> + select PM_GENERIC_DOMAINS if PM
> help
> Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
> containing the RK2928, RK30xx and RK31xx series.
> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
> index 5c3a9b2..9c902d3 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -1,5 +1,6 @@
> CFLAGS_platsmp.o := -march=armv7-a
>
> obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
> obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
> obj-$(CONFIG_SMP) += headsmp.o platsmp.o
> diff --git a/arch/arm/mach-rockchip/pm_domains.c
> b/arch/arm/mach-rockchip/pm_domains.c
> new file mode 100644
> index 000..92d2569
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/pm_domains.c
> @@ -0,0 +1,506 @@
> +/*
> + * Rockchip Generic power domain support.
> + *
> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
clk-provider.h, why?
> +#include
> +#include
> +#include
Same comment as in patch1. I don't find this header.
> +
> +struct rockchip_domain_info {
> + int pwr_mask;
> + int status_mask;
> + int req_mask;
> + int idle_mask;
> + int ack_mask;
> +};
> +
> +struct rockchip_pmu_info {
> + u32 pwr_offset;
> + u32 status_offset;
> + u32 req_offset;
> + u32 idle_offset;
> + u32 ack_offset;
> +
> + u32 core_pwrcnt_offset;
> + u32 gpu_pwrcnt_offset;
> +
> + unsigned int core_power_transition_time;
> + unsigned int gpu_power_transition_time;
> +
> + int num_domains;
> + const struct rockchip_domain_info *domain_info;
> +};
> +
> +struct rockchip_pm_domain {
> + struct generic_pm_domain genpd;
> + const struct rockchip_domain_info *info;
> + struct rockchip_pmu *pmu;
> + int num_clks;
> + struct clk *clks[];
> +};
> +
> +struct rockchip_pmu {
> + struct device *dev;
> + struct regmap *regmap;
> + const struct rockchip_pmu_info *info;
> + struct mutex mutex; /* mutex lock for pmu */
> + struct genpd_onecell_data genpd_data;
> + struct generic_pm_domain *domains[];
> +};
> +
> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain,
> genpd)
> +
> +#define DOMAIN(pwr, status, req, idle, ack)\
> +{ \
> + .pwr_mask = BIT(pwr), \
> + .status_mask = BIT(status), \
> + .req_mask = BIT(req), \
> + .idle_mask = BIT(idle), \
> + .ack_mask = BIT(ack), \
> +}
> +
> +#define DOMAIN_RK3288(pwr, status, req)\
> + DOMAIN(pwr, status, req, req, (req) + 16)
> +
> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
> +{
> + struct rockchip_pmu *pmu = pd->pmu;
> + const struct rockchip_domain_info *pd_info = pd->info;
> + unsigned int val;
> +
> + regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
> + return (val & pd_info->idle_mask) == pd_info->idle_mask;
> +}
> +
> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
> +bool idle)
> +{
> + const struct rockchip_domain_info *pd_info = pd->info;
> + struct rockchip_pmu *pmu = pd->pmu;
> + unsigned int val;
> +
> + regmap_update_bits(pmu->regmap, pmu->info->req_offset,
> + pd_info->req_mask, idle ? -1U : 0);
> +
> + dsb();
> +
> + do {
> + regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
> + } while ((val & pd_info->ack_mask) != (idle ? pd_info
Re: [PATCH v14 1/3] dt-bindings: add document of Rockchip power domain
On 24 April 2015 at 10:07, Caesar Wang wrote:
> This add the necessary binding documentation for the power domain
> found on Rockchip Socs.
>
> Signed-off-by: jinkun.hong
> Signed-off-by: Caesar Wang
> ---
>
> .../bindings/arm/rockchip/power_domain.txt | 48
> ++
> 1 file changed, 48 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> new file mode 100644
> index 000..3e74e6d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> @@ -0,0 +1,48 @@
> +* Rockchip Power Domains
> +
> +Rockchip processors include support for multiple power domains which can be
> +powered up/down by software based on different application scenes to save
> power.
> +
> +Required properties for power domain controller:
> +- compatible: should be one of the following.
> +* rockchip,rk3288-power-controller - for rk3288 type power domain.
> +- #power-domain-cells: Number of cells in a power-domain specifier.
> + should be 1.
> +- rockchip,pmu: phandle referencing a syscon providing the pmu registers
> +- #address-cells: should be 1.
> +- #size-cells: should be 0.
> +
> +Required properties for power domain sub nodes:
> +- reg: index of the power domain, should use macros in:
> +* include/dt-bindings/power-domain/rk3288.h - for rk3288 type power
> domain.
I can't find the above file, nor is it being adding in $subject patch.
Moreover, there are already
"include/dt-bindings/arm/ux500_pm_domains.h". I suppose we could move
that file into your suggested path, since it seems more generic?
> +- clocks (optional): phandles to clocks which need to be enabled while power
> domain
> + switches state.
> +
> +Example:
> +
> + power: power-controller {
> + compatible = "rockchip,rk3288-power-controller";
> + #power-domain-cells = <1>;
> + rockchip,pmu = <&pmu>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pd_gpu {
> + reg = ;
> + clocks = <&cru ACLK_GPU>;
> + };
> + };
> +
> +Node of a device using power domains must have a power-domains property,
> +containing a phandle to the power device node and an index specifying which
> +power domain to use.
> +The index should use macros in:
> + * include/dt-bindings/power-domain/rk3288.h - for rk3288 type power
> domain.
Same comment as above.
> +
> +Example of the node using power domain:
> +
> + node {
> + /* ... */
> + power-domains = <&power RK3288_PD_GPU>;
> + /* ... */
> + };
> --
> 1.9.1
>
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/13] mmc: davinci_mmc: Support for deferred probing when requesting DMA channels
On 26 May 2015 at 15:26, Peter Ujfalusi wrote:
> Switch to use ma_request_slave_channel_compat_reason() to request the DMA
> channels. Only fall back to pio mode if the error code returned is not
> -EPROBE_DEFER, otherwise return from the probe with the -EPROBE_DEFER.
>
> Signed-off-by: Peter Ujfalusi
> CC: Ulf Hansson
Acked-by: Ulf Hansson
Kind regards
Uffe
> ---
> drivers/mmc/host/davinci_mmc.c | 26 +++---
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
> index b2b3f8bbfd8c..df81e4e2f662 100644
> --- a/drivers/mmc/host/davinci_mmc.c
> +++ b/drivers/mmc/host/davinci_mmc.c
> @@ -530,20 +530,20 @@ static int __init davinci_acquire_dma_channels(struct
> mmc_davinci_host *host)
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
>
> - host->dma_tx =
> - dma_request_slave_channel_compat(mask, edma_filter_fn,
> - &host->txdma, mmc_dev(host->mmc), "tx");
> - if (!host->dma_tx) {
> + host->dma_tx = dma_request_slave_channel_compat_reason(mask,
> + edma_filter_fn, &host->txdma,
> + mmc_dev(host->mmc), "tx");
> + if (IS_ERR(host->dma_tx)) {
> dev_err(mmc_dev(host->mmc), "Can't get dma_tx channel\n");
> - return -ENODEV;
> + return PTR_ERR(host->dma_tx);
> }
>
> - host->dma_rx =
> - dma_request_slave_channel_compat(mask, edma_filter_fn,
> - &host->rxdma, mmc_dev(host->mmc), "rx");
> - if (!host->dma_rx) {
> + host->dma_rx = dma_request_slave_channel_compat_reason(mask,
> + edma_filter_fn, &host->rxdma,
> + mmc_dev(host->mmc), "rx");
> + if (IS_ERR(host->dma_rx)) {
> dev_err(mmc_dev(host->mmc), "Can't get dma_rx channel\n");
> - r = -ENODEV;
> + r = PTR_ERR(host->dma_rx);
> goto free_master_write;
> }
>
> @@ -1307,8 +1307,12 @@ static int __init davinci_mmcsd_probe(struct
> platform_device *pdev)
> host->mmc_irq = irq;
> host->sdio_irq = platform_get_irq(pdev, 1);
>
> - if (host->use_dma && davinci_acquire_dma_channels(host) != 0)
> + if (host->use_dma) {
> + ret = davinci_acquire_dma_channels(host);
> + if (ret == -EPROBE_DEFER)
> + goto out;
> host->use_dma = 0;
> + }
>
> /* REVISIT: someday, support IRQ-driven card detection. */
> mmc->caps |= MMC_CAP_NEEDS_POLL;
> --
> 2.3.5
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/13] mmc: omap_hsmmc: Support for deferred probing when requesting DMA channels
On 26 May 2015 at 15:26, Peter Ujfalusi wrote:
> Switch to use ma_request_slave_channel_compat_reason() to request the DMA
I guess it should be dma_request_slave_... huh, that was a long name. :-)
> channels. In case of error, return the error code we received including
> -EPROBE_DEFER
>
> Signed-off-by: Peter Ujfalusi
> CC: Ulf Hansson
With the minor change above.
Acked-by: Ulf Hansson
Kind regards
Uffe
> ---
> drivers/mmc/host/omap_hsmmc.c | 22 ++
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 57bb85930f81..d252478391ee 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -2088,23 +2088,21 @@ static int omap_hsmmc_probe(struct platform_device
> *pdev)
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
>
> - host->rx_chan =
> - dma_request_slave_channel_compat(mask, omap_dma_filter_fn,
> -&rx_req, &pdev->dev, "rx");
> + host->rx_chan = dma_request_slave_channel_compat_reason(mask,
> + omap_dma_filter_fn, &rx_req, &pdev->dev,
> "rx");
>
> - if (!host->rx_chan) {
> + if (IS_ERR(host->rx_chan)) {
> dev_err(mmc_dev(host->mmc), "unable to obtain RX DMA engine
> channel %u\n", rx_req);
> - ret = -ENXIO;
> + ret = PTR_ERR(host->rx_chan);
> goto err_irq;
> }
>
> - host->tx_chan =
> - dma_request_slave_channel_compat(mask, omap_dma_filter_fn,
> -&tx_req, &pdev->dev, "tx");
> + host->tx_chan = dma_request_slave_channel_compat_reason(mask,
> + omap_dma_filter_fn, &tx_req, &pdev->dev,
> "tx");
>
> - if (!host->tx_chan) {
> + if (IS_ERR(host->tx_chan)) {
> dev_err(mmc_dev(host->mmc), "unable to obtain TX DMA engine
> channel %u\n", tx_req);
> - ret = -ENXIO;
> + ret = PTR_ERR(host->tx_chan);
> goto err_irq;
> }
>
> @@ -2166,9 +2164,9 @@ err_slot_name:
> if (host->use_reg)
> omap_hsmmc_reg_put(host);
> err_irq:
> - if (host->tx_chan)
> + if (!IS_ERR_OR_NULL(host->tx_chan))
> dma_release_channel(host->tx_chan);
> - if (host->rx_chan)
> + if (!IS_ERR_OR_NULL(host->rx_chan))
> dma_release_channel(host->rx_chan);
> pm_runtime_put_sync(host->dev);
> pm_runtime_disable(host->dev);
> --
> 2.3.5
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/13] mmc: omap_hsmmc: No need to check DMA channel validity at module remove
On 26 May 2015 at 15:25, Peter Ujfalusi wrote: > The driver will not probe without valid DMA channels so no need to check > if they are valid when the module is removed. > > Signed-off-by: Peter Ujfalusi > CC: Ulf Hansson Acked-by: Ulf Hansson Kind regards Uffe > --- > drivers/mmc/host/omap_hsmmc.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 2cd828f42151..57bb85930f81 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -2190,10 +2190,8 @@ static int omap_hsmmc_remove(struct platform_device > *pdev) > if (host->use_reg) > omap_hsmmc_reg_put(host); > > - if (host->tx_chan) > - dma_release_channel(host->tx_chan); > - if (host->rx_chan) > - dma_release_channel(host->rx_chan); > + dma_release_channel(host->tx_chan); > + dma_release_channel(host->rx_chan); > > pm_runtime_put_sync(host->dev); > pm_runtime_disable(host->dev); > -- > 2.3.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v7 2/2] mmc: host: sdhci: Add support to disable SDR104/SDR50/DDR50 based on capability register 0.
On 21 May 2015 at 10:43, Suman Tripathi wrote: > The sdhci framework disables SDR104/SDR50/DDR50 based on only quirk. > This patch adds the support to disable SDR104/SDR50/DDR50 based on > reading the capability register 0. > > Signed-off-by: Suman Tripathi > --- > --- > drivers/mmc/host/sdhci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 58c1770..a3d9b8a 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -3118,7 +3118,8 @@ int sdhci_add_host(struct sdhci_host *host) > } > } > > - if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) > + if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V || > + !(caps[0] & SDHCI_CAN_VDD_180)) > caps[1] &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | >SDHCI_SUPPORT_DDR50); > > -- > 1.8.2.1 > I have no problem with this patch, except that it would be nice to get a few "tested by" to make sure it doesn't break UHS support for some SoCs. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/7] mmc: mediatek: Add Mediatek MMC driver
[...]
>> >> > +{
>> >> > + unsigned long tmo = jiffies + msecs_to_jiffies(20);
>> >> > +
>> >> > + while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
>> >> > + && time_before(jiffies, tmo))
>> >> > + continue;
>> >> > +
>> >> > + if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
>> >> > + dev_err(host->dev, "CMD bus busy detected\n");
>> >> > + host->error |= REQ_CMD_BUSY;
>> >> > + msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
>> >> > + return false;
>> >> > + }
>> >> > +
>> >> > + if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
>> >> > + /* R1B or with data, should check SDCBUSY */
>> >> > + while (readl(host->base + SDC_STS) & SDC_STS_SDCBUSY)
>> >> > + cpu_relax();
>> >> > + }
>> >>
>> >> MSDC seems to be handling card busy detection in HW, right?
>> >>
>> > Do not have this ability, HW only know if CMD/DAT is low, but do not
>> > have any interrupt for it,
>>
>> I see, but doesn't the above polling mean that msdc will not propagate
>> the response until the card have stopped signal busy? That's what
>> MMC_CAP_WAIT_WHILE_BUSY shall be used for.
>>
> As you see, we only check the "busy state" BEFORE issue a R1B command or
> with data command, but do not check if AFTER the request was done, that
> would do not match the "MMC_CAP_WAIT_WHILE_BUSY"(eg. CMD5 to sleep)
Okay, fair enough. Still, I don't understand why you want to do that?
> In addition, about CMD5, I find that the suspend/resume flow of EMMC is
> stranger in new kernel version, when suspend, it may issue CMD5 to enter
> sleep mode, then power off MMC, but when resume, it will
> re-initialization, So that why need do the redundant CMD5 in suspend ?
Both CMD0 and CMD5 are valid as wakeup commands.
To be able to use CMD5, the VCCQ regulator must be kept enabled in the
sleep state. That's when it becomes a bit tricky, due to the range of
different host drivers and SoCs for how VCCQ is managed.
To be safe we have chosen to use CMD0, since it works for *all* cases
no matter if VCCQ get gated or not.
Moreover, using CMD5 as the wakeup command would require added
complexity in the code dealing with suspend/resume. I don't think the
effort is worth it, at least until someone has proven that the resume
time is greatly decreased by using CMD5.
>
>> Perhaps you should remove the above polling, and rely on the MMC core
>> to poll with CMD13 instead?
> before any read/write command, core will issue CMD13 to confirm card
> status, here is just only do double confirm to avoid HW issue.
What HW issue and why do you need to double confirm? It seems strange.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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/7] mmc: mediatek: Add Mediatek MMC driver
[...]
>> You are invoking msdc_gate_clock() and msdc_ungate_clock() in a
>> balanced manner, thus hclk_enabled is redundant. Please remove it.
>
> on drv->probe(), already invoke the msdc_ungate_clock(), so, when the
> runtime pm resume invoke the msdc_ungate_clock(), the hclk already
> enabled.
That's why you invoke pm_runtime_set_active() during ->probe() when
deploying PM support in patch3. It's not an issue then.
[...]
>> I assume it's possible to gate the clock by updating a MSDC register
>> instead!? That would be prefereable since then you can leave clock
>> gating/ungating via the clk API, to be dealt from runtime PM. That
>> would also make "sclk_enabled" in the struct msdc_host redundant.
>>
>> Adopting to above, obviously requires MSDC to be able to ungate the
>> clock by also updating a MSDC register. I assume that's possible as
>> well!?
>>
> We can set the bit1 of MSDC_CFG, when this bit is 0, the bus clock was
> gated to 0 if no command or data is transmitted.
> And, from our designer, when we operate the MSDC register, we need make
> sure both HCLK and source are enabled, if source clock was disabled,
> write some MSDC registers will have no effect(eg. send CMD, without
> source clock, not only cannot send CMD, but also cannot get CMD timeout
> interrupt.)
Thanks, that answered my question. As I understand it you should be
able to adopt to my propsual.
[...]
>> > +{
>> > + unsigned long tmo = jiffies + msecs_to_jiffies(20);
>> > +
>> > + while ((readl(host->base + SDC_STS) & SDC_STS_CMDBUSY)
>> > + && time_before(jiffies, tmo))
>> > + continue;
>> > +
>> > + if (readl(host->base + SDC_STS) & SDC_STS_CMDBUSY) {
>> > + dev_err(host->dev, "CMD bus busy detected\n");
>> > + host->error |= REQ_CMD_BUSY;
>> > + msdc_cmd_done(host, MSDC_INT_CMDTMO, mrq, cmd);
>> > + return false;
>> > + }
>> > +
>> > + if (mmc_resp_type(cmd) == MMC_RSP_R1B || cmd->data) {
>> > + /* R1B or with data, should check SDCBUSY */
>> > + while (readl(host->base + SDC_STS) & SDC_STS_SDCBUSY)
>> > + cpu_relax();
>> > + }
>>
>> MSDC seems to be handling card busy detection in HW, right?
>>
> Do not have this ability, HW only know if CMD/DAT is low, but do not
> have any interrupt for it,
I see, but doesn't the above polling mean that msdc will not propagate
the response until the card have stopped signal busy? That's what
MMC_CAP_WAIT_WHILE_BUSY shall be used for.
Perhaps you should remove the above polling, and rely on the MMC core
to poll with CMD13 instead?
>> If so, you should enable MMC_CAP_WAIT_WHILE_BUSY and set
>> "max_busy_timeout" to DAT_TIMEOUT to inform the mmc core about it.
>>
[...]
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 v6 4/7] PM / Domains: Add DT bindings for PM QoS device latencies
On 27 April 2015 at 14:43, Geert Uytterhoeven wrote:
> PM QoS device start/stop are properties of the hardware.
> In legacy code, they're specified from platform code.
> On DT platforms, their values should come from DT.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> v6:
> - Rebased on top of v4.1-rc1 for new RFC,
>
> v4:
> - Drop save/restore state latencies, as they're Linux driver-specific,
> - Change state to RFC, as this is put on hold,
>
> v3:
> - No changes,
>
> v2:
> - New.
> ---
> Documentation/devicetree/bindings/power/power_domain.txt | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/power_domain.txt
> b/Documentation/devicetree/bindings/power/power_domain.txt
> index d659e5cb39be6057..32d1d3a399fe2a48 100644
> --- a/Documentation/devicetree/bindings/power/power_domain.txt
> +++ b/Documentation/devicetree/bindings/power/power_domain.txt
> @@ -73,12 +73,18 @@ Required properties:
> - power-domains : A phandle and PM domain specifier as defined by bindings
> of
> the power controller specified by phandle.
>
> +Optional properties:
> + - stop-latency: Stop latency of the device, in ns,
> + - start-latency: Start latency of the device, in ns,
> +
What do you think of renaming these to "suspend-latency" and
"resume-latency" instead? I think that better reflects their purpose.
I have no strong opinion though.
> Example:
>
> leaky-device@1235 {
> compatible = "foo,i-leak-current";
> reg = <0x1235 0x1000>;
> power-domains = <&power 0>;
> + stop-latency = <25>;
> + start-latency = <25>;
> };
>
> The node above defines a typical PM domain consumer device, which is located
> --
> 1.9.1
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v6 1/2] arm64: dts: Add the arasan sdhci nodes in apm-storm.dtsi
On 12 May 2015 at 09:28, Arnd Bergmann wrote:
> On Tuesday 12 May 2015 11:03:46 Suman Tripathi wrote:
>> > @@ -533,6 +567,15 @@
>> > interrupts = <0x0 0x4f 0x4>;
>> > };
>> >
>> > + sdhci0: sdhci@1c00 {
>> > + compatible = "arasan,sdhci-4.9a";
>> > + reg = <0x0 0x1c00 0x0 0x100>;
>> > + interrupts = <0x0 0x49 0x4>;
>> > + dma-coherent;
>> > + clock-names = "clk_xin", "clk_ahb";
>> > + clocks = <&sdioclk 0>, <&ahbclk 0>;
>> > + };
>> > +
>> > phy1: phy@1f21a000 {
>> > compatible = "apm,xgene-phy";
>> > reg = <0x0 0x1f21a000 0x0 0x100>;
>> > --
>> > 1.8.2.1
>> >
>>
>> Can anyone from dt community review this patch ? I have changed the dts node
>> names from sdhc to sdhci as per Arnd, Michael comments .
>
> I was actually asking for it to be named 'mmc', not 'sdhci', because the
> name is supposed to indicate the purpose of the device, not the
> implementation. I realize that we are inconsistent here, just as with
> 'uart' vs 'serial', and that ePAPR does not define what to do.
Then we need a common name to address the following purposes: SD, MMC,
eMMC, SDIO.
>
> We should probably add something to
> Documentation/devicetree/bindings/mmc/mmc.txt
> about this topic and change all the dts files accordingly (unless there
> is a risk for regressions). At the moment, the mmc.txt file also includes
> an example with 'sdhci', not 'mmc'.
>
I am happy to change, as long as we can come up with something better.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/7] mmc: mediatek: Add Mediatek MMC driver
On 7 May 2015 at 03:42, Chaotian Jing wrote: > Dear Ulf, > > Thanks! > Below is my comment: > > On Wed, 2015-05-06 at 18:31 +0200, Ulf Hansson wrote: >> On 6 May 2015 at 08:54, chaotian.jing wrote: >> > Dear Ulf, >> > >> > Thanks for your review. >> > I must do a explain of our MMC host: >> > Source clock is source clock of the MMC bus, MMC host has a divider to >> > get different bus clock frequency. now the runtime suspend is gating >> > this clock. >> > >> > Hclk is the power domain of the MMC host, if Hclk is gated, the MMC host >> > cannot work(all registers readout is zero). and, all registers would be >> > reset to default value if Hclk is gated/ungated. >> > At MT8173, MSDC0 and MSDC2 has independent Hclk, MSDC1 and MSDC3's Hclk >> > was controlled by "Infra module". > Sorry for mistake, MSDC0 and MSDC3 has independent Hclk, MSDC1 and > MSDC2's Hclk was controlled by "Infra module". the Infra module is a > "power saving module", when the system go to sleep, Infra module will be > set and MSDC1 & MSDC2's Hclk will be gated automatically. >> >> Thanks for clarifying! >> >> I don't have enough knowledge about your SoC to understand the detail, >> but it seems like we are mixing clocks and power domains. I would >> rather keep this separate - if the HW allows it. >> >> I guess the key question I have is the following: >> 1) Is it hardware wise possible to gate the hclk, but without gating >> the power domain? >> 2) At what level is the reference counting done for each device in the >> power domain? In HW or in sofftware? >> > > Actually, Our MMC host do not have power domain, all the control of the > host is the Hclk. Okay, but I am not sure that fully answered my question. You said that hclk will be gated when the power domain gets gated. And the power domain (infra module, right?) will be gated at system PM sleep. And when that happens the mmc controller loses register context. But, again, can hclk be gated without gating the power domain? No matter what, it seems like a good idea to gate the power domain when all clients of it are idle. Yes we would need to manage register save/restore context, but on the other hand allow you to save more power, right? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: ux500: Move GPIO regulator for SD-card into board DTSs
On 21 April 2015 at 09:33, Lee Jones wrote: > On Tue, 21 Apr 2015, Ulf Hansson wrote: > >> On 20 April 2015 at 20:26, Lee Jones wrote: >> > On Mon, 20 Apr 2015, Ulf Hansson wrote: >> > >> >> The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but >> >> instead it's specific to the board. Move the definition of it, into the >> >> board DTSs. >> > >> > What makes you think that? >> >> Because of how it was structured today. >> >> ste-dbx5x0.dtsi - common for all ux500 boards, thus I considered this >> as the SoC configuration. > > ste-dbx5x0.dtsi is common for all ux500 and ux540 boards. > >> Then below are board configs which uses the above dtsi: >> ste-href.dtsi - common for href boards (used by ste-hrefprev60.dtsi >> and ste-hrefv60plus.dtsi), have vmmci >> ste-snowball.dts, have vmmci >> ste-ccu8540.dts, don't have vmmci >> ste-ccu9540.dts, don't have vmmci > > Ah, got you. In which case it doesn't belong in ste-dbx5x0.dtsi. > >> > We normally place the common pieces (of which there are many in this >> > node) in the highest level DTSI file, then add the platform specific >> > ones in the DTS files. >> >> Okay, so maybe it's due to the naming of ste-dbx5x0.dtsi, that I >> thought this was intended to cover the SoC configuration and not any >> of the platform specific stuff. > > ste-dbx5x0.dtsi should cover all pieces which are common to all ux500 > and ux540 devices. Then the lower level file ste-href-ab8500.dtsi > should cover all pieces which are common to ux500 devices and finally > the DTS files should add board specific information. Duplicate > nodes and properties are frowned upon. > >> So what your advise of doing this? > > So the file which covers the x500 boards is ste-href-ab8500.dtsi. I > would move the node into there instead. Keep it disabled and enable > the associated nodes in the 2 DTS files. Why ste-href-ab8500.dtsi? Isn't that suppose to cover configurations common to the ab8500 subsystem? The vmmci models a board specific mounted circuit (aka level-shifter). Thus it exist on some boards but not on others. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: ux500: Move GPIO regulator for SD-card into board DTSs
On 20 April 2015 at 20:26, Lee Jones wrote:
> On Mon, 20 Apr 2015, Ulf Hansson wrote:
>
>> The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but
>> instead it's specific to the board. Move the definition of it, into the
>> board DTSs.
>
> What makes you think that?
Because of how it was structured today.
ste-dbx5x0.dtsi - common for all ux500 boards, thus I considered this
as the SoC configuration.
Then below are board configs which uses the above dtsi:
ste-href.dtsi - common for href boards (used by ste-hrefprev60.dtsi
and ste-hrefv60plus.dtsi), have vmmci
ste-snowball.dts, have vmmci
ste-ccu8540.dts, don't have vmmci
ste-ccu9540.dts, don't have vmmci
>
> We normally place the common pieces (of which there are many in this
> node) in the highest level DTSI file, then add the platform specific
> ones in the DTS files.
Okay, so maybe it's due to the naming of ste-dbx5x0.dtsi, that I
thought this was intended to cover the SoC configuration and not any
of the platform specific stuff.
So what your advise of doing this?
Kind regards
Uffe
>
>> Fixes: c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by
>> default")
>> Signed-off-by: Ulf Hansson
>> ---
>> arch/arm/boot/dts/ste-dbx5x0.dtsi | 17 -
>> arch/arm/boot/dts/ste-href.dtsi| 17 +
>> arch/arm/boot/dts/ste-snowball.dts | 15 +++
>> 3 files changed, 32 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/ste-dbx5x0.dtsi
>> b/arch/arm/boot/dts/ste-dbx5x0.dtsi
>> index bfd3f1c..2201cd5 100644
>> --- a/arch/arm/boot/dts/ste-dbx5x0.dtsi
>> +++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi
>> @@ -1017,23 +1017,6 @@
>> status = "disabled";
>> };
>>
>> - vmmci: regulator-gpio {
>> - compatible = "regulator-gpio";
>> -
>> - regulator-min-microvolt = <180>;
>> - regulator-max-microvolt = <290>;
>> - regulator-name = "mmci-reg";
>> - regulator-type = "voltage";
>> -
>> - startup-delay-us = <100>;
>> - enable-active-high;
>> -
>> - states = <180 0x1
>> - 290 0x0>;
>> -
>> - status = "disabled";
>> - };
>> -
>> mcde@a035 {
>> compatible = "stericsson,mcde";
>> reg = <0xa035 0x1000>, /* MCDE */
>> diff --git a/arch/arm/boot/dts/ste-href.dtsi
>> b/arch/arm/boot/dts/ste-href.dtsi
>> index bf8f0ed..8cf499a 100644
>> --- a/arch/arm/boot/dts/ste-href.dtsi
>> +++ b/arch/arm/boot/dts/ste-href.dtsi
>> @@ -111,6 +111,23 @@
>> pinctrl-1 = <&i2c3_sleep_mode>;
>> };
>>
>> + vmmci: regulator-gpio {
>> + compatible = "regulator-gpio";
>> +
>> + regulator-min-microvolt = <180>;
>> + regulator-max-microvolt = <290>;
>> + regulator-name = "mmci-reg";
>> + regulator-type = "voltage";
>> +
>> + startup-delay-us = <100>;
>> + enable-active-high;
>> +
>> + states = <180 0x1
>> + 290 0x0>;
>> +
>> + status = "disabled";
>> + };
>> +
>> // External Micro SD slot
>> sdi0_per1@80126000 {
>> arm,primecell-periphid = <0x10480180>;
>> diff --git a/arch/arm/boot/dts/ste-snowball.dts
>> b/arch/arm/boot/dts/ste-snowball.dts
>> index 206826a..65a7f63 100644
>> --- a/arch/arm/boot/dts/ste-snowball.dts
>> +++ b/arch/arm/boot/dts/ste-snowball.dts
>> @@ -146,8 +146,23 @@
>> };
>>
>> vmmci: regulator-gpio {
>> + compatible = "regulator-gpio";
>> +
>> gpios = <&gpio7 4 0x4>;
>> enable-gpio = <&gpio6 25 0x4>;
>> +
>> + regulator-min-microvolt = <180>;
>> + regulator-max-microvolt = <290>;
>> + regulator-name = "mmci-reg";
>> + regulator-type = "voltage";
>> +
>> + startup-delay-us = <100>;
>> + enable-active-high;
>> +
>> + states = <180 0x1
>> + 290 0x0>;
>> +
>> + status = "disabled";
>> };
>>
>> // External Micro SD slot
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] ARM: ux500: Enable GPIO regulator for SD-card for HREF boards
Fixes: c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by default")
Signed-off-by: Ulf Hansson
---
arch/arm/boot/dts/ste-href.dtsi | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm/boot/dts/ste-href.dtsi b/arch/arm/boot/dts/ste-href.dtsi
index 8cf499a..744c1e3 100644
--- a/arch/arm/boot/dts/ste-href.dtsi
+++ b/arch/arm/boot/dts/ste-href.dtsi
@@ -124,8 +124,6 @@
states = <180 0x1
290 0x0>;
-
- status = "disabled";
};
// External Micro SD slot
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] ARM: ux500: Enable GPIO regulator for SD-card for snowball
Fixes: c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by default")
Signed-off-by: Ulf Hansson
---
arch/arm/boot/dts/ste-snowball.dts | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/arm/boot/dts/ste-snowball.dts
b/arch/arm/boot/dts/ste-snowball.dts
index 65a7f63..1bc84eb 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -161,8 +161,6 @@
states = <180 0x1
290 0x0>;
-
- status = "disabled";
};
// External Micro SD slot
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] ARM: ux500: Fix SD-card regression by using GPIO regulator
This patchset fixes an old stability issue for detection of SD-cards. The DT
node for the GPIO regulator which controls the I/O voltage level towards the
SD-card was disabled in below commit.
c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by default")
The conseqeunce from that commit, was that the initially chosen voltage level
remained, even after I/O voltage switch sequence was performed for UHS-I cards,
since no GPIO regulator could be fetched.
Appearantly some SD-cards didn't have any problem to cope with that, but some
had.
Recently thorough error handling was introduced for the MMCI mmc host driver,
while fetching the regulators for the SD-card. That actually made the driver to
continously return -EPROBE_DEFER while probing, which thus meant failing all the
times instead of just failing "sometimes".
The commit that introduced the thorough error handling is:
9369c97cc7ec ("mmc: mmci: Cascade EPROBE_DEFER from regulators")
Ulf Hansson (3):
ARM: ux500: Move GPIO regulator for SD-card into board DTSs
ARM: ux500: Enable GPIO regulator for SD-card for HREF boards
ARM: ux500: Enable GPIO regulator for SD-card for snowball
arch/arm/boot/dts/ste-dbx5x0.dtsi | 17 -
arch/arm/boot/dts/ste-href.dtsi| 15 +++
arch/arm/boot/dts/ste-snowball.dts | 13 +
3 files changed, 28 insertions(+), 17 deletions(-)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] ARM: ux500: Move GPIO regulator for SD-card into board DTSs
The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but
instead it's specific to the board. Move the definition of it, into the
board DTSs.
Fixes: c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by default")
Signed-off-by: Ulf Hansson
---
arch/arm/boot/dts/ste-dbx5x0.dtsi | 17 -
arch/arm/boot/dts/ste-href.dtsi| 17 +
arch/arm/boot/dts/ste-snowball.dts | 15 +++
3 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/arch/arm/boot/dts/ste-dbx5x0.dtsi
b/arch/arm/boot/dts/ste-dbx5x0.dtsi
index bfd3f1c..2201cd5 100644
--- a/arch/arm/boot/dts/ste-dbx5x0.dtsi
+++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi
@@ -1017,23 +1017,6 @@
status = "disabled";
};
- vmmci: regulator-gpio {
- compatible = "regulator-gpio";
-
- regulator-min-microvolt = <180>;
- regulator-max-microvolt = <290>;
- regulator-name = "mmci-reg";
- regulator-type = "voltage";
-
- startup-delay-us = <100>;
- enable-active-high;
-
- states = <180 0x1
- 290 0x0>;
-
- status = "disabled";
- };
-
mcde@a035 {
compatible = "stericsson,mcde";
reg = <0xa035 0x1000>, /* MCDE */
diff --git a/arch/arm/boot/dts/ste-href.dtsi b/arch/arm/boot/dts/ste-href.dtsi
index bf8f0ed..8cf499a 100644
--- a/arch/arm/boot/dts/ste-href.dtsi
+++ b/arch/arm/boot/dts/ste-href.dtsi
@@ -111,6 +111,23 @@
pinctrl-1 = <&i2c3_sleep_mode>;
};
+ vmmci: regulator-gpio {
+ compatible = "regulator-gpio";
+
+ regulator-min-microvolt = <180>;
+ regulator-max-microvolt = <290>;
+ regulator-name = "mmci-reg";
+ regulator-type = "voltage";
+
+ startup-delay-us = <100>;
+ enable-active-high;
+
+ states = <180 0x1
+ 290 0x0>;
+
+ status = "disabled";
+ };
+
// External Micro SD slot
sdi0_per1@80126000 {
arm,primecell-periphid = <0x10480180>;
diff --git a/arch/arm/boot/dts/ste-snowball.dts
b/arch/arm/boot/dts/ste-snowball.dts
index 206826a..65a7f63 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -146,8 +146,23 @@
};
vmmci: regulator-gpio {
+ compatible = "regulator-gpio";
+
gpios = <&gpio7 4 0x4>;
enable-gpio = <&gpio6 25 0x4>;
+
+ regulator-min-microvolt = <180>;
+ regulator-max-microvolt = <290>;
+ regulator-name = "mmci-reg";
+ regulator-type = "voltage";
+
+ startup-delay-us = <100>;
+ enable-active-high;
+
+ states = <180 0x1
+ 290 0x0>;
+
+ status = "disabled";
};
// External Micro SD slot
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 2/5] mmc: mediatek: Add Mediatek MMC driver
On 17 April 2015 at 11:12, Sascha Hauer wrote:
> Ulf,
>
> On Tue, Mar 31, 2015 at 02:23:06PM +0200, Ulf Hansson wrote:
>> On 17 March 2015 at 04:13, Chaotian Jing wrote:
>> > +
>> > + msdc_set_buswidth(host, ios->bus_width);
>> > +
>> > + /* Suspend/Resume will do power off/on */
>> > + switch (ios->power_mode) {
>> > + case MMC_POWER_UP:
>> > + msdc_init_hw(host);
>> > + if (!IS_ERR(mmc->supply.vmmc)) {
>> > + ret = mmc_regulator_set_ocr(mmc, mmc->supply.vmmc,
>> > + ios->vdd);
>> > + if (ret) {
>> > + dev_err(host->dev, "Failed to set vmmc
>> > power!\n");
>> > + return;
>> > + }
>> > + }
>> > + break;
>> > + case MMC_POWER_ON:
>> > + if (!IS_ERR(mmc->supply.vqmmc)) {
>> > + ret = regulator_enable(mmc->supply.vqmmc);
>>
>> The calls to regulator_enable|disable() for the vqmmc will not be
>> balanced properly here. You need a local cache variable like
>> "is_enabled" to keep track of this.
>
> Shouldn't the MMC core provide balanced hooks for this? What about
> MMC_POWER_UP, can this be used for enabling regulators?
Currently host drivers deals with vqmmc enabling/disabling, setting
voltage, etc - entirely by them selves.
Dough Anderson is working on fixing that:
http://lkml.iu.edu/hypermail/linux/kernel/1503.1/03108.html
MMC_POWER_UP is when vmmc shall be enabled.
MMC_POWER_ON is when vqmmc shall be enabled.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 2/5] mmc: mediatek: Add Mediatek MMC driver
[...]
>> > +
>> > +struct mt_bdma_desc {
>> > + u32 first_u32;
>> > +#define BDMA_DESC_EOL (1 << 0)
>> > +#define BDMA_DESC_CHECKSUM (0xff << 8) /* bit8 ~ bit15 */
>> > +#define BDMA_DESC_BLKPAD (1 << 17)
>> > +#define BDMA_DESC_DWPAD(1 << 18)
>> > + u32 next;
>> > + u32 ptr;
>> > + u32 second_u32;
>> > +#define BDMA_DESC_BUFLEN (0x) /* bit0 ~ bit15 */
>> > +};
>> > +
>> > +struct msdc_dma {
>> > + struct scatterlist *sg; /* I/O scatter list */
>> > + struct mt_gpdma_desc *gpd; /* pointer to gpd
>> array */
>> > + struct mt_bdma_desc *bd;/* pointer to bd
>> array */
>> > + dma_addr_t gpd_addr;/* the physical address of gpd array */
>> > + dma_addr_t bd_addr; /* the physical address of bd array */
>> > +};
>>
>> This looks weird from DMA perspective. Can you elaborate on why you
>> can't use the dmaengine API?
>>
> The gpd and bd structure are determined by the MSDC hw, and, the DMA
> controller is a part of the MSDC hw, different with the chain dma implemented
> by the kernel.
Hmm. I haven't reviewed the DMA related parts in detail. I will do
that when you have sent the next version.
>> > +
>> > +struct msdc_host {
>> > + struct device *dev;
>> > + struct mmc_host *mmc; /* mmc structure */
>> > + int cmd_rsp;
>> > +
>> > + spinlock_t lock;
>> > + struct mmc_request *mrq;
>> > + struct mmc_command *cmd;
>> > + struct mmc_data *data;
>> > + int error;
>> > +
>> > + void __iomem *base; /* host base address */
>> > +
>> > + struct msdc_dma dma;/* dma channel */
>> > +
>> > + u32 timeout_ns; /* data timeout ns */
>> > + u32 timeout_clks; /* data timeout clks */
>> > +
>> > + struct pinctrl *pinctrl;
>> > + struct pinctrl_state *pins_default;
>> > + struct pinctrl_state *pins_uhs;
>> > + struct delayed_work req_timeout;
>> > + int irq;/* host interrupt */
>> > +
>> > + struct clk *src_clk;/* msdc source clock */
>> > + u32 mclk; /* mmc subsystem clock */
>> > + u32 hclk; /* host clock speed */
>> > + u32 sclk; /* SD/MS clock speed */
>> > + bool ddr;
>> > +};
>> > +
>> > +static void sdr_set_bits(void __iomem *reg, u32 bs)
>> > +{
>> > + u32 val = readl(reg);
>> > +
>> > + val |= bs;
>> > + writel(val, reg);
>> > +}
>> > +
>> > +static void sdr_clr_bits(void __iomem *reg, u32 bs)
>> > +{
>> > + u32 val = readl(reg);
>> > +
>> > + val &= ~(u32)bs;
>> > + writel(val, reg);
>> > +}
>> > +
>> > +static void sdr_set_field(void __iomem *reg, u32 field, u32 val)
>> > +{
>> > + unsigned int tv = readl(reg);
>> > +
>> > + tv &= ~field;
>> > + tv |= ((val) << (ffs((unsigned int)field) - 1));
>> > + writel(tv, reg);
>> > +}
>>
>> A common thought for all the three above functions:
>>
>> Have you considered using a cache variable for those registers that
>> often gets updated? In that way you would have to read the register
>> value every time when you want to write to it. It should improve
>> performance a bit.
>>
> These register can be modified by the MSDC hw, cannot cache it.
Is that true for all registers?
Anyway, let's leave this as is.
>> > +
>> > +static void sdr_get_field(void __iomem *reg, u32 field, u32 *val)
>> > +{
>> > + unsigned int tv = readl(reg);
>> > +
>> > + *val = ((tv & field) >> (ffs((unsigned int)field) - 1));
>> > +}
>> > +
>> > +static void msdc_reset_hw(struct msdc_host *host)
>> > +{
>> > + u32 val;
>> > +
>> > + sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_RST);
>> > + while (readl(host->base + MSDC_CFG) & MSDC_CFG_RST)
>> > + cpu_relax();
>> > +
>> > + sdr_set_bits(host->base + MSDC_FIFOCS, MSDC_FIFOCS_CLR);
>> > + while (readl(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_CLR)
>> > + cpu_relax();
>> > +
>> > + val = readl(host->base + MSDC_INT);
>> > + writel(val, host->base + MSDC_INT);
>> > +}
>> > +
>> > +static void msdc_cmd_next(struct msdc_host *host,
>> > + struct mmc_request *mrq, struct mmc_command *cmd);
>>
>> Please investigate whether you can move around the code to prevent
>> this static declaration of the function.
>>
> It is hard to do it, because the first cmd may be CMD23, and do the next is
> send the mrq->cmd, so, there is a loop here, at least on method need do
> static declaration,
Okay.
[...]
>> > +/* clock control primitives */
>> > +static void msdc_set_timeout(struct msdc_host *host, u32 ns, u32 clks)
>> > +{
>> > + u32 timeout, clk_ns;
>> > + u32 mode = 0;
>> > +
>> > + host->timeout_ns = ns;
>> > + host->timeout_clks = clks;
>> > + if (host->sclk == 0) {
>> > + timeout = 0;
>> > + } else {
>> > +
Re: [PATCH v2 2/2] mmc: host: Add some quirks to be read from fdt in sdhci-pltm.c
On 30 March 2015 at 16:46, Suman Tripathi wrote: > This patch adds some quirks support to be read from fdt. > > Signed-off-by: Suman Tripathi > --- > drivers/mmc/host/sdhci-pltfm.c | 15 +++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c > index bef250e..9f6a4b9 100644 > --- a/drivers/mmc/host/sdhci-pltfm.c > +++ b/drivers/mmc/host/sdhci-pltfm.c > @@ -85,6 +85,21 @@ void sdhci_get_of_property(struct platform_device *pdev) > > if (of_get_property(np, "broken-cd", NULL)) > host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION; > + > + if (of_get_property(np, "delay-after-power", NULL)) > + host->quirks |= SDHCI_QUIRK_DELAY_AFTER_POWER; > + > + if (of_get_property(np, "no-hispd", NULL)) > + host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT; > + > + if (of_get_property(np, "broken-adma", NULL)) > + host->quirks |= SDHCI_QUIRK_BROKEN_ADMA; > + > + if (of_get_property(np, "broken-dma", NULL)) > + host->quirks |= SDHCI_QUIRK_BROKEN_DMA; > + > + if (of_get_property(np, "no-cmd23", NULL)) > + host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23; Can't at least some of these be distinguished from what sdhci variant that is being used? Instead of having them in DT... Kind regards Uffe > > if (of_get_property(np, "no-1-8-v", NULL)) > host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > -- > 1.8.2.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/9] Add sd/emmc support for stih407 family silicon
On 30 March 2015 at 16:39, Peter Griffin wrote: > Hi, > > This series adds sd/emmc support to the sdhci-st.c driver for stih407 > family silicon. The changes mainly involve configuring some extra glue > registers in the flashSS which configure the Arasan controller. > > This series also adds support for UHS modes for eMMC. To allow > UHS HS200/SD104 modes to function correctly, due to the > tight timing constriants, support for delay management is also added. > Two types of delay management are supported, static delay management and > dynamic delay management, this delay management is only available > on eMMC pads on stih410 and later silicon. > > This series has been tested with stih410-b2120 revd on eMMC and sd, at > various clock speeds. As part of this testing a bug was also found in the > upstream flexgen clock set_rate implementation (now fixed upstream). > > max-frequency = 200Mhz > /dev/mmcblk0p1: > Timing buffered disk reads: 270 MB in 3.02 seconds = 89.54 MB/sec > > max-frequency = 100Mhz > root@debian-armhf:~# hdparm -t /dev/mmcblk0p1 > /dev/mmcblk0p1: > Timing buffered disk reads: 210 MB in 3.00 seconds = 70.00 MB/sec > > max-frequency = 50Mhz > root@debian-armhf:~# hdparm -t /dev/mmcblk0p1 > /dev/mmcblk0p1: > Timing buffered disk reads: 118 MB in 3.00 seconds = 39.28 MB/sec > > It has also been tested on stih416-b2020 to ensure we have caused no > regressions. Finally the dt documentation has been updated to reflect > the changes in the driver code. Intrestingly it seems we are the first > upstream platform to be using some of the uhs bindings such as > sd-uhs-sdr104. > > Changes since v2: > - Some whitespace fixups (Max) > - if (!ioaddr) suggestion (Max) > - Add stih418-b2199 suport (Max) > - Stih410 to STiH410 fixes (Max) > - rebased on v4.0-rc6 (Pete) > > Changes since v1: > - Partition the changes into smaller patches to aid review process (Ulf) > > regards, > > Peter. > > Peter Griffin (9): > mmc: sdhci-st: Add macros for register offsets and bitfields for mmcss > glue regs > mmc: sdhci-st: Add support for de-asserting reset signal and top regs > resource > mmc: sdhci-st: Add delay management functions for top registers > (eMMC). > mmc: sdhci-st: Add st_mmcss_cconfig function to configure mmcss glue > registers. > mmc: sdhci-st: Add sdhci_st_set_uhs_signaling function. > mmc: sdhci-st: Update the quirks for this controller. > mmc: sdhci-st: Update ST SDHCI binding documentation. > ARM: STi: DT: STiH407: Add dt nodes for sdhci and emmc. > ARM: STi: DT: STiH418: Add dt nodes for sdhci and emmc. > > Documentation/devicetree/bindings/mmc/sdhci-st.txt | 100 +- > arch/arm/boot/dts/stih407-family.dtsi | 30 ++ > arch/arm/boot/dts/stih410-b2120.dts| 10 + > arch/arm/boot/dts/stih418-b2199.dts| 12 + > arch/arm/boot/dts/stihxxx-b2120.dtsi | 8 + > drivers/mmc/host/sdhci-st.c| 346 > - > 6 files changed, 492 insertions(+), 14 deletions(-) > Hi Peter, I was about to apply patches 1->7. Patch2, I can't apply, it needs a rebase. While doing that, please also fix the checkpatch warning for patch5. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
