Re: [PATCH v2 1/2] usb: dwc2: optionally assert phy "full reset" when waking up

[Doug Anderson]

Hi,

On Mon, Nov 2, 2015 at 9:16 AM, Rob Herring  wrote:
> On Mon, Nov 2, 2015 at 10:22 AM, Doug Anderson  wrote:
>> Rob,
>>
>> On Mon, Nov 2, 2015 at 8:12 AM, Rob Herring  wrote:
>>> On Fri, Oct 30, 2015 at 3:17 PM, Douglas Anderson  
>>> wrote:
>>>> From: Doug Anderson 
>
>>>> We can get the PHY out of its bad state by asserting its "port reset",
>>>> but unfortunately that seems to assert a reset onto the USB bus so it
>>>> could confuse things if we don't actually deenumerate / reenumerate the
>>>> device.
>>>>
>>>> We can also get the PHY out of its bad state by fully resetting it using
>>>> the reset from the CRU (clock reset unit), which does a more full
>>>> reset.  The CRU-based reset appears to actually cause devices on the bus
>>>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>>>> way).
>>>
>>> The reset from the CRU goes to the PHY, correct? Therefore, the
>>> binding should reflect that. Connecting it to the host controller is a
>>> hack.
>>>
>>> So describe the reset connection properly and then add a .phy_reset()
>>> hook to the phy subsystem. Then call that when flag property you added
>>> is set.
>>
>> As per previous email, I disagree.  The fact that there may be more
>> than one reset exposed from the PHY and that there is not a tightly
>> coupled relationship between a PHY driver and a USB driver means that
>> we would need to re-invent the reset API on top of the PHY API.  In my
>> case I am exposing a single reset at the moment, but there may be
>> reasons to expose additional "PHY" resets in the future.
>
> Your previous approach was completely different. I was not arguing
> that the PHY driving reset to the host was wrong use of reset binding,
> just that it was an overkill. Now, it is just flat wrong unless you
> convince me that SRST_USBHOST1_PHY is a connection to the host rather
> than the PHY.

It still seems to me that the argument that there can be multiple
types of "phy reset" is still a sane argument.  Just because we
currently happen to need the one that resets the whole PHY now doesn't
mean that we won't need other different resets later.

...and if you're exposing multiple different resets, then the reset
API is the best one to use.

I _could_ see the argument that perhaps the reset should pass through
the PHY driver so that it could possibly in the future be aware of the
reset.  That is: the SRST could use the reset API to expose the PHY
reset to the PHY driver and then the PHY driver could in turn expose
the PHY reset (using the reset API) to the dwc2 driver.  At the moment
this would be a complete pass through, but structuring it that way
would later allow the PHY to do something with the reset.  How about
that?

-Doug
--
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] usb: dwc2: optionally assert phy "full reset" when waking up

[Doug Anderson]

Rob,

On Mon, Nov 2, 2015 at 8:12 AM, Rob Herring  wrote:
> On Fri, Oct 30, 2015 at 3:17 PM, Douglas Anderson  
> wrote:
>> From: Doug Anderson 
>>
>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>> port) the PHY can get into a bad state when a wakeup is asserted (not
>> just a wakeup from full system suspend but also a wakeup from
>> autosuspend).
>
> I would think re-enumerating means autosuspend is broken.

Yes, it is broken on this port.  I've been trying to figure out how to
disable autosuspend at the driver level.  Until then we can do it with
udev rules but that's much less ideal.

On our current system we have autosuispend turned off for most things
via udev and laptop-mode-tools.  ...so really the only case we see
this reset happen is if an empty hub is plugged in and then you plug
something into the hub.  The reset isn't terrible and is much better
than the device getting stuck.

You can also see the reset doing its thing if you fully suspend the
system and try to wake up from a keyboard or a mouse attached to the
port.  Being able to wake up (and then seeing a reset) is still better
than not being able to wake up.


>> We can get the PHY out of its bad state by asserting its "port reset",
>> but unfortunately that seems to assert a reset onto the USB bus so it
>> could confuse things if we don't actually deenumerate / reenumerate the
>> device.
>>
>> We can also get the PHY out of its bad state by fully resetting it using
>> the reset from the CRU (clock reset unit), which does a more full
>> reset.  The CRU-based reset appears to actually cause devices on the bus
>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>> way).
>
> The reset from the CRU goes to the PHY, correct? Therefore, the
> binding should reflect that. Connecting it to the host controller is a
> hack.
>
> So describe the reset connection properly and then add a .phy_reset()
> hook to the phy subsystem. Then call that when flag property you added
> is set.

As per previous email, I disagree.  The fact that there may be more
than one reset exposed from the PHY and that there is not a tightly
coupled relationship between a PHY driver and a USB driver means that
we would need to re-invent the reset API on top of the PHY API.  In my
case I am exposing a single reset at the moment, but there may be
reasons to expose additional "PHY" resets in the future.

http://www.gossamer-threads.com/lists/linux/kernel/2289780#2289780

...using the existing reset API seems better.
--
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: [REPOST PATCH 0/3] dwc2 patches to allow wakeup on Rockchip rk3288

[Doug Anderson]

John,

On Mon, Oct 26, 2015 at 7:05 PM, John Youn  wrote:
> On 10/21/2015 9:23 AM, Doug Anderson wrote:
>> John,
>>
>> On Mon, Jul 6, 2015 at 11:27 AM, Douglas Anderson  
>> wrote:
>>> This series of patches, together with
>>> <https://patchwork.kernel.org/patch/6652341/> from Chris Zhong and a
>>> dts change allow us to wake up from a USB device on rk3288 boards.
>>> The patches were tested on rk3288-jerry in the chromeos-3.14 kernel.
>>> The chromeos-3.14 kernel tested included a full set of dwc2 backports
>>> from upstream, so this is expected to function upstream once we get
>>> everything setup there.
>>>
>>>
>>> Douglas Anderson (3):
>>>   USB: Export usb_wakeup_enabled_descendants()
>>>   Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
>>>   USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
>>>
>>>  Documentation/devicetree/bindings/usb/dwc2.txt |  4 +++
>>>  drivers/usb/core/hub.c |  7 ++--
>>>  drivers/usb/dwc2/core.h|  2 ++
>>>  drivers/usb/dwc2/platform.c| 45 
>>> --
>>>  include/linux/usb/hcd.h|  5 +++
>>>  5 files changed, 58 insertions(+), 5 deletions(-)
>>
>> This series was posted a while ago.  Do you have any comments on it?
>> Should I repost it?
>>
>> Thanks!
>>
>> -Doug
>>
>
>
> Sorry, I must have missed it earlier.
>
> Could you repost based on latest and I'll review.

I tried today but something has changed in mainline Linux and the
rk3288 isn't waking up from USB even when I've configured it
correctly.  I can still send up the patches, but they won't actually
produce something that will work upstream.  :(

I'll keep it my back burner to investigate, though if someone else
(like maybe someone from Rockchip) wanted to, they could to it too...

...until we figure out what's going on we should probably consider
these patches abandoned.

-Doug
--
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/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port

[Doug Anderson]

Hi,

On Mon, Oct 26, 2015 at 4:49 PM, Doug Anderson  wrote:
> One note: the "full" PHY reset is actually not in the register map of
> the PHY.  It is amazingly enough in the CRU (clock reset unit).  So if
> we actually exposed the "full" reset through the PHY framework, the
> PHY would then turn around and pass it off to the reset framework,
> which is how the full PHY reset is exposed from the CRU.

Interestingly enough, it looks like there's a reasonable chance that
we won't be able to use the PHY port reset.  We might have to go back
to the full PHY reset (though it causes de-enumeration and
re-enumeration) for safety, since there might be some side effects of
the "phy port reset".

Probably shouldn't land this series until we can figure that out.
...but in that case we still have the question of how we should export
it.  It would certainly be easy to keep using the reset framework...
--
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/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port

[Doug Anderson]

Rob,

On Mon, Oct 26, 2015 at 4:05 PM, Rob Herring  wrote:
>>> A DT reset controller seems like a bit of an overkill here. I think this
>>> would be much more simple if we just add a phy reset hook to the phy
>>> subsystem.
>>
>> Adding a reset hook in the PHY subsystem does seem like a reasonable
>> idea to me.  I was considering it in an earlier version of this series
>> that actually used a reset of the PHY to the fix the stuck dwc2.
>>
>> ...but I think that even if the phy subsystem had a reset hook it
>> wouldn't be the ideal solution here.  When we assert the PHY "port
>> reset" we're not actually fully resetting the PHY.  We're instead
>> doing some sort of a more minor "state machine" reset in the PHY.
>> This appears better (in my case) than resetting the whole PHY because
>> it doesn't force a de-enumeration / re-enumeration.  Exposing this
>> more minor reset as a PHY reset seems wrong to me.  ...and it also
>> precludes us later also exposing the more full reset through the PHY
>> framework if that later becomes useful.
>
> Doesn't creating a binding also have similar possibility? Maybe an
> "attach host" or re-init hook would be more appropriate. I'm sure
> there are other such cases where the host and phy need more tight
> coupling. I recently had a similar case where there was an interleaved
> sequence of phy and host register writes. I managed rework things and
> avoid changing the phy interface, but there will certainly be cases
> where we can't.
>
> Changing function names in the kernel is easy. Changing the binding
> later not so much. Also as we're looking toward dependency handling,
> this creates a circular dependency. We'll probably have to deal with
> those anyway, but here it seems a bit pointless. We already have a
> connection between the host and phy defined. Let's use that and not
> define another.

I'm probably not understanding what you're proposing.  I was imagining
that you were proposing adding into "include/linux/phy/phy.h" a
definition like:

  int phy_reset(struct phy *phy);

If that existed in the PHY interface, it wouldn't be enough for me
since the "reset" that I'm doing isn't a full reset (and there does
exist another, fuller PHY reset on this SoC).  So if we wanted to use
the PHY interface, I'd need either:

  int phy_reset(struct phy *phy, int reset_id);
...or...
  int phy_reset(struct phy *phy, const char *reset_id);

I was expecting that to stay generic you'd somehow need to add a
mapping of these resets into the device tree because a given USB
controller may have different PHYs on different boards and the same
PHY might be used with more than one USB controller.  If you don't add
some type of mapping in the device tree then in _some_ type of include
file you'd need something like:

#define DWC2_RESET_ID_PHY_PORT_RESET  1
#define DWC2_RESET_ID_PHY_FULL_RESET  2
...or...
#define DWC2_RESET_ID_PHY_PORT_RESET  "phy_port_reset"
#define DWC2_RESET_ID_PHY_FULL_RESET  "phy_full_reset"

...presumably you'd expect that to be in a dwc2 header file, to be
included by the "rk3288 USB PHY"?  ...but, oddly enough, the "rk3288
USB PHY" is not specific to dwc2, so having it include a dwc2 header
file is pretty odd.  Specifically on rk3288 there are 3 instances of
the same PHY.  One is hooked up to the dwc2 "OTG" port.  One is hooked
up to the dwc2 "host" port (which doesn't have device functionality
and also has the need for this strange reset quirk).  One is hooked up
to an EHCI port (using the generic EHCI driver).

...so, while I could add a dwc2 specific include into the rk3288 PHY
driver, I'd need to somehow figure out which of the PHYs it applied
to.  For instance, maybe we later find that we need to activate this
reset for the EHCI port.  Now we'll need to include both headers.
...and hopefully there aren't name / number conflicts.

One note: the "full" PHY reset is actually not in the register map of
the PHY.  It is amazingly enough in the CRU (clock reset unit).  So if
we actually exposed the "full" reset through the PHY framework, the
PHY would then turn around and pass it off to the reset framework,
which is how the full PHY reset is exposed from the CRU.


>> ...we could, of course, re-invent the reset framework (with string or
>> integral IDs so we can assert different types of resets) within the
>> PHY framework.  That doesn't seem ideal to me, but if that's what
>> others want to do then I guess it would be OK...
>
> I think that should already be possible as you can have multiple
> cells. Of course, you have to plan for that with your cell values.

I didn't understand this comment.


Basically: to summarize, I think that we have a reset framework that
handles this beautifully and gets all the corner cases down.  Adding a
second link to the PHY seems totally reasonable to me because I could
totally imagine that this reset could be implemented in a totally
different place in some SoCs.  It happens to be in the PHY in my
particular case, but it need not be on all 

Re: [PATCH 0/4] Patches to fix remote wakeup on rk3288 dwc2 "host" port

[Doug Anderson]

Rob,

On Sat, Oct 24, 2015 at 11:10 AM, Rob Herring  wrote:
> On 10/23/2015 01:28 PM, Douglas Anderson wrote:
>> The "host1" port (AKA the dwc2 port that isn't the OTG port) on rk3288
>> has a hardware errata that causes everything to get confused when we get
>> a remote wakeup.  It appears that the "port reset" bit that's in the USB
>> phy (located in the rk3288 GRF) fixes things up and appears safe to do.
>>
>> This series of patches exports the "port reset" from the PHY and then
>> hooks it up to dwc2 through a quirk.
>>
>> I've tested this series atop a bit of a conglomeration of Heiko's github
>> "somewhat stable" branch (v4.3-rc3-876-g6509232) but with Greg KH's
>> usb-next merged in.
>>
>> These patches currently conflict with patches that I posted previously
>> to enable USB wakeup from S3, specifically:
>> * https://patchwork.kernel.org/patch/6727081/
>> * https://patchwork.kernel.org/patch/6727121/
>> ...those patches no longer apply anyway, so presumably they need to be
>> reposted and I can do so later atop these patches.
>>
>>
>> Douglas Anderson (4):
>>   phy: rockchip-usb: Support the PHY's "port reset"
>>   usb: dwc2: optionally assert phy "port reset" when waking up
>>   ARM: dts: rockchip: Enable the USB phys as reset providers on rk3288
>>   ARM: dts: rockchip: Point rk3288 dwc2 usb at phy port reset
>>
>>  .../devicetree/bindings/phy/rockchip-usb-phy.txt   |  6 ++
>>  Documentation/devicetree/bindings/usb/dwc2.txt |  7 ++
>>  arch/arm/boot/dts/rk3288.dtsi  |  8 +++
>>  drivers/phy/phy-rockchip-usb.c | 74 
>> ++
>>  drivers/usb/dwc2/core.h|  5 ++
>>  drivers/usb/dwc2/core_intr.c   |  7 ++
>>  drivers/usb/dwc2/platform.c| 13 
>>  7 files changed, 120 insertions(+)
>
> A DT reset controller seems like a bit of an overkill here. I think this
> would be much more simple if we just add a phy reset hook to the phy
> subsystem.

Adding a reset hook in the PHY subsystem does seem like a reasonable
idea to me.  I was considering it in an earlier version of this series
that actually used a reset of the PHY to the fix the stuck dwc2.

...but I think that even if the phy subsystem had a reset hook it
wouldn't be the ideal solution here.  When we assert the PHY "port
reset" we're not actually fully resetting the PHY.  We're instead
doing some sort of a more minor "state machine" reset in the PHY.
This appears better (in my case) than resetting the whole PHY because
it doesn't force a de-enumeration / re-enumeration.  Exposing this
more minor reset as a PHY reset seems wrong to me.  ...and it also
precludes us later also exposing the more full reset through the PHY
framework if that later becomes useful.

...we could, of course, re-invent the reset framework (with string or
integral IDs so we can assert different types of resets) within the
PHY framework.  That doesn't seem ideal to me, but if that's what
others want to do then I guess it would be OK...

-Doug
--
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 3/3] ARM: dts: rockchip: Add the OTP gpio pinctrl

[Doug Anderson]

Hi,

On Fri, Oct 23, 2015 at 4:25 AM, Caesar Wang  wrote:
> Add the "init" anf "sleep" pinctrl as the OTP gpio state.
> We need the OTP pin is gpio state before resetting the TSADC controller,
> since the tshut polarity will generate a high signal.
>
> "init" pinctrl property is defined by Doug's Patch[0].
>
> Patch[0]:
> https://patchwork.kernel.org/patch/7454311/
>
> Signed-off-by: Caesar Wang 
> Reviewed-by: Douglas Anderson 
> ---
>
> Changes in v4: None
> Changes in v3:
>   - Add the "sleep" pinctrl as the gpio state in PATCH[3/3]
>
> Changes in v2:
>   - Add some commits for more obvious in PATCH[2/2]
>
> Changes in v1:
>   - As the Doug comments, drop the thermal driver patchs since
> we can with pinctrl changing to work.
>   - As the Doug's patch to add the 'init' property.
>
>  arch/arm/boot/dts/rk3288.dtsi | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

I realized that the subject of this patch should probably contain the
word rk3288, but I presume Heiko would rather add that himself than
for you to spin this again.  ;)


-Doug
--
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: [RESEND PATCH v2 1/2] dt-bindings: rockchip-thermal: Add the "init" pinctrl in this document

[Doug Anderson]

Caesar,

On Wed, Oct 21, 2015 at 7:30 PM, Caesar Wang  wrote:
> The "init" pinctrl is defined we'll set
> pinctrl to this state before probe and then "default" after probe.
>
> Add the "init" pinctrl as the OTP gpio state, since we need switch
> the pin to gpio state before the TSADC controller is reset.
>
> As I know, the TSADC controller is reset, the tshut polarity will be
> a *low* signal in a short period of time for some devices.
>
> Says:
> The TSADC get the temperature on rockchip thermal.
>
> If T(current temperature) < (setting temperature), the OTP output the
> *high* signal.
> If T(current temperature) > (setting temperature), the OTP output the
> *low* Signal.
>
> In some cases, the OTP pin is connected to the PMIC, maybe the
> PMIC can accept the reset response time to avoid this issue.
>
> In other words, the system will be always reboot if we make the
> OTP pin is connected the others IC to control the power.
>
> Signed-off-by: Caesar Wang 
> Reviewed-by: Douglas Anderson 
> ---
>
> Changes in v2:
>   - Add the 'init' pinctrl more decription in commit.
>   - Fix the subject to make more obvious in PATCH[1/2]
>   - Resend this patch v2 since fix the subject to be specific.
>
> Changes in v1:
>   - As the Doug comments, add the 'init' property to sync document.
>
>  Documentation/devicetree/bindings/thermal/rockchip-thermal.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt 
> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> index ef802de..28e84f7 100644
> --- a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt

I think Rob wanted something more in the "rockchip-thermal.txt"
document itself.  Like:

Pinctrl states:
- During device probe a driver may glitch the output line.  If this is
not acceptable for your board, you can use the standard "init" and
"default" pinctrl states.  The "init" state will be set before device
probe and "default" after.
--
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: [REPOST PATCH 0/3] dwc2 patches to allow wakeup on Rockchip rk3288

[Doug Anderson]

John,

On Mon, Jul 6, 2015 at 11:27 AM, Douglas Anderson  wrote:
> This series of patches, together with
>  from Chris Zhong and a
> dts change allow us to wake up from a USB device on rk3288 boards.
> The patches were tested on rk3288-jerry in the chromeos-3.14 kernel.
> The chromeos-3.14 kernel tested included a full set of dwc2 backports
> from upstream, so this is expected to function upstream once we get
> everything setup there.
>
>
> Douglas Anderson (3):
>   USB: Export usb_wakeup_enabled_descendants()
>   Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
>   USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled
>
>  Documentation/devicetree/bindings/usb/dwc2.txt |  4 +++
>  drivers/usb/core/hub.c |  7 ++--
>  drivers/usb/dwc2/core.h|  2 ++
>  drivers/usb/dwc2/platform.c| 45 
> --
>  include/linux/usb/hcd.h|  5 +++
>  5 files changed, 58 insertions(+), 5 deletions(-)

This series was posted a while ago.  Do you have any comments on it?
Should I repost it?

Thanks!

-Doug
--
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 v1 2/2] ARM: dts: rockchip: Add the OTP gpio pinctrl

[Doug Anderson]

Caesar,

On Tue, Oct 20, 2015 at 9:42 PM, Caesar Wang  wrote:
> I think the description is right,   maybe need other decriptions.
> The tshut polarity is low in a short period of time when the TSADC
> controller is reset.
>
> In other words,
>
> If T < (setting temperature), the OTP output the High Signal. --> if the
> otp out polarity is high, the TSHUT will work.
> If T > (setting temperature), the OTP output the Low Signal.

Ah!  I re-read the cover letter more carefully and now I see.  OK, I
think your current description is fine, then.  ;)

-Doug
--
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 v1 2/2] ARM: dts: rockchip: Add the OTP gpio pinctrl

[Doug Anderson]

Caesar,

On Tue, Oct 20, 2015 at 7:43 PM, Caesar Wang  wrote:
> We need the OTP pin is gpio state before resetting the TSADC controller,
> since the tshut polarity will generate a high signal.

It might or might not be "high" depending on polarity, right?  It's
just possible that it could glitch during probe.  Other than that nit,
this seems fine to me.

If it's not too much trouble it'd be nice if you could spin with the
description change.  Otherwise:

Reviewed-by: Douglas Anderson 
--
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 v1 1/2] dt-bindings: Sync the dts to this document

[Doug Anderson]

Hi,

