* [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure
From: Tejun Heo @ 2014-01-19 19:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DC28DB.7070804@redhat.com>
On Sun, Jan 19, 2014 at 08:34:51PM +0100, Hans de Goede wrote:
> Well the current error handling still re-disables the clks on resume failure,
> if you want to proceed with resume as far as possible, rather then return to
Let's put it as "put it in a state where it can be reinitialized
afterwards".
Thanks.
--
tejun
^ permalink raw reply
* [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks
From: Hans de Goede @ 2014-01-19 19:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119193258.GT15937@n2100.arm.linux.org.uk>
Hi,
On 01/19/2014 08:32 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 08:30:37PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote:
>>> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
>>>> +enum {
>>>> + CLK_SATA,
>>>> + CLK_SATA_REF,
>>>> + CLK_AHB
>>>> +};
>>>
>>> Err, so we now rely on the order that these clocks are specified in DT
>>> rather than their name properties to provide the correct clock... that
>>> sounds particularly fragile to me.
>>
>> Both in the ohci- / ehci-platform case, where the idea of purely addressing
>> clocks by index comes from, as well as in the ahci-platform case, people
>> have been asking me to make things more generic, so as to avoid having
>> a gazillion almost but not quite the same ehci-foo platform drivers.
>>
>> This has already happened with ohci-foo.c drivers, and the hope is that
>> with the new generalized ohci-plaform.c many of the existing ohci-foo
>> drivers can go away over time.
>>
>> The downside of this generalized approach is that we cannot use clock-names
>> since those tend to be implementation specific.
>>
>> In the specific case of the ahci-imx driver this means that certain clocks
>> must be at a specific index, since it needs to know which one is the AHB
>> clock, as documented in the bindings documentation. I don't see how mandating
>> certain indices is different and/or more fragile then mandating certain names
>> in the bindings document. I agree that is is slightly less clear to someone
>> reading the dts, but that is the price we have to pay for this desire for
>> things to be generic.
>
> So what happens if we have the same IP block appearing, but the SATA_REF
> or SATA clock isn't present - how do we still provide an AHB clock (for
> argument sake)?
Then it will not use the same compatible string, since then clearly it is not
compatible with "fsl,imx6q-ahci"
And we can document different indices for the new compatible string (and adjust
the code to match).
Regards,
Hans
^ permalink raw reply
* [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure
From: Hans de Goede @ 2014-01-19 19:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119191349.GA32165@mtj.dyndns.org>
Hi,
On 01/19/2014 08:13 PM, Tejun Heo wrote:
> Hello,
>
> On Sun, Jan 19, 2014 at 07:40:21PM +0100, Hans de Goede wrote:
>> That sounds like your advocating for just returning from resume on the
>> first error without undoing any of the previous steps, have I gotten that
>> right?
>
> Yeah, or just ignore reset failure and proceed.
That seems like a bad idea IMHO, if reset failed something is seriously
amiss and just continuing as nothing happened seems unhelpful.
>> That sounds as sensible as any other approach on resume errors
>> (there are IMHO no good answers), if that is what you mean, shall I do a
>
> Suspending back is the wrong answer tho.
>
>> patch in the next versions of my patch-set doing that ?
>
> Isn't just dropping this patch enough tho?
Well the current error handling still re-disables the clks on resume failure,
if you want to proceed with resume as far as possible, rather then return to
a suspended state it seems sensible to just leave the clocks on as well.
Also disabling the clocks on resume failure, followed by a rmmod will cause
a WARN_ON to trigger in the clock-framework when ahci_host_stop tries to
disable the clks for a second time.
Regards,
Hans
^ permalink raw reply
* [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks
From: Russell King - ARM Linux @ 2014-01-19 19:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DC27DD.5030309@redhat.com>
On Sun, Jan 19, 2014 at 08:30:37PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote:
>> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
>>> +enum {
>>> + CLK_SATA,
>>> + CLK_SATA_REF,
>>> + CLK_AHB
>>> +};
>>
>> Err, so we now rely on the order that these clocks are specified in DT
>> rather than their name properties to provide the correct clock... that
>> sounds particularly fragile to me.
>
> Both in the ohci- / ehci-platform case, where the idea of purely addressing
> clocks by index comes from, as well as in the ahci-platform case, people
> have been asking me to make things more generic, so as to avoid having
> a gazillion almost but not quite the same ehci-foo platform drivers.
>
> This has already happened with ohci-foo.c drivers, and the hope is that
> with the new generalized ohci-plaform.c many of the existing ohci-foo
> drivers can go away over time.
>
> The downside of this generalized approach is that we cannot use clock-names
> since those tend to be implementation specific.
>
> In the specific case of the ahci-imx driver this means that certain clocks
> must be at a specific index, since it needs to know which one is the AHB
> clock, as documented in the bindings documentation. I don't see how mandating
> certain indices is different and/or more fragile then mandating certain names
> in the bindings document. I agree that is is slightly less clear to someone
> reading the dts, but that is the price we have to pay for this desire for
> things to be generic.
So what happens if we have the same IP block appearing, but the SATA_REF
or SATA clock isn't present - how do we still provide an AHB clock (for
argument sake)?
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply
* [RFC v3 10/13] ahci_imx: Adjust for ahci_platform managing the clocks
From: Hans de Goede @ 2014-01-19 19:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119124134.GQ15937@n2100.arm.linux.org.uk>
Hi,
On 01/19/2014 01:41 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:52AM +0100, Hans de Goede wrote:
>> +enum {
>> + CLK_SATA,
>> + CLK_SATA_REF,
>> + CLK_AHB
>> +};
>
> Err, so we now rely on the order that these clocks are specified in DT
> rather than their name properties to provide the correct clock... that
> sounds particularly fragile to me.
Both in the ohci- / ehci-platform case, where the idea of purely addressing
clocks by index comes from, as well as in the ahci-platform case, people
have been asking me to make things more generic, so as to avoid having
a gazillion almost but not quite the same ehci-foo platform drivers.
This has already happened with ohci-foo.c drivers, and the hope is that
with the new generalized ohci-plaform.c many of the existing ohci-foo
drivers can go away over time.
The downside of this generalized approach is that we cannot use clock-names
since those tend to be implementation specific.
In the specific case of the ahci-imx driver this means that certain clocks
must be at a specific index, since it needs to know which one is the AHB
clock, as documented in the bindings documentation. I don't see how mandating
certain indices is different and/or more fragile then mandating certain names
in the bindings document. I agree that is is slightly less clear to someone
reading the dts, but that is the price we have to pay for this desire for
things to be generic.
Regards,
Hans
^ permalink raw reply
* PWM...
From: Russell King - ARM Linux @ 2014-01-19 19:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMi7g=PHjzhkuObkeu+P_c6kUz5uFcND7euafUW56CvsnQ@mail.gmail.com>
On Sun, Jan 19, 2014 at 11:11:41AM -0800, Olof Johansson wrote:
> On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sunday 19 January 2014 11:03:24 Olof Johansson wrote:
> >> >
> >> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> >> > be specified in the pwms declaration in pwmleds. So that doesn't work.
> >> > There's no property to tell pwmleds that it should use inverted sense
> >> > either.
> >>
> >> Adding a property for active-low to the pwm-leds binding would be
> >> easy, and backwards compatible. I'm surprised the original binding
> >> didn't specify it. The leds-pwm driver already seems to support it for
> >> C-configured instances.
> >>
> >> I'm also surprised that the imx pwm driver even has a #pwm-cells of
> >> two, since the driver only supports one output. It'd be nice if they
> >> had allocated the extra cell for flags, but it's hard to change now,
> >> unless you do a new binding/compatible value and deprecate the old one.
> >
> > Actually I think it's not that hard to change: The binding can specify
> > that either #pwm-cells=<2> or #pwm-cells=<3> is supported, and the
> > driver extended to handle both cases. This would maintain backwards
> > compatibility for old dtb files, though no forward compatibility for
> > new dtb files with old kernels.
>
> Ah, yes, if you add a cell that can be done. There'll still be the
> "dead" first cell that will always be 0, but that's alright.
Does it not mean that PWM specifications of:
<&pwm1 0 n> <&pwm2 0 n>
would need to be converted to:
<&pwm1 0 n 0> <&pwm2 0 n 0>
in every DT file referring to these PWMs - because isn't this just
treated in DT as one single array of values? (If DT knew how many
were in each specification, we wouldn't need the #foo-cells...)
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply
* How to support SDIO wifi/bt in DT
From: Olof Johansson @ 2014-01-19 19:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <alpine.LFD.2.10.1401171157340.28907@knanqh.ubzr>
On Fri, Jan 17, 2014 at 8:58 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Fri, 17 Jan 2014, Rob Herring wrote:
>
>> On Thu, Jan 16, 2014 at 9:08 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
>> > On Thu, 16 Jan 2014, Olof Johansson wrote:
>> >
>> >> "for SDIO slots" is somewhat misleading; nearly all controllers only
>> >> do one slot/device per controller. The designware controller can do
>> >> multiple slots, and that adds a bit of driver and binding complexity
>> >> for something that seemingly not a single vendor has actually
>> >> implemented.
>> >
>> > The Marvell 8688, just to name one, does both WIFI and BlueTooth over
>> > the same SDIO controller using separate functions/slots.
>>
>> But that is logical functions within a chip, not multiple discrete
>> chips (i.e. slots) with their own reset, regulators, etc.
>
> OK. If that's the definition of a slot in this context then I agree.
Correct -- the device that I've been doing the work in is actually the
combined wifi/bluetooth module on the Samsung Chromebook, which is a
Marvell 8797. It has multiple functions which still work well.
The "multi-slot" concept I was talking about was one host controller
for several physical slots. Seems like at91 implements _and_ uses this
but nobody else. However, it seems that they allocate one mmc host per
slot, so the same model still works.
-Olof
^ permalink raw reply
* [RFC v3 08/13] ahci-platform: Allow specifying platform_data through of_device_id
From: Tejun Heo @ 2014-01-19 19:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DC1FF5.80101@redhat.com>
On Sun, Jan 19, 2014 at 07:56:53PM +0100, Hans de Goede wrote:
> 1) of_match_device returns const, so without the const the code would need
> to cast that const away somewhere
> 2) of_match_device is right to return const because
> 2a) the data can actually be const
> 2b) even if not const, it is shared between multiple instances of the same
> device-type and thus should never be written too
Sure, if it has already gone that way, keeping it on is probably the
easiest way.
> So as Russell already said, the use of const is correct here, and the best
> thing to do is to simply keep it.
It's not about whether this specific annotation is correct or not.
It's that C as a language doesn't have good enough const support to
have proper const annotations and tends to lead to practical problems
often making the trade-off unclear in complex cases.
Anyways, as written above, if it's propagation of existing ones and
doesn't have mixed ro/rw usages, keeping it on probably is the eaiest.
Thanks.
--
tejun
^ permalink raw reply
* [RFC v3 06/13] ahci-platform: Add support for devices with more then 1 clock
From: Hans de Goede @ 2014-01-19 19:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119123854.GP15937@n2100.arm.linux.org.uk>
Hi,
On 01/19/2014 01:38 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:48AM +0100, Hans de Goede wrote:
>> +static int ahci_enable_clks(struct device *dev, struct ahci_host_priv *hpriv)
>> +{
>> + int c, rc;
>> +
>> + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
>
> for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++) {
That won't work, hpriv->clks == NULL for clks entries which are not used,
and before we get into a discussion about leaving any PTR_ERR returns
from clk_get in-place. I've had similar discussions when doing similar
changes to ohci-platform.c and ehci-platform.c and there the conclusion
was that "if (clk)" is just much more nice to read then "if (!IS_ERR(clk))",
I would like to avoid having the same discussion again.
More-over all clk_foo() methods check for and will safely handle clk == NULL,
and will crash and burn with clk == IS_ERR(clk).
I've chosen to still explicitly check for clk == NULL as that makes it much more
clear when reading the code that clk maybe NULL.
>> + rc = clk_prepare_enable(hpriv->clks[c]);
>> + if (rc) {
>> + dev_err(dev, "clock prepare enable failed");
>> + goto disable_unprepare_clk;
>> + }
>> + }
>> + return 0;
>> +
>> +disable_unprepare_clk:
>> + while (--c >= 0)
>> + clk_disable_unprepare(hpriv->clks[c]);
>> + return rc;
>> +}
>> +
>> +static void ahci_disable_clks(struct ahci_host_priv *hpriv)
>> +{
>> + int c;
>> +
>> + for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
>> + if (hpriv->clks[c])
>
> if (!IS_ERR(hpriv->clks[c]))
>
Idem.
>> + clk_disable_unprepare(hpriv->clks[c]);
>> +}
>> +
>> +static void ahci_put_clks(struct ahci_host_priv *hpriv)
>> +{
>> + int c;
>> +
>> + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
>
> for (c = 0; c < AHCI_MAX_CLKS && !IS_ERR(hpriv->clks[c]); c++)
>
Idem.
>> + clk_put(hpriv->clks[c]);
>> +}
>
> Better still for this one, consider using devm_clk_get() - in which case
> the above is even more important to get right.
The above depends on how errors are handled when calling clk_get (or variants),
which in the case of this patch is such that hpriv->clks[i] == NULL when not
present.
> We really should have a devm_of_clk_get() too.
Agreed, but that seems something for another patch-set, this one is big
enough as is.
Regards,
Hans
^ permalink raw reply
* [RFC v3 05/13] ahci-platform: Pass ahci_host_priv ptr to ahci_platform_data init method
From: Tejun Heo @ 2014-01-19 19:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DC1EC4.3090807@redhat.com>
On Sun, Jan 19, 2014 at 07:51:48PM +0100, Hans de Goede wrote:
> Yes, but they are part of arch/arm/mach-foo, so moving them is non trivial, and
> I don't want to go make these kinda changes without hardware to test. Please keep
> in mind that I'm partially cleaning up other peoples mess here (the imx bits
> specifically). I'm willing to do that to some extend (*). But buying an imx6q board
> and fixing that is about as far as I'm willing to go.
If they can't be moved, the right thing to do is moving the header out
to include/linux so that they can be included from those directories.
Let's please not contort the API to fit the organizational issues.
Thanks.
--
tejun
^ permalink raw reply
* [RFC v3 03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume
From: Tejun Heo @ 2014-01-19 19:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DC1DAA.3010403@redhat.com>
Hey,
On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote:
> As I see it either doing clks, regulator and sata-core things in a common
> place makes sense, and then it goes for suspend and resume too, or we
> opt for always following the complete override model, and which point
> it becomes more sensible to just do a separate platform driver per
> ahci implementation.
It makes sense in light of those specific cases, but there are gonna
be cases where the placement of the callback is slightly wrong and we
end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on.
Please make the whole op overridable and then export the default op
and use it as library.
Thanks.
--
tejun
^ permalink raw reply
* [RFC v3 04/13] ahci-platform: Undo pdata->resume on resume failure
From: Tejun Heo @ 2014-01-19 19:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <52DC1C15.1030107@redhat.com>
Hello,
On Sun, Jan 19, 2014 at 07:40:21PM +0100, Hans de Goede wrote:
> That sounds like your advocating for just returning from resume on the
> first error without undoing any of the previous steps, have I gotten that
> right?
Yeah, or just ignore reset failure and proceed.
> That sounds as sensible as any other approach on resume errors
> (there are IMHO no good answers), if that is what you mean, shall I do a
Suspending back is the wrong answer tho.
> patch in the next versions of my patch-set doing that ?
Isn't just dropping this patch enough tho?
Thanks.
--
tejun
^ permalink raw reply
* PWM...
From: Olof Johansson @ 2014-01-19 19:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <59642204.3UzJCDJHaS@wuerfel>
On Sun, Jan 19, 2014 at 11:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 19 January 2014 11:03:24 Olof Johansson wrote:
>> >
>> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
>> > be specified in the pwms declaration in pwmleds. So that doesn't work.
>> > There's no property to tell pwmleds that it should use inverted sense
>> > either.
>>
>> Adding a property for active-low to the pwm-leds binding would be
>> easy, and backwards compatible. I'm surprised the original binding
>> didn't specify it. The leds-pwm driver already seems to support it for
>> C-configured instances.
>>
>> I'm also surprised that the imx pwm driver even has a #pwm-cells of
>> two, since the driver only supports one output. It'd be nice if they
>> had allocated the extra cell for flags, but it's hard to change now,
>> unless you do a new binding/compatible value and deprecate the old one.
>
> Actually I think it's not that hard to change: The binding can specify
> that either #pwm-cells=<2> or #pwm-cells=<3> is supported, and the
> driver extended to handle both cases. This would maintain backwards
> compatibility for old dtb files, though no forward compatibility for
> new dtb files with old kernels.
Ah, yes, if you add a cell that can be done. There'll still be the
"dead" first cell that will always be 0, but that's alright.
-Olof
^ permalink raw reply
* [RFC v3 11/13] ahci-imx: Don't create a nested platform device from probe
From: Hans de Goede @ 2014-01-19 19:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119122540.GN15937@n2100.arm.linux.org.uk>
Hi,
On 01/19/2014 01:25 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:53AM +0100, Hans de Goede wrote:
>> Instead only provide platform_data through of_device_id, like the ahci-sunxi
>> driver does.
>
> It looks like the whole of ahci_imx.c (apart from the inclues) is
> enclosed in a big ifdef. You can avoid that by using this in the
> Makefile:
>
>> @@ -10,9 +10,8 @@ obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o
>> obj-$(CONFIG_SATA_SIL24) += sata_sil24.o
>> obj-$(CONFIG_SATA_DWC) += sata_dwc_460ex.o
>> obj-$(CONFIG_SATA_HIGHBANK) += sata_highbank.o libahci.o
>> -obj-$(CONFIG_AHCI_IMX) += ahci_imx.o
>>
>> -ahci_plat-objs := ahci_platform.o ahci_sunxi.o
>> +ahci_plat-objs := ahci_platform.o ahci_imx.o ahci_sunxi.o
>
> ahci_plat-y := ahci_platform.o ahci_sunxi.o
> ahci_plat-$(CONFIG_AHCI_IMX) += ahci_imx.o
>
Ah, good idea, the same goes for the sunxi bits. I'll fix this in the next
revision of the patch-set.
Regards,
Hans
^ permalink raw reply
* PWM...
From: Arnd Bergmann @ 2014-01-19 19:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119190324.GC28056@quad.lixom.net>
On Sunday 19 January 2014 11:03:24 Olof Johansson wrote:
> >
> > Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> > be specified in the pwms declaration in pwmleds. So that doesn't work.
> > There's no property to tell pwmleds that it should use inverted sense
> > either.
>
> Adding a property for active-low to the pwm-leds binding would be
> easy, and backwards compatible. I'm surprised the original binding
> didn't specify it. The leds-pwm driver already seems to support it for
> C-configured instances.
>
> I'm also surprised that the imx pwm driver even has a #pwm-cells of
> two, since the driver only supports one output. It'd be nice if they
> had allocated the extra cell for flags, but it's hard to change now,
> unless you do a new binding/compatible value and deprecate the old one.
Actually I think it's not that hard to change: The binding can specify
that either #pwm-cells=<2> or #pwm-cells=<3> is supported, and the
driver extended to handle both cases. This would maintain backwards
compatibility for old dtb files, though no forward compatibility for
new dtb files with old kernels.
Arnd
^ permalink raw reply
* [RFC v3 09/13] ARM: sunxi: Add support for Allwinner SUNXi SoCs sata to ahci_platform
From: Hans de Goede @ 2014-01-19 19:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119122216.GM15937@n2100.arm.linux.org.uk>
Hi,
On 01/19/2014 01:22 PM, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 12:48:51AM +0100, Hans de Goede wrote:
>> + timeout = 0x100000;
>> + do {
>> + reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28);
>> + } while (--timeout && (reg_val != 0x2));
>> + if (!timeout) {
>> + dev_err(dev, "PHY power up failed.\n");
>> + return -EIO;
>> + }
>
> This is not a good way to detect failure - there's several things wrong
> here.
>
> First, how long does sunxi_getbits() take? What does that depend on?
> Therefore, how long does it take to time out?
You're interpreting the timeout in the above code as an actual timeout, but
that is not what it is, it is a means to avoid looping forever if something
is seriously amiss. The only time I've ever seen the timeout trigger is when
I forgot to enable some clks iirc.
I can rename the variable from timeout to max_tries to make this more clear.
> Secondly, what if the success condition becomes true at the same time that
> a timeout occurs?
We should never get anywhere near timeout becoming 0, so if both happen at
the same time, then something is pretty seriously broken and the returning of
an error as the code does now is the right thing to do.
Regards,
Hans
^ permalink raw reply
* PWM...
From: Olof Johansson @ 2014-01-19 19:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140119164956.GR15937@n2100.arm.linux.org.uk>
[Adding devicetree list since we're talking bindings]
On Sun, Jan 19, 2014 at 04:49:57PM +0000, Russell King - ARM Linux wrote:
> So, having looked at what else I can add support for on the cubox-i, I
> decided it would be nice and simple to add support for the front panel
> LED. What could possibly go wrong with this.
>
> Well, the hardware is wired such that the LED is connected between the
> PWM output and +3.3v. So, a constant low turns the LED on full, whereas
> a constant high turns the LED off.
>
> So, the polarity of the LED is inverted - but this _can't_ be specified
> in the totally and utterly fucked misdesigned crap that DT is:
>
> pwmleds {
> compatible = "pwm-leds";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_cubox_i_pwm1>;
>
> front {
> label = "imx6:red:front";
> pwms = <&pwm1 0 50000>;
> max-brightness = <248>;
> };
> };
>
> pwm1: pwm at 02080000 {
> #pwm-cells = <2>;
> compatible = "fsl,imx6q-pwm", "fsl,imx27-pwm";
> reg = <0x02080000 0x4000>;
> interrupts = <0 83 0x04>;
> clocks = <&clks 62>, <&clks 145>;
> clock-names = "ipg", "per";
> };
>
> Yes, because iMX6 specifies #pwm-cells as 2, there's no flags able to
> be specified in the pwms declaration in pwmleds. So that doesn't work.
> There's no property to tell pwmleds that it should use inverted sense
> either.
Adding a property for active-low to the pwm-leds binding would be
easy, and backwards compatible. I'm surprised the original binding
didn't specify it. The leds-pwm driver already seems to support it for
C-configured instances.
I'm also surprised that the imx pwm driver even has a #pwm-cells of
two, since the driver only supports one output. It'd be nice if they
had allocated the extra cell for flags, but it's hard to change now,
unless you do a new binding/compatible value and deprecate the old one.
> Moreover, there's no way to specify a default brightness for the LED
> in the absence of any trigger, so this results in the LED being fully
> on.
That seems to be missing from the led-pwm driver altogether, not just the DT
bindings? Shouldn't be too bad to add a default-brightness property and plumb
that up through the driver in this case.
> So, something which _should_ be nice and simple is turned into a major
> fuckup because of the total and utter crappiness that DT is and the
> total misdesign that this shite is.
It's not a major fuckup. None of the above is unfixable.
-Olof
^ permalink raw reply
* [PATCH v3 24/24] drm/i2c: tda998x: adjust the audio clock divider for S/PDIF
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
The S/PDIF input asks for a greater audio clock divider.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 0716a75..06c484d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -707,6 +707,10 @@ tda998x_configure_audio(struct tda998x_priv *priv,
reg_write(priv, REG_CTS_N, cts_n);
reg_write(priv, REG_ENA_ACLK, aclk);
+ /* S/PDIF asks for a large divider */
+ if (priv->audio_format == AFMT_SPDIF)
+ adiv = AUDIO_DIV_SERCLK_32;
+
/*
* Audio input somehow depends on HDMI line rate which is
* related to pixclk. Testing showed that modes with pixclk
@@ -714,7 +718,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
* There is no detailed info in the datasheet, so we just
* assume 100MHz requires larger divider.
*/
- if (mode->clock > 100000)
+ else if (mode->clock > 100000)
adiv = AUDIO_DIV_SERCLK_16;
else
adiv = AUDIO_DIV_SERCLK_8;
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 23/24] drm/i2c: tda998x: code optimization
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
This patch reduces the number of I2C exchanges by setting many bits in
one write and removing a useless write.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2fcc432..0716a75 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -702,10 +702,8 @@ tda998x_configure_audio(struct tda998x_priv *priv,
}
reg_write(priv, REG_AIP_CLKSEL, clksel_aip);
- reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
-
- /* Enable automatic CTS generation */
- reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
+ reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT |
+ AIP_CNTRL_0_ACR_MAN); /* auto CTS */
reg_write(priv, REG_CTS_N, cts_n);
reg_write(priv, REG_ENA_ACLK, aclk);
@@ -953,10 +951,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
reg_write(priv, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0));
reg_write(priv, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) |
VIP_CNTRL_4_BLC(0));
- reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR);
reg_clear(priv, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ);
- reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE);
+ reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR |
+ PLL_SERIAL_3_SRL_DE);
reg_write(priv, REG_SERIALIZER, 0);
reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
@@ -976,8 +974,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
/* set BIAS tmds value: */
reg_write(priv, REG_ANA_GENERAL, 0x09);
- reg_write(priv, REG_TBG_CNTRL_0, 0);
-
/*
* Sync on rising HSYNC/VSYNC
*/
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 22/24] drm/i2c: tda998x: change the frequence in the audio channel
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
This patch sets the frequence as 'not indicated' instead of '48kHz'
and adds some comments.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 05713f7..2fcc432 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -745,10 +745,11 @@ tda998x_configure_audio(struct tda998x_priv *priv,
reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
/* Write the channel status */
- buf[0] = 0x04;
+ buf[0] = (1 << 2); /* copyright unprotected */
buf[1] = 0x00;
- buf[2] = 0x00;
- buf[3] = 0xf1;
+ buf[2] = 1; /* freq not indicated */
+ buf[3] = (0 << 4) | /* orig freq = not indicated */
+ 1; /* max word length 24 bits */
reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
tda998x_audio_mute(priv, true);
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 21/24] drm/i2c: tda998x: add the active aspect in HDMI AVI frame
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a2ad2ac..05713f7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -649,6 +649,7 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
buf[HB(1)] = 0x02;
buf[HB(2)] = HDMI_AVI_INFOFRAME_SIZE;
buf[PB(1)] = HDMI_SCAN_MODE_UNDERSCAN;
+ buf[PB(2)] = HDMI_ACTIVE_ASPECT_PICTURE;
buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2;
buf[PB(4)] = drm_match_cea_mode(mode);
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 20/24] drm/i2c: tda998x: remove the unused variable ca_i2s
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 324dc13..a2ad2ac 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -671,7 +671,7 @@ static void
tda998x_configure_audio(struct tda998x_priv *priv,
struct drm_display_mode *mode)
{
- uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
+ uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv;
uint32_t aclk, n;
/* Enable audio ports */
@@ -685,7 +685,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
cts_n = CTS_N_M(3) | CTS_N_K(3);
aclk = 0; /* no clock */
- ca_i2s = 0;
break;
case AFMT_I2S:
@@ -694,7 +693,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
clksel_fs = AIP_CLKSEL_FS_ACLK;
cts_n = CTS_N_M(3) | CTS_N_K(3);
aclk = 1; /* clock enable */
- ca_i2s = CA_I2S_CA_I2S(0);
break;
default:
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 18/24] drm/i2c: tda998x: fix the ENABLE_SPACE register
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
This patch fixes the ENABLE_SPACE register, the value of which was
inverted.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ea7d1b4..ce832f0 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -220,7 +220,7 @@ struct tda998x_priv {
# define PLL_SERIAL_1_SRL_IZ(x) (((x) & 3) << 1)
# define PLL_SERIAL_1_SRL_MAN_IZ (1 << 6)
#define REG_PLL_SERIAL_2 REG(0x02, 0x01) /* read/write */
-# define PLL_SERIAL_2_SRL_NOSC(x) (((x) & 3) << 0)
+# define PLL_SERIAL_2_SRL_NOSC(x) ((x) << 0)
# define PLL_SERIAL_2_SRL_PR(x) (((x) & 0xf) << 4)
#define REG_PLL_SERIAL_3 REG(0x02, 0x02) /* read/write */
# define PLL_SERIAL_3_SRL_CCIR (1 << 0)
@@ -932,6 +932,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
}
div = 148500 / adj_mode->clock;
+ if (div != 0) {
+ div--;
+ if (div > 3)
+ div = 3;
+ }
/* mute the audio FIFO: */
reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -1011,7 +1016,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
if (priv->rev == TDA19988) {
/* let incoming pixels fill the active space (if any) */
- reg_write(priv, REG_ENABLE_SPACE, 0x01);
+ reg_write(priv, REG_ENABLE_SPACE, 0x00);
}
/*
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 19/24] drm/i2c: tda998x: use global constants
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ce832f0..324dc13 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -132,6 +132,8 @@ struct tda998x_priv {
# define VIP_CNTRL_5_CKCASE (1 << 0)
# define VIP_CNTRL_5_SP_CNT(x) (((x) & 3) << 1)
#define REG_MUX_AP REG(0x00, 0x26) /* read/write */
+# define MUX_AP_SELECT_I2S 0x64
+# define MUX_AP_SELECT_SPDIF 0x40
#define REG_MUX_VP_VIP_OUT REG(0x00, 0x27) /* read/write */
#define REG_MAT_CONTRL REG(0x00, 0x80) /* write */
# define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0)
@@ -209,10 +211,11 @@ struct tda998x_priv {
#define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */
# define I2S_FORMAT(x) (((x) & 3) << 0)
#define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */
-# define AIP_CLKSEL_FS(x) (((x) & 3) << 0)
-# define AIP_CLKSEL_CLK_POL(x) (((x) & 1) << 2)
-# define AIP_CLKSEL_AIP(x) (((x) & 7) << 3)
-
+# define AIP_CLKSEL_AIP_SPDIF (0 << 3)
+# define AIP_CLKSEL_AIP_I2S (1 << 3)
+# define AIP_CLKSEL_FS_ACLK (0 << 0)
+# define AIP_CLKSEL_FS_MCLK (1 << 0)
+# define AIP_CLKSEL_FS_FS64SPDIF (2 << 0)
/* Page 02h: PLL settings */
#define REG_PLL_SERIAL_1 REG(0x02, 0x00) /* read/write */
@@ -677,20 +680,18 @@ tda998x_configure_audio(struct tda998x_priv *priv,
/* Set audio input source */
switch (priv->audio_format) {
case AFMT_SPDIF:
- reg_write(priv, REG_MUX_AP, 0x40);
- clksel_aip = AIP_CLKSEL_AIP(0);
- /* FS64SPDIF */
- clksel_fs = AIP_CLKSEL_FS(2);
+ reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
+ clksel_aip = AIP_CLKSEL_AIP_SPDIF;
+ clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
cts_n = CTS_N_M(3) | CTS_N_K(3);
aclk = 0; /* no clock */
ca_i2s = 0;
break;
case AFMT_I2S:
- reg_write(priv, REG_MUX_AP, 0x64);
- clksel_aip = AIP_CLKSEL_AIP(1);
- /* ACLK */
- clksel_fs = AIP_CLKSEL_FS(0);
+ reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
+ clksel_aip = AIP_CLKSEL_AIP_I2S;
+ clksel_fs = AIP_CLKSEL_FS_ACLK;
cts_n = CTS_N_M(3) | CTS_N_K(3);
aclk = 1; /* clock enable */
ca_i2s = CA_I2S_CA_I2S(0);
--
1.8.5.3
^ permalink raw reply related
* [PATCH v3 17/24] drm/i2c: tda998x: get a better status of the connection
From: Jean-Francois Moine @ 2014-01-19 18:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cover.1390153344.git.moinejf@free.fr>
This patch refines the connection status testing both bits RXSENS and
HPD of the CEC register giving the connection level.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 013a67c..ea7d1b4 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1050,7 +1050,9 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
struct tda998x_priv *priv = to_tda998x_priv(encoder);
uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);
- return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
+ return (val & (CEC_RXSHPDLEV_RXSENS | CEC_RXSHPDLEV_HPD)) ==
+ (CEC_RXSHPDLEV_RXSENS | CEC_RXSHPDLEV_HPD) ?
+ connector_status_connected :
connector_status_disconnected;
}
--
1.8.5.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox