| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Aug 2014 09:41:26 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Thor Thayer <tthayer@...nsource.altera.com>
Cc: robherring2@...il.com, pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
rob@...dley.net, linux@....linux.org.uk, atull@...era.com,
delicious.quinoa@...il.com, dinguyen@...era.com,
dougthompson@...ssion.com, grant.likely@...aro.org, bp@...en8.de,
sameo@...ux.intel.com, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org, linux-edac@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
tthayer.linux@...il.com, Alan Tull <atull@...nsource.altera.com>
Subject: Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
On Fri, 01 Aug 2014, Thor Thayer wrote:
> On 08/01/2014 03:13 AM, Lee Jones wrote:
> >On Thu, 31 Jul 2014, Thor Thayer wrote:
> >>On 07/31/2014 03:26 AM, Lee Jones wrote:
> >>>On Wed, 30 Jul 2014, tthayer@...nsource.altera.com wrote:
> >>>
> >>>>From: Thor Thayer <tthayer@...nsource.altera.com>
> >>>>
> >>>>Add a simple MFD for the Altera SDRAM Controller.
> >>>>
> >>>>Signed-off-by: Alan Tull <atull@...nsource.altera.com>
> >>>>Signed-off-by: Thor Thayer <tthayer@...nsource.altera.com>
> >>>>---
> >>>>v1-8: The MFD implementation was not included in the original series.
> >>>>
> >>>>v9: New MFD implementation.
> >>>>---
> >>>> MAINTAINERS | 5 ++
> >>>> drivers/mfd/Kconfig | 7 ++
> >>>> drivers/mfd/Makefile | 1 +
> >>>> drivers/mfd/altera-sdr.c | 162 ++++++++++++++++++++++++++++++++++++++++
> >>>> include/linux/mfd/altera-sdr.h | 102 +++++++++++++++++++++++++
> >>>> 5 files changed, 277 insertions(+)
> >>>> create mode 100644 drivers/mfd/altera-sdr.c
> >>>> create mode 100644 include/linux/mfd/altera-sdr.h
[...]
> >>>>+ return readl(sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_readl);
> >>>>+
> >>>>+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
> >>>>+{
> >>>>+ writel(value, sdr->reg_base + reg_offset);
> >>>>+}
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_writel);
> >>>Why are you abstracting these?
You still didn't answer this?
> >>>Might be better to use Regmap even.
> >>regmap seems unnecessarily complex for what we're doing which is why
> >>this method was chosen.
> >>
> >>Future drivers will access different sets of registers in the
> >>device. These drivers won't share bitfields in the same register so
> >>the MFD seemed like the best solution. Originally we implemented
> >>this using syscon but that seems to be frowned upon so we changed to
> >>using a MFD.
> >Why was the use of syscon frowned upon? Can you link me to the
> >thread? Writing directly to the registers sounds to me a lot worse
> >than using infrastructure which was designed for these kinds of
> >accesses.
> >
> >If you do choose to fiddle with the registers in this manner, is there
> >any reason why you're calling back into here, rather than using
> >readl() and writel() directly?
> >
> We'd prefer to use syscon and that is what we started with. If you'd
> like to be our advocate, I will return to that because it was pretty
> clean. My primary concern is to get it upstreamed and if it is MFD
> then I'll make the changes.
>
> Here are the threads.
> http://marc.info/?l=linux-kernel&m=140128791902800&w=2
> and
> http://article.gmane.org/gmane.linux.kernel/1679601
Syscon looks the most appropriate to me.
[...]
> >>>>+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
> >>>Should this really be done in here? Isn't this an SDRAM function?
> >>>
> >>This register is part of the SDRAM controller and size information
> >>may be required by the other drivers that share this memory
> >>area/need SDRAM information.
> >Then export a function from the SDRAM driver, not from here.
> We don't have an SDRAM driver. Although I could put this in the
> EDAC driver it would be lost to anyone else wanting this
> functionality so this seemed to be the logical place.
Why can't you export it from the EDAC driver?
[...]
> >>>>+static int __init altera_sdr_init(void)
> >>>>+{
> >>>>+ return platform_driver_register(&altera_sdr_driver);
> >>>>+}
> >>>>+postcore_initcall(altera_sdr_init);
> >>>Why was this chosen?
> >>We want this to happen pretty early.
> >If you _need_ this is happen early, core_initcall() is more commonly
> >used, but _why_ do you need it to happen this early?
> The syscon driver used this designation. After talking with Alan,
> this could be changed to a core_initcall(). However, it could also
> be a subsys_initcall which seems to be more common in the MFD
> drivers.
That doesn't answer my question still.
What is the reason, requirement, need for this driver to be probed so
early during the boot process?
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists