diff mbox

[06/23] ARM: OMAP: add OMAP5 DSI muxing

Message ID 1398334639-14172-7-git-send-email-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomi Valkeinen April 24, 2014, 10:17 a.m. UTC
Add support to set OMAP5 DSI pin muxing.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/display.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

archit taneja April 25, 2014, 11:11 a.m. UTC | #1
Hi,

On Thursday 24 April 2014 03:47 PM, Tomi Valkeinen wrote:
> Add support to set OMAP5 DSI pin muxing.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> ---
>   arch/arm/mach-omap2/display.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
> index 16d33d831287..974461441fc3 100644
> --- a/arch/arm/mach-omap2/display.c
> +++ b/arch/arm/mach-omap2/display.c
> @@ -137,11 +137,42 @@ static int omap4_dsi_mux_pads(int dsi_id, unsigned lanes)
>   	return 0;
>   }
>
> +#define CONTROL_PAD_BASE	0x4A002800
> +#define CONTROL_DSIPHY		0x614
> +

I guess this is something we can move to our driver, and use sysconf to 
get the register from DT.

Archit
Tomi Valkeinen April 25, 2014, 11:18 a.m. UTC | #2
On 25/04/14 14:11, Archit Taneja wrote:
> Hi,
> 
> On Thursday 24 April 2014 03:47 PM, Tomi Valkeinen wrote:
>> Add support to set OMAP5 DSI pin muxing.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>> ---
>>   arch/arm/mach-omap2/display.c | 35 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-omap2/display.c
>> b/arch/arm/mach-omap2/display.c
>> index 16d33d831287..974461441fc3 100644
>> --- a/arch/arm/mach-omap2/display.c
>> +++ b/arch/arm/mach-omap2/display.c
>> @@ -137,11 +137,42 @@ static int omap4_dsi_mux_pads(int dsi_id,
>> unsigned lanes)
>>       return 0;
>>   }
>>
>> +#define CONTROL_PAD_BASE    0x4A002800
>> +#define CONTROL_DSIPHY        0x614
>> +
> 
> I guess this is something we can move to our driver, and use sysconf to
> get the register from DT.

I just copied the same method as used for OMAP4.

I guess sysconf is an option. But I really dislike the idea of moving
omap control module code to a display driver... I'm not sure what other
options we have, though. Maybe an OMAP DSI specific pinctrl driver?

 Tomi
archit taneja April 25, 2014, 12:58 p.m. UTC | #3
On Friday 25 April 2014 04:48 PM, Tomi Valkeinen wrote:
> On 25/04/14 14:11, Archit Taneja wrote:
>> Hi,
>>
>> On Thursday 24 April 2014 03:47 PM, Tomi Valkeinen wrote:
>>> Add support to set OMAP5 DSI pin muxing.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> ---
>>>    arch/arm/mach-omap2/display.c | 35 ++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/display.c
>>> b/arch/arm/mach-omap2/display.c
>>> index 16d33d831287..974461441fc3 100644
>>> --- a/arch/arm/mach-omap2/display.c
>>> +++ b/arch/arm/mach-omap2/display.c
>>> @@ -137,11 +137,42 @@ static int omap4_dsi_mux_pads(int dsi_id,
>>> unsigned lanes)
>>>        return 0;
>>>    }
>>>
>>> +#define CONTROL_PAD_BASE    0x4A002800
>>> +#define CONTROL_DSIPHY        0x614
>>> +
>>
>> I guess this is something we can move to our driver, and use sysconf to
>> get the register from DT.
>
> I just copied the same method as used for OMAP4.
>
> I guess sysconf is an option. But I really dislike the idea of moving
> omap control module code to a display driver... I'm not sure what other
> options we have, though. Maybe an OMAP DSI specific pinctrl driver?

OMAP4 has CONTROL_DSIPHY for configuring both lane enable/disbale, and 
pull up/down, but OMAP5 has normal PAD_CONF registers for DSI lines(2 
pins per register) for configuring pull up/down, and CONTROL_DSIPHY for 
lane enable/disable.

We would have a very messed up pinctrl driver, but it would probably be 
better than doing all this stuff in the driver.

Archit
Tomi Valkeinen April 25, 2014, 2:08 p.m. UTC | #4
On 25/04/14 15:58, Archit Taneja wrote:
> On Friday 25 April 2014 04:48 PM, Tomi Valkeinen wrote:
>> On 25/04/14 14:11, Archit Taneja wrote:
>>> Hi,
>>>
>>> On Thursday 24 April 2014 03:47 PM, Tomi Valkeinen wrote:
>>>> Add support to set OMAP5 DSI pin muxing.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> ---
>>>>    arch/arm/mach-omap2/display.c | 35
>>>> ++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 34 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/display.c
>>>> b/arch/arm/mach-omap2/display.c
>>>> index 16d33d831287..974461441fc3 100644
>>>> --- a/arch/arm/mach-omap2/display.c
>>>> +++ b/arch/arm/mach-omap2/display.c
>>>> @@ -137,11 +137,42 @@ static int omap4_dsi_mux_pads(int dsi_id,
>>>> unsigned lanes)
>>>>        return 0;
>>>>    }
>>>>
>>>> +#define CONTROL_PAD_BASE    0x4A002800
>>>> +#define CONTROL_DSIPHY        0x614
>>>> +
>>>
>>> I guess this is something we can move to our driver, and use sysconf to
>>> get the register from DT.
>>
>> I just copied the same method as used for OMAP4.
>>
>> I guess sysconf is an option. But I really dislike the idea of moving
>> omap control module code to a display driver... I'm not sure what other
>> options we have, though. Maybe an OMAP DSI specific pinctrl driver?
> 
> OMAP4 has CONTROL_DSIPHY for configuring both lane enable/disbale, and
> pull up/down, but OMAP5 has normal PAD_CONF registers for DSI lines(2
> pins per register) for configuring pull up/down, and CONTROL_DSIPHY for
> lane enable/disable.
> 
> We would have a very messed up pinctrl driver, but it would probably be
> better than doing all this stuff in the driver.

Actually, this patch is not good. I should've looked at the code more
carefully =).

This one does ioremap every time the function is called, which could be
done multiple times.

And I think omap4_ctrl_pad_readl() can be used to access the registers.
Like this (not tested):

#define OMAP5_CONTROL_DSIPHY		0x614

static int omap5_dsi_mux_pads(int dsi_id, unsigned lanes)
{
	u32 enable_mask, enable_shift, reg;

	if (dsi_id == 0) {
		enable_mask = OMAP4_DSI1_LANEENABLE_MASK;
		enable_shift = OMAP4_DSI1_LANEENABLE_SHIFT;
	} else if (dsi_id == 1) {
		enable_mask = OMAP4_DSI2_LANEENABLE_MASK;
		enable_shift = OMAP4_DSI2_LANEENABLE_SHIFT;
	} else {
		return -ENODEV;
	}

	reg = omap4_ctrl_pad_readl(OMAP5_CONTROL_DSIPHY);
	reg &= ~enable_mask;
	reg |= (lanes << enable_shift) & enable_mask;
	omap4_ctrl_pad_writel(reg, OMAP5_CONTROL_DSIPHY);

	return 0;
}

 Tomi
