Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
On 5/5/2020 3:00 pm, Boris Brezillon wrote:
Hello Vadivel,
On Tue, 5 May 2020 13:28:58 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
ebu_nand: ebu_nand@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100
0xe100 0x300
0xe140 0x8
0xe1c0 0x1>;
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@x {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x1600 0xe000 0x100>;
...
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@fec0 {
#interrupt-cells = <2>;
#address-cells = <0>;
compatible = "intel,ce4100-ioapic";
interrupt-controller;
reg = <0xfec0 0x1000>;
nr_entries = <256>;
};
hpet: timer@fed0 {
compatible = "intel,ce4100-hpet";
reg = <0xfed0 0x400>;
};
lapic0: interrupt-controller@fee0 {
compatible = "intel,ce4100-lapic";
reg = <0xfee0 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! for the keep reviewing.
SW Address translation is not required, after discussion with HW team ,
came to know that 0x17400 and 0x17C00 internal to the SoC.
NOC will translate 0xE1XX... to FPI address 0x17X... internally.
There is an address translation in the NOC.
0x17X... is not visible to user.
so far added hard-coded values to CS0 and CS1 is not at required.
I will change the code accordingly and sent to you.
Hm, you told me last week that writing wrong values to this register
caused the NAND controller to not work properly (you even had code that
was overwriting the dynamically calculated values by hardcoded ones, so
I suspect it indeed didn't work) and now you say this write to
EBU_ADDR_SEL() is optional?! Sorry but it's hard to believe, and I've
received so many contradictory information from you on that matter that
I can't really tell which one is correct. Not sure I want to keep
reviewing new versions of this driver in this context.
oh my bad really sorry , since last week based on the input given from
person who has worked on legacy IP, but now I have discussed with
low-level HW team and confirmed. also we don't have proper document
reference manual since it's new SoC.
Please forgive me , this shouldn't be happen once again, Thanks a lot!!
Regards
Vadivel
Regards,
Boris
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hello Vadivel,
On Tue, 5 May 2020 13:28:58 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
> >>
> >> ebu_nand: ebu_nand@e0f0 {
> >> compatible = "intel,lgm-ebu-nand";
> >> reg = <0xe0f0 0x100
> >> 0xe100 0x300
> >> 0xe140 0x8
> >> 0xe1c0 0x1>;
> >> 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@x {
> > compatible = "intel,lgm-fpi", "simple-bus";
> >
> > /* You might have other ranges to define here */
> > ranges = <0x1600 0xe000 0x100>;
> >
> > ...
>
> 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@fec0 {
> #interrupt-cells = <2>;
> #address-cells = <0>;
> compatible = "intel,ce4100-ioapic";
> interrupt-controller;
> reg = <0xfec0 0x1000>;
> nr_entries = <256>;
> };
>
> hpet: timer@fed0 {
> compatible = "intel,ce4100-hpet";
> reg = <0xfed0 0x400>;
> };
>
> lapic0: interrupt-controller@fee0 {
> compatible = "intel,ce4100-lapic";
> reg = <0xfee0 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! for the keep reviewing.
>
> SW Address translation is not required, after discussion with HW team ,
> came to know that 0x17400 and 0x17C00 internal to the SoC.
>
> NOC will translate 0xE1XX... to FPI address 0x17X... internally.
> There is an address translation in the NOC.
> 0x17X... is not visible to user.
>
> so far added hard-coded values to CS0 and CS1 is not at required.
> I will change the code accordingly and sent to you.
Hm, you told me last week that writing wrong values to this register
caused the NAND controller to not work properly (you even had code that
was overwriting the dynamically calculated values by hardcoded ones, so
I suspect it indeed didn't work) and now you say this write to
EBU_ADDR_SEL() is optional?! Sorry but it's hard to believe, and I've
received so many contradictory information from you on that matter that
I can't really tell which one is correct. Not sure I want to keep
reviewing new versions of this driver in this context.
Regards,
Boris
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
On 4/5/2020 4:58 pm, Boris Brezillon wrote:
On Mon, 4 May 2020 16:50:08 +0800
"Ramuthevar, Vadivel MuruganX"
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"
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"
wrote:
Hi Boris,
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100
0xe100 0x300
0xe140 0x8
0xe1c0 0x1>;
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@x {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x1600 0xe000 0x100>;
...
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@fec0 {
#interrupt-cells = <2>;
#address-cells = <0>;
compatible = "intel,ce4100-ioapic";
interrupt-controller;
reg = <0xfec0 0x1000>;
nr_entries = <256>;
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
On 4/5/2020 4:58 pm, Boris Brezillon wrote:
On Mon, 4 May 2020 16:50:08 +0800
"Ramuthevar, Vadivel MuruganX"
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"
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"
wrote:
Hi Boris,
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100
0xe100 0x300
0xe140 0x8
0xe1c0 0x1>;
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@x {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x1600 0xe000 0x100>;
...
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@fec0 {
#interrupt-cells = <2>;
#address-cells = <0>;
compatible = "intel,ce4100-ioapic";
interrupt-controller;
reg = <0xfec0 0x1000>;
nr_entries = <256>;
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Mon, 4 May 2020 16:50:08 +0800
"Ramuthevar, Vadivel MuruganX"
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"
> > 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"
> >>> wrote:
> >>>
> Hi Boris,
>
> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
> > On Thu, 30 Apr 2020 14:36:00 +0200
> > Boris Brezillon wrote:
> >
> >> On Thu, 30 Apr 2020 17:07:03 +0800
> >> "Ramuthevar, Vadivel MuruganX"
> >> wrote:
> >>
> >> The question is, is it the same value we have in nand_pa or it is
> >> different?
> >>
> > Different address which is 0xE140 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_
> >>> HSNAND_BASE: 0xE100_
> >>> NAND_CS0: 0xE140_
> >>> NAND_CS1: 0xE1C0_
> >>>
> >>> 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 = <0x1740 0xe140 0x1000>,
> >> <0x17c0 0xe1c0 0x1000>;
> >>reg = <0x1740>, <0x17c0>;
> >>reg-names = "cs-0", "cs-1";
> >>}
> >>
> >> The translated address (0xE1X0) will be available in res->start,
> >> and the non-translated one (0x17X0) 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@e0f0 {
> compatible = "intel,lgm-ebu-nand";
> reg = <0xe0f0 0x100
> 0xe100 0x300
> 0xe140 0x8
> 0xe1c0 0x1>;
> 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@x {
> >>> compatible = "intel,lgm-fpi", "simple-bus";
> >>>
> >>> /* You might have other ranges to define here */
> >>> ranges = <0x1600 0xe000 0x100>;
> >>>
> >>>
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
On 4/5/2020 3:17 pm, Boris Brezillon wrote:
On Mon, 4 May 2020 15:15:08 +0800
"Ramuthevar, Vadivel MuruganX"
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"
wrote:
Hi Boris,
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100
0xe100 0x300
0xe140 0x8
0xe1c0 0x1>;
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@x {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x1600 0xe000 0x100>;
...
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@fec0 {
#interrupt-cells = <2>;
#address-cells = <0>;
compatible = "intel,ce4100-ioapic";
interrupt-controller;
reg = <0xfec0 0x1000>;
nr_entries = <256>;
};
hpet: timer@fed0 {
compatible = "intel,ce4100-hpet";
reg = <0xfed0 0x400>;
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Mon, 4 May 2020 15:15:08 +0800
"Ramuthevar, Vadivel MuruganX"
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"
> > wrote:
> >
> >> Hi Boris,
> >>
> >> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
> >>> On Thu, 30 Apr 2020 14:36:00 +0200
> >>> Boris Brezillon wrote:
> >>>
> On Thu, 30 Apr 2020 17:07:03 +0800
> "Ramuthevar, Vadivel MuruganX"
> wrote:
>
> The question is, is it the same value we have in nand_pa or it is
> different?
>
> >>> Different address which is 0xE140 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_
> > HSNAND_BASE: 0xE100_
> > NAND_CS0: 0xE140_
> > NAND_CS1: 0xE1C0_
> >
> > 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 = <0x1740 0xe140 0x1000>,
> <0x17c0 0xe1c0 0x1000>;
> reg = <0x1740>, <0x17c0>;
> reg-names = "cs-0", "cs-1";
> }
>
> The translated address (0xE1X0) will be available in res->start,
> and the non-translated one (0x17X0) 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@e0f0 {
> >>compatible = "intel,lgm-ebu-nand";
> >>reg = <0xe0f0 0x100
> >>0xe100 0x300
> >>0xe140 0x8
> >>0xe1c0 0x1>;
> >>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@x {
> > compatible = "intel,lgm-fpi", "simple-bus";
> >
> > /* You might have other ranges to define here */
> > ranges = <0x1600 0xe000 0x100>;
> >
> > ...
>
> 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@fec0 {
>
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
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"
wrote:
Hi Boris,
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100
0xe100 0x300
0xe140 0x8
0xe1c0 0x1>;
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@x {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x1600 0xe000 0x100>;
...
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@fec0 {
#interrupt-cells = <2>;
#address-cells = <0>;
compatible = "intel,ce4100-ioapic";
interrupt-controller;
reg = <0xfec0 0x1000>;
nr_entries = <256>;
};
hpet: timer@fed0 {
compatible = "intel,ce4100-hpet";
reg = <0xfed0 0x400>;
};
lapic0: interrupt-controller@fee0 {
compatible = "intel,ce4100-lapic";
reg = <0xfee0 0x1000>;
no_
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Mon, 4 May 2020 10:02:35 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
> Hi Boris,
>
> On 30/4/2020 9:01 pm, Boris Brezillon wrote:
> > On Thu, 30 Apr 2020 14:36:00 +0200
> > Boris Brezillon wrote:
> >
> >> On Thu, 30 Apr 2020 17:07:03 +0800
> >> "Ramuthevar, Vadivel MuruganX"
> >> wrote:
> >>
> >> The question is, is it the same value we have in nand_pa or it is
> >> different?
> >>
> > Different address which is 0xE140 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_
> >>> HSNAND_BASE: 0xE100_
> >>> NAND_CS0: 0xE140_
> >>> NAND_CS1: 0xE1C0_
> >>>
> >>> 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 = <0x1740 0xe140 0x1000>,
> >> <0x17c0 0xe1c0 0x1000>;
> >>reg = <0x1740>, <0x17c0>;
> >>reg-names = "cs-0", "cs-1";
> >>}
> >>
> >> The translated address (0xE1X0) will be available in res->start,
> >> and the non-translated one (0x17X0) 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@e0f0 {
> compatible = "intel,lgm-ebu-nand";
> reg = <0xe0f0 0x100
> 0xe100 0x300
> 0xe140 0x8
> 0xe1c0 0x1>;
> 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@x {
compatible = "intel,lgm-fpi", "simple-bus";
/* You might have other ranges to define here */
ranges = <0x1600 0xe000 0x100>;
...
ebu@ {
compatible = "intel,lgm-ebu", "simple-bus";
ranges;
pinctrl-names = "default";
pinctrl-0 = <&ebu_nand_base &ebu_cs1>;
/*
* Add your PCI and NOR controller definitions
* here.
*/
...
nand-controller@16f0 {
compatible = "intel,lgm-ebu-nand-controller";
reg = <0x16f0 0x100>,
<0x1700 0x300>,
<0x1740 0x8>,
<0x17c0 0x1>;
reg-names = "ebunand", "hsnand",
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100
0xe100 0x300
0xe140 0x8
0xe1c0 0x1>;
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>;
};
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
Thank you very much for the review comments and your time...
On 30/4/2020 9:01 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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
for the ebu-nand node in the dts file.
ebu_nand: ebu_nand@e0f0 {
compatible = "intel,lgm-ebu-nand";
reg = <0xe0f0 0x100 //EBU_NAND controller
0xe100 0x300 //NAND ECC Extension access
0xe140 0x8
0xe1c0 0x1>;
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>;
};
This is not comes under fpi in our devicetree.
Regards
Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris,
Thank you very much for the review comments and giving inputs...
On 30/4/2020 8:36 pm, Boris Brezillon wrote:
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
The question is, is it the same value we have in nand_pa or it is
different?
Different address which is 0xE140 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_
HSNAND_BASE: 0xE100_
NAND_CS0: 0xE140_
NAND_CS1: 0xE1C0_
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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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.
Thanks! , for the optimized and made it generic way, will update in the
next patch-set.
Regards
Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 30 Apr 2020 14:36:00 +0200
Boris Brezillon wrote:
> On Thu, 30 Apr 2020 17:07:03 +0800
> "Ramuthevar, Vadivel MuruganX"
> wrote:
>
> > >>> The question is, is it the same value we have in nand_pa or it is
> > >>> different?
> > >>>
> > >> Different address which is 0xE140 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_
> > HSNAND_BASE: 0xE100_
> > NAND_CS0: 0xE140_
> > NAND_CS1: 0xE1C0_
> >
> > 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 = <0x1740 0xe140 0x1000>,
><0x17c0 0xe1c0 0x1000>;
> reg = <0x1740>, <0x17c0>;
> reg-names = "cs-0", "cs-1";
> }
>
> The translated address (0xE1X0) will be available in res->start,
> and the non-translated one (0x17X0) 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
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 30 Apr 2020 17:07:03 +0800
"Ramuthevar, Vadivel MuruganX"
wrote:
> >>> The question is, is it the same value we have in nand_pa or it is
> >>> different?
> >>>
> >> Different address which is 0xE140 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_
> HSNAND_BASE: 0xE100_
> NAND_CS0: 0xE140_
> NAND_CS1: 0xE1C0_
>
> 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 = <0x1740 0xe140 0x1000>,
<0x17c0 0xe1c0 0x1000>;
reg = <0x1740>, <0x17c0>;
reg-names = "cs-0", "cs-1";
}
The translated address (0xE1X0) will be available in res->start,
and the non-translated one (0x17X0) 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.
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, On 30/4/2020 4:36 pm, Boris Brezillon wrote: On Thu, 30 Apr 2020 16:30:15 +0800 "Ramuthevar, Vadivel MuruganX" wrote: And now I'd like you to explain why 5 is the right value for that field (I guess that has to do with the position of the CS/ALE/CLE pins). 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the That's 6 bits to me, not 5. No , 5 bits only the above case. Oops, right, sorry for the brain fart. The question is, is it the same value we have in nand_pa or it is different? Different address which is 0xE140 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_ HSNAND_BASE: 0xE100_ NAND_CS0: 0xE140_ NAND_CS1: 0xE1C0_ MEM_REGION_BASE_CS0: 0x17400 (internal to ebu controller ) MEM_REGION_BASE_CS1: 0x17C00 Do you have a reference manual I can look at to understand what this is? We dont have reference manual since it is new SoC, we keep get information from HW team and we have only register map Thanks a lot !!! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 30 Apr 2020 16:30:15 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > >>> > >>> And now I'd like you to explain why 5 is the right value for that field > >>> (I guess that has to do with the position of the CS/ALE/CLE pins). > >> > >> 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the > > > > That's 6 bits to me, not 5. > > No , 5 bits only the above case. Oops, right, sorry for the brain fart. > > > > The question is, is it the same value we have in nand_pa or it is > > different? > > > Different address which is 0xE140 NAND_BASE_PHY address. Then why didn't you tell me they didn't match when I suggested to pass nand_pa? So now the question is, what does this address represent? Do you have a reference manual I can look at to understand what this is?
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
H Boris, On 30/4/2020 4:21 pm, Boris Brezillon wrote: On Thu, 30 Apr 2020 15:50:30 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, Thank you very much for keep reviewing the patches and more queries... On 29/4/2020 11:31 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 23:18:31 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, On 29/4/2020 10:48 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 22:33:37 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, On 29/4/2020 10:22 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar, Vadivel MuruganX" wrote: + +#define EBU_ADDR_SEL(n)(0x20 + (n) * 4) +#define EBU_ADDR_MASK (5 << 4) It's still unclear what ADDR_MASK is for. Can you add a comment explaining what it does? Thank you Boris, keep review and giving inputs, will update. Can you please explain it here before sending a new version? Memory Region Address Mask: Specifies the number of right-most bits in the base address that should be included in the address comparison. bits positions(7:4). Okay, then the macro should be #define EBU_ADDR_MASK(x)((x) << 4) And now I'd like you to explain why 5 is the right value for that field (I guess that has to do with the position of the CS/ALE/CLE pins). 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the That's 6 bits to me, not 5. No , 5 bits only the above case. ADDR_SELx , it compares only 5 bits. Definitely not what I would qualify as right-most bits. So, you say the comparison always starts at bit 22, and ends at 22+? Correct Yes , we are setting both CS0 and CS1 memory access region, if you have any concern to optimize, please suggest me, Thanks! If you want to setup both CS, and the address written in EBU_ADDR_SEL(x) is really related to the nand_pa address, then retrieve resources for all CS ranges. If it's not related, please explain what those EBU_MEM_BASE_CS_X values encode. Memory Region Base Address FPI Bus addresses are compared to this base address in conjunction with the mask control(EBU_ADDR_MASK). Driver need to program this field! That's not explaining what the base address should be. Is 'nand_pa' the value we should have there? The one prorgrammed in the addr_sel register is used by the HW controller, it remaps to 0x174XX-> CS0 and 0x17CXX->CS1. The hardware itself, decodes only for 1740xx/17c0xx, other random values cannot be programmed The question is, is it the same value we have in nand_pa or it is different? Different address which is 0xE140 NAND_BASE_PHY address. Thanks! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Thu, 30 Apr 2020 15:50:30 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > Hi Boris, > >Thank you very much for keep reviewing the patches and more queries... > > On 29/4/2020 11:31 pm, Boris Brezillon wrote: > > On Wed, 29 Apr 2020 23:18:31 +0800 > > "Ramuthevar, Vadivel MuruganX" > > wrote: > > > >> Hi Boris, > >> > >> On 29/4/2020 10:48 pm, Boris Brezillon wrote: > >>> On Wed, 29 Apr 2020 22:33:37 +0800 > >>> "Ramuthevar, Vadivel MuruganX" > >>> wrote: > >>> > Hi Boris, > > On 29/4/2020 10:22 pm, Boris Brezillon wrote: > > On Wed, 29 Apr 2020 18:42:05 +0800 > > "Ramuthevar, Vadivel MuruganX" > > wrote: > > > >> + > >> +#define EBU_ADDR_SEL(n) (0x20 + (n) * 4) > >> +#define EBU_ADDR_MASK (5 << 4) > > > > It's still unclear what ADDR_MASK is for. Can you add a comment > > explaining what it does? > > Thank you Boris, keep review and giving inputs, will update. > >>> > >>> Can you please explain it here before sending a new version? > >> > >> Memory Region Address Mask: > >> Specifies the number of right-most bits in the base address that should > >> be included in the address comparison. bits positions(7:4). > > > > Okay, then the macro should be > > > > #define EBU_ADDR_MASK(x)((x) << 4) > > > > And now I'd like you to explain why 5 is the right value for that field > > (I guess that has to do with the position of the CS/ALE/CLE pins). > > 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the That's 6 bits to me, not 5. > ADDR_SELx , it compares only 5 bits. Definitely not what I would qualify as right-most bits. So, you say the comparison always starts at bit 22, and ends at 22+? > Yes , we are setting both CS0 and CS1 memory access region, if you have > any concern to optimize, please suggest me, Thanks! > >>> > >>> If you want to setup both CS, and the address written in EBU_ADDR_SEL(x) > >>> is really related to the nand_pa address, then retrieve resources for > >>> all CS ranges. > >> If it's not related, please explain what those > >>> EBU_MEM_BASE_CS_X values encode. > >> > >> Memory Region Base Address > >> FPI Bus addresses are compared to this base address in conjunction with > >> the mask control(EBU_ADDR_MASK). Driver need to program this field! > > > > That's not explaining what the base address should be. Is 'nand_pa' the > > value we should have there? > > The one prorgrammed in the addr_sel register is used by the HW > controller, it remaps to 0x174XX-> CS0 and 0x17CXX->CS1. > The hardware itself, decodes only for 1740xx/17c0xx, other random values > cannot be programmed The question is, is it the same value we have in nand_pa or it is different?
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, Thank you very much for keep reviewing the patches and more queries... On 29/4/2020 11:31 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 23:18:31 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, On 29/4/2020 10:48 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 22:33:37 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, On 29/4/2020 10:22 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar, Vadivel MuruganX" wrote: + +#define EBU_ADDR_SEL(n)(0x20 + (n) * 4) +#define EBU_ADDR_MASK (5 << 4) It's still unclear what ADDR_MASK is for. Can you add a comment explaining what it does? Thank you Boris, keep review and giving inputs, will update. Can you please explain it here before sending a new version? Memory Region Address Mask: Specifies the number of right-most bits in the base address that should be included in the address comparison. bits positions(7:4). Okay, then the macro should be #define EBU_ADDR_MASK(x)((x) << 4) And now I'd like you to explain why 5 is the right value for that field (I guess that has to do with the position of the CS/ALE/CLE pins). 5 : bit 26, 25, 24, 23, 22 to be included for comparison in the ADDR_SELx , it compares only 5 bits. +#define EBU_ADDR_SEL_REGEN 0x1 + + writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | + EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, + ebu_host->ebu + EBU_ADDR_SEL(reg)); You set EBU_ADDR_SEL(reg) once here... + + writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(0)); + writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(reg)); ... and a second time here. That sounds like overwriting the EBU_ADDR_SEL(reg) register to me. That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you sure that's needed, and are we setting EBU_ADDR_SEL(0) here? You are right, its weird only, but we need it, since different chip select has different memory region access address. Well, that doesn't make any sense, the second write to EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is actually never written to ADDR_SEL(reg). it will not overwrite the first one, since two different registers EBU_ADDR_SEL_0 EBU_ADDR_SEL 20H EBU_ADDR_SEL_1 EBU_ADDR_SEL 24H See my above. it is an internal address selection w.r.t chip select for nand physical address update. Yes , we are setting both CS0 and CS1 memory access region, if you have any concern to optimize, please suggest me, Thanks! If you want to setup both CS, and the address written in EBU_ADDR_SEL(x) is really related to the nand_pa address, then retrieve resources for all CS ranges. If it's not related, please explain what those EBU_MEM_BASE_CS_X values encode. Memory Region Base Address FPI Bus addresses are compared to this base address in conjunction with the mask control(EBU_ADDR_MASK). Driver need to program this field! That's not explaining what the base address should be. Is 'nand_pa' the value we should have there? The one prorgrammed in the addr_sel register is used by the HW controller, it remaps to 0x174XX-> CS0 and 0x17CXX->CS1. The hardware itself, decodes only for 1740xx/17c0xx, other random values cannot be programmed Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Wed, 29 Apr 2020 23:18:31 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > Hi Boris, > > On 29/4/2020 10:48 pm, Boris Brezillon wrote: > > On Wed, 29 Apr 2020 22:33:37 +0800 > > "Ramuthevar, Vadivel MuruganX" > > wrote: > > > >> Hi Boris, > >> > >> On 29/4/2020 10:22 pm, Boris Brezillon wrote: > >>> On Wed, 29 Apr 2020 18:42:05 +0800 > >>> "Ramuthevar, Vadivel MuruganX" > >>> wrote: > >>> > + > +#define EBU_ADDR_SEL(n) (0x20 + (n) * 4) > +#define EBU_ADDR_MASK (5 << 4) > >>> > >>> It's still unclear what ADDR_MASK is for. Can you add a comment > >>> explaining what it does? > >> > >> Thank you Boris, keep review and giving inputs, will update. > > > > Can you please explain it here before sending a new version? > > Memory Region Address Mask: > Specifies the number of right-most bits in the base address that should > be included in the address comparison. bits positions(7:4). Okay, then the macro should be #define EBU_ADDR_MASK(x)((x) << 4) And now I'd like you to explain why 5 is the right value for that field (I guess that has to do with the position of the CS/ALE/CLE pins). > > >>> > +#define EBU_ADDR_SEL_REGEN 0x1 > >>> > >>> > + > +writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | > + EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, > + ebu_host->ebu + EBU_ADDR_SEL(reg)); You set EBU_ADDR_SEL(reg) once here... > + > +writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, > + ebu_host->ebu + EBU_ADDR_SEL(0)); > +writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, > + ebu_host->ebu + EBU_ADDR_SEL(reg)); ... and a second time here. That sounds like overwriting the EBU_ADDR_SEL(reg) register to me. > >>> > >>> That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you > >>> sure that's needed, and are we setting EBU_ADDR_SEL(0) here? > >> > >> You are right, its weird only, but we need it, since different chip > >> select has different memory region access address. > > > > Well, that doesn't make any sense, the second write to > > EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is > > actually never written to ADDR_SEL(reg). > > it will not overwrite the first one, since two different registers > EBU_ADDR_SEL_0 EBU_ADDR_SEL 20H > EBU_ADDR_SEL_1 EBU_ADDR_SEL 24H See my above. > > it is an internal address selection w.r.t chip select for nand physical > address update. > > > > > >> > >> Yes , we are setting both CS0 and CS1 memory access region, if you have > >> any concern to optimize, please suggest me, Thanks! > > > > If you want to setup both CS, and the address written in EBU_ADDR_SEL(x) > > is really related to the nand_pa address, then retrieve resources for > > all CS ranges. > If it's not related, please explain what those > > EBU_MEM_BASE_CS_X values encode. > > Memory Region Base Address > FPI Bus addresses are compared to this base address in conjunction with > the mask control(EBU_ADDR_MASK). Driver need to program this field! That's not explaining what the base address should be. Is 'nand_pa' the value we should have there?
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, On 29/4/2020 11:18 pm, Ramuthevar, Vadivel MuruganX wrote: + writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | + EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, + ebu_host->ebu + EBU_ADDR_SEL(reg)); + + writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(0)); Good catch!, one will be removed , Thanks ! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, On 29/4/2020 10:48 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 22:33:37 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, On 29/4/2020 10:22 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar, Vadivel MuruganX" wrote: + +#define EBU_ADDR_SEL(n)(0x20 + (n) * 4) +#define EBU_ADDR_MASK (5 << 4) It's still unclear what ADDR_MASK is for. Can you add a comment explaining what it does? Thank you Boris, keep review and giving inputs, will update. Can you please explain it here before sending a new version? Memory Region Address Mask: Specifies the number of right-most bits in the base address that should be included in the address comparison. bits positions(7:4). +#define EBU_ADDR_SEL_REGEN 0x1 + + writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | + EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, + ebu_host->ebu + EBU_ADDR_SEL(reg)); + + writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(0)); + writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(reg)); That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you sure that's needed, and are we setting EBU_ADDR_SEL(0) here? You are right, its weird only, but we need it, since different chip select has different memory region access address. Well, that doesn't make any sense, the second write to EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is actually never written to ADDR_SEL(reg). it will not overwrite the first one, since two different registers EBU_ADDR_SEL_0 EBU_ADDR_SEL 20H EBU_ADDR_SEL_1 EBU_ADDR_SEL 24H it is an internal address selection w.r.t chip select for nand physical address update. Yes , we are setting both CS0 and CS1 memory access region, if you have any concern to optimize, please suggest me, Thanks! If you want to setup both CS, and the address written in EBU_ADDR_SEL(x) is really related to the nand_pa address, then retrieve resources for all CS ranges. If it's not related, please explain what those EBU_MEM_BASE_CS_X values encode. Memory Region Base Address FPI Bus addresses are compared to this base address in conjunction with the mask control(EBU_ADDR_MASK). Driver need to program this field! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Wed, 29 Apr 2020 22:33:37 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > Hi Boris, > > On 29/4/2020 10:22 pm, Boris Brezillon wrote: > > On Wed, 29 Apr 2020 18:42:05 +0800 > > "Ramuthevar, Vadivel MuruganX" > > wrote: > > > >> + > >> +#define EBU_ADDR_SEL(n) (0x20 + (n) * 4) > >> +#define EBU_ADDR_MASK (5 << 4) > > > > It's still unclear what ADDR_MASK is for. Can you add a comment > > explaining what it does? > > Thank you Boris, keep review and giving inputs, will update. Can you please explain it here before sending a new version? > > > >> +#define EBU_ADDR_SEL_REGEN0x1 > > > > > >> + > >> + writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | > >> + EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, > >> + ebu_host->ebu + EBU_ADDR_SEL(reg)); > >> + > >> + writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, > >> + ebu_host->ebu + EBU_ADDR_SEL(0)); > >> + writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, > >> + ebu_host->ebu + EBU_ADDR_SEL(reg)); > > > > That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you > > sure that's needed, and are we setting EBU_ADDR_SEL(0) here? > > You are right, its weird only, but we need it, since different chip > select has different memory region access address. Well, that doesn't make any sense, the second write to EBU_ADDR_SEL(reg) overrides the first one, meaning that nand_pa is actually never written to ADDR_SEL(reg). > > Yes , we are setting both CS0 and CS1 memory access region, if you have > any concern to optimize, please suggest me, Thanks! If you want to setup both CS, and the address written in EBU_ADDR_SEL(x) is really related to the nand_pa address, then retrieve resources for all CS ranges. If it's not related, please explain what those EBU_MEM_BASE_CS_X values encode.
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, On 29/4/2020 10:22 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar, Vadivel MuruganX" wrote: + +#define EBU_ADDR_SEL(n)(0x20 + (n) * 4) +#define EBU_ADDR_MASK (5 << 4) It's still unclear what ADDR_MASK is for. Can you add a comment explaining what it does? Thank you Boris, keep review and giving inputs, will update. +#define EBU_ADDR_SEL_REGEN 0x1 + + writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | + EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, + ebu_host->ebu + EBU_ADDR_SEL(reg)); + + writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(0)); + writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, + ebu_host->ebu + EBU_ADDR_SEL(reg)); That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you sure that's needed, and are we setting EBU_ADDR_SEL(0) here? You are right, its weird only, but we need it, since different chip select has different memory region access address. Yes , we are setting both CS0 and CS1 memory access region, if you have any concern to optimize, please suggest me, Thanks! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, Thank you very much for the review comments and your time... On 29/4/2020 9:32 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 21:29:40 +0800 "Ramuthevar, Vadivel MuruganX" wrote: Hi Boris, Thank you very much for the review comments and your time.. On 29/4/2020 7:33 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar,Vadivel MuruganX" wrote: +#define NAND_WRITE_CMD (EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS) +#define NAND_WRITE_ADDR(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS) + I thought we agreed on dropping those definitions. Yes , we agreed upon it, due to assertion/de-assertion of CS kept it. And I keep thinking we don't need that, just pass 'HSNAND_CLE_OFFS | HSNAND_CS_OFFS' directly. Agreed!, will update in the next-patch version, Thanks! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > + > +#define EBU_ADDR_SEL(n) (0x20 + (n) * 4) > +#define EBU_ADDR_MASK(5 << 4) It's still unclear what ADDR_MASK is for. Can you add a comment explaining what it does? > +#define EBU_ADDR_SEL_REGEN 0x1 > + > + writel(lower_32_bits(ebu_host->cs[ebu_host->cs_num].nand_pa) | > +EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK, > +ebu_host->ebu + EBU_ADDR_SEL(reg)); > + > + writel(EBU_MEM_BASE_CS_0 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, > +ebu_host->ebu + EBU_ADDR_SEL(0)); > + writel(EBU_MEM_BASE_CS_1 | EBU_ADDR_MASK | EBU_ADDR_SEL_REGEN, > +ebu_host->ebu + EBU_ADDR_SEL(reg)); That's super weird. You seem to set EBU_ADDR_SEL(reg) twice. Are you sure that's needed, and are we setting EBU_ADDR_SEL(0) here?
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Wed, 29 Apr 2020 21:29:40 +0800 "Ramuthevar, Vadivel MuruganX" wrote: > Hi Boris, > > Thank you very much for the review comments and your time.. > > On 29/4/2020 7:33 pm, Boris Brezillon wrote: > > On Wed, 29 Apr 2020 18:42:05 +0800 > > "Ramuthevar,Vadivel MuruganX" > > wrote: > > > >> +#define NAND_WRITE_CMD(EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS) > >> +#define NAND_WRITE_ADDR (EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS) > >> + > > > > I thought we agreed on dropping those definitions. > > Yes , we agreed upon it, due to assertion/de-assertion of CS kept it. And I keep thinking we don't need that, just pass 'HSNAND_CLE_OFFS | HSNAND_CS_OFFS' directly.
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
Hi Boris, Thank you very much for the review comments and your time.. On 29/4/2020 7:33 pm, Boris Brezillon wrote: On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar,Vadivel MuruganX" wrote: +#define NAND_WRITE_CMD (EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS) +#define NAND_WRITE_ADDR(EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS) + I thought we agreed on dropping those definitions. Yes , we agreed upon it, due to assertion/de-assertion of CS kept it. Thanks! Regards Vadivel
Re: [PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
On Wed, 29 Apr 2020 18:42:05 +0800 "Ramuthevar,Vadivel MuruganX" wrote: > +#define NAND_WRITE_CMD (EBU_CON_CS_P_LOW | HSNAND_CLE_OFFS) > +#define NAND_WRITE_ADDR (EBU_CON_CS_P_LOW | HSNAND_ALE_OFFS) > + I thought we agreed on dropping those definitions.
[PATCH v4 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC
From: Ramuthevar Vadivel Murugan This patch adds the new IP of Nand Flash Controller(NFC) support on Intel's Lightning Mountain(LGM) SoC. DMA is used for burst data transfer operation, also DMA HW supports aligned 32bit memory address and aligned data access by default. DMA burst of 8 supported. Data register used to support the read/write operation from/to device. NAND controller driver implements ->exec_op() to replace legacy hooks, these specific call-back method to execute NAND operations. Signed-off-by: Ramuthevar Vadivel Murugan --- drivers/mtd/nand/raw/Kconfig | 8 + drivers/mtd/nand/raw/Makefile| 1 + drivers/mtd/nand/raw/intel-nand-controller.c | 750 +++ 3 files changed, 759 insertions(+) create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig index a80a46bb5b8b..a026bec28f39 100644 --- a/drivers/mtd/nand/raw/Kconfig +++ b/drivers/mtd/nand/raw/Kconfig @@ -457,6 +457,14 @@ config MTD_NAND_CADENCE Enable the driver for NAND flash on platforms using a Cadence NAND controller. +config MTD_NAND_INTEL_LGM + tristate "Support for NAND controller on Intel LGM SoC" + depends on OF || COMPILE_TEST + depends on HAS_IOMEM + help + Enables support for NAND Flash chips on Intel's LGM SoC. + NAND flash interfaced through the External Bus Unit. + comment "Misc" config MTD_SM_COMMON diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile index 2d136b158fb7..bfc8fe4d2cb0 100644 --- a/drivers/mtd/nand/raw/Makefile +++ b/drivers/mtd/nand/raw/Makefile @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o obj-$(CONFIG_MTD_NAND_STM32_FMC2) += stm32_fmc2_nand.o obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o obj-$(CONFIG_MTD_NAND_CADENCE) += cadence-nand-controller.o +obj-$(CONFIG_MTD_NAND_INTEL_LGM) += intel-nand-controller.o nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o nand-objs += nand_onfi.o diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c new file mode 100644 index ..fc69c7a9280c --- /dev/null +++ b/drivers/mtd/nand/raw/intel-nand-controller.c @@ -0,0 +1,750 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) 2020 Intel Corporation. */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define EBU_CLC0x000 +#define EBU_CLC_RST0xu + +#define EBU_ADDR_SEL(n)(0x20 + (n) * 4) +#define EBU_ADDR_MASK (5 << 4) +#define EBU_ADDR_SEL_REGEN 0x1 + +#define EBU_BUSCON(n) (0x60 + (n) * 4) +#define EBU_BUSCON_CMULT_V40x1 +#define EBU_BUSCON_RECOVC(n) ((n) << 2) +#define EBU_BUSCON_HOLDC(n)((n) << 4) +#define EBU_BUSCON_WAITRDC(n) ((n) << 6) +#define EBU_BUSCON_WAITWRC(n) ((n) << 8) +#define EBU_BUSCON_BCGEN_CS0x0 +#define EBU_BUSCON_SETUP_ENBIT(22) +#define EBU_BUSCON_ALEC0xC000 + +#define EBU_CON0x0B0 +#define EBU_CON_NANDM_EN BIT(0) +#define EBU_CON_NANDM_DIS 0x0 +#define EBU_CON_CSMUX_E_EN BIT(1) +#define EBU_CON_ALE_P_LOW BIT(2) +#define EBU_CON_CLE_P_LOW BIT(3) +#define EBU_CON_CS_P_LOW BIT(4) +#define EBU_CON_SE_P_LOW BIT(5) +#define EBU_CON_WP_P_LOW BIT(6) +#define EBU_CON_PRE_P_LOW BIT(7) +#define EBU_CON_IN_CS_S(n) ((n) << 8) +#define EBU_CON_OUT_CS_S(n)((n) << 10) +#define EBU_CON_LAT_EN_CS_P((0x3D) << 18) + +#define EBU_WAIT 0x0B4 +#define EBU_WAIT_RDBY BIT(0) +#define EBU_WAIT_WR_C BIT(3) + +#define EBU_MEM_BASE_CS_0 0x1740 +#define EBU_MEM_BASE_CS_1 0x17C0 + +#define HSNAND_CTL10x110 +#define HSNAND_CTL1_ADDR_SHIFT 24 + +#define HSNAND_CTL20x114 +#define HSNAND_CTL2_ADDR_SHIFT 8 +#define HSNAND_CTL2_CYC_N_V5 (0x2 << 16) + +#define HSNAND_INT_MSK_CTL 0x124 +#define HSNAND_INT_MSK_CTL_WR_CBIT(4) + +#define HSNAND_INT_STA 0x128 +#define HSNAND_INT_STA_WR_CBIT(4) + +#define HSNAND_CTL 0x130 +#define HSNAND_CTL_ENABLE_ECC BIT(0) +#define HSNAND_CTL_GO BIT(2) +#define HSNAND_CTL_CE_SEL_CS(n)BIT(3 + (n)) +#define HSNAND_CTL_RW_READ 0x0 +#define HSNAND_CTL_RW_WRITEBIT(10) +#define HSNAND_CTL_ECC_OFF_V8THBIT(11) +#define HSNAND_CTL_CKFF_EN 0x0 +#define HSNAND_CTL_MSG_EN BIT(17) + +#define HSNAND_PARA0 0x13c +#define HSNAND_PARA0_PAGE_V81920x3 +#define HSNAND_PARA0_PIB_V256 (0x3 << 4) +#define HSNAND_PARA0_BYP_EN_NP 0x0 +#define HSNAND_PARA0_BYP_DEC_NP0x0 +#define HSNAND_PARA0_TYP