On Tue, Oct 20, 2015 at 7:42 PM, Caesar Wang  wrote:
> Add the OTP gpio state, we need switch the pin to gpio state
> before the TSADC controller is reset.
>
> Signed-off-by: Caesar Wang 
> ---
>
> Changes in v1:
>   - As the Doug comments, add the 'init' property to sync document.
>
>  Documentation/devicetree/bindings/thermal/rockchip-thermal.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Seems reasonable to me.  I do wonder if we need to make it more
obvious that things might glitch if the "init" pinctrl isn't there?
...probably not too critical unless others think it needs to be more
obvious...

Reviewed-by: Douglas Anderson 
--
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/3] ARM: dts: exynos5422-odroidxu3: Added UHS-I bus speed support

[Doug Anderson]

Hi,

On Mon, Oct 19, 2015 at 3:11 AM, Anand Moon  wrote:
> 1 Drop the cd-gpios changes. pinctrl-0 changes.

Right.


> 2 Fix the regulator changes for vmmc/vqmmc with disable of 
> regulator-always-on;

I'm not totally sure I understand.  You should need to keep vmmc and
vqmmc always on because otherwise you can't do card detection (unless
you poll for card detect).


> 3 Drop this UHS-I changes as if now.

Sounds right.
--
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/3] ARM: dts: exynos5422-odroidxu3: Added UHS-I bus speed support

[Doug Anderson]

Hi,

On Tue, Oct 13, 2015 at 6:06 PM, Alim Akhtar  wrote:
> +Doug
> Hello,
> AFAIR, dw_mmc host controller does support UHS-I [1], specially SDR50
> and SDR104 modes.
>
> [1]: http://www.spinics.net/lists/linux-mmc/msg28186.html
>
> What I remember is, one need to set "broken-cd" property also in order
> to make it work because of the vqmmc and vmmc connection on board. I
> didn't find the link right now, but you can search on the web, there
> was a long discussion about handling this.
> Have not checked it recently, so not sure if this got broken somehow.

Right.  It _shouldn't_ be possible to add "vmmc/vqmmc" supplies to
your DTS (which you do in patch 2/3) and also to use the "gpc2-2" pin
for card detect (even if you configure it as a GPIO).  Once you add
"vmmc/vqmmc" then Linux ought to be turning these regulators off when
no card is plugged in.  Presumably the "vqmmc" regulator is hooked up
to the "VDDQ_MMC2".  If you look in the user manual for 5422 you can
see that "GPC2[2]/SD_2_CDn" has power domain "VDDQ_MMC2".  Thus you
really shouldn't be using that pin when vqmmc is off.  I think at some
point someone claimed that it still worked for them, but nobody could
ever explain why.  Full discussion at


---

In case it matters, comments on stuff from earlier in the thread:

* As people pointed out, exynos5422 certainly supports all these modes
(including DDR50) in the SoC.

* Just because the SoC supports these modes doesn't mean that the
boards do, which is why the SoC .dtsi doesn't include them.  Thus,
this patch is "right" in that it changes a board-specific file.

* As Krzysztof points out this board doesn't "add" support but rather
"enables" support.  The distinction is subtle.

* You might be able to get DDR50 working, but probably better to just
start with SDR modes.  Previously I never attempted to get DDR50 cards
working, so possibly the software needs extra work?


-Doug
--
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: dts: Add Exynos5250 Snow Rev5+ support

[Doug Anderson]

Hi,

On Wed, Sep 30, 2015 at 4:44 PM, Krzysztof Kozlowski
 wrote:
>> Switching the default meaning of "google,snow" to Rev5 is probably not
>> something we'd ever want to do, since it could confuse "rev3" boards
>> (which should be serviced by the rev4 dts).  From comments in the
>> Chrome OS tree:
>>
>> In the "rev4" DTS:
>>  * - Any real rev 0-3 boards in the field will match "google,snow",
>>  *   since older U-Boots don't look for a revision specific device 
>> tree.
>>  * - Any real rev 4 boards in the field will match "google,snow-rev4"
>>  *   first.  If that's not present they will pick the first
>>  *   "google,snow" device tree that they find (ignoring the kernel
>>  *   ordering).
>>
>> In the "rev5" DTS:
>>  * - Purposely don't add "google,snow" so old firmware (which didn't
>>  *   include a -revN suffix) won't pick this one.
>
> These are requirements written somewhere in the downstream tree... not
> in upstream. Have in mind that someone may not be using Chrome OS on
> Chromebook but custom U-Boot and different distro.
>
> You cannot expect that everyone will new some requirements of some
> downstream OS. If you need such requirements, write them in bindings
> documentation *in the upstream*.

Agreed, which is why I bring it up here.  I think Javier wasn't aware
of these when he sent up his patch.  I believe when he adds the
bindings docs he'll put this information.

Previously rev5 wasn't supported upstream and I believe rev5 didn't
even exist when the upstream stuff was submitted.

-Doug
--
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: dts: Add Exynos5250 Snow Rev5+ support

[Doug Anderson]

Hi,

On Tue, Sep 29, 2015 at 5:30 PM, Krzysztof Kozlowski
 wrote:
> Now the exynos5250-snow.dts means in fact Rev4... but there is no
> information in DTS about it. I think adding compatible
> "google,snow-rev4" makes sense:
> 1. For informational purposes (this could be also handled with a comment).
> 2. Later one could decide to switch the default meaning of "google,snow"
> to Rev5 and the real compatible (rev4) will be there already.
>
> Could you add the new compatible and fix patch issues pointed by Doug?

Looks like everything got applied, but just for the record:

Switching the default meaning of "google,snow" to Rev5 is probably not
something we'd ever want to do, since it could confuse "rev3" boards
(which should be serviced by the rev4 dts).  From comments in the
Chrome OS tree:

In the "rev4" DTS:
 * - Any real rev 0-3 boards in the field will match "google,snow",
 *   since older U-Boots don't look for a revision specific device tree.
 * - Any real rev 4 boards in the field will match "google,snow-rev4"
 *   first.  If that's not present they will pick the first
 *   "google,snow" device tree that they find (ignoring the kernel
 *   ordering).

In the "rev5" DTS:
 * - Purposely don't add "google,snow" so old firmware (which didn't
 *   include a -revN suffix) won't pick this one.


-Doug
--
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: dts: Add Exynos5250 Snow Rev5+ support

[Doug Anderson]

Javier,

On Tue, Sep 29, 2015 at 4:57 AM, Javier Martinez Canillas
 wrote:
> There are 2 revisions of the Exynos5250 Snow Chromebook that were shipped:
> Rev4 and Rev5. The only difference between these 2 revisions is the codec,
> Rev4 has a max98095 codec while Rev5 has a max98090.
>
> Mainline only supports Rev4 so this patch moves the common device nodes to
> a DTSI file and adds a DTS for the Exynos5250 Snow Rev5.
>
> The Snow Rev5 DTS is based on the DTS found in the ChromiumOS 3.8 tree.
>
> Signed-off-by: Javier Martinez Canillas 
>
> ---
>
> The DTS in the vendor ChromeOS tree are called exynos5250-snow-rev{4,5}.dtb
> but I decided to leave Rev4 as exynos5250-snow.dtb to avoid breaking u-boot
> that has CONFIG_DEFAULT_DEVICE_TREE="exynos5250-snow" in snow_defconfig.
>
> Also, ChromiumOS Rev4 DTS has "google,snow-rev4" in its compatible string
> but was not added in mainline since Rev4 firmware fallbacks to "google,snow"
> and Rev5 searches for "google,snow-rev5". That way the compatible string
> could be consistent with the DTS naming and still be able to pack both Rev4
> and Rev5 FDT in the same FIT image and let the firmware pick the correct FDT.

Looking at all the notes in the DTS in the ChromeOS tree, this sounds
like it should be fine.


>  arch/arm/boot/dts/Makefile|   1 +
>  arch/arm/boot/dts/exynos5250-snow-common.dtsi | 684 
> ++
>  arch/arm/boot/dts/exynos5250-snow-rev5.dts|  47 ++
>  arch/arm/boot/dts/exynos5250-snow.dts | 666 +
>  4 files changed, 733 insertions(+), 665 deletions(-)

Thanks!  Note:

$ pwclient git-am 7285451
Applying patch #7285451 using 'git am'
Description: ARM: dts: Add Exynos5250 Snow Rev5+ support
Applying: ARM: dts: Add Exynos5250 Snow Rev5+ support
.git/rebase-apply/patch:774: new blank line at EOF.
+
warning: 1 line adds whitespace errors.


One other nit is that the exynos5250-snow.dts" ends up with the
"max98095" pinctrl properties sorted differently than the
"exynos5250-snow-rev5.dts".  Is it worth reordering the
"exynos5250-snow.dts" in the same patch?

Otherwise this looks fine to me.

Reviewed-by: Douglas Anderson 
--
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: dts: Add ddc i2c reference to veyron

[Doug Anderson]

Hi,

On Wed, Sep 2, 2015 at 2:25 PM, Douglas Anderson  wrote:
> The ddc-i2c-bus property was missing from the veyron dtsi file since
> downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and
> nobody noticed when the veyron dtsi was sent upstream.  Add it.
>
> Signed-off-by: Douglas Anderson 
> ---
> Note: I noticed that this was wrong but I don't currently have
> graphics up and running on upstream on veyron.  Posting this anyway
> since it's pretty clear that it's needed.  If someone else wants to
> try it out that'd be nice, otherwise I'll put it on my list to figure
> out how to get myself setup for graphics upstream.  ;)

OK, I've used Heiko's "somewhat stable" branch to test this against
something very close to mainline.

Without my patch I can't read the HDMI EDID.  With my patch I can.  :)

-Doug
--
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: dts: Add ddc i2c reference to veyron

[Doug Anderson]

Lucas,

On Thu, Sep 3, 2015 at 9:13 AM, Lucas Stach  wrote:
>> 6. Once you start using the dw_hdmi's i2c block with the currently
>> posted patch against mainline (to do this you not only need the patch
>> but you need to remove the ddc-i2c-bus property, set the pinmux, and
>> disable i2c5) then you'll see a bonafide i2c bus exposed to Linux.  In
>> my case this stole i2c0 away from the builtin SoC I2C bus and caused
>> the SoC I2C bus to fail to probe.  Doh.
>>
> This shouldn't happen. I don't know if the patches landed yet, but I
> know Wolfram (i2c maintainer) had patches to reserve the range of bus
> numbers that are fixed by alias nodes and don't hand out those numbers
> to adapters with a dynamic bus number allocation.
>
>> 7. I was trying to solve #6 by adding an "of_node" to the i2c bus
>> which allowed me to give it a (non-conflicting) bus ID.
>>
> This should not be needed.

Ah.  It's very possible that the issue is due to the fact that I was
testing this on an old kernel (3.14) that's missing that feature.  OK,
if that's fixed then at least the actual bug (making i2c0 fail to
load) is not a problem.  It'd still be nice to be able to assign a bus
number to this bus just to make reading of the logs easier, but it's
not a functionality requirement.

-Doug
--
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: dts: Add ddc i2c reference to veyron

[Doug Anderson]

Hi,

On Thu, Sep 3, 2015 at 8:46 AM, Rob Herring  wrote:
> On Thu, Sep 3, 2015 at 10:18 AM, Russell King - ARM Linux
>  wrote:
>> On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote:
>>> Yes, that is fairly common (ADV75xx is same), and we would not
>>> describe an I2C bus in DT in that case. Same with HPD directly handled
>>> vs. a GPIO line. That is no different than what Doug has said:
>>> ddc-i2c-bus is present if using the SOC's I2C host and absent if using
>>> the HDMI block's DDC functionality. I'm only questioning the location
>>> of the property.
>>
>> No, I don't think that's what Doug wants.  Doug wants the bridge's
>> internal I2C host to be exposed, so he can number it through a DT
>> alias.
>
> See his earlier reply and other patch[1] which states once the dw_hdmi
> built-in I2C controller support is added in mainline, then this
> property is not needed. For now, the SOC's general purpose I2C
> controller is used.
>
> Rob
>
> [1] https://lkml.org/lkml/2015/9/2/571

Hmmm, I think we're getting all mixed up here.  To summarize:

1. On rk3288 you can mux the same pins on the SoC to _either_ be
controlled by a generic rk3288 i2c controller (i2c5) or controlled by
the dw_hdmi's i2c block.

2. At the moment, there's no code in mainline to handle the dw_hdmi's i2c block.

3. Right now there _is_ code in mainline to handle specifying
"ddc-i2c-bus" and have it point to the generic rk3288 i2c controller.

4. So in mainline if you want to read an EDID, you've got to configure
the pinmux as "i2c5" and add a "ddc-i2c-bus" reference to the HDMI
section of the device tree.  That's what most rk3288 boards do and (as
far as I understand) matches existing bindings.  The only reason
veyron didn't have this reference was due to a small oversight when
sending the DTS file upstream.

5. There are apparently benefits to using the builtin i2c controller
in dw_hdmi.  There's an outstanding patch add code to support the
dw_hdmi's i2c block.

6. Once you start using the dw_hdmi's i2c block with the currently
posted patch against mainline (to do this you not only need the patch
but you need to remove the ddc-i2c-bus property, set the pinmux, and
disable i2c5) then you'll see a bonafide i2c bus exposed to Linux.  In
my case this stole i2c0 away from the builtin SoC I2C bus and caused
the SoC I2C bus to fail to probe.  Doh.

7. I was trying to solve #6 by adding an "of_node" to the i2c bus
which allowed me to give it a (non-conflicting) bus ID.


-Doug
--
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: dts: Add ddc i2c reference to veyron

[Doug Anderson]

Hi,

On Thu, Sep 3, 2015 at 8:18 AM, Russell King - ARM Linux
 wrote:
> On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote:
>> Yes, that is fairly common (ADV75xx is same), and we would not
>> describe an I2C bus in DT in that case. Same with HPD directly handled
>> vs. a GPIO line. That is no different than what Doug has said:
>> ddc-i2c-bus is present if using the SOC's I2C host and absent if using
>> the HDMI block's DDC functionality. I'm only questioning the location
>> of the property.
>
> No, I don't think that's what Doug wants.  Doug wants the bridge's
> internal I2C host to be exposed, so he can number it through a DT
> alias.

Actually, my primary concern is that it _doesn't_ conflict with the DT
aliases for busses I care about.  If I didn't provide it with a number
the i2c subsystem was assigning 0 which was conflicting with the real
i2c bus 0.  If the bus was simply not exposed that would have solved
my problem just fine.  ...but if it is being exposed, we need a way to
give it a number.

That being said, my (uninformed) opinion is that if the hardware does
provide enough functionality to expose an i2c bus it's convenient to
expose it, even if it's under a DEBUG config option.  That allows you
to use standard i2c tools during bringup or to debug strange problems.

Obviously since there is hardware that doesn't expose a full i2c bus
then the abstraction should handle that and let a driver just provide
the data directly.

-Doug
--
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: dts: Add ddc i2c reference to veyron

[Doug Anderson]

Rob,

On Wed, Sep 2, 2015 at 5:13 PM, Rob Herring  wrote:
> On Wed, Sep 2, 2015 at 4:25 PM, Douglas Anderson  
> wrote:
>> The ddc-i2c-bus property was missing from the veyron dtsi file since
>> downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and
>> nobody noticed when the veyron dtsi was sent upstream.  Add it.
>>
>> Signed-off-by: Douglas Anderson 
>> ---
>> Note: I noticed that this was wrong but I don't currently have
>> graphics up and running on upstream on veyron.  Posting this anyway
>> since it's pretty clear that it's needed.  If someone else wants to
>> try it out that'd be nice, otherwise I'll put it on my list to figure
>> out how to get myself setup for graphics upstream.  ;)
>
> Based on your other patch, this is temporary, right?

Yes, though since I'm not personally working on the other patch series
upstream I can't say for how long the "temporary" is..  I mostly
posted the 2nd patch because it was clearly correct to add some
pinmuxing states and could land any time, so I thought I'd be helpful.

You're right that in the Chrome OS tree I turned right around and
effectively removed the "ddc-i2c-bus", but having it land first adds a
much better logical progression (make it the same as everyone else and
_then_ change it).  It also provides a revert path if something goes
wrong.  :)


> I've been looking at DRM a lot lately. I think specifying the i2c bus
> in the hdmi chip or IP block node is wrong. If the I2C host is
> separate from the HDMI block, then it's only connection is to the HDMI
> connector. So the I2C host to the connector relationship is what the
> DT should describe. HPD gpio is similar. Now if the HDMI bridge
> controls DDC and HPD directly, then we don't need to describe those
> connections.

I will say that I know very very little about DRM.  Mostly I just
visit it when there's some bug I'm running into that I can't find a
better suited owner for.  ;)

I'm not sure I followed your whole paragraph.  Could you give a
fragment of DTS for how you'd imagine this ought to work?  Also: the
patch I submitted does match the current bindings if I understand it
right.  ...as is typical with device tree, if we want to change the
bindings we've got to have a really good reason because we'd either
need to figure out how to deal with existing DTBs in the field that
need to run with newer kernels (if those exist).

-Doug
--
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/2] ARM: dts: rockchip: Remove specific cts pullup from veyron

[Doug Anderson]

Alex,

On Wed, Sep 2, 2015 at 4:27 PM, Alexandru M Stan  wrote:
> With the previous patch ("rk3288: pull up cts lines") this is redundant,
> I sent that patch for the same reason this existed here, so the lines don't
> wiggle randomly when disconnected.
>
> Signed-off-by: Alexandru M Stan 
> ---
> Changes in v2:
> - Restrict changes only to cts pin, leave rts alone
> - CC people with the firefly board
> - New patch removing redundant pullup code from veyron
> - cover letter
>
>  arch/arm/boot/dts/rk3288-veyron.dtsi | 12 
>  1 file changed, 12 deletions(-)

Reviewed-by: Douglas Anderson 
--
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] ARM: dts: rockchip: pull up cts lines on rk3288

[Doug Anderson]

Alex,

On Wed, Sep 2, 2015 at 4:27 PM, Alexandru M Stan  wrote:
> The flow control lines from a user accessible UART are optional,
> the user might not have anything connected to those pins.
> In order to prevent random interrupts happening and noise affecting
> the cts pin should be pulled up.
>
> Note that the default state for that pin on the rk3288 is pulled up,
> so this patch merely restores them.
>
> This is similar to what we're already doing with the RX pin,
> so it should be safe. At worst it might be a slightly higher power usage
> (through ~50 kohms) when the cts is low.
>
> Suggested-by: Neil Hendin 
> Signed-off-by: Alexandru M Stan 
> ---
> Changes in v2: None

Probably not entirely true.  ;)

>  arch/arm/boot/dts/rk3288.dtsi | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Looks good to me.

Reviewed-by: Douglas Anderson 
--
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: dts: rk3288: pull up cts lines

[Doug Anderson]

Alex,

