Re: [PATCH V7 5/9] mfd: Add driver for NVIDIA Tegra XUSB

[Andrew Bresticker]

Lee,

On Wed, Apr 29, 2015 at 2:23 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Mon, 27 Apr 2015, Andrew Bresticker wrote:
>
>> Add an MFD driver for the XUSB host complex found on NVIDIA Tegra124
>> and later SoCs.
>>
>> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>> Cc: Samuel Ortiz <sameo@xxxxxxxxxxxxxxx>
>> Cc: Lee Jones <lee.jones@xxxxxxxxxx>

>> --- /dev/null
>> +++ b/drivers/mfd/tegra-xusb.c

>> +struct tegra_xusb_soc_data {
>> +     struct mfd_cell *devs;
>> +     unsigned int num_devs;
>> +};
>> +
>> +static struct resource tegra_xhci_resources[] = {
>> +     {
>> +             .name = "host",
>> +             .flags = IORESOURCE_IRQ,
>> +     },
>> +     {
>> +             .name = "xhci",
>> +             .flags = IORESOURCE_MEM,
>> +     },
>> +     {
>> +             .name = "ipfs",
>> +             .flags = IORESOURCE_MEM,
>> +     },
>> +};
>> +
>> +static struct resource tegra_xusb_mbox_resources[] = {
>> +     {
>> +             .name = "smi",
>> +             .flags = IORESOURCE_IRQ,
>> +     },
>> +};
>
> DEFINE_RES_IRQ_NAMED()
>
>> +static struct mfd_cell tegra124_xusb_devs[] = {
>> +     {
>> +             .name = "tegra-xhci",
>> +             .of_compatible = "nvidia,tegra124-xhci",
>> +     },
>> +     {
>> +             .name = "tegra-xusb-mbox",
>> +             .of_compatible = "nvidia,tegra124-xusb-mbox",
>> +     },
>> +};
>> +
>> +static const struct tegra_xusb_soc_data tegra124_xusb_data = {
>> +     .devs = tegra124_xusb_devs,
>> +     .num_devs = ARRAY_SIZE(tegra124_xusb_devs),
>> +};
>> +
>> +static const struct of_device_id tegra_xusb_of_match[] = {
>> +     { .compatible = "nvidia,tegra124-xusb", .data = &tegra124_xusb_data },
>
> Yuk!  Why are you mixing platform data and DT in this way?
>
> Why can't you just stick all of this in DT?

I assume you mean the resources?  The compatible strings will at least
need to be SoC-specific since they will change from SoC to SoC.

>> +     {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, tegra_xusb_of_match);
>> +static struct regmap_config tegra_fpci_regmap_config = {
>> +     .reg_bits = 32,
>> +     .val_bits = 32,
>> +     .reg_stride = 4,
>> +};
>> +
>> +static int tegra_xusb_probe(struct platform_device *pdev)
>> +{
>> +     const struct tegra_xusb_soc_data *soc;
>> +     const struct of_device_id *match;
>> +     struct tegra_xusb *xusb;
>> +     struct resource *res;
>> +     void __iomem *fpci_base;
>> +     int irq, ret;
>> +
>> +     xusb = devm_kzalloc(&pdev->dev, sizeof(*xusb), GFP_KERNEL);
>> +     if (!xusb)
>> +             return -ENOMEM;
>> +     platform_set_drvdata(pdev, xusb);
>> +
>> +     match = of_match_node(tegra_xusb_of_match, pdev->dev.of_node);
>> +     soc = match->data;
>> +
>> +     irq = platform_get_irq_byname(pdev, "host");
>> +     if (irq < 0)
>> +             return irq;
>> +     tegra_xhci_resources[0].start = irq;
>> +     tegra_xhci_resources[0].end = irq;
>> +
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "xhci");
>> +     if (!res)
>> +             return -ENODEV;
>> +     tegra_xhci_resources[1].start = res->start;
>> +     tegra_xhci_resources[1].end = res->end;
>> +
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ipfs");
>> +     if (!res)
>> +             return -ENODEV;
>> +     tegra_xhci_resources[2].start = res->start;
>> +     tegra_xhci_resources[2].end = res->end;
>> +
>> +     soc->devs[0].resources = tegra_xhci_resources;
>> +     soc->devs[0].num_resources = ARRAY_SIZE(tegra_xhci_resources);
>> +
>> +     irq = platform_get_irq_byname(pdev, "smi");
>> +     if (irq < 0)
>> +             return irq;
>> +     tegra_xusb_mbox_resources[0].start = irq;
>> +     tegra_xusb_mbox_resources[0].end = irq;
>> +
>> +     soc->devs[1].resources = tegra_xusb_mbox_resources;
>> +     soc->devs[1].num_resources = ARRAY_SIZE(tegra_xusb_mbox_resources);
>> +
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "fpci");
>> +     fpci_base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(fpci_base))
>> +             return PTR_ERR(fpci_base);
>
> This stuff is not good.
>
> Either let MFD handle it with mfd_cells or stick all of this stuff in
> DT and parse it from the child devices.

I'm not sure what exactly you mean by "let MFD handle it with
mfd_cells" here - is that not what I'm doing now?  Anyway I think that
leaves two ways of representing this in DT.

Either have the MFD take a single IOMEM resource and divide it up
statically within the driver:

usb@0,70090000 {
        compatible = "nvidia,tegra124-xusb";
        reg = <0x0 0x70090000 0x0 0xa000>;

        usb-host {
                compatible = "nvidia,tegra124-xhci";
                interrupts = <...>;
                ...
        }:

        mailbox {
                compatible = "nvidia,tegra124-xusb-mbox";
                interrupts = <...>;
                ...
        };
};

... or have the MFD take only the shared FPCI resource and have the
sub-devices parse the rest from DT:

usb@0,70098000 {
        compatible = "nvidia,tegra124-xusb";
        reg = <0x0 0x70098000 0x0 0x1000>;
        ranges;

        usb-host@0,70090000 {
                compatible = "nvidia,tegra124-xhci";
                reg = <0x0 0x70090000 0x0 0x8000>,
                        <0x0 0x70099000 0x0 0x1000>;
                interrupts = <...>;
                ...
        }:

        mailbox {
                compatible = "nvidia,tegra124-xusb-mbox";
                interrupts = <...>;
                ...
        };
};

I don't have a strong preference here, but I think the former more
accurately represents the resource hierarchy.  Since Thierry requested
the use of an MFD, I'd like him to weigh in on this - Thierry?

>> +     tegra_fpci_regmap_config.max_register = res->end - res->start - 3;
>> +     xusb->fpci_regs = devm_regmap_init_mmio(&pdev->dev, fpci_base,
>> +                                             &tegra_fpci_regmap_config);
>> +     if (IS_ERR(xusb->fpci_regs)) {
>> +             ret = PTR_ERR(xusb->fpci_regs);
>> +             dev_err(&pdev->dev, "Failed to init regmap: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     ret = mfd_add_devices(&pdev->dev, -1, soc->devs, soc->num_devs,
>> +                           NULL, 0, NULL);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to add MFD devices: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}

>> --- /dev/null
>> +++ b/include/soc/tegra/xusb.h

>> +struct tegra_xusb {
>> +     struct regmap *fpci_regs;
>
> Are you going to add to this?

No, I don't have any plans to add to this struct.

-Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/