Tony Lindgren April 25, 2014, 3:31 p.m. UTC | #5
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140425 07:08]:
> On 25/04/14 15:58, Archit Taneja wrote:
> > On Friday 25 April 2014 04:48 PM, Tomi Valkeinen wrote:
> >> On 25/04/14 14:11, Archit Taneja wrote:
> >>> Hi,
> >>>
> >>> On Thursday 24 April 2014 03:47 PM, Tomi Valkeinen wrote:
> >>>> Add support to set OMAP5 DSI pin muxing.
> >>>>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >>>> Cc: Tony Lindgren <tony@atomide.com>
> >>>> ---
> >>>>    arch/arm/mach-omap2/display.c | 35
> >>>> ++++++++++++++++++++++++++++++++++-
> >>>>    1 file changed, 34 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/arm/mach-omap2/display.c
> >>>> b/arch/arm/mach-omap2/display.c
> >>>> index 16d33d831287..974461441fc3 100644
> >>>> --- a/arch/arm/mach-omap2/display.c
> >>>> +++ b/arch/arm/mach-omap2/display.c
> >>>> @@ -137,11 +137,42 @@ static int omap4_dsi_mux_pads(int dsi_id,
> >>>> unsigned lanes)
> >>>>        return 0;
> >>>>    }
> >>>>
> >>>> +#define CONTROL_PAD_BASE    0x4A002800
> >>>> +#define CONTROL_DSIPHY        0x614
> >>>> +
> >>>
> >>> I guess this is something we can move to our driver, and use sysconf to
> >>> get the register from DT.
> >>
> >> I just copied the same method as used for OMAP4.
> >>
> >> I guess sysconf is an option. But I really dislike the idea of moving
> >> omap control module code to a display driver... I'm not sure what other
> >> options we have, though. Maybe an OMAP DSI specific pinctrl driver?
> > 
> > OMAP4 has CONTROL_DSIPHY for configuring both lane enable/disbale, and
> > pull up/down, but OMAP5 has normal PAD_CONF registers for DSI lines(2
> > pins per register) for configuring pull up/down, and CONTROL_DSIPHY for
> > lane enable/disable.
> > 
> > We would have a very messed up pinctrl driver, but it would probably be
> > better than doing all this stuff in the driver.
> 
> Actually, this patch is not good. I should've looked at the code more
> carefully =).
> 
> This one does ioremap every time the function is called, which could be
> done multiple times.
> 
> And I think omap4_ctrl_pad_readl() can be used to access the registers.
> Like this (not tested):
> 
> #define OMAP5_CONTROL_DSIPHY		0x614
> 
> static int omap5_dsi_mux_pads(int dsi_id, unsigned lanes)
> {
> 	u32 enable_mask, enable_shift, reg;
> 
> 	if (dsi_id == 0) {
> 		enable_mask = OMAP4_DSI1_LANEENABLE_MASK;
> 		enable_shift = OMAP4_DSI1_LANEENABLE_SHIFT;
> 	} else if (dsi_id == 1) {
> 		enable_mask = OMAP4_DSI2_LANEENABLE_MASK;
> 		enable_shift = OMAP4_DSI2_LANEENABLE_SHIFT;
> 	} else {
> 		return -ENODEV;
> 	}
> 
> 	reg = omap4_ctrl_pad_readl(OMAP5_CONTROL_DSIPHY);
> 	reg &= ~enable_mask;
> 	reg |= (lanes << enable_shift) & enable_mask;
> 	omap4_ctrl_pad_writel(reg, OMAP5_CONTROL_DSIPHY);
> 
> 	return 0;
> }

Chances are any mux register in the syscon area already works with
pinctrl-single,pins or pinctrl-single,bits option. The ones in the
padconf area should be already mapped so the driver just has to
request them.

Regards,

Tony
Tomi Valkeinen April 28, 2014, 6:52 a.m. UTC | #6
On 25/04/14 18:31, Tony Lindgren wrote:

> Chances are any mux register in the syscon area already works with
> pinctrl-single,pins or pinctrl-single,bits option. The ones in the
> padconf area should be already mapped so the driver just has to
> request them.

If using the padconf (say omap4_padconf_global for omap4), doesn't that
mean we need to have platform specific bits in the driver? Isn't that
something we've been trying to remove all the time?

 Tomi
Tony Lindgren April 28, 2014, 4:45 p.m. UTC | #7
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140427 23:53]:
> On 25/04/14 18:31, Tony Lindgren wrote:
> 
> > Chances are any mux register in the syscon area already works with
> > pinctrl-single,pins or pinctrl-single,bits option. The ones in the
> > padconf area should be already mapped so the driver just has to
> > request them.
> 
> If using the padconf (say omap4_padconf_global for omap4), doesn't that
> mean we need to have platform specific bits in the driver? Isn't that
> something we've been trying to remove all the time?