On Wed, Sep 2, 2015 at 3:20 PM, Alexandru M Stan  wrote:
> The flow control lines from a user accessible UART are optional,
> the user might not have anything connected to those pins.
> In order to prevent random interrupts happening and noise affecting
> that pin it should be pulled up.
>
> Note that the default state for those pins on the rk3288 is pulled up,
> so this patch merely restores them.
>
> Suggested-by: Neil Hendin 
> Signed-off-by: Alexandru M Stan 
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 906e938..a059367 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -1211,11 +1211,11 @@
> };
>
> uart0_cts: uart0-cts {
> -   rockchip,pins = <4 18 RK_FUNC_1 
> &pcfg_pull_none>;
> +   rockchip,pins = <4 18 RK_FUNC_1 
> &pcfg_pull_up>;
> };
>
> uart0_rts: uart0-rts {
> -   rockchip,pins = <4 19 RK_FUNC_1 
> &pcfg_pull_none>;
> +   rockchip,pins = <4 19 RK_FUNC_1 
> &pcfg_pull_up>;

I would probably do just CTS.  That would match the TX / RX lines
where the RX is pulled but not the TX.

You might also mention in the commit message that this really should
be safe since we're already doing something similar for RX.


-Doug
--
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: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

[Doug Anderson]

Mike,

On Fri, Aug 28, 2015 at 1:02 PM, Michael Turquette
 wrote:
> Hi Doug,
>
> Quoting Doug Anderson (2015-08-27 19:03:20)
>> Kevin,
>>
>> On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman  wrote:
>> >> That is not really workable: the attach and detach happen in
>> >> probe/remove path; if you do not have driver for the device you will
>> >> miss the clocks for it.
>> >
>> > And in my proposal, I suggested that clocks without drivers are
>> > good candidates to list in the domain, with the caveat that the be
>> > called out (documented) as being device clocks that are missing a
>> > driver, so when a driver shows up they can be moved accordingly, and in
>> > a way that actually describes the hardware.
>>
>> What happens if someone disables the driver using the CONFIG subsystem?
>
> Kevin asked me to chime in on this thread, as I have a half-baked idea
> that might solve the problem posed by your question above.
>
> One thing I have been considering for a while is a fallback compatible
> string that can be used for an IP block when either there is no driver
> loaded or no driver exists at all. Something like "generic-ip-block".
>
> The purpose of this compatible string is to allow us to model resource
> consumption in dts accurately, regardless of whether or not a proper
> driver has been written in Linux. This idea was born out of the
> simple-fb binding/driver discussion last year[0].
>
> Obviously such a binding would not enable any of the logic or function
> of that IP block; that would require a proper driver. But it would allow
> us to properly link system-wide resources that are consumed: the
> generic-ip could consume clocks and regulators, it could belong to power
> domains, etc. For this reason I have also thought that
> "generic-resource-consumer" is an accurate compatible string.
>
> This spares us from having to encode nasty details into the power domain
> binding, which is exactly what would happen if you needed a dedicated
> list of clocks in the power domain node that were not claimed by device
> nodes/drivers.
>
> Note that a real driver might exist for an IP block, but if that driver
> is disabled in Kconfig AND the corresponding dt node has this fallback
> compatible string, then we could be OK, from the perspective of the
> power domain problem.

OK, so that could solve the "Kconfig" problem.


>> What happens if this is a device that someone has set to 'status =
>> "disabled";' in the device tree?
>
> If someone does that, then I think we should let that break power domain
> transitions.

So if you're on a board that doesn't use EDP but uses LVDS then their
suspend/resume should be broken?

Specifically, it's my understanding that on this SoC:

1. There's a single "video IO" power domain that contains things like
the video output processor, EDP, LVDS, HDMI, etc.

2. When you turn on/off the power domain it's important to clock all
devices in the domain during the reset.

3. If you don't clock all devices during the reset they get left in a
"wedged" state.

4. If you try to do things like suspend/resume it will query all
devices.  If they've been left in the wedged state then suspend/resume
will be blocked.

...so this problem is still not solved.

>> Even if the device is disabled in one of those two ways, we still need
>> the clocks to be turned on.  ...so if we turn on/off the VIO domain we
>> need to turn on the EDP clock even if there's no EDP in the current
>> board / config.  We might turn on/off VIO for one of the other devices
>> in the VIO domain for one of the other devices in VIO that we are
>> using.
>
> I'm hesitant to mention this but I am working on a patch series to
> implement a clock "handoff" mechanism (also inspired by the simplefb
> discussion). This allows us to set a per-clock flag that tells the
> framework to enable that clock at registration time, and then the first
> clock consumer driver to come along and claim that clock inherits that
> clock enable reference count.
>
> I'm working on v2 that lets us set this flag from DT, but I really only
> plan to do this for special cases. For the normal case the flag should
> be set in the Linux clock provider driver. In the mean time v1 is under
> discussion[1].

In this case if we're using LVDS and HDMI on a system, we don't want
the EDP clock always on (that just wastes power).  We just need to
clock it for the power domain transition.


Note that nothing in the above solves the problem that not all clocks
for a given device need to be turned on during power domain
transitio

Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

[Doug Anderson]

Kevin,

On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman  wrote:
>> That is not really workable: the attach and detach happen in
>> probe/remove path; if you do not have driver for the device you will
>> miss the clocks for it.
>
> And in my proposal, I suggested that clocks without drivers are
> good candidates to list in the domain, with the caveat that the be
> called out (documented) as being device clocks that are missing a
> driver, so when a driver shows up they can be moved accordingly, and in
> a way that actually describes the hardware.

What happens if someone disables the driver using the CONFIG subsystem?

What happens if this is a device that someone has set to 'status =
"disabled";' in the device tree?

Even if the device is disabled in one of those two ways, we still need
the clocks to be turned on.  ...so if we turn on/off the VIO domain we
need to turn on the EDP clock even if there's no EDP in the current
board / config.  We might turn on/off VIO for one of the other devices
in the VIO domain for one of the other devices in VIO that we are
using.

-Doug
--
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] ARM: dts: rockchip: add veyron-jaq board

[Doug Anderson]

Brian,

On Mon, Aug 24, 2015 at 3:58 PM, Brian Norris  wrote:
> a.k.a. Haier Chromebook 11, and others
>
> Signed-off-by: Brian Norris 
> Cc: Alexandru M Stan 
> Cc: Douglas Anderson 
> Reviewed-by: Javier Martinez Canillas 
> ---
> v2 -> v3:
>  - add "and others" language
>  - drop other patches from series (picked up already by Heiko)
>
> v1 -> v2:
>  - add overlooked DT binding doc
>  - fixup regulator suspend state for LDO_REG2
>
>
>  Documentation/devicetree/bindings/arm/rockchip.txt |   7 +
>  arch/arm/boot/dts/Makefile |   1 +
>  arch/arm/boot/dts/rk3288-veyron-jaq.dts| 176 
> +
>  3 files changed, 184 insertions(+)

Reviewed-by: Douglas Anderson 
--
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/2] ARM: dts: rockchip: correct regulator PM properties

[Doug Anderson]

Hi,

On Tue, Aug 18, 2015 at 11:19 PM, Heiko Stuebner  wrote:
> great, just take into account the deep vs. shallow suspend modes :-)

One note: do you think it would make sense to re-implement shallow
suspend as "standby"?  I had a proof of concept doing that in
.  One nice
advantage is that you "magically" get a second set of regulator states
for standby vs "mem".

If I understand correctly, the distinction between "standby" and "mem"
is not too clearly defined, so if we wanted to use it for this it
wouldn't be terrible?

-Doug
--
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: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

[Doug Anderson]

Kevin,

On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman  wrote:
>> To put things in a
>> concrete way, for pd_vio we'd go through the entire device tree
>> ourselves and find all properties that look like "power-domains =
>> <&power RK3288_PD_VIO>;".  We'd then find the parent of those
>> properties and look for a property named "clocks".  We'd then iterate
>> over all those clocks and turn those on.  Did I get that right?
>
> ... but you make it sound like more work than it is.  The genpd already
> keeps a list of devices that refer to the power domain.  In fact, the
> genpd 'attach' method can be platform-specific, so could be used to keep
> track of a list (or a subset) of clocks which are needed for reset.

OK.  I'll need to dig through and figure out how this works.  So this
list will include devices whose drivers are compiled out and also
devices that are marked as 'status = "disabled";' in the device tree?
It would need to do this in case someone had a board that didn't use
one of these peripherals (since we still need to clock it as we make
the domain go up and down).

I took a quick gander and didn't see code that would do this.  Can you
give me a quick pointer?

We also need to really make sure that there are no probe order issues...


>> 2. If we absolutely need to turn all clocks and we get clocks from
>> device tree nodes on then it means we need device tree nodes for every
>> device in the domain.
>
> Isn't that the end goal?

In the ideal world, yes.  ...but it's possibly years before all
drivers are added, or possibly they never will be.  To name a concrete
example, unless I'm mistaken, the "hardware crypto" device present in
exynos5250 still isn't anywhere in upstream despite several years
having passed.  There are just so many devices in these SoCs that
aren't used.


>> These would be needed even if there are no
>> accepted bindings for this device yet.  So we'd need to do one of: A)
>> Block power domain patches on feature complete bindings for all
>> drivers; B) Make up non-approved compatible strings for all devices
>> and throw them into the DTS; C) Add nodes in the DTS without a
>> compatible string just to satisfy the power domain requirements.  None
>> of these seem terribly appealing.
>
> well, I think we can be slightly more accomodating than that and go for
> somewhere in between:
>
> D) In the power-domain DTS, clearly describe why each clock is needed by
> the power-domain.  In particular list out clocks that are not device
> clocks (and why they need to be asserted for reset) and separate those
> from device clocks which are only listed in the power-domain because
> there is not *yet* a driver/binding for that device node.
>
> Doing it that way also makes it clear that when a new driver/binding is
> added, the clocks should be removed from the power-domain node and put
> into the device node.
>
> Also, this addresses my primary concern that the DTS acurately describes
> the hardware.  IOW, in hardware, most of these clocks are are properties
> of devices, not power-domains, and the DT should reflect that.
>
> IMO, if it's not describing the hardware, and is a placeholder until a
> driver/binding is in place, it should be properly documented.

Agreed that more documentation about why each clock is needed would go
a long way to helping.

I'd say that the clocks are properties of the device, but I'd again
assert that not all clocks that are owned by the device are relevant
to turning power domains on and off.  If you really wanted to be
correct, you'd add a property like this to devices:

power-on-clocks = <..>;

...this would be a list of clocks needed to power on the device
properly.  If such a property was added that would erase my objection
to that.  Note that you could say that if "power-on-clocks" wasn't
present then you could fall back to "clocks".  I do worry a little bit
about over-engineering, so I guess this could just be a future
improvement...


>> 3. It is entirely possible that there are clocks that will be listed
>> in the individual devices that aren't needed for powering on the power
>> domain.  I'd tend to believe that PCLK_EDP_CTRL (the pixel clock)
>> doesn't really need to be turned on when adjusting the "VIO" power
>> domain.  Right now Caesar has it listed, but it probably isn't needed
>> (Caesar: can you confirm?).
>
> Yes, I suspect there are several of those, which is also why I'd like to
> see the clocks in the power-domain node described in detail, and exacly
> why they're needed to be enabled.

I guess my point was that this was a reason for doing something like
"power-on-clocks" because it would let you avoid turning on
PCLK_EDP_CTRL (which is listed in the "clocks" of the EDP device but
maybe not needed while powering on/off the domain).  ...or just have
the list as part of the power domain.  :-P


> Also, in the current proposed DTS, clocks that are listed in both device
> nodes and the power domain are also suspicous, especially when the
> device

Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

[Doug Anderson]

Kevin,

On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman  wrote:
> Caesar Wang  writes:
>
>> We can add more domains node in the future.
>> This patch add the needed clocks into power-controller.
>> As the discuess about all the device clocks being listed in
>> the power-domains itself.
>>
>> 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.
>
> Yes, but this is the job of device drivers which are runtime PM adapted
> to gate their own clocks.  I agree these clocks need to be enumerated in
> the DTS, but they should be in the device nodes.

I _think_ what Caesar means is that the alternative to this patch is
to leave the clocks on all the time as they were during the early days
of Rockchip (AKA last year).  If the clocks are on all the time then
the power domain patches can work fine.  However, once you start
letting clocks turn off then you need to make sure that the power
domain code turns the back on temporarily.


>> Secondly, the reset-circuit should reset be synchronous on RK3288,
>> then sync revoked. So we need to enable clocks of all devices.
>> In other words, we have to enable the clocks before you operate them
>> if all the device clocks are included in someone domians.
>
> Yes, this is pretty common for reset.
>
>> Someone wish was to get the clocks by reading the clocks from the
>> device nodes, We can do that but we can solve the above issues.
>
> I don't follow this sentence.  Are you saying doing that will not solve
> the above issues?  Why not?  Please explain.
>
> If there are non-device clocks that also need to be enabled before
> asserting reset, then those are candidates for the power-domain node,
> but not device clocks.

It's been a long time and I don't know that I've reviewed every
revision of this series, but I think there was a proposal that we
shouldn't list clocks here.  Instead we should search through and find
all devices that refer to this power domain, reach in and find their
clocks, and turn them on.  Did I get that right?  To put things in a
concrete way, for pd_vio we'd go through the entire device tree
ourselves and find all properties that look like "power-domains =
<&power RK3288_PD_VIO>;".  We'd then find the parent of those
properties and look for a property named "clocks".  We'd then iterate
over all those clocks and turn those on.  Did I get that right?

The above doesn't seem like a terribly great idea to me for a number
of reasons, including:

1. If I remember correctly, it's important to turn on clocks for
devices even if they're not something you're using / have a driver
for.  If you don't then the device won't get reset properly and this
can affect things like suspend/resume because the hardware in the SoC
will query all devices at suspend time to make sure they're ready.  If
a device is wedged because its clock wasn't on at the right them then
it will cause problems.

2. If we absolutely need to turn all clocks and we get clocks from
device tree nodes on then it means we need device tree nodes for every
device in the domain.  These would be needed even if there are no
accepted bindings for this device yet.  So we'd need to do one of: A)
Block power domain patches on feature complete bindings for all
drivers; B) Make up non-approved compatible strings for all devices
and throw them into the DTS; C) Add nodes in the DTS without a
compatible string just to satisfy the power domain requirements.  None
of these seem terribly appealing.

3. It is entirely possible that there are clocks that will be listed
in the individual devices that aren't needed for powering on the power
domain.  I'd tend to believe that PCLK_EDP_CTRL (the pixel clock)
doesn't really need to be turned on when adjusting the "VIO" power
domain.  Right now Caesar has it listed, but it probably isn't needed
(Caesar: can you confirm?).

4. It seems just slightly brittle to be reaching into other device
nodes and making assumptions about their properties.  Yeah, it's
probably safe to assume that "clocks" has a list of clocks and
"power-domains" will point to something whose first entry is a
phandle, but it still seems just a tad bit like violating an
abstraction barrier.


Anyway, perhaps I'm misunderstanding, or perhaps my concerns are
simply not for important things.  If so feel free to yell at me.  ;)


>> Anyway, the best ideas we can fix it in the future SoCs.
>
> I don't think this is an SoC design issue as this is needed when you
> have synchronous reset.  My concern is primarily around how to describe
> this in the DT.

I suppose the SoC could override things and make sure clocks are on in
this case?  ...or if a clock is off it could defer powering it up
somehow until the clock came on?  ...dunno how this actually looks in
hardware.  In any case letting devices get wedged doesn't seem
ideal...

-Doug
--
To uns

Re: [RFC PATCH v5 1/9] mmc: dw_mmc: Add external dma interface support

[Doug Anderson]

Heiko,

On Fri, Aug 14, 2015 at 3:13 PM, Heiko Stübner  wrote:
> Hi Shawn,
>
> Am Freitag, 14. August 2015, 16:34:35 schrieb Shawn Lin:
>> DesignWare MMC Controller can supports two types of DMA
>> mode: external dma and internal dma. We get a RK312x platform
>> integrated dw_mmc and ARM pl330 dma controller. This patch add
>> edmac ops to support these platforms. I've tested it on RK312x
>> platform with edmac mode and RK3288 platform with idmac mode.
>>
>> Signed-off-by: Shawn Lin 
>
> judging by your "from", I guess you're running this on some older Rockchip soc
> without the idma? Because I tried testing this on a Radxa Rock, but only got
> failures, from the start (failed to read card status register). In PIO mode
> everything works again.
>
>
> I guess I overlooked just some tiny detail, but to me the dma channel ids seem
> correct after all. Maybe you have any hints what I'm doing wrong?

If I were a guessing man (which I'm not), I'd guess that perhaps
you're running into troubles with our friend the PL330.

There appear to be strange issues with the PL330 on Rockchip SoCs.  I
was only peripherally involved with them, but I know at least about
some of the patches in our tree, like:

https://chromium-review.googlesource.com/237607
FROMLIST: DMA: pl330: support burst mode for dev-to-mem and mem-to-dev transmit

https://chromium-review.googlesource.com/237393
CHROMIUM: dmaengine: pl330: support quirks for some broken

https://chromium-review.googlesource.com/237396
CHROMIUM: dmaengine: pl330: add quirk for broken no flushp

https://chromium-review.googlesource.com/237394
CHROMIUM: ARM: dts: rockchip: Add broken-no-flushp into rk3288.dtsi

https://chromium-review.googlesource.com/242063
CHROMIUM: ASoC: rockchip_i2s: modify DMA max burst to 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 v1 2/3] Documentation: dt-bindings: add dt binding info for dwc2 reset control

[Doug Anderson]

lyz,

On Tue, Aug 11, 2015 at 12:56 AM, Yunzhi Li  wrote:
> Signed-off-by: Yunzhi Li 
> ---
>
>  Documentation/devicetree/bindings/usb/dwc2.txt | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt 
> b/Documentation/devicetree/bindings/usb/dwc2.txt
> index fd132cb..6a84099 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -24,6 +24,12 @@ Refer to phy/phy-bindings.txt for generic phy consumer 
> properties
>  - g-rx-fifo-size: size of rx fifo size in gadget mode.
>  - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode.
>  - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in 
> gadget mode.
> +- resets: A list of phandle + reset-specifier pairs for the resets listed in
> +  reset-names
> +- reset-names: Should contain the following:
> +  "ahb": AHB interface reset
> +  "phy": PHY reset
> +  "con": controller reset

Would it be worth it to leave the "phy" out?  You don't use it in the
driver yet and it's unclear to me whether it belongs here or as part
of "rockchip,rk3288-usb-phy"?

-Doug
--
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/2] ARM: dts: rockchip: Add veyron-speedy board

[Doug Anderson]

Hi,

On Tue, Jul 21, 2015 at 10:44 PM, Romain Perier  wrote:
> Which is formally known as The Asus C201 chromebook
>
> Signed-off-by: Romain Perier 
> ---
>  Documentation/devicetree/bindings/arm/rockchip.txt |  10 +-
>  arch/arm/boot/dts/Makefile |   3 +-
>  arch/arm/boot/dts/rk3288-veyron-speedy.dts | 163 
> +
>  3 files changed, 174 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/boot/dts/rk3288-veyron-speedy.dts
>
> diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt 
> b/Documentation/devicetree/bindings/arm/rockchip.txt
> index 6de18bd2..da02022 100644
> --- a/Documentation/devicetree/bindings/arm/rockchip.txt
> +++ b/Documentation/devicetree/bindings/arm/rockchip.txt
> @@ -37,4 +37,12 @@ Rockchip platforms device tree bindings
>  - Google Pinky (dev-board):
>  Required root node properties:
>- compatible = "google,veyron-pinky-rev2", "google,veyron-pinky",
> -"google,veyron", "rockchip,rk3288";
> \ No newline at end of file
> +"google,veyron", "rockchip,rk3288";
> +
> +- Google Speedy (Asus C201 Chromebook):
> +Required root node properties:
> +  - compatible = "google,veyron-speedy-rev9", 
> "google,veyron-speedy-rev8",
> +"google,veyron-speedy-rev7", "google,veyron-speedy-rev6",
> +"google,veyron-speedy-rev5", "google,veyron-speedy-rev4",
> +"google,veyron-speedy-rev3", "google,veyron-speedy-rev2",
> +"google,veyron-speedy", "google,veyron", 
> "rockchip,rk3288";
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 653ede7..bfa7c86 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -498,7 +498,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += \
> rk3288-firefly-beta.dtb \
> rk3288-firefly.dtb \
> rk3288-veyron-jerry.dtb \
> -   rk3288-veyron-pinky.dtb
> +   rk3288-veyron-pinky.dtb \
> +   rk3288-veyron-speedy.dtb
>  dtb-$(CONFIG_ARCH_S3C24XX) += \
> s3c2416-smdk2416.dtb
>  dtb-$(CONFIG_ARCH_S3C64XX) += \
> diff --git a/arch/arm/boot/dts/rk3288-veyron-speedy.dts 
> b/arch/arm/boot/dts/rk3288-veyron-speedy.dts
> new file mode 100644
> index 000..6578e2a
> --- /dev/null
> +++ b/arch/arm/boot/dts/rk3288-veyron-speedy.dts
> @@ -0,0 +1,163 @@
> +/*
> + * Google Veyron Speedy Rev 1+ board device tree source
> + *
> + * Copyright 2015 Google, Inc
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file 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.
> + *
> + * This file 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.
> + *
> + *  Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */

License change is fine.


> +&rk808 {
> +   pinctrl-names = "default";
> +   pinctrl-0 = <&pmic_int_l &dvs_1 &dvs_2>;
> +   dvs-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>,
> +   <&gpio7 15 GPIO_ACTIVE_HIGH>;
> +};

As Heiko said, dvs GPIOs aren't accepted yet.

Also a nit that you changed the sort ordering of "disable-wp" compared
to the jerry DTS.  Please make it match.

> +   edp {
> +   edp_hpd: edp_hpd {
> +

Re: [PATCH 0/3] dwc2 patches to allow wakeup on Rockchip rk3288

[Doug Anderson]

Felipe,

On Mon, Jul 6, 2015 at 10:48 AM, Felipe Balbi  wrote:
> On Mon, Jun 22, 2015 at 04:52:21PM -0700, Douglas Anderson wrote:
>> This series of patches, together with
>>  from Chris Zhong and a
>> dts change allow us to wake up from a USB device on rk3288 boards.
>> The patches were tested on rk3288-jerry in the chromeos-3.14 kernel.
>> The chromeos-3.14 kernel tested included a full set of dwc2 backports
>> from upstream, so this is expected to function upstream once we get
>> everything setup there.
>>
>>
>> Douglas Anderson (3):
>>   USB: Export usb_wakeup_enabled_descendants()
>>   Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB
>
> I didn't get 2/3 :-(

Odd.  I'm happy to repost the series, but you were certainly on the
"To" for the whole series.  You can see the Headers at


...please let me know if you'd like me to repost and we can hope the
email system works better next time...

-Doug
--
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] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB

[Doug Anderson]

Rob,

On Tue, Jun 23, 2015 at 7:17 AM, Rob Herring  wrote:
> On Mon, Jun 22, 2015 at 6:52 PM, Douglas Anderson  
> wrote:
>> Some SoCs with a dwc2 USB controller may need to keep the PHY on to
>> support remote wakeup.  Allow specifying this as a device tree
>> property.
>
> I find it hard to believe that any host can support wake-up without
> the PHY.

I am told by Andrew Bresticker that on tegra there is complicated
logic in the PMU that allows USB wakeup while powering off the PHY.
If I hadn't been aware of such a feature I probably would have called
the property "wakeup-supported" or something like that.

> Does this really need to be conditional? Perhaps other cases
> are just always-on or remote wake-up has not been tested.

When I worked on exynos products I was told by Samsung that USB wakeup
was simply not possible.  True that they have a different USB
controller, but I can certainly believe that someone could design a
system with dwc2 where USB wakeup was not possible.

Specifically, in order to get USB wakeup we had to switch on the 24MHz
clock at suspend time.  Had that not been possible (or had the 24MHz
clock not been able to be a clock source for the USB controller) then
USB wakeup would not be possible on rk3288 even if we left the phy on.

So to me the three states are:
1. USB wake not possible
2. USB wake possible, don't need PHY
3. USB wake possible, do need PHY

I don't know of any dwc2 users that are in #2 (so I didn't add a
property now), but I figure that when they exist someone should add a
property then.


-Doug
--
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: [RESEND PATCH] ARM: dts: cros-ec-keyboard: Add support for some Japanese keys

[Doug Anderson]

Hi,

On Tue, May 5, 2015 at 6:03 PM, Chris Zhong  wrote:
> Add support for 4 Japanese keys
>
> Signed-off-by: Chris Zhong 
> Reviewed-by: Doug Anderson 
>
> ---
>
>  arch/arm/boot/dts/cros-ec-keyboard.dtsi | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/cros-ec-keyboard.dtsi 
> b/arch/arm/boot/dts/cros-ec-keyboard.dtsi
> index 9c7fb0a..4e42f30c 100644
> --- a/arch/arm/boot/dts/cros-ec-keyboard.dtsi
> +++ b/arch/arm/boot/dts/cros-ec-keyboard.dtsi
> @@ -22,6 +22,7 @@
> MATRIX_KEY(0x00, 0x02, KEY_F1)
> MATRIX_KEY(0x00, 0x03, KEY_B)
> MATRIX_KEY(0x00, 0x04, KEY_F10)
> +   MATRIX_KEY(0x00, 0x05, KEY_RO)
> MATRIX_KEY(0x00, 0x06, KEY_N)
> MATRIX_KEY(0x00, 0x08, KEY_EQUAL)
> MATRIX_KEY(0x00, 0x0a, KEY_RIGHTALT)
> @@ -34,6 +35,7 @@
> MATRIX_KEY(0x01, 0x08, KEY_APOSTROPHE)
> MATRIX_KEY(0x01, 0x09, KEY_F9)
> MATRIX_KEY(0x01, 0x0b, KEY_BACKSPACE)
> +   MATRIX_KEY(0x01, 0x0c, KEY_HENKAN)
>
> MATRIX_KEY(0x02, 0x00, KEY_LEFTCTRL)
> MATRIX_KEY(0x02, 0x01, KEY_TAB)
> @@ -45,6 +47,7 @@
> MATRIX_KEY(0x02, 0x07, KEY_102ND)
> MATRIX_KEY(0x02, 0x08, KEY_LEFTBRACE)
> MATRIX_KEY(0x02, 0x09, KEY_F8)
> +   MATRIX_KEY(0x02, 0x0a, KEY_YEN)
>
> MATRIX_KEY(0x03, 0x01, KEY_GRAVE)
> MATRIX_KEY(0x03, 0x02, KEY_F2)
> @@ -53,6 +56,7 @@
> MATRIX_KEY(0x03, 0x06, KEY_6)
> MATRIX_KEY(0x03, 0x08, KEY_MINUS)
> MATRIX_KEY(0x03, 0x0b, KEY_BACKSLASH)
> +   MATRIX_KEY(0x03, 0x0c, KEY_MUHENKAN)
>
> MATRIX_KEY(0x04, 0x00, KEY_RIGHTCTRL)
> MATRIX_KEY(0x04, 0x01, KEY_A)
> --
> 1.9.1

Is anyone planning to pick up this trivial change?

-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in

Re: [PATCH] ARM: dts: rockchip: Add dmac_bus rx and tx for uart2

[Doug Anderson]

Hi,

On Tue, Jun 16, 2015 at 4:37 AM, Heiko Stuebner  wrote:
> Just for the record and if anybody is interested to work in this, we have an
> issue with the dma implementation [0] that is not yet solved upstream. From
> what I've remember, that mostly got triggered on higher speeds, like higher
> spi-speeds or i2s audio stuff (poping).
>
>
> Heiko
>
> [0]
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/93c3b192dca7e8c773c4d28873114c325382b63e%5E%21/#F0

Right.  In the ChromeOS tree (used for rk3288-based Chromebooks) we
think we solved the DMA issues for the most part by using burst mode.
...but we weren't brave enough to enable DMA for SPI and UART because
we didn't want to track down any additional weird issues and PIO mode
was working well enough.


The full list of things I see in the PL330 driver in ChromeOS:

3e12510 CHROMIUM: dmaengine: pl330: add quirk for broken no flushp
ded3780 CHROMIUM: dmaengine: pl330: support quirks for some broken
7af450f FROMLIST: DMA: pl330: support burst mode for dev-to-mem and
mem-to-dev transmit

Skimming through the i2s driver (which we definitely use with DMA):

21fee4b CHROMIUM: ASoC: rockchip_i2s: modify DMA max burst to 1
4001251 UPSTREAM: ASoC: rockchip: i2s: fix maxburst of dma data to 4

...I was only peripherally involved here, but as you can see it looks
like there were not only changes needed to the PL330 driver but also
changes needed to the DMA client.

You can find links to patches from the same git tree that Heiko pointed at.
--
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: socfpga: dts: Add a ciu clock node for sdmmc

[Doug Anderson]

Hi,

On Fri, Apr 10, 2015 at 1:56 PM,   wrote:
> From: Dinh Nguyen 
>
> The CIU(Card Interface Unit) clock is used by the dw_mmc IP to clock an SD
> card. The ciu_clk is the sdmmc_clk passed through a fixed divider of 4. This 
> patch
> adds the ciu_clk node and makes the sdmmc_clk it's parent.
>
> Signed-off-by: Dinh Nguyen 
> ---
>  arch/arm/boot/dts/socfpga.dtsi | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index d9176e6..25418ee 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -451,6 +451,14 @@
> clk-phase = <0 135>;
> };
>
> +   ciu_clk: ciu_clk {

Can't say I'm an expert on socfpga, but seems like this clock is only
for sdmmc, right?  ...so its name ought to have sdmmc in its name?

-Doug
--
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: dts: rk3288: Enable Cortex-A12 HW PMU events

[Doug Anderson]

Hi,

On Tue, Apr 7, 2015 at 10:52 AM, Sonny Rao  wrote:
> This adds the dts node for the PMU with the correct PMUIRQ interrupts
> for each core.
>
> Signed-off-by: Sonny Rao 
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 165968d..8253abb 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -44,6 +44,14 @@
> spi2 = &spi2;
> };
>
> +   arm-pmu {
> +   compatible = "arm,cortex-a12-pmu";
> +   interrupts = ,
> +,
> +,
> +;
> +   };
> +

As per discussion with Rockchip: these numbers don't actually match
the TRM, but apparently the TRM is wrong.

Since these numbers work and the numbers came from Rockchip:

Reviewed-by: Doug Anderson 
--
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/2] spi/rockchip: Add device tree property to configure Rx Sample Delay

[Doug Anderson]

Julius,

On Thu, Mar 26, 2015 at 4:30 PM, Julius Werner  wrote:
> We have found that we can sometimes see read failures on boards with
> high-capacitance SPI lines. It seems that the controller samples the Rx
> data line too early, and its register interface has an "Rx Sample Delay"
> setting to fine-tune against this issue.
>
> This patch adds a new optional device tree entry that can configure this
> delay in terms of nanoseconds. The kernel will calculate the
> best-fitting amount of parent clock ticks to program the controller with
> based on that.
>
> Signed-off-by: Julius Werner 
> ---
>  .../devicetree/bindings/spi/spi-rockchip.txt|  4 
>  drivers/spi/spi-rockchip.c  | 21 
> +++++
>  2 files changed, 25 insertions(+)

Reviewed-by: Doug Anderson 
--
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] spi/rockchip: Round up clock rate divisor to err on the safe side

[Doug Anderson]

Julius,

On Thu, Mar 26, 2015 at 4:30 PM, Julius Werner  wrote:
> The Rockchip SPI driver currently calculates its clock rate divisor by
> integer dividing the parent rate by the target rate, and then rounding
> the result up to the next even number (since the divisor must be
> even).
>
> Clock rate divisors should always be rounded up, so that the resulting
> frequency is lower or equal to the target. This is correctly done in the
> second step here but not in the first, so we still have a risk of
> exceeding the desired target frequency (e.g. setting spi-max-frequency
> to 4000 with a parent clock of 9900 could lead to a divisor of
> 9900 / 4000 == 2 (which is even) that then results in an
> effective frequency of 9900 / 2 == 4950 (potentially exceeding
> the flash chip's specifications).
>
> This patch changes the division to round up to fix this problem.
>
> Signed-off-by: Julius Werner 
> ---
>  drivers/spi/spi-rockchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Doug Anderson 
--
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 v4 4/4] ARM: dts: Specify VMMC and VQMMC on rk3288-evb

[Doug Anderson]

Specifying these rails should eventually let us do UHS.

Signed-off-by: Doug Anderson 
---
Changes in v4:
- Add vcc_sd regulator which is present on EVB 2.0 boards

Changes in v3: None
Changes in v2:
- Fix subject line

 arch/arm/boot/dts/rk3288-evb.dtsi | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi 
b/arch/arm/boot/dts/rk3288-evb.dtsi
index 5e895a5..6d4ae9f 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -103,6 +103,23 @@
regulator-always-on;
regulator-boot-on;
};
+
+   /*
+* NOTE: vcc_sd isn't hooked up on v1.0 boards where power comes from
+* vcc_io directly.  Those boards won't be able to power cycle SD cards
+* but it shouldn't hurt to toggle this pin there anyway.
+*/
+   vcc_sd: sdmmc-regulator {
+   compatible = "regulator-fixed";
+   gpio = <&gpio7 11 GPIO_ACTIVE_LOW>;
+   pinctrl-names = "default";
+   pinctrl-0 = <&sdmmc_pwr>;
+   regulator-name = "vcc_sd";
+   regulator-min-microvolt = <330>;
+   regulator-max-microvolt = <330>;
+   startup-delay-us = <10>;
+   vin-supply = <&vcc_io>;
+   };
 };
 
 &emmc {
@@ -132,6 +149,8 @@
pinctrl-names = "default";
pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
status = "okay";
+   vmmc-supply = <&vcc_sd>;
+   vqmmc-supply = <&vccio_sd>;
 };
 
 &i2c0 {
@@ -223,6 +242,10 @@
sdmmc_cmd: sdmmc-cmd {
rockchip,pins = <6 21 RK_FUNC_1 &pcfg_pull_up_drv_8ma>;
};
+
+   sdmmc_pwr: sdmmc-pwr {
+   rockchip,pins = <7 11 RK_FUNC_GPIO &pcfg_pull_none>;
+   };
};
 
usb {
-- 
2.2.0.rc0.207.ga3a616c

--
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 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb

[Doug Anderson]

Heiko,

On Tue, Jan 6, 2015 at 9:44 AM, Heiko Stübner  wrote:
> Hi Doug,
>
> Am Montag, 15. Dezember 2014, 16:22:20 schrieb Doug Anderson:
>> Specifying these rails should eventually let us do UHS.
>>
>> Signed-off-by: Doug Anderson 
>> ---
>> Changes in v3: None
>> Changes in v2:
>> - Fix subject line
>>
>>  arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi
>> b/arch/arm/boot/dts/rk3288-evb.dtsi index 3e067dd..d660fe6 100644
>> --- a/arch/arm/boot/dts/rk3288-evb.dtsi
>> +++ b/arch/arm/boot/dts/rk3288-evb.dtsi
>> @@ -114,6 +114,8 @@
>>   pinctrl-names = "default";
>>   pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
>>   status = "okay";
>> + vmmc-supply = <&vcc_io>;
>
> according to the schematics there is a switch between the card and vcc_io with
> a controlling pin called SDMMC_PWR providing a supply called vcc_sd.
>
> So there should probably be a fixed-regulator get introduced for this, like 
> the
> older rockchip boards do.

OK, I found the problem.  I've got a 1.0 board and the associated
schematics.  Supporting your 2.0 board with the extra regulator should
be no problem though.

Patch coming up shortly.

-Doug
--
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] mmc: dw_mmc: fix bug that cause mmc_test failture

[Doug Anderson]

Addy,

Your subject needs work.  It should at least touch on what the bug
was.  Please use a subject more like:
  mmc: dw_mmc: fix mmc_test by not sending abort for DRTO / EBE errors

On Mon, Jan 26, 2015 at 4:04 AM, Addy Ke  wrote:
> The STOP command can terminate a data transfer between a memory card and
> mmc controller.
>
> As show in Synopsys DesignWare Cores Mobile Stroage Host Databook:
> Data timeout and Data end-bit error will terminate further data transfer
> by mmc controller. So we should not send abort command to terminate a
> data transfer again if we got DRTO and EBE interrupt.

OK, I see this in the section "Error Detection".

Looking at the section titled "Error Handling" in the version of the
databook I see it suggesting "STOP or ABORT" in the case of Data
Errors which might include "end bit not found".  In another section
(the Card Interface Unit (CIU) section) it talks about the data end
bit error and suggests issuing the stop/abort command.

...that being said, it does appear that you're right that we don't
want to send an abort in the EBE case since it appears necessary to
fully fix mmc_test...

> After this patch, all mmc_test cases can pass on RK3288-Pink2 board.

I didn't find this to be the case.  I asked Alexandru to
double-confirm for me and he agrees, it doesn't fix mmc_test.
Probably because your code is broken.  See below.


> Signed-off-by: Addy Ke 
> ---
>  drivers/mmc/host/dw_mmc.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 4d2e3c2..4bd7df1 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1520,7 +1520,10 @@ static void dw_mci_tasklet_func(unsigned long priv)
> if (test_and_clear_bit(EVENT_DATA_ERROR,
>&host->pending_events)) {
> dw_mci_stop_dma(host);
> -   send_stop_abort(host, data);
> +   if (data->stop ||
> +   !(host->data_status & SDMMC_INT_DRTO) ||
> +   !(host->data_status & SDMMC_INT_EBE))
> +   send_stop_abort(host, data);

Your concept appears right, but your code is totally wrong.  Let's
imagine that data->stop is NULL so your check matters.  Now:

DRTO is set, EBE is set: you won't send stop abort
DRTO is set, EBE is _not_ set: you _will_ send stop aborft
DRTO is _not_ set, EBE is set: you _will_ send stop aborft

I can fix this with:

-   !(host->data_status & SDMMC_INT_DRTO) ||
-   !(host->data_status & SDMMC_INT_EBE))
+   !(host->data_status & (SDMMC_INT_DRTO |
+  SDMMC_INT_EBE)))

When I do that then mmc_test is working much better.  Note that is
appears that both EBE and DRTO are necessary here.  Can you please
respin?  Please make sure to include Addy and Javier on your patch as
they may be able to do some extra testing.

-Doug
--
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 5/6] ASoC: samsung: Extend Snow driver to support max98089

[Doug Anderson]

Andreas,

On Thu, Feb 19, 2015 at 9:56 AM, Andreas Färber  wrote:
> Am 19.02.2015 um 18:44 schrieb Doug Anderson:
>> On Thu, Feb 19, 2015 at 1:44 AM, Mark Brown  wrote:
>>> On Wed, Feb 18, 2015 at 07:25:58PM +0100, Andreas Färber wrote:
>>>
>>>>  static const struct of_device_id snow_of_match[] = {
>>>> + { .compatible = "google,snow-audio-max98089", },
>>>>   { .compatible = "google,snow-audio-max98090", },
>>>>   { .compatible = "google,snow-audio-max98091", },
>>>>   { .compatible = "google,snow-audio-max98095", },
>>>
>>> Since we completely ignore the CODEC in the property might it not be
>>> better to just add a plain old snow-audio compatible and bind to that,
>>> that way we don't need these driver updates?  Just the "snow" bit should
>>> be enough to know it's one of this class of machines.
>>
>> I think what you're suggesting is that here we should add a new
>> compatible string "google,snow-audio" instead of adding
>> "google,snow-audio-max98089" here.  Then the sound node in the spring
>> DTS would look like:
>>
>>   compatible = "google,snow-audio-max98089", "google,snow-audio";
>
> If we want to be specific just in case, was it an active decision not to
> use "google,peach-pi[t]-audio-max98..."?
>
> Again, either way works for me.

I don't think it was an active decision, but I am certainly nowhere
near an audio expert and I don't think I made that particular decision
(I could be wrong).

One could make the argument that "snow" describes the general hookup
style of the hardware and then you list the actual codec after that,
but that's a pretty weak argument...

-Doug
--
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/6] ASoC: max98088: Document DT bindings

[Doug Anderson]

Andreas,

On Thu, Feb 19, 2015 at 6:13 AM, Andreas Färber  wrote:
>> I see that a master clock (mclk) is added in patch 6/6 but the
>> max98088 codec driver does handle this clock.
>>
>> If the SoC XCLKOUT provides the master clock to the max98089
>> codec in Spring like is the case for the max9809{0,5} codecs
>> in the Snow and Peach Pit/Pi Chromebooks then you need to do
>> something along the lines of the following commits:
>>
>> e3048c3d2be5 ASoC: max98095: Add master clock handling
>> b10ab7b838bd ASoC: max98090: Add master clock handling
>>
>> If that's the case you also have to mention in the DT binding
>> doc that "clocks" and "clock-names" are optional properties
>> like Documentation/devicetree/bindings/sound/max9809{0,5}.txt.
>
> When I prepared this patch, I believe it was a straight copy from
> max98090. Sounds like they changed since then.
>
> My 6/6 adopted the mclk clock from your now-cancelled v2 patch for Snow,
> assuming it would be the same on all Chromebooks. I tested that last
> change by checking for errors in dmesg.
>
> Doug, can you advise on how the clock wiring is for Spring?

I can confirm that XCLKOUT is connected to the codec MCLK on the
Spring schematics I have.

-Doug
--
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 5/6] ASoC: samsung: Extend Snow driver to support max98089

[Doug Anderson]

Mark,

On Thu, Feb 19, 2015 at 1:44 AM, Mark Brown  wrote:
> On Wed, Feb 18, 2015 at 07:25:58PM +0100, Andreas Färber wrote:
>
>>  static const struct of_device_id snow_of_match[] = {
>> + { .compatible = "google,snow-audio-max98089", },
>>   { .compatible = "google,snow-audio-max98090", },
>>   { .compatible = "google,snow-audio-max98091", },
>>   { .compatible = "google,snow-audio-max98095", },
>
> Since we completely ignore the CODEC in the property might it not be
> better to just add a plain old snow-audio compatible and bind to that,
> that way we don't need these driver updates?  Just the "snow" bit should
> be enough to know it's one of this class of machines.

I think what you're suggesting is that here we should add a new
compatible string "google,snow-audio" instead of adding
"google,snow-audio-max98089" here.  Then the sound node in the spring
DTS would look like:

  compatible = "google,snow-audio-max98089", "google,snow-audio";

That would allow us to later figure out that we're on a board with
max98089 in case it became important but means that any other minor
tweaks like this wouldn't need anything special.  I haven't tried that
to make sure that the fallback compatible string actually works in
this case, but it seems like the right way to go...

Sound good?

-Doug
--
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: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Tue, Jan 27, 2015 at 7:18 AM, Ulf Hansson  wrote:
>> I asked Addy to post upstream against mmc_send_tuning(), but I guess
>> he didn't (he posted against Alex's NAKed patch instead).
>>
>> ...when I talked to him about it, Addy was asserting that when tuning
>> fails it is important (at least on dw_mmc on rk3288) that we wait for
>> the card to stop being busy and that the way to detect was using
>> mmc_send_status().
>
> So, could that be due to the internal logic of the error handling in
> dw_mmc driver? Or you think this is a generic issue?
>
> According to the specifications (eMMC and SD) both states that the
> tuning command has an R1 response. So, there shouldn't be any busy
> signalling involved - at least according to spec.

I did a bit of digging into this issue myself.  What I found was that
a "response CRC" and "end of transfer".  This was why I posted up
.  From that patch:

> Specifically it looks like in certain error conditions (I saw this
> with Response CRC errors) that data keeps showing up in the FIFO even
> after the error is reported and the CD (command done) bit is set.  If
> we don't wait for this data to finish transferring then it confuses
> the next transaction.  In the specific failure case I ran into I found
> that I could monitor the data_state_mc_busy bit and wait for it to
> clear, but in other failure cases this bit was stuck at busy when we
> saw an error.  Hence a generic big delay seems like the only option.

...Addy instead fixed the problem using mmc_send_status() to try to
detect when the transfer was all done and it apparently worked, but it
seemed odd to me.  My MMC "expertise" pretty much ends with looking
for simple logic errors in the MMC driver, so my hope was that one of
you guys would know this better...


>> That would mean that against upstream you'd need to change
>> mmc_send_tuning() to take in the card as well (or move the "host->card
>> = card" assignment to before UHS init, which seems less desirable?)
>>
>> What do you think about that?  Is there a better solution?
>
> Why do we need to change mmc_send_tuning()? I thought the issue was
> that mmc_send_status(), which currently takes "card" as a parameter.

Well, if mmc_send_tuning() needed to call mmc_send_status() then
mmc_send_tuning() would need the card parameter, right?


Doug
--
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: core: use card pointer as the first parameter of execute_tuning()

[Doug Anderson]

Ulf,

On Mon, Jan 26, 2015 at 7:15 AM, Ulf Hansson  wrote:
> On 26 January 2015 at 12:19, Addy Ke  wrote:
>> We need to take the card pointer in execute_tuning() for mmc_send_status(),
>
> mmc_send_status() is an mmc core function, not intended for host's to call.
>
>> but mmc->card is NULL in tuning state. So we need change the first parameter
>> of execute_tuning() to card pointer(struct mmc_card * card).
>
> So, why do we need this?

I asked Addy to post upstream against mmc_send_tuning(), but I guess
he didn't (he posted against Alex's NAKed patch instead).

