Hi Christophe,
Christophe Kerello <christophe.kere...@st.com> wrote on Mon, 11 May
2020 19:02:51 +0200:
> Hi Miquel,
>
> On 5/11/20 4:45 PM, Miquel Raynal wrote:
> > Hi Christophe,
> >
> > Christophe Kerello <christophe.kere...@st.com> wrote on Mon, 11 May
> > 2020 16:19:47 +0200:
> >
> >> Hi Miquel,
> >>
> >> On 5/11/20 2:58 PM, Miquel Raynal wrote:
> >>> Hi Christophe,
> >>>
> >>> Christophe Kerello <christophe.kere...@st.com> wrote on Mon, 11 May
> >>> 2020 14:47:09 +0200:
> >>> >>>> Hi Miquel,
> >>>>
> >>>> On 5/11/20 1:59 PM, Miquel Raynal wrote:
> >>>>> Hi Christophe,
> >>>>>
> >>>>> Christophe Kerello <christophe.kere...@st.com> wrote on Mon, 11 May
> >>>>> 2020 12:21:03 +0200:
> >>>>> >>>> Hi Miquel,
> >>>>>>
> >>>>>> On 5/11/20 11:18 AM, Miquel Raynal wrote:
> >>>>>>> Hi Christophe,
> >>>>>>>
> >>>>>>> Christophe Kerello <christophe.kere...@st.com> wrote on Wed, 6 May
> >>>>>>> 2020
> >>>>>>> 11:11:19 +0200:
> >>>>>>> >>>> FMC2 EBI support has been added. Common resources
> >>>>>>> (registers base
> >>>>>>>> and clock) are now shared between the 2 drivers. It means that the
> >>>>>>>> common resources should now be found in the parent device when EBI
> >>>>>>>> node is available.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Christophe Kerello <christophe.kere...@st.com>
> >>>>>>>> ---
> >>>>>>>
> >>>>>>> [...]
> >>>>>>> >>>> +
> >>>>>>>> +static bool stm32_fmc2_nfc_check_for_parent(struct platform_device
> >>>>>>>> *pdev)
> >>>>>>>> +{
> >>>>>>>> + u32 i;
> >>>>>>>> + int nb_resources = 0;
> >>>>>>>> +
> >>>>>>>> + /* Count the number of resources in reg property */
> >>>>>>>> + for (i = 0; i < pdev->num_resources; i++) {
> >>>>>>>> + struct resource *res = &pdev->resource[i];
> >>>>>>>> +
> >>>>>>>> + if (resource_type(res) == IORESOURCE_MEM)
> >>>>>>>> + nb_resources++;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + /* Each CS needs 3 resources defined (data, cmd and addr) */
> >>>>>>>> + if (nb_resources % 3)
> >>>>>>>> + return false;
> >>>>>>>> +
> >>>>>>>> + return true;
> >>>>>>>> +}
> >>>>>>>
> >>>>>>> This function looks fragile. Why not just checking the compatible
> >>>>>>> string of the parent node?
> >>>>>>> >>
> >>>>>> Yes, it is another way to check that we have an EBI parent node.
> >>>>>>
> >>>>>> In this implementation, I was checking the number of reg tuples.
> >>>>>> In case we have 6, it means that the register base address is defined
> >>>>>> in the parent node (EBI node).
> >>>>>> In case we have 7, it means that the register base address is defined
> >>>>>> in the current node (NFC node).
> >>>>>
> >>>>> Yes, I understand what you are doing, but I kind of dislike the logic.
> >>>>> Relying on the number of reg tuples is something that can be done (I
> >>>>> used it myself one time), but I think this is more a hack that you do
> >>>>> when you have no other way to differentiate. I guess the proper way
> >>>>> would be to look at the parent's compatible. If it matches what you
> >>>>> expect, then you can store the dev->of_node->parent->dev somewhere in
> >>>>> your controller's structure and then use it to initialize the clock and
> >>>>> regmap. This way you don't have to move anything else in the probe
> >>>>> path.
> >>>>> >>
> >>>> OK, I will check the compatible string of the parent device using
> >>>> of_device_is_compatible API in v5.
> >>>> In case of the parent is found, I will add it in the structure of the
> >>>> controller (dev_parent).
> >>>> I will rely on this field only to get the common resources (the register
> >>>> base address and the clock) in the NFC node or in the EBI node.
> >>>
> >>> I had something slightly different in mind: what about setting a
> >>> default value to this field as being the controller's device itself.
> >>> This way, once it is set to either the parent device or the device
> >>> itself, you can use it "blindly" in your devm_clk_get/regmap_init calls?
> >>> >>
> >> I will try to explain what I have in mind.
> >>
> >> I will add a new field in the structure of the controller (not called
> >> dev_parent but cdev)
> >> struct device *cdev;
> >>
> >> Then, at probe time, this field will be assigned:
> >> nfc->cdev = of_device_is_compatible(dev->parent->of_node, "bla bla") :
> >> dev->parent ? dev;
> >
> > That's what I had in mind. Maybe you'll have to use
> > dev->of_node->parent though, I think they are not equivalent.
> >
> >>
> >> For the clock, it will be
> >> nfc->clk = devm_clk_get(nfc->cdev, NULL);
> >>
> >> For the register base, I need to replace:
> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> mmio = devm_ioremap_resource(dev, res);
> >> if (IS_ERR(mmio))
> >> return PTR_ERR(mmio);
> >>
> >> nfc->regmap = devm_regmap_init_mmio(dev, mmio, &stm32_fmc2_regmap_cfg);
> >> if (IS_ERR(nfc->regmap))
> >> return PTR_ERR(nfc->regmap);
> >>
> >> nfc->io_phys_addr = res->start;
> >>
> >> With:
> >>
> >> ret = of_address_to_resource(nfc->cdev->of_node, 0, &res);
> >> if (ret)
> >> return ret;
> >>
> >> nfc->io_phys_addr = res.start;
> >>
> >> nfc->regmap = device_node_to_regmap(nfc->cdev->of_node);
> >> if (IS_ERR(nfc->regmap))
> >> return PTR_ERR(nfc->regmap);
> >>
> >> I expect that you were thinking about something like this proposal.
> >
> > This means the regmap has already been initialized, can you make sure
> > it is actually the case? What if the probe of the EBI block happens
> > next, or is deferred? (maybe you'll get a -EPROBE_DEFER, which is fine
> > then). Please try booting with the EBI node but without the EBI driver
> > and see if this is handled gracefully.
> >
>
> In case we have an EBI node, the NFC node will be a child node of the EBI
> node.
> The EBI driver will first be probed, getting all its resources and then it
> will populate all its children node (including the NFC node if this one is
> enabled). If the EBI driver is deferred because of getting one of its
> resources has failed, none of its children will be probed. They will be
> probed later when the EBI driver will succeed to get all its resources and
> will then populate all of its children.
>
Ok, I just reviewed it more in depth and I get it now, should be fine.
> In case we have the EBI node without its driver, none of its children will be
> populated, so the NFC node will not be probed.
>
> Here is an exemple of the bindings proposal:
> fmc: memory-controller@58002000 {
> #address-cells = <2>;
> #size-cells = <1>;
> compatible = "st,stm32mp1-fmc2-ebi";
> reg = <0x58002000 0x1000>;
> clocks = <&rcc FMC_K>;
> resets = <&rcc FMC_R>;
>
> ranges = <0 0 0x60000000 0x04000000>, /* EBI CS 1 */
> <1 0 0x64000000 0x04000000>, /* EBI CS 2 */
> <2 0 0x68000000 0x04000000>, /* EBI CS 3 */
> <3 0 0x6c000000 0x04000000>, /* EBI CS 4 */
> <4 0 0x80000000 0x10000000>; /* NAND */
>
> psram@0,0 {
> compatible = "mtd-ram";
> reg = <0 0x00000000 0x100000>;
> bank-width = <2>;
>
> st,fmc2-ebi-cs-transaction-type = <1>;
> st,fmc2-ebi-cs-address-setup-ns = <60>;
> st,fmc2-ebi-cs-data-setup-ns = <30>;
> st,fmc2-ebi-cs-bus-turnaround-ns = <5>;
> };
>
> nand-controller@4,0 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "st,stm32mp15-fmc2";
> reg = <4 0x00000000 0x1000>,
> <4 0x08010000 0x1000>,
> <4 0x08020000 0x1000>,
> <4 0x01000000 0x1000>,
> <4 0x09010000 0x1000>,
> <4 0x09020000 0x1000>;
> interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
> dmas = <&mdma1 20 0x2 0x12000a02 0x0 0x0>,
> <&mdma1 20 0x2 0x12000a08 0x0 0x0>,
> <&mdma1 21 0x2 0x12000a0a 0x0 0x0>;
> dma-names = "tx", "rx", "ecc";
>
> nand@0 {
> reg = <0>;
> nand-on-flash-bbt;
> #address-cells = <1>;
> #size-cells = <1>;
> };
> };
> };
>
> Regards,
> Christophe Kerello.
Thanks,
Miquèl