No, it's all done in a Linux generic way during driver probe, see
drivers/base/pinctrl.c. You just need to define the default pins
in the .dts files. If you need dynamic remuxing in the driver,
you can define other named states that the driver can then toggle
with pinctrl_select_state().

Regards,

Tony
Tomi Valkeinen April 29, 2014, 5:26 a.m. UTC | #8
On 28/04/14 19:45, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [140427 23:53]:
>> On 25/04/14 18:31, Tony Lindgren wrote:
>>
>>> Chances are any mux register in the syscon area already works with
>>> pinctrl-single,pins or pinctrl-single,bits option. The ones in the
>>> padconf area should be already mapped so the driver just has to
>>> request them.
>>
>> If using the padconf (say omap4_padconf_global for omap4), doesn't that
>> mean we need to have platform specific bits in the driver? Isn't that
>> something we've been trying to remove all the time?
> 
> No, it's all done in a Linux generic way during driver probe, see
> drivers/base/pinctrl.c. You just need to define the default pins
> in the .dts files. If you need dynamic remuxing in the driver,
> you can define other named states that the driver can then toggle
> with pinctrl_select_state().

omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
a raw regmap to its memory area, the driver needs to know about the OMAP
control registers to use it.

Pinctrl-single cannot be used for CONTROL_DSIPHY register, as the
register contents are a bit funny and DSI1 and DSI2 bits are mixed
together. And CONTROL_DSIPHY is already in the memory region defined by
the omap4_padconf_global, so I guess it wouldn't be good to map parts of
the same memory region in a pinctrl node.

 Tomi
Tony Lindgren April 29, 2014, 3:05 p.m. UTC | #9
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140428 22:26]:
> On 28/04/14 19:45, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140427 23:53]:
> >> On 25/04/14 18:31, Tony Lindgren wrote:
> >>
> >>> Chances are any mux register in the syscon area already works with
> >>> pinctrl-single,pins or pinctrl-single,bits option. The ones in the
> >>> padconf area should be already mapped so the driver just has to
> >>> request them.
> >>
> >> If using the padconf (say omap4_padconf_global for omap4), doesn't that
> >> mean we need to have platform specific bits in the driver? Isn't that
> >> something we've been trying to remove all the time?
> > 
> > No, it's all done in a Linux generic way during driver probe, see
> > drivers/base/pinctrl.c. You just need to define the default pins
> > in the .dts files. If you need dynamic remuxing in the driver,
> > you can define other named states that the driver can then toggle
> > with pinctrl_select_state().
> 
> omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
> a raw regmap to its memory area, the driver needs to know about the OMAP
> control registers to use it.

That would be probably best set up the same way we have already set up
for example omap4_padconf_global: tisyscon@4a1005a0. Then drivers can
access it using regmap, see how drivers/regulator/pbias-regulator.c
sets up the pbias regulator with regmap for MMC.
 
> Pinctrl-single cannot be used for CONTROL_DSIPHY register, as the
> register contents are a bit funny and DSI1 and DSI2 bits are mixed
> together. And CONTROL_DSIPHY is already in the memory region defined by
> the omap4_padconf_global, so I guess it wouldn't be good to map parts of
> the same memory region in a pinctrl node.

If it's more than a mux, then it should not be set up as a pinctrl
register. Looks like CONTROL_DSIPHY is already available for drivers
via regmap as it falls into the *_padconf_global mappings for omap4
and omap5.

Regards,

Tony
Tomi Valkeinen April 29, 2014, 4:19 p.m. UTC | #10
On 29/04/14 18:05, Tony Lindgren wrote:

>> omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
>> a raw regmap to its memory area, the driver needs to know about the OMAP
>> control registers to use it.
> 
> That would be probably best set up the same way we have already set up
> for example omap4_padconf_global: tisyscon@4a1005a0. Then drivers can
> access it using regmap, see how drivers/regulator/pbias-regulator.c
> sets up the pbias regulator with regmap for MMC.

Right, but it means that the driver will contain platform specific code
for all the omap revisions it supports. Isn't that wrong?