...when I talked to him about it, Addy was asserting that when tuning
fails it is important (at least on dw_mmc on rk3288) that we wait for
the card to stop being busy and that the way to detect was using
mmc_send_status().

That would mean that against upstream you'd need to change
mmc_send_tuning() to take in the card as well (or move the "host->card
= card" assignment to before UHS init, which seems less desirable?)

What do you think about that?  Is there a better solution?

-Doug
--
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 1/4] mmc: dw_mmc: exynos: incorporate ciu_div into timing property

[Doug Anderson]

Alim,

On Sun, Jan 4, 2015 at 2:43 PM, Alim Akhtar  wrote:
>> You are breaking backward compatibility here.  If your change is
>> merged then all old boards will instantly break.  Since the "dts" and
>> code changes will likely be merged through different trees you'll end
>> up with a bunch of broken trees until everything is merged together.
>> Even if you don't subscribe to the stable bindings theory this is not
>> acceptable.
>>
> yes the major concern in this series is probably this, which breaks
> things unless everything merge in one go and via one tree. Thats why I
> re-based everything including dts change on mmc-tree for this case and
> added device-tree mailing list for more opinion etc.

Got it.  I doubt that folks will like this, but I could be wrong.  In
order for this to work, you'd need all changes in the series to land
in _both_ the ARMSoC tree and the MMC tree.  That's not unheard of,
but it doesn't seem ideal.

You also break bisect-ability here since without the code the DTS
change will break things and without the DTS change the code will
break things.

If you add all the above to the fact that bindings are supposed to be
stable (ish) I'm not convinced this will land.


-Doug
--
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 1/4] mmc: dw_mmc: exynos: incorporate ciu_div into timing property

[Doug Anderson]

Alim,

On Tue, Dec 30, 2014 at 10:43 PM, Alim Akhtar  wrote:
> From: Seungwon Jeon 
>
> ciu_div may not be common value for all speed mode.
> So, it needs to be attached to CLKSEL timing.

The more time I've spent looking at all of this stuff the less I like
the exynos bindings.  Personally I'd prefer to see the exynos bindings
fixed rather than go further down the path of these bindings.

Specifically:

1. The "drive" really should be specified quite differently.
According to the DesignWare docs, the "drive" phase is there to meet
hold time requirements.  Hold time requirements are different for
different SD/MMC modes and are specified in nanoseconds (SDR104 is
.8ns, ID mode is 5.0ns).  There is a per-SoC parameter needed that
indicates some built-in delay (in nanoseconds) and that shouldn't
change based on clock speed or card mode.

2. The ciu_div really ought to be automatic.  Start out at a divide by
4.  If you end up with both drive and sample at phases of 0, 90, 180,
270 then you can change to divide by 2.

I still haven't looked at every last detail of these delays though, so
please correct me if I'm wrong.  I've added Alex who may have looked
at this more than I have.


With all of the above, there's also the fact that (I think) we should
be moving towards working with the clock phase API since all of your
values are effectively specifying clock phases, just in a very arcane
way.


> This also introduce a new compatibale 'dw-mshc-hs200-timing'
> for selecting hs200 timing value

As per above, I don't think this is needed.


> Signed-off-by: Seungwon Jeon 
> Signed-off-by: Alim Akhtar 
> ---
>  .../devicetree/bindings/mmc/exynos-dw-mshc.txt |   15 ++--
>  drivers/mmc/host/dw_mmc-exynos.c   |   81 
> ++--
>  drivers/mmc/host/dw_mmc-exynos.h   |1 +
>  3 files changed, 67 insertions(+), 30 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt 
> b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt
> index ee4fc05..06455de 100644
> --- a/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt
> @@ -23,10 +23,6 @@ Required Properties:
> - "samsung,exynos7-dw-mshc-smu": for controllers with Samsung Exynos7
>   specific extensions having an SMU.
>
> -* samsung,dw-mshc-ciu-div: Specifies the divider value for the card interface
> -  unit (ciu) clock. This property is applicable only for Exynos5 SoC's and
> -  ignored for Exynos4 SoC's. The valid range of divider value is 0 to 7.
> -
>  * samsung,dw-mshc-sdr-timing: Specifies the value of CIU clock phase shift 
> value
>in transmit mode and CIU clock phase shift value in receive mode for single
>data rate mode operation. Refer notes below for the order of the cells and 
> the
> @@ -37,11 +33,16 @@ Required Properties:
>data rate mode operation. Refer notes below for the order of the cells and 
> the
>valid values.
>
> +* samsung,dw-mshc-hs200-timing: Similar with dw-mshc-sdr-timing.
> +
>Notes for the sdr-timing and ddr-timing values:
>
>  The order of the cells should be
>- First Cell: CIU clock phase shift value for tx mode.
>- Second Cell: CIU clock phase shift value for rx mode.
> +  - Thrid Cell: Specifies the divider value for the card interface
> +unit (ciu) clock. This property is applicable only for Exynos5 SoC's 
> and
> +ignored for Exynos4 SoC's. The valid range of divider value is 0 to 
> 7.
>
>  Valid values for SDR and DDR CIU clock timing for Exynos5250:
>- valid value for tx phase shift and rx phase shift is 0 to 7.
> @@ -79,8 +80,8 @@ Example:
> broken-cd;
> fifo-depth = <0x80>;
> card-detect-delay = <200>;
> -   samsung,dw-mshc-ciu-div = <3>;
> -   samsung,dw-mshc-sdr-timing = <2 3>;
> -   samsung,dw-mshc-ddr-timing = <1 2>;
> +   samsung,dw-mshc-sdr-timing = <2 3 3>;
> +   samsung,dw-mshc-ddr-timing = <1 2 3>;
> +   samsung,dw-mshc-hs200-timing = <0 2 3>;

You are breaking backward compatibility here.  If your change is
merged then all old boards will instantly break.  Since the "dts" and
code changes will likely be merged through different trees you'll end
up with a bunch of broken trees until everything is merged together.
Even if you don't subscribe to the stable bindings theory this is not
acceptable.

-Doug
--
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/2] CHROMIUM: clk: rockchip: add PVTM clocks on rk3288

[Doug Anderson]

Dmitry,

On Thu, Dec 18, 2014 at 4:13 PM, Dmitry Torokhov  wrote:
> From: huang lin 
>
> Process-Voltage-Temperatiure Monitor block on RK3288 has two clocks:
> PVTM_CORE and PVTM_GPU.
>
> Signed-off-by: Huang Lin 
> Signed-off-by: Dmitry Torokhov 
> ---
>  drivers/clk/rockchip/clk-rk3288.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks for sending!

Reviewed-by: Doug Anderson 
--
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] CHROMIUM: clk: rockchip: add clock IDs for the PVTM clocks

[Doug Anderson]

Dmitry,

On Thu, Dec 18, 2014 at 4:13 PM, Dmitry Torokhov  wrote:
> From: Huang Lin 
>
> Process-Voltage-Temperature Monitor has two clocks, PVTM_CORE and
> PVTM_GPU.
>
> Signed-off-by: Huang Lin 
> Signed-off-by: Dmitry Torokhov 
> ---
>
> Note that I left a hole at 122 for SCLK_USBPHY480M_SRC which is floating
> around.

I'm sure Heiko can adjust when he lands.


>  include/dt-bindings/clock/rk3288-cru.h | 3 +++
>  1 file changed, 3 insertions(+)

Thanks for sending!

Reviewed-by: Doug Anderson 
--
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] i2c: rk3x: Account for repeated start time requirement

[Doug Anderson]

Hi,

On Thu, Dec 11, 2014 at 11:18 AM, Doug Anderson  wrote:
> On Rockchip I2C the controller drops SDA low in the repeated start
> condition at half the SCL high time.
>
> If we want to meet timing requirements, that means we need to hold SCL
> high for (4.7us * 2) when we're sending a repeated start (.6us * 2 for
> Fast-mode).  That lets us achieve minimum tSU;STA.  However, we don't
> want to always hold SCL high for that long because we'd never be able
> to make 100kHz or 400kHz speeds.
>
> Let's fix this by doing our clock calculations twice: once taking the
> above into account and once running at normal speeds.  We'll use the
> slower speed when sending the start bit and the normal speed
> otherwise.
>
> Note: we really only need the conservative timing when sending
> _repeated_ starts, not when sending the first start.  We don't account
> for this so technically the first start bit will be longer too.
> ...well, except in the case when we use the combined write/read
> optimization which doesn't use the same code.
>
> As part of this change we needed to account for the SDA falling time.
> The specification indicates that this should be the same, but we'll
> follow Designware and add a binding.  Note that we deviate from
> Designware and assign the default SDA falling time to be the same as
> the SCL falling time, which is incredibly likely.
>
> Signed-off-by: Doug Anderson 
> ---
> Note: This is based on Addy's patch (i2c: rk3x: fix bug that cause
> measured high_ns doesn't meet I2C specification) that can be found at
> <https://patchwork.kernel.org/patch/5475331/>.
>
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt |  7 +-
>  drivers/i2c/busses/i2c-rk3x.c  | 90 
> +-
>  2 files changed, 74 insertions(+), 23 deletions(-)

So apparently the person who tested this for me got mixed up and told
me it was good, but it wasn't.  :(

I've sent up a new version.  I've tested it myself this time but
certainly would appreciate any extra testing folks can do on it...
See <https://patchwork.kernel.org/patch/5515551/>

-Doug
--
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 v3] i2c: rk3x: Account for repeated start time requirement

[Doug Anderson]

On Rockchip I2C the controller drops SDA low slightly too soon to meet
the "repeated start" requirements.

>From my own experimentation over a number of rates:
 - controller appears to drop SDA at .875x (7/8) programmed clk high.
 - controller appears to keep SCL high for 2x programmed clk high.

The first rule isn't enough to meet tSU;STA requirements in
Standard-mode on the system I tested on.  The second rule is probably
enough to meet tHD;STA requirements in nearly all cases (especially
after accounting for the first), but it doesn't hurt to account for it
anyway just in case.

Even though the repeated start requirement only need to be accounted
for during a small part of the transfer, we'll adjust the timings for
the whole transfer to meet it.  I believe that adjusting the timings
in just the right place to switch things up for repeated start would
require several extra interrupts and that doesn't seem terribly worth
it.

With this change and worst case rise/fall times, I see 100kHz i2c
going to ~85kHz.  With slightly optimized rise/fall (800ns / 50ns) I
see i2c going to ~89kHz.  Fast-mode isn't affected much because
tSU;STA is shorter relative to tHD;STA there.

As part of this change we needed to account for the SDA falling time.
The specification indicates that this should be the same, but we'll
follow Designware's lead and add a binding.  Note that we deviate from
Designware and assign the default SDA falling time to be the same as
the SCL falling time, which is incredibly likely.

Signed-off-by: Doug Anderson 
---
Note: This is based on Addy's patch (i2c: rk3x: fix bug that cause
measured high_ns doesn't meet I2C specification) that can be found at
<https://patchwork.kernel.org/patch/5475331/>.

Changes in v3:
- Totally changed code and tested myself; now actually works

Changes in v2:
- Also multiply SCL rise time by 2 for repeated start calculation

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt |  7 ++-
 drivers/i2c/busses/i2c-rk3x.c  | 61 +++---
 2 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt 
b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index 1885bd8..8cc75d9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,14 +21,17 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
- - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
+ - i2c-scl-rising-time-ns : Number of nanoseconds the SCL signal takes to rise
(t(r) in I2C specification). If not specified this is assumed to be
the maximum the specification allows(1000 ns for Standard-mode,
300 ns for Fast-mode) which might cause slightly slower communication.
- - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
+ - i2c-scl-falling-time-ns : Number of nanoseconds the SCL signal takes to fall
(t(f) in the I2C specification). If not specified this is assumed to
be the maximum the specification allows (300 ns) which might cause
slightly slowercommunication.
+ - i2c-sda-falling-time-ns : Number of nanoseconds the SDA signal takes to fall
+   (t(f) in the I2C specification). If not specified we'll use the SCL
+   value since they are the same in nearly all cases.
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 36a9224..5f96b1b 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,8 +102,9 @@ struct rk3x_i2c {
 
/* Settings */
unsigned int scl_frequency;
-   unsigned int rise_ns;
-   unsigned int fall_ns;
+   unsigned int scl_rise_ns;
+   unsigned int scl_fall_ns;
+   unsigned int sda_fall_ns;
 
/* Synchronization & notification */
spinlock_t lock;
@@ -437,8 +438,9 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
- * @rise_ns: How many ns it takes for signals to rise.
- * @fall_ns: How many ns it takes for signals to fall.
+ * @scl_rise_ns: How many ns it takes for SCL to rise.
+ * @scl_fall_ns: How many ns it takes for SCL to fall.
+ * @sda_fall_ns: How many ns it takes for SDA to fall.
  * @div_low: Divider output for low
  * @div_high: Divider output for high
  *
@@ -447,11 +449,13 @@ out:
  * too high, we silently use the highest possible rate.
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
- unsigned long rise_ns, unsigned long fall_ns,
+ unsigned long scl_rise_ns,
+ unsigned long scl_fall_ns,
+ unsigned long sda_fall_ns,
  unsigned long *div_low, unsigned l

Re: [PATCH 1/2] clk: rockchip: add clock ID for usbphy480m_src

[Doug Anderson]

Kever,

On Wed, Nov 12, 2014 at 11:22 PM, Kever Yang  wrote:
> There are 3 different parent clock from different usbphy,
> all of them are fixed 480MHz, it is not able to auto select
> by clock core to the 2nd and the 3rd parent.
> For different use case for different board, we may need to
> select different usbphy clock out as parent manually.
>
> Add the clock ID for it so that we can use in dts.
>
> Signed-off-by: Kever Yang 
> ---
>
>  include/dt-bindings/clock/rk3288-cru.h | 1 +
>  1 file changed, 1 insertion(+)

Aside from clock ID collisions that Heiko can fixup, this looks good to me.

Reviewed-by: Doug Anderson 
--
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 v3 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb

[Doug Anderson]

Specifying these rails should eventually let us do UHS.

Signed-off-by: Doug Anderson 
---
Changes in v3: None
Changes in v2:
- Fix subject line

 arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi 
b/arch/arm/boot/dts/rk3288-evb.dtsi
index 3e067dd..d660fe6 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -114,6 +114,8 @@
pinctrl-names = "default";
pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
status = "okay";
+   vmmc-supply = <&vcc_io>;
+   vqmmc-supply = <&vccio_sd>;
 };
 
 &i2c0 {
-- 
2.2.0.rc0.207.ga3a616c

--
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 v2 3/3] ARM: dts: Specify VMMC and VQMMC on rk3288-evb

[Doug Anderson]

Specifying these rails should eventually let us do UHS.

Signed-off-by: Doug Anderson 
---
Changes in v2:
- Fix subject line

 arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi 
b/arch/arm/boot/dts/rk3288-evb.dtsi
index 3e067dd..d660fe6 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -114,6 +114,8 @@
pinctrl-names = "default";
pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
status = "okay";
+   vmmc-supply = <&vcc_io>;
+   vqmmc-supply = <&vccio_sd>;
 };
 
 &i2c0 {
-- 
2.2.0.rc0.207.ga3a616c

--
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 v7 5/5] ARM: dts: rockchip: Enable usb PHY on rk3288-evb board

[Doug Anderson]

Yunzhi,

On Fri, Dec 12, 2014 at 7:17 AM, Yunzhi Li  wrote:
> Enable usb PHY for all usb ports on rk3288-evb.
>
> Signed-off-by: Yunzhi Li 
>
> ---
>
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
>
>  arch/arm/boot/dts/rk3288-evb.dtsi | 4 
>  1 file changed, 4 insertions(+)

I have tested a similar change on rk3288-pinky (on a 3.14 tree with
backports).  There I can confirm that this properly gets us into low
power at suspend time.  I have not tested on EVB itself though, so
leaving off my tested tag.

This does look reasonable to me, though:

Reviewed-by: Doug Anderson 
--
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 v7 4/5] ARM: dts: rockchip: add rk3288 usb PHY

[Doug Anderson]

Yunzhi,

On Fri, Dec 12, 2014 at 7:12 AM, Yunzhi Li  wrote:
> This patch adds a device_node for RK3288 SoC usb phy. It also
> defines the phy to be used by three usb controllers: usb_host0/1
> and usb_otg.
>
> Signed-off-by: Yunzhi Li 
>
> ---
>
> Changes in v7:
> - Update dtsi for new usb phy driver.
>
> Changes in v6: None
> Changes in v5:
> - reorder the phy dt node to a correct position.
>
> Changes in v4:
> - Add phy subnodes.
>
> Changes in v3: None
>
>  arch/arm/boot/dts/rk3288.dtsi | 35 +++
>  1 file changed, 35 insertions(+)

On rk3288-pinky (on a 3.14 tree with backports), I can confirm that
this properly gets us into low power at suspend time.

Tested-by: Doug Anderson 


This looks reasonable to me:

Reviewed-by: Doug Anderson 
--
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 v7 2/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY

[Doug Anderson]

Yunzhi,

On Fri, Dec 12, 2014 at 7:07 AM, Yunzhi Li  wrote:
> This patch to add a generic PHY driver for ROCKCHIP usb PHYs,
> currently this driver can support RK3288. The RK3288 SoC have
> three independent USB PHY IPs which are all configured through a
> set of registers located in the GRF (general register files)
> module.
>
> Signed-off-by: Yunzhi Li 
>
> ---
>
> Changes in v7:
> - Accept Kishon's comments to use phandle args to find a phy
>   struct directly and get rid of using a custom of_xlate
>   function.
>
> Changes in v6:
> - Rename SIDDQ_MSK to SIDDQ_WRITE_ENA.
>
> Changes in v5: None
> Changes in v4:
> - Get number of PHYs from device tree.
> - Model each PHY as subnode of the phy provider node.
>
> Changes in v3:
> - Use BIT macro instead of bit shift ops.
> - Rename the config entry to PHY_ROCKCHIP_USB.
>
>  drivers/phy/Kconfig|   7 ++
>  drivers/phy/Makefile   |   1 +
>  drivers/phy/phy-rockchip-usb.c | 158 
> +
>  3 files changed, 166 insertions(+)
>  create mode 100644 drivers/phy/phy-rockchip-usb.c

On rk3288-pinky (on a 3.14 tree with backports), I can confirm that
this properly gets us into low power at suspend time.

Tested-by: Doug Anderson 
--
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 v7 2/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY

[Doug Anderson]

Hi,

On Fri, Dec 12, 2014 at 11:24 PM, Kishon Vijay Abraham I  wrote:
> hi,
>
> On Saturday 13 December 2014 05:49 AM, Doug Anderson wrote:
>> Yunzhi,
>>
>> On Fri, Dec 12, 2014 at 7:07 AM, Yunzhi Li  wrote:
>>> This patch to add a generic PHY driver for ROCKCHIP usb PHYs,
>>> currently this driver can support RK3288. The RK3288 SoC have
>>> three independent USB PHY IPs which are all configured through a
>>> set of registers located in the GRF (general register files)
>>> module.
>>>
>>> Signed-off-by: Yunzhi Li 
>>>
>>> ---
>>>
>>> Changes in v7:
>>> - Accept Kishon's comments to use phandle args to find a phy
>>>   struct directly and get rid of using a custom of_xlate
>>>   function.
>>
>> I'm going to assume you didn't test this version, since it doesn't
>> work for me.  At suspend time power is high and my printouts in the
>> powerup/powerdown code aren't called...
>>
>>
>>> +   for_each_available_child_of_node(dev->of_node, child) {
>>> +   rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
>>> +   if (!rk_phy)
>>> +   return -ENOMEM;
>>> +
>>> +   if (of_property_read_u32(child, "reg", ®_offset)) {
>>> +   dev_err(dev, "missing reg property in node %s\n",
>>> +   child->name);
>>> +   return -EINVAL;
>>> +   }
>>> +
>>> +   rk_phy->reg_offset = reg_offset;
>>> +   rk_phy->reg_base = grf;
>>> +
>>> +   rk_phy->clk = of_clk_get_by_name(child, "phyclk");
>>> +   if (IS_ERR(rk_phy->clk))
>>> +   rk_phy->clk = NULL;
>>> +
>>> +   rk_phy->phy = devm_phy_create(dev, child, &ops);
>>> +   if (IS_ERR(rk_phy->phy)) {
>>> +   dev_err(dev, "failed to create PHY\n");
>>> +   return PTR_ERR(rk_phy->phy);
>>> +   }
>>> +   phy_set_drvdata(rk_phy->phy, rk_phy);
>>> +   }
>>> +
>>> +   phy_provider = devm_of_phy_provider_register(dev, 
>>> of_phy_simple_xlate);
>>
>> I think your bug is here.  I think you now need to register 3 phy
>> providers, not just one.
>
> No there should be only one phy provider. It means the bug is elsewhere.

