| Message ID | 20200705211016.15241-3-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/milkymist: Do not create SD card within the SDHCI controller | expand |
On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > The 'card is readonly' and 'card inserted' IRQs are not wired. > Add a comment in case someone know where to wire them. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> I'm not convinced adding fixmes or todos in the code is the right direction. It would be better to file bugs or use some other more official tracking mechanism. Alistair > --- > hw/lm32/milkymist.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c > index 469e3c4322..117973c967 100644 > --- a/hw/lm32/milkymist.c > +++ b/hw/lm32/milkymist.c > @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) > dev = qdev_new("milkymist-memcard"); > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ > > return dev; > } > -- > 2.21.3 > >
On 7/6/20 6:19 PM, Alistair Francis wrote: > On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> The 'card is readonly' and 'card inserted' IRQs are not wired. >> Add a comment in case someone know where to wire them. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > I'm not convinced adding fixmes or todos in the code is the right > direction. It would be better to file bugs or use some other more > official tracking mechanism. This code is orphan :S I'll fill a launchpad bug ticket. OTOH we could also log UNIMP for lost IRQs (triggered but no handler registered). > > Alistair > >> --- >> hw/lm32/milkymist.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c >> index 469e3c4322..117973c967 100644 >> --- a/hw/lm32/milkymist.c >> +++ b/hw/lm32/milkymist.c >> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) >> dev = qdev_new("milkymist-memcard"); >> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); >> + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ >> >> return dev; >> } >> -- >> 2.21.3 >> >> >
On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/6/20 6:19 PM, Alistair Francis wrote: > > On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> > >> The 'card is readonly' and 'card inserted' IRQs are not wired. > >> Add a comment in case someone know where to wire them. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > > I'm not convinced adding fixmes or todos in the code is the right > > direction. It would be better to file bugs or use some other more > > official tracking mechanism. > > This code is orphan :S > > I'll fill a launchpad bug ticket. I also mean in general (you have some other patches that add TODOs or FIXMEs). > > OTOH we could also log UNIMP for lost IRQs (triggered but > no handler registered). That would also work. Alistair > > > > > Alistair > > > >> --- > >> hw/lm32/milkymist.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c > >> index 469e3c4322..117973c967 100644 > >> --- a/hw/lm32/milkymist.c > >> +++ b/hw/lm32/milkymist.c > >> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) > >> dev = qdev_new("milkymist-memcard"); > >> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > >> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > >> + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ > >> > >> return dev; > >> } > >> -- > >> 2.21.3 > >> > >> > >
On 7/6/20 8:32 PM, Alistair Francis wrote: > On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 7/6/20 6:19 PM, Alistair Francis wrote: >>> On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> >>>> The 'card is readonly' and 'card inserted' IRQs are not wired. >>>> Add a comment in case someone know where to wire them. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> >>> I'm not convinced adding fixmes or todos in the code is the right >>> direction. It would be better to file bugs or use some other more >>> official tracking mechanism. >> >> This code is orphan :S >> >> I'll fill a launchpad bug ticket. > > I also mean in general (you have some other patches that add TODOs or FIXMEs). Not all developers look at launchpad, while all of them read the code ;) $ git grep -E '(TODO|FIXME)' | wc -l 1899 For orphan code, a comment in the code might be better to remember the technical debt to the next developers interested to rework this piece of code (I'd rather not trust they'll dig in the mailing list archive and launchpad tickets while staring at the code). > >> >> OTOH we could also log UNIMP for lost IRQs (triggered but >> no handler registered). > > That would also work. > > Alistair > >> >>> >>> Alistair >>> >>>> --- >>>> hw/lm32/milkymist.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c >>>> index 469e3c4322..117973c967 100644 >>>> --- a/hw/lm32/milkymist.c >>>> +++ b/hw/lm32/milkymist.c >>>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) >>>> dev = qdev_new("milkymist-memcard"); >>>> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>>> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); >>>> + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ >>>> >>>> return dev; >>>> } >>>> -- >>>> 2.21.3 >>>> >>>> >>> >
On Mon, Jul 6, 2020 at 7:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/6/20 8:32 PM, Alistair Francis wrote: > > On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> > >> On 7/6/20 6:19 PM, Alistair Francis wrote: > >>> On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >>>> > >>>> The 'card is readonly' and 'card inserted' IRQs are not wired. > >>>> Add a comment in case someone know where to wire them. > >>>> > >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >>> > >>> I'm not convinced adding fixmes or todos in the code is the right > >>> direction. It would be better to file bugs or use some other more > >>> official tracking mechanism. > >> > >> This code is orphan :S > >> > >> I'll fill a launchpad bug ticket. > > > > I also mean in general (you have some other patches that add TODOs or FIXMEs). > > Not all developers look at launchpad, while all of them read the code ;) Good point. > > $ git grep -E '(TODO|FIXME)' | wc -l > 1899 > > For orphan code, a comment in the code might be better to remember > the technical debt to the next developers interested to rework this > piece of code (I'd rather not trust they'll dig in the mailing list > archive and launchpad tickets while staring at the code). Agreed. I guess this is fine then. If possible/applicable a log message would be more helpful. Alistair > > > > >> > >> OTOH we could also log UNIMP for lost IRQs (triggered but > >> no handler registered). > > > > That would also work. > > > > Alistair > > > >> > >>> > >>> Alistair > >>> > >>>> --- > >>>> hw/lm32/milkymist.c | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c > >>>> index 469e3c4322..117973c967 100644 > >>>> --- a/hw/lm32/milkymist.c > >>>> +++ b/hw/lm32/milkymist.c > >>>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) > >>>> dev = qdev_new("milkymist-memcard"); > >>>> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > >>>> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > >>>> + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ > >>>> > >>>> return dev; > >>>> } > >>>> -- > >>>> 2.21.3 > >>>> > >>>> > >>> > >
On Sun, 5 Jul 2020 at 22:10, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > The 'card is readonly' and 'card inserted' IRQs are not wired. > Add a comment in case someone know where to wire them. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/lm32/milkymist.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c > index 469e3c4322..117973c967 100644 > --- a/hw/lm32/milkymist.c > +++ b/hw/lm32/milkymist.c > @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) > dev = qdev_new("milkymist-memcard"); > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ It's possible that these lines are correctly not wired up (ie that the hardware does not provide any kind of indication of the r/o or insertion events). The milkymist mmc device is a very simple one. AIUI the RTL for the board is on github if anybody wants to go check. thanks -- PMM
Am 2020-07-07 18:30, schrieb Peter Maydell: > On Sun, 5 Jul 2020 at 22:10, Philippe Mathieu-Daudé <f4bug@amsat.org> > wrote: >> >> The 'card is readonly' and 'card inserted' IRQs are not wired. >> Add a comment in case someone know where to wire them. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/lm32/milkymist.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c >> index 469e3c4322..117973c967 100644 >> --- a/hw/lm32/milkymist.c >> +++ b/hw/lm32/milkymist.c >> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr >> base) >> dev = qdev_new("milkymist-memcard"); >> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); >> + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ > > It's possible that these lines are correctly not wired up > (ie that the hardware does not provide any kind of indication > of the r/o or insertion events). The milkymist mmc device is a > very simple one. AIUI the RTL for the board is on github if > anybody wants to go check. That is correct, there are not indications nor are there any interrupts. -michael
diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c index 469e3c4322..117973c967 100644 --- a/hw/lm32/milkymist.c +++ b/hw/lm32/milkymist.c @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base) dev = qdev_new("milkymist-memcard"); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); + /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */ return dev; }
The 'card is readonly' and 'card inserted' IRQs are not wired. Add a comment in case someone know where to wire them. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/lm32/milkymist.c | 1 + 1 file changed, 1 insertion(+)