Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
From: Ramuthevar, Vadivel MuruganX
Date: Mon May 04 2020 - 05:17:15 EST
Hi Boris,
On 4/5/2020 4:58 pm, Boris Brezillon wrote:
On Mon, 4 May 2020 16:50:08 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
Hi Boris,
On 4/5/2020 3:17 pm, Boris Brezillon wrote:
On Mon, 4 May 2020 15:15:08 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
Hi Boris,
Thank you very much for the prompt review and suggestions...
On 4/5/2020 3:08 pm, Boris Brezillon wrote:
On Mon, 4 May 2020 10:02:35 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
Hi Boris,
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon<boris.brezillon@xxxxxxxxxxxxx> wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx> wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE1400000 NAND_BASE_PHY address.
Then why didn't you tell me they didn't match when I suggested to pass
sorry, because you keep asking nand_pa after that only I realized that.
nand_pa? So now the question is, what does this address represent?
EBU-MODULE
_________ _______________________
| | | |NAND CTRL |
| FPI BUS |==>| CS0(0x174) | 0xE100 ( 0xE14/0xE1C) NAND_PHY_BASE
|_________| |_CS1(0x17C)_|__________ |
EBU_CONRTROLLER_BASE : 0xE0F0_0000
HSNAND_BASE: 0xE100_0000
NAND_CS0: 0xE140_0000
NAND_CS1: 0xE1C0_0000
MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller )
MEM_REGION_BASE_CS1: 0x17C00
Hm, I wonder if we shouldn't use a 'ranges' property to describe this
address translation. Something like
ebu@xxx {
ranges = <0x17400000 0xe1400000 0x1000>,
<0x17c00000 0xe1c00000 0x1000>;
reg = <0x17400000>, <0x17c00000>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X00000) will be available in res->start,
and the non-translated one (0x17X00000) can be retrieved with
of_get_address(). All you'd have to do then would be calculate the
mask:
mask = (translated_address & original_address) >> 22;
num_comp_bits = fls(mask);
WARN_ON(mask != GENMASK(num_comp_bits - 1, 0));
Which allows you to properly set the ADDR_SEL() register without
relying on some hardcoded values:
writel(original_address | EBU_ADDR_SEL_REGEN |
EBU_ADDR_COMP_BITS(num_comp_bits),
ebu_host->ebu + EBU_ADDR_SEL(csid));
That's quite important if we want to merge the xway NAND driver with
this one.
Looks like the translation is done at the FPI bus declaration level (see
[1]). We really need to see the big picture to take a wise decision
about the bindings. Would you mind pasting your dsti/dts files
somewhere? It feels like the NAND controller is a sub-part of a more
generic 'memory' controller, in which case the NAND controller should be
declared as a child of this generic memory bus (called localbus in [1],
but maybe EBU is more accurate).
[1]https://github.com/xieyaxiongfly/Atheros_CSI_tool_OpenWRT_src/blob/master/target/linux/lantiq/files-4.14/arch/mips/boot/dts/vr9.dtsi#L162
ebu_nand: ebu_nand@e0f00000 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f00000 0x100
0xe1000000 0x300
0xe1400000 0x80000
0xe1c00000 0x10000>;
reg-names = "ebunand", "hsnand", "nand_cs0", nand_cs1";
dmas = <&dma0 8>, <&dma0 9>;
dma-names = "ebu_rx", "ebu_tx";
clocks = <&cgu0 LGM_GCLK_EBU>;
};
&ebu_nand {
status = "disabled";
nand,cs = <1>;
nand-ecc-mode = "hw";
pinctrl-names = "default";
pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
};
Ok. If I understand the SoC topology correctly it should actually be
something like that:
{
...
fpi@xxxxx {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x16000000 0xe0000000 0x1000000>;
...
Sorry, we do not have fpi tree node in our dts/dtsi file instead we have
the below one.. , that also not included the major peripherals
controllers node.
/* Special part from CPU core */
core: core {
compatible = "intel,core", "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges;
ioapic1: interrupt-controller@fec00000 {
#interrupt-cells = <2>;
#address-cells = <0>;
compatible = "intel,ce4100-ioapic";
interrupt-controller;
reg = <0xfec00000 0x1000>;
nr_entries = <256>;
};
hpet: timer@fed00000 {
compatible = "intel,ce4100-hpet";
reg = <0xfed00000 0x400>;
};
lapic0: interrupt-controller@fee00000 {
compatible = "intel,ce4100-lapic";
reg = <0xfee00000 0x1000>;
no_pic_mode;
};
};
other than this, rest all in independent node .
But you do have an FPI bus, right? If this is the case it should be
represented.
Yes, FPI bus is slave to core which connects all the peripherals.
Or is the "intel,core" bus actually the FPI bus that you
named differently?
FPI slave bus connects to core bus by OCP bridge, so here it is named
FPI bus, but SW perspective didn't have root tree which has all
sub-nodes, as of now each peripheral has its own node.
Duh, not sure that's a good idea to hide that, especially since you
have to describe the address translation that happens when crossing the
FPI bus (the ranges thing I mentioned previously).
Thanks Boris, Sure I will do as you have suggested.
can I proceed to send next patch-set.
Regards
Vadivel