I already have a patch for DSI that uses the syscon-method, and it works
fine. But it's quite ugly, imo, to fiddle with the OMAP control
registers in a driver.

 Tomi
Tomi Valkeinen April 29, 2014, 4:32 p.m. UTC | #11
On 29/04/14 19:19, Tomi Valkeinen wrote:
> On 29/04/14 18:05, Tony Lindgren wrote:
> 
>>> omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
>>> a raw regmap to its memory area, the driver needs to know about the OMAP
>>> control registers to use it.
>>
>> That would be probably best set up the same way we have already set up
>> for example omap4_padconf_global: tisyscon@4a1005a0. Then drivers can
>> access it using regmap, see how drivers/regulator/pbias-regulator.c
>> sets up the pbias regulator with regmap for MMC.
> 
> Right, but it means that the driver will contain platform specific code
> for all the omap revisions it supports. Isn't that wrong?
> 
> I already have a patch for DSI that uses the syscon-method, and it works
> fine. But it's quite ugly, imo, to fiddle with the OMAP control
> registers in a driver.

Oh, also, if I do that, I need to know both the SoC version and the DSS
version in the driver.

 Tomi
Tony Lindgren April 29, 2014, 5:38 p.m. UTC | #12
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140429 09:33]:
> On 29/04/14 19:19, Tomi Valkeinen wrote:
> > On 29/04/14 18:05, Tony Lindgren wrote:
> > 
> >>> omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
> >>> a raw regmap to its memory area, the driver needs to know about the OMAP
> >>> control registers to use it.
> >>
> >> That would be probably best set up the same way we have already set up
> >> for example omap4_padconf_global: tisyscon@4a1005a0. Then drivers can
> >> access it using regmap, see how drivers/regulator/pbias-regulator.c
> >> sets up the pbias regulator with regmap for MMC.
> > 
> > Right, but it means that the driver will contain platform specific code
> > for all the omap revisions it supports. Isn't that wrong?
> > 
> > I already have a patch for DSI that uses the syscon-method, and it works
> > fine. But it's quite ugly, imo, to fiddle with the OMAP control
> > registers in a driver.

Anything using the system control module registers should be a separate
driver. And it should ideally be implemeting some Linux generic framework
that the consumer driver can then use. That leaves out the need to export
custom functions.

I guess we don't have a PHY framework for displays though, so how about
just a separate minimal driver under drivers/video/omap2 that uses the
syscon?
 
> Oh, also, if I do that, I need to know both the SoC version and the DSS
> version in the driver.

Don't you get all you need in the compatible string? Something like
compatible ti,dss-phy-omap5?

Regards,

Tony
Tomi Valkeinen April 30, 2014, 6:13 a.m. UTC | #13
On 29/04/14 20:38, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [140429 09:33]:
>> On 29/04/14 19:19, Tomi Valkeinen wrote:
>>> On 29/04/14 18:05, Tony Lindgren wrote:
>>>
>>>>> omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
>>>>> a raw regmap to its memory area, the driver needs to know about the OMAP
>>>>> control registers to use it.
>>>>
>>>> That would be probably best set up the same way we have already set up
>>>> for example omap4_padconf_global: tisyscon@4a1005a0. Then drivers can
>>>> access it using regmap, see how drivers/regulator/pbias-regulator.c
>>>> sets up the pbias regulator with regmap for MMC.
>>>
>>> Right, but it means that the driver will contain platform specific code
>>> for all the omap revisions it supports. Isn't that wrong?
>>>
>>> I already have a patch for DSI that uses the syscon-method, and it works
>>> fine. But it's quite ugly, imo, to fiddle with the OMAP control
>>> registers in a driver.
> 
> Anything using the system control module registers should be a separate
> driver. And it should ideally be implemeting some Linux generic framework
> that the consumer driver can then use. That leaves out the need to export
> custom functions.

Ok.

> I guess we don't have a PHY framework for displays though, so how about
> just a separate minimal driver under drivers/video/omap2 that uses the
> syscon?

