| 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
| ||
|
Message-ID: <20200504091755.0d0e73aa@collabora.com>
Date: Mon, 4 May 2020 09:17:55 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: "Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@...ux.intel.com>, tglx@...utronix.de,
cheol.yong.kim@...el.com, devicetree@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, masonccyang@...c.com.tw,
anders.roxell@...aro.org, vigneshr@...com, arnd@...db.de,
hauke.mehrtens@...el.com, richard@....at,
brendanhiggins@...gle.com, linux-mips@...r.kernel.org,
robh+dt@...nel.org, linux-mtd@...ts.infradead.org,
miquel.raynal@...tlin.com, qi-ming.wu@...el.com,
andriy.shevchenko@...el.com
Subject: 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"
<vadivel.muruganx.ramuthevar@...ux.intel.com> 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@...ux.intel.com> 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@...labora.com> wrote:
> >>>
> >>>> On Thu, 30 Apr 2020 17:07:03 +0800
> >>>> "Ramuthevar, Vadivel MuruganX"
> >>>> <vadivel.muruganx.ramuthevar@...ux.intel.com> 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@...00000 {
> >> 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@...xx {
> > 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@...00000 {
> #interrupt-cells = <2>;
> #address-cells = <0>;
> compatible = "intel,ce4100-ioapic";
> interrupt-controller;
> reg = <0xfec00000 0x1000>;
> nr_entries = <256>;
> };
>
> hpet: timer@...00000 {
> compatible = "intel,ce4100-hpet";
> reg = <0xfed00000 0x400>;
> };
>
> lapic0: interrupt-controller@...00000 {
> 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. Or is the "intel,core" bus actually the FPI bus that you
named differently?
Powered by blists - more mailing lists