3xx won't always be the only game in town. Not that I have a better name,
just pointing out that it has a limited shelf life.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
3xx won't always be the only game in town. Not that I have a better name,
just pointing out that it has a limited shelf life.
yup, I'm expecting we'll add new compatible strings, ie.
qcom,adreno-4xx, etc, as we add support for various future generations
BR,
-R
Alright, fair enough. Compatible strings are cheap.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
3xx won't always be the only game in town. Not that I have a better name,
just pointing out that it has a limited shelf life.
Actually, we prefer to avoid "xx" compatible strings. The compatible
should be the first actual device that is part of the xx series, and
newer IP should claim to be compatible with their own version plus the
original one (if they are possible to use with the same exact
programming model and only need extra code to deal with extensions).
That way you'll also have a way to tell them apart in the driver if
you can't derive it out of register contents, etc.
-Olof
As Olof said, choose a definite name here, and have variants claim
compatibility with that in addition to a variant specific string.
+- reg: Physical base address and length of the controller's registers.
Just the one reg entry?
The example has reg-names. Either document the name or drop it.
+- interrupts: The interrupt outputs from the controller.
How many? Which ones? Names?
+- clocks: device clocks
+ See ../clocks/clock-bindings.txt for details.
Similarly?
I should be able to read the binding and put together a DTS. Currently I
cannot.
+- qcom,chipid: gpu chip-id. Note this may become optional for future
+ devices if we can reliably read the chipid from hw
What's the problem with reading chipid from HW at the moment?
Should this possibly be optional, and only if not possible to read from
HW?
+- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
This is confusing. This is a node rather than a property, and in general
it's not a good idea to rely on node ordering (there's no such thing as
an array of nodes in the DTB).
+ - compatible: "qcom,gpu-pwrlevels"
+ - for each qcom,gpu-pwrlevel:
Is that a compatible string for each node, name, or what?
Any reason for having this as a container node at all?
Please don't use wildcard strings.
Is a node expected to have one of these entries, or all?
I would recommend using a bulleted list to describe compatible strings.
It allows for easier extension and the easy addition of notes. e.g.
- compatible: should contain:
* "foo,bar-xtreme" for bar variants with the xtreme extensions.
* "foo,bar" for all bar variants (inc. xtreme).
* "foo,baz" for baz variants.
* "foo,foo" for all variants.
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- clocks: device clocks
One entry? Multiple?
What do they correspond to on the data sheet?
Names?
+ See ../clocks/clock-bindings.txt for details.
+- qcom,hdmi-tx-ddc-clk: ddc clk pin
+- qcom,hdmi-tx-ddc-data: ddc data pin
+- qcom,hdmi-tx-hpd: hpd pin
What exactly is this unit? Judging by the GPU phandle it's not the GPU
itself.
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
All of my prior points for these properties stand.
+- connectors: array of phandles for output device(s)
What are valid devices to point this at?
+- clocks: device clocks
Likewise, see my above points regarding clocks.
+ See ../clocks/clock-bindings.txt for details.
+
+Optional properties:
+- gpus: phandle for gpu device
3xx won't always be the only game in town. Not that I have a better name,
just pointing out that it has a limited shelf life.
Actually, we prefer to avoid "xx" compatible strings. The compatible
should be the first actual device that is part of the xx series, and
newer IP should claim to be compatible with their own version plus the
original one (if they are possible to use with the same exact
programming model and only need extra code to deal with extensions).
That way you'll also have a way to tell them apart in the driver if
you can't derive it out of register contents, etc.
well, we could put entire chip-id (including the patch revision) in
the compatible string.. ie. qcom,adreno-320.2
BR,
-R
As Olof said, choose a definite name here, and have variants claim
compatibility with that in addition to a variant specific string.
quoted
+- reg: Physical base address and length of the controller's registers.
Just the one reg entry?
as far as I know.. keep in mind, I'm working without docs
The example has reg-names. Either document the name or drop it.
quoted
+- interrupts: The interrupt outputs from the controller.
How many? Which ones? Names?
quoted
+- clocks: device clocks
+ See ../clocks/clock-bindings.txt for details.
Similarly?
I should be able to read the binding and put together a DTS. Currently I
cannot.
quoted
+- qcom,chipid: gpu chip-id. Note this may become optional for future
+ devices if we can reliably read the chipid from hw
What's the problem with reading chipid from HW at the moment?
Should this possibly be optional, and only if not possible to read from
HW?
As far as I can tell from diving downstream android kgsl code, seems
like some a2xx you might be able to read the value from hw. Beyond
that I'm not entirely sure. (Remember, no docs.)
I'm leaving it as-is and if someone who has docs wants to submit patch
to change it, that is fine.
quoted
+- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
This is confusing. This is a node rather than a property, and in general
it's not a good idea to rely on node ordering (there's no such thing as
an array of nodes in the DTB).
quoted
+ - compatible: "qcom,gpu-pwrlevels"
+ - for each qcom,gpu-pwrlevel:
Is that a compatible string for each node, name, or what?
Any reason for having this as a container node at all?
some of the decisions are made to make it easier to re-use/support
existing bindings in downstream kernel..
Please don't use wildcard strings.
Is a node expected to have one of these entries, or all?
not quite sure what you mean.. Yes a node is expected to have
'compatible'. Which should have one of those values (with potentially
more values added later)
I would recommend using a bulleted list to describe compatible strings.
It allows for easier extension and the easy addition of notes. e.g.
- compatible: should contain:
* "foo,bar-xtreme" for bar variants with the xtreme extensions.
* "foo,bar" for all bar variants (inc. xtreme).
* "foo,baz" for baz variants.
* "foo,foo" for all variants.
sure
quoted
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- clocks: device clocks
One entry? Multiple?
What do they correspond to on the data sheet?
get someone to send me a data sheet. I'd love to tell you then ;-)
Names?
quoted
+ See ../clocks/clock-bindings.txt for details.
+- qcom,hdmi-tx-ddc-clk: ddc clk pin
+- qcom,hdmi-tx-ddc-data: ddc data pin
+- qcom,hdmi-tx-hpd: hpd pin
GPIOs should be called *-gpios.
again just trying to be compatible with some existing downstream DT..
What exactly is this unit? Judging by the GPU phandle it's not the GPU
itself.
the display
quoted
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
All of my prior points for these properties stand.
quoted
+- connectors: array of phandles for output device(s)
What are valid devices to point this at?
quoted
+- clocks: device clocks
Likewise, see my above points regarding clocks.
quoted
+ See ../clocks/clock-bindings.txt for details.
+
+Optional properties:
+- gpus: phandle for gpu device
What exactly is this used for?
'gpus' and 'connectors' is basically part of the componentized device
support. It is how the master/toplevel drm device knows what
sub-devices exist.
Currently there is just one valid possibility for each, but as we add
DSI, DP, etc, and support for various other generations there will
hopefully be more. (For example, some older snapdragons had one 3d
core and zero to two 2d cores.)
BR,
-R
From: Jordan Crouse <hidden> Date: 2014-07-02 21:09:39
On 07/02/2014 03:01 PM, Rob Clark wrote:
As far as I can tell from diving downstream android kgsl code, seems
like some a2xx you might be able to read the value from hw. Beyond
that I'm not entirely sure. (Remember, no docs.)
Ohhh! I get to tell a story!
The on chip registers have a long and consistent history of being wrong.
And not just the spin ID being wrong - the major and minor have had their
issues too. It got to the point that sometime around the A320 timeframe
we put our foot down and decided that we would never again rely on the
hardware. The reasoning is that the SoC and the GPU are on the same die
so we can use the one to identify the other. And thats what we've done
since then - first in the board files and now in the DT.
So long story short - the registers are there, we don't trust them as
far as we could throw them. Use the SoC instead.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
As Olof said, choose a definite name here, and have variants claim
compatibility with that in addition to a variant specific string.
quoted
+- reg: Physical base address and length of the controller's registers.
Just the one reg entry?
as far as I know.. keep in mind, I'm working without docs
Sure. If we only know about one reg entrty that's fine.
quoted
The example has reg-names. Either document the name or drop it.
quoted
+- interrupts: The interrupt outputs from the controller.
How many? Which ones? Names?
quoted
+- clocks: device clocks
+ See ../clocks/clock-bindings.txt for details.
Similarly?
I should be able to read the binding and put together a DTS. Currently I
cannot.
quoted
+- qcom,chipid: gpu chip-id. Note this may become optional for future
+ devices if we can reliably read the chipid from hw
What's the problem with reading chipid from HW at the moment?
Should this possibly be optional, and only if not possible to read from
HW?
As far as I can tell from diving downstream android kgsl code, seems
like some a2xx you might be able to read the value from hw. Beyond
that I'm not entirely sure. (Remember, no docs.)
I'm leaving it as-is and if someone who has docs wants to submit patch
to change it, that is fine.
My concern is that the statement "this may become optional" is useless,
as it doesn't tell you what expected if it's not present.
I would reword this as:
- qcom,chip-id: The gpu chip-id, if the hardware value is not reliable.
And then read from the HW if it's not present.
quoted
quoted
+- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
This is confusing. This is a node rather than a property, and in general
it's not a good idea to rely on node ordering (there's no such thing as
an array of nodes in the DTB).
quoted
+ - compatible: "qcom,gpu-pwrlevels"
+ - for each qcom,gpu-pwrlevel:
Is that a compatible string for each node, name, or what?
Any reason for having this as a container node at all?
some of the decisions are made to make it easier to re-use/support
existing bindings in downstream kernel..
I don't see why we should be beholden to downstream kernel bindings.
Please don't use wildcard strings.
Is a node expected to have one of these entries, or all?
not quite sure what you mean.. Yes a node is expected to have
'compatible'. Which should have one of those values (with potentially
more values added later)
Sorry for the lack of clarity. I meant is it expected that I should
have:
compatible = "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960",
"qcom,hdmi-tx-8x74";
Or just a single string for a given device?
I assume the latter.
quoted
I would recommend using a bulleted list to describe compatible strings.
It allows for easier extension and the easy addition of notes. e.g.
- compatible: should contain:
* "foo,bar-xtreme" for bar variants with the xtreme extensions.
* "foo,bar" for all bar variants (inc. xtreme).
* "foo,baz" for baz variants.
* "foo,foo" for all variants.
sure
quoted
quoted
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- clocks: device clocks
One entry? Multiple?
What do they correspond to on the data sheet?
get someone to send me a data sheet. I'd love to tell you then ;-)
Sure :)
quoted
Names?
quoted
+ See ../clocks/clock-bindings.txt for details.
+- qcom,hdmi-tx-ddc-clk: ddc clk pin
+- qcom,hdmi-tx-ddc-data: ddc data pin
+- qcom,hdmi-tx-hpd: hpd pin
GPIOs should be called *-gpios.
again just trying to be compatible with some existing downstream DT..
What exactly is this unit? Judging by the GPU phandle it's not the GPU
itself.
the display
I see. Could that be mentioned in the heading line above?
quoted
quoted
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
All of my prior points for these properties stand.
quoted
+- connectors: array of phandles for output device(s)
What are valid devices to point this at?
quoted
+- clocks: device clocks
Likewise, see my above points regarding clocks.
quoted
+ See ../clocks/clock-bindings.txt for details.
+
+Optional properties:
+- gpus: phandle for gpu device
What exactly is this used for?
'gpus' and 'connectors' is basically part of the componentized device
support. It is how the master/toplevel drm device knows what
sub-devices exist.
Currently there is just one valid possibility for each, but as we add
DSI, DP, etc, and support for various other generations there will
hopefully be more. (For example, some older snapdragons had one 3d
core and zero to two 2d cores.)
Ok. I must admit to being unfamiliar with how that's expected to work.
Thanks,
Mark.
As Olof said, choose a definite name here, and have variants claim
compatibility with that in addition to a variant specific string.
quoted
+- reg: Physical base address and length of the controller's registers.
Just the one reg entry?
as far as I know.. keep in mind, I'm working without docs
Sure. If we only know about one reg entrty that's fine.
quoted
quoted
The example has reg-names. Either document the name or drop it.
quoted
+- interrupts: The interrupt outputs from the controller.
How many? Which ones? Names?
quoted
+- clocks: device clocks
+ See ../clocks/clock-bindings.txt for details.
Similarly?
I should be able to read the binding and put together a DTS. Currently I
cannot.
quoted
+- qcom,chipid: gpu chip-id. Note this may become optional for future
+ devices if we can reliably read the chipid from hw
What's the problem with reading chipid from HW at the moment?
Should this possibly be optional, and only if not possible to read from
HW?
As far as I can tell from diving downstream android kgsl code, seems
like some a2xx you might be able to read the value from hw. Beyond
that I'm not entirely sure. (Remember, no docs.)
I'm leaving it as-is and if someone who has docs wants to submit patch
to change it, that is fine.
My concern is that the statement "this may become optional" is useless,
as it doesn't tell you what expected if it's not present.
I would reword this as:
- qcom,chip-id: The gpu chip-id, if the hardware value is not reliable.
yeah, ok. That is more clear.
And then read from the HW if it's not present.
quoted
quoted
quoted
+- qcom,gpu-pwrlevels: array of OPPs, sorted highest to lowest
This is confusing. This is a node rather than a property, and in general
it's not a good idea to rely on node ordering (there's no such thing as
an array of nodes in the DTB).
btw, we don't actually rely on the ordering in the parsing code.
Although I think it is more readable if it is kept sorted. So I
suppose consider that as more of a suggestion.
quoted
quoted
quoted
+ - compatible: "qcom,gpu-pwrlevels"
+ - for each qcom,gpu-pwrlevel:
Is that a compatible string for each node, name, or what?
Any reason for having this as a container node at all?
some of the decisions are made to make it easier to re-use/support
existing bindings in downstream kernel..
I don't see why we should be beholden to downstream kernel bindings.
Well, unfortunately since devices that run upstream kernel are few and
far between, I get to port upstream driver to more downstream device
kernels than I'd care to. To simplify this, as much as possible I'd
like to share DT binding code. The alternative is more error prone
and time consuming, and it's not exactly like I'm running out of
things to do.
So? Either define those or don't bother. Having a halfway house binding
is not helpful.
I'm not really in a position to document them at this point, sorry.
Then drop it.
Not sure I understand. The gpu-freq we need. I could drop the NOTE,
although it does kind of explain why a simple list of integers is not
used.
Anything added later would be optional, ofc, for backwards compat.
Please don't use wildcard strings.
Is a node expected to have one of these entries, or all?
not quite sure what you mean.. Yes a node is expected to have
'compatible'. Which should have one of those values (with potentially
more values added later)
Sorry for the lack of clarity. I meant is it expected that I should
have:
compatible = "qcom,hdmi-tx-8x60", "qcom,hdmi-tx-8960",
"qcom,hdmi-tx-8x74";
Or just a single string for a given device?
I assume the latter.
oh, yes. I kinda thought the use of "compatible" everywhere was
enough that it would be obvious that this meant "pick one"..
I guess re-arranging as a bullet-list would make that more clear.
quoted
quoted
I would recommend using a bulleted list to describe compatible strings.
It allows for easier extension and the easy addition of notes. e.g.
- compatible: should contain:
* "foo,bar-xtreme" for bar variants with the xtreme extensions.
* "foo,bar" for all bar variants (inc. xtreme).
* "foo,baz" for baz variants.
* "foo,foo" for all variants.
sure
quoted
quoted
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
+- clocks: device clocks
One entry? Multiple?
What do they correspond to on the data sheet?
get someone to send me a data sheet. I'd love to tell you then ;-)
Sure :)
quoted
quoted
Names?
quoted
+ See ../clocks/clock-bindings.txt for details.
+- qcom,hdmi-tx-ddc-clk: ddc clk pin
+- qcom,hdmi-tx-ddc-data: ddc data pin
+- qcom,hdmi-tx-hpd: hpd pin
GPIOs should be called *-gpios.
again just trying to be compatible with some existing downstream DT..
What exactly is this unit? Judging by the GPU phandle it's not the GPU
itself.
the display
I see. Could that be mentioned in the heading line above?
will do
quoted
quoted
quoted
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
+- interrupts: The interrupt outputs from the controller.
All of my prior points for these properties stand.
quoted
+- connectors: array of phandles for output device(s)
What are valid devices to point this at?
quoted
+- clocks: device clocks
Likewise, see my above points regarding clocks.
quoted
+ See ../clocks/clock-bindings.txt for details.
+
+Optional properties:
+- gpus: phandle for gpu device
What exactly is this used for?
'gpus' and 'connectors' is basically part of the componentized device
support. It is how the master/toplevel drm device knows what
sub-devices exist.
Currently there is just one valid possibility for each, but as we add
DSI, DP, etc, and support for various other generations there will
hopefully be more. (For example, some older snapdragons had one 3d
core and zero to two 2d cores.)
Ok. I must admit to being unfamiliar with how that's expected to work.
Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.
Signed-off-by: Rob Clark <redacted>
---
Commence bikeshedding :-)
"qcom,hdmi-tx-8x74"
+- reg: Physical base address and length of the controller's registers.
Since you are adding "qcom,hdmi-tx-8x74" (separate address space for PHY
registers) in the compatible entry, how about this for the register
description:
- reg: Physical base address and length of the controllers' registers.
- reg-names: names corresponding to the defined register sets,
- "core_physical": HDMI Core registers
- (optional) "phy_physical": HDMI PHY registers
--- /dev/null+++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
<at> <at> -0,0 +1,40 <at> <at>
+Qualcomm adreno/snapdragon
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
As per the code (mdp5_kms.c), there are two sets of registers: "mdp_phys"
and "vbif_phys". They should probably be added in the description here.
"reg-names" entry might be needed as well.
From: Rob Clark <hidden> Date: 2014-07-03 18:14:28
On Thu, Jul 3, 2014 at 12:36 PM, [off-list ref] wrote:
Hi Rob,
quoted
Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.
Signed-off-by: Rob Clark <redacted>
---
Commence bikeshedding :-)
"qcom,hdmi-tx-8x74"
+- reg: Physical base address and length of the controller's registers.
Since you are adding "qcom,hdmi-tx-8x74" (separate address space for PHY
registers) in the compatible entry, how about this for the register
description:
- reg: Physical base address and length of the controllers' registers.
- reg-names: names corresponding to the defined register sets,
- "core_physical": HDMI Core registers
- (optional) "phy_physical": HDMI PHY registers
Oh, right.. I forgot 8x74 had phy in a separate region..
--- /dev/null+++ b/Documentation/devicetree/bindings/drm/msm/msm.txt
<at> <at> -0,0 +1,40 <at> <at>
+Qualcomm adreno/snapdragon
+
+Required properties:
+- compatible: "qcom,mdp" (mdp4) or "qcom,mdss_mdp" (mdp5)
+- reg: Physical base address and length of the controller's registers.
As per the code (mdp5_kms.c), there are two sets of registers: "mdp_phys"
and "vbif_phys". They should probably be added in the description here.
"reg-names" entry might be needed as well.
hmm.. well, we aren't actually using vbif_phys *yet*.. although not
really sure if that will change.
I am wondering if maybe we should split out mdp4 vs mdp5 DT bindings.
Or at least remove the mdp5 compat string for now. So far I've only
got mdp4 working upstream, so probably a few things I've missed for
mdp5. Maybe just removing "qcom,mdss_mdp" for the time being is safer
until we decide.
I guess we should be really close to having all the needed
dependencies for 8074/dragonboard, so I suppose that is something I
should play with. Although seems unlikely that I'll have time until
after 3.17 merge window. Mostly I wanted to get mdp4 working first,
since I already have enough overlay support working there to be useful
for atomic modeset/pageflip ;-)
BR,
-R
From: Rob Clark <hidden> Date: 2014-07-08 16:00:44
Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.
Signed-off-by: Rob Clark <redacted>
---
I thought I sent this already, but looks like I've forgot.
I've updated the names for the GPIO properties (and, via helper, made
driver check both with and without -gpio suffix). Added clock-names,
etc.
For now I've removed the compatible string for 8074 hdmi phy. Due to
integration differences between mdp4 and mdp5 devices, it may end up
being easier to document the bindings separately (even though most of
the code is in common). When paired with mdp5 display controller, for
example, the HDMI block does not have it's own irq.
Documentation/devicetree/bindings/drm/msm/gpu.txt | 52 +++++++++++++++++
Documentation/devicetree/bindings/drm/msm/hdmi.txt | 46 +++++++++++++++
Documentation/devicetree/bindings/drm/msm/mdp.txt | 48 +++++++++++++++
drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 2 +
drivers/gpu/drm/msm/hdmi/hdmi.c | 68 ++++++++++++++--------
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 10 +++-
drivers/gpu/drm/msm/msm_drv.c | 29 +++++++--
7 files changed, 225 insertions(+), 30 deletions(-)
create mode 100644 Documentation/devicetree/bindings/drm/msm/gpu.txt
create mode 100644 Documentation/devicetree/bindings/drm/msm/hdmi.txt
create mode 100644 Documentation/devicetree/bindings/drm/msm/mdp.txt
@@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)for(i=0;i<config->hpd_reg_cnt;i++){structregulator*reg;-reg=devm_regulator_get(&pdev->dev,config->hpd_reg_names[i]);+reg=devm_regulator_get_exclusive(&pdev->dev,+config->hpd_reg_names[i]);if(IS_ERR(reg)){ret=PTR_ERR(reg);dev_err(dev->dev,"failed to get hpd regulator: %s (%d)\n",
@@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)for(i=0;i<config->pwr_reg_cnt;i++){structregulator*reg;-reg=devm_regulator_get(&pdev->dev,config->pwr_reg_names[i]);+reg=devm_regulator_get_exclusive(&pdev->dev,+config->pwr_reg_names[i]);if(IS_ERR(reg)){ret=PTR_ERR(reg);dev_err(dev->dev,"failed to get pwr regulator: %s (%d)\n",
@@ -266,37 +268,55 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data){intgpio=of_get_named_gpio(of_node,name,0);if(gpio<0){-dev_err(dev,"failed to get gpio: %s (%d)\n",-name,gpio);-gpio=-1;+charname2[32];+snprintf(name2,sizeof(name2),"%s-gpio",name);+gpio=of_get_named_gpio(of_node,name2,0);+if(gpio<0){+dev_err(dev,"failed to get gpio: %s (%d)\n",+name,gpio);+gpio=-1;+}}returngpio;}-/* TODO actually use DT.. */-staticconstchar*hpd_reg_names[]={"hpd-gdsc","hpd-5v"};-staticconstchar*pwr_reg_names[]={"core-vdda","core-vcc"};-staticconstchar*hpd_clk_names[]={"iface_clk","core_clk","mdp_core_clk"};-staticunsignedlonghpd_clk_freq[]={0,19200000,0};-staticconstchar*pwr_clk_names[]={"extp_clk","alt_iface_clk"};+if(of_device_is_compatible(of_node,"qcom,hdmi-tx-8074")){+staticconstchar*hpd_reg_names[]={"hpd-gdsc","hpd-5v"};+staticconstchar*pwr_reg_names[]={"core-vdda","core-vcc"};+staticconstchar*hpd_clk_names[]={"iface_clk","core_clk","mdp_core_clk"};+staticunsignedlonghpd_clk_freq[]={0,19200000,0};+staticconstchar*pwr_clk_names[]={"extp_clk","alt_iface_clk"};+config.phy_init=hdmi_phy_8x74_init;+config.hpd_reg_names=hpd_reg_names;+config.hpd_reg_cnt=ARRAY_SIZE(hpd_reg_names);+config.pwr_reg_names=pwr_reg_names;+config.pwr_reg_cnt=ARRAY_SIZE(pwr_reg_names);+config.hpd_clk_names=hpd_clk_names;+config.hpd_freq=hpd_clk_freq;+config.hpd_clk_cnt=ARRAY_SIZE(hpd_clk_names);+config.pwr_clk_names=pwr_clk_names;+config.pwr_clk_cnt=ARRAY_SIZE(pwr_clk_names);+config.shared_irq=true;+}elseif(of_device_is_compatible(of_node,"qcom,hdmi-tx-8960")){+staticconstchar*hpd_clk_names[]={"core_clk","master_iface_clk","slave_iface_clk"};+staticconstchar*hpd_reg_names[]={"core-vdda","hdmi-mux"};+config.phy_init=hdmi_phy_8960_init;+config.hpd_reg_names=hpd_reg_names;+config.hpd_reg_cnt=ARRAY_SIZE(hpd_reg_names);+config.hpd_clk_names=hpd_clk_names;+config.hpd_clk_cnt=ARRAY_SIZE(hpd_clk_names);+}elseif(of_device_is_compatible(of_node,"qcom,hdmi-tx-8660")){+config.phy_init=hdmi_phy_8x60_init;+}else{+dev_err(dev,"unknown phy: %s\n",of_node->name);+}-config.phy_init=hdmi_phy_8x74_init;config.mmio_name="core_physical";-config.hpd_reg_names=hpd_reg_names;-config.hpd_reg_cnt=ARRAY_SIZE(hpd_reg_names);-config.pwr_reg_names=pwr_reg_names;-config.pwr_reg_cnt=ARRAY_SIZE(pwr_reg_names);-config.hpd_clk_names=hpd_clk_names;-config.hpd_freq=hpd_clk_freq;-config.hpd_clk_cnt=ARRAY_SIZE(hpd_clk_names);-config.pwr_clk_names=pwr_clk_names;-config.pwr_clk_cnt=ARRAY_SIZE(pwr_clk_names);config.ddc_clk_gpio=get_gpio("qcom,hdmi-tx-ddc-clk");config.ddc_data_gpio=get_gpio("qcom,hdmi-tx-ddc-data");config.hpd_gpio=get_gpio("qcom,hdmi-tx-hpd");config.mux_en_gpio=get_gpio("qcom,hdmi-tx-mux-en");config.mux_sel_gpio=get_gpio("qcom,hdmi-tx-mux-sel");-config.shared_irq=true;#elsestaticconstchar*hpd_clk_names[]={
@@ -373,7 +393,9 @@ static int hdmi_dev_remove(struct platform_device *pdev)}staticconststructof_device_iddt_match[]={-{.compatible="qcom,hdmi-tx"},+{.compatible="qcom,hdmi-tx-8074"},+{.compatible="qcom,hdmi-tx-8960"},+{.compatible="qcom,hdmi-tx-8660"},{}};
Hi Rob,
On Tue, Jul 8, 2014 at 9:30 PM, Rob Clark [off-list ref] wrote:
Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.
Signed-off-by: Rob Clark <redacted>
---
I thought I sent this already, but looks like I've forgot.
@@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)for(i=0;i<config->hpd_reg_cnt;i++){structregulator*reg;-reg=devm_regulator_get(&pdev->dev,config->hpd_reg_names[i]);+reg=devm_regulator_get_exclusive(&pdev->dev,+config->hpd_reg_names[i]);if(IS_ERR(reg)){ret=PTR_ERR(reg);dev_err(dev->dev,"failed to get hpd regulator: %s (%d)\n",
@@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)for(i=0;i<config->pwr_reg_cnt;i++){structregulator*reg;-reg=devm_regulator_get(&pdev->dev,config->pwr_reg_names[i]);+reg=devm_regulator_get_exclusive(&pdev->dev,+config->pwr_reg_names[i]);if(IS_ERR(reg)){ret=PTR_ERR(reg);dev_err(dev->dev,"failed to get pwr regulator: %s (%d)\n",
Don't we need to have a if(regulator_enabled) check after
devm_regulator_get function ?
I see a similar test after camera regulator_get function call.
-Regards
Divya
From: Rob Clark <hidden> Date: 2014-07-17 15:29:33
On Thu, Jul 17, 2014 at 4:10 AM, divya ojha [off-list ref] wrote:
Hi Rob,
On Tue, Jul 8, 2014 at 9:30 PM, Rob Clark [off-list ref] wrote:
quoted
Now that we (almost) have enough dependencies in place (MMCC, RPM, etc),
add necessary DT support so that we can use drm/msm on upstream kernel.
Signed-off-by: Rob Clark <redacted>
---
I thought I sent this already, but looks like I've forgot.
@@ -123,7 +123,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)for(i=0;i<config->hpd_reg_cnt;i++){structregulator*reg;-reg=devm_regulator_get(&pdev->dev,config->hpd_reg_names[i]);+reg=devm_regulator_get_exclusive(&pdev->dev,+config->hpd_reg_names[i]);if(IS_ERR(reg)){ret=PTR_ERR(reg);dev_err(dev->dev,"failed to get hpd regulator: %s (%d)\n",
@@ -138,7 +139,8 @@ struct hdmi *hdmi_init(struct drm_device *dev, struct drm_encoder *encoder)for(i=0;i<config->pwr_reg_cnt;i++){structregulator*reg;-reg=devm_regulator_get(&pdev->dev,config->pwr_reg_names[i]);+reg=devm_regulator_get_exclusive(&pdev->dev,+config->pwr_reg_names[i]);if(IS_ERR(reg)){ret=PTR_ERR(reg);dev_err(dev->dev,"failed to get pwr regulator: %s (%d)\n",
Don't we need to have a if(regulator_enabled) check after
devm_regulator_get function ?
I see a similar test after camera regulator_get function call.
tbh, I'm not 100% sure.
Normally I would say that any driver using some resource (regulator,
etc) should take it's own reference irrespective of whether some other
driver already has the regulator/etc, enabled. Otherwise the
reference counting doesn't work out.
The thing I'm not 100% sure about is interaction w/ bootloader, in
cases where bootloader does initial modeset. I've had some problems
in this area in the past when I was trying to get DSI working on my
nexus4. I doubt the correct solution is for the driver to check if
regulator is already enabled by bootloader before enabling it, but I'm
not sure if something else somewhere needs to drop references to
resources enabled by bootloader.
BR,
-R
On Thu, Jul 17, 2014 at 8:29 AM, Rob Clark [off-list ref] wrote:
On Thu, Jul 17, 2014 at 4:10 AM, divya ojha [off-list ref] wrote:
[...]
quoted
Don't we need to have a if(regulator_enabled) check after
devm_regulator_get function ?
I see a similar test after camera regulator_get function call.
tbh, I'm not 100% sure.
Regulators in the Qualcomm platform works by casting votes with the
RPM and there is no way to query the current state of a regulator, so
the regulator code will only return information regarding the Linux
systems votes.
So, we can unfortunately not check this.
This setup also makes it important that if we want a regulator on and
set to a certain voltage then we need to cast our vote with these
values, or some other subsystem might lower it by releasing their
votes.
Regards,
Bjorn