Well, this one is not really about DSI PHY. The CONTROL_DSIPHY register
is not in the DSI PHY block, but in the control module, and it is used
to enable/disable the pins (for omap4/5) and to set pull up/down for the
pins (only for omap4). Oddly, for omap5, there's also a normal padconfig
register for the DSI pins, but not so for omap4.

To me it looks like a pad config register. I don't know why there's a
separate odd register for it and it's not using the normal padconfig system.

I'd like to use the pinctrl framework for it, but the pinctrl-single
cannot handle such a funny register. So, if "Anything using the system
control module registers should be a separate driver", then I guess I
need to write a pinctrl driver for this single register?

>> Oh, also, if I do that, I need to know both the SoC version and the DSS
>> version in the driver.
> 
> Don't you get all you need in the compatible string? Something like
> compatible ti,dss-phy-omap5?

We do use different compatible strings for different major versions of
the DSS blocks, like ti,omap5-dsi. But that exactly same DSS block could
be used on some other SoC, with different control stuff.

We could use separate compatible string for each SoC that uses DSS, but
then we're really encoding the SoC version into the compatible string,
not DSS version.

Of course, if there will be a separate driver managing the
CONTROL_DSIPHY register, then that one should use compatible string
specific to the SoC, as it's not a DSS driver at all.

 Tomi
Tony Lindgren April 30, 2014, 5:56 p.m. UTC | #14
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140429 23:14]:
> On 29/04/14 20:38, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140429 09:33]:
> >> On 29/04/14 19:19, Tomi Valkeinen wrote:
> >>> On 29/04/14 18:05, Tony Lindgren wrote:
> >>>
> >>>>> omap4_padconf_global is a syscon node, not pinctrl. As syscon just gives
> >>>>> a raw regmap to its memory area, the driver needs to know about the OMAP
> >>>>> control registers to use it.
> >>>>
> >>>> That would be probably best set up the same way we have already set up
> >>>> for example omap4_padconf_global: tisyscon@4a1005a0. Then drivers can
> >>>> access it using regmap, see how drivers/regulator/pbias-regulator.c
> >>>> sets up the pbias regulator with regmap for MMC.
> >>>
> >>> Right, but it means that the driver will contain platform specific code
> >>> for all the omap revisions it supports. Isn't that wrong?
> >>>
> >>> I already have a patch for DSI that uses the syscon-method, and it works
> >>> fine. But it's quite ugly, imo, to fiddle with the OMAP control
> >>> registers in a driver.
> > 
> > Anything using the system control module registers should be a separate
> > driver. And it should ideally be implemeting some Linux generic framework
> > that the consumer driver can then use. That leaves out the need to export
> > custom functions.
> 
> Ok.
> 
> > I guess we don't have a PHY framework for displays though, so how about
> > just a separate minimal driver under drivers/video/omap2 that uses the
> > syscon?
> 
> Well, this one is not really about DSI PHY. The CONTROL_DSIPHY register
> is not in the DSI PHY block, but in the control module, and it is used
> to enable/disable the pins (for omap4/5) and to set pull up/down for the
> pins (only for omap4). Oddly, for omap5, there's also a normal padconfig
> register for the DSI pins, but not so for omap4.
> 
> To me it looks like a pad config register. I don't know why there's a
> separate odd register for it and it's not using the normal padconfig system.
> 
> I'd like to use the pinctrl framework for it, but the pinctrl-single
> cannot handle such a funny register. So, if "Anything using the system
> control module registers should be a separate driver", then I guess I
> need to write a pinctrl driver for this single register?

Have you checked out pinctrl-single,bits binding? That should allow
you to map random bits in a single register to a pinctrl driver
instance.
 
> >> Oh, also, if I do that, I need to know both the SoC version and the DSS
> >> version in the driver.
> > 
> > Don't you get all you need in the compatible string? Something like
> > compatible ti,dss-phy-omap5?
> 
> We do use different compatible strings for different major versions of
> the DSS blocks, like ti,omap5-dsi. But that exactly same DSS block could
> be used on some other SoC, with different control stuff.
> 
> We could use separate compatible string for each SoC that uses DSS, but
> then we're really encoding the SoC version into the compatible string,
> not DSS version.
> 
> Of course, if there will be a separate driver managing the
> CONTROL_DSIPHY register, then that one should use compatible string
> specific to the SoC, as it's not a DSS driver at all.

Yeah probably using pinctrl-single,bits, or a separate driver with syscon
makes most sense here.

Regards,

Tony
Tomi Valkeinen May 2, 2014, 1:06 p.m. UTC | #15
On 30/04/14 20:56, Tony Lindgren wrote:

> Have you checked out pinctrl-single,bits binding? That should allow
> you to map random bits in a single register to a pinctrl driver
> instance.

If I recall right, the problem there was that we have one register,
which has bits for two separate devices, and the bits are mixed up. i.e.
not 16 upper bits for one, 16 lower for the other, but 5 bits for one
device, the next 5 bits for the other, then again few bits for the
first, etc.

pinctrl-single,bits didn't like it that.

>>>> Oh, also, if I do that, I need to know both the SoC version and the DSS
>>>> version in the driver.
>>>
>>> Don't you get all you need in the compatible string? Something like
>>> compatible ti,dss-phy-omap5?
>>
>> We do use different compatible strings for different major versions of
>> the DSS blocks, like ti,omap5-dsi. But that exactly same DSS block could
>> be used on some other SoC, with different control stuff.
>>
>> We could use separate compatible string for each SoC that uses DSS, but
>> then we're really encoding the SoC version into the compatible string,
>> not DSS version.
>>
>> Of course, if there will be a separate driver managing the
>> CONTROL_DSIPHY register, then that one should use compatible string
>> specific to the SoC, as it's not a DSS driver at all.
> 
> Yeah probably using pinctrl-single,bits, or a separate driver with syscon
> makes most sense here.

Yep, I'll have to come up with something.

 Tomi
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 16d33d831287..974461441fc3 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -137,11 +137,42 @@  static int omap4_dsi_mux_pads(int dsi_id, unsigned lanes)
 	return 0;
 }
 
+#define CONTROL_PAD_BASE	0x4A002800
+#define CONTROL_DSIPHY		0x614
+
+static int omap5_dsi_mux_pads(int dsi_id, unsigned lanes)
+{
+	u32 enable_mask, enable_shift, reg;
+	void __iomem *ctrl_pad_base = NULL;
+
+	ctrl_pad_base = ioremap(CONTROL_PAD_BASE, SZ_4K);
+	if (!ctrl_pad_base)
+		return -ENXIO;
+
+	if (dsi_id == 0) {
+		enable_mask = OMAP4_DSI1_LANEENABLE_MASK;
+		enable_shift = OMAP4_DSI1_LANEENABLE_SHIFT;
+	} else if (dsi_id == 1) {
+		enable_mask = OMAP4_DSI2_LANEENABLE_MASK;
+		enable_shift = OMAP4_DSI2_LANEENABLE_SHIFT;
+	} else {
+		return -ENODEV;
+	}
+
+	reg = __raw_readl(ctrl_pad_base + CONTROL_DSIPHY);
+	reg &= ~enable_mask;
+	reg |= (lanes << enable_shift) & enable_mask;
+	__raw_writel(reg, ctrl_pad_base + CONTROL_DSIPHY);
+
+	return 0;
+}
+
 static int omap_dsi_enable_pads(int dsi_id, unsigned lane_mask)
 {
 	if (cpu_is_omap44xx())
 		return omap4_dsi_mux_pads(dsi_id, lane_mask);
-
+	else if (soc_is_omap54xx())
+		return omap5_dsi_mux_pads(dsi_id, lane_mask);
 	return 0;
 }
 
@@ -149,6 +180,8 @@  static void omap_dsi_disable_pads(int dsi_id, unsigned lane_mask)
 {
 	if (cpu_is_omap44xx())
 		omap4_dsi_mux_pads(dsi_id, 0);
+	else if (soc_is_omap54xx())
+		omap5_dsi_mux_pads(dsi_id, 0);
 }
 
 static int omap_dss_set_min_bus_tput(struct device *dev, unsigned long tput)