lkml.org 
[lkml]   [2017]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v12 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs
    [...]

    >> > +
    >> > +const char *cvm_mmc_irq_names[] = {
    >> > + "MMC Buffer",
    >> > + "MMC Command",
    >> > + "MMC DMA",
    >> > + "MMC Command Error",
    >> > + "MMC DMA Error",
    >> > + "MMC Switch",
    >> > + "MMC Switch Error",
    >> > + "MMC DMA int Fifo",
    >> > + "MMC DMA int",
    >> > +};
    >>
    >> Debug-leftover?
    >>
    >> [...]
    >
    > No, this is used by both Octeon and ThunderX drivers. Maybe I should
    > have put it into an extra patch.

    No, it's fine let's keep here.

    >
    >> > +
    >> > +static void cvm_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
    >> > +{
    >> > + struct cvm_mmc_slot *slot = mmc_priv(mmc);
    >> > + struct cvm_mmc_host *host = slot->host;
    >> > + int clk_period = 0, power_class = 10, bus_width = 0;
    >> > + u64 clock, emm_switch;
    >> > +
    >> > + host->acquire_bus(host);
    >> > + cvm_mmc_switch_to(slot);
    >> > +
    >> > + /* Set the power state */
    >> > + switch (ios->power_mode) {
    >> > + case MMC_POWER_ON:
    >> > + break;
    >> > +
    >> > + case MMC_POWER_OFF:
    >> > + cvm_mmc_reset_bus(slot);
    >> > +
    >> > + if (host->global_pwr_gpiod)
    >> > + gpiod_set_value_cansleep(host->global_pwr_gpiod, 0);
    >>
    >> If I have understood the changelog correctly this GPIO is shared for
    >> all slots, right?
    >
    > Yes.
    >
    >> In such case, this doesn't work. You need to centralize the management
    >> of this GPIO pin (to enable reference counting), as otherwise one slot
    >> can decide to power off its card while another still uses their card
    >> and expecting the power to be on.
    >
    > OK. I could create a function in the shared part with ref counting.
    > On the other side, only the existing Octeon HW will use the global_pwr_gpiod,
    > and this HW only has one slot. For all new HW we'll use the GPIO regulator,
    > so I think it is not worth changing it. I'll add a comment.

    In such case, let's not make it part of the cavium core set ios
    function. Instead leave it to the octeon variant to deal with this.

    >
    >> Another option would be to model it as GPIO regulator (using a device
    >> tree overlay as we discussed earlier), then you get the reference
    >> counting for free - and easily get ocr_avail mask from the mmc core's
    >> regulator API. :-)
    >>
    >
    > No, this is what I already do in case host->global_pwr_gpiod is not set.

    Yes, that was my point. You would be able to remove some code both in
    cavium core and octeon variant.

    >
    >> Moreover, I didn't find this GPIO being documented as a DT binding, it
    >> should and it should also be marked as deprecated.
    >
    > Good point, I'll add it.

    Great!

    [...]

    >> > + break;
    >> > + case 4:
    >> > + slot->bus_width = 1;
    >> > + slot->mmc->caps = MMC_CAP_4_BIT_DATA;
    >> > + break;
    >> > + case 1:
    >> > + slot->bus_width = 0;
    >> > + break;
    >> > + }
    >>
    >> I would rather make the deprecated bindings to take the lowest
    >> precedence and besides, this bus_width setup looks messy. How about
    >> something like this instead:
    >
    > Previously you said I should parse deprecated bindings first, so I did
    > that ;-

    Yes, I remember that now. Sorry! :-)

    However, my point is about which binding that has the highest precedence.

    >
    >> mmc_of_parse();
    >>
    >> if (!(mmc->caps & (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA))) {
    >> of_property_read_u32(node, "cavium,bus-max-width", &bus_width);
    >> if (bus_width == 8)
    >> mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA;
    >> else if (bus_width == 4)
    >> mmc->caps |= MMC_CAP_4_BIT_DATA;
    >> }
    >
    > OK.
    >
    >> > +
    >> > + /* Set maximum and minimum frequency */
    >> > + if (f_max)
    >> > + mmc->f_max = f_max;
    >>
    >> Again, let's make sure the deprecated bindings takes lower precedence.
    >> Thus if mmc->f_max has a value let's use that and if not, then parse
    >> the deprecated DT binding and use that value instead.
    >
    > OK.
    >

    Great!

    >> > + if (!mmc->f_max || mmc->f_max > 52000000)
    >> > + mmc->f_max = 52000000;
    >> > + mmc->f_min = 400000;
    >> > +
    >> > + /* Sampling register settings, period in picoseconds */
    >> > + clock_period = 1000000000000ull / slot->host->sys_freq;
    >> > + slot->cmd_cnt = (cmd_skew + clock_period / 2) / clock_period;
    >> > + slot->dat_cnt = (dat_skew + clock_period / 2) / clock_period;
    >> > +
    >> > + return id;
    >> > +}
    >>
    >> [...]
    >>
    >> > diff --git a/drivers/mmc/host/cavium-mmc.h b/drivers/mmc/host/cavium-mmc.h
    >> > new file mode 100644
    >> > index 0000000..c3843448
    >> > --- /dev/null
    >> > +++ b/drivers/mmc/host/cavium-mmc.h
    >>
    >> [...]
    >>
    >> +
    >> > +/* Protoypes */
    >> > +irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id);
    >> > +int cvm_mmc_of_slot_probe(struct device *dev, struct cvm_mmc_host *host);
    >> > +int cvm_mmc_of_slot_remove(struct cvm_mmc_slot *slot);
    >> > +extern const char *cvm_mmc_irq_names[];
    >>
    >> Debug leftover?
    >
    > No, as I said before this is the interface I need for sharing
    > cavium-mmc.c and using it from the Octeon and ThunderX drivers.
    >
    > Should I put the interface into a separate patch (together with the
    > interrupt names)?

    No, it's fine!

    Kind regards
    Uffe

    \
     
     \ /
      Last update: 2017-03-17 14:49    [W:3.134 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site