Ah.  That's what I get for testing on a backported kernel.  I bet it's
because I'm missing:

2a4c370 phy: core: Fix of_phy_provider_lookup to return PHY provider
for sub node

Hrm, that made things better, but I still only got one printout when I
expected 3 (one for each user of the PHY).  I bet there are more picks
I need then...  :-/  Ah, yup.  When I pick the whole load of PHY
related stuff then I get all 3.  :)

I'll do more testing when I have more time and post up a Tested-by, then...

-Doug
--
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 v7 2/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY

[Doug Anderson]

Yunzhi,

On Fri, Dec 12, 2014 at 7:07 AM, Yunzhi Li  wrote:
> This patch to add a generic PHY driver for ROCKCHIP usb PHYs,
> currently this driver can support RK3288. The RK3288 SoC have
> three independent USB PHY IPs which are all configured through a
> set of registers located in the GRF (general register files)
> module.
>
> Signed-off-by: Yunzhi Li 
>
> ---
>
> Changes in v7:
> - Accept Kishon's comments to use phandle args to find a phy
>   struct directly and get rid of using a custom of_xlate
>   function.

I'm going to assume you didn't test this version, since it doesn't
work for me.  At suspend time power is high and my printouts in the
powerup/powerdown code aren't called...


> +   for_each_available_child_of_node(dev->of_node, child) {
> +   rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
> +   if (!rk_phy)
> +   return -ENOMEM;
> +
> +   if (of_property_read_u32(child, "reg", ®_offset)) {
> +   dev_err(dev, "missing reg property in node %s\n",
> +   child->name);
> +   return -EINVAL;
> +   }
> +
> +   rk_phy->reg_offset = reg_offset;
> +   rk_phy->reg_base = grf;
> +
> +   rk_phy->clk = of_clk_get_by_name(child, "phyclk");
> +   if (IS_ERR(rk_phy->clk))
> +   rk_phy->clk = NULL;
> +
> +   rk_phy->phy = devm_phy_create(dev, child, &ops);
> +   if (IS_ERR(rk_phy->phy)) {
> +   dev_err(dev, "failed to create PHY\n");
> +   return PTR_ERR(rk_phy->phy);
> +   }
> +   phy_set_drvdata(rk_phy->phy, rk_phy);
> +   }
> +
> +   phy_provider = devm_of_phy_provider_register(dev, 
> of_phy_simple_xlate);

I think your bug is here.  I think you now need to register 3 phy
providers, not just one.

I'll continue to assert my utter noviceness with this code, but my
attempt at a solution (which works) can be found at:

https://chromium-review.googlesource.com/235456

-Doug
--
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 1/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY

[Doug Anderson]

Yunzhi,

On Thu, Dec 11, 2014 at 10:09 AM, Doug Anderson  wrote:
>> +   rk_phy->phy = devm_phy_create(dev, NULL, &ops);
>
> This has the wrong number of arguments.  Even before the change that
> added the 4th argument, this is still wrong because "ops" is supposed
> to be the 2nd argument, not the 3rd.
>
> ...so I'm confused how this compiled for you.  I think this ought to be:
>
> rk_phy->phy = devm_phy_create(dev, child, &ops, NULL);
>
> ...but please correct me if I'm mistaken!

As you pointed out privately, I didn't have (dbc9863 phy: remove the
old lookup method).  Sorry about the noise..

Hopefully my other comments are not quite as stupid...

-Doug
--
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 v2] i2c: rk3x: Account for repeated start time requirement

[Doug Anderson]

On Rockchip I2C the controller drops SDA low in the repeated start
condition at half the SCL high time.

If we want to meet timing requirements, that means we need to hold SCL
high for (4.7us * 2) when we're sending a repeated start (.6us * 2 for
Fast-mode).  That lets us achieve minimum tSU;STA.  However, we don't
want to always hold SCL high for that long because we'd never be able
to make 100kHz or 400kHz speeds.

Let's fix this by doing our clock calculations twice: once taking the
above into account and once running at normal speeds.  We'll use the
slower speed when sending the start bit and the normal speed
otherwise.

Note: we really only need the conservative timing when sending
_repeated_ starts, not when sending the first start.  We don't account
for this so technically the first start bit will be longer too.
...well, except in the case when we use the combined write/read
optimization which doesn't use the same code.

As part of this change we needed to account for the SDA falling time.
The specification indicates that this should be the same, but we'll
follow Designware and add a binding.  Note that we deviate from
Designware and assign the default SDA falling time to be the same as
the SCL falling time, which is incredibly likely.

Signed-off-by: Doug Anderson 
---
Note: This is based on Addy's patch (i2c: rk3x: fix bug that cause
measured high_ns doesn't meet I2C specification) that can be found at
<https://patchwork.kernel.org/patch/5475331/>.

Changes in v2:
- Also multiply SCL rise time by 2 for repeated start calculation

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt |  7 +-
 drivers/i2c/busses/i2c-rk3x.c  | 89 +-
 2 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt 
b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index 1885bd8..8cc75d9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,14 +21,17 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
- - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
+ - i2c-scl-rising-time-ns : Number of nanoseconds the SCL signal takes to rise
(t(r) in I2C specification). If not specified this is assumed to be
the maximum the specification allows(1000 ns for Standard-mode,
300 ns for Fast-mode) which might cause slightly slower communication.
- - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
+ - i2c-scl-falling-time-ns : Number of nanoseconds the SCL signal takes to fall
(t(f) in the I2C specification). If not specified this is assumed to
be the maximum the specification allows (300 ns) which might cause
slightly slowercommunication.
+ - i2c-sda-falling-time-ns : Number of nanoseconds the SDA signal takes to fall
+   (t(f) in the I2C specification). If not specified we'll use the SCL
+   value since they are the same in nearly all cases.
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 36a9224..f0e5da9 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,8 +102,14 @@ struct rk3x_i2c {
 
/* Settings */
unsigned int scl_frequency;
-   unsigned int rise_ns;
-   unsigned int fall_ns;
+   unsigned int scl_rise_ns;
+   unsigned int scl_fall_ns;
+   unsigned int sda_fall_ns;
+
+   /* DIV changes when we're sending a repeated start; keep both */
+   bool sending_start;
+   u32 div_normal;
+   u32 div_start;
 
/* Synchronization & notification */
spinlock_t lock;
@@ -146,6 +152,9 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
u32 val;
 
+   i2c->sending_start = true;
+   i2c_writel(i2c, i2c->div_start, REG_CLKDIV);
+
rk3x_i2c_clean_ipd(i2c);
i2c_writel(i2c, REG_INT_START, REG_IEN);
 
@@ -281,6 +290,9 @@ static void rk3x_i2c_handle_start(struct rk3x_i2c *i2c, 
unsigned int ipd)
/* disable start bit */
i2c_writel(i2c, i2c_readl(i2c, REG_CON) & ~REG_CON_START, REG_CON);
 
+   i2c->sending_start = false;
+   i2c_writel(i2c, i2c->div_normal, REG_CLKDIV);
+
/* enable appropriate interrupts and transition */
if (i2c->mode == REG_CON_MOD_TX) {
i2c_writel(i2c, REG_INT_MBTF | REG_INT_NAKRCV, REG_IEN);
@@ -437,21 +449,26 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
- * @rise_ns: How many ns it takes for signals to rise.
- * @fall_ns: How many ns it takes for signals to fall.
+ * @scl_rise_ns: How many ns it takes for SCL to rise.
+ * @scl_fall_ns: How many ns it takes for SCL to fall.
+ * @sda_fall_ns: How many ns it takes for

Re: [PATCH v2 1/2] mfd: dt-bindings: add the description about dvs gpio for rk808

[Doug Anderson]

Chris,

On Wed, Dec 10, 2014 at 7:16 PM, Chris Zhong  wrote:
> +- dvs-gpios:  buck1/2 can be controlled by gpio dvs, this is GPIO specifiers
> +  for 2 host gpio's used for dvs. The format of the gpio specifier depends in
> +  the gpio controller. If DVS GPIOs aren't present, voltage changes will 
> happen
> +  without being very quickly with no slow ramp time.

Remove the words "without being"

I assume Mark would rather make that change himself instead of getting
a respin, so probably don't send out a new version unless other
changes are needed.

Reviewed-by: Doug Anderson 
--
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] i2c: rk3x: Account for repeated start time requirement

[Doug Anderson]

Hi,

On Thu, Dec 11, 2014 at 11:18 AM, Doug Anderson  wrote:
> -   min_low_ns = spec_min_low_ns + fall_ns;
> -   min_high_ns = spec_min_high_ns + rise_ns;
> +   /*
> +* For repeated start we need at least (spec_setup_start * 2) to meet
> +* (tSU;SDA) requirements. The controller drops data low at half the
> +* high time). Also need to meet normal specification requirements.
> +*/
> +   if (for_start)
> +   min_high_ns = max(spec_setup_start * 2,
> + spec_setup_start + sda_fall_ns +
> + spec_min_high_ns);
> +   else
> +   min_high_ns = spec_min_high_ns;
> +   min_high_ns += scl_rise_ns;
> +
> +   min_low_ns = spec_min_low_ns + scl_fall_ns;

Sorry I didn't think about this before, but I think the above has a
slight bug.  I need to account for the scl_rise_ns rise time twice in
the repeated start case, I believe.  I'll send out a new version
shortly.
--
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] i2c: rk3x: Account for repeated start time requirement

[Doug Anderson]

On Rockchip I2C the controller drops SDA low in the repeated start
condition at half the SCL high time.

If we want to meet timing requirements, that means we need to hold SCL
high for (4.7us * 2) when we're sending a repeated start (.6us * 2 for
Fast-mode).  That lets us achieve minimum tSU;STA.  However, we don't
want to always hold SCL high for that long because we'd never be able
to make 100kHz or 400kHz speeds.

Let's fix this by doing our clock calculations twice: once taking the
above into account and once running at normal speeds.  We'll use the
slower speed when sending the start bit and the normal speed
otherwise.

Note: we really only need the conservative timing when sending
_repeated_ starts, not when sending the first start.  We don't account
for this so technically the first start bit will be longer too.
...well, except in the case when we use the combined write/read
optimization which doesn't use the same code.

As part of this change we needed to account for the SDA falling time.
The specification indicates that this should be the same, but we'll
follow Designware and add a binding.  Note that we deviate from
Designware and assign the default SDA falling time to be the same as
the SCL falling time, which is incredibly likely.

Signed-off-by: Doug Anderson 
---
Note: This is based on Addy's patch (i2c: rk3x: fix bug that cause
measured high_ns doesn't meet I2C specification) that can be found at
<https://patchwork.kernel.org/patch/5475331/>.

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt |  7 +-
 drivers/i2c/busses/i2c-rk3x.c  | 90 +-
 2 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt 
b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index 1885bd8..8cc75d9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,14 +21,17 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
- - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
+ - i2c-scl-rising-time-ns : Number of nanoseconds the SCL signal takes to rise
(t(r) in I2C specification). If not specified this is assumed to be
the maximum the specification allows(1000 ns for Standard-mode,
300 ns for Fast-mode) which might cause slightly slower communication.
- - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
+ - i2c-scl-falling-time-ns : Number of nanoseconds the SCL signal takes to fall
(t(f) in the I2C specification). If not specified this is assumed to
be the maximum the specification allows (300 ns) which might cause
slightly slowercommunication.
+ - i2c-sda-falling-time-ns : Number of nanoseconds the SDA signal takes to fall
+   (t(f) in the I2C specification). If not specified we'll use the SCL
+   value since they are the same in nearly all cases.
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 36a9224..1f994df 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,8 +102,14 @@ struct rk3x_i2c {
 
/* Settings */
unsigned int scl_frequency;
-   unsigned int rise_ns;
-   unsigned int fall_ns;
+   unsigned int scl_rise_ns;
+   unsigned int scl_fall_ns;
+   unsigned int sda_fall_ns;
+
+   /* DIV changes when we're sending a repeated start; keep both */
+   bool sending_start;
+   u32 div_normal;
+   u32 div_start;
 
/* Synchronization & notification */
spinlock_t lock;
@@ -146,6 +152,9 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
u32 val;
 
+   i2c->sending_start = true;
+   i2c_writel(i2c, i2c->div_start, REG_CLKDIV);
+
rk3x_i2c_clean_ipd(i2c);
i2c_writel(i2c, REG_INT_START, REG_IEN);
 
@@ -281,6 +290,9 @@ static void rk3x_i2c_handle_start(struct rk3x_i2c *i2c, 
unsigned int ipd)
/* disable start bit */
i2c_writel(i2c, i2c_readl(i2c, REG_CON) & ~REG_CON_START, REG_CON);
 
+   i2c->sending_start = false;
+   i2c_writel(i2c, i2c->div_normal, REG_CLKDIV);
+
/* enable appropriate interrupts and transition */
if (i2c->mode == REG_CON_MOD_TX) {
i2c_writel(i2c, REG_INT_MBTF | REG_INT_NAKRCV, REG_IEN);
@@ -437,21 +449,26 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
- * @rise_ns: How many ns it takes for signals to rise.
- * @fall_ns: How many ns it takes for signals to fall.
+ * @scl_rise_ns: How many ns it takes for SCL to rise.
+ * @scl_fall_ns: How many ns it takes for SCL to fall.
+ * @sda_fall_ns: How many ns it takes for SDA to fall.
  * @div_low: Divider output for low
  * @div_high: Divider output for high
+

Re: [PATCH v6 2/5] Documentation: bindings: add dt documentation for Rockchip usb PHY

[Doug Anderson]

Hi,

On Thu, Dec 11, 2014 at 10:46 AM, Doug Anderson  wrote:
> Yunzhi,
>
> On Thu, Dec 11, 2014 at 1:55 AM, Yunzhi Li  wrote:
>> This patch adds a binding that describes the Rockchip usb PHYs
>> found on Rockchip SoCs usb interface.
>
> Technically the bindings patch is supposed to come before the driver.
> So this should be patch #1 and the driver patch #2.
>
>
>> +Required properties:
>> + - compatible: rockchip,rk3288-usb-phy
>> + - rockchip,grf : phandle to the syscon managing the "general
>> +   register files"
>> + - #phy-cells: should be 1
>> + - #address-cells: should be 1
>> + - #size-cells: should be 0
>> +
>> +Sub-nodes:
>> +Each PHY should be represented as a sub-node.
>> +
>> +Sub-nodes
>> +required properties:
>> +- reg: the PHY number
>> +   "0" - PHY connect to OTG controller
>> +   "1" - PHY connect to HOST0 controller
>> +   "2" - PHY connect to HOST1 controller
>
> You don't have any sub nodes and are using the phy-cells.  Seems like
> you should get rid of this?  ...or I guess switch to using sub nodes
> and set "phy-cells" to 0?

Oh.  You actually have them in the ".dtsi" patch, but not here in the
example.  Hrm.  I don't know enough about phys in general to say
whether they're needed (I will learn if someone else on this thread
doesn't just know), but if they are required then they ought to be in
the example...
--
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 2/5] Documentation: bindings: add dt documentation for Rockchip usb PHY

[Doug Anderson]

Yunzhi,

On Thu, Dec 11, 2014 at 1:55 AM, Yunzhi Li  wrote:
> This patch adds a binding that describes the Rockchip usb PHYs
> found on Rockchip SoCs usb interface.

Technically the bindings patch is supposed to come before the driver.
So this should be patch #1 and the driver patch #2.


> +Required properties:
> + - compatible: rockchip,rk3288-usb-phy
> + - rockchip,grf : phandle to the syscon managing the "general
> +   register files"
> + - #phy-cells: should be 1
> + - #address-cells: should be 1
> + - #size-cells: should be 0
> +
> +Sub-nodes:
> +Each PHY should be represented as a sub-node.
> +
> +Sub-nodes
> +required properties:
> +- reg: the PHY number
> +   "0" - PHY connect to OTG controller
> +   "1" - PHY connect to HOST0 controller
> +   "2" - PHY connect to HOST1 controller

You don't have any sub nodes and are using the phy-cells.  Seems like
you should get rid of this?  ...or I guess switch to using sub nodes
and set "phy-cells" to 0?

> +
> +Optional Properties:
> +- clocks : phandle + clock specifier for the phy clocks

As per earlier, you should get rid of clocks.  If you really want a
clock here and it's optional:

* Back in the driver it shouldn't be a "warn".  You don't warn when
optional things are missing.

* You really should specify a clock name.  Right now this will pick
the first clock, which makes it hard to later add clocks.
--
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 1/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY

[Doug Anderson]

Kishon,

On Thu, Dec 11, 2014 at 2:27 AM, Kishon Vijay Abraham I  wrote:
> I didn't mean that. You can get rid of this entire xlate stuff if you use
> something like below
>
> phy@xxx {
> compatible = "";
> phy1:usb_phy {
> }
> phy2:usb_phy {
> };
> };
>
>
> usb@xx {
> compatible = "";
> phys = <&phy1>; //doesn't need xlate
> /* this needs xlate
>phys = <&phy 1>;
> */
> phy-names = "phy";
> };

Is the syntax you proposed really better?  Are you saying that you
advocate never using "#phy-cells" other than 0 for new bindings?  Is
that your own personal preference, or is there a discussion somewhere
where everyone agreed on this?

My vote is that since "phy-cells" exists and is part of the generic
phy bindings that it's meant to be used whenever you have a single PHY
driver that controls multiple PHYs.


-Doug
--
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 1/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY

[Doug Anderson]

Yunzhi,

On Thu, Dec 11, 2014 at 1:55 AM, Yunzhi Li  wrote:
> +   rk_phy->clk = of_clk_get(child, 0);
> +   if (IS_ERR(rk_phy->clk)) {
> +   dev_warn(dev, "failed to get clock\n");
> +   rk_phy->clk = NULL;
> +   }

The device tree bindings don't specify a clock and the "dtsi" added to
rk3288 don't reference a clock.  Take that code out and avoid a
warning in the logs at bootup.

...or should there be a clock?


> +   rk_phy->phy = devm_phy_create(dev, NULL, &ops);

This has the wrong number of arguments.  Even before the change that
added the 4th argument, this is still wrong because "ops" is supposed
to be the 2nd argument, not the 3rd.

...so I'm confused how this compiled for you.  I think this ought to be:

rk_phy->phy = devm_phy_create(dev, child, &ops, NULL);

...but please correct me if I'm mistaken!
--
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 4/4] ARK: dts: Specify VMMC and VQMMC on rk3288-evb

[Doug Anderson]

Specifying these rails should eventually let us do UHS.

Signed-off-by: Doug Anderson 
---
 arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi 
b/arch/arm/boot/dts/rk3288-evb.dtsi
index 6194d67..1956a23 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -114,6 +114,8 @@
pinctrl-names = "default";
pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>;
status = "okay";
+   vmmc-supply = <&vcc_io>;
+   vqmmc-supply = <&vccio_sd>;
 };
 
 &i2c0 {
-- 
2.2.0.rc0.207.ga3a616c

--
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] ARM: dts: Bump SD card pin drive strength up on rk3288-evb

[Doug Anderson]

It seems that ever since (536f6b9 mmc: dw_mmc: Reset DMA before
enabling IDMAC) landed upstream that SD cards have been very unhappy
on rk3288-evb.  They were a little unhappy before that change, but
after that change they're REALLY unhappy.

It turns out that the above fix happens to fix a corruption when
reading card information during probe time.  Without the fix we didn't
detect that high speed SD cards could actually support high speed.
With the fix we suddenly detect that they're high speed and we try to
use them at 50MHz.  That doesn't work so well on EVB with the default
drive strength (maybe because there are two physical SD card slots
hooked up to the same pin?).

Fix the problem by bumping up the drive strength of the sdmmc lines.

Signed-off-by: Doug Anderson 
Fixes: 536f6b91d21b ("mmc: dw_mmc: Reset DMA before enabling IDMAC")
---
 arch/arm/boot/dts/rk3288-evb.dtsi | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi 
b/arch/arm/boot/dts/rk3288-evb.dtsi
index 3e067dd..6194d67 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -155,6 +155,15 @@
 };
 
 &pinctrl {
+   pcfg_pull_none_drv_8ma: pcfg-pull-none-drv-8ma {
+   drive-strength = <8>;
+   };
+
+   pcfg_pull_up_drv_8ma: pcfg-pull-up-drv-8ma {
+   bias-pull-up;
+   drive-strength = <8>;
+   };
+
backlight {
bl_en: bl-en {
rockchip,pins = <7 2 RK_FUNC_GPIO &pcfg_pull_none>;
@@ -173,6 +182,27 @@
};
};
 
+   sdmmc {
+   /*
+* Default drive strength isn't enough to achieve even
+* high-speed mode on EVB board so bump up to 8ma.
+*/
+   sdmmc_bus4: sdmmc-bus4 {
+   rockchip,pins = <6 16 RK_FUNC_1 &pcfg_pull_up_drv_8ma>,
+   <6 17 RK_FUNC_1 &pcfg_pull_up_drv_8ma>,
+   <6 18 RK_FUNC_1 &pcfg_pull_up_drv_8ma>,
+   <6 19 RK_FUNC_1 &pcfg_pull_up_drv_8ma>;
+   };
+
+   sdmmc_clk: sdmmc-clk {
+   rockchip,pins = <6 20 RK_FUNC_1 
&pcfg_pull_none_drv_8ma>;
+   };
+
+   sdmmc_cmd: sdmmc-cmd {
+   rockchip,pins = <6 21 RK_FUNC_1 &pcfg_pull_up_drv_8ma>;
+   };
+   };
+
usb {
host_vbus_drv: host-vbus-drv {
rockchip,pins = <0 14 RK_FUNC_GPIO &pcfg_pull_none>;
-- 
2.2.0.rc0.207.ga3a616c

--
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] i2c: designware: Fix falling time bindings doc

[Doug Anderson]

In (6468276 i2c: designware: make SCL and SDA falling time
configurable) new device tree properties were added for setting the
falling time of SDA and SCL.  The device tree bindings doc had a typo
in it: it forgot the "-ns" suffix for both properies in the prose of
the bindings.

I assume this is a typo because:
* The source code includes the "-ns"
* The example in the bindings includes the "-ns".

Fix the typo.

Signed-off-by: Doug Anderson 
Fixes: 6468276b2206 ("i2c: designware: make SCL and SDA falling time 
configurable")
---
 Documentation/devicetree/bindings/i2c/i2c-designware.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt 
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index 5199b0c..fee26dc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -14,10 +14,10 @@ Optional properties :
  - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
This option is only supported in hardware blocks version 1.11a or newer.
 
- - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds.
+ - i2c-scl-falling-time-ns : should contain the SCL falling time in 
nanoseconds.
This value which is by default 300ns is used to compute the tLOW period.
 
- - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds.
+ - i2c-sda-falling-time-ns : should contain the SDA falling time in 
nanoseconds.
This value which is by default 300ns is used to compute the tHIGH period.
 
 Example :
-- 
2.2.0.rc0.207.ga3a616c

--
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: dts: rockchip: set dw_mmc max-freq 150Mhz

[Doug Anderson]

Addy,

On Wed, Dec 3, 2014 at 6:49 PM, Addy Ke  wrote:
> All of mmc controllers include SDMMC, SDIO0, SDIO1, and EMMC on RK3288
> are limited to 150Mhz. It was mainly caused by two reasons:
> - RK3288's IO pad(except DDR IO pad) is generic, which can only support
>   the max of 150Mhz.
> - Mmc controller was designed at 150Mhz, and the pressure test by IC team
>   was based on this freequency point.
>
> Signed-off-by: Addy Ke 
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 4 
>  1 file changed, 4 insertions(+)

Your explanation is reasonable.  The 150MHz rate is also listed in the
latest datasheet.  It's unfortunate that we won't get full speed of
SDR104 or hs200 on this SoC, but correctness certainly outweighs
performance.

Reviewed-by: Doug Anderson 

I have tested that this prevents the speed from going above 150MHz on
rk3288-pinky on SD Cards, so:

Tested-by: Doug Anderson 

-Doug
--
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/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec

[Doug Anderson]

On Tue, Dec 2, 2014 at 5:03 PM, Jianqun  wrote:
> Hi Doug:
>
> 在 12/03/2014 01:54 AM, Doug Anderson 写道:
>> Jianqun,
>>
>> This ought to be a "v3" patch and ideally ought to describe
>> differences from v2 (after the cut).  Please have Kever or Chris
>> review your next patch before sending it out since I think they are
>> familiar with the process.

You still seem to be missing versions in your subject line and missing
info on what changed version to version.  The latest version you just
sent (v4?) is still missing it.

Unless Mark says so then I don't think you need to spin this series,
but please try to add that in the future.


>> You still forgot the blank line here requested by Heiko.  Please try
>> again.  See <https://patchwork.kernel.org/patch/5334991/>
> Although I thought there needn't a blank ~, I wll add it next patch

That would have been OK, but in that case you should have responded to
Heiko's patch and explained that you weren't taking his feedback (and
why you weren't).

-Doug
--
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] mmc: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

Addy,

On Tue, Dec 2, 2014 at 7:16 PM, Addy Ke  wrote:
> This patch add a new quirk to add a s/w timer to notify the driver
> to terminate current transfer and report a data timeout to the core,
> if DTO interrupt does NOT come within the given time.
>
> dw_mmc call mmc_request_done func to finish transfer depends on
> DTO interrupt. If DTO interrupt does not come in sending data state,
> the current transfer will be blocked.
>
> But this case really exists, when driver reads tuning data from
> card on RK3288-pink2 board. I measured waveforms by oscilloscope
> and found that card clock was always on and data lines were always
> holded high level in sending data state.
>
> We got the reply from synopsys:
> There are two counters but both use the same value of [31:8] bits.
> Data timeout counter doesn't wait for stop clock and you should get
> DRTO even when the clock is not stopped.
> Host Starvation timeout counter is triggered with stop clock condition.
>
> This means that host should get DRTO and DTO interrupt.
>
> But we really don't get any data-related interrupt in RK3X SoCs.
> And driver can't get data transfer state, it can do nothing but wait for.
>
> We don't know why we have this problem, but we need it to fix this problem 
> now.
> And I will post a follow up change when we find the root cause.
>
> Signed-off-by: Addy Ke 
> ---
> Changes in v2:
> - fix some typo.
> - remove extra timeout value (250ms).
> - remove dw_mci_dto_start_monitor func.
> - use "broken-dto" for new quirk and change Subject for it.
>
> Changes in v3:
> - Remove dts for broken-dto, just add this quirk in dw_mci_rockchip_init
>
>  drivers/mmc/host/dw_mmc-rockchip.c |  3 +++
>  drivers/mmc/host/dw_mmc.c  | 41 
> +-
>  include/linux/mmc/dw_mmc.h |  5 +
>  3 files changed, 48 insertions(+), 1 deletion(-)

This seems reasonable to me.  I do hope that you can get to a root
cause, but I don't think this is a terrible bit of code to carry.
Obviously it's up to the folks who maintain dw_mmc and the mmc
subsystem, but:

Reviewed-by: Doug Anderson 
--
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] ASoC: rockchip-i2s: dt: add an optional property "i2s_clk_out"

[Doug Anderson]

Jianqun,

On Tue, Dec 2, 2014 at 6:49 AM, Jianqun Xu  wrote:
> Add an property "i2s_clk_out", which enables to output clock to outside
> of rockchip SoCs. Let's make it optional since not each board needs it.
>
> Signed-off-by: Jianqun Xu 
> ---
>  Documentation/devicetree/bindings/sound/rockchip-i2s.txt | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

You forgot to mention that this is a v3 in the subject line and forgot
to mention what changed between v2 and v3 (nothing changed, but you
should say that).

Despite that, this patch looks reasonable to me.

Reviewed-by: Doug Anderson 
--
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/2] ASoC: rockchip: i2s: add support for grabbing output clock to codec

[Doug Anderson]

Jianqun,

This ought to be a "v3" patch and ideally ought to describe
differences from v2 (after the cut).  Please have Kever or Chris
review your next patch before sending it out since I think they are
familiar with the process.


On Tue, Dec 2, 2014 at 6:52 AM, Jianqun Xu  wrote:
> From: Sonny Rao 
>
> We need to claim the clock which is driving the codec so that when we enable
> clock gating, we continue to clock the codec when needed.  I make this an
> optional clock since there might be some applications where we don't need it
> but can still use the I2S block.
>
> Signed-off-by: Sonny Rao 

You still forgot your own signed-off-by.  Please try again.  See



> +   i2s->oclk = devm_clk_get(&pdev->dev, "i2s_clk_out");
> +   if (IS_ERR(i2s->oclk)) {
> +   dev_dbg(&pdev->dev, "Didn't find output clock\n");
> +   i2s->oclk = NULL;
> +   }

You still forgot the blank line here requested by Heiko.  Please try
again.  See 


> +   if (i2s->oclk)
> +   ret = clk_prepare_enable(i2s->oclk);
> +
--
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: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

Addy,

On Mon, Dec 1, 2014 at 11:50 PM, addy ke  wrote:
> We don't know why we have this problem,
> but this problem is really exist, and we need patch to fix this problem now.
> I will post a follow up change when we find the root cause.

To me that seems reasonable.  Certainly I would prefer to have this
patch while waiting for the root cause, but it's up to Ulf.


> And there is a little probability of this problem on RK SoC, such as RK3188, 
> RK3066,
> when worse card inserted in.
> Maybe the other SoCs have the similar problem.
>
> So I will add this quirk in rockchip code(dw_mmc-rockchip.c) as follows:
> static int dw_mci_rockchip_parse_dt(struct dw_mci *host)
> {
> host->quirk |= DW_MCI_QUIRK_BROKEN_DTO;
>
> return 0;
> }
>
> ..
> .parse_dt = dw_mci_rockchip_parse_dt,
> ..
>
> is right?

When you added "sdio_id0" you got feedback that you should use
dw_mci_rockchip_init().  Why not do the same thing here?
--
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 v10 0/4] This suspend patch is only support cut off the power of cpu and some external

[Doug Anderson]

Hi,

On Mon, Dec 1, 2014 at 2:08 PM, Doug Anderson  wrote:
> Hi,
>
> On Mon, Dec 1, 2014 at 11:51 AM, Kevin Hilman  wrote:
>> Chris Zhong  writes:
>>
>>> devices, since we still lack power_domain driver, so the other power rail
>>> of rk3288 need keep power on.
>>> I have tested it on rk3288-evb board, atop next-20141112. goto suspend by 
>>> type
>>> "echo mem > /sys/power/state", vdd_cpu is about 0mv by measuring, so it can 
>>> be
>>> determined in sleep mode, then press power button to wakeup it.
>>
>> I tested this on top of today's linux-next (next-20141201) and it
>> suspends, but doesn't wake up from any of the button presses.  What
>> wakeup sources are configured for the rk3288-evb-rk808?
>
> Just to close the loop (I talked with Kevin over IM about this, too):
>
> I have a huge description of how I tested this as part of my patch at
> <https://patchwork.kernel.org/patch/5414941/>.  Chris:  I think Kevin
> has asked you several times to include information like this in your
> cover letter.  Please, please, please can you try to remember to do
> this?

Talked to Chris offline.  He said that in his tests the other patches
weren't needed, so he didn't list any other patches.  Things just
worked for him.  ...so I guess he did post the instructions that
worked for him.  Sorry for the complaint.  Possibly things are
different on "next-20141112" and that's where Chris said he tested.

I know that I personally needed some of the extra patches.  I guess
the USB one wasn't truly needed (only needed for hotplug), but for me
things were unhappy without SMP.  It was hanging when trying to turn
off secondary CPUs.  I didn't dig, though.  The clocksource patches
are needed for me because I'm using an old bootloader, but I think
they're also relevant for S2R because (I'm told) we can lose the
virtual offset at suspend time in certain modes.

-Doug

.
--
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 v10 4/4] ARM: dts: rockchip: add suspend settings for rk3288-evb-rk808

[Doug Anderson]

Chris,

On Mon, Dec 1, 2014 at 12:52 AM, Chris Zhong  wrote:
> Add suspend-voltages and necessary pin-states for suspend on
> rk3288-evb-rk808 boards. global_pwroff would be pulled high when
> RK3288 entering suspend, this pin is a sleep signal for RK808, so
> RK808 could goto sleep mode, and some regulators would be disable.
>
> Signed-off-by: Chris Zhong 
>
> ---
>
> Changes in v10:
> - remove regulator-suspend-mem-enabled and regulator-suspend-mem-microvolt
> - enable the lcd, codec, sdmmc power during suspend
>
> Changes in v9:
> - update the subject and description
>
> Changes in v8:
> - keep all except cpu&tp power rail on during suspend
> - add regulator-on-in-suspend before set suspend voltage
> - add a reference of ddrio_pwroff and ddr0_retention
>
> Changes in v7:
> - add regulator-state-mem sub node for suspend
>
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  arch/arm/boot/dts/rk3288-evb-rk808.dts | 53 
> +-
>  1 file changed, 52 insertions(+), 1 deletion(-)

I'm still not 100% clear on why it wakes up by itself, but I guess (?)
that could be solved in a separate patch.  I did confirm that if I
hacked the wakeup source to be just from GPIOs that it stayed asleep
and could be awakened by the power button.  I tested on linux-next
with evb-rk808.

Reviewed-by: Doug Anderson 
Tested-by: Doug Anderson 

-Doug
--
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 v10 0/4] This suspend patch is only support cut off the power of cpu and some external

[Doug Anderson]

Hi,

On Mon, Dec 1, 2014 at 11:51 AM, Kevin Hilman  wrote:
> Chris Zhong  writes:
>
>> devices, since we still lack power_domain driver, so the other power rail
>> of rk3288 need keep power on.
>> I have tested it on rk3288-evb board, atop next-20141112. goto suspend by 
>> type
>> "echo mem > /sys/power/state", vdd_cpu is about 0mv by measuring, so it can 
>> be
>> determined in sleep mode, then press power button to wakeup it.
>
> I tested this on top of today's linux-next (next-20141201) and it
> suspends, but doesn't wake up from any of the button presses.  What
> wakeup sources are configured for the rk3288-evb-rk808?

Just to close the loop (I talked with Kevin over IM about this, too):

I have a huge description of how I tested this as part of my patch at
.  Chris:  I think Kevin
has asked you several times to include information like this in your
cover letter.  Please, please, please can you try to remember to do
this?

For those that don't want to click on my link, I'll include the
relevant bits here:

---

Total patches atop that version of Linux were:

1. https://patchwork.kernel.org/patch/5051881/ - clocksource:
   arch_timer: Allow the device tree to specify uninitialized timer
   registers

2. https://patchwork.kernel.org/patch/5363671/ - clocksource:
   arch_timer: Fix code to use physical timers when requested

3. https://patchwork.kernel.org/patch/5382141/ - ARM: dts: rk3288: add
   arm,cpu-registers-not-fw-configured

4. Revert (b77d439 ARM: dts: rockchip: temporarily disable smp on
   rk3288)

5. https://patchwork.kernel.org/patch/5325111/ - usb: dwc2: resume
   root hub when device detect with suspend state

6. https://patchwork.kernel.org/patch/5410611/ - ARM: rockchip: add
   suspend and resume for RK3288

7. https://patchwork.kernel.org/patch/5410621/ - ARM: rockchip: Add
   pmu-sram binding

8. https://patchwork.kernel.org/patch/5410631/ - ARM: dts: add RK3288
   suspend support

9. https://patchwork.kernel.org/patch/5410641/ - ARM: dts: rockchip:
   add suspend settings for rk3288-evb-rk808

It looks like my pinctrl patches might be dropped due to cross
dependency problems, so tomorrow's linux-next will probably also need
(https://patchwork.kernel.org/patch/5344551/ - pinctrl: rockchip:
Handle wakeup pins).

I've also got a local hack to the Rockchip "pm.c" to replace the usage
of "PMU_ARMINT_WAKEUP_EN" with 0x0e.  There seems to be some sort of
ARM Interrupt waking us up all the time right when we go to sleep and
the above will hack it so that only GPIOs + SDMMC Card Detect can wake
us up.  Someone should track down what's going on there, but for now
I've used the hack to prove that the basic code actually works.
--
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 v9 3/4] ARM: dts: add RK3288 suspend support

[Doug Anderson]

Chris,

On Mon, Nov 24, 2014 at 11:32 PM, Chris Zhong  wrote:
> add pmu sram node for suspend, add global_pwroff pinctrl.
> The pmu sram is used to store the resume code.
> global_pwroff is held low level at work, it would be pull to high
> when entering suspend. reference this in the board DTS file since
> some boards need it.
>
> Signed-off-by: Tony Xie 
> Signed-off-by: Chris Zhong 
> Reviewed-by: Doug Anderson 
> Tested-by: Doug Anderson 
>
> ---
>
> Changes in v9: None

This is untrue.  v8 had more stuff than v9.  See:

https://patchwork.kernel.org/patch/5311621/

vs.

https://patchwork.kernel.org/patch/5372911/

I prefer v8.
--
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 v9 4/4] ARM: dts: rockchip: add suspend settings for rk3288-evb-rk808

[Doug Anderson]

Chris,

On Mon, Nov 24, 2014 at 11:32 PM, Chris Zhong  wrote:
> vcc_ddr: DCDC_REG3 {
> regulator-always-on;
> regulator-boot-on;
> regulator-name = "vcc_ddr";
> +   regulator-suspend-mem-enabled;

The "regulator-suspend-mem-enabled" is not an upstream property and
isn't doing anything.  It should be removed.

Possibly this is something Heiko could do when applying  (depends on
what he wants) if no other spins are required.
> vcc_18: LDO_REG7 {
> @@ -128,6 +163,11 @@
> regulator-min-microvolt = <180>;
> regulator-max-microvolt = <180>;
> regulator-name = "vcc_18";
> +   regulator-suspend-mem-microvolt = <180>;

Another non-upstream property: regulator-suspend-mem-microvolt
--
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: dw_mmc: add quirk for broken data transfer over scheme

[Doug Anderson]

Hi,

On Tue, Nov 25, 2014 at 12:10 AM, Addy Ke  wrote:
> This patch add a new quirk to add a s/w timer to notify the driver
> to terminate current transfer and report a data timeout to the core,
> if DTO interrupt does NOT come within the given time.
>
> dw_mmc call mmc_request_done func to finish transfer depends on
> DTO interrupt. If DTO interrupt does not come in sending data state,
> the current transfer will be blocked.
>
> But this case really exists, when driver reads tuning data from
> card on RK3288-pink2 board. I measured waveforms by oscilloscope
> and found that card clock was always on and data lines were always
> holded high level in sending data state.
>
> We got the reply from synopsys:
> There are two counters but both use the same value of [31:8] bits.
> Data timeout counter doesn't wait for stop clock and you should get
> DRTO even when the clock is not stopped.
> Host Starvation timeout counter is triggered with stop clock condition.
>
> This means that host should get DRTO and DTO interrupt.
>
> But we really don't get any data-related interrupt in RK3X SoCs.
> And driver can't get data transfer state, it can do nothing but wait for.

Have you asked someone on your IC team to confirm this is an SoC
errata on your SoC?  ...or is there something else we could be doing
wrong (overclocking?  jitter in the clock?  bad dividers?) that could
be causing this problem?


>  #ifdef CONFIG_OF
>  static struct dw_mci_of_quirks {
> char *quirk;
> @@ -2513,6 +2549,9 @@ static struct dw_mci_of_quirks {
> }, {
> .quirk  = "disable-wp",
> .id = DW_MCI_QUIRK_NO_WRITE_PROTECT,
> +   }, {
> +   .quirk  = "broken-dto",
> +   .id = DW_MCI_QUIRK_BROKEN_DTO,

You're adding a device tree property without any binding.  If you need
to add this please send a patch before this one modifying the device
tree bindings.

...but that brings up the question: do you _really_ need to add a
property?  You already know that all rk3288 SoCs need this and you
already know that you're an rk3288 SoC.  Just add this quirk in the
rk3288 code always and be done with it.  ...and if this is also needed
on other Rockchip parts, add it there too.

-Doug
--
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] clocksource: arch_timer: Allow the device tree to specify uninitialized timer registers

[Doug Anderson]

Yingjoe,

On Wed, Nov 26, 2014 at 6:41 AM, Yingjoe Chen  wrote:
> Sorry for the (very) late reply.
> I just realize today MT8135 need this and the other patch [1] to boot
> SMP correctly. I've applied both patches and they works fine. Thanks :)

Excellent.  It's helpful to include a Tested-by: tag in your email.
You'd have a line with just "Tested-by: Yingjoe Chen
"


> However, I'm not sure if we really need to add new property.
> arm_arch_timer driver will only use virtual timer when virtual PPI
> interrupt is provided, so the following patch to timer dtsi will also
> works. I think if the firmware doesn't support virtual timer, it make
> sense to not supply virtual interrupt.
>
> timer {
> compatible = "arm,armv7-timer";
> interrupts = ,
> -,
> -,
> -;
> +;
> clock-frequency = <1300>;
> };

Once you have Sonny's patch then I believe that the above would work.
However we rejected something like this because device tree is
supposed to describe the hardware.  The hardware really does provide
the virtual timer interrupts and they really are at PPI 11 and PPI 10.
It's just that firmware doesn't handle things properly so they can't
be used.

NOTE: If we add the "arm,cpu-registers-not-fw-configured" to the
device tree and firmware actually works out how to configure things
(like if somehow has firmware that has a hypervisor) then it can
easily remove this device tree property before calling through to the
kernel.  It would be much harder for the firmware to add back in the
"PPI 11" and "PPI 10" entries to the timer.

-Doug
--
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] clk: rockchip: add binding ID for DMC (memory controller) clocks on rk3288

[Doug Anderson]

Heiko,

On Tue, Nov 25, 2014 at 4:45 PM, Heiko Stübner  wrote:
> Am Dienstag, 25. November 2014, 16:13:02 schrieb Doug Anderson:
>> From: Jeff Chen 
>>
>> The DMC clocks need to be turned off at runtime, so we should have IDs
>> so we can export them.
>>
>> Signed-off-by: Jeff Chen 
>> [dianders: split into two patches; adjusted commit msg]
>> Signed-off-by: Doug Anderson 
>
> both patches look ok to me and I'll probably pick them up tomorrow.

OK, thanks!


> Just as handling question, do you plan on sending dts changes using these
> clocks in time for 3.19 [i.e. in the next couple of days]?

There's pretty much zero chance.  Maybe negative.


> We have currently three clock patches/series adding new clock ids [this one,
> the mmc phases and Sonny's i2s_clkout] and I'm trying to dertermine if I need
> a shared branch for those.
>
> Currently it doesn't look like the others will provide dts patches using the
> clocks in time, so it looks like I can skip the shared branch and we can use
> the clock ids regularly after the merge-window. Some true for your clocks?

Yup, no problem for me.  I provided this as two patches to you just in
case it was convenient, since I saw that as feedback in previous
submissions.

Thanks!

-Doug
--
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/2] clk: rockchip: add binding ID for DMC (memory controller) clocks on rk3288

[Doug Anderson]

From: Jeff Chen 

The DMC clocks need to be turned off at runtime, so we should have IDs
so we can export them.

Signed-off-by: Jeff Chen 
[dianders: split into two patches; adjusted commit msg]
Signed-off-by: Doug Anderson 
---
 include/dt-bindings/clock/rk3288-cru.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/clock/rk3288-cru.h 
b/include/dt-bindings/clock/rk3288-cru.h
index 100a08c..8e19ed5 100644
--- a/include/dt-bindings/clock/rk3288-cru.h
+++ b/include/dt-bindings/clock/rk3288-cru.h
@@ -141,6 +141,10 @@
 #define PCLK_VIO2_H2P  361
 #define PCLK_CPU   362
 #define PCLK_PERI  363
+#define PCLK_DDRUPCTL0 364
+#define PCLK_PUBL0 365
+#define PCLK_DDRUPCTL1 366
+#define PCLK_PUBL1 367
 
 /* hclk gates */
 #define HCLK_GPS   448
-- 
2.1.0.rc2.206.gedb03e5

--
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] Increase the maximum cpu frequency of rk3288

[Doug Anderson]

Hi,

On Tue, Nov 25, 2014 at 8:23 AM, Heiko Stübner  wrote:
> Hi Chris, Kever,
>
> Am Mittwoch, 26. November 2014, 00:13:40 schrieb Kever Yang:
>> On 11/25/2014 05:37 PM, Chris Zhong wrote:
>> > The maximum cpu frequency of rk3288 can up to 1.8Ghz, but the vdd_cpu need
>> > set to 1.4v. I've tested these patches on rk3288 evb board.
>>
>> I'm not sure why you need this patch, I think we have a discuss
>> for the cpu operating point before.
>> In this case:
>> 1. rk3288.dtsi is for all the rk3288 Soc based system, you may need
>> a separate opp table in rk3288-evb-rk808.dts;
>> 2. 1.4V may beyond the supported voltage range too much,
>>  and it's not a good idea to add it to rk3288.dtsi as a safe voltage.
>> 3. Do you have a stress/heavy load test on evb with 1.4v at 1.8GHz?
>
> That is how I remember it as well ... the default opp table is supposed to
> contain only the safest of values, while individual boards can then increase
> those in conjunction with their thermal design.
>
> As the evb itself does only have the naked soc without any heat-sink or
> similar, I'm not even sure if these additional frequencies should be enabled
> there at all.

I'm not sure that the story from Rockchip is all straight about all of
this stuff, but I will say that:

* The latest datasheet I have (1.3) shows the maximum CPU frequency as
1.8GHz.  It shows the maximum CPU voltage as TBD.  That indicates that
rk3288.dtsi ought to list 1.8GHz values.

* The 1.4V number comes from folks at Rockchip.  Please check with
Jianqun Xu and Eddie Cai to check.  If 1.4V is not safe we certainly
shouldn't use it.  I believe they have seen boards where 1.8GHz needed
1.4V to be reliable.  Either the SoCs on these boards are out of spec
or 1.4V is needed.

* If you're worried about heat we should make sure that all the
thermal changes are in.  That will still let you run at 1.8GHz
temporarily.  You can put a heatsync on and run faster.

* These numbers are still conservative as far as I understand.  For
instance on rk3288-pinky 1.8GHz is a full .1V lower at 1.3V.

* If EVB was shipped with pre-production chips that can't support high
voltages or frequencies, it should override the table to remove the
high frequencies.  However, if the table that Chris submitted is
generally correct (and conservative) for rk3288 devices that are
expected in real products then it seems like it ought to land in
rk3288.dtsi.


-Doug
--
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/2] Add support for the rockchip mmc clock phases using the framework

[Doug Anderson]

Heiko,

On Tue, Nov 25, 2014 at 1:35 AM, Heiko Stübner  wrote:
> Am Dienstag, 18. November 2014, 13:08:26 schrieb Alexandru M Stan:
>> For now all I have is the getter and setter for the phase, nothing that uses
>> it (that is ready). You can test the getter like this:
>> localhost ~ # cat /sys/kernel/debug/clk/clk_summary|grep sample -C 1
>> sclk_sdio1002400
>>  0 0 sdio1_sample   001200
>> 0 0 sdio1_drv  001200
>> 0 90 --
>>   sclk_sdmmc  11   29700
>>  0 0 sdmmc_sample 00   14850  0 134
>> sdmmc_drv00   14850  0 90 --
>>   sclk_sdio0  11   1
>>  0 0 sdio0_sample 005000  0 0
>> sdio0_drv005000  0 90
>> sclk_emmc   11   1  0 0
>> emmc_sample  005000  0 0
>> emmc_drv 005000  0 180
>>
>> Next thing that will come is some dts changes that will make use of these
>> new clocks, and eventually mmc code will be changed to tune with these
>> clocks.
>
> As already said in v1, this looks nice to me, but I'll give Mike another day
> or two to voice possible concerns.

Please hold off on applying.  I spent some time with Alex over the
last few days and we found a few issues.  There were a few tiny bugs,
but also we found that some cards were not tuning properly.

Specifically it appears that on our board most UHS cards will reports
two valid ranges of valid phases, like:
  0 - 135: good
  225 - 270: good

That means that 180 was bad and 315 were bad.  It's a little unclear
why there are two bad ranges (I know very little of the signaling of
MMC), but my uneducated guess was that one of the bad ranges was
because of timing and the other because of signal integrity
(reflections, etc?)

In any case, on some cards we were actually finding only one range,
like maybe we'd find that 0 - 270 were good and only 315 was bad.
Then we'd pick right in the middle of the range, like 135.  The
problem was that on these cards 135 was a little iffy.  It somehow
managed to pass the tuning most of the time but then it would fail in
real usage.  Even repeating tuning 1000 times wasn't enough to get it
to fail reliably.  Could it be that the "fine delay" actually varies
with a very long period, so maybe it acts like "40ps" for a while then
"80ps" for a while?  That means that when we thought tuned with 135
degrees maybe we actually tuned with 120 degrees or 150 degrees.  When
it changed we started getting failures?


Our current proposal is to add "22.5" degree increments to the driver,
which seemed to fix the problem on the cards we tested.  It
successfully noticed some extra failing phases.  Going by 22.5 is the
smallest increment we can go without exposing the iffy-ness of the
fine delay to the driver.  Remember that the fine delay is between
40-80ps per count and we assume 60ps.  So our delay can be 1.33x as
long or .67 times as long.  Going by 22.5, we get

0 = exactly 0 degrees (coarse delay)
22.5 = 15 degrees - 30 degrees (coarse delay + fine delay)
45.0 = 30 degrees - 60 degrees (coarse delay + fine delay)
67.5 = 45 degrees - 90 degrees (coarse delay + fine delay)
90.0 = exactly 90 degrees (coarse delay)

That means we can be guaranteed that the real delay when the user
requests 90.0 degrees is >= the relay delay when they request 67.5.
We also get a guarantee that we _definitely_ tested a phase that is 0,
a phase that is < 45 degrees and one that was >= 45 degrees.


If the above proposal is not OK or we find cards that still fail with
that, we might have to go back to the drawing board and somehow expose
the fact that our fine delay is not very precise.  Other proposals
that were talked about:

1. On higher speed cards, we could completely use the fine delay.  The
fine delay can easily make all 360 degrees, but it doesn't make them
reliably so you can't assume that 0 and 360 are the same  dw_mmc would
need to be aware of that.  Note that if the fine delay really does
change over time (like if it changed from 40ps per to 80ps per), this
could be a non-starter.

2. The really important thing is to find failure points and make sure
that we don't pick phases that are close to them.  As long as dw_mmc
was aware that requesting 80 degrees might give you anywhere between
53 and 107 degrees it could still gather good information.  If it saw
a failure when that failed then it probably shouldn't pick 45 or 90
degrees.  We could add some logic to the clock phase API for this.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More

Re: [PATCH] mmc: dw_mmc: try pick the exact same voltage as vmmc for vqmmc

[Doug Anderson]

Addy,

On Mon, Nov 24, 2014 at 6:38 PM, Addy  wrote:
>> In worst case scenario, VDD = 3.6V and VIO = 2.7V. That gives as the
>> factor of 0.75, thus we are inside spec but without margins.
>
> * From eMMC4.5 spec:
>   1. (VDDF)vcc: Supply voltage for flash memory,  which is  2.7v -- 3.3v
>   2. (VDD)vccq: Supply voltage for memory controller, which is  1.7v --
> 1.95v  and 2,7v -- 3.6v
>
> * And from RK3288 datasheet:
>   Digtial GPIO Power(SDMMC0_VDD --> vccq) is 3.0v -- 3.6v and 1.62v - 1.98v
>
> So I think:
> 3.3v:  (2.7v < vccq < 3.6v)   &&  (3.0v < vccq < 3.6v)  ==> (3.0v < vccq <
> 3.6v)
> 1.8v:  (1.7v < vccq < 1.95v)  && (1.62v < vccq < 1.98v) ==> (1.7v < vccq <
> 1.95v)
>
> and (2.7v < vcc < 3.3v)
>
> * And according to our hardware engineer:
>   All of supply voltage must have +/- 10% cushion.
>
> * And we have found in some worse card that there is 200mv voltage collapse
> when these card is insert.
>
> So I think the best resolution is that vcc and vccq is configurable int dt
> table.

Ah, interesting.  ...so what we really need to be able to do is to say
that the regulator we for vqmmc have supports the ranges 3.0V - 3.3V
and 1.7V - 1.95V but not anything in between 1.95V ad 3.0V.  I have no
idea how to express that in the regulator framework.

Technically you could take the IO Voltage Domains code (responsible
for choosing the 1.8V range or the 3.3V range) and have it communicate
the requirements to the regulator framework if you could figure out
how to communicate them.


...of course if you implemented my suggestion of keeping vqmmc as the
highest voltage <= vmmc then maybe the whole point is moot and we
don't have to figure it out.  Just make sure that vmmc never goes
below 3.0V.


-Doug
--
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] mmc: dw_mmc: try pick the exact same voltage as vmmc for vqmmc

[Doug Anderson]

Ulf,

On Mon, Nov 24, 2014 at 5:29 AM, Ulf Hansson  wrote:
>> 2. Several people I've talked to have expressed concerns that our
>> minimum value is 2.7V.  Apparently that's really on the edge and makes
>> EEs a little nervous.  The quick sample of cards sitting on my desk
>> shows that they seem to claim 0x00ff8000, which doesn't include 2.7V.
>
> 0x00ff8000 states what values of VDD levels the device supports. Not VIO.

Yup, I was just pointing out that possibly others were trying to get a
little bit of margin (not going all the way down to 2.7V) too.

>> Both of the above make me feel like dw_mmc should try its best to pick
>> a value for vqmmc that is closest to the value of vmmc (and >= 2.7V).
>> That also happens to make us work exactly like hosts where vmmc and
>> vqmmc are supplied by the same supply.
>
> I do see your point. And I agree that it would be nice to achieve
> something like this.
>
> The question is how to do this. For sure, we need to involve the mmc
> core to handle this correctly.

You could add a function to the core that we could call from
dw_mci_switch_voltage() and it would do all the work except trying to
set the UHS register.  That would certainly make it easy.  That could
try to set the highest voltage that is <= vmmc when we're at
MMC_SIGNAL_VOLTAGE_330.

-Doug
--
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] mmc: dw_mmc: try pick the exact same voltage as vmmc for vqmmc

[Doug Anderson]

Hi,

On Fri, Nov 21, 2014 at 9:42 AM, Doug Anderson  wrote:
> Ulf,
>
> On Fri, Nov 21, 2014 at 4:06 AM, Ulf Hansson  wrote:
>> [...]
>>
>>> Sure
>>> If the first card is sd2.0 since startup, dw_mci_switch_voltage will not be 
>>> called,
>>
>> That can't be right. mmc_power_up() should trigger
>> dw_mci_switch_voltage() to be invoked.
>
> Hmmm, I think you're right.  Addy: can you double check if it's only
> the 2nd card for you?  I was thinking that if a regulator is currently
> 3.3V and you request 2.7 - 3.3V the regulator framework will treat
> that as a noop.  ...but that definitely doesn't appear to be the case.
> When I boot up the first time even with no SD card plugged in, I see
> this at bootup:
>
> [3.042234] vccio_sd: 1800 <--> 3300 mV at 3300 mV
>
> ...showing that it started at 3.3V.  Then I see:
>
> $ grep "" /sys/class/regulator/regulator.16/{name,microvolts}
> /sys/class/regulator/regulator.16/name:vccio_sd
> /sys/class/regulator/regulator.16/microvolts:270
>
> ...so it is certainly getting changed even with no card plugged in.
>
>
> BTW: I don't actually have one of these failing cards--all of mine
> work.  Addy, do you know the make and model of the card you have that
> fails?

Just as a bit of a followup, I did some more digging...

1. It looks as if we now have a bit of "opposite" logic for vmmc vs.
vqmmc.  In mmc_power_up() I see that it sets the initial voltage as:

  host->ios.vdd = fls(ocr) - 1;

That actually means that we're going to pick the maximum voltage for
vmmc (of the supported voltages).  For vqmmc dw_mmc is using the
regulator framework which (as described in my previous message) will
pick the minimum.

2. Several people I've talked to have expressed concerns that our
minimum value is 2.7V.  Apparently that's really on the edge and makes
EEs a little nervous.  The quick sample of cards sitting on my desk
shows that they seem to claim 0x00ff8000, which doesn't include 2.7V.


Both of the above make me feel like dw_mmc should try its best to pick
a value for vqmmc that is closest to the value of vmmc (and >= 2.7V).
That also happens to make us work exactly like hosts where vmmc and
vqmmc are supplied by the same supply.


-Doug
--
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] mmc: dw_mmc: try pick the exact same voltage as vmmc for vqmmc

[Doug Anderson]

Ulf,

On Fri, Nov 21, 2014 at 4:06 AM, Ulf Hansson  wrote:
> [...]
>
>> Sure
>> If the first card is sd2.0 since startup, dw_mci_switch_voltage will not be 
>> called,
>
> That can't be right. mmc_power_up() should trigger
> dw_mci_switch_voltage() to be invoked.

Hmmm, I think you're right.  Addy: can you double check if it's only
the 2nd card for you?  I was thinking that if a regulator is currently
3.3V and you request 2.7 - 3.3V the regulator framework will treat
that as a noop.  ...but that definitely doesn't appear to be the case.
When I boot up the first time even with no SD card plugged in, I see
this at bootup:

[3.042234] vccio_sd: 1800 <--> 3300 mV at 3300 mV

...showing that it started at 3.3V.  Then I see:

$ grep "" /sys/class/regulator/regulator.16/{name,microvolts}
/sys/class/regulator/regulator.16/name:vccio_sd
/sys/class/regulator/regulator.16/microvolts:270

...so it is certainly getting changed even with no card plugged in.


BTW: I don't actually have one of these failing cards--all of mine
work.  Addy, do you know the make and model of the card you have that
fails?


-Doug
--
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] clocksource: arch_timer: Allow the device tree to specify uninitialized timer registers

[Doug Anderson]

Daniel,

On Wed, Oct 8, 2014 at 12:33 AM, Sonny Rao  wrote:
> From: Doug Anderson 
>
> Some 32-bit (ARMv7) systems are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
>   we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset (CNTVOFF)
>   between the virtual and physical counters.  Each core gets a
>   different random offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>   CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> On systems like the above, it doesn't make sense to use the virtual
> counter.  There's nobody managing the offset and each time a core goes
> down and comes back up it will get reinitialized to some other random
> value.
>
> This adds an optional property which can inform the kernel of this
> situation, and firmware is free to remove the property if it is going
> to initialize the CNTVOFF registers when each CPU comes out of reset.
>
> Currently, the best course of action in this case is to use the
> physical timer, which is why it is important that CNTHCTL hasn't been
> changed from its reset value and it's a reasonable assumption given
> that the firmware has never entered HYP mode.
>
> Note that it's been said that on ARMv8 systems the firmware and
> kernel really can't be architected as described above.  That means
> using the physical timer like this really only makes sense for ARMv7
> systems.
>
> Signed-off-by: Doug Anderson 
> Signed-off-by: Sonny Rao 
> Reviewed-by: Mark Rutland 
> ---
> Changes in v2:
> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>
> Changes in v3:
> - change property name to arm,cntvoff-not-fw-configured and specify
>   that the value of CNTHCTL.PL1PC(T)EN must still be the reset value
>   of 1 as per Mark Rutland
>
> Changes in v4:
> - change property name to arm,cpu-registers-not-fw-configured and
>   specify that all cpu registers must have architected reset values
>   per Mark Rutland
> - change from "#ifdef CONFIG_ARM" to "if (IS_ENABLED(CONFIG_ARM))" per
>   Arnd Bergmann
> ---
>  Documentation/devicetree/bindings/arm/arch_timer.txt | 8 
>  drivers/clocksource/arm_arch_timer.c | 8 
>  2 files changed, 16 insertions(+)

Do you know what the status of this patch is?  This patch and Sonny's
patch at <https://patchwork.kernel.org/patch/5051901/> are needed on
Rockchip rk3288 for some specific things:

1. To make SMP happy with coreboot.

2. To (I assume) make SMP happy after S2R (no matter which firmware is
used since I don't think anyone has PSCI for rk3288).

...we still need a DTS entry atop these patches, but that's trivial to
add once these land.

Thanks!
--
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: dts: fix PWM clock found on RK3288 Socs

[Doug Anderson]

Caesar,

On Tue, Nov 18, 2014 at 7:25 PM, Caesar Wang  wrote:
> We use the new PWM IP on RK3288,but the PWM's clock indeed incorrect.
>
> Signed-off-by: Caesar Wang 
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Right, I reported this when Kever was doing his "unused clocks" patch.
That's why this clock is currently "ignore unused".

...but though your change is right, I'd rather your change doesn't
land quite yet.  It totally wrecks havoc with the PWM regulator used
on many rk3288 boards and will glitch the main "logic" voltage at
bootup.  I have some patches that are MASSIVELY WIP:

https://chromium-review.googlesource.com/230640
WIP: regulator: pwm: Tell regulator framework that we can't turn off

https://chromium-review.googlesource.com/230641
WIP: PWM: handle the fact that firmware might have left us enabled

https://chromium-review.googlesource.com/230642
WIP: pwm-regulator stuff

I think that last one might blow up into several patches.  We need to
solve the problems from those patches before yours can really land.


It's pretty high on my list to finish the above patches, but right now
suspend/resume issues are higher priority so it's a bit stalled...  If
someone wants to hijack any of the above patches please let me know--I
won't be offended.

-Doug
--
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] clk: rockchip: add bindings for the mmc clock phases

[Doug Anderson]

Hi,

On Fri, Nov 14, 2014 at 2:52 PM, Alexandru M Stan  wrote:
> This will be used in a later patch for clock phase tuning.
>
> Suggested-by: Heiko Stuebner 
> Signed-off-by: Alexandru M Stan 
> ---
>  include/dt-bindings/clock/rk3288-cru.h | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/dt-bindings/clock/rk3288-cru.h 
> b/include/dt-bindings/clock/rk3288-cru.h
> index 100a08c..465d0f6 100644
> --- a/include/dt-bindings/clock/rk3288-cru.h
> +++ b/include/dt-bindings/clock/rk3288-cru.h
> @@ -72,6 +72,16 @@
>  #define SCLK_HEVC_CABAC111
>  #define SCLK_HEVC_CORE 112
>
> +#define SCLK_SDMMC_DRV_PHASE   113
> +#define SCLK_SDIO0_DRV_PHASE   114
> +#define SCLK_SDIO1_DRV_PHASE   115
> +#define SCLK_EMMC_DRV_PHASE116
> +
> +#define SCLK_SDMMC_SAMPLE_PHASE117
> +#define SCLK_SDIO0_SAMPLE_PHASE118
> +#define SCLK_SDIO1_SAMPLE_PHASE119
> +#define SCLK_EMMC_SAMPLE_PHASE 120
> +

Thinking about Mike T's comment, we might want to actually rename
these defines and just remove the "_PHASE".  These clocks _are_ the
drive and sample clocks.  Sure, they happen to have phases as one of
the attributes, but they aren't "phase clocks".

FYI, the TRM shows these clocks as:

- Into "clkgen" you see cclkin (the *2 clock)
- Out of "clkgen" you see:
  --> "cclk_in" (the non *2 clock)
  --> "cclk_in_drv" (the drive clock, which might be shifted)
  --> "cclk_in_sample" (the sample clock, which might be shifted)

Right now we're not modeling the non-shifted, non *2 clock.  I think
that's OK (otherwise we've got a bunch of old device trees to change,
since the old DTS files passed in the *2 clock as the card clock).


-Doug
--
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