* Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
From: Bjorn Helgaas @ 2015-04-02 20:13 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Konrad Rzeszutek Wilk, Luis R. Rodriguez, Andy Lutomirski,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, jgross, Jan Beulich,
Borislav Petkov, Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, linux-fbdev, x86@kernel.org,
xen-devel@lists.xenproject.org, Ingo Molnar, Daniel Vetter,
Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Dave Hansen, Stefan Bader, Ville Syrjälä, David Vrabel,
Toshi Kani, Roger Pau Monné, xen-devel
In-Reply-To: <20150326233555.GE5622@wotan.suse.de>
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> I'll rephrase this to:
>
> ---
> It is possible to enable CONFIG_MTRR and up with it
> disabled at run time and yet CONFIG_X86_PAT continues
> to kick through with all functionally enabled. This
> can happen for instance on Xen where MTRR is not
> supported but PAT is, this can happen now on Linux as
> of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean? And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
Please use the conventional citation format:
47591df50512 ("xen: Support Xen pv-domains using PAT")
A one-character typo in a SHA1 makes it completely useless, so it's
nice to have the summary line both for readability and a bit of
redundancy.
Bjorn
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Andy Lutomirski @ 2015-04-02 19:50 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Mel Gorman, Vlastimil Babka, Ville Syrjälä,
Bjorn Helgaas, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Juergen Gross, Jan Beulich, Borislav Petkov,
Suresh Siddha, venkatesh.pallipadi, Dave Airlie,
linux-kernel@vger.kernel.org, Linux Fbdev development list,
X86 ML, xen-devel@lists.xenproject.org, Ingo Molnar,
Linus Torvalds, Daniel Vetter, Antonino Daplas,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen
In-Reply-To: <20150402194517.GW5622@wotan.suse.de>
On Thu, Apr 2, 2015 at 12:45 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Wed, Apr 01, 2015 at 05:04:08PM -0700, Andy Lutomirski wrote:
>> On Wed, Apr 1, 2015 at 4:52 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > On Sat, Mar 28, 2015 at 02:23:34PM +0200, Ville Syrjälä wrote:
>> >> On Sat, Mar 28, 2015 at 01:28:18AM +0100, Luis R. Rodriguez wrote:
>> >> > On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:
>> >> > > On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
>> >> > > > On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
>> >> > > >> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
>> >> > > >> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> >> > > >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
>> >> > > >> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
>> >> > > >> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
>> >> > > >> > >> > index 8025624..8875e56 100644
>> >> > > >> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
>> >> > > >> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
>> >> > > >> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
>> >> > > >> > >> >
>> >> > > >> > >> > #ifdef CONFIG_MTRR
>> >> > > >> > >> > par->mtrr_aper = -1;
>> >> > > >> > >> > - par->mtrr_reg = -1;
>> >> > > >> > >> > if (!nomtrr) {
>> >> > > >> > >> > - /* Cover the whole resource. */
>> >> > > >> > >> > - par->mtrr_aper = mtrr_add(par->res_start, par->res_size,
>> >> > > >> > >> > + par->mtrr_aper = mtrr_add(info->fix.smem_start,
>> >> > > >> > >> > + info->fix.smem_len,
>> >> > > >> > >> > MTRR_TYPE_WRCOMB, 1);
>> >> > > >> > >>
>> >> > > >> > >> MTRRs need power of two size, so how is this supposed to work?
>> >> > > >> > >
>> >> > > >> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
>> >> > > >> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
>> >> > > >> > > is not standardized and by no means recorded as a requirement. Obviously
>> >> > > >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
>> >> > > >> > > will use mtrr_check() to verify the the same requirement. Furthermore,
>> >> > > >> > > as per my commit log message:
>> >> > > >> >
>> >> > > >> > Whatever the code may or may not do, the x86 architecture uses
>> >> > > >> > power-of-two MTRR sizes. So I'm confused.
>> >> > > >>
>> >> > > >> There should be no confusion, I simply did not know that *was* the
>> >> > > >> requirement for x86, if that is the case we should add a check for that
>> >> > > >> and perhaps generalize a helper that does the power of two helper changes,
>> >> > > >> the cleanest I found was the vesafb driver solution.
>> >> > > >>
>> >> > > >> Thoughts?
>> >> > > >
>> >> > > > The vesafb solution is bad since you'll only end up covering only
>> >> > > > the first 4MB of the framebuffer instead of the almost 8MB you want.
>> >> > > > Which in practice will mean throwing away half the VRAM since you really
>> >> > > > don't want the massive performance hit from accessing it as UC. And that
>> >> > > > would mean giving up decent display resolutions as well :(
>> >> > > >
>> >> > > > And the other option of trying to cover the remainder with multiple ever
>> >> > > > smaller MTRRs doesn't work either since you'll run out of MTRRs very
>> >> > > > quickly.
>> >> > > >
>> >> > > > This is precisely why I used the hole method in atyfb in the first
>> >> > > > place.
>> >> > > >
>> >> > > > I don't really like the idea of any new mtrr code not supporting that
>> >> > > > use case, especially as these things tend to be present in older machines
>> >> > > > where PAT isn't an option.
>> >> > >
>> >> > > According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
>> >> > > non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
>> >> > > an effective memory type of UC.
>> >
>> > This is true but non-PAT systems that use just ioremap() will default to
>> > _PAGE_CACHE_MODE_UC_MINUS, not _PAGE_CACHE_MODE_UC, and _PAGE_CACHE_MODE_UC_MINUS
>> > on Linux has PCD = 1, PWT = 0. The list comes from:
>> >
>> > uint16_t __cachemode2pte_tbl[_PAGE_CACHE_MODE_NUM] = {
>> > [_PAGE_CACHE_MODE_WB ] = 0 | 0 ,
>> > [_PAGE_CACHE_MODE_WC ] = _PAGE_PWT | 0 ,
>> > [_PAGE_CACHE_MODE_UC_MINUS] = 0 | _PAGE_PCD,
>> > [_PAGE_CACHE_MODE_UC ] = _PAGE_PWT | _PAGE_PCD,
>> > [_PAGE_CACHE_MODE_WT ] = 0 | _PAGE_PCD,
>> > [_PAGE_CACHE_MODE_WP ] = 0 | _PAGE_PCD,
>> > };
>> >
>> > This can better be read here:
>> >
>> > PAT
>> > |PCD
>> > ||PWT
>> > |||
>> > 000 WB _PAGE_CACHE_MODE_WB
>> > 001 WC _PAGE_CACHE_MODE_WC
>> > 010 UC- _PAGE_CACHE_MODE_UC_MINUS
>> > 011 UC _PAGE_CACHE_MODE_UC
>> >
>> > On x86 ioremap() defaults to ioremap_nocache() and right now that uses
>> > _PAGE_CACHE_MODE_UC_MINUS not _PAGE_CACHE_MODE_UC. We have two cases
>> > to consider for non-PAT systems then:
>> >
>> > a) Right now as ioremap() and ioremap_nocache() default to _PAGE_CACHE_MODE_UC_MINUS
>> > on x86. In this case using a WC MTRR seems to use PWT=0, PCD=1, and
>> > table table 11-6 on non-PAT systems seems to place this situation as
>> > "implementation defined" and not encouraged.
>> >
>> > a) when commit de33c442e "x86 PAT: fix performance drop for glx, use
>> > UC minus for ioremap(), ioremap_nocache() and pci_mmap_page_range()"
>> > gets reverted and we use _PAGE_CACHE_MODE_UC by default. In this
>> > case on x86 for both ioremap() and ioremap_nocache() as they will
>> > both default to _PAGE_CACHE_MODE_UC we'll end up as you note with
>> > an effective memory type of UC.
>> >
>> > If I've understood this correctly then neither of these situations are good and
>> > its just by chance that on some systems situation a) has lead to proper WC.
>> >
>> > On a PAT system we have a bit different combinatorial results (based on Table
>> > 11-7):
>> >
>> > a) Right now ioremap() and ioremap_nocache() defaulting to
>> > _PAGE_CACHE_MODE_UC_MINUS yields + MTRR WC = WC
>> >
>> > b) When commit de33c442e gets reverted _PAGE_CACHE_MODE_UC + MTRR WC = UC
>> >
>> > So to be clear right now atyfb should work fine on PAT systems
>> > with de33c442e in place, once reverted as-is right now we'd end
>> > up with UC effective memory type.
>> >
>> > For both PAT and non-PAT systems when commit de33c442e gets reverted
>> > we'd end up with UC as the effective memory type for atyfb. Right
>> > now it shoud work on PAT systems and by chance its suspected to work
>> > on non-PAT systems. We want to phase MTRR though, specially to avoid
>> > all this insane combinatorial nightmware.
>> >
>> >> > > Hence my suggestion to add
>> >> > > ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
>> >> > > otherwise WC MTRR-covered region.
>> >
>> > To be clear I think you mean then that ioremap_x86_uc() would help us avoid the
>> > jumps between combinatorial issues with MTRR on PAT / non-PAT systems before
>> > and after commit de33c442e gets reverted. So for instance if we had on the
>> > atyfb driver:
>> >
>> > ioremap_x86_uc(PCI BAR)
>> > ioremap_wc(framebuffer)
>> > arch_phys_add_wc(PCI BAR)
>> >
>> > On non-PAT systems on the MMIO region with PWT=1, PCD=1 we'd end up with UC.
>> > Sadly though since _PAGE_CACHE_WC on non-PAT has PWT=1, PCD=0, the WC
>> > MTRR that follows would mean we'd end up with another grey area (but
>> > similar to before as technically an effectivethe memory type of WC).
>> >
>> > On PAT systems the above would not use MTRRs but we'd be counting on
>> > overlapping memory types -- its not clear if aliasing here is a problem.
>> >
>> > Also Intel SDM, volume 3, section "11.11.4 Range Size and Alignment Requirement"
>> > describes that: "the minimum range size is 4 KiB, the base address must be on
>> > a 4 KiB boundary. For ranges greater than 4 KiB each range must be of length
>> > 2^n and its base address must be alinged on a 2^n boundary where n is a value
>> > equal or greatar then 12. The base-address alignment value cannot be less
>> > than its length. For example, an 8-KiB range cannot be aligned on a
>> > 4-KiB boundary. It must be aligned on at least an 8-KiB boundary"
>> >
>> > So to answer my own question: indeed, our framebuffer base address must be
>> > aligned on a 2^n boundary, the size also has to be a power of 2. MTRR supports
>> > fixed range sizes and variable range sizes, in case of the MMIO that does
>> > not need to abide by the power of 2 rule as a fixed range size of 4 KiB
>> > could be used although upon review ouf our own implemetnation its unclear if
>> > that is what is used for 4 KiB sized MTRRs.
>> >
>> > Hence my arch_phys_add_wc(PCI BAR) as above.
>> >
>> >> > OK I think I get it now.
>> >> >
>> >> > And I take it this would hopefully only be used for non-PAT systems?
>> >
>> > Since we likely could care to use ioremap_x86_uc() on PAT systems as well we
>> > could make the effective for both PAT and non-PAT obviously then. Later when
>> > we get ioremap() to default to strong UC we could drop ioremap_x86_uc() as we'd
>> > only need it as transitory until then -- that is unless we want perhaps a strong
>> > UC ioremap primitive which is always following strong UC when available regardless
>> > of these default transitions.
>> >
>> > The big issue I see here is simply the combinatorial issues, so I do think
>> > its best to annotate these corner cases well and avoid them.
>> >
>> >> > Would there be a use case for PAT systems? I wonder if we can wrap
>> >> > this under some APIs to make it clean and hide this dirty thing
>> >> > behind the scenes, it seems a fragile and error prone and my hope
>> >> > would be that we won't need more specialization in this area for
>> >> > PAT systems.
>> >>
>> >> One potential complication is kernel vs. userspace mmap. MTRR applies to
>> >> the physical address, but PAT applies to the virtual address, so with
>> >> the WC MTRR you get WC for userspace "for free" as well.
>> >
>> > What is the performance impact of having the conversion being done by the
>> > kernel? Has anyone done measurements? If significant can't the subsystem mmap()
>> > cache the phys address for PAT? Shouldn't the TLB take care of those considerations
>> > for us? If this is generally desirable shouldn't we just generalize the cache
>> > for devices for O(1) access through a generic API?
>>
>> We're pretty much required to keep the PTE memory types consistent for
>> aliasses of the same page.
>
> Hrm, OK so overlapping ioremap() calls should be frowed upon?
>
> I think its important to clarify the few different scenarios we have
> for atyfb, both for today when uc- is default and when uc becomes the
> default. I'll also clarify what this series originally tried to do
> but the issues that size requirements prohibit us to do along with
> combinatorial issues that would also be present when and if uc becomes
> default. Finally I'll clarify what I am thinking we should do in light
> of all this.
>
> _______________________________________________________________________
> | | |
> |_______________________________________________________|_____________|
>
> \______________________________________________________/ \____________/
>
> Framebuffer (8 MiB) MMIO (4 KiB)
>
> Currently we have:
>
> Page_cache_mode's _PAGE_CACHE_MODE_ is removed below for brevity.
> The atyfb PCI BAR is condensed to:
>
> Frambuffer,MMIO
>
> Keeping in mind:
>
> Intel SDM, volume 3, section 11.5.2.1, table 11-6 (NonPAT combinatorial)
> Intel SDM, volume 3, section 11.5.2.2, table 11-7 (PAT combinatorial)
>
> Linux PCD, PWT bits:
>
> PAT
> |PCD
> ||PWT
> |||
> 000 WB _PAGE_CACHE_MODE_WB
> 001 WC _PAGE_CACHE_MODE_WC
> 010 UC- _PAGE_CACHE_MODE_UC_MINUS
> 011 UC _PAGE_CACHE_MODE_UC
>
> (*) below denotes grey area as per SDM, implementation-defined
> (%) below denotes not posislbe due to size / base requirements of MTRRs
> (+) below denotes combinatorial issue
>
> Non-PAT systems use PCD, PWT values, their respective bit settings for
> these are given although internally we use _PAGE_CACHE_MODE* on the
> ioremap* calls for both non-PAT and PAT. For instance
> _PAGE_CACHE_MODE_UC_MINUS is 10 for PCD=1, PWT=0.
>
> Today we have:
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap(MMIO) | xxx, 10 | xxx, UC- | xxx, UC | xxx, UC- |
> ioremap(PCI BAR) | 10 , 10 | UC-, UC- | UC, UC | UC-, UC- |
> MTRR WC(PCI BAR) | 10 , 10 | UC-, UC- | WC*, WC* | WC , WC |
> MTRR UC(MMIO) | 10 , 10 | UC-, UC- | WC*, UC | WC , UC |
> --------------------------------------------------------------------
>
> If today we revert commit de33c442e and UC becomes default this would run into
> the combinatorial issue:
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap(MMIO) | xxx, 11 | xxx, UC | xxx, UC | xxx, UC |
> ioremap(PCI BAR) | 11 , 11 | UC , UC | UC, UC | UC , UC |
> MTRR WC(PCI BAR) | 11 , 11 | UC, UC | UC+, UC+ | UC+, UC+ |
> MTRR UC(MMIO) | 11 , 11 | UC, UC | UC+, UC | UC+, UC |
> --------------------------------------------------------------------
>
> We ideally would like to do the following but can't because of the restriction
> of having to use powers of two for both size and base address for MTRRs, we'd
> have two steps, one with mtrr_add, and another with arch_phys_add_wc(). This is
> what this series was proposing for atyfb.
>
> With mtrr_add():
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap_nocache(MMIO) | xxx, 10 | xxx, UC- | xxx, UC | xxx, UC- |
> ioremap_wc(fb) | 01 , 10 | WC , UC- | UC , UC | WC , UC- |
> MTRR WC(fb) | 01 , 10 | UC-, WC | WC%*,UC | WC%, UC- |
> --------------------------------------------------------------------
>
> Then we'd change this to arch_phys_add_wc():
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap_nocache(MMIO) | xxx, 10 | xxx, UC- | xxx, UC | UC-, UC- |
> ioremap_wc(fb) | 01 , 10 | WC , UC- | UC , UC | WC , UC- |
> arch_phys_add_wc(fb) | 01 , 10 | WC , WC | WC%*,UC | WC , UC- |
> --------------------------------------------------------------------
>
> With the above code as well we have to consider the issues if we
> revert commit de33c442e and UC becomes default, we'd run into then
> both the size issue and also a grey area:
>
> With mtrr_add():
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap_nocache(MMIO) | xxx, 11 | xxx, UC | xxx, UC | xxx, UC |
> ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
> MTRR WC(fb) | 01 , 11 | WC , UC | WC%* ,UC | WC , UC |
> --------------------------------------------------------------------
>
> Then with arch_phys_add_wc():
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap_nocache(MMIO) | xxx, 11 | xxx, UC | xxx, UC | xxx, UC |
> ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
> arch_phys_add_wc(fb) | 01 , 11 | WC , UC | WC%*,UC | WC , UC |
> --------------------------------------------------------------------
>
> So what we *could* do then if we add ioremap_uc() (use strong UC always),
> then override the framebuffer area with wc, and finally use MTRR on the
> full PCI BAR, relying on that strong UC won't let the MTRR override
> the earlier UC on the MMIO area. There is a grey area here for non-PAT
> systemes but that is also the case as-is today.
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap_uc(PCI BAR) | 11 , 11 | UC , UC | UC , UC | UC , UC |
> ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
> MTRR_WC(PCI BAR) | 01 , 11 | WC , UC | WC*, UC | WC , UC |
> --------------------------------------------------------------------
>
> Finally with the arch_phys_add_wc() we'd end up with:
>
> --------------------------------------------------------------------
> Calls | Page_cache_mode | Effective memtype |
> ------------------------|---------------------|---------------------
> | Non-PAT | PAT | Non-PAT | PAT |
> --------------------------------------------------------------------
> ioremap_uc(PCI BAR) | 11 , 11 | UC , UC | UC , UC | UC , UC |
> ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
> arch_phys_add_wc(PCIBAR)| 01 , 11 | WC , UC | WC*, UC | WC , UC |
> --------------------------------------------------------------------
>
> In this case a revert of de33c442e won't have any effect as the driver
> was already well prepared for it by using ioremap_uc().
>
>> I think that the x86 pageattr code is
>> supposed to take care of this. IOW, if everything is working right,
>> then the supposedly uncached mmap should either fail, be promoted to
>> WC, or cause the existing WC map to degrade to UC. The code is really
>> overcomplicated right now.
>
> Yeah aliasing things are not clear for the above picture for me, someone
> who is knee-deep in this can likely confirm of any issues with the above
> pictures. But most importrantly if we believe however that the last two sets
> above don't have any issues then I think we can move forward. Since we only
> have a few drivers that need special handling I think it makes sense to treat
> them specially and document this strategy for the "hole" work around.
>
Seems reaonable to me.
--Andy
> Thoughts?
>
> Luis
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-04-02 19:45 UTC (permalink / raw)
To: Andy Lutomirski, Mel Gorman, Vlastimil Babka
Cc: Ville Syrjälä, Bjorn Helgaas, Luis R. Rodriguez,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Juergen Gross,
Jan Beulich, Borislav Petkov, Suresh Siddha, venkatesh.pallipadi,
Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <CALCETrWLBCp7vseAUYRjiM3h5fvO-jB6zc0qGsSCiJ6Pr3eQaA@mail.gmail.com>
On Wed, Apr 01, 2015 at 05:04:08PM -0700, Andy Lutomirski wrote:
> On Wed, Apr 1, 2015 at 4:52 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Sat, Mar 28, 2015 at 02:23:34PM +0200, Ville Syrjälä wrote:
> >> On Sat, Mar 28, 2015 at 01:28:18AM +0100, Luis R. Rodriguez wrote:
> >> > On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:
> >> > > On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> >> > > > On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
> >> > > >> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
> >> > > >> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> >> > > >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
> >> > > >> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
> >> > > >> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> >> > > >> > >> > index 8025624..8875e56 100644
> >> > > >> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> >> > > >> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> >> > > >> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> >> > > >> > >> >
> >> > > >> > >> > #ifdef CONFIG_MTRR
> >> > > >> > >> > par->mtrr_aper = -1;
> >> > > >> > >> > - par->mtrr_reg = -1;
> >> > > >> > >> > if (!nomtrr) {
> >> > > >> > >> > - /* Cover the whole resource. */
> >> > > >> > >> > - par->mtrr_aper = mtrr_add(par->res_start, par->res_size,
> >> > > >> > >> > + par->mtrr_aper = mtrr_add(info->fix.smem_start,
> >> > > >> > >> > + info->fix.smem_len,
> >> > > >> > >> > MTRR_TYPE_WRCOMB, 1);
> >> > > >> > >>
> >> > > >> > >> MTRRs need power of two size, so how is this supposed to work?
> >> > > >> > >
> >> > > >> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
> >> > > >> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
> >> > > >> > > is not standardized and by no means recorded as a requirement. Obviously
> >> > > >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
> >> > > >> > > will use mtrr_check() to verify the the same requirement. Furthermore,
> >> > > >> > > as per my commit log message:
> >> > > >> >
> >> > > >> > Whatever the code may or may not do, the x86 architecture uses
> >> > > >> > power-of-two MTRR sizes. So I'm confused.
> >> > > >>
> >> > > >> There should be no confusion, I simply did not know that *was* the
> >> > > >> requirement for x86, if that is the case we should add a check for that
> >> > > >> and perhaps generalize a helper that does the power of two helper changes,
> >> > > >> the cleanest I found was the vesafb driver solution.
> >> > > >>
> >> > > >> Thoughts?
> >> > > >
> >> > > > The vesafb solution is bad since you'll only end up covering only
> >> > > > the first 4MB of the framebuffer instead of the almost 8MB you want.
> >> > > > Which in practice will mean throwing away half the VRAM since you really
> >> > > > don't want the massive performance hit from accessing it as UC. And that
> >> > > > would mean giving up decent display resolutions as well :(
> >> > > >
> >> > > > And the other option of trying to cover the remainder with multiple ever
> >> > > > smaller MTRRs doesn't work either since you'll run out of MTRRs very
> >> > > > quickly.
> >> > > >
> >> > > > This is precisely why I used the hole method in atyfb in the first
> >> > > > place.
> >> > > >
> >> > > > I don't really like the idea of any new mtrr code not supporting that
> >> > > > use case, especially as these things tend to be present in older machines
> >> > > > where PAT isn't an option.
> >> > >
> >> > > According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
> >> > > non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
> >> > > an effective memory type of UC.
> >
> > This is true but non-PAT systems that use just ioremap() will default to
> > _PAGE_CACHE_MODE_UC_MINUS, not _PAGE_CACHE_MODE_UC, and _PAGE_CACHE_MODE_UC_MINUS
> > on Linux has PCD = 1, PWT = 0. The list comes from:
> >
> > uint16_t __cachemode2pte_tbl[_PAGE_CACHE_MODE_NUM] = {
> > [_PAGE_CACHE_MODE_WB ] = 0 | 0 ,
> > [_PAGE_CACHE_MODE_WC ] = _PAGE_PWT | 0 ,
> > [_PAGE_CACHE_MODE_UC_MINUS] = 0 | _PAGE_PCD,
> > [_PAGE_CACHE_MODE_UC ] = _PAGE_PWT | _PAGE_PCD,
> > [_PAGE_CACHE_MODE_WT ] = 0 | _PAGE_PCD,
> > [_PAGE_CACHE_MODE_WP ] = 0 | _PAGE_PCD,
> > };
> >
> > This can better be read here:
> >
> > PAT
> > |PCD
> > ||PWT
> > |||
> > 000 WB _PAGE_CACHE_MODE_WB
> > 001 WC _PAGE_CACHE_MODE_WC
> > 010 UC- _PAGE_CACHE_MODE_UC_MINUS
> > 011 UC _PAGE_CACHE_MODE_UC
> >
> > On x86 ioremap() defaults to ioremap_nocache() and right now that uses
> > _PAGE_CACHE_MODE_UC_MINUS not _PAGE_CACHE_MODE_UC. We have two cases
> > to consider for non-PAT systems then:
> >
> > a) Right now as ioremap() and ioremap_nocache() default to _PAGE_CACHE_MODE_UC_MINUS
> > on x86. In this case using a WC MTRR seems to use PWT=0, PCD=1, and
> > table table 11-6 on non-PAT systems seems to place this situation as
> > "implementation defined" and not encouraged.
> >
> > a) when commit de33c442e "x86 PAT: fix performance drop for glx, use
> > UC minus for ioremap(), ioremap_nocache() and pci_mmap_page_range()"
> > gets reverted and we use _PAGE_CACHE_MODE_UC by default. In this
> > case on x86 for both ioremap() and ioremap_nocache() as they will
> > both default to _PAGE_CACHE_MODE_UC we'll end up as you note with
> > an effective memory type of UC.
> >
> > If I've understood this correctly then neither of these situations are good and
> > its just by chance that on some systems situation a) has lead to proper WC.
> >
> > On a PAT system we have a bit different combinatorial results (based on Table
> > 11-7):
> >
> > a) Right now ioremap() and ioremap_nocache() defaulting to
> > _PAGE_CACHE_MODE_UC_MINUS yields + MTRR WC = WC
> >
> > b) When commit de33c442e gets reverted _PAGE_CACHE_MODE_UC + MTRR WC = UC
> >
> > So to be clear right now atyfb should work fine on PAT systems
> > with de33c442e in place, once reverted as-is right now we'd end
> > up with UC effective memory type.
> >
> > For both PAT and non-PAT systems when commit de33c442e gets reverted
> > we'd end up with UC as the effective memory type for atyfb. Right
> > now it shoud work on PAT systems and by chance its suspected to work
> > on non-PAT systems. We want to phase MTRR though, specially to avoid
> > all this insane combinatorial nightmware.
> >
> >> > > Hence my suggestion to add
> >> > > ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
> >> > > otherwise WC MTRR-covered region.
> >
> > To be clear I think you mean then that ioremap_x86_uc() would help us avoid the
> > jumps between combinatorial issues with MTRR on PAT / non-PAT systems before
> > and after commit de33c442e gets reverted. So for instance if we had on the
> > atyfb driver:
> >
> > ioremap_x86_uc(PCI BAR)
> > ioremap_wc(framebuffer)
> > arch_phys_add_wc(PCI BAR)
> >
> > On non-PAT systems on the MMIO region with PWT=1, PCD=1 we'd end up with UC.
> > Sadly though since _PAGE_CACHE_WC on non-PAT has PWT=1, PCD=0, the WC
> > MTRR that follows would mean we'd end up with another grey area (but
> > similar to before as technically an effectivethe memory type of WC).
> >
> > On PAT systems the above would not use MTRRs but we'd be counting on
> > overlapping memory types -- its not clear if aliasing here is a problem.
> >
> > Also Intel SDM, volume 3, section "11.11.4 Range Size and Alignment Requirement"
> > describes that: "the minimum range size is 4 KiB, the base address must be on
> > a 4 KiB boundary. For ranges greater than 4 KiB each range must be of length
> > 2^n and its base address must be alinged on a 2^n boundary where n is a value
> > equal or greatar then 12. The base-address alignment value cannot be less
> > than its length. For example, an 8-KiB range cannot be aligned on a
> > 4-KiB boundary. It must be aligned on at least an 8-KiB boundary"
> >
> > So to answer my own question: indeed, our framebuffer base address must be
> > aligned on a 2^n boundary, the size also has to be a power of 2. MTRR supports
> > fixed range sizes and variable range sizes, in case of the MMIO that does
> > not need to abide by the power of 2 rule as a fixed range size of 4 KiB
> > could be used although upon review ouf our own implemetnation its unclear if
> > that is what is used for 4 KiB sized MTRRs.
> >
> > Hence my arch_phys_add_wc(PCI BAR) as above.
> >
> >> > OK I think I get it now.
> >> >
> >> > And I take it this would hopefully only be used for non-PAT systems?
> >
> > Since we likely could care to use ioremap_x86_uc() on PAT systems as well we
> > could make the effective for both PAT and non-PAT obviously then. Later when
> > we get ioremap() to default to strong UC we could drop ioremap_x86_uc() as we'd
> > only need it as transitory until then -- that is unless we want perhaps a strong
> > UC ioremap primitive which is always following strong UC when available regardless
> > of these default transitions.
> >
> > The big issue I see here is simply the combinatorial issues, so I do think
> > its best to annotate these corner cases well and avoid them.
> >
> >> > Would there be a use case for PAT systems? I wonder if we can wrap
> >> > this under some APIs to make it clean and hide this dirty thing
> >> > behind the scenes, it seems a fragile and error prone and my hope
> >> > would be that we won't need more specialization in this area for
> >> > PAT systems.
> >>
> >> One potential complication is kernel vs. userspace mmap. MTRR applies to
> >> the physical address, but PAT applies to the virtual address, so with
> >> the WC MTRR you get WC for userspace "for free" as well.
> >
> > What is the performance impact of having the conversion being done by the
> > kernel? Has anyone done measurements? If significant can't the subsystem mmap()
> > cache the phys address for PAT? Shouldn't the TLB take care of those considerations
> > for us? If this is generally desirable shouldn't we just generalize the cache
> > for devices for O(1) access through a generic API?
>
> We're pretty much required to keep the PTE memory types consistent for
> aliasses of the same page.
Hrm, OK so overlapping ioremap() calls should be frowed upon?
I think its important to clarify the few different scenarios we have
for atyfb, both for today when uc- is default and when uc becomes the
default. I'll also clarify what this series originally tried to do
but the issues that size requirements prohibit us to do along with
combinatorial issues that would also be present when and if uc becomes
default. Finally I'll clarify what I am thinking we should do in light
of all this.
_______________________________________________________________________
| | |
|_______________________________________________________|_____________|
\______________________________________________________/ \____________/
Framebuffer (8 MiB) MMIO (4 KiB)
Currently we have:
Page_cache_mode's _PAGE_CACHE_MODE_ is removed below for brevity.
The atyfb PCI BAR is condensed to:
Frambuffer,MMIO
Keeping in mind:
Intel SDM, volume 3, section 11.5.2.1, table 11-6 (NonPAT combinatorial)
Intel SDM, volume 3, section 11.5.2.2, table 11-7 (PAT combinatorial)
Linux PCD, PWT bits:
PAT
|PCD
||PWT
|||
000 WB _PAGE_CACHE_MODE_WB
001 WC _PAGE_CACHE_MODE_WC
010 UC- _PAGE_CACHE_MODE_UC_MINUS
011 UC _PAGE_CACHE_MODE_UC
(*) below denotes grey area as per SDM, implementation-defined
(%) below denotes not posislbe due to size / base requirements of MTRRs
(+) below denotes combinatorial issue
Non-PAT systems use PCD, PWT values, their respective bit settings for
these are given although internally we use _PAGE_CACHE_MODE* on the
ioremap* calls for both non-PAT and PAT. For instance
_PAGE_CACHE_MODE_UC_MINUS is 10 for PCD=1, PWT=0.
Today we have:
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap(MMIO) | xxx, 10 | xxx, UC- | xxx, UC | xxx, UC- |
ioremap(PCI BAR) | 10 , 10 | UC-, UC- | UC, UC | UC-, UC- |
MTRR WC(PCI BAR) | 10 , 10 | UC-, UC- | WC*, WC* | WC , WC |
MTRR UC(MMIO) | 10 , 10 | UC-, UC- | WC*, UC | WC , UC |
--------------------------------------------------------------------
If today we revert commit de33c442e and UC becomes default this would run into
the combinatorial issue:
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap(MMIO) | xxx, 11 | xxx, UC | xxx, UC | xxx, UC |
ioremap(PCI BAR) | 11 , 11 | UC , UC | UC, UC | UC , UC |
MTRR WC(PCI BAR) | 11 , 11 | UC, UC | UC+, UC+ | UC+, UC+ |
MTRR UC(MMIO) | 11 , 11 | UC, UC | UC+, UC | UC+, UC |
--------------------------------------------------------------------
We ideally would like to do the following but can't because of the restriction
of having to use powers of two for both size and base address for MTRRs, we'd
have two steps, one with mtrr_add, and another with arch_phys_add_wc(). This is
what this series was proposing for atyfb.
With mtrr_add():
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap_nocache(MMIO) | xxx, 10 | xxx, UC- | xxx, UC | xxx, UC- |
ioremap_wc(fb) | 01 , 10 | WC , UC- | UC , UC | WC , UC- |
MTRR WC(fb) | 01 , 10 | UC-, WC | WC%*,UC | WC%, UC- |
--------------------------------------------------------------------
Then we'd change this to arch_phys_add_wc():
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap_nocache(MMIO) | xxx, 10 | xxx, UC- | xxx, UC | UC-, UC- |
ioremap_wc(fb) | 01 , 10 | WC , UC- | UC , UC | WC , UC- |
arch_phys_add_wc(fb) | 01 , 10 | WC , WC | WC%*,UC | WC , UC- |
--------------------------------------------------------------------
With the above code as well we have to consider the issues if we
revert commit de33c442e and UC becomes default, we'd run into then
both the size issue and also a grey area:
With mtrr_add():
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap_nocache(MMIO) | xxx, 11 | xxx, UC | xxx, UC | xxx, UC |
ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
MTRR WC(fb) | 01 , 11 | WC , UC | WC%* ,UC | WC , UC |
--------------------------------------------------------------------
Then with arch_phys_add_wc():
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap_nocache(MMIO) | xxx, 11 | xxx, UC | xxx, UC | xxx, UC |
ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
arch_phys_add_wc(fb) | 01 , 11 | WC , UC | WC%*,UC | WC , UC |
--------------------------------------------------------------------
So what we *could* do then if we add ioremap_uc() (use strong UC always),
then override the framebuffer area with wc, and finally use MTRR on the
full PCI BAR, relying on that strong UC won't let the MTRR override
the earlier UC on the MMIO area. There is a grey area here for non-PAT
systemes but that is also the case as-is today.
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap_uc(PCI BAR) | 11 , 11 | UC , UC | UC , UC | UC , UC |
ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
MTRR_WC(PCI BAR) | 01 , 11 | WC , UC | WC*, UC | WC , UC |
--------------------------------------------------------------------
Finally with the arch_phys_add_wc() we'd end up with:
--------------------------------------------------------------------
Calls | Page_cache_mode | Effective memtype |
------------------------|---------------------|---------------------
| Non-PAT | PAT | Non-PAT | PAT |
--------------------------------------------------------------------
ioremap_uc(PCI BAR) | 11 , 11 | UC , UC | UC , UC | UC , UC |
ioremap_wc(fb) | 01 , 11 | WC , UC | UC , UC | WC , UC |
arch_phys_add_wc(PCIBAR)| 01 , 11 | WC , UC | WC*, UC | WC , UC |
--------------------------------------------------------------------
In this case a revert of de33c442e won't have any effect as the driver
was already well prepared for it by using ioremap_uc().
> I think that the x86 pageattr code is
> supposed to take care of this. IOW, if everything is working right,
> then the supposedly uncached mmap should either fail, be promoted to
> WC, or cause the existing WC map to degrade to UC. The code is really
> overcomplicated right now.
Yeah aliasing things are not clear for the above picture for me, someone
who is knee-deep in this can likely confirm of any issues with the above
pictures. But most importrantly if we believe however that the last two sets
above don't have any issues then I think we can move forward. Since we only
have a few drivers that need special handling I think it makes sense to treat
them specially and document this strategy for the "hole" work around.
Thoughts?
Luis
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Andy Lutomirski @ 2015-04-02 0:04 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Ville Syrjälä, Bjorn Helgaas, Luis R. Rodriguez,
Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Juergen Gross,
Jan Beulich, Borislav Petkov, Suresh Siddha, venkatesh.pallipadi,
Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150401235211.GV5622@wotan.suse.de>
On Wed, Apr 1, 2015 at 4:52 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Sat, Mar 28, 2015 at 02:23:34PM +0200, Ville Syrjälä wrote:
>> On Sat, Mar 28, 2015 at 01:28:18AM +0100, Luis R. Rodriguez wrote:
>> > On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:
>> > > On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
>> > > > On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
>> > > >> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
>> > > >> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > > >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
>> > > >> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
>> > > >> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
>> > > >> > >> > index 8025624..8875e56 100644
>> > > >> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
>> > > >> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
>> > > >> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
>> > > >> > >> >
>> > > >> > >> > #ifdef CONFIG_MTRR
>> > > >> > >> > par->mtrr_aper = -1;
>> > > >> > >> > - par->mtrr_reg = -1;
>> > > >> > >> > if (!nomtrr) {
>> > > >> > >> > - /* Cover the whole resource. */
>> > > >> > >> > - par->mtrr_aper = mtrr_add(par->res_start, par->res_size,
>> > > >> > >> > + par->mtrr_aper = mtrr_add(info->fix.smem_start,
>> > > >> > >> > + info->fix.smem_len,
>> > > >> > >> > MTRR_TYPE_WRCOMB, 1);
>> > > >> > >>
>> > > >> > >> MTRRs need power of two size, so how is this supposed to work?
>> > > >> > >
>> > > >> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
>> > > >> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
>> > > >> > > is not standardized and by no means recorded as a requirement. Obviously
>> > > >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
>> > > >> > > will use mtrr_check() to verify the the same requirement. Furthermore,
>> > > >> > > as per my commit log message:
>> > > >> >
>> > > >> > Whatever the code may or may not do, the x86 architecture uses
>> > > >> > power-of-two MTRR sizes. So I'm confused.
>> > > >>
>> > > >> There should be no confusion, I simply did not know that *was* the
>> > > >> requirement for x86, if that is the case we should add a check for that
>> > > >> and perhaps generalize a helper that does the power of two helper changes,
>> > > >> the cleanest I found was the vesafb driver solution.
>> > > >>
>> > > >> Thoughts?
>> > > >
>> > > > The vesafb solution is bad since you'll only end up covering only
>> > > > the first 4MB of the framebuffer instead of the almost 8MB you want.
>> > > > Which in practice will mean throwing away half the VRAM since you really
>> > > > don't want the massive performance hit from accessing it as UC. And that
>> > > > would mean giving up decent display resolutions as well :(
>> > > >
>> > > > And the other option of trying to cover the remainder with multiple ever
>> > > > smaller MTRRs doesn't work either since you'll run out of MTRRs very
>> > > > quickly.
>> > > >
>> > > > This is precisely why I used the hole method in atyfb in the first
>> > > > place.
>> > > >
>> > > > I don't really like the idea of any new mtrr code not supporting that
>> > > > use case, especially as these things tend to be present in older machines
>> > > > where PAT isn't an option.
>> > >
>> > > According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
>> > > non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
>> > > an effective memory type of UC.
>
> This is true but non-PAT systems that use just ioremap() will default to
> _PAGE_CACHE_MODE_UC_MINUS, not _PAGE_CACHE_MODE_UC, and _PAGE_CACHE_MODE_UC_MINUS
> on Linux has PCD = 1, PWT = 0. The list comes from:
>
> uint16_t __cachemode2pte_tbl[_PAGE_CACHE_MODE_NUM] = {
> [_PAGE_CACHE_MODE_WB ] = 0 | 0 ,
> [_PAGE_CACHE_MODE_WC ] = _PAGE_PWT | 0 ,
> [_PAGE_CACHE_MODE_UC_MINUS] = 0 | _PAGE_PCD,
> [_PAGE_CACHE_MODE_UC ] = _PAGE_PWT | _PAGE_PCD,
> [_PAGE_CACHE_MODE_WT ] = 0 | _PAGE_PCD,
> [_PAGE_CACHE_MODE_WP ] = 0 | _PAGE_PCD,
> };
>
> This can better be read here:
>
> PAT
> |PCD
> ||PWT
> |||
> 000 WB _PAGE_CACHE_MODE_WB
> 001 WC _PAGE_CACHE_MODE_WC
> 010 UC- _PAGE_CACHE_MODE_UC_MINUS
> 011 UC _PAGE_CACHE_MODE_UC
>
> On x86 ioremap() defaults to ioremap_nocache() and right now that uses
> _PAGE_CACHE_MODE_UC_MINUS not _PAGE_CACHE_MODE_UC. We have two cases
> to consider for non-PAT systems then:
>
> a) Right now as ioremap() and ioremap_nocache() default to _PAGE_CACHE_MODE_UC_MINUS
> on x86. In this case using a WC MTRR seems to use PWT=0, PCD=1, and
> table table 11-6 on non-PAT systems seems to place this situation as
> "implementation defined" and not encouraged.
>
> a) when commit de33c442e "x86 PAT: fix performance drop for glx, use
> UC minus for ioremap(), ioremap_nocache() and pci_mmap_page_range()"
> gets reverted and we use _PAGE_CACHE_MODE_UC by default. In this
> case on x86 for both ioremap() and ioremap_nocache() as they will
> both default to _PAGE_CACHE_MODE_UC we'll end up as you note with
> an effective memory type of UC.
>
> If I've understood this correctly then neither of these situations are good and
> its just by chance that on some systems situation a) has lead to proper WC.
>
> On a PAT system we have a bit different combinatorial results (based on Table
> 11-7):
>
> a) Right now ioremap() and ioremap_nocache() defaulting to
> _PAGE_CACHE_MODE_UC_MINUS yields + MTRR WC = WC
>
> b) When commit de33c442e gets reverted _PAGE_CACHE_MODE_UC + MTRR WC = UC
>
> So to be clear right now atyfb should work fine on PAT systems
> with de33c442e in place, once reverted as-is right now we'd end
> up with UC effective memory type.
>
> For both PAT and non-PAT systems when commit de33c442e gets reverted
> we'd end up with UC as the effective memory type for atyfb. Right
> now it shoud work on PAT systems and by chance its suspected to work
> on non-PAT systems. We want to phase MTRR though, specially to avoid
> all this insane combinatorial nightmware.
>
>> > > Hence my suggestion to add
>> > > ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
>> > > otherwise WC MTRR-covered region.
>
> To be clear I think you mean then that ioremap_x86_uc() would help us avoid the
> jumps between combinatorial issues with MTRR on PAT / non-PAT systems before
> and after commit de33c442e gets reverted. So for instance if we had on the
> atyfb driver:
>
> ioremap_x86_uc(PCI BAR)
> ioremap_wc(framebuffer)
> arch_phys_add_wc(PCI BAR)
>
> On non-PAT systems on the MMIO region with PWT=1, PCD=1 we'd end up with UC.
> Sadly though since _PAGE_CACHE_WC on non-PAT has PWT=1, PCD=0, the WC
> MTRR that follows would mean we'd end up with another grey area (but
> similar to before as technically an effectivethe memory type of WC).
>
> On PAT systems the above would not use MTRRs but we'd be counting on
> overlapping memory types -- its not clear if aliasing here is a problem.
>
> Also Intel SDM, volume 3, section "11.11.4 Range Size and Alignment Requirement"
> describes that: "the minimum range size is 4 KiB, the base address must be on
> a 4 KiB boundary. For ranges greater than 4 KiB each range must be of length
> 2^n and its base address must be alinged on a 2^n boundary where n is a value
> equal or greatar then 12. The base-address alignment value cannot be less
> than its length. For example, an 8-KiB range cannot be aligned on a
> 4-KiB boundary. It must be aligned on at least an 8-KiB boundary"
>
> So to answer my own question: indeed, our framebuffer base address must be
> aligned on a 2^n boundary, the size also has to be a power of 2. MTRR supports
> fixed range sizes and variable range sizes, in case of the MMIO that does
> not need to abide by the power of 2 rule as a fixed range size of 4 KiB
> could be used although upon review ouf our own implemetnation its unclear if
> that is what is used for 4 KiB sized MTRRs.
>
> Hence my arch_phys_add_wc(PCI BAR) as above.
>
>> > OK I think I get it now.
>> >
>> > And I take it this would hopefully only be used for non-PAT systems?
>
> Since we likely could care to use ioremap_x86_uc() on PAT systems as well we
> could make the effective for both PAT and non-PAT obviously then. Later when
> we get ioremap() to default to strong UC we could drop ioremap_x86_uc() as we'd
> only need it as transitory until then -- that is unless we want perhaps a strong
> UC ioremap primitive which is always following strong UC when available regardless
> of these default transitions.
>
> The big issue I see here is simply the combinatorial issues, so I do think
> its best to annotate these corner cases well and avoid them.
>
>> > Would there be a use case for PAT systems? I wonder if we can wrap
>> > this under some APIs to make it clean and hide this dirty thing
>> > behind the scenes, it seems a fragile and error prone and my hope
>> > would be that we won't need more specialization in this area for
>> > PAT systems.
>>
>> One potential complication is kernel vs. userspace mmap. MTRR applies to
>> the physical address, but PAT applies to the virtual address, so with
>> the WC MTRR you get WC for userspace "for free" as well.
>
> What is the performance impact of having the conversion being done by the
> kernel? Has anyone done measurements? If significant can't the subsystem mmap()
> cache the phys address for PAT? Shouldn't the TLB take care of those considerations
> for us? If this is generally desirable shouldn't we just generalize the cache
> for devices for O(1) access through a generic API?
We're pretty much required to keep the PTE memory types consistent for
aliasses of the same page. I think that the x86 pageattr code is
supposed to take care of this. IOW, if everything is working right,
then the supposedly uncached mmap should either fail, be promoted to
WC, or cause the existing WC map to degrade to UC. The code is really
overcomplicated right now.
--Andy
^ permalink raw reply
* Re: [PATCH v1 09/47] vidoe: fbdev: atyfb: remove and fix MTRR MMIO "hole" work around
From: Luis R. Rodriguez @ 2015-04-01 23:52 UTC (permalink / raw)
To: Ville Syrjälä, Andy Lutomirski, Bjorn Helgaas,
Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Juergen Gross, Jan Beulich, Borislav Petkov, Suresh Siddha,
venkatesh.pallipadi, Dave Airlie, linux-kernel@vger.kernel.org,
Linux Fbdev development list, X86 ML,
xen-devel@lists.xenproject.org, Ingo Molnar, Linus Torvalds,
Daniel Vetter, Antonino Daplas, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen
In-Reply-To: <20150328122334.GA32543@sci.fi>
On Sat, Mar 28, 2015 at 02:23:34PM +0200, Ville Syrjälä wrote:
> On Sat, Mar 28, 2015 at 01:28:18AM +0100, Luis R. Rodriguez wrote:
> > On Fri, Mar 27, 2015 at 03:02:10PM -0700, Andy Lutomirski wrote:
> > > On Fri, Mar 27, 2015 at 2:56 PM, Ville Syrjälä <syrjala@sci.fi> wrote:
> > > > On Fri, Mar 27, 2015 at 08:57:59PM +0100, Luis R. Rodriguez wrote:
> > > >> On Fri, Mar 27, 2015 at 12:43:55PM -0700, Andy Lutomirski wrote:
> > > >> > On Fri, Mar 27, 2015 at 12:38 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > >> > > On Sat, Mar 21, 2015 at 11:15:14AM +0200, Ville Syrjälä wrote:
> > > >> > >> On Fri, Mar 20, 2015 at 04:17:59PM -0700, Luis R. Rodriguez wrote:
> > > >> > >> > diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> > > >> > >> > index 8025624..8875e56 100644
> > > >> > >> > --- a/drivers/video/fbdev/aty/atyfb_base.c
> > > >> > >> > +++ b/drivers/video/fbdev/aty/atyfb_base.c
> > > >> > >> > @@ -2630,21 +2630,10 @@ static int aty_init(struct fb_info *info)
> > > >> > >> >
> > > >> > >> > #ifdef CONFIG_MTRR
> > > >> > >> > par->mtrr_aper = -1;
> > > >> > >> > - par->mtrr_reg = -1;
> > > >> > >> > if (!nomtrr) {
> > > >> > >> > - /* Cover the whole resource. */
> > > >> > >> > - par->mtrr_aper = mtrr_add(par->res_start, par->res_size,
> > > >> > >> > + par->mtrr_aper = mtrr_add(info->fix.smem_start,
> > > >> > >> > + info->fix.smem_len,
> > > >> > >> > MTRR_TYPE_WRCOMB, 1);
> > > >> > >>
> > > >> > >> MTRRs need power of two size, so how is this supposed to work?
> > > >> > >
> > > >> > > As per mtrr_add_page() [0] the base and size are just supposed to be in units
> > > >> > > of 4 KiB, although the practice is to use powers of 2 in *some* drivers this
> > > >> > > is not standardized and by no means recorded as a requirement. Obviously
> > > >> > > powers of 2 will work too and you'd end up neatly aligned as well. mtrr_add()
> > > >> > > will use mtrr_check() to verify the the same requirement. Furthermore,
> > > >> > > as per my commit log message:
> > > >> >
> > > >> > Whatever the code may or may not do, the x86 architecture uses
> > > >> > power-of-two MTRR sizes. So I'm confused.
> > > >>
> > > >> There should be no confusion, I simply did not know that *was* the
> > > >> requirement for x86, if that is the case we should add a check for that
> > > >> and perhaps generalize a helper that does the power of two helper changes,
> > > >> the cleanest I found was the vesafb driver solution.
> > > >>
> > > >> Thoughts?
> > > >
> > > > The vesafb solution is bad since you'll only end up covering only
> > > > the first 4MB of the framebuffer instead of the almost 8MB you want.
> > > > Which in practice will mean throwing away half the VRAM since you really
> > > > don't want the massive performance hit from accessing it as UC. And that
> > > > would mean giving up decent display resolutions as well :(
> > > >
> > > > And the other option of trying to cover the remainder with multiple ever
> > > > smaller MTRRs doesn't work either since you'll run out of MTRRs very
> > > > quickly.
> > > >
> > > > This is precisely why I used the hole method in atyfb in the first
> > > > place.
> > > >
> > > > I don't really like the idea of any new mtrr code not supporting that
> > > > use case, especially as these things tend to be present in older machines
> > > > where PAT isn't an option.
> > >
> > > According to the Intel SDM, volume 3, section 11.5.2.1, table 11-6,
> > > non-PAT CPUs that have a WC MTRR, PCD = 1, and PWT = 1 (aka UC) have
> > > an effective memory type of UC.
This is true but non-PAT systems that use just ioremap() will default to
_PAGE_CACHE_MODE_UC_MINUS, not _PAGE_CACHE_MODE_UC, and _PAGE_CACHE_MODE_UC_MINUS
on Linux has PCD = 1, PWT = 0. The list comes from:
uint16_t __cachemode2pte_tbl[_PAGE_CACHE_MODE_NUM] = {
[_PAGE_CACHE_MODE_WB ] = 0 | 0 ,
[_PAGE_CACHE_MODE_WC ] = _PAGE_PWT | 0 ,
[_PAGE_CACHE_MODE_UC_MINUS] = 0 | _PAGE_PCD,
[_PAGE_CACHE_MODE_UC ] = _PAGE_PWT | _PAGE_PCD,
[_PAGE_CACHE_MODE_WT ] = 0 | _PAGE_PCD,
[_PAGE_CACHE_MODE_WP ] = 0 | _PAGE_PCD,
};
This can better be read here:
PAT
|PCD
||PWT
|||
000 WB _PAGE_CACHE_MODE_WB
001 WC _PAGE_CACHE_MODE_WC
010 UC- _PAGE_CACHE_MODE_UC_MINUS
011 UC _PAGE_CACHE_MODE_UC
On x86 ioremap() defaults to ioremap_nocache() and right now that uses
_PAGE_CACHE_MODE_UC_MINUS not _PAGE_CACHE_MODE_UC. We have two cases
to consider for non-PAT systems then:
a) Right now as ioremap() and ioremap_nocache() default to _PAGE_CACHE_MODE_UC_MINUS
on x86. In this case using a WC MTRR seems to use PWT=0, PCD=1, and
table table 11-6 on non-PAT systems seems to place this situation as
"implementation defined" and not encouraged.
a) when commit de33c442e "x86 PAT: fix performance drop for glx, use
UC minus for ioremap(), ioremap_nocache() and pci_mmap_page_range()"
gets reverted and we use _PAGE_CACHE_MODE_UC by default. In this
case on x86 for both ioremap() and ioremap_nocache() as they will
both default to _PAGE_CACHE_MODE_UC we'll end up as you note with
an effective memory type of UC.
If I've understood this correctly then neither of these situations are good and
its just by chance that on some systems situation a) has lead to proper WC.
On a PAT system we have a bit different combinatorial results (based on Table
11-7):
a) Right now ioremap() and ioremap_nocache() defaulting to
_PAGE_CACHE_MODE_UC_MINUS yields + MTRR WC = WC
b) When commit de33c442e gets reverted _PAGE_CACHE_MODE_UC + MTRR WC = UC
So to be clear right now atyfb should work fine on PAT systems
with de33c442e in place, once reverted as-is right now we'd end
up with UC effective memory type.
For both PAT and non-PAT systems when commit de33c442e gets reverted
we'd end up with UC as the effective memory type for atyfb. Right
now it shoud work on PAT systems and by chance its suspected to work
on non-PAT systems. We want to phase MTRR though, specially to avoid
all this insane combinatorial nightmware.
> > > Hence my suggestion to add
> > > ioremap_x86_uc and/or set_memory_x86_uc to punch a UC hole in an
> > > otherwise WC MTRR-covered region.
To be clear I think you mean then that ioremap_x86_uc() would help us avoid the
jumps between combinatorial issues with MTRR on PAT / non-PAT systems before
and after commit de33c442e gets reverted. So for instance if we had on the
atyfb driver:
ioremap_x86_uc(PCI BAR)
ioremap_wc(framebuffer)
arch_phys_add_wc(PCI BAR)
On non-PAT systems on the MMIO region with PWT=1, PCD=1 we'd end up with UC.
Sadly though since _PAGE_CACHE_WC on non-PAT has PWT=1, PCD=0, the WC
MTRR that follows would mean we'd end up with another grey area (but
similar to before as technically an effectivethe memory type of WC).
On PAT systems the above would not use MTRRs but we'd be counting on
overlapping memory types -- its not clear if aliasing here is a problem.
Also Intel SDM, volume 3, section "11.11.4 Range Size and Alignment Requirement"
describes that: "the minimum range size is 4 KiB, the base address must be on
a 4 KiB boundary. For ranges greater than 4 KiB each range must be of length
2^n and its base address must be alinged on a 2^n boundary where n is a value
equal or greatar then 12. The base-address alignment value cannot be less
than its length. For example, an 8-KiB range cannot be aligned on a
4-KiB boundary. It must be aligned on at least an 8-KiB boundary"
So to answer my own question: indeed, our framebuffer base address must be
aligned on a 2^n boundary, the size also has to be a power of 2. MTRR supports
fixed range sizes and variable range sizes, in case of the MMIO that does
not need to abide by the power of 2 rule as a fixed range size of 4 KiB
could be used although upon review ouf our own implemetnation its unclear if
that is what is used for 4 KiB sized MTRRs.
Hence my arch_phys_add_wc(PCI BAR) as above.
> > OK I think I get it now.
> >
> > And I take it this would hopefully only be used for non-PAT systems?
Since we likely could care to use ioremap_x86_uc() on PAT systems as well we
could make the effective for both PAT and non-PAT obviously then. Later when
we get ioremap() to default to strong UC we could drop ioremap_x86_uc() as we'd
only need it as transitory until then -- that is unless we want perhaps a strong
UC ioremap primitive which is always following strong UC when available regardless
of these default transitions.
The big issue I see here is simply the combinatorial issues, so I do think
its best to annotate these corner cases well and avoid them.
> > Would there be a use case for PAT systems? I wonder if we can wrap
> > this under some APIs to make it clean and hide this dirty thing
> > behind the scenes, it seems a fragile and error prone and my hope
> > would be that we won't need more specialization in this area for
> > PAT systems.
>
> One potential complication is kernel vs. userspace mmap. MTRR applies to
> the physical address, but PAT applies to the virtual address, so with
> the WC MTRR you get WC for userspace "for free" as well.
What is the performance impact of having the conversion being done by the
kernel? Has anyone done measurements? If significant can't the subsystem mmap()
cache the phys address for PAT? Shouldn't the TLB take care of those considerations
for us? If this is generally desirable shouldn't we just generalize the cache
for devices for O(1) access through a generic API?
Can the difference, other than a possible performance hit, implicate a userspace
visible change?
If the performance / userspace effect is neglibable then I'd expect the gains
from cleaner code / APIs to outweight the cons. After all the goal is to
streamline PAT when possible here.
> Also the
> userspace mmaps request will have the length of smem_len (at most), so
> it won't be the nice power of two in that case.
Does that length change implicate a userspace visible change?
> Also on PAT systems w/o a BIOS provided WC MTRR, the fbdev mmap seems
> to be total crap at the moment. IIRC I have a patch to fix things a bit...
Isn't that becuase of the lack of the ioremap_wc()'s? You seem to be
alternatively doing this with pgprot_writecombine(), more on this strategy
below though.
> From 4e6d70d223f35953c8a11a58cf3376a8a001fa4f Mon Sep 17 00:00:00 2001
> From: Ville Syrjala <syrjala@sci.fi>
> Date: Fri, 15 Apr 2011 04:02:43 +0300
> Subject: [PATCH] fb: writecombine fb
>
> ---
> drivers/video/fbdev/core/fbmem.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 0705d88..ecbde0e 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1396,6 +1396,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> unsigned long mmio_pgoff;
> unsigned long start;
> u32 len;
> + bool mmio = false;
>
> if (!info)
> return -ENODEV;
> @@ -1426,11 +1427,20 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> vma->vm_pgoff -= mmio_pgoff;
> start = info->fix.mmio_start;
> len = info->fix.mmio_len;
> + mmio = true;
> }
> mutex_unlock(&info->mm_lock);
>
> + if (!mmio) {
> + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +
> + if (!vm_iomap_memory(vma, start, len))
> + return 0;
> + }
> +
> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> - fb_pgprotect(file, vma, start);
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>
> return vm_iomap_memory(vma, start, len);
> }
>
> Perhaps it's time I tried to send that upstream properly :P
Lets assume drivers all have ioremap_wc() on the framebuffer, would the
the above not be needed then (disregarding the corner cases such as atyfb)?
If your goal is to generalize a place to make framebuffer WC instead of doing
that at mmap() why not do it at register_framebuffer() time and do it
only once? I suspect all this might also be easier to do and generalize
after this series.
So as we can see from this series there are tons of drivers that can safely
be moved to use ioremap_wc() already, provided there are no regressions with
the simple ioremap_wc() / arch_phys_wc_add() switch. There are only a few corner
cases to address after that. Addressing both of these *first* would simplify
the code and gramatically make it a bit more consistent while trying to avoid a
generalized regression. I believe a generalized solution is definitely in order
but we also should first address the corner cases.
So how about we:
a) convert all drivers over that are safe to convert to ioremap_wc() /
arch_phys_add_wc()
b) address all corner cases and try to avoid further combinatorial
issues
c) after a while push for reverting de33c442e
d) generalize a solution / for framebuffer
Ideally as I mentioned in the other thread with Bjorn we could even
have the WC be done further below for us but it was very unclear
if we could accomplish this due the definition of the PCI flags,
the way we'd use it and the way they could be integrated on hardware
by manufacturers. I think generalizing things under the frambuffer
code would be good intermediate step but I think we need to phase
this in in light of the corner cases, combinatorial issues with
PAT / non-PAT and eventual reverting goals of commit de33c442e
in order to generalize strong UC.
Luis
^ permalink raw reply
* Re: [PATCHv6 03/10] of: Add Solomon Systech vendor prefix.
From: Rob Herring @ 2015-04-01 20:33 UTC (permalink / raw)
To: Thomas Niederprüm
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Maxime Ripard,
kernel@pengutronix.de, Shawn Guo, Rob Herring,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1427826436-21783-4-git-send-email-niederp@physik.uni-kl.de>
On Tue, Mar 31, 2015 at 1:27 PM, Thomas Niederprüm
<niederp@physik.uni-kl.de> wrote:
> This patch adds the solomon prefix for Solomon Systech Limited.
>
> Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
In the future, please copy devicetree list.
Acked-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index d3f4809..933c8f5 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -169,6 +169,7 @@ sitronix Sitronix Technology Corporation
> smsc Standard Microsystems Corporation
> snps Synopsys, Inc.
> solidrun SolidRun
> +solomon Solomon Systech Limited
> sony Sony Corporation
> spansion Spansion Inc.
> sprd Spreadtrum Communications Inc.
> --
> 2.3.0
>
^ permalink raw reply
* Re: [PATCHv6 06/10] fbdev: ssd1307fb: Add support for SSD1305
From: Maxime Ripard @ 2015-04-01 16:01 UTC (permalink / raw)
To: Thomas Niederprüm
Cc: plagnioj, tomi.valkeinen, kernel, shawn.guo, robh+dt, linux-fbdev,
linux-kernel
In-Reply-To: <1427826436-21783-7-git-send-email-niederp@physik.uni-kl.de>
[-- Attachment #1: Type: text/plain, Size: 389 bytes --]
On Tue, Mar 31, 2015 at 08:27:12PM +0200, Thomas Niederprüm wrote:
> This patch adds support for the SSD1305 OLED controller.
>
> Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv6 04/10] fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
From: Maxime Ripard @ 2015-04-01 16:00 UTC (permalink / raw)
To: Thomas Niederprüm
Cc: plagnioj, tomi.valkeinen, kernel, shawn.guo, robh+dt, linux-fbdev,
linux-kernel
In-Reply-To: <1427826436-21783-5-git-send-email-niederp@physik.uni-kl.de>
[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]
On Tue, Mar 31, 2015 at 08:27:10PM +0200, Thomas Niederprüm wrote:
> The 130X controllers are very similar from the configuration point of view.
> The configuration registers for the SSD1305/6/7 are bit identical (except the
> the VHCOM register and the the default values for clock setup register). This
> patch unifies the init code of the controller and adds hardware specific
> properties to DT that are needed to correctly initialize the device.
>
> The SSD130X can be wired to the OLED panel in various ways. Even for the
> same controller this wiring can differ from one display module to another
> and can not be probed by software. The added DT properties reflect these
> hardware decisions of the display module manufacturer.
> The 'com-sequential', 'com-lrremap' and 'com-invdir' values define different
> possibilities for the COM signals pin configuration and readout direction
> of the video memory. The 'segment-no-remap' allows the inversion of the
> memory-to-pin mapping ultimately inverting the order of the controllers
> output pins. The 'prechargepX' values need to be adapted according to the
> capacitance of the OLEDs pixel cells.
>
> So far these hardware specific bits are hard coded in the init code, making
> the driver usable only for one certain wiring of the controller. This patch
> makes the driver usable with all possible hardware setups, given a valid hw
> description in DT. If these values are not set in DT the default values,
> as they are set in the ssd1307 init code right now, are used. This implies
> that without the corresponding DT property "segment-no-remap" the segment
> remap of the ssd130X controller gets activated. Even though this is not the
> default behaviour according to the datasheet it maintains backward
> compatibility with older DTBs.
>
> Note that the SSD1306 does not seem to be using the configuration written to
> the registers at all. Therefore this patch does not try to maintain these
> values without changes in DT. For reference an example is added to the DT
> bindings documentation that reproduces the configuration that is set in the
> current init code.
>
> Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
> Tested-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCHv6 10/10] fbdev: ssd1307fb: Add blank mode
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
This patch adds ssd1307fb_blank() to make the framebuffer capable
of blanking.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
---
drivers/video/fbdev/ssd1307fb.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 77efed7..8fc224c 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -235,6 +235,16 @@ static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf,
return count;
}
+static int ssd1307fb_blank(int blank_mode, struct fb_info *info)
+{
+ struct ssd1307fb_par *par = info->par;
+
+ if (blank_mode != FB_BLANK_UNBLANK)
+ return ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_OFF);
+ else
+ return ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
+}
+
static void ssd1307fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
{
struct ssd1307fb_par *par = info->par;
@@ -260,6 +270,7 @@ static struct fb_ops ssd1307fb_ops = {
.owner = THIS_MODULE,
.fb_read = fb_sys_read,
.fb_write = ssd1307fb_write,
+ .fb_blank = ssd1307fb_blank,
.fb_fillrect = ssd1307fb_fillrect,
.fb_copyarea = ssd1307fb_copyarea,
.fb_imageblit = ssd1307fb_imageblit,
--
2.3.0
^ permalink raw reply related
* [PATCHv6 09/10] fbdev: ssd1307fb: add backlight controls for setting the contrast
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
The backlight class is used to create userspace handles for
setting the OLED contrast.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/ssd1307fb.c | 58 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index b3dd417..da70bae 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2478,6 +2478,7 @@ config FB_SSD1307
select FB_SYS_IMAGEBLIT
select FB_DEFERRED_IO
select PWM
+ select FB_BACKLIGHT
help
This driver implements support for the Solomon SSD1307
OLED controller over I2C.
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 5f3d5a8..77efed7 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -7,6 +7,7 @@
*/
#include <linux/module.h>
+#include <linux/backlight.h>
#include <linux/kernel.h>
#include <linux/i2c.h>
#include <linux/fb.h>
@@ -38,6 +39,8 @@
#define SSD1307FB_SET_COM_PINS_CONFIG 0xda
#define SSD1307FB_SET_VCOMH 0xdb
+#define MAX_CONTRAST 255
+
#define REFRESHRATE 1
static u_int refreshrate = REFRESHRATE;
@@ -424,6 +427,43 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
return 0;
}
+static int ssd1307fb_update_bl(struct backlight_device *bdev)
+{
+ struct ssd1307fb_par *par = bl_get_data(bdev);
+ int ret;
+ int brightness = bdev->props.brightness;
+
+ par->contrast = brightness;
+
+ ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CONTRAST);
+ if (ret < 0)
+ return ret;
+ ret = ssd1307fb_write_cmd(par->client, par->contrast);
+ if (ret < 0)
+ return ret;
+ return 0;
+}
+
+static int ssd1307fb_get_brightness(struct backlight_device *bdev)
+{
+ struct ssd1307fb_par *par = bl_get_data(bdev);
+
+ return par->contrast;
+}
+
+static int ssd1307fb_check_fb(struct backlight_device *bdev,
+ struct fb_info *info)
+{
+ return (info->bl_dev = bdev);
+}
+
+static const struct backlight_ops ssd1307fb_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = ssd1307fb_update_bl,
+ .get_brightness = ssd1307fb_get_brightness,
+ .check_fb = ssd1307fb_check_fb,
+};
+
static struct ssd1307fb_deviceinfo ssd1307fb_ssd1305_deviceinfo = {
.default_vcomh = 0x34,
.default_dclk_div = 1,
@@ -464,6 +504,8 @@ MODULE_DEVICE_TABLE(of, ssd1307fb_of_match);
static int ssd1307fb_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
+ struct backlight_device *bl;
+ char bl_name[12];
struct fb_info *info;
struct device_node *node = client->dev.of_node;
struct fb_deferred_io *ssd1307fb_defio;
@@ -599,10 +641,24 @@ static int ssd1307fb_probe(struct i2c_client *client,
goto panel_init_error;
}
+ snprintf(bl_name, sizeof(bl_name), "ssd1307fb%d", info->node);
+ bl = backlight_device_register(bl_name, &client->dev, par,
+ &ssd1307fb_bl_ops, NULL);
+ bl->props.brightness = par->contrast;
+ bl->props.max_brightness = MAX_CONTRAST;
+ info->bl_dev = bl;
+
+ if (IS_ERR(bl)) {
+ dev_err(&client->dev, "unable to register backlight device: %ld\n",
+ PTR_ERR(bl));
+ goto bl_init_error;
+ }
dev_info(&client->dev, "fb%d: %s framebuffer device registered, using %d bytes of video memory\n", info->node, info->fix.id, vmem_size);
return 0;
+bl_init_error:
+ unregister_framebuffer(info);
panel_init_error:
if (par->device_info->need_pwm) {
pwm_disable(par->pwm);
@@ -622,6 +678,8 @@ static int ssd1307fb_remove(struct i2c_client *client)
ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_OFF);
+ backlight_device_unregister(info->bl_dev);
+
unregister_framebuffer(info);
if (par->device_info->need_pwm) {
pwm_disable(par->pwm);
--
2.3.0
^ permalink raw reply related
* [PATCHv6 08/10] fbdev: ssd1307fb: Turn off display on driver unload.
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
This patch turns off the display when the driver is unloaded.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/fbdev/ssd1307fb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 85eeda0..5f3d5a8 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -620,6 +620,8 @@ static int ssd1307fb_remove(struct i2c_client *client)
struct fb_info *info = i2c_get_clientdata(client);
struct ssd1307fb_par *par = info->par;
+ ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_OFF);
+
unregister_framebuffer(info);
if (par->device_info->need_pwm) {
pwm_disable(par->pwm);
--
2.3.0
^ permalink raw reply related
* [PATCHv6 07/10] fbdev: ssd1307fb: Add a module parameter to set the refresh rate
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
This patch adds the module parameter "refreshrate" to set delay for the
deferred io. The refresh rate is given in units of Hertz. The default
refresh rate is 1 Hz. The refresh rate set through the newly introduced
parameter applies to all instances of the driver and for now it is not
possible to change it individually.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/fbdev/ssd1307fb.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index f685d24..85eeda0 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -38,6 +38,11 @@
#define SSD1307FB_SET_COM_PINS_CONFIG 0xda
#define SSD1307FB_SET_VCOMH 0xdb
+#define REFRESHRATE 1
+
+static u_int refreshrate = REFRESHRATE;
+module_param(refreshrate, uint, 0);
+
struct ssd1307fb_par;
struct ssd1307fb_deviceinfo {
@@ -263,11 +268,6 @@ static void ssd1307fb_deferred_io(struct fb_info *info,
ssd1307fb_update_display(info->par);
}
-static struct fb_deferred_io ssd1307fb_defio = {
- .delay = HZ,
- .deferred_io = ssd1307fb_deferred_io,
-};
-
static int ssd1307fb_init(struct ssd1307fb_par *par)
{
int ret;
@@ -466,6 +466,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
{
struct fb_info *info;
struct device_node *node = client->dev.of_node;
+ struct fb_deferred_io *ssd1307fb_defio;
u32 vmem_size;
struct ssd1307fb_par *par;
u8 *vmem;
@@ -536,10 +537,20 @@ static int ssd1307fb_probe(struct i2c_client *client,
goto fb_alloc_error;
}
+ ssd1307fb_defio = devm_kzalloc(&client->dev, sizeof(struct fb_deferred_io), GFP_KERNEL);
+ if (!ssd1307fb_defio) {
+ dev_err(&client->dev, "Couldn't allocate deferred io.\n");
+ ret = -ENOMEM;
+ goto fb_alloc_error;
+ }
+
+ ssd1307fb_defio->delay = HZ / refreshrate;
+ ssd1307fb_defio->deferred_io = ssd1307fb_deferred_io;
+
info->fbops = &ssd1307fb_ops;
info->fix = ssd1307fb_fix;
info->fix.line_length = par->width / 8;
- info->fbdefio = &ssd1307fb_defio;
+ info->fbdefio = ssd1307fb_defio;
info->var = ssd1307fb_var;
info->var.xres = par->width;
--
2.3.0
^ permalink raw reply related
* [PATCHv6 06/10] fbdev: ssd1307fb: Add support for SSD1305
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
This patch adds support for the SSD1305 OLED controller.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
---
Documentation/devicetree/bindings/video/ssd1307fb.txt | 2 +-
drivers/video/fbdev/ssd1307fb.c | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt b/Documentation/devicetree/bindings/video/ssd1307fb.txt
index 635efa3..d1be78d 100644
--- a/Documentation/devicetree/bindings/video/ssd1307fb.txt
+++ b/Documentation/devicetree/bindings/video/ssd1307fb.txt
@@ -2,7 +2,7 @@
Required properties:
- compatible: Should be "solomon,<chip>fb-<bus>". The only supported bus for
- now is i2c, and the supported chips are ssd1306 and ssd1307.
+ now is i2c, and the supported chips are ssd1305, ssd1306 and ssd1307.
- reg: Should contain address of the controller on the I2C bus. Most likely
0x3c or 0x3d
- pwm: Should contain the pwm to use according to the OF device tree PWM
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 8667c77..f685d24 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -424,6 +424,12 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
return 0;
}
+static struct ssd1307fb_deviceinfo ssd1307fb_ssd1305_deviceinfo = {
+ .default_vcomh = 0x34,
+ .default_dclk_div = 1,
+ .default_dclk_frq = 7,
+};
+
static struct ssd1307fb_deviceinfo ssd1307fb_ssd1306_deviceinfo = {
.default_vcomh = 0x20,
.default_dclk_div = 1,
@@ -440,6 +446,10 @@ static struct ssd1307fb_deviceinfo ssd1307fb_ssd1307_deviceinfo = {
static const struct of_device_id ssd1307fb_of_match[] = {
{
+ .compatible = "solomon,ssd1305fb-i2c",
+ .data = (void *)&ssd1307fb_ssd1305_deviceinfo,
+ },
+ {
.compatible = "solomon,ssd1306fb-i2c",
.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
},
@@ -612,6 +622,7 @@ static int ssd1307fb_remove(struct i2c_client *client)
}
static const struct i2c_device_id ssd1307fb_i2c_id[] = {
+ { "ssd1305fb", 0 },
{ "ssd1306fb", 0 },
{ "ssd1307fb", 0 },
{ }
--
2.3.0
^ permalink raw reply related
* [PATCHv6 05/10] ARM: mxs: fix in tree users of ssd1306
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
This patch updates the in tree-users of the SSD1306 controller for using
the newly introduced DT properties.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
---
arch/arm/boot/dts/imx28-cfa10036.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/boot/dts/imx28-cfa10036.dts b/arch/arm/boot/dts/imx28-cfa10036.dts
index b04b6b8..570aa33 100644
--- a/arch/arm/boot/dts/imx28-cfa10036.dts
+++ b/arch/arm/boot/dts/imx28-cfa10036.dts
@@ -99,6 +99,9 @@
solomon,height = <32>;
solomon,width = <128>;
solomon,page-offset = <0>;
+ solomon,com-lrremap;
+ solomon,com-invdir;
+ solomon,com-offset = <32>;
};
};
--
2.3.0
^ permalink raw reply related
* [PATCHv6 04/10] fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
The 130X controllers are very similar from the configuration point of view.
The configuration registers for the SSD1305/6/7 are bit identical (except the
the VHCOM register and the the default values for clock setup register). This
patch unifies the init code of the controller and adds hardware specific
properties to DT that are needed to correctly initialize the device.
The SSD130X can be wired to the OLED panel in various ways. Even for the
same controller this wiring can differ from one display module to another
and can not be probed by software. The added DT properties reflect these
hardware decisions of the display module manufacturer.
The 'com-sequential', 'com-lrremap' and 'com-invdir' values define different
possibilities for the COM signals pin configuration and readout direction
of the video memory. The 'segment-no-remap' allows the inversion of the
memory-to-pin mapping ultimately inverting the order of the controllers
output pins. The 'prechargepX' values need to be adapted according to the
capacitance of the OLEDs pixel cells.
So far these hardware specific bits are hard coded in the init code, making
the driver usable only for one certain wiring of the controller. This patch
makes the driver usable with all possible hardware setups, given a valid hw
description in DT. If these values are not set in DT the default values,
as they are set in the ssd1307 init code right now, are used. This implies
that without the corresponding DT property "segment-no-remap" the segment
remap of the ssd130X controller gets activated. Even though this is not the
default behaviour according to the datasheet it maintains backward
compatibility with older DTBs.
Note that the SSD1306 does not seem to be using the configuration written to
the registers at all. Therefore this patch does not try to maintain these
values without changes in DT. For reference an example is added to the DT
bindings documentation that reproduces the configuration that is set in the
current init code.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Tested-by: Olliver Schinagl <o.schinagl@ultimaker.com>
---
.../devicetree/bindings/video/ssd1307fb.txt | 21 +++
drivers/video/fbdev/ssd1307fb.c | 177 ++++++++++++---------
2 files changed, 125 insertions(+), 73 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt b/Documentation/devicetree/bindings/video/ssd1307fb.txt
index 7a12542..635efa3 100644
--- a/Documentation/devicetree/bindings/video/ssd1307fb.txt
+++ b/Documentation/devicetree/bindings/video/ssd1307fb.txt
@@ -15,6 +15,16 @@ Required properties:
Optional properties:
- reset-active-low: Is the reset gpio is active on physical low?
+ - solomon,segment-no-remap: Display needs normal (non-inverted) data column
+ to segment mapping
+ - solomon,com-seq: Display uses sequential COM pin configuration
+ - solomon,com-lrremap: Display uses left-right COM pin remap
+ - solomon,com-invdir: Display uses inverted COM pin scan direction
+ - solomon,com-offset: Number of the COM pin wired to the first display line
+ - solomon,prechargep1: Length of deselect period (phase 1) in clock cycles.
+ - solomon,prechargep2: Length of precharge period (phase 2) in clock cycles.
+ This needs to be the higher, the higher the capacitance
+ of the OLED's pixels is
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
@@ -26,3 +36,14 @@ ssd1307: oled@3c {
reset-gpios = <&gpio2 7>;
reset-active-low;
};
+
+ssd1306: oled@3c {
+ compatible = "solomon,ssd1306fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ reset-gpios = <&gpio2 7>;
+ reset-active-low;
+ solomon,com-lrremap;
+ solomon,com-invdir;
+ solomon,com-offset = <32>;
+};
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 8d34c56..8667c77 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -40,20 +40,34 @@
struct ssd1307fb_par;
-struct ssd1307fb_ops {
- int (*init)(struct ssd1307fb_par *);
- int (*remove)(struct ssd1307fb_par *);
+struct ssd1307fb_deviceinfo {
+ u32 default_vcomh;
+ u32 default_dclk_div;
+ u32 default_dclk_frq;
+ int need_pwm;
+ int need_chargepump;
};
struct ssd1307fb_par {
+ u32 com_invdir;
+ u32 com_lrremap;
+ u32 com_offset;
+ u32 com_seq;
+ u32 contrast;
+ u32 dclk_div;
+ u32 dclk_frq;
+ struct ssd1307fb_deviceinfo *device_info;
struct i2c_client *client;
u32 height;
struct fb_info *info;
- struct ssd1307fb_ops *ops;
u32 page_offset;
+ u32 prechargep1;
+ u32 prechargep2;
struct pwm_device *pwm;
u32 pwm_period;
int reset;
+ u32 seg_remap;
+ u32 vcomh;
u32 width;
};
@@ -254,69 +268,46 @@ static struct fb_deferred_io ssd1307fb_defio = {
.deferred_io = ssd1307fb_deferred_io,
};
-static int ssd1307fb_ssd1307_init(struct ssd1307fb_par *par)
+static int ssd1307fb_init(struct ssd1307fb_par *par)
{
int ret;
+ u32 precharge, dclk, com_invdir, compins;
- par->pwm = pwm_get(&par->client->dev, NULL);
- if (IS_ERR(par->pwm)) {
- dev_err(&par->client->dev, "Could not get PWM from device tree!\n");
- return PTR_ERR(par->pwm);
- }
-
- par->pwm_period = pwm_get_period(par->pwm);
- /* Enable the PWM */
- pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
- pwm_enable(par->pwm);
-
- dev_dbg(&par->client->dev, "Using PWM%d with a %dns period.\n",
- par->pwm->pwm, par->pwm_period);
-
- /* Map column 127 of the OLED to segment 0 */
- ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SEG_REMAP_ON);
- if (ret < 0)
- return ret;
-
- /* Turn on the display */
- ret = ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
- if (ret < 0)
- return ret;
-
- return 0;
-}
+ if (par->device_info->need_pwm) {
+ par->pwm = pwm_get(&par->client->dev, NULL);
+ if (IS_ERR(par->pwm)) {
+ dev_err(&par->client->dev, "Could not get PWM from device tree!\n");
+ return PTR_ERR(par->pwm);
+ }
-static int ssd1307fb_ssd1307_remove(struct ssd1307fb_par *par)
-{
- pwm_disable(par->pwm);
- pwm_put(par->pwm);
- return 0;
-}
+ par->pwm_period = pwm_get_period(par->pwm);
+ /* Enable the PWM */
+ pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
+ pwm_enable(par->pwm);
-static struct ssd1307fb_ops ssd1307fb_ssd1307_ops = {
- .init = ssd1307fb_ssd1307_init,
- .remove = ssd1307fb_ssd1307_remove,
-};
-
-static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
-{
- int ret;
+ dev_dbg(&par->client->dev, "Using PWM%d with a %dns period.\n",
+ par->pwm->pwm, par->pwm_period);
+ };
/* Set initial contrast */
ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CONTRAST);
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0x7f);
- if (ret < 0)
- return ret;
-
- /* Set COM direction */
- ret = ssd1307fb_write_cmd(par->client, 0xc8);
+ ret = ssd1307fb_write_cmd(par->client, par->contrast);
if (ret < 0)
return ret;
/* Set segment re-map */
- ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SEG_REMAP_ON);
+ if (par->seg_remap) {
+ ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SEG_REMAP_ON);
+ if (ret < 0)
+ return ret;
+ };
+
+ /* Set COM direction */
+ com_invdir = 0xc0 | (par->com_invdir & 0x1) << 3;
+ ret = ssd1307fb_write_cmd(par->client, com_invdir);
if (ret < 0)
return ret;
@@ -334,7 +325,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0x20);
+ ret = ssd1307fb_write_cmd(par->client, par->com_offset);
if (ret < 0)
return ret;
@@ -343,7 +334,8 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0xf0);
+ dclk = ((par->dclk_div - 1) & 0xf) | (par->dclk_frq & 0xf) << 4;
+ ret = ssd1307fb_write_cmd(par->client, dclk);
if (ret < 0)
return ret;
@@ -352,7 +344,8 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0x22);
+ precharge = (par->prechargep1 & 0xf) | (par->prechargep2 & 0xf) << 4;
+ ret = ssd1307fb_write_cmd(par->client, precharge);
if (ret < 0)
return ret;
@@ -361,7 +354,9 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0x22);
+ compins = 0x02 | (!par->com_seq & 0x1) << 4
+ | (par->com_lrremap & 0x1) << 5;
+ ret = ssd1307fb_write_cmd(par->client, compins);
if (ret < 0)
return ret;
@@ -370,7 +365,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0x49);
+ ret = ssd1307fb_write_cmd(par->client, par->vcomh);
if (ret < 0)
return ret;
@@ -379,7 +374,8 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
- ret = ssd1307fb_write_cmd(par->client, 0x14);
+ ret = ssd1307fb_write_cmd(par->client,
+ (par->device_info->need_chargepump & 0x1 << 2) & 0x14);
if (ret < 0)
return ret;
@@ -393,6 +389,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
+ /* Set column range */
ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_COL_RANGE);
if (ret < 0)
return ret;
@@ -405,6 +402,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
if (ret < 0)
return ret;
+ /* Set page range */
ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_PAGE_RANGE);
if (ret < 0)
return ret;
@@ -426,18 +424,28 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
return 0;
}
-static struct ssd1307fb_ops ssd1307fb_ssd1306_ops = {
- .init = ssd1307fb_ssd1306_init,
+static struct ssd1307fb_deviceinfo ssd1307fb_ssd1306_deviceinfo = {
+ .default_vcomh = 0x20,
+ .default_dclk_div = 1,
+ .default_dclk_frq = 8,
+ .need_chargepump = 1,
+};
+
+static struct ssd1307fb_deviceinfo ssd1307fb_ssd1307_deviceinfo = {
+ .default_vcomh = 0x20,
+ .default_dclk_div = 2,
+ .default_dclk_frq = 12,
+ .need_pwm = 1,
};
static const struct of_device_id ssd1307fb_of_match[] = {
{
.compatible = "solomon,ssd1306fb-i2c",
- .data = (void *)&ssd1307fb_ssd1306_ops,
+ .data = (void *)&ssd1307fb_ssd1306_deviceinfo,
},
{
.compatible = "solomon,ssd1307fb-i2c",
- .data = (void *)&ssd1307fb_ssd1307_ops,
+ .data = (void *)&ssd1307fb_ssd1307_deviceinfo,
},
{},
};
@@ -468,8 +476,8 @@ static int ssd1307fb_probe(struct i2c_client *client,
par->info = info;
par->client = client;
- par->ops = (struct ssd1307fb_ops *)of_match_device(ssd1307fb_of_match,
- &client->dev)->data;
+ par->device_info = (struct ssd1307fb_deviceinfo *)of_match_device(
+ ssd1307fb_of_match, &client->dev)->data;
par->reset = of_get_named_gpio(client->dev.of_node,
"reset-gpios", 0);
@@ -487,6 +495,27 @@ static int ssd1307fb_probe(struct i2c_client *client,
if (of_property_read_u32(node, "solomon,page-offset", &par->page_offset))
par->page_offset = 1;
+ if (of_property_read_u32(node, "solomon,com-offset", &par->com_offset))
+ par->com_offset = 0;
+
+ if (of_property_read_u32(node, "solomon,prechargep1", &par->prechargep1))
+ par->prechargep1 = 2;
+
+ if (of_property_read_u32(node, "solomon,prechargep2", &par->prechargep2))
+ par->prechargep2 = 2;
+
+ par->seg_remap = !of_property_read_bool(node, "solomon,segment-no-remap");
+ par->com_seq = of_property_read_bool(node, "solomon,com-seq");
+ par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
+ par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
+
+ par->contrast = 127;
+ par->vcomh = par->device_info->default_vcomh;
+
+ /* Setup display timing */
+ par->dclk_div = par->device_info->default_dclk_div;
+ par->dclk_frq = par->device_info->default_dclk_frq;
+
vmem_size = par->width * par->height / 8;
vmem = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
@@ -539,11 +568,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
gpio_set_value(par->reset, 1);
udelay(4);
- if (par->ops->init) {
- ret = par->ops->init(par);
- if (ret)
- goto reset_oled_error;
- }
+ ret = ssd1307fb_init(par);
+ if (ret)
+ goto reset_oled_error;
ret = register_framebuffer(info);
if (ret) {
@@ -556,8 +583,10 @@ static int ssd1307fb_probe(struct i2c_client *client,
return 0;
panel_init_error:
- if (par->ops->remove)
- par->ops->remove(par);
+ if (par->device_info->need_pwm) {
+ pwm_disable(par->pwm);
+ pwm_put(par->pwm);
+ };
reset_oled_error:
fb_deferred_io_cleanup(info);
fb_alloc_error:
@@ -571,8 +600,10 @@ static int ssd1307fb_remove(struct i2c_client *client)
struct ssd1307fb_par *par = info->par;
unregister_framebuffer(info);
- if (par->ops->remove)
- par->ops->remove(par);
+ if (par->device_info->need_pwm) {
+ pwm_disable(par->pwm);
+ pwm_put(par->pwm);
+ };
fb_deferred_io_cleanup(info);
__free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));
framebuffer_release(info);
--
2.3.0
^ permalink raw reply related
* [PATCHv6 03/10] of: Add Solomon Systech vendor prefix.
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
This patch adds the solomon prefix for Solomon Systech Limited.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
---
Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index d3f4809..933c8f5 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -169,6 +169,7 @@ sitronix Sitronix Technology Corporation
smsc Standard Microsystems Corporation
snps Synopsys, Inc.
solidrun SolidRun
+solomon Solomon Systech Limited
sony Sony Corporation
spansion Spansion Inc.
sprd Spreadtrum Communications Inc.
--
2.3.0
^ permalink raw reply related
* [PATCHv6 02/10] fbdev: ssd1307fb: Allocate page aligned video memory.
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
Currently the videomemory is allocated by kmalloc, making it a memory
region that is not necessarily page aligend. This leads to problems
upon mmap call, where the video memory's address gets aligned to the
next page boundary. The result is that the userspace program that issued
the mmap call is not able to access the video memory from the start to
the next page boundary.
This patch changes the allocation of the video memory to use
__get_free_pages() in order to obtain memory that is aligned
to page boundaries.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/fbdev/ssd1307fb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 61e0ce8..8d34c56 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -489,7 +489,8 @@ static int ssd1307fb_probe(struct i2c_client *client,
vmem_size = par->width * par->height / 8;
- vmem = devm_kzalloc(&client->dev, vmem_size, GFP_KERNEL);
+ vmem = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+ get_order(vmem_size));
if (!vmem) {
dev_err(&client->dev, "Couldn't allocate graphical memory.\n");
ret = -ENOMEM;
@@ -573,6 +574,7 @@ static int ssd1307fb_remove(struct i2c_client *client)
if (par->ops->remove)
par->ops->remove(par);
fb_deferred_io_cleanup(info);
+ __free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));
framebuffer_release(info);
return 0;
--
2.3.0
^ permalink raw reply related
* [PATCHv6 01/10] fbdev: ssd1307fb: fix memory address smem_start.
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1427826436-21783-1-git-send-email-niederp@physik.uni-kl.de>
the smem_start pointer of the framebuffer info struct needs to hold the
physical address rather than the logical address. Right now the logical
address returned by kmalloc is stored. This patch converts this address
to a physical address and thus fixes a driver crash on mmaping the
framebuffer memory due to an access to the wrong memory address.
Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/fbdev/ssd1307fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index f7ed6d9..61e0ce8 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -515,7 +515,7 @@ static int ssd1307fb_probe(struct i2c_client *client,
info->var.blue.offset = 0;
info->screen_base = (u8 __force __iomem *)vmem;
- info->fix.smem_start = (unsigned long)vmem;
+ info->fix.smem_start = __pa(vmem);
info->fix.smem_len = vmem_size;
fb_deferred_io_init(info);
--
2.3.0
^ permalink raw reply related
* [PATCHv6 00/10] Cleanup and add support for SSD1305
From: Thomas Niederprüm @ 2015-03-31 18:27 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, maxime.ripard, kernel, shawn.guo,
robh+dt
Cc: linux-fbdev, linux-kernel, Thomas Niederprüm
In-Reply-To: <1423261694-5939-1-git-send-email-niederp@physik.uni-kl.de>
Hi,
this patch series is the result of making the ssd1307fb driver work with
a Newhaven OLED display using the Solomon SSD1305 controller. To achieve
this the intialization code for the SSD1306 and the SSD1307 is merged
and based on DT configuration to reflect the various possible wirings
of the SSD130X controller (04/10). Based on these changes it is straight
forward to add support for the SSD1305 controller (06/10).
While working on the driver I realized that it was not possible to
correctly mmap the video memory from userspace since the address handed
to the userspace app is a logical one where it should be a physical one.
Patch 01/10 fixes this. Furthermore the memory reserved by kzalloc is
not page aligned while the address handed to userspace is aligned to the
next page frame. This problem is fixed by using __get_free_pages() in 02/10.
Furthermore a module parameter is added to set the delay for the
deferred io update (07/10). Also the backlight class is implemented to make
the contrast setting available in userspace (09/10).
changes since v1 (thanks to Maxime for the feedback):
- dedicated patch for fixing smem_start address
- remove page reserve upon vmalloc
- remove return value check upon display turn-off at module unload
- use a module parameter refreshrate rather than delaydivider
- allocate fbdefio dynamically
- use sysfs_create_groups to create sysfs entries
- remove contrast, vhcom and dclk properties from DT since they are
not part of hw description. The contrast module parameter was added
to set contrast at load time. vhcom and dclk stays at it's default
values for now.
- add new DT properties to in tree users of ssd130X
- rebased to apply on top of linux-next
changes since v2 (thanks to Maxime again):
- free memory allocated by vmalloc on driver unload
- set default values in the init code to the ones of the existing ssd1307
init code
- added two ACKs (Maxime Ripard)
changes since v3:
- use backlight class rather than dedicated sysfs files to set the
contrast (Thanks to Tomi Valkeinen)
- remove [PATCHv3 08/10] fbdev: ssd1307fb: Add module parameter bitsperpixel
- add new patch to blank the display (unreviewed)
- allocate video memory through __get_free_pages() rather than vmalloc
(Thanks to Geert Uytterhoeven)
- minor rewordings of the commit messages
changes since v4 (thanks to Maxime):
- added two ACKs (Maxime Ripard)
- fixed typo: s/REFRASHRATE/REFRESHRATE
- updated the documentation to make clear the unit of com-offset
- move addition of the module parameter contrast to a separate patch (09/11)
- fix indentation errors
- get rid of device_id in the device_info struct
changes since v5:
- remove [PATCHv5 09/11] fbdev: ssd1307fb: Add module parameter to set
the initial contrast
- remove unnecessary variable initialization in the deviceinfo struct as
pointed out by Maxime
- make the variable dclk_div hold the actual divider rather than the
corresponding bit value
- remove ret variable in ssd1307fb_blank() (thanks to Olliver Schinagl)
- fixed bit mask for com_invdir as indicated by Olliver
- change initialization of the charge pump as suggested by Olliver
- change the default value for prechargep2 to 0x2 (thanks to Olliver)
- change DT property name "com-sequential" to "com-seq"
- added one ACK (Maxime Ripard)
- added Tested-by Olliver Schinagl
Thomas Niederprüm (10):
fbdev: ssd1307fb: fix memory address smem_start.
fbdev: ssd1307fb: Allocate page aligned video memory.
of: Add Solomon Systech vendor prefix.
fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
ARM: mxs: fix in tree users of ssd1306
fbdev: ssd1307fb: Add support for SSD1305
fbdev: ssd1307fb: Add a module parameter to set the refresh rate
fbdev: ssd1307fb: Turn off display on driver unload.
fbdev: ssd1307fb: add backlight controls for setting the contrast
fbdev: ssd1307fb: Add blank mode
.../devicetree/bindings/vendor-prefixes.txt | 1 +
.../devicetree/bindings/video/ssd1307fb.txt | 23 +-
arch/arm/boot/dts/imx28-cfa10036.dts | 3 +
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/ssd1307fb.c | 288 +++++++++++++++------
5 files changed, 234 insertions(+), 82 deletions(-)
--
2.3.0
^ permalink raw reply
* Re: [RFT PATCHv2] drm/exynos: Enable DP clock to fix display on Exynos5250 and other
From: Andreas Färber @ 2015-03-31 11:42 UTC (permalink / raw)
To: Javier Martinez Canillas, Krzysztof Kozlowski, Jingoo Han,
Inki Dae, Joonyoung Shim, Seung-Woo Kim, Kyungmin Park,
David Airlie, Kukjin Kim, dri-devel, linux-arm-kernel,
linux-samsung-soc, linux-kernel, linux-fbdev
Cc: Marek Szyprowski, Andrzej Hajda, stable
In-Reply-To: <5515ADB6.6050204@collabora.co.uk>
Am 27.03.2015 um 20:21 schrieb Javier Martinez Canillas:
> Hello Krzysztof,
>
> On 03/27/2015 05:08 PM, Krzysztof Kozlowski wrote:
>> After adding display power domain for Exynos5250 in commit
>> 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250") the
>> display on Chromebook Snow and others stopped working after boot.
>>
>> The reason for this suggested Andrzej Hajda: the DP clock was disabled.
>> This clock is required by Display Port and is enabled by bootloader.
>> However when FIMD driver probing was deferred, the display power domain
>> was turned off. This effectively reset the value of DP clock enable
>> register.
>>
>> When exynos-dp is later probed, the clock is not enabled and display is
>> not properly configured:
>>
>> exynos-dp 145b0000.dp-controller: Timeout of video streamclk ok
>> exynos-dp 145b0000.dp-controller: unable to config video
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> Fixes: 2d2c9a8d0a4f ("ARM: dts: add display power domain for exynos5250")
>> Cc: <stable@vger.kernel.org>
>>
>> ---
>>
>> This should fix issue reported by Javier [1][2].
>>
>
> I tested this patch and does indeed solves both issues I reported
> The exynos-dp probe deferral does not make the display to not be
> working and also disabling and enabling the display with:
>
> with /sys/devices/platform/exynos-drm/graphics/fb0/blank works.
>
> Thanks a lot for fixing this issue.
>
> On an Exynos5250 Snow Chromebook:
>
> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Seems to fix Spring Chromebook as well,
Tested-by: Andreas Färber <afaerber@suse.de>
Thanks a lot,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH 6/8] mtd: replace CONFIG_8xx by CONFIG_PPC_8xx
From: Brian Norris @ 2015-03-31 0:55 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-fbdev, linux-serial, user-mode-linux-devel, David Woodhouse,
linux-kernel, linux-mtd, user-mode-linux-user, netdev,
linuxppc-dev
In-Reply-To: <20150312152404.D6C391A2421@localhost.localdomain>
On Thu, Mar 12, 2015 at 04:24:04PM +0100, Christophe Leroy wrote:
> Two config options exist to define powerpc MPC8xx:
> * CONFIG_PPC_8xx
> * CONFIG_8xx
> In addition, CONFIG_PPC_8xx also defines CONFIG_CPM1 as
> communication co-processor
>
> arch/powerpc/platforms/Kconfig.cputype has contained the following
> comment about CONFIG_8xx item for some years:
> "# this is temp to handle compat with arch=ppc"
>
> It looks like not many places still have that old CONFIG_8xx used,
> so it is likely to be a good time to get rid of it completely ?
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
I'm not sure if the consistency aspect is worked out for all symbols
(Geert's comment on the cover letter), but this one looks fine.
Applied this patch to l2-mtd.git.
Brian
^ permalink raw reply
* [PATCH] sisfb: Replace creative use of the ternary operator
From: Ben Hutchings @ 2015-03-29 22:28 UTC (permalink / raw)
To: linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]
GETBITSTR() and related macros are defined in such a way that two of
the macro arguments must be pairs of integers joined with a ':',
selected between by prepending '1?' or '0?'. This is rather clever
but not very readable. Redefine the macros and change the users so
that each integer is a separate argument.
While we're at it, add a BUILD_BUG_ON_ZERO() to check that the
redundant first part of the 'to' argument is consistent with the
other arguments.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
drivers/video/fbdev/sis/init.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/video/fbdev/sis/init.c b/drivers/video/fbdev/sis/init.c
index dfe3eb7..d0c2e72 100644
--- a/drivers/video/fbdev/sis/init.c
+++ b/drivers/video/fbdev/sis/init.c
@@ -3320,11 +3320,9 @@ SiSSetMode(struct SiS_Private *SiS_Pr, unsigned short ModeNo)
return true;
}
-#ifndef GETBITSTR
-#define GENBITSMASK(mask) GENMASK(1?mask,0?mask)
-#define GETBITS(var,mask) (((var) & GENBITSMASK(mask)) >> (0?mask))
-#define GETBITSTR(val,from,to) ((GETBITS(val,from)) << (0?to))
-#endif
+#define GETBITSTR(val, fromhigh, fromlow, tohigh, tolow) \
+ (((((val) & GENMASK(fromhigh, fromlow)) >> (fromlow)) << (tolow)) + \
+ BUILD_BUG_ON_ZERO((fromhigh) - (fromlow) != (tohigh) - (tolow)))
void
SiS_CalcCRRegisters(struct SiS_Private *SiS_Pr, int depth)
@@ -3363,23 +3361,23 @@ SiS_CalcCRRegisters(struct SiS_Private *SiS_Pr, int depth)
SiS_Pr->CCRT1CRTC[12] = (SiS_Pr->CVBlankEnd - 1) & 0xFF; /* CR16 */
SiS_Pr->CCRT1CRTC[13] = /* SRA */
- GETBITSTR((SiS_Pr->CVTotal -2), 10:10, 0:0) |
- GETBITSTR((SiS_Pr->CVDisplay -1), 10:10, 1:1) |
- GETBITSTR((SiS_Pr->CVBlankStart-1), 10:10, 2:2) |
- GETBITSTR((SiS_Pr->CVSyncStart -x), 10:10, 3:3) |
- GETBITSTR((SiS_Pr->CVBlankEnd -1), 8:8, 4:4) |
- GETBITSTR((SiS_Pr->CVSyncEnd ), 4:4, 5:5) ;
+ GETBITSTR((SiS_Pr->CVTotal -2), 10, 10, 0, 0) |
+ GETBITSTR((SiS_Pr->CVDisplay -1), 10, 10, 1, 1) |
+ GETBITSTR((SiS_Pr->CVBlankStart-1), 10, 10, 2, 2) |
+ GETBITSTR((SiS_Pr->CVSyncStart -x), 10, 10, 3, 3) |
+ GETBITSTR((SiS_Pr->CVBlankEnd -1), 8, 8, 4, 4) |
+ GETBITSTR((SiS_Pr->CVSyncEnd ), 4, 4, 5, 5) ;
SiS_Pr->CCRT1CRTC[14] = /* SRB */
- GETBITSTR((SiS_Pr->CHTotal >> 3) - 5, 9:8, 1:0) |
- GETBITSTR((SiS_Pr->CHDisplay >> 3) - 1, 9:8, 3:2) |
- GETBITSTR((SiS_Pr->CHBlankStart >> 3) - 1, 9:8, 5:4) |
- GETBITSTR((SiS_Pr->CHSyncStart >> 3) + 3, 9:8, 7:6) ;
+ GETBITSTR((SiS_Pr->CHTotal >> 3) - 5, 9, 8, 1, 0) |
+ GETBITSTR((SiS_Pr->CHDisplay >> 3) - 1, 9, 8, 3, 2) |
+ GETBITSTR((SiS_Pr->CHBlankStart >> 3) - 1, 9, 8, 5, 4) |
+ GETBITSTR((SiS_Pr->CHSyncStart >> 3) + 3, 9, 8, 7, 6) ;
SiS_Pr->CCRT1CRTC[15] = /* SRC */
- GETBITSTR((SiS_Pr->CHBlankEnd >> 3) - 1, 7:6, 1:0) |
- GETBITSTR((SiS_Pr->CHSyncEnd >> 3) + 3, 5:5, 2:2) ;
+ GETBITSTR((SiS_Pr->CHBlankEnd >> 3) - 1, 7, 6, 1, 0) |
+ GETBITSTR((SiS_Pr->CHSyncEnd >> 3) + 3, 5, 5, 2, 2) ;
}
void
--
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply related
* [PATCH 73/86] fbdev/riva: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/riva/nv_driver.c | 2 +-
drivers/video/fbdev/riva/riva_hw.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/riva/nv_driver.c b/drivers/video/fbdev/riva/nv_driver.c
index f3694cf..f597ee5 100644
--- a/drivers/video/fbdev/riva/nv_driver.c
+++ b/drivers/video/fbdev/riva/nv_driver.c
@@ -36,7 +36,7 @@
#include <linux/delay.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include "nv_type.h"
#include "rivafb.h"
#include "nvreg.h"
diff --git a/drivers/video/fbdev/riva/riva_hw.c b/drivers/video/fbdev/riva/riva_hw.c
index 8bdf37f..0d399f6 100644
--- a/drivers/video/fbdev/riva/riva_hw.c
+++ b/drivers/video/fbdev/riva/riva_hw.c
@@ -48,7 +48,7 @@
#include <linux/kernel.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include "riva_hw.h"
#include "riva_tbl.h"
#include "nv_type.h"
--
MST
^ permalink raw reply related
* [PATCH 72/86] fbdev/i810: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Antonino Daplas, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/i810/i810_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/i810/i810_main.c b/drivers/video/fbdev/i810/i810_main.c
index bb674e4..c4e13bb 100644
--- a/drivers/video/fbdev/i810/i810_main.c
+++ b/drivers/video/fbdev/i810/i810_main.c
@@ -37,7 +37,7 @@
#include <linux/fb.h>
#include <linux/init.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/resource.h>
#include <linux/unistd.h>
#include <linux/console.h>
--
MST
^ permalink raw reply related
* [PATCH 71/86] fbdev/i740fb: use uapi/linux/pci_ids.h directly
From: Michael S. Tsirkin @ 2015-03-29 13:42 UTC (permalink / raw)
To: linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Bjorn Helgaas,
Laurent Pinchart, Benoit Taine, linux-fbdev
In-Reply-To: <1427635734-24786-1-git-send-email-mst@redhat.com>
Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/video/fbdev/i740fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c
index a2b4204..abad615 100644
--- a/drivers/video/fbdev/i740fb.c
+++ b/drivers/video/fbdev/i740fb.c
@@ -21,7 +21,7 @@
#include <linux/fb.h>
#include <linux/init.h>
#include <linux/pci.h>
-#include <linux/pci_ids.h>
+#include <uapi/linux/pci_ids.h>
#include <linux/i2c.h>
#include <linux/i2c-algo-bit.h>
#include <linux/console.h>
--
MST
^ 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