On Thu, 31 Jul 2014, Thor Thayer wrote:Hi Lee.
On 07/31/2014 03:26 AM, Lee Jones wrote:[...]
On Wed, 30 Jul 2014, tthayer@xxxxxxxxxxxxxxxxxxxxx wrote:
From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>
Add a simple MFD for the Altera SDRAM Controller.
Signed-off-by: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>
---
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
This is the long(ish) version. Many programs have the Copyright lineHi. This seems to be the shorter version of the license agreement+++ b/drivers/mfd/altera-sdr.cCan you use the shorter version of the licence?
@@ -0,0 +1,162 @@
+/*
+ * SDRAM Controller (SDR) MFD
+ *
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
and is fairly common in the kernel, right?
and the "This program is free software;" line. It'll also be a good
idea to keep the link to the full licence.
[...]
The other drivers are waiting on this file as a pre-requisite.Okay. Do you know when they'll be upstreamed?There will be an FPGA bridge and a power control driver that will+ {What other devices will there be?
+ .name = "altr_sdram_edac",
+ .of_compatible = "altr,sdram-edac",
need access to the SDR Controller registers.
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.Why was the use of syscon frowned upon? Can you link me to theregmap seems unnecessarily complex for what we're doing which is why+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)Why are you abstracting these?
+{
+ 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);
Might be better to use Regmap even.
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.
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?
Whoops. Good catch.u32s cant be < 0. The 'u' means 'unsigned'.OK. I will change this.+u32 altera_sdr_mem_size(struct altera_sdr *sdr)Weird that size is on its own. Either place on a single line or
+{
+ u32 size;
+ u32 read_reg, row, bank, col, cs, width;
separate them all.
+ read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
+ if (read_reg < 0)
+ return 0;
+
+ width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
+ if (width < 0)
+ return 0;
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.Then export a function from the SDRAM driver, not from here.OK. I will fix this.+ col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>These would probably be better as macros.
+ SDR_DRAMADDRW_COLBITS_LSB;
+ row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
+ SDR_DRAMADDRW_ROWBITS_LSB;
+ bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
+ SDR_DRAMADDRW_BANKBITS_LSB;
+ cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
+ SDR_DRAMADDRW_CSBITS_LSB;
This register is part of the SDRAM controller and size information+ /* Correct for ECC as its not addressible */Should this really be done in here? Isn't this an SDRAM function?
+ if (width == SDR_DRAMIFWIDTH_32B_ECC)
+ width = 32;
+ if (width == SDR_DRAMIFWIDTH_16B_ECC)
+ width = 16;
+
+ /* calculate the SDRAM size base on this info */
+ size = 1 << (row + bank + col);
+ size = size * cs * (width / 8);
+ return size;
+}
+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);
may be required by the other drivers that share this memory
area/need SDRAM information.
OK. I will remove it.
[...]
It's a problem because it's unused, superfluous bumph. :)We don't strictly need it because we are driven by the device tree.+static const struct platform_device_id altera_sdr_ids[] = {What's this for?
+ { "altera_sdr", },
+ { }
+};
It can be removed it if is a problem but I'm not clear why it is a
problem.
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.
[...]
If you _need_ this is happen early, core_initcall() is more commonlyWe want this to happen pretty early.+static int __init altera_sdr_init(void)Why was this chosen?
+{
+ return platform_driver_register(&altera_sdr_driver);
+}
+postcore_initcall(altera_sdr_init);
used, but _why_ do you need it to happen this early?
OK. This is a moot point if we decide to go back to using syscon.If you read COPYING, you only require the Copyright line and a link toSame as above. This is the shorter version that we've been using at Altera.+/*Use the short version.
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
the full licence. In this case I'll be happy if you left in the "This
program is free software;" line as well, but remove the paragraph
below it.
[...]
I'm not sure I believe that. Regmap is as complex or as simple as youSame as above. This seems to be more complex than we need.+/* Register access API */Regmap?
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);
need.
Thanks for your comments and for reviewing!You're welcome. OOI, do you own a hammer? ;)